Re: [PATCH 2/2] ASoC: fsi: add master clock control functions

[Mark Brown <broonie@opensource.wolfsonmicro.com>]

[-- Attachment #1.1: Type: text/plain, Size: 1060 bytes --]

On Tue, Oct 30, 2012 at 07:59:45PM -0700, Kuninori Morimoto wrote:

> +	own = clk_get(dev, NULL);
> +	if (IS_ERR(own))
> +		return -EINVAL;

Any reason not to use devm_clk_get()?

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

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

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]