Re: [PATCH v4 2/2] leds: Add driver for NCP5623 3-channel I2C LED driver

[Andy Shevchenko]

On Wed, Feb 21, 2018 at 11:46 PM, Florian Vaussard
<florian.vaussard@xxxxxxxxx> wrote:
> The NCP5623 is a 3-channel LED driver from On Semiconductor controlled
> through I2C. The PWM of each channel can be independently set with 32
> distinct levels. In addition, the intensity of the current source can be
> globally set using an external bias resistor fixing the reference
> current (Iref) and a dedicated register (ILED), following the
> relationship:
>
> I = 2400*Iref/(31-ILED)
>
> with Iref = Vref/Rbias, and Vref = 0.6V.

First of all, why it's rebased on top of v4.15 while we have v4.16-rc2 already?

> +/*
> + * Copyright 2018 Florian Vaussard <florian.vaussard@xxxxxxxxx>
> + *
> + * SPDX-License-Identifier: GPL-2.0
> + */

Check license-rules.rst.


> +#define NCP5623_CMD_SHIFT      5
> +#define CMD_SHUTDOWN           (0x00 << NCP5623_CMD_SHIFT)
> +#define CMD_ILED               (0x01 << NCP5623_CMD_SHIFT)
> +#define CMD_PWM1               (0x02 << NCP5623_CMD_SHIFT)
> +#define CMD_PWM2               (0x03 << NCP5623_CMD_SHIFT)
> +#define CMD_PWM3               (0x04 << NCP5623_CMD_SHIFT)
> +#define CMD_UPWARD_DIM         (0x05 << NCP5623_CMD_SHIFT)
> +#define CMD_DOWNWARD_DIM       (0x06 << NCP5623_CMD_SHIFT)
> +#define CMD_DIM_STEP           (0x07 << NCP5623_CMD_SHIFT)

Just put 5 instead. My eyes hurts by above.

> +#define LED_TO_PWM_CMD(led)    ((0x02 + led - 1) << NCP5623_CMD_SHIFT)

led -> (led)

> +
> +#define NCP5623_DATA_MASK      GENMASK(NCP5623_CMD_SHIFT - 1, 0)

While here is OK.

> +static struct ncp5623_led *ldev_to_led(struct led_classdev *ldev)

More usual pattern is
to_ncp5623_led()

> +       return (err < 0 ? err : 0);

Where this style comes from?
I mean redundant parens.

> +       err = of_property_read_u32(np, "onnn,led-iref-microamp",
> +                                  &priv->led_iref);

Don't we have some generic property for iref:s?

> +static int ncp5623_probe(struct i2c_client *client,
> +                        const struct i2c_device_id *id)
> +{
> +       struct device *dev = &client->dev;
> +       struct device_node *np = dev->of_node;
> +       struct ncp5623_priv *priv;
> +       int err;
> +
> +       priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +       if (!priv)
> +               return -ENOMEM;
> +
> +       priv->client = client;
> +       i2c_set_clientdata(client, priv);
> +
> +       err = ncp5623_parse_dt(priv, np);
> +       if (err)
> +               return err;
> +

> +       return ncp5623_configure(dev, priv);

This sounds wrong.
You call configure while it does two unrelated things: configure + registration.

Name it properly and move closer to ->probe().

> +static const struct i2c_device_id ncp5623_id[] = {
> +       { "ncp5623" },
> +       {},
> +};
> +MODULE_DEVICE_TABLE(i2c, ncp5623_id);

Do you really need this?
Consider to switch to ->probe_new().

> +               .of_match_table = of_match_ptr(ncp5623_of_match),

Here you will get a compiler warning.
Remove this pointless of_match_ptr().

-- 
With Best Regards,
Andy Shevchenko