linux-kernel - RE: [PATCH 1/3] mfd: add LP3943 MFD driver

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]

Date:	Mon, 22 Jul 2013 01:18:54 +0000
From:	"Kim, Milo" <Milo.Kim@...com>
To:	Lee Jones <lee.jones@...aro.org>
CC:	"sameo@...ux.intel.com" <sameo@...ux.intel.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-pwm@...r.kernel.org" <linux-pwm@...r.kernel.org>,
	"thierry.reding@...il.com" <thierry.reding@...il.com>,
	"grant.likely@...aro.org" <grant.likely@...aro.org>,
	"linus.walleij@...aro.org" <linus.walleij@...aro.org>,
	"broonie@...nel.org" <broonie@...nel.org>
Subject: RE: [PATCH 1/3] mfd: add LP3943 MFD driver

Hi Lee,
Thanks for your detailed review.

> > diff --git a/Documentation/devicetree/bindings/mfd/lp3943.txt
> b/Documentation/devicetree/bindings/mfd/lp3943.txt
> > new file mode 100644
> > index 0000000..4eb251d
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mfd/lp3943.txt
> > @@ -0,0 +1,23 @@
> > +Bindings for TI/National Semiconductor LP3943 Driver
> > +
> > +Required properties:
> > +  - compatible: "ti,lp3943"
> > +  - reg: 7bit I2C slave address. 0x60 ~ 0x67
> > +
> > +Optional properties:
> > +  - ti,pwm0, ti,pwm1: Output channel definition for PWM port 0 and 1
> > +                      0 = invalid, 1 = LED0, 2 = LED 1, ... 16 = LED15
>                                          No space here ^      ^ comma here
> 
> This is actually pretty confusing.

Thanks for catching this. my ugly formatting :(

> Any way you can put the invalid entry at the end so, 0 == LED0?
> 
> > +Datasheet
> > +  http://www.ti.com/lit/ds/snvs256b/snvs256b.pdf
> 
> Ah, I see.... So the above are pins, rather than arbitrary channel Nos.
> 
> Would it make sense to use pinctrl instead then?

I'm not familiar with the PINCTRL subsystem. I'll check it.

> > +int lp3943_read_byte(struct lp3943 *l, u8 reg, u8 *read)
> 
> Not sure I like 'l' as a variable name. The function is small enough
> to get away with it in this context, but it would probably be better
> if it were renamed to something more meaningful.

LP3943 consists of two devices - GPIO and PWM drivers.
So each private data was defined as 

struct lp3943 *l;			/* MFD device */
struct lp3943_gpio *lg;		/* GPIO driver */
struct lp3943_pwm *lp;		/* PWM driver */

As you pointed, the 'l' may look like a list of something.
I'll rename them as below.

struct lp3943 *lp3943;
struct lp3943_gpio *lp3943_gpio;
struct lp3943_pwm *lp3943_pwm;

> > +static const struct i2c_device_id lp3943_ids[] = {
> > +	{"lp3943", 0},
> 
> Lack of consistency ...

I don't know exactly what it means. Do you mean the name of I2C device?

Regards,
Milo