Re: [PATCH v3 2/2] leds: pca995x: Add support for PCA995X chips

[Marek Vasut <marex@denx.de>]

On 7/13/23 16:07, Lee Jones wrote:

[...]

>> +#define PCA995X_LDRX_MASK	0x3	/* 2 bits per output state control */
>> +
>> +#define PCA995X_MODE_1_CFG	0x00	/* Auto-increment disabled. Normal mode */
> 
> Is this not the reset value?

The chip may not be power-cycled on reboot, so this register is not 
necessarily 0x0.

>> +#define PCA995X_IREFALL_CFG	0x7F	/* Half of max current gain multiplier */
> 
> PCA995X_IREFALL_FULL_CFG	0xFF
> PCA995X_IREFALL_HALF_CFG	(PCA995X_IREFALL_MAX_CFG / 2)
> 
> ?
> 
>> +#define PCA995X_MAX_OUTPUTS	16	/* Supported outputs */
> 
> The define nomenclature is clear enough.
> 
>> +#define ldev_to_led(c)	container_of(c, struct pca995x_led, ldev)
>> +
>> +struct pca995x_led {
>> +	unsigned int led_no;
>> +	struct led_classdev ldev;
>> +	struct pca995x_chip *chip;
>> +};
>> +
>> +struct pca995x_chip {
>> +	struct regmap *regmap;
>> +	struct pca995x_led leds[PCA995X_MAX_OUTPUTS];
>> +	int btype;
>> +};
> 
> It's weird to have 2 structs reference each other.
> 
> This should not be allowed:
> 
> pca995x_led->pca995x_chip->pca995x_led->pca995x_chip->pca995x_led->pca995x_chip
> 
> Some implementation of container_of() should do the trick.

container_of() wouldn't work here because struct pca995x_chip contains 
array of LEDs .

The structure layout is taken from:
8325642d2757e ("leds: bd2606mvv: Driver for the Rohm 6 Channel i2c LED 
driver")

>> +static int pca995x_brightness_set(struct led_classdev *led_cdev,
>> +				  enum led_brightness brightness)
>> +{
>> +	struct pca995x_led *led = ldev_to_led(led_cdev);
>> +	struct pca995x_chip *chip = led->chip;
>> +	u8 ledout_addr, pwmout_addr;
>> +	int shift, ret;
>> +
>> +	pwmout_addr = (chip->btype ? PCA9955B_PWM0 : PCA9952_PWM0) + led->led_no;
> 
> All this btype stuff is a little crazy and totally not scalable.

That's right, but until NXP changes the register layout again, this 
should be sufficient to support the entire 995x line up.

>> +	ledout_addr = PCA995X_LEDOUT0 + (led->led_no / 4);
>> +	shift = 2 * (led->led_no % 4);
> 
> Why 2 and 4?  I suggest that they are defined.
> 
>> +	switch (brightness) {
>> +	case LED_FULL:
> 
> https://elixir.bootlin.com/linux/latest/source/include/linux/leds.h#L32

LED_HALF is just PWM (default) mode, set to half duty cycle .

>> +		return regmap_update_bits(chip->regmap, ledout_addr,
>> +					  PCA995X_LDRX_MASK << shift,
>> +					  PCA995X_LED_ON << shift);
>> +	case LED_OFF:
>> +		return regmap_update_bits(chip->regmap, ledout_addr,
>> +					  PCA995X_LDRX_MASK << shift, 0);
>> +	default:
> 
> Are there no invalid values here?

Not to my knowledge, this should be 0..255 clamped by 
ldev.max_brightness further down.

>> +	/* Disable LED all-call address and set normal mode */
>> +	ret = regmap_write(chip->regmap, PCA995X_MODE1, PCA995X_MODE_1_CFG);
> 
> The formatting of "MODE1" and "MODE_1" is making me twitch!
> 
> Is this how they are referenced in the datasheet?

It's called MODE1 in datasheet, fixed.

>> +	if (ret)
>> +		return ret;
>> +
>> +	/* IREF Output current value for all LEDn outputs */
>> +	return regmap_write(chip->regmap,
>> +			    btype ? PCA9955B_IREFALL : PCA9952_IREFALL,
>> +			    PCA995X_IREFALL_CFG);
>> +}
>> +
>> +static const struct i2c_device_id pca995x_id[] = {
>> +	{ "pca9952", .driver_data = (kernel_ulong_t)0 /* non-B chip */ },
> 
> Defines at least please.
> 
> Are you sure these are the only 2 types of chip this driver will
> support?

No, I cannot predict the future or how NXP will or won't change the 
register layout of it. When it cames, we can deal with that.

[...]

The rest is addressed in V4.