Re: [PATCH v1 1/6] dt_bindings: mfd: Add ROHM BD9576MUF and BD9573MUF PMICs

[Rob Herring]

On Sat, Sep 19, 2020 at 5:46 AM Vaittinen, Matti
<Matti.Vaittinen@xxxxxxxxxxxxxxxxx> wrote:
>
> Thanks Rob for taking a look at this!
>
> On Fri, 2020-09-18 at 11:28 -0600, Rob Herring wrote:
> > On Thu, Sep 17, 2020 at 11:01:52AM +0300, Matti Vaittinen wrote:
> > > Add bindings for ROHM BD9576MUF and BD9573MUF PMICs. These
> > > PMICs are primarily intended to be used to power the R-Car series
> > > processors. They provide 6 power outputs, safety features and a
> > > watchdog with two functional modes.
> > >
> > > Signed-off-by: Matti Vaittinen <matti.vaittinen@xxxxxxxxxxxxxxxxx>
> > > ---
> > >  .../bindings/mfd/rohm,bd9576-pmic.yaml        | 129
> > > ++++++++++++++++++
> > >  1 file changed, 129 insertions(+)
> > >  create mode 100644
> > > Documentation/devicetree/bindings/mfd/rohm,bd9576-pmic.yaml
> > >
> > > diff --git a/Documentation/devicetree/bindings/mfd/rohm,bd9576-
> > > pmic.yaml b/Documentation/devicetree/bindings/mfd/rohm,bd9576-
> > > pmic.yaml
> > > new file mode 100644
> > > index 000000000000..f17d4d621585
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/mfd/rohm,bd9576-pmic.yaml
> > > @@ -0,0 +1,129 @@
> > > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/mfd/rohm,bd9576-pmic.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: ROHM BD9576MUF and BD9573MUF Power Management Integrated
> > > Circuit bindings
> > > +
> > > +maintainers:
> > > +  - Matti Vaittinen <matti.vaittinen@xxxxxxxxxxxxxxxxx>
> > > +
> > > +description: |
> > > +  BD9576MUF and BD9573MUF are power management ICs primarily
> > > intended for
> > > +  powering the R-Car series processors.
> > > +  The IC provides 6 power outputs with configurable sequencing and
> > > safety
> > > +  monitoring. A watchdog logic with slow ping/windowed modes is
> > > also included.
> > > +
> > > +properties:
> > > +  compatible:
> > > +    enum:
> > > +      - rohm,bd9576
> > > +      - rohm,bd9573
> > > +
> > > +  reg:
> > > +    description:
> > > +      I2C slave address.
> > > +    maxItems: 1
> > > +
> > > +  interrupts:
> > > +    maxItems: 1
> > > +
> > > +  rohm,vout1-en-low:
> > > +    description:
> > > +      BD9576 and BD9573 VOUT1 regulator enable state can be
> > > individually
> > > +      controlled by a GPIO. This is dictated by state of vout1-en
> > > pin during
> > > +      the PMIC startup. If vout1-en is LOW during PMIC startup
> > > then the VOUT1
> > > +      enable sate is controlled via this pin. Set this property if
> > > vout1-en
> > > +      is wired to be down at PMIC start-up.
> > > +    type: boolean
> > > +
> > > +  rohm,vout1-en-gpios:
> > > +    description:
> > > +      GPIO specifier to specify the GPIO connected to vout1-en for
> > > vout1 ON/OFF
> > > +      state control.
> > > +    maxItems: 1
> > > +
> > > +  rohm,ddr-sel-low:
> > > +    description:
> > > +      The BD9576 and BD9573 output voltage for DDR can be selected
> > > by setting
> > > +      the ddr-sel pin low or high. Set this property if ddr-sel is
> > > grounded.
> > > +    type: boolean
> > > +
> > > +  rohm,watchdog-enable-gpios:
> > > +    description: The GPIO line used to enable the watchdog.
> > > +    maxItems: 1
> > > +
> > > +  rohm,watchdog-ping-gpios:
> > > +    description: The GPIO line used to ping the watchdog.
> > > +    maxItems: 1
> > > +
> > > +  hw_margin_ms:
> >
> > Needs a vendor prefix.
> >
> > s/_/-/
> >
> > > +    minimum: 4
> > > +    maximum: 4416
> > > +    description: Watchog timeout in milliseconds
> >
> > Maybe the words in the description should be in the property name as
> > I don't see how 'h/w margin' relates to 'watchdog timeout'.
>
> The hw_margin_ms is an existing property. As I wrote to Guenter:
> "hw_margin_ms" is an existing binding for specifying the maximum TMO in
> HW (if I understood it correctly). (It is used at least by the generig
> GPIO watchdog) I thought it's better to not invent a new vendor
> specific binding when we have a generic one.
>
> https://elixir.bootlin.com/linux/v5.9-rc2/source/Documentation/devicetree/bindings/watchdog/gpio-wdt.txt

That one is odd and I haven't found an actual user of it. It would
make more sense as a collection of properties devices could use rather
than a virtual device.

I think I'd do something like 'watchdog-ping-time-msec' that can be
either '<min> <max>' or '<max>'.

Rob