On Wed, 23 Nov 2016, Tony Lindgren wrote: > * Lee Jones <lee.jones@xxxxxxxxxx> [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? > > > + 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? > > > + 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 > > > + 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 I'm missing context now you've <snip>ed. These look straight, however is the whole file lined up (as much as *practically* possible)? > 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. :) > Anywys thanks for the review, the rest of the comments I will just > fix and repost. Welcome. -- Lee Jones Linaro STMicroelectronics Landing Team Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
