lkml.org 
[lkml]   [2012]   [Oct]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [PATCH v2 2/4] gpio: add viperboard gpio driver
    From
    On Fri, Oct 12, 2012 at 4:34 PM, Lars Poeschel <larsi@wh2.tu-dresden.de> wrote:

    > From: Lars Poeschel <poeschel@lemonage.de>
    >
    > 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@lemonage.de>

    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


    \
     
     \ /
      Last update: 2012-10-15 15:41    [W:2.465 / U:0.148 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site