Re: [PATCH] leds: Add DT support for leds-pca9532

[Arnd Bergmann]

On Tuesday 09 September 2014 15:28:00 Roland Stigge wrote:
> +#ifdef CONFIG_OF
> +static struct pca9532_platform_data *pca9532_parse_dt(struct device *dev)
> +{
...
> +
>  static int pca9532_probe(struct i2c_client *client,
>         const struct i2c_device_id *id)
>  {
> @@ -449,6 +489,11 @@ static int pca9532_probe(struct i2c_clie
>         struct pca9532_platform_data *pca9532_pdata =
>                         dev_get_platdata(&client->dev);
>  
> +#ifdef CONFIG_OF
> +       if (!pca9532_pdata && client->dev.of_node)
> +               pca9532_pdata = pca9532_parse_dt(&client->dev);
> +#endif
> +

It would be better to remove the two #ifdef statements and instead
do this in C language as

	if (IS_ENABLED(CONFIG_OF) && !pca9532_pdata && client->dev.of_node)
		pca9532_pdata = pca9532_parse_dt(&client->dev);

This gives you extra compile-time coverage when CONFIG_OF is not
enabled.

I would probably also remove the !pca9532_pdata check there, since
we rarely do auxdata for new devices these days, but that is a separate
matter, and the check does not hurt.

> +       if (!of_property_read_u32_array(np, "nxp,pwm", &pwm[0], 2)) {
> +               for (i = 0; i < 2; i++)
> +                       pca9532_pdata->pwm[i] = pwm[i];
> +       }

Is this a full PWM controller? If it is, it would be better to also
provide a #pwm-cells property and register the device with the PWM
subsystem to allow arbitrary users.

If the PWM is only really usable for LED, it's probably enough to
leave this used by the LED subsystem.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/