linux-kernel - Re: [PATCH v2 2/2] leds: add new LED driver for TI LP5812

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: <20250226-lovely-bald-rabbit-08ce5b@krzk-bin>
Date: Wed, 26 Feb 2025 08:40:42 +0100
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Nam Tran <trannamatk@...il.com>
Cc: pavel@...nel.org, lee@...nel.org, krzk+dt@...nel.org, robh@...nel.org, 
	conor+dt@...nel.org, devicetree@...r.kernel.org, linux-leds@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/2] leds: add new LED driver for TI LP5812

On Wed, Feb 26, 2025 at 12:06:01AM +0700, Nam Tran wrote:
> The chip can drive LED matrix 4x3.
> This driver uses I2C to communicate with the chip.

You decided to ignore my previous question, but it still valid. Respond
to it.

Sorry, if you keep ignoring reviewers, you will not get anywhere.

> 
> Signed-off-by: Nam Tran <trannamatk@...il.com>
> ---
>  MAINTAINERS                                   |    6 +
>  .../arm/boot/dts/broadcom/bcm2711-rpi-4-b.dts |    7 +

No, DTS is not part of LED. You cannot combine it in the same patch.


>  drivers/leds/Kconfig                          |   13 +
>  drivers/leds/Makefile                         |    2 +
>  drivers/leds/leds-lp5812-common.c             | 1097 ++++++++
>  drivers/leds/leds-lp5812-common.h             |  323 +++
>  drivers/leds/leds-lp5812-regs.h               |   96 +
>  drivers/leds/leds-lp5812.c                    | 2317 +++++++++++++++++
>  8 files changed, 3861 insertions(+)
>  create mode 100644 drivers/leds/leds-lp5812-common.c
>  create mode 100644 drivers/leds/leds-lp5812-common.h
>  create mode 100644 drivers/leds/leds-lp5812-regs.h
>  create mode 100644 drivers/leds/leds-lp5812.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 32312959d595..fabd09eb6c0b 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -23499,6 +23499,12 @@ M:	Nam Tran <trannamatk@...il.com>
>  L:	linux-leds@...r.kernel.org
>  S:	Maintained
>  F:	Documentation/devicetree/bindings/leds/ti,lp5812.yaml
> +F:	drivers/leds/Kconfig
> +F:	drivers/leds/Makefile
> +F:	drivers/leds/leds-lp5812-common.c
> +F:	drivers/leds/leds-lp5812-common.h
> +F:	drivers/leds/leds-lp5812-regs.h
> +F:	drivers/leds/leds-lp5812.c
>  
>  TEXAS INSTRUMENTS' SYSTEM CONTROL INTERFACE (TISCI) PROTOCOL DRIVER
>  M:	Nishanth Menon <nm@...com>
> diff --git a/arch/arm/boot/dts/broadcom/bcm2711-rpi-4-b.dts b/arch/arm/boot/dts/broadcom/bcm2711-rpi-4-b.dts
> index 353bb50ce542..0d01c6b6a5ba 100644
> --- a/arch/arm/boot/dts/broadcom/bcm2711-rpi-4-b.dts
> +++ b/arch/arm/boot/dts/broadcom/bcm2711-rpi-4-b.dts
> @@ -270,3 +270,10 @@ &vec {
>  &wifi_pwrseq {
>  	reset-gpios = <&expgpio 1 GPIO_ACTIVE_LOW>;
>  };
> +
> +&i2c1 {
> +	lp5812@1b {

Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation

> +		compatible = "ti,lp5812";
> +		reg = <0x1b>;
> +	};
> +};
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index 2b27d043921c..e43dbd5a4d8c 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -483,6 +483,19 @@ config LEDS_LP5569
>  	  Driver provides direct control via LED class and interface for
>  	  programming the engines.
>  

...

> +	pr_info("[DEBUG]: sysfs_create_group successfully\n");
> +
> +	ret = lp5812_init_dev_config(chip, "tcmscan:4:0:1:2:3", 0);
> +	if (ret) {
> +		pr_info("[DEBUG]: %s: lp5812_init_dev_config failed\n",
> +			__func__);
> +		return ret;
> +	}
> +	pr_info("[DEBUG]: lp5812_init_dev_config successfully\n");
> +	/* initialize lp5812 chip */
> +	ret = lp5812_initialize(chip);
> +
> +	/* code to verify i2c read/write ok or not */
> +	lp5812_read(chip, (u16)DEV_CONFIG2, &val);
> +	pr_info("[DEBUG]: DEV_CONFIG2 value: 0x%02X\n", val);
> +
> +	lp5812_write(chip, (u16)LED_A1_AUTO_BASE_ADRR, 0x14);
> +	lp5812_read(chip, (u16)LED_A1_AUTO_BASE_ADRR, &val);
> +	pr_info("[DEBUG]: LED_A1_AUTO_BASE_ADRR value: 0x%02X\n", val);

So entire driver is full of such debug code, looking straight from some
Android vendor tree.

No, that's not how upstreaming works. You now copy all bad habits, all
poor code and all the issues we fixed. Start from scratch from latest
mainline driver and customize it to your needs.


Best regards,
Krzysztof