On Thu, 18 Dec 2014, Arnd Bergmann wrote:
> On Thursday 18 December 2014 09:04:04 Lee Jones wrote:
> > We
> > On Thu, 18 Dec 2014, Arnd Bergmann wrote:
> >
> > > On Thursday 18 December 2014 08:13:34 Lee Jones wrote:
> > > > On Wed, 17 Dec 2014, Arnd Bergmann wrote:
> > > >
> > > > > On Wednesday 17 December 2014 16:45:24 Lee Jones wrote:
> > > > > > +- compatible : Must be one of: "st,stih407-lpc" "st,stih416-lpc"
> > > > > > + "st,stih415-lpc" "st,stid127-lpc"
> > > > > > +- reg : LPC registers base address + size
> > > > > > +- interrupts : LPC interrupt line number and associated flags
> > > > > > +- clocks : Clock used by LPC device (See: ../clock/clock-bindings.txt)
> > > > > > +- st,lpc-mode : The LPC can run either one of two modes ST_LPC_MODE_RTC [0] or
> > > > > > + ST_LPC_MODE_WDT [1]. One (and only one) mode must be
> > > > > > + selected.
> > > > > >
> > > > >
> > > > > I'm glad you got it to work with two drivers for the same device.
> > > > >
> > > > > With this binding, I'm still a bit unhappy about the st,lpc-mode property,
> > > > > in particular since you rely on a shared include file for something that
> > > > > can only be set in one way or another and always has to be present.
> > > > >
> > > > > Why not just use a boolean property that enforces one mode when present
> > > > > and another mode when absent?
> > > >
> > > > There is nothing stopping me from doing that, and it was a
> > > > consideration. I concluded that this method would be more explicit
> > > > however. Both when describing our choices in DT and at a functional
> > > > level within each of the drivers.
> > > >
> > > > Let me know if you fundamentally disagree and I can fix-up.
> > >
> > > I generally don't like header files that define interfaces between C code
> > > and DT nodes. There are cases where it's the least ugly solution, but I don't
> > > think this is one of them.
> > >
> > > If you want to be more explicit about the modes, how about having one
> > > boolean property per mode? That would also allow devices that could be
> > > driven in either mode, e.g. if you have only one instance of this device.
> >
> > Isn't this was you suggested above?
>
> My first suggestion was to just have one boolean property, and use one
> driver if that is absent. The second one was to have two (or three) separate
> boolean properties that each refer to whether a particular driver is allowed
> to use this device or not.
>
> > But as I briefly mentioned to you elsewhere, there are actually 3
> > devices (Watchdog, RTC and Global Timer). How would you like to
> > handle that with a Boolean property when we introduce this new driver?
>
> Right, this would require having more than one property, but I still think
> it's better than the header file.
I'll put my point across just once and then become subservient once
more. I don't agree that defining 3 properties is better than
creating just 1. We have lots of properties containing indexes and
flags. Just because we've decided to #define them in order to read
them easily shouldn't detract from the fact that it's a better setup.
st,lpc-mode <1|2|3>;
Must be better than:
st,lpc-globaltimer-mode;
st,lpc-watchdog-mode;
st,lpc-rtc-mode;
If each of the drivers only checks for it's own property and fails to
probe if it's not present how will we detect and warn about a lack of
any of the 3 properties without a central, all-knowing (MFD) driver?
This is likely to cause someone [why isn't my driver probing] issues
and subsequently waste valuable engineering time in the future.
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
