From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tony Subject: Re: [PATCH v10 1/1] leds: LED driver for TI LP3952 6-Channel Color LED Date: Mon, 11 Jul 2016 13:20:54 +0100 Message-ID: <57838F26.9050100@daqri.com> References: <1468234129-26890-1-git-send-email-tony.makkiel@daqri.com> <57838227.7070401@samsung.com> <5783857B.1030208@daqri.com> <57838C2D.4060607@samsung.com> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <57838C2D.4060607@samsung.com> Sender: linux-leds-owner@vger.kernel.org To: Jacek Anaszewski 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 Thank you for the comments Jacek. On 11/07/16 13:08, Jacek Anaszewski wrote: >>>> @@ -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. Yes please :). Thank you for your help. > >>>> +} >>>> + >>>> +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 >> >> > >