lkml.org 
[lkml]   [2020]   [Sep]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH net-next + leds v2 2/7] leds: add generic API for LEDs that can be controlled by hardware
    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.

    \
     
     \ /
      Last update: 2020-09-09 20:31    [W:4.172 / U:0.168 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site