On 06/09/2017 10:56 PM, Jacek Anaszewski wrote:
> 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.
No big deal if not. The alternative would be something like :
gpio@15 {
reg = <15>;
type = <PCA955X_TYPE_GPIO>;
};
led@0 {
label = "red:power";
linux,default-trigger = "default-on";
reg = <0>;
type = <PCA955X_TYPE_LED>;
};
led@1 {
label = "green:power";
reg = <1>;
type = <PCA955X_TYPE_LED>;
};
Thanks,
C.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
