Re: [PATCH v5 3/5] misc serdev: Add w2sg0004 (gps receiver) power control driver

2018-03-19 Thread Johan Hovold
On Wed, Mar 07, 2018 at 04:53:12PM +0100, H. Nikolaus Schaller wrote:

> If I look for example at the camera module drivers provided by
> drivers/media/i2c, most of them could be easily power-controlled from
> user-space by i2c-tools and 1-2 gpios through /sys/class/gpio and
> a big set of scripts.
> 
> Still they have a place in the kernel and cameras are powered on
> if the device is opened and powered down if it is closed.
> 
> So I am still trying to understand the rationale and logic (if one exists)
> behind having them in kernel but rejecting our driver which does the
> same for a different class of devices.

For media we have a framework in place; for gps we do not (yet).

Johan


Re: [PATCH v5 3/5] misc serdev: Add w2sg0004 (gps receiver) power control driver

2018-03-07 Thread H. Nikolaus Schaller
Hi Johan,
I know you have a lot of other things to do, but we are still waiting for a
statement that this driver will be accepted after fixing the final coding 
issues.

Please advise on how you want to proceed with this.

One more technical comment/question/aspect below:

> Am 18.01.2018 um 14:43 schrieb H. Nikolaus Schaller :
> 
> Hi Johan,
> 
>> Am 18.01.2018 um 07:13 schrieb Johan Hovold :
>> 
>> Hacks are never good, but sometimes needed. But we should try to keep
>> them contained in drivers rather than allow them to spread to core code,
>> was what I was trying to say above.
> 
> Well, aren't we talking here about a well isolated driver? And not about
> core code?
> 
> Core code already provides everything to build a driver for this chip
> to make us happy with.
> 
> It is just not yet providing a generic gps interface API to make everybody
> happy with.
> 
>>> That is what Andreas did remark as motivation: provide a solution
>>> for *existing* user spaces.
>> 
>> I understand that this is what you want, but that in itself is not a
>> sufficient reason to put something in the kernel.
> 
> Agreed, but it is a secondary (but still strong) motivation, not the primary 
> one.
> 
>> 
> - can not guarantee (!) to power off the chip if the last user-space
> process using it is killed (which is essential for power-management of
> a handheld, battery operated device)
 
 That depends on how you implement things (extending gpsd, wrapper
 script, pty daemon, ...).
>>> 
>>> No. You can of course cover all standard cases but there is one fundamental
>>> issue which is IMHO a problem of any user-space implementation:
>>> 
>>> How can you guarantee that the chip is powered off if no
>>> user-space process is using it or if the last process doing
>>> this is killed by *whatever* reason?
>>> 
>>> E.g. after a kill -9. Or if someone deinstalls gpsd or whatever and assumes
>>> (and wants a guarantee) that GPS is now turned off and never turned on 
>>> drawing
>>> precious milliamps from the battery for no use.
>> 
>> Have something run at init to put the device in a low power state.

If I look for example at the camera module drivers provided by
drivers/media/i2c, most of them could be easily power-controlled from
user-space by i2c-tools and 1-2 gpios through /sys/class/gpio and
a big set of scripts.

Still they have a place in the kernel and cameras are powered on
if the device is opened and powered down if it is closed.

So I am still trying to understand the rationale and logic (if one exists)
behind having them in kernel but rejecting our driver which does the
same for a different class of devices.

> This does *not* solve the issue how to *guarantee* that it becomes
> powered off if the number of user-space processes goes down to zero
> *after* init.
> 
> Please consider that a portable device is rarely booted but might be
> operated over several days with many suspend cycles. And people may
> still expect that the power consumer "GPS" is turned off if their
> personal user-space setup simply kills gpsd.

> 
>> 
>>> As it is well known, a user-space process can't protect itself against kill 
>>> -9.
>>> Or has this recently been changed and I am not aware of?
>>> 
>>> This is the fundamental reason why we need a kernel driver to provide
>>> reliable, repeatable and trustable power management of this chip.
>>> 
>>> It is equally fundamental as a hard disk should spin down after the last
>>> file is closed. Even if this process ends by a kill -9.
> 
> Please advise how we should solve this fundamental problem in user-space.
> 
>>> 
>>> This seems to contradict your argument that user-space can very easily
>>> adapt to everything. If the latter were true there would be no need to
>>> keep old interfaces supported for a long time.
>> 
>> You probably know that we try hard never to change an interface that
>> would break user space, and that's why we need to get it right.
> 
> Yes, I know and agree that it is very important (and difficult to achieve).
> 
> But it seems that there are different opinions of what "right" is...
> 
> You seem to focus on the "right" API only (where we agree that the "right"
> API does not exist and likely will never come or at least in the near future).
> 
> But for us the whole combination of kernel + user-space must behave "right"
> (and use a function split that allows to optimally achieve this goal).
> 
>> 
>>> So can you agree to that a battery powered portable device must have
>>> reliable and trustable power management? And if it provable can't be
>>> implemented in user-space (a single counter example suffices) it must
>>> be a kernel driver?
>> 
>> Having a kernel driver would make things easier for user space, sure,
>> but again, that's not a sufficient reason to merge just any kernel
>> implementation.
> 
> It is not about "easier" for anyone. Neither for you nor for me. For us
> it would be much easier not to have to run this never-ending disc

Re: [PATCH v5 3/5] misc serdev: Add w2sg0004 (gps receiver) power control driver

2018-01-18 Thread H. Nikolaus Schaller
Hi Johan,

> Am 18.01.2018 um 07:13 schrieb Johan Hovold :
> 
> Hacks are never good, but sometimes needed. But we should try to keep
> them contained in drivers rather than allow them to spread to core code,
> was what I was trying to say above.

Well, aren't we talking here about a well isolated driver? And not about
core code?

Core code already provides everything to build a driver for this chip
to make us happy with.

It is just not yet providing a generic gps interface API to make everybody
happy with.

>> That is what Andreas did remark as motivation: provide a solution
>> for *existing* user spaces.
> 
> I understand that this is what you want, but that in itself is not a
> sufficient reason to put something in the kernel.

Agreed, but it is a secondary (but still strong) motivation, not the primary 
one.

> 
 - can not guarantee (!) to power off the chip if the last user-space
 process using it is killed (which is essential for power-management of
 a handheld, battery operated device)
>>> 
>>> That depends on how you implement things (extending gpsd, wrapper
>>> script, pty daemon, ...).
>> 
>> No. You can of course cover all standard cases but there is one fundamental
>> issue which is IMHO a problem of any user-space implementation:
>> 
>>  How can you guarantee that the chip is powered off if no
>>  user-space process is using it or if the last process doing
>>  this is killed by *whatever* reason?
>> 
>> E.g. after a kill -9. Or if someone deinstalls gpsd or whatever and assumes
>> (and wants a guarantee) that GPS is now turned off and never turned on 
>> drawing
>> precious milliamps from the battery for no use.
> 
> Have something run at init to put the device in a low power state.

This does *not* solve the issue how to *guarantee* that it becomes
powered off if the number of user-space processes goes down to zero
*after* init.

Please consider that a portable device is rarely booted but might be
operated over several days with many suspend cycles. And people may
still expect that the power consumer "GPS" is turned off if their
personal user-space setup simply kills gpsd.

> 
>> As it is well known, a user-space process can't protect itself against kill 
>> -9.
>> Or has this recently been changed and I am not aware of?
>> 
>> This is the fundamental reason why we need a kernel driver to provide
>> reliable, repeatable and trustable power management of this chip.
>> 
>> It is equally fundamental as a hard disk should spin down after the last
>> file is closed. Even if this process ends by a kill -9.

Please advise how we should solve this fundamental problem in user-space.

>> 
>> This seems to contradict your argument that user-space can very easily
>> adapt to everything. If the latter were true there would be no need to
>> keep old interfaces supported for a long time.
> 
> You probably know that we try hard never to change an interface that
> would break user space, and that's why we need to get it right.

Yes, I know and agree that it is very important (and difficult to achieve).

But it seems that there are different opinions of what "right" is...

You seem to focus on the "right" API only (where we agree that the "right"
API does not exist and likely will never come or at least in the near future).

But for us the whole combination of kernel + user-space must behave "right"
(and use a function split that allows to optimally achieve this goal).

> 
>> So can you agree to that a battery powered portable device must have
>> reliable and trustable power management? And if it provable can't be
>> implemented in user-space (a single counter example suffices) it must
>> be a kernel driver?
> 
> Having a kernel driver would make things easier for user space, sure,
> but again, that's not a sufficient reason to merge just any kernel
> implementation.

It is not about "easier" for anyone. Neither for you nor for me. For us
it would be much easier not to have to run this never-ending discussion
over and over...

It is about making it technically fully "reliable". And not 99% as per
quick and dirty user-space hacks.

It is so easy to invent scenarios in user-space to make the device practically
unusable by unnoticed draining a full battery within less than a day when
not perfectly turning off the chip power.

So if a kernel driver can better protect users against this situation for
a portable device with this chip, why shouldn't it do with a handful LOC?
What requirement is more important than this?

BR and thanks,
Nikolaus



Re: [PATCH v5 3/5] misc serdev: Add w2sg0004 (gps receiver) power control driver

2018-01-17 Thread Johan Hovold
On Fri, Jan 12, 2018 at 06:59:59PM +0100, H. Nikolaus Schaller wrote:
> Hi Johan,
> 
> > Am 12.01.2018 um 16:39 schrieb Johan Hovold :
> > 
> >> Let's restart this discussion and focus on the main roadblock (others
> >> are minor details which can be sorted out later).
> >> 
> >> If it feels like a hack, the key issue seems to me to be the choice of
> >> the API to present the GPS data to user space. Right?
> > 
> > Or even more fundamentally, does this belong in the kernel at all?
> 
> Yes, that can be questioned of course. It was questioned and discussed
> several times and I thought the answer was a clear yes. But let's reiterate.
> 
> > Also it seems at least part of your specific problem is that you have
> > failed to wire up the WAKEUP pin of the W2SG0004/84 properly,
> 
> The w2sg0004 has no wakeup pin. At least I can't find one in the data sheet.

I should have said w2sg0084 above, which is the only datasheet I have found.

> The two pins you refer to from the 0084 data sheet are called BootSelect0/1
> in the 0004 and have a different function.
> 
> To be clear, we did not fail to wire it up. We did the design before the
> 0084 was announced and available. We just had to swap in the 0084 into
> existing PCBs during production because the 0004 became EOL. Otherwise
> we would probably still use the 0004 without WAKEUP output.
> 
> To make it worse, we have no documentation for an individual board if
> an 0004 or 0084 chip is installed and there is no means how a software
> can find out which one it is talking to (especially before properly
> powering on). Therefore we can not even provide two different device
> trees or drivers or whatever, unless we ask people to open their device
> and look on the chip. Quite crazy wrt. user-friendlyness of software
> installation in 2018...
> 
> Therefore, a driver must be capable to handle both chips in the same way,
> with minimalistic assumptions, even if the 0084 could provide a direct
> signal to make it easier than using serdev to monitor the data stream.

Fair enough.

> >  which then
> > forces you to look at the data stream to determine the power state of
> > the chip. Judging from a quick look at the GTA04 schematics it seems
> > you've even connected the WAKEUP output to the 1V8_OUT output?!
> 
> No. You failed to see that this is an optional 0R, which is not installed.
> The 0R on pin 7 (BootSelect1) to GND was removed when we did switch from
> 0004 to 0084. Pin 6 (BootSelect0/WAKEUP) was never connected.

Ok.

> > The kernel is probably not the place to be working around issues like
> > that,

> > even if serdev at least allows for such hacks to be fairly
> > isolated in drivers (unlike some of the earlier proposals touching core
> > code).
> 
> Please tell me why there are so many hacks for hardware issues in certain
> drivers. Any why those are good and this one (if it is one at all) is not.

Hacks are never good, but sometimes needed. But we should try to keep
them contained in drivers rather than allow them to spread to core code,
was what I was trying to say above.

> What I can learn from your discussion is that it might be considerable
> to add an optional gpio for the 0084 WAKEUP and add some logic to
> support users who have or will have that pin connected.
> 
> But even then we would need a driver to handle this gpio and issue
> an on/off impulse on the other to switch states. It would be a different
> driver (variant - maybe some CONFIG option or handled by code), but not
> "no driver".

Having a WAKEUP signal would allow for a more straight-forward
implementation, be it in the kernel or in user space.

> >> I see three reasonable options how this presentation can be done:
> >> 
> >> 1. char device
> >> 2. tty device
> >> 3. some new gps interface API (similar to network, bluetooth interfaces)
> >> 4. no driver and use the UART tty directly
> >> 
> >> Pros and cons:
> > 
> >> 4. no driver and use UART directly
> >> + a non-solution seems to be attractive
> >> - must turn on/off chip by gpio hacks from user-space
> > 
> > I'm not sure that would amount to more of hack then doing it in the
> > kernel would.
> 
> It might not be big effort in the user-space code/scripts.
> 
> But much effort to convince all the plethora of user-space client maintainers
> to integrate something. And have them roll out. And have distributions take 
> it.
> And have users upgrade to it. 5 years later...
> 
> Do you think it is easier to convince them than you? They usually assume a
> power management issue should be solved by the kernel driver.
> 
> That is what Andreas did remark as motivation: provide a solution
> for *existing* user spaces.

I understand that this is what you want, but that in itself is not a
sufficient reason to put something in the kernel.

> >> - can not guarantee (!) to power off the chip if the last user-space
> >> process using it is killed (which is essential for power-management of
> >> a handheld, battery operated device)
> > 
> > Th

Re: [PATCH v5 3/5] misc serdev: Add w2sg0004 (gps receiver) power control driver

2018-01-12 Thread H. Nikolaus Schaller
Hi Johan,

> Am 12.01.2018 um 16:39 schrieb Johan Hovold :
> 
>> Let's restart this discussion and focus on the main roadblock (others
>> are minor details which can be sorted out later).
>> 
>> If it feels like a hack, the key issue seems to me to be the choice of
>> the API to present the GPS data to user space. Right?
> 
> Or even more fundamentally, does this belong in the kernel at all?

Yes, that can be questioned of course. It was questioned and discussed
several times and I thought the answer was a clear yes. But let's reiterate.

> 
> Also it seems at least part of your specific problem is that you have
> failed to wire up the WAKEUP pin of the W2SG0004/84 properly,

The w2sg0004 has no wakeup pin. At least I can't find one in the data sheet.

The two pins you refer to from the 0084 data sheet are called BootSelect0/1
in the 0004 and have a different function.

To be clear, we did not fail to wire it up. We did the design before the
0084 was announced and available. We just had to swap in the 0084 into
existing PCBs during production because the 0004 became EOL. Otherwise
we would probably still use the 0004 without WAKEUP output.

To make it worse, we have no documentation for an individual board if
an 0004 or 0084 chip is installed and there is no means how a software
can find out which one it is talking to (especially before properly
powering on). Therefore we can not even provide two different device
trees or drivers or whatever, unless we ask people to open their device
and look on the chip. Quite crazy wrt. user-friendlyness of software
installation in 2018...

Therefore, a driver must be capable to handle both chips in the same way,
with minimalistic assumptions, even if the 0084 could provide a direct
signal to make it easier than using serdev to monitor the data stream.

>  which then
> forces you to look at the data stream to determine the power state of
> the chip. Judging from a quick look at the GTA04 schematics it seems
> you've even connected the WAKEUP output to the 1V8_OUT output?!

No. You failed to see that this is an optional 0R, which is not installed.
The 0R on pin 7 (BootSelect1) to GND was removed when we did switch from
0004 to 0084. Pin 6 (BootSelect0/WAKEUP) was never connected.

> The kernel is probably not the place to be working around issues like
> that,

You appear to assume this our only motivation is to make a workaround for
a hardware design flaw but that isn't.

The purpose of the driver is to provide power management for the GPS
subsystem which happens to be based on a chip with limited functionality.

And the serdev thing is the solution, not the requirement...

> even if serdev at least allows for such hacks to be fairly
> isolated in drivers (unlike some of the earlier proposals touching core
> code).

Please tell me why there are so many hacks for hardware issues in certain
drivers. Any why those are good and this one (if it is one at all) is not.

Some picks random fgrep -iR hack drivers

drivers/char/random.c: * Hack to deal with crazy userspace progams when they 
are all trying
drivers/clk/meson/meson8b.c: * a new clk_hw, and this hack will no longer 
work. Releasing the ccr
drivers/clk/samsung/clk-exynos3250.c:   /* HACK: fin_pll hardcoded to xusbxti 
until detection is implemented. */
drivers/gpu/drm/amd/amdkfd/kfd_events.c: * This hack is wrong, but 
nobody is likely to notice.
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c:   * HACK: IGT 
tests expect that each plane can only have one
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c:  /* It's a hack for s3 
since in 4.9 kernel filter out cursor buffer
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c:  /* TODO This 
hack should go away */
drivers/gpu/drm/amd/display/dc/core/dc_link.c:  /* A hack to avoid failing any 
modes for EDID override feature on

What I can learn from your discussion is that it might be considerable
to add an optional gpio for the 0084 WAKEUP and add some logic to
support users who have or will have that pin connected.

But even then we would need a driver to handle this gpio and issue
an on/off impulse on the other to switch states. It would be a different
driver (variant - maybe some CONFIG option or handled by code), but not
"no driver".

> 
>> I see three reasonable options how this presentation can be done:
>> 
>> 1. char device
>> 2. tty device
>> 3. some new gps interface API (similar to network, bluetooth interfaces)
>> 4. no driver and use the UART tty directly
>> 
>> Pros and cons:
> 
>> 4. no driver and use UART directly
>> + a non-solution seems to be attractive
>> - must turn on/off chip by gpio hacks from user-space
> 
> I'm not sure that would amount to more of hack then doing it in the
> kernel would.

It might not be big effort in the user-space code/scripts.

But much effort to convince all the plethora of user-space client maintainers
to integrate something. And have them roll out. And have distributions take it.
An

Re: [PATCH v5 3/5] misc serdev: Add w2sg0004 (gps receiver) power control driver

2017-12-22 Thread H. Nikolaus Schaller
Hi Johan,

> Am 22.12.2017 um 13:44 schrieb Johan Hovold :
> 
> On Fri, Dec 01, 2017 at 08:49:36AM +0100, H. Nikolaus Schaller wrote:
>> Add driver for Wi2Wi W2SG0004/84 GPS module connected to some SoC UART.
>> 
>> It uses serdev API hooks to monitor and forward the UART traffic to 
>> /dev/ttyGPSn
>> and turn on/off the module. It also detects if the module is turned on 
>> (sends data)
>> but should be off, e.g. if it was already turned on during boot or 
>> power-on-reset.
>> 
>> Additionally, rfkill block/unblock can be used to control an external LNA
>> (and power down the module if not needed).
>> 
>> The driver concept is based on code developed by Neil Brown 
>> but simplified and adapted to use the new serdev API introduced in v4.11.
> 
> I'm sorry (and I know this discussion has been going on for a long
> time),

I'd say: already too much time.

> but this still feels like too much of a hack.

Well, to me it feels like review process is trying to get things 200% right
in the first step and therefore delays proposals that may already be useful
to real world users. Review is of course important to protect against severe
bugs and security issues.

My concern is that in 5 more years this chip is obsolete and then we never
had a working solution in distributions that base directly on kernel.org...

What I am lacking in this discussion is the spirit of starting with something
that basically works and permanently enhancing things. Like Linux started
25 years ago and many drivers look very different in v4.15 than v2.6.32.

Instead, we are sent back every time to rework it completely and at some point
our enthusiasm (and time budget) has boiled down to 0 because we see no more
reward for working on this.

> 
> You're registering a tty driver to allow user space to continue treat
> this as a tty device, but you provide no means of actually modifying
> anything (line settings, etc).

That was planned for a second step but is not important for pure GPS
reception.

> It's essentially just a character device
> with common tty ioctls as noops from a device PoV (well, plus the ldisc
> buffering and processing).

Yes. The buffering is the more important aspect and as far as I understand
we would have to implement our own buffer for a chardev driver. Buffering is
perfectly solved for tty devices.

Another aspect is that the same chip connected through an USB or Bluetooth
connection is presented as a tty device and not a char dev.

> This will probably require someone to first implement a generic gps
> framework with a properly defined interface which you may then teach
> gpsd to use (e.g. to avoid all its autodetection functionality) instead.

Yes, that is what I dream of.

But in past 5 years there was no "someone" to pop up and my time to work
on this topic is too limited. It is not even my main focus of efforts.

So I prefer solutions that can be done today with today's means and not
dreams (even if we share them).

> Or some entirely different approach, for example, where you manage
> everything from user space.

> 
> I'd suggest reiterating the problem you're trying to solve and
> enumerating the previously discussed potential solutions in order to
> find a proper abstraction level for this (before getting lost in
> implementation details).

Yes, please feel free to write patches that implement it that way.

> 
> That being said, I'm still pointing some bugs and issue below that you
> can consider for future versions of this (and other drivers) below.

Thanks!

> 
>> Signed-off-by: H. Nikolaus Schaller 
>> ---
>> drivers/misc/Kconfig|  10 +
>> drivers/misc/Makefile   |   1 +
>> drivers/misc/w2sg0004.c | 553 
>> 
>> 3 files changed, 564 insertions(+)
>> create mode 100644 drivers/misc/w2sg0004.c
>> 
>> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
>> index f1a5c2357b14..a3b11016ed2b 100644
>> --- a/drivers/misc/Kconfig
>> +++ b/drivers/misc/Kconfig
>> @@ -508,4 +508,14 @@ source "drivers/misc/mic/Kconfig"
>> source "drivers/misc/genwqe/Kconfig"
>> source "drivers/misc/echo/Kconfig"
>> source "drivers/misc/cxl/Kconfig"
>> +
>> +config W2SG0004
>> +tristate "W2SG00x4 on/off control"
> 
> Please provide a better summary for what this driver does.

Ok.

> 
>> +depends on GPIOLIB && SERIAL_DEV_BUS
>> +help
>> +  Enable on/off control of W2SG00x4 GPS moduled connected

s/moduled/module/

> 
> Some whitespace issue here.

Ok.

> 
>> +  to some SoC UART to allow powering up/down if the /dev/ttyGPSn
>> +  is opened/closed.
>> +  It also provides a rfkill gps name to control the LNA power.
>> +
>> endmenu
>> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
>> index 5ca5f64df478..d9d824b3d20a 100644
>> --- a/drivers/misc/Makefile
>> +++ b/drivers/misc/Makefile
>> @@ -50,6 +50,7 @@ obj-$(CONFIG_SRAM_EXEC)+= sram-exec.o
>> obj-y+= mic/
>> obj-$(CONFIG_GENWQE) += genwq

Re: [PATCH v5 3/5] misc serdev: Add w2sg0004 (gps receiver) power control driver

2017-12-22 Thread Johan Hovold
On Fri, Dec 01, 2017 at 08:49:36AM +0100, H. Nikolaus Schaller wrote:
> Add driver for Wi2Wi W2SG0004/84 GPS module connected to some SoC UART.
> 
> It uses serdev API hooks to monitor and forward the UART traffic to 
> /dev/ttyGPSn
> and turn on/off the module. It also detects if the module is turned on (sends 
> data)
> but should be off, e.g. if it was already turned on during boot or 
> power-on-reset.
> 
> Additionally, rfkill block/unblock can be used to control an external LNA
> (and power down the module if not needed).
> 
> The driver concept is based on code developed by Neil Brown 
> but simplified and adapted to use the new serdev API introduced in v4.11.

I'm sorry (and I know this discussion has been going on for a long
time), but this still feels like too much of a hack.

You're registering a tty driver to allow user space to continue treat
this as a tty device, but you provide no means of actually modifying
anything (line settings, etc). It's essentially just a character device
with common tty ioctls as noops from a device PoV (well, plus the ldisc
buffering and processing).

This will probably require someone to first implement a generic gps
framework with a properly defined interface which you may then teach
gpsd to use (e.g. to avoid all its autodetection functionality) instead.

Or some entirely different approach, for example, where you manage
everything from user space.

I'd suggest reiterating the problem you're trying to solve and
enumerating the previously discussed potential solutions in order to
find a proper abstraction level for this (before getting lost in
implementation details).

That being said, I'm still pointing some bugs and issue below that you
can consider for future versions of this (and other drivers) below.

> Signed-off-by: H. Nikolaus Schaller 
> ---
>  drivers/misc/Kconfig|  10 +
>  drivers/misc/Makefile   |   1 +
>  drivers/misc/w2sg0004.c | 553 
> 
>  3 files changed, 564 insertions(+)
>  create mode 100644 drivers/misc/w2sg0004.c
> 
> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> index f1a5c2357b14..a3b11016ed2b 100644
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -508,4 +508,14 @@ source "drivers/misc/mic/Kconfig"
>  source "drivers/misc/genwqe/Kconfig"
>  source "drivers/misc/echo/Kconfig"
>  source "drivers/misc/cxl/Kconfig"
> +
> +config W2SG0004
> + tristate "W2SG00x4 on/off control"

Please provide a better summary for what this driver does.

> + depends on GPIOLIB && SERIAL_DEV_BUS
> + help
> +  Enable on/off control of W2SG00x4 GPS moduled connected

Some whitespace issue here.

> +   to some SoC UART to allow powering up/down if the /dev/ttyGPSn
> +   is opened/closed.
> +   It also provides a rfkill gps name to control the LNA power.
> +
>  endmenu
> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
> index 5ca5f64df478..d9d824b3d20a 100644
> --- a/drivers/misc/Makefile
> +++ b/drivers/misc/Makefile
> @@ -50,6 +50,7 @@ obj-$(CONFIG_SRAM_EXEC) += sram-exec.o
>  obj-y+= mic/
>  obj-$(CONFIG_GENWQE) += genwqe/
>  obj-$(CONFIG_ECHO)   += echo/
> +obj-$(CONFIG_W2SG0004)   += w2sg0004.o
>  obj-$(CONFIG_VEXPRESS_SYSCFG)+= vexpress-syscfg.o
>  obj-$(CONFIG_CXL_BASE)   += cxl/
>  obj-$(CONFIG_ASPEED_LPC_CTRL)+= aspeed-lpc-ctrl.o
> diff --git a/drivers/misc/w2sg0004.c b/drivers/misc/w2sg0004.c
> new file mode 100644
> index ..6bfd12eb8e02
> --- /dev/null
> +++ b/drivers/misc/w2sg0004.c
> @@ -0,0 +1,553 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Driver for power controlling the w2sg0004/w2sg0084 GPS receiver.
> + *
> + * Copyright (C) 2013 Neil Brown 
> + * Copyright (C) 2015-2017 H. Nikolaus Schaller ,
> + *   Golden Delicious Computers
> + *
> + * This receiver has an ON/OFF pin which must be toggled to
> + * turn the device 'on' of 'off'.  A high->low->high toggle

s/of/or/

> + * will switch the device on if it is off, and off if it is on.
> + *
> + * To enable receiving on/off requests we register with the
> + * UART power management notifications.

No, the UART (serial device) would be the grandparent of your serdev
device (for which you register PM callbacks).

> + *
> + * It is not possible to directly detect the state of the device.

Didn't the 0084 version have a pin for this?

> + * However when it is on it will send characters on a UART line
> + * regularly.
> + *
> + * To detect that the power state is out of sync (e.g. if GPS
> + * was enabled before a reboot), we register for UART data received
> + * notifications.
> + *
> + * In addition we register as a rfkill client so that we can
> + * control the LNA power.
> + *
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#inclu