* [PATCH v3 1/2] dt-bindings: leds: pca995x: Add binding document for PCA995X chips @ 2023-07-05 23:13 Marek Vasut 2023-07-05 23:13 ` [PATCH v3 2/2] leds: pca995x: Add support " Marek Vasut 2023-07-06 19:06 ` [PATCH v3 1/2] dt-bindings: leds: pca995x: Add binding document " Rob Herring 0 siblings, 2 replies; 5+ messages in thread From: Marek Vasut @ 2023-07-05 23:13 UTC (permalink / raw) To: linux-leds Cc: Marek Vasut, Conor Dooley, Isai Gaspar, Krzysztof Kozlowski, Lee Jones, Pavel Machek, Rob Herring, devicetree 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 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; + }; + }; + }; + +... -- 2.40.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH v3 2/2] leds: pca995x: Add support for PCA995X chips 2023-07-05 23:13 [PATCH v3 1/2] dt-bindings: leds: pca995x: Add binding document for PCA995X chips Marek Vasut @ 2023-07-05 23:13 ` Marek Vasut 2023-07-13 14:07 ` Lee Jones 2023-07-06 19:06 ` [PATCH v3 1/2] dt-bindings: leds: pca995x: Add binding document " Rob Herring 1 sibling, 1 reply; 5+ messages in thread From: Marek Vasut @ 2023-07-05 23:13 UTC (permalink / raw) To: linux-leds Cc: Isai Gaspar, Marek Vasut, Conor Dooley, Krzysztof Kozlowski, Lee Jones, Pavel Machek, Rob Herring, devicetree 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 */ +#define PCA995X_LED_ON 0x1 /* LED driver on */ +#define PCA995X_LED_PWM 0x2 /* Controlled through PWM */ +#define PCA995X_LDRX_MASK 0x3 /* 2 bits per output state control */ + +#define PCA995X_MODE_1_CFG 0x00 /* Auto-increment disabled. Normal mode */ +#define PCA995X_IREFALL_CFG 0x7F /* Half of max current gain multiplier */ + +#define PCA995X_MAX_OUTPUTS 16 /* Supported outputs */ + +#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; +}; + +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; + ledout_addr = PCA995X_LEDOUT0 + (led->led_no / 4); + shift = 2 * (led->led_no % 4); + + switch (brightness) { + case LED_FULL: + 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: + /* 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 + */ + 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", ®); + 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); + 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 */ }, + { "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, + .of_match_table = pca995x_of_match, + }, + .probe = pca995x_probe, + .id_table = pca995x_id, +}; + +module_i2c_driver(pca995x_driver); + +MODULE_AUTHOR("Isai Gaspar <isaiezequiel.gaspar@nxp.com>"); +MODULE_DESCRIPTION("PCA995x LED driver"); +MODULE_LICENSE("GPL"); -- 2.40.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v3 2/2] leds: pca995x: Add support for PCA995X chips 2023-07-05 23:13 ` [PATCH v3 2/2] leds: pca995x: Add support " Marek Vasut @ 2023-07-13 14:07 ` Lee Jones 2023-07-13 15:59 ` Marek Vasut 0 siblings, 1 reply; 5+ messages in thread From: Lee Jones @ 2023-07-13 14:07 UTC (permalink / raw) To: Marek Vasut Cc: linux-leds, Isai Gaspar, Conor Dooley, Krzysztof Kozlowski, Pavel Machek, Rob Herring, devicetree 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", ®); > + 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 > -- Lee Jones [李琼斯] ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v3 2/2] leds: pca995x: Add support for PCA995X chips 2023-07-13 14:07 ` Lee Jones @ 2023-07-13 15:59 ` Marek Vasut 0 siblings, 0 replies; 5+ messages in thread From: Marek Vasut @ 2023-07-13 15:59 UTC (permalink / raw) To: Lee Jones Cc: linux-leds, Isai Gaspar, Conor Dooley, Krzysztof Kozlowski, Pavel Machek, Rob Herring, devicetree 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. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v3 1/2] dt-bindings: leds: pca995x: Add binding document for PCA995X chips 2023-07-05 23:13 [PATCH v3 1/2] dt-bindings: leds: pca995x: Add binding document for PCA995X chips Marek Vasut 2023-07-05 23:13 ` [PATCH v3 2/2] leds: pca995x: Add support " Marek Vasut @ 2023-07-06 19:06 ` Rob Herring 1 sibling, 0 replies; 5+ messages in thread From: Rob Herring @ 2023-07-06 19:06 UTC (permalink / raw) To: Marek Vasut Cc: devicetree, Isai Gaspar, linux-leds, Pavel Machek, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Lee Jones 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> ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-07-13 15:59 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-07-05 23:13 [PATCH v3 1/2] dt-bindings: leds: pca995x: Add binding document for PCA995X chips Marek Vasut 2023-07-05 23:13 ` [PATCH v3 2/2] leds: pca995x: Add support " Marek Vasut 2023-07-13 14:07 ` Lee Jones 2023-07-13 15:59 ` Marek Vasut 2023-07-06 19:06 ` [PATCH v3 1/2] dt-bindings: leds: pca995x: Add binding document " Rob Herring
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).