Re: [PATCH] pxamci: remove an ifdef about CONFIG_REGULATOR

[Antonio Ospite <ospite@xxxxxxxxxxxxxxxxx>]

On Mon, 9 May 2011 22:36:12 +0200
Mark Brown <broonie@xxxxxxxxxxxxxxxxxxxxxxxxxxx> wrote:

> On Mon, May 09, 2011 at 09:23:25PM +0100, Russell King - ARM Linux wrote:
> 
> > NAK.  This has been proposed before, and rejected.  See the comments
> > against the original proposal:
> 
> > http://www.arm.linux.org.uk/developer/patches/viewpatch.php?id=6525/1
> 
> Hrm, this looks like the bodge we're doing for !REGULATOR isn't actually
> helping here unless we do a NULL || IS_ERR() in the error check.  The
> ifdefs are certainly fail.
> 

[Adding Linus Walleij to CC as he was the author of a similar NAKed
patch]

I was blindly trusting code already in mainline again, and for that I
apologize, I finally took the time to look at the implementation
of IS_ERR() and test its use, and being IS_ERR(NULL) true it is not what
we want indeed, see the attached test program.

So, I am going to remove the ifdefs anyway but use IS_ERR_OR_NULL();
how does that sound? Am I still missing anything?
Or changing the regulator_get() stub to return an error (-ENOSYS?) might
be worth it?

Thanks Russel for pointing out the issue BTW.

Thanks,
   Antonio

-- 
Antonio Ospite
http://ao2.it

PGP public key ID: 0x4553B001

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?

#include <stdio.h>

/* from /include/linux/compiler.h */
#define likely_notrace(x)       __builtin_expect(!!(x), 1)
#define unlikely_notrace(x)     __builtin_expect(!!(x), 0)

#define likely(x)   likely_notrace(x)
#define unlikely(x) unlikely_notrace(x)

/* from include/linux/compiler-gcc4.h */
#define __must_check            __attribute__((warn_unused_result))

/* From include/linux/err.h */
#define MAX_ERRNO       4095

#define IS_ERR_VALUE(x) unlikely((x) >= (unsigned long)-MAX_ERRNO)

static inline long __must_check IS_ERR(const void *ptr)
{

	return IS_ERR_VALUE((unsigned long)ptr);
}

static inline long __must_check IS_ERR_OR_NULL(const void *ptr)
{
	return !ptr || IS_ERR_VALUE((unsigned long)ptr);
}


/* Mimic regulator_get() when CONFIG_REGULATOR is defined */
unsigned long *regulator_get_full()
{
	unsigned long *ptr;

	return ptr;
}

/* Mimic regulator_get() when CONFIG_REGULATOR is NOT defined */
unsigned long *regulator_get_stub()
{
	return NULL; /* or maybe return -ENOSYS; ? */
}

int main(void)
{
	unsigned long *vcc = NULL;

	vcc = regulator_get_full();
	if (IS_ERR(vcc))
		printf("full: IS_ERR         = true\n");
	else
		printf("full: IS_ERR         = false\n");

	if (IS_ERR_OR_NULL(vcc))
		printf("full: IS_ERR_OR_NULL = true\n");
	else
		printf("full: IS_ERR_OR_NULL = false\n");


	vcc = regulator_get_stub();
	if (IS_ERR(vcc))
		printf("stub: IS_ERR         = true\n");
	else
		printf("stub: IS_ERR         = false\n");

	if (IS_ERR_OR_NULL(vcc))
		printf("stub: IS_ERR_OR_NULL = true\n");
	else
		printf("stub: IS_ERR_OR_NULL = false\n");

	return 0;
}

Attachment: pgpW2617h1Pp3.pgp
Description: PGP signature