Re: [PATCH v5] GPIO: add support for 74x164 serial-in/parallel-out8-bit shift register

[Miguel Ojeda]

On Wed, Oct 20, 2010 at 2:42 PM, Florian Fainelli <florian@xxxxxxxxxxx> wrote:
> On Wednesday 20 October 2010 10:17:32 Miguel Ojeda wrote:
>> On Wed, Oct 20, 2010 at 9:52 AM, Miguel Ojeda
>>
>> <miguel.ojeda.sandonis@xxxxxxxxx> wrote:
>> > On Wed, Oct 20, 2010 at 1:00 AM, Andrew Morton
>> >
>> > <akpm@xxxxxxxxxxxxxxxxxxxx> wrote:
>> >> On Tue, 19 Oct 2010 09:26:42 +0200
>> >>
>> >> Miguel Ojeda <miguel.ojeda.sandonis@xxxxxxxxx> wrote:
>> >>> On Mon, Oct 18, 2010 at 9:35 PM, Florian Fainelli <florian@xxxxxxxxxxx>
> wrote:
>> >>> > From: Miguel Gaio <miguel.gaio@xxxxxxxxx>
>> >>> >
>> >>> > This patch adds support for generic 74x164 serial-in/parallel-out
>> >>> > 8-bits shift register. This driver can be used as a GPIO output
>> >>> > expander.
>> >>>
>> >>> ...
>> >>>
>> >>> > +struct gen_74x164_chip {
>> >>> > +       struct spi_device       *spi;
>> >>> > +       struct gpio_chip        gpio_chip;
>> >>> > +       struct mutex            lock;
>> >>> > +       u8                      port_config;
>> >>> > +};
>> >>>
>> >>> ...
>> >>>
>> >>> > +static void gen_74x164_set_value(struct gpio_chip *gc,
>> >>> > +               unsigned offset, int val)
>> >>> > +{
>> >>> > +       struct gen_74x164_chip *chip = gpio_to_chip(gc);
>> >>> > +       bool refresh;
>> >>> > +
>> >>> > +       mutex_lock(&chip->lock);
>> >>> > +       if (val)
>> >>> > +               chip->port_config |= (1 << offset);
>> >>> > +       else
>> >>> > +               chip->port_config &= ~(1 << offset);
>> >>>
>> >>> set_bit(), clear_bit() ?
>> >>
>> >> They're only to be used on `unsigned long' types, and `port_config' is
>> >> u8.
>> >
>> > Right as always! Maybe BIT()? Don't we have a {SET,CLEAR}_BIT()-like
>> > macros somewhere?
>> >
>> > #define SET_BIT(var,nr) (var) |= BIT((nr))
>> > #define CLEAR_BIT(var,nr) (var) &= ~BIT((nr))
>> > #define PUT_BIT(var,nr,value) do { \
>> >        if ((value)) \
>> >                SET_BIT((var), (nr)); \
>> >        else \
>> >                CLEAR_BIT((var), (nr)); \
>> > } while(0)
>> >
>> > May I make a patch and try to see who could use it? I suppose a
>> > Coccinelle's semantic patch would be great here.
>>
>> Well, after trying a few minutes spatch for my first time it has
>> already found a lot of places where the macros could be applied. I
>> will prepare a patch.
>
> Though this is certainly valid, what's the benefit in using a macro to do that
> instead of open coding the toggle of a bit in a variable?

To avoid "code duplication" and simplify (it is harder to make a
mistake in the PUT_BIT() case I think) and maybe optimize [*]. See for
example this change from drivers/gpio/timbgpio.c:

-		bflr &= ~(1 << offset);
-		flr |= 1 << offset;
-		if (trigger & IRQ_TYPE_EDGE_FALLING)
-			lvr &= ~(1 << offset);
-		else
-			lvr |= 1 << offset;
+		CLEAR_BIT(bflr, offset);
+		SET_BIT(flr, offset);
+		PUT_BIT(lvr, offset, !(trigger & IRQ_TYPE_EDGE_FALLING));

I think it is clearer. Possibly it is only worth it in the PUT_BIT case.

[*] I know that gcc already optimizes this stuff, but maybe we could
call __set_bit()/__clear_bit()/__toggle_bit() from the macro if the
argument's type is UL. I am supposing here that __*_bit() functions
were coded for some reason.).

On the other hand, if *bit_() functions are out there for UL, I
suppose it is good to have a macro for the general case.

> --
> Florian
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/