From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751951AbcFVH0w (ORCPT ); Wed, 22 Jun 2016 03:26:52 -0400 Received: from mailout4.w1.samsung.com ([210.118.77.14]:9854 "EHLO mailout4.w1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750854AbcFVH0u (ORCPT ); Wed, 22 Jun 2016 03:26:50 -0400 MIME-version: 1.0 Content-type: text/plain; charset=windows-1252; format=flowed X-AuditID: cbfec7f5-f792a6d000001302-ab-576a3db69a3a Content-transfer-encoding: 8BIT Message-id: <576A3DB5.4080703@samsung.com> Date: Wed, 22 Jun 2016 09:26:45 +0200 From: Jacek Anaszewski User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130804 Thunderbird/17.0.8 To: Florian Vaussard Cc: Florian Vaussard , devicetree@vger.kernel.org, Richard Purdie , Rob Herring , Mark Rutland , linux-leds@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 2/2] leds: Add driver for NCP5623 3-channel I2C LED driver References: <1466494154-3786-1-git-send-email-florian.vaussard@heig-vd.ch> <1466494154-3786-3-git-send-email-florian.vaussard@heig-vd.ch> <57695D45.60107@samsung.com> <9375e68a-9920-8e84-419d-890f57b6c9d2@heig-vd.ch> In-reply-to: <9375e68a-9920-8e84-419d-890f57b6c9d2@heig-vd.ch> X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFnrCLMWRmVeSWpSXmKPExsVy+t/xa7rbbLPCDU7dNbaYf+Qcq8XkQ4vZ LV5tt7a4vGsOm8XWN+sYLZZev8hk0br3CLvF7l1PWR04PNbMW8PosXPWXXaPp+v6WTw2repk 89gz/werx+dNcgFsUVw2Kak5mWWpRfp2CVwZn9t+sBd8NKu4fmItUwNjl1YXIyeHhICJxKwv 25kgbDGJC/fWs3UxcnEICSxllNjf380MkuAVEJT4MfkeSxcjBwezgLzEkUvZIGFmAVuJBe/X sUDUP2OUuP71DhNEvZbEku4FLCA2i4CqxPZfbYwgNpuAocTPF6/BakQFIiT+nN7HCmKLCBhJ TJy4mBli6A9GiaUns0BsYQE/iUkLnzNCLLjDKDG17ycbSIJTwE7iwt+TLBMYBWYhuW8Wwn2z kNy3gJF5FaNoamlyQXFSeq6RXnFibnFpXrpecn7uJkZI2H/dwbj0mNUhRgEORiUeXoauzHAh 1sSy4srcQ4wSHMxKIrwpllnhQrwpiZVVqUX58UWlOanFhxilOViUxHln7nofIiSQnliSmp2a WpBaBJNl4uCUamBU+xa3KOXAEn4NhmIf1Z++/58zGl/mD0lU4nRZ/OtRG0/cIe1Xpl/mnck9 nnjh1WUrlZaNamwc4tf2LtPrmp135+4NDhWB2Fl3YjPvvtv1XLckR8RoTtCtl5qyH3a8ZbV7 pB3ktNuHs6Ah/k3tE+1GvR8LA0pnTlnNeMfhuEX1GtE1BwPDtzYosRRnJBpqMRcVJwIAWzUH WncCAAA= Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Florian, On 06/22/2016 08:08 AM, Florian Vaussard wrote: > Hi Jacek, > > Le 21. 06. 16 à 17:29, Jacek Anaszewski a écrit : >> Hi Florian, >> >> Thanks for the patch. Please refer to my comments in the code. >> > > Thanks for your review. I completely agree with you, I will address your > comments in V2. I just have one small question below. > >> On 06/21/2016 09:29 AM, Florian Vaussard wrote: >>> The NCP5623 is a 3-channel LED driver from On Semiconductor controlled >>> through I2C. The PWM of each channel can be independently set with 32 >>> distinct levels. In addition, the intensity of the current source can be >>> globally set using an external bias resistor fixing the reference >>> current (Iref) and a dedicated register (ILED), following the >>> relationship: >>> >>> I = 2400*Iref/(31-ILED) >>> >>> with Iref = Vref/Rbias, and Vref = 0.6V. >>> >>> Signed-off-by: Florian Vaussard >>> --- >>> drivers/leds/Kconfig | 11 ++ >>> drivers/leds/Makefile | 1 + >>> drivers/leds/leds-ncp5623.c | 265 ++++++++++++++++++++++++++++++++++++++++++++ >>> 3 files changed, 277 insertions(+) >>> create mode 100644 drivers/leds/leds-ncp5623.c >>> >>> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig >>> index 5ae2834..6d3e44d 100644 >>> --- a/drivers/leds/Kconfig >>> +++ b/drivers/leds/Kconfig >>> @@ -588,6 +588,17 @@ config LEDS_BLINKM >>> This option enables support for the BlinkM RGB LED connected >>> through I2C. Say Y to enable support for the BlinkM LED. >>> >>> +config LEDS_NCP5623 >>> + tristate "LED Support for NCP5623 I2C chip" >>> + depends on LEDS_CLASS >>> + depends on I2C >>> + help >>> + This option enables support for LEDs connected to NCP5623 >>> + LED driver chips accessed via the I2C bus. >>> + Driver supports brightness control. >>> + >>> + Say Y to enable this driver. >>> + >>> config LEDS_POWERNV >>> tristate "LED support for PowerNV Platform" >>> depends on LEDS_CLASS >>> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile >>> index cb2013d..a2f0e10 100644 >>> --- a/drivers/leds/Makefile >>> +++ b/drivers/leds/Makefile >>> @@ -67,6 +67,7 @@ obj-$(CONFIG_LEDS_KTD2692) += leds-ktd2692.o >>> obj-$(CONFIG_LEDS_POWERNV) += leds-powernv.o >>> obj-$(CONFIG_LEDS_SEAD3) += leds-sead3.o >>> obj-$(CONFIG_LEDS_IS31FL32XX) += leds-is31fl32xx.o >>> +obj-$(CONFIG_LEDS_NCP5623) += leds-ncp5623.o >>> >>> # LED SPI Drivers >>> obj-$(CONFIG_LEDS_DAC124S085) += leds-dac124s085.o >>> diff --git a/drivers/leds/leds-ncp5623.c b/drivers/leds/leds-ncp5623.c >>> new file mode 100644 >>> index 0000000..a341e4a >>> --- /dev/null >>> +++ b/drivers/leds/leds-ncp5623.c >>> @@ -0,0 +1,265 @@ >>> +/* >>> + * Copyright 2016 Florian Vaussard >>> + * >>> + * Based on leds-tlc591xx.c >>> + * >>> + * This program is free software; you can redistribute it and/or modify >>> + * it under the terms of the GNU General Public License as published by >>> + * the Free Software Foundation; version 2 of the License. >>> + */ >>> + >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> + >>> +#define NCP5623_MAX_LEDS 3 >>> +#define NCP5623_MAX_STEPS 32 >>> +#define NCP5623_MAX_CURRENT 31 >>> +#define NCP5623_MAX_CURRENT_UA 30000 >>> + >>> +#define NCP5623_CMD_SHIFT 5 >>> +#define CMD_SHUTDOWN (0x00 << NCP5623_CMD_SHIFT) >>> +#define CMD_ILED (0x01 << NCP5623_CMD_SHIFT) >>> +#define CMD_PWM1 (0x02 << NCP5623_CMD_SHIFT) >>> +#define CMD_PWM2 (0x03 << NCP5623_CMD_SHIFT) >>> +#define CMD_PWM3 (0x04 << NCP5623_CMD_SHIFT) >>> +#define CMD_UPWARD_DIM (0x05 << NCP5623_CMD_SHIFT) >>> +#define CMD_DOWNWARD_DIM (0x06 << NCP5623_CMD_SHIFT) >>> +#define CMD_DIM_STEP (0x07 << NCP5623_CMD_SHIFT) >>> + >>> +#define NCP5623_DATA_MASK GENMASK(NCP5623_CMD_SHIFT - 1, 0) >>> + >>> +#define NCP5623_CMD(cmd, data) (cmd | (data & NCP5623_DATA_MASK)) >>> + > > [snip] > >>> + >>> +static int ncp5623_configure(struct device *dev, >>> + struct ncp5623_priv *priv) >>> +{ >>> + unsigned int i; >>> + unsigned int n; >>> + struct ncp5623_led *led; >>> + int err; >>> + >>> + /* Compute the value of ILED register to honor led_max_current */ >>> + n = 2400 * priv->led_iref / priv->led_max_current + 1; >>> + if (n > NCP5623_MAX_CURRENT) >>> + n = NCP5623_MAX_CURRENT; >>> + n = NCP5623_MAX_CURRENT - n; >>> + >>> + dev_dbg(dev, "setting maximum current to %u uA\n", >>> + 2400 * priv->led_iref / (NCP5623_MAX_CURRENT - n)); >>> + >>> + err = ncp5623_send_cmd(priv, CMD_ILED, n); >>> + if (err < 0) { >>> + dev_err(dev, "cannot set the current\n"); >>> + return err; >>> + } >>> + >>> + /* Setup each individual LED */ >>> + for (i = 0; i < NCP5623_MAX_LEDS; i++) { >>> + led = &priv->leds[i]; >>> + >>> + if (!led->active) >>> + continue; >>> + >>> + led->priv = priv; >>> + led->led_no = i; >>> + led->ldev.brightness_set = ncp5623_brightness_set; >>> + led->ldev.max_brightness = NCP5623_MAX_STEPS - 1; >> >> max_brightness should be asserted according to led-max-microamp >> property. >> > > We can set two parameters on this chip: > > - The intensity of the current source (common to all LEDs) from 0 to > NCP5623_MAX_CURRENT (31) > - The PWM for each LED independently from 0 to NCP5623_MAX_STEPS-1 (31) > > My understanding was to use the led-max-microamp property to set up the current > source so that even with a duty cycle of 100% (PWM=31) the LED cannot be > damaged. In this case, I think that is unnecessary to assert max_brightness, as > we are guaranteed by design that led-max-microamp will not be surpassed for any > value of the PWM. Would you suggest another approach? Yes, if step 31 reflects led-max-microamp value, then max_brightness should be 31, regardless of led-max-microamp value. >>> + INIT_WORK(&led->work, ncp5623_led_work); >> >> Please remove it and use brightness_set_blocking op. >> >>> + err = led_classdev_register(dev, &led->ldev); >> >> Use devm prefixed version. >> >>> + if (err < 0) { >>> + dev_err(dev, "couldn't register LED %s\n", >>> + led->ldev.name); >>> + goto exit; >>> + } >>> + } >>> + >>> + return 0; >>> + >>> +exit: >>> + ncp5623_destroy_devices(priv); >>> + return err; >>> +} >>> + > > -- Best regards, Jacek Anaszewski