Message-Id: <20250706102844.35856-1-trannamatk@gmail.com>
Date: Sun,  6 Jul 2025 17:28:44 +0700
From: Nam Tran <trannamatk@...il.com>
To: lee@...nel.org
Cc: pavel@...nel.org,
	rdunlap@...radead.org,
	christophe.jaillet@...adoo.fr,
	krzk+dt@...nel.org,
	robh@...nel.org,
	conor+dt@...nel.org,
	corbet@....net,
	linux-leds@...r.kernel.org,
	linux-kernel@...r.kernel.org,
	devicetree@...r.kernel.org,
	linux-doc@...r.kernel.org
Subject: Re: [PATCH v10 2/4] leds: add TI/National Semiconductor LP5812 LED Driver

On Thu, 26 Jun 2025, Lee Jones wrote:

> On Thu, 19 Jun 2025, Nam Tran wrote:
> 
> > The LP5812 is a 4x3 matrix RGB LED driver with an autonomous animation
> > engine and time-cross-multiplexing (TCM) support for up to 12 LEDs or
> > 4 RGB LEDs. Each LED can be configured through the related registers
> > to realize vivid and fancy lighting effects.
> > 
> > Signed-off-by: Nam Tran <trannamatk@...il.com>
> > ---
> >  MAINTAINERS                    |    4 +
> >  drivers/leds/rgb/Kconfig       |   13 +
> >  drivers/leds/rgb/Makefile      |    1 +
> >  drivers/leds/rgb/leds-lp5812.c | 1943 ++++++++++++++++++++++++++++++++
> >  drivers/leds/rgb/leds-lp5812.h |  227 ++++
> >  5 files changed, 2188 insertions(+)
> 
> Reviewing a 2k+ line driver is not fun!
> 
> Better to reduce it down to bare bones and add new functionality over
> time.  Or at least over different patches within a set.

I agree - reviewing such a large patch isn't ideal. For the next revision,
I'll submit the smallest version of the driver that retains only the essential
functionality, such as basic brightness control and LED registration.
Additional features like the autonomous animation engine will be submitted as
separate follow-up patches.

...

> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/init.h>
> > +#include <linux/i2c.h>
> > +#include <linux/sysfs.h>
> > +#include <linux/mutex.h>
> > +#include <linux/types.h>
> > +#include <linux/leds.h>
> > +#include <linux/delay.h>
> > +#include <linux/led-class-multicolor.h>
> > +#include "leds-lp5812.h"
> 
> Alphabetical.
> 

I will reorder them.

> > +#define LP5812_SC_LED "SC_LED"
> > +#define LP5812_MC_LED "MC_LED"
> > +
> > +#define LP5812_AUTO_PAUSE_ADDR(chan)  (LP5812_AEU_BASE + (chan) * 26)
> > +#define LP5812_AUTO_PLAYBACK_ADDR(chan)  (LP5812_AEU_BASE + (chan) * 260 + 1)
> 
> Bring these 2 down to the line below like the others.  Just to make it
> more uniform / easier to read.
> 

I will update it.

> > +#define LP5812_AEU_PWM_ADDR(chan, aeu, pwm_chan)  \
> > +	(LP5812_AEU_BASE + (chan) * 26 + ((aeu) - 1) * 8 + 2 + (pwm_chan) - 1)
> > +#define LP5812_AEU_SLOPE_TIME_ADDR(chan, aeu, slope_chan)  \
> > +	(LP5812_AEU_BASE + (chan) * 26 + ((aeu) - 1) * 8 + 2 + 5 + ((slope_chan) / 2))
> > +#define LP5812_AEU_PLAYBACK_ADDR(chan, aeu)  \
> > +	(LP5812_AEU_BASE + (chan) * 26 + ((aeu) - 1) * 8 + 2 + 5 + 2)
> > +
> > +#define to_lp5812_led(x) container_of(x, struct lp5812_data, kobj)
> > +#define to_anim_engine_unit(x) container_of(x, struct anim_engine_unit, kobj)
> > +
> > +static int lp5812_read_tsd_config_status(struct lp5812_chip *chip, u8 *reg_val);
> 
> No forward declarations.  Rearrange the functions instead.
> 

I will rearrange the functions.

> > +
> > +/* Begin common functions */
> 
> This comment is superfluous.
> 

I will drop it.

> > +static struct lp5812_led *cdev_to_lp5812_led(struct led_classdev *cdev)
> > +{
> > +	return container_of(cdev, struct lp5812_led, cdev);
> > +}
> > +
> > +static struct lp5812_led *mcled_cdev_to_lp5812_led(struct led_classdev_mc *mc_cdev)
> > +{
> > +	return container_of(mc_cdev, struct lp5812_led, mc_cdev);
> > +}
> 
> Are you sure you need 4 of these.  This seems excessive.
> 

You are right. I will inline them to simplify the code.

> > +static struct lp5812_led *dev_to_lp5812_led(struct device *dev)
> > +{
> > +	return cdev_to_lp5812_led(dev_get_drvdata(dev));
> > +}
> > +
> > +static struct lp5812_led *dev_to_lp5812_led_mc(struct device *dev)
> > +{
> > +	return mcled_cdev_to_lp5812_led(dev_get_drvdata(dev));
> > +}
> > +
> > +static int lp5812_write(struct lp5812_chip *chip, u16 reg, u8 val)
> > +{
> > +	int ret;
> 
> Nit: Small variables at the bottom.
> 

I will reorder them.

> > +	struct i2c_msg msg;
> > +	struct device *dev = &chip->i2c_cl->dev;
> > +	u8 extracted_bits; /* save 9th and 8th bit of reg address */
> 
> All comments should start with an uppercase char.
> 
> Place this comment where you do the work.
> 
> Save them to do what?  More information please.
> 

I will fix the comment placement, and clarify it as suggested.

> > +	u8 buf[2] = {(u8)(reg & 0xFF), val};
> > +
> > +	extracted_bits = (reg >> 8) & 0x03;
> > +	msg.addr = (chip->i2c_cl->addr << 2) | extracted_bits;
> > +	msg.flags = 0;
> > +	msg.len = sizeof(buf);
> > +	msg.buf = buf;
> > +
> > +	ret = i2c_transfer(chip->i2c_cl->adapter, &msg, 1);
> > +	if (ret != 1) {
> > +		dev_err(dev, "i2c write error, ret=%d\n", ret);
> 
> I2C
> 

I will update it.

> > +		ret = ret < 0 ? ret : -EIO;
> > +	} else {
> > +		ret = 0;
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> > +static int lp5812_read(struct lp5812_chip *chip, u16 reg, u8 *val)
> > +{
> > +	int ret;
> > +	u8 ret_val;  /* lp5812_chip return value */
> 
> /* Value returned by the hardware */   ?
> 
> > +	u8 extracted_bits; /* save 9th and 8th bit of reg address */
> > +	u8 converted_reg;  /* extracted 8bit from reg */
> 
> Place all of these comments in the code.
> 
> Saved and extracted for what purpose?  Tell us about the h/w.
> 

I will clarify the comments and move them next to the relevant code lines.

> > +	struct device *dev = &chip->i2c_cl->dev;
> > +	struct i2c_msg msgs[2];
> > +
> > +	extracted_bits = (reg >> 8) & 0x03;
> > +	converted_reg = (u8)(reg & 0xFF);
> > +
> > +	msgs[0].addr = (chip->i2c_cl->addr << 2) | extracted_bits;
> > +	msgs[0].flags = 0;
> > +	msgs[0].len = 1;
> > +	msgs[0].buf = &converted_reg;
> > +
> > +	msgs[1].addr = (chip->i2c_cl->addr << 2) | extracted_bits;
> > +	msgs[1].flags = I2C_M_RD;
> > +	msgs[1].len = 1;
> > +	msgs[1].buf = &ret_val;
> > +
> > +	ret = i2c_transfer(chip->i2c_cl->adapter, msgs, 2);
> > +	if (ret != 2) {
> > +		dev_err(dev, "Read reg value error, ret=%d\n", ret);
> 
> Why is this different to the one above?
> 
> "I2C read error" ?
> 

Agreed, I'll update the message to "I2C read error" for consistency.

> > +		*val = 0;
> > +		ret = ret < 0 ? ret : -EIO;
> > +	} else {
> > +		*val = ret_val;
> > +		ret = 0;
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> > +static int lp5812_parse_common_child(struct device_node *child,
> 
> 'child' here is Device Tree terminology.
> 
> Speak in terms of the hardware / device instead.

I'll rename child to led_node to better reflect the hardware context.

...
> > +static int lp5812_parse_single_led(struct device_node *np,
> > +				   struct lp5812_led_config *cfg,
> > +				   int child_number)
> > +{
> > +	int ret;
> > +
> > +	ret = lp5812_parse_common_child(np, cfg, child_number, 0);
> > +	if (ret)
> > +		return ret;
> > +
> > +	cfg[child_number].num_colors = 1;
> > +	cfg[child_number].is_sc_led = 1;
> > +
> > +	return 0;
> > +}
> 
> Wouldn't it make things twice as easy if we took the multi path every
> time and just make the color optional?
> 
> > +static int lp5812_parse_logical_led(struct device_node *np,
> > +				    struct lp5812_led_config *cfg,
> > +				    int child_number)
> > +{
> > +	int chan_nr, ret;
> > +
> > +	of_property_read_string(np, "label", &cfg[child_number].name);
> > +
> > +	ret = of_property_read_u32(np, "reg", &chan_nr);
> > +	if (ret)
> > +		return ret;
> > +
> > +	cfg[child_number].chan_nr = chan_nr;
> > +
> > +	if (of_node_name_eq(np, "multi-led"))
> > +		return lp5812_parse_multi_led(np, cfg, child_number);
> > +	else
> > +		return lp5812_parse_single_led(np, cfg, child_number);
> 
> Most devices have more than one LED and the supporting code can often
> handle one or more than one LEDs.  Why do we need to take two different
> paths here to handle this?
> 

Got it. I'll refactor to use a single path that handles both single and
multi-color LEDs.

> > +}
> > +
> > +static struct lp5812_data *lp5812_of_populate_pdata(struct device *dev,
> > +						    struct device_node *np,
> > +						    struct lp5812_chip *chip)
> > +{
> > +	struct lp5812_data *pdata;
> > +	struct lp5812_led_config *cfg;
> > +	int num_channels, i = 0, ret;
> > +
> > +	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
> 
> pdata is collated and passed IN from the platform device registering
> code.  Once we have it, it becomes device data, assuming we're not
> passing it forward to yet another child device?

I will refactor the code to store the data directly in the 'chip',
so there’s no need to allocate memory for pdata.

...
> > +static ssize_t lp5812_auto_time_pause_store(struct device *dev,
> > +					    struct device_attribute *attr,
> > +					    const char *buf, size_t len, bool start)
> > +{
> > +	struct lp5812_led *led;
> > +	struct lp5812_chip *chip;
> > +	struct lp5812_led_config *led_cfg;
> > +	const char *name = dev->platform_data;
> > +	u8 chan_nr = 0, curr_val;
> > +	int i, ret, val[LED_COLOR_ID_MAX];
> > +	char *sub_str, *str = (char *)buf;
> > +	u16 reg;
> > +
> > +	if (strcmp(name, LP5812_SC_LED) == 0)
> > +		led = dev_to_lp5812_led(dev);
> > +	else
> > +		led = dev_to_lp5812_led_mc(dev);
> 
> Put this whole thing in the helper above.

Agreed, I will move that logic into a helper to avoid duplication.

...
> > +struct lp5812_reg {
> > +	u16 addr;
> > +	union {
> > +		u8 val;
> > +		u8 mask;
> > +		u8 shift;
> > +	};
> > +};
> > +
> > +struct lp5812_led;
> 
> Move the definition here instead.
> 

Sure, I will move it.

> > +struct lp5812_device_config {
> > +	const struct lp5812_reg reg_reset;
> > +	const struct lp5812_reg reg_chip_en;
> > +	const struct lp5812_reg reg_dev_config_0;
> > +	const struct lp5812_reg reg_dev_config_1;
> > +	const struct lp5812_reg reg_dev_config_2;
> > +	const struct lp5812_reg reg_dev_config_3;
> > +	const struct lp5812_reg reg_dev_config_4;
> > +	const struct lp5812_reg reg_dev_config_5;
> > +	const struct lp5812_reg reg_dev_config_6;
> > +	const struct lp5812_reg reg_dev_config_7;
> > +	const struct lp5812_reg reg_dev_config_12;
> > +	const struct lp5812_reg reg_cmd_update;
> > +	const struct lp5812_reg reg_cmd_start;
> > +	const struct lp5812_reg reg_cmd_stop;
> > +	const struct lp5812_reg reg_cmd_pause;
> > +	const struct lp5812_reg reg_cmd_continue;
> > +
> > +	const struct lp5812_reg reg_led_en_1;
> > +	const struct lp5812_reg reg_led_en_2;
> > +	const struct lp5812_reg reg_fault_clear;
> > +	const struct lp5812_reg reg_manual_dc_base;
> > +	const struct lp5812_reg reg_auto_dc_base;
> > +	const struct lp5812_reg reg_manual_pwm_base;
> > +	const struct lp5812_reg reg_tsd_config_status;
> > +	const struct lp5812_reg reg_aeu_base;
> > +	const struct lp5812_reg reg_lod_status_base;
> > +	const struct lp5812_reg reg_lsd_status_base;
> > +
> > +	/* set LED brightness */
> > +	int (*brightness_fn)(struct lp5812_led *led);
> > +
> > +	/* set multicolor LED brightness */
> > +	int (*multicolor_brightness_fn)(struct lp5812_led *led);
> 
> Why do you need call-back functions for these?
> 

They are not needed at the moment.
I will remove the callbacks and use direct function calls instead.

> > +	/* additional device specific attributes */
> > +	const struct attribute_group *dev_attr_group;
> > +};
> > +
> > +struct lp5812_led_config {
> > +	const char *name;
> > +	int led_id[LED_COLOR_ID_MAX];
> > +	u8 color_id[LED_COLOR_ID_MAX];
> > +	u8 max_current[LED_COLOR_ID_MAX];
> > +	int num_colors;
> > +	u8 chan_nr;
> > +	bool is_sc_led;
> > +};
> > +
> > +struct lp5812_data {
> > +	struct lp5812_led_config *led_config;
> > +	u8 num_channels;
> > +	const char *label;
> > +};
> 
> How is data different from config here?
> 
> Config to me would be the parameters used to set-up a device.
> 

Based on the comment above regarding 'pdata', I'll merge lp5812_data
into lp5812_chip to simplify the design.

> > +struct lp5812_chip {
> > +	struct i2c_client *i2c_cl;
> 
> Just "client" is fine.
> 

Yes, I will rename it to 'client'.

> > +	struct mutex lock; /* Protects reg access */
> 
> "register"
> 

I will update it.

> > +	struct lp5812_data *pdata;
> > +	const struct lp5812_device_config *cfg;
> > +	union u_scan_order u_scan_order;
> > +	union u_drive_mode u_drive_mode;
> > +};
> > +
> > +struct lp5812_led {
> > +	int chan_nr;
> > +	struct led_classdev cdev;
> > +	struct led_classdev_mc mc_cdev;
> > +	u8 brightness;
> > +	struct lp5812_chip *chip;
> > +};
> > +
> > +#endif /*_LP5812_H_*/
> > -- 
> > 2.25.1

Thank you for the review and your time. I truly appreciate it!

Best regards,
Nam Tran

Powered by blists - more mailing lists