linux-kernel - Re: [PATCH] mfd: cpcap: Add minimal support

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: <20161123152652.GB4082@atomide.com>
Date:   Wed, 23 Nov 2016 07:26:52 -0800
From:   Tony Lindgren <tony@...mide.com>
To:     Lee Jones <lee.jones@...aro.org>
Cc:     Samuel Ortiz <sameo@...ux.intel.com>, linux-kernel@...r.kernel.org,
        linux-omap@...r.kernel.org, devicetree@...r.kernel.org,
        Marcel Partap <mpartap@....net>,
        Mark Rutland <mark.rutland@....com>,
        Michael Scott <michael.scott@...aro.org>,
        Rob Herring <robh+dt@...nel.org>
Subject: Re: [PATCH] mfd: cpcap: Add minimal support

* Lee Jones <lee.jones@...aro.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 :)

> > +	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.

> > +	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.

> > +	error = cpcap_init_irq_bank(cpcap, 2, 32, 64);
> > +	if (error)
> > +		return error;
> 
> I don't think I've seen this method of adding bulk IRQ chips before.
> Isn't there a cleaner or generic way to do this?

I'll check.

...
> > +#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
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? :)

Anywys thanks for the review, the rest of the comments I will just
fix and repost.

Regards,

Tony