Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751731AbbKJHjF (ORCPT ); Tue, 10 Nov 2015 02:39:05 -0500 Received: from comal.ext.ti.com ([198.47.26.152]:59232 "EHLO comal.ext.ti.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751129AbbKJHjC (ORCPT ); Tue, 10 Nov 2015 02:39:02 -0500 Subject: Re: [PATCH RESEND 15/16] leds: add LM3633 driver To: Jacek Anaszewski References: <1446441875-1256-1-git-send-email-milo.kim@ti.com> <1446441875-1256-16-git-send-email-milo.kim@ti.com> <5638DD99.9070502@samsung.com> CC: , , , From: "Kim, Milo" Message-ID: <56419F0C.90009@ti.com> Date: Tue, 10 Nov 2015 16:38:52 +0900 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:38.0) Gecko/20100101 Thunderbird/38.1.0 MIME-Version: 1.0 In-Reply-To: <5638DD99.9070502@samsung.com> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 8313 Lines: 257 Hi Jacek, On 11/4/2015 1:15 AM, Jacek Anaszewski wrote: > Hi Milo, > > Thanks for the patch. Please find my comments in the code. > >> diff --git a/Documentation/ABI/testing/sysfs-class-led-lm3633 b/Documentation/ABI/testing/sysfs-class-led-lm3633 >> new file mode 100644 >> index 0000000..c1d8759 >> --- /dev/null >> +++ b/Documentation/ABI/testing/sysfs-class-led-lm3633 >> @@ -0,0 +1,60 @@ >> +LM3633 LED driver generates programmable pattern via the sysfs. >> + >> +LED Pattern Generator Structure >> + >> + (3) >> + (a) ---------------> ___________ >> + / \ >> + (2) / \ (4) >> + (b) ----> _________/ \_________ ... >> + (1) (5) >> + >> + |<----- period -----> | >> + >> +What: /sys/class/leds//pattern_times > > "time" noun is uncountable. > >> +Date: Oct 2015 >> +KernelVersion: 4.3 > > These properties certainly will not appear in 4.3. Oops! 4.4 gonna be OK? > >> +Contact: Milo Kim >> +Description: read/write >> + Set pattern time dimension. There are five arguments. >> + (1) startup delay >> + (2) rising dimming time >> + (3) how much time stays at high level >> + (4) falling dimming time >> + (5) how much time stays at low level >> + Ranges are >> + (1), (3), (5): 0 ~ 10000. Unit is millisecond. >> + (2), (4): 0 ~ 16000. Unit is millisecond. >> + >> + Example: >> + No delay, rising 200ms, high 300ms, falling 100ms, low 400ms. >> + echo "0 200 300 100 400" > /sys/class/leds//pattern_times >> + >> + cat /sys/class/leds//pattern_times >> + delay: 0, rise: 200, high: 300, fall: 100, low: 400 > > Generally a sysfs attribute should represent a single value. > There are cases where the attribute comprises a list of space separated > values, but certainly colons and commas adds to much noise, and are > cumbersome to parse. I'd opt for creating a separate sysfs attribute > for each of the above 5 properties. Got it, thanks! > >> +What: /sys/class/leds//pattern_levels >> +Date: Oct 2015 >> +KernelVersion: 4.3 >> +Contact: Milo Kim >> +Description: read/write >> + Set pattern level(brightness). There are two arguments. >> + (a) Low brightness level >> + (b) High brightness level >> + Ranges are from 0 to 255. >> + >> + Example: >> + Low level is 0, high level is 255. >> + echo "0 255" > /sys/class/leds//pattern_levels > > I'd go also for two separate attributes. E.g. pattern_brightness_lo and > pattern_brightness_hi, or maybe you'll have better idea. OK. > >> + cat /sys/class/leds//pattern_levels >> + low brightness: 0, high brightness: 255 >> + >> +What: /sys/class/leds//run_pattern >> +Date: Oct 2015 >> +KernelVersion: 4.3 >> +Contact: Milo Kim >> +Description: write only >> + After 'pattern_times' and 'pattern_levels' are updated, >> + run the pattern by writing 1 to 'run_pattern'. >> + To stop running pattern, writes 0 to 'run_pattern'. > > I wonder how registering an in-driver trigger would work. It would > allow for hiding above pattern attributes when the trigger is inactive, > and thus making the sysfs interface more transparent. You could avoid > the need for run_pattern attribute, as setting the trigger would itself > activate the pattern, and setting brightness to 0 would turn it off. I like this idea, let me try to fix it. >> +/** >> + * struct ti_lmu_led >> + * >> + * @chip: Pointer to parent LED device >> + * @bank_id: LED bank ID >> + * @cdev: LED subsystem device structure >> + * @name: LED channel name >> + * @led_string: LED string configuration. >> + * Bit mask is set on parsing DT. >> + * @imax: [Optional] Max current index. >> + * It's result of ti_lmu_get_current_code(). > > Why is this optional? You're correct, this is not optional. DT property is optional. >> +static ssize_t lm3633_led_store_pattern_levels(struct device *dev, >> + struct device_attribute *attr, >> + const char *buf, size_t len) >> +{ >> + struct ti_lmu_led *lmu_led = to_ti_lmu_led(dev); >> + struct ti_lmu_led_chip *chip = lmu_led->chip; >> + unsigned int low, high; >> + u8 reg, offset, val; >> + int ret; >> + >> + /* >> + * Sequence >> + * >> + * 1) Read pattern level data >> + * 2) Disable a bank before programming a pattern >> + * 3) Update LOW BRIGHTNESS register >> + * 4) Update HIGH BRIGHTNESS register >> + * >> + * Level register addresses have offset number based on the LED bank. >> + */ >> + >> + ret = sscanf(buf, "%u %u", &low, &high); >> + if (ret != 2) >> + return -EINVAL; >> + >> + low = min_t(unsigned int, low, LM3633_LED_MAX_BRIGHTNESS); > > Why don't you take into account the value defined by led-max-microamp > DT property here? 'low' and 'high' are brightness value. The range is from 0 to 255. It is mostly related with LED sysfs - /sys/class/led//brightness. On the other hand, led-max-microamp is current limit. It is from 5mA to 30mA. It's different configuration in this device. >> +static void lm3633_led_work(struct work_struct *work) >> +{ >> + struct ti_lmu_led *lmu_led = container_of(work, struct ti_lmu_led, >> + work); >> + struct ti_lmu_led_chip *chip = lmu_led->chip; >> + int ret; >> + >> + mutex_lock(&chip->lock); >> + >> + ret = ti_lmu_write_byte(chip->lmu, >> + LM3633_REG_BRT_LVLED_BASE + lmu_led->bank_id, >> + lmu_led->brightness); >> + if (ret) { >> + mutex_unlock(&chip->lock); >> + return; >> + } >> + >> + if (lmu_led->brightness == 0) >> + lm3633_led_disable_bank(lmu_led); >> + else >> + lm3633_led_enable_bank(lmu_led); > > Is it possible to control a brightness of a whole bank only, > and not individual LEDs? No, LM3633 has internal control banks. Let me assume that two LED groups are assigned below. LED 1, 2, 3 - RGB LED 4 - status Two control banks should be allocated. The bank should be controlled separately. If we try to enable/disabe all banks, then 6 output LED channels are controlled at the same time. >> +static int lm3633_led_init(struct ti_lmu_led *lmu_led, int bank_id) >> +{ >> + struct device *dev = lmu_led->chip->dev; >> + char name[12]; >> + int ret; >> + >> + /* >> + * Sequence >> + * >> + * 1) Configure LED bank which is used for brightness control >> + * 2) Set max current for each output channel >> + * 3) Add LED device >> + */ >> + >> + ret = lm3633_led_config_bank(lmu_led); >> + if (ret) { >> + dev_err(dev, "Output bank register err: %d\n", ret); >> + return ret; >> + } >> + >> + ret = lm3633_led_set_max_current(lmu_led); >> + if (ret) { >> + dev_err(dev, "Set max current err: %d\n", ret); >> + return ret; >> + } >> + >> + lmu_led->cdev.max_brightness = LM3633_LED_MAX_BRIGHTNESS; > > The value assigned here should be determined by led-max-microamp > DT property. Ditto. Current limit is different configuration from brightness value. >> +static int lm3633_led_probe(struct platform_device *pdev) >> +{ >> + struct device *dev = &pdev->dev; >> + struct ti_lmu *lmu = dev_get_drvdata(dev->parent); >> + struct ti_lmu_led_chip *chip; >> + struct ti_lmu_led *each; >> + int i, ret; >> + >> + chip = devm_kzalloc(dev, sizeof(*chip), GFP_KERNEL); >> + if (!chip) >> + return -ENOMEM; >> + >> + chip->dev = dev; >> + chip->lmu = lmu; >> + >> + ret = lm3633_led_of_create(chip, dev->of_node); >> + if (ret) >> + return ret; >> + >> + /* >> + * Notifier callback is required because LED device needs >> + * reconfiguration after opened/shorted circuit fault monitoring > > Shouldn't this be open/short circuit? You're correct. Thanks! Best regards, Milo -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/