[PATCH V2 2/2] leds: bcm63xxx: add support for BCM63138 controller

[Rafał Miłecki]

On 15.12.2021 21:26, Pavel Machek wrote:
>> It's a new controller first introduced in BCM63138 SoC. Later it was
>> also used in BCM4908, some BCM68xx and some BCM63xxx SoCs.
>>
> 
>> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
>> index ed800f5da7d8..3bde795f0951 100644
>> --- a/drivers/leds/Kconfig
> 
> Lets put it into drivers/leds/blink/, please.
> 
>> --- /dev/null
>> +++ b/drivers/leds/leds-bcm63138.c
>> @@ -0,0 +1,314 @@
> 
>> +#define BCM63138_LED_BITS				4				/* how many bits control a single LED */
>> +#define BCM63138_LED_MASK				((1 << BCM63138_LED_BITS) - 1)	/* 0xf */
>> +#define BCM63138_LEDS_PER_REG				(32 / BCM63138_LED_BITS)	/* 8 */
> 
> I'm not sure these kinds of defines are useful.

What do you suggest? I think defines are prefered in Linux kernel
compared to magic values in code.


>> +static void bcm63138_leds_create_led(struct bcm63138_leds *leds,
>> +				     struct device_node *np)
>> +{
>> +	struct led_init_data init_data = {
>> +		.fwnode = of_fwnode_handle(np),
>> +	};
>> +	struct device *dev = leds->dev;
>> +	struct bcm63138_led *led;
>> +	struct pinctrl *pinctrl;
>> +	const char *state;
>> +	u32 bit;
>> +	int err;
>> +
>> +	led = devm_kzalloc(dev, sizeof(*led), GFP_KERNEL);
>> +	if (!led)
>> +		return;
> 
> At least warn. User wants to know why his leds don't work.

That would be against Linux's rule. I'm not sure if/where it's explained
in Documentation/ but scripts/checkpatch.pl will complain about that for
sure. OOM errors are loud enough not to require an extra error at driver
level.


>> +	if (!of_property_read_string(np, "default-state", &state)) {
>> +		if (!strcmp(state, "on"))
>> +			led->cdev.brightness = LED_FULL;
>> +		else
>> +			led->cdev.brightness = LED_OFF;
>> +	} else {
>> +		led->cdev.brightness = LED_OFF;
>> +	}
> 
> Do you actually need default-state support? Just remove it if not.
> 
> You support 4 bit brightness. You should set max_brightness to
> 15. LED_FULL is mistake (or very old API) in your case.