Re: [PATCH 2/6] mfd: cros_ec: free IRQ automatically

[Lee Jones]

On Tue, 20 Feb 2018, Enric Balletbo Serra wrote:

> Hi Vincent,
> 
> 2018-02-20 9:00 GMT+01:00 Vincent Palatin :
> > On Mon, Feb 19, 2018 at 11:40 PM, Enric Balletbo i Serra
> >  wrote:
> >> From: Vincent Palatin 
> >>
> >> Free the IRQ we might have requested when removing the cros_ec device,
> >> so we can unload and reload the driver properly.
> >>
> >> Signed-off-by: Vincent Palatin 
> >> Signed-off-by: Enric Balletbo i Serra 
> >> ---
> >>  drivers/mfd/cros_ec.c | 17 ++---
> >>  1 file changed, 6 insertions(+), 11 deletions(-)
> >>
> >> diff --git a/drivers/mfd/cros_ec.c b/drivers/mfd/cros_ec.c
> >> index 74780f2964a1..d1a4fbee9380 100644
> >> --- a/drivers/mfd/cros_ec.c
> >> +++ b/drivers/mfd/cros_ec.c
> >> @@ -119,9 +119,9 @@ int cros_ec_register(struct cros_ec_device *ec_dev)
> >> }
> >>
> >> if (ec_dev->irq) {
> >> -   err = request_threaded_irq(ec_dev->irq, NULL, 
> >> ec_irq_thread,
> >> -  IRQF_TRIGGER_LOW | IRQF_ONESHOT,
> >> -  "chromeos-ec", ec_dev);
> >> +   err = devm_request_threaded_irq(dev, ec_dev->irq, NULL,
> >> +   ec_irq_thread, IRQF_TRIGGER_LOW | 
> >> IRQF_ONESHOT,
> >> +   "chromeos-ec", ec_dev);
> >> if (err) {
> >> dev_err(dev, "Failed to request IRQ %d: %d",
> >> ec_dev->irq, err);
> >> @@ -135,7 +135,7 @@ int cros_ec_register(struct cros_ec_device *ec_dev)
> >> dev_err(dev,
> >> "Failed to register Embedded Controller subdevice 
> >> %d\n",
> >> err);
> >> -   goto fail_mfd;
> >> +   return err;
> >> }
> >>
> >> if (ec_dev->max_passthru) {
> >> @@ -153,7 +153,7 @@ int cros_ec_register(struct cros_ec_device *ec_dev)
> >> dev_err(dev,
> >> "Failed to register Power Delivery 
> >> subdevice %d\n",
> >> err);
> >> -   goto fail_mfd;
> >> +   return err;
> >> }
> >> }
> >>
> >> @@ -162,7 +162,7 @@ int cros_ec_register(struct cros_ec_device *ec_dev)
> >> if (err) {
> >> mfd_remove_devices(dev);
> >> dev_err(dev, "Failed to register sub-devices\n");
> >> -   goto fail_mfd;
> >> +   return err;
> >> }
> >> }
> >>
> >> @@ -180,11 +180,6 @@ int cros_ec_register(struct cros_ec_device *ec_dev)
> >> cros_ec_acpi_install_gpe_handler(dev);
> >>
> >> return 0;
> >> -
> >> -fail_mfd:
> >> -   if (ec_dev->irq)
> >> -   free_irq(ec_dev->irq, ec_dev);
> >> -   return err;
> >>  }
> >>  EXPORT_SYMBOL(cros_ec_register);
> >
> >
> > You need to remove the "free_irq(ec_dev->irq, ec_dev);" in cros_ec_remove() 
> > too.
> > as done there:
> > https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/866858/15/drivers/mfd/cros_ec.c
> >
> > it was missing originally but was added by: 'f58b14e6632a mfd:
> > cros_ec: Free IRQ on exit'
> >
> 
> Many thanks to catch this. I'll wait a bit more for if I receive more
> feedback on the other patches and send a second version of this
> patchset.

Hmm... rather than mess around any further with this set, would you be
kind enough to pull all of the patches back into a single set and send
them out threaded with a cover-letter please?

I'm also going to un-apply 1/6 for completeness.

-- 
Lee Jones
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 2/6] mfd: cros_ec: free IRQ automatically

[Enric Balletbo Serra]

Hi Vincent,

2018-02-20 9:00 GMT+01:00 Vincent Palatin :
> On Mon, Feb 19, 2018 at 11:40 PM, Enric Balletbo i Serra
>  wrote:
>> From: Vincent Palatin 
>>
>> Free the IRQ we might have requested when removing the cros_ec device,
>> so we can unload and reload the driver properly.
>>
>> Signed-off-by: Vincent Palatin 
>> Signed-off-by: Enric Balletbo i Serra 
>> ---
>>  drivers/mfd/cros_ec.c | 17 ++---
>>  1 file changed, 6 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/mfd/cros_ec.c b/drivers/mfd/cros_ec.c
>> index 74780f2964a1..d1a4fbee9380 100644
>> --- a/drivers/mfd/cros_ec.c
>> +++ b/drivers/mfd/cros_ec.c
>> @@ -119,9 +119,9 @@ int cros_ec_register(struct cros_ec_device *ec_dev)
>> }
>>
>> if (ec_dev->irq) {
>> -   err = request_threaded_irq(ec_dev->irq, NULL, ec_irq_thread,
>> -  IRQF_TRIGGER_LOW | IRQF_ONESHOT,
>> -  "chromeos-ec", ec_dev);
>> +   err = devm_request_threaded_irq(dev, ec_dev->irq, NULL,
>> +   ec_irq_thread, IRQF_TRIGGER_LOW | 
>> IRQF_ONESHOT,
>> +   "chromeos-ec", ec_dev);
>> if (err) {
>> dev_err(dev, "Failed to request IRQ %d: %d",
>> ec_dev->irq, err);
>> @@ -135,7 +135,7 @@ int cros_ec_register(struct cros_ec_device *ec_dev)
>> dev_err(dev,
>> "Failed to register Embedded Controller subdevice 
>> %d\n",
>> err);
>> -   goto fail_mfd;
>> +   return err;
>> }
>>
>> if (ec_dev->max_passthru) {
>> @@ -153,7 +153,7 @@ int cros_ec_register(struct cros_ec_device *ec_dev)
>> dev_err(dev,
>> "Failed to register Power Delivery subdevice 
>> %d\n",
>> err);
>> -   goto fail_mfd;
>> +   return err;
>> }
>> }
>>
>> @@ -162,7 +162,7 @@ int cros_ec_register(struct cros_ec_device *ec_dev)
>> if (err) {
>> mfd_remove_devices(dev);
>> dev_err(dev, "Failed to register sub-devices\n");
>> -   goto fail_mfd;
>> +   return err;
>> }
>> }
>>
>> @@ -180,11 +180,6 @@ int cros_ec_register(struct cros_ec_device *ec_dev)
>> cros_ec_acpi_install_gpe_handler(dev);
>>
>> return 0;
>> -
>> -fail_mfd:
>> -   if (ec_dev->irq)
>> -   free_irq(ec_dev->irq, ec_dev);
>> -   return err;
>>  }
>>  EXPORT_SYMBOL(cros_ec_register);
>
>
> You need to remove the "free_irq(ec_dev->irq, ec_dev);" in cros_ec_remove() 
> too.
> as done there:
> https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/866858/15/drivers/mfd/cros_ec.c
>
> it was missing originally but was added by: 'f58b14e6632a mfd:
> cros_ec: Free IRQ on exit'
>

Many thanks to catch this. I'll wait a bit more for if I receive more
feedback on the other patches and send a second version of this
patchset.

Regards,
  Enric

>
>>
>> --
>> 2.16.1
>>

Re: [PATCH 2/6] mfd: cros_ec: free IRQ automatically

[Vincent Palatin]

On Mon, Feb 19, 2018 at 11:40 PM, Enric Balletbo i Serra
 wrote:
> From: Vincent Palatin 
>
> Free the IRQ we might have requested when removing the cros_ec device,
> so we can unload and reload the driver properly.
>
> Signed-off-by: Vincent Palatin 
> Signed-off-by: Enric Balletbo i Serra 
> ---
>  drivers/mfd/cros_ec.c | 17 ++---
>  1 file changed, 6 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/mfd/cros_ec.c b/drivers/mfd/cros_ec.c
> index 74780f2964a1..d1a4fbee9380 100644
> --- a/drivers/mfd/cros_ec.c
> +++ b/drivers/mfd/cros_ec.c
> @@ -119,9 +119,9 @@ int cros_ec_register(struct cros_ec_device *ec_dev)
> }
>
> if (ec_dev->irq) {
> -   err = request_threaded_irq(ec_dev->irq, NULL, ec_irq_thread,
> -  IRQF_TRIGGER_LOW | IRQF_ONESHOT,
> -  "chromeos-ec", ec_dev);
> +   err = devm_request_threaded_irq(dev, ec_dev->irq, NULL,
> +   ec_irq_thread, IRQF_TRIGGER_LOW | 
> IRQF_ONESHOT,
> +   "chromeos-ec", ec_dev);
> if (err) {
> dev_err(dev, "Failed to request IRQ %d: %d",
> ec_dev->irq, err);
> @@ -135,7 +135,7 @@ int cros_ec_register(struct cros_ec_device *ec_dev)
> dev_err(dev,
> "Failed to register Embedded Controller subdevice 
> %d\n",
> err);
> -   goto fail_mfd;
> +   return err;
> }
>
> if (ec_dev->max_passthru) {
> @@ -153,7 +153,7 @@ int cros_ec_register(struct cros_ec_device *ec_dev)
> dev_err(dev,
> "Failed to register Power Delivery subdevice 
> %d\n",
> err);
> -   goto fail_mfd;
> +   return err;
> }
> }
>
> @@ -162,7 +162,7 @@ int cros_ec_register(struct cros_ec_device *ec_dev)
> if (err) {
> mfd_remove_devices(dev);
> dev_err(dev, "Failed to register sub-devices\n");
> -   goto fail_mfd;
> +   return err;
> }
> }
>
> @@ -180,11 +180,6 @@ int cros_ec_register(struct cros_ec_device *ec_dev)
> cros_ec_acpi_install_gpe_handler(dev);
>
> return 0;
> -
> -fail_mfd:
> -   if (ec_dev->irq)
> -   free_irq(ec_dev->irq, ec_dev);
> -   return err;
>  }
>  EXPORT_SYMBOL(cros_ec_register);


You need to remove the "free_irq(ec_dev->irq, ec_dev);" in cros_ec_remove() too.
as done there:
https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/866858/15/drivers/mfd/cros_ec.c

it was missing originally but was added by: 'f58b14e6632a mfd:
cros_ec: Free IRQ on exit'


>
> --
> 2.16.1
>

[PATCH 2/6] mfd: cros_ec: free IRQ automatically

[Enric Balletbo i Serra]

From: Vincent Palatin 

Free the IRQ we might have requested when removing the cros_ec device,
so we can unload and reload the driver properly.

Signed-off-by: Vincent Palatin 
Signed-off-by: Enric Balletbo i Serra 
---
 drivers/mfd/cros_ec.c | 17 ++---
 1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/drivers/mfd/cros_ec.c b/drivers/mfd/cros_ec.c
index 74780f2964a1..d1a4fbee9380 100644
--- a/drivers/mfd/cros_ec.c
+++ b/drivers/mfd/cros_ec.c
@@ -119,9 +119,9 @@ int cros_ec_register(struct cros_ec_device *ec_dev)
}
 
if (ec_dev->irq) {
-   err = request_threaded_irq(ec_dev->irq, NULL, ec_irq_thread,
-  IRQF_TRIGGER_LOW | IRQF_ONESHOT,
-  "chromeos-ec", ec_dev);
+   err = devm_request_threaded_irq(dev, ec_dev->irq, NULL,
+   ec_irq_thread, IRQF_TRIGGER_LOW | IRQF_ONESHOT,
+   "chromeos-ec", ec_dev);
if (err) {
dev_err(dev, "Failed to request IRQ %d: %d",
ec_dev->irq, err);
@@ -135,7 +135,7 @@ int cros_ec_register(struct cros_ec_device *ec_dev)
dev_err(dev,
"Failed to register Embedded Controller subdevice %d\n",
err);
-   goto fail_mfd;
+   return err;
}
 
if (ec_dev->max_passthru) {
@@ -153,7 +153,7 @@ int cros_ec_register(struct cros_ec_device *ec_dev)
dev_err(dev,
"Failed to register Power Delivery subdevice 
%d\n",
err);
-   goto fail_mfd;
+   return err;
}
}
 
@@ -162,7 +162,7 @@ int cros_ec_register(struct cros_ec_device *ec_dev)
if (err) {
mfd_remove_devices(dev);
dev_err(dev, "Failed to register sub-devices\n");
-   goto fail_mfd;
+   return err;
}
}
 
@@ -180,11 +180,6 @@ int cros_ec_register(struct cros_ec_device *ec_dev)
cros_ec_acpi_install_gpe_handler(dev);
 
return 0;
-
-fail_mfd:
-   if (ec_dev->irq)
-   free_irq(ec_dev->irq, ec_dev);
-   return err;
 }
 EXPORT_SYMBOL(cros_ec_register);
 
-- 
2.16.1