Message-ID: <68dbe6b1-891c-76d4-7e5f-97cb6cc81b51@linaro.org>
Date:   Fri, 28 Jul 2023 09:41:53 +0200
From:   Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To:     Matus Gajdos <matuszpd@...il.com>, Pavel Machek <pavel@....cz>,
        Lee Jones <lee@...nel.org>, Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Conor Dooley <conor+dt@...nel.org>
Cc:     linux-leds@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/3] leds: Add Broadchip BCT3024 LED driver

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@...il.com>
> ---
>  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@...il.com>");
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("Broadchip BCT3024 LED driver");

Best regards,
Krzysztof

Powered by blists - more mailing lists