| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CACRpkdadDJDU_UUmdFoKBUm49_1CPjBnibq_RsiSeu+y4zRTsw@mail.gmail.com>
Date: Mon, 15 Oct 2012 15:00:12 +0200
From: Linus Walleij <linus.walleij@...aro.org>
To: Lars Poeschel <larsi@....tu-dresden.de>
Cc: sameo@...ux.intel.com, linux-kernel@...r.kernel.org,
jic23@....ac.uk, khali@...ux-fr.org, ben-linux@...ff.org,
w.sang@...gutronix.de, grant.likely@...retlab.ca,
Lars Poeschel <poeschel@...onage.de>
Subject: Re: [PATCH v2 2/4] gpio: add viperboard gpio driver
On Fri, Oct 12, 2012 at 4:34 PM, Lars Poeschel <larsi@....tu-dresden.de> wrote:
> From: Lars Poeschel <poeschel@...onage.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@...onage.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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists