Hi Rob,
On 06/09/2017 01:13 PM, Cédric Le Goater wrote:
> Hello,
>
>>>> Thanks for the patch.
>>>>
>>>> Generally I'd split it into two patches:
>>>> 1/2: addition of LED class specific DT support
>>>> 2/2: addition of GPIO support (for this one please cc also
>>>> GPIO subsystem maintainer)
>>>
>>> OK. I will in next version.
>>>
>>>> Please also see my comments below.
>>>>
>>>> On 05/09/2017 08:36 AM, Cédric Le Goater wrote:
>>>>> The PCA955x family of chips are I2C LED blinkers whose pins not used
>>>>> to control LEDs can be used as general purpose I/Os (GPIOs).
>>>>>
>>>>> The following adds support for device tree and Open Firmware to be
>>>>> able do define different operation modes for each pin. See bindings
>>>>> documentation for more details. The pca955x driver is then extended
>>>>> with a gpio_chip when pins are operating in GPIO mode.
>>>>>
>>>>> Signed-off-by: Cédric Le Goater <clg@xxxxxxxx>
>>>>> ---
>>>>> .../devicetree/bindings/leds/leds-pca955x.txt | 103 ++++++++
>>>>> drivers/leds/Kconfig | 11 +
>>>>> drivers/leds/leds-pca955x.c | 290 ++++++++++++++++++---
>>>>> 3 files changed, 374 insertions(+), 30 deletions(-)
>>>>> create mode 100644 Documentation/devicetree/bindings/leds/leds-pca955x.txt
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/leds/leds-pca955x.txt b/Documentation/devicetree/bindings/leds/leds-pca955x.txt
>>>>> new file mode 100644
>>>>> index 000000000000..98d1053dd1b0
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/leds/leds-pca955x.txt
>>>>> @@ -0,0 +1,103 @@
>>>>> +LEDs connected to pca9550, pca9551, pca9552, pca9553,
>>>>> +
>>>>> +Required properties:
>>>>> +- compatible : should be one of :
>>>>> + "nxp,pca9550"
>>>>> + "nxp,pca9551"
>>>>> + "nxp,pca9552"
>>>>> + "nxp,pca9553"
>>>>> +- #address-cells: must be 1
>>>>> +- #size-cells: must be 0
>>>>> +- reg: I2C slave address. depends on the model.
>>>>> +
>>>>> +Optional properties:
>>>>> +- gpio-controller: allows pins to be used as GPIOs.
>>>>> +- #gpio-cells: if present, must not be 0.
>>>>> +- gpio-base : base number of the pins used as GPIOs. If there are more
>>>>> + than one, they should be contiguous. See 'type' property
>>>>> + below.
>>>>> +
>>>>> +LED sub-node properties:
>>>>> +- reg : number of LED line.
>>>>> + from 0 to 1 in pca9550
>>>>> + from 0 to 7 in pca9551
>>>>> + from 0 to 15 in pca9552
>>>>> + from 0 to 3 in pca9553
>>>>> +- compatible: either "none", "led" (default) or "gpio".
>>>>> +- label : (optional)
>>>>> + see Documentation/devicetree/bindings/leds/common.txt
>>>>> +- linux,default-trigger : (optional)
>>>>> + see Documentation/devicetree/bindings/leds/common.txt
>>>>> +
>>>>> +Examples:
>>>>> +
>>>>> +pca9552: pca9552@60 {
>>>>> + compatible = "nxp,pca9552";
>>>>> + #address-cells = <1>;
>>>>> + #size-cells = <0>;
>>>>> + reg = <0x60>;
>>>>> +
>>>>> + gpio-controller;
>>>>> + #gpio-cells = <2>;
>>>>> + gpio-base = <12>;
>>>>> +
>>>>> + gpio@12 {
>>>>> + label = "GPIO12";
>>>>> + reg = <12>;
>>>>> + compatible = "gpio";
>>>>> + };
>>>>> + gpio@13 {
>>>>> + label = "GPIO13";
>>>>> + reg = <13>;
>>>>> + compatible = "gpio";
>>>>> + };
>>>>> + gpio@14 {
>>>>> + label = "GPIO14";
>>>>> + reg = <14>;
>>>>> + compatible = "gpio";
>>>>> + };
>>>>> + gpio@15 {
>>>>> + label = "GPIO15";
>>>>> + reg = <15>;
>>>>> + compatible = "gpio";
>>>>> + };
>>>>
>>>> I think that for GPIO pins we should have GPIO specific bindings,
>>>> but DT maintainer would have to give his opinion here.
>>>
>>> I have got my inspiration from the tca6507 bindings.
>>
>> Right, but AFAIK there was no DT maintainer position at that time
>> and those bindings don't have relevant ack. I'd rather wait for
>> DT maintainer opinion so as to avoid spawning wrong practices.
>>
>>>> Also I'm not sure if using compatible property this way is correct.
>>>
>>> yes.
>
> So, is the usage of the 'compatible' property correct ?
>
> If not, we could use a 'type' property with an include file defining
> the possible values. This is a common pattern.
Could you please confirm if using compatible property this way
is acceptable?
It follows Documentation/devicetree/bindings/leds/tca6507.txt.
--
Best regards,
Jacek Anaszewski
