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

[Vaittinen, Matti]

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

> 
> Is this a max and below is the min?:
> 
> > +
> > +  rohm,hw-margin-min-ms:
> > +    minimum: 2
> > +    maximum: 220
> > +    description:
> > +      Watchdog on these ICs can be configured in a window mode
> > where the ping
> > +      must come within certain time-window. Eg. too quick pinging
> > will also
> > +      trigger timeout. Specify the minimum delay between pings if
> > you wish to
> > +      use the window mode. Note, the maximum delay is internally
> > configured as
> > +      a certain multiple of this value so maximum delay can be
> > only up to 15
> > +      times this value. For example for 73 ms short ping value the
> > maximum
> > +      timeout will be close to 1 sec.

Yes. I didn't find existing property for "minimum time to ping WDG"
from existing properties. And I think it is not so common to have such
"ping window" in watchdog HW that it would warrant generic minimum tmo.
Hence vendor specific property for minimum tmo.

Would you still prefer me to introduce a new vendor specific property
for "max tmo"?

Best Regards
	Matti Vaittinen