| Message ID | 20230802160748.11208-3-kabel@kernel.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | leds: turris-omnia: updates | expand |
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. > +} > + > +static int omnia_cmd_read(const struct i2c_client *client, u8 cmd) > +{ > + struct i2c_msg msgs[2]; > + u8 reply; > + int ret; > + > + msgs[0].addr = client->addr; > + msgs[0].flags = 0; > + msgs[0].len = 1; > + msgs[0].buf = &cmd; > + msgs[1].addr = client->addr; > + msgs[1].flags = I2C_M_RD; > + msgs[1].len = 1; > + msgs[1].buf = &reply; > + > + 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; > +} > + > static int omnia_led_brightness_set_blocking(struct led_classdev *cdev, > enum led_brightness brightness) > { > @@ -64,7 +98,7 @@ static int omnia_led_brightness_set_blocking(struct led_classdev *cdev, > if (buf[2] || buf[3] || buf[4]) > state |= CMD_LED_STATE_ON; > > - ret = i2c_smbus_write_byte_data(leds->client, CMD_LED_STATE, state); > + ret = omnia_cmd_write(leds->client, CMD_LED_STATE, state); > if (ret >= 0 && (state & CMD_LED_STATE_ON)) > ret = i2c_master_send(leds->client, buf, 5); > > @@ -114,9 +148,8 @@ static int omnia_led_register(struct i2c_client *client, struct omnia_led *led, > cdev->brightness_set_blocking = omnia_led_brightness_set_blocking; > > /* put the LED into software mode */ > - ret = i2c_smbus_write_byte_data(client, CMD_LED_MODE, > - CMD_LED_MODE_LED(led->reg) | > - CMD_LED_MODE_USER); > + ret = omnia_cmd_write(client, CMD_LED_MODE, CMD_LED_MODE_LED(led->reg) | > + CMD_LED_MODE_USER); > if (ret < 0) { > dev_err(dev, "Cannot set LED %pOF to software mode: %i\n", np, > ret); > @@ -124,8 +157,8 @@ static int omnia_led_register(struct i2c_client *client, struct omnia_led *led, > } > > /* disable the LED */ > - ret = i2c_smbus_write_byte_data(client, CMD_LED_STATE, > - CMD_LED_STATE_LED(led->reg)); > + ret = omnia_cmd_write(client, CMD_LED_STATE, > + CMD_LED_STATE_LED(led->reg)); > if (ret < 0) { > dev_err(dev, "Cannot set LED %pOF brightness: %i\n", np, ret); > return ret; > @@ -158,7 +191,7 @@ static ssize_t brightness_show(struct device *dev, struct device_attribute *a, > struct i2c_client *client = to_i2c_client(dev); > int ret; > > - ret = i2c_smbus_read_byte_data(client, CMD_LED_GET_BRIGHTNESS); > + ret = omnia_cmd_read(client, CMD_LED_GET_BRIGHTNESS); > > if (ret < 0) > return ret; > @@ -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. > } > @@ -237,8 +269,8 @@ static void omnia_leds_remove(struct i2c_client *client) > u8 buf[5]; > > /* put all LEDs into default (HW triggered) mode */ > - i2c_smbus_write_byte_data(client, CMD_LED_MODE, > - CMD_LED_MODE_LED(OMNIA_BOARD_LEDS)); > + omnia_cmd_write(client, CMD_LED_MODE, > + CMD_LED_MODE_LED(OMNIA_BOARD_LEDS)); > > /* set all LEDs color to [255, 255, 255] */ > buf[0] = CMD_LED_COLOR; > -- > 2.41.0 >
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
On Mon, 21 Aug 2023, Marek Behún wrote: > 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. I don't insist on much. This is not a dictatorship. My job is to get people to reason about their choices. Ideally read() and write() type functions should return how many successful Bytes have been read and written, but there are exceptions to every rule. > > > @@ -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... Right, which is why I have previously said that normalising isn't the issue. Normalising to Bytes read/written would be the ideal. However, if that's not practical for any reason then common sense would win out.
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; +} + +static int omnia_cmd_read(const struct i2c_client *client, u8 cmd) +{ + struct i2c_msg msgs[2]; + u8 reply; + int ret; + + msgs[0].addr = client->addr; + msgs[0].flags = 0; + msgs[0].len = 1; + msgs[0].buf = &cmd; + msgs[1].addr = client->addr; + msgs[1].flags = I2C_M_RD; + msgs[1].len = 1; + msgs[1].buf = &reply; + + 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; +} + static int omnia_led_brightness_set_blocking(struct led_classdev *cdev, enum led_brightness brightness) { @@ -64,7 +98,7 @@ static int omnia_led_brightness_set_blocking(struct led_classdev *cdev, if (buf[2] || buf[3] || buf[4]) state |= CMD_LED_STATE_ON; - ret = i2c_smbus_write_byte_data(leds->client, CMD_LED_STATE, state); + ret = omnia_cmd_write(leds->client, CMD_LED_STATE, state); if (ret >= 0 && (state & CMD_LED_STATE_ON)) ret = i2c_master_send(leds->client, buf, 5); @@ -114,9 +148,8 @@ static int omnia_led_register(struct i2c_client *client, struct omnia_led *led, cdev->brightness_set_blocking = omnia_led_brightness_set_blocking; /* put the LED into software mode */ - ret = i2c_smbus_write_byte_data(client, CMD_LED_MODE, - CMD_LED_MODE_LED(led->reg) | - CMD_LED_MODE_USER); + ret = omnia_cmd_write(client, CMD_LED_MODE, CMD_LED_MODE_LED(led->reg) | + CMD_LED_MODE_USER); if (ret < 0) { dev_err(dev, "Cannot set LED %pOF to software mode: %i\n", np, ret); @@ -124,8 +157,8 @@ static int omnia_led_register(struct i2c_client *client, struct omnia_led *led, } /* disable the LED */ - ret = i2c_smbus_write_byte_data(client, CMD_LED_STATE, - CMD_LED_STATE_LED(led->reg)); + ret = omnia_cmd_write(client, CMD_LED_STATE, + CMD_LED_STATE_LED(led->reg)); if (ret < 0) { dev_err(dev, "Cannot set LED %pOF brightness: %i\n", np, ret); return ret; @@ -158,7 +191,7 @@ static ssize_t brightness_show(struct device *dev, struct device_attribute *a, struct i2c_client *client = to_i2c_client(dev); int ret; - ret = i2c_smbus_read_byte_data(client, CMD_LED_GET_BRIGHTNESS); + ret = omnia_cmd_read(client, CMD_LED_GET_BRIGHTNESS); if (ret < 0) return ret; @@ -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; } @@ -237,8 +269,8 @@ static void omnia_leds_remove(struct i2c_client *client) u8 buf[5]; /* put all LEDs into default (HW triggered) mode */ - i2c_smbus_write_byte_data(client, CMD_LED_MODE, - CMD_LED_MODE_LED(OMNIA_BOARD_LEDS)); + omnia_cmd_write(client, CMD_LED_MODE, + CMD_LED_MODE_LED(OMNIA_BOARD_LEDS)); /* set all LEDs color to [255, 255, 255] */ buf[0] = CMD_LED_COLOR;
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(-)