Re: [PATCH v4 2/6] mfd: bd71837: Devicetree bindings for ROHM BD71837 PMIC

[Matti Vaittinen]

On Thu, May 31, 2018 at 09:07:24AM -0500, Rob Herring wrote:
> On Thu, May 31, 2018 at 5:23 AM, Matti Vaittinen
> <mazziesaccount@xxxxxxxxx> wrote:
> > On Thu, May 31, 2018 at 10:17:17AM +0300, Matti Vaittinen wrote:
> >> On Wed, May 30, 2018 at 10:01:29PM -0500, Rob Herring wrote:
> >> > On Wed, May 30, 2018 at 11:42:03AM +0300, Matti Vaittinen wrote:
> >> > > Document devicetree bindings for ROHM BD71837 PMIC MFD.
> >> > > + - interrupts            : The interrupt line the device is connected to.
> >> > > + - interrupt-controller  : Marks the device node as an interrupt controller.
> >> >
> >> > What sub blocks have interrupts?
> >>
> >> The PMIC can generate interrupts from events which cause it to reset.
> >> Eg, irq from watchdog line change, power button pushes, reset request
> >> via register interface etc. I don't know any generic handling for these
> >> interrupts. In "normal" use-case this PMIC is powering the processor
> >> where driver is running and I do not see reasonable handling because
> >> power-reset is going to follow the irq.
> >>
> >
> > Oh, but when reading this I understand that the interrupt-controller
> > property should at least be optional.
> 
> I don't think it should. The h/w either has an interrupt controller or
> it doesn't.

I hope this explains why I did this interrupt controller - please tell
me if this is legitimate use-case and what you think of following:

+Optional properties:
+ - interrupt-controller	: Marks the device node as an interrupt controller.
+			  BD71837MWV can report different power state change
+			  events to other devices. Different events can be seen
+			  as separate BD71837 domain interrupts.
+ - #interrupt-cells	: The number of cells to describe an IRQ should be 1.
+			    The first cell is the IRQ number.
+			    masks from ../interrupt-controller/interrupts.txt.

> My concern is you added it but nothing uses it which tells
> me your binding is incomplete. I'd rather see complete bindings even
> if you don't have drivers.

So this makes me wonder if my use-case for interrupt controller is
valid. I thought making this PMIC as interrupt controller is a nice way
of hiding the irq register and i2c access from other potential drivers
using these interrupts. But as I don't know what could be the potential
user for these irqs, I don't know how to complete binding. This is why I
also thought of making this optional, so that the potential for using
the interrupts would be there but it was not required when interrupts
are not needed.

> For example, as-is, there's not really any
> need for the clocks child node. You can just make the parent a clock
> provider.

This sounds correct. I just lack of knowledge on how to handle clocks
in "standard way" using the clock framework and this was a result of
my first attempt. (Funny, I have written clk / synchronization drivers
for work in the past but still I have no idea on how to do this in
"standard way").

> But we need a complete picture of the h/w to make that
> determination.

My attempt is to create generic driver for this PMIC. I would rather not
limit it's use to any particular board/soc. The example binding is based
on my test environment where I simply connected this PMIC break out
board to beagle bone black. (I do also have a test board where i.MX8 and
peripherials are powered by this PMIC but I rather not limit this driver
to such single setup. Besides the linux running on that board is not
'standard')

The PMIC itself just has this 32.768 KHz clock output. Clock can be
enabled/disabled via register interface. I think this is intended to be
used for RTC but I thought this driver does not need to care about that.
I thought it is just a good idea to provide control via clk subsystem
and to not make assumptions on use-cases in this driver.

Best Regards
    Matti Vaittinen