| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAHp75Vd4pwFXoF=xS5cskM2GDNb6c6RXTYo3j3FLP+nuQVoRyg@mail.gmail.com>
Date: Fri, 21 Jan 2022 17:46:30 +0200
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Florian Eckert <fe@....tdt.de>
Cc: Pavel Machek <pavel@....cz>, Rob Herring <robh+dt@...nel.org>,
Eckert.Florian@...glemail.com,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
Linux LED Subsystem <linux-leds@...r.kernel.org>,
devicetree <devicetree@...r.kernel.org>
Subject: Re: [PATCH v4 1/2] leds: ktd20xx: Extension of the KTD20xx family of
LED drivers from Kinetic
On Fri, Jan 21, 2022 at 4:01 PM Florian Eckert <fe@....tdt.de> wrote:
>
> Introducing the KTD2061/58/59/60 RGB LED drivers. The difference in
> these are the address numbers on the I2C bus that the device listens to.
>
> All KT20xx units can drive up to 12 LEDs.
>
> Due to the hardware limitation, we can only set 7 colors and the color
> black (LED off) for each LED independently and not the full RGB range.
> This is because the chip only has two color registers.
>
> To control the LEDs independently, the chip has to be configured in a
> special way.
>
> Color register 0 must be loaded with the current value 0mA, and color
> register 1 must be loaded with the value 'kinetic,led-current' from the
> device tree node. If the property is omitted, the register is loaded
> with the default value (0x28 = 5mA).
>
> To select a color for an LED, a combination must be written to the color
> selection register of that LED. This range for selecting the value is 3
> bits wide (RGB). A '0' in any of the bits uses color register '0' and a
> '1' uses color register '1'.
>
> So we could choose the following combination for each LED:
> R G B
> 0 0 0 = Black (off)
> 0 0 1 = Blue
> 0 1 0 = green
> 0 1 1 = Cyan
> 1 0 0 = Red
> 1 0 1 = Magenta
> 1 1 0 = Yellow
> 1 1 1 = White
...
> +/*
> + * LEDs driver for the Kinetic KDT20xx device
> + *
> + * Copyright (C) 2021 TDT AG Florian Eckert <fe@....tdt.de>
> + *
Redundant (blank) line.
> + */
...
> +static const struct reg_default ktd20xx_reg_defs[] = {
> + /* Color0 Configuration Registers */
> + {KTD20XX_IRED0, 0x28},
> + {KTD20XX_IGRN0, 0x28},
> + {KTD20XX_IBLU0, 0x28},
> + /* Color1 Configuration Registers */
> + {KTD20XX_IRED1, 0x60},
> + {KTD20XX_IGRN1, 0x60},
> + {KTD20XX_IBLU1, 0x60},
> + /* Selection Configuration Register */
> + {KTD20XX_ISELA12, 0x00},
> + {KTD20XX_ISELA34, 0x00},
> + {KTD20XX_ISELB12, 0x00},
> + {KTD20XX_ISELB34, 0x00},
> + {KTD20XX_ISELC12, 0x00},
> + {KTD20XX_ISELC34, 0x00}
+ Comma?
> +};
...
> + struct device *dev = &chip->client->dev;
> + int ret;
> + unsigned int value;
Here and everywhere can you use reverse xmas tree ordering?
struct device *dev = &chip->client->dev;
unsigned int value;
int ret;
...
> + /*
> + * If the device tree property 'kinetc,led-current' is found
> + * then set this value into the color0 register as the max current
> + * for all color channel LEDs. If this poperty is not set then
property
> + * use the default value 0x28 set by the chip after a hardware reset.
> + * The hardware default value 0x28 corresponds to 5mA.
> + */
...
> + set_bit(channel, &rgb);
__set_bit() will be sufficient here (no need an atomic version)
...
> + /*
> + * To use the color0 registers default value after an hadware reset,
hardware
> + * if the device tree property 'kinetc,led-current' is not set,
> + * we have to 'invert' the rgb channel!
> + */
...
> +unlock_out:
out_unlock is more usual, but it's up to you.
...
> + chip->regmap = devm_regmap_init_i2c(client, &ktd20xx_regmap_config);
> + if (IS_ERR(chip->regmap)) {
> + dev_err_probe(dev, PTR_ERR(chip->regmap),
> + "Failed to allocate register map\n");
> + goto error;
return dev_err_probe(...);
> + }
...
> + ret = regmap_field_read(chip->vendor, &vendor);
> + if (ret) {
> + dev_err_probe(dev, ret, "Failed to read vendor\n");
> + goto error;
Ditto.
> + }
> +
> + ret = regmap_field_read(chip->chip_id, &chip_id);
> + if (ret) {
> + dev_err_probe(dev, ret, "Failed to read chip id\n");
> + goto error;
Ditto,
> + }
> +
> + ret = regmap_field_read(chip->chip_rev, &chip_rev);
> + if (ret) {
> + dev_err_probe(dev, ret, "Failed to read chip rev\n");
> + goto error;
Ditto.
> + }
...
> + dev_info(dev, "vendor: 0x%02x chip-id: 0x%02x chip-rev: 0x%02x\n",
> + vendor, chip_id, chip_rev);
Why on the info level?
...
> + ret = ktd20xx_probe_dt(chip);
> + if (ret)
return ret;
> + goto error;
> +
> + ret = ktd20xx_hwinit(chip);
> + if (ret)
> + goto error;
Ditto.
...
> +error:
> + return ret;
Unneeded label, see above.
...
> +static struct i2c_driver ktd20xx_driver = {
> + .driver = {
> + .name = "ktd20xx",
> + .dev_groups = ktd20xx_led_controller_groups,
> + .of_match_table = of_ktd20xx_leds_match,
> + },
> + .probe = ktd20xx_probe,
Why not .probe_new?
> + .remove = ktd20xx_remove,
> + .id_table = ktd20xx_id
+ Comma.
> +};
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists