| 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: <CACRpkdaSLGmmvWYG6ez1h90e=OZXq4yqtUFXKnTpUWG916LmjA@mail.gmail.com>
Date: Wed, 24 Apr 2013 15:15:16 +0200
From: Linus Walleij <linus.walleij@...aro.org>
To: Anthony Olech <anthony.olech.opensource@...semi.com>
Cc: Grant Likely <grant.likely@...retlab.ca>,
Linus Walleij <linus.walleij@...ricsson.com>,
Mark Brown <broonie@...nsource.wolfsonmicro.com>,
LKML <linux-kernel@...r.kernel.org>,
David Dajun Chen <david.chen@...semi.com>,
Lee Jones <lee.jones@...aro.org>
Subject: Re: [NEW DRIVER V6 5/7] drivers/gpio: DA9058 GPIO driver
On Fri, Apr 19, 2013 at 6:56 PM, Anthony Olech
<anthony.olech.opensource@...semi.com> wrote:
> This patch is relative to next-20130419 of linux-next
>
> This is the GPIO component driver of the Dialog DA9058 PMIC.
> This driver is just one component of the whole DA9058 PMIC driver.
> It depends on the CORE component driver of the DA9058 MFD.
> The meaning of the PMIC register 21 bits 1 and 5 has been documented
> in the driver source.
>
> Changes relative to V5 of this patch:
> - rebased to next-20130419 in git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git
> - removed redundant #include <linux/mfd/da9058/version.h>
> - corrected dates on copyright statements
>
> Signed-off-by: Anthony Olech <anthony.olech.opensource@...semi.com>
> Signed-off-by: David Dajun Chen <david.chen@...semi.com>
Sorry for slow review ...
> +struct da9058_gpio {
> + struct da9058 *da9058;
> + struct platform_device *pdev;
> + struct gpio_chip gp;
> + struct mutex lock;
> + int irq_0;
> + int irq_1;
Why are you keeping thes two numbers here? Use the irqdomain
to contain irq mappings, this is just confusion things. (See review
commens further below.)
> + u8 inp_config;
> + u8 out_config;
> +static int da9058_gpio_get(struct gpio_chip *gc, unsigned offset)
> +{
> + struct da9058_gpio *gpio = gpio_chip_to_da9058_gpio(gc);
> + struct da9058 *da9058 = gpio->da9058;
> + unsigned int gpio_level;
> + int ret;
> +
> + if (offset > 1)
> + return -EINVAL;
So we have only two pins, OK that's said in Kconfig too...
(...)
> +static void da9058_gpio_set(struct gpio_chip *gc, unsigned offset, int value)
> +{
> + struct da9058_gpio *gpio = gpio_chip_to_da9058_gpio(gc);
> + struct da9058 *da9058 = gpio->da9058;
> + unsigned int gpio_cntrl;
> + int ret;
> +
> + if (offset > 1) {
> + dev_err(da9058->dev,
> + "Failed to set GPIO%d output=%d because illegal GPIO\n",
> + offset, value);
> + return;
> + }
Here you have an error print, pls insert that on the get function too
then.
(...)
> + if (offset) {
> +
I would put a comment here like
/* pin 1 clause */
Maybe it's more logical to have if (!offset) and handle pin
0 first, then pin 1 in the else clause?
> + ret = da9058_reg_read(da9058, DA9058_GPIO0001_REG, &gpio_cntrl);
> + if (ret)
> + goto exit;
> + gpio->out_config &= ~0x80;
> + gpio->out_config |= value ? 0x80 : 0x00;
> +
> + if (!(gpio_cntrl & 0x20)) /* an INP pin */
> + goto exit;
Please use #define to define these magic bits.
Something like this:
#include <linux/bitops.h>
#define DA9058_GPIO0_VAL BIT(3)
#define DA9058_GPIO0_INOUT BIT(1)
#define DA9058_GPIO1_VAL BIT(7)
#define DA9058_GPIO1_INOUT BIT(5)
(etc, so we see what all bits are for)
then use these defines in the code...
> +
> + gpio_cntrl &= ~0xF0;
> + gpio_cntrl |= 0xF0 & gpio->out_config;
> +
> + ret = da9058_reg_write(da9058, DA9058_GPIO0001_REG, gpio_cntrl);
> + } else {
> +
I would put a comment here like
/* pin 0 clause */
> + ret = da9058_reg_read(da9058, DA9058_GPIO0001_REG, &gpio_cntrl);
> + if (ret)
> + goto exit;
> + gpio->out_config &= ~0x08;
> + gpio->out_config |= value ? 0x08 : 0x00;
> +
> + if (!(gpio_cntrl & 0x02)) /* an INP pin */
> + goto exit;
Magic bits, see above.
(...)
> +/*
> + * da9058_gpio_to_irq is an implementation of the GPIO Hook
> + * @to_irq: supporting non-static gpio_to_irq() mappings
> + * whose implementation may not sleep. This hook is called
> + * when setting up the threaded GPIO irq handler.
> + */
> +static int da9058_gpio_to_irq(struct gpio_chip *gc, u32 offset)
> +{
> + struct da9058_gpio *gpio = gpio_chip_to_da9058_gpio(gc);
> + struct da9058 *da9058 = gpio->da9058;
> +
> + if (offset > 1)
> + return -EINVAL;
> +
> + if (offset)
> + return da9058_to_virt_irq_num(da9058, gpio->irq_1);
> + else
> + return da9058_to_virt_irq_num(da9058, gpio->irq_0);
> +}
OK, if the DA9058 core is using irqdomain.
> + if (offset) {
> + u8 debounce_bits = debounce ? 0x80 : 0x00;
Is this really correct??
In earlier code you use bit 7 to set the value!
This is partly why I ask you to use #defines for the bits:
less risk to do things wrong by e.g. copy/paste bugs.
> + gpio->inp_config &= ~0x08;
> + gpio->inp_config |= debounce_bits;
Dito.
(...)
> +static int da9058_gpio_probe(struct platform_device *pdev)
(...)
> + /*
> + * INP configured pins:
> + * 9 == DebounceOn+ActiceLowPullUp
> + *
> + * OUT configured pins:
> + * 7 == PushPull+ExternalPullUpToVDDIO
> + * 3 == PushPull+InternalPullUpToVDDIO
> + * 2 == OpenDrain+InternalPullUpToVDDIO
> + */
> +
> + gpio->inp_config = 0x99;
> + gpio->out_config = 0x77;
Again here you should #define the bits and | or them together
instead of using comments to clarify it.
This is pinctrl basically but since you seem to hardcode it it can
be in the GPIO subsystem anyway.
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