On Mon, 24 Jun 2024, Vicentiu Galanopulo wrote: > On Thu, Jun 20, 2024 at 06:55:43PM +0100, Lee Jones wrote: > > I'm going do a very quick once through of this one. > > > > There is a lot of work to do. > > > > > The LED1202 is a 12-channel low quiescent > > > > Please line wrap at 70-something chars not 40 > > > > Ok, will do. I'm using Visual Code as editor. > Do you know any config options for it? > If not maybe another editor that is free > and works on Mac ARM. No idea. I use NeoVim (with kickstart.nvim). https://dev.to/lico/set-up-neovim-with-kickstartnvim-on-mac-as-a-vimginner-53f5 > > > current LED driver. 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 active=1 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> > > > --- > > > > > > Changes in v2: > > > - Fix build error for device_attribute modes > > > > > > drivers/leds/Kconfig | 10 + > > > drivers/leds/Makefile | 1 + > > > drivers/leds/leds-led1202.c | 617 ++++++++++++++++++++++++++++++++++++ > > > 3 files changed, 628 insertions(+) > > > create mode 100644 drivers/leds/leds-led1202.c > > > > > > diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig > > > index 05e6af88b88c..c65f2b1bbe30 100644 > > > --- a/drivers/leds/Kconfig > > > +++ b/drivers/leds/Kconfig > > > @@ -899,6 +899,16 @@ config LEDS_LM36274 > > > Say Y to enable the LM36274 LED driver for TI LMU devices. > > > This supports the LED device LM36274. > > > > > > +config LEDS_LED1202 > > > + tristate "LED Support for LED1202 I2C chips" > > > + depends on LEDS_CLASS > > > + depends on I2C > > > + depends on OF > > > + help > > > + Say Y to enable support for LEDs connected to LED1202 > > > + LED driver chips accessed via the I2C bus. > > > + Supported devices include LED1202. > > > + > > > config LEDS_TPS6105X > > > tristate "LED support for TI TPS6105X" > > > depends on LEDS_CLASS > > > diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile > > > index effdfc6f1e95..80423fa8818e 100644 > > > --- a/drivers/leds/Makefile > > > +++ b/drivers/leds/Makefile > > > @@ -36,6 +36,7 @@ obj-$(CONFIG_LEDS_IP30) += leds-ip30.o > > > obj-$(CONFIG_LEDS_IPAQ_MICRO) += leds-ipaq-micro.o > > > obj-$(CONFIG_LEDS_IS31FL319X) += leds-is31fl319x.o > > > obj-$(CONFIG_LEDS_IS31FL32XX) += leds-is31fl32xx.o > > > +obj-$(CONFIG_LEDS_LED1202) += leds-led1202.o > > > obj-$(CONFIG_LEDS_LM3530) += leds-lm3530.o > > > obj-$(CONFIG_LEDS_LM3532) += leds-lm3532.o > > > obj-$(CONFIG_LEDS_LM3533) += leds-lm3533.o > > > diff --git a/drivers/leds/leds-led1202.c b/drivers/leds/leds-led1202.c > > > new file mode 100644 > > > index 000000000000..4e82f0e66168 > > > --- /dev/null > > > +++ b/drivers/leds/leds-led1202.c > > > @@ -0,0 +1,617 @@ > > > +// SPDX-License-Identifier: GPL-2.0-only > > > +/* > > > + * Simple LED driver for ST LED1202 chip > > > > Just a simple 600+ line driver? > > > > With you help maybe I can reduce it. > > > ST as in STMicroelectronics? > > > > Please make that clear in the header and in the Kconfig entry. > > > > Sure, will do. > > > > + * Copyright (C) 2024 Remote-Tech Ltd. UK > > > + */ > > > > New line here. > > > Noted, will add. > > > > +#include <linux/module.h> > > > +#include <linux/string.h> > > > +#include <linux/ctype.h> > > > +#include <linux/leds.h> > > > +#include <linux/err.h> > > > +#include <linux/i2c.h> > > > +#include <linux/slab.h> > > > +#include <linux/gpio.h> > > > +#include <linux/delay.h> > > > + > > > +#define DRIVER_NAME "led-driver-1202" > > > > Remove this and user dev_err() instead. > > > > > +#define DRIVER_VERSION "0.0.1" > > > > Remove this entirely, it's useless. > > > > Will do in v3. Please strip out review comments that you agree with. > > > + > > > +#define LL1202_MAX_LEDS 12 > > > + > > > +#define LL1202_DEVICE_ID 0x00 > > > +#define LL1202_DEV_ENABLE 0x01 > > > +#define LL1202_CHAN_ENABLE_LOW 0x02 > > > +#define LL1202_CHAN_ENABLE_HIGH 0x03 > > > +#define LL1202_CONFIG_REG 0x04 > > > +#define LL1202_ILED_REG0 0x09 > > > +#define LL1202_PATTERN_REP 0x15 > > > +#define LL1202_PATTERN_DUR 0x16 > > > +#define LL1202_PATTERN_PWM 0x1E > > > +#define LL1202_CLOCK_REG 0xE0 > > > > Tab out the values so they line up. > > > OK > > > +struct ll1202_led { > > > + struct led_classdev led_cdev; > > > + struct ll1202_chip *chip; > > > + int led_num; > > > + char name[32]; > > > > Define this and all magic numbers. > > > Ok, there is some bitwise operations to form a 16bit from 8bit values. > And some magic number coming from the datasheet. > Those also? Numbers should be easily identifiable/readable by humans. > > > + int is_active; > > > +}; > > > + > > > +struct ll1202_chip { > > > + struct i2c_client *client; > > > + struct mutex lock; > > > + struct ll1202_led leds[LL1202_MAX_LEDS]; > > > +}; > > > + > > > +static struct ll1202_led *cdev_to_ll1202_led(struct led_classdev *cdev) > > > +{ > > > + return container_of(cdev, struct ll1202_led, led_cdev); > > > +} > > > + > > > +static int ll1202_read_reg(struct ll1202_chip *chip, int reg, uint8_t *val) > > > +{ > > > + int ret = i2c_smbus_read_byte_data(chip->client, reg); > > > > Separate the declaration and the function call. > > > Ok > > > + > > > + if (ret < 0) > > > + return ret; > > > + > > > + *val = (uint8_t)ret; > > > + return 0; > > > +} > > > + > > > +static int ll1202_write_reg(struct ll1202_chip *chip, int reg, uint8_t val) > > > > What's "ll"? > > > > I reused some naming. Should it be led1202_ for all? st1202_? > > > +{ > > > + return i2c_smbus_write_byte_data(chip->client, reg, val); > > > +} > > > + > > > +static int ll1202_get_channel(struct device *dev) > > > +{ > > > + struct device_node *np = dev->parent->of_node, *child; > > > + int err, ret = -1; > > > > What is -1? > > Just negative return value. It's a helper function that returns the LED > channel number based on struct device. > > If this is not appropiate or custom practice I can redo it, but I need some pointers > on where to look as "good" examples. Google: "Linux Error Codes" `git grep "return " -- drivers` > > > + for_each_child_of_node(np, child) { > > > + if (strncmp(dev->kobj.name, > > > + of_get_property(child, "label", NULL), > > > + strnlen(dev->kobj.name, MAX_INPUT)) == 0) { > > > > Pull all of these embedded functions out. > > > Ok. > > > + err = of_property_read_u32(child, "reg", &ret); > > > + if (err) { > > > + of_node_put(child); > > > + pr_err(DRIVER_NAME > > > + ": Failed to read property %s", child->name); > > > + return ret; > > > + } > > > + break; > > > + } > > > + } > > > + return ret; > > > +} > > > + > > > +static ssize_t ll1202_show_all_registers(struct device *dev, > > > + struct device_attribute *devattr, > > > + char *buf) > > > +{ > > > + struct ll1202_chip *chip = dev_get_drvdata(dev); > > > + uint8_t reg_value = 0; > > > + int ret, i; > > > + char *bufp = buf; > > > + > > > + mutex_lock(&chip->lock); > > > + > > > + for (i = LL1202_DEVICE_ID; i <= LL1202_CLOCK_REG; i++) { > > > + ret = ll1202_read_reg(chip, i, ®_value); > > > + if (ret != 0) > > > + dev_err(&chip->client->dev, > > > + "Reading register [0x%x] failed.\n", i); > > > + > > > + bufp += snprintf(bufp, PAGE_SIZE, "Addr[0x%x] = 0x%x\n", i, > > > + reg_value); > > > + } > > > + > > > + mutex_unlock(&chip->lock); > > > + return strlen(buf); > > > +} > > > > Does this have any real world use? > > > > A dump of all the registers with their values. I didn't add show/get functions for > all the registers. > Remove it? How often are people going to need that after initial authorship, really? > > > +static ssize_t > > > +ll1202_show_patt_sequence_repetition(struct device *dev, > > > + struct device_attribute *attr, char *buf) > > > +{ > > > + struct ll1202_chip *chip = dev_get_drvdata(dev); > > > + unsigned int ret; > > > + uint8_t reg_value; > > > + char *bufp = buf; > > > + > > > + mutex_lock(&chip->lock); > > > + ret = ll1202_read_reg(chip, LL1202_PATTERN_REP, ®_value); > > > + if (ret != 0) > > > + dev_err(&chip->client->dev, "Reading register [0x%x] failed\n", LL1202_PATTERN_REP); > > > + mutex_unlock(&chip->lock); > > > + bufp += snprintf(bufp, PAGE_SIZE, > > > + "Pattern sequence register, repetition value = %d (times)\n", > > > + reg_value); > > > + return strlen(buf); > > > +} > > > + > > > +static ssize_t > > > +ll1202_store_patt_sequence_repetition(struct device *dev, > > > + struct device_attribute *attr, > > > + const char *buf, size_t count) > > > +{ > > > + struct ll1202_chip *chip = dev_get_drvdata(dev); > > > + unsigned int ret; > > > + unsigned long duration; > > > + > > > + if (!count) > > > + return -EINVAL; > > > + > > > + ret = kstrtoul(buf, 10, &duration); > > > + if (ret) { > > > + dev_err(&chip->client->dev, "sscanf failed with error :%d\n", > > > + ret); > > > + return ret; > > > + } > > > + > > > + mutex_lock(&chip->lock); > > > + ret = ll1202_write_reg(chip, LL1202_PATTERN_REP, duration); > > > + if (ret != 0) > > > + dev_err(&chip->client->dev, "Writing register [0x%x] failed\n", > > > + LL1202_PATTERN_REP); > > > + mutex_unlock(&chip->lock); > > > + return count; > > > +} > > > + > > > +static int ll1202_prescalar_to_miliamps(uint8_t reg_value) > > > +{ > > > + return reg_value * 20 / 255; > > > > Define _all_ magic numbers. > > > Ok, will be done in v3 > > > +} > > > + > > > +static int ll1202_prescalar_to_miliseconds(uint8_t reg_value) > > > +{ > > > + return reg_value * 5660 / 255; > > > +} > > > + > > > +static ssize_t ll1202_show_channel_mA_current(struct device *dev, > > > + struct device_attribute *attr, > > > + char *buf) > > > +{ > > > + struct ll1202_chip *chip = dev_get_drvdata(dev->parent); > > > + unsigned int ret; > > > + uint8_t reg_value; > > > + char *bufp = buf; > > > + int led_num = ll1202_get_channel(dev); > > > + > > > + if (led_num < 0 || led_num >= LL1202_MAX_LEDS) { > > > + dev_err(&chip->client->dev, > > > + "Invalid register [0x%x] (out of range)\n", > > > + led_num); > > > + } > > > + mutex_lock(&chip->lock); > > > + ret = ll1202_read_reg(chip, LL1202_ILED_REG0 + led_num, ®_value); > > > + if (ret != 0) > > > + dev_err(&chip->client->dev, "Reading analog dimming register [0x%x] failed\n", > > > + led_num); > > > + mutex_unlock(&chip->lock); > > > + bufp += snprintf(bufp, PAGE_SIZE, "Channel[%d] = %d mA\n", led_num, > > > + ll1202_prescalar_to_miliamps(reg_value)); > > > + return strlen(buf); > > > > Space out the code properly - this is really tough to read. > > > Ok.. with or without the help of the IDE, it shall be done I mean new lines between functional groups. > > > +} > > > + > > > +static int ll1202_channel_activate(struct ll1202_led *led) > > > +{ > > > + struct ll1202_chip *chip; > > > + uint8_t reg_chan_low, reg_chan_high; > > > + int ret = 0; > > > + > > > + chip = led->chip; > > > + if (led->is_active) { > > > > Reverse this logic and unindent this block. > > > Sorry, I need some more details on what I need to do here. if (!led->is_active) return ret; > > > + mutex_lock(&chip->lock); > > > + > > > + ret = ll1202_read_reg(chip, LL1202_CHAN_ENABLE_LOW, > > > + ®_chan_low); > > > + if (ret < 0) { > > > + dev_err(&chip->client->dev, > > > + "Failed reading register [0x%x]\n", LL1202_CHAN_ENABLE_LOW); > > > + } > > > + > > > + ret = ll1202_read_reg(chip, LL1202_CHAN_ENABLE_HIGH, > > > + ®_chan_high); > > > + if (ret < 0) { > > > + dev_err(&chip->client->dev, > > > + "Failed reading register [0x%x]\n", LL1202_CHAN_ENABLE_HIGH); > > > + } > > > + > > > + reg_chan_low = reg_chan_low | BIT(led->led_num); > > > + ret = ll1202_write_reg(chip, LL1202_CHAN_ENABLE_LOW, > > > + reg_chan_low); > > > + if (ret < 0) { > > > + dev_err(&chip->client->dev, > > > + "Failed writing to register [0x%x]\n", LL1202_CHAN_ENABLE_LOW); > > > + } > > > + reg_chan_high = reg_chan_high | (BIT(led->led_num) >> 7); > > > + ret = ll1202_write_reg(chip, LL1202_CHAN_ENABLE_HIGH, > > > + reg_chan_high); > > > + if (ret < 0) { > > > + dev_err(&chip->client->dev, > > > + "Failed writing to register [0x%x]\n", LL1202_CHAN_ENABLE_HIGH); > > > + } > > > > Provide a comment as to why this cycle needs to be done twice. > > I will paste the description from the datasheet in v3 > > > > > > + mutex_unlock(&chip->lock); > > > + } > > > + return ret; > > > +} > > > + > > > +#define LL1202_PWM_PATTERN_ATTR(pattern) \ > > > > No chance! > > > > Where else do you see code like this? > > > > The driver version is the one used for our custom f1c200s board. > It's a customization of the lctech,pi-f1c200s. > > The kernel version provided by the SoC vendor is 5.2.0. > https://elixir.bootlin.com/linux/v5.2/source/drivers/leds/leds-bd2802.c > line 317 > > > > > + static ssize_t ll1202_show_pwm_pattern##pattern( \ > > > + struct device *dev, struct device_attribute *attr, char *buf) \ > > > + { \ > > > + struct ll1202_chip *chip = dev_get_drvdata(dev->parent); \ > > > + uint8_t duration = 0; \ > > > + uint8_t reg_value_l = 0; \ > > > + uint8_t reg_value_h = 0; \ > > > + uint16_t reg_value = 0; \ > > > + int ret; \ > > > + char *bufp = buf; \ > > > + int led_num = ll1202_get_channel(dev); \ > > > + if (led_num < 0 || led_num >= LL1202_MAX_LEDS) { \ > > > + dev_err(&chip->client->dev, \ > > > + "Invalid register [0x%x] (out of range)\n", \ > > > + led_num); \ > > > + } \ > > > + mutex_lock(&chip->lock); \ > > > + ret = ll1202_read_reg( \ > > > + chip, \ > > > + (LL1202_PATTERN_PWM + (led_num * 2) + 0x18 * pattern), \ > > > + ®_value_l); \ > > > + if (ret != 0) \ > > > + dev_err(&chip->client->dev, \ > > > + "Reading pattern PWM register [0x%x] failed\n", led_num); \ > > > + ret = ll1202_read_reg(chip, \ > > > + (LL1202_PATTERN_PWM + 0x1 + \ > > > + (led_num * 2) + 0x18 * pattern), \ > > > + ®_value_h); \ > > > + if (ret != 0) \ > > > + dev_err(&chip->client->dev, \ > > > + "Reading pattern PWM register [0x%x] failed\n", led_num); \ > > > + reg_value = (uint16_t)reg_value_h << 8 | reg_value_l; \ > > > + ret = ll1202_read_reg(chip, (LL1202_PATTERN_DUR + pattern), \ > > > + &duration); \ > > > + if (ret != 0) \ > > > + dev_err(&chip->client->dev, \ > > > + "Reading pattern durating register [0x%x] failed\n", led_num); \ > > > + bufp += snprintf( \ > > > + bufp, PAGE_SIZE, \ > > > + "Pattern[%d][cs%d]: PWM = 0x%03X; DURATION = %d ms\n", \ > > > + pattern, led_num, reg_value, \ > > > + ll1202_prescalar_to_miliseconds(duration)); \ > > > + mutex_unlock(&chip->lock); \ > > > + return strlen(buf); \ > > > + } \ > > > + static ssize_t ll1202_store_pwm_pattern##pattern( \ > > > + struct device *dev, struct device_attribute *attr, \ > > > + const char *buf, size_t count) \ > > > + { \ > > > + struct ll1202_chip *chip = dev_get_drvdata(dev->parent); \ > > > + unsigned int ret, reg_value; \ > > > + unsigned long duration; \ > > > + char buf_u8[16]; \ > > > + uint8_t reg_value_l = 0; \ > > > + uint8_t reg_value_h = 0; \ > > > + int led_num = ll1202_get_channel(dev); \ > > > + if (led_num < 0 || led_num >= LL1202_MAX_LEDS) { \ > > > + dev_err(&chip->client->dev, \ > > > + "Invalid register [0x%x] (out of range)\n", \ > > > + led_num); \ > > > + return count; \ > > > + } \ > > > + if (!count) \ > > > + return -EINVAL; \ > > > + ret = sscanf(buf, "%X %s", ®_value, buf_u8); \ > > > + if (ret == 0) { \ > > > + dev_err(&chip->client->dev, \ > > > + "sscanf failed with error :%d\n", ret); \ > > > + return ret; \ > > > + } \ > > > + ret = kstrtoul(buf_u8, 10, &duration); \ > > > + if (ret) \ > > > + return ret; \ > > > + reg_value_l = (uint8_t)reg_value; \ > > > + reg_value_h = (uint8_t)(reg_value >> 8); \ > > > + mutex_lock(&chip->lock); \ > > > + ret = ll1202_write_reg( \ > > > + chip, \ > > > + (LL1202_PATTERN_PWM + (led_num * 2) + 0x18 * pattern), \ > > > + (uint8_t)reg_value_l); \ > > > + if (ret != 0) \ > > > + dev_err(&chip->client->dev, \ > > > + "Writing to register [0x%x] failed, value %d\n", \ > > > + LL1202_PATTERN_PWM + (led_num * 2) + \ > > > + 0x18 * pattern, \ > > > + reg_value_l); \ > > > + ret = ll1202_write_reg(chip, \ > > > + (LL1202_PATTERN_PWM + 0x1 + \ > > > + (led_num * 2) + 0x18 * pattern), \ > > > + (uint8_t)reg_value_h); \ > > > + if (ret != 0) \ > > > + dev_err(&chip->client->dev, \ > > > + "Writing to register [0x%x] failed, value %d\n", \ > > > + (LL1202_PATTERN_PWM + 0x1 + (led_num * 2) + \ > > > + 0x18 * pattern), \ > > > + reg_value_h); \ > > > + ret = ll1202_write_reg(chip, (LL1202_PATTERN_DUR + pattern), \ > > > + (u8)duration); \ > > > + if (ret != 0) \ > > > + dev_err(&chip->client->dev, \ > > > + "Writing to register [0x%x] failed, value %d\n", \ > > > + (LL1202_PATTERN_DUR + pattern), (u8)duration); \ > > > + ret = ll1202_write_reg(chip, LL1202_CONFIG_REG, \ > > > + (0xC0 | pattern)); \ > > > + if (ret != 0) { \ > > > + dev_err(&chip->client->dev, \ > > > + "Failed writing to reg [0x%x]\n", LL1202_CONFIG_REG); \ > > > + } \ > > > + mutex_unlock(&chip->lock); \ > > > + ll1202_channel_activate(&chip->leds[led_num]); \ > > > + return count; \ > > > + } \ > > > + static struct device_attribute dev_attr_led_pwm_pattern##pattern = { \ > > > + .attr = { \ > > > + .name = __stringify(pwm_pattern##pattern), \ > > > + .mode = 00444 | 00200, \ > > > + }, \ > > > + .show = ll1202_show_pwm_pattern##pattern, \ > > > + .store = ll1202_store_pwm_pattern##pattern, \ > > > +} > > > + > > > +LL1202_PWM_PATTERN_ATTR(0); > > > +LL1202_PWM_PATTERN_ATTR(1); > > > +LL1202_PWM_PATTERN_ATTR(2); > > > +LL1202_PWM_PATTERN_ATTR(3); > > > +LL1202_PWM_PATTERN_ATTR(4); > > > +LL1202_PWM_PATTERN_ATTR(5); > > > +LL1202_PWM_PATTERN_ATTR(6); > > > +LL1202_PWM_PATTERN_ATTR(7); > > > > We already have global helpers for this type of thing. > > > Ok, could you please point me to the file/link? I suggest you pull as much of this out to another _normal_ function as you can, then have the fewest lines possible inside the macro instead. > > > +static DEVICE_ATTR(led_device_regsdump, 00444, ll1202_show_all_registers, > > > + NULL); > > > +static DEVICE_ATTR(patt_sequence_repetition, 00444 | 00200, > > > + ll1202_show_patt_sequence_repetition, > > > + ll1202_store_patt_sequence_repetition); > > > +static DEVICE_ATTR(current_mA, 00444, ll1202_show_channel_mA_current, NULL); > > > + > > > +static struct attribute *led_attrs[] = { > > > + &dev_attr_led_device_regsdump.attr, > > > + &dev_attr_patt_sequence_repetition.attr, > > > + NULL, > > > +}; > > > + > > > +static struct attribute *led_group_attrs[] = { > > > + &dev_attr_led_pwm_pattern0.attr, &dev_attr_led_pwm_pattern1.attr, > > > + &dev_attr_led_pwm_pattern2.attr, &dev_attr_led_pwm_pattern3.attr, > > > + &dev_attr_led_pwm_pattern4.attr, &dev_attr_led_pwm_pattern5.attr, > > > + &dev_attr_led_pwm_pattern6.attr, &dev_attr_led_pwm_pattern7.attr, > > > + &dev_attr_current_mA.attr, NULL, > > > +}; > > > + > > > +static struct attribute_group attr_group = { > > > + .attrs = led_attrs, > > > +}; > > > + > > > +static struct attribute_group attr_pat_group = { > > > + .attrs = led_group_attrs, > > > +}; > > > + > > > +static const struct attribute_group *ll1202_groups[] = { &attr_pat_group, > > > + NULL }; > > > + > > > +static void ll1202_brightness_set(struct led_classdev *led_cdev, > > > + enum led_brightness value) > > > +{ > > > + struct ll1202_led *led = cdev_to_ll1202_led(led_cdev); > > > + struct ll1202_chip *chip = led->chip; > > > + int ret; > > > + > > > + mutex_lock(&chip->lock); > > > + ret = ll1202_write_reg(chip, LL1202_ILED_REG0 + led->led_num, value); > > > + if (ret != 0) > > > + dev_err(&chip->client->dev, "Reading register [0x%x] failed\n", > > > + LL1202_ILED_REG0 + led->led_num); > > > + mutex_unlock(&chip->lock); > > > +} > > > + > > > +static enum led_brightness ll1202_brightness_get(struct led_classdev *led_cdev) > > > +{ > > > + struct ll1202_led *led = cdev_to_ll1202_led(led_cdev); > > > + struct ll1202_chip *chip = led->chip; > > > + uint8_t reg_value; > > > + int ret; > > > + > > > + mutex_lock(&chip->lock); > > > + ret = ll1202_read_reg(chip, LL1202_ILED_REG0 + led->led_num, > > > + ®_value); > > > + if (ret != 0) > > > + dev_err(&chip->client->dev, "Reading register [0x%x] failed\n", > > > + LL1202_ILED_REG0 + led->led_num); > > > + > > > + mutex_unlock(&chip->lock); > > > + return reg_value; > > > +} > > > + > > > +static int ll1202_dt_init(struct ll1202_chip *chip) > > > +{ > > > + struct device_node *np = chip->client->dev.of_node, *child; > > > + struct ll1202_led *led; > > > + int err, reg; > > > + > > > + for_each_child_of_node(np, child) { > > > + err = of_property_read_u32(child, "reg", ®); > > > + if (err) { > > > + of_node_put(child); > > > + pr_err(DRIVER_NAME ": Failed to get child node"); > > > + return err; > > > + } > > > + if (reg < 0 || reg >= LL1202_MAX_LEDS) { > > > + of_node_put(child); > > > + pr_err(DRIVER_NAME ": Invalid register value [0x%x] (out of range)", reg); > > > + return -EINVAL; > > > + } > > > + > > > + led = &chip->leds[reg]; > > > + led->led_cdev.name = of_get_property(child, "label", NULL) ?: > > > + child->name; > > > + > > > + err = of_property_read_u32(child, "active", &led->is_active); > > > + if (err) { > > > + of_node_put(child); > > > + pr_err(DRIVER_NAME ": Failed to get child node"); > > > + return err; > > > + } > > > + > > > + led->led_cdev.brightness_set = ll1202_brightness_set; > > > + led->led_cdev.brightness_get = ll1202_brightness_get; > > > + led->led_cdev.groups = ll1202_groups; > > > + } > > > + return 0; > > > +} > > > + > > > +static int ll1202_setup(struct ll1202_chip *chip) > > > +{ > > > + int ret; > > > + > > > + mutex_lock(&chip->lock); > > > + ret = ll1202_write_reg(chip, LL1202_DEV_ENABLE, 0x1); > > > + if (ret < 0) { > > > + dev_err(&chip->client->dev, "Failed writing to register [0x%x]\n", > > > + LL1202_DEV_ENABLE); > > > + } > > > + mutex_unlock(&chip->lock); > > > + usleep_range(6500, 10000); > > > + mutex_lock(&chip->lock); > > > + ret = ll1202_write_reg(chip, LL1202_DEV_ENABLE, 0x80); > > > + if (ret < 0) { > > > + dev_err(&chip->client->dev, "Failed writing to register [0x%x]\n", > > > + LL1202_DEV_ENABLE); > > > + } > > > + mutex_unlock(&chip->lock); > > > + usleep_range(6500, 10000); > > > + mutex_lock(&chip->lock); > > > + ret = ll1202_write_reg(chip, LL1202_PATTERN_REP, 0xFF); > > > + if (ret < 0) { > > > + dev_err(&chip->client->dev, "Failed writing to register [0x%x]\n", > > > + LL1202_PATTERN_REP); > > > + return ret; > > > + } > > > + mutex_unlock(&chip->lock); > > > + return ret; > > > +} > > > + > > > +static int ll1202_probe(struct i2c_client *client) > > > +{ > > > + struct ll1202_chip *chip; > > > + struct ll1202_led *led; > > > + int ret, err; > > > + int i; > > > + > > > + pr_info(DRIVER_NAME ": (I2C) " DRIVER_VERSION "\n"); > > > + > > > + if (!i2c_check_functionality(client->adapter, > > > + I2C_FUNC_SMBUS_BYTE_DATA)) { > > > + dev_err(&client->dev, "SMBUS Byte Data not Supported\n"); > > > + return -EIO; > > > + } > > > + > > > + chip = devm_kzalloc(&client->dev, sizeof(*chip), GFP_KERNEL); > > > + if (!chip) > > > + return -ENOMEM; > > > + > > > + i2c_set_clientdata(client, chip); > > > + > > > + mutex_init(&chip->lock); > > > + chip->client = client; > > > + > > > + /* Device tree setup */ > > > + ret = ll1202_dt_init(chip); > > > + if (ret < 0) > > > + goto exit; > > > + > > > + /* Configuration setup */ > > > + ret = ll1202_setup(chip); > > > + if (ret < 0) > > > + goto exit; > > > + > > > + for (i = 0; i < LL1202_MAX_LEDS; i++) { > > > + led = &chip->leds[i]; > > > + led->chip = chip; > > > + led->led_num = i; > > > + if (led->is_active) { > > > + err = led_classdev_register(&client->dev, > > > + &led->led_cdev); > > > + if (err < 0) { > > > + pr_err(DRIVER_NAME > > > + ": Failed to register LED class dev"); > > > + goto exit; > > > + } > > > + } > > > + } > > > + > > > + ret = sysfs_create_group(&client->dev.kobj, &attr_group); > > > + if (ret) { > > > + dev_err(&client->dev, > > > + "Failed to create sysfs group for ll1202\n"); > > > + goto err_setup; > > > + } > > > + > > > + return 0; > > > + > > > +err_setup: > > > + for (i = 0; i < LL1202_MAX_LEDS; i++) > > > + led_classdev_unregister(&chip->leds[i].led_cdev); > > > +exit: > > > + mutex_destroy(&chip->lock); > > > + devm_kfree(&client->dev, chip); > > > + return ret; > > > +} > > > + > > > +static void ll1202_remove(struct i2c_client *client) > > > +{ > > > + struct ll1202_chip *dev = i2c_get_clientdata(client); > > > + int i; > > > + > > > + for (i = 0; i < LL1202_MAX_LEDS; i++) > > > + led_classdev_unregister(&dev->leds[i].led_cdev); > > > + > > > + sysfs_remove_group(&client->dev.kobj, &attr_group); > > > + > > > + mutex_destroy(&dev->lock); > > > + devm_kfree(&client->dev, dev->leds); > > > + devm_kfree(&client->dev, dev); > > > +} > > > + > > > +static const struct i2c_device_id ll1202_id[] = { > > > + { DRIVER_NAME "-i2c", 0 }, > > > + {} > > > +}; > > > + > > > +MODULE_DEVICE_TABLE(i2c, ll1202_id); > > > + > > > +static const struct of_device_id ll1202_dt_ids[] = { > > > + { > > > + .compatible = "st,led1202", > > > + }, > > > +}; > > > + > > > +MODULE_DEVICE_TABLE(of, ll1202_dt_ids); > > > + > > > +static struct i2c_driver ll1202_driver = { > > > + .driver = { > > > + .name = "ll1202", > > > + .of_match_table = of_match_ptr(ll1202_dt_ids), > > > + }, > > > + .probe = ll1202_probe, > > > + .remove = ll1202_remove, > > > + .id_table = ll1202_id, > > > +}; > > > + > > > +module_i2c_driver(ll1202_driver); > > > + > > > +MODULE_AUTHOR("Remote Tech LTD"); > > > +MODULE_DESCRIPTION("LED1202 : 12-channel constant current LED driver"); > > > +MODULE_LICENSE("GPL"); > > > -- > > > 2.25.1 > > > > > Thank you Lee for the review! > > Kind regards, > Vicentiu > > > > > -- > > Lee Jones [李琼斯] -- Lee Jones [李琼斯]
