Re: [PATCH 2/2] leds: Add support for rohm,bd65b60 led driver

[Krzysztof Kozlowski]

On 08/03/2023 21:14, Bogdan Ionescu wrote:
> This commit adds support for ROHM BD65B60 led driver.

Do not use "This commit/patch".
https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95

> The chip supports 2 outpus sharing the same current setting
> and is controlled over I2C.
> 
> Signed-off-by: Bogdan Ionescu <bogdan.ionescu.work@xxxxxxxxx>
> ---

Thank you for your patch. There is something to discuss/improve.

> +
> +	/* Check required properties */
> +	if (!fwnode_property_present(child, "rohm,enable-outputs")) {
> +		dev_err(dev, "No rohm,enable-outputs property found");
> +		return -ENOENT;

The property is not required by your binding, so you cannot bail here.

> +	}
> +
> +	ret = fwnode_property_read_u32(child, "rohm,enable-outputs", &led->enable);
> +	if (ret || (led->enable & LEDSEL_MASK) != led->enable) {
> +		dev_err(dev, "Failed to read rohm,enable-outputs property");

No need to check for properties twice...

> +		return ret;
> +	}
> +
> +	/* Check optional properties */
> +	led->state = BD65B60_OFF;
> +	if (!fwnode_property_present(child, "default-state")) {
> +		ret = fwnode_property_read_string(child, "default-state",
> +						(const char **)&default_state);
> +		if (ret) {
> +			dev_err(dev, "Failed to read default-state property");
> +			return ret;
> +		}
> +
> +		if (strcmp(default_state, "keep") == 0) {
> +			led->state = BD65B60_KEEP;
> +		} else if (strcmp(default_state, "on") == 0) {
> +			led->state = BD65B60_ON;
> +		} else if (strcmp(default_state, "off") == 0) {
> +			led->state = BD65B60_OFF;
> +		} else {
> +			dev_err(dev, "Invalid default-state property");
> +			return -EINVAL;
> +		}
> +	}
> +
> +	led->ovp = BD65B60_DEFAULT_OVP_VAL;
> +	if (fwnode_property_present(child, "rohm,ovp")) {
> +		ret = fwnode_property_read_u32(child, "rohm,ovp", &led->ovp);
> +
> +		if (ret || (led->ovp & OVP_MASK) != led->ovp) {
> +			dev_err(dev, "Failed to read rohm,ovp property");
> +			return ret;
> +		}
> +	}
> +
> +	*fwnode = child;
> +
> +	return 0;
> +}
> +
> +static int bd65b60_probe(struct i2c_client *client)
> +{
> +	struct bd65b60_led *led;
> +	struct led_init_data init_data = {};
> +	struct fwnode_handle *fwnode = NULL;
> +	int ret;
> +
> +	led = devm_kzalloc(&client->dev, sizeof(*led), GFP_KERNEL);
> +	if (!led)
> +		return -ENOMEM;
> +
> +	led->client = client;
> +	i2c_set_clientdata(client, led);
> +
> +	ret = bd65b60_parse_dt(led, &fwnode);
> +	if (ret)
> +		return ret;
> +
> +	led->cdev.name = BD65B60_DEFAULT_NAME;
> +	led->cdev.brightness_set = bd65b60_brightness_set;
> +	led->cdev.brightness = BD65B60_DEFAULT_BRIGHTNESS;
> +	led->cdev.max_brightness = BD65B60_MAX_BRIGHTNESS;
> +	led->cdev.default_trigger = BD65B60_DEFAULT_TRIGGER;
> +	led->client = client;
> +
> +	led->regmap = devm_regmap_init_i2c(client, &bd65b60_regmap_config);
> +	if (IS_ERR(led->regmap)) {
> +		ret = PTR_ERR(led->regmap);
> +		dev_err(&client->dev, "Failed to allocate register map: %d",
> +			ret);

return dev_err_probe

> +		return ret;
> +	}
> +
> +	mutex_init(&led->lock);
> +
> +	ret = bd65b60_init(led);
> +	if (ret) {
> +		dev_err(&client->dev, "Failed to initialize led: %d", ret);

return dev_err_probe

> +		mutex_destroy(&led->lock);

or ret = dev_err_probe and goto to common cleanup part

> +		return ret;
> +	}
> +
> +	init_data.fwnode = fwnode;
> +	init_data.devicename = led->client->name;
> +	init_data.default_label = ":";
> +	ret = devm_led_classdev_register_ext(&client->dev, &led->cdev,
> +					     &init_data);
> +	if (ret) {
> +		dev_err(&client->dev, "Failed to register led: %d", ret);

return dev_err_probe

> +		mutex_destroy(&led->lock);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static void bd65b60_remove(struct i2c_client *client)
> +{
> +	int ret;
> +	struct bd65b60_led *led = i2c_get_clientdata(client);
> +
> +	ret = regmap_write(led->regmap, REG_PON, BD65B60_OFF);
> +	if (ret)
> +		dev_err(&client->dev, "Failed to turn off led: %d", ret);
> +
> +	mutex_destroy(&led->lock);
> +}
> +
> +static const struct i2c_device_id bd65b60_id[] = {
> +	{ "bd65b60", 0 },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(i2c, bd65b60_id);
> +
> +static const struct of_device_id of_bd65b60_leds_match[] = {
> +	{ .compatible = "rohm,bd65b60" },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, of_bd65b60_leds_match);
> +
> +static struct i2c_driver bd65b60_i2c_driver = {
> +	.driver = {
> +		.name = "bd65b60",
> +		.owner = THIS_MODULE,
> +		.of_match_table = of_match_ptr(of_bd65b60_leds_match),

Drop of_match_ptr. It requires maybe_unused which you do not have, so
this will cause warnings.

Best regards,
Krzysztof