Re: Inconsistency in clk framework
On Thu, Dec 20, 2012 at 05:13:37PM +1300, Tony Prisk wrote:
> On Wed, 2012-12-19 at 19:08 +, Russell King - ARM Linux wrote:
> > On Thu, Dec 20, 2012 at 08:00:49AM +1300, Tony Prisk wrote:
> > > On Thu, 2012-12-20 at 06:34 +1300, Tony Prisk wrote:
> > > > On Wed, 2012-12-19 at 09:26 +, Russell King - ARM Linux wrote:
> > > > > On Wed, Dec 19, 2012 at 05:10:33PM +1300, Tony Prisk wrote:
> > > > > > Hi Mike,
> > > > > >
> > > > > > In attempting to remove some IS_ERR_OR_NULL references, it was
> > > > > > pointed
> > > > > > out that clk_get() can return NULL if CONFIG_HAVE_CLK is not
> > > > > > defined.
> > > > >
> > > > > That is correct - but why is that a problem? As far as users are
> > > > > concerned, NULL is a valid clock. If HAVE_CLK is undefined, do you
> > > > > want all your drivers to suddenly stop working?
> > > >
> > > > That will be where the misunderstanding has occurred - I didn't consider
> > > > NULL to be a valid clock.
> > > >
> > > > Given that NULL is a valid clock, I guess all tests against get_clk and
> > > > of_get_clk results should be IS_ERR_OR_NULL. Correct?
> > > >
> > > For the sake of clarity, I should rephrase:
> > >
> > > If the driver can't operate with a NULL clk, it should use a
> > > IS_ERR_OR_NULL test to test for failure, rather than IS_ERR.
> >
> > Why should a _consumer_ of a clock care? It is _very_ important that
> > people get this idea - to a consumer, the struct clk is just an opaque
> > cookie. The fact that it appears to be a pointer does _not_ mean that
> > the driver can do any kind of dereferencing on that pointer - it should
> > never do so.
>
> As a simple example:
> We have a PWM module that requires a clock source to be enabled before
> registers can be read/written.
>
> *pseudo code*
> x = clk_get("pwm_clock")
> if IS_ERR(x) then fail
> err = clk_enable(x)
> if (err != 0) then fail
> start writing to module registers
>
>
> Assuming HAVE_CLK is undefined:
>
> x = clk_get("pwm_clock") (= NULL)
> if IS_ERR(x) then fail (not an error)
> err = clk_enable(x) (= 0)
> if (err) then fail (not an error)
> start writing to module registers
> (register writes lock the bus because the clock wasn't really enabled,
> but no errors occurred enabling the clock)
Which is really silly if you have a platform which requires clock control
and HAVE_CLK is not selected. The clk API is not designed to cope with
that situation.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: Inconsistency in clk framework
On Thu, Dec 20, 2012 at 05:13:37PM +1300, Tony Prisk wrote: On Wed, 2012-12-19 at 19:08 +, Russell King - ARM Linux wrote: On Thu, Dec 20, 2012 at 08:00:49AM +1300, Tony Prisk wrote: On Thu, 2012-12-20 at 06:34 +1300, Tony Prisk wrote: On Wed, 2012-12-19 at 09:26 +, Russell King - ARM Linux wrote: On Wed, Dec 19, 2012 at 05:10:33PM +1300, Tony Prisk wrote: Hi Mike, In attempting to remove some IS_ERR_OR_NULL references, it was pointed out that clk_get() can return NULL if CONFIG_HAVE_CLK is not defined. That is correct - but why is that a problem? As far as users are concerned, NULL is a valid clock. If HAVE_CLK is undefined, do you want all your drivers to suddenly stop working? That will be where the misunderstanding has occurred - I didn't consider NULL to be a valid clock. Given that NULL is a valid clock, I guess all tests against get_clk and of_get_clk results should be IS_ERR_OR_NULL. Correct? For the sake of clarity, I should rephrase: If the driver can't operate with a NULL clk, it should use a IS_ERR_OR_NULL test to test for failure, rather than IS_ERR. Why should a _consumer_ of a clock care? It is _very_ important that people get this idea - to a consumer, the struct clk is just an opaque cookie. The fact that it appears to be a pointer does _not_ mean that the driver can do any kind of dereferencing on that pointer - it should never do so. As a simple example: We have a PWM module that requires a clock source to be enabled before registers can be read/written. *pseudo code* x = clk_get(pwm_clock) if IS_ERR(x) then fail err = clk_enable(x) if (err != 0) then fail start writing to module registers Assuming HAVE_CLK is undefined: x = clk_get(pwm_clock) (= NULL) if IS_ERR(x) then fail (not an error) err = clk_enable(x) (= 0) if (err) then fail (not an error) start writing to module registers (register writes lock the bus because the clock wasn't really enabled, but no errors occurred enabling the clock) Which is really silly if you have a platform which requires clock control and HAVE_CLK is not selected. The clk API is not designed to cope with that situation. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Inconsistency in clk framework
On Wed, 2012-12-19 at 19:08 +, Russell King - ARM Linux wrote:
> On Thu, Dec 20, 2012 at 08:00:49AM +1300, Tony Prisk wrote:
> > On Thu, 2012-12-20 at 06:34 +1300, Tony Prisk wrote:
> > > On Wed, 2012-12-19 at 09:26 +, Russell King - ARM Linux wrote:
> > > > On Wed, Dec 19, 2012 at 05:10:33PM +1300, Tony Prisk wrote:
> > > > > Hi Mike,
> > > > >
> > > > > In attempting to remove some IS_ERR_OR_NULL references, it was pointed
> > > > > out that clk_get() can return NULL if CONFIG_HAVE_CLK is not defined.
> > > >
> > > > That is correct - but why is that a problem? As far as users are
> > > > concerned, NULL is a valid clock. If HAVE_CLK is undefined, do you
> > > > want all your drivers to suddenly stop working?
> > >
> > > That will be where the misunderstanding has occurred - I didn't consider
> > > NULL to be a valid clock.
> > >
> > > Given that NULL is a valid clock, I guess all tests against get_clk and
> > > of_get_clk results should be IS_ERR_OR_NULL. Correct?
> > >
> > For the sake of clarity, I should rephrase:
> >
> > If the driver can't operate with a NULL clk, it should use a
> > IS_ERR_OR_NULL test to test for failure, rather than IS_ERR.
>
> Why should a _consumer_ of a clock care? It is _very_ important that
> people get this idea - to a consumer, the struct clk is just an opaque
> cookie. The fact that it appears to be a pointer does _not_ mean that
> the driver can do any kind of dereferencing on that pointer - it should
> never do so.
As a simple example:
We have a PWM module that requires a clock source to be enabled before
registers can be read/written.
*pseudo code*
x = clk_get("pwm_clock")
if IS_ERR(x) then fail
err = clk_enable(x)
if (err != 0) then fail
start writing to module registers
Assuming HAVE_CLK is undefined:
x = clk_get("pwm_clock") (= NULL)
if IS_ERR(x) then fail (not an error)
err = clk_enable(x) (= 0)
if (err) then fail (not an error)
start writing to module registers
(register writes lock the bus because the clock wasn't really enabled,
but no errors occurred enabling the clock)
I apologise if it seems like I am not getting it, but I would like to
understand this properly to avoid further problems later.
Regards
Tony P
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: Inconsistency in clk framework
On Thu, Dec 20, 2012 at 08:00:49AM +1300, Tony Prisk wrote: > On Thu, 2012-12-20 at 06:34 +1300, Tony Prisk wrote: > > On Wed, 2012-12-19 at 09:26 +, Russell King - ARM Linux wrote: > > > On Wed, Dec 19, 2012 at 05:10:33PM +1300, Tony Prisk wrote: > > > > Hi Mike, > > > > > > > > In attempting to remove some IS_ERR_OR_NULL references, it was pointed > > > > out that clk_get() can return NULL if CONFIG_HAVE_CLK is not defined. > > > > > > That is correct - but why is that a problem? As far as users are > > > concerned, NULL is a valid clock. If HAVE_CLK is undefined, do you > > > want all your drivers to suddenly stop working? > > > > That will be where the misunderstanding has occurred - I didn't consider > > NULL to be a valid clock. > > > > Given that NULL is a valid clock, I guess all tests against get_clk and > > of_get_clk results should be IS_ERR_OR_NULL. Correct? > > > For the sake of clarity, I should rephrase: > > If the driver can't operate with a NULL clk, it should use a > IS_ERR_OR_NULL test to test for failure, rather than IS_ERR. Why should a _consumer_ of a clock care? It is _very_ important that people get this idea - to a consumer, the struct clk is just an opaque cookie. The fact that it appears to be a pointer does _not_ mean that the driver can do any kind of dereferencing on that pointer - it should never do so. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Inconsistency in clk framework
On Thu, 2012-12-20 at 06:34 +1300, Tony Prisk wrote: > On Wed, 2012-12-19 at 09:26 +, Russell King - ARM Linux wrote: > > On Wed, Dec 19, 2012 at 05:10:33PM +1300, Tony Prisk wrote: > > > Hi Mike, > > > > > > In attempting to remove some IS_ERR_OR_NULL references, it was pointed > > > out that clk_get() can return NULL if CONFIG_HAVE_CLK is not defined. > > > > That is correct - but why is that a problem? As far as users are > > concerned, NULL is a valid clock. If HAVE_CLK is undefined, do you > > want all your drivers to suddenly stop working? > > That will be where the misunderstanding has occurred - I didn't consider > NULL to be a valid clock. > > Given that NULL is a valid clock, I guess all tests against get_clk and > of_get_clk results should be IS_ERR_OR_NULL. Correct? > For the sake of clarity, I should rephrase: If the driver can't operate with a NULL clk, it should use a IS_ERR_OR_NULL test to test for failure, rather than IS_ERR. Regards Tony P -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Inconsistency in clk framework
On Wed, 2012-12-19 at 09:26 +, Russell King - ARM Linux wrote: > On Wed, Dec 19, 2012 at 05:10:33PM +1300, Tony Prisk wrote: > > Hi Mike, > > > > In attempting to remove some IS_ERR_OR_NULL references, it was pointed > > out that clk_get() can return NULL if CONFIG_HAVE_CLK is not defined. > > That is correct - but why is that a problem? As far as users are > concerned, NULL is a valid clock. If HAVE_CLK is undefined, do you > want all your drivers to suddenly stop working? That will be where the misunderstanding has occurred - I didn't consider NULL to be a valid clock. Given that NULL is a valid clock, I guess all tests against get_clk and of_get_clk results should be IS_ERR_OR_NULL. Correct? Regards Tony P -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Inconsistency in clk framework
On Wed, Dec 19, 2012 at 05:10:33PM +1300, Tony Prisk wrote: > Hi Mike, > > In attempting to remove some IS_ERR_OR_NULL references, it was pointed > out that clk_get() can return NULL if CONFIG_HAVE_CLK is not defined. That is correct - but why is that a problem? As far as users are concerned, NULL is a valid clock. If HAVE_CLK is undefined, do you want all your drivers to suddenly stop working? > This seems to contradict the kernel docs associated with the normal > clk_get (when HAVE_CLK is defined) which states: > > * Returns a struct clk corresponding to the clock producer, or > * valid IS_ERR() condition containing errno. > > Wouldn't a return code of ERR_PTR(-ENOENT) make more sense and be inline > with the empty of_ versions as well (which return -ENOENT when CONFIG_OF > is undefined). No. > Also, I noticed that clk_get_sys() doesn't appear to be defined in clk.h > when HAVE_CLK is undefined - is this correct? It was never promoted to an official API because its only platform code which should be using it; no device driver (which should have a struct device to pass into clk_get()) should ever use clk_get_sys(). -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Inconsistency in clk framework
On Wed, Dec 19, 2012 at 05:10:33PM +1300, Tony Prisk wrote: Hi Mike, In attempting to remove some IS_ERR_OR_NULL references, it was pointed out that clk_get() can return NULL if CONFIG_HAVE_CLK is not defined. That is correct - but why is that a problem? As far as users are concerned, NULL is a valid clock. If HAVE_CLK is undefined, do you want all your drivers to suddenly stop working? This seems to contradict the kernel docs associated with the normal clk_get (when HAVE_CLK is defined) which states: * Returns a struct clk corresponding to the clock producer, or * valid IS_ERR() condition containing errno. Wouldn't a return code of ERR_PTR(-ENOENT) make more sense and be inline with the empty of_ versions as well (which return -ENOENT when CONFIG_OF is undefined). No. Also, I noticed that clk_get_sys() doesn't appear to be defined in clk.h when HAVE_CLK is undefined - is this correct? It was never promoted to an official API because its only platform code which should be using it; no device driver (which should have a struct device to pass into clk_get()) should ever use clk_get_sys(). -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Inconsistency in clk framework
On Wed, 2012-12-19 at 09:26 +, Russell King - ARM Linux wrote: On Wed, Dec 19, 2012 at 05:10:33PM +1300, Tony Prisk wrote: Hi Mike, In attempting to remove some IS_ERR_OR_NULL references, it was pointed out that clk_get() can return NULL if CONFIG_HAVE_CLK is not defined. That is correct - but why is that a problem? As far as users are concerned, NULL is a valid clock. If HAVE_CLK is undefined, do you want all your drivers to suddenly stop working? That will be where the misunderstanding has occurred - I didn't consider NULL to be a valid clock. Given that NULL is a valid clock, I guess all tests against get_clk and of_get_clk results should be IS_ERR_OR_NULL. Correct? Regards Tony P -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Inconsistency in clk framework
On Thu, 2012-12-20 at 06:34 +1300, Tony Prisk wrote: On Wed, 2012-12-19 at 09:26 +, Russell King - ARM Linux wrote: On Wed, Dec 19, 2012 at 05:10:33PM +1300, Tony Prisk wrote: Hi Mike, In attempting to remove some IS_ERR_OR_NULL references, it was pointed out that clk_get() can return NULL if CONFIG_HAVE_CLK is not defined. That is correct - but why is that a problem? As far as users are concerned, NULL is a valid clock. If HAVE_CLK is undefined, do you want all your drivers to suddenly stop working? That will be where the misunderstanding has occurred - I didn't consider NULL to be a valid clock. Given that NULL is a valid clock, I guess all tests against get_clk and of_get_clk results should be IS_ERR_OR_NULL. Correct? For the sake of clarity, I should rephrase: If the driver can't operate with a NULL clk, it should use a IS_ERR_OR_NULL test to test for failure, rather than IS_ERR. Regards Tony P -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Inconsistency in clk framework
On Thu, Dec 20, 2012 at 08:00:49AM +1300, Tony Prisk wrote: On Thu, 2012-12-20 at 06:34 +1300, Tony Prisk wrote: On Wed, 2012-12-19 at 09:26 +, Russell King - ARM Linux wrote: On Wed, Dec 19, 2012 at 05:10:33PM +1300, Tony Prisk wrote: Hi Mike, In attempting to remove some IS_ERR_OR_NULL references, it was pointed out that clk_get() can return NULL if CONFIG_HAVE_CLK is not defined. That is correct - but why is that a problem? As far as users are concerned, NULL is a valid clock. If HAVE_CLK is undefined, do you want all your drivers to suddenly stop working? That will be where the misunderstanding has occurred - I didn't consider NULL to be a valid clock. Given that NULL is a valid clock, I guess all tests against get_clk and of_get_clk results should be IS_ERR_OR_NULL. Correct? For the sake of clarity, I should rephrase: If the driver can't operate with a NULL clk, it should use a IS_ERR_OR_NULL test to test for failure, rather than IS_ERR. Why should a _consumer_ of a clock care? It is _very_ important that people get this idea - to a consumer, the struct clk is just an opaque cookie. The fact that it appears to be a pointer does _not_ mean that the driver can do any kind of dereferencing on that pointer - it should never do so. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Inconsistency in clk framework
On Wed, 2012-12-19 at 19:08 +, Russell King - ARM Linux wrote: On Thu, Dec 20, 2012 at 08:00:49AM +1300, Tony Prisk wrote: On Thu, 2012-12-20 at 06:34 +1300, Tony Prisk wrote: On Wed, 2012-12-19 at 09:26 +, Russell King - ARM Linux wrote: On Wed, Dec 19, 2012 at 05:10:33PM +1300, Tony Prisk wrote: Hi Mike, In attempting to remove some IS_ERR_OR_NULL references, it was pointed out that clk_get() can return NULL if CONFIG_HAVE_CLK is not defined. That is correct - but why is that a problem? As far as users are concerned, NULL is a valid clock. If HAVE_CLK is undefined, do you want all your drivers to suddenly stop working? That will be where the misunderstanding has occurred - I didn't consider NULL to be a valid clock. Given that NULL is a valid clock, I guess all tests against get_clk and of_get_clk results should be IS_ERR_OR_NULL. Correct? For the sake of clarity, I should rephrase: If the driver can't operate with a NULL clk, it should use a IS_ERR_OR_NULL test to test for failure, rather than IS_ERR. Why should a _consumer_ of a clock care? It is _very_ important that people get this idea - to a consumer, the struct clk is just an opaque cookie. The fact that it appears to be a pointer does _not_ mean that the driver can do any kind of dereferencing on that pointer - it should never do so. As a simple example: We have a PWM module that requires a clock source to be enabled before registers can be read/written. *pseudo code* x = clk_get(pwm_clock) if IS_ERR(x) then fail err = clk_enable(x) if (err != 0) then fail start writing to module registers Assuming HAVE_CLK is undefined: x = clk_get(pwm_clock) (= NULL) if IS_ERR(x) then fail (not an error) err = clk_enable(x) (= 0) if (err) then fail (not an error) start writing to module registers (register writes lock the bus because the clock wasn't really enabled, but no errors occurred enabling the clock) I apologise if it seems like I am not getting it, but I would like to understand this properly to avoid further problems later. Regards Tony P -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Inconsistency in clk framework
Hi Mike, In attempting to remove some IS_ERR_OR_NULL references, it was pointed out that clk_get() can return NULL if CONFIG_HAVE_CLK is not defined. This seems to contradict the kernel docs associated with the normal clk_get (when HAVE_CLK is defined) which states: * Returns a struct clk corresponding to the clock producer, or * valid IS_ERR() condition containing errno. Wouldn't a return code of ERR_PTR(-ENOENT) make more sense and be inline with the empty of_ versions as well (which return -ENOENT when CONFIG_OF is undefined). Also, I noticed that clk_get_sys() doesn't appear to be defined in clk.h when HAVE_CLK is undefined - is this correct? Regards Tony Prisk -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Inconsistency in clk framework
Hi Mike, In attempting to remove some IS_ERR_OR_NULL references, it was pointed out that clk_get() can return NULL if CONFIG_HAVE_CLK is not defined. This seems to contradict the kernel docs associated with the normal clk_get (when HAVE_CLK is defined) which states: * Returns a struct clk corresponding to the clock producer, or * valid IS_ERR() condition containing errno. Wouldn't a return code of ERR_PTR(-ENOENT) make more sense and be inline with the empty of_ versions as well (which return -ENOENT when CONFIG_OF is undefined). Also, I noticed that clk_get_sys() doesn't appear to be defined in clk.h when HAVE_CLK is undefined - is this correct? Regards Tony Prisk -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
