Re: [PATCH v7 3/3] leds: Add LED1202 I2C driver

[Christophe JAILLET]

Le 18/11/2024 à 15:54, Vicentiu Galanopulo a écrit :
> The output current can be adjusted separately for each channel by 8-bit
> analog (current sink input) and 12-bit digital (PWM) dimming control. The
> LED1202 implements 12 low-side current generators with independent dimming
> control.
> Internal volatile memory allows the user to store up to 8 different patterns,
> each pattern is a particular output configuration in terms of PWM
> duty-cycle (on 4096 steps). Analog dimming (on 256 steps) is per channel but
> common to all patterns. Each device tree LED node will have a corresponding
> entry in /sys/class/leds with the label name. The brightness property
> corresponds to the per channel analog dimming, while the patterns[1-8] to the
> PWM dimming control.
>
> Signed-off-by: Vicentiu Galanopulo <vicentiu.galanopulo@xxxxxxxxxxxxxxxxx>
...

> +static int st1202_write_reg(struct st1202_chip *chip, int reg, uint8_t val)
> +{
> +	return i2c_smbus_write_byte_data(chip->client, reg, val);
> +}

Many duplicated message could be saved if errors where logged directly 
in st1202_write_reg() and st1202_read_reg()

This would simplify a lot the callers that would only need to return 
directly.

> +
> +static uint8_t st1202_prescalar_to_miliseconds(unsigned int value)
> +{
> +	return value/ST1202_MILLIS_PATTERN_DUR_MIN - 1;

Can value be 0?
If yes, should the return value be clamped?

> +}

...

> +static int st1202_dt_init(struct st1202_chip *chip)
> +{
> +	struct device *dev = &chip->client->dev;
> +	struct st1202_led *led;
> +	int err, reg;
> +
> +	for_each_available_child_of_node_scoped(dev_of_node(dev), child) {
> +		struct led_init_data init_data = {};
> +
> +		err = of_property_read_u32(child, "reg", &reg);
> +		if (err) {
> +			of_node_put(child);

The _scoped version of the iterator is used, so I think that this 
of_node_put() is not needed.

> +			return dev_err_probe(dev, err, "Invalid register, error: %d\n", err);

No need to duplicate err. dev_err_probe() already prints it, in a human 
readble form.

> +		}
> +
> +		led = &chip->leds[reg];
> +		led->is_active = true;
> +		led->fwnode = of_fwnode_handle(child);
> +
> +		led->led_cdev.max_brightness = U8_MAX;
> +		led->led_cdev.brightness_set_blocking = st1202_led_set;
> +		led->led_cdev.pattern_set = st1202_led_pattern_set;
> +		led->led_cdev.pattern_clear = st1202_led_pattern_clear;
> +		led->led_cdev.default_trigger = "pattern";
> +
> +		init_data.fwnode = led->fwnode;
> +		init_data.devicename = "st1202";
> +		init_data.default_label = ":";
> +		err = devm_led_classdev_register_ext(dev,
> +					&led->led_cdev, &init_data);
> +		if (err < 0) {
> +			dev_err_probe(dev, err,
> +				"Failed to register LED class dev, error: %d\n", err);

No need to duplicate err. dev_err_probe() already prints it, in a human 
readble form.

Also return dev_err_probe() would save a few lines.

> +			return err;
> +		}
> +		led->led_cdev.brightness_set = st1202_brightness_set;
> +		led->led_cdev.brightness_get = st1202_brightness_get;
> +	}
> +
> +	return 0;
> +}
> +
> +static int st1202_setup(struct st1202_chip *chip)
> +{
> +	int ret;
> +
> +	mutex_lock(&chip->lock);

if guard(mutex)(&chip->lock); was used you could return directly in all 
error paths below.

Same applies in several functions above.

> +	/*
> +	 * Once the supply voltage is applied, the LED1202 executes some internal checks,
> +	 * afterwords it stops the oscillator and puts the internal LDO in quiescent mode.
> +	 * To start the device, EN bit must be set inside the “Device Enable” register at
> +	 * address 01h. As soon as EN is set, the LED1202 loads the adjustment parameters
> +	 * from the internal non-volatile memory and performs an auto-calibration procedure
> +	 * in order to increase the output current precision.
> +	 * Such initialization lasts about 6.5 ms.
> +	 */
> +
> +	/* Reset the chip during setup */
> +	ret = st1202_write_reg(chip, ST1202_DEV_ENABLE, ST1202_DEV_ENABLE_RESET);
> +	if (ret < 0) {
> +		dev_err_probe(&chip->client->dev, -EIO,
> +				"Failed to write to register [0x%x], error: %d\n",
> +				ST1202_DEV_ENABLE, ret);

Why return -EIO if you have an error code in ret?
If you return ret, the ", error: %d\n", ret" part can be removed

Same everywhere in this function

> +		goto exit;
> +	}
> +
> +	/* Enable phase-shift delay feature */
> +	ret = st1202_write_reg(chip, ST1202_CONFIG_REG, ST1202_CONFIG_REG_SHFT);
> +	if (ret < 0) {
> +		dev_err_probe(&chip->client->dev, -EIO,
> +				"Failed to write to register [0x%x], error: %d\n",
> +				ST1202_CONFIG_REG, ret);
> +		goto exit;
> +	}
> +
> +	/* Enable the device */
> +	ret = st1202_write_reg(chip, ST1202_DEV_ENABLE, ST1202_DEV_ENABLE_ON);
> +	if (ret < 0) {
> +		dev_err_probe(&chip->client->dev, -EIO,
> +				"Failed to write to register [0x%x], error: %d\n",
> +				ST1202_DEV_ENABLE, ret);
> +		goto exit;
> +	}
> +
> +	/* Duration of initialization */
> +	usleep_range(6500, 10000);
> +
> +	/* Deactivate all LEDS (channels) and activate only the ones found in Device Tree */
> +	ret = st1202_write_reg(chip, ST1202_CHAN_ENABLE_LOW, ST1202_CHAN_DISABLE_ALL);
> +	if (ret < 0) {
> +		dev_err_probe(&chip->client->dev, -EIO,
> +				"Failed writing to register [0x%x], error: %d\n",
> +				ST1202_CHAN_ENABLE_LOW, ret);
> +		goto exit;
> +	}
> +
> +	ret = st1202_write_reg(chip, ST1202_CHAN_ENABLE_HIGH, ST1202_CHAN_DISABLE_ALL);
> +	if (ret < 0) {
> +		dev_err_probe(&chip->client->dev, -EIO,
> +				"Failed writing to register [0x%x], error: %d\n",
> +				ST1202_CHAN_ENABLE_HIGH, ret);
> +		goto exit;
> +	}
> +
> +	ret = st1202_write_reg(chip, ST1202_CONFIG_REG,
> +				ST1202_CONFIG_REG_PATS | ST1202_CONFIG_REG_PATSR);
> +	if (ret < 0) {
> +		dev_err_probe(&chip->client->dev, -EIO,
> +				"Failed writing to register [0x%lx], error: %d\n",
> +				ST1202_CONFIG_REG_PATS | ST1202_CONFIG_REG_PATSR, ret);
> +		goto exit;
> +	}
> +
> +exit:
> +	mutex_unlock(&chip->lock);
> +	return ret;
> +}
> +
> +static int st1202_probe(struct i2c_client *client)
> +{
> +	struct st1202_chip *chip;
> +	struct st1202_led *led;
> +	int ret;
> +
> +	if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_BYTE_DATA))
> +		return dev_err_probe(&client->dev, -EIO, "SMBUS Byte Data not Supported\n");
> +
> +	chip = devm_kzalloc(&client->dev, sizeof(*chip), GFP_KERNEL);
> +	if (!chip)
> +		return -ENOMEM;
> +
> +	i2c_set_clientdata(client, chip);

Is it needed?
(There is no i2c_get_clientdata())

> +
> +	devm_mutex_init(&client->dev, &chip->lock);
> +	chip->client = client;
> +
> +	ret = st1202_dt_init(chip);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = st1202_setup(chip);
> +	if (ret < 0)
> +		return ret;
> +
> +	for (int i = 0; i < ST1202_MAX_LEDS; i++) {
> +		led = &chip->leds[i];
> +		led->chip = chip;
> +		led->led_num = i;
> +
> +		if (led->is_active) {
> +			ret = st1202_channel_set(led->chip, led->led_num, true);
> +			if (ret < 0) {
> +				return dev_err_probe(&client->dev, -EIO,
> +					"Failed to activate LED channel, error: %d\n", ret);
> +			}

Why return -EIO if you have an error code in ret?
If you return ret, the ", error: %d\n", ret" part can be removed

Also some { } could be removed.

> +
> +			ret = st1202_led_pattern_clear(&led->led_cdev);
> +			if (ret < 0) {
> +				return dev_err_probe(&client->dev, -EIO,
> +					"Failed to clear LED pattern, error: %d\n", ret);

Why return -EIO if you have an error code in ret?
If you return ret, the ", error: %d\n", ret" part can be removed

Also some { } could be removed.

> +			}
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct i2c_device_id st1202_id[] = {
> +	{ "st1202-i2c" },
> +	{ /* sentinel */ },

Nitpick: no need for ending comma after a terminator

> +};
> +MODULE_DEVICE_TABLE(i2c, st1202_id);
> +
> +static const struct of_device_id st1202_dt_ids[] = {
> +	{ .compatible = "st,led1202", },
> +	{ /* sentinel */ },

Nitpick: no need for ending comma after a terminator

> +};
> +MODULE_DEVICE_TABLE(of, st1202_dt_ids);
...

CJ