lkml.org 
[lkml]   [2013]   [Jul]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRE: [PATCH 1/3] mfd: add LP3943 MFD driver
Date
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
\
 
 \ /
  Last update: 2013-07-22 04:21    [W:0.452 / U:1.712 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site