On Fri, Sep 03, 2021 at 03:45:45PM -0500, Ian Pilcher wrote:
> /sys/class/leds/<led>/add_blkdev - to create device/LED associations
>
> /sys/class/leds/<led>/delete_blkdev to remove device/LED associations
>
> For both attributes, accept multiple device names separated by whitespace
>
> Signed-off-by: Ian Pilcher <arequipeno@xxxxxxxxx>
> ---
> drivers/leds/trigger/ledtrig-blkdev.c | 48 +++++++++++++++++++++++++++
> 1 file changed, 48 insertions(+)
>
> diff --git a/drivers/leds/trigger/ledtrig-blkdev.c b/drivers/leds/trigger/ledtrig-blkdev.c
> index b2ec85b805d0..db82d37fc721 100644
> --- a/drivers/leds/trigger/ledtrig-blkdev.c
> +++ b/drivers/leds/trigger/ledtrig-blkdev.c
> @@ -509,3 +509,51 @@ static void blkdev_deactivate(struct led_classdev *const led_dev)
>
> module_put(THIS_MODULE);
> }
> +
> +
> +/*
> + *
> + * sysfs attributes to add & delete devices from LEDs
> + *
> + */
> +
> +static ssize_t blkdev_add_or_del(struct device *const dev,
> + struct device_attribute *const attr,
> + const char *const buf, const size_t count);
> +
> +static struct device_attribute ledtrig_blkdev_attr_add =
> + __ATTR(add_blkdev, 0200, NULL, blkdev_add_or_del);
> +
> +static struct device_attribute ledtrig_blkdev_attr_del =
> + __ATTR(delete_blkdev, 0200, NULL, blkdev_add_or_del);
DEVICE_ATTR_RO()? Or something like that? Do not use __ATTR() for
device attributes if at all possible, worst case, use DEVICE_ATTR()
here.
And the mode settings are odd, are you sure you want that?
> +static ssize_t blkdev_add_or_del(struct device *const dev,
> + struct device_attribute *const attr,
> + const char *const buf, const size_t count)
> +{
> + struct ledtrig_blkdev_led *const led = led_trigger_get_drvdata(dev);
> + const char *const disk_name = blkdev_skip_space(buf);
> + const char *const endp = blkdev_find_space(disk_name);
> + const ptrdiff_t name_len = endp - disk_name; /* always >= 0 */
> + int ret;
> +
> + if (name_len == 0) {
> + pr_info("blkdev LED: empty block device name\n");
Looks like debugging code, please remove.
And how can this ever happen?
> + return -EINVAL;
> + }
> +
> + if (attr == &ledtrig_blkdev_attr_del) {
> + blkdev_disk_delete(led, disk_name, name_len);
> + } else { /* attr == &ledtrig_blkdev_attr_add */
> + ret = blkdev_disk_add(led, disk_name, name_len);
Why do you have a single attribute callback that does two totally
different things? Just have 2 different callback functions please, it
makes things much easier to review and maintain over time.
thanks,
greg k-h
