linux-kernel - Re: [PATCH] leds: add aw91xxx driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CACRpkdb5Vatr+GfNwUfou4+5NviNkdYZYH2hSNYo2qifX324Uw@mail.gmail.com>
Date: Wed, 19 Nov 2025 00:42:51 +0100
From: Linus Walleij <linus.walleij@...aro.org>
To: 429368636@...com
Cc: lee@...nel.org, pavel@...nel.org, brgl@...ev.pl, 
	linux-kernel@...r.kernel.org, linux-leds@...r.kernel.org, 
	linux-gpio@...r.kernel.org, zhangxinyu <gavin.zhang@...ot.com>
Subject: Re: [PATCH] leds: add aw91xxx driver

Hi Zhang,

thanks for your patch!

I see others gave some comments but here are some stuff I saw:

On Mon, Nov 17, 2025 at 10:36 AM <429368636@...com> wrote:

> +static int aw91xxx_i2c_write_bits(struct aw91xxx *aw91xxx,
> +               unsigned char reg_addr, unsigned char *buf, unsigned int len)
> +static int aw91xxx_i2c_read_bits(struct aw91xxx *aw91xxx,
> +               unsigned char reg_addr, unsigned char *buf, unsigned int len)
(etc)

Use regmap abstractions to read/write registers in a
controlled fashion. Check good examples!
git grep regmap drivers/leds/

> +static void aw91xxc_led_blink(struct work_struct *work)
> +static void aw91xxx_led_blink(struct aw91xxx *aw91xxx, unsigned char blink, unsigned char delay)

> +/******************************************************
> + *
> + * sys group attribute: reg
> + *
> + ******************************************************/

Regmap will give you debugfs automatically if you wanna play
around with registers.

> +static ssize_t
> +blink_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +       ssize_t len = 0;
> +
> +       len += snprintf(buf + len, PAGE_SIZE - len, "aw91xx_blink()\n");
> +       len += snprintf(buf + len, PAGE_SIZE - len, "echo 0x9 > blink\n");
> +       len += snprintf(buf + len, PAGE_SIZE - len, "echo 0x12> blink\n");
> +       len += snprintf(buf + len, PAGE_SIZE - len, "echo 0x24> blink\n");
> +
> +       return len;
> +}

But the LED API already supports blinking, don't invent new sysfs
ABIs for this:

struct led_classdev {
(...)
        /*
         * Activate hardware accelerated blink, delays are in milliseconds
         * and if both are zero then a sensible default should be chosen.
         * The call should adjust the timings in that case and if it can't
         * match the values specified exactly.
         * Deactivate blinking again when the brightness is set to LED_OFF
         * via the brightness_set() callback.
         * For led_blink_set_nosleep() the LED core assumes that blink_set
         * implementations, of drivers which do not use brightness_set_blocking,
         * will not sleep. Therefor if brightness_set_blocking is not set
         * this function must not sleep!
         */
        int             (*blink_set)(struct led_classdev *led_cdev,
                                     unsigned long *delay_on,
                                     unsigned long *delay_off);


When it comes to dim:ing and fading, I'm pretty sure that either can be
done with existing APIs or we can add new ones, with some coordination
with the input maintainer, this must be common for input devices?

Yours,
Linus Walleij