Re: [PATCH RESEND 6/6] clk: s5p-g2d: Fix incorrect usage of IS_ERR_OR_NULL

Russell King - ARM Linux
2013-01-02T10:15:54Z

On Wed, Jan 02, 2013 at 10:44:32AM +0100, Julia Lawall wrote:
> On Wed, 2 Jan 2013, Russell King - ARM Linux wrote:
>
> > On Wed, Jan 02, 2013 at 08:10:36AM +0300, Dan Carpenter wrote:
> > > clk_get() returns NULL if CONFIG_HAVE_CLK is disabled.
> > >
> > > I told Tony about this but everyone has been gone with end of year
> > > holidays so it hasn't been addressed.
> > >
> > > Tony, please fix it so people don't apply these patches until
> > > clk_get() is updated to not return NULL. It sucks to have to revert
> > > patches.
> >
> > How about people stop using IS_ERR_OR_NULL for stuff which it shouldn't
> > be used for?
>
> Perhaps the cases where clk_get returns NULL could have a comment
> indicating that NULL does not represent a failure?

No. More documentation is never the answer to people not reading
existing documentation.

> In 3.7.1, it looks like it might have been possible for NULL to be
> returned by clk_get in arch/mips/loongson1/common/clock.c, but that
> definition seems to be gone in a recent linux-next. The remaining
> definitions look OK.

How about people just read the API and comply with it rather than
doing their own thing all the time? We've already had at least one
instance where someone has tried using IS_ERR() with the ioremap()
return value.

Really, if you're going to program kernel space, it is very important
that you *know* the interfaces that you're using and you comply with
them. Otherwise, you have no business saying that you're a kernel
programmer.

Yes, the odd mistake happens, but that's no excuse for the constant
blatent mistakes with stuff like IS_ERR_OR_NULL() with clk_get()
which just comes from total laziness on the part of the coder to
understand the interfaces being used. Hell, it's even documented
in linux/clk.h - that just shows how many people read the
documentation which has been around since the clk API came about.