[PATCH v3 0/6] leds: turris-omnia: updates

[PATCH v3 0/6] leds: turris-omnia: updates

[Marek Behún]

Hi Pavel, Lee,

I am sending version 3 of Turris Omnia's LED controller updates.
Please read the summary (changes from version 2):
- added patch 1, which drops unnecessary mutex locking
- added patch 2, which changes SMBUS calls to regular I2C transfers
- added patch 3, changing sprintf() to dedicated sysfs_emit()
- patch 4 replaces patch 1 from v2, which changed max_brightness to 1.
  Instead, we make set_brightness() more effective by avoiding
  unnecessary I2C transactions (for example if brightness is being
  changed between 0 and 255, we do not send the color changing I2C
  command, only the enabling/disabling command)
- patch 5 is updated patch 3 from v2, which adds support for enabling
  HW blinking mode on the LEDs
- patch 6 adds support for enabling/disabling HW gamma correction of
  the RGB LEDs. Gamma correction is supported by newer MCU firmware
  versions

Marek

Marek Behún (6):
  leds: turris-omnia: drop unnecessary mutex locking
  leds: turris-omnia: do not use SMBUS calls
  leds: turris-omnia: use sysfs_emit() instead of sprintf()
  leds: turris-omnia: make set_brightness() more efficient
  leds: turris-omnia: support HW controlled mode via private trigger
  leds: turris-omnia: add support for enabling/disabling HW gamma
    correction

 .../sysfs-class-led-driver-turris-omnia       |  14 +
 drivers/leds/Kconfig                          |   1 +
 drivers/leds/leds-turris-omnia.c              | 363 +++++++++++++++---
 3 files changed, 329 insertions(+), 49 deletions(-)

-- 
2.41.0

[PATCH v3 1/6] leds: turris-omnia: drop unnecessary mutex locking

[Marek Behún]

Do not lock driver mutex in the global LED panel brightness sysfs
accessors brightness_show() and brightness_store().

The mutex locking is unnecessary here. The I2C transfers are guarded by
I2C core locking mechanism, and the LED commands itself do not interfere
with other commands.

Fixes: 089381b27abe ("leds: initial support for Turris Omnia LEDs")
Signed-off-by: Marek Behún <kabel@kernel.org>
---
 drivers/leds/leds-turris-omnia.c | 11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/drivers/leds/leds-turris-omnia.c b/drivers/leds/leds-turris-omnia.c
index 64b2d7b6d3f3..bbd610100e41 100644
--- a/drivers/leds/leds-turris-omnia.c
+++ b/drivers/leds/leds-turris-omnia.c
@@ -156,12 +156,9 @@ static ssize_t brightness_show(struct device *dev, struct device_attribute *a,
 			       char *buf)
 {
 	struct i2c_client *client = to_i2c_client(dev);
-	struct omnia_leds *leds = i2c_get_clientdata(client);
 	int ret;
 
-	mutex_lock(&leds->lock);
 	ret = i2c_smbus_read_byte_data(client, CMD_LED_GET_BRIGHTNESS);
-	mutex_unlock(&leds->lock);
 
 	if (ret < 0)
 		return ret;
@@ -173,7 +170,6 @@ static ssize_t brightness_store(struct device *dev, struct device_attribute *a,
 				const char *buf, size_t count)
 {
 	struct i2c_client *client = to_i2c_client(dev);
-	struct omnia_leds *leds = i2c_get_clientdata(client);
 	unsigned long brightness;
 	int ret;
 
@@ -183,15 +179,10 @@ static ssize_t brightness_store(struct device *dev, struct device_attribute *a,
 	if (brightness > 100)
 		return -EINVAL;
 
-	mutex_lock(&leds->lock);
 	ret = i2c_smbus_write_byte_data(client, CMD_LED_SET_BRIGHTNESS,
 					(u8)brightness);
-	mutex_unlock(&leds->lock);
-
-	if (ret < 0)
-		return ret;
 
-	return count;
+	return ret < 0 ? ret : count;
 }
 static DEVICE_ATTR_RW(brightness);
 
-- 
2.41.0

Re: [PATCH v3 1/6] leds: turris-omnia: drop unnecessary mutex locking

[Lee Jones]

On Wed, 02 Aug 2023, Marek Behún wrote:

> Do not lock driver mutex in the global LED panel brightness sysfs
> accessors brightness_show() and brightness_store().
> 
> The mutex locking is unnecessary here. The I2C transfers are guarded by
> I2C core locking mechanism, and the LED commands itself do not interfere
> with other commands.
> 
> Fixes: 089381b27abe ("leds: initial support for Turris Omnia LEDs")
> Signed-off-by: Marek Behún <kabel@kernel.org>
> ---
>  drivers/leds/leds-turris-omnia.c | 11 +----------
>  1 file changed, 1 insertion(+), 10 deletions(-)

Reviewed-by: Lee Jones <lee@kernel.org>

-- 
Lee Jones [李琼斯]

Re: (subset) [PATCH v3 1/6] leds: turris-omnia: drop unnecessary mutex locking

[Lee Jones]

On Wed, 02 Aug 2023 18:07:43 +0200, Marek Behún wrote:
> Do not lock driver mutex in the global LED panel brightness sysfs
> accessors brightness_show() and brightness_store().
> 
> The mutex locking is unnecessary here. The I2C transfers are guarded by
> I2C core locking mechanism, and the LED commands itself do not interfere
> with other commands.
> 
> [...]

Applied, thanks!

[1/6] leds: turris-omnia: drop unnecessary mutex locking
      commit: 760b6b7925bf09491aafa4727eef74fc6bf738b0

--
Lee Jones [李琼斯]

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

[Marek Behún]

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;
+}
+
+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;
-- 
2.41.0

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

[Lee Jones]

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
> 

-- 
Lee Jones [李琼斯]

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

[Marek Behún]

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

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

[Lee Jones]

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.

-- 
Lee Jones [李琼斯]

[PATCH v3 3/6] leds: turris-omnia: use sysfs_emit() instead of sprintf()

[Marek Behún]

Use the dedicated sysfs_emit() function instead of sprintf() in sysfs
attribute accessor brightness_show().

Signed-off-by: Marek Behún <kabel@kernel.org>
---
 drivers/leds/leds-turris-omnia.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/leds/leds-turris-omnia.c b/drivers/leds/leds-turris-omnia.c
index bb2a2b411a56..9fca0acb2270 100644
--- a/drivers/leds/leds-turris-omnia.c
+++ b/drivers/leds/leds-turris-omnia.c
@@ -196,7 +196,7 @@ static ssize_t brightness_show(struct device *dev, struct device_attribute *a,
 	if (ret < 0)
 		return ret;
 
-	return sprintf(buf, "%d\n", ret);
+	return sysfs_emit(buf, "%d\n", ret);
 }
 
 static ssize_t brightness_store(struct device *dev, struct device_attribute *a,
-- 
2.41.0

Re: (subset) [PATCH v3 3/6] leds: turris-omnia: use sysfs_emit() instead of sprintf()

[Lee Jones]

On Wed, 02 Aug 2023 18:07:45 +0200, Marek Behún wrote:
> Use the dedicated sysfs_emit() function instead of sprintf() in sysfs
> attribute accessor brightness_show().
> 
> 

Applied, thanks!

[3/6] leds: turris-omnia: use sysfs_emit() instead of sprintf()
      commit: 72a29725b6f2577fa447ca9059cdcd17100043b4

--
Lee Jones [李琼斯]

[PATCH v3 4/6] leds: turris-omnia: make set_brightness() more efficient

[Marek Behún]

Implement caching of the LED color and state values that are sent to MCU
in order to make the set_brightness() operation more efficient by
avoiding I2C transactions which are not needed.

On Turris Omnia's MCU, which acts as the RGB LED controller, each LED
has a RGB color, and a ON/OFF state, which are configurable via I2C
commands CMD_LED_COLOR and CMD_LED_STATE.

The CMD_LED_COLOR command sends 5 bytes and the CMD_LED_STATE command 2
bytes over the I2C bus, which operates at 100 kHz. With I2C overhead
this allows only ~1670 color changing commands and ~3200 state changing
commands per second.

Currently, every time LED brightness or LED multi intensity is changed,
we send a CMD_LED_STATE command, and if the computed color (brightness
adjusted multi_intensity) is non-zero, we also send a CMD_LED_COLOR
command.

Consider for example the situation when we have a netdev trigger enabled
for a LED. The netdev trigger does not change the LED color, only the
brightness (either to 0 or to currently configured brightness), and so
there is no need to send the CMD_LED_COLOR command. But each change of
brightness to 0 sends one CMD_LED_STATE command, and each change of
brightness to max_brightness sends one CMD_LED_STATE command and one
CMD_LED_COLOR command:
    set_brightness(0)   ->  CMD_LED_STATE
    set_brightness(255) ->  CMD_LED_STATE + CMD_LED_COLOR
                                            (unnecessary)

We can avoid the unnecessary I2C transactions if we cache the values of
state and color that are sent to the controller. If the color does not
change from the one previously sent, there is no need to do the
CMD_LED_COLOR I2C transaction, and if the state does not change, there
is no need to do the CMD_LED_STATE transaction.

Because we need to make sure that out cached values are consistent with
the controller state, add explicit setting of the LED color to white at
probe time (this is the default setting when MCU resets, but does not
necessarily need to be the case, for example if U-Boot played with the
LED colors).

Signed-off-by: Marek Behún <kabel@kernel.org>
---
 drivers/leds/leds-turris-omnia.c | 96 ++++++++++++++++++++++++++------
 1 file changed, 78 insertions(+), 18 deletions(-)

diff --git a/drivers/leds/leds-turris-omnia.c b/drivers/leds/leds-turris-omnia.c
index 9fca0acb2270..636c6f802bcf 100644
--- a/drivers/leds/leds-turris-omnia.c
+++ b/drivers/leds/leds-turris-omnia.c
@@ -30,6 +30,8 @@
 struct omnia_led {
 	struct led_classdev_mc mc_cdev;
 	struct mc_subled subled_info[OMNIA_LED_NUM_CHANNELS];
+	u8 cached_channels[OMNIA_LED_NUM_CHANNELS];
+	bool on;
 	int reg;
 };
 
@@ -75,36 +77,82 @@ static int omnia_cmd_read(const struct i2c_client *client, u8 cmd)
 		return -EIO;
 }
 
+static int omnia_led_send_color_cmd(const struct i2c_client *client,
+				    struct omnia_led *led)
+{
+	char cmd[5];
+	int ret;
+
+	cmd[0] = CMD_LED_COLOR;
+	cmd[1] = led->reg;
+	cmd[2] = led->subled_info[0].brightness;
+	cmd[3] = led->subled_info[1].brightness;
+	cmd[4] = led->subled_info[2].brightness;
+
+	/* send the color change command */
+	ret = i2c_master_send(client, cmd, 5);
+	if (ret < 0)
+		return ret;
+
+	/* cache the RGB channel brightnesses */
+	for (int i = 0; i < OMNIA_LED_NUM_CHANNELS; ++i)
+		led->cached_channels[i] = led->subled_info[i].brightness;
+
+	return 0;
+}
+
+/* determine if the computed RGB channels are different from the cached ones */
+static bool omnia_led_channels_changed(struct omnia_led *led)
+{
+	for (int i = 0; i < OMNIA_LED_NUM_CHANNELS; ++i)
+		if (led->subled_info[i].brightness != led->cached_channels[i])
+			return true;
+
+	return false;
+}
+
 static int omnia_led_brightness_set_blocking(struct led_classdev *cdev,
 					     enum led_brightness brightness)
 {
 	struct led_classdev_mc *mc_cdev = lcdev_to_mccdev(cdev);
 	struct omnia_leds *leds = dev_get_drvdata(cdev->dev->parent);
 	struct omnia_led *led = to_omnia_led(mc_cdev);
-	u8 buf[5], state;
-	int ret;
+	int err = 0;
 
 	mutex_lock(&leds->lock);
 
-	led_mc_calc_color_components(&led->mc_cdev, brightness);
+	/*
+	 * Only recalculate RGB brightnesses from intensities if brightness is
+	 * non-zero. Otherwise we won't be using them and we can save ourselves
+	 * some software divisions (Omnia's CPU does not implement the division
+	 * instruction).
+	 */
+	if (brightness) {
+		led_mc_calc_color_components(mc_cdev, brightness);
+
+		/*
+		 * Send color command only if brightness is non-zero and the RGB
+		 * channel brightnesses changed.
+		 */
+		if (omnia_led_channels_changed(led))
+			err = omnia_led_send_color_cmd(leds->client, led);
+	}
 
-	buf[0] = CMD_LED_COLOR;
-	buf[1] = led->reg;
-	buf[2] = mc_cdev->subled_info[0].brightness;
-	buf[3] = mc_cdev->subled_info[1].brightness;
-	buf[4] = mc_cdev->subled_info[2].brightness;
+	/* send on/off state change only if (bool)brightness changed */
+	if (!err && !brightness != !led->on) {
+		u8 state = CMD_LED_STATE_LED(led->reg);
 
-	state = CMD_LED_STATE_LED(led->reg);
-	if (buf[2] || buf[3] || buf[4])
-		state |= CMD_LED_STATE_ON;
+		if (brightness)
+			state |= CMD_LED_STATE_ON;
 
-	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);
+		err = omnia_cmd_write(leds->client, CMD_LED_STATE, state);
+		if (!err)
+			led->on = !!brightness;
+	}
 
 	mutex_unlock(&leds->lock);
 
-	return ret;
+	return err;
 }
 
 static int omnia_led_register(struct i2c_client *client, struct omnia_led *led,
@@ -132,11 +180,15 @@ static int omnia_led_register(struct i2c_client *client, struct omnia_led *led,
 	}
 
 	led->subled_info[0].color_index = LED_COLOR_ID_RED;
-	led->subled_info[0].channel = 0;
 	led->subled_info[1].color_index = LED_COLOR_ID_GREEN;
-	led->subled_info[1].channel = 1;
 	led->subled_info[2].color_index = LED_COLOR_ID_BLUE;
-	led->subled_info[2].channel = 2;
+
+	/* initial color is white */
+	for (int i = 0; i < OMNIA_LED_NUM_CHANNELS; ++i) {
+		led->subled_info[i].intensity = 255;
+		led->subled_info[i].brightness = 255;
+		led->subled_info[i].channel = i;
+	}
 
 	led->mc_cdev.subled_info = led->subled_info;
 	led->mc_cdev.num_colors = OMNIA_LED_NUM_CHANNELS;
@@ -164,6 +216,14 @@ static int omnia_led_register(struct i2c_client *client, struct omnia_led *led,
 		return ret;
 	}
 
+	/* set initial color and cache it */
+	ret = omnia_led_send_color_cmd(client, led);
+	if (ret < 0) {
+		dev_err(dev, "Cannot set LED %pOF initial color: %i\n", np,
+			ret);
+		return ret;
+	}
+
 	ret = devm_led_classdev_multicolor_register_ext(dev, &led->mc_cdev,
 							&init_data);
 	if (ret < 0) {
-- 
2.41.0

Re: [PATCH v3 4/6] leds: turris-omnia: make set_brightness() more efficient

[Lee Jones]

On Wed, 02 Aug 2023, Marek Behún wrote:

> Implement caching of the LED color and state values that are sent to MCU
> in order to make the set_brightness() operation more efficient by
> avoiding I2C transactions which are not needed.

How many transactions does all of this extra code actually save?

> On Turris Omnia's MCU, which acts as the RGB LED controller, each LED
> has a RGB color, and a ON/OFF state, which are configurable via I2C
> commands CMD_LED_COLOR and CMD_LED_STATE.
> 
> The CMD_LED_COLOR command sends 5 bytes and the CMD_LED_STATE command 2
> bytes over the I2C bus, which operates at 100 kHz. With I2C overhead
> this allows only ~1670 color changing commands and ~3200 state changing
> commands per second.

Only?  Seems like quite a lot.

> Currently, every time LED brightness or LED multi intensity is changed,
> we send a CMD_LED_STATE command, and if the computed color (brightness
> adjusted multi_intensity) is non-zero, we also send a CMD_LED_COLOR
> command.
> 
> Consider for example the situation when we have a netdev trigger enabled
> for a LED. The netdev trigger does not change the LED color, only the
> brightness (either to 0 or to currently configured brightness), and so
> there is no need to send the CMD_LED_COLOR command. But each change of
> brightness to 0 sends one CMD_LED_STATE command, and each change of
> brightness to max_brightness sends one CMD_LED_STATE command and one
> CMD_LED_COLOR command:
>     set_brightness(0)   ->  CMD_LED_STATE
>     set_brightness(255) ->  CMD_LED_STATE + CMD_LED_COLOR
>                                             (unnecessary)
> 
> We can avoid the unnecessary I2C transactions if we cache the values of
> state and color that are sent to the controller. If the color does not
> change from the one previously sent, there is no need to do the
> CMD_LED_COLOR I2C transaction, and if the state does not change, there
> is no need to do the CMD_LED_STATE transaction.
> 
> Because we need to make sure that out cached values are consistent with

Nit: "our"

> the controller state, add explicit setting of the LED color to white at
> probe time (this is the default setting when MCU resets, but does not
> necessarily need to be the case, for example if U-Boot played with the
> LED colors).
> 
> Signed-off-by: Marek Behún <kabel@kernel.org>
> ---
>  drivers/leds/leds-turris-omnia.c | 96 ++++++++++++++++++++++++++------
>  1 file changed, 78 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/leds/leds-turris-omnia.c b/drivers/leds/leds-turris-omnia.c
> index 9fca0acb2270..636c6f802bcf 100644
> --- a/drivers/leds/leds-turris-omnia.c
> +++ b/drivers/leds/leds-turris-omnia.c
> @@ -30,6 +30,8 @@
>  struct omnia_led {
>  	struct led_classdev_mc mc_cdev;
>  	struct mc_subled subled_info[OMNIA_LED_NUM_CHANNELS];
> +	u8 cached_channels[OMNIA_LED_NUM_CHANNELS];
> +	bool on;
>  	int reg;
>  };
>  
> @@ -75,36 +77,82 @@ static int omnia_cmd_read(const struct i2c_client *client, u8 cmd)
>  		return -EIO;
>  }
>  
> +static int omnia_led_send_color_cmd(const struct i2c_client *client,
> +				    struct omnia_led *led)
> +{
> +	char cmd[5];
> +	int ret;
> +
> +	cmd[0] = CMD_LED_COLOR;
> +	cmd[1] = led->reg;
> +	cmd[2] = led->subled_info[0].brightness;
> +	cmd[3] = led->subled_info[1].brightness;
> +	cmd[4] = led->subled_info[2].brightness;
> +
> +	/* send the color change command */

Nit: Please start comments with an upper case char.

> +	ret = i2c_master_send(client, cmd, 5);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* cache the RGB channel brightnesses */
> +	for (int i = 0; i < OMNIA_LED_NUM_CHANNELS; ++i)

Why the pre-increment?

It's non-standard and doesn't appear to have any affect.

> +		led->cached_channels[i] = led->subled_info[i].brightness;
> +
> +	return 0;
> +}
> +
> +/* determine if the computed RGB channels are different from the cached ones */
> +static bool omnia_led_channels_changed(struct omnia_led *led)
> +{
> +	for (int i = 0; i < OMNIA_LED_NUM_CHANNELS; ++i)
> +		if (led->subled_info[i].brightness != led->cached_channels[i])
> +			return true;
> +
> +	return false;
> +}
> +
>  static int omnia_led_brightness_set_blocking(struct led_classdev *cdev,
>  					     enum led_brightness brightness)
>  {
>  	struct led_classdev_mc *mc_cdev = lcdev_to_mccdev(cdev);
>  	struct omnia_leds *leds = dev_get_drvdata(cdev->dev->parent);
>  	struct omnia_led *led = to_omnia_led(mc_cdev);
> -	u8 buf[5], state;
> -	int ret;
> +	int err = 0;
>  
>  	mutex_lock(&leds->lock);
>  
> -	led_mc_calc_color_components(&led->mc_cdev, brightness);
> +	/*
> +	 * Only recalculate RGB brightnesses from intensities if brightness is
> +	 * non-zero. Otherwise we won't be using them and we can save ourselves
> +	 * some software divisions (Omnia's CPU does not implement the division
> +	 * instruction).
> +	 */
> +	if (brightness) {
> +		led_mc_calc_color_components(mc_cdev, brightness);
> +
> +		/*
> +		 * Send color command only if brightness is non-zero and the RGB
> +		 * channel brightnesses changed.
> +		 */
> +		if (omnia_led_channels_changed(led))
> +			err = omnia_led_send_color_cmd(leds->client, led);
> +	}
>  
> -	buf[0] = CMD_LED_COLOR;
> -	buf[1] = led->reg;
> -	buf[2] = mc_cdev->subled_info[0].brightness;
> -	buf[3] = mc_cdev->subled_info[1].brightness;
> -	buf[4] = mc_cdev->subled_info[2].brightness;
> +	/* send on/off state change only if (bool)brightness changed */
> +	if (!err && !brightness != !led->on) {
> +		u8 state = CMD_LED_STATE_LED(led->reg);
>  
> -	state = CMD_LED_STATE_LED(led->reg);
> -	if (buf[2] || buf[3] || buf[4])
> -		state |= CMD_LED_STATE_ON;
> +		if (brightness)
> +			state |= CMD_LED_STATE_ON;
>  
> -	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);
> +		err = omnia_cmd_write(leds->client, CMD_LED_STATE, state);
> +		if (!err)
> +			led->on = !!brightness;
> +	}
>  
>  	mutex_unlock(&leds->lock);
>  
> -	return ret;
> +	return err;
>  }
>  
>  static int omnia_led_register(struct i2c_client *client, struct omnia_led *led,
> @@ -132,11 +180,15 @@ static int omnia_led_register(struct i2c_client *client, struct omnia_led *led,
>  	}
>  
>  	led->subled_info[0].color_index = LED_COLOR_ID_RED;
> -	led->subled_info[0].channel = 0;
>  	led->subled_info[1].color_index = LED_COLOR_ID_GREEN;
> -	led->subled_info[1].channel = 1;
>  	led->subled_info[2].color_index = LED_COLOR_ID_BLUE;
> -	led->subled_info[2].channel = 2;
> +
> +	/* initial color is white */
> +	for (int i = 0; i < OMNIA_LED_NUM_CHANNELS; ++i) {
> +		led->subled_info[i].intensity = 255;
> +		led->subled_info[i].brightness = 255;
> +		led->subled_info[i].channel = i;
> +	}
>  
>  	led->mc_cdev.subled_info = led->subled_info;
>  	led->mc_cdev.num_colors = OMNIA_LED_NUM_CHANNELS;
> @@ -164,6 +216,14 @@ static int omnia_led_register(struct i2c_client *client, struct omnia_led *led,
>  		return ret;
>  	}
>  
> +	/* set initial color and cache it */
> +	ret = omnia_led_send_color_cmd(client, led);
> +	if (ret < 0) {
> +		dev_err(dev, "Cannot set LED %pOF initial color: %i\n", np,
> +			ret);
> +		return ret;
> +	}
> +
>  	ret = devm_led_classdev_multicolor_register_ext(dev, &led->mc_cdev,
>  							&init_data);
>  	if (ret < 0) {
> -- 
> 2.41.0
> 

-- 
Lee Jones [李琼斯]

Re: [PATCH v3 4/6] leds: turris-omnia: make set_brightness() more efficient

[Marek Behún]

On Fri, 18 Aug 2023 10:42:55 +0100
Lee Jones <lee@kernel.org> wrote:

> On Wed, 02 Aug 2023, Marek Behún wrote:
> 
> > Implement caching of the LED color and state values that are sent to MCU
> > in order to make the set_brightness() operation more efficient by
> > avoiding I2C transactions which are not needed.  
> 
> How many transactions does all of this extra code actually save?

Depends on how many transactions are requested by userspace wanting to
change LED brightness, but see below.

> > On Turris Omnia's MCU, which acts as the RGB LED controller, each LED
> > has a RGB color, and a ON/OFF state, which are configurable via I2C
> > commands CMD_LED_COLOR and CMD_LED_STATE.
> > 
> > The CMD_LED_COLOR command sends 5 bytes and the CMD_LED_STATE command 2
> > bytes over the I2C bus, which operates at 100 kHz. With I2C overhead
> > this allows only ~1670 color changing commands and ~3200 state changing
> > commands per second.  
> 
> Only?  Seems like quite a lot.

I may have chosen the wrong argument here. Yes, 1670 color changing
commands are more than enough for a LED controller. This is not the
problem.

The problem is that the we need to be able to send other commands over
the same I2C bus, unrelated to the LED controller, and they need the
bandwidth. The MCU exposes an interrupt interface, and it can trigger
hundreds of interrupts per second. Each time we need to read the
interrupt state register. Whenever we are sending a LED color/state
changing command, the interrupt reading is waiting.

So this is the reason why I want to avoid unnecessary I2C transactions.

If I change the commit message to explain this, will you be satisfied?


> > Currently, every time LED brightness or LED multi intensity is changed,
> > we send a CMD_LED_STATE command, and if the computed color (brightness
> > adjusted multi_intensity) is non-zero, we also send a CMD_LED_COLOR
> > command.
> > 
> > Consider for example the situation when we have a netdev trigger enabled
> > for a LED. The netdev trigger does not change the LED color, only the
> > brightness (either to 0 or to currently configured brightness), and so
> > there is no need to send the CMD_LED_COLOR command. But each change of
> > brightness to 0 sends one CMD_LED_STATE command, and each change of
> > brightness to max_brightness sends one CMD_LED_STATE command and one
> > CMD_LED_COLOR command:
> >     set_brightness(0)   ->  CMD_LED_STATE
> >     set_brightness(255) ->  CMD_LED_STATE + CMD_LED_COLOR
> >                                             (unnecessary)
> > 
> > We can avoid the unnecessary I2C transactions if we cache the values of
> > state and color that are sent to the controller. If the color does not
> > change from the one previously sent, there is no need to do the
> > CMD_LED_COLOR I2C transaction, and if the state does not change, there
> > is no need to do the CMD_LED_STATE transaction.
> > 
> > Because we need to make sure that out cached values are consistent with  
> 
> Nit: "our"
> 

Thanks, I will fix this.

> > +
> > +	/* send the color change command */  
> 
> Nit: Please start comments with an upper case char.

OK, I will change it.

> > +	ret = i2c_master_send(client, cmd, 5);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	/* cache the RGB channel brightnesses */
> > +	for (int i = 0; i < OMNIA_LED_NUM_CHANNELS; ++i)  
> 
> Why the pre-increment?
> 
> It's non-standard and doesn't appear to have any affect.

According to git grep, current Linux sources contain around 7606
occurances of pre-increment (++x) in for cycles:
  git grep '; ++[a-z_]\+)' | wc -l

and 81360 occurances of post-increment (x++):
  git grep '; [a-z_]\+++)' | wc -l

Yes, I admit that pre-increment is 10 times less frequent, but I do not
consider that to be non-standard.

If you insist on this, I will change it. But I consider this
non-consequential...

Please let me know whether you insist on it.

Marek

Re: [PATCH v3 4/6] leds: turris-omnia: make set_brightness() more efficient

[Lee Jones]

On Mon, 21 Aug 2023, Marek Behún wrote:

> On Fri, 18 Aug 2023 10:42:55 +0100
> Lee Jones <lee@kernel.org> wrote:
> 
> > On Wed, 02 Aug 2023, Marek Behún wrote:
> > 
> > > Implement caching of the LED color and state values that are sent to MCU
> > > in order to make the set_brightness() operation more efficient by
> > > avoiding I2C transactions which are not needed.  
> > 
> > How many transactions does all of this extra code actually save?
> 
> Depends on how many transactions are requested by userspace wanting to
> change LED brightness, but see below.
> 
> > > On Turris Omnia's MCU, which acts as the RGB LED controller, each LED
> > > has a RGB color, and a ON/OFF state, which are configurable via I2C
> > > commands CMD_LED_COLOR and CMD_LED_STATE.
> > > 
> > > The CMD_LED_COLOR command sends 5 bytes and the CMD_LED_STATE command 2
> > > bytes over the I2C bus, which operates at 100 kHz. With I2C overhead
> > > this allows only ~1670 color changing commands and ~3200 state changing
> > > commands per second.  
> > 
> > Only?  Seems like quite a lot.
> 
> I may have chosen the wrong argument here. Yes, 1670 color changing
> commands are more than enough for a LED controller. This is not the
> problem.
> 
> The problem is that the we need to be able to send other commands over
> the same I2C bus, unrelated to the LED controller, and they need the
> bandwidth. The MCU exposes an interrupt interface, and it can trigger
> hundreds of interrupts per second. Each time we need to read the
> interrupt state register. Whenever we are sending a LED color/state
> changing command, the interrupt reading is waiting.
> 
> So this is the reason why I want to avoid unnecessary I2C transactions.
> 
> If I change the commit message to explain this, will you be satisfied?

Yes, I expect so.

> > > Currently, every time LED brightness or LED multi intensity is changed,
> > > we send a CMD_LED_STATE command, and if the computed color (brightness
> > > adjusted multi_intensity) is non-zero, we also send a CMD_LED_COLOR
> > > command.
> > > 
> > > Consider for example the situation when we have a netdev trigger enabled
> > > for a LED. The netdev trigger does not change the LED color, only the
> > > brightness (either to 0 or to currently configured brightness), and so
> > > there is no need to send the CMD_LED_COLOR command. But each change of
> > > brightness to 0 sends one CMD_LED_STATE command, and each change of
> > > brightness to max_brightness sends one CMD_LED_STATE command and one
> > > CMD_LED_COLOR command:
> > >     set_brightness(0)   ->  CMD_LED_STATE
> > >     set_brightness(255) ->  CMD_LED_STATE + CMD_LED_COLOR
> > >                                             (unnecessary)
> > > 
> > > We can avoid the unnecessary I2C transactions if we cache the values of
> > > state and color that are sent to the controller. If the color does not
> > > change from the one previously sent, there is no need to do the
> > > CMD_LED_COLOR I2C transaction, and if the state does not change, there
> > > is no need to do the CMD_LED_STATE transaction.
> > > 
> > > Because we need to make sure that out cached values are consistent with  
> > 
> > Nit: "our"
> > 
> 
> Thanks, I will fix this.
> 
> > > +
> > > +	/* send the color change command */  
> > 
> > Nit: Please start comments with an upper case char.
> 
> OK, I will change it.
> 
> > > +	ret = i2c_master_send(client, cmd, 5);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > > +	/* cache the RGB channel brightnesses */
> > > +	for (int i = 0; i < OMNIA_LED_NUM_CHANNELS; ++i)  
> > 
> > Why the pre-increment?
> > 
> > It's non-standard and doesn't appear to have any affect.
> 
> According to git grep, current Linux sources contain around 7606
> occurances of pre-increment (++x) in for cycles:
>   git grep '; ++[a-z_]\+)' | wc -l
> 
> and 81360 occurances of post-increment (x++):
>   git grep '; [a-z_]\+++)' | wc -l
> 
> Yes, I admit that pre-increment is 10 times less frequent, but I do not
> consider that to be non-standard.
> 
> If you insist on this, I will change it. But I consider this
> non-consequential...
> 
> Please let me know whether you insist on it.

I don't.  It was a genuine question I was curious about.

Just looks odd and made me wonder if it actually served a purpose.

-- 
Lee Jones [李琼斯]

[PATCH v3 5/6] leds: turris-omnia: support HW controlled mode via private trigger

[Marek Behún]

Add support for enabling MCU controlled mode of the Turris Omnia LEDs
via a LED private trigger called "omnia-mcu". Recall that private LED
triggers will only be listed in the sysfs trigger file for LEDs that
support them (currently there is no user of this mechanism).

When in MCU controlled mode, the user can still set LED color, but the
blinking is done by MCU, which does different things for different LEDs:
- WAN LED is blinked according to the LED[0] pin of the WAN PHY
- LAN LEDs are blinked according to the LED[0] output of the
  corresponding port of the LAN switch
- PCIe LEDs are blinked according to the logical OR of the MiniPCIe port
  LED pins

In the future I want to make the netdev trigger to transparently offload
the blinking to the HW if user sets compatible settings for the netdev
trigger (for LEDs associated with network devices).
There was some work on this already, and hopefully we will be able to
complete it sometime, but for now there are still multiple blockers for
this, and even if there weren't, we still would not be able to configure
HW controlled mode for the LEDs associated with MiniPCIe ports.

In the meantime let's support HW controlled mode via the private LED
trigger mechanism. If, in the future, we manage to complete the netdev
trigger offloading, we can still keep this private trigger for backwards
compatibility, if needed.

We also set "omnia-mcu" to cdev->default_trigger, so that the MCU keeps
control until the user first wants to take over it. If a different
default trigger is specified in device-tree via the
'linux,default-trigger' property, LED class will overwrite
cdev->default_trigger, and so the DT property will be respected.

Signed-off-by: Marek Behún <kabel@kernel.org>
---
 drivers/leds/Kconfig             |  1 +
 drivers/leds/leds-turris-omnia.c | 97 +++++++++++++++++++++++++++++---
 2 files changed, 90 insertions(+), 8 deletions(-)

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 6046dfeca16f..ebb3b84d7a4f 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -187,6 +187,7 @@ config LEDS_TURRIS_OMNIA
 	depends on I2C
 	depends on MACH_ARMADA_38X || COMPILE_TEST
 	depends on OF
+	select LEDS_TRIGGERS
 	help
 	  This option enables basic support for the LEDs found on the front
 	  side of CZ.NIC's Turris Omnia router. There are 12 RGB LEDs on the
diff --git a/drivers/leds/leds-turris-omnia.c b/drivers/leds/leds-turris-omnia.c
index 636c6f802bcf..180b0cbeb92e 100644
--- a/drivers/leds/leds-turris-omnia.c
+++ b/drivers/leds/leds-turris-omnia.c
@@ -31,7 +31,7 @@ struct omnia_led {
 	struct led_classdev_mc mc_cdev;
 	struct mc_subled subled_info[OMNIA_LED_NUM_CHANNELS];
 	u8 cached_channels[OMNIA_LED_NUM_CHANNELS];
-	bool on;
+	bool on, hwtrig;
 	int reg;
 };
 
@@ -123,12 +123,14 @@ static int omnia_led_brightness_set_blocking(struct led_classdev *cdev,
 
 	/*
 	 * Only recalculate RGB brightnesses from intensities if brightness is
-	 * non-zero. Otherwise we won't be using them and we can save ourselves
-	 * some software divisions (Omnia's CPU does not implement the division
-	 * instruction).
+	 * non-zero (if it is zero and the LED is in HW blinking mode, we use
+	 * max_brightness as brightness). Otherwise we won't be using them and
+	 * we can save ourselves some software divisions (Omnia's CPU does not
+	 * implement the division instruction).
 	 */
-	if (brightness) {
-		led_mc_calc_color_components(mc_cdev, brightness);
+	if (brightness || led->hwtrig) {
+		led_mc_calc_color_components(mc_cdev, brightness ?:
+						      cdev->max_brightness);
 
 		/*
 		 * Send color command only if brightness is non-zero and the RGB
@@ -138,8 +140,11 @@ static int omnia_led_brightness_set_blocking(struct led_classdev *cdev,
 			err = omnia_led_send_color_cmd(leds->client, led);
 	}
 
-	/* send on/off state change only if (bool)brightness changed */
-	if (!err && !brightness != !led->on) {
+	/*
+	 * Send on/off state change only if (bool)brightness changed and the LED
+	 * is not being blinked by HW.
+	 */
+	if (!err && !led->hwtrig && !brightness != !led->on) {
 		u8 state = CMD_LED_STATE_LED(led->reg);
 
 		if (brightness)
@@ -155,6 +160,70 @@ static int omnia_led_brightness_set_blocking(struct led_classdev *cdev,
 	return err;
 }
 
+static struct led_hw_trigger_type omnia_hw_trigger_type;
+
+static int omnia_hwtrig_activate(struct led_classdev *cdev)
+{
+	struct led_classdev_mc *mc_cdev = lcdev_to_mccdev(cdev);
+	struct omnia_leds *leds = dev_get_drvdata(cdev->dev->parent);
+	struct omnia_led *led = to_omnia_led(mc_cdev);
+	int err = 0;
+
+	mutex_lock(&leds->lock);
+
+	if (!led->on) {
+		/*
+		 * If the LED is off (brightness was set to 0), the last
+		 * configured color was not necessarily sent to the MCU.
+		 * Recompute with max_brightness and send if needed.
+		 */
+		led_mc_calc_color_components(mc_cdev, cdev->max_brightness);
+
+		if (omnia_led_channels_changed(led))
+			err = omnia_led_send_color_cmd(leds->client, led);
+	}
+
+	if (!err) {
+		/* put the LED into MCU controlled mode */
+		err = omnia_cmd_write(leds->client, CMD_LED_MODE,
+				      CMD_LED_MODE_LED(led->reg));
+		if (!err)
+			led->hwtrig = true;
+	}
+
+	mutex_unlock(&leds->lock);
+
+	return err;
+}
+
+static void omnia_hwtrig_deactivate(struct led_classdev *cdev)
+{
+	struct omnia_leds *leds = dev_get_drvdata(cdev->dev->parent);
+	struct omnia_led *led = to_omnia_led(lcdev_to_mccdev(cdev));
+	int err;
+
+	mutex_lock(&leds->lock);
+
+	led->hwtrig = false;
+
+	/* put the LED into software mode */
+	err = omnia_cmd_write(leds->client, CMD_LED_MODE,
+			      CMD_LED_MODE_LED(led->reg) | CMD_LED_MODE_USER);
+
+	mutex_unlock(&leds->lock);
+
+	if (err < 0)
+		dev_err(cdev->dev, "Cannot put LED to software mode: %i\n",
+			err);
+}
+
+static struct led_trigger omnia_hw_trigger = {
+	.name		= "omnia-mcu",
+	.activate	= omnia_hwtrig_activate,
+	.deactivate	= omnia_hwtrig_deactivate,
+	.trigger_type	= &omnia_hw_trigger_type,
+};
+
 static int omnia_led_register(struct i2c_client *client, struct omnia_led *led,
 			      struct device_node *np)
 {
@@ -198,6 +267,12 @@ static int omnia_led_register(struct i2c_client *client, struct omnia_led *led,
 	cdev = &led->mc_cdev.led_cdev;
 	cdev->max_brightness = 255;
 	cdev->brightness_set_blocking = omnia_led_brightness_set_blocking;
+	cdev->trigger_type = &omnia_hw_trigger_type;
+	/*
+	 * Use the omnia-mcu trigger as the default trigger. It may be rewritten
+	 * by LED class from the linux,default-trigger property.
+	 */
+	cdev->default_trigger = omnia_hw_trigger.name;
 
 	/* put the LED into software mode */
 	ret = omnia_cmd_write(client, CMD_LED_MODE, CMD_LED_MODE_LED(led->reg) |
@@ -310,6 +385,12 @@ static int omnia_leds_probe(struct i2c_client *client)
 
 	mutex_init(&leds->lock);
 
+	ret = devm_led_trigger_register(dev, &omnia_hw_trigger);
+	if (ret < 0) {
+		dev_err(dev, "Cannot register private LED trigger: %d\n", ret);
+		return ret;
+	}
+
 	led = &leds->leds[0];
 	for_each_available_child_of_node(np, child) {
 		ret = omnia_led_register(client, led, child);
-- 
2.41.0

Re: [PATCH v3 5/6] leds: turris-omnia: support HW controlled mode via private trigger

[Marek Behún]

On Wed,  2 Aug 2023 18:07:47 +0200
Marek Behún <kabel@kernel.org> wrote:

> +static int omnia_hwtrig_activate(struct led_classdev *cdev)
> +{
> +	struct led_classdev_mc *mc_cdev = lcdev_to_mccdev(cdev);
> +	struct omnia_leds *leds = dev_get_drvdata(cdev->dev->parent);
> +	struct omnia_led *led = to_omnia_led(mc_cdev);
> +	int err = 0;
> +
> +	mutex_lock(&leds->lock);
> +
> +	if (!led->on) {
> +		/*
> +		 * If the LED is off (brightness was set to 0), the last
> +		 * configured color was not necessarily sent to the MCU.
> +		 * Recompute with max_brightness and send if needed.
> +		 */
> +		led_mc_calc_color_components(mc_cdev, cdev->max_brightness);
> +
> +		if (omnia_led_channels_changed(led))
> +			err = omnia_led_send_color_cmd(leds->client, led);
> +	}
> +
> +	if (!err) {
> +		/* put the LED into MCU controlled mode */
> +		err = omnia_cmd_write(leds->client, CMD_LED_MODE,
> +				      CMD_LED_MODE_LED(led->reg));
> +		if (!err)
> +			led->hwtrig = true;
> +	}
> +
> +	mutex_unlock(&leds->lock);
> +
> +	return err;
> +}

Pavel, Lee, here I wanted to ask: can the .activate() method of a LED
trigger sleep? The .brightness_set() method of a LED cannot sleep, and
therefore we have .brightness_set_blocking() method, which schedules
the potentially sleeping call into a work. But there is no such
mechanism for the LED trigger .activate() method.

I looked at the LED core code, and it does not seem to me that
.activate() sleeping would cause issues, besides keeping trigger list
lock locked...

Note that previously none of the LED triggers in drivers/leds/trigger
sleeped in .activate(), but recently the ethernet PHY subsystem was
wired into the netdev trigger, which may cause the .activate() method
to do a PHY transfer, which AFAIK may sleep...

Marek

Re: [PATCH v3 5/6] leds: turris-omnia: support HW controlled mode via private trigger

[Lee Jones]

On Wed, 02 Aug 2023, Marek Behún wrote:

> On Wed,  2 Aug 2023 18:07:47 +0200
> Marek Behún <kabel@kernel.org> wrote:
> 
> > +static int omnia_hwtrig_activate(struct led_classdev *cdev)
> > +{
> > +	struct led_classdev_mc *mc_cdev = lcdev_to_mccdev(cdev);
> > +	struct omnia_leds *leds = dev_get_drvdata(cdev->dev->parent);
> > +	struct omnia_led *led = to_omnia_led(mc_cdev);
> > +	int err = 0;
> > +
> > +	mutex_lock(&leds->lock);
> > +
> > +	if (!led->on) {
> > +		/*
> > +		 * If the LED is off (brightness was set to 0), the last
> > +		 * configured color was not necessarily sent to the MCU.
> > +		 * Recompute with max_brightness and send if needed.
> > +		 */
> > +		led_mc_calc_color_components(mc_cdev, cdev->max_brightness);
> > +
> > +		if (omnia_led_channels_changed(led))
> > +			err = omnia_led_send_color_cmd(leds->client, led);
> > +	}
> > +
> > +	if (!err) {
> > +		/* put the LED into MCU controlled mode */
> > +		err = omnia_cmd_write(leds->client, CMD_LED_MODE,
> > +				      CMD_LED_MODE_LED(led->reg));
> > +		if (!err)
> > +			led->hwtrig = true;
> > +	}
> > +
> > +	mutex_unlock(&leds->lock);
> > +
> > +	return err;
> > +}
> 
> Pavel, Lee, here I wanted to ask: can the .activate() method of a LED
> trigger sleep? The .brightness_set() method of a LED cannot sleep, and
> therefore we have .brightness_set_blocking() method, which schedules
> the potentially sleeping call into a work. But there is no such
> mechanism for the LED trigger .activate() method.
> 
> I looked at the LED core code, and it does not seem to me that
> .activate() sleeping would cause issues, besides keeping trigger list
> lock locked...
> 
> Note that previously none of the LED triggers in drivers/leds/trigger
> sleeped in .activate(), but recently the ethernet PHY subsystem was
> wired into the netdev trigger, which may cause the .activate() method
> to do a PHY transfer, which AFAIK may sleep...

I suspect you know more than I do at this point.  I could take a
deep-dive into the code however a) I'm presently swamped with incoming
reviews and b) I do not have any additional resources at my disposable
than you do - it would consist of reading through and brain-grepping the
code.

Pavel or Jacek may have more knowledge at-hand though.

-- 
Lee Jones [李琼斯]

Re: [PATCH v3 5/6] leds: turris-omnia: support HW controlled mode via private trigger

[Jacek Anaszewski]

Hi Marek,

On 8/2/23 18:13, Marek Behún wrote:
> On Wed,  2 Aug 2023 18:07:47 +0200
> Marek Behún <kabel@kernel.org> wrote:
> 
>> +static int omnia_hwtrig_activate(struct led_classdev *cdev)
>> +{
>> +	struct led_classdev_mc *mc_cdev = lcdev_to_mccdev(cdev);
>> +	struct omnia_leds *leds = dev_get_drvdata(cdev->dev->parent);
>> +	struct omnia_led *led = to_omnia_led(mc_cdev);
>> +	int err = 0;
>> +
>> +	mutex_lock(&leds->lock);
>> +
>> +	if (!led->on) {
>> +		/*
>> +		 * If the LED is off (brightness was set to 0), the last
>> +		 * configured color was not necessarily sent to the MCU.
>> +		 * Recompute with max_brightness and send if needed.
>> +		 */
>> +		led_mc_calc_color_components(mc_cdev, cdev->max_brightness);
>> +
>> +		if (omnia_led_channels_changed(led))
>> +			err = omnia_led_send_color_cmd(leds->client, led);
>> +	}
>> +
>> +	if (!err) {
>> +		/* put the LED into MCU controlled mode */
>> +		err = omnia_cmd_write(leds->client, CMD_LED_MODE,
>> +				      CMD_LED_MODE_LED(led->reg));
>> +		if (!err)
>> +			led->hwtrig = true;
>> +	}
>> +
>> +	mutex_unlock(&leds->lock);
>> +
>> +	return err;
>> +}
> 
> Pavel, Lee, here I wanted to ask: can the .activate() method of a LED
> trigger sleep? The .brightness_set() method of a LED cannot sleep, and
> therefore we have .brightness_set_blocking() method, which schedules
> the potentially sleeping call into a work. But there is no such
> mechanism for the LED trigger .activate() method.
> 
> I looked at the LED core code, and it does not seem to me that
> .activate() sleeping would cause issues, besides keeping trigger list
> lock locked...
> 
> Note that previously none of the LED triggers in drivers/leds/trigger
> sleeped in .activate(), but recently the ethernet PHY subsystem was
> wired into the netdev trigger, which may cause the .activate() method
> to do a PHY transfer, which AFAIK may sleep...

In general led_set_brightness() can't sleep because it is called from
triggers in atomic contexts, which shouldn't be the case for activate().

.activate() is called from led_trigger_{set|remove}() which is called 
from led_classdev_{register|unregister}) and from sysfs trigger attr's
led_trigger_write() handler, so no risk here.

Triggers already call e.g. kzalloc() in .activate() which may sleep.

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v3 5/6] leds: turris-omnia: support HW controlled mode via private trigger

[Lee Jones]

On Fri, 18 Aug 2023, Jacek Anaszewski wrote:

> Hi Marek,
> 
> On 8/2/23 18:13, Marek Behún wrote:
> > On Wed,  2 Aug 2023 18:07:47 +0200
> > Marek Behún <kabel@kernel.org> wrote:
> > 
> > > +static int omnia_hwtrig_activate(struct led_classdev *cdev)
> > > +{
> > > +	struct led_classdev_mc *mc_cdev = lcdev_to_mccdev(cdev);
> > > +	struct omnia_leds *leds = dev_get_drvdata(cdev->dev->parent);
> > > +	struct omnia_led *led = to_omnia_led(mc_cdev);
> > > +	int err = 0;
> > > +
> > > +	mutex_lock(&leds->lock);
> > > +
> > > +	if (!led->on) {
> > > +		/*
> > > +		 * If the LED is off (brightness was set to 0), the last
> > > +		 * configured color was not necessarily sent to the MCU.
> > > +		 * Recompute with max_brightness and send if needed.
> > > +		 */
> > > +		led_mc_calc_color_components(mc_cdev, cdev->max_brightness);
> > > +
> > > +		if (omnia_led_channels_changed(led))
> > > +			err = omnia_led_send_color_cmd(leds->client, led);
> > > +	}
> > > +
> > > +	if (!err) {
> > > +		/* put the LED into MCU controlled mode */
> > > +		err = omnia_cmd_write(leds->client, CMD_LED_MODE,
> > > +				      CMD_LED_MODE_LED(led->reg));
> > > +		if (!err)
> > > +			led->hwtrig = true;
> > > +	}
> > > +
> > > +	mutex_unlock(&leds->lock);
> > > +
> > > +	return err;
> > > +}
> > 
> > Pavel, Lee, here I wanted to ask: can the .activate() method of a LED
> > trigger sleep? The .brightness_set() method of a LED cannot sleep, and
> > therefore we have .brightness_set_blocking() method, which schedules
> > the potentially sleeping call into a work. But there is no such
> > mechanism for the LED trigger .activate() method.
> > 
> > I looked at the LED core code, and it does not seem to me that
> > .activate() sleeping would cause issues, besides keeping trigger list
> > lock locked...
> > 
> > Note that previously none of the LED triggers in drivers/leds/trigger
> > sleeped in .activate(), but recently the ethernet PHY subsystem was
> > wired into the netdev trigger, which may cause the .activate() method
> > to do a PHY transfer, which AFAIK may sleep...
> 
> In general led_set_brightness() can't sleep because it is called from
> triggers in atomic contexts, which shouldn't be the case for activate().
> 
> .activate() is called from led_trigger_{set|remove}() which is called from
> led_classdev_{register|unregister}) and from sysfs trigger attr's
> led_trigger_write() handler, so no risk here.
> 
> Triggers already call e.g. kzalloc() in .activate() which may sleep.

Thanks for stepping in Jacek.  I really appreciate your help.

-- 
Lee Jones [李琼斯]

Re: [PATCH v3 5/6] leds: turris-omnia: support HW controlled mode via private trigger

[Lee Jones]

On Wed, 02 Aug 2023, Marek Behún wrote:

> Add support for enabling MCU controlled mode of the Turris Omnia LEDs
> via a LED private trigger called "omnia-mcu". Recall that private LED
> triggers will only be listed in the sysfs trigger file for LEDs that
> support them (currently there is no user of this mechanism).
> 
> When in MCU controlled mode, the user can still set LED color, but the
> blinking is done by MCU, which does different things for different LEDs:
> - WAN LED is blinked according to the LED[0] pin of the WAN PHY
> - LAN LEDs are blinked according to the LED[0] output of the
>   corresponding port of the LAN switch
> - PCIe LEDs are blinked according to the logical OR of the MiniPCIe port
>   LED pins
> 
> In the future I want to make the netdev trigger to transparently offload
> the blinking to the HW if user sets compatible settings for the netdev
> trigger (for LEDs associated with network devices).
> There was some work on this already, and hopefully we will be able to
> complete it sometime, but for now there are still multiple blockers for
> this, and even if there weren't, we still would not be able to configure
> HW controlled mode for the LEDs associated with MiniPCIe ports.
> 
> In the meantime let's support HW controlled mode via the private LED
> trigger mechanism. If, in the future, we manage to complete the netdev
> trigger offloading, we can still keep this private trigger for backwards
> compatibility, if needed.
> 
> We also set "omnia-mcu" to cdev->default_trigger, so that the MCU keeps
> control until the user first wants to take over it. If a different
> default trigger is specified in device-tree via the
> 'linux,default-trigger' property, LED class will overwrite
> cdev->default_trigger, and so the DT property will be respected.
> 
> Signed-off-by: Marek Behún <kabel@kernel.org>
> ---
>  drivers/leds/Kconfig             |  1 +
>  drivers/leds/leds-turris-omnia.c | 97 +++++++++++++++++++++++++++++---
>  2 files changed, 90 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index 6046dfeca16f..ebb3b84d7a4f 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -187,6 +187,7 @@ config LEDS_TURRIS_OMNIA
>  	depends on I2C
>  	depends on MACH_ARMADA_38X || COMPILE_TEST
>  	depends on OF
> +	select LEDS_TRIGGERS
>  	help
>  	  This option enables basic support for the LEDs found on the front
>  	  side of CZ.NIC's Turris Omnia router. There are 12 RGB LEDs on the
> diff --git a/drivers/leds/leds-turris-omnia.c b/drivers/leds/leds-turris-omnia.c
> index 636c6f802bcf..180b0cbeb92e 100644
> --- a/drivers/leds/leds-turris-omnia.c
> +++ b/drivers/leds/leds-turris-omnia.c
> @@ -31,7 +31,7 @@ struct omnia_led {
>  	struct led_classdev_mc mc_cdev;
>  	struct mc_subled subled_info[OMNIA_LED_NUM_CHANNELS];
>  	u8 cached_channels[OMNIA_LED_NUM_CHANNELS];
> -	bool on;
> +	bool on, hwtrig;
>  	int reg;
>  };
>  
> @@ -123,12 +123,14 @@ static int omnia_led_brightness_set_blocking(struct led_classdev *cdev,
>  
>  	/*
>  	 * Only recalculate RGB brightnesses from intensities if brightness is
> -	 * non-zero. Otherwise we won't be using them and we can save ourselves
> -	 * some software divisions (Omnia's CPU does not implement the division
> -	 * instruction).
> +	 * non-zero (if it is zero and the LED is in HW blinking mode, we use
> +	 * max_brightness as brightness). Otherwise we won't be using them and
> +	 * we can save ourselves some software divisions (Omnia's CPU does not
> +	 * implement the division instruction).
>  	 */
> -	if (brightness) {
> -		led_mc_calc_color_components(mc_cdev, brightness);
> +	if (brightness || led->hwtrig) {
> +		led_mc_calc_color_components(mc_cdev, brightness ?:
> +						      cdev->max_brightness);
>  
>  		/*
>  		 * Send color command only if brightness is non-zero and the RGB
> @@ -138,8 +140,11 @@ static int omnia_led_brightness_set_blocking(struct led_classdev *cdev,
>  			err = omnia_led_send_color_cmd(leds->client, led);
>  	}
>  
> -	/* send on/off state change only if (bool)brightness changed */
> -	if (!err && !brightness != !led->on) {
> +	/*
> +	 * Send on/off state change only if (bool)brightness changed and the LED
> +	 * is not being blinked by HW.
> +	 */
> +	if (!err && !led->hwtrig && !brightness != !led->on) {
>  		u8 state = CMD_LED_STATE_LED(led->reg);
>  
>  		if (brightness)
> @@ -155,6 +160,70 @@ static int omnia_led_brightness_set_blocking(struct led_classdev *cdev,
>  	return err;
>  }
>  
> +static struct led_hw_trigger_type omnia_hw_trigger_type;
> +
> +static int omnia_hwtrig_activate(struct led_classdev *cdev)
> +{
> +	struct led_classdev_mc *mc_cdev = lcdev_to_mccdev(cdev);
> +	struct omnia_leds *leds = dev_get_drvdata(cdev->dev->parent);
> +	struct omnia_led *led = to_omnia_led(mc_cdev);
> +	int err = 0;
> +
> +	mutex_lock(&leds->lock);
> +
> +	if (!led->on) {
> +		/*
> +		 * If the LED is off (brightness was set to 0), the last
> +		 * configured color was not necessarily sent to the MCU.
> +		 * Recompute with max_brightness and send if needed.
> +		 */
> +		led_mc_calc_color_components(mc_cdev, cdev->max_brightness);
> +
> +		if (omnia_led_channels_changed(led))
> +			err = omnia_led_send_color_cmd(leds->client, led);
> +	}
> +
> +	if (!err) {
> +		/* put the LED into MCU controlled mode */

Nit: You improved the comment above to be more grammatically correct by
starting with an uppercase character.  Please continue with this
improvement for all comments there in.

> +		err = omnia_cmd_write(leds->client, CMD_LED_MODE,
> +				      CMD_LED_MODE_LED(led->reg));
> +		if (!err)
> +			led->hwtrig = true;
> +	}
> +
> +	mutex_unlock(&leds->lock);
> +
> +	return err;
> +}
> +
> +static void omnia_hwtrig_deactivate(struct led_classdev *cdev)
> +{
> +	struct omnia_leds *leds = dev_get_drvdata(cdev->dev->parent);
> +	struct omnia_led *led = to_omnia_led(lcdev_to_mccdev(cdev));
> +	int err;
> +
> +	mutex_lock(&leds->lock);
> +
> +	led->hwtrig = false;
> +
> +	/* put the LED into software mode */
> +	err = omnia_cmd_write(leds->client, CMD_LED_MODE,
> +			      CMD_LED_MODE_LED(led->reg) | CMD_LED_MODE_USER);
> +
> +	mutex_unlock(&leds->lock);
> +
> +	if (err < 0)
> +		dev_err(cdev->dev, "Cannot put LED to software mode: %i\n",
> +			err);
> +}
> +
> +static struct led_trigger omnia_hw_trigger = {
> +	.name		= "omnia-mcu",
> +	.activate	= omnia_hwtrig_activate,
> +	.deactivate	= omnia_hwtrig_deactivate,
> +	.trigger_type	= &omnia_hw_trigger_type,

Not sure I understand this interface.

Why not just set a bool instead of carrying around an empty struct?

> +};
> +
>  static int omnia_led_register(struct i2c_client *client, struct omnia_led *led,
>  			      struct device_node *np)
>  {
> @@ -198,6 +267,12 @@ static int omnia_led_register(struct i2c_client *client, struct omnia_led *led,
>  	cdev = &led->mc_cdev.led_cdev;
>  	cdev->max_brightness = 255;
>  	cdev->brightness_set_blocking = omnia_led_brightness_set_blocking;
> +	cdev->trigger_type = &omnia_hw_trigger_type;
> +	/*
> +	 * Use the omnia-mcu trigger as the default trigger. It may be rewritten
> +	 * by LED class from the linux,default-trigger property.
> +	 */
> +	cdev->default_trigger = omnia_hw_trigger.name;
>  
>  	/* put the LED into software mode */
>  	ret = omnia_cmd_write(client, CMD_LED_MODE, CMD_LED_MODE_LED(led->reg) |
> @@ -310,6 +385,12 @@ static int omnia_leds_probe(struct i2c_client *client)
>  
>  	mutex_init(&leds->lock);
>  
> +	ret = devm_led_trigger_register(dev, &omnia_hw_trigger);
> +	if (ret < 0) {
> +		dev_err(dev, "Cannot register private LED trigger: %d\n", ret);
> +		return ret;
> +	}
> +
>  	led = &leds->leds[0];
>  	for_each_available_child_of_node(np, child) {
>  		ret = omnia_led_register(client, led, child);
> -- 
> 2.41.0
> 

-- 
Lee Jones [李琼斯]

Re: [PATCH v3 5/6] leds: turris-omnia: support HW controlled mode via private trigger

[Marek Behún]

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

> > +	if (!err) {
> > +		/* put the LED into MCU controlled mode */  
> 
> Nit: You improved the comment above to be more grammatically correct by
> starting with an uppercase character.  Please continue with this
> improvement for all comments there in.

OK.

> > +static struct led_trigger omnia_hw_trigger = {
> > +	.name		= "omnia-mcu",
> > +	.activate	= omnia_hwtrig_activate,
> > +	.deactivate	= omnia_hwtrig_deactivate,
> > +	.trigger_type	= &omnia_hw_trigger_type,  
> 
> Not sure I understand this interface.
> 
> Why not just set a bool instead of carrying around an empty struct?

Let me explain:

So, if a LED class device has the same trigger type as a LED trigger,
the trigger will be available for that LED (it is listed in the sysfs
trigger file and can be chosen).

So we have a mechanism to "pair" a LED with a given trigger, to make it
possible for the LED core to distinguish whether a given trigger is
available for the LED.

A boolean information would not be enough: if we used a bool, we would
know that the trigger is private. But the LED core would not know for
which LEDs the trigger should be avaiable.

In pseudocode:

list_triggers_for_led(led) {
  for (trigger in trigger_list) {
    if (!trigger.trigger_type || trigger.trigger_type ==
                                 led.trigger_type)
      trigger is available for led
    else 
      trigger is not available for led
  }
}

Is this explaination good enough?

Marek

Re: [PATCH v3 5/6] leds: turris-omnia: support HW controlled mode via private trigger

[Lee Jones]

On Mon, 21 Aug 2023, Marek Behún wrote:

> On Fri, 18 Aug 2023 10:09:21 +0100
> Lee Jones <lee@kernel.org> wrote:
> 
> > > +	if (!err) {
> > > +		/* put the LED into MCU controlled mode */  
> > 
> > Nit: You improved the comment above to be more grammatically correct by
> > starting with an uppercase character.  Please continue with this
> > improvement for all comments there in.
> 
> OK.
> 
> > > +static struct led_trigger omnia_hw_trigger = {
> > > +	.name		= "omnia-mcu",
> > > +	.activate	= omnia_hwtrig_activate,
> > > +	.deactivate	= omnia_hwtrig_deactivate,
> > > +	.trigger_type	= &omnia_hw_trigger_type,  
> > 
> > Not sure I understand this interface.
> > 
> > Why not just set a bool instead of carrying around an empty struct?
> 
> Let me explain:
> 
> So, if a LED class device has the same trigger type as a LED trigger,
> the trigger will be available for that LED (it is listed in the sysfs
> trigger file and can be chosen).
> 
> So we have a mechanism to "pair" a LED with a given trigger, to make it
> possible for the LED core to distinguish whether a given trigger is
> available for the LED.
> 
> A boolean information would not be enough: if we used a bool, we would
> know that the trigger is private. But the LED core would not know for
> which LEDs the trigger should be avaiable.
> 
> In pseudocode:
> 
> list_triggers_for_led(led) {
>   for (trigger in trigger_list) {
>     if (!trigger.trigger_type || trigger.trigger_type ==
>                                  led.trigger_type)
>       trigger is available for led
>     else 
>       trigger is not available for led
>   }
> }
> 
> Is this explaination good enough?

Yes, thank you.

-- 
Lee Jones [李琼斯]

[PATCH v3 6/6] leds: turris-omnia: add support for enabling/disabling HW gamma correction

[Marek Behún]

If the MCU on Turris Omnia is running newer firmware versions, the LED
controller supports RGB gamma correction (and enables it by default for
newer boards).

Determine whether the gamma correction setting feature is supported and
add the ability to set it via sysfs attribute file.

Signed-off-by: Marek Behún <kabel@kernel.org>
---
 .../sysfs-class-led-driver-turris-omnia       |  14 ++
 drivers/leds/leds-turris-omnia.c              | 135 +++++++++++++++---
 2 files changed, 132 insertions(+), 17 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-class-led-driver-turris-omnia b/Documentation/ABI/testing/sysfs-class-led-driver-turris-omnia
index c4d46970c1cf..369b4ae8be5f 100644
--- a/Documentation/ABI/testing/sysfs-class-led-driver-turris-omnia
+++ b/Documentation/ABI/testing/sysfs-class-led-driver-turris-omnia
@@ -12,3 +12,17 @@ Description:	(RW) On the front panel of the Turris Omnia router there is also
 		able to change this setting from software.
 
 		Format: %i
+
+What:		/sys/class/leds/<led>/device/gamma_correction
+Date:		August 2023
+KernelVersion:	6.6
+Contact:	Marek Behún <kabel@kernel.org>
+Description:	(RW) Newer versions of the microcontroller firmware of the
+		Turris Omnia router support gamma correction for the RGB LEDs.
+		This feature can be enabled/disabled by writing to this file.
+
+		If the feature is not supported because the MCU firmware is too
+		old, the file always reads as 0, and writing to the file results
+		in the EOPNOTSUPP error.
+
+		Format: %i
diff --git a/drivers/leds/leds-turris-omnia.c b/drivers/leds/leds-turris-omnia.c
index 180b0cbeb92e..75cc7d2cf6d1 100644
--- a/drivers/leds/leds-turris-omnia.c
+++ b/drivers/leds/leds-turris-omnia.c
@@ -15,17 +15,30 @@
 #define OMNIA_BOARD_LEDS	12
 #define OMNIA_LED_NUM_CHANNELS	3
 
-#define CMD_LED_MODE		3
-#define CMD_LED_MODE_LED(l)	((l) & 0x0f)
-#define CMD_LED_MODE_USER	0x10
+/* MCU controller commands at I2C address 0x2a */
+#define OMNIA_MCU_I2C_ADDR		0x2a
 
-#define CMD_LED_STATE		4
-#define CMD_LED_STATE_LED(l)	((l) & 0x0f)
-#define CMD_LED_STATE_ON	0x10
+#define CMD_GET_STATUS_WORD		0x01
+#define STS_FEATURES_SUPPORTED		BIT(2)
 
-#define CMD_LED_COLOR		5
-#define CMD_LED_SET_BRIGHTNESS	7
-#define CMD_LED_GET_BRIGHTNESS	8
+#define CMD_GET_FEATURES		0x10
+#define FEAT_LED_GAMMA_CORRECTION	BIT(5)
+
+/* LED controller commands at I2C address 0x2b */
+#define CMD_LED_MODE			0x03
+#define CMD_LED_MODE_LED(l)		((l) & 0x0f)
+#define CMD_LED_MODE_USER		0x10
+
+#define CMD_LED_STATE			0x04
+#define CMD_LED_STATE_LED(l)		((l) & 0x0f)
+#define CMD_LED_STATE_ON		0x10
+
+#define CMD_LED_COLOR			0x05
+#define CMD_LED_SET_BRIGHTNESS		0x07
+#define CMD_LED_GET_BRIGHTNESS		0x08
+
+#define CMD_SET_GAMMA_CORRECTION	0x30
+#define CMD_GET_GAMMA_CORRECTION	0x31
 
 struct omnia_led {
 	struct led_classdev_mc mc_cdev;
@@ -40,6 +53,7 @@ struct omnia_led {
 struct omnia_leds {
 	struct i2c_client *client;
 	struct mutex lock;
+	bool has_gamma_correction;
 	struct omnia_led leds[];
 };
 
@@ -53,30 +67,42 @@ static int omnia_cmd_write(const struct i2c_client *client, u8 cmd, u8 val)
 	return ret < 0 ? ret : 0;
 }
 
-static int omnia_cmd_read(const struct i2c_client *client, u8 cmd)
+static int omnia_cmd_read_raw(struct i2c_adapter *adapter, u8 addr, u8 cmd,
+			      void *reply, size_t len)
 {
 	struct i2c_msg msgs[2];
-	u8 reply;
 	int ret;
 
-	msgs[0].addr = client->addr;
+	msgs[0].addr = addr;
 	msgs[0].flags = 0;
 	msgs[0].len = 1;
 	msgs[0].buf = &cmd;
-	msgs[1].addr = client->addr;
+	msgs[1].addr = addr;
 	msgs[1].flags = I2C_M_RD;
-	msgs[1].len = 1;
-	msgs[1].buf = &reply;
+	msgs[1].len = len;
+	msgs[1].buf = reply;
 
-	ret = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
+	ret = i2c_transfer(adapter, msgs, ARRAY_SIZE(msgs));
 	if (likely(ret == ARRAY_SIZE(msgs)))
-		return reply;
+		return 0;
 	else if (ret < 0)
 		return ret;
 	else
 		return -EIO;
 }
 
+static int omnia_cmd_read(const struct i2c_client *client, u8 cmd)
+{
+	u8 reply;
+	int ret;
+
+	ret = omnia_cmd_read_raw(client->adapter, client->addr, cmd, &reply, 1);
+	if (ret < 0)
+		return ret;
+
+	return reply;
+}
+
 static int omnia_led_send_color_cmd(const struct i2c_client *client,
 				    struct omnia_led *led)
 {
@@ -353,12 +379,74 @@ static ssize_t brightness_store(struct device *dev, struct device_attribute *a,
 }
 static DEVICE_ATTR_RW(brightness);
 
+static ssize_t gamma_correction_show(struct device *dev,
+				     struct device_attribute *a, char *buf)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct omnia_leds *leds = i2c_get_clientdata(client);
+	int ret;
+
+	if (leds->has_gamma_correction) {
+		ret = omnia_cmd_read(client, CMD_GET_GAMMA_CORRECTION);
+		if (ret < 0)
+			return ret;
+	} else {
+		ret = 0;
+	}
+
+	return sysfs_emit(buf, "%d\n", !!ret);
+}
+
+static ssize_t gamma_correction_store(struct device *dev,
+				      struct device_attribute *a,
+				      const char *buf, size_t count)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct omnia_leds *leds = i2c_get_clientdata(client);
+	bool val;
+	int ret;
+
+	if (!leds->has_gamma_correction)
+		return -EOPNOTSUPP;
+
+	if (kstrtobool(buf, &val) < 0)
+		return -EINVAL;
+
+	ret = omnia_cmd_write(client, CMD_SET_GAMMA_CORRECTION, val);
+
+	return ret < 0 ? ret : count;
+}
+static DEVICE_ATTR_RW(gamma_correction);
+
 static struct attribute *omnia_led_controller_attrs[] = {
 	&dev_attr_brightness.attr,
+	&dev_attr_gamma_correction.attr,
 	NULL,
 };
 ATTRIBUTE_GROUPS(omnia_led_controller);
 
+static int omnia_mcu_get_features(const struct i2c_client *client)
+{
+	u16 reply;
+	int err;
+
+	err = omnia_cmd_read_raw(client->adapter, OMNIA_MCU_I2C_ADDR,
+				 CMD_GET_STATUS_WORD, &reply, sizeof(reply));
+	if (err < 0)
+		return err;
+
+	/* check whether MCU firmware supports the CMD_GET_FEAUTRES command */
+	if (!(le16_to_cpu(reply) & STS_FEATURES_SUPPORTED))
+		return 0;
+
+	err = omnia_cmd_read_raw(client->adapter, OMNIA_MCU_I2C_ADDR,
+				 CMD_GET_FEATURES, &reply, sizeof(reply));
+	if (err < 0)
+		return err;
+
+	return le16_to_cpu(reply);
+}
+
 static int omnia_leds_probe(struct i2c_client *client)
 {
 	struct device *dev = &client->dev;
@@ -383,6 +471,19 @@ static int omnia_leds_probe(struct i2c_client *client)
 	leds->client = client;
 	i2c_set_clientdata(client, leds);
 
+	ret = omnia_mcu_get_features(client);
+	if (ret < 0) {
+		dev_err(dev, "Cannot determine MCU supported features: %d\n",
+			ret);
+		return ret;
+	}
+
+	leds->has_gamma_correction = ret & FEAT_LED_GAMMA_CORRECTION;
+	if (!leds->has_gamma_correction)
+		dev_info(dev,
+			 "Your board's MCU firmware does not support the LED gamma correction feature.\n"
+			 "   Consider upgrading MCU firmware with the omnia-mcutool utility.\n");
+
 	mutex_init(&leds->lock);
 
 	ret = devm_led_trigger_register(dev, &omnia_hw_trigger);
-- 
2.41.0

Re: [PATCH v3 6/6] leds: turris-omnia: add support for enabling/disabling HW gamma correction

[Lee Jones]

On Wed, 02 Aug 2023, Marek Behún wrote:

> If the MCU on Turris Omnia is running newer firmware versions, the LED
> controller supports RGB gamma correction (and enables it by default for
> newer boards).
> 
> Determine whether the gamma correction setting feature is supported and
> add the ability to set it via sysfs attribute file.
> 
> Signed-off-by: Marek Behún <kabel@kernel.org>
> ---
>  .../sysfs-class-led-driver-turris-omnia       |  14 ++
>  drivers/leds/leds-turris-omnia.c              | 135 +++++++++++++++---
>  2 files changed, 132 insertions(+), 17 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-class-led-driver-turris-omnia b/Documentation/ABI/testing/sysfs-class-led-driver-turris-omnia
> index c4d46970c1cf..369b4ae8be5f 100644
> --- a/Documentation/ABI/testing/sysfs-class-led-driver-turris-omnia
> +++ b/Documentation/ABI/testing/sysfs-class-led-driver-turris-omnia
> @@ -12,3 +12,17 @@ Description:	(RW) On the front panel of the Turris Omnia router there is also
>  		able to change this setting from software.
>  
>  		Format: %i
> +
> +What:		/sys/class/leds/<led>/device/gamma_correction
> +Date:		August 2023
> +KernelVersion:	6.6
> +Contact:	Marek Behún <kabel@kernel.org>
> +Description:	(RW) Newer versions of the microcontroller firmware of the
> +		Turris Omnia router support gamma correction for the RGB LEDs.
> +		This feature can be enabled/disabled by writing to this file.
> +
> +		If the feature is not supported because the MCU firmware is too
> +		old, the file always reads as 0, and writing to the file results
> +		in the EOPNOTSUPP error.
> +
> +		Format: %i
> diff --git a/drivers/leds/leds-turris-omnia.c b/drivers/leds/leds-turris-omnia.c
> index 180b0cbeb92e..75cc7d2cf6d1 100644
> --- a/drivers/leds/leds-turris-omnia.c
> +++ b/drivers/leds/leds-turris-omnia.c
> @@ -15,17 +15,30 @@
>  #define OMNIA_BOARD_LEDS	12
>  #define OMNIA_LED_NUM_CHANNELS	3
>  
> -#define CMD_LED_MODE		3
> -#define CMD_LED_MODE_LED(l)	((l) & 0x0f)
> -#define CMD_LED_MODE_USER	0x10
> +/* MCU controller commands at I2C address 0x2a */
> +#define OMNIA_MCU_I2C_ADDR		0x2a
>  
> -#define CMD_LED_STATE		4
> -#define CMD_LED_STATE_LED(l)	((l) & 0x0f)
> -#define CMD_LED_STATE_ON	0x10
> +#define CMD_GET_STATUS_WORD		0x01
> +#define STS_FEATURES_SUPPORTED		BIT(2)
>  
> -#define CMD_LED_COLOR		5
> -#define CMD_LED_SET_BRIGHTNESS	7
> -#define CMD_LED_GET_BRIGHTNESS	8
> +#define CMD_GET_FEATURES		0x10
> +#define FEAT_LED_GAMMA_CORRECTION	BIT(5)
> +
> +/* LED controller commands at I2C address 0x2b */
> +#define CMD_LED_MODE			0x03
> +#define CMD_LED_MODE_LED(l)		((l) & 0x0f)
> +#define CMD_LED_MODE_USER		0x10
> +
> +#define CMD_LED_STATE			0x04
> +#define CMD_LED_STATE_LED(l)		((l) & 0x0f)
> +#define CMD_LED_STATE_ON		0x10
> +
> +#define CMD_LED_COLOR			0x05
> +#define CMD_LED_SET_BRIGHTNESS		0x07
> +#define CMD_LED_GET_BRIGHTNESS		0x08
> +
> +#define CMD_SET_GAMMA_CORRECTION	0x30
> +#define CMD_GET_GAMMA_CORRECTION	0x31
>  
>  struct omnia_led {
>  	struct led_classdev_mc mc_cdev;
> @@ -40,6 +53,7 @@ struct omnia_led {
>  struct omnia_leds {
>  	struct i2c_client *client;
>  	struct mutex lock;
> +	bool has_gamma_correction;
>  	struct omnia_led leds[];
>  };
>  
> @@ -53,30 +67,42 @@ static int omnia_cmd_write(const struct i2c_client *client, u8 cmd, u8 val)
>  	return ret < 0 ? ret : 0;
>  }
>  
> -static int omnia_cmd_read(const struct i2c_client *client, u8 cmd)
> +static int omnia_cmd_read_raw(struct i2c_adapter *adapter, u8 addr, u8 cmd,
> +			      void *reply, size_t len)
>  {
>  	struct i2c_msg msgs[2];
> -	u8 reply;
>  	int ret;
>  
> -	msgs[0].addr = client->addr;
> +	msgs[0].addr = addr;
>  	msgs[0].flags = 0;
>  	msgs[0].len = 1;
>  	msgs[0].buf = &cmd;
> -	msgs[1].addr = client->addr;
> +	msgs[1].addr = addr;
>  	msgs[1].flags = I2C_M_RD;
> -	msgs[1].len = 1;
> -	msgs[1].buf = &reply;
> +	msgs[1].len = len;
> +	msgs[1].buf = reply;
>  
> -	ret = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
> +	ret = i2c_transfer(adapter, msgs, ARRAY_SIZE(msgs));
>  	if (likely(ret == ARRAY_SIZE(msgs)))
> -		return reply;
> +		return 0;

Why not return ret and use that throughout?

>  	else if (ret < 0)
>  		return ret;
>  	else
>  		return -EIO;
>  }

[...]

-- 
Lee Jones [李琼斯]

Re: [PATCH v3 6/6] leds: turris-omnia: add support for enabling/disabling HW gamma correction

[Marek Behún]

On Fri, 18 Aug 2023 11:30:03 +0100
Lee Jones <lee@kernel.org> wrote:

> > -static int omnia_cmd_read(const struct i2c_client *client, u8 cmd)
> > +static int omnia_cmd_read_raw(struct i2c_adapter *adapter, u8 addr, u8 cmd,
> > +			      void *reply, size_t len)
> >  {
> >  	struct i2c_msg msgs[2];
> > -	u8 reply;
> >  	int ret;
> >  
> > -	msgs[0].addr = client->addr;
> > +	msgs[0].addr = addr;
> >  	msgs[0].flags = 0;
> >  	msgs[0].len = 1;
> >  	msgs[0].buf = &cmd;
> > -	msgs[1].addr = client->addr;
> > +	msgs[1].addr = addr;
> >  	msgs[1].flags = I2C_M_RD;
> > -	msgs[1].len = 1;
> > -	msgs[1].buf = &reply;
> > +	msgs[1].len = len;
> > +	msgs[1].buf = reply;
> >  
> > -	ret = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
> > +	ret = i2c_transfer(adapter, msgs, ARRAY_SIZE(msgs));
> >  	if (likely(ret == ARRAY_SIZE(msgs)))
> > -		return reply;
> > +		return 0;  
> 
> Why not return ret and use that throughout?
> 
> >  	else if (ret < 0)
> >  		return ret;
> >  	else
> >  		return -EIO;
> >  }  
> 

As I explained to your similar question in your reply to patch 2/6

  https://lore.kernel.org/linux-leds/20230821120136.130cc916@dellmb/T/#me5782c9896bc3d994cb36e8b5485d6368cd92da0

the reason I normalize to zero is because of the intended semantics of
these functions: return 0 on success, -errno on error.

If instead in omnia_cmd_read() and omnia_cmd_write() I simply return
the return value of the underlying functions, which are
  i2c_transfer()    for omnia_cmd_read()
  i2c_master_send() for omnia_cmd_write()
the semantics would be inconsistent, because:
  i2c_transfer() returns the number of successful I2C transfers,
  i2c_manster_send() returns the number of written bytes over I2C.

Nonetheless, if you insist on this change, I will do it. Reluctantly
and with a silent protest, but I will not argue further, since I want
this functionality in upstream more than to argue over nitpicks :-)

Marek

Re: [PATCH v3 6/6] leds: turris-omnia: add support for enabling/disabling HW gamma correction

[Lee Jones]

On Mon, 21 Aug 2023, Marek Behún wrote:

> On Fri, 18 Aug 2023 11:30:03 +0100
> Lee Jones <lee@kernel.org> wrote:
> 
> > > -static int omnia_cmd_read(const struct i2c_client *client, u8 cmd)
> > > +static int omnia_cmd_read_raw(struct i2c_adapter *adapter, u8 addr, u8 cmd,
> > > +			      void *reply, size_t len)
> > >  {
> > >  	struct i2c_msg msgs[2];
> > > -	u8 reply;
> > >  	int ret;
> > >  
> > > -	msgs[0].addr = client->addr;
> > > +	msgs[0].addr = addr;
> > >  	msgs[0].flags = 0;
> > >  	msgs[0].len = 1;
> > >  	msgs[0].buf = &cmd;
> > > -	msgs[1].addr = client->addr;
> > > +	msgs[1].addr = addr;
> > >  	msgs[1].flags = I2C_M_RD;
> > > -	msgs[1].len = 1;
> > > -	msgs[1].buf = &reply;
> > > +	msgs[1].len = len;
> > > +	msgs[1].buf = reply;
> > >  
> > > -	ret = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
> > > +	ret = i2c_transfer(adapter, msgs, ARRAY_SIZE(msgs));
> > >  	if (likely(ret == ARRAY_SIZE(msgs)))
> > > -		return reply;
> > > +		return 0;  
> > 
> > Why not return ret and use that throughout?
> > 
> > >  	else if (ret < 0)
> > >  		return ret;
> > >  	else
> > >  		return -EIO;
> > >  }  
> > 
> 
> As I explained to your similar question in your reply to patch 2/6
> 
>   https://lore.kernel.org/linux-leds/20230821120136.130cc916@dellmb/T/#me5782c9896bc3d994cb36e8b5485d6368cd92da0
> 
> the reason I normalize to zero is because of the intended semantics of
> these functions: return 0 on success, -errno on error.
> 
> If instead in omnia_cmd_read() and omnia_cmd_write() I simply return
> the return value of the underlying functions, which are
>   i2c_transfer()    for omnia_cmd_read()
>   i2c_master_send() for omnia_cmd_write()
> the semantics would be inconsistent, because:
>   i2c_transfer() returns the number of successful I2C transfers,
>   i2c_manster_send() returns the number of written bytes over I2C.
> 
> Nonetheless, if you insist on this change, I will do it. Reluctantly
> and with a silent protest, but I will not argue further, since I want
> this functionality in upstream more than to argue over nitpicks :-)

API semantics is not nitpicking.  Besides, we have time to talk.  We are
too late in the cycle for this to get merge this side of the merge
window anyway.

Read and write APIs, even abstracted ones such as these generally return
the number of bytes successfully read and written respectively.  If you
are going to normalise, then please do so against this standard.

-- 
Lee Jones [李琼斯]

Re: [PATCH v3 0/6] leds: turris-omnia: updates

[Marek Behún]

Any comments on this series?

Marek