Re: [Letux-kernel] [PATCH] arm: dts: gta04: add gps support

[H. Nikolaus Schaller]

Hi Andreas,

> Am 30.01.2019 um 10:02 schrieb Johan Hovold <johan@xxxxxxxxxx>:
> 
> 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@xxxxxxxxxx> 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@xxxxxxxxxxxx>
>>>> ---
>>>> 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@xxxxxxxxxxxxx/T/#u
> 	https://lore.kernel.org/lkml/20180409102244.GB11532@xxxxxxxxxxxxx/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