Re: [PATCH] mfd: cpcap: Add minimal support

[Tony Lindgren <tony@atomide.com>]

* Lee Jones <lee.jones@linaro.org> [161124 00:56]:
> On Wed, 23 Nov 2016, Tony Lindgren wrote:
> 
> > * Lee Jones <lee.jones@linaro.org> [161121 03:43]:
> > > On Fri, 18 Nov 2016, Tony Lindgren wrote:
> > > > --- a/drivers/mfd/Makefile
> > > > +++ b/drivers/mfd/Makefile
> > > > @@ -97,6 +97,7 @@ obj-$(CONFIG_MFD_MC13XXX_I2C)	+= mc13xxx-i2c.o
> > > >  obj-$(CONFIG_MFD_CORE)		+= mfd-core.o
> > > >  
> > > >  obj-$(CONFIG_EZX_PCAP)		+= ezx-pcap.o
> > > > +obj-$(CONFIG_MFD_CPCAP)		+= cpcap.o
> > > 
> > > Who is the manufacturer?
> > 
> > Hmm that I don't know. There seems to be both ST and TI versions
> > of this chip manufactured for Motorola. So my guess is that it
> > should be Motorola unless there's some similar catalog part
> > available from ST used by others. If anybody has more info
> > on this please let me know :)
> 
> If this IP is shared amongst vendors, it usually means it was designed
> by someone else?  Synopsis perhaps?

After searching around, many specs say "ST Ericsson CPCAP". So let's
assume the manufacturer should be ste.

> > > > +	cpcap->vendor = (val >> 6) & 0x0007;
> > > > +	cpcap->revision = ((val >> 3) & 0x0007) | ((val << 3) & 0x0038);
> > > 
> > > Lots of magic numbers here.  I suggest you define them.
> > 
> > I'll check if some earlier code has these defined. Otherwise I'll
> > just add a comment on the lack of available documentation.
> 
> *sad face*
> 
> Does that mean you don't even know what they're for?

Luckily the Motorola driver folks documented all the registers.
Unfortunately the register bits just have names. I need to
check if we have names for these bits.

> > > > +	error = cpcap_init_irq_bank(cpcap, 0, 0, 16);
> > > 
> > > 'ret' is more traditional.
> > 
> > FYI error seems to be preferred over ret as it's meaning is
> > clear, git grep "error =" drivers/input for example.
> > I can of course change it if you prefer ret over error.
> 
> I'd prefer to stick to the conventions of *this* subsystem.
> 
> ... and the most common convention used kernel wide:
> 
> $ git grep "ret =" | wc -l
> 117976
> $ git grep "err =" | wc -l
> 56708
> $ git grep "error =" | wc -l
> 14427

OK sure will rename.

> > > > +#define CPCAP_REG_LDEB		0x1270	/* LMR Debounce Settings */
> > > > +#define CPCAP_REG_LGDET		0x1274	/* LMR GCAI Detach Detect */
> > > > +#define CPCAP_REG_LMISC		0x1278	/* LMR Misc Bits */
> > > > +#define CPCAP_REG_LMACE		0x127c	/* LMR Mace IC Support */
> > > > +
> > > > +#define CPCAP_REG_TEST		0x7c00	/* Test */
> > > > +
> > > > +#define CPCAP_REG_ST_TEST1	0x7d08	/* ST Test1 */
> > > > +
> > > > +#define CPCAP_REG_ST_TEST2	0x7d18	/* ST Test2 */
> > > 
> > > It would be nice to line up the entire file. #OCD
> > 
> > Hmm care to clarify what you mean here? I think it's lined up with
> 
> I'm missing context now you've <snip>ed.
> 
> These look straight, however is the whole file lined up (as much as
> *practically* possible)?

Yeah it should be, I'll check.

> > tabs to line up. I left empty lines where the registers are not
> > contiguous. What does #OCD mean, Obsessive Compulsive Disorder over
> > header files maybe? :)
> 
> Yes, that's what it means.
> 
> /me likes straight lines. :)

Sure nothing wrong with that ;)

Regards,

Tony