Re: [PATCH] clk: return proper ERR_PTR for clk_get when !HAVE_CLK

[Russell King - ARM Linux]

On Sat, Feb 07, 2015 at 05:42:09PM +0100, Wolfram Sang wrote:
> On Thu, Feb 05, 2015 at 11:40:40PM +0000, Russell King - ARM Linux wrote:
> > On Thu, Feb 05, 2015 at 08:09:22PM +0100, Wolfram Sang wrote:
> > > clk_get functions return an ERR_PTR and not NULL in the error case. Make
> > > that consistent for the dummy functions when HAVE_CLK is not enabled.
> > > Otherwise unexpected codepaths might be trying to use a NULL pointer.
> > 
> > NAK.
> > 
> > There are some drivers which rely on this behaviour (take a driver,
> > such as some PXA IP) which uses the clk API but is also reused on
> > x86 which doesn't.
> 
> Okay, thanks for the explanation. Let me recap, there are three cases
> that clk_get can return:
> 
> a) pointer to a clk
> b) ERR_PTR meaning something went wrong when trying to get a clk
> c) NULL meaning there is no clk to get
> 
> So, if this patch from i2c/for-next [1] wants to add optional clk
> support, it should fallback to the old handling not by checking for
> (!IS_ERR()) but for (!IS_ERR_OR_NULL()).
> 
> Correct?
> 
> [1] https://git.kernel.org/cgit/linux/kernel/git/wsa/linux.git/commit/?h=i2c/for-next&id=e961a094afe04c6c8ca1adac50c8d16513f31b93

Not really.

/**
 * clk_get - lookup and obtain a reference to a clock producer.
 * @dev: device for clock "consumer"
 * @id: clock consumer ID
 *
 * Returns a struct clk corresponding to the clock producer, or
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 * valid IS_ERR() condition containing errno.  The implementation
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

That description applies to users of clk_get() and the rest of the clk
API, and according to that definition:

	clk = clk_get();
	if (IS_ERR(clk)) {
		/*
		 * clk represents an error or a deferred probe
		 */
		error = PTR_ERR(clk);
	} else {
		/*
		 * the clk cookie is valid and can be passed into
		 * every other clk function.
		 */
	}

Now, as far as specific errors go, clk_get() (and devm_clk_get()) will
behave as follows with DT (I think - the inventers of this code really
need to get better documentation - and it's worth pointing out that
there's a lot of redundant error checking going on):

1. the specified clk is found
2. -EPROBE_DEFER error pointer if the clk is found in DT, but is not
   yet available
3. -ENOENT error pointer if the clk is not found

The NULL value is basically undefined for drivers, and according to the
definition above, drivers should treat it as "success".  It can be
passed into the rest of the clk API harmlessly, which may or may not
return an error (it's defined that the clk API shall be safe for any
value returned by clk_get() which is not an error-pointer.)

It was a concious decision to have the !CONFIG_HAVE_CLK cause the API
to return success when finding a clock - the reason for it is to allow
drivers (such as PXA) which require a struct clk to be re-used on x86
without requiring x86 to implement the clk API (which they objected to.)

The problem comes is that you seem to want something different to that.
This is for opencores, right?  It's in much the same position as
everything else - you don't want the driver to fail when we have
!CONFIG_HAVE_CLK, but you need the clock frequency internally.

I would suggest something like:

	i2c->clk = devm_clk_get(&pdev->dev, NULL);
	if (!IS_ERR(i2c->clk)) {
		int ret = clk_prepare_enable(i2c->clk);
		if (ret) {
			dev_err(&pdev->dev,
				"clk_prepare_enable failed: %d\n", ret);
			return ret;
		}
		i2c->ip_clock_khz = clk_get_rate(i2c->clk) / 1000;
	}

	if (i2c->ip_clock_khz == 0) {
		/*
		 * We don't have a valid clock rate from the clk API, try
		 * to get the clock frequency from DT.
		 */
		if (of_property_read_u32(np, "opencores,ip-clock-frequency",
					 &val)) {
			if (!clock_frequency_present) {
				dev_err(&pdev->dev,
					"Missing required parameter 'opencores,ip-clock-frequency'\n");
				/* presumably this is a probe failure */
			}
		}
	} else {
		if (clock_frequency_present)
			i2c->bus_clock_khz = clock_frequency / 1000;
	}

I'm not sure what you do with clock_frequency_present and bus_clock_khz
so that bit of the above is a guess (I'm not sure why you have it in the
if (!IS_ERR()) { } block.)

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/