Re: [PATCH v2 2/5] mfd: lochnagar: Add support for the Cirrus Logic Lochnagar

[Richard Fitzgerald]

On 01/11/18 10:28, Charles Keepax wrote:
> On Mon, Oct 29, 2018 at 11:04:39AM +0000, Lee Jones wrote:
> > On Fri, 26 Oct 2018, Mark Brown wrote:
> > > On Fri, Oct 26, 2018 at 09:00:51AM +0100, Lee Jones wrote:
> > > > On Thu, 25 Oct 2018, Richard Fitzgerald wrote:
>
> I have re-ordered some of the quotes here for the benefit
> of clarity. I will start with the Lochnagar focused bits
> and then cover some of the more general regmap discussion.
> Apologies for the wall of text towards the end but it seemed
> wise to explore some of the why for parts of the current regmap
> implementation and some of the implications for changing it.
>
> > > I think from the perspective of Richard and Charles who are just trying
> > > to get their driver merged this is something of an abstract distinction.
> > > If the driver were merged and this discussion were happening separately
> > > their perspective would most likely be different.
> >
> > Charles has already mentioned that he'd take a look at the current
> > *use* (not changing the API, but the way in which Lochnagar
> > *uses/consumes* it).  Actions which would be most welcomed.
> >
> > > I kind of said this above but just to be clear this driver seems to me
> > > like an idiomatic user of the regmap API as it is today.  My guess is
> > > that we could probably loose the defaults tables and not suffer too much
> > > but equally well they don't hurt from a regmap point of view.
> >
> > Perhaps Charles could elaborate on whether this is possible or not?
>
> So on this front I have had a look through and we can drop the
> defaults tables for Lochnagar, although as I shall cover later
> Lochnagar is the exceptional case with respect to our normal
> devices.
>
> > Utilising range support here would certainly help IMHO.
>
> I am rather hesitant to switch to the range API. Whilst it is
> significantly more clear in certain situations, such as say
> mapping out the memory for a DSP, where there exist large
> amorphis blobs of identical function registers. I am really
> not convinced it really suits something like the register map
> that controls Lochnagar. You have an intertwinned mix of
> various purpose registers, each with a clearly defined name
> and potentially with quite fine grained properties.
>
> Certainly when working with the driver I want to be able to
> fairly quickly see what registers are marked as by name. The
> callbacks are very simple and I don't need to look up where
> register are in the regmap to be able check their attributes.
> But perhaps I have just got too used to seeing these callbacks,
> things do have a way of becoming normal after being exposed to
> them for a while.
>
> What I will try for the next spin is to try to use as much:
>
>    case REG1...REG2:
>
> As I can without totally sacrificing grepability/clarity and
> hopefully that will get us to a compromise we can all live
> with at least for now. Lochnagar is a fairly small part so it
> feels like this should be doable.
>
> > > > > +       ret = devm_of_platform_populate(dev);
> > > > > +       if (ret < 0) {
> > > > > +               dev_err(dev, "Failed to populate child
> > > > > nodes:
> > > > > %d\n", ret);
> > > > > +               return ret;
> > > > > +       }
> > > >
> > > > Please do not mix OF and MFD device registration
> > > > strategies.
> > > >
> > > > Pick one and register all devices through your chosen
> > > > method.
> > >
> > > Hmmm we use this to do things like register some fixed
> > > regulators
> > > and clocks that don't need any control but do need to be
> > > associated
> > > with the device. I could do that through the MFD although it
> > > is in
> > > direct conflict with the feedback on the clock patches I
> > > received
> > > to move the fixed clocks into devicetree rather than
> > > registering
> > > them manually (see v2 of the patch chain).
> >
> > The I suggest moving everything to DT.
>
> So pulling this out from earlier discussions in this thread,
> it seems I can happily move all the child device registration
> into device tree. I will also try this for the next version of
> the patch, unless anyone wants to object? But it does change
> the DT binding quite a lot as the individual sub drivers now
> each require their own node rather than one single unified
> Lochnagar node.

We went through this discussion with the Madera MFD patches. I had
originally implemented it using DT to register the child drivers and
it was nice in some ways each driver having its own node. But Mark
and Rob didn't like it so I went back to non-DT child registration with
all sharing the parent MFD node. It would be nice if we could stick to
one way of doing it so that Cirrus drivers don't flip-flop between
different styles of DT binding.

With the Madera MFD, since it now uses non-DT registration of children
if there was a reason we need to be able to register DT-defined children
(and there are potential uses like adding platform-specific virtual
regulators that are treated as a child) the bindings are now fixed so we
would end up having a mixed non-DT and DT registration.

<snip>