Patchwork [PATCHv2,3/3] arm: dts: add bandgap entry for OMAP4460 devices

login
register
mail settings
Submitter Eduardo Valentin
Date May 15, 2013, 2:58 p.m.
Message ID <1368629928-6723-4-git-send-email-eduardo.valentin@ti.com>
Download mbox | patch
Permalink /patch/244147/
State New
Headers show

Comments

Eduardo Valentin - May 15, 2013, 2:58 p.m.
Include bandgap devices for OMAP4460 devices.

Cc: "Benoît Cousson" <b-cousson@ti.com>
Cc: Tony Lindgren <tony@atomide.com>
Cc: Russell King <linux@arm.linux.org.uk>
Cc: linux-omap@vger.kernel.org
Cc: devicetree-discuss@lists.ozlabs.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Eduardo Valentin <eduardo.valentin@ti.com>
---
 arch/arm/boot/dts/omap4460.dtsi | 9 +++++++++
 1 file changed, 9 insertions(+)
Cousson, Benoit - May 16, 2013, 7:20 a.m.
Hi Eduardo,

On 05/15/2013 06:36 PM, Eduardo Valentin wrote:
> On 15-05-2013 11:23, Benoit Cousson wrote:
>> Hi Eduardo,
>>
>> On 05/15/2013 04:58 PM, Eduardo Valentin wrote:
>>> Include bandgap devices for OMAP4460 devices.
>>>
>>> Cc: "Benoît Cousson" <b-cousson@ti.com>
>>> Cc: Tony Lindgren <tony@atomide.com>
>>> Cc: Russell King <linux@arm.linux.org.uk>
>>> Cc: linux-omap@vger.kernel.org
>>> Cc: devicetree-discuss@lists.ozlabs.org
>>> Cc: linux-arm-kernel@lists.infradead.org
>>> Cc: linux-kernel@vger.kernel.org
>>> Signed-off-by: Eduardo Valentin <eduardo.valentin@ti.com>
>>> ---
>>>  arch/arm/boot/dts/omap4460.dtsi | 9 +++++++++
>>>  1 file changed, 9 insertions(+)
>>>
>>> diff --git a/arch/arm/boot/dts/omap4460.dtsi b/arch/arm/boot/dts/omap4460.dtsi
>>> index 2cf227c..e5bfbfe 100644
>>> --- a/arch/arm/boot/dts/omap4460.dtsi
>>> +++ b/arch/arm/boot/dts/omap4460.dtsi
>>> @@ -29,4 +29,13 @@
>>>  			     <0 55 0x4>;
>>>  		ti,hwmods = "debugss";
>>>  	};
>>> +
>>> +	bandgap {
>>> +		reg = <0x4a002260 0x4
>>> +			0x4a00232C 0x4
>>> +			0x4a002378 0x18>;
>>> +		compatible = "ti,omap4460-bandgap";
>>> +		interrupts = <0 126 4>; /* talert */
>>> +		ti,tshut-gpio = <86>;
> 
> 
> 
>>
>> Why do you need a custom attribute for GPIO? Cannot you use the standard
>> one?
> 
> I believe it was by your suggestion :-), during the first attempts to
> send this driver. But could not find the thread link :-( sorry.

Ooops :-) I do not remember that... maybe it was long time ago, before
we had any decent binding available for GPIO and IRQ...

> I guess the reasoning to mark it as a ti specific is because it will be
> used as IRQ line to treat thermal shutdown (in SW).

Mmm, ok, so in that case, it is not even a gpio, but an interrupt entry
that is needed like that:

	interrupt-parent = <&gpio3>;
	interrupts = <22>; /* gpio line 86 */

Except that we already have an IRQ line connected to GIC for the
Talert... I'm not sure we can have 2 different IRQ controllers for one
device :-(

We need to check.

Regards,
Benoit


>> Where is the gpio controller phandle?
>>
>> Usually it looks like this:
>>
>> 	gpios = <&gpio1 8 0>;
>>
>>
>> Regards,
>> Benoit
>>
>>
>>
> 
>
Eduardo Valentin - May 16, 2013, 12:27 p.m.
On 16-05-2013 03:20, Benoit Cousson wrote:
> Hi Eduardo,
> 
> On 05/15/2013 06:36 PM, Eduardo Valentin wrote:
>> On 15-05-2013 11:23, Benoit Cousson wrote:
>>> Hi Eduardo,
>>>
>>> On 05/15/2013 04:58 PM, Eduardo Valentin wrote:
>>>> Include bandgap devices for OMAP4460 devices.
>>>>
>>>> Cc: "Benoît Cousson" <b-cousson@ti.com>
>>>> Cc: Tony Lindgren <tony@atomide.com>
>>>> Cc: Russell King <linux@arm.linux.org.uk>
>>>> Cc: linux-omap@vger.kernel.org
>>>> Cc: devicetree-discuss@lists.ozlabs.org
>>>> Cc: linux-arm-kernel@lists.infradead.org
>>>> Cc: linux-kernel@vger.kernel.org
>>>> Signed-off-by: Eduardo Valentin <eduardo.valentin@ti.com>
>>>> ---
>>>>  arch/arm/boot/dts/omap4460.dtsi | 9 +++++++++
>>>>  1 file changed, 9 insertions(+)
>>>>
>>>> diff --git a/arch/arm/boot/dts/omap4460.dtsi b/arch/arm/boot/dts/omap4460.dtsi
>>>> index 2cf227c..e5bfbfe 100644
>>>> --- a/arch/arm/boot/dts/omap4460.dtsi
>>>> +++ b/arch/arm/boot/dts/omap4460.dtsi
>>>> @@ -29,4 +29,13 @@
>>>>  			     <0 55 0x4>;
>>>>  		ti,hwmods = "debugss";
>>>>  	};
>>>> +
>>>> +	bandgap {
>>>> +		reg = <0x4a002260 0x4
>>>> +			0x4a00232C 0x4
>>>> +			0x4a002378 0x18>;
>>>> +		compatible = "ti,omap4460-bandgap";
>>>> +		interrupts = <0 126 4>; /* talert */
>>>> +		ti,tshut-gpio = <86>;
>>
>>
>>
>>>
>>> Why do you need a custom attribute for GPIO? Cannot you use the standard
>>> one?
>>
>> I believe it was by your suggestion :-), during the first attempts to
>> send this driver. But could not find the thread link :-( sorry.
> 
> Ooops :-) I do not remember that... maybe it was long time ago, before
> we had any decent binding available for GPIO and IRQ...
> 


Probably it was because by that time we didnt have GPIO binding ready, yeah.

>> I guess the reasoning to mark it as a ti specific is because it will be
>> used as IRQ line to treat thermal shutdown (in SW).
> 
> Mmm, ok, so in that case, it is not even a gpio, but an interrupt entry
> that is needed like that:
> 
> 	interrupt-parent = <&gpio3>;
> 	interrupts = <22>; /* gpio line 86 */
> 
> Except that we already have an IRQ line connected to GIC for the
> Talert... I'm not sure we can have 2 different IRQ controllers for one
> device :-(
> 
> We need to check.
> 


Yeah, I also dont think this will work, because we will reparent the
interrupt, setting to a different controller. That will break the TALERT
signal already defined at GIC (check original patch).

I propose keeping the way I sent. Unless there is a way to set two
different controllers to same device.

> Regards,
> Benoit
> 
> 
>>> Where is the gpio controller phandle?
>>>
>>> Usually it looks like this:
>>>
>>> 	gpios = <&gpio1 8 0>;
>>>
>>>
>>> Regards,
>>> Benoit
>>>
>>>
>>>
>>
>>
> 
> 
>
Eduardo Valentin - May 16, 2013, 12:29 p.m.
On 15-05-2013 12:57, Jean-Christophe PLAGNIOL-VILLARD wrote:
> On 12:36 Wed 15 May     , Eduardo Valentin wrote:
>> On 15-05-2013 11:23, Benoit Cousson wrote:
>>> Hi Eduardo,
>>>
>>> On 05/15/2013 04:58 PM, Eduardo Valentin wrote:
>>>> Include bandgap devices for OMAP4460 devices.
>>>>
>>>> Cc: "Benoît Cousson" <b-cousson@ti.com>
>>>> Cc: Tony Lindgren <tony@atomide.com>
>>>> Cc: Russell King <linux@arm.linux.org.uk>
>>>> Cc: linux-omap@vger.kernel.org
>>>> Cc: devicetree-discuss@lists.ozlabs.org
>>>> Cc: linux-arm-kernel@lists.infradead.org
>>>> Cc: linux-kernel@vger.kernel.org
>>>> Signed-off-by: Eduardo Valentin <eduardo.valentin@ti.com>
>>>> ---
>>>>  arch/arm/boot/dts/omap4460.dtsi | 9 +++++++++
>>>>  1 file changed, 9 insertions(+)
>>>>
>>>> diff --git a/arch/arm/boot/dts/omap4460.dtsi b/arch/arm/boot/dts/omap4460.dtsi
>>>> index 2cf227c..e5bfbfe 100644
>>>> --- a/arch/arm/boot/dts/omap4460.dtsi
>>>> +++ b/arch/arm/boot/dts/omap4460.dtsi
>>>> @@ -29,4 +29,13 @@
>>>>  			     <0 55 0x4>;
>>>>  		ti,hwmods = "debugss";
>>>>  	};
>>>> +
>>>> +	bandgap {
>>>> +		reg = <0x4a002260 0x4
>>>> +			0x4a00232C 0x4
>>>> +			0x4a002378 0x18>;
>>>> +		compatible = "ti,omap4460-bandgap";
>>>> +		interrupts = <0 126 4>; /* talert */
>>>> +		ti,tshut-gpio = <86>;
>>
>>
>>
>>>
>>> Why do you need a custom attribute for GPIO? Cannot you use the standard
>>> one?
>>
>> I believe it was by your suggestion :-), during the first attempts to
>> send this driver. But could not find the thread link :-( sorry.
>>
>>
>> I guess the reasoning to mark it as a ti specific is because it will be
>> used as IRQ line to treat thermal shutdown (in SW).
> so use interrup-parent


Jean-Christophe,

That will change the controller and break the configuration for the
other IRQ line, coming from GIC.

>>
>>>
>>> Where is the gpio controller phandle?
>>>
>>> Usually it looks like this:
>>>
>>> 	gpios = <&gpio1 8 0>;
>>>
>>>
>>> Regards,
>>> Benoit
>>>
>>>
>>>
>>
>>
> 
> 
> 
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 
> 
>
Eduardo Valentin - May 29, 2013, 2:11 p.m.
Salut Monsieur Benoit,

On 16-05-2013 08:27, Eduardo Valentin wrote:
> On 16-05-2013 03:20, Benoit Cousson wrote:
>> Hi Eduardo,
>>

<cut>

>> We need to check.
>>
> 
> 
> Yeah, I also dont think this will work, because we will reparent the
> interrupt, setting to a different controller. That will break the TALERT
> signal already defined at GIC (check original patch).
> 
> I propose keeping the way I sent. Unless there is a way to set two
> different controllers to same device.
> 

Any idea on this patch? Shall we keep the way it is?


>> Regards,
>> Benoit
>>
>>
>>>> Where is the gpio controller phandle?
>>>>
>>>> Usually it looks like this:
>>>>
>>>> 	gpios = <&gpio1 8 0>;
>>>>
>>>>
>>>> Regards,
>>>> Benoit
>>>>
>>>>
>>>>
>>>
>>>
>>
>>
>>
> 
>
Cousson, Benoit - May 29, 2013, 2:19 p.m.
Hi Eduardo,

On 5/29/2013 4:11 PM, Eduardo Valentin wrote:
> Salut Monsieur Benoit,
>
> On 16-05-2013 08:27, Eduardo Valentin wrote:
>> On 16-05-2013 03:20, Benoit Cousson wrote:
>>> Hi Eduardo,
>>>
>
> <cut>
>
>>> We need to check.
>>
>> Yeah, I also dont think this will work, because we will reparent the
>> interrupt, setting to a different controller. That will break the TALERT
>> signal already defined at GIC (check original patch).
>>
>> I propose keeping the way I sent. Unless there is a way to set two
>> different controllers to same device.
>>
>
> Any idea on this patch? Shall we keep the way it is?

Well since we cannot use directly interrupt, I think we need to use at 
least the proper gpio binding.

Thanks,
Benoit
Eduardo Valentin - May 29, 2013, 3:42 p.m.
On 29-05-2013 10:19, Cousson, Benoit wrote:
> Hi Eduardo,
> 
> On 5/29/2013 4:11 PM, Eduardo Valentin wrote:
>> Salut Monsieur Benoit,
>>
>> On 16-05-2013 08:27, Eduardo Valentin wrote:
>>> On 16-05-2013 03:20, Benoit Cousson wrote:
>>>> Hi Eduardo,
>>>>
>>
>> <cut>
>>
>>>> We need to check.
>>>
>>> Yeah, I also dont think this will work, because we will reparent the
>>> interrupt, setting to a different controller. That will break the TALERT
>>> signal already defined at GIC (check original patch).
>>>
>>> I propose keeping the way I sent. Unless there is a way to set two
>>> different controllers to same device.
>>>
>>
>> Any idea on this patch? Shall we keep the way it is?
> 
> Well since we cannot use directly interrupt, I think we need to use at
> least the proper gpio binding.
> 

hmm... OK. sounds reasonable. I will change the driver and resend this
one in one single series.

> Thanks,
> Benoit
> 
> 
>
Jean-Christophe PLAGNIOL-VILLARD - May 31, 2013, 3:24 p.m.
On 08:29 Thu 16 May     , Eduardo Valentin wrote:
> On 15-05-2013 12:57, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > On 12:36 Wed 15 May     , Eduardo Valentin wrote:
> >> On 15-05-2013 11:23, Benoit Cousson wrote:
> >>> Hi Eduardo,
> >>>
> >>> On 05/15/2013 04:58 PM, Eduardo Valentin wrote:
> >>>> Include bandgap devices for OMAP4460 devices.
> >>>>
> >>>> Cc: "Benoît Cousson" <b-cousson@ti.com>
> >>>> Cc: Tony Lindgren <tony@atomide.com>
> >>>> Cc: Russell King <linux@arm.linux.org.uk>
> >>>> Cc: linux-omap@vger.kernel.org
> >>>> Cc: devicetree-discuss@lists.ozlabs.org
> >>>> Cc: linux-arm-kernel@lists.infradead.org
> >>>> Cc: linux-kernel@vger.kernel.org
> >>>> Signed-off-by: Eduardo Valentin <eduardo.valentin@ti.com>
> >>>> ---
> >>>>  arch/arm/boot/dts/omap4460.dtsi | 9 +++++++++
> >>>>  1 file changed, 9 insertions(+)
> >>>>
> >>>> diff --git a/arch/arm/boot/dts/omap4460.dtsi b/arch/arm/boot/dts/omap4460.dtsi
> >>>> index 2cf227c..e5bfbfe 100644
> >>>> --- a/arch/arm/boot/dts/omap4460.dtsi
> >>>> +++ b/arch/arm/boot/dts/omap4460.dtsi
> >>>> @@ -29,4 +29,13 @@
> >>>>  			     <0 55 0x4>;
> >>>>  		ti,hwmods = "debugss";
> >>>>  	};
> >>>> +
> >>>> +	bandgap {
> >>>> +		reg = <0x4a002260 0x4
> >>>> +			0x4a00232C 0x4
> >>>> +			0x4a002378 0x18>;
> >>>> +		compatible = "ti,omap4460-bandgap";
> >>>> +		interrupts = <0 126 4>; /* talert */
> >>>> +		ti,tshut-gpio = <86>;
> >>
> >>
> >>
> >>>
> >>> Why do you need a custom attribute for GPIO? Cannot you use the standard
> >>> one?
> >>
> >> I believe it was by your suggestion :-), during the first attempts to
> >> send this driver. But could not find the thread link :-( sorry.
> >>
> >>
> >> I guess the reasoning to mark it as a ti specific is because it will be
> >> used as IRQ line to treat thermal shutdown (in SW).
> > so use interrup-parent
> 
> 
> Jean-Christophe,
> 
> That will change the controller and break the configuration for the
> other IRQ line, coming from GIC.

put this on hold I'm preparing a fix for this
I've discuss with Grant already.

This way for me is buggy

Best Regards,
J.
> 
> >>
> >>>
> >>> Where is the gpio controller phandle?
> >>>
> >>> Usually it looks like this:
> >>>
> >>> 	gpios = <&gpio1 8 0>;
> >>>
> >>>
> >>> Regards,
> >>> Benoit
> >>>
> >>>
> >>>
> >>
> >>
> > 
> > 
> > 
> >> _______________________________________________
> >> linux-arm-kernel mailing list
> >> linux-arm-kernel@lists.infradead.org
> >> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> > 
> > 
> > 
> 
>

Patch

diff --git a/arch/arm/boot/dts/omap4460.dtsi b/arch/arm/boot/dts/omap4460.dtsi
index 2cf227c..e5bfbfe 100644
--- a/arch/arm/boot/dts/omap4460.dtsi
+++ b/arch/arm/boot/dts/omap4460.dtsi
@@ -29,4 +29,13 @@ 
 			     <0 55 0x4>;
 		ti,hwmods = "debugss";
 	};
+
+	bandgap {
+		reg = <0x4a002260 0x4
+			0x4a00232C 0x4
+			0x4a002378 0x18>;
+		compatible = "ti,omap4460-bandgap";
+		interrupts = <0 126 4>; /* talert */
+		ti,tshut-gpio = <86>;
+	};
 };