Re: [PATCH v2 2/4] gpio: add viperboard gpio driver

[Lars Poeschel]

On Monday 15 October 2012 at 15:00:12, Linus Walleij wrote:

> > +#define VPRBRD_GPIOA_CLK_1             0           /* (1us   = 1MHz)  
> > */ +#define VPRBRD_GPIOA_CLK_10            1           /* (10us  =
> > 100kHz) */ +#define VPRBRD_GPIOA_CLK_100           2           /* (100us
> > = 10kHz)  */ +#define VPRBRD_GPIOA_CLK_1000          3           /* (1ms
> >   = 1kHz)   */ +#define VPRBRD_GPIOA_CLK_10000         4           /*
> > (10ms  = 100Hz)  */ +#define VPRBRD_GPIOA_CLK_100000                5   
> >        /* (100ms = 10Hz)   */
> 
> So instead of #defining something noone understands and
> then writing in the comment what it actually means, why
> don't you just:
> 
> #define VPRBRD_GPIOA_CLK_1MHZ           0
> #define VPRBRD_GPIOA_CLK_100KHZ          1
> 
> or maybe:
> 
> #define VPRBRD_GPIOA_CLK_PERIOD_1US             0
> #define VPRBRD_GPIOA_CLK_PERIOD_10US            1
> 
> or something else you will understand immediately when reading the
> code?

Yes, you are right. I was too involved with my hardware to see this. I will 
change this.
 
> > +struct __packed vprbrd_gpioa_msg {
> 
> __packed always goes *after* the struct does it not?
> 
> > +       u8 cmd;
> > +       u8 clk;
> > +       u8 offset;
> > +       u8 t1;
> > +       u8 t2;
> > +       u8 invert;
> > +       u8 pwmlevel;
> > +       u8 outval;
> > +       u8 risefall;
> > +       u8 answer;
> > +       u8 __fill;
> > +};  <- i.e. here, before the semicolon.

GCC does allow both alternatives. See description of packed attribute under:

http://gcc.gnu.org/onlinedocs/gcc/Type-Attributes.html#Type-Attributes

But since most kernel code does it right before the semicolon, I will change 
that too.

> > +/* gpioa sampling clock module parameter */
> > +static unsigned char gpioa_clk = 3;
> 
> Isn't this actually
> 
> static unsigned char gpioa_clk = VPRBRD_GPIOA_CLK_1000
> 
> > +module_param(gpioa_clk, byte, 0);
> > +MODULE_PARM_DESC(gpioa_clk, "gpio a sampling clk (default is 3 for 1
> > kHz)");
> 
> So if you're adding very magic module parameters maybe
> this magic number isn't such a good idea. Oh well, there
> are stranger things in the world so OK...

Also I will change this to be more descriptive.

> > +static int vprbrd_gpioa_get(struct gpio_chip *chip,
> > +               unsigned offset)
> > +{
> > +       int ret, answer, error = 0;
> > +       struct vprbrd_gpio *gpio =
> > +                       container_of(chip, struct vprbrd_gpio, gpioa);
> > +       struct vprbrd *vb = gpio->vb;
> > +       struct vprbrd_gpioa_msg *gamsg = (struct vprbrd_gpioa_msg
> > *)vb->buf; +
> > +       /* if io is set to output, just return the saved value */
> > +       if (gpio->gpioa_out & (1 << offset))
> > +               return gpio->gpioa_val & (1 << offset);
> 
> That's not going to work if the hardware changes state
> behind the back of the driver right? Oh well, maybe
> it doesn't matter.

I thought about that and then did this "cache" only in case the gpio is a 
output to save to usb ping-pong that is needed otherwise. I thought that 
nothing can change to state of the output but the driver itself.

> The rest does some clever USB marshalling that I trust
> is doing what it should :-)
> 
> > +       ret = usb_control_msg(vb->usb_dev, usb_sndctrlpipe(vb->usb_dev,
> > 0), +               0xed, 0x40, 0x0000, 0x0000, gamsg,
> > +               sizeof(struct vprbrd_gpioa_msg), 100);
> 
> 0xed? 0x40? 100?
> 
> Can you #define the magic constants, or are they already available
> in some existing header file?
> 
> (The zeros are OK.)

No there are no constants in some existing file. I will introduce them.

> > +       if (ret != sizeof(struct vprbrd_gpioa_msg))
> > +               error = -EREMOTEIO;
> > +
> > +       ret = usb_control_msg(vb->usb_dev, usb_rcvctrlpipe(vb->usb_dev,
> > 0), +               0xed, 0xc0, 0x0000, 0x0000, gamsg,
> > +               sizeof(struct vprbrd_gpioa_msg), 100);
> 
> Dito...
> 
> Same comment for *set, *direction_input, *direction_output,
> *setdir,
> 
> (...)
> 
> > +static int __devinit vprbrd_gpio_probe(struct platform_device *pdev)
> > +{
> > +       struct vprbrd *vb = dev_get_drvdata(pdev->dev.parent);
> > +       struct vprbrd_gpio *vb_gpio;
> > +       int ret;
> > +
> > +       vb_gpio = kzalloc(sizeof(*vb_gpio), GFP_KERNEL);
> 
> Can you use devm_kzalloc(&pdev->dev, ...)?

Ofcourse. Thanks for the hint. I did not knew this function.
 
> > +       if (vb_gpio == NULL)
> > +               return -ENOMEM;
> > +
> > +       vb_gpio->vb = vb;
> > +       /* registering gpio a */
> > +       vb_gpio->gpioa.label = "viperboard gpio a";
> > +       vb_gpio->gpioa.dev = &pdev->dev;
> > +       vb_gpio->gpioa.owner = THIS_MODULE;
> > +       vb_gpio->gpioa.base = -1;
> > +       vb_gpio->gpioa.ngpio = 16;
> > +       vb_gpio->gpioa.can_sleep = 1;
> > +       vb_gpio->gpioa.set = vprbrd_gpioa_set;
> > +       vb_gpio->gpioa.get = vprbrd_gpioa_get;
> > +       vb_gpio->gpioa.direction_input = vprbrd_gpioa_direction_input;
> > +       vb_gpio->gpioa.direction_output = vprbrd_gpioa_direction_output;
> > +       ret = gpiochip_add(&vb_gpio->gpioa);
> > +       if (ret < 0) {
> > +               dev_err(vb_gpio->gpioa.dev, "could not add gpio a");
> > +               goto err_gpioa;
> > +       }
> > +
> > +       /* registering gpio b */
> > +       vb_gpio->gpiob.label = "viperboard gpio b";
> > +       vb_gpio->gpiob.dev = &pdev->dev;
> > +       vb_gpio->gpiob.owner = THIS_MODULE;
> > +       vb_gpio->gpiob.base = -1;
> > +       vb_gpio->gpiob.ngpio = 16;
> > +       vb_gpio->gpiob.can_sleep = 1;
> > +       vb_gpio->gpiob.set = vprbrd_gpiob_set;
> > +       vb_gpio->gpiob.get = vprbrd_gpiob_get;
> > +       vb_gpio->gpiob.direction_input = vprbrd_gpiob_direction_input;
> > +       vb_gpio->gpiob.direction_output = vprbrd_gpiob_direction_output;
> > +       ret = gpiochip_add(&vb_gpio->gpiob);
> > +       if (ret < 0) {
> > +               dev_err(vb_gpio->gpiob.dev, "could not add gpio b");
> > +               goto err_gpiob;
> > +       }
> > +
> > +       platform_set_drvdata(pdev, vb_gpio);
> > +
> > +       return ret;
> > +
> > +err_gpiob:
> > +       ret = gpiochip_remove(&vb_gpio->gpioa);
> > +
> > +err_gpioa:
> > +       kfree(vb_gpio);
> 
> With devm_kzalloc you don't need this free.
> 
> (...)
> 
> > +static int __devexit vprbrd_gpio_remove(struct platform_device *pdev)
> > +{
> > +       struct vprbrd_gpio *vb_gpio = platform_get_drvdata(pdev);
> > +       int ret;
> > +
> > +       ret = gpiochip_remove(&vb_gpio->gpiob);
> > +       if (ret == 0)
> > +               ret = gpiochip_remove(&vb_gpio->gpioa);
> > +       if (ret == 0)
> > +               kfree(vb_gpio);
> 
> Nor this.
> 
> Apart from this the driver is looking nice!

Thank you for your feedback. I will wait some time for responses of the other 
maintainers and then do a version 3 of the whole patchset.

Regards,
Lars
--
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/