[En-Nut-Discussion] Missing possibility in the I2CBUS API

[Ulrich Prinz]

Hi!

I would not touch the fine working I2C system for some dump devices
where again some chip engineers didn't read the spec. So preferred
solution would be to buy a different chip that works according I2C
specification.

Second, PMBus ist not I2C, it just uses some basic I2C physical
transport but it is a different protocol. (It often requires even
additional signals) A second candidate that is build ontop of I2C is
SMBus (System Management Bus).
So combining that will end up in a messy driver doing half of this and
half of that.

I would recommend to write a second set of functions [
PmBusListTransact(), SMBusTransact() ] that make use of the supporting
functions that TwMasterTransact uses already. So you can surround it
with an configuration option to disable ist if it isn't needed at all.

Regards
Ulrich



2013/2/19 Uwe Bonnes <bon at elektron.ikp.physik.tu-darmstadt.de>:
>>>>>> "Harald" == Harald Kipp <harald.kipp at egnite.de> writes:
>
>     Harald> Hi Uwe, On 19.02.2013 12:29, Uwe Bonnes wrote:
>     >> while most devices allow to read multiple I2C registers with some
>     >> auto-increment scheme, Maxim for it's MAX44009 ambient light sensor
>     >> decided to require another sequence: Start- Chip Address/Write -
>     >> Write Register number - repeated start - Chip Address/Read - Read
>     >> register content- Start- Chip Address/Write - Write Register next
>     >> number - repeated start - Chip Address/Read - Read register content -
>     >> Stop.  This means multiple transactions without STOP during the
>     >> sequence of transactions.
>
>     Harald> There is even more: PMBus allows to change the slave address
>     Harald> after a repeated start (see chapter 9 of the I2C specs.).
>
>     Harald> I assume, that not all I2C master hardware is able to support
>     Harald> this, neither the Maxim nor the PMBus scheme.
>
>
>     >> The i2cbus API implemented in dev/i2cbus.c so long doesn't allow such
>     >> a sequence, to my understanding. Should we extend the API?
>
>     Harald> For the Maxim, we could argue: "Well, use two separate
>     Harald> transactions in that case." But for the PMBus we are busted in
>     Harald> any case.
>
> Sorry, forgot to mention that it is about atomic access to a two byte
> variable of the MAX44009 that gets updated by the MAX44009 when no I2C
> access is going on, that means a I2C-Stop has been issued.
>
>     Harald> I do not have the right idea now, but we better change the API
>     Harald> soon, before running into similar backward compatibility issues
>     Harald> as before.
>
>     Harald> I remember a different approach I saw somewhere else, where a
>     Harald> list of transactions was passed to the driver. May be that is
>     Harald> more difficult to use, but would more closely reflect the design
>     Harald> of the I2C bus. E.g.
>
>     Harald> list[0].sla = 0x22; list[0].txlen = 1; list[0].txbuf = &reg_num;
>     Harald> list[0].rxlen = 0; list[0].rxbuf = NULL;
>
>     Harald> list[1].sla = 0x22; list[1].txlen = 0; list[1].txbuf = NULL;
>     Harald> list[1].rxlen = 1; list[1].rxbuf = &reg_val;
>
>     Harald> ....
>
>     Harald> list[n].sla = 0x55;
>
>     Harald> ...
>
>     Harald> NutI2cBusTransact(bus, list, list_len)
>
>     Harald> If the hardware is able to do this in one transaction, it can be
>     Harald> done.  Otherwise the driver must use several start-stop
>     Harald> transactions to execute the list.
>
> Yes, I think we need to be able to (also) supply a list of transactions.
>
> What about adding
> NutI2cBusTransact(bus, list, list_len)
>
> and keeping
>
> NutI2cMasterTransceive(NUTI2C_SLAVE *slave, const void *wdat, int wlen,\
> void *rdat, int rsiz)
> which would set up the arguments to finally call NutI2cBusTransact()?
>
> Otherwise, is there a reason you do
>
>         (*bus->bus_transceive) (slave, &msg);
>         /* Release the bus. */
>         NutEventPost(&bus->bus_mutex);
>         /* Return the number of bytes received. */
>         rc = msg.msg_ridx;
>     }
>     return rc;
>
> and not
>         rc = (*bus->bus_transceive) (slave, &msg);
>         /* Release the bus. */
>         NutEventPost(&bus->bus_mutex);
>         /* Return the number of bytes received. */
>         if (rc == 0)
>           rc = msg.msg_ridx;
>     }
>     return rc;
>
> In your approach, the device driver in the case of failure must write
> the failurereason to msg.msg_ridx and so we loose the number of bytes
> successfully transferred.
>
> Bye
>
> --
> Uwe Bonnes                bon at elektron.ikp.physik.tu-darmstadt.de
>
> Institut fuer Kernphysik  Schlossgartenstrasse 9  64289 Darmstadt
> --------- Tel. 06151 162516 -------- Fax. 06151 164321 ----------
> _______________________________________________
> http://lists.egnite.de/mailman/listinfo/en-nut-discussion