Re: [PATCH 4/5] Input: pinephone-keyboard - Support the proxied I2C bus

["Ondřej Jirman" <x@xff.cz>]

On Sat, Jan 29, 2022 at 08:43:30PM -0600, Samuel Holland wrote:
> On 1/29/22 8:05 PM, Ondřej Jirman wrote:
> > 
> > Please use a single read transfer to get both command result and data.
> > There will be less risk that some userspace app will issue another command
> > in between command status being read as 0 and data byte being read.
> > 
> > Otherwise if you use this in some read/modify/write operation, you
> > may write unexpected value to PMIC. I2C register layout is designed
> > to make this as optimal as possible in a single I2C transaction, so
> > you only need 3 bytes to start command and 2 bytes to read the result
> > and data, both in a single xfer. There's very high likelihood the command
> > will complete in those 300 - 500 us anyway, because the timing is
> > predictable. If this delay is set right, it's almost guaranteed,
> > only two xfers will be necessary to run the command and get the result+
> > status.
> 
> I did this originally, but it causes a different race condition: since the data
> is read first, the command can complete between when the data is read and when
> the result is read. If this happens, the command will be seen as complete, but
> the data will be garbage.
> 
> This caused occasional read errors for the charger's power supply properties,
> because I2C reads sometimes returned nonsensical values for those bytes.

Oh, well. :) I guess the firmware would need to wait for any ongoing I2C
tranfer to finish before setting the command status to 0, for this to work.
Another lesson learned. :(

> > And if possible, it would be best if the bus was somehow made busy for
> > other users, until the whole comand/result sequence completes, to eliminate
> > the possibility of another command being issued by other bus users
> > around [1].
> 
> Yes, I can add a call to i2c_lock_bus() here.

Perfect.

thank you,
	o.

> Regards,
> Samuel