Re: [PATCH v3 2/6] leds: turris-omnia: do not use SMBUS calls

["Marek Behún" <kabel@kernel.org>]

On Fri, 18 Aug 2023 09:08:54 +0100
Lee Jones <lee@kernel.org> wrote:

> On Wed, 02 Aug 2023, Marek Behún wrote:
> 
> > The leds-turris-omnia driver uses three function for I2C access:
> > - i2c_smbus_write_byte_data() and i2c_smbus_read_byte_data(), which
> >   cause an emulated SMBUS transfer,
> > - i2c_master_send(), which causes an ordinary I2C transfer.
> > 
> > The Turris Omnia MCU LED controller is not semantically SMBUS, it
> > operates as a simple I2C bus. It does not implement any of the SMBUS
> > specific features, like PEC, or procedure calls, or anything. Moreover
> > the I2C controller driver also does not implement SMBUS, and so the
> > emulated SMBUS procedure from drivers/i2c/i2c-core-smbus.c is used for
> > the SMBUS calls, which gives an unnecessary overhead.
> > 
> > When I first wrote the driver, I was unaware of these facts, and I
> > simply used the first function that worked.
> > 
> > Drop the I2C SMBUS calls and instead use simple I2C transfers.
> > 
> > Fixes: 089381b27abe ("leds: initial support for Turris Omnia LEDs")
> > Signed-off-by: Marek Behún <kabel@kernel.org>
> > ---
> >  drivers/leds/leds-turris-omnia.c | 56 +++++++++++++++++++++++++-------
> >  1 file changed, 44 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/leds/leds-turris-omnia.c b/drivers/leds/leds-turris-omnia.c
> > index bbd610100e41..bb2a2b411a56 100644
> > --- a/drivers/leds/leds-turris-omnia.c
> > +++ b/drivers/leds/leds-turris-omnia.c
> > @@ -2,7 +2,7 @@
> >  /*
> >   * CZ.NIC's Turris Omnia LEDs driver
> >   *
> > - * 2020 by Marek Behún <kabel@kernel.org>
> > + * 2020, 2023 by Marek Behún <kabel@kernel.org>
> >   */
> >  
> >  #include <linux/i2c.h>
> > @@ -41,6 +41,40 @@ struct omnia_leds {
> >  	struct omnia_led leds[];
> >  };
> >  
> > +static int omnia_cmd_write(const struct i2c_client *client, u8 cmd, u8 val)
> > +{
> > +	u8 buf[2] = { cmd, val };
> > +	int ret;
> > +
> > +	ret = i2c_master_send(client, buf, sizeof(buf));
> > +
> > +	return ret < 0 ? ret : 0;  
> 
> You don't need to normalise to zero here.
> 
> The checks below all appear adequate to accept >0 as success.

The intended semantics of the new functions omnia_cmd_write()
and omnia_cmd_read() are that they inform about success. No other
information is required.

If I do not normalize to zero and simply return ret, on success, the
omnia_cmd_write() function would return the number of bytes written
over I2C, since that is what i2c_master_send(). But the code below that
uses these function is not interested in that information.

Moreover if I do this, one would expect similar semantics in the other
function introduced by this patch, omnia_cmd_read(). I do normalization
to zero here as well:

> > +	ret = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
> > +	if (likely(ret == ARRAY_SIZE(msgs)))
> > +		return reply;
> > +	else if (ret < 0)
> > +		return ret;
> > +	else
> > +		return -EIO;
> > +}

But how to do similar semantics here? i2c_transfer() returns the number
of successful I2C transfers, not the number of bytes read + written.

This is why I chose the semantics that I did: that both of these
functions should return zero on success, and negative errno on error.
This is a standard thing in Linux sources.

So, if you do insist on dropping the normalization to zero, I will do
it. But I do not agree with it...

Please reply if you do insist.

> > @@ -179,8 +212,7 @@ static ssize_t brightness_store(struct device *dev, struct device_attribute *a,
> >  	if (brightness > 100)
> >  		return -EINVAL;
> >  
> > -	ret = i2c_smbus_write_byte_data(client, CMD_LED_SET_BRIGHTNESS,
> > -					(u8)brightness);
> > +	ret = omnia_cmd_write(client, CMD_LED_SET_BRIGHTNESS, brightness);
> >  
> >  	return ret < 0 ? ret : count;  
> 
> What's count here?  Is it bytes written?
> 
> If so, can you simplify again and just return ret.

Device attribute _store method must always return count on success.
Count is the number of bytes written into the sysfs file. This has
nothing to do with the return value of omnia_cmd_write().

I can't return ret. If I did, on success, omnia_cmd_write() returns 0,
or it would return 2 if I dropped the normalization to zero as you
suggested above. None of these are related to the actual value I need
to return, which can be 2, 3 or 4. Consider the following command

  $ echo 100 >/sys/class/leds/<LED>/device/brightness

This would invoke calling the brightness_store() function with count=4,
because the buffer is 4 bytes long: "100\n". If I return ret, the
userspace would think that not all 4 bytes were written, and it would
try to write the remainign bytes again, invoking the function agagin...

Marek