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", ®);
+ 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
