| Message ID | 20230912094519.22769-1-brgl@bgdev.pl |
|---|---|
| State | New |
| Headers | show |
| Series | [RFT,1/3] gpio: eic-sprd: unregister from the irq notifier on remove() | expand |
On 9/12/2023 5:45 PM, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > Instead of dereferencing pdev everywhere, just store the address of the > underlying struct device in a local variable. > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> Reviewed-by: Baolin Wang <baolin.wang@linux.alibaba.com> > --- > drivers/gpio/gpio-eic-sprd.c | 20 ++++++++++---------- > 1 file changed, 10 insertions(+), 10 deletions(-) > > diff --git a/drivers/gpio/gpio-eic-sprd.c b/drivers/gpio/gpio-eic-sprd.c > index 9b2f9ccf8d77..be7f2fa5aa7b 100644 > --- a/drivers/gpio/gpio-eic-sprd.c > +++ b/drivers/gpio/gpio-eic-sprd.c > @@ -591,18 +591,19 @@ static void sprd_eic_unregister_notifier(void *data) > static int sprd_eic_probe(struct platform_device *pdev) > { > const struct sprd_eic_variant_data *pdata; > + struct device *dev = &pdev->dev; > struct gpio_irq_chip *irq; > struct sprd_eic *sprd_eic; > struct resource *res; > int ret, i; > > - pdata = of_device_get_match_data(&pdev->dev); > + pdata = of_device_get_match_data(dev); > if (!pdata) { > - dev_err(&pdev->dev, "No matching driver data found.\n"); > + dev_err(dev, "No matching driver data found.\n"); > return -EINVAL; > } > > - sprd_eic = devm_kzalloc(&pdev->dev, sizeof(*sprd_eic), GFP_KERNEL); > + sprd_eic = devm_kzalloc(dev, sizeof(*sprd_eic), GFP_KERNEL); > if (!sprd_eic) > return -ENOMEM; > > @@ -624,7 +625,7 @@ static int sprd_eic_probe(struct platform_device *pdev) > if (!res) > break; > > - sprd_eic->base[i] = devm_ioremap_resource(&pdev->dev, res); > + sprd_eic->base[i] = devm_ioremap_resource(dev, res); > if (IS_ERR(sprd_eic->base[i])) > return PTR_ERR(sprd_eic->base[i]); > } > @@ -632,7 +633,7 @@ static int sprd_eic_probe(struct platform_device *pdev) > sprd_eic->chip.label = sprd_eic_label_name[sprd_eic->type]; > sprd_eic->chip.ngpio = pdata->num_eics; > sprd_eic->chip.base = -1; > - sprd_eic->chip.parent = &pdev->dev; > + sprd_eic->chip.parent = dev; > sprd_eic->chip.direction_input = sprd_eic_direction_input; > switch (sprd_eic->type) { > case SPRD_EIC_DEBOUNCE: > @@ -659,9 +660,9 @@ static int sprd_eic_probe(struct platform_device *pdev) > irq->num_parents = 1; > irq->parents = &sprd_eic->irq; > > - ret = devm_gpiochip_add_data(&pdev->dev, &sprd_eic->chip, sprd_eic); > + ret = devm_gpiochip_add_data(dev, &sprd_eic->chip, sprd_eic); > if (ret < 0) { > - dev_err(&pdev->dev, "Could not register gpiochip %d.\n", ret); > + dev_err(dev, "Could not register gpiochip %d.\n", ret); > return ret; > } > > @@ -669,11 +670,10 @@ static int sprd_eic_probe(struct platform_device *pdev) > ret = atomic_notifier_chain_register(&sprd_eic_irq_notifier, > &sprd_eic->irq_nb); > if (ret) > - return dev_err_probe(&pdev->dev, ret, > + return dev_err_probe(dev, ret, > "Failed to register with the interrupt notifier"); > > - return devm_add_action_or_reset(&pdev->dev, > - sprd_eic_unregister_notifier, > + return devm_add_action_or_reset(dev, sprd_eic_unregister_notifier, > &sprd_eic->irq_nb); > } >
On Tue, Sep 12, 2023 at 12:35 PM Andy Shevchenko <andy@kernel.org> wrote: > > On Tue, Sep 12, 2023 at 11:45:18AM +0200, Bartosz Golaszewski wrote: > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > Instead of dereferencing pdev everywhere, just store the address of the > > underlying struct device in a local variable. > > ... > > > - return devm_add_action_or_reset(&pdev->dev, > > - sprd_eic_unregister_notifier, > > + return devm_add_action_or_reset(dev, sprd_eic_unregister_notifier, > > &sprd_eic->irq_nb); > > Ping-pong style detected: Lines added / modified by previous patch in the same > series got modified again. > > If you look at how I do that, I introduce the temporary variable with my new > code and then reuse it later on. > > OTOH, I see that the first one is supposed to be backported (?) in such case > perhaps it's fine. I would typically do the same but the fix comes first in series. Bart
On Tue, Sep 12, 2023 at 1:02 PM Baolin Wang <baolin.wang@linux.alibaba.com> wrote: > > > > On 9/12/2023 5:45 PM, Bartosz Golaszewski wrote: > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > This is a tristate module, it can be unloaded. We need to cleanup properly > > and unregister from the interrupt notifier on driver detach. > > > > Fixes: b32415652a4d ("gpio: eic-sprd: use atomic notifiers to notify all chips about irqs") > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > LGTM. > Reviewed-by: Baolin Wang <baolin.wang@linux.alibaba.com> > Thanks, queued patches 1 and 2. Bart > > --- > > drivers/gpio/gpio-eic-sprd.c | 19 +++++++++++++++++-- > > 1 file changed, 17 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpio/gpio-eic-sprd.c b/drivers/gpio/gpio-eic-sprd.c > > index 21a1afe358d6..9b2f9ccf8d77 100644 > > --- a/drivers/gpio/gpio-eic-sprd.c > > +++ b/drivers/gpio/gpio-eic-sprd.c > > @@ -580,6 +580,14 @@ static const struct irq_chip sprd_eic_irq = { > > .flags = IRQCHIP_SKIP_SET_WAKE | IRQCHIP_IMMUTABLE, > > GPIOCHIP_IRQ_RESOURCE_HELPERS, > > }; > > + > > +static void sprd_eic_unregister_notifier(void *data) > > +{ > > + struct notifier_block *nb = data; > > + > > + atomic_notifier_chain_unregister(&sprd_eic_irq_notifier, nb); > > +} > > + > > static int sprd_eic_probe(struct platform_device *pdev) > > { > > const struct sprd_eic_variant_data *pdata; > > @@ -658,8 +666,15 @@ static int sprd_eic_probe(struct platform_device *pdev) > > } > > > > sprd_eic->irq_nb.notifier_call = sprd_eic_irq_notify; > > - return atomic_notifier_chain_register(&sprd_eic_irq_notifier, > > - &sprd_eic->irq_nb); > > + ret = atomic_notifier_chain_register(&sprd_eic_irq_notifier, > > + &sprd_eic->irq_nb); > > + if (ret) > > + return dev_err_probe(&pdev->dev, ret, > > + "Failed to register with the interrupt notifier"); > > + > > + return devm_add_action_or_reset(&pdev->dev, > > + sprd_eic_unregister_notifier, > > + &sprd_eic->irq_nb); > > } > > > > static const struct of_device_id sprd_eic_of_match[] = {
diff --git a/drivers/gpio/gpio-eic-sprd.c b/drivers/gpio/gpio-eic-sprd.c index 21a1afe358d6..9b2f9ccf8d77 100644 --- a/drivers/gpio/gpio-eic-sprd.c +++ b/drivers/gpio/gpio-eic-sprd.c @@ -580,6 +580,14 @@ static const struct irq_chip sprd_eic_irq = { .flags = IRQCHIP_SKIP_SET_WAKE | IRQCHIP_IMMUTABLE, GPIOCHIP_IRQ_RESOURCE_HELPERS, }; + +static void sprd_eic_unregister_notifier(void *data) +{ + struct notifier_block *nb = data; + + atomic_notifier_chain_unregister(&sprd_eic_irq_notifier, nb); +} + static int sprd_eic_probe(struct platform_device *pdev) { const struct sprd_eic_variant_data *pdata; @@ -658,8 +666,15 @@ static int sprd_eic_probe(struct platform_device *pdev) } sprd_eic->irq_nb.notifier_call = sprd_eic_irq_notify; - return atomic_notifier_chain_register(&sprd_eic_irq_notifier, - &sprd_eic->irq_nb); + ret = atomic_notifier_chain_register(&sprd_eic_irq_notifier, + &sprd_eic->irq_nb); + if (ret) + return dev_err_probe(&pdev->dev, ret, + "Failed to register with the interrupt notifier"); + + return devm_add_action_or_reset(&pdev->dev, + sprd_eic_unregister_notifier, + &sprd_eic->irq_nb); } static const struct of_device_id sprd_eic_of_match[] = {