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 <matuszpd@xxxxxxxxx>
> ---
> 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 <matuszpd@xxxxxxxxx>");
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("Broadchip BCT3024 LED driver");
Best regards,
Krzysztof
