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@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>'. 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
