Re: [PATCH RESEND 15/16] leds: add LM3633 driver

["Kim, Milo" <milo.kim-l0cyMroinI0@public.gmane.org>]

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/<led>/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 <milo.kim-l0cyMroinI0@public.gmane.org>
>> +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/<led>/pattern_times
>> +
>> +                cat /sys/class/leds/<led>/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/<led>/pattern_levels
>> +Date:		Oct 2015
>> +KernelVersion:	4.3
>> +Contact:	Milo Kim <milo.kim-l0cyMroinI0@public.gmane.org>
>> +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/<led>/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/<led>/pattern_levels
>> +                low brightness: 0, high brightness: 255
>> +
>> +What:		/sys/class/leds/<led>/run_pattern
>> +Date:		Oct 2015
>> +KernelVersion:	4.3
>> +Contact:	Milo Kim <milo.kim-l0cyMroinI0@public.gmane.org>
>> +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/<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 devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html