leds: add ktd20xx LED driver support
Subsystems (2)
v4 cover leds: add ktd20xx LED driver support
v1: Initial send
v2: Remove variant 1 from source
v3: Changes requested by Andy Shevchenko added. Thanks for reviewing
- Removing OF dependency
- Add missing includes
- Use device_property_read_u32() instead of fwnode_property_read_u32()
- Use one liner function pattern <test> ? <value-true> : <value-false>
- Remove switch case call for intensity color selection use BIT() instead
- Remove not needed fwnode_handle_put() in ktd200xx_probe_dt() function
- Use dev_get_drvdata() instead of i2c_get_clientdata() function call
- Use sysfs_emit() function call
- Use kstrtobool() function call
- Remove not needed comma after last array element
- Use dev_err_probe() instead of dev_error() in driver probe function
- Do not use dev_group registration function set .dev_groups directly
into ktd20xx_driver struct.
v4: Changes requested by Andy Shevchenko. Thanks again for your review
- Fix Author indentation
- Reduce logging noise
- Use 'if' standard pattern
- Use set_bit function to make code cleaner
- Use meaningful jump labels
- Updating the logging output to better match the source code
- Remove duplicate dev pointer usage. This is not necessary as the information can be used directly from the client structure
- Do not hide return value from kstrbool function
- Do not use mutex_destroy function in devm mananged structs
Florian Eckert (2):
leds: ktd20xx: Extension of the KTD20xx family of LED drivers from
Kinetic
dt: bindings: KTD20xx: Introduce the ktd20xx family of RGB drivers
.../bindings/leds/leds-ktd20xx.yaml | 130 ++++ MAINTAINERS | 7 + drivers/leds/Kconfig | 12 + drivers/leds/Makefile | 1 + drivers/leds/leds-ktd20xx.c | 582 ++++++++++++++++++ 5 files changed, 732 insertions(+) create mode 100644 Documentation/devicetree/bindings/leds/leds-ktd20xx.yaml create mode 100644 drivers/leds/leds-ktd20xx.c
Signature · 1 line
2.20.1
PATCH v4 1/2 leds: ktd20xx: Extension of the KTD20xx family of LED drivers from Kinetic
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
--- MAINTAINERS | 6 + drivers/leds/Kconfig | 12 + drivers/leds/Makefile | 1 + drivers/leds/leds-ktd20xx.c | 582 ++++++++++++++++++++++++++++++++++++ 4 files changed, 601 insertions(+) create mode 100644 drivers/leds/leds-ktd20xx.c
MAINTAINERS +6 -0
@@ -10739,6 +10739,12 @@ S: Maintained F: Documentation/devicetree/bindings/leds/backlight/kinetic,ktd253.yaml F: drivers/video/backlight/ktd253-backlight.c +KTD20XX LED CONTROLLER DRIVER +M: Florian Eckert <[email protected]> +L: [email protected] +S: Maintained +F: drivers/leds/leds-ktd20xx.c + KTEST M: Steven Rostedt <[email protected]> M: John Hawley <[email protected]>
drivers/leds/Kconfig +12 -0
@@ -157,6 +157,18 @@ config LEDS_EL15203000 To compile this driver as a module, choose M here: the module will be called leds-el15203000. +config LEDS_KTD20XX + tristate "LED Support for KTD2061/58/59/60 LED driver chip" + depends on LEDS_CLASS && I2C + depends on LEDS_CLASS_MULTICOLOR + select REGMAP_I2C + help + If you say yes here you get support for the Kinetic + KTD2061, KTD2058, KTD2059 and KTD2060 LED driver. + + To compile this driver as a module, choose M here: the + module will be called leds-ktd20xx. + config LEDS_TURRIS_OMNIA tristate "LED support for CZ.NIC's Turris Omnia" depends on LEDS_CLASS_MULTICOLOR
drivers/leds/Makefile +1 -0
@@ -34,6 +34,7 @@ obj-$(CONFIG_LEDS_IP30) += leds-ip30.o obj-$(CONFIG_LEDS_IPAQ_MICRO) += leds-ipaq-micro.o obj-$(CONFIG_LEDS_IS31FL319X) += leds-is31fl319x.o obj-$(CONFIG_LEDS_IS31FL32XX) += leds-is31fl32xx.o +obj-${CONFIG_LEDS_KTD20XX} += leds-ktd20xx.o obj-$(CONFIG_LEDS_LM3530) += leds-lm3530.o obj-$(CONFIG_LEDS_LM3532) += leds-lm3532.o obj-$(CONFIG_LEDS_LM3533) += leds-lm3533.o
drivers/leds/leds-ktd20xx.c new file +582 -0
582 lines not on this page · raw message
Signature · 1 line
2.20.1
Re: [PATCH v4 1/2] leds: ktd20xx: Extension of the KTD20xx family of LED drivers from Kinetic
Quoted 33 lines · Florian Eckert
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
...
Quoted 6 lines
+/*
+ * LEDs driver for the Kinetic KDT20xx device
+ *
+ * Copyright (C) 2021 TDT AG Florian Eckert <[email protected]>+ *
Redundant (blank) line.
Quoted 1 line
+ */
...
Quoted 7 lines
drivers/leds/leds-ktd20xx.c new file +7
-0+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 },
drivers/leds/leds-ktd20xx.c line 88
Quoted 10 lines
drivers/leds/leds-ktd20xx.c new file +9
-0+ {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?
Quoted 1 line
+};
...
drivers/leds/leds-ktd20xx.c line 194
Quoted 3 lines
drivers/leds/leds-ktd20xx.c new file +3
-0+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;...
drivers/leds/leds-ktd20xx.c line 199
Quoted 4 lines
drivers/leds/leds-ktd20xx.c new file +4
-0+/* +* 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
Quoted 3 lines
drivers/leds/leds-ktd20xx.c new file +3
-0+* use the default value 0x28 set by the chip after a hardware reset. +* The hardware default value 0x28 corresponds to 5mA. +*/
...
Quoted 1 line
+
set_bit(channel, &rgb);
__set_bit() will be sufficient here (no need an atomic version)
...
Review
Quoted 2 lines
(quoted diff) +2
-0+ /* + * To use the color0 registers default value after an hadware reset,
hardware
Quoted 3 lines
drivers/leds/leds-ktd20xx.c new file +3
-0+* if the device tree property 'kinetc,led-current' is not set, +* we have to 'invert' the rgb channel! +*/
...
Quoted 1 line
+unlock_out:
out_unlock is more usual, but it's up to you.
...
drivers/leds/leds-ktd20xx.c line 493
Quoted 6 lines
drivers/leds/leds-ktd20xx.c new file +5
-0+ 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(...);
Quoted 1 line
+ }
...
drivers/leds/leds-ktd20xx.c line 513
Quoted 5 lines
drivers/leds/leds-ktd20xx.c new file +4
-0+ ret =regmap_field_read (chip->vendor, &vendor); +if (ret) { +dev_err_probe (dev, ret,"Failed to read vendor\n" ); + goto error;
Ditto.
drivers/leds/leds-ktd20xx.c line 519
Quoted 7 lines
drivers/leds/leds-ktd20xx.c new file +6
-0+ } + + 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,
drivers/leds/leds-ktd20xx.c line 525
Quoted 6 lines
drivers/leds/leds-ktd20xx.c new file +6
-0+ } + + 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.
Quoted 1 line
+ }
...
drivers/leds/leds-ktd20xx.c line 529
Quoted 2 lines
drivers/leds/leds-ktd20xx.c new file +2
-0+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?
...
Review
Quoted 2 lines
(quoted diff) +2
-0+ ret = ktd20xx_probe_dt(chip); + if (ret)
return ret;
drivers/leds/leds-ktd20xx.c line 537
Quoted 5 lines
drivers/leds/leds-ktd20xx.c new file +5
-0+ goto error; + + ret =ktd20xx_hwinit (chip); +if (ret) + goto error;
Ditto.
...
drivers/leds/leds-ktd20xx.c line 542
Quoted 2 lines
drivers/leds/leds-ktd20xx.c new file +2
-0+error: +return ret;
Unneeded label, see above.
...
drivers/leds/leds-ktd20xx.c line 574
Quoted 8 lines
drivers/leds/leds-ktd20xx.c new file +7
-0+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?
drivers/leds/leds-ktd20xx.c line 576
Quoted 3 lines
drivers/leds/leds-ktd20xx.c new file +2
-0+ .remove = ktd20xx_remove, + .id_table = ktd20xx_id
+ Comma.
Quoted 1 line
+};
Signature · 2 lines
With Best Regards, Andy Shevchenko
Re: [PATCH v4 1/2] leds: ktd20xx: Extension of the KTD20xx family of LED drivers from Kinetic
Hi!
Quoted 9 lines
drivers/leds/leds-ktd20xx.c line 194
Quoted 3 lines
drivers/leds/leds-ktd20xx.c new file +3
-0+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;
Lets not ask people to do that.
Best regards,
Pavel
Signature · 1 line
http://www.livejournal.com/~pavelmachek
Re: [PATCH v4 1/2] leds: ktd20xx: Extension of the KTD20xx family of LED drivers from Kinetic
Hello Pavel,
Haven't seen the new ktd20xx for the next [1] kernel release yet. Do I still have to do something?
Best regards,
Florian Eckert
[1] https://git.kernel.org/pub/scm/linux/kernel/git/pavel/linux-leds.git/log/?h=for-next
kernel test robot
build botRe: [PATCH v4 1/2] leds: ktd20xx: Extension of the KTD20xx family of LED drivers from Kinetic
In reply to Florian EckertI love your patch! Perhaps something to improve:
Hi Florian,
I love your patch! Perhaps something to improve:
[auto build test WARNING on pavel-leds/for-next]
[also build test WARNING on robh/for-next linux/master linus/master v5.17-rc1 next-20220124] [If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Florian-Eckert/leds-add-ktd20xx-LED-driver-support/20220121-220338 base: git://git.kernel.org/pub/scm/linux/kernel/git/pavel/linux-leds.git for-next config: i386-allmodconfig (https://download.01.org/0day-ci/archive/20220125/202201251140.YNV7J1Ot-lkp(a)intel.com/config) compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project 2e58a18910867ba6795066e044293e6daf89edf5)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/8e24c9cc5b5a807f40eec9cc643c2ea43f75e8a3
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Florian-Eckert/leds-add-ktd20xx-LED-driver-support/20220121-220338
git checkout 8e24c9cc5b5a807f40eec9cc643c2ea43f75e8a3
# save the config file to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash arch/x86/kvm/ drivers/leds/
If you fix the issue, kindly add following tag as appropriate
All warnings (new ones prefixed by >>):
>> drivers/leds/leds-ktd20xx.c:490:6: warning: variable 'ret' is used uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized]
if (IS_ERR(chip->regmap)) {
^~~~~~~~~~~~~~~~~~~~
drivers/leds/leds-ktd20xx.c:542:9: note: uninitialized use occurs here
return ret;
^~~
drivers/leds/leds-ktd20xx.c:490:2: note: remove the 'if' if its condition is always false
if (IS_ERR(chip->regmap)) {
^~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/leds/leds-ktd20xx.c:478:9: note: initialize the variable 'ret' to silence this warning
int ret;
^
= 01 warning generated.
vim +490 drivers/leds/leds-ktd20xx.c
469
470 static int ktd20xx_probe(struct i2c_client *client,
471 const struct i2c_device_id *id)
472 {
473 struct ktd20xx *chip;
474 unsigned int vendor;
475 unsigned int chip_id;
476 unsigned int chip_rev;
477 struct device *dev;
478 int ret;
479
480 chip = devm_kzalloc(&client->dev, sizeof(*chip), GFP_KERNEL);
481 if (!chip)
482 return -ENOMEM;
483
484 mutex_init(&chip->lock);
485 chip->client = client;
486 dev = &client->dev;
487 i2c_set_clientdata(client, chip);
488
489 chip->regmap = devm_regmap_init_i2c(client, &ktd20xx_regmap_config);
> 490 if (IS_ERR(chip->regmap)) { 491 dev_err_probe(dev, PTR_ERR(chip->regmap),
492 "Failed to allocate register map\n");
493 goto error;
494 }
495
496 chip->control_mode = devm_regmap_field_alloc(dev, chip->regmap,
497 kt20xx_control_mode);
498 chip->faderate = devm_regmap_field_alloc(dev, chip->regmap,
499 kt20xx_faderate);
500 chip->vendor = devm_regmap_field_alloc(dev, chip->regmap,
501 kt20xx_vendor);
502 chip->chip_id = devm_regmap_field_alloc(dev, chip->regmap,
503 kt20xx_chip_id);
504 chip->chip_rev = devm_regmap_field_alloc(dev, chip->regmap,
505 kt20xx_chip_rev);
506
507 /* Reset all registers to hardware device default settings */
508 regmap_field_write(chip->control_mode, CONTROL_MODE_RESET);
509
510 ret = regmap_field_read(chip->vendor, &vendor);
511 if (ret) {
512 dev_err_probe(dev, ret, "Failed to read vendor\n");
513 goto error;
514 }
515
516 ret = regmap_field_read(chip->chip_id, &chip_id);
517 if (ret) {
518 dev_err_probe(dev, ret, "Failed to read chip id\n");
519 goto error;
520 }
521
522 ret = regmap_field_read(chip->chip_rev, &chip_rev);
523 if (ret) {
524 dev_err_probe(dev, ret, "Failed to read chip rev\n");
525 goto error;
526 }
527
528 dev_info(dev, "vendor: 0x%02x chip-id: 0x%02x chip-rev: 0x%02x\n",
529 vendor, chip_id, chip_rev);
530
531 ret = ktd20xx_probe_dt(chip);
532 if (ret)
533 goto error;
534
535 ret = ktd20xx_hwinit(chip);
536 if (ret)
537 goto error;
538
539 return 0;
540
541 error:
542 return ret;
543 }
544
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
PATCH v4 2/2 dt: bindings: KTD20xx: Introduce the ktd20xx family of RGB drivers
In reply to Florian EckertIntroduce the bindings for the Kinetic KTD2061/58/59/60RGB LED device driver. The KTD20xx can control RGB LEDs individually. Because of the hardware limitations, only 7 colors and the color black (off) can be set.
--- .../bindings/leds/leds-ktd20xx.yaml | 130 ++++++++++++++++++ MAINTAINERS | 1 + 2 files changed, 131 insertions(+) create mode 100644 Documentation/devicetree/bindings/leds/leds-ktd20xx.yaml
Documentation/devicetree/bindings/leds/leds-ktd20xx.yaml new file +130 -0
@@ -0,0 +1,130 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/leds/leds-ktd20xx.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: LED driver for KTD20xx RGB LED from Kinetic. + +maintainers: + - Florian Eckert <[email protected]> + +description: | + The KTD20XX is multi-channel, I2C LED driver. Into can control up to 12 + LEDs per device. The RGB value can be set for each LED. Due to hardware + limitations, the full RGB range cannot be used. Only 7 colors and the + color black can be set (black means off). + 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 + +properties: + compatible: + enum: + - kinetic,ktd20xx + + reg: + maxItems: 1 + description: + I2C slave address + ktd2061/58/59/60 0x68 0x69 0x6A 0x6B + + '#address-cells': + const: 1 + + '#size-cells': + const: 0 + + 'kinetic,led-current': + $ref: /schemas/types.yaml#/definitions/uint8 + description: + This value is a current setting for all LEDs connected to this device. + If this value is not set then the default value off 0x28 (5mA) is set. + This means all LEDs get 5mA. The max value is 24mA. We have the + following mapping in 125uA steps. We can set a maximum of 24mA. + 0000 0000 (0x00) = 0uA + 0000 0001 (0x01) = 125uA + .... .... + 0010 1000 (0x28) = 5mA + .... .... + 1100 0000 (0xC0) = 24mA + 1100 0001 (0xC1) = 24mA + .... .... + 1111 1111 (0xFF) = 24mA + minimum: 0 + maximum: 255 + +patternProperties: + '^multi-led@[0-9a-f]$': + type: object + allOf: + - $ref: leds-class-multicolor.yaml# + description: + This node represents one of the Multicolor LED. No subnodes need to + be added for subchannels since this controller only supports 1bit + RGB values. We could display seven different colors and the color + black which means off. + + properties: + reg: + minimum: 0 + maximum: 11 + description: + This property identifies wired connection of the LED to this device. + 0x00 LEDA1 + 0x01 LEDA2 + 0x02 LEDA3 + 0x03 LEDA4 + 0x04 LEDB1 + 0x05 LEDB2 + 0x06 LEDB3 + 0x07 LEDB4 + 0x08 LEDC1 + 0x09 LEDC2 + 0x0A LEDC3 + 0x0B LEDC4 + +required: + - compatible + - reg + +additionalProperties: false + +examples: + - | + #include <dt-bindings/leds/common.h> + + i2c { + #address-cells = <1>; + #size-cells = <0>; + + led-controller@14 { + compatible = "kinetic,ktd20xx"; + reg = <0x68>; + #address-cells = <1>; + #size-cells = <0>; + kinetic,led-current = <0x28>; // Current for all LEDs is 5mA + + multi-led@0 { + reg = <0x0>; + color = <LED_COLOR_ID_MULTI>; + function = LED_FUNCTION_CHARGING; + linux,default-trigger = "default-on"; + }; + + multi-led@2 { + reg = <0x2>; + color = <LED_COLOR_ID_MULTI>; + function = LED_FUNCTION_STANDBY; + linux,default-trigger = "default-on"; + }; + }; + }; + +...
MAINTAINERS +1 -0
@@ -10743,6 +10743,7 @@ KTD20XX LED CONTROLLER DRIVER M: Florian Eckert <[email protected]> L: [email protected] S: Maintained +F: Documentation/devicetree/bindings/leds/leds-ktd20xx.yaml F: drivers/leds/leds-ktd20xx.c KTEST
Signature · 1 line
2.20.1