All of lore.kernel.org
 help / color / mirror / Atom feed
From: Alexandru Ardelean <aardelean@deviqon.com>
To: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: "linux-gpio@vger.kernel.org" <linux-gpio@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"linus.walleij@linaro.org" <linus.walleij@linaro.org>,
	"bgolaszewski@baylibre.com" <bgolaszewski@baylibre.com>,
	"alexandre.torgue@foss.st.com" <alexandre.torgue@foss.st.com>,
	"mcoquelin.stm32@gmail.com" <mcoquelin.stm32@gmail.com>
Subject: Re: [PATCH] gpio: gpio-stmpe: fully use convert probe to device-managed
Date: Thu, 20 May 2021 13:06:30 +0300	[thread overview]
Message-ID: <CAASAkoa6btmLCj-XouKVj=hCG2euhT2MnNWWQAS7jorJBEoQEA@mail.gmail.com> (raw)
In-Reply-To: <CAHp75VcesNFXCMoWbdBR2mFCt89p8aycWbheMhv9DnU8TBqNSA@mail.gmail.com>

On Thu, 20 May 2021 at 09:40, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>
>
>
> On Sunday, May 16, 2021, Alexandru Ardelean <aardelean@deviqon.com> wrote:
>>
>> The driver doesn't look like it can be built as a kmod, so leaks cannot
>> happen via a rmmod mechanism.
>> The remove hook was removed via commit 3b52bb960ec6 ("gpio: stmpe: make
>> it explicitly non-modular").
>>
>> The IRQ is registered via devm_request_threaded_irq(), making the driver
>> only partially device-managed.
>>
>> In any case all resources should be made device-managed, mostly as a good
>> practice. That way at least the unwinding on error is happening in reverse
>> order (as the probe).
>>
>> This change also removes platform_set_drvdata() since the information is
>> never retrieved to be used in the driver.
>
>
> Any driver can be unbind from device thru sysfs. The exception is when they (device drivers) specifically disable that.

Oh, I see.
Thanks for the info :)

>
>>
>> Signed-off-by: Alexandru Ardelean <aardelean@deviqon.com>
>> ---
>>
>> I'm not sure if this should be marked with a Fixes tag.
>> But if so, it should probably be for commit 3b52bb960ec6 (also mentioned in
>> the comment above).
>>
>>  drivers/gpio/gpio-stmpe.c | 32 +++++++++++++-------------------
>>  1 file changed, 13 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/gpio/gpio-stmpe.c b/drivers/gpio/gpio-stmpe.c
>> index b94ef8181428..dd4d58b4ae49 100644
>> --- a/drivers/gpio/gpio-stmpe.c
>> +++ b/drivers/gpio/gpio-stmpe.c
>> @@ -449,6 +449,11 @@ static void stmpe_init_irq_valid_mask(struct gpio_chip *gc,
>>         }
>>  }
>>
>> +static void stmpe_gpio_disable(void *stmpe)
>> +{
>> +       stmpe_disable(stmpe, STMPE_BLOCK_GPIO);
>> +}
>> +
>>  static int stmpe_gpio_probe(struct platform_device *pdev)
>>  {
>>         struct stmpe *stmpe = dev_get_drvdata(pdev->dev.parent);
>> @@ -461,7 +466,7 @@ static int stmpe_gpio_probe(struct platform_device *pdev)
>>                 return -EINVAL;
>>         }
>>
>> -       stmpe_gpio = kzalloc(sizeof(*stmpe_gpio), GFP_KERNEL);
>> +       stmpe_gpio = devm_kzalloc(&pdev->dev, sizeof(*stmpe_gpio), GFP_KERNEL);
>>         if (!stmpe_gpio)
>>                 return -ENOMEM;
>>
>> @@ -489,7 +494,11 @@ static int stmpe_gpio_probe(struct platform_device *pdev)
>>
>>         ret = stmpe_enable(stmpe, STMPE_BLOCK_GPIO);
>>         if (ret)
>> -               goto out_free;
>> +               return ret;
>> +
>> +       ret = devm_add_action_or_reset(&pdev->dev, stmpe_gpio_disable, stmpe);
>> +       if (ret)
>> +               return ret;
>>
>>         if (irq > 0) {
>>                 struct gpio_irq_chip *girq;
>> @@ -499,7 +508,7 @@ static int stmpe_gpio_probe(struct platform_device *pdev)
>>                                 "stmpe-gpio", stmpe_gpio);
>>                 if (ret) {
>>                         dev_err(&pdev->dev, "unable to get irq: %d\n", ret);
>> -                       goto out_disable;
>> +                       return ret;
>>                 }
>>
>>                 girq = &stmpe_gpio->chip.irq;
>> @@ -514,22 +523,7 @@ static int stmpe_gpio_probe(struct platform_device *pdev)
>>                 girq->init_valid_mask = stmpe_init_irq_valid_mask;
>>         }
>>
>> -       ret = gpiochip_add_data(&stmpe_gpio->chip, stmpe_gpio);
>> -       if (ret) {
>> -               dev_err(&pdev->dev, "unable to add gpiochip: %d\n", ret);
>> -               goto out_disable;
>> -       }
>> -
>> -       platform_set_drvdata(pdev, stmpe_gpio);
>> -
>> -       return 0;
>> -
>> -out_disable:
>> -       stmpe_disable(stmpe, STMPE_BLOCK_GPIO);
>> -       gpiochip_remove(&stmpe_gpio->chip);
>> -out_free:
>> -       kfree(stmpe_gpio);
>> -       return ret;
>> +       return devm_gpiochip_add_data(&pdev->dev, &stmpe_gpio->chip, stmpe_gpio);
>>  }
>>
>>  static struct platform_driver stmpe_gpio_driver = {
>> --
>> 2.31.1
>>
>
>
> --
> With Best Regards,
> Andy Shevchenko
>
>

  parent reply	other threads:[~2021-05-20 11:26 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-05-16  6:14 [PATCH] gpio: gpio-stmpe: fully use convert probe to device-managed Alexandru Ardelean
2021-05-19 23:55 ` Linus Walleij
     [not found] ` <CAHp75VcesNFXCMoWbdBR2mFCt89p8aycWbheMhv9DnU8TBqNSA@mail.gmail.com>
2021-05-20 10:06   ` Alexandru Ardelean [this message]
2021-05-21 12:42 ` Bartosz Golaszewski

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAASAkoa6btmLCj-XouKVj=hCG2euhT2MnNWWQAS7jorJBEoQEA@mail.gmail.com' \
    --to=aardelean@deviqon.com \
    --cc=alexandre.torgue@foss.st.com \
    --cc=andy.shevchenko@gmail.com \
    --cc=bgolaszewski@baylibre.com \
    --cc=linus.walleij@linaro.org \
    --cc=linux-gpio@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mcoquelin.stm32@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.