diff mbox series

[leds,v5,02/12] leds: turris-omnia: Use command execution functions from the MCU driver

Message ID 20241104141924.18816-3-kabel@kernel.org (mailing list archive)
State Superseded
Headers show
Series Turris Omnia LED driver changes | expand

Commit Message

Marek Behún Nov. 4, 2024, 2:19 p.m. UTC
Use the MCU command execution functions from the MCU driver instead of
the ad-hoc implementation in the LED driver. This allows as to drop the
LED driver implementation, which is a duplicate.

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

Comments

Andy Shevchenko Nov. 4, 2024, 2:51 p.m. UTC | #1
On Mon, Nov 04, 2024 at 03:19:14PM +0100, Marek Behún wrote:
> Use the MCU command execution functions from the MCU driver instead of
> the ad-hoc implementation in the LED driver. This allows as to drop the
> LED driver implementation, which is a duplicate.

...

> +static int omnia_match_mcu_client(struct device *dev, void *data)
> +{
> +	struct i2c_client *client;
> +
> +	client = i2c_verify_client(dev);
> +	if (!client)
> +		return 0;
> +
> +	return client->addr == OMNIA_MCU_I2C_ADDR;
> +}
> +
> +static int omnia_find_mcu_and_get_features(struct device *dev)
> +{
> +	struct device *mcu_dev;
> +	int ret;
> +
> +	mcu_dev = device_find_child(dev->parent, NULL, omnia_match_mcu_client);
> +	if (!mcu_dev)
> +		return -ENODEV;
> +
> +	ret = omnia_mcu_get_features(i2c_verify_client(mcu_dev));
> +
> +	put_device(mcu_dev);
> +
> +	return ret;
>  }

I'm wondering why the MCU driver (and node) is not represented as syscon
(with some regmap beneath it).

In such a case it would be something like

  foo = syscon_regmap_lookup_by_compatible();

here instead of all these dances.
Marek Behún Nov. 5, 2024, 9:07 a.m. UTC | #2
On Mon, Nov 04, 2024 at 04:51:24PM +0200, Andy Shevchenko wrote:
> On Mon, Nov 04, 2024 at 03:19:14PM +0100, Marek Behún wrote:
> > Use the MCU command execution functions from the MCU driver instead of
> > the ad-hoc implementation in the LED driver. This allows as to drop the
> > LED driver implementation, which is a duplicate.
> 
> ...
> 
> > +static int omnia_match_mcu_client(struct device *dev, void *data)
> > +{
> > +	struct i2c_client *client;
> > +
> > +	client = i2c_verify_client(dev);
> > +	if (!client)
> > +		return 0;
> > +
> > +	return client->addr == OMNIA_MCU_I2C_ADDR;
> > +}
> > +
> > +static int omnia_find_mcu_and_get_features(struct device *dev)
> > +{
> > +	struct device *mcu_dev;
> > +	int ret;
> > +
> > +	mcu_dev = device_find_child(dev->parent, NULL, omnia_match_mcu_client);
> > +	if (!mcu_dev)
> > +		return -ENODEV;
> > +
> > +	ret = omnia_mcu_get_features(i2c_verify_client(mcu_dev));
> > +
> > +	put_device(mcu_dev);
> > +
> > +	return ret;
> >  }
> 
> I'm wondering why the MCU driver (and node) is not represented as syscon
> (with some regmap beneath it).
> 
> In such a case it would be something like
> 
>   foo = syscon_regmap_lookup_by_compatible();
> 
> here instead of all these dances.

Hi Andy,

the MCU interface is command-reply oriented. It is incompatible with
regmap. I investigated this back in 2019 and explained to Jacek why it
is not possible, but can't find the e-mail on mailing lists, so I am
attaching it.

So regmap is most probably not possible, unless things changed.

It is possible to add MCU node to the DT binding and find the device
that way. But if the device-tree does not contain the MCU node, the
driver would still have to fall back to this dance, for backwards
compatibility. Otherwise it would not be able to determine whether gamma
correction is supported with old device tree, as it does currently.

I guess I could break backwards compatibility with old device tree with
this small feature. I don't think there are any users that don't use
TurrisOS, do upgrade the kernel, but don't upgrade the device-tree...

Marek
Andy Shevchenko Nov. 5, 2024, 2:49 p.m. UTC | #3
On Tue, Nov 05, 2024 at 10:07:51AM +0100, Marek Behún wrote:
> On Mon, Nov 04, 2024 at 04:51:24PM +0200, Andy Shevchenko wrote:
> > On Mon, Nov 04, 2024 at 03:19:14PM +0100, Marek Behún wrote:

...

> > I'm wondering why the MCU driver (and node) is not represented as syscon
> > (with some regmap beneath it).
> > 
> > In such a case it would be something like
> > 
> >   foo = syscon_regmap_lookup_by_compatible();
> > 
> > here instead of all these dances.
> 
> Hi Andy,
> 
> the MCU interface is command-reply oriented. It is incompatible with
> regmap.

I'm not sure I understand the impediment here. There are plenty of hardware
that uses the similar approach and well compatible with regmap (assuming
custom ->read*() / ->write*() callbacks)...

> I investigated this back in 2019 and explained to Jacek why it
> is not possible, but can't find the e-mail on mailing lists, so I am
> attaching it.

...but I'm not insisting you to revisit this right now, just maybe
you can think more about this again at some point.

> So regmap is most probably not possible, unless things changed.
> 
> It is possible to add MCU node to the DT binding and find the device
> that way. But if the device-tree does not contain the MCU node, the
> driver would still have to fall back to this dance, for backwards
> compatibility. Otherwise it would not be able to determine whether gamma
> correction is supported with old device tree, as it does currently.
> 
> I guess I could break backwards compatibility with old device tree with
> this small feature. I don't think there are any users that don't use
> TurrisOS, do upgrade the kernel, but don't upgrade the device-tree...
diff mbox series

Patch

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index b784bb74a837..fcbe93b61e49 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -217,6 +217,7 @@  config LEDS_TURRIS_OMNIA
 	depends on I2C
 	depends on MACH_ARMADA_38X || COMPILE_TEST
 	depends on OF
+	depends on TURRIS_OMNIA_MCU
 	select LEDS_TRIGGERS
 	help
 	  This option enables basic support for the LEDs found on the front
diff --git a/drivers/leds/leds-turris-omnia.c b/drivers/leds/leds-turris-omnia.c
index 4cff8c4b020c..00cd3bb86703 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, 2023 by Marek Behún <kabel@kernel.org>
+ * 2020, 2023, 2024 by Marek Behún <kabel@kernel.org>
  */
 
 #include <linux/i2c.h>
@@ -10,6 +10,7 @@ 
 #include <linux/module.h>
 #include <linux/mutex.h>
 #include <linux/of.h>
+#include <linux/turris-omnia-mcu-interface.h>
 #include "leds.h"
 
 #define OMNIA_BOARD_LEDS	12
@@ -57,66 +58,21 @@  struct omnia_leds {
 	struct omnia_led leds[];
 };
 
-static int omnia_cmd_write_u8(const struct i2c_client *client, u8 cmd, u8 val)
+static int omnia_cmd_set_color(const struct i2c_client *client, u8 led, u8 r, u8 g, u8 b)
 {
-	u8 buf[2] = { cmd, val };
-	int ret;
-
-	ret = i2c_master_send(client, buf, sizeof(buf));
+	u8 buf[5] = { CMD_LED_COLOR, led, r, g, b };
 
-	return ret < 0 ? ret : 0;
-}
-
-static int omnia_cmd_read_raw(struct i2c_adapter *adapter, u8 addr, u8 cmd,
-			      void *reply, size_t len)
-{
-	struct i2c_msg msgs[2];
-	int ret;
-
-	msgs[0].addr = addr;
-	msgs[0].flags = 0;
-	msgs[0].len = 1;
-	msgs[0].buf = &cmd;
-	msgs[1].addr = addr;
-	msgs[1].flags = I2C_M_RD;
-	msgs[1].len = len;
-	msgs[1].buf = reply;
-
-	ret = i2c_transfer(adapter, msgs, ARRAY_SIZE(msgs));
-	if (likely(ret == ARRAY_SIZE(msgs)))
-		return 0;
-	else if (ret < 0)
-		return ret;
-	else
-		return -EIO;
-}
-
-static int omnia_cmd_read_u8(const struct i2c_client *client, u8 cmd)
-{
-	u8 reply;
-	int err;
-
-	err = omnia_cmd_read_raw(client->adapter, client->addr, cmd, &reply, 1);
-	if (err)
-		return err;
-
-	return reply;
+	return omnia_cmd_write(client, buf, sizeof(buf));
 }
 
 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);
+	ret = omnia_cmd_set_color(client, led->reg, led->subled_info[0].brightness,
+				  led->subled_info[1].brightness, led->subled_info[2].brightness);
 	if (ret < 0)
 		return ret;
 
@@ -352,14 +308,14 @@  static ssize_t brightness_show(struct device *dev, struct device_attribute *a,
 			       char *buf)
 {
 	struct i2c_client *client = to_i2c_client(dev);
-	int ret;
-
-	ret = omnia_cmd_read_u8(client, CMD_LED_GET_BRIGHTNESS);
+	u8 reply;
+	int err;
 
-	if (ret < 0)
-		return ret;
+	err = omnia_cmd_read_u8(client, CMD_LED_GET_BRIGHTNESS, &reply);
+	if (err < 0)
+		return err;
 
-	return sysfs_emit(buf, "%d\n", ret);
+	return sysfs_emit(buf, "%d\n", reply);
 }
 
 static ssize_t brightness_store(struct device *dev, struct device_attribute *a,
@@ -386,17 +342,16 @@  static ssize_t gamma_correction_show(struct device *dev,
 {
 	struct i2c_client *client = to_i2c_client(dev);
 	struct omnia_leds *leds = i2c_get_clientdata(client);
-	int ret;
+	u8 reply = 0;
+	int err;
 
 	if (leds->has_gamma_correction) {
-		ret = omnia_cmd_read_u8(client, CMD_GET_GAMMA_CORRECTION);
-		if (ret < 0)
-			return ret;
-	} else {
-		ret = 0;
+		err = omnia_cmd_read_u8(client, CMD_GET_GAMMA_CORRECTION, &reply);
+		if (err < 0)
+			return err;
 	}
 
-	return sysfs_emit(buf, "%d\n", !!ret);
+	return sysfs_emit(buf, "%d\n", !!reply);
 }
 
 static ssize_t gamma_correction_store(struct device *dev,
@@ -427,26 +382,51 @@  static struct attribute *omnia_led_controller_attrs[] = {
 };
 ATTRIBUTE_GROUPS(omnia_led_controller);
 
-static int omnia_mcu_get_features(const struct i2c_client *client)
+static int omnia_mcu_get_features(const struct i2c_client *mcu_client)
 {
 	u16 reply;
 	int err;
 
-	err = omnia_cmd_read_raw(client->adapter, OMNIA_MCU_I2C_ADDR,
-				 CMD_GET_STATUS_WORD, &reply, sizeof(reply));
+	err = omnia_cmd_read_u16(mcu_client, CMD_GET_STATUS_WORD, &reply);
 	if (err)
 		return err;
 
 	/* Check whether MCU firmware supports the CMD_GET_FEAUTRES command */
-	if (!(le16_to_cpu(reply) & STS_FEATURES_SUPPORTED))
+	if (!(reply & STS_FEATURES_SUPPORTED))
 		return 0;
 
-	err = omnia_cmd_read_raw(client->adapter, OMNIA_MCU_I2C_ADDR,
-				 CMD_GET_FEATURES, &reply, sizeof(reply));
+	err = omnia_cmd_read_u16(mcu_client, CMD_GET_FEATURES, &reply);
 	if (err)
 		return err;
 
-	return le16_to_cpu(reply);
+	return reply;
+}
+
+static int omnia_match_mcu_client(struct device *dev, void *data)
+{
+	struct i2c_client *client;
+
+	client = i2c_verify_client(dev);
+	if (!client)
+		return 0;
+
+	return client->addr == OMNIA_MCU_I2C_ADDR;
+}
+
+static int omnia_find_mcu_and_get_features(struct device *dev)
+{
+	struct device *mcu_dev;
+	int ret;
+
+	mcu_dev = device_find_child(dev->parent, NULL, omnia_match_mcu_client);
+	if (!mcu_dev)
+		return -ENODEV;
+
+	ret = omnia_mcu_get_features(i2c_verify_client(mcu_dev));
+
+	put_device(mcu_dev);
+
+	return ret;
 }
 
 static int omnia_leds_probe(struct i2c_client *client)
@@ -473,7 +453,7 @@  static int omnia_leds_probe(struct i2c_client *client)
 	leds->client = client;
 	i2c_set_clientdata(client, leds);
 
-	ret = omnia_mcu_get_features(client);
+	ret = omnia_find_mcu_and_get_features(dev);
 	if (ret < 0) {
 		dev_err(dev, "Cannot determine MCU supported features: %d\n",
 			ret);
@@ -510,20 +490,11 @@  static int omnia_leds_probe(struct i2c_client *client)
 
 static void omnia_leds_remove(struct i2c_client *client)
 {
-	u8 buf[5];
-
 	/* put all LEDs into default (HW triggered) mode */
-	omnia_cmd_write_u8(client, CMD_LED_MODE,
-			   CMD_LED_MODE_LED(OMNIA_BOARD_LEDS));
+	omnia_cmd_write_u8(client, CMD_LED_MODE, CMD_LED_MODE_LED(OMNIA_BOARD_LEDS));
 
 	/* set all LEDs color to [255, 255, 255] */
-	buf[0] = CMD_LED_COLOR;
-	buf[1] = OMNIA_BOARD_LEDS;
-	buf[2] = 255;
-	buf[3] = 255;
-	buf[4] = 255;
-
-	i2c_master_send(client, buf, 5);
+	omnia_cmd_set_color(client, OMNIA_BOARD_LEDS, 255, 255, 255);
 }
 
 static const struct of_device_id of_omnia_leds_match[] = {