Messages in this thread | | | From | Andy Shevchenko <> | Date | Thu, 22 Feb 2018 00:20:43 +0200 | Subject | Re: [PATCH v4 2/2] leds: Add driver for NCP5623 3-channel I2C LED driver |
| |
On Wed, Feb 21, 2018 at 11:46 PM, Florian Vaussard <florian.vaussard@gmail.com> 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@gmail.com> > + * > + * 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
| |