diff mbox series

[v3,1/2] dt-bindings: leds: pca995x: Add binding document for PCA995X chips

Message ID 20230705231326.232356-1-marex@denx.de
State Not Applicable, archived
Headers show
Series [v3,1/2] dt-bindings: leds: pca995x: Add binding document for PCA995X chips | expand

Checks

Context Check Description
robh/checkpatch success
robh/patch-applied success
robh/dtbs-check warning build log
robh/dt-meta-schema success

Commit Message

Marek Vasut July 5, 2023, 11:13 p.m. UTC 
The PCA995x chips are I2C controlled LED drivers. Each chip has
up to 16 outputs, each one with an individual 8-bit resolution
PWM for brightness control. Add binding document.

Signed-off-by: Marek Vasut <marex@denx.de>
---
Cc: Conor Dooley <conor+dt@kernel.org>
Cc: Isai Gaspar <isaiezequiel.gaspar@nxp.com>
Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
Cc: Lee Jones <lee@kernel.org>
Cc: Marek Vasut <marex@denx.de>
Cc: Pavel Machek <pavel@ucw.cz>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: devicetree@vger.kernel.org
Cc: linux-leds@vger.kernel.org
---
V2: Fix the led@[0-9]+ match
V3: Unit addresses for leds@ are hex, add a-f into the glob
---
 .../devicetree/bindings/leds/nxp,pca995x.yaml | 81 +++++++++++++++++++
 1 file changed, 81 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/leds/nxp,pca995x.yaml

Comments

Rob Herring (Arm) July 6, 2023, 7:06 p.m. UTC | #1 
On Thu, 06 Jul 2023 01:13:25 +0200, Marek Vasut wrote:
> The PCA995x chips are I2C controlled LED drivers. Each chip has
> up to 16 outputs, each one with an individual 8-bit resolution
> PWM for brightness control. Add binding document.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> ---
> Cc: Conor Dooley <conor+dt@kernel.org>
> Cc: Isai Gaspar <isaiezequiel.gaspar@nxp.com>
> Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
> Cc: Lee Jones <lee@kernel.org>
> Cc: Marek Vasut <marex@denx.de>
> Cc: Pavel Machek <pavel@ucw.cz>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: devicetree@vger.kernel.org
> Cc: linux-leds@vger.kernel.org
> ---
> V2: Fix the led@[0-9]+ match
> V3: Unit addresses for leds@ are hex, add a-f into the glob
> ---
>  .../devicetree/bindings/leds/nxp,pca995x.yaml | 81 +++++++++++++++++++
>  1 file changed, 81 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/leds/nxp,pca995x.yaml
> 

Reviewed-by: Rob Herring <robh@kernel.org>
Lee Jones July 13, 2023, 2:07 p.m. UTC | #2 
On Thu, 06 Jul 2023, Marek Vasut wrote:

> From: Isai Gaspar <isaiezequiel.gaspar@nxp.com>
> 
> The PCA995x chips are I2C controlled LED drivers. Each chip has
> up to 16 outputs, each one with an individual 8-bit resolution
> PWM for brightness control.
> 
> Signed-off-by: Isai Gaspar <isaiezequiel.gaspar@nxp.com>
> Signed-off-by: Marek Vasut <marex@denx.de> # Basically rewrite the driver
> ---
> Cc: Conor Dooley <conor+dt@kernel.org>
> Cc: Isai Gaspar <isaiezequiel.gaspar@nxp.com>
> Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
> Cc: Lee Jones <lee@kernel.org>
> Cc: Marek Vasut <marex@denx.de>
> Cc: Pavel Machek <pavel@ucw.cz>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: devicetree@vger.kernel.org
> Cc: linux-leds@vger.kernel.org
> ---
> V2: - Fix pca995x_probe() type
>     - Fix device_get_match_data() cast
> V3: - Drop of_match_ptr()
> ---
>  drivers/leds/Kconfig        |   9 ++
>  drivers/leds/Makefile       |   1 +
>  drivers/leds/leds-pca995x.c | 198 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 208 insertions(+)
>  create mode 100644 drivers/leds/leds-pca995x.c
> 
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index 6046dfeca16fc..b92208eccdea9 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -521,6 +521,15 @@ config LEDS_PCA963X
>  	  LED driver chip accessed via the I2C bus. Supported
>  	  devices include PCA9633 and PCA9634
>  
> +config LEDS_PCA995X
> +	tristate "LED Support for PCA995x I2C chips"
> +	depends on LEDS_CLASS
> +	depends on I2C
> +	help
> +	  This option enables support for LEDs connected to PCA995x
> +	  LED driver chips accessed via the I2C bus. Supported
> +	  devices include PCA9955BTW, PCA9952TW and PCA9955TW.
> +
>  config LEDS_WM831X_STATUS
>  	tristate "LED support for status LEDs on WM831x PMICs"
>  	depends on LEDS_CLASS
> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> index d71f1226540c2..d7348e8bc019a 100644
> --- a/drivers/leds/Makefile
> +++ b/drivers/leds/Makefile
> @@ -72,6 +72,7 @@ obj-$(CONFIG_LEDS_OT200)		+= leds-ot200.o
>  obj-$(CONFIG_LEDS_PCA9532)		+= leds-pca9532.o
>  obj-$(CONFIG_LEDS_PCA955X)		+= leds-pca955x.o
>  obj-$(CONFIG_LEDS_PCA963X)		+= leds-pca963x.o
> +obj-$(CONFIG_LEDS_PCA995X)		+= leds-pca995x.o
>  obj-$(CONFIG_LEDS_PM8058)		+= leds-pm8058.o
>  obj-$(CONFIG_LEDS_POWERNV)		+= leds-powernv.o
>  obj-$(CONFIG_LEDS_PWM)			+= leds-pwm.o
> diff --git a/drivers/leds/leds-pca995x.c b/drivers/leds/leds-pca995x.c
> new file mode 100644
> index 0000000000000..dde99f4a4c1c7
> --- /dev/null
> +++ b/drivers/leds/leds-pca995x.c
> @@ -0,0 +1,198 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * LED driver for PCA995x I2C LED drivers
> + *
> + * Copyright 2011 bct electronic GmbH
> + * Copyright 2013 Qtechnology/AS
> + * Copyright 2022 NXP
> + * Copyright 2023 Marek Vasut
> + */
> +
> +#include <linux/i2c.h>
> +#include <linux/leds.h>
> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/property.h>
> +#include <linux/regmap.h>
> +
> +/* Register definition */
> +#define PCA995X_MODE1		0x00
> +#define PCA995X_MODE2		0x01
> +#define PCA995X_LEDOUT0		0x02
> +#define PCA9955B_PWM0		0x08
> +#define PCA9952_PWM0		0x0A
> +#define PCA9952_IREFALL		0x43
> +#define PCA9955B_IREFALL	0x45
> +
> +/* LED select registers determine the source that drives LED outputs */
> +#define PCA995X_LED_OFF		0x0	/* LED driver off */

The define nomenclature is clear enough.

/> +#define PCA995X_LED_ON		0x1	/* LED driver on */

The define nomenclature is clear enough.

> +#define PCA995X_LED_PWM		0x2	/* Controlled through PWM */

PCA995X_LED_PWM_MODE

Then, the define nomenclature will be clear enough.

> +#define PCA995X_LDRX_MASK	0x3	/* 2 bits per output state control */
> +
> +#define PCA995X_MODE_1_CFG	0x00	/* Auto-increment disabled. Normal mode */

Is this not the reset value?

> +#define PCA995X_IREFALL_CFG	0x7F	/* Half of max current gain multiplier */

PCA995X_IREFALL_FULL_CFG	0xFF
PCA995X_IREFALL_HALF_CFG	(PCA995X_IREFALL_MAX_CFG / 2)

?

> +#define PCA995X_MAX_OUTPUTS	16	/* Supported outputs */

The define nomenclature is clear enough.

> +#define ldev_to_led(c)	container_of(c, struct pca995x_led, ldev)
> +
> +struct pca995x_led {
> +	unsigned int led_no;
> +	struct led_classdev ldev;
> +	struct pca995x_chip *chip;
> +};
> +
> +struct pca995x_chip {
> +	struct regmap *regmap;
> +	struct pca995x_led leds[PCA995X_MAX_OUTPUTS];
> +	int btype;
> +};

It's weird to have 2 structs reference each other.

This should not be allowed:

pca995x_led->pca995x_chip->pca995x_led->pca995x_chip->pca995x_led->pca995x_chip

Some implementation of container_of() should do the trick.

> +static int pca995x_brightness_set(struct led_classdev *led_cdev,
> +				  enum led_brightness brightness)
> +{
> +	struct pca995x_led *led = ldev_to_led(led_cdev);
> +	struct pca995x_chip *chip = led->chip;
> +	u8 ledout_addr, pwmout_addr;
> +	int shift, ret;
> +
> +	pwmout_addr = (chip->btype ? PCA9955B_PWM0 : PCA9952_PWM0) + led->led_no;

All this btype stuff is a little crazy and totally not scalable.

> +	ledout_addr = PCA995X_LEDOUT0 + (led->led_no / 4);
> +	shift = 2 * (led->led_no % 4);

Why 2 and 4?  I suggest that they are defined.

> +	switch (brightness) {
> +	case LED_FULL:

https://elixir.bootlin.com/linux/latest/source/include/linux/leds.h#L32

> +		return regmap_update_bits(chip->regmap, ledout_addr,
> +					  PCA995X_LDRX_MASK << shift,
> +					  PCA995X_LED_ON << shift);
> +	case LED_OFF:
> +		return regmap_update_bits(chip->regmap, ledout_addr,
> +					  PCA995X_LDRX_MASK << shift, 0);
> +	default:

Are there no invalid values here?

> +		/* Adjust brightness as per user input by changing individual PWM */
> +		ret = regmap_write(chip->regmap, pwmout_addr, brightness);
> +		if (ret)
> +			return ret;
> +
> +		/*
> +		 * Change LDRx configuration to individual brightness via PWM.
> +		 * Led will stop blinking if it's doing so

LED, it's an abbreviation.

> +		 */
> +		return regmap_update_bits(chip->regmap, ledout_addr,
> +					  PCA995X_LDRX_MASK << shift,
> +					  PCA995X_LED_PWM << shift);
> +	}
> +}
> +
> +static const struct regmap_config pca995x_regmap = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +	.max_register = 0x49,
> +};
> +
> +static int pca995x_probe(struct i2c_client *client)
> +{
> +	struct fwnode_handle *led_fwnodes[PCA995X_MAX_OUTPUTS] = { 0 };
> +	struct fwnode_handle *np, *child;
> +	struct device *dev = &client->dev;
> +	struct pca995x_chip *chip;
> +	struct pca995x_led *led;
> +	int i, btype, reg, ret;
> +
> +	btype = (unsigned long)device_get_match_data(&client->dev);
> +
> +	np = dev_fwnode(dev);
> +	if (!np)
> +		return -ENODEV;
> +
> +	chip = devm_kzalloc(dev, sizeof(*chip), GFP_KERNEL);
> +	if (!chip)
> +		return -ENOMEM;
> +
> +	chip->btype = btype;
> +	chip->regmap = devm_regmap_init_i2c(client, &pca995x_regmap);
> +	if (IS_ERR(chip->regmap))
> +		return PTR_ERR(chip->regmap);
> +
> +	i2c_set_clientdata(client, chip);
> +
> +	fwnode_for_each_available_child_node(np, child) {
> +		ret = fwnode_property_read_u32(child, "reg", &reg);
> +		if (ret) {
> +			fwnode_handle_put(child);
> +			return ret;
> +		}
> +
> +		if (reg < 0 || reg >= PCA995X_MAX_OUTPUTS || led_fwnodes[reg]) {
> +			fwnode_handle_put(child);
> +			return -EINVAL;
> +		}
> +
> +		led = &chip->leds[reg];
> +		led_fwnodes[reg] = child;
> +		led->chip = chip;
> +		led->led_no = reg;
> +		led->ldev.brightness_set_blocking = pca995x_brightness_set;
> +		led->ldev.max_brightness = 255;
> +	}
> +
> +	for (i = 0; i < PCA995X_MAX_OUTPUTS; i++) {
> +		struct led_init_data init_data = {};
> +
> +		if (!led_fwnodes[i])
> +			continue;
> +
> +		init_data.fwnode = led_fwnodes[i];
> +
> +		ret = devm_led_classdev_register_ext(dev,
> +						     &chip->leds[i].ldev,
> +						     &init_data);
> +		if (ret < 0) {
> +			fwnode_handle_put(child);
> +			return dev_err_probe(dev, ret,
> +					     "Could not register LED %s\n",
> +					     chip->leds[i].ldev.name);
> +		}
> +	}
> +
> +	/* Disable LED all-call address and set normal mode */
> +	ret = regmap_write(chip->regmap, PCA995X_MODE1, PCA995X_MODE_1_CFG);

The formatting of "MODE1" and "MODE_1" is making me twitch!

Is this how they are referenced in the datasheet?

> +	if (ret)
> +		return ret;
> +
> +	/* IREF Output current value for all LEDn outputs */
> +	return regmap_write(chip->regmap,
> +			    btype ? PCA9955B_IREFALL : PCA9952_IREFALL,
> +			    PCA995X_IREFALL_CFG);
> +}
> +
> +static const struct i2c_device_id pca995x_id[] = {
> +	{ "pca9952", .driver_data = (kernel_ulong_t)0 /* non-B chip */ },

Defines at least please.

Are you sure these are the only 2 types of chip this driver will
support?

> +	{ "pca9955b", .driver_data = (kernel_ulong_t)1 /* B-type chip */ },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(i2c, pca995x_id);
> +
> +static const struct of_device_id pca995x_of_match[] = {
> +	{ .compatible = "nxp,pca9952",  .data = (void *)0 /* non-B chip */ },
> +	{ .compatible = "nxp,pca9955b", .data = (void *)1 /* B-type chip */ },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(i2c, pca995x_of_match);
> +
> +static struct i2c_driver pca995x_driver = {
> +	.driver = {
> +		.name = "leds-pca995x",
> +		.owner = THIS_MODULE,

Does the core not do this for you?

> +		.of_match_table = pca995x_of_match,
> +	},
> +	.probe = pca995x_probe,
> +	.id_table = pca995x_id,
> +};
> +

Remove this line please.

> +module_i2c_driver(pca995x_driver);
> +
> +MODULE_AUTHOR("Isai Gaspar <isaiezequiel.gaspar@nxp.com>");
> +MODULE_DESCRIPTION("PCA995x LED driver");
> +MODULE_LICENSE("GPL");
> -- 
> 2.40.1
>
Marek Vasut July 13, 2023, 3:59 p.m. UTC | #3 
On 7/13/23 16:07, Lee Jones wrote:

[...]

>> +#define PCA995X_LDRX_MASK	0x3	/* 2 bits per output state control */
>> +
>> +#define PCA995X_MODE_1_CFG	0x00	/* Auto-increment disabled. Normal mode */
> 
> Is this not the reset value?

The chip may not be power-cycled on reboot, so this register is not 
necessarily 0x0.

>> +#define PCA995X_IREFALL_CFG	0x7F	/* Half of max current gain multiplier */
> 
> PCA995X_IREFALL_FULL_CFG	0xFF
> PCA995X_IREFALL_HALF_CFG	(PCA995X_IREFALL_MAX_CFG / 2)
> 
> ?
> 
>> +#define PCA995X_MAX_OUTPUTS	16	/* Supported outputs */
> 
> The define nomenclature is clear enough.
> 
>> +#define ldev_to_led(c)	container_of(c, struct pca995x_led, ldev)
>> +
>> +struct pca995x_led {
>> +	unsigned int led_no;
>> +	struct led_classdev ldev;
>> +	struct pca995x_chip *chip;
>> +};
>> +
>> +struct pca995x_chip {
>> +	struct regmap *regmap;
>> +	struct pca995x_led leds[PCA995X_MAX_OUTPUTS];
>> +	int btype;
>> +};
> 
> It's weird to have 2 structs reference each other.
> 
> This should not be allowed:
> 
> pca995x_led->pca995x_chip->pca995x_led->pca995x_chip->pca995x_led->pca995x_chip
> 
> Some implementation of container_of() should do the trick.

container_of() wouldn't work here because struct pca995x_chip contains 
array of LEDs .

The structure layout is taken from:
8325642d2757e ("leds: bd2606mvv: Driver for the Rohm 6 Channel i2c LED 
driver")

>> +static int pca995x_brightness_set(struct led_classdev *led_cdev,
>> +				  enum led_brightness brightness)
>> +{
>> +	struct pca995x_led *led = ldev_to_led(led_cdev);
>> +	struct pca995x_chip *chip = led->chip;
>> +	u8 ledout_addr, pwmout_addr;
>> +	int shift, ret;
>> +
>> +	pwmout_addr = (chip->btype ? PCA9955B_PWM0 : PCA9952_PWM0) + led->led_no;
> 
> All this btype stuff is a little crazy and totally not scalable.

That's right, but until NXP changes the register layout again, this 
should be sufficient to support the entire 995x line up.

>> +	ledout_addr = PCA995X_LEDOUT0 + (led->led_no / 4);
>> +	shift = 2 * (led->led_no % 4);
> 
> Why 2 and 4?  I suggest that they are defined.
> 
>> +	switch (brightness) {
>> +	case LED_FULL:
> 
> https://elixir.bootlin.com/linux/latest/source/include/linux/leds.h#L32

LED_HALF is just PWM (default) mode, set to half duty cycle .

>> +		return regmap_update_bits(chip->regmap, ledout_addr,
>> +					  PCA995X_LDRX_MASK << shift,
>> +					  PCA995X_LED_ON << shift);
>> +	case LED_OFF:
>> +		return regmap_update_bits(chip->regmap, ledout_addr,
>> +					  PCA995X_LDRX_MASK << shift, 0);
>> +	default:
> 
> Are there no invalid values here?

Not to my knowledge, this should be 0..255 clamped by 
ldev.max_brightness further down.

>> +	/* Disable LED all-call address and set normal mode */
>> +	ret = regmap_write(chip->regmap, PCA995X_MODE1, PCA995X_MODE_1_CFG);
> 
> The formatting of "MODE1" and "MODE_1" is making me twitch!
> 
> Is this how they are referenced in the datasheet?

It's called MODE1 in datasheet, fixed.

>> +	if (ret)
>> +		return ret;
>> +
>> +	/* IREF Output current value for all LEDn outputs */
>> +	return regmap_write(chip->regmap,
>> +			    btype ? PCA9955B_IREFALL : PCA9952_IREFALL,
>> +			    PCA995X_IREFALL_CFG);
>> +}
>> +
>> +static const struct i2c_device_id pca995x_id[] = {
>> +	{ "pca9952", .driver_data = (kernel_ulong_t)0 /* non-B chip */ },
> 
> Defines at least please.
> 
> Are you sure these are the only 2 types of chip this driver will
> support?

No, I cannot predict the future or how NXP will or won't change the 
register layout of it. When it cames, we can deal with that.

[...]

The rest is addressed in V4.
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/leds/nxp,pca995x.yaml b/Documentation/devicetree/bindings/leds/nxp,pca995x.yaml
new file mode 100644
index 0000000000000..654915c1f687f
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/nxp,pca995x.yaml
@@ -0,0 +1,81 @@ 
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/leds/nxp,pca995x.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: NXP PCA995x LED controllers
+
+maintainers:
+  - Isai Gaspar <isaiezequiel.gaspar@nxp.com>
+  - Marek Vasut <marex@denx.de>
+
+description:
+  The NXP PCA9952/PCA9955B are programmable LED controllers connected via I2C
+  that can drive 16 separate lines. Each of them can be individually switched
+  on and off, and brightness can be controlled via individual PWM.
+
+  Datasheets are available at
+  https://www.nxp.com/docs/en/data-sheet/PCA9952_PCA9955.pdf
+  https://www.nxp.com/docs/en/data-sheet/PCA9955B.pdf
+
+properties:
+  compatible:
+    enum:
+      - nxp,pca9952
+      - nxp,pca9955b
+
+  reg:
+    maxItems: 1
+
+  "#address-cells":
+    const: 1
+
+  "#size-cells":
+    const: 0
+
+patternProperties:
+  "^led@[0-9a-f]+$":
+    type: object
+    $ref: common.yaml#
+    unevaluatedProperties: false
+
+    properties:
+      reg:
+        minimum: 0
+        maximum: 15
+
+    required:
+      - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/leds/common.h>
+
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        led-controller@1 {
+            compatible = "nxp,pca9955b";
+            reg = <0x01>;
+            #address-cells = <1>;
+            #size-cells = <0>;
+
+            led@0 {
+                reg = <0x0>;
+                color = <LED_COLOR_ID_RED>;
+                function = LED_FUNCTION_POWER;
+            };
+
+            led@2 {
+                reg = <0x2>;
+                color = <LED_COLOR_ID_WHITE>;
+                function = LED_FUNCTION_STATUS;
+            };
+        };
+    };
+
+...