Re: [PATCH] leds: pca955x: add GPIO support

[Jacek Anaszewski <jacek.anaszewski-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

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-Bxea+6Xhats@public.gmane.org>
>>>>> ---
>>>>>  .../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
--
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