diff mbox series

leds: pca955x: Allow full control over led name

Message ID 20210325005456.152469-1-joel@jms.id.au
State New
Headers show
Series leds: pca955x: Allow full control over led name | expand

Commit Message

Joel Stanley March 25, 2021, 12:54 a.m. UTC 
The PCA955x driver since it was merged has added a "pca955x:" prefix to
LED names. This meanas platform data (such as the device tree) cannot
fully control the name. Here's what this produces using the example in
the device tree bindings:

 # cat /sys/class/leds/
 pca955x:green:power/
 pca955x:pca9552:white/
 pca955x:pca9552:yellow/
 pca955x:red:power/

Some systems want the ability to configure the full string. To do this,
introduce a build time option that adds the prefix that defaults to
enabled in order to preserve existing behaviour.

Signed-off-by: Joel Stanley <joel@jms.id.au>

---
 drivers/leds/Kconfig        | 11 +++++++++++
 drivers/leds/leds-pca955x.c |  8 +++++++-
 2 files changed, 18 insertions(+), 1 deletion(-)

-- 
2.30.2

Comments

Eddie James March 26, 2021, 9:41 p.m. UTC | #1 
On 3/24/21 7:54 PM, Joel Stanley wrote:
> The PCA955x driver since it was merged has added a "pca955x:" prefix to

> LED names. This meanas platform data (such as the device tree) cannot

> fully control the name. Here's what this produces using the example in

> the device tree bindings:

>

>   # cat /sys/class/leds/

>   pca955x:green:power/

>   pca955x:pca9552:white/

>   pca955x:pca9552:yellow/

>   pca955x:red:power/

>

> Some systems want the ability to configure the full string. To do this,

> introduce a build time option that adds the prefix that defaults to

> enabled in order to preserve existing behaviour.



Reviewed-by: Eddie James <eajames@linux.ibm.com>



>

> Signed-off-by: Joel Stanley <joel@jms.id.au>

> ---

>   drivers/leds/Kconfig        | 11 +++++++++++

>   drivers/leds/leds-pca955x.c |  8 +++++++-

>   2 files changed, 18 insertions(+), 1 deletion(-)

>
vishwanatha subbanna April 14, 2021, 11:06 a.m. UTC | #2 
Pavel / Dan,

Related to this below email, Can this https://github.com/openbmc/linux/blob/dev-5.10/drivers/leds/leds-pca955x.c#L507 naming change ever ?.

If there is any change in https://github.com/openbmc/linux/blob/dev-5.10/drivers/leds/leds-pca955x.c#L507, then my config having led names would break.
So, please suggest me what is the right approach here for defining LEDs. I have all LEDs defined as 


    leds {
        compatible = "gpio-leds”;

I am being asked by Joel Stanley that I should not do that way, but instead, have the LEDs defined as PCA_955X_LED and if I do that, I have this concern with the led names.

Thanks,
Vishwa.

> On 09-Apr-2021, at 4:43 PM, vishwanatha subbanna <vishwa@linux.vnet.ibm.com> wrote:

> 

> 

> Hello Pavel / Dan,

> 

> Wonder if you have any feedback on this patch from Joel ?.. This patch would help me have predictable LED names as I wanted instead of having the prefixes.

> I have some configuration files that depend on these names. So, if the LED name ( prefix ) changes in the future, then it breaks the configuration.

> 

> 

> I see good value in getting this patch in. Please could you help chime in ?

> 

> Thank you,

> !! Vishwa !!

> 

> 

>> On 25-Mar-2021, at 6:24 AM, Joel Stanley <joel@jms.id.au> wrote:

>> 

>> The PCA955x driver since it was merged has added a "pca955x:" prefix to

>> LED names. This meanas platform data (such as the device tree) cannot

>> fully control the name. Here's what this produces using the example in

>> the device tree bindings:

>> 

>> # cat /sys/class/leds/

>> pca955x:green:power/

>> pca955x:pca9552:white/

>> pca955x:pca9552:yellow/

>> pca955x:red:power/

>> 

>> Some systems want the ability to configure the full string. To do this,

>> introduce a build time option that adds the prefix that defaults to

>> enabled in order to preserve existing behaviour.

>> 

>> Signed-off-by: Joel Stanley <joel@jms.id.au>

>> ---

>> drivers/leds/Kconfig        | 11 +++++++++++

>> drivers/leds/leds-pca955x.c |  8 +++++++-

>> 2 files changed, 18 insertions(+), 1 deletion(-)

>> 

>> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig

>> index b6742b4231bf..dcdb107e0967 100644

>> --- a/drivers/leds/Kconfig

>> +++ b/drivers/leds/Kconfig

>> @@ -519,6 +519,17 @@ config LEDS_PCA955X

>> 	  LED driver chips accessed via the I2C bus.  Supported

>> 	  devices include PCA9550, PCA9551, PCA9552, and PCA9553.

>> 

>> +config LEDS_PCA955X_SYSFS_PREFIX

>> +	bool "PCA955x sysfs prefix"

>> +	depends on LEDS_PCA955X

>> +	default y

>> +	help

>> +	  This option adds the string "pca955x:" to the start of the name

>> +	  presented in sysfs.

>> +

>> +	  If you wish to use the label provided by platform data without a

>> +	  prefix, disable this option.

>> +

>> config LEDS_PCA955X_GPIO

>> 	bool "Enable GPIO support for PCA955X"

>> 	depends on LEDS_PCA955X

>> diff --git a/drivers/leds/leds-pca955x.c b/drivers/leds/leds-pca955x.c

>> index 7087ca4592fc..b7742d6226b5 100644

>> --- a/drivers/leds/leds-pca955x.c

>> +++ b/drivers/leds/leds-pca955x.c

>> @@ -61,6 +61,12 @@

>> #define PCA955X_GPIO_HIGH	LED_OFF

>> #define PCA955X_GPIO_LOW	LED_FULL

>> 

>> +#ifdef CONFIG_LEDS_PCA955X_SYSFS_PREFIX

>> +#define SYSFS_NAME "pca955x:%s"

>> +#else

>> +#define SYSFS_NAME "%s"

>> +#endif

>> +

>> enum pca955x_type {

>> 	pca9550,

>> 	pca9551,

>> @@ -504,7 +510,7 @@ static int pca955x_probe(struct i2c_client *client,

>> 					sizeof(pdata->leds[i].name), "%d", i);

>> 

>> 			snprintf(pca955x_led->name,

>> -				sizeof(pca955x_led->name), "pca955x:%s",

>> +				sizeof(pca955x_led->name), SYSFS_NAME,

>> 				pdata->leds[i].name);

>> 

>> 			if (pdata->leds[i].default_trigger)

>> -- 

>> 2.30.2

>> 

>
Pavel Machek April 25, 2021, 9:46 p.m. UTC | #3 
On Thu 2021-03-25 11:24:56, Joel Stanley wrote:
> The PCA955x driver since it was merged has added a "pca955x:" prefix to

> LED names. This meanas platform data (such as the device tree) cannot

> fully control the name. Here's what this produces using the example in

> the device tree bindings:

> 

>  # cat /sys/class/leds/

>  pca955x:green:power/

>  pca955x:pca9552:white/

>  pca955x:pca9552:yellow/

>  pca955x:red:power/

> 

> Some systems want the ability to configure the full string. To do this,

> introduce a build time option that adds the prefix that defaults to

> enabled in order to preserve existing behaviour.


No. Config option is not acceptable for this.

Just delete the prefix and fix the breakage.
								Pavel
								
-- 
http://www.livejournal.com/~pavelmachek
diff mbox series

Patch

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index b6742b4231bf..dcdb107e0967 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -519,6 +519,17 @@  config LEDS_PCA955X
 	  LED driver chips accessed via the I2C bus.  Supported
 	  devices include PCA9550, PCA9551, PCA9552, and PCA9553.
 
+config LEDS_PCA955X_SYSFS_PREFIX
+	bool "PCA955x sysfs prefix"
+	depends on LEDS_PCA955X
+	default y
+	help
+	  This option adds the string "pca955x:" to the start of the name
+	  presented in sysfs.
+
+	  If you wish to use the label provided by platform data without a
+	  prefix, disable this option.
+
 config LEDS_PCA955X_GPIO
 	bool "Enable GPIO support for PCA955X"
 	depends on LEDS_PCA955X
diff --git a/drivers/leds/leds-pca955x.c b/drivers/leds/leds-pca955x.c
index 7087ca4592fc..b7742d6226b5 100644
--- a/drivers/leds/leds-pca955x.c
+++ b/drivers/leds/leds-pca955x.c
@@ -61,6 +61,12 @@ 
 #define PCA955X_GPIO_HIGH	LED_OFF
 #define PCA955X_GPIO_LOW	LED_FULL
 
+#ifdef CONFIG_LEDS_PCA955X_SYSFS_PREFIX
+#define SYSFS_NAME "pca955x:%s"
+#else
+#define SYSFS_NAME "%s"
+#endif
+
 enum pca955x_type {
 	pca9550,
 	pca9551,
@@ -504,7 +510,7 @@  static int pca955x_probe(struct i2c_client *client,
 					sizeof(pdata->leds[i].name), "%d", i);
 
 			snprintf(pca955x_led->name,
-				sizeof(pca955x_led->name), "pca955x:%s",
+				sizeof(pca955x_led->name), SYSFS_NAME,
 				pdata->leds[i].name);
 
 			if (pdata->leds[i].default_trigger)