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
