Re: [PATCH v3 1/2] gpio: davinci: Add keystone-k2g compatible

[Suman Anna <s-anna@xxxxxx>]

Franklin,

On 07/26/2017 08:27 AM, Franklin S Cooper Jr wrote:
> 
> On 07/26/2017 08:00 AM, Suman Anna wrote:
>> Hi Keerthy,
>>
>> On 07/26/2017 01:45 AM, Keerthy wrote:
>>> The patch adds keystone-k2g compatible, specific properties and
>>> an example.
> 
> Seems we are adding information regarding several Keystone 2 SoCs. So
> the commit and subject should be tweaked to reflect this.
>>
>> Please update patch header to "dt-bindings: gpio: davinci: ...."
>>
>>>
>>> Signed-off-by: Keerthy <j-keerthy@xxxxxx>
>>> ---
>>>
>>> Changes in v3:
>>>
>>>   * Added details about family of SoCs corresponding to compatibles.
>>>
>>>  .../devicetree/bindings/gpio/gpio-davinci.txt      | 40 +++++++++++++++++++++-
>>>  1 file changed, 39 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/gpio/gpio-davinci.txt b/Documentation/devicetree/bindings/gpio/gpio-davinci.txt
>>> index 5079ba7..fb9efee 100644
>>> --- a/Documentation/devicetree/bindings/gpio/gpio-davinci.txt
>>> +++ b/Documentation/devicetree/bindings/gpio/gpio-davinci.txt
>>> @@ -1,7 +1,10 @@
>>>  Davinci/Keystone GPIO controller bindings
>>>  
>>>  Required Properties:
>>> -- compatible: should be "ti,dm6441-gpio", "ti,keystone-gpio"
>>> +- compatible: should be "ti,dm6441-gpio": for Davinci da850 SoCs 
>>> +			"ti,keystone-gpio": for Keystone 2 66AK2H/K, 66AK2L,
>>> +						66AK2E SoCs
>>> +			"ti,keystone-k2g-gpio", "ti,keystone-gpio": for 66AK2G
>>>  
>>>  - reg: Physical base address of the controller and the size of memory mapped
>>>         registers.
>>> @@ -26,6 +29,17 @@ The GPIO controller also acts as an interrupt controller. It uses the default
>>>  two cells specifier as described in Documentation/devicetree/bindings/
>>>  interrupt-controller/interrupts.txt.
>>>  
>>> +Required Properties specific to keystone-k2g
>>
>> Thanks for updating the binding for the clocks, but clocks are not
>> specific to K2G. They also apply to other K2 SoCs. Davinci platforms do
>> not have DT clocks atm, so the Davinci portion can be updated later if
>> and when they get added.
>>
> 
> What about power-domain property?
> 
>>> +
>>> +- clocks: Should contain devices input clock. The first parameter
>>> +           is a handle to k2g_clks. The second parameter is the
>>> +           device ID and the third parameter is the clock ID. One can
>>> +           refer: http://processors.wiki.ti.com/index.php/TISCI#66AK2G02_Data
>>
>> No need for this link here, just refer to the appropriate clock
>> bindings. Some thing like the following would be better
> 
> This information is helpful especially with the macros being dropped.
> However, I agree that this is not the place for this information.
> Probably should be linked to in the ti,sci-clk.txt and
> sci-pm-domain.txt. However, both of these are outdated since it is
> referring to macros and includes that don't exist or will no longer exist.

You are probably looking at the bindings on current master. These have
already been addressed/updated, lookup linux-next.

regards
Suman

> 
>>
>> - clocks:               Should contain the device's input clock, and
>>                         should be defined as per the appropriate clock
>> bindings consumer usage in,
>>
>> Documentation/devicetree/bindings/clock/keystone-gate.txt
>>                             for 66AK2HK/66AK2L/66AK2E SoCs or,
>>
>> Documentation/devicetree/bindings/clock/ti,sci-clk.txt
>>                             for 66AK2G SoCs
>>
>>> +
>>> +           Example: <&k2g_clks 0x001c 0x0>;
>>
>> This can be dropped as well, below example already demonstrates it.
>>
>>> +
>>> +- clock-names: The driver expects the clock name to be "gpio";
>>
>> Just say, Should be "gpio". No need of mentioning about the driver.
>>
>>> +
>>>  Example:
>>>  
>>>  gpio: gpio@1e26000 {
>>> @@ -60,3 +74,27 @@ leds {
>>>  		...
>>>  	};
>>>  };
>>> +
>>> +Example for keystone-k2g:
>>
>> s/keystone-k2g/66AK2G/
>>
>>> +
>>> +gpio0: gpio@2603000 {
>>> +	compatible = "ti,keystone-k2g-gpio", "ti,keystone-gpio";
>>> +	reg = <0x02603000 0x100>;
>>> +	gpio-controller;
>>> +	#gpio-cells = <2>;
>>> +	interrupts = <GIC_SPI 432 IRQ_TYPE_EDGE_RISING>,
>>> +			<GIC_SPI 433 IRQ_TYPE_EDGE_RISING>,
>>> +			<GIC_SPI 434 IRQ_TYPE_EDGE_RISING>,
>>> +			<GIC_SPI 435 IRQ_TYPE_EDGE_RISING>,
>>> +			<GIC_SPI 436 IRQ_TYPE_EDGE_RISING>,
>>> +			<GIC_SPI 437 IRQ_TYPE_EDGE_RISING>,
>>> +			<GIC_SPI 438 IRQ_TYPE_EDGE_RISING>,
>>> +			<GIC_SPI 439 IRQ_TYPE_EDGE_RISING>,
>>> +			<GIC_SPI 440 IRQ_TYPE_EDGE_RISING>;
>>> +	interrupt-controller;
>>> +	#interrupt-cells = <2>;
>>> +	ti,ngpio = <144>;
>>> +	ti,davinci-gpio-unbanked = <0>;
>>> +	clocks = <&k2g_clks 0x001b 0x0>;
>>> +	clock-names = "gpio";
>>> +};
> 
> If your going to talk about other Keystone 2 devices it would be helpful
> to include an example for one of them since they have slightly different
> properties.
>>>
>>
>> regards
>> Suman
>>

--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html