On Fri, 18 Aug 2023 09:08:54 +0100
Lee Jones <lee@xxxxxxxxxx> 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@xxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxx>
> > + * 2020, 2023 by Marek Behún <kabel@xxxxxxxxxx>
> > */
> >
> > #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
