Message ID | 20210707135144.191567-1-aardelean@deviqon.com |
---|---|
State | New |
Headers | show |
Series | gpio: viperboard: remove platform_set_drvdata() call in probe | expand |
On Wed, Jul 7, 2021 at 3:51 PM Alexandru Ardelean <aardelean@deviqon.com> wrote: > > The platform_set_drvdata() call is only useful if we need to retrieve back > the private information. > Since the driver doesn't do that, it's not useful to have it. > > This change removes it. > > Signed-off-by: Alexandru Ardelean <aardelean@deviqon.com> > --- > drivers/gpio/gpio-viperboard.c | 6 +----- > 1 file changed, 1 insertion(+), 5 deletions(-) > > diff --git a/drivers/gpio/gpio-viperboard.c b/drivers/gpio/gpio-viperboard.c > index c301c1d56dd2..98ddd6590362 100644 > --- a/drivers/gpio/gpio-viperboard.c > +++ b/drivers/gpio/gpio-viperboard.c > @@ -422,12 +422,8 @@ static int vprbrd_gpio_probe(struct platform_device *pdev) > vb_gpio->gpiob.direction_input = vprbrd_gpiob_direction_input; > vb_gpio->gpiob.direction_output = vprbrd_gpiob_direction_output; > ret = devm_gpiochip_add_data(&pdev->dev, &vb_gpio->gpiob, vb_gpio); > - if (ret < 0) { > + if (ret < 0) > dev_err(vb_gpio->gpiob.parent, "could not add gpio b"); > - return ret; > - } > - > - platform_set_drvdata(pdev, vb_gpio); > > return ret; > } > -- > 2.31.1 > The log is not really needed, we'll get an error message from gpiolib core. Can you remove it while you're at it and just return the result of devm_gpiochip_add_data()? Bart
On Wed, 21 Jul 2021 at 16:16, Bartosz Golaszewski <bgolaszewski@baylibre.com> wrote: > > On Wed, Jul 7, 2021 at 3:51 PM Alexandru Ardelean <aardelean@deviqon.com> wrote: > > > > The platform_set_drvdata() call is only useful if we need to retrieve back > > the private information. > > Since the driver doesn't do that, it's not useful to have it. > > > > This change removes it. > > > > Signed-off-by: Alexandru Ardelean <aardelean@deviqon.com> > > --- > > drivers/gpio/gpio-viperboard.c | 6 +----- > > 1 file changed, 1 insertion(+), 5 deletions(-) > > > > diff --git a/drivers/gpio/gpio-viperboard.c b/drivers/gpio/gpio-viperboard.c > > index c301c1d56dd2..98ddd6590362 100644 > > --- a/drivers/gpio/gpio-viperboard.c > > +++ b/drivers/gpio/gpio-viperboard.c > > @@ -422,12 +422,8 @@ static int vprbrd_gpio_probe(struct platform_device *pdev) > > vb_gpio->gpiob.direction_input = vprbrd_gpiob_direction_input; > > vb_gpio->gpiob.direction_output = vprbrd_gpiob_direction_output; > > ret = devm_gpiochip_add_data(&pdev->dev, &vb_gpio->gpiob, vb_gpio); > > - if (ret < 0) { > > + if (ret < 0) > > dev_err(vb_gpio->gpiob.parent, "could not add gpio b"); > > - return ret; > > - } > > - > > - platform_set_drvdata(pdev, vb_gpio); > > > > return ret; > > } > > -- > > 2.31.1 > > > > The log is not really needed, we'll get an error message from gpiolib > core. Can you remove it while you're at it and just return the result > of devm_gpiochip_add_data()? I thought about removing it, but in this driver there are 2 devm_gpiochip_add_data() calls. It registers 2 GPIOchip instances. Which is not so easy to see in this patch. First one says "could not add gpio a" and this one says "could not add gpio b". I hesitated to remove either of these. In this case, it may be a little helpful to know which GPIOchip failed to be registered. But I don't mind removing them both. Whatever you prefer. I'm undecided. > > Bart
On Wed, Jul 21, 2021 at 4:13 PM Alexandru Ardelean <aardelean@deviqon.com> wrote: > > On Wed, 21 Jul 2021 at 16:16, Bartosz Golaszewski > <bgolaszewski@baylibre.com> wrote: > > > > On Wed, Jul 7, 2021 at 3:51 PM Alexandru Ardelean <aardelean@deviqon.com> wrote: > > > > > > The platform_set_drvdata() call is only useful if we need to retrieve back > > > the private information. > > > Since the driver doesn't do that, it's not useful to have it. > > > > > > This change removes it. > > > > > > Signed-off-by: Alexandru Ardelean <aardelean@deviqon.com> > > > --- > > > drivers/gpio/gpio-viperboard.c | 6 +----- > > > 1 file changed, 1 insertion(+), 5 deletions(-) > > > > > > diff --git a/drivers/gpio/gpio-viperboard.c b/drivers/gpio/gpio-viperboard.c > > > index c301c1d56dd2..98ddd6590362 100644 > > > --- a/drivers/gpio/gpio-viperboard.c > > > +++ b/drivers/gpio/gpio-viperboard.c > > > @@ -422,12 +422,8 @@ static int vprbrd_gpio_probe(struct platform_device *pdev) > > > vb_gpio->gpiob.direction_input = vprbrd_gpiob_direction_input; > > > vb_gpio->gpiob.direction_output = vprbrd_gpiob_direction_output; > > > ret = devm_gpiochip_add_data(&pdev->dev, &vb_gpio->gpiob, vb_gpio); > > > - if (ret < 0) { > > > + if (ret < 0) > > > dev_err(vb_gpio->gpiob.parent, "could not add gpio b"); > > > - return ret; > > > - } > > > - > > > - platform_set_drvdata(pdev, vb_gpio); > > > > > > return ret; > > > } > > > -- > > > 2.31.1 > > > > > > > The log is not really needed, we'll get an error message from gpiolib > > core. Can you remove it while you're at it and just return the result > > of devm_gpiochip_add_data()? > > I thought about removing it, but in this driver there are 2 > devm_gpiochip_add_data() calls. > It registers 2 GPIOchip instances. > Which is not so easy to see in this patch. > > First one says "could not add gpio a" and this one says "could not add gpio b". > I hesitated to remove either of these. > > In this case, it may be a little helpful to know which GPIOchip failed > to be registered. > > But I don't mind removing them both. > Whatever you prefer. I'm undecided. > The core code will still use the label for the error message which says 'a' or 'b' already. I think we can remove it.
diff --git a/drivers/gpio/gpio-viperboard.c b/drivers/gpio/gpio-viperboard.c index c301c1d56dd2..98ddd6590362 100644 --- a/drivers/gpio/gpio-viperboard.c +++ b/drivers/gpio/gpio-viperboard.c @@ -422,12 +422,8 @@ static int vprbrd_gpio_probe(struct platform_device *pdev) vb_gpio->gpiob.direction_input = vprbrd_gpiob_direction_input; vb_gpio->gpiob.direction_output = vprbrd_gpiob_direction_output; ret = devm_gpiochip_add_data(&pdev->dev, &vb_gpio->gpiob, vb_gpio); - if (ret < 0) { + if (ret < 0) dev_err(vb_gpio->gpiob.parent, "could not add gpio b"); - return ret; - } - - platform_set_drvdata(pdev, vb_gpio); return ret; }
The platform_set_drvdata() call is only useful if we need to retrieve back the private information. Since the driver doesn't do that, it's not useful to have it. This change removes it. Signed-off-by: Alexandru Ardelean <aardelean@deviqon.com> --- drivers/gpio/gpio-viperboard.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-)