Quoting Charles Keepax (2018-10-15 03:49:05)

On Fri, Oct 12, 2018 at 08:59:56AM -0700, Stephen Boyd wrote:

quoted

Quoting Charles Keepax (2018-10-11 06:26:02)

quoted

On Thu, Oct 11, 2018 at 12:00:46AM -0700, Stephen Boyd wrote:

quoted

Quoting Charles Keepax (2018-10-08 06:25:40)

quoted

+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.

quoted

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.

quoted

quoted

quoted

quoted

+       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!