Skip to contentSkip to conversation

Turris Omnia LED driver changes

PATCH Superseded Marek Behún 6 participants 19 messages Sep 3, 2024 – Sep 4, 2024

Subsystems (4)
Device tree bindings LED Arm/Marvell Kirkwood and Armada 370, 375, 38x, 39x, XP, 3700, 7K/8K, CN9130 SoC support Arm/cz.nic turris support

v2 of 7 Superseded Latest is v7 →

lore
Share & tools
lore

Marek Behún

author maintainer
#0 lore raw

v2 cover Turris Omnia LED driver changes

Hello Lee,

this is v2 of Turris Omnia LED driver changes.
v1 can be found at
https://lore.kernel.org/linux-leds/[email protected]/

This series is for 6.12, but it depends on changes that have been merged to 6.11-rc3. Your for-leds-next branch is based on 6.11-rc1, so it won't apply there.

Changes since v1:
- patch 2 does away also with i2c_master_send() call, in favor of omnia_cmd_write()
- patch 5 was rewritten to not use devm_device_add_group() anymore, since this function was deprecated. Instead, the sysfs_put() call for the "brightness" node is done the first time the IRQ handler is called, instead of driver probe.
- the changes were done with 100 columns limit in mind
- added 3 more "cosmetic" patches

Marek

Marek Behún (11):
turris-omnia-mcu-interface.h: Move command execution function to
global header
leds: turris-omnia: Use command execution functions from the MCU
driver
turris-omnia-mcu-interface.h: Add LED commands related definitions to
global header
leds: turris-omnia: Use global header for MCU command definitions
leds: turris-omnia: Notify sysfs on MCU global LEDs brightness change
platform: cznic: turris-omnia-mcu: Inform about missing LED panel
brightness change interrupt feature
leds: turris-omnia: Inform about missing LED gamma correction feature
in the MCU driver
leds: turris-omnia: Use dev_err_probe() where appropriate
leds: turris-omnia: Use uppercase first letter in all comments
leds: turris-omnia: Use 100 columns in the rest of the code
ARM: dts: turris-omnia: Add global LED brightness change interrupt

 .../dts/marvell/armada-385-turris-omnia.dts   |   1 +
 drivers/leds/leds-turris-omnia.c              | 283 +++++++-----------
 .../platform/cznic/turris-omnia-mcu-base.c    |   3 +
 drivers/platform/cznic/turris-omnia-mcu.h     | 130 --------
 include/linux/turris-omnia-mcu-interface.h    | 148 ++++++++-
 5 files changed, 257 insertions(+), 308 deletions(-)
Signature · 1 line
2.44.2
author maintainer
#1 lore raw

PATCH v2 1/11 turris-omnia-mcu-interface.h: Move command execution function to global header

Move the command execution functions from the turris-omnia-mcu platform driver private header to the global turris-omnia-mcu-interface.h header, so that they can be used by the LED driver.

Signed-off-by Marek Behún <[email protected]>
---
 .../platform/cznic/turris-omnia-mcu-base.c    |   1 +
 drivers/platform/cznic/turris-omnia-mcu.h     | 130 -----------------
 include/linux/turris-omnia-mcu-interface.h    | 136 +++++++++++++++++-
 3 files changed, 136 insertions(+), 131 deletions(-)
drivers/platform/cznic/turris-omnia-mcu-base.c +1 -0
index 58f9afae2867..bb871226e357 100644
@@ -52,6 +52,7 @@ int omnia_cmd_write_read(const struct i2c_client *client,
 
 	return 0;
 }
+EXPORT_SYMBOL_GPL(omnia_cmd_write_read);
 
 static int omnia_get_version_hash(struct omnia_mcu *mcu, bool bootloader,
 				  char version[static OMNIA_FW_VERSION_HEX_LEN])
drivers/platform/cznic/turris-omnia-mcu.h +0 -130

148 lines not on this page · raw message

include/linux/turris-omnia-mcu-interface.h +135 -1

146 lines not on this page · raw message

Signature · 1 line
2.44.2
author maintainer
#2 lore raw

PATCH v2 2/11 leds: turris-omnia: Use command execution functions from the MCU driver

In reply to Marek Behún

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 <[email protected]>
---
 drivers/leds/leds-turris-omnia.c | 110 ++++++++-----------------------
 1 file changed, 29 insertions(+), 81 deletions(-)
drivers/leds/leds-turris-omnia.c +29 -81
index 4cff8c4b020c..0b71848930dd 100644
@@ -2,7 +2,7 @@
 /*
  * CZ.NIC's Turris Omnia LEDs driver
  *
- * 2020, 2023 by Marek Behún <[email protected]>
+ * 2020, 2023, 2024 by Marek Behún <[email protected]>
  */
 
 #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,

@@ -429,24 +384,26 @@ ATTRIBUTE_GROUPS(omnia_led_controller);
 
 static int omnia_mcu_get_features(const struct i2c_client *client)
 {
+	struct i2c_client mcu_client = *client;
 	u16 reply;
 	int err;
 
-	err = omnia_cmd_read_raw(client->adapter, OMNIA_MCU_I2C_ADDR,
-				 CMD_GET_STATUS_WORD, &reply, sizeof(reply));
+	/* We have to read features from different I2C address */
+	mcu_client.addr = OMNIA_MCU_I2C_ADDR;
+
+	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_leds_probe(struct i2c_client *client)

@@ -510,20 +467,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[] = {
Signature · 1 line
2.44.2
author maintainer
#3 lore raw

PATCH v2 3/11 turris-omnia-mcu-interface.h: Add LED commands related definitions to global header

In reply to Marek Behún

Add definitions for contents of the OMNIA_CMD_LED_MODE and OMNIA_CMD_LED_STATE commands to the global turris-omnia-mcu-interface.h header.

Signed-off-by Marek Behún <[email protected]>
---
 include/linux/turris-omnia-mcu-interface.h | 12 ++++++++++++
 1 file changed, 12 insertions(+)
include/linux/turris-omnia-mcu-interface.h +12 -0
index b587549141cb..bac275b0ae42 100644
@@ -239,6 +239,18 @@ enum omnia_int_e {
 	OMNIA_INT_LAN5_LED1		= BIT(31),
 };
 
+enum omnia_cmd_led_mode_e {
+	OMNIA_CMD_LED_MODE_LED_MASK	= GENMASK(3, 0),
+#define OMNIA_CMD_LED_MODE_LED(_l)	FIELD_PREP(OMNIA_CMD_LED_MODE_LED_MASK, _l)
+	OMNIA_CMD_LED_MODE_USER		= BIT(4),
+};
+
+enum omnia_cmd_led_state_e {
+	OMNIA_CMD_LED_STATE_LED_MASK	= GENMASK(3, 0),
+#define OMNIA_CMD_LED_STATE_LED(_l)	FIELD_PREP(OMNIA_CMD_LED_STATE_LED_MASK, _l)
+	OMNIA_CMD_LED_STATE_ON		= BIT(4),
+};
+
 enum omnia_cmd_poweroff_e {
 	OMNIA_CMD_POWER_OFF_POWERON_BUTTON	= BIT(0),
 	OMNIA_CMD_POWER_OFF_MAGIC		= 0xdead,
Signature · 1 line
2.44.2
author maintainer
#4 lore raw

PATCH v2 4/11 leds: turris-omnia: Use global header for MCU command definitions

In reply to Marek Behún

The global turris-omnia-mcu-interface.h header file contains the definitions for MCU commands. Drop the driver-internal definitions and use the global ones.

Signed-off-by Marek Behún <[email protected]>
---
 drivers/leds/leds-turris-omnia.c | 71 +++++++++++---------------------
 1 file changed, 23 insertions(+), 48 deletions(-)
drivers/leds/leds-turris-omnia.c +23 -48
index 0b71848930dd..82cf58fbe946 100644
@@ -16,30 +16,8 @@
 #define OMNIA_BOARD_LEDS	12
 #define OMNIA_LED_NUM_CHANNELS	3
 
-/* MCU controller commands at I2C address 0x2a */
-#define OMNIA_MCU_I2C_ADDR		0x2a
-
-#define CMD_GET_STATUS_WORD		0x01
-#define STS_FEATURES_SUPPORTED		BIT(2)
-
-#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
+/* MCU controller I2C address 0x2a, needed for detecting MCU features */
+#define OMNIA_MCU_I2C_ADDR	0x2a
 
 struct omnia_led {
 	struct led_classdev_mc mc_cdev;

@@ -60,7 +38,7 @@ struct omnia_leds {
 
 static int omnia_cmd_set_color(const struct i2c_client *client, u8 led, u8 r, u8 g, u8 b)
 {
-	u8 buf[5] = { CMD_LED_COLOR, led, r, g, b };
+	u8 buf[5] = { OMNIA_CMD_LED_COLOR, led, r, g, b };
 
 	return omnia_cmd_write(client, buf, sizeof(buf));
 }

@@ -127,12 +105,12 @@ static int omnia_led_brightness_set_blocking(struct led_classdev *cdev,
 	 * is not being blinked by HW.
 	 */
 	if (!err && !led->hwtrig && !brightness != !led->on) {
-		u8 state = CMD_LED_STATE_LED(led->reg);
+		u8 state = OMNIA_CMD_LED_STATE_LED(led->reg);
 
 		if (brightness)
-			state |= CMD_LED_STATE_ON;
+			state |= OMNIA_CMD_LED_STATE_ON;
 
-		err = omnia_cmd_write_u8(leds->client, CMD_LED_STATE, state);
+		err = omnia_cmd_write_u8(leds->client, OMNIA_CMD_LED_STATE, state);
 		if (!err)
 			led->on = !!brightness;
 	}

@@ -167,8 +145,8 @@ static int omnia_hwtrig_activate(struct led_classdev *cdev)
 
 	if (!err) {
 		/* Put the LED into MCU controlled mode */
-		err = omnia_cmd_write_u8(leds->client, CMD_LED_MODE,
-					 CMD_LED_MODE_LED(led->reg));
+		err = omnia_cmd_write_u8(leds->client, OMNIA_CMD_LED_MODE,
+					 OMNIA_CMD_LED_MODE_LED(led->reg));
 		if (!err)
 			led->hwtrig = true;
 	}

@@ -189,9 +167,8 @@ static void omnia_hwtrig_deactivate(struct led_classdev *cdev)
 	led->hwtrig = false;
 
 	/* Put the LED into software mode */
-	err = omnia_cmd_write_u8(leds->client, CMD_LED_MODE,
-				 CMD_LED_MODE_LED(led->reg) |
-				 CMD_LED_MODE_USER);
+	err = omnia_cmd_write_u8(leds->client, OMNIA_CMD_LED_MODE,
+				 OMNIA_CMD_LED_MODE_LED(led->reg) | OMNIA_CMD_LED_MODE_USER);
 
 	mutex_unlock(&leds->lock);
 

@@ -258,9 +235,8 @@ static int omnia_led_register(struct i2c_client *client, struct omnia_led *led,
 	cdev->default_trigger = omnia_hw_trigger.name;
 
 	/* put the LED into software mode */
-	ret = omnia_cmd_write_u8(client, CMD_LED_MODE,
-				 CMD_LED_MODE_LED(led->reg) |
-				 CMD_LED_MODE_USER);
+	ret = omnia_cmd_write_u8(client, OMNIA_CMD_LED_MODE, OMNIA_CMD_LED_MODE_LED(led->reg) |
+							     OMNIA_CMD_LED_MODE_USER);
 	if (ret) {
 		dev_err(dev, "Cannot set LED %pOF to software mode: %i\n", np,
 			ret);

@@ -268,8 +244,7 @@ static int omnia_led_register(struct i2c_client *client, struct omnia_led *led,
 	}
 
 	/* disable the LED */
-	ret = omnia_cmd_write_u8(client, CMD_LED_STATE,
-				 CMD_LED_STATE_LED(led->reg));
+	ret = omnia_cmd_write_u8(client, OMNIA_CMD_LED_STATE, OMNIA_CMD_LED_STATE_LED(led->reg));
 	if (ret) {
 		dev_err(dev, "Cannot set LED %pOF brightness: %i\n", np, ret);
 		return ret;

@@ -311,7 +286,7 @@ static ssize_t brightness_show(struct device *dev, struct device_attribute *a,
 	u8 reply;
 	int err;
 
-	err = omnia_cmd_read_u8(client, CMD_LED_GET_BRIGHTNESS, &reply);
+	err = omnia_cmd_read_u8(client, OMNIA_CMD_GET_BRIGHTNESS, &reply);
 	if (err < 0)
 		return err;
 

@@ -331,7 +306,7 @@ static ssize_t brightness_store(struct device *dev, struct device_attribute *a,
 	if (brightness > 100)
 		return -EINVAL;
 
-	err = omnia_cmd_write_u8(client, CMD_LED_SET_BRIGHTNESS, brightness);
+	err = omnia_cmd_write_u8(client, OMNIA_CMD_SET_BRIGHTNESS, brightness);
 
 	return err ?: count;
 }

@@ -346,7 +321,7 @@ static ssize_t gamma_correction_show(struct device *dev,
 	int err;
 
 	if (leds->has_gamma_correction) {
-		err = omnia_cmd_read_u8(client, CMD_GET_GAMMA_CORRECTION, &reply);
+		err = omnia_cmd_read_u8(client, OMNIA_CMD_GET_GAMMA_CORRECTION, &reply);
 		if (err < 0)
 			return err;
 	}

@@ -369,7 +344,7 @@ static ssize_t gamma_correction_store(struct device *dev,
 	if (kstrtobool(buf, &val) < 0)
 		return -EINVAL;
 
-	err = omnia_cmd_write_u8(client, CMD_SET_GAMMA_CORRECTION, val);
+	err = omnia_cmd_write_u8(client, OMNIA_CMD_SET_GAMMA_CORRECTION, val);
 
 	return err ?: count;
 }

@@ -391,15 +366,15 @@ static int omnia_mcu_get_features(const struct i2c_client *client)
 	/* We have to read features from different I2C address */
 	mcu_client.addr = OMNIA_MCU_I2C_ADDR;
 
-	err = omnia_cmd_read_u16(&mcu_client, CMD_GET_STATUS_WORD, &reply);
+	err = omnia_cmd_read_u16(&mcu_client, OMNIA_CMD_GET_STATUS_WORD, &reply);
 	if (err)
 		return err;
 
-	/* Check whether MCU firmware supports the CMD_GET_FEAUTRES command */
-	if (!(reply & STS_FEATURES_SUPPORTED))
+	/* Check whether MCU firmware supports the OMNIA_CMD_GET_FEAUTRES command */
+	if (!(reply & OMNIA_STS_FEATURES_SUPPORTED))
 		return 0;
 
-	err = omnia_cmd_read_u16(&mcu_client, CMD_GET_FEATURES, &reply);
+	err = omnia_cmd_read_u16(&mcu_client, OMNIA_CMD_GET_FEATURES, &reply);
 	if (err)
 		return err;
 

@@ -437,7 +412,7 @@ static int omnia_leds_probe(struct i2c_client *client)
 		return ret;
 	}
 
-	leds->has_gamma_correction = ret & FEAT_LED_GAMMA_CORRECTION;
+	leds->has_gamma_correction = ret & OMNIA_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");

@@ -468,7 +443,7 @@ static int omnia_leds_probe(struct i2c_client *client)
 static void omnia_leds_remove(struct i2c_client *client)
 {
 	/* 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, OMNIA_CMD_LED_MODE, OMNIA_CMD_LED_MODE_LED(OMNIA_BOARD_LEDS));
 
 	/* set all LEDs color to [255, 255, 255] */
 	omnia_cmd_set_color(client, OMNIA_BOARD_LEDS, 255, 255, 255);
Signature · 1 line
2.44.2
author maintainer
#5 lore raw

PATCH v2 5/11 leds: turris-omnia: Notify sysfs on MCU global LEDs brightness change

In reply to Marek Behún

Recall that on Turris Omnia, the LED controller has a global brightness property, which allows the user to make the front LED panel dimmer.

There is also a button on the front panel, which by default is configured so that pressing it changes the global brightness to a lower value (unless it is at 0%, in which case pressing the button changes the global brightness to 100%).

Newer versions of the MCU firmware support informing the SOC that the brightness was changed by button press event via an interrupt.

Now that we have the turris-omnia-mcu driver, which adds support for MCU interrupts, add the ability to inform the userspace (via a sysfs notification) that the global brightness was changed.

Signed-off-by Marek Behún <[email protected]>
---
 drivers/leds/leds-turris-omnia.c | 37 ++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)
drivers/leds/leds-turris-omnia.c +37 -0
index 82cf58fbe946..51b13d3d7476 100644
@@ -33,6 +33,7 @@ struct omnia_leds {
 	struct i2c_client *client;
 	struct mutex lock;
 	bool has_gamma_correction;
+	struct kernfs_node *brightness_kn;
 	struct omnia_led leds[];
 };
 

@@ -357,6 +358,21 @@ static struct attribute *omnia_led_controller_attrs[] = {
 };
 ATTRIBUTE_GROUPS(omnia_led_controller);
 
+static irqreturn_t omnia_brightness_changed_handler(int irq, void *dev_id)
+{
+	struct omnia_leds *leds = dev_id;
+
+	if (unlikely(!leds->brightness_kn)) {
+		leds->brightness_kn = sysfs_get_dirent(leds->client->dev.kobj.sd, "brightness");
+		if (!leds->brightness_kn)
+			return IRQ_NONE;
+	}
+
+	sysfs_notify_dirent(leds->brightness_kn);
+
+	return IRQ_HANDLED;
+}
+
 static int omnia_mcu_get_features(const struct i2c_client *client)
 {
 	struct i2c_client mcu_client = *client;

@@ -420,6 +436,14 @@ static int omnia_leds_probe(struct i2c_client *client)
 			 "Consider upgrading MCU firmware with the omnia-mcutool utility.\n");
 	}
 
+	if (ret & OMNIA_FEAT_BRIGHTNESS_INT) {
+		ret = devm_request_any_context_irq(dev, client->irq,
+						   omnia_brightness_changed_handler, IRQF_ONESHOT,
+						   "leds-turris-omnia", leds);
+		if (ret <= 0)
+			return dev_err_probe(dev, ret ?: -ENXIO, "Cannot request brightness IRQ\n");
+	}
+
 	mutex_init(&leds->lock);
 
 	ret = devm_led_trigger_register(dev, &omnia_hw_trigger);

@@ -442,6 +466,19 @@ static int omnia_leds_probe(struct i2c_client *client)
 
 static void omnia_leds_remove(struct i2c_client *client)
 {
+	struct omnia_leds *leds = i2c_get_clientdata(client);
+
+	/*
+	 * We need to free the brightness IRQ here, before putting away the brightness sysfs node.
+	 * Otherwise devres would free the interrupt only after the sysfs node is removed, and if
+	 * an interrupt occurred between those two events, it would use a removed sysfs node.
+	 */
+	devm_free_irq(&client->dev, client->irq, leds);
+
+	/* Now put away the sysfs node we got the first time the interrupt handler was called */
+	if (leds->brightness_kn)
+		sysfs_put(leds->brightness_kn);
+
 	/* put all LEDs into default (HW triggered) mode */
 	omnia_cmd_write_u8(client, OMNIA_CMD_LED_MODE, OMNIA_CMD_LED_MODE_LED(OMNIA_BOARD_LEDS));
Signature · 1 line
2.44.2
author maintainer
#6 lore raw

PATCH v2 6/11 platform: cznic: turris-omnia-mcu: Inform about missing LED panel brightness change interrupt feature

In reply to Marek Behún

When reading MCU firmware features, check also for the LED panel brightness change interrupt feature, and suggest upgrading the firmware if it is missing.

Signed-off-by Marek Behún <[email protected]>
---
 drivers/platform/cznic/turris-omnia-mcu-base.c | 1 +
 1 file changed, 1 insertion(+)
drivers/platform/cznic/turris-omnia-mcu-base.c +1 -0
index bb871226e357..3bb4a3cfdb29 100644
@@ -258,6 +258,7 @@ static int omnia_mcu_read_features(struct omnia_mcu *mcu)
 		_DEF_FEAT(NEW_INT_API,		"new interrupt API"),
 		_DEF_FEAT(POWEROFF_WAKEUP,	"poweroff and wakeup"),
 		_DEF_FEAT(TRNG,			"true random number generator"),
+		_DEF_FEAT(BRIGHTNESS_INT,	"LED panel brightness change interrupt"),
 #undef _DEF_FEAT
 	};
 	struct i2c_client *client = mcu->client;
Signature · 1 line
2.44.2
author maintainer
#7 lore raw

PATCH v2 7/11 leds: turris-omnia: Inform about missing LED gamma correction feature in the MCU driver

In reply to Marek Behún

If the LED gamma correction feature is missing in the MCU firmware, inform about this in the MCU firmware probe function instead of LED driver probe function, so that all the feature checks are in one place.

Signed-off-by Marek Behún <[email protected]>
---
 drivers/leds/leds-turris-omnia.c               | 6 ------
 drivers/platform/cznic/turris-omnia-mcu-base.c | 1 +
 2 files changed, 1 insertion(+), 6 deletions(-)
drivers/leds/leds-turris-omnia.c +0 -6
index 51b13d3d7476..8d3bddc90fe0 100644
@@ -429,12 +429,6 @@ static int omnia_leds_probe(struct i2c_client *client)
 	}
 
 	leds->has_gamma_correction = ret & OMNIA_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");
-		dev_info(dev,
-			 "Consider upgrading MCU firmware with the omnia-mcutool utility.\n");
-	}
 
 	if (ret & OMNIA_FEAT_BRIGHTNESS_INT) {
 		ret = devm_request_any_context_irq(dev, client->irq,
drivers/platform/cznic/turris-omnia-mcu-base.c +1 -0
index 3bb4a3cfdb29..770e680b96f9 100644
@@ -259,6 +259,7 @@ static int omnia_mcu_read_features(struct omnia_mcu *mcu)
 		_DEF_FEAT(POWEROFF_WAKEUP,	"poweroff and wakeup"),
 		_DEF_FEAT(TRNG,			"true random number generator"),
 		_DEF_FEAT(BRIGHTNESS_INT,	"LED panel brightness change interrupt"),
+		_DEF_FEAT(LED_GAMMA_CORRECTION,	"LED gamma correction"),
 #undef _DEF_FEAT
 	};
 	struct i2c_client *client = mcu->client;
Signature · 1 line
2.44.2
author maintainer
#8 lore raw

PATCH v2 8/11 leds: turris-omnia: Use dev_err_probe() where appropriate

In reply to Marek Behún

Use dev_err_probe() instead of dev_err() + separate return where appropriate.

Signed-off-by Marek Behún <[email protected]>
---
 drivers/leds/leds-turris-omnia.c | 50 ++++++++++----------------------
 1 file changed, 16 insertions(+), 34 deletions(-)
drivers/leds/leds-turris-omnia.c +16 -34
index 8d3bddc90fe0..857dba811d5e 100644
@@ -238,33 +238,23 @@ static int omnia_led_register(struct i2c_client *client, struct omnia_led *led,
 	/* put the LED into software mode */
 	ret = omnia_cmd_write_u8(client, OMNIA_CMD_LED_MODE, OMNIA_CMD_LED_MODE_LED(led->reg) |
 							     OMNIA_CMD_LED_MODE_USER);
-	if (ret) {
-		dev_err(dev, "Cannot set LED %pOF to software mode: %i\n", np,
-			ret);
-		return ret;
-	}
+	if (ret)
+		return dev_err_probe(dev, ret, "Cannot set LED %pOF to software mode\n", np);
 
 	/* disable the LED */
 	ret = omnia_cmd_write_u8(client, OMNIA_CMD_LED_STATE, OMNIA_CMD_LED_STATE_LED(led->reg));
-	if (ret) {
-		dev_err(dev, "Cannot set LED %pOF brightness: %i\n", np, ret);
-		return ret;
-	}
+	if (ret)
+		return dev_err_probe(dev, ret, "Cannot set LED %pOF brightness\n", np);
 
 	/* 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;
-	}
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "Cannot set LED %pOF initial color\n", np);
 
 	ret = devm_led_classdev_multicolor_register_ext(dev, &led->mc_cdev,
 							&init_data);
-	if (ret < 0) {
-		dev_err(dev, "Cannot register LED %pOF: %i\n", np, ret);
-		return ret;
-	}
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "Cannot register LED %pOF\n", np);
 
 	return 1;
 }

@@ -406,13 +396,10 @@ static int omnia_leds_probe(struct i2c_client *client)
 	int ret, count;
 
 	count = of_get_available_child_count(np);
-	if (!count) {
-		dev_err(dev, "LEDs are not defined in device tree!\n");
-		return -ENODEV;
-	} else if (count > OMNIA_BOARD_LEDS) {
-		dev_err(dev, "Too many LEDs defined in device tree!\n");
-		return -EINVAL;
-	}
+	if (!count)
+		return dev_err_probe(dev, -ENODEV, "LEDs are not defined in device tree!\n");
+	else if (count > OMNIA_BOARD_LEDS)
+		return dev_err_probe(dev, -EINVAL, "Too many LEDs defined in device tree!\n");
 
 	leds = devm_kzalloc(dev, struct_size(leds, leds, count), GFP_KERNEL);
 	if (!leds)

@@ -422,11 +409,8 @@ static int omnia_leds_probe(struct i2c_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;
-	}
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "Cannot determine MCU supported features\n");
 
 	leds->has_gamma_correction = ret & OMNIA_FEAT_LED_GAMMA_CORRECTION;
 

@@ -441,10 +425,8 @@ 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;
-	}
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "Cannot register private LED trigger\n");
 
 	led = &leds->leds[0];
 	for_each_available_child_of_node_scoped(np, child) {
Signature · 1 line
2.44.2
author maintainer
#13 lore raw

PATCH v2 9/11 leds: turris-omnia: Use uppercase first letter in all comments

In reply to Marek Behún

Change first letter of 4 more comments to uppercase to make the driver comments uniform.

Signed-off-by Marek Behún <[email protected]>
---
 drivers/leds/leds-turris-omnia.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
drivers/leds/leds-turris-omnia.c +4 -4
index 857dba811d5e..58c3f3afbca5 100644
@@ -235,13 +235,13 @@ static int omnia_led_register(struct i2c_client *client, struct omnia_led *led,
 	 */
 	cdev->default_trigger = omnia_hw_trigger.name;
 
-	/* put the LED into software mode */
+	/* Put the LED into software mode */
 	ret = omnia_cmd_write_u8(client, OMNIA_CMD_LED_MODE, OMNIA_CMD_LED_MODE_LED(led->reg) |
 							     OMNIA_CMD_LED_MODE_USER);
 	if (ret)
 		return dev_err_probe(dev, ret, "Cannot set LED %pOF to software mode\n", np);
 
-	/* disable the LED */
+	/* Disable the LED */
 	ret = omnia_cmd_write_u8(client, OMNIA_CMD_LED_STATE, OMNIA_CMD_LED_STATE_LED(led->reg));
 	if (ret)
 		return dev_err_probe(dev, ret, "Cannot set LED %pOF brightness\n", np);

@@ -455,10 +455,10 @@ static void omnia_leds_remove(struct i2c_client *client)
 	if (leds->brightness_kn)
 		sysfs_put(leds->brightness_kn);
 
-	/* put all LEDs into default (HW triggered) mode */
+	/* Put all LEDs into default (HW triggered) mode */
 	omnia_cmd_write_u8(client, OMNIA_CMD_LED_MODE, OMNIA_CMD_LED_MODE_LED(OMNIA_BOARD_LEDS));
 
-	/* set all LEDs color to [255, 255, 255] */
+	/* Set all LEDs color to [255, 255, 255] */
 	omnia_cmd_set_color(client, OMNIA_BOARD_LEDS, 255, 255, 255);
 }
Signature · 1 line
2.44.2
author maintainer
#14 lore raw

PATCH v2 10/11 leds: turris-omnia: Use 100 columns in the rest of the code

In reply to Marek Behún

There are 7 more places in the code where the 100 column limit can be used to make the code more uniform. Do it.

Signed-off-by Marek Behún <[email protected]>
---
 drivers/leds/leds-turris-omnia.c | 21 +++++++--------------
 1 file changed, 7 insertions(+), 14 deletions(-)
drivers/leds/leds-turris-omnia.c +7 -14
index 58c3f3afbca5..4ab76b5b5e68 100644
@@ -44,8 +44,7 @@ static int omnia_cmd_set_color(const struct i2c_client *client, u8 led, u8 r, u8
 	return omnia_cmd_write(client, buf, sizeof(buf));
 }
 
-static int omnia_led_send_color_cmd(const struct i2c_client *client,
-				    struct omnia_led *led)
+static int omnia_led_send_color_cmd(const struct i2c_client *client, struct omnia_led *led)
 {
 	int ret;
 

@@ -90,8 +89,7 @@ static int omnia_led_brightness_set_blocking(struct led_classdev *cdev,
 	 * implement the division instruction).
 	 */
 	if (brightness || led->hwtrig) {
-		led_mc_calc_color_components(mc_cdev, brightness ?:
-						      cdev->max_brightness);
+		led_mc_calc_color_components(mc_cdev, brightness ?: cdev->max_brightness);
 
 		/*
 		 * Send color command only if brightness is non-zero and the RGB

@@ -174,8 +172,7 @@ static void omnia_hwtrig_deactivate(struct led_classdev *cdev)
 	mutex_unlock(&leds->lock);
 
 	if (err)
-		dev_err(cdev->dev, "Cannot put LED to software mode: %i\n",
-			err);
+		dev_err(cdev->dev, "Cannot put LED to software mode: %i\n", err);
 }
 
 static struct led_trigger omnia_hw_trigger = {

@@ -251,8 +248,7 @@ static int omnia_led_register(struct i2c_client *client, struct omnia_led *led,
 	if (ret < 0)
 		return dev_err_probe(dev, ret, "Cannot set LED %pOF initial color\n", np);
 
-	ret = devm_led_classdev_multicolor_register_ext(dev, &led->mc_cdev,
-							&init_data);
+	ret = devm_led_classdev_multicolor_register_ext(dev, &led->mc_cdev, &init_data);
 	if (ret < 0)
 		return dev_err_probe(dev, ret, "Cannot register LED %pOF\n", np);
 

@@ -270,8 +266,7 @@ static int omnia_led_register(struct i2c_client *client, struct omnia_led *led,
  * file lives in the device directory of the LED controller, not an individual
  * LED, so it should not confuse users.
  */
-static ssize_t brightness_show(struct device *dev, struct device_attribute *a,
-			       char *buf)
+static ssize_t brightness_show(struct device *dev, struct device_attribute *a, char *buf)
 {
 	struct i2c_client *client = to_i2c_client(dev);
 	u8 reply;

@@ -303,8 +298,7 @@ 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)
+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);

@@ -320,8 +314,7 @@ static ssize_t gamma_correction_show(struct device *dev,
 	return sysfs_emit(buf, "%d\n", !!reply);
 }
 
-static ssize_t gamma_correction_store(struct device *dev,
-				      struct device_attribute *a,
+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);
Signature · 1 line
2.44.2
author maintainer
#17 lore raw

PATCH v2 11/11 ARM: dts: turris-omnia: Add global LED brightness change interrupt

In reply to Marek Behún

When global LED brightness is changed by pressing the front button on Turris Omnia, the MCU can produce an interrupt to the CPU. Add the description of this interrupt to the LED controller node.

Signed-off-by Marek Behún <[email protected]>
---
 arch/arm/boot/dts/marvell/armada-385-turris-omnia.dts | 1 +
 1 file changed, 1 insertion(+)
arch/arm/boot/dts/marvell/armada-385-turris-omnia.dts +1 -0
index 43202890c959..83fe00abd652 100644
@@ -251,6 +251,7 @@ mcu: system-controller@2a {
 			led-controller@2b {
 				compatible = "cznic,turris-omnia-leds";
 				reg = <0x2b>;
+				interrupts-extended = <&mcu 11 IRQ_TYPE_NONE>;
 				#address-cells = <1>;
 				#size-cells = <0>;
 				status = "okay";
Signature · 1 line
2.44.2
#9 lore raw

Re: [PATCH leds v2 08/11] leds: turris-omnia: Use dev_err_probe() where appropriate

In reply to Marek Behún
Quoted 3 lines · Marek Behún

Use dev_err_probe() instead of dev_err() + separate return where appropriate.

...

drivers/leds/leds-turris-omnia.c static int omnia_led_register(struct i2c_client *client, struct omnia_led *led, · line 242

Quoted 2 lines
drivers/leds/leds-turris-omnia.c +2 -0
+       if (ret)
+               return dev_err_probe(dev, ret, "Cannot set LED %pOF to software mode\n", np);

Side note: Not sure how np is being used besides the messaging. If it's only for the messaging, I would rather see %pfw and fwnode based approach.

...

drivers/leds/leds-turris-omnia.c static int omnia_leds_probe(struct i2c_client *client) · line 399

Quoted 9 lines
        count = of_get_available_child_count(np);
drivers/leds/leds-turris-omnia.c +1 -7
-       if (!count) {
-               dev_err(dev, "LEDs are not defined in device tree!\n");
-               return -ENODEV;
-       } else if (count > OMNIA_BOARD_LEDS) {
-               dev_err(dev, "Too many LEDs defined in device tree!\n");
-               return -EINVAL;
-       }
+       if (!count)

Dunno if 'if (count == 0)' would look better from the readability perspective.

drivers/leds/leds-turris-omnia.c static int omnia_leds_probe(struct i2c_client *client) · line 401

Quoted 2 lines
drivers/leds/leds-turris-omnia.c +2 -0
+               return dev_err_probe(dev, -ENODEV, "LEDs are not defined in device tree!\n");
+       else if (count > OMNIA_BOARD_LEDS)

Even in the original code the 'else' is redundant.

Quoted 1 line

+ return dev_err_probe(dev, -EINVAL, "Too many LEDs defined in device tree!\n");

Signature · 2 lines
With Best Regards,
Andy Shevchenko
#15 lore raw

Re: [PATCH leds v2 10/11] leds: turris-omnia: Use 100 columns in the rest of the code

In reply to Marek Behún
Quoted 4 lines · Marek Behún

There are 7 more places in the code where the 100 column limit can be used to make the code more uniform. Do it.

Signed-off-by Marek Behún <[email protected]>

I feel like it's ok to use either 80-character or 100-character line lengths in a file, but I don't see much value in changing from one to another here, maybe just drop this patch.

Arnd

maintainer
#18 lore raw
In reply to Marek Behún
Quoted 47 lines · Marek Behún

Hello Lee,

this is v2 of Turris Omnia LED driver changes.
v1 can be found at
https://lore.kernel.org/linux-leds/[email protected]/

This series is for 6.12, but it depends on changes that have been merged to 6.11-rc3. Your for-leds-next branch is based on 6.11-rc1, so it won't apply there.

Changes since v1:
- patch 2 does away also with i2c_master_send() call, in favor of omnia_cmd_write()
- patch 5 was rewritten to not use devm_device_add_group() anymore, since this function was deprecated. Instead, the sysfs_put() call for the "brightness" node is done the first time the IRQ handler is called, instead of driver probe.
- the changes were done with 100 columns limit in mind
- added 3 more "cosmetic" patches

Marek

Marek Behún (11):
turris-omnia-mcu-interface.h: Move command execution function to
global header
leds: turris-omnia: Use command execution functions from the MCU
driver
turris-omnia-mcu-interface.h: Add LED commands related definitions to
global header
leds: turris-omnia: Use global header for MCU command definitions
leds: turris-omnia: Notify sysfs on MCU global LEDs brightness change
platform: cznic: turris-omnia-mcu: Inform about missing LED panel
brightness change interrupt feature
leds: turris-omnia: Inform about missing LED gamma correction feature
in the MCU driver
leds: turris-omnia: Use dev_err_probe() where appropriate
leds: turris-omnia: Use uppercase first letter in all comments
leds: turris-omnia: Use 100 columns in the rest of the code
ARM: dts: turris-omnia: Add global LED brightness change interrupt

 .../dts/marvell/armada-385-turris-omnia.dts   |   1 +
 drivers/leds/leds-turris-omnia.c              | 283 +++++++-----------
 .../platform/cznic/turris-omnia-mcu-base.c    |   3 +
 drivers/platform/cznic/turris-omnia-mcu.h     | 130 --------
 include/linux/turris-omnia-mcu-interface.h    | 148 ++++++++-
 5 files changed, 257 insertions(+), 308 deletions(-)
Quoted 4 lines

--
2.44.2

My bot found new DTB warnings on the .dts files added or changed in this series.

Some warnings may be from an existing SoC .dtsi. Or perhaps the warnings are fixed by another series. Ultimately, it is up to the platform maintainer whether these warnings are acceptable or not. No need to reply unless the platform maintainer has comments.

If you already ran DT checks and didn't see these error(s), then make sure dt-schema is up to date:

  pip3 install dtschema --upgrade

New warnings running 'make CHECK_DTBS=y marvell/armada-385-turris-omnia.dtb' for [email protected]:

arch/arm/boot/dts/marvell/armada-385-turris-omnia.dtb: led-controller@2b: 'interrupts-extended' does not match any of the regexes: '^multi-led@[0-9a-b]$', 'pinctrl-[0-9]+'
from schema $id: http://devicetree.org/schemas/leds/cznic,turris-omnia-leds.yaml#

maintainer
#10 lore raw

Re: [PATCH leds v2 08/11] leds: turris-omnia: Use dev_err_probe() where appropriate

In reply to Andy Shevchenko
Quoted 5 lines · Andy Shevchenko
Quoted 3 lines · Marek Behún

Use dev_err_probe() instead of dev_err() + separate return where appropriate.

Quoted 8 lines

...

drivers/leds/leds-turris-omnia.c static int omnia_led_register(struct i2c_client *client, struct omnia_led *led, · line 242

Quoted 2 lines
drivers/leds/leds-turris-omnia.c +2 -0
+       if (ret)
+               return dev_err_probe(dev, ret, "Cannot set LED %pOF to software mode\n", np);

Side note: Not sure how np is being used besides the messaging. If it's only for the messaging, I would rather see %pfw and fwnode based approach.

This board has a number of Ethernet switches described in DT. Nobody takes ACPI seriously for any sort of complex networking. So using fwnode is probably pointless, this board will probably never be used with anything other than DT.

Andrew

#11 lore raw

Re: [PATCH leds v2 08/11] leds: turris-omnia: Use dev_err_probe() where appropriate

Quoted 2 lines · Andrew Lunn
Quoted 1 line · Andy Shevchenko

On Tue, Sep 3, 2024 at 1:20 PM Marek Behún <[email protected]> wrote:

...

Quoted 11 lines
Quoted 6 lines

drivers/leds/leds-turris-omnia.c static int omnia_led_register(struct i2c_client *client, struct omnia_led *led, · line 242

Quoted 2 lines
drivers/leds/leds-turris-omnia.c +2 -0
+       if (ret)
+               return dev_err_probe(dev, ret, "Cannot set LED %pOF to software mode\n", np);

Side note: Not sure how np is being used besides the messaging. If it's only for the messaging, I would rather see %pfw and fwnode based approach.

This board has a number of Ethernet switches described in DT. Nobody takes ACPI seriously for any sort of complex networking. So using fwnode is probably pointless, this board will probably never be used with anything other than DT.

Okay, fair enough!

Signature · 2 lines
With Best Regards,
Andy Shevchenko
#16 lore raw

Re: [PATCH leds v2 10/11] leds: turris-omnia: Use 100 columns in the rest of the code

In reply to Arnd Bergmann
Quoted 9 lines · Arnd Bergmann
Quoted 4 lines · Marek Behún

There are 7 more places in the code where the 100 column limit can be used to make the code more uniform. Do it.

Signed-off-by Marek Behún <[email protected]>

I feel like it's ok to use either 80-character or 100-character line lengths in a file, but I don't see much value in changing from one to another here, maybe just drop this patch.

By either-or you mean exclusive or inclusive OR? The driver already has some lines at 80 and other at 100, so I just wanted to convert to 100 completely.

But I don't mind dropping it.

author maintainer
#12 lore raw

Re: [PATCH leds v2 08/11] leds: turris-omnia: Use dev_err_probe() where appropriate

In reply to Andrew Lunn
Quoted 11 lines · Andrew Lunn
Quoted 10 lines · Andy Shevchenko
Quoted 3 lines · Marek Behún

Use dev_err_probe() instead of dev_err() + separate return where appropriate.

...

Quoted 2 lines
drivers/leds/leds-turris-omnia.c +2 -0
+       if (ret)
+               return dev_err_probe(dev, ret, "Cannot set LED %pOF to software mode\n", np);
Quoted 8 lines
Quoted 3 lines

Side note: Not sure how np is being used besides the messaging. If it's only for the messaging, I would rather see %pfw and fwnode based approach.

This board has a number of Ethernet switches described in DT. Nobody takes ACPI seriously for any sort of complex networking. So using fwnode is probably pointless, this board will probably never be used with anything other than DT.

Although this is true, my opinion is that converting drivers to fwnode API would still have some theoretical merit. For example using the device_property_*() functions, where possible, seems nicer semantically.

I think I tried it once, or at least asked Pavel if I should, and he turned me down, so I abandoned the effort.

But the patch would be a very simple one.

Marek