On Tue, Apr 30, 2013 at 6:17 PM, Opensource [Anthony Olech]
<anthony.olech.opensou...@diasemi.com> wrote:
> [Me]
>> > +struct da9058_gpio {
>> > + struct da9058 *da9058;
>> > + struct platform_device *pdev;
>> > + struct gpio_chip gp;
>> > + struct mutex lock;
>> > + int irq_0;
>> > + int irq_1;
>>
>> Why are you keeping thes two numbers here? Use the irqdomain to contain irq
>> mappings,
>
> this module does not use the irqdomain directly
This does not compute.
> so it gets the numbers from
> the platform data, which I thought did not persist past the probe() function.
> Is the platform data permanently available? - if that is the case, I can of
> course
> re-get the values on demand from the platform data - please confirm or suggest
> an alternative method of extracting the irq number.
It is indeed usual practice to cache such things from the platform
data in the local state container, do not worry about that.
I am more after where the IRQs are tracked, in an irqdomain or
as some variables somewhere.
I'm OK with this now, I think...
>> > + if (offset > 1) {
>> > + dev_err(da9058->dev,
>> > + "Failed to set GPIO%d output=%d because illegal
>> > GPIO\n",
>> > + offset, value);
>> > + return;
>> > + }
>>
>> Here you have an error print, pls insert that on the get function too then.
>
> the error print was used here because there is no other way to report an
> error.
> On the get function a negative number can be returned, so therefore there is
> no need to report it via the error log. I can put more diagnostics back in,
> but
> is it really necessary to duplicate the error indication in some cases?
No not necessary, OK I get it...
>> 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?
>> 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.
Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
