Re: [PATCH v6 1/2] dt: bindings: lm3601x: Introduce the lm3601x driver

[Jacek Anaszewski <jacek.anaszewski@gmail.com>]

Dan,

On 05/15/2018 11:29 PM, Dan Murphy wrote:
> Jacek
> 
> On 05/15/2018 04:13 PM, Jacek Anaszewski wrote:
>> Hi Dan,
>>
>> Thanks for the update.
>>
>> On 05/15/2018 05:43 PM, Dan Murphy wrote:
>>> Introduce the device tree bindings for the lm3601x
>>> family of LED torch, flash and IR drivers.
>>>
>>> Signed-off-by: Dan Murphy <dmurphy@ti.com>
>>> ---
>>>
>>> v6 - Removed multiple led child nodes, fixed example to display micro ranges
>>> for corresponding child nodes and added led-sources to define the current driver -
>>> https://patchwork.kernel.org/patch/10392121/
>>>
>>> v5 - No changes - https://patchwork.kernel.org/patch/10391743/
>>> v4 - Added " " around "=", changed strobe to flash on label, removed "support and
>>> register" comment and change ir lable to ir:torch - See v2 patchworks for comments
>>> v3 - Removed wildcard compatible - https://patchwork.kernel.org/patch/10386241/
>>> v2 - No changes - https://patchwork.kernel.org/patch/10384587/
>>>
>>>    .../devicetree/bindings/leds/leds-lm3601x.txt | 47 +++++++++++++++++++
>>>    1 file changed, 47 insertions(+)
>>>    create mode 100644 Documentation/devicetree/bindings/leds/leds-lm3601x.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/leds/leds-lm3601x.txt b/Documentation/devicetree/bindings/leds/leds-lm3601x.txt
>>> new file mode 100644
>>> index 000000000000..27930a89e9a5
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/leds/leds-lm3601x.txt
>>> @@ -0,0 +1,47 @@
>>> +* Texas Instruments - lm3601x Single-LED Flash Driver
>>> +
>>> +The LM3601X are ultra-small LED flash drivers that
>>> +provide a high level of adjustability.
>>> +
>>> +Required properties:
>>> +    - compatible : Can be one of the following
>>> +        "ti,lm36010"
>>> +        "ti,lm36011"
>>> +    - reg : I2C slave address
>>> +    - #address-cells : 1
>>> +    - #size-cells : 0
>>> +
>>> +Required child properties:
>>> +    - reg : 0
>>> +    - led-sources:    0 - Indicates a IR mode
>>> +            1 - Indicates a Torch (white LED) mode
>>
>> You don't need led-sources at all. Please use reg as in
>> the previous version. led-sources is useful for describing
>> more complex arrangements. Here reg will do.
> 
> OK.  Thought we would keep consistent with the max IC.
> 
>>
>>> +
>>> +Required properties for flash LED child nodes:
>>> +    See Documentation/devicetree/bindings/leds/common.txt
>>> +    - flash-max-microamp : Range from 11mA -> 1.5A
>>> +    - flash-max-timeout-us : Range from 40ms -> 1600ms
>>> +    - led-max-microamp : Range from 2.4mA -> 376mA
>>
>> Please give also a step in the format like below
>> (taken from max77693):
>>
>>      Valid values: 62500 - 1000000, step by 62500 (rounded down)
> 
> I would do this but the step is not linear.  The step is 40ms from 40 -> 400.
> after that the step goes to 200ms from 400->1600.
> 
> same with the current ranges.

If you're going to apply Andy's formula in the driver, then you can
present it also here. Anyway, please avoid using non-standard "->"
symbol in favor of "to" if you're using "from".

Or even better I propose to list allowed values - there are not
so many of them.

-- 
Best regards,
Jacek Anaszewski