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

[Pavel Machek <pavel@ucw.cz>]

[-- Attachment #1: Type: text/plain, Size: 1733 bytes --]

Hi!

> 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.

> +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.

> +	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.

Otherwise looks quite sane.

Thank you,
								Pavel
-- 
http://www.livejournal.com/~pavelmachek

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]