Re: [PATCH v5 2/2] leds: add ktd202x driver

[Christophe JAILLET <christophe.jaillet@xxxxxxxxxx>]

Le 01/10/2023 à 15:52, André Apitzsch a écrit :
> This commit adds support for Kinetic KTD2026/7 RGB/White LED driver.
>
> Signed-off-by: André Apitzsch <git-AtRKszJ1oGPsq35pWSNszA@xxxxxxxxxxxxxxxx>

...

> +static int ktd202x_setup_led_rgb(struct ktd202x *chip, struct device_node *np,
> +				 struct ktd202x_led *led, struct led_init_data *init_data)
> +{
> +	struct led_classdev *cdev;
> +	struct device_node *child;
> +	struct mc_subled *info;
> +	int num_channels;
> +	int i = 0;
> +	u32 reg;
> +	int ret;
> +
> +	num_channels = of_get_available_child_count(np);
> +	if (!num_channels || num_channels > chip->num_leds)
> +		return -EINVAL;
> +
> +	info = devm_kcalloc(chip->dev, num_channels, sizeof(*info), GFP_KERNEL);
> +	if (!info)
> +		return -ENOMEM;
> +
> +	for_each_available_child_of_node(np, child) {
> +		u32 mono_color = 0;

Un-needed init.
And, why is it defined here, while reg is defined out-side the loop?

> +
> +		ret = of_property_read_u32(child, "reg", &reg);
> +		if (ret != 0 || reg >= chip->num_leds) {
> +			dev_err(chip->dev, "invalid 'reg' of %pOFn\n", np);

Mossing of_node_put(np);?

> +			return -EINVAL;
> +		}
> +
> +		ret = of_property_read_u32(child, "color", &mono_color);
> +		if (ret < 0 && ret != -EINVAL) {
> +			dev_err(chip->dev, "failed to parse 'color' of %pOF\n", np);

Mossing of_node_put(np);?

> +			return ret;
> +		}
> +
> +		info[i].color_index = mono_color;
> +		info[i].channel = reg;
> +		info[i].intensity = KTD202X_MAX_BRIGHTNESS;
> +		i++;
> +	}
> +
> +	led->mcdev.subled_info = info;
> +	led->mcdev.num_colors = num_channels;
> +
> +	cdev = &led->mcdev.led_cdev;
> +	cdev->brightness_set_blocking = ktd202x_brightness_mc_set;
> +	cdev->blink_set = ktd202x_blink_mc_set;
> +
> +	return devm_led_classdev_multicolor_register_ext(chip->dev, &led->mcdev, init_data);
> +}
> +
> +static int ktd202x_setup_led_single(struct ktd202x *chip, struct device_node *np,
> +				    struct ktd202x_led *led, struct led_init_data *init_data)
> +{
> +	struct led_classdev *cdev;
> +	u32 reg;
> +	int ret;
> +
> +	ret = of_property_read_u32(np, "reg", &reg);
> +	if (ret != 0 || reg >= chip->num_leds) {
> +		dev_err(chip->dev, "invalid 'reg' of %pOFn\n", np);
> +		return -EINVAL;
> +	}
> +	led->index = reg;
> +
> +	cdev = &led->cdev;
> +	cdev->brightness_set_blocking = ktd202x_brightness_single_set;
> +	cdev->blink_set = ktd202x_blink_single_set;
> +
> +	return devm_led_classdev_register_ext(chip->dev, &led->cdev, init_data);
> +}
> +
> +static int ktd202x_add_led(struct ktd202x *chip, struct device_node *np, unsigned int index)
> +{
> +	struct ktd202x_led *led = &chip->leds[index];
> +	struct led_init_data init_data = {};
> +	struct led_classdev *cdev;
> +	u32 color = 0;
Un-needed init.

> +	int ret;
> +
> +	/* Color property is optional in single color case */
> +	ret = of_property_read_u32(np, "color", &color);
> +	if (ret < 0 && ret != -EINVAL) {
> +		dev_err(chip->dev, "failed to parse 'color' of %pOF\n", np);
> +		return ret;
> +	}
> +
> +	led->chip = chip;
> +	init_data.fwnode = of_fwnode_handle(np);
> +
> +	if (color == LED_COLOR_ID_RGB) {
> +		cdev = &led->mcdev.led_cdev;
> +		ret = ktd202x_setup_led_rgb(chip, np, led, &init_data);
> +	} else {
> +		cdev = &led->cdev;
> +		ret = ktd202x_setup_led_single(chip, np, led, &init_data);
> +	}
> +
> +	if (ret) {
> +		dev_err(chip->dev, "unable to register %s\n", cdev->name);
> +		of_node_put(np);

This is strange to have it here.
Why not above after "if (ret < 0 && ret != -EINVAL) {"?

It would look much more natural to have it a few lines below, ... [1]

> +		return ret;
> +	}
> +
> +	cdev->max_brightness = KTD202X_MAX_BRIGHTNESS;
> +
> +	return 0;
> +}
> +
> +static int ktd202x_probe_dt(struct ktd202x *chip)
> +{
> +	struct device_node *np = dev_of_node(chip->dev), *child;
> +	unsigned int i;
> +	int count, ret;
> +
> +	chip->num_leds = (int)(unsigned long)of_device_get_match_data(chip->dev);
> +
> +	count = of_get_available_child_count(np);
> +	if (!count || count > chip->num_leds)
> +		return -EINVAL;
> +
> +	regmap_write(chip->regmap, KTD202X_REG_RESET_CONTROL, KTD202X_RSTR_RESET);
> +
> +	/* Allow the device to execute the complete reset */
> +	usleep_range(200, 300);
> +
> +	i = 0;
> +	for_each_available_child_of_node(np, child) {
> +		ret = ktd202x_add_led(chip, child, i);
> +		if (ret)

[1] ... here.

Otherwise, it is likely that, thanks to a static checker, an additionnal 
of_node_put() will be added on early exit of the loop.

CJ

> +			return ret;
> +		i++;
> +	}
> +
> +	return 0;
> +}

...