RE: [NEW DRIVER V6 5/7] drivers/gpio: DA9058 GPIO driver

[Opensource [Anthony Olech]]

> -----Original Message-----
> From: Linus Walleij [mailto:linus.walleij@xxxxxxxxxx]
> Sent: 03 May 2013 12:59
> To: Opensource [Anthony Olech]
> Cc: Grant Likely; Linus Walleij; Mark Brown; LKML; Lee Jones
> Subject: Re: [NEW DRIVER V6 5/7] drivers/gpio: DA9058 GPIO driver
> 
> On Tue, Apr 30, 2013 at 6:17 PM, Opensource [Anthony Olech]
> <anthony.olech.opensource@xxxxxxxxxxx> wrote:
[...]
> >> Maybe it's more logical to have if (!offset) and handle pin
> >> 0 first, then pin 1 in the else clause?
> >
> > why is it logical for one state to be preferred over the other?
> 
> Because 0 comes before 1 in the system of numbers?

OK, good enough reason - I will change for next submission.
 
> >> Please use #define to define these magic bits.
> >> Something like this:
> >>
> >> #include <linux/bitops.h>
> >>
> >> #define DA9058_GPIO0_VAL  BIT(3)
> >> #define DA9058_GPIO0_INOUT  BIT(1)
> >> #define DA9058_GPIO1_VAL  BIT(7)
> >> #define DA9058_GPIO1_INOUT  BIT(5)
> >>
> >> (etc, so we see what all bits are for)
> >>
> >> then use these defines in the code...
> >
> > the meaning of the bits other than the INP/OUT bits are not
> > independent, but depend on the direction. Thus each nibble contains 3
> dependent bits.
> 
> (...)
> >> > +       if (offset) {
> >> > +               u8 debounce_bits = debounce ? 0x80 : 0x00;
> >>
> >> Is this really correct??
> >>
> >> In earlier code you use bit 7 to set the value!
> >
> > You are confusing input and output operations, the meanings of each
> > nibble's other 3 bits is depends on the direction. That is why the INP
> > and OUT configuration needs to be saved in a control structure.
> >
> >> This is partly why I ask you to use #defines for the bits:
> >> less risk to do things wrong by e.g. copy/paste bugs.
> >
> > The hardware definition is in terms of bit patterns in each nibble, so
> > introducing a dual name for 3 of the 4 bits means double the number of
> > points an error can be made.
> 
> (...)
> >> > +       gpio->inp_config = 0x99;
> >> > +       gpio->out_config = 0x77;
> >>
> >> Again here you should #define the bits and | or them together instead
> >> of using comments to clarify it.
> >
> > I think that in this particular case it will be more confusing trying
> > to name the bits due to the fact that 3 out of 4 bit in each nibble depend on
> the 4th bit.
> 
> (...)
> > I failed previously to imagine any naming scheme that would make the
> > meaning of the magic bits in each nibble clearer. The best I could
> > come up with was the comment you referred to.
> 
> So add #defines for all combinations, IN and OUT directions.
> 
> Also write a comment explaining that the meaning of some bits change
> depending on how other bits are set.
> 
> #include <linux/bitops.h>
> 
> 
> /* This bit in each nybble detemines if the pin is input or output */ #define
> DA9058_GPIO0_IN  0 #define DA9058_GPIO1_IN  0 #define
> DA9058_GPIO0_OUT  BIT(1) #define DA9058_GPIO1_OUT  BIT(5)
> /* This is the meaning of bits 0, 2, 3 in the input mode */ #define
> DA9058_GPIO0_IN_PU  BIT(0) #define DA9058_GPIO0_IN_PU  BIT(4) #define
> DA9058_GPIO0_IN_DEB  BIT(3) #define DA9058_GPIO1_IN_DEB  BIT(7) (what
> is bit 2 in input mode?)
> (...)
> /* This is the meaning of bits 0, 2, 3 in the output mode */ #define
> DA9058_GPIO0_OUT_VAL  BIT(3) #define DA9058_GPIO0_OUT_PP_EXT  BIT(2)
> | BIT(0) #define DA9058_GPIO0_OUT_PP_INT  BIT(0) #define
> DA9058_GPIO0_OUT_OD  0 #define DA9058_GPIO1_OUT_VAL  BIT(7) #define
> DA9058_GPIO1_OUT_PP_EXT  BIT(6) | BIT(4) #define
> DA9058_GPIO1_OUT_PP_INT  BIT(4) #define DA9058_GPIO1_OUT_OD  0
> (...)
> 
> Then the init in probe() becomes readable:
> 
> gpio->inp_config = DA9058_GPIO0_IN |
>      DA9058_GPIO0_IN_PU |
>      DA9058_GPIO0_IN_DEB |
>      DA9058_GPIO1_IN |
>      DA9058_GPIO0_IN_PU |
>      DA9058_GPIO0_IN_DEB;
> gpio->out_config = DA9058_GPIO0_OUT |
>      DA9058_GPIO0_OUT_PP_EXT |
>      DA9058_GPIO1_OUT |
>      DA9058_GPIO1_OUT_PP_EXT;
> 
> This is way more readable than:
> 
> gpio->inp_config = 0x99;
> gpio->out_config = 0x77;
> 
> After this though, I start to wonder if it's not smarter to just
> have:
> 
> struct da9058_gpio {
>     (...)
>     u8 gpio0_in_config:4;
>     u8 gpio0_out_config:4;
>     u8 gpio1_in_config:4;
>     u8 gpio1_out_config:4;
> };
> 
> Then only define one set of bits for a single nybble and use some <<4 to |
> together the apropriate config at runtime.

Thanks for this comment - bit fields look like the best way of managing the bits. I will change the source for the next submission attempt.

Many thanks for taking the time to review this driver.

Tony Olech
Dialog Semiconductor
--
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/