linux-kernel - Re: [PATCH 3/3] TTY/slave: add driver for w2sg0004 GPS

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20141212160607.361d20db@notabene.brown>
Date:	Fri, 12 Dec 2014 16:06:07 +1100
From:	NeilBrown <neilb@...e.de>
To:	One Thousand Gnomes <gnomes@...rguk.ukuu.org.uk>
Cc:	Grant Likely <grant.likely@...aro.org>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Mark Rutland <mark.rutland@....com>,
	Jiri Slaby <jslaby@...e.cz>, NeilBrown <neil@...wn.name>,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/3] TTY/slave: add driver for w2sg0004 GPS

On Thu, 11 Dec 2014 23:11:00 +0000 One Thousand Gnomes
<gnomes@...rguk.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
> 


Content of type "application/pgp-signature" skipped