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

[Stephen Boyd <sboyd@kernel.org>]

Quoting Charles Keepax (2018-10-15 03:49:05)
> On Fri, Oct 12, 2018 at 08:59:56AM -0700, Stephen Boyd wrote:
> > Quoting Charles Keepax (2018-10-11 06:26:02)
> > > On Thu, Oct 11, 2018 at 12:00:46AM -0700, Stephen Boyd wrote:
> > > > Quoting Charles Keepax (2018-10-08 06:25:40)
> > > > > +struct lochnagar_clk_priv {
> > > > > +       struct device *dev;
> > > > > +       struct lochnagar *lochnagar;
> > > > 
> > > > Is this used for anything besides getting the regmap? Can you get the
> > > > pointer to the parent in probe and use that to get the regmap pointer
> > > > from dev_get_remap() and also use the of_node of the parent to register
> > > > a clk provider? It would be nice to avoid including the mfd header file
> > > > unless it's providing something useful.
> > > > 
> > > 
> > > It is also used to find out which type of Lochnagar we have
> > > connected, which determines which clocks we should register. I
> > 
> > Can that be done through some device ID? So the driver can be untangled
> > from the MFD part.
> > 
> > > could perhaps pass that using another mechanism but we would
> > > still want to include the MFD stuff to get the register
> > > definitions. So this approach seems simplest.
> > 
> > Can the register definitions be moved to this clk driver?
> > 
> > Maybe you now get the hint, but I'd really like to be able to merge and
> > compile the clk driver all by itself without relying on the parent MFD
> > device to provide anything at compile time.
> > 
> 
> If you feel strongly but since the MFD needs to hold the regmap
> (which needs to define the read/volatile regs and defaults)
> these will need to be duplicate defines and personally i would
> rather only have one copy as it makes updating things much less
> error prone.

Ok if there's going to be read/volatile regs and defaults then it makes
sense to leave the defines in some shared header file, which is
unfortunate for the independent merge of driver bits. Either way, I
would prefer we don't use struct lochnagar in this driver and move to
more generic structures like struct regmap and express the type of MFD
to this device driver some other way.

> 
> > > > > +       if (lclk->regmap.dir_mask) {
> > > > > +               ret = regmap_update_bits(regmap, lclk->regmap.cfg_reg,
> > > > > +                                        lclk->regmap.dir_mask,
> > > > > +                                        lclk->regmap.dir_mask);
> > > > > +               if (ret < 0) {
> > > > > +                       dev_err(priv->dev, "Failed to set %s direction: %d\n",
> > > > 
> > > > What does direction mean?
> > > > 
> > > 
> > > Some of the clocks can both generate and receive a clock. For
> > > example the PSIA (external audio interface) MCLKs, the attached
> > > device could be expecting or providing a master audio clock. If
> > > the user assigns a parent to the clock we assume the attached
> > > device is providing a clock to us, otherwise we assume we are
> > > providing the clock.
> > 
> > And this directionality is determined by dir_mask? It would be great if
> > this sort of information was in the commit text or in a comment in the
> > driver so we know what's going on here.
> > 
> 
> No problem will make this more clear.
> 

Thanks!