lkml.org 
[lkml]   [2014]   [Dec]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 3/3] TTY/slave: add driver for w2sg0004 GPS
On Thu, 11 Dec 2014 23:11:00 +0000 One Thousand Gnomes
<gnomes@lxorguk.ukuu.org.uk> wrote:

> > +w2sg0004 UART-attached GPS receiver
> > +
>
> I'm wondering why it's tied to the w2sg0004
>
>
> > +struct w2sg_data {
> > + int gpio;
> > + int irq; /* irq line from RX pin when pinctrl
> > + * set to 'idle' */
> > + struct regulator *reg;
> > +
> > + unsigned long last_toggle; /* jiffies when last toggle completed. */
> > + unsigned long backoff; /* jiffies since last_toggle when
> > + * we try again
> > + */
> > + enum {Idle, Down, Up} state; /* state-machine state. */
> > + bool requested, is_on;
> > + bool suspended;
> > + bool reg_enabled;
> > +
> > + struct delayed_work work;
> > + spinlock_t lock;
> > + struct device *dev;
> > +
> > + struct rfkill *rfkill;
>
> So its
> - a regulator (optional)
> - an irq (optional)
> - a gpio (could be optional)
> - an optional rfkill
> - a pulse time (10ms fixed)
> - a backoff time (1 second fixed)
>
>
> It looks identical to half a dozen other widgets that are found in
> Android phones. Would it perhaps be better to make the tiny tweaks to
> make it generic
>
> - make the timers configurable
> - make the pulse time or high/low selectable for on/off
> - make the gpio optional
>
> and just have one driver with the right DT for all similar devices?
>
> Am I missing some w2sg004 specific bits here ?

There is particular behaviour that the device is both turned on and turned
off by toggling a GPIO, and the only way to detect which state it is in is
to watch the RX uart line (by reconfiguring it as a GPIO).

I'm sure that could describe other devices, but I don't personally know of
any.

I want to avoid premature generalisation. When we have another device it
would certainly make sense to extend this driver to support the new device.
Values like the timeouts could be tied to the particular 'compatible' value.

I guess the one drive could support both of my devices, as the simpler one
just needs a regulator to be enabled/disabled, and this driver can do that.

But then we would need a name for this driver. "generic.c" ???

>
>
> I think the general model is right, and there will be other slaves that
> don't fit the pattern but I do think this one could be generalised.

Thanks,
NeilBrown

>
> Alan
>

[unhandled content-type:application/pgp-signature]
\
 
 \ /
  Last update: 2014-12-12 06:41    [W:2.526 / U:0.460 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site