Date: Mon, 9 Feb 2015 15:29:54 +0100
From: Wolfram Sang <wsa@the-dreams.de>
To: Russell King - ARM Linux <linux@arm.linux.org.uk>
Cc: linux-kernel@vger.kernel.org
Subject: Re: [PATCH] clk: return proper ERR_PTR for clk_get when !HAVE_CLK
Message-ID: <20150209142954.GA8024@katana>
References: <1423163365-15267-1-git-send-email-wsa@the-dreams.de>
 <20150205234040.GV8656@n2100.arm.linux.org.uk>
 <20150207164209.GA6263@katana>
 <20150207172949.GE8656@n2100.arm.linux.org.uk>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha1;
	protocol="application/pgp-signature"; boundary="3MwIy2ne0vdjdPXF"
Content-Disposition: inline
In-Reply-To: <20150207172949.GE8656@n2100.arm.linux.org.uk>
User-Agent: Mutt/1.5.23 (2014-03-12)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3220
Lines: 78


--3MwIy2ne0vdjdPXF
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

Russell,

thanks for the details again!

>  * Returns a struct clk corresponding to the clock producer, or
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>  * valid IS_ERR() condition containing errno.  The implementation
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Yes, I read that...

> The NULL value is basically undefined for drivers, and according to the
> definition above, drivers should treat it as "success".

... and this is what I stumbled over. For me, the NULL pointer meant
failure. But given your background information, especially about the
intentional and defined usage of the pointers returned by clk_get, I
get a better picture of the NULL return value.

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

Yes, but it is a bit more complicated...

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

... which has to do with this. Historically, "clock-frequency" in DT
means the i2c bus speed. This driver used the binding wrong to specify
the IP core clock speed instead and also used a fixed I2C bus speed
then. Now, when wanting to support a) flexible I2C bus speeds and b)
clock information from standard DT clock bindings, we also need to
ensure that we support the now deprecated and wrong binding as a
fallback. And even after having learnt something about the clk API now,
I still think the best fix is to replace all instances of 'if
(!IS_ERR(dev->clk))' with (!IS_ERR_OR_NULL()). Because in our case, NULL
is not success. We should skip any clk API interaction then and use the
fallback mechanisms.

   Wolfram


--3MwIy2ne0vdjdPXF
Content-Type: application/pgp-signature; name="signature.asc"
Content-Description: Digital signature

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQIcBAEBAgAGBQJU2MRhAAoJEBQN5MwUoCm2xqwQAJZ3UhSsUlRIUy54Bn7qJUMC
OeXAjfrPjZ7gqenTcVfGVNRvAoEeMaG450L9fgiRvDCXzeqh0uSlvMftxjffZPEZ
2/RHVSIgzc87KyVmWeCQt0yUk6yQbnlWx2EqRWdWDYm2o5PYHeKj/EXaSne1PFp1
SdTj9n+WG30ubrEkujngTFnd9s09q0rgOFGzRbx136xiNaeobsDBhP99MWCIBuaN
+kXK5PmRsfQ1LFgaB/nv0D8y8l8NvVTtGqIFJHEyzXGtOU7vf5gHSvMZKx7+zKkA
U6ocq4cgXJFb+GGkjaXyU5FDkFBH/fdeTboLJQsHCCU6kXfUIHN70kusGq5bdCWd
TyYTnxSZamDhwh6xu8SfJ061htZHhGtX8w/XVYQsXx7NnE2v+Qxd6ec7E7Kg4pWe
YN5ObSNDECYihRoh+0ONvjcX9QIu6yXOvTiOd220HC6Vmi+Ia1awdKV+Ro7Mdql7
zufjLT6kdNCVY1mmfM6Au7Af6STiknI/nPbFUuMHW1d4xZHKY0Bfp1Jp+wFM+zew
F/3rq9kic3QuVQR3HVOQk3jYwNbBz6ZXlNLmkLrK/QtkXlYMY1faTze52IeP832Y
IMblMK1O5E7OcoKtdU7KcgbiklWO5WvEK8XI0v4w1IIKHlbR84vTKS07Wltr6C6y
qyVd7miZj79LWc+DndPF
=jwao
-----END PGP SIGNATURE-----

--3MwIy2ne0vdjdPXF--
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/