[PATCH] GPIO: Add support for GPIO on CLPS711X-target platform

[Linus Walleij]

On Fri, Aug 24, 2012 at 5:45 AM, Alexander Shiyan <shc_work at mail.ru> wrote:
> On Fri, 24 Aug 2012 00:08:45 +0200
> Linus Walleij <linus.walleij at linaro.org> wrote:
>
> ...
>> > +static int gpio_clps711x_get(struct gpio_chip *chip, unsigned offset)
>> > +{
>> > +       int ret;
>> > +       struct clps711x_gpio *gpio = dev_get_drvdata(chip->dev);
>> > +
>> > +       mutex_lock(&gpio->lock);
>> > +       ret = clps_readb(gpio_clps711x_port_dataaddr(chip)) & (1 << offset);
>> > +       mutex_unlock(&gpio->lock);
>> > +
>> > +       return !!ret;
>> > +}
>>
>> Please get rid of this clps_readb() business. I can see it's just this:
>> #define clps_readb(off)         readb(CLPS711X_VIRT_BASE + (off))
>>
>> This makes stuff hard to understand, please pass the virtual base as
>> a memory resource to the platform device, store it in the state holder
>> and just use readb() in the driver from that offset.
>
> This will break the whole concept of this platform. All the CPU registers
> are in one particular area, and therefore access to the registers of all
> the drivers performed using the macro.
> Maybe just add a short comment?

It does not break if you pass the offset CLPS711X_VIRT_BASE as
a memory resource and reference from that, e.g.:

Add a base:

struct clps711x_gpio {
       struct gpio_chip       chip[CLPS711X_GPIO_PORTS];
       struct mutex            lock;
+     void __iomem         *base;
};

Fish out the base from the platform device (provided it's added to
the platform device) in .probe:

res = platform_get_resource(dev, IORESOURCE_MEM, 0);
if (!res) {
        ret = -ENOENT;
        goto out;
}
gpio->base = res->start;

The access registers like this:

foo = readb(gpio->base + gpio_clps711x_port_diraddr(chip));

It's no big change, just that the offset is included in every
access instead of using custom accessor functions which
are in an obscure <mach/*> header, and we want to get rid
of all such headers.

If you do this, you should no longer need to include
<mach/hardware.h> (or something more is wrong...)

BTW I cannot see where this patch adds the platform device
named "gpio-clps711x"? Are you intending to do this later?

Yours,
Linus Walleij