* [RFT PATCH 1/3] gpio: eic-sprd: unregister from the irq notifier on remove()
@ 2023-09-12 9:45 Bartosz Golaszewski
2023-09-12 9:45 ` [RFT PATCH 2/3] gpio: eic-sprd: use a helper variable for &pdev->dev Bartosz Golaszewski
` (2 more replies)
0 siblings, 3 replies; 12+ messages in thread
From: Bartosz Golaszewski @ 2023-09-12 9:45 UTC (permalink / raw)
To: Linus Walleij, Andy Shevchenko, Orson Zhai, Baolin Wang,
Chunyan Zhang
Cc: linux-gpio, linux-kernel, Bartosz Golaszewski
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>
---
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[] = {
--
2.39.2
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [RFT PATCH 2/3] gpio: eic-sprd: use a helper variable for &pdev->dev
2023-09-12 9:45 [RFT PATCH 1/3] gpio: eic-sprd: unregister from the irq notifier on remove() Bartosz Golaszewski
@ 2023-09-12 9:45 ` Bartosz Golaszewski
2023-09-12 10:07 ` Baolin Wang
2023-09-12 10:35 ` Andy Shevchenko
2023-09-12 9:45 ` [RFT PATCH 3/3] gpio: eic-sprd: use devm_platform_ioremap_resource() Bartosz Golaszewski
2023-09-12 11:02 ` [RFT PATCH 1/3] gpio: eic-sprd: unregister from the irq notifier on remove() Baolin Wang
2 siblings, 2 replies; 12+ messages in thread
From: Bartosz Golaszewski @ 2023-09-12 9:45 UTC (permalink / raw)
To: Linus Walleij, Andy Shevchenko, Orson Zhai, Baolin Wang,
Chunyan Zhang
Cc: linux-gpio, linux-kernel, Bartosz Golaszewski
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>
---
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);
}
--
2.39.2
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [RFT PATCH 3/3] gpio: eic-sprd: use devm_platform_ioremap_resource()
2023-09-12 9:45 [RFT PATCH 1/3] gpio: eic-sprd: unregister from the irq notifier on remove() Bartosz Golaszewski
2023-09-12 9:45 ` [RFT PATCH 2/3] gpio: eic-sprd: use a helper variable for &pdev->dev Bartosz Golaszewski
@ 2023-09-12 9:45 ` Bartosz Golaszewski
2023-09-12 10:05 ` Baolin Wang
2023-09-12 10:37 ` Andy Shevchenko
2023-09-12 11:02 ` [RFT PATCH 1/3] gpio: eic-sprd: unregister from the irq notifier on remove() Baolin Wang
2 siblings, 2 replies; 12+ messages in thread
From: Bartosz Golaszewski @ 2023-09-12 9:45 UTC (permalink / raw)
To: Linus Walleij, Andy Shevchenko, Orson Zhai, Baolin Wang,
Chunyan Zhang
Cc: linux-gpio, linux-kernel, Bartosz Golaszewski
From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Make two calls into one by using devm_platform_ioremap_resource().
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
drivers/gpio/gpio-eic-sprd.c | 7 +------
1 file changed, 1 insertion(+), 6 deletions(-)
diff --git a/drivers/gpio/gpio-eic-sprd.c b/drivers/gpio/gpio-eic-sprd.c
index be7f2fa5aa7b..1e548e4e4cb8 100644
--- a/drivers/gpio/gpio-eic-sprd.c
+++ b/drivers/gpio/gpio-eic-sprd.c
@@ -594,7 +594,6 @@ static int sprd_eic_probe(struct platform_device *pdev)
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(dev);
@@ -621,11 +620,7 @@ static int sprd_eic_probe(struct platform_device *pdev)
* have one bank EIC, thus base[1] and base[2] can be
* optional.
*/
- res = platform_get_resource(pdev, IORESOURCE_MEM, i);
- if (!res)
- break;
-
- sprd_eic->base[i] = devm_ioremap_resource(dev, res);
+ sprd_eic->base[i] = devm_platform_ioremap_resource(pdev, i);
if (IS_ERR(sprd_eic->base[i]))
return PTR_ERR(sprd_eic->base[i]);
}
--
2.39.2
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [RFT PATCH 3/3] gpio: eic-sprd: use devm_platform_ioremap_resource()
2023-09-12 9:45 ` [RFT PATCH 3/3] gpio: eic-sprd: use devm_platform_ioremap_resource() Bartosz Golaszewski
@ 2023-09-12 10:05 ` Baolin Wang
2023-09-12 10:09 ` Bartosz Golaszewski
2023-09-12 10:37 ` Andy Shevchenko
1 sibling, 1 reply; 12+ messages in thread
From: Baolin Wang @ 2023-09-12 10:05 UTC (permalink / raw)
To: Bartosz Golaszewski, Linus Walleij, Andy Shevchenko, Orson Zhai,
Chunyan Zhang
Cc: linux-gpio, linux-kernel, Bartosz Golaszewski
On 9/12/2023 5:45 PM, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> Make two calls into one by using devm_platform_ioremap_resource().
>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Please don't do this. See the previous commit:
4ed7d7dd4890bb8120a3e77c16191a695fdfcc5a ("Revert "gpio: eic-sprd: Use
devm_platform_ioremap_resource()"")
> ---
> drivers/gpio/gpio-eic-sprd.c | 7 +------
> 1 file changed, 1 insertion(+), 6 deletions(-)
>
> diff --git a/drivers/gpio/gpio-eic-sprd.c b/drivers/gpio/gpio-eic-sprd.c
> index be7f2fa5aa7b..1e548e4e4cb8 100644
> --- a/drivers/gpio/gpio-eic-sprd.c
> +++ b/drivers/gpio/gpio-eic-sprd.c
> @@ -594,7 +594,6 @@ static int sprd_eic_probe(struct platform_device *pdev)
> 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(dev);
> @@ -621,11 +620,7 @@ static int sprd_eic_probe(struct platform_device *pdev)
> * have one bank EIC, thus base[1] and base[2] can be
> * optional.
> */
> - res = platform_get_resource(pdev, IORESOURCE_MEM, i);
> - if (!res)
> - break;
> -
> - sprd_eic->base[i] = devm_ioremap_resource(dev, res);
> + sprd_eic->base[i] = devm_platform_ioremap_resource(pdev, i);
> if (IS_ERR(sprd_eic->base[i]))
> return PTR_ERR(sprd_eic->base[i]);
> }
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFT PATCH 2/3] gpio: eic-sprd: use a helper variable for &pdev->dev
2023-09-12 9:45 ` [RFT PATCH 2/3] gpio: eic-sprd: use a helper variable for &pdev->dev Bartosz Golaszewski
@ 2023-09-12 10:07 ` Baolin Wang
2023-09-12 10:35 ` Andy Shevchenko
1 sibling, 0 replies; 12+ messages in thread
From: Baolin Wang @ 2023-09-12 10:07 UTC (permalink / raw)
To: Bartosz Golaszewski, Linus Walleij, Andy Shevchenko, Orson Zhai,
Chunyan Zhang
Cc: linux-gpio, linux-kernel, Bartosz Golaszewski
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);
> }
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFT PATCH 3/3] gpio: eic-sprd: use devm_platform_ioremap_resource()
2023-09-12 10:05 ` Baolin Wang
@ 2023-09-12 10:09 ` Bartosz Golaszewski
2023-09-12 11:07 ` Baolin Wang
0 siblings, 1 reply; 12+ messages in thread
From: Bartosz Golaszewski @ 2023-09-12 10:09 UTC (permalink / raw)
To: Baolin Wang
Cc: Linus Walleij, Andy Shevchenko, Orson Zhai, Chunyan Zhang,
linux-gpio, linux-kernel, Bartosz Golaszewski
On Tue, Sep 12, 2023 at 12:05 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>
> >
> > Make two calls into one by using devm_platform_ioremap_resource().
> >
> > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> Please don't do this. See the previous commit:
> 4ed7d7dd4890bb8120a3e77c16191a695fdfcc5a ("Revert "gpio: eic-sprd: Use
> devm_platform_ioremap_resource()"")
>
Ah, I see. Dropped. Could use a comment describing the problem though.
Bart
> > ---
> > drivers/gpio/gpio-eic-sprd.c | 7 +------
> > 1 file changed, 1 insertion(+), 6 deletions(-)
> >
> > diff --git a/drivers/gpio/gpio-eic-sprd.c b/drivers/gpio/gpio-eic-sprd.c
> > index be7f2fa5aa7b..1e548e4e4cb8 100644
> > --- a/drivers/gpio/gpio-eic-sprd.c
> > +++ b/drivers/gpio/gpio-eic-sprd.c
> > @@ -594,7 +594,6 @@ static int sprd_eic_probe(struct platform_device *pdev)
> > 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(dev);
> > @@ -621,11 +620,7 @@ static int sprd_eic_probe(struct platform_device *pdev)
> > * have one bank EIC, thus base[1] and base[2] can be
> > * optional.
> > */
> > - res = platform_get_resource(pdev, IORESOURCE_MEM, i);
> > - if (!res)
> > - break;
> > -
> > - sprd_eic->base[i] = devm_ioremap_resource(dev, res);
> > + sprd_eic->base[i] = devm_platform_ioremap_resource(pdev, i);
> > if (IS_ERR(sprd_eic->base[i]))
> > return PTR_ERR(sprd_eic->base[i]);
> > }
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFT PATCH 2/3] gpio: eic-sprd: use a helper variable for &pdev->dev
2023-09-12 9:45 ` [RFT PATCH 2/3] gpio: eic-sprd: use a helper variable for &pdev->dev Bartosz Golaszewski
2023-09-12 10:07 ` Baolin Wang
@ 2023-09-12 10:35 ` Andy Shevchenko
2023-09-12 11:23 ` Bartosz Golaszewski
1 sibling, 1 reply; 12+ messages in thread
From: Andy Shevchenko @ 2023-09-12 10:35 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Linus Walleij, Orson Zhai, Baolin Wang, Chunyan Zhang, linux-gpio,
linux-kernel, Bartosz Golaszewski
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.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFT PATCH 3/3] gpio: eic-sprd: use devm_platform_ioremap_resource()
2023-09-12 9:45 ` [RFT PATCH 3/3] gpio: eic-sprd: use devm_platform_ioremap_resource() Bartosz Golaszewski
2023-09-12 10:05 ` Baolin Wang
@ 2023-09-12 10:37 ` Andy Shevchenko
1 sibling, 0 replies; 12+ messages in thread
From: Andy Shevchenko @ 2023-09-12 10:37 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Linus Walleij, Orson Zhai, Baolin Wang, Chunyan Zhang, linux-gpio,
linux-kernel, Bartosz Golaszewski
On Tue, Sep 12, 2023 at 11:45:19AM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> Make two calls into one by using devm_platform_ioremap_resource().
...
> - res = platform_get_resource(pdev, IORESOURCE_MEM, i);
> - if (!res)
> - break;
> -
> - sprd_eic->base[i] = devm_ioremap_resource(dev, res);
> + sprd_eic->base[i] = devm_platform_ioremap_resource(pdev, i);
> if (IS_ERR(sprd_eic->base[i]))
> return PTR_ERR(sprd_eic->base[i]);
break != return
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFT PATCH 1/3] gpio: eic-sprd: unregister from the irq notifier on remove()
2023-09-12 9:45 [RFT PATCH 1/3] gpio: eic-sprd: unregister from the irq notifier on remove() Bartosz Golaszewski
2023-09-12 9:45 ` [RFT PATCH 2/3] gpio: eic-sprd: use a helper variable for &pdev->dev Bartosz Golaszewski
2023-09-12 9:45 ` [RFT PATCH 3/3] gpio: eic-sprd: use devm_platform_ioremap_resource() Bartosz Golaszewski
@ 2023-09-12 11:02 ` Baolin Wang
2023-09-13 12:14 ` Bartosz Golaszewski
2 siblings, 1 reply; 12+ messages in thread
From: Baolin Wang @ 2023-09-12 11:02 UTC (permalink / raw)
To: Bartosz Golaszewski, Linus Walleij, Andy Shevchenko, Orson Zhai,
Chunyan Zhang
Cc: linux-gpio, linux-kernel, Bartosz Golaszewski
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>
> ---
> 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[] = {
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFT PATCH 3/3] gpio: eic-sprd: use devm_platform_ioremap_resource()
2023-09-12 10:09 ` Bartosz Golaszewski
@ 2023-09-12 11:07 ` Baolin Wang
0 siblings, 0 replies; 12+ messages in thread
From: Baolin Wang @ 2023-09-12 11:07 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Linus Walleij, Andy Shevchenko, Orson Zhai, Chunyan Zhang,
linux-gpio, linux-kernel, Bartosz Golaszewski
On 9/12/2023 6:09 PM, Bartosz Golaszewski wrote:
> On Tue, Sep 12, 2023 at 12:05 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>
>>>
>>> Make two calls into one by using devm_platform_ioremap_resource().
>>>
>>> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>>
>> Please don't do this. See the previous commit:
>> 4ed7d7dd4890bb8120a3e77c16191a695fdfcc5a ("Revert "gpio: eic-sprd: Use
>> devm_platform_ioremap_resource()"")
>>
>
> Ah, I see. Dropped. Could use a comment describing the problem though.
Yes, I think so, in case someone does the same thing in the future.
>>> ---
>>> drivers/gpio/gpio-eic-sprd.c | 7 +------
>>> 1 file changed, 1 insertion(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/gpio/gpio-eic-sprd.c b/drivers/gpio/gpio-eic-sprd.c
>>> index be7f2fa5aa7b..1e548e4e4cb8 100644
>>> --- a/drivers/gpio/gpio-eic-sprd.c
>>> +++ b/drivers/gpio/gpio-eic-sprd.c
>>> @@ -594,7 +594,6 @@ static int sprd_eic_probe(struct platform_device *pdev)
>>> 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(dev);
>>> @@ -621,11 +620,7 @@ static int sprd_eic_probe(struct platform_device *pdev)
>>> * have one bank EIC, thus base[1] and base[2] can be
>>> * optional.
>>> */
>>> - res = platform_get_resource(pdev, IORESOURCE_MEM, i);
>>> - if (!res)
>>> - break;
>>> -
>>> - sprd_eic->base[i] = devm_ioremap_resource(dev, res);
>>> + sprd_eic->base[i] = devm_platform_ioremap_resource(pdev, i);
>>> if (IS_ERR(sprd_eic->base[i]))
>>> return PTR_ERR(sprd_eic->base[i]);
>>> }
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFT PATCH 2/3] gpio: eic-sprd: use a helper variable for &pdev->dev
2023-09-12 10:35 ` Andy Shevchenko
@ 2023-09-12 11:23 ` Bartosz Golaszewski
0 siblings, 0 replies; 12+ messages in thread
From: Bartosz Golaszewski @ 2023-09-12 11:23 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Linus Walleij, Orson Zhai, Baolin Wang, Chunyan Zhang, linux-gpio,
linux-kernel, Bartosz Golaszewski
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFT PATCH 1/3] gpio: eic-sprd: unregister from the irq notifier on remove()
2023-09-12 11:02 ` [RFT PATCH 1/3] gpio: eic-sprd: unregister from the irq notifier on remove() Baolin Wang
@ 2023-09-13 12:14 ` Bartosz Golaszewski
0 siblings, 0 replies; 12+ messages in thread
From: Bartosz Golaszewski @ 2023-09-13 12:14 UTC (permalink / raw)
To: Baolin Wang
Cc: Linus Walleij, Andy Shevchenko, Orson Zhai, Chunyan Zhang,
linux-gpio, linux-kernel, Bartosz Golaszewski
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[] = {
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2023-09-13 12:14 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-09-12 9:45 [RFT PATCH 1/3] gpio: eic-sprd: unregister from the irq notifier on remove() Bartosz Golaszewski
2023-09-12 9:45 ` [RFT PATCH 2/3] gpio: eic-sprd: use a helper variable for &pdev->dev Bartosz Golaszewski
2023-09-12 10:07 ` Baolin Wang
2023-09-12 10:35 ` Andy Shevchenko
2023-09-12 11:23 ` Bartosz Golaszewski
2023-09-12 9:45 ` [RFT PATCH 3/3] gpio: eic-sprd: use devm_platform_ioremap_resource() Bartosz Golaszewski
2023-09-12 10:05 ` Baolin Wang
2023-09-12 10:09 ` Bartosz Golaszewski
2023-09-12 11:07 ` Baolin Wang
2023-09-12 10:37 ` Andy Shevchenko
2023-09-12 11:02 ` [RFT PATCH 1/3] gpio: eic-sprd: unregister from the irq notifier on remove() Baolin Wang
2023-09-13 12:14 ` Bartosz Golaszewski
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).