From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jacek Anaszewski Subject: Re: [PATCH v10 1/1] leds: LED driver for TI LP3952 6-Channel Color LED Date: Mon, 11 Jul 2016 14:08:13 +0200 Message-ID: <57838C2D.4060607@samsung.com> References: <1468234129-26890-1-git-send-email-tony.makkiel@daqri.com> <57838227.7070401@samsung.com> <5783857B.1030208@daqri.com> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Return-path: In-reply-to: <5783857B.1030208@daqri.com> Sender: linux-leds-owner@vger.kernel.org To: Tony Cc: linux-leds@vger.kernel.org, linux-acpi@vger.kernel.org, rpurdie@rpsys.net, rjw@rjwysocki.net, lenb@kernel.org, mika.westerberg@linux.intel.com List-Id: linux-acpi@vger.kernel.org On 07/11/2016 01:39 PM, Tony wrote: > > > On 11/07/16 12:25, Jacek Anaszewski wrote: >> On 07/11/2016 12:48 PM, Tony Makkiel wrote: >>> The chip can drive 2 sets of RGB leds. Controller can >>> be controlled via PWM, I2C and audio synchronisation. >>> This driver uses I2C to communicate with the chip. >>> >>> Datasheet: http://www.ti.com/lit/gpn/lp3952 >>> >>> Signed-off-by: Tony Makkiel >>> --- >>> >>> Changes from v9: >>> - Add dependancy on GPIOLIB. >>> - make on/off variable of lp3952_on_off function bool. >>> >>> Changes from v8: >>> - Removed macro LP3952_ACPI_NAME. >>> - Replaced acpi_dev_get_property with device_property_read_string >>> - Skip led class registration if led data couldn't be read from >>> ACPI table. >>> >>> >>> drivers/leds/Kconfig | 14 ++ >>> drivers/leds/Makefile | 1 + >>> drivers/leds/leds-lp3952.c | 306 >>> ++++++++++++++++++++++++++++++++++++++++++++ >>> include/linux/leds-lp3952.h | 125 ++++++++++++++++++ >>> 4 files changed, 446 insertions(+) >>> create mode 100644 drivers/leds/leds-lp3952.c >>> create mode 100644 include/linux/leds-lp3952.h >>> >>> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig >>> index 5ae2834..9dcc9b1 100644 >>> --- a/drivers/leds/Kconfig >>> +++ b/drivers/leds/Kconfig >>> @@ -228,6 +228,20 @@ config LEDS_LP3944 >>> To compile this driver as a module, choose M here: the >>> module will be called leds-lp3944. >>> >>> +config LEDS_LP3952 >>> + tristate "LED Support for TI LP3952 2 channel LED driver" >>> + depends on LEDS_CLASS >>> + depends on I2C >>> + depends on ACPI >>> + depends on GPIOLIB >> >> Any specific reason for choosing "depends" instead of "select"? > > select causes "recursive dependency error". > > > drivers/gpio/Kconfig:34:error: recursive dependency detected! > For a resolution refer to Documentation/kbuild/kconfig-language.txt > subsection "Kconfig recursive dependency limitations" > drivers/gpio/Kconfig:34: symbol GPIOLIB is selected by LEDS_LP3952 > For a resolution refer to Documentation/kbuild/kconfig-language.txt > subsection "Kconfig recursive dependency limitations" > drivers/leds/Kconfig:231: symbol LEDS_LP3952 depends on NEW_LEDS > For a resolution refer to Documentation/kbuild/kconfig-language.txt > subsection "Kconfig recursive dependency limitations" > drivers/leds/Kconfig:8: symbol NEW_LEDS is selected by BCMA_DRIVER_GPIO > For a resolution refer to Documentation/kbuild/kconfig-language.txt > subsection "Kconfig recursive dependency limitations" > drivers/bcma/Kconfig:97: symbol BCMA_DRIVER_GPIO depends on GPIOLIB I didn't get this with my configuration, but it seems that it is likely to happen. Let's abide by "depends on" then. Besides, some redundant brackets are left, see below. >> >>> + select REGMAP_I2C >>> + help >>> + This option enables support for LEDs connected to the Texas >>> + Instruments LP3952 LED driver. >>> + >>> + To compile this driver as a module, choose M here: the >>> + module will be called leds-lp3952. >>> + >>> config LEDS_LP55XX_COMMON >>> tristate "Common Driver for TI/National >>> LP5521/5523/55231/5562/8501" >>> depends on LEDS_LP5521 || LEDS_LP5523 || LEDS_LP5562 || >>> LEDS_LP8501 >>> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile >>> index cb2013d..0684c86 100644 >>> --- a/drivers/leds/Makefile >>> +++ b/drivers/leds/Makefile >>> @@ -26,6 +26,7 @@ obj-$(CONFIG_LEDS_PCA9532) += leds-pca9532.o >>> obj-$(CONFIG_LEDS_GPIO_REGISTER) += leds-gpio-register.o >>> obj-$(CONFIG_LEDS_GPIO) += leds-gpio.o >>> obj-$(CONFIG_LEDS_LP3944) += leds-lp3944.o >>> +obj-$(CONFIG_LEDS_LP3952) += leds-lp3952.o >>> obj-$(CONFIG_LEDS_LP55XX_COMMON) += leds-lp55xx-common.o >>> obj-$(CONFIG_LEDS_LP5521) += leds-lp5521.o >>> obj-$(CONFIG_LEDS_LP5523) += leds-lp5523.o >>> diff --git a/drivers/leds/leds-lp3952.c b/drivers/leds/leds-lp3952.c >>> new file mode 100644 >>> index 0000000..c0eb2f4 >>> --- /dev/null >>> +++ b/drivers/leds/leds-lp3952.c >>> @@ -0,0 +1,306 @@ >>> +/* >>> + * LED driver for TI lp3952 controller >>> + * >>> + * Copyright (C) 2016, DAQRI, LLC. >>> + * Author: Tony Makkiel >>> + * >>> + * This program is free software; you can redistribute it and/or modify >>> + * it under the terms of the GNU General Public License version 2 as >>> + * published by the Free Software Foundation. >>> + * >>> + */ >>> + >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> + >>> +static int lp3952_register_write(struct i2c_client *client, u8 reg, >>> u8 val) >>> +{ >>> + int ret; >>> + struct lp3952_led_array *priv = i2c_get_clientdata(client); >>> + >>> + ret = regmap_write(priv->regmap, reg, val); >>> + >>> + if (ret) >>> + dev_err(&client->dev, "%s: reg 0x%x, val 0x%x, err %d\n", >>> + __func__, reg, val, ret); >>> + return ret; >>> +} >>> + >>> +static void lp3952_on_off(struct lp3952_led_array *priv, >>> + enum lp3952_leds led_id, bool on) >>> +{ >>> + int ret, val; >>> + >>> + dev_dbg(&priv->client->dev, "%s LED %d to %d\n", __func__, >>> led_id, on); >>> + >>> + val = 1 << led_id; >>> + if (led_id == LP3952_LED_ALL) >>> + val = LP3952_LED_MASK_ALL; >>> + >>> + ret = regmap_update_bits(priv->regmap, LP3952_REG_LED_CTRL, val, >>> + on ? val : 0); >>> + if (ret) >>> + dev_err(&priv->client->dev, "%s, Error %d\n", __func__, ret); >>> +} >>> + >>> +/* >>> + * Using Imax to control brightness. There are 4 possible >>> + * setting 25, 50, 75 and 100 % of Imax. Possible values are >>> + * values 0-4. 0 meaning turn off. >>> + */ >>> +static int lp3952_set_brightness(struct led_classdev *cdev, >>> + enum led_brightness value) >>> +{ >>> + unsigned int reg, shift_val; >>> + struct lp3952_ctrl_hdl *led = container_of(cdev, >>> + struct lp3952_ctrl_hdl, >>> + cdev); >>> + struct lp3952_led_array *priv = (struct lp3952_led_array >>> *)led->priv; >>> + >>> + dev_dbg(cdev->dev, "Brightness request: %d on %d\n", value, >>> + led->channel); >>> + >>> + if (value == LED_OFF) { >>> + lp3952_on_off(priv, led->channel, false); >>> + return 0; >>> + } >>> + >>> + if (led->channel > LP3952_RED_1) { >>> + dev_err(cdev->dev, " %s Invalid LED requested", __func__); >>> + return -EINVAL; >>> + } >>> + >>> + if (led->channel >= LP3952_BLUE_1) { >>> + reg = LP3952_REG_RGB1_MAX_I_CTRL; >>> + shift_val = (led->channel - LP3952_BLUE_1) * 2; >>> + } else { >>> + reg = LP3952_REG_RGB2_MAX_I_CTRL; >>> + shift_val = led->channel * 2; >>> + } >>> + >>> + /* Enable the LED in case it is not enabled already */ >>> + lp3952_on_off(priv, led->channel, true); >>> + >>> + return regmap_update_bits(priv->regmap, reg, 3 << shift_val, >>> + --value << shift_val); >>> +} >>> + >>> +static int lp3952_get_label(struct device *dev, const char *label, >>> char *dest) >>> +{ >>> + int ret; >>> + const char *str; >>> + >>> + ret = device_property_read_string(dev, label, &str); >>> + if (!ret) >>> + strncpy(dest, str, LP3952_LABEL_MAX_LEN); >>> + >>> + return ret; >>> +} >>> + >>> +static int lp3952_register_led_classdev(struct lp3952_led_array *priv) >>> +{ >>> + int i, acpi_ret, ret = -ENODEV; >>> + static const char *led_name_hdl[LP3952_LED_ALL] = { >>> + "blue2", >>> + "green2", >>> + "red2", >>> + "blue1", >>> + "green1", >>> + "red1" >>> + }; >>> + >>> + for (i = 0; i < LP3952_LED_ALL; i++) { >>> + acpi_ret = lp3952_get_label(&priv->client->dev, >>> led_name_hdl[i], >>> + priv->leds[i].name); >>> + if (acpi_ret) >>> + continue; >>> + >>> + priv->leds[i].cdev.name = priv->leds[i].name; >>> + priv->leds[i].cdev.brightness = LED_OFF; >>> + priv->leds[i].cdev.max_brightness = LP3952_BRIGHT_MAX; >>> + priv->leds[i].cdev.brightness_set_blocking = >>> + lp3952_set_brightness; >>> + priv->leds[i].channel = i; >>> + priv->leds[i].priv = priv; >>> + >>> + ret = devm_led_classdev_register(&priv->client->dev, >>> + &priv->leds[i].cdev); >>> + if (ret < 0) { >>> + dev_err(&priv->client->dev, >>> + "couldn't register LED %s\n", >>> + priv->leds[i].cdev.name); >>> + break; >>> + } >>> + } >>> + return ret; >>> +} >>> + >>> +static int lp3952_set_pattern_gen_cmd(struct lp3952_led_array *priv, >>> + u8 cmd_index, u8 r, u8 g, u8 b, >>> + enum lp3952_tt tt, enum lp3952_cet cet) >>> +{ >>> + int ret; >>> + struct ptrn_gen_cmd line = { >>> + .r = r, >>> + .g = g, >>> + .b = b, >>> + .cet = cet, >>> + .tt = tt >>> + }; >>> + >>> + if (cmd_index >= LP3952_CMD_REG_COUNT) >>> + return -EINVAL; >>> + >>> + ret = lp3952_register_write(priv->client, >>> + LP3952_REG_CMD_0 + cmd_index * 2, >>> + line.bytes.msb); >>> + if (ret) >>> + return ret; >>> + >>> + return (lp3952_register_write(priv->client, >>> + LP3952_REG_CMD_0 + cmd_index * 2 + 1, >>> + line.bytes.lsb)); These brackets are redundant. >>> +} >>> + >>> +static int lp3952_configure(struct lp3952_led_array *priv) >>> +{ >>> + int ret; >>> + >>> + /* Disable any LEDs on from any previous conf. */ >>> + ret = lp3952_register_write(priv->client, LP3952_REG_LED_CTRL, 0); >>> + if (ret) >>> + return ret; >>> + >>> + /* enable rgb patter, loop */ >>> + ret = lp3952_register_write(priv->client, LP3952_REG_PAT_GEN_CTRL, >>> + LP3952_PATRN_LOOP | LP3952_PATRN_GEN_EN); >>> + if (ret) >>> + return ret; >>> + >>> + /* Update Bit 6 (Active mode), Select both Led sets, Bit [1:0] */ >>> + ret = lp3952_register_write(priv->client, LP3952_REG_ENABLES, >>> + LP3952_ACTIVE_MODE | LP3952_INT_B00ST_LDR); >>> + if (ret) >>> + return ret; >>> + >>> + /* Set Cmd1 for RGB intensity,cmd and transition time */ >>> + return (lp3952_set_pattern_gen_cmd(priv, 0, I46, I71, I100, TT0, >>> + CET197)); Ditto. If you agree I can remove them and take the patch. >>> +} >>> + >>> +static const struct regmap_config lp3952_regmap = { >>> + .reg_bits = 8, >>> + .val_bits = 8, >>> + .max_register = REG_MAX, >>> + .cache_type = REGCACHE_RBTREE, >>> +}; >>> + >>> +static int lp3952_probe(struct i2c_client *client, >>> + const struct i2c_device_id *id) >>> +{ >>> + int status; >>> + struct lp3952_ctrl_hdl *leds; >>> + struct lp3952_led_array *priv; >>> + >>> + priv = devm_kzalloc(&client->dev, sizeof(*priv), GFP_KERNEL); >>> + if (!priv) >>> + return -ENOMEM; >>> + >>> + leds = devm_kcalloc(&client->dev, LP3952_LED_ALL, sizeof(*leds), >>> + GFP_KERNEL); >>> + if (!leds) { >>> + devm_kfree(&client->dev, priv); >>> + return -ENOMEM; >>> + } >>> + >>> + priv->leds = leds; >>> + priv->client = client; >>> + >>> + priv->enable_gpio = devm_gpiod_get(&client->dev, "nrst", >>> + GPIOD_OUT_HIGH); >>> + if (IS_ERR(priv->enable_gpio)) { >>> + status = PTR_ERR(priv->enable_gpio); >>> + dev_err(&client->dev, "Failed to enable gpio: %d\n", status); >>> + return status; >>> + } >>> + >>> + priv->regmap = devm_regmap_init_i2c(client, &lp3952_regmap); >>> + if (IS_ERR(priv->regmap)) { >>> + int err = PTR_ERR(priv->regmap); >>> + >>> + dev_err(&client->dev, "Failed to allocate register map: %d\n", >>> + err); >>> + return err; >>> + } >>> + >>> + i2c_set_clientdata(client, priv); >>> + >>> + status = lp3952_configure(priv); >>> + if (status) { >>> + dev_err(&client->dev, "Probe failed. Device not found (%d)\n", >>> + status); >>> + return status; >>> + } >>> + >>> + status = lp3952_register_led_classdev(priv); >>> + if (status) { >>> + dev_err(&client->dev, "Unable to register led_classdev: %d\n", >>> + status); >>> + return status; >>> + } >>> + >>> + return 0; >>> +} >>> + >>> +static int lp3952_remove(struct i2c_client *client) >>> +{ >>> + struct lp3952_led_array *priv; >>> + >>> + priv = i2c_get_clientdata(client); >>> + lp3952_on_off(priv, LP3952_LED_ALL, false); >>> + gpiod_set_value(priv->enable_gpio, 0); >>> + >>> + return 0; >>> +} >>> + >>> +static const struct i2c_device_id lp3952_id[] = { >>> + {LP3952_NAME, 0}, >>> + {} >>> +}; >>> + >>> +#ifdef CONFIG_ACPI >>> +static const struct acpi_device_id lp3952_acpi_match[] = { >>> + {"TXNW3952", 0}, >>> + {} >>> +}; >>> + >>> +MODULE_DEVICE_TABLE(acpi, lp3952_acpi_match); >>> +#endif >>> + >>> +static struct i2c_driver lp3952_i2c_driver = { >>> + .driver = { >>> + .name = LP3952_NAME, >>> + .acpi_match_table = ACPI_PTR(lp3952_acpi_match), >>> + }, >>> + .probe = lp3952_probe, >>> + .remove = lp3952_remove, >>> + .id_table = lp3952_id, >>> +}; >>> + >>> +module_i2c_driver(lp3952_i2c_driver); >>> + >>> +MODULE_AUTHOR("Tony Makkiel "); >>> +MODULE_DESCRIPTION("lp3952 I2C LED controller driver"); >>> +MODULE_LICENSE("GPL v2"); >>> diff --git a/include/linux/leds-lp3952.h b/include/linux/leds-lp3952.h >>> new file mode 100644 >>> index 0000000..edd5ed6 >>> --- /dev/null >>> +++ b/include/linux/leds-lp3952.h >>> @@ -0,0 +1,125 @@ >>> +/* >>> + * LED driver for TI lp3952 controller >>> + * >>> + * Copyright (C) 2016, DAQRI, LLC. >>> + * Author: Tony Makkiel >>> + * >>> + * This program is free software; you can redistribute it and/or modify >>> + * it under the terms of the GNU General Public License version 2 as >>> + * published by the Free Software Foundation. >>> + * >>> + */ >>> + >>> +#ifndef LEDS_LP3952_H_ >>> +#define LEDS_LP3952_H_ >>> + >>> +#define LP3952_NAME "lp3952" >>> +#define LP3952_CMD_REG_COUNT 8 >>> +#define LP3952_BRIGHT_MAX 4 >>> +#define LP3952_LABEL_MAX_LEN 15 >>> + >>> +#define LP3952_REG_LED_CTRL 0x00 >>> +#define LP3952_REG_R1_BLNK_TIME_CTRL 0x01 >>> +#define LP3952_REG_R1_BLNK_CYCLE_CTRL 0x02 >>> +#define LP3952_REG_G1_BLNK_TIME_CTRL 0x03 >>> +#define LP3952_REG_G1_BLNK_CYCLE_CTRL 0x04 >>> +#define LP3952_REG_B1_BLNK_TIME_CTRL 0x05 >>> +#define LP3952_REG_B1_BLNK_CYCLE_CTRL 0x06 >>> +#define LP3952_REG_ENABLES 0x0B >>> +#define LP3952_REG_PAT_GEN_CTRL 0x11 >>> +#define LP3952_REG_RGB1_MAX_I_CTRL 0x12 >>> +#define LP3952_REG_RGB2_MAX_I_CTRL 0x13 >>> +#define LP3952_REG_CMD_0 0x50 >>> +#define LP3952_REG_RESET 0x60 >>> +#define REG_MAX LP3952_REG_RESET >>> + >>> +#define LP3952_PATRN_LOOP BIT(1) >>> +#define LP3952_PATRN_GEN_EN BIT(2) >>> +#define LP3952_INT_B00ST_LDR BIT(2) >>> +#define LP3952_ACTIVE_MODE BIT(6) >>> +#define LP3952_LED_MASK_ALL 0x3f >>> + >>> +/* Transition Time in ms */ >>> +enum lp3952_tt { >>> + TT0, >>> + TT55, >>> + TT110, >>> + TT221, >>> + TT422, >>> + TT885, >>> + TT1770, >>> + TT3539 >>> +}; >>> + >>> +/* Command Execution Time in ms */ >>> +enum lp3952_cet { >>> + CET197, >>> + CET393, >>> + CET590, >>> + CET786, >>> + CET1180, >>> + CET1376, >>> + CET1573, >>> + CET1769, >>> + CET1966, >>> + CET2163, >>> + CET2359, >>> + CET2556, >>> + CET2763, >>> + CET2949, >>> + CET3146 >>> +}; >>> + >>> +/* Max Current in % */ >>> +enum lp3952_colour_I_log_0 { >>> + I0, >>> + I7, >>> + I14, >>> + I21, >>> + I32, >>> + I46, >>> + I71, >>> + I100 >>> +}; >>> + >>> +enum lp3952_leds { >>> + LP3952_BLUE_2, >>> + LP3952_GREEN_2, >>> + LP3952_RED_2, >>> + LP3952_BLUE_1, >>> + LP3952_GREEN_1, >>> + LP3952_RED_1, >>> + LP3952_LED_ALL >>> +}; >>> + >>> +struct lp3952_ctrl_hdl { >>> + struct led_classdev cdev; >>> + char name[LP3952_LABEL_MAX_LEN]; >>> + enum lp3952_leds channel; >>> + void *priv; >>> +}; >>> + >>> +struct ptrn_gen_cmd { >>> + union { >>> + struct { >>> + u16 tt:3; >>> + u16 b:3; >>> + u16 cet:4; >>> + u16 g:3; >>> + u16 r:3; >>> + }; >>> + struct { >>> + u8 lsb; >>> + u8 msb; >>> + } bytes; >>> + }; >>> +} __packed; >>> + >>> +struct lp3952_led_array { >>> + struct regmap *regmap; >>> + struct i2c_client *client; >>> + struct gpio_desc *enable_gpio; >>> + struct lp3952_ctrl_hdl *leds; >>> +}; >>> + >>> +#endif /* LEDS_LP3952_H_ */ >>> >> >> > -- > To unsubscribe from this list: send the line "unsubscribe linux-leds" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > > -- Best regards, Jacek Anaszewski