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 "::". > + 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. -- Best regards, Jacek Anaszewski -- 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
