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

[Linus Walleij]

On Fri, Oct 12, 2012 at 4:34 PM, Lars Poeschel <larsi@xxxxxxxxxxxxxxxxx> wrote:

> From: Lars Poeschel <poeschel@xxxxxxxxxxx>
>
> This adds the mfd cell to use the gpio a and gpio b part
> of the Nano River Technologies viperboard.
>
> Signed-off-by: Lars Poeschel <poeschel@xxxxxxxxxxx>

OK...
(...)
> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
(...)

> +#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?

(...)
> +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.

> +/* 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...

(...)
> +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.

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.)

> +       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, ...)?

> +       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!

Yours,
Linus Walleij
--
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/