Re: [PATCH v3 2/2] leds: add aw20xx driver

[Andy Shevchenko]

On Tue, Mar 14, 2023 at 2:03 PM Martin Kurbanov
<mmkurbanov@xxxxxxxxxxxxxx> wrote:
>
> This commit adds support for AWINIC AW20036/AW20054/AW20072 LED driver.
> This driver supports following AW200XX features:
>   - Individual 64-level DIM currents

As said, please give a chance reviewer to answer your questions before
issuing a new version. You will save yours time in the first place and
reviewer's and others' at the second.


...

> +config LEDS_AW200XX
> +       tristate "LED support for Awinic AW20036/AW20054/AW20072"
> +       depends on LEDS_CLASS
> +       depends on I2C

> +       depends on OF

I don't see any dependency. Do you?

...

> +#include <linux/bitfield.h>

bits.h needs to be included, it's not guaranteed to be by any of the
present headers in this list.

> +#include <linux/i2c.h>
> +#include <linux/leds.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/regmap.h>

+ container_of.h

...

> +#define AW200XX_PAT0_T2_LT_MSB(x)   ((x) >> 8)
> +#define AW200XX_PAT0_T3_LT_LSB(x)   ((x) & 0xFF)
> +#define AW200XX_PAT0_T_LT(msb, lsb) ((msb) << 8 | (lsb))

Are these being in use anyhow?

Please, do not add dead definitions if they are not currently in use
and do not add value to understanding how hardware works for the
implemented things.

...

> +/* Patern selection register*/

Pattern

...

> +/* Duty ratio of display scan (see p.15 of datasheet for formula) */
> +#define AW200XX_DUTY_RATIO(rows) \
> +       (((592UL * 1000000UL) / 600500UL) * (1000UL / (rows)) / 1000UL)

Instead of referring to a page in the datasheet, quote the formula and
a bit of text.

So, now as I read it I can tell how it can be improved.
The first six 0:s multiplier is actually USEC_PER_SEC in the kernel.

The 1000 is your addition which needs to be explained. It may be just
MILLI or KILO depending on what you have in mind with it.

...

> +struct aw200xx {
> +       const struct aw200xx_chipdef *cdef;
> +       struct i2c_client *client;

> +       struct regmap *regmap;

You may derive this from &client->dev, right? The API dev_get_regmap().

> +       struct mutex mutex;
> +       u32 num_leds;
> +       u32 display_rows;
> +       struct aw200xx_led leds[];
> +};

...

> +       if (sysfs_streq(buf, "auto")) {
> +               dim = -1;
> +       } else {
> +               ret = kstrtoint(buf, 0, &dim);
> +               if (ret)
> +                       return ret;
> +
> +               if (dim > AW200XX_DIM_MAX)
> +                       return -EINVAL;

And if dim is < 0, does ir mean "auto"? If so, is it documented?

> +       }

...

> +static DEVICE_ATTR(dim, 0644, aw200xx_dim_show, aw200xx_dim_store);

DEVICE_ATTR_RW() ?

...

> +       mutex_lock(&chip->mutex);
> +
> +       reg = AW200XX_REG_DIM(led->num, chip->cdef->display_size_columns);

> +       dim = led->dim;
> +

This blank line should go before a dim assignment.

> +       if (dim < 0) {

> +               dim = brightness / (AW200XX_FADE_MAX / AW200XX_DIM_MAX);
> +               dim = max(dim, 1);

Can it be written in a single assignment?

  dim = max(brightness / (AW200XX_FADE_MAX / AW200XX_DIM_MAX), 1)

> +       }

...

> +error:

In one function it's called the exit, here it's the error, please be
consistent and I think the more precise is to use something like
'out_unlock'.

> +       mutex_unlock(&chip->mutex);
> +
> +       return ret;
> +}

...

> +       /* The output current of each LED (see p.14 of datasheet for formula) */

Again, put more comments. Do not push the reader to find a Datasheet
which I do not see the link to in the commit message.

> +       return (duty * global_imax_uA) / 1000U;

So, this seems like the same meaning as 1000 in the macro, correct?
Please explain the use of it.
The best, if you think units.h doesn't provide it, define in your driver.

...

> +       /* The output current of each LED (see p.14 of datasheet for formula) */
> +       return (led_imax_uA * 1000U) / duty;

All the same as per above.

...

> +       {
> +               { 8,  3300 },
> +               { 9,  6700 },
> +               { 0,  10000 },
> +               { 11, 13300 },
> +               { 1,  20000 },
> +               { 13, 26700 },
> +               { 2,  30000 },
> +               { 3,  40000 },
> +               { 15, 53300 },
> +               { 4,  60000 },
> +               { 5,  80000 },
> +               { 6,  120000 },
> +               { 7,  160000 },
> +       };

Yeah, as per previous email, use exactly what Datasheet provides you.
There is a clear pattern and clear math mistakes in the datasheet
itself.

...

> +static const struct regmap_config aw200xx_regmap_config = {
> +       .reg_bits = 8,
> +       .val_bits = 8,
> +       .max_register = AW200XX_REG_MAX,
> +       .ranges = aw200xx_ranges,
> +       .num_ranges = ARRAY_SIZE(aw200xx_ranges),
> +       .rd_table = &aw200xx_readable_table,
> +       .wr_table = &aw200xx_writeable_table,
> +       .volatile_table = &aw200xx_volatile_table,
> +       .cache_type = REGCACHE_RBTREE,

You have a mutex, why do you need to use regmap's lock?

> +};

...

> +       cdef = device_get_match_data(&client->dev);

Is it fine if it's NULL?

...

> +       chip->cdef = cdef;

Can it be assigned directly here w.o temporary variable?

--
With Best Regards,
Andy Shevchenko