Mainlining of Pyra nub joystick driver

Mainlining of Pyra nub joystick driver

[Andrey Utkin]

There's a pair of "nub" devices on Pyra handheld PC
(https://pyra-handheld.com/), and there's driver for nub, which is going
to be reworked for upstreaming. While the device itself fits most to
"joystick" category, the computer itself lacks touchpad and mouse
buttons, and the existing driver is capable of switching between modes, in
which it shows up like one of the following:
 - scrolling wheel,
 - mouse buttons set,
 - pointer updating its absolute position (graphic pad alike AFAIU),
 - pointer device behaving like actual joystick / pointing stick.

Currently modes switching happens through r/w file in /proc, which is of
couse going to be changed.

I wonder if such mode switching mechanism is tolerable for inclusion to
upstream kernel in this case. I'd like some advice how to rearrange the
driver to save most of flexibility while matching upstream kernel
conventions. I am especially interested in comments from subsystem
maintainers.

Existing driver:
http://git.goldelico.com/?p=gta04-kernel.git;a=blob;f=drivers/input/misc/as5013.c;h=1bfb5243f692c0c0a9c93881968849cac947c92d;hb=refs/heads/work/hns/input/as5013

Re: Mainlining of Pyra nub joystick driver

[Arnd Bergmann]

On Tuesday, June 14, 2016 2:45:23 PM CEST Andrey Utkin wrote:
> There's a pair of "nub" devices on Pyra handheld PC
> (https://pyra-handheld.com/), and there's driver for nub, which is going
> to be reworked for upstreaming. While the device itself fits most to
> "joystick" category, the computer itself lacks touchpad and mouse
> buttons, and the existing driver is capable of switching between modes, in
> which it shows up like one of the following:
>  - scrolling wheel,
>  - mouse buttons set,
>  - pointer updating its absolute position (graphic pad alike AFAIU),
>  - pointer device behaving like actual joystick / pointing stick.
> 
> Currently modes switching happens through r/w file in /proc, which is of
> couse going to be changed.
> 
> I wonder if such mode switching mechanism is tolerable for inclusion to
> upstream kernel in this case. I'd like some advice how to rearrange the
> driver to save most of flexibility while matching upstream kernel
> conventions. I am especially interested in comments from subsystem
> maintainers.
> 

My best idea for this is that the device driver should just report one
type of device, with the other modes being handled on top of that,
by a generic kernel framework or in user space.

Code for registering the same inputs to all those frameworks doesn't
seem to belong into a driver.

	Arnd

Re: [Kernel] Mainlining of Pyra nub joystick driver

[Andrey Utkin]

On Tue, Jun 14, 2016 at 02:45:23PM +0300, Andrey Utkin wrote:
> There's a pair of "nub" devices on Pyra handheld PC
> ...

Update: found drivers/input/joystick/as5011.c in mainline kernel,
will look how it works with as5013 hardware.

Re: [Kernel] Mainlining of Pyra nub joystick driver

[H. Nikolaus Schaller]

> Am 14.06.2016 um 19:02 schrieb Andrey Utkin <andrey_utkin@fastmail.com>:
> 
> On Tue, Jun 14, 2016 at 02:45:23PM +0300, Andrey Utkin wrote:
>> There's a pair of "nub" devices on Pyra handheld PC
>> ...
> 
> Update: found drivers/input/joystick/as5011.c in mainline kernel,
> will look how it works with as5013 hardware.

Before you spend too much time on it, they have not much in
common except the manufacturer and the as501x designation.

Register models (data sheet) are very different.

BR,
Nikolaus

Re: [Kernel] Mainlining of Pyra nub joystick driver

[Andrey Utkin]

On Tue, Jun 14, 2016 at 07:09:50PM +0200, H. Nikolaus Schaller wrote:
> 
> > Am 14.06.2016 um 19:02 schrieb Andrey Utkin <andrey_utkin@fastmail.com>:
> > Update: found drivers/input/joystick/as5011.c in mainline kernel,
> > will look how it works with as5013 hardware.
> 
> Before you spend too much time on it, they have not much in
> common except the manufacturer and the as501x designation.
> 
> Register models (data sheet) are very different.

Thanks for this hint, Nikolaus!

But still, apart from register models I guess as5011 driver is a good
starting point for a fork. I am considering this because
 - as5011 driver has support for button push event, and as5013 doesn't;
 - as5011 driver conforms to mainline standards much better than
   existing out-of-tree as5013 driver. It also conforms to what
   reviewers expect to see: anyway as5013 won't pass review unless all
   tricky modes are dropped. This is according to Arnd reply; Pyra dev
   team member aTc also votes for userspace input management daemon.

I am considering forking instead of adding conditional logic to as5011
because I don't have a chance to test as5011 hardware, so I could break
it unintentionally and never know about that. This is serious because
I'm going to add support of all power modes and other parameters
eventually.

There is one thing which both as5011 and as5013 existing drivers do:
expose a header file in include/linux/input/, with some "struct
blah_blah_platform_data" declaration. This header file is not included
anywhere except for corresponding driver's .c file. So I think this
header file is not needed, and all its contents should be inlined into
single driver's source file. From asking on #kernelnewbies IRC chat, I
gained some confidence that there's no subtleties regarding this.

I guess "struct drivername_platform_data" is something deprecated by
now, as long as there are devicetree files and of_...() API, so I should
port vsense_dt_init() logic from existing as5013 driver?

Also info about GPIO pin for button push interrupt should be added to
devicetree files. Or please indicate that it's not going to have its
GPIO pin for button interrupt - then driver should check button status
at once with periodic position check.

Re: [Kernel] Mainlining of Pyra nub joystick driver

[H. Nikolaus Schaller]

Hi Andrey,

> Am 16.06.2016 um 21:51 schrieb Andrey Utkin <andrey_utkin@fastmail.com>:
> 
> On Tue, Jun 14, 2016 at 07:09:50PM +0200, H. Nikolaus Schaller wrote:
>> 
>>> Am 14.06.2016 um 19:02 schrieb Andrey Utkin <andrey_utkin@fastmail.com>:
>>> Update: found drivers/input/joystick/as5011.c in mainline kernel,
>>> will look how it works with as5013 hardware.
>> 
>> Before you spend too much time on it, they have not much in
>> common except the manufacturer and the as501x designation.
>> 
>> Register models (data sheet) are very different.
> 
> Thanks for this hint, Nikolaus!
> 
> But still, apart from register models I guess as5011 driver is a good
> starting point for a fork. I am considering this because
> - as5011 driver has support for button push event, and as5013 doesn't;

> - as5011 driver conforms to mainline standards much better than
>   existing out-of-tree as5013 driver. It also conforms to what
>   reviewers expect to see: anyway as5013 won't pass review unless all
>   tricky modes are dropped. This is according to Arnd reply; Pyra dev
>   team member aTc also votes for userspace input management daemon.

Yes, that are strong arguments. We should also look for notaz' comments
since he wrote the as5013 driver and might have some ideas what is
important from user side to have reliable operation.

> 
> I am considering forking instead of adding conditional logic to as5011
> because I don't have a chance to test as5011 hardware, so I could break
> it unintentionally and never know about that. This is serious because
> I'm going to add support of all power modes and other parameters
> eventually.

One thing which makes me wonder is that the as5011 driver appears to
be used nowhere and did only have 2 patches since introduced in 2011:

https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/log/?id=refs%2Ftags%2Fv4.7-rc3&qt=grep&q=as5011

I.e. it might be unused and untested for a long time while our as5013
driver works.

This could also mean the as5011 driver can potentially be removed
from kernel and a new as5013 driver can start from scratch.

Anyways, you will be able to test a new driver on real hardware soon.
So mix&merge is probably the best strategy.

> 
> There is one thing which both as5011 and as5013 existing drivers do:
> expose a header file in include/linux/input/, with some "struct
> blah_blah_platform_data" declaration. This header file is not included
> anywhere except for corresponding driver's .c file. So I think this
> header file is not needed, and all its contents should be inlined into
> single driver's source file. From asking on #kernelnewbies IRC chat, I
> gained some confidence that there's no subtleties regarding this.
> 
> I guess "struct drivername_platform_data" is something deprecated by
> now, as long as there are devicetree files and of_...() API, so I should
> port vsense_dt_init() logic from existing as5013 driver?

I recently got similar feedback when submitting another driver, that
new drivers should be DT only and no longer need platform_data at all.
So there is no need for an include file to expose pdata to board files.

> 
> Also info about GPIO pin for button push interrupt should be added to
> devicetree files. Or please indicate that it's not going to have its
> GPIO pin for button interrupt - then driver should check button status
> at once with periodic position check.

The gpio can also interrupt but we might also want to do some debouncing.

This is already solved by the gpio-button driver. I.e. we would more
or less duplicate all code from there.

Logically it looks better to have the push button integrated in the driver and
grouped with it in DT. From DT perspective it could suffice to make the
gpio-button node a subnode of the as5013 node and keep separate
drivers? Just an idea.

BR,
Nikolaus

Re: [Kernel] Mainlining of Pyra nub joystick driver

[H. Nikolaus Schaller]

Hi Andrey,

> Am 16.06.2016 um 21:51 schrieb Andrey Utkin <andrey_utkin@fastmail.com>:
> 
> On Tue, Jun 14, 2016 at 07:09:50PM +0200, H. Nikolaus Schaller wrote:
>> 
>>> Am 14.06.2016 um 19:02 schrieb Andrey Utkin <andrey_utkin@fastmail.com>:
>>> Update: found drivers/input/joystick/as5011.c in mainline kernel,
>>> will look how it works with as5013 hardware.
>> 
>> Before you spend too much time on it, they have not much in
>> common except the manufacturer and the as501x designation.
>> 
>> Register models (data sheet) are very different.
> 
> Thanks for this hint, Nikolaus!
> 
> But still, apart from register models I guess as5011 driver is a good
> starting point for a fork. I am considering this because
> - as5011 driver has support for button push event, and as5013 doesn't;

> - as5011 driver conforms to mainline standards much better than
>   existing out-of-tree as5013 driver. It also conforms to what
>   reviewers expect to see: anyway as5013 won't pass review unless all
>   tricky modes are dropped. This is according to Arnd reply; Pyra dev
>   team member aTc also votes for userspace input management daemon.

Yes, that are strong arguments. We should also look for notaz' comments
since he wrote the as5013 driver and might have some ideas what is
important from user side to have reliable operation.

> 
> I am considering forking instead of adding conditional logic to as5011
> because I don't have a chance to test as5011 hardware, so I could break
> it unintentionally and never know about that. This is serious because
> I'm going to add support of all power modes and other parameters
> eventually.

One thing which makes me wonder is that the as5011 driver appears to
be used nowhere and did only have 2 patches since introduced in 2011:

https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/log/?id=refs%2Ftags%2Fv4.7-rc3&qt=grep&q=as5011

I.e. it might be unused and untested for a long time while our as5013
driver works.

This could also mean the as5011 driver can potentially be removed
from kernel and a new as5013 driver can start from scratch.

Anyways, you will be able to test a new driver on real hardware soon.
So mix&merge is probably the best strategy.

> 
> There is one thing which both as5011 and as5013 existing drivers do:
> expose a header file in include/linux/input/, with some "struct
> blah_blah_platform_data" declaration. This header file is not included
> anywhere except for corresponding driver's .c file. So I think this
> header file is not needed, and all its contents should be inlined into
> single driver's source file. From asking on #kernelnewbies IRC chat, I
> gained some confidence that there's no subtleties regarding this.
> 
> I guess "struct drivername_platform_data" is something deprecated by
> now, as long as there are devicetree files and of_...() API, so I should
> port vsense_dt_init() logic from existing as5013 driver?

I recently got similar feedback when submitting another driver, that
new drivers should be DT only and no longer need platform_data at all.
So there is no need for an include file to expose pdata to board files.

> 
> Also info about GPIO pin for button push interrupt should be added to
> devicetree files. Or please indicate that it's not going to have its
> GPIO pin for button interrupt - then driver should check button status
> at once with periodic position check.

The gpio can also interrupt but we might also want to do some debouncing.

This is already solved by the gpio-button driver. I.e. we would more
or less duplicate all code from there.

Logically it looks better to have the push button integrated in the driver and
grouped with it in DT. From DT perspective it could suffice to make the
gpio-button node a subnode of the as5013 node and keep separate
drivers? Just an idea.

BR,
Nikolaus