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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Feb 28, 2023 at 11:11 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

I'm wondering if I already commented on the v1 of this. A lot of
issues with the code and your email rings a bell...
Okay, I have dug into archives and it was something else.

...

> +Date:          February 2023

Blast from the past? The best you can get is March 2023.

...

> +Description:   64-level DIM current. If write negative value or "auto",

If you write a

> +               the dim will be calculated according to the brightness.

...

> +config LEDS_AW200XX
> +       tristate "LED support for Awinic AW20036/AW20054/AW20072"
> +       depends on LEDS_CLASS
> +       depends on I2C
> +       help
> +         This option enables support for the AW20036/AW20054/AW20072 LED driver.
> +         It is a 3x12/6x9/6x12 matrix LED driver programmed via
> +         an I2C interface, up to 36/54/72 LEDs or 12/18/24 RGBs,
> +         3 pattern controllers for auto breathing or group dimming control.

What would be the name of the module if M?

...

> +/**

Is it a kernel doc?

> + * leds-aw200xx.c - Awinic AW20036/AW20054/AW20072 LED driver

No name of the file in the file. It's impractical (in case it will be
moved/renamed).

> + *
> + * Copyright (c) 2023, SberDevices. All Rights Reserved.
> + *
> + * Author: Martin Kurbanov <mmkurbanov@xxxxxxxxxxxxxx>
> + */

...

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

Can you keep this sorted?

...

> +#define AW200XX_DIM_MAX                  0x3F
> +#define AW200XX_FADE_MAX                 0xFF

If it is limited by the amount of bits in the bitfields, better to use
the form of (BIT(x) - 1), where x is the amount of bits.

...

> +#define AW200XX_IMAX_DEFAULT_MICROAMP    60000
> +#define AW200XX_IMAX_MAX_MICROAMP        160000
> +#define AW200XX_IMAX_MIN_MICROAMP        3300

A is the unit, and for microamperes we already use (in another
driver(s)) the _uA suffix.

...

> +/* Page 0 */
> +#define AW200XX_REG_PAGE0_BASE 0xc000

Indeed, like Pavel mentioned, why not consider the DRM approach for
this? If it's not really a display similar to LCD, then there is
drivers/auxdisplay folder for the non-standard / alphanumeric / etc
cases.

...

> +       (AW200XX_REG_PAGE0_BASE + ((page) * AW200XX_PAGE_SIZE) + (reg))

Multiplication doesn't require parentheses.

...

> +#define AW200XX_PAT_T3_LT_MASK      0xFF

> +#define AW200XX_PAT0_T3_LT_LSB(x)   ((x) & 0xFF)

GENMASK()

...

> +#define AW200XX_PAT0_T_LT_MAX       0xFFF

(BIT(12) - 1) ?

...

> +#define AW200XX_PAT_T1_T3_MASK      0xF0
> +#define AW200XX_PAT_T2_T4_MASK      0x0F

GENMASK()

...

> +#define AW200XX_TEMPLATE_TIME_MAX   0x0F

(BIT(4) - 1)

...

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

Something to use from units.h?

...

> +struct aw200xx_led {
> +       struct aw200xx *chip;

> +       struct led_classdev cdev;

Moving embedded structure to be the first member might make some code
to be no-op at compile time.

> +       int dim;
> +       u32 num;
> +};

...

> +       ssize_t ret;

Useless, just use return directly.

> +       if (dim < 0)
> +               ret = sysfs_emit(buf, "auto\n");
> +       else
> +               ret = sysfs_emit(buf, "%d\n", dim);
> +
> +       return ret;

if (dim >= 0)
  return sysfs_emit(...);

return sysfs_emit(...);

...

> +               ret = kstrtoint(buf, 0, &dim);
> +               if (ret < 0 || dim > AW200XX_DIM_MAX)
> +                       return -EINVAL;

Why shadowing ret?
Hint: it may not be EINVAL in some cases.

...

> +       if (dim >= 0) {

Hmm... Why not simply use an unsigned type?

> +       }

...

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

units.h ?

...

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

Ditto.

...

> +static int aw200xx_set_imax(const struct aw200xx *const chip,
> +                           u32 led_imax_microamp)
> +{
> +       struct imax_global {
> +               u32 regval;
> +               u32 microamp;
> +       } imaxs[] = {
> +               { 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 },

This looks a bit random. Is there any pattern on how value is
connected to the register value?

> +       };
> +       u32 g_imax_microamp = aw200xx_imax_to_global(chip, led_imax_microamp);
> +       int i;

int i = ARRAY_SIZE(...);

> +       for (i = ARRAY_SIZE(imaxs) - 1; i >= 0; i--) {

while (i--) {

> +               if (g_imax_microamp >= imaxs[i].microamp)
> +                       break;
> +       }

> +

Redundant blank line.

> +       if (i < 0)
> +               return -EINVAL;
> +
> +       return regmap_update_bits(chip->regmap, AW200XX_REG_GCCR,
> +                                 AW200XX_GCCR_IMAX_MASK,
> +                                 AW200XX_GCCR_IMAX(imaxs[i].regval));
> +}

...

> +       ret = regmap_write(chip->regmap, AW200XX_REG_FCD, AW200XX_FCD_CLEAR);
> +
> +       return ret;

return regmap_write(...);

...

> +       ret = regmap_update_bits(chip->regmap, AW200XX_REG_GCCR,
> +                                AW200XX_GCCR_ALLON, AW200XX_GCCR_ALLON);
> +
> +       return ret;

Ditto.

...

> +       ret = aw200xx_set_imax(chip, min_microamp);
> +
> +       return ret;

Ditto.

...

> +       chip = devm_kzalloc(&client->dev,
> +                           struct_size(chip, leds, count),
> +                           GFP_KERNEL);

There is a lot of room on the previous lines.

> +       if (!chip)
> +               return -ENOMEM;

...

> +static const struct of_device_id __maybe_unused aw200xx_match_table[] = {
> +       { .compatible = "awinic,aw20036", .data = &aw20036_cdef, },
> +       { .compatible = "awinic,aw20054", .data = &aw20054_cdef, },
> +       { .compatible = "awinic,aw20072", .data = &aw20072_cdef, },
> +       {},

Comma is not needed for the terminator entry.

> +};

...

> +static struct i2c_driver aw200xx_driver = {
> +       .driver = {
> +               .name = "aw200xx",
> +               .of_match_table = of_match_ptr(aw200xx_match_table),

Why of_match_ptr()? It's a very rare case when you really need this.

> +       },
> +       .probe_new = aw200xx_probe,
> +       .remove = aw200xx_remove,
> +       .id_table = aw200xx_id,
> +};

> +

Redundant blank line.

> +module_i2c_driver(aw200xx_driver);

...

> +MODULE_ALIAS("platform:leds-aw200xx");

What is this?! Or i.o.w. why is this violation of the subsystems?


--
With Best Regards,
Andy Shevchenko




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux