Hi Jacek, Thanks for the review! On Tue, Feb 20, 2018 at 10:51:40PM +0100, Jacek Anaszewski wrote: > Hi Simon, > > Thank you for the patch. Please refer to my comments below. > > On 02/20/2018 01:54 AM, Simon Shields wrote: > > AN30259A is a 3-channel LED driver which uses I2C. It supports timed > > operation via an internal PWM clock, and variable brightness. This > > driver offers support for basic hardware-based blinking and brightness > > control. > > > > The datasheet is freely available: > > https://www.alliedelec.com/m/d/a9d2b3ee87c2d1a535a41dd747b1c247.pdf > > > > Signed-off-by: Simon Shields <simon@xxxxxxxxxxxxx> > > --- > > drivers/leds/Kconfig | 10 ++ > > drivers/leds/Makefile | 1 + > > drivers/leds/leds-an30259a.c | 338 +++++++++++++++++++++++++++++++++++++++++++ > > 3 files changed, 349 insertions(+) > > create mode 100644 drivers/leds/leds-an30259a.c > > > > diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig > > index 3e763d2a0cb3..80bed557cc6b 100644 > > --- a/drivers/leds/Kconfig > > +++ b/drivers/leds/Kconfig > > @@ -57,6 +57,16 @@ config LEDS_AAT1290 > > depends on PINCTRL > > help > > This option enables support for the LEDs on the AAT1290. > > + > > +config LEDS_AN30259A > > + tristate "LED support for Panasonic AN30259A" > > + depends on OF > > + depends on I2C > > + depends on LEDS_CLASS > > + help > > + This option enables support for the AN30259A 3-channel > > + LED driver. > > + > > config LEDS_APU > > tristate "Front panel LED support for PC Engines APU/APU2 boards" > > depends on LEDS_CLASS > > diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile > > index 987884a5b9a5..44f9b42d1600 100644 > > --- a/drivers/leds/Makefile > > +++ b/drivers/leds/Makefile > > @@ -11,6 +11,7 @@ obj-$(CONFIG_LEDS_88PM860X) += leds-88pm860x.o > > obj-$(CONFIG_LEDS_AAT1290) += leds-aat1290.o > > obj-$(CONFIG_LEDS_APU) += leds-apu.o > > obj-$(CONFIG_LEDS_AS3645A) += leds-as3645a.o > > +obj-$(CONFIG_LEDS_AN30259A) += leds-an30259a.o > > obj-$(CONFIG_LEDS_BCM6328) += leds-bcm6328.o > > obj-$(CONFIG_LEDS_BCM6358) += leds-bcm6358.o > > obj-$(CONFIG_LEDS_BD2802) += leds-bd2802.o > > diff --git a/drivers/leds/leds-an30259a.c b/drivers/leds/leds-an30259a.c > > new file mode 100644 > > index 000000000000..b51faf67e7de > > --- /dev/null > > +++ b/drivers/leds/leds-an30259a.c > > @@ -0,0 +1,338 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > You could mention yourself as an Author here too. > > > +#include <linux/i2c.h> > > +#include <linux/leds.h> > > +#include <linux/module.h> > > +#include <linux/mutex.h> > > +#include <linux/of.h> > > +#include <linux/regmap.h> > > + > > +#define MAX_LEDS 3 > > + > > +#define REG_SRESET 0x00 > > +#define LED_SRESET (1 << 0) > > + > > +/* LED power registers */ > > +#define REG_LEDON 0x01 > > +#define LED_EN(x) (1 << (x)) > > +#define LED_SLOPE(x) (1 << ((x) + 4)) > > + > > +#define REG_LEDCC(x) (0x03 + (x)) > > + > > +/* slope control registers */ > > +#define REG_SLOPE(x) (0x06 + (x)) > > +#define LED_SLOPETIME1(x) (x) > > +#define LED_SLOPETIME2(x) ((x) << 4) > > + > > +#define REG_LEDCNT1(x) (0x09 + (4 * (x))) > > +#define LED_DUTYMAX(x) ((x) << 4) > > +#define LED_DUTYMID(x) (x) > > + > > +#define REG_LEDCNT2(x) (0x0A + (4 * (x))) > > +#define LED_DELAY(x) ((x) << 4) > > +#define LED_DUTYMIN(x) (x) > > + > > +/* detention time control (length of each slope step) */ > > +#define REG_LEDCNT3(x) (0x0B + (4 * (x))) > > +#define LED_DT1(x) (x) > > +#define LED_DT2(x) ((x) << 4) > > + > > +#define REG_LEDCNT4(x) (0x0C + (4 * (x))) > > +#define LED_DT3(x) (x) > > +#define LED_DT4(x) ((x) << 4) > > + > > +#define REG_MAX 0x14 > > + > > +#define BLINK_MAX_TIME 7500 /* ms */ > > + > > +struct an30259a; > > + > > +struct an30259a_led { > > + struct an30259a *chip; > > + struct led_classdev cdev; > > + int num; > > + char name[32]; > > +}; > > + > > +struct an30259a { > > + struct mutex mutex; > > + struct i2c_client *client; > > + struct an30259a_led leds[MAX_LEDS]; > > + struct regmap *regmap; > > +}; > > + > > +static u8 an30259a_get_duty_max(u8 brightness) > > +{ > > + u8 duty_max, floor, ceil; > > + > > + /* squash 8 bit number into 7-bit PWM range */ > > + duty_max = brightness >> 1; > > + > > + /* bottom 3 bits are always set for DUTYMAX > > + * figure out the closest value > > + */ > > + ceil = duty_max | 0x7; > > + floor = ceil - 0x8; > > + > > + if ((duty_max - floor) < (ceil - duty_max)) > > + duty_max = floor >> 3; > > + else > > + duty_max = ceil >> 3; > > + > > + return duty_max; > > +} > > + > > +static int an30259a_brightness(struct an30259a_led *led, > > + enum led_brightness brightness) > > +{ > > + int ret; > > + unsigned int ledon; > > + u8 duty_max; > > + > > + ret = regmap_read(led->chip->regmap, REG_LEDON, &ledon); > > Please put an empty line here - it improves readability. > > > + switch (brightness) { > > + case LED_OFF: > > + ledon &= ~LED_EN(led->num); > > + ledon &= ~LED_SLOPE(led->num); > > + break; > > + default: > > + ledon |= LED_EN(led->num); > > + duty_max = an30259a_get_duty_max(brightness & 0xff); > > + ret = regmap_write(led->chip->regmap, > > + REG_LEDCNT1(led->num), > > + LED_DUTYMAX(duty_max) | LED_DUTYMID(duty_max)); > > + if (ret) > > + return ret; > > + break; > > + } > > + ret = regmap_write(led->chip->regmap, REG_LEDON, > > + ledon); > > + if (ret) > > + return ret; > > + > > + ret = regmap_write(led->chip->regmap, REG_LEDCC(led->num), > > + brightness & 0xff); > > + if (ret) > > + return ret; > > + > > + return ret; > > +} > > + > > +static int an30259a_blink(struct an30259a_led *led, > > + unsigned long *poff, unsigned long *pon) > > +{ > > + int ret, num = led->num; > > + unsigned int ledon; > > + unsigned long off = *poff, on = *pon; > > + > > + /* slope time - multiples of 500ms only */ > > + off -= off % 500; > > + if (!off) > > + off += 500; > > + else if (off > BLINK_MAX_TIME) > > + off = BLINK_MAX_TIME; > > + *poff = off; > > + off /= 500; > > + > > + on -= on % 500; > > + if (!on) > > + on += 500; > > + else if (on > BLINK_MAX_TIME) > > + on = BLINK_MAX_TIME; > > + *pon = on; > > + on /= 500; > > + > > + /* duty min should be zero (=off), delay should be zero */ > > + ret = regmap_write(led->chip->regmap, REG_LEDCNT2(num), > > + LED_DELAY(0) | LED_DUTYMIN(0)); > > + if (ret) > > + return ret; > > + > > + /* reset detention time (no "breathing" effect) */ > > + ret = regmap_write(led->chip->regmap, REG_LEDCNT3(num), > > + LED_DT1(0) | LED_DT2(0)); > > + if (ret) > > + return ret; > > + ret = regmap_write(led->chip->regmap, REG_LEDCNT4(num), > > + LED_DT3(0) | LED_DT4(0)); > > + if (ret) > > + return ret; > > + > > + /* slope time controls on/off cycle length */ > > + ret = regmap_write(led->chip->regmap, REG_SLOPE(num), > > + LED_SLOPETIME1(off) | LED_SLOPETIME2(on)); > > + if (ret) > > + return ret; > > + > > + /* Finally, enable slope mode. */ > > + ret = regmap_read(led->chip->regmap, REG_LEDON, &ledon); > > + if (ret) > > + return ret; > > + ledon |= LED_SLOPE(num); > > Empty line here please. > > > + return regmap_write(led->chip->regmap, REG_LEDON, > > + ledon); > > +} > > + > > +static int an30259a_led_set(struct led_classdev *cdev, > > + enum led_brightness value) > > +{ > > + struct an30259a_led *led; > > + int ret; > > + > > + led = container_of(cdev, struct an30259a_led, cdev); > > + > > + mutex_lock(&led->chip->mutex); > > + > > + ret = an30259a_brightness(led, value); > > + > > + mutex_unlock(&led->chip->mutex); > > + return ret; > > +} > > + > > +static int an30259a_blink_set(struct led_classdev *cdev, > > + unsigned long *delay_off, unsigned long *delay_on) > > +{ > > + struct an30259a_led *led; > > + int ret; > > + > > + led = container_of(cdev, struct an30259a_led, cdev); > > + > > + mutex_lock(&led->chip->mutex); > > + > > + ret = an30259a_blink(led, delay_off, delay_on); > > + > > + mutex_unlock(&led->chip->mutex); > > + return ret; > > +} > > + > > +static struct led_platform_data * > > +an30259a_dt_init(struct i2c_client *client) > > +{ > > + struct led_platform_data *pdata; > > + struct device_node *np = client->dev.of_node, *child; > > + struct led_info *leds; > > + int count; > > + int res; > > + > > + count = of_get_child_count(np); > > + if (!count || count > MAX_LEDS) > > + return ERR_PTR(-EINVAL); > > + > > + leds = devm_kzalloc(&client->dev, sizeof(*leds) * count, GFP_KERNEL); > > + if (!leds) > > + return ERR_PTR(-ENOMEM); > > + > > + for_each_child_of_node(np, child) { > > + u32 source; > > + > > + res = of_property_read_u32(child, "led-sources", &source); > > + if ((res != 0) || source >= MAX_LEDS) > > + continue; > > + > > + leds[source].name = of_get_property(child, "label", NULL); > > + leds[source].default_trigger = > > + of_get_property(child, "linux,default-trigger", NULL); > > + } > > + > > + pdata = devm_kzalloc(&client->dev, sizeof(*pdata), GFP_KERNEL); > > + if (!pdata) > > + return ERR_PTR(-ENOMEM); > > + > > + pdata->leds = leds; > > + pdata->num_leds = MAX_LEDS; > > + > > + return pdata; > > +} > > + > > +static const struct regmap_config an30259a_regmap_config = { > > + .reg_bits = 8, > > + .val_bits = 8, > > + .max_register = REG_MAX, > > +}; > > + > > +static int an30259a_probe(struct i2c_client *client) > > +{ > > + struct led_platform_data *pdata; > > + struct an30259a *chip; > > + int i, err; > > + > > + pdata = an30259a_dt_init(client); > > + if (IS_ERR(pdata)) > > + return PTR_ERR(pdata); > > + > > + chip = devm_kzalloc(&client->dev, sizeof(*chip), GFP_KERNEL); > > + if (!chip) > > + return -ENOMEM; > > + > > + mutex_init(&chip->mutex); > > + chip->client = client; > > + chip->regmap = devm_regmap_init_i2c(client, &an30259a_regmap_config); > > + > > + i2c_set_clientdata(client, chip); > > + > > + /* Reset the IC */ > > + regmap_write(chip->regmap, REG_SRESET, LED_SRESET); > > + > > + for (i = 0; i < pdata->num_leds; i++) { > > + chip->leds[i].num = i; > > + chip->leds[i].chip = chip; > > + > > + if (pdata->leds[i].name) > > + snprintf(chip->leds[i].name, sizeof(chip->leds[i].name), > > + "an30259a:%s", pdata->leds[i].name); > > + else > > + snprintf(chip->leds[i].name, sizeof(chip->leds[i].name), > > + "an30259a:%d:%.2x:%d", > > We should have color in the second segment of the LED class device name. > If label is absent, please just leave it blank "::". How should we handle the case where two LEDs have an empty name (or the same name)? > > > + client->adapter->nr, client->addr, i); > > + if (pdata->leds[i].default_trigger) > > + chip->leds[i].cdev.default_trigger = > > + pdata->leds[i].default_trigger; > > + > > + chip->leds[i].cdev.name = chip->leds[i].name; > > + > > + chip->leds[i].cdev.brightness_set_blocking = an30259a_led_set; > > + chip->leds[i].cdev.blink_set = an30259a_blink_set; > > + > > + err = led_classdev_register(&client->dev, &chip->leds[i].cdev); > > Please use devm prefixed API here. > > > + if (err < 0) > > + goto exit; > > + } > > + return 0; > > + > > +exit: > > + while (i--) > > + led_classdev_unregister(&chip->leds[i].cdev); > > + return err; > > +} > > + > > +static int an30259a_remove(struct i2c_client *client) > > +{ > > + struct an30259a *chip = i2c_get_clientdata(client); > > + int i; > > + > > + for (i = 0; i < MAX_LEDS; i++) > > + led_classdev_unregister(&chip->leds[i].cdev) > > You won't need this loop then, but please add mutex_destroy(). > > > + > > + return 0; > > +} > > + > > +static const struct of_device_id an30259a_match_table[] = { > > + { .compatible = "panasonic,an30259a", }, > > + { /* sentinel */ }, > > +}; > > + > > +MODULE_DEVICE_TABLE(of, an30259a_match_table); > > + > > +static struct i2c_driver an30259a_driver = { > > + .driver = { > > + .name = "leds-an32059a", > > + .of_match_table = of_match_ptr(an30259a_match_table), > > + }, > > + .probe_new = an30259a_probe, > > + .remove = an30259a_remove, > > +}; > > + > > +module_i2c_driver(an30259a_driver); > > + > > +MODULE_AUTHOR("Simon Shields <simon@xxxxxxxxxxxxx>"); > > +MODULE_DESCRIPTION("AN32059A LED driver"); > > +MODULE_LICENSE("GPL v2"); > > > > You could consider also addressing some (all?) of problems > reported by scripts/checkpatch.pl --strict. Ack, will do :) > > -- > Best regards, > Jacek Anaszewski Cheers, Simon -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
