| Message ID | 1428718808.25635.2.camel@phoenix |
|---|---|
| State | Superseded |
| Delegated to: | Albert ARIBAUD |
| Headers | show |
Hi Axel, Le Sat, 11 Apr 2015 10:20:08 +0800, Axel Lin <axel.lin@ingics.com> a écrit : > Initially I found this driver has set priv_auto_alloc_size but it actually > never use dev->priv. The U_BOOT_DEVICE(lpc32xx_gpios) does not provide the > platdata and all fields in struct lpc32xx_gpio_platdata are set in probe. > It looks like the struct lpc32xx_gpio_platdata actually should be a priv > data. Thus this patch renames lpc32xx_gpio_platdata to lpc32xx_gpio_priv > and converts all dev_get_platdata() to dev_get_priv(). > > Signed-off-by: Axel Lin <axel.lin@ingics.com> > --- > Hi Albert, > I don't have this h/w for testing, so only compile test. > I'd appreciate if you can review and test this patch. > Thanks, > Axel Indeed the driver allocates priv and does not use it. However, I think that the allocation should be removed as useless, rather than plat data be converted to priv. IIUC, priv is a way to apply a single driver to several similar devices, and LPC32XX only has one GPIO device. Cordialement, Albert ARIBAUD 3ADEV
2015-04-13 16:41 GMT+08:00 Albert ARIBAUD <albert.aribaud@3adev.fr>: > Hi Axel, > > Le Sat, 11 Apr 2015 10:20:08 +0800, Axel Lin <axel.lin@ingics.com> a > écrit : > >> Initially I found this driver has set priv_auto_alloc_size but it actually >> never use dev->priv. The U_BOOT_DEVICE(lpc32xx_gpios) does not provide the >> platdata and all fields in struct lpc32xx_gpio_platdata are set in probe. >> It looks like the struct lpc32xx_gpio_platdata actually should be a priv >> data. Thus this patch renames lpc32xx_gpio_platdata to lpc32xx_gpio_priv >> and converts all dev_get_platdata() to dev_get_priv(). >> >> Signed-off-by: Axel Lin <axel.lin@ingics.com> >> --- >> Hi Albert, >> I don't have this h/w for testing, so only compile test. >> I'd appreciate if you can review and test this patch. >> Thanks, >> Axel > > Indeed the driver allocates priv and does not use it. However, I think > that the allocation should be removed as useless, rather than plat data > be converted to priv. IIUC, priv is a way to apply a single driver to > several similar devices, and LPC32XX only has one GPIO device. Hi Albert, I think it's fine to use privdata here even though LPC32XX only has one GPIO device. gpio_platdata->function stores the runtime state which should be stored in privdata. I don't see any good reason to use platdata here, that is why I think it's a misue of platdata and thus convert it to use privdata. Regards, Axel
Hi Axel, Le Tue, 14 Apr 2015 08:30:03 +0800, Axel Lin <axel.lin@ingics.com> a écrit : > 2015-04-13 16:41 GMT+08:00 Albert ARIBAUD <albert.aribaud@3adev.fr>: > > Hi Axel, > > > > Le Sat, 11 Apr 2015 10:20:08 +0800, Axel Lin <axel.lin@ingics.com> a > > écrit : > > > >> Initially I found this driver has set priv_auto_alloc_size but it actually > >> never use dev->priv. The U_BOOT_DEVICE(lpc32xx_gpios) does not provide the > >> platdata and all fields in struct lpc32xx_gpio_platdata are set in probe. > >> It looks like the struct lpc32xx_gpio_platdata actually should be a priv > >> data. Thus this patch renames lpc32xx_gpio_platdata to lpc32xx_gpio_priv > >> and converts all dev_get_platdata() to dev_get_priv(). > >> > >> Signed-off-by: Axel Lin <axel.lin@ingics.com> > >> --- > >> Hi Albert, > >> I don't have this h/w for testing, so only compile test. > >> I'd appreciate if you can review and test this patch. > >> Thanks, > >> Axel > > > > Indeed the driver allocates priv and does not use it. However, I think > > that the allocation should be removed as useless, rather than plat data > > be converted to priv. IIUC, priv is a way to apply a single driver to > > several similar devices, and LPC32XX only has one GPIO device. > > Hi Albert, > > I think it's fine to use privdata here even though LPC32XX only has > one GPIO device. > gpio_platdata->function stores the runtime state which should be > stored in privdata. > I don't see any good reason to use platdata here, that is why I think it's a > misue of platdata and thus convert it to use privdata. That's a good point, sorry to have missed it. Can you please reword the commit message so that it describes what you just wrote? Something along the lines of "The LPC32XX GPIO driver platdata currently contains GPIO state information, which should go into priv_data"? Thanks in advance. > Regards, > Axel Cordialement, Albert ARIBAUD 3ADEV
diff --git a/drivers/gpio/lpc32xx_gpio.c b/drivers/gpio/lpc32xx_gpio.c index 96b3125..8a9826e 100644 --- a/drivers/gpio/lpc32xx_gpio.c +++ b/drivers/gpio/lpc32xx_gpio.c @@ -37,7 +37,7 @@ #define LPC32XX_GPIOS 128 -struct lpc32xx_gpio_platdata { +struct lpc32xx_gpio_priv { struct gpio_regs *regs; /* GPIO FUNCTION: SEE WARNING #2 */ signed char function[LPC32XX_GPIOS]; @@ -60,8 +60,8 @@ struct lpc32xx_gpio_platdata { static int lpc32xx_gpio_direction_input(struct udevice *dev, unsigned offset) { int port, mask; - struct lpc32xx_gpio_platdata *gpio_platdata = dev_get_platdata(dev); - struct gpio_regs *regs = gpio_platdata->regs; + struct lpc32xx_gpio_priv *gpio_priv = dev_get_priv(dev); + struct gpio_regs *regs = gpio_priv->regs; port = GPIO_TO_PORT(offset); mask = GPIO_TO_MASK(offset); @@ -83,7 +83,7 @@ static int lpc32xx_gpio_direction_input(struct udevice *dev, unsigned offset) } /* GPIO FUNCTION: SEE WARNING #2 */ - gpio_platdata->function[offset] = GPIOF_INPUT; + gpio_priv->function[offset] = GPIOF_INPUT; return 0; } @@ -95,8 +95,8 @@ static int lpc32xx_gpio_direction_input(struct udevice *dev, unsigned offset) static int lpc32xx_gpio_get_value(struct udevice *dev, unsigned offset) { int port, rank, mask, value; - struct lpc32xx_gpio_platdata *gpio_platdata = dev_get_platdata(dev); - struct gpio_regs *regs = gpio_platdata->regs; + struct lpc32xx_gpio_priv *gpio_priv = dev_get_priv(dev); + struct gpio_regs *regs = gpio_priv->regs; port = GPIO_TO_PORT(offset); @@ -130,8 +130,8 @@ static int lpc32xx_gpio_get_value(struct udevice *dev, unsigned offset) static int gpio_set(struct udevice *dev, unsigned gpio) { int port, mask; - struct lpc32xx_gpio_platdata *gpio_platdata = dev_get_platdata(dev); - struct gpio_regs *regs = gpio_platdata->regs; + struct lpc32xx_gpio_priv *gpio_priv = dev_get_priv(dev); + struct gpio_regs *regs = gpio_priv->regs; port = GPIO_TO_PORT(gpio); mask = GPIO_TO_MASK(gpio); @@ -162,8 +162,8 @@ static int gpio_set(struct udevice *dev, unsigned gpio) static int gpio_clr(struct udevice *dev, unsigned gpio) { int port, mask; - struct lpc32xx_gpio_platdata *gpio_platdata = dev_get_platdata(dev); - struct gpio_regs *regs = gpio_platdata->regs; + struct lpc32xx_gpio_priv *gpio_priv = dev_get_priv(dev); + struct gpio_regs *regs = gpio_priv->regs; port = GPIO_TO_PORT(gpio); mask = GPIO_TO_MASK(gpio); @@ -208,8 +208,8 @@ static int lpc32xx_gpio_direction_output(struct udevice *dev, unsigned offset, int value) { int port, mask; - struct lpc32xx_gpio_platdata *gpio_platdata = dev_get_platdata(dev); - struct gpio_regs *regs = gpio_platdata->regs; + struct lpc32xx_gpio_priv *gpio_priv = dev_get_priv(dev); + struct gpio_regs *regs = gpio_priv->regs; port = GPIO_TO_PORT(offset); mask = GPIO_TO_MASK(offset); @@ -231,7 +231,7 @@ static int lpc32xx_gpio_direction_output(struct udevice *dev, unsigned offset, } /* GPIO FUNCTION: SEE WARNING #2 */ - gpio_platdata->function[offset] = GPIOF_OUTPUT; + gpio_priv->function[offset] = GPIOF_OUTPUT; return lpc32xx_gpio_set_value(dev, offset, value); } @@ -251,8 +251,8 @@ static int lpc32xx_gpio_direction_output(struct udevice *dev, unsigned offset, static int lpc32xx_gpio_get_function(struct udevice *dev, unsigned offset) { - struct lpc32xx_gpio_platdata *gpio_platdata = dev_get_platdata(dev); - return gpio_platdata->function[offset]; + struct lpc32xx_gpio_priv *gpio_priv = dev_get_priv(dev); + return gpio_priv->function[offset]; } static const struct dm_gpio_ops gpio_lpc32xx_ops = { @@ -265,7 +265,7 @@ static const struct dm_gpio_ops gpio_lpc32xx_ops = { static int lpc32xx_gpio_probe(struct udevice *dev) { - struct lpc32xx_gpio_platdata *gpio_platdata = dev_get_platdata(dev); + struct lpc32xx_gpio_priv *gpio_priv = dev_get_priv(dev); struct gpio_dev_priv *uc_priv = dev->uclass_priv; if (dev->of_offset == -1) { @@ -274,12 +274,11 @@ static int lpc32xx_gpio_probe(struct udevice *dev) } /* set base address for GPIO registers */ - gpio_platdata->regs = (struct gpio_regs *)GPIO_BASE; + gpio_priv->regs = (struct gpio_regs *)GPIO_BASE; /* all GPIO functions are unknown until requested */ /* GPIO FUNCTION: SEE WARNING #2 */ - memset(gpio_platdata->function, GPIOF_UNKNOWN, - sizeof(gpio_platdata->function)); + memset(gpio_priv->function, GPIOF_UNKNOWN, sizeof(gpio_priv->function)); return 0; } @@ -289,5 +288,5 @@ U_BOOT_DRIVER(gpio_lpc32xx) = { .id = UCLASS_GPIO, .ops = &gpio_lpc32xx_ops, .probe = lpc32xx_gpio_probe, - .priv_auto_alloc_size = sizeof(struct lpc32xx_gpio_platdata), + .priv_auto_alloc_size = sizeof(struct lpc32xx_gpio_priv), };
Initially I found this driver has set priv_auto_alloc_size but it actually never use dev->priv. The U_BOOT_DEVICE(lpc32xx_gpios) does not provide the platdata and all fields in struct lpc32xx_gpio_platdata are set in probe. It looks like the struct lpc32xx_gpio_platdata actually should be a priv data. Thus this patch renames lpc32xx_gpio_platdata to lpc32xx_gpio_priv and converts all dev_get_platdata() to dev_get_priv(). Signed-off-by: Axel Lin <axel.lin@ingics.com> --- Hi Albert, I don't have this h/w for testing, so only compile test. I'd appreciate if you can review and test this patch. Thanks, Axel drivers/gpio/lpc32xx_gpio.c | 39 +++++++++++++++++++-------------------- 1 file changed, 19 insertions(+), 20 deletions(-)