Hi,
I'm sending a series of patches to add support for the Broadchip BCT3024
LED driver. The first adds Broadchip to the DT vendor prefixes, the
second is the DT bindings documentation and the third is the driver
itself.
The Broadchip BCT3024 is an I2C LED driver with 24 independent channels,
each with 256 brightness levels. It is targeted mainly for mobile phones
and other hand-held devices.
As we use the chip in a device that needs to keep its power consumption
to a minimum, I added support for the runtime PM to let the kernel
disable the chip and its power supply when brightness is set to zero.
Any comments are welcome.
Matus Gajdos (3):
dt-bindings: Add vendor prefix for Broadchip Technology Group Co.,
Ltd.
dt-bindings: leds: Add binding for Broadchip BCT3024 LED driver
leds: Add Broadchip BCT3024 LED driver
.../bindings/leds/broadchip,bct3024.yaml | 89 +++
.../devicetree/bindings/vendor-prefixes.yaml | 2 +
drivers/leds/Kconfig | 9 +
drivers/leds/Makefile | 1 +
drivers/leds/leds-bct3024.c | 564 ++++++++++++++++++
5 files changed, 665 insertions(+)
create mode 100644 Documentation/devicetree/bindings/leds/broadchip,bct3024.yaml
create mode 100644 drivers/leds/leds-bct3024.c
--
2.25.1
Website: http://www.broadchip.com
Signed-off-by: Matus Gajdos <[email protected]>
---
Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++
1 file changed, 2 insertions(+)
diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml
index af60bf1a6664..dc1ed3dc0c9f 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
+++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
@@ -202,6 +202,8 @@ patternProperties:
description: Bosch Sensortec GmbH
"^boundary,.*":
description: Boundary Devices Inc.
+ "^broadchip,.*":
+ description: Broadchip Technology Group Co., Ltd.
"^brcm,.*":
description: Broadcom Corporation
"^broadmobi,.*":
--
2.25.1
The BCT3024 chip is an I2C LED driver with independent 24 output
channels. Each channel supports 256 levels.
Signed-off-by: Matus Gajdos <[email protected]>
---
drivers/leds/Kconfig | 9 +
drivers/leds/Makefile | 1 +
drivers/leds/leds-bct3024.c | 564 ++++++++++++++++++++++++++++++++++++
3 files changed, 574 insertions(+)
create mode 100644 drivers/leds/leds-bct3024.c
diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 6046dfeca16f..75059db201e2 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -135,6 +135,15 @@ config LEDS_BCM6358
This option enables support for LEDs connected to the BCM6358
LED HW controller accessed via MMIO registers.
+config LEDS_BCT3024
+ tristate "LED Support for Broadchip BCT3024"
+ depends on LEDS_CLASS
+ depends on I2C
+ depends on OF
+ help
+ This option enables support for LEDs connected to the BCT3024
+ LED driver.
+
config LEDS_CHT_WCOVE
tristate "LED support for Intel Cherry Trail Whiskey Cove PMIC"
depends on LEDS_CLASS
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index d71f1226540c..f9eaed4c2317 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -18,6 +18,7 @@ obj-$(CONFIG_LEDS_AW200XX) += leds-aw200xx.o
obj-$(CONFIG_LEDS_AW2013) += leds-aw2013.o
obj-$(CONFIG_LEDS_BCM6328) += leds-bcm6328.o
obj-$(CONFIG_LEDS_BCM6358) += leds-bcm6358.o
+obj-$(CONFIG_LEDS_BCT3024) += leds-bct3024.o
obj-$(CONFIG_LEDS_BD2606MVV) += leds-bd2606mvv.o
obj-$(CONFIG_LEDS_BD2802) += leds-bd2802.o
obj-$(CONFIG_LEDS_BLINKM) += leds-blinkm.o
diff --git a/drivers/leds/leds-bct3024.c b/drivers/leds/leds-bct3024.c
new file mode 100644
index 000000000000..7732fe022093
--- /dev/null
+++ b/drivers/leds/leds-bct3024.c
@@ -0,0 +1,564 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * LED driver for Broadchip BCT3024, based on leds-syscon.c
+ *
+ * Copyright 2023 Matus Gajdos <[email protected]>
+ */
+
+#include <linux/i2c.h>
+#include <linux/leds.h>
+#include <linux/regmap.h>
+#include <linux/gpio/consumer.h>
+#include <linux/pm_runtime.h>
+#include <linux/regulator/consumer.h>
+
+#define BCT3024_LED_COUNT 36
+#define BCT3024_REG_SHUTDOWN (0x00)
+#define BCT3024_REG_BRIGHTNESS(n) (0x01 + (n))
+#define BCT3024_REG_UPDATE (0x25)
+#define BCT3024_REG_CONTROL(n) (0x26 + (n))
+#define BCT3024_REG_GLOBAL_CONTROL (0x4a)
+#define BCT3024_REG_RESET (0x4f)
+
+enum bct3024_state {
+ BCT3024_STATE_INIT,
+ BCT3024_STATE_SHUTDOWN,
+ BCT3024_STATE_IDLE,
+ BCT3024_STATE_ACTIVE,
+};
+
+struct bct3024_led {
+ struct bct3024_led *next;
+ bool active;
+ unsigned int idx;
+ struct led_classdev ldev;
+ struct bct3024_data *priv;
+};
+
+struct bct3024_data {
+ enum bct3024_state state;
+ struct device *dev;
+ struct gpio_desc *shutdown_gpiod;
+ struct regulator *supply;
+ struct regmap *regmap;
+ struct mutex lock;
+ struct bct3024_led leds[BCT3024_LED_COUNT];
+};
+
+static const struct reg_default bct3024_defaults[] = {
+ { BCT3024_REG_SHUTDOWN, 0x00 },
+ { BCT3024_REG_BRIGHTNESS(0), 0x00 },
+ { BCT3024_REG_BRIGHTNESS(1), 0x00 },
+ { BCT3024_REG_BRIGHTNESS(2), 0x00 },
+ { BCT3024_REG_BRIGHTNESS(3), 0x00 },
+ { BCT3024_REG_BRIGHTNESS(4), 0x00 },
+ { BCT3024_REG_BRIGHTNESS(5), 0x00 },
+ { BCT3024_REG_BRIGHTNESS(6), 0x00 },
+ { BCT3024_REG_BRIGHTNESS(7), 0x00 },
+ { BCT3024_REG_BRIGHTNESS(8), 0x00 },
+ { BCT3024_REG_BRIGHTNESS(9), 0x00 },
+ { BCT3024_REG_BRIGHTNESS(10), 0x00 },
+ { BCT3024_REG_BRIGHTNESS(11), 0x00 },
+ { BCT3024_REG_BRIGHTNESS(12), 0x00 },
+ { BCT3024_REG_BRIGHTNESS(13), 0x00 },
+ { BCT3024_REG_BRIGHTNESS(14), 0x00 },
+ { BCT3024_REG_BRIGHTNESS(15), 0x00 },
+ { BCT3024_REG_BRIGHTNESS(16), 0x00 },
+ { BCT3024_REG_BRIGHTNESS(17), 0x00 },
+ { BCT3024_REG_BRIGHTNESS(18), 0x00 },
+ { BCT3024_REG_BRIGHTNESS(19), 0x00 },
+ { BCT3024_REG_BRIGHTNESS(20), 0x00 },
+ { BCT3024_REG_BRIGHTNESS(21), 0x00 },
+ { BCT3024_REG_BRIGHTNESS(22), 0x00 },
+ { BCT3024_REG_BRIGHTNESS(23), 0x00 },
+ { BCT3024_REG_BRIGHTNESS(24), 0x00 },
+ { BCT3024_REG_BRIGHTNESS(25), 0x00 },
+ { BCT3024_REG_BRIGHTNESS(26), 0x00 },
+ { BCT3024_REG_BRIGHTNESS(27), 0x00 },
+ { BCT3024_REG_BRIGHTNESS(28), 0x00 },
+ { BCT3024_REG_BRIGHTNESS(29), 0x00 },
+ { BCT3024_REG_BRIGHTNESS(30), 0x00 },
+ { BCT3024_REG_BRIGHTNESS(31), 0x00 },
+ { BCT3024_REG_BRIGHTNESS(32), 0x00 },
+ { BCT3024_REG_BRIGHTNESS(33), 0x00 },
+ { BCT3024_REG_BRIGHTNESS(34), 0x00 },
+ { BCT3024_REG_BRIGHTNESS(35), 0x00 },
+ { BCT3024_REG_UPDATE, 0x00 },
+ { BCT3024_REG_CONTROL(0), 0x00 },
+ { BCT3024_REG_CONTROL(1), 0x00 },
+ { BCT3024_REG_CONTROL(2), 0x00 },
+ { BCT3024_REG_CONTROL(3), 0x00 },
+ { BCT3024_REG_CONTROL(4), 0x00 },
+ { BCT3024_REG_CONTROL(5), 0x00 },
+ { BCT3024_REG_CONTROL(6), 0x00 },
+ { BCT3024_REG_CONTROL(7), 0x00 },
+ { BCT3024_REG_CONTROL(8), 0x00 },
+ { BCT3024_REG_CONTROL(9), 0x00 },
+ { BCT3024_REG_CONTROL(10), 0x00 },
+ { BCT3024_REG_CONTROL(11), 0x00 },
+ { BCT3024_REG_CONTROL(12), 0x00 },
+ { BCT3024_REG_CONTROL(13), 0x00 },
+ { BCT3024_REG_CONTROL(14), 0x00 },
+ { BCT3024_REG_CONTROL(15), 0x00 },
+ { BCT3024_REG_CONTROL(16), 0x00 },
+ { BCT3024_REG_CONTROL(17), 0x00 },
+ { BCT3024_REG_CONTROL(18), 0x00 },
+ { BCT3024_REG_CONTROL(19), 0x00 },
+ { BCT3024_REG_CONTROL(20), 0x00 },
+ { BCT3024_REG_CONTROL(21), 0x00 },
+ { BCT3024_REG_CONTROL(22), 0x00 },
+ { BCT3024_REG_CONTROL(23), 0x00 },
+ { BCT3024_REG_CONTROL(24), 0x00 },
+ { BCT3024_REG_CONTROL(25), 0x00 },
+ { BCT3024_REG_CONTROL(26), 0x00 },
+ { BCT3024_REG_CONTROL(27), 0x00 },
+ { BCT3024_REG_CONTROL(28), 0x00 },
+ { BCT3024_REG_CONTROL(29), 0x00 },
+ { BCT3024_REG_CONTROL(30), 0x00 },
+ { BCT3024_REG_CONTROL(31), 0x00 },
+ { BCT3024_REG_CONTROL(32), 0x00 },
+ { BCT3024_REG_CONTROL(33), 0x00 },
+ { BCT3024_REG_CONTROL(34), 0x00 },
+ { BCT3024_REG_CONTROL(35), 0x00 },
+ { BCT3024_REG_GLOBAL_CONTROL, 0x00 },
+ { BCT3024_REG_RESET, 0x00 },
+};
+
+static bool bct3024_volatile_reg(struct device *dev, unsigned int reg)
+{
+ switch (reg) {
+ case BCT3024_REG_UPDATE:
+ case BCT3024_REG_RESET:
+ return true;
+ default:
+ return false;
+ }
+}
+
+static bool bct3024_readable_reg(struct device *dev, unsigned int reg)
+{
+ /* The chip doesn't support i2c read */
+ return false;
+}
+
+static bool bct3024_writeable_reg(struct device *dev, unsigned int reg)
+{
+ switch (reg) {
+ case BCT3024_REG_SHUTDOWN:
+ case BCT3024_REG_BRIGHTNESS(0) ... BCT3024_REG_BRIGHTNESS(35):
+ case BCT3024_REG_UPDATE:
+ case BCT3024_REG_CONTROL(0) ... BCT3024_REG_CONTROL(35):
+ case BCT3024_REG_GLOBAL_CONTROL:
+ case BCT3024_REG_RESET:
+ return true;
+ default:
+ return false;
+ }
+}
+
+static const struct regmap_config bct3024_regmap = {
+ .reg_bits = 8,
+ .val_bits = 8,
+ .max_register = BCT3024_REG_RESET,
+ .reg_defaults = bct3024_defaults,
+ .num_reg_defaults = ARRAY_SIZE(bct3024_defaults),
+ .volatile_reg = bct3024_volatile_reg,
+ .readable_reg = bct3024_readable_reg,
+ .writeable_reg = bct3024_writeable_reg,
+ .cache_type = REGCACHE_FLAT,
+};
+
+static int bct3024_write(struct bct3024_data *priv, unsigned int reg,
+ unsigned int val)
+{
+ int ret = regmap_write(priv->regmap, reg, val);
+
+ if (ret < 0)
+ dev_err(priv->dev, "failed to write register 0x%x: %d\n", reg, ret);
+
+ return ret;
+}
+
+static bool bct3024_any_active(struct bct3024_data *priv)
+{
+ int i;
+
+ for (i = ARRAY_SIZE(priv->leds); i--; )
+ if (priv->leds[i].active && priv->leds[i].ldev.brightness)
+ return true;
+
+ return false;
+}
+
+static int bct3024_set_shutdown_reg(struct bct3024_data *priv, bool shutdown)
+{
+ unsigned int val = shutdown || !bct3024_any_active(priv) ? 0 : 1;
+ int ret = bct3024_write(priv, BCT3024_REG_SHUTDOWN, val);
+
+ return ret < 0 ? ret : val;
+}
+
+static int bct3024_shutdown(struct bct3024_data *priv, int active)
+{
+ struct device *dev = priv->dev;
+ int is_off = priv->state == BCT3024_STATE_INIT ||
+ priv->state == BCT3024_STATE_SHUTDOWN;
+ int ret;
+
+ if (is_off == active) {
+ /* Nothing to do here */
+ } else if (active) {
+ priv->state = BCT3024_STATE_SHUTDOWN;
+
+ bct3024_set_shutdown_reg(priv, true);
+ regcache_cache_only(priv->regmap, true);
+
+ if (priv->shutdown_gpiod)
+ gpiod_set_value_cansleep(priv->shutdown_gpiod, 1);
+
+ if (priv->supply) {
+ ret = regulator_disable(priv->supply);
+ if (ret < 0)
+ dev_err(dev, "failed to disable supply: %d\n", ret);
+ }
+ } else {
+ if (priv->supply) {
+ ret = regulator_enable(priv->supply);
+ if (ret < 0) {
+ dev_err(dev, "failed to enable supply: %d\n", ret);
+ return ret;
+ }
+ }
+
+ if (priv->shutdown_gpiod)
+ gpiod_set_value_cansleep(priv->shutdown_gpiod, 0);
+
+ if (priv->state == BCT3024_STATE_SHUTDOWN) {
+ regcache_cache_only(priv->regmap, false);
+ regcache_mark_dirty(priv->regmap);
+ ret = regcache_sync(priv->regmap);
+ if (ret < 0) {
+ dev_err(dev, "failed to sync regmap: %d\n", ret);
+ return ret;
+ }
+ }
+
+ ret = bct3024_set_shutdown_reg(priv, false);
+ if (ret < 0)
+ return ret;
+
+ priv->state = ret ? BCT3024_STATE_ACTIVE : BCT3024_STATE_IDLE;
+ }
+
+ return 0;
+}
+
+static int bct3024_brightness_set(struct led_classdev *ldev,
+ enum led_brightness brightness)
+{
+ struct bct3024_led *led = container_of(ldev, struct bct3024_led, ldev);
+ struct bct3024_data *priv = led->priv;
+ struct device *dev = priv->dev;
+ int ret;
+ unsigned int ctrl, bright;
+
+ if (priv->state == BCT3024_STATE_INIT)
+ return -EIO;
+
+ if (brightness == 0) {
+ ctrl = 0x00;
+ bright = 0;
+ } else if (brightness < 256) {
+ ctrl = 0x01;
+ bright = brightness;
+ }
+
+ mutex_lock(&priv->lock);
+
+ if (bct3024_any_active(priv) && (priv->state == BCT3024_STATE_IDLE ||
+ priv->state == BCT3024_STATE_SHUTDOWN)) {
+ pm_runtime_get_sync(dev);
+ priv->state = BCT3024_STATE_ACTIVE;
+ }
+
+ for (; led; led = led->next) {
+ ret = bct3024_write(priv, BCT3024_REG_CONTROL(led->idx), ctrl);
+ if (ret < 0)
+ goto exit;
+ ret = bct3024_write(priv, BCT3024_REG_BRIGHTNESS(led->idx), bright);
+ if (ret < 0)
+ goto exit;
+ }
+
+ ret = bct3024_write(priv, BCT3024_REG_UPDATE, 0);
+ if (ret < 0)
+ goto exit;
+
+ ret = bct3024_set_shutdown_reg(priv, false);
+ if (ret < 0)
+ goto exit;
+
+ if (!ret && priv->state == BCT3024_STATE_ACTIVE) {
+ priv->state = BCT3024_STATE_IDLE;
+ pm_runtime_mark_last_busy(dev);
+ pm_runtime_put_autosuspend(dev);
+ /* Activate autosuspend */
+ pm_runtime_idle(dev);
+ }
+
+ ret = 0;
+
+exit:
+ mutex_unlock(&priv->lock);
+
+ return ret;
+}
+
+static int bct3024_setup_led(struct bct3024_data *priv, u32 reg,
+ struct bct3024_led **prev, struct led_init_data *idata)
+{
+ struct device *dev = priv->dev;
+ struct bct3024_led *led;
+ int ret;
+
+ if (reg >= BCT3024_LED_COUNT) {
+ ret = -EINVAL;
+ dev_err_probe(dev, ret, "invalid reg value: %u\n", reg);
+ return ret;
+ }
+
+ led = &priv->leds[reg];
+
+ if (led->active) {
+ ret = -EINVAL;
+ dev_err_probe(dev, ret, "reg redeclared: %u\n", reg);
+ return ret;
+ }
+
+ led->active = true;
+ led->priv = priv;
+ led->idx = reg;
+
+ if (!*prev) {
+ led->ldev.max_brightness = 255;
+ led->ldev.brightness_set_blocking = bct3024_brightness_set;
+
+ ret = devm_led_classdev_register_ext(dev, &led->ldev, idata);
+ if (ret < 0) {
+ dev_err_probe(dev, ret, "failed to register led %u\n", reg);
+ return ret;
+ }
+ } else
+ (*prev)->next = led;
+
+ *prev = led;
+
+ return 0;
+}
+
+static int bct3024_of_parse(struct i2c_client *client)
+{
+ struct bct3024_data *priv = i2c_get_clientdata(client);
+ struct device *dev = priv->dev;
+ struct device_node *np;
+ int ret;
+
+ ret = of_get_child_count(dev->of_node);
+ if (!ret || ret > ARRAY_SIZE(priv->leds)) {
+ dev_err_probe(dev, -EINVAL, "invalid nodes count: %d\n", ret);
+ return -EINVAL;
+ }
+
+ for_each_child_of_node(dev->of_node, np) {
+ u32 regs[BCT3024_LED_COUNT];
+ struct led_init_data idata = {
+ .fwnode = of_fwnode_handle(np),
+ .devicename = client->name,
+ };
+ struct bct3024_led *led;
+ int count, i;
+
+ count = of_property_read_variable_u32_array(np, "reg", regs, 1,
+ BCT3024_LED_COUNT);
+ if (count < 0) {
+ ret = count;
+ dev_err_probe(dev, ret, "failed to read node reg\n");
+ goto fail;
+ }
+
+ for (i = 0, led = NULL; i < count; i++) {
+ ret = bct3024_setup_led(priv, regs[i], &led, &idata);
+ if (ret < 0)
+ goto fail;
+ }
+ }
+
+ return 0;
+
+fail:
+ of_node_put(np);
+
+ return ret;
+}
+
+static int bct3024_i2c_probe(struct i2c_client *client)
+{
+ struct bct3024_data *priv;
+ struct device *dev = &client->dev;
+ int ret;
+
+ priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ priv->dev = dev;
+
+ priv->supply = devm_regulator_get_optional(dev, "vdd");
+ if (IS_ERR(priv->supply)) {
+ ret = PTR_ERR(priv->supply);
+ if (ret != -ENODEV) {
+ dev_err_probe(dev, ret, "failed to get supply\n");
+ return ret;
+ }
+ priv->supply = NULL;
+ }
+
+ priv->shutdown_gpiod = devm_gpiod_get_optional(dev, "shutdown", GPIOD_OUT_HIGH);
+ if (IS_ERR(priv->shutdown_gpiod)) {
+ ret = PTR_ERR(priv->shutdown_gpiod);
+ dev_err_probe(dev, ret, "failed to get shutdown gpio\n");
+ return ret;
+ }
+
+ priv->regmap = devm_regmap_init_i2c(client, &bct3024_regmap);
+ if (IS_ERR(priv->regmap)) {
+ ret = PTR_ERR(priv->regmap);
+ return ret;
+ }
+
+ mutex_init(&priv->lock);
+ i2c_set_clientdata(client, priv);
+
+ pm_runtime_set_autosuspend_delay(dev, 2000);
+ pm_runtime_use_autosuspend(dev);
+ pm_runtime_enable(dev);
+ if (!pm_runtime_enabled(dev)) {
+ ret = bct3024_shutdown(priv, false);
+ if (ret < 0)
+ return ret;
+ }
+
+ ret = pm_runtime_resume_and_get(dev);
+ if (ret < 0)
+ goto fail;
+
+ ret = bct3024_of_parse(client);
+ if (ret < 0)
+ goto fail;
+
+ pm_runtime_mark_last_busy(dev);
+ pm_runtime_put_autosuspend(dev);
+
+ return 0;
+
+fail:
+ dev_err_probe(dev, ret, "probe failed\n");
+ if (!pm_runtime_status_suspended(dev))
+ bct3024_shutdown(priv, 2);
+ return ret;
+}
+
+static void bct3024_i2c_remove(struct i2c_client *client)
+{
+ struct bct3024_data *priv = i2c_get_clientdata(client);
+
+ bct3024_shutdown(priv, true);
+ pm_runtime_disable(priv->dev);
+ mutex_destroy(&priv->lock);
+}
+
+static void bct3024_i2c_shutdown(struct i2c_client *client)
+{
+ struct bct3024_data *priv = i2c_get_clientdata(client);
+
+ bct3024_shutdown(priv, true);
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int bct3024_runtime_idle(struct device *dev)
+{
+ struct bct3024_data *priv = dev_get_drvdata(dev);
+
+ dev_dbg(dev, "%s: %d\n", __func__, priv->state);
+
+ return priv->state == BCT3024_STATE_ACTIVE ? -EBUSY : 0;
+}
+
+static int bct3024_runtime_suspend(struct device *dev)
+{
+ struct bct3024_data *priv = dev_get_drvdata(dev);
+
+ dev_dbg(dev, "%s: %d\n", __func__, priv->state);
+
+ switch (priv->state) {
+ case BCT3024_STATE_INIT:
+ case BCT3024_STATE_SHUTDOWN:
+ return 0;
+ default:
+ break;
+ }
+
+ return bct3024_shutdown(priv, true);
+}
+
+static int bct3024_runtime_resume(struct device *dev)
+{
+ struct bct3024_data *priv = dev_get_drvdata(dev);
+ int ret;
+
+ dev_dbg(dev, "%s: %d\n", __func__, priv->state);
+
+ switch (priv->state) {
+ case BCT3024_STATE_INIT:
+ case BCT3024_STATE_SHUTDOWN:
+ break;
+ default:
+ return 0;
+ }
+
+ ret = bct3024_shutdown(priv, false);
+ if (ret < 0)
+ bct3024_shutdown(priv, 2);
+
+ return ret;
+}
+#endif
+
+static const struct dev_pm_ops bct3024_pm_ops = {
+ SET_RUNTIME_PM_OPS(bct3024_runtime_suspend, bct3024_runtime_resume,
+ bct3024_runtime_idle)
+ SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, pm_runtime_force_resume)
+};
+
+static const struct of_device_id bct3024_of_match[] = {
+ { .compatible = "broadchip,bct3024" },
+ { }
+};
+MODULE_DEVICE_TABLE(of, bct3024_of_match);
+
+static struct i2c_driver bct3024_driver = {
+ .driver = {
+ .name = "bct3024",
+ .of_match_table = bct3024_of_match,
+ .pm = &bct3024_pm_ops,
+ },
+ .probe_new = bct3024_i2c_probe,
+ .shutdown = bct3024_i2c_shutdown,
+ .remove = bct3024_i2c_remove,
+};
+
+module_i2c_driver(bct3024_driver);
+
+MODULE_AUTHOR("Matus Gajdos <[email protected]>");
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Broadchip BCT3024 LED driver");
--
2.25.1
The BCT3024 chip is an I2C LED driver with independent 24 output
channels. Each channel supports 256 levels.
Signed-off-by: Matus Gajdos <[email protected]>
---
.../bindings/leds/broadchip,bct3024.yaml | 89 +++++++++++++++++++
1 file changed, 89 insertions(+)
create mode 100644 Documentation/devicetree/bindings/leds/broadchip,bct3024.yaml
diff --git a/Documentation/devicetree/bindings/leds/broadchip,bct3024.yaml b/Documentation/devicetree/bindings/leds/broadchip,bct3024.yaml
new file mode 100644
index 000000000000..0d622894e79c
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/broadchip,bct3024.yaml
@@ -0,0 +1,89 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/leds/broadchip,bct3024.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Broadchip BCT3024 LED Driver
+
+maintainers:
+ - Matus Gajdos <[email protected]>
+
+description: |
+ The BCT3024 is an I2C LED driver with independent 24 output channels. Each
+ channel supports 256 levels and its output current can be scaled by a factor
+ of 1, 1/2, 1/3 and 1/4.
+
+properties:
+ compatible:
+ const: broadchip,bct3024
+
+ reg:
+ description: I2C slave address of the driver.
+ maxItems: 1
+
+ vdd-supply:
+ description: Regulator providing power to the VDD pin.
+
+ shutdown-gpios:
+ maxItems: 1
+ description: GPIO attached to the SDB pin.
+
+ "#address-cells":
+ const: 1
+
+ "#size-cells":
+ const: 0
+
+patternProperties:
+ "^led@[0-9a-f]+$":
+ type: object
+ $ref: common.yaml#
+ unevaluatedProperties: false
+
+ properties:
+ reg:
+ description: Index of the LED channel.
+ minimum: 0
+ maximum: 23
+
+required:
+ - compatible
+ - reg
+ - "#address-cells"
+ - "#size-cells"
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/gpio/gpio.h>
+ #include <dt-bindings/leds/common.h>
+
+ i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ led-controller@3c {
+ compatible = "broadchip,bct3024";
+ reg = <0x3c>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ vdd-supply = <®_3v3d>;
+
+ led@0 {
+ reg = <0x00>;
+ function = LED_FUNCTION_INDICATOR;
+ color = <LED_COLOR_ID_RED>;
+ };
+
+ led@1 {
+ reg = <0x01>;
+ function = LED_FUNCTION_INDICATOR;
+ color = <LED_COLOR_ID_GREEN>;
+ };
+ };
+ };
+
+...
--
2.25.1
Hi Matus,
kernel test robot noticed the following build errors:
[auto build test ERROR on robh/for-next]
[also build test ERROR on lee-leds/for-leds-next linus/master v6.5-rc3 next-20230727]
[cannot apply to pavel-leds/for-next]
[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#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Matus-Gajdos/dt-bindings-Add-vendor-prefix-for-Broadchip-Technology-Group-Co-Ltd/20230728-001607
base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link: https://lore.kernel.org/r/20230727160525.23312-4-matuszpd%40gmail.com
patch subject: [PATCH 3/3] leds: Add Broadchip BCT3024 LED driver
config: sh-allmodconfig (https://download.01.org/0day-ci/archive/20230728/[email protected]/config)
compiler: sh4-linux-gcc (GCC) 12.3.0
reproduce: (https://download.01.org/0day-ci/archive/20230728/[email protected]/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All errors (new ones prefixed by >>):
In file included from include/linux/device.h:25,
from include/linux/acpi.h:14,
from include/linux/i2c.h:13,
from drivers/leds/leds-bct3024.c:8:
>> drivers/leds/leds-bct3024.c:538:28: error: 'bct3024_runtime_suspend' undeclared here (not in a function); did you mean 'pm_runtime_suspend'?
538 | SET_RUNTIME_PM_OPS(bct3024_runtime_suspend, bct3024_runtime_resume,
| ^~~~~~~~~~~~~~~~~~~~~~~
include/linux/pm.h:337:28: note: in definition of macro 'RUNTIME_PM_OPS'
337 | .runtime_suspend = suspend_fn, \
| ^~~~~~~~~~
drivers/leds/leds-bct3024.c:538:9: note: in expansion of macro 'SET_RUNTIME_PM_OPS'
538 | SET_RUNTIME_PM_OPS(bct3024_runtime_suspend, bct3024_runtime_resume,
| ^~~~~~~~~~~~~~~~~~
>> drivers/leds/leds-bct3024.c:538:53: error: 'bct3024_runtime_resume' undeclared here (not in a function); did you mean 'pm_runtime_resume'?
538 | SET_RUNTIME_PM_OPS(bct3024_runtime_suspend, bct3024_runtime_resume,
| ^~~~~~~~~~~~~~~~~~~~~~
include/linux/pm.h:338:27: note: in definition of macro 'RUNTIME_PM_OPS'
338 | .runtime_resume = resume_fn, \
| ^~~~~~~~~
drivers/leds/leds-bct3024.c:538:9: note: in expansion of macro 'SET_RUNTIME_PM_OPS'
538 | SET_RUNTIME_PM_OPS(bct3024_runtime_suspend, bct3024_runtime_resume,
| ^~~~~~~~~~~~~~~~~~
>> drivers/leds/leds-bct3024.c:539:28: error: 'bct3024_runtime_idle' undeclared here (not in a function); did you mean 'pm_runtime_idle'?
539 | bct3024_runtime_idle)
| ^~~~~~~~~~~~~~~~~~~~
include/linux/pm.h:339:25: note: in definition of macro 'RUNTIME_PM_OPS'
339 | .runtime_idle = idle_fn,
| ^~~~~~~
drivers/leds/leds-bct3024.c:538:9: note: in expansion of macro 'SET_RUNTIME_PM_OPS'
538 | SET_RUNTIME_PM_OPS(bct3024_runtime_suspend, bct3024_runtime_resume,
| ^~~~~~~~~~~~~~~~~~
vim +538 drivers/leds/leds-bct3024.c
536
537 static const struct dev_pm_ops bct3024_pm_ops = {
> 538 SET_RUNTIME_PM_OPS(bct3024_runtime_suspend, bct3024_runtime_resume,
> 539 bct3024_runtime_idle)
540 SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, pm_runtime_force_resume)
541 };
542
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
On 27/07/2023 18:05, Matus Gajdos wrote:
> The BCT3024 chip is an I2C LED driver with independent 24 output
> channels. Each channel supports 256 levels.
>
> Signed-off-by: Matus Gajdos <[email protected]>
Thank you for your patch. There is something to discuss/improve.
A nit, subject: drop second/last, redundant "binding for". The
"dt-bindings" prefix is already stating that these are bindings.
> ---
> .../bindings/leds/broadchip,bct3024.yaml | 89 +++++++++++++++++++
> 1 file changed, 89 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/leds/broadchip,bct3024.yaml
>
> diff --git a/Documentation/devicetree/bindings/leds/broadchip,bct3024.yaml b/Documentation/devicetree/bindings/leds/broadchip,bct3024.yaml
> new file mode 100644
> index 000000000000..0d622894e79c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/broadchip,bct3024.yaml
> @@ -0,0 +1,89 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/leds/broadchip,bct3024.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Broadchip BCT3024 LED Driver
> +
> +maintainers:
> + - Matus Gajdos <[email protected]>
> +
> +description: |
> + The BCT3024 is an I2C LED driver with independent 24 output channels. Each
> + channel supports 256 levels and its output current can be scaled by a factor
> + of 1, 1/2, 1/3 and 1/4.
> +
> +properties:
> + compatible:
> + const: broadchip,bct3024
> +
> + reg:
> + description: I2C slave address of the driver.
Drop description, it is obvious.
> + maxItems: 1
> +
> + vdd-supply:
> + description: Regulator providing power to the VDD pin.
> +
> + shutdown-gpios:
powerdown-gpios instead. See gpio-consumer-common.yaml
> + maxItems: 1
And this won't be needed.
> + description: GPIO attached to the SDB pin.
> +
> + "#address-cells":
> + const: 1
> +
> + "#size-cells":
> + const: 0
> +
Best regards,
Krzysztof
On 27/07/2023 18:05, Matus Gajdos wrote:
> Website: http://www.broadchip.com
>
> Signed-off-by: Matus Gajdos <[email protected]>
> ---
> Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml
> index af60bf1a6664..dc1ed3dc0c9f 100644
> --- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
> +++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
> @@ -202,6 +202,8 @@ patternProperties:
> description: Bosch Sensortec GmbH
> "^boundary,.*":
> description: Boundary Devices Inc.
> + "^broadchip,.*":
> + description: Broadchip Technology Group Co., Ltd.
> "^brcm,.*":
Does not look ordered. 'c' is before 'o'.
Best regards,
Krzysztof
On 27/07/2023 18:05, Matus Gajdos wrote:
> The BCT3024 chip is an I2C LED driver with independent 24 output
> channels. Each channel supports 256 levels.
>
> Signed-off-by: Matus Gajdos <[email protected]>
> ---
> drivers/leds/Kconfig | 9 +
> drivers/leds/Makefile | 1 +
> drivers/leds/leds-bct3024.c | 564 ++++++++++++++++++++++++++++++++++++
> 3 files changed, 574 insertions(+)
> create mode 100644 drivers/leds/leds-bct3024.c
Thank you for your patch. There is something to discuss/improve.
>
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index 6046dfeca16f..75059db201e2 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -135,6 +135,15 @@ config LEDS_BCM6358
> This option enables support for LEDs connected to the BCM6358
> LED HW controller accessed via
...
> +}
> +
> +static int bct3024_brightness_set(struct led_classdev *ldev,
> + enum led_brightness brightness)
> +{
> + struct bct3024_led *led = container_of(ldev, struct bct3024_led, ldev);
> + struct bct3024_data *priv = led->priv;
> + struct device *dev = priv->dev;
> + int ret;
> + unsigned int ctrl, bright;
> +
> + if (priv->state == BCT3024_STATE_INIT)
> + return -EIO;
> +
> + if (brightness == 0) {
> + ctrl = 0x00;
> + bright = 0;
> + } else if (brightness < 256) {
> + ctrl = 0x01;
> + bright = brightness;
> + }
> +
> + mutex_lock(&priv->lock);
What do you protect with lock? This must be documented in lock's
definition in your struct.
> +
> + if (bct3024_any_active(priv) && (priv->state == BCT3024_STATE_IDLE ||
> + priv->state == BCT3024_STATE_SHUTDOWN)) {
> + pm_runtime_get_sync(dev);
> + priv->state = BCT3024_STATE_ACTIVE;
I don't understand why you added state machine for handling state. You
are basically duplicating runtime PM...
> + }
> +
> + for (; led; led = led->next) {
> + ret = bct3024_write(priv, BCT3024_REG_CONTROL(led->idx), ctrl);
> + if (ret < 0)
> + goto exit;
> + ret = bct3024_write(priv, BCT3024_REG_BRIGHTNESS(led->idx), bright);
> + if (ret < 0)
> + goto exit;
> + }
> +
> + ret = bct3024_write(priv, BCT3024_REG_UPDATE, 0);
> + if (ret < 0)
> + goto exit;
> +
> + ret = bct3024_set_shutdown_reg(priv, false);
> + if (ret < 0)
> + goto exit;
> +
> + if (!ret && priv->state == BCT3024_STATE_ACTIVE) {
> + priv->state = BCT3024_STATE_IDLE;
> + pm_runtime_mark_last_busy(dev);
> + pm_runtime_put_autosuspend(dev);
> + /* Activate autosuspend */
> + pm_runtime_idle(dev);
> + }
> +
> + ret = 0;
> +
> +exit:
> + mutex_unlock(&priv->lock);
> +
> + return ret;
> +}
> +
> +static int bct3024_setup_led(struct bct3024_data *priv, u32 reg,
> + struct bct3024_led **prev, struct led_init_data *idata)
> +{
> + struct device *dev = priv->dev;
> + struct bct3024_led *led;
> + int ret;
> +
> + if (reg >= BCT3024_LED_COUNT) {
> + ret = -EINVAL;
> + dev_err_probe(dev, ret, "invalid reg value: %u\n", reg);
> + return ret;
That's not correct syntax.
return dev_err_probe
> + }
> +
> + led = &priv->leds[reg];
> +
> + if (led->active) {
> + ret = -EINVAL;
> + dev_err_probe(dev, ret, "reg redeclared: %u\n", reg);
> + return ret;
Ditto
> + }
> +
> + led->active = true;
> + led->priv = priv;
> + led->idx = reg;
> +
> + if (!*prev) {
> + led->ldev.max_brightness = 255;
> + led->ldev.brightness_set_blocking = bct3024_brightness_set;
> +
> + ret = devm_led_classdev_register_ext(dev, &led->ldev, idata);
> + if (ret < 0) {
> + dev_err_probe(dev, ret, "failed to register led %u\n", reg);
> + return ret;
Ditto
> + }
> + } else
> + (*prev)->next = led;
> +
> + *prev = led;
> +
> + return 0;
> +}
> +
> +static int bct3024_of_parse(struct i2c_client *client)
> +{
> + struct bct3024_data *priv = i2c_get_clientdata(client);
> + struct device *dev = priv->dev;
> + struct device_node *np;
> + int ret;
> +
> + ret = of_get_child_count(dev->of_node);
> + if (!ret || ret > ARRAY_SIZE(priv->leds)) {
> + dev_err_probe(dev, -EINVAL, "invalid nodes count: %d\n", ret);
> + return -EINVAL;
Ditto
> + }
> +
> + for_each_child_of_node(dev->of_node, np) {
> + u32 regs[BCT3024_LED_COUNT];
> + struct led_init_data idata = {
> + .fwnode = of_fwnode_handle(np),
> + .devicename = client->name,
> + };
> + struct bct3024_led *led;
> + int count, i;
> +
> + count = of_property_read_variable_u32_array(np, "reg", regs, 1,
> + BCT3024_LED_COUNT);
> + if (count < 0) {
> + ret = count;
> + dev_err_probe(dev, ret, "failed to read node reg\n");
> + goto fail;
Ditto
> + }
> +
> + for (i = 0, led = NULL; i < count; i++) {
> + ret = bct3024_setup_led(priv, regs[i], &led, &idata);
> + if (ret < 0)
> + goto fail;
> + }
> + }
> +
> + return 0;
> +
> +fail:
> + of_node_put(np);
> +
> + return ret;
> +}
> +
> +static int bct3024_i2c_probe(struct i2c_client *client)
> +{
> + struct bct3024_data *priv;
> + struct device *dev = &client->dev;
> + int ret;
> +
> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + priv->dev = dev;
> +
> + priv->supply = devm_regulator_get_optional(dev, "vdd");
> + if (IS_ERR(priv->supply)) {
> + ret = PTR_ERR(priv->supply);
> + if (ret != -ENODEV) {
> + dev_err_probe(dev, ret, "failed to get supply\n");
> + return ret;
Ditto
> + }
> + priv->supply = NULL;
> + }
> +
> + priv->shutdown_gpiod = devm_gpiod_get_optional(dev, "shutdown", GPIOD_OUT_HIGH);
> + if (IS_ERR(priv->shutdown_gpiod)) {
> + ret = PTR_ERR(priv->shutdown_gpiod);
> + dev_err_probe(dev, ret, "failed to get shutdown gpio\n");
> + return ret;
Everywhere...
> + }
> +
> + priv->regmap = devm_regmap_init_i2c(client, &bct3024_regmap);
> + if (IS_ERR(priv->regmap)) {
> + ret = PTR_ERR(priv->regmap);
> + return ret;
It's return PTR_ERR....
> + }
> +
> + mutex_init(&priv->lock);
> + i2c_set_clientdata(client, priv);
> +
> + pm_runtime_set_autosuspend_delay(dev, 2000);
> + pm_runtime_use_autosuspend(dev);
> + pm_runtime_enable(dev);
> + if (!pm_runtime_enabled(dev)) {
> + ret = bct3024_shutdown(priv, false);
This should go to error handling path... Although why? There was no
power on code between devm_regmap_init_i2c() and here.
> + if (ret < 0)
> + return ret;
> + }
> +
> + ret = pm_runtime_resume_and_get(dev);
> + if (ret < 0)
> + goto fail;
> +
> + ret = bct3024_of_parse(client);
> + if (ret < 0)
> + goto fail;
> +
> + pm_runtime_mark_last_busy(dev);
> + pm_runtime_put_autosuspend(dev);
> +
> + return 0;
> +
> +fail:
> + dev_err_probe(dev, ret, "probe failed\n");
No, no no. Do you see any driver doing it?
> + if (!pm_runtime_status_suspended(dev))
> + bct3024_shutdown(priv, 2);
Which call this is reversing? Each step in probe should have its reverse
(when applicable of course). Don't add some new unrelated reverse calls
which do not have a matching enable. If you shutdown here, I expect
there was "bct3024_powerup". Where is it? Looks like you put unrelated
code into the shutdown making it all very difficult to understand.
Where do you reverse PM runtime calls here?
> + return ret;
> +}
> +
> +static void bct3024_i2c_remove(struct i2c_client *client)
> +{
> + struct bct3024_data *priv = i2c_get_clientdata(client);
> +
> + bct3024_shutdown(priv, true);
> + pm_runtime_disable(priv->dev);
> + mutex_destroy(&priv->lock);
> +}
> +
> +static void bct3024_i2c_shutdown(struct i2c_client *client)
> +{
> + struct bct3024_data *priv = i2c_get_clientdata(client);
> +
> + bct3024_shutdown(priv, true);
> +}
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int bct3024_runtime_idle(struct device *dev)
> +{
> + struct bct3024_data *priv = dev_get_drvdata(dev);
> +
> + dev_dbg(dev, "%s: %d\n", __func__, priv->state);
No simple debug statements for entry/exit of functions. There is
extensive trace infrastructure for all this.
> +
> + return priv->state == BCT3024_STATE_ACTIVE ? -EBUSY : 0;
> +}
> +
> +static int bct3024_runtime_suspend(struct device *dev)
> +{
> + struct bct3024_data *priv = dev_get_drvdata(dev);
> +
> + dev_dbg(dev, "%s: %d\n", __func__, priv->state);
Ditto
> +
> + switch (priv->state) {
> + case BCT3024_STATE_INIT:
> + case BCT3024_STATE_SHUTDOWN:
> + return 0;
> + default:
> + break;
> + }
> +
> + return bct3024_shutdown(priv, true);
> +}
> +
> +static int bct3024_runtime_resume(struct device *dev)
> +{
> + struct bct3024_data *priv = dev_get_drvdata(dev);
> + int ret;
> +
> + dev_dbg(dev, "%s: %d\n", __func__, priv->state);
Ditto
> +
> + switch (priv->state) {
> + case BCT3024_STATE_INIT:
> + case BCT3024_STATE_SHUTDOWN:
> + break;
> + default:
> + return 0;
> + }
> +
> + ret = bct3024_shutdown(priv, false);
> + if (ret < 0)
> + bct3024_shutdown(priv, 2);
> +
> + return ret;
> +}
> +#endif
> +
> +static const struct dev_pm_ops bct3024_pm_ops = {
> + SET_RUNTIME_PM_OPS(bct3024_runtime_suspend, bct3024_runtime_resume,
> + bct3024_runtime_idle)
> + SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, pm_runtime_force_resume)
> +};
> +
> +static const struct of_device_id bct3024_of_match[] = {
> + { .compatible = "broadchip,bct3024" },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, bct3024_of_match);
> +
> +static struct i2c_driver bct3024_driver = {
> + .driver = {
> + .name = "bct3024",
> + .of_match_table = bct3024_of_match,
> + .pm = &bct3024_pm_ops,
> + },
> + .probe_new = bct3024_i2c_probe,
> + .shutdown = bct3024_i2c_shutdown,
> + .remove = bct3024_i2c_remove,
> +};
> +
> +module_i2c_driver(bct3024_driver);
> +
> +MODULE_AUTHOR("Matus Gajdos <[email protected]>");
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("Broadchip BCT3024 LED driver");
Best regards,
Krzysztof