| Message ID | 20200909162552.11032-3-marek.behun@nic.cz |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | PLEASE REVIEW: Add support for LEDs on Marvell PHYs | expand |
Hi, On 9/9/20 9:25 AM, Marek Behún wrote: > Many an ethernet PHY (and other chips) supports various HW control modes > for LEDs connected directly to them. > > This patch adds a generic API for registering such LEDs when described > in device tree. This API also exposes generic way to select between > these hardware control modes. > > This API registers a new private LED trigger called dev-hw-mode. When > this trigger is enabled for a LED, the various HW control modes which > are supported by the device for given LED can be get/set via hw_mode > sysfs file. > > Signed-off-by: Marek Behún <marek.behun@nic.cz> > --- > .../sysfs-class-led-trigger-dev-hw-mode | 8 + > drivers/leds/Kconfig | 10 + > drivers/leds/Makefile | 1 + > drivers/leds/leds-hw-controlled.c | 227 ++++++++++++++++++ > include/linux/leds-hw-controlled.h | 74 ++++++ > 5 files changed, 320 insertions(+) > create mode 100644 Documentation/ABI/testing/sysfs-class-led-trigger-dev-hw-mode > create mode 100644 drivers/leds/leds-hw-controlled.c > create mode 100644 include/linux/leds-hw-controlled.h > > diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig > index 1c181df24eae4..5e47ab21aafb4 100644 > --- a/drivers/leds/Kconfig > +++ b/drivers/leds/Kconfig > @@ -49,6 +49,16 @@ config LEDS_BRIGHTNESS_HW_CHANGED > > See Documentation/ABI/testing/sysfs-class-led for details. > > +config LEDS_HW_CONTROLLED > + bool "API for LEDs that can be controlled by hardware" > + depends on LEDS_CLASS depends on OF || COMPILE_TEST ? > + select LEDS_TRIGGERS > + help > + This option enables support for a generic API via which other drivers > + can register LEDs that can be put into hardware controlled mode, eg. e.g. > + a LED connected to an ethernet PHY can be configured to blink on an LED > + network activity. > + > comment "LED drivers" > > config LEDS_88PM860X > diff --git a/include/linux/leds-hw-controlled.h b/include/linux/leds-hw-controlled.h > new file mode 100644 > index 0000000000000..2c9b8a06def18 > --- /dev/null > +++ b/include/linux/leds-hw-controlled.h > @@ -0,0 +1,74 @@ > +/* SPDX-License-Identifier: GPL-2.0+ */ > +/* > + * Generic API for LEDs that can be controlled by hardware (eg. by an ethernet PHY chip) > + * > + * Copyright (C) 2020 Marek Behun <marek.behun@nic.cz> > + */ > +#ifndef _LINUX_LEDS_HW_CONTROLLED_H_ > +#define _LINUX_LEDS_HW_CONTROLLED_H_ > + > +#include <linux/kernel.h> > +#include <linux/leds.h> > + > +struct hw_controlled_led { > + struct led_classdev cdev; > + const struct hw_controlled_led_ops *ops; > + struct mutex lock; > + > + /* these members are filled in by OF if OF is enabled */ > + int addr; > + bool active_low; > + bool tristate; > + > + /* also filled in by OF, but changed by led_set_hw_mode operation */ > + const char *hw_mode; > + > + void *priv; > +}; > +#define led_cdev_to_hw_controlled_led(l) container_of(l, struct hw_controlled_led, cdev) > + > +/* struct hw_controlled_led_ops: Operations on LEDs that can be controlled by HW > + * > + * All the following operations must be implemented: > + * @led_init: Should initialize the LED from OF data (and sanity check whether they are correct). > + * This should also change led->cdev.max_brightness, if the value differs from default, > + * which is 1. > + * @led_brightness_set: Sets brightness. > + * @led_iter_hw_mode: Iterates available HW control mode names for this LED. > + * @led_set_hw_mode: Sets HW control mode to value specified by given name. > + * @led_get_hw_mode: Returns current HW control mode name. > + */ Convert that struct info to kernel-doc? > +struct hw_controlled_led_ops { > + int (*led_init)(struct device *dev, struct hw_controlled_led *led); > + int (*led_brightness_set)(struct device *dev, struct hw_controlled_led *led, > + enum led_brightness brightness); > + const char *(*led_iter_hw_mode)(struct device *dev, struct hw_controlled_led *led, > + void **iter); > + int (*led_set_hw_mode)(struct device *dev, struct hw_controlled_led *led, > + const char *mode); > + const char *(*led_get_hw_mode)(struct device *dev, struct hw_controlled_led *led); > +}; > + > + > +#endif /* _LINUX_LEDS_HW_CONTROLLED_H_ */ thanks.
On Wed, 9 Sep 2020 11:20:00 -0700 Randy Dunlap <rdunlap@infradead.org> wrote: > Hi, > > On 9/9/20 9:25 AM, Marek Behún wrote: > > Many an ethernet PHY (and other chips) supports various HW control > > modes for LEDs connected directly to them. > > > > This patch adds a generic API for registering such LEDs when > > described in device tree. This API also exposes generic way to > > select between these hardware control modes. > > > > This API registers a new private LED trigger called dev-hw-mode. > > When this trigger is enabled for a LED, the various HW control > > modes which are supported by the device for given LED can be > > get/set via hw_mode sysfs file. > > > > Signed-off-by: Marek Behún <marek.behun@nic.cz> > > --- > > .../sysfs-class-led-trigger-dev-hw-mode | 8 + > > drivers/leds/Kconfig | 10 + > > drivers/leds/Makefile | 1 + > > drivers/leds/leds-hw-controlled.c | 227 > > ++++++++++++++++++ include/linux/leds-hw-controlled.h | > > 74 ++++++ 5 files changed, 320 insertions(+) > > create mode 100644 > > Documentation/ABI/testing/sysfs-class-led-trigger-dev-hw-mode > > create mode 100644 drivers/leds/leds-hw-controlled.c create mode > > 100644 include/linux/leds-hw-controlled.h > > > diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig > > index 1c181df24eae4..5e47ab21aafb4 100644 > > --- a/drivers/leds/Kconfig > > +++ b/drivers/leds/Kconfig > > @@ -49,6 +49,16 @@ config LEDS_BRIGHTNESS_HW_CHANGED > > > > See Documentation/ABI/testing/sysfs-class-led for > > details. > > +config LEDS_HW_CONTROLLED > > + bool "API for LEDs that can be controlled by hardware" > > + depends on LEDS_CLASS > > depends on OF || COMPILE_TEST > ? > I specifically did not add OF dependency so that this can be also used on non-OF systems. A device driver may register such LED itself... That's why hw_controlled_led_brightness_set symbol is exported. Do you think I shouldn't do it? > > + select LEDS_TRIGGERS > > + help > > + This option enables support for a generic API via which > > other drivers > > + can register LEDs that can be put into hardware > > controlled mode, eg. > > e.g. > This will need to be changed on multiple places, thanks. > > + a LED connected to an ethernet PHY can be configured to > > blink on > > an LED > This as well > > + network activity. > > + > > comment "LED drivers" > > > > config LEDS_88PM860X > > > > diff --git a/include/linux/leds-hw-controlled.h > > b/include/linux/leds-hw-controlled.h new file mode 100644 > > index 0000000000000..2c9b8a06def18 > > --- /dev/null > > +++ b/include/linux/leds-hw-controlled.h > > @@ -0,0 +1,74 @@ > > +/* SPDX-License-Identifier: GPL-2.0+ */ > > +/* > > + * Generic API for LEDs that can be controlled by hardware (eg. by > > an ethernet PHY chip) > > + * > > + * Copyright (C) 2020 Marek Behun <marek.behun@nic.cz> > > + */ > > +#ifndef _LINUX_LEDS_HW_CONTROLLED_H_ > > +#define _LINUX_LEDS_HW_CONTROLLED_H_ > > + > > +#include <linux/kernel.h> > > +#include <linux/leds.h> > > + > > +struct hw_controlled_led { > > + struct led_classdev cdev; > > + const struct hw_controlled_led_ops *ops; > > + struct mutex lock; > > + > > + /* these members are filled in by OF if OF is enabled */ > > + int addr; > > + bool active_low; > > + bool tristate; > > + > > + /* also filled in by OF, but changed by led_set_hw_mode > > operation */ > > + const char *hw_mode; > > + > > + void *priv; > > +}; > > +#define led_cdev_to_hw_controlled_led(l) container_of(l, struct > > hw_controlled_led, cdev) + > > +/* struct hw_controlled_led_ops: Operations on LEDs that can be > > controlled by HW > > + * > > + * All the following operations must be implemented: > > + * @led_init: Should initialize the LED from OF data (and sanity > > check whether they are correct). > > + * This should also change led->cdev.max_brightness, if > > the value differs from default, > > + * which is 1. > > + * @led_brightness_set: Sets brightness. > > + * @led_iter_hw_mode: Iterates available HW control mode names for > > this LED. > > + * @led_set_hw_mode: Sets HW control mode to value specified by > > given name. > > + * @led_get_hw_mode: Returns current HW control mode name. > > + */ > > Convert that struct info to kernel-doc? > Will look into this. Thanks. > > +struct hw_controlled_led_ops { > > + int (*led_init)(struct device *dev, struct > > hw_controlled_led *led); > > + int (*led_brightness_set)(struct device *dev, struct > > hw_controlled_led *led, > > + enum led_brightness brightness); > > + const char *(*led_iter_hw_mode)(struct device *dev, struct > > hw_controlled_led *led, > > + void **iter); > > + int (*led_set_hw_mode)(struct device *dev, struct > > hw_controlled_led *led, > > + const char *mode); > > + const char *(*led_get_hw_mode)(struct device *dev, struct > > hw_controlled_led *led); +}; > > + > > > + > > +#endif /* _LINUX_LEDS_HW_CONTROLLED_H_ */ > > thanks.
On 9/9/20 11:31 AM, Marek Behún wrote: > On Wed, 9 Sep 2020 11:20:00 -0700 > Randy Dunlap <rdunlap@infradead.org> wrote: > >> Hi, >> >> On 9/9/20 9:25 AM, Marek Behún wrote: >>> Many an ethernet PHY (and other chips) supports various HW control >>> modes for LEDs connected directly to them. >>> >>> This patch adds a generic API for registering such LEDs when >>> described in device tree. This API also exposes generic way to >>> select between these hardware control modes. >>> >>> This API registers a new private LED trigger called dev-hw-mode. >>> When this trigger is enabled for a LED, the various HW control >>> modes which are supported by the device for given LED can be >>> get/set via hw_mode sysfs file. >>> >>> Signed-off-by: Marek Behún <marek.behun@nic.cz> >>> --- >>> .../sysfs-class-led-trigger-dev-hw-mode | 8 + >>> drivers/leds/Kconfig | 10 + >>> drivers/leds/Makefile | 1 + >>> drivers/leds/leds-hw-controlled.c | 227 >>> ++++++++++++++++++ include/linux/leds-hw-controlled.h | >>> 74 ++++++ 5 files changed, 320 insertions(+) >>> create mode 100644 >>> Documentation/ABI/testing/sysfs-class-led-trigger-dev-hw-mode >>> create mode 100644 drivers/leds/leds-hw-controlled.c create mode >>> 100644 include/linux/leds-hw-controlled.h >> >>> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig >>> index 1c181df24eae4..5e47ab21aafb4 100644 >>> --- a/drivers/leds/Kconfig >>> +++ b/drivers/leds/Kconfig >>> @@ -49,6 +49,16 @@ config LEDS_BRIGHTNESS_HW_CHANGED >>> >>> See Documentation/ABI/testing/sysfs-class-led for >>> details. >>> +config LEDS_HW_CONTROLLED >>> + bool "API for LEDs that can be controlled by hardware" >>> + depends on LEDS_CLASS >> >> depends on OF || COMPILE_TEST >> ? >> > > I specifically did not add OF dependency so that this can be also used > on non-OF systems. A device driver may register such LED itself... > That's why hw_controlled_led_brightness_set symbol is exported. > > Do you think I shouldn't do it? I have no problem with it as it is. >>> + select LEDS_TRIGGERS >>> + help >>> + This option enables support for a generic API via which >>> other drivers >>> + can register LEDs that can be put into hardware >>> controlled mode, eg. thanks.
Hi! > Many an ethernet PHY (and other chips) supports various HW control modes > for LEDs connected directly to them. I guess this should be "Many ethernet PHYs (and other chips) support various HW control modes for LEDs connected directly to them." > This API registers a new private LED trigger called dev-hw-mode. When > this trigger is enabled for a LED, the various HW control modes which > are supported by the device for given LED can be get/set via hw_mode > sysfs file. > > Signed-off-by: Marek Behún <marek.behun@nic.cz> > --- > .../sysfs-class-led-trigger-dev-hw-mode | 8 + > drivers/leds/Kconfig | 10 + I guess this should live in drivers/leds/trigger/ledtrig-hw.c . I'd call the trigger just "hw"... > +Contact: Marek Behún <marek.behun@nic.cz> > + linux-leds@vger.kernel.org > +Description: (W) Set the HW control mode of this LED. The various available HW control modes > + are specific per device to which the LED is connected to and per LED itself. > + (R) Show the available HW control modes and the currently selected one. 80 columns :-) (and please fix that globally, at least at places where it is easy, like comments). > + return 0; > +err_free: > + devm_kfree(dev, led); > + return ret; > +} No need for explicit free with devm_ infrastructure? > + cur_mode = led->ops->led_get_hw_mode(dev->parent, led); > + > + for (mode = led->ops->led_iter_hw_mode(dev->parent, led, &iter); > + mode; > + mode = led->ops->led_iter_hw_mode(dev->parent, led, &iter)) { > + bool sel; > + > + sel = cur_mode && !strcmp(mode, cur_mode); > + > + len += scnprintf(buf + len, PAGE_SIZE - len, "%s%s%s ", sel ? "[" : "", mode, > + sel ? "]" : ""); > + } > + > + if (buf[len - 1] == ' ') > + buf[len - 1] = '\n'; Can this ever be false? Are you accessing buf[-1] in such case? > +int of_register_hw_controlled_leds(struct device *dev, const char *devicename, > + const struct hw_controlled_led_ops *ops); > +int hw_controlled_led_brightness_set(struct led_classdev *cdev, enum led_brightness brightness); > + Could we do something like hw_controlled_led -> hw_led to keep verbosity down and line lengths reasonable? Or hwc_led? > +extern struct led_hw_trigger_type hw_control_led_trig_type; > +extern struct led_trigger hw_control_led_trig; > + > +#else /* !IS_ENABLED(CONFIG_LEDS_HW_CONTROLLED) */ CONFIG_LEDS_HWC? Or maybe CONFIG_LEDTRIG_HW? Best regards, Pavel
On Wed, 9 Sep 2020 22:48:15 +0200 Pavel Machek <pavel@ucw.cz> wrote: > Hi! > > > Many an ethernet PHY (and other chips) supports various HW control modes > > for LEDs connected directly to them. > > I guess this should be > > "Many ethernet PHYs (and other chips) support various HW control modes > for LEDs connected directly to them." > I guess it is older English, used mainly in poetry, but I read it in works of contemporary fiction as well. As far as I could find, it is still actually gramatically correct. https://idioms.thefreedictionary.com/many+an https://en.wiktionary.org/wiki/many_a But I will change it if you insist on it. > > This API registers a new private LED trigger called dev-hw-mode. When > > this trigger is enabled for a LED, the various HW control modes which > > are supported by the device for given LED can be get/set via hw_mode > > sysfs file. > > > > Signed-off-by: Marek Behún <marek.behun@nic.cz> > > --- > > .../sysfs-class-led-trigger-dev-hw-mode | 8 + > > drivers/leds/Kconfig | 10 + > > I guess this should live in drivers/leds/trigger/ledtrig-hw.c . I'd > call the trigger just "hw"... > Will move in next version. Lets see what others think about the trigger name. > > +Contact: Marek Behún <marek.behun@nic.cz> > > + linux-leds@vger.kernel.org > > +Description: (W) Set the HW control mode of this LED. The various available HW control modes > > + are specific per device to which the LED is connected to and per LED itself. > > + (R) Show the available HW control modes and the currently selected one. > > 80 columns :-) (and please fix that globally, at least at places where > it is easy, like comments). > Linux is at 100 columns now since commit bdc48fa11e46, commited by Linus. See https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/scripts/checkpatch.pl?h=v5.9-rc4&id=bdc48fa11e46f867ea4d75fa59ee87a7f48be144 There was actually an article about this on Phoronix, I think. > > + return 0; > > +err_free: > > + devm_kfree(dev, led); > > + return ret; > > +} > > No need for explicit free with devm_ infrastructure? No, but if the registration failed, I don't see any reason why the memory should be freed only when the PHY device is destroyed, if the memory is not used... On failures other code in Linux frees even devm_ allocated resources. > > + cur_mode = led->ops->led_get_hw_mode(dev->parent, led); > > + > > + for (mode = led->ops->led_iter_hw_mode(dev->parent, led, &iter); > > + mode; > > + mode = led->ops->led_iter_hw_mode(dev->parent, led, &iter)) { > > + bool sel; > > + > > + sel = cur_mode && !strcmp(mode, cur_mode); > > + > > + len += scnprintf(buf + len, PAGE_SIZE - len, "%s%s%s ", sel ? "[" : "", mode, > > + sel ? "]" : ""); > > + } > > + > > + if (buf[len - 1] == ' ') > > + buf[len - 1] = '\n'; > > Can this ever be false? Are you accessing buf[-1] in such case? > It can be false if whole PAGE_SIZE is written. The code above using scnprintf only writes the first PAGE_SIZE bytes. You are right about the buf[-1] case though. len has to be nonzero. Thanks. > > +int of_register_hw_controlled_leds(struct device *dev, const char *devicename, > > + const struct hw_controlled_led_ops *ops); > > +int hw_controlled_led_brightness_set(struct led_classdev *cdev, enum led_brightness brightness); > > + > > Could we do something like hw_controlled_led -> hw_led to keep > verbosity down and line lengths reasonable? Or hwc_led? > I am not opposed, lets see what Andrew / others think. > > +extern struct led_hw_trigger_type hw_control_led_trig_type; > > +extern struct led_trigger hw_control_led_trig; > > + > > +#else /* !IS_ENABLED(CONFIG_LEDS_HW_CONTROLLED) */ > > CONFIG_LEDS_HWC? Or maybe CONFIG_LEDTRIG_HW? The second option looks more reasonable to me, if we move to drivers/leds/trigger. Marek
Hi! > > > Many an ethernet PHY (and other chips) supports various HW control modes > > > for LEDs connected directly to them. > > > > I guess this should be > > > > "Many ethernet PHYs (and other chips) support various HW control modes > > for LEDs connected directly to them." > > > > I guess it is older English, used mainly in poetry, but I read it in > works of contemporary fiction as well. As far as I could find, it is still > actually gramatically correct. > https://idioms.thefreedictionary.com/many+an > https://en.wiktionary.org/wiki/many_a > But I will change it if you insist on it. Okay, you got me. > > > +Contact: Marek Behún <marek.behun@nic.cz> > > > + linux-leds@vger.kernel.org > > > +Description: (W) Set the HW control mode of this LED. The various available HW control modes > > > + are specific per device to which the LED is connected to and per LED itself. > > > + (R) Show the available HW control modes and the currently selected one. > > > > 80 columns :-) (and please fix that globally, at least at places where > > it is easy, like comments). > > > > Linux is at 100 columns now since commit bdc48fa11e46, commited by > Linus. See > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/scripts/checkpatch.pl?h=v5.9-rc4&id=bdc48fa11e46f867ea4d75fa59ee87a7f48be144 > There was actually an article about this on Phoronix, I think. It is not. Checkpatch no longer warns about it, but 80 columns is still preffered, see Documentation/process/coding-style.rst . Plus, you want me to take the patch, not Linus. > > > +extern struct led_hw_trigger_type hw_control_led_trig_type; > > > +extern struct led_trigger hw_control_led_trig; > > > + > > > +#else /* !IS_ENABLED(CONFIG_LEDS_HW_CONTROLLED) */ > > > > CONFIG_LEDS_HWC? Or maybe CONFIG_LEDTRIG_HW? > > The second option looks more reasonable to me, if we move to > drivers/leds/trigger. Ok :-). Best regards, Pavel
On Wed, 9 Sep 2020 23:40:09 +0200 Pavel Machek <pavel@ucw.cz> wrote: > > > > > > 80 columns :-) (and please fix that globally, at least at places where > > > it is easy, like comments). > > > > > > > Linux is at 100 columns now since commit bdc48fa11e46, commited by > > Linus. See > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/scripts/checkpatch.pl?h=v5.9-rc4&id=bdc48fa11e46f867ea4d75fa59ee87a7f48be144 > > There was actually an article about this on Phoronix, I think. > > It is not. Checkpatch no longer warns about it, but 80 columns is > still preffered, see Documentation/process/coding-style.rst . Plus, > you want me to take the patch, not Linus. Very well, I shall rewrap it to 80 columns :) Marek
On Thu 2020-09-10 00:15:26, Marek Behun wrote: > On Wed, 9 Sep 2020 23:40:09 +0200 > Pavel Machek <pavel@ucw.cz> wrote: > > > > > > > > > 80 columns :-) (and please fix that globally, at least at places where > > > > it is easy, like comments). > > > > > > > > > > Linux is at 100 columns now since commit bdc48fa11e46, commited by > > > Linus. See > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/scripts/checkpatch.pl?h=v5.9-rc4&id=bdc48fa11e46f867ea4d75fa59ee87a7f48be144 > > > There was actually an article about this on Phoronix, I think. > > > > It is not. Checkpatch no longer warns about it, but 80 columns is > > still preffered, see Documentation/process/coding-style.rst . Plus, > > you want me to take the patch, not Linus. > > Very well, I shall rewrap it to 80 columns :) And thou shalt wrap to 80 columns ever after! Pavel
diff --git a/Documentation/ABI/testing/sysfs-class-led-trigger-dev-hw-mode b/Documentation/ABI/testing/sysfs-class-led-trigger-dev-hw-mode new file mode 100644 index 0000000000000..7bca112e7ff93 --- /dev/null +++ b/Documentation/ABI/testing/sysfs-class-led-trigger-dev-hw-mode @@ -0,0 +1,8 @@ +What: /sys/class/leds/<led>/hw_mode +Date: September 2020 +KernelVersion: 5.10 +Contact: Marek Behún <marek.behun@nic.cz> + linux-leds@vger.kernel.org +Description: (W) Set the HW control mode of this LED. The various available HW control modes + are specific per device to which the LED is connected to and per LED itself. + (R) Show the available HW control modes and the currently selected one. diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig index 1c181df24eae4..5e47ab21aafb4 100644 --- a/drivers/leds/Kconfig +++ b/drivers/leds/Kconfig @@ -49,6 +49,16 @@ config LEDS_BRIGHTNESS_HW_CHANGED See Documentation/ABI/testing/sysfs-class-led for details. +config LEDS_HW_CONTROLLED + bool "API for LEDs that can be controlled by hardware" + depends on LEDS_CLASS + select LEDS_TRIGGERS + help + This option enables support for a generic API via which other drivers + can register LEDs that can be put into hardware controlled mode, eg. + a LED connected to an ethernet PHY can be configured to blink on + network activity. + comment "LED drivers" config LEDS_88PM860X diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile index c2c7d7ade0d06..858e468e40df0 100644 --- a/drivers/leds/Makefile +++ b/drivers/leds/Makefile @@ -6,6 +6,7 @@ obj-$(CONFIG_LEDS_CLASS) += led-class.o obj-$(CONFIG_LEDS_CLASS_FLASH) += led-class-flash.o obj-$(CONFIG_LEDS_CLASS_MULTICOLOR) += led-class-multicolor.o obj-$(CONFIG_LEDS_TRIGGERS) += led-triggers.o +obj-$(CONFIG_LEDS_HW_CONTROLLED) += leds-hw-controlled.o # LED Platform Drivers (keep this sorted, M-| sort) obj-$(CONFIG_LEDS_88PM860X) += leds-88pm860x.o diff --git a/drivers/leds/leds-hw-controlled.c b/drivers/leds/leds-hw-controlled.c new file mode 100644 index 0000000000000..9ef58bf275efd --- /dev/null +++ b/drivers/leds/leds-hw-controlled.c @@ -0,0 +1,227 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * Generic API for LEDs that can be controlled by hardware (eg. by an ethernet PHY chip) + * + * Copyright (C) 2020 Marek Behun <marek.behun@nic.cz> + */ +#include <linux/leds-hw-controlled.h> +#include <linux/module.h> +#include <linux/of.h> + +int hw_controlled_led_brightness_set(struct led_classdev *cdev, enum led_brightness brightness) +{ + struct hw_controlled_led *led = led_cdev_to_hw_controlled_led(cdev); + int ret; + + mutex_lock(&led->lock); + ret = led->ops->led_brightness_set(cdev->dev->parent, led, brightness); + mutex_unlock(&led->lock); + + return ret; +} +EXPORT_SYMBOL_GPL(hw_controlled_led_brightness_set); + +static int of_register_hw_controlled_led(struct device *dev, struct device_node *np, + const char *devicename, + const struct hw_controlled_led_ops *ops) +{ + struct led_init_data init_data = {}; + struct hw_controlled_led *led; + u32 reg; + int ret; + + ret = of_property_read_u32(np, "reg", ®); + if (ret < 0) + return ret; + + led = devm_kzalloc(dev, sizeof(struct hw_controlled_led), GFP_KERNEL); + if (!led) + return -ENOMEM; + + led->ops = ops; + + led->cdev.max_brightness = 1; + led->cdev.brightness_set_blocking = hw_controlled_led_brightness_set; + led->cdev.trigger_type = &hw_control_led_trig_type; + led->addr = reg; + + of_property_read_string(np, "linux,default-trigger", &led->cdev.default_trigger); + of_property_read_string(np, "linux,default-hw-mode", &led->hw_mode); + + led->active_low = !of_property_read_bool(np, "enable-active-high"); + led->tristate = of_property_read_bool(np, "led-tristate"); + + init_data.fwnode = &np->fwnode; + init_data.devname_mandatory = true; + init_data.devicename = devicename; + + ret = led->ops->led_init(dev, led); + if (ret < 0) + goto err_free; + + ret = devm_led_classdev_register_ext(dev, &led->cdev, &init_data); + if (ret < 0) + goto err_free; + + return 0; +err_free: + devm_kfree(dev, led); + return ret; +} + +int of_register_hw_controlled_leds(struct device *dev, const char *devicename, + const struct hw_controlled_led_ops *ops) +{ + struct device_node *node = dev->of_node; + struct device_node *leds, *led; + int ret; + + if (!IS_ENABLED(CONFIG_OF_MDIO)) + return 0; + + if (!ops) + return -EINVAL; + + /* maybe we should have of_get_compatible_available_child as well */ + leds = of_get_compatible_child(node, "linux,hw-controlled-leds"); + if (!leds) + return 0; + + if (!devicename) + devicename = dev_name(dev); + + for_each_available_child_of_node(leds, led) { + ret = of_register_hw_controlled_led(dev, led, devicename, ops); + if (ret < 0) + dev_err(dev, "Nonfatal error: cannot register LED from node %pOFn: %i\n", + led, ret); + } + + return 0; +} +EXPORT_SYMBOL_GPL(of_register_hw_controlled_leds); + +static int hw_control_led_trig_activate(struct led_classdev *cdev) +{ + struct hw_controlled_led *led; + int ret; + + led = led_cdev_to_hw_controlled_led(cdev); + + if (!led->hw_mode) + return 0; + + mutex_lock(&led->lock); + ret = led->ops->led_set_hw_mode(cdev->dev->parent, led, led->hw_mode); + mutex_unlock(&led->lock); + + if (ret < 0) + dev_warn(cdev->dev->parent, "Could not set HW mode %s on LED %s: %i\n", + led->hw_mode, cdev->name, ret); + + /* don't fail to activate this trigger so that user can write hw_mode file */ + return 0; +} + +static void hw_control_led_trig_deactivate(struct led_classdev *cdev) +{ + struct hw_controlled_led *led; + int ret; + + led = led_cdev_to_hw_controlled_led(cdev); + + mutex_lock(&led->lock); + /* store HW mode before deactivation */ + led->hw_mode = led->ops->led_get_hw_mode(cdev->dev->parent, led); + ret = led->ops->led_set_hw_mode(cdev->dev->parent, led, NULL); + mutex_unlock(&led->lock); + + if (ret < 0) + dev_err(cdev->dev->parent, "Failed deactivating HW mode on LED %s\n", cdev->name); +} + +static ssize_t hw_mode_show(struct device *dev, struct device_attribute *attr, char *buf) +{ + struct hw_controlled_led *led; + const char *mode, *cur_mode; + void *iter = NULL; + int len = 0; + + led = led_cdev_to_hw_controlled_led(led_trigger_get_led(dev)); + + mutex_lock(&led->lock); + + cur_mode = led->ops->led_get_hw_mode(dev->parent, led); + + for (mode = led->ops->led_iter_hw_mode(dev->parent, led, &iter); + mode; + mode = led->ops->led_iter_hw_mode(dev->parent, led, &iter)) { + bool sel; + + sel = cur_mode && !strcmp(mode, cur_mode); + + len += scnprintf(buf + len, PAGE_SIZE - len, "%s%s%s ", sel ? "[" : "", mode, + sel ? "]" : ""); + } + + if (buf[len - 1] == ' ') + buf[len - 1] = '\n'; + + mutex_unlock(&led->lock); + + return len; +} + +static ssize_t hw_mode_store(struct device *dev, struct device_attribute *attr, const char *buf, + size_t count) +{ + struct hw_controlled_led *led; + int ret; + + led = led_cdev_to_hw_controlled_led(led_trigger_get_led(dev)); + + mutex_lock(&led->lock); + ret = led->ops->led_set_hw_mode(dev->parent, led, buf); + if (ret < 0) + return ret; + mutex_unlock(&led->lock); + + return count; +} + +static DEVICE_ATTR_RW(hw_mode); + +static struct attribute *hw_control_led_trig_attrs[] = { + &dev_attr_hw_mode.attr, + NULL +}; +ATTRIBUTE_GROUPS(hw_control_led_trig); + +struct led_hw_trigger_type hw_control_led_trig_type; +EXPORT_SYMBOL_GPL(hw_control_led_trig_type); + +struct led_trigger hw_control_led_trig = { + .name = "dev-hw-mode", + .activate = hw_control_led_trig_activate, + .deactivate = hw_control_led_trig_deactivate, + .trigger_type = &hw_control_led_trig_type, + .groups = hw_control_led_trig_groups, +}; +EXPORT_SYMBOL_GPL(hw_control_led_trig); + +static int __init hw_controlled_leds_init(void) +{ + return led_trigger_register(&hw_control_led_trig); +} + +static void __exit hw_controlled_leds_exit(void) +{ + led_trigger_unregister(&hw_control_led_trig); +} + +subsys_initcall(hw_controlled_leds_init); +module_exit(hw_controlled_leds_exit); + +MODULE_AUTHOR("Marek Behun <marek.behun@nic.cz>"); +MODULE_LICENSE("GPL v2"); +MODULE_DESCRIPTION("API for HW controlled LEDs"); diff --git a/include/linux/leds-hw-controlled.h b/include/linux/leds-hw-controlled.h new file mode 100644 index 0000000000000..2c9b8a06def18 --- /dev/null +++ b/include/linux/leds-hw-controlled.h @@ -0,0 +1,74 @@ +/* SPDX-License-Identifier: GPL-2.0+ */ +/* + * Generic API for LEDs that can be controlled by hardware (eg. by an ethernet PHY chip) + * + * Copyright (C) 2020 Marek Behun <marek.behun@nic.cz> + */ +#ifndef _LINUX_LEDS_HW_CONTROLLED_H_ +#define _LINUX_LEDS_HW_CONTROLLED_H_ + +#include <linux/kernel.h> +#include <linux/leds.h> + +struct hw_controlled_led { + struct led_classdev cdev; + const struct hw_controlled_led_ops *ops; + struct mutex lock; + + /* these members are filled in by OF if OF is enabled */ + int addr; + bool active_low; + bool tristate; + + /* also filled in by OF, but changed by led_set_hw_mode operation */ + const char *hw_mode; + + void *priv; +}; +#define led_cdev_to_hw_controlled_led(l) container_of(l, struct hw_controlled_led, cdev) + +/* struct hw_controlled_led_ops: Operations on LEDs that can be controlled by HW + * + * All the following operations must be implemented: + * @led_init: Should initialize the LED from OF data (and sanity check whether they are correct). + * This should also change led->cdev.max_brightness, if the value differs from default, + * which is 1. + * @led_brightness_set: Sets brightness. + * @led_iter_hw_mode: Iterates available HW control mode names for this LED. + * @led_set_hw_mode: Sets HW control mode to value specified by given name. + * @led_get_hw_mode: Returns current HW control mode name. + */ +struct hw_controlled_led_ops { + int (*led_init)(struct device *dev, struct hw_controlled_led *led); + int (*led_brightness_set)(struct device *dev, struct hw_controlled_led *led, + enum led_brightness brightness); + const char *(*led_iter_hw_mode)(struct device *dev, struct hw_controlled_led *led, + void **iter); + int (*led_set_hw_mode)(struct device *dev, struct hw_controlled_led *led, + const char *mode); + const char *(*led_get_hw_mode)(struct device *dev, struct hw_controlled_led *led); +}; + +#if IS_ENABLED(CONFIG_LEDS_HW_CONTROLLED) + +#define hw_controlled_led_ops_ptr(s) (s) + +int of_register_hw_controlled_leds(struct device *dev, const char *devicename, + const struct hw_controlled_led_ops *ops); +int hw_controlled_led_brightness_set(struct led_classdev *cdev, enum led_brightness brightness); + +extern struct led_hw_trigger_type hw_control_led_trig_type; +extern struct led_trigger hw_control_led_trig; + +#else /* !IS_ENABLED(CONFIG_LEDS_HW_CONTROLLED) */ + +#define hw_controlled_led_ops_ptr(s) NULL +static inline int of_register_hw_controlled_leds(struct device *dev, const char *devicename, + const struct hw_controlled_led_ops *ops) +{ + return 0; +} + +#endif /* !IS_ENABLED(CONFIG_LEDS_HW_CONTROLLED) */ + +#endif /* _LINUX_LEDS_HW_CONTROLLED_H_ */
Many an ethernet PHY (and other chips) supports various HW control modes for LEDs connected directly to them. This patch adds a generic API for registering such LEDs when described in device tree. This API also exposes generic way to select between these hardware control modes. This API registers a new private LED trigger called dev-hw-mode. When this trigger is enabled for a LED, the various HW control modes which are supported by the device for given LED can be get/set via hw_mode sysfs file. Signed-off-by: Marek Behún <marek.behun@nic.cz> --- .../sysfs-class-led-trigger-dev-hw-mode | 8 + drivers/leds/Kconfig | 10 + drivers/leds/Makefile | 1 + drivers/leds/leds-hw-controlled.c | 227 ++++++++++++++++++ include/linux/leds-hw-controlled.h | 74 ++++++ 5 files changed, 320 insertions(+) create mode 100644 Documentation/ABI/testing/sysfs-class-led-trigger-dev-hw-mode create mode 100644 drivers/leds/leds-hw-controlled.c create mode 100644 include/linux/leds-hw-controlled.h