[alsa-devel] [PATCH 2/2] ASoC: fsi: add master clock control functions

[Kuninori Morimoto]

Hi Mark

Thank you for checking this patch

> > +	own = clk_get(dev, NULL);
> > +	if (IS_ERR(own))
> > +		return -EINVAL;
> 
> Any reason not to use devm_clk_get()?

Ahh yes, Sorry.
BTW, currect code calls this function many times now,
(this clk is local variable now)
then should I use...

 1) use devm_clk_get() / devm_clk_put() pair
 2) use clk_get() / clk_put() pair
 3) keep these clocks under private date (struct fsi_prive)
    and use devm_clk_get() only _probe timing.
 
> > +	if (xck) {
> > +		*xck = clk_get(NULL, fsi_is_port_a(fsi) ? "fsiack" : "fsibck");
> > +		if (IS_ERR(*xck)) {
> > +			dev_err(dev, "can't get fsixck clock\n");
> > +			return -EINVAL;
> > +		}
> > +	}
> 
> This looks wrong - I would expect the driver to be requesting the clock
> with some fixed name and the struct device.  The platform should remap
> this onto the appropriate underlying clock using clkdev or something.

I see.
I will fix in v2.

> > +static int fsi_clk_set_rate_external(struct device *dev,
> > +				     struct fsi_priv *fsi,
> > +				     int enable)
> > +{
> > +	struct clk *xck, *ick;
> > +	int ret = 0;
> > +
> > +	ret = fsi_clk_gets(dev, fsi, &xck, &ick, NULL);
> > +	if (ret < 0) {
> > +		dev_err(dev, "clk gets failed\n");
> > +		return ret;
> > +	}
> > +
> > +	if (enable) {
> > +		clk_disable(ick);
> 
> Are we sure that calls to this function are always balanced so that
> calls to clk_enable() and clk_disable() are balanced?

I balanced it by local technic.
But I can say it is un-understandable.
Can I use counter ?

Best regards
---
Kuninori Morimoto