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

["Vaittinen, Matti" <Matti.Vaittinen@fi.rohmeurope.com>]

On Wed, 2020-09-23 at 08:27 -0600, Rob Herring wrote:
> On Sat, Sep 19, 2020 at 5:46 AM Vaittinen, Matti
> <Matti.Vaittinen@fi.rohmeurope.com> 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@fi.rohmeurope.com>
> > > > ---
> > > >  .../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@fi.rohmeurope.com>
> > > > +
> > > > +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>'.

Your suggestion looks good to me. If we introduce such then it would
make sense to add handling for this in GPIO watchdog too.

What I do wonder is how "hw_margin_ms" is unused? I see it is a
required property for GPIO watchdog. The GPIO WDG probe seems to
actually error out if reading this property fails with any error. I
would assume the GPIO WDG is used somewhere? Hence I am a bit afraid of
touching it. Breaking existing setups would not be nice.

Guenter - how do you see this? Should we leave GPIO WDG as it is,
convert it to use this new binding Rob suggested - or support both the
old and new (at least for some time) in the driver - and possibly print
a warning when old is used?

Best Regards
	Matti Vaittinen