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] [day] [month] [year] [list]

Message-ID: <CAMRc=Md9-Tqejkmw0dTOj686ZZ=QABEqeKgz1xUYniJ-swnxxA@mail.gmail.com>
Date: Mon, 17 Nov 2025 11:28:30 +0100
From: Bartosz Golaszewski <brgl@...ev.pl>
To: 429368636@...com
Cc: lee@...nel.org, pavel@...nel.org, linus.walleij@...aro.org, 
	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

On Mon, Nov 17, 2025 at 10:36 AM <429368636@...com> wrote:
>
> From: zhangxinyu <gavin.zhang@...ot.com>
>
> This commit adds support for AWINIC AW91XXX 6-channel LED driver.
> The chip supports 6 PWM channels and is controlled with I2C.
>
> Signed-off-by: zhangxinyu <429368636@...com>
> ---

Hi!

I have only skimmed through the code as it still requires a lot of work.

> diff --git a/drivers/leds/leds-aw91xxx.c b/drivers/leds/leds-aw91xxx.c
> new file mode 100644
> index 000000000000..8d809f3e443b
> --- /dev/null
> +++ b/drivers/leds/leds-aw91xxx.c
> @@ -0,0 +1,1865 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * leds-aw91xxx.c   aw91xxx led module
> + *
> + * Copyright (c) 2021 AWINIC Technology CO., LTD
> + *
> + * This program is free software; you can redistribute  it and/or modify it
> + * under  the terms of  the GNU General  Public License as published by the
> + * Free Software Foundation;  either version 2 of the  License, or (at your
> + * option) any later version.
> + */
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> +#include <linux/workqueue.h>
> +#include <linux/errno.h>
> +#include <linux/pm.h>
> +#include <linux/platform_device.h>
> +#include <linux/input.h>
> +#include <linux/i2c.h>
> +#include <linux/gpio.h>

Including this legacy header makes it an immediate NAK. Please use
interfaces from linux/gpio/consumer.h.

> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/of_gpio.h>
> +#include <linux/slab.h>
> +#include <linux/wait.h>
> +#include <linux/time.h>
> +#include <linux/delay.h>
> +#include <linux/of_gpio.h>

Duplicated include. Also: you don't really need it.

> +#include <linux/miscdevice.h>
> +#include <linux/uaccess.h>
> +#include <linux/leds.h>
> +#include <linux/pinctrl/consumer.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/hrtimer.h>
> +#include <linux/kernel.h>
> +#include <linux/firmware.h>
> +#include <linux/version.h>
> +#include <linux/debugfs.h>
> +#include <linux/leds.h>
> +#include <linux/fb.h>

Please order includes alphabetically, you'll avoid duplications.

> +//#include <stddef.h>

??

> +#include "leds-aw91xxx.h"

What's the reason for this header's existence? Doesn't seem like
anything here should be public?

> +
> +static DEVICE_ATTR_RW(reg);
> +static DEVICE_ATTR_RW(hwen);
> +static DEVICE_ATTR_RW(blink);
> +static DEVICE_ATTR_WO(dim);
> +static DEVICE_ATTR_WO(all_dim);
> +static DEVICE_ATTR_WO(fade_mode);
> +
> +
> +static struct attribute *aw91xxx_attributes[] = {
> +       &dev_attr_reg.attr,
> +       &dev_attr_hwen.attr,
> +       &dev_attr_blink.attr,
> +       &dev_attr_dim.attr,
> +       &dev_attr_all_dim.attr,
> +       &dev_attr_fade_mode.attr,
> +       NULL
> +};
> +
> +static struct attribute_group aw91xxx_attribute_group = {
> +       .attrs = aw91xxx_attributes
> +};

This whole driver looks like it belongs in driver/staging/ for now.
The LEDs subsystem provides all the relevant sysfs attributes already.
You shouldn't create your own.

> +
> +static void aw91xxx_i2c_remove(struct i2c_client *i2c)
> +{
> +       struct aw91xxx *aw91xxx = i2c_get_clientdata(i2c);
> +
> +       if (gpio_is_valid(aw91xxx->reset_gpio))
> +               gpio_free(aw91xxx->reset_gpio);
> +       aw91xxx_gpio_free_all_resource(aw91xxx);
> +       devm_kfree(aw91xxx->dev, aw91xxx);

The whole purpose of devres is to not have to do this.

> +
> +}
> +
> +static const struct i2c_device_id aw91xxx_i2c_id[] = {
> +       { AW91XXX_I2C_NAME, 0 },
> +       { }
> +};
> +MODULE_DEVICE_TABLE(i2c, aw91xxx_i2c_id);
> +
> +static const struct of_device_id aw91xxx_dt_match[] = {
> +       { .compatible = "awinic,aw91xxx_led" },
> +       { },
> +};

You need DT bindings for this too.

Bart