| 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: <201210160851.33324.poeschel@lemonage.de>
Date: Tue, 16 Oct 2012 08:51:33 +0200
From: Lars Poeschel <poeschel@...onage.de>
To: Linus Walleij <linus.walleij@...aro.org>
Cc: Lars Poeschel <larsi@....tu-dresden.de>, 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
Subject: Re: [PATCH v2 2/4] gpio: add viperboard gpio driver
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@...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