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.
> +}
> +
> +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
>
--
Lee Jones [李琼斯]
