Re: [PATCH 1/4] mfd: 88pm800: add device tree support

[Vaibhav Hiremath]

On Tuesday 16 June 2015 06:32 PM, Rob Herring wrote:
> On Tue, Jun 16, 2015 at 2:52 AM, Vaibhav Hiremath
> <vaibhav.hiremath@xxxxxxxxxx> wrote:
> > On Monday 01 June 2015 02:08 PM, Lee Jones wrote:
> > > On Sat, 30 May 2015, Vaibhav Hiremath wrote:
> >
> > Thanks for your review. and sorry for delayed response.
> >
> > > > Add DT support to the 88pm800 driver along with below properties
> > > >          - marvell,88pm800-irq-write-clear :
> > > >            Idicates whether interrupt status is cleared by write
> > > >
> > > > Also, creates the DT binding text file,
> > > > Documentation/devicetree/bindings/mfd/88pm800.txt
> > > >
> > > > Signed-off-by: Chao Xie <chao.xie@xxxxxxxxxxx>
> > > > Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@xxxxxxxxxx>
> > > > ---
> > > >    Documentation/devicetree/bindings/mfd/88pm800.txt | 57
> > > > +++++++++++++++++++++++
> > > >    drivers/mfd/88pm800.c                             | 39 ++++++++++++++++
> > >
> > >
> > > These should be submitted separately.
> >
> >
> > Ok, will separate it in next version.
> >
> >
> > > >    2 files changed, 96 insertions(+)
> > > >    create mode 100644 Documentation/devicetree/bindings/mfd/88pm800.txt
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/mfd/88pm800.txt
> > > > b/Documentation/devicetree/bindings/mfd/88pm800.txt
> > > > new file mode 100644
> > > > index 0000000..094951b
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/mfd/88pm800.txt
> > > > @@ -0,0 +1,57 @@
> > > > +* Marvell 88PM800 Power Management IC
>
> Might as well name this 88PM8xx from the start.

Good point.
Will include it in next version.

I am ready with V2, but luckily I did not sent next version, as I
wanted to wait for Lee's response. :)

> > > > +
> > > > +Required parent device properties:
> > > > +- compatible : "marvell,88pm800"
>
> You might as well include 88pm805 and 88pm860 here since 805 support
> is in the kernel and you will be submitting 860 support. They don't
> have to be added with driver support.

Ok.

> > > > +- reg : the I2C slave address for the 88pm800 chip
> > > > +- interrupts : IRQ line for the 88pm800 chip
> > > > +- interrupt-controller: describes the 88pm800 as an interrupt controller
> > > > +- #interrupt-cells : should be 1.
> > > > +               - The cell is the 88pm800 local IRQ number
> > > > +
> > > > +Optional parent device properties:
> > > > +- marvell,88pm800-irq-write-clr: indicates whether interrupt status is
> > > > cleared by write
> > >
> > >
> > > Drop the device name.  These bindings should be as generic as possible.
> >
> > OK, how about simply
> >
> > "mfd-irq_clr_on_write"
>
> No, mfd is a Linux name which doesn't belong in bindings, and
> underscores are generally not used.

Oops.

underscore is just typo. did not meant it.

> I think Lee meant marvell,irq-write-clr or irq-write-clr.

will pick "marvell,irq-write-clr"


> This could also just be dropped altogether and set based on the
> compatible strings with match data.

It is configurable option, can be changed. so we still need get an
input.

> > > Also describe what the absence of the property means.
> >
> > Ok.
> >
> > > > +88pm800 consists of a large and varied group of sub-devices:
> > >
> > >
> > > 3?
> >
> > I have explicitly mentioned in note that more device list will follow.
> > I just wanted to add entries as and when we add/enable driver support.
> >
> > > > +Device                  Supply Names    Description
> > > > +------                  ------------    -----------
> > > > +88pm80x-onkey          :               : On key
> > > > +88pm80x-rtc            :               : RTC
> > > > +88pm80x                        :               : Regulators
> > >
> > >
> > > Surely regulators is 88pm80x-regulator, no?
> >
> > did not understand what change is expected here.
>
> The node name for regulators node should be 88pm80x-regulator? But
> these don't correspond to node names based on the example and the
> example is correct IMO. So these are Linux driver names? If so, they
> don't belong in the binding doc. What you need is to document the
> sub-node compatible strings

Hmmm,
Got it.

rtc and onkey are compatible strings only, I made mistake on regulator.
I will change to "88pm80x-regulator".


Thanks,
Vaibhav



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/