diff mbox series

arm: dts: gta04: add gps support

Message ID 20190125194310.11170-1-andreas@kemnade.info (mailing list archive)
State New, archived
Headers show
Series arm: dts: gta04: add gps support | expand

Commit Message

Andreas Kemnade Jan. 25, 2019, 7:43 p.m. UTC 
The GTA04 has a w2sg0004 or w2sg0084 gps chip. Not detectable
which one is mounted so use the compatibility entry for w2sg0004
for all which will work for both.

Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
---
w2sg0004 bindings (together with the corresponding support is in
https://git.kernel.org/pub/scm/linux/kernel/git/johan/gnss gnss-next)
 arch/arm/boot/dts/omap3-gta04.dtsi | 13 +++++++++++++
 1 file changed, 13 insertions(+)

Comments

Johan Hovold Jan. 28, 2019, 7:53 a.m. UTC | #1 
On Fri, Jan 25, 2019 at 08:43:10PM +0100, Andreas Kemnade wrote:
> The GTA04 has a w2sg0004 or w2sg0084 gps chip. Not detectable
> which one is mounted so use the compatibility entry for w2sg0004
> for all which will work for both.
> 
> Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
> ---
> w2sg0004 bindings (together with the corresponding support is in
> https://git.kernel.org/pub/scm/linux/kernel/git/johan/gnss gnss-next)
>  arch/arm/boot/dts/omap3-gta04.dtsi | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/omap3-gta04.dtsi b/arch/arm/boot/dts/omap3-gta04.dtsi
> index e53d32691308..d58c117e429f 100644
> --- a/arch/arm/boot/dts/omap3-gta04.dtsi
> +++ b/arch/arm/boot/dts/omap3-gta04.dtsi
> @@ -312,6 +312,12 @@
>  		>;
>         };
>  
> +	gps_pins: pinmux_gps_pins {
> +		pinctrl-single,pins = <
> +			OMAP3_CORE1_IOPAD(0x2176, PIN_OUTPUT_PULLDOWN | MUX_MODE4) /* gpio145 */
> +		>;
> +	};
> +
>  	hdq_pins: hdq_pins {
>  		pinctrl-single,pins = <
>  			OMAP3_CORE1_IOPAD(0x21c6, PIN_INPUT_PULLUP | MUX_MODE0) /* i2c3_sda.hdq */
> @@ -644,6 +650,13 @@
>  &uart2 {
>  	pinctrl-names = "default";
>  	pinctrl-0 = <&uart2_pins>;
> +	gps: gps {

The node should be named "gnss" as per the binding.

> +		compatible = "wi2wi,w2sg0004";
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&gps_pins>;
> +		sirf,onoff-gpios = <&gpio5 17 GPIO_ACTIVE_HIGH>;
> +		lna-supply = <&vsim>;

Also, the vcc-supply is a required property.

> +	};
>  };
>  
>  &uart3 {

Johan
Andreas Kemnade Jan. 28, 2019, 4:44 p.m. UTC | #2 
On Mon, 28 Jan 2019 08:53:56 +0100
Johan Hovold <johan@kernel.org> wrote:

> On Fri, Jan 25, 2019 at 08:43:10PM +0100, Andreas Kemnade wrote:
> > The GTA04 has a w2sg0004 or w2sg0084 gps chip. Not detectable
> > which one is mounted so use the compatibility entry for w2sg0004
> > for all which will work for both.
> > 
> > Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
> > ---
> > w2sg0004 bindings (together with the corresponding support is in
> > https://git.kernel.org/pub/scm/linux/kernel/git/johan/gnss gnss-next)
> >  arch/arm/boot/dts/omap3-gta04.dtsi | 13 +++++++++++++
> >  1 file changed, 13 insertions(+)
> > 
> > diff --git a/arch/arm/boot/dts/omap3-gta04.dtsi b/arch/arm/boot/dts/omap3-gta04.dtsi
> > index e53d32691308..d58c117e429f 100644
> > --- a/arch/arm/boot/dts/omap3-gta04.dtsi
> > +++ b/arch/arm/boot/dts/omap3-gta04.dtsi
> > @@ -312,6 +312,12 @@  
> >  		>;  
> >         };
> >  
> > +	gps_pins: pinmux_gps_pins {
> > +		pinctrl-single,pins = <
> > +			OMAP3_CORE1_IOPAD(0x2176, PIN_OUTPUT_PULLDOWN | MUX_MODE4) /* gpio145 */
> > +		>;
> > +	};
> > +
> >  	hdq_pins: hdq_pins {
> >  		pinctrl-single,pins = <
> >  			OMAP3_CORE1_IOPAD(0x21c6, PIN_INPUT_PULLUP | MUX_MODE0) /* i2c3_sda.hdq */
> > @@ -644,6 +650,13 @@
> >  &uart2 {
> >  	pinctrl-names = "default";
> >  	pinctrl-0 = <&uart2_pins>;
> > +	gps: gps {  
> 
> The node should be named "gnss" as per the binding.
> 
> > +		compatible = "wi2wi,w2sg0004";
> > +		pinctrl-names = "default";
> > +		pinctrl-0 = <&gps_pins>;
> > +		sirf,onoff-gpios = <&gpio5 17 GPIO_ACTIVE_HIGH>;
> > +		lna-supply = <&vsim>;  
> 
> Also, the vcc-supply is a required property.
> 
well, it is not require in the driver and it has different behavior (on even when not opened if
on-off is there) than the lna-supply used here. So maybe fix the binding documentation?

Regards,
Andreas
Johan Hovold Jan. 30, 2019, 9:02 a.m. UTC | #3 
On Mon, Jan 28, 2019 at 05:44:29PM +0100, Andreas Kemnade wrote:
> On Mon, 28 Jan 2019 08:53:56 +0100
> Johan Hovold <johan@kernel.org> wrote:
> 
> > On Fri, Jan 25, 2019 at 08:43:10PM +0100, Andreas Kemnade wrote:
> > > The GTA04 has a w2sg0004 or w2sg0084 gps chip. Not detectable
> > > which one is mounted so use the compatibility entry for w2sg0004
> > > for all which will work for both.
> > > 
> > > Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
> > > ---
> > > w2sg0004 bindings (together with the corresponding support is in
> > > https://git.kernel.org/pub/scm/linux/kernel/git/johan/gnss gnss-next)
> > >  arch/arm/boot/dts/omap3-gta04.dtsi | 13 +++++++++++++
> > >  1 file changed, 13 insertions(+)

> > > +	gps: gps {  
> > 
> > The node should be named "gnss" as per the binding.
> > 
> > > +		compatible = "wi2wi,w2sg0004";
> > > +		pinctrl-names = "default";
> > > +		pinctrl-0 = <&gps_pins>;
> > > +		sirf,onoff-gpios = <&gpio5 17 GPIO_ACTIVE_HIGH>;
> > > +		lna-supply = <&vsim>;  
> > 
> > Also, the vcc-supply is a required property.
> > 
> well, it is not require in the driver and it has different behavior
> (on even when not opened if on-off is there) than the lna-supply used
> here. So maybe fix the binding documentation?

The device-tree describes hardware, and how a particular driver happens
to implement a binding is not relevant.

That said, there is a bit of an on-going, shall we say philosophical,
debate about this. The regulator maintainer takes a firm position that
all mandatory physical supplies should be represented in firmware
	
	https://lore.kernel.org/lkml/20181123133126.GF2089@sirena.org.uk/T/#u
	https://lore.kernel.org/lkml/20180409102244.GB11532@sirena.org.uk/T/#u

while Rob appears to take a slightly different stance on fixed
regulators while admitting that this an issue which has not yet been
fully resolved:

	https://lore.kernel.org/lkml/20180425171123.xhyoay3nu463btoq@rob-hp-laptop/T/#u

Since this is a new binding, and the hardware requires the vcc supply
and this is reflected in the binding, I think you should add a fixed
regulator. At least until you hear otherwise. ;)

Johan
H. Nikolaus Schaller Jan. 30, 2019, 2:06 p.m. UTC | #4 
Hi Andreas,

> Am 30.01.2019 um 10:02 schrieb Johan Hovold <johan@kernel.org>:
> 
> On Mon, Jan 28, 2019 at 05:44:29PM +0100, Andreas Kemnade wrote:
>> On Mon, 28 Jan 2019 08:53:56 +0100
>> Johan Hovold <johan@kernel.org> wrote:
>> 
>>> On Fri, Jan 25, 2019 at 08:43:10PM +0100, Andreas Kemnade wrote:
>>>> The GTA04 has a w2sg0004 or w2sg0084 gps chip. Not detectable
>>>> which one is mounted so use the compatibility entry for w2sg0004
>>>> for all which will work for both.
>>>> 
>>>> Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
>>>> ---
>>>> w2sg0004 bindings (together with the corresponding support is in
>>>> https://git.kernel.org/pub/scm/linux/kernel/git/johan/gnss gnss-next)
>>>> arch/arm/boot/dts/omap3-gta04.dtsi | 13 +++++++++++++
>>>> 1 file changed, 13 insertions(+)
> 
>>>> +	gps: gps {  
>>> 
>>> The node should be named "gnss" as per the binding.
>>> 
>>>> +		compatible = "wi2wi,w2sg0004";
>>>> +		pinctrl-names = "default";
>>>> +		pinctrl-0 = <&gps_pins>;
>>>> +		sirf,onoff-gpios = <&gpio5 17 GPIO_ACTIVE_HIGH>;
>>>> +		lna-supply = <&vsim>;  
>>> 
>>> Also, the vcc-supply is a required property.
>>> 
>> well, it is not require in the driver and it has different behavior
>> (on even when not opened if on-off is there) than the lna-supply used
>> here. So maybe fix the binding documentation?
> 
> The device-tree describes hardware, and how a particular driver happens
> to implement a binding is not relevant.
> 
> That said, there is a bit of an on-going, shall we say philosophical,
> debate about this. The regulator maintainer takes a firm position that
> all mandatory physical supplies should be represented in firmware
> 	
> 	https://lore.kernel.org/lkml/20181123133126.GF2089@sirena.org.uk/T/#u
> 	https://lore.kernel.org/lkml/20180409102244.GB11532@sirena.org.uk/T/#u
> 
> while Rob appears to take a slightly different stance on fixed
> regulators while admitting that this an issue which has not yet been
> fully resolved:
> 
> 	https://lore.kernel.org/lkml/20180425171123.xhyoay3nu463btoq@rob-hp-laptop/T/#u
> 
> Since this is a new binding, and the hardware requires the vcc supply
> and this is reflected in the binding, I think you should add a fixed
> regulator. At least until you hear otherwise. ;)

Assuming that there is no REGEN signal from the twl4030 unless 1V8 is also
stable, I'd suggest as a simple solution:

	 vcc-supply = <&vio>;

Alternatively, we could define a dedicated fixed-regulator in omap3-gta04.dtsi
for the 3V3 rail. Which is always-on. This would allow to describe that e.g. the
itg3200, panel and other chips and sensors are also supplied by this. But since
no driver can really make use of it (turn on/off on demand) this is IMHO quite
needless.

BR,
Nikolaus
Andreas Kemnade Jan. 30, 2019, 8:11 p.m. UTC | #5 
On Wed, 30 Jan 2019 15:06:12 +0100
"H. Nikolaus Schaller" <hns@goldelico.com> wrote:

> Hi Andreas,
> 
> > Am 30.01.2019 um 10:02 schrieb Johan Hovold <johan@kernel.org>:
> > 
> > On Mon, Jan 28, 2019 at 05:44:29PM +0100, Andreas Kemnade wrote:  
> >> On Mon, 28 Jan 2019 08:53:56 +0100
> >> Johan Hovold <johan@kernel.org> wrote:
> >>   
> >>> On Fri, Jan 25, 2019 at 08:43:10PM +0100, Andreas Kemnade wrote:  
> >>>> The GTA04 has a w2sg0004 or w2sg0084 gps chip. Not detectable
> >>>> which one is mounted so use the compatibility entry for w2sg0004
> >>>> for all which will work for both.
> >>>> 
> >>>> Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
> >>>> ---
> >>>> w2sg0004 bindings (together with the corresponding support is in
> >>>> https://git.kernel.org/pub/scm/linux/kernel/git/johan/gnss gnss-next)
> >>>> arch/arm/boot/dts/omap3-gta04.dtsi | 13 +++++++++++++
> >>>> 1 file changed, 13 insertions(+)  
> >   
> >>>> +	gps: gps {    
> >>> 
> >>> The node should be named "gnss" as per the binding.
> >>>   
> >>>> +		compatible = "wi2wi,w2sg0004";
> >>>> +		pinctrl-names = "default";
> >>>> +		pinctrl-0 = <&gps_pins>;
> >>>> +		sirf,onoff-gpios = <&gpio5 17 GPIO_ACTIVE_HIGH>;
> >>>> +		lna-supply = <&vsim>;    
> >>> 
> >>> Also, the vcc-supply is a required property.
> >>>   
> >> well, it is not require in the driver and it has different behavior
> >> (on even when not opened if on-off is there) than the lna-supply used
> >> here. So maybe fix the binding documentation?  
> > 
> > The device-tree describes hardware, and how a particular driver happens
> > to implement a binding is not relevant.
> > 
> > That said, there is a bit of an on-going, shall we say philosophical,
> > debate about this. The regulator maintainer takes a firm position that
> > all mandatory physical supplies should be represented in firmware
> > 	
> > 	https://lore.kernel.org/lkml/20181123133126.GF2089@sirena.org.uk/T/#u
> > 	https://lore.kernel.org/lkml/20180409102244.GB11532@sirena.org.uk/T/#u
> > 
> > while Rob appears to take a slightly different stance on fixed
> > regulators while admitting that this an issue which has not yet been
> > fully resolved:
> > 
> > 	https://lore.kernel.org/lkml/20180425171123.xhyoay3nu463btoq@rob-hp-laptop/T/#u
> > 
> > Since this is a new binding, and the hardware requires the vcc supply
> > and this is reflected in the binding, I think you should add a fixed
> > regulator. At least until you hear otherwise. ;)  
> 
> Assuming that there is no REGEN signal from the twl4030 unless 1V8 is also
> stable, I'd suggest as a simple solution:
> 
> 	 vcc-supply = <&vio>;
> 
> Alternatively, we could define a dedicated fixed-regulator in omap3-gta04.dtsi
> for the 3V3 rail. Which is always-on. This would allow to describe that e.g. the
> itg3200, panel and other chips and sensors are also supplied by this. But since
> no driver can really make use of it (turn on/off on demand) this is IMHO quite
> needless.
> 
well, probably better to add that regulator, so we match the real
hardware and not doing some fake here just to satisfy that binding
requirement. I will send a new version with that regulator added.

Regards,
Andreas
diff mbox series

Patch

diff --git a/arch/arm/boot/dts/omap3-gta04.dtsi b/arch/arm/boot/dts/omap3-gta04.dtsi
index e53d32691308..d58c117e429f 100644
--- a/arch/arm/boot/dts/omap3-gta04.dtsi
+++ b/arch/arm/boot/dts/omap3-gta04.dtsi
@@ -312,6 +312,12 @@ 
 		>;
        };
 
+	gps_pins: pinmux_gps_pins {
+		pinctrl-single,pins = <
+			OMAP3_CORE1_IOPAD(0x2176, PIN_OUTPUT_PULLDOWN | MUX_MODE4) /* gpio145 */
+		>;
+	};
+
 	hdq_pins: hdq_pins {
 		pinctrl-single,pins = <
 			OMAP3_CORE1_IOPAD(0x21c6, PIN_INPUT_PULLUP | MUX_MODE0) /* i2c3_sda.hdq */
@@ -644,6 +650,13 @@ 
 &uart2 {
 	pinctrl-names = "default";
 	pinctrl-0 = <&uart2_pins>;
+	gps: gps {
+		compatible = "wi2wi,w2sg0004";
+		pinctrl-names = "default";
+		pinctrl-0 = <&gps_pins>;
+		sirf,onoff-gpios = <&gpio5 17 GPIO_ACTIVE_HIGH>;
+		lna-supply = <&vsim>;
+	};
 };
 
 &uart3 {