| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <45a44e480812282012m6ebcc648h9eaaa6a6973fd27c@mail.gmail.com>
Date: Sun, 28 Dec 2008 23:12:04 -0500
From: "Jaya Kumar" <jayakumar.lkml@...il.com>
To: "Eric Miao" <eric.y.miao@...il.com>
Cc: "David Brownell" <dbrownell@...rs.sourceforge.net>,
"Eric Miao" <eric.miao@...vell.com>,
"Paulius Zaleckas" <paulius.zaleckas@...tonika.lt>,
"Geert Uytterhoeven" <geert@...ux-m68k.org>,
"David Brownell" <david-b@...bell.net>,
"Sam Ravnborg" <sam@...nborg.org>,
"Haavard Skinnemoen" <hskinnemoen@...el.com>,
"Philipp Zabel" <philipp.zabel@...il.com>,
"Russell King" <rmk@....linux.org.uk>,
"Ben Gardner" <bgardner@...tec.com>, "Greg KH" <greg@...ah.com>,
linux-arm-kernel@...ts.arm.linux.org.uk,
linux-fbdev-devel@...ts.sourceforge.net,
linux-kernel@...r.kernel.org
Subject: Re: [RFC 2.6.27 1/1] gpiolib: add batch set/get
On Sun, Dec 28, 2008 at 10:38 PM, Eric Miao <eric.y.miao@...il.com> wrote:
> On Sun, Dec 28, 2008 at 12:24 AM, Jaya Kumar <jayakumar.lkml@...il.com> wrote:
>> +#ifdef CONFIG_GPIOLIB_BATCH
>> + gpio_set_batch(DB0_GPIO_PIN, data, 0xFFFF, 16);
>> +#else
>> for (i = 0; i <= (DB15_GPIO_PIN - DB0_GPIO_PIN) ; i++)
>> gpio_set_value(DB0_GPIO_PIN + i, (data >> i) & 0x01);
>> +#endif
>
> Well, if AM300 selects GPIOLIB_BATCH, I don't think we need the
> gpio_set_value() stuffs, and get rid of this #ifdef completely.
Good point. Will do.
>> + /* shift the bits into our register specific position */
>> + values <<= offset;
>> + bitmask <<= offset;
>> +
>> + values &= bitmask;
>
> or a single 'values = (values & bitmask) << offset' ?
Yup, good point. Will do.
>
>> + if (values)
>> + __raw_writel(values, pxa->regbase + GPSR_OFFSET);
>> +
>> + values = ~values;
>> + values &= bitmask;
>
> ditto
Will do.
>
>> + if (values)
>> + __raw_writel(values, pxa->regbase + GPCR_OFFSET);
>> +}
>> +
>> +/*
>> + * Get output GPIO level in batches
>> + */
>> +static u32 pxa_gpio_get_batch(struct gpio_chip *chip, unsigned offset,
>> + u32 bitmask)
>> +{
>> + u32 values;
>> + struct pxa_gpio_chip *pxa;
>> +
>> + /* we're guaranteed by the caller that offset + bitmask remains
>> + * in this chip.
>> + */
>> + pxa = container_of(chip, struct pxa_gpio_chip, chip);
>> +
>> + values = __raw_readl(pxa->regbase + GPLR_OFFSET);
>> +
>> + /* shift the result back into original position */
>> + values >>= offset;
>> + /* no need to shift bitmask since we've already shifted values */
>> + values &= bitmask;
>> +
>> + return values;
>
> or a single 'return (values >> offset) & bitmask;' should be enough.
Agreed.
>
>> +}
>> +#endif
>> +
>> +#ifdef CONFIG_GPIOLIB_BATCH
>> +#define GPIO_CHIP(_n) \
>> + [_n] = { \
>> + .regbase = GPIO##_n##_BASE, \
>> + .chip = { \
>> + .label = "gpio-" #_n, \
>> + .direction_input = pxa_gpio_direction_input, \
>> + .direction_output = pxa_gpio_direction_output, \
>> + .get = pxa_gpio_get, \
>> + .set = pxa_gpio_set, \
>> + .base = (_n) * 32, \
>> + .ngpio = 32, \
>> + .set_batch = pxa_gpio_set_batch, \
>> + .get_batch = pxa_gpio_get_batch, \
>
> This is a bit ugly, define pxa_gpio_set_batch to NULL #ifndef GPIOLIB_BATCH
> in the above code, and force .{set,get}_batch assignment anyway, this will
> look a bit better, the same way as PM. However, this requires a modification
> to gpio_chip to always allow these two pointers, which might be a concern.
I think I tried that but then encountered the problem that I can't put
ifdefs within the define GPIO_CHIP macro. Will try to find a different
way. How about if I do:
#ifdef GPIOLIB_BATCH
#define SET_BATCH_MACRO .set_batch = pxa_gpio_set_batch \
#else
#define SET_BATCH_MACRO
#endif
then leave SET_BATCH_MACRO in the GPIO_CHIP macro. I think that would work.
>> + if (!chip->set_batch) {
>> + while (((gpio + i) < (chip->base + chip->ngpio))
>> + && bitwidth) {
>> + mask = 1 << i;
>> + value = values & mask;
>> + if (bitmask & mask)
>> + chip->set(chip, gpio + i - chip->base,
>> + value);
>> + i++;
>> + bitwidth--;
>
> I recommend this being put into something like 'default_gpio_set_batch', and
> assign this to 'chip->set_batch' when the gpio chip is being registered and
> found 'chip->set_batch == NULL', so to keep this block consistent.
>
> Same comment to the 'get_batch' implementation below.
Ok, that should also make the code nicer, will do.
Thanks,
jaya
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists