Re: [PATCH] leds: add aw91xxx driver

[Krzysztof Kozlowski <krzk@kernel.org>]

On 17/11/2025 10:35, 429368636@qq.com wrote:
> From: zhangxinyu <gavin.zhang@faiot.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@qq.com>

Mixed up SoB.

> ---
>  drivers/leds/Kconfig        |   11 +



...

> +
> +	if (aw91xxx->matrix_key_enable) {
> +		/* key init */
> +		ret = aw91xxx_key_feature_init(aw91xxx);
> +		if (ret) {
> +			dev_err(aw91xxx->dev, "aw91xxx key feature init failed \r\n");
> +			goto err_free_rst;
> +		}
> +	}
> +
> +	if (aw91xxx->led_feature_enable) {
> +		ret = aw91xxx_parse_led_cdev(aw91xxx, np);
> +		if (ret < 0) {
> +			dev_err(&i2c->dev, "%s error creating led class dev\n", __func__);
> +			goto free_key;
> +		}
> +	}
> +
> +	if (aw91xxx->gpio_feature_enable) {
> +		/* gpio init */
> +		ret = aw91xxx_gpio_feature_init(aw91xxx);
> +		if (ret) {
> +			dev_err(aw91xxx->dev, "aw91xxx gpio feature init failed \r\n");
> +			goto free_key;
> +		}
> +	}
> +	aw91xxx->screen_state = true;
> +
> +	pr_err("%s probe completed successfully!\n", __func__);

No, drop.

> +
> +	return 0;
> +
> +free_key:
> +	aw91xxx_key_free_all_resource(aw91xxx);
> +err_free_rst:
> +	gpio_free(aw91xxx->reset_gpio);
> +err:
> +	devm_kfree(&i2c->dev, aw91xxx);

What?

> +	return ret;
> +}



> +
> +static const struct of_device_id aw91xxx_dt_match[] = {
> +	{ .compatible = "awinic,aw91xxx_led" },

Undocumented ABI.

> +	{ },
> +};
> +
> +static struct i2c_driver aw91xxx_i2c_driver = {
> +	.driver = {
> +		.name = AW91XXX_I2C_NAME,
> +		.owner = THIS_MODULE,

So you upstream us 12 year old code, with same issues, same bugs like
above and below:

> +		.of_match_table = of_match_ptr(aw91xxx_dt_match),

Here with warning...

You should start by taking a recently reviewed driver as base.

> +	},
> +	.probe = aw91xxx_i2c_probe,
> +	.remove = aw91xxx_i2c_remove,
> +	.id_table = aw91xxx_i2c_id,
> +};
> +
> +static int __init aw91xxx_i2c_init(void)
> +{
> +	int ret = 0;
> +
> +	pr_err("aw91xxx driver version %s\n", AW91XXX_DRIVER_VERSION);

There are no versions of drivers. Don't add it.

> +
> +	ret = i2c_add_driver(&aw91xxx_i2c_driver);
> +	if (ret) {
> +		pr_err("fail to add aw91xxx device into i2c\n");
> +		return ret;
> +	}

No, drop such stuff. Just module_i2c_driver.

> +
> +	return 0;
> +}
> +module_init(aw91xxx_i2c_init);
> +
> +static void __exit aw91xxx_i2c_exit(void)
> +{
> +	i2c_del_driver(&aw91xxx_i2c_driver);
> +}
> +module_exit(aw91xxx_i2c_exit);
> +
> +
> +MODULE_DESCRIPTION("AW91XXX LED Driver");
> +MODULE_LICENSE("GPL v2");
> diff --git a/drivers/leds/leds-aw91xxx.h b/drivers/leds/leds-aw91xxx.h
> new file mode 100644
> index 000000000000..d69c2334ffe0
> --- /dev/null
> +++ b/drivers/leds/leds-aw91xxx.h
> @@ -0,0 +1,128 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +#ifndef _AW91XXX_H_
> +#define _AW91XXX_H_
> +
> +#define AWINIC_DEBUG		1
> +
> +#ifdef AWINIC_DEBUG
> +#define AW_DEBUG(fmt, args...)	pr_info(fmt, ##args)
> +#else
> +#define AW_DEBUG(fmt, ...)
> +
> +#endif
> +
> +#define MAX_I2C_BUFFER_SIZE 65536
> +
> +#define AW91XXX_ID 0x23
> +#define AW91XXX_KEY_PORT_MAX (0x10) /* 16 */
> +#define AW91XXX_INT_MASK (0xFFFF)
> +
> +enum AW91XXX_FADE_TIME {
> +	AW91XXX_FADE_TIME_0000MS = 0x00,
> +	AW91XXX_FADE_TIME_0315MS = 0X01,
> +	AW91XXX_FADE_TIME_0630MS = 0x02,
> +	AW91XXX_FADE_TIME_1260MS = 0x03,
> +	AW91XXX_FADE_TIME_2520MS = 0x04,
> +	AW91XXX_FADE_TIME_5040MS = 0x05
> +};
> +
> +enum aw91xxx_gpio_dir {
> +	AW91XXX_GPIO_INPUT = 0,
> +	AW91XXX_GPIO_OUTPUT = 1,
> +};
> +
> +enum aw91xxx_gpio_val {
> +	AW91XXX_GPIO_HIGH = 1,
> +	AW91XXX_GPIO_LOW = 0,
> +};
> +
> +enum aw91xxx_gpio_output_mode {
> +	AW91XXX_OPEN_DRAIN_OUTPUT = 0,
> +	AW91XXX_PUSH_PULL_OUTPUT = 1,
> +};
> +
> +struct aw91xxx_singel_gpio {
> +	unsigned int gpio_idx;
> +	enum aw91xxx_gpio_dir gpio_direction;
> +	enum aw91xxx_gpio_val state;
> +	struct aw91xxx *priv;
> +};
> +
> +struct aw91xxx_gpio {
> +	unsigned int gpio_mask;
> +	unsigned int gpio_num;
> +	enum aw91xxx_gpio_output_mode output_mode;
> +	struct aw91xxx_singel_gpio *single_gpio_data;
> +};
> +
> +typedef struct {
> +	char name[10];
> +	int key_code;
> +	int key_val;
> +} KEY_STATE;
> +
> +unsigned int aw91xxx_separate_key_data[AW91XXX_KEY_PORT_MAX] = {
> +/*      0    1    2    3 */
> +	1,   2,   3,   4,
> +	5,   6,   7,   8,
> +	9,   10,  11,  12,
> +	13,  14,  15,  16
> +};
> +
> +struct aw91xxx_key {
> +	unsigned int key_mask;
> +	unsigned int input_port_nums;
> +	unsigned int output_port_nums;
> +	unsigned int input_port_mask;
> +	unsigned int output_port_mask;
> +	unsigned int new_input_state;
> +	unsigned int old_input_state;
> +	unsigned int *new_output_state;
> +	unsigned int *old_output_state;
> +	unsigned int *def_output_state;
> +	bool wake_up_enable;
> +	struct input_dev *input;
> +
> +	unsigned int debounce_delay;
> +	struct delayed_work int_work;
> +	struct hrtimer key_timer;
> +	struct work_struct key_work;
> +	KEY_STATE *keymap;
> +	int keymap_len;
> +	struct aw91xxx *priv;
> +};
> +
> +struct aw91xxx {
> +	struct i2c_client *i2c;
> +	struct device *dev;
> +	struct led_classdev cdev;
> +	struct work_struct brightness_work;
> +	struct delayed_work int_work;
> +
> +	int reset_gpio;
> +	int irq_gpio;
> +	int irq_num;
> +
> +	unsigned char chipid;
> +	unsigned char vendor_id;
> +	unsigned char blink;
> +
> +	int imax;
> +	int rise_time;
> +	int on_time;
> +	int fall_time;
> +	int off_time;
> +
> +	bool led_feature_enable;
> +	bool gpio_feature_enable;
> +	bool matrix_key_enable;
> +	bool single_key_enable;
> +	bool screen_state;
> +
> +	struct aw91xxx_gpio *gpio_data;
> +	struct aw91xxx_key *key_data;
> +};
> +
> +
> +#endif
> +


Best regards,
Krzysztof