This simplifies error handling in probe() and reduces amount of explicit
code in remove().
Signed-off-by: Dmitry Torokhov <[email protected]>
---
drivers/input/keyboard/adp5588-keys.c | 111 +++++++++++---------------
1 file changed, 45 insertions(+), 66 deletions(-)
diff --git a/drivers/input/keyboard/adp5588-keys.c b/drivers/input/keyboard/adp5588-keys.c
index ac21873ba1d7..df84a2998ed2 100644
--- a/drivers/input/keyboard/adp5588-keys.c
+++ b/drivers/input/keyboard/adp5588-keys.c
@@ -182,6 +182,21 @@ static int adp5588_build_gpiomap(struct adp5588_kpad *kpad,
return n_unused;
}
+static void adp5588_gpio_do_teardown(void *_kpad)
+{
+ struct adp5588_kpad *kpad = _kpad;
+ struct device *dev = &kpad->client->dev;
+ const struct adp5588_kpad_platform_data *pdata = dev_get_platdata(dev);
+ const struct adp5588_gpio_platform_data *gpio_data = pdata->gpio_data;
+ int error;
+
+ error = gpio_data->teardown(kpad->client,
+ kpad->gc.base, kpad->gc.ngpio,
+ gpio_data->context);
+ if (error)
+ dev_warn(&kpad->client->dev, "teardown failed %d\n", error);
+}
+
static int adp5588_gpio_add(struct adp5588_kpad *kpad)
{
struct device *dev = &kpad->client->dev;
@@ -198,8 +213,6 @@ static int adp5588_gpio_add(struct adp5588_kpad *kpad)
return 0;
}
- kpad->export_gpio = true;
-
kpad->gc.direction_input = adp5588_gpio_direction_input;
kpad->gc.direction_output = adp5588_gpio_direction_output;
kpad->gc.get = adp5588_gpio_get_value;
@@ -213,9 +226,9 @@ static int adp5588_gpio_add(struct adp5588_kpad *kpad)
mutex_init(&kpad->gpio_lock);
- error = gpiochip_add_data(&kpad->gc, kpad);
+ error = devm_gpiochip_add_data(dev, &kpad->gc, kpad);
if (error) {
- dev_err(dev, "gpiochip_add failed, err: %d\n", error);
+ dev_err(dev, "gpiochip_add failed: %d\n", error);
return error;
}
@@ -230,41 +243,24 @@ static int adp5588_gpio_add(struct adp5588_kpad *kpad)
kpad->gc.base, kpad->gc.ngpio,
gpio_data->context);
if (error)
- dev_warn(dev, "setup failed, %d\n", error);
+ dev_warn(dev, "setup failed: %d\n", error);
}
- return 0;
-}
-
-static void adp5588_gpio_remove(struct adp5588_kpad *kpad)
-{
- struct device *dev = &kpad->client->dev;
- const struct adp5588_kpad_platform_data *pdata = dev_get_platdata(dev);
- const struct adp5588_gpio_platform_data *gpio_data = pdata->gpio_data;
- int error;
-
- if (!kpad->export_gpio)
- return;
-
if (gpio_data->teardown) {
- error = gpio_data->teardown(kpad->client,
- kpad->gc.base, kpad->gc.ngpio,
- gpio_data->context);
+ error = devm_add_action(dev, adp5588_gpio_do_teardown, kpad);
if (error)
- dev_warn(dev, "teardown failed %d\n", error);
+ dev_warn(dev, "failed to schedule teardown: %d\n",
+ error);
}
- gpiochip_remove(&kpad->gc);
+ return 0;
}
+
#else
static inline int adp5588_gpio_add(struct adp5588_kpad *kpad)
{
return 0;
}
-
-static inline void adp5588_gpio_remove(struct adp5588_kpad *kpad)
-{
-}
#endif
static void adp5588_report_events(struct adp5588_kpad *kpad, int ev_cnt)
@@ -510,21 +506,20 @@ static int adp5588_probe(struct i2c_client *client,
return -EINVAL;
}
- kpad = kzalloc(sizeof(*kpad), GFP_KERNEL);
- input = input_allocate_device();
- if (!kpad || !input) {
- error = -ENOMEM;
- goto err_free_mem;
- }
+ kpad = devm_kzalloc(&client->dev, sizeof(*kpad), GFP_KERNEL);
+ if (!kpad)
+ return -ENOMEM;
+
+ input = devm_input_allocate_device(&client->dev);
+ if (!input)
+ return -ENOMEM;
kpad->client = client;
kpad->input = input;
ret = adp5588_read(client, DEV_ID);
- if (ret < 0) {
- error = ret;
- goto err_free_mem;
- }
+ if (ret < 0)
+ return ret;
revid = (u8) ret & ADP5588_DEVICE_ID_MASK;
if (WA_DELAYED_READOUT_REVID(revid))
@@ -532,7 +527,6 @@ static int adp5588_probe(struct i2c_client *client,
input->name = client->name;
input->phys = "adp5588-keys/input0";
- input->dev.parent = &client->dev;
input_set_drvdata(input, kpad);
@@ -569,58 +563,43 @@ static int adp5588_probe(struct i2c_client *client,
error = input_register_device(input);
if (error) {
- dev_err(&client->dev, "unable to register input device\n");
- goto err_free_mem;
+ dev_err(&client->dev, "unable to register input device: %d\n",
+ error);
+ return error;
}
- error = request_threaded_irq(client->irq,
- adp5588_hard_irq, adp5588_thread_irq,
- IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
- client->dev.driver->name, kpad);
+ error = devm_request_threaded_irq(&client->dev, client->irq,
+ adp5588_hard_irq, adp5588_thread_irq,
+ IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
+ client->dev.driver->name, kpad);
if (error) {
- dev_err(&client->dev, "irq %d busy?\n", client->irq);
- goto err_unreg_dev;
+ dev_err(&client->dev, "failed to request irq %d: %d\n",
+ client->irq, error);
+ return error;
}
error = adp5588_setup(client);
if (error)
- goto err_free_irq;
+ return error;
if (kpad->gpimapsize)
adp5588_report_switch_state(kpad);
error = adp5588_gpio_add(kpad);
if (error)
- goto err_free_irq;
+ return error;
device_init_wakeup(&client->dev, 1);
- i2c_set_clientdata(client, kpad);
dev_info(&client->dev, "Rev.%d keypad, irq %d\n", revid, client->irq);
return 0;
-
- err_free_irq:
- free_irq(client->irq, kpad);
- err_unreg_dev:
- input_unregister_device(input);
- input = NULL;
- err_free_mem:
- input_free_device(input);
- kfree(kpad);
-
- return error;
}
static int adp5588_remove(struct i2c_client *client)
{
- struct adp5588_kpad *kpad = i2c_get_clientdata(client);
-
adp5588_write(client, CFG, 0);
- free_irq(client->irq, kpad);
- input_unregister_device(kpad->input);
- adp5588_gpio_remove(kpad);
- kfree(kpad);
+ /* all resources will be freed by devm */
return 0;
}
--
2.36.1.124.g0e6072fb45-goog
On Fri, May 27, 2022 at 09:56:30PM -0700, Dmitry Torokhov wrote:
> This simplifies error handling in probe() and reduces amount of explicit
> code in remove().
>
> Signed-off-by: Dmitry Torokhov <[email protected]>
> ---
> drivers/input/keyboard/adp5588-keys.c | 111 +++++++++++---------------
> 1 file changed, 45 insertions(+), 66 deletions(-)
>
> diff --git a/drivers/input/keyboard/adp5588-keys.c b/drivers/input/keyboard/adp5588-keys.c
> index ac21873ba1d7..df84a2998ed2 100644
> --- a/drivers/input/keyboard/adp5588-keys.c
> +++ b/drivers/input/keyboard/adp5588-keys.c
> @@ -182,6 +182,21 @@ static int adp5588_build_gpiomap(struct adp5588_kpad *kpad,
> return n_unused;
> }
>
> +static void adp5588_gpio_do_teardown(void *_kpad)
> +{
> + struct adp5588_kpad *kpad = _kpad;
> + struct device *dev = &kpad->client->dev;
> + const struct adp5588_kpad_platform_data *pdata = dev_get_platdata(dev);
> + const struct adp5588_gpio_platform_data *gpio_data = pdata->gpio_data;
> + int error;
> +
> + error = gpio_data->teardown(kpad->client,
> + kpad->gc.base, kpad->gc.ngpio,
> + gpio_data->context);
> + if (error)
> + dev_warn(&kpad->client->dev, "teardown failed %d\n", error);
> +}
I think the more sensible approach is to drop support for setup and
teardown. Maybe even rip all usage of adp5588_gpio_platform_data.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |
On Sat, May 28, 2022 at 09:37:55PM +0200, Uwe Kleine-K?nig wrote:
> On Fri, May 27, 2022 at 09:56:30PM -0700, Dmitry Torokhov wrote:
> > This simplifies error handling in probe() and reduces amount of explicit
> > code in remove().
> >
> > Signed-off-by: Dmitry Torokhov <[email protected]>
> > ---
> > drivers/input/keyboard/adp5588-keys.c | 111 +++++++++++---------------
> > 1 file changed, 45 insertions(+), 66 deletions(-)
> >
> > diff --git a/drivers/input/keyboard/adp5588-keys.c b/drivers/input/keyboard/adp5588-keys.c
> > index ac21873ba1d7..df84a2998ed2 100644
> > --- a/drivers/input/keyboard/adp5588-keys.c
> > +++ b/drivers/input/keyboard/adp5588-keys.c
> > @@ -182,6 +182,21 @@ static int adp5588_build_gpiomap(struct adp5588_kpad *kpad,
> > return n_unused;
> > }
> >
> > +static void adp5588_gpio_do_teardown(void *_kpad)
> > +{
> > + struct adp5588_kpad *kpad = _kpad;
> > + struct device *dev = &kpad->client->dev;
> > + const struct adp5588_kpad_platform_data *pdata = dev_get_platdata(dev);
> > + const struct adp5588_gpio_platform_data *gpio_data = pdata->gpio_data;
> > + int error;
> > +
> > + error = gpio_data->teardown(kpad->client,
> > + kpad->gc.base, kpad->gc.ngpio,
> > + gpio_data->context);
> > + if (error)
> > + dev_warn(&kpad->client->dev, "teardown failed %d\n", error);
> > +}
>
> I think the more sensible approach is to drop support for setup and
> teardown. Maybe even rip all usage of adp5588_gpio_platform_data.
That will come with the transition to device tree/device properties. For
now wanted to keep existing functionality intact.
Thanks.
--
Dmitry
On Sat, May 28, 2022 at 10:22:07PM -0700, Dmitry Torokhov wrote:
> On Sat, May 28, 2022 at 09:37:55PM +0200, Uwe Kleine-K?nig wrote:
> > On Fri, May 27, 2022 at 09:56:30PM -0700, Dmitry Torokhov wrote:
> > > This simplifies error handling in probe() and reduces amount of explicit
> > > code in remove().
> > >
> > > Signed-off-by: Dmitry Torokhov <[email protected]>
> > > ---
> > > drivers/input/keyboard/adp5588-keys.c | 111 +++++++++++---------------
> > > 1 file changed, 45 insertions(+), 66 deletions(-)
> > >
> > > diff --git a/drivers/input/keyboard/adp5588-keys.c b/drivers/input/keyboard/adp5588-keys.c
> > > index ac21873ba1d7..df84a2998ed2 100644
> > > --- a/drivers/input/keyboard/adp5588-keys.c
> > > +++ b/drivers/input/keyboard/adp5588-keys.c
> > > @@ -182,6 +182,21 @@ static int adp5588_build_gpiomap(struct adp5588_kpad *kpad,
> > > return n_unused;
> > > }
> > >
> > > +static void adp5588_gpio_do_teardown(void *_kpad)
> > > +{
> > > + struct adp5588_kpad *kpad = _kpad;
> > > + struct device *dev = &kpad->client->dev;
> > > + const struct adp5588_kpad_platform_data *pdata = dev_get_platdata(dev);
> > > + const struct adp5588_gpio_platform_data *gpio_data = pdata->gpio_data;
> > > + int error;
> > > +
> > > + error = gpio_data->teardown(kpad->client,
> > > + kpad->gc.base, kpad->gc.ngpio,
> > > + gpio_data->context);
> > > + if (error)
> > > + dev_warn(&kpad->client->dev, "teardown failed %d\n", error);
> > > +}
> >
> > I think the more sensible approach is to drop support for setup and
> > teardown. Maybe even rip all usage of adp5588_gpio_platform_data.
>
> That will come with the transition to device tree/device properties. For
> now wanted to keep existing functionality intact.
That's up to you. I wouldn't spend an effort to clean up a feature that
is about to be removed.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |
> -----Original Message-----
> From: Dmitry Torokhov <[email protected]>
> Sent: Samstag, 28. Mai 2022 06:57
> To: Hennerich, Michael <[email protected]>
> Cc: Uwe Kleine-K?nig <[email protected]>; linux-
> [email protected]; [email protected]
> Subject: [PATCH 3/4] Input: adp5588-keys - switch to using managed resources
>
>
> This simplifies error handling in probe() and reduces amount of explicit code in
> remove().
>
> Signed-off-by: Dmitry Torokhov <[email protected]>
Acked-by: Michael Hennerich <[email protected]>
> ---
> drivers/input/keyboard/adp5588-keys.c | 111 +++++++++++---------------
> 1 file changed, 45 insertions(+), 66 deletions(-)
>
> diff --git a/drivers/input/keyboard/adp5588-keys.c
> b/drivers/input/keyboard/adp5588-keys.c
> index ac21873ba1d7..df84a2998ed2 100644
> --- a/drivers/input/keyboard/adp5588-keys.c
> +++ b/drivers/input/keyboard/adp5588-keys.c
> @@ -182,6 +182,21 @@ static int adp5588_build_gpiomap(struct
> adp5588_kpad *kpad,
> return n_unused;
> }
>
> +static void adp5588_gpio_do_teardown(void *_kpad) {
> + struct adp5588_kpad *kpad = _kpad;
> + struct device *dev = &kpad->client->dev;
> + const struct adp5588_kpad_platform_data *pdata =
> dev_get_platdata(dev);
> + const struct adp5588_gpio_platform_data *gpio_data = pdata-
> >gpio_data;
> + int error;
> +
> + error = gpio_data->teardown(kpad->client,
> + kpad->gc.base, kpad->gc.ngpio,
> + gpio_data->context);
> + if (error)
> + dev_warn(&kpad->client->dev, "teardown failed %d\n", error);
> }
> +
> static int adp5588_gpio_add(struct adp5588_kpad *kpad) {
> struct device *dev = &kpad->client->dev; @@ -198,8 +213,6 @@ static
> int adp5588_gpio_add(struct adp5588_kpad *kpad)
> return 0;
> }
>
> - kpad->export_gpio = true;
> -
> kpad->gc.direction_input = adp5588_gpio_direction_input;
> kpad->gc.direction_output = adp5588_gpio_direction_output;
> kpad->gc.get = adp5588_gpio_get_value; @@ -213,9 +226,9 @@ static
> int adp5588_gpio_add(struct adp5588_kpad *kpad)
>
> mutex_init(&kpad->gpio_lock);
>
> - error = gpiochip_add_data(&kpad->gc, kpad);
> + error = devm_gpiochip_add_data(dev, &kpad->gc, kpad);
> if (error) {
> - dev_err(dev, "gpiochip_add failed, err: %d\n", error);
> + dev_err(dev, "gpiochip_add failed: %d\n", error);
> return error;
> }
>
> @@ -230,41 +243,24 @@ static int adp5588_gpio_add(struct adp5588_kpad
> *kpad)
> kpad->gc.base, kpad->gc.ngpio,
> gpio_data->context);
> if (error)
> - dev_warn(dev, "setup failed, %d\n", error);
> + dev_warn(dev, "setup failed: %d\n", error);
> }
>
> - return 0;
> -}
> -
> -static void adp5588_gpio_remove(struct adp5588_kpad *kpad) -{
> - struct device *dev = &kpad->client->dev;
> - const struct adp5588_kpad_platform_data *pdata =
> dev_get_platdata(dev);
> - const struct adp5588_gpio_platform_data *gpio_data = pdata-
> >gpio_data;
> - int error;
> -
> - if (!kpad->export_gpio)
> - return;
> -
> if (gpio_data->teardown) {
> - error = gpio_data->teardown(kpad->client,
> - kpad->gc.base, kpad->gc.ngpio,
> - gpio_data->context);
> + error = devm_add_action(dev, adp5588_gpio_do_teardown,
> kpad);
> if (error)
> - dev_warn(dev, "teardown failed %d\n", error);
> + dev_warn(dev, "failed to schedule teardown: %d\n",
> + error);
> }
>
> - gpiochip_remove(&kpad->gc);
> + return 0;
> }
> +
> #else
> static inline int adp5588_gpio_add(struct adp5588_kpad *kpad) {
> return 0;
> }
> -
> -static inline void adp5588_gpio_remove(struct adp5588_kpad *kpad) -{ -}
> #endif
>
> static void adp5588_report_events(struct adp5588_kpad *kpad, int ev_cnt)
> @@ -510,21 +506,20 @@ static int adp5588_probe(struct i2c_client *client,
> return -EINVAL;
> }
>
> - kpad = kzalloc(sizeof(*kpad), GFP_KERNEL);
> - input = input_allocate_device();
> - if (!kpad || !input) {
> - error = -ENOMEM;
> - goto err_free_mem;
> - }
> + kpad = devm_kzalloc(&client->dev, sizeof(*kpad), GFP_KERNEL);
> + if (!kpad)
> + return -ENOMEM;
> +
> + input = devm_input_allocate_device(&client->dev);
> + if (!input)
> + return -ENOMEM;
>
> kpad->client = client;
> kpad->input = input;
>
> ret = adp5588_read(client, DEV_ID);
> - if (ret < 0) {
> - error = ret;
> - goto err_free_mem;
> - }
> + if (ret < 0)
> + return ret;
>
> revid = (u8) ret & ADP5588_DEVICE_ID_MASK;
> if (WA_DELAYED_READOUT_REVID(revid))
> @@ -532,7 +527,6 @@ static int adp5588_probe(struct i2c_client *client,
>
> input->name = client->name;
> input->phys = "adp5588-keys/input0";
> - input->dev.parent = &client->dev;
>
> input_set_drvdata(input, kpad);
>
> @@ -569,58 +563,43 @@ static int adp5588_probe(struct i2c_client *client,
>
> error = input_register_device(input);
> if (error) {
> - dev_err(&client->dev, "unable to register input device\n");
> - goto err_free_mem;
> + dev_err(&client->dev, "unable to register input device: %d\n",
> + error);
> + return error;
> }
>
> - error = request_threaded_irq(client->irq,
> - adp5588_hard_irq, adp5588_thread_irq,
> - IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> - client->dev.driver->name, kpad);
> + error = devm_request_threaded_irq(&client->dev, client->irq,
> + adp5588_hard_irq,
> adp5588_thread_irq,
> + IRQF_TRIGGER_FALLING |
> IRQF_ONESHOT,
> + client->dev.driver->name, kpad);
> if (error) {
> - dev_err(&client->dev, "irq %d busy?\n", client->irq);
> - goto err_unreg_dev;
> + dev_err(&client->dev, "failed to request irq %d: %d\n",
> + client->irq, error);
> + return error;
> }
>
> error = adp5588_setup(client);
> if (error)
> - goto err_free_irq;
> + return error;
>
> if (kpad->gpimapsize)
> adp5588_report_switch_state(kpad);
>
> error = adp5588_gpio_add(kpad);
> if (error)
> - goto err_free_irq;
> + return error;
>
> device_init_wakeup(&client->dev, 1);
> - i2c_set_clientdata(client, kpad);
>
> dev_info(&client->dev, "Rev.%d keypad, irq %d\n", revid, client->irq);
> return 0;
> -
> - err_free_irq:
> - free_irq(client->irq, kpad);
> - err_unreg_dev:
> - input_unregister_device(input);
> - input = NULL;
> - err_free_mem:
> - input_free_device(input);
> - kfree(kpad);
> -
> - return error;
> }
>
> static int adp5588_remove(struct i2c_client *client) {
> - struct adp5588_kpad *kpad = i2c_get_clientdata(client);
> -
> adp5588_write(client, CFG, 0);
> - free_irq(client->irq, kpad);
> - input_unregister_device(kpad->input);
> - adp5588_gpio_remove(kpad);
> - kfree(kpad);
>
> + /* all resources will be freed by devm */
> return 0;
> }
>
> --
> 2.36.1.124.g0e6072fb45-goog