Hi Tony, On 05/31/2016 05:31 PM, Tony Makkiel wrote:
Thank you for the comments Jacek. I have done all the changes suggested, except some on which, needed some clarification please. On 30/05/16 13:51, Jacek Anaszewski wrote:Hi Tony, Thanks for the patch. Please refer to my comments in the code. Please address also all issues filed by scripts/checkpatch.pl --strict. On 05/26/2016 12:26 PM, Tony Makkiel wrote:Datasheet: http://www.ti.com/lit/gpn/lp3952Please put this line at the end of the commit message.The chip can drive 2 sets of RGB leds. Controller can be controlled via PWM, I2C and audio sychnronisation.s/sychnronisation/synchronisation/This driver use I2C to communicate with chip.s/use/uses/ s/chip/the chip/Signed-off-by: Tony Makkiel <tony.makkiel@xxxxxxxxx> --- drivers/leds/Kconfig | 12 ++ drivers/leds/Makefile | 1 + drivers/leds/leds-lp3952.c | 411 ++++++++++++++++++++++++++++++++++++++++++++ include/linux/leds-lp3952.h | 89 ++++++++++ 4 files changed, 513 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..305faf9 100644 --- a/drivers/leds/Kconfig +++ b/drivers/leds/Kconfig @@ -228,6 +228,18 @@ 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 + 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..8229d5a --- /dev/null +++ b/drivers/leds/leds-lp3952.c @@ -0,0 +1,411 @@ +/* + * Copyright (c) 2016, DAQRI, LLC. + * + * License Terms: GNU General Public License v2 + * + * leds-lp3952 - LED class driver for TI lp3952 controller + * + * Based on: + * leds-tlc9516 - LED class driver for TLC95XX series + * of I2C driven LED controllers from NXP + * + * Derived from leds-atmel-pwm, leds-bd2802 and initial PWM led 3952 + * driver written by Alex Feinman + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + */ + +/*#define DEBUG 1*/This seems to be stray debug definition. Let's drop it.+ +#include <linux/io.h> +#include <linux/pm.h> +#include <linux/i2c.h> +#include <linux/gpio.h> +#include <linux/leds.h> +#include <linux/acpi.h> +#include <linux/delay.h> +#include <linux/module.h> +#include <linux/kernel.h> +#include <linux/reboot.h> +#include <linux/regmap.h> +#include <linux/notifier.h> +#include <linux/platform_device.h> +#include <linux/leds-lp3952.h>Please arrange include directives in the alphabetical order.Have arranged them in alphabetical order. But have left <linux/leds-lp3952.h> at the end as it is specific to this file. Is that ok? Or should that also follow the order?
Let's treat all the includes in the same way.
+ +#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 + +struct lp3952_ctrl_hdl { + struct led_classdev cdev; + char name[10]; + 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; + }; +} __attribute__ ((packed)); + +struct lp3952_led_array { + u8 ndev; + u32 enable_gpio; + struct regmap *regmap; + struct i2c_client *client; + struct ptrn_gen_cmd pgm[8];Why 8? Please add a macro definition for this constant.+ struct lp3952_ctrl_hdl *leds; +}; + +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; +} + +int lp3952_register_read(struct i2c_client *client, u8 reg, u8 len, + u8 *valarray) +{ + struct lp3952_led_array *priv = i2c_get_clientdata(client); + int ret = regmap_bulk_read(priv->regmap, reg, valarray, len); + + if (ret < 0) + dev_err(&client->dev, "%s: reg 0x%x, val 0x%x, err %d\n", + __func__, reg, valarray[0], ret); + return ret; +}This function seems to be unused.Removed the function.+ +static void lp3952_on_off(struct lp3952_led_array *priv, enum lp3952_leds led, + int on) +{ + int val = 1 << led; + + dev_dbg(&priv->client->dev, "%s LED %d to %d\n", __func__, led, on);Please add empty line here.+ if (LP3952_LED_ALL == led) + val = 0x3f; + + regmap_update_bits(priv->regmap, LP3952_REG_LED_CTRL, val, + on ? val : 0);Please check return value.+} + +/* + * 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 lp3952_brightness b) +{I'd prefer something more meaningful than 'b'. brightness or value?+ unsigned int reg, val, shiftVal; + 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", b, led->channel); + + if (LP3952_OFF == b) { + lp3952_on_off(priv, led->channel, LP3952_OFF); + return 0; + } + + val = b - 1; + reg = LP3952_REG_RGB1_MAX_I_CTRL; + shiftVal = (led->channel - LP3952_BLUE_1) * 2; + + if (led->channel > LP3952_RED_1) { + dev_err(cdev->dev, " %s Invalid LED requested", __func__); + return -EINVAL; + } else if (led->channel < LP3952_BLUE_1) { + shiftVal = led->channel * 2; + reg = LP3952_REG_RGB2_MAX_I_CTRL; + }Empty line here+ /* Enable the LED in case it is not enabled already */ + lp3952_on_off(priv, led->channel, 1);and here would improve readability.+ return (regmap_update_bits(priv->regmap, reg, 3 << shiftVal, + val << shiftVal)); +} + +static void lp3952_brightness_ctrl(struct led_classdev *cdev, + enum led_brightness b)brightness_set_blocking op returns int.+{ + int range = LED_HALF / 2; + enum lp3952_brightness val = LP3952_FULL_BRIGHT; + + dev_dbg(cdev->dev, "Requested Brightness %d\n", b); + + if (LED_OFF == b) + val = LP3952_OFF; + else if (b < (LED_HALF - range)) + val = LP3952_QUARTER_BRIGHT; + else if (b <= LED_HALF) + val = LP3952_HALF_BRIGHT; + else if (b <= LED_HALF + range) + val = LP3952_THREE_QRTR_BRIGHT;You should operate directly on brightness levels, i.e. write the brightness level passed from user space directly to the device. enum lp3952_brightness is redundant IMO.The chip supports 5 brightness values, hence lp3952_brightness. The brightness register takes values between 0-3 (2 bits). This function should convert user requested legacy values 0-255 to a value within the register range.
You have four active brightness levels: 00, 01, 10, 11. If you initialized max_brightness to 4, then you wouldn't need any translation. There is max_brightness sysfs attribute that says what is the maximum allowed brightness level. LED_FULL is a legacy enum value. Apart of that, 2-bit color intensity resolution is very poor. Actually this is a resolution of max current for each LED. I can see that pattern control registers allow to set 3-bit intensity for each color. I assume that that the same can't be applied for non-pattern use cases?
+ + lp3952_set_brightness(cdev, val); +} + +static int lp3952_register_led_classdev(struct lp3952_led_array *priv) +{ + int ret, i; + const char *led_name[] = { + "blue2", + "green2", + "red2", + "blue1", + "green1", + "red1" + };According to Documentation/leds/leds-class.txt LED class device name should match following pattern: devicename:colour:function. Colour can be omitted if not relevant.+ for (i = 0; i < priv->ndev; i++) { + sprintf(priv->leds[i].name, "%s", led_name[i]); + priv->leds[i].cdev.name = priv->leds[i].name; + priv->leds[i].cdev.brightness = LED_OFF; + priv->leds[i].cdev.max_brightness = LED_FULL;max_brightness should reflect the maximum brightness level allowed for given LED. LED_FULL is a legacy enum, that max_brightness property overrides.lp3952_brightness_ctrl should convert the legacy value to value understood by the chip. The function ( + lp3952_set_brightness) should convert users max request 255 to chips max supported value 3.
You don't need the conversion as I explained above.
+ priv->leds[i].cdev.brightness_set_blocking = + lp3952_brightness_ctrl; + priv->leds[i].channel = i; + priv->leds[i].priv = priv; + + ret = led_classdev_register(&priv->client->dev, + &priv->leds[i].cdev);Please use devm prefixed version.+ if (ret < 0) { + dev_err(&priv->client->dev, + "couldn't register LED %s\n", + priv->leds[i].cdev.name); + goto error; + } + } + return 0; + +error: + for (; i >= 0; i--) + led_classdev_unregister(&priv->leds[i].cdev); + return ret; +} + +static void lp3952_unregister_led_classdev(struct lp3952_led_array *priv) +{ + int i; + + for (i = 0; i < priv->ndev; i++) + led_classdev_unregister(&priv->leds[i].cdev); +} + +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) +{ + struct ptrn_gen_cmd line = { + .r = r, + .g = g, + .b = b, + .cet = cet, + .tt = tt + }; + if (cmd_index >= 8) + return -EINVAL; + + priv->pgm[cmd_index] = line; + lp3952_register_write(priv->client, LP3952_REG_CMD_0 + cmd_index * 2, + line.bytes.msb);Please check return value.+ return (lp3952_register_write(priv->client, + LP3952_REG_CMD_0 + cmd_index * 2 + 1, + line.bytes.lsb)); +} + +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;Empty line here please.+ /* enable rgb patter, loop */ + lp3952_register_write(priv->client, LP3952_REG_PAT_GEN_CTRL, 0x06);Please check return value and add empty line after that.+ /* Update Bit 6 (Active mode),1 and 0 for pattern Gen */ + ret = regmap_update_bits(priv->regmap, LP3952_REG_ENABLES, 0x43, 0xfc);Please add bit definitions for all the register bit fields.+ if (ret) + return ret;Empty line here please.+ /* Set Cmd1 for RGB intensity,cmd and transition time */ + return (lp3952_set_pattern_gen_cmd(priv, 0, I46, I71, I100, TT0, + CET197)); +} + +static const struct regmap_config lp3952_regmap = { + .reg_bits = 8, + .val_bits = 8, + .max_register = REG_MAX, +}; + +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; + struct lp3952_platform_data *pdata = dev_get_platdata(&client->dev); + + dev_info(&client->dev, "lp3952 probe\n");I believe this is stray debug log. Please remove it.+ + if (!pdata) { + dev_err(&client->dev, + "lp3952: failed to obtain platform_data\n"); + return -EINVAL; + } + priv = kzalloc(sizeof(struct lp3952_led_array), GFP_KERNEL);Please use devm prefixed version.+ if (!priv) + return -ENOMEM; + + priv->ndev = 6; + + leds = kcalloc(priv->ndev, sizeof(*leds), GFP_KERNEL);Please use devm prefixed version.+ if (!leds) { + kfree(priv); + return -ENOMEM; + } + priv->leds = leds; + priv->client = client; + priv->enable_gpio = pdata->enable_gpio; + 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; + }Empty line here please.+ status = gpio_request(priv->enable_gpio, "lp3952_gpio"); + if (status) + dev_warn(&client->dev, "Unable to disable reset gpio: %d\n", + status);Empty line here please.+ status = gpio_direction_output(priv->enable_gpio, 1); + if (status) + dev_warn(&client->dev, "Unable to disable reset gpio: %d\n", + status);Empty line here please.+ i2c_set_clientdata(client, priv); + + status = lp3952_configure(priv); + if (status) { + dev_err(&client->dev, "Probe failed. Device not found (%d)\n", + status); + kfree(leds); + kfree(priv); + return status; + }Empty line here please.+ lp3952_register_led_classdev(priv);Please check return value.+ + lp3952_on_off(priv, LP3952_LED_ALL, 1);If it is possible then device should remain in power down mode if brightness equals 0. And this should be secured before the LED class device is registered. Preferrably to be moved to the lp3952_configure().+ 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, 0); + + lp3952_unregister_led_classdev(priv); + + kfree(priv->leds); + gpio_direction_input(priv->enable_gpio); + gpio_free(priv->enable_gpio); + kfree(priv); + return 0; +} + +#ifdef CONFIG_PM +static int lp3952_suspend(struct device *dev) +{ + struct i2c_client *client = to_i2c_client(dev); + struct lp3952_led_array *priv = i2c_get_clientdata(client); + + lp3952_on_off(priv, LP3952_LED_ALL, 0); + gpio_direction_input(priv->enable_gpio); + return 0; +} + +static int lp3952_resume(struct device *dev) +{ + struct i2c_client *client = to_i2c_client(dev); + struct lp3952_led_array *priv = i2c_get_clientdata(client); + + gpio_direction_output(priv->enable_gpio, 1); + return 0; +}Power management is already handled in led-class.c. Please refer to led_suspend(). It sets brightness to 0, and thus the LED class drivers are expected to turn the devices they control into power down mode in case all LEDs controlled by them are set to LED_OFF. Therefore you should control the "enable_gpio" state in the brightness_set_blocking() op.Removed the suspend/resume routine within the driver. Power management in led-class.c should be enough.+ +static SIMPLE_DEV_PM_OPS(lp3952_pm, lp3952_suspend, lp3952_resume); +#define LP3952_PM (&lp3952_pm) +#else /* CONFIG_PM */ +#define LP3952_PM NULL +#endif + +#ifdef CONFIG_ACPI +static const struct acpi_device_id lp3952_acpi_match[] = { + {LP3952_NAME, 0}, + {} +}; + +MODULE_DEVICE_TABLE(acpi, lp3952_acpi_match); +#endif + +static const struct i2c_device_id lp3952_id[] = { + {LP3952_NAME, 0}, + {} +}; + +static struct i2c_driver lp3952_i2c_driver = { + .driver = { + .name = LP3952_NAME, + .owner = THIS_MODULE, + .pm = LP3952_PM, +#ifdef CONFIG_ACPI + .acpi_match_table = ACPI_PTR(lp3952_acpi_match), +#endifDid you test booting using ACPI? Would it be possible to switch to using Device Tree? This device is controlled via I2C so this would be a more suitable way.My host platform runs intel chipset which prefers ACPI( dont have firmware code). I couldn't test it as ACPI device. But have followed instructions in Documentation/acpi/enumeration.txt. I tested the led controller registering it, from host board init file. #include <linux/leds-lp3952.h> static struct lp3952_platform_data lp3952_data = { .enable_gpio = LP3952_NRST_GPIO }; static struct i2c_board_info __initdata lp3952_i2c_board_info = { I2C_BOARD_INFO(LP3952_NAME, LP3952_DEV_ADDR), .platform_data = &lp3952_data, }; then i2c_new_device(adap, &lp3952_i2c_board_info); from host board initialisation. Should I remove the ACPI registration?
I'd like to have ACPI maintainer's ack for this. Cc linux-acpi list and maintainers.
+ }, + .probe = lp3952_probe, + .remove = __exit_p(lp3952_remove)If not building the driver as a module I am getting the following: drivers/leds/leds-lp3952.c:337:12: warning: ‘lp3952_remove’ defined but not used Is there some specific reason for which you don't want to have the "remove" op initialized when driver is built into the kernel? What config are you using for building this driver?I have been building it as module. I assumed, if it was built in, wont be using remove op. I have removed the __exit_p so that it is built in all the time. I have tested the module as built in. The warning is gone, with the change.,+ .id_table = lp3952_id, +}; + +module_i2c_driver(lp3952_i2c_driver); + +MODULE_AUTHOR("Tony Makkiel <tony.makkiel@xxxxxxxxx>"); +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..1b1e7c3 --- /dev/null +++ b/include/linux/leds-lp3952.h @@ -0,0 +1,89 @@ +/* + * Copyright (C) 2016, DAQRI LLC + * + * License Terms: GPL v2 + * + * TI lp3952 Controller driver + * + * Author: Tony Makkiel <tony.makkiel@xxxxxxxxx> + * + */ + +#ifndef LEDS_LP3952_H_ +#define LEDS_LP3952_H_ + +#define LP3952_NAME "lp3952" +#define LP3952_DEV_ADDR 0x54 +/* Following 2 MACRO are specific to Minnowboard Max + * Use the appropriate host specific values when + * instantiating device + */ +#define LP3952_BUS_ADDR 7 +#define LP3952_NRST_GPIO 464 + +/* 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_brightness { + LP3952_OFF = 0, + LP3952_QUARTER_BRIGHT, + LP3952_HALF_BRIGHT, + LP3952_THREE_QRTR_BRIGHT, + LP3952_FULL_BRIGHT +}; + +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_platform_data { + u32 enable_gpio; +}; + +#endif /* LEDS_LP3952_H_ */
-- Best regards, Jacek Anaszewski -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
