From: "Heiko Stübner" <heiko@sntech.de>
To: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Grant Likely <grant.likely@secretlab.ca>,
Rob Herring <rob.herring@calxeda.com>,
linux-input@vger.kernel.org, devicetree-discuss@lists.ozlabs.org
Subject: Re: [PATCH 4/4] Input: auo_pixcir_ts - add devicetree support
Date: Mon, 25 Feb 2013 14:47:17 +0100 [thread overview]
Message-ID: <201302251447.18385.heiko@sntech.de> (raw)
In-Reply-To: <20130225030641.GA18433@core.coreip.homeip.net>
Am Montag, 25. Februar 2013, 04:06:41 schrieb Dmitry Torokhov:
> On Sun, Feb 24, 2013 at 10:58:00AM +0100, Heiko Stübner wrote:
> > Hi Dmitry,
> >
> > Am Sonntag, 24. Februar 2013, 09:00:15 schrieb Dmitry Torokhov:
> > > Hi Heiko,
> > >
> > > On Sat, Feb 23, 2013 at 12:58:16PM +0100, Heiko Stübner wrote:
> > > > +static struct auo_pixcir_ts_platdata *auo_pixcir_parse_dt(struct
> > > > device *dev) +{
> > > > + struct auo_pixcir_ts_platdata *pdata = NULL;
> > > > +
> > > > +#ifdef CONFIG_OF
> > > > + struct device_node *np = dev->of_node;
> > > > +
> > > > + if (!np)
> > > > + return NULL;
> > > > +
> > > > + pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
> > > > + if (!pdata) {
> > > > + dev_err(dev, "failed to allocate platform data\n");
> > > > + return NULL;
> > > > + }
> > >
> > > I disike #ifdefs in the middle of the code, also it would be nice if we
> > > signal the proper error instead of always using -EINVAL when there are
> > > issues with platform/DT data.
> > >
> > > How about the version of the patch below?
> >
> > I tested it and everything of course still works :-) .
>
> OK, great, then I will queue these for the next merge window.
>
> Could you also try this patch (it however needs attached patch enhancing
> devres to support custom actions).
>
> Thanks.
In general looks really nice and also works. I have some small nitpicks (patch
desc, reset gpio naming) and it will also need to use
devm_request_threaded_irq to get the irq correctly freed on removal. [all also
marked at the corresponding places below]
Otherwise:
Tested-by: Heiko Stuebner <heiko@sntech.de>
Heiko
> Input: auo-pixer-ts - switch to using managed resources
>
> From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>
> This simplifier error unwinding and device teardown.
^^ simplifies
>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>
> drivers/input/touchscreen/auo-pixcir-ts.c | 162
> ++++++++++++----------------- 1 file changed, 68 insertions(+), 94
> deletions(-)
>
> diff --git a/drivers/input/touchscreen/auo-pixcir-ts.c
> b/drivers/input/touchscreen/auo-pixcir-ts.c index dfa6d54..f4ba6ffa 100644
> --- a/drivers/input/touchscreen/auo-pixcir-ts.c
> +++ b/drivers/input/touchscreen/auo-pixcir-ts.c
> @@ -533,13 +533,21 @@ static struct auo_pixcir_ts_platdata
> *auo_pixcir_parse_dt(struct device *dev)
>
> }
> #endif
>
> +static void auo_pixcir_reset(void *data)
> +{
> + struct auo_pixcir_ts *ts = data;
> +
> + gpio_set_value(ts->pdata->gpio_rst, 0);
> +}
> +
>
> static int auo_pixcir_probe(struct i2c_client *client,
>
> - const struct i2c_device_id *id)
> + const struct i2c_device_id *id)
>
> {
>
> const struct auo_pixcir_ts_platdata *pdata;
> struct auo_pixcir_ts *ts;
> struct input_dev *input_dev;
>
> - int ret;
> + int version;
> + int error;
>
> pdata = dev_get_platdata(&client->dev);
> if (!pdata) {
>
> @@ -548,60 +556,30 @@ static int auo_pixcir_probe(struct i2c_client
> *client,
>
> return PTR_ERR(pdata);
>
> }
>
> - ts = kzalloc(sizeof(struct auo_pixcir_ts), GFP_KERNEL);
> + ts = devm_kzalloc(&client->dev,
> + sizeof(struct auo_pixcir_ts), GFP_KERNEL);
>
> if (!ts)
>
> return -ENOMEM;
>
> - ret = gpio_request(pdata->gpio_int, "auo_pixcir_ts_int");
> - if (ret) {
> - dev_err(&client->dev, "request of gpio %d failed, %d\n",
> - pdata->gpio_int, ret);
> - goto err_gpio_int;
> - }
> -
> - ret = gpio_direction_input(pdata->gpio_int);
> - if (ret) {
> - dev_err(&client->dev, "setting direction of gpio %d failed
> %d\n", - pdata->gpio_int, ret);
> - goto err_gpio_dir;
> - }
> -
> - ret = gpio_request(pdata->gpio_rst, "auo_pixcir_ts_rst");
> - if (ret) {
> - dev_err(&client->dev, "request of gpio %d failed, %d\n",
> - pdata->gpio_rst, ret);
> - goto err_gpio_dir;
> - }
> -
> - ret = gpio_direction_output(pdata->gpio_rst, 1);
> - if (ret) {
> - dev_err(&client->dev, "setting direction of gpio %d failed
> %d\n", - pdata->gpio_rst, ret);
> - goto err_gpio_rst;
> + input_dev = devm_input_allocate_device(&client->dev);
> + if (!input_dev) {
> + dev_err(&client->dev, "could not allocate input device\n");
> + return -ENOMEM;
>
> }
>
> - msleep(200);
> -
>
> ts->pdata = pdata;
> ts->client = client;
>
> + ts->input = input_dev;
>
> ts->touch_ind_mode = 0;
>
> + ts->stopped = true;
>
> init_waitqueue_head(&ts->wait);
>
> snprintf(ts->phys, sizeof(ts->phys),
>
> "%s/input0", dev_name(&client->dev));
>
> - input_dev = input_allocate_device();
> - if (!input_dev) {
> - dev_err(&client->dev, "could not allocate input device\n");
> - goto err_input_alloc;
> - }
> -
> - ts->input = input_dev;
> -
>
> input_dev->name = "AUO-Pixcir touchscreen";
> input_dev->phys = ts->phys;
> input_dev->id.bustype = BUS_I2C;
>
> - input_dev->dev.parent = &client->dev;
>
> input_dev->open = auo_pixcir_input_open;
> input_dev->close = auo_pixcir_input_close;
>
> @@ -626,72 +604,69 @@ static int auo_pixcir_probe(struct i2c_client
> *client,
>
> AUO_PIXCIR_MAX_AREA, 0, 0);
>
> input_set_abs_params(input_dev, ABS_MT_ORIENTATION, 0, 1, 0, 0);
>
> - ret = i2c_smbus_read_byte_data(client, AUO_PIXCIR_REG_VERSION);
> - if (ret < 0)
> - goto err_fw_vers;
> - dev_info(&client->dev, "firmware version 0x%X\n", ret);
> -
> - ret = auo_pixcir_int_config(ts, pdata->int_setting);
> - if (ret)
> - goto err_fw_vers;
> -
>
> input_set_drvdata(ts->input, ts);
>
> - ts->stopped = true;
>
> - ret = request_threaded_irq(client->irq, NULL, auo_pixcir_interrupt,
> - IRQF_TRIGGER_RISING | IRQF_ONESHOT,
> - input_dev->name, ts);
> - if (ret) {
> - dev_err(&client->dev, "irq %d requested failed\n",
> client->irq); - goto err_fw_vers;
> + error = devm_gpio_request_one(&client->dev, pdata->gpio_int,
> + GPIOF_DIR_IN, "auo_pixcir_ts_int");
> + if (error) {
> + dev_err(&client->dev, "request of gpio %d failed, %d\n",
> + pdata->gpio_int, error);
> + return error;
>
> }
>
> - /* stop device and put it into deep sleep until it is opened */
> - ret = auo_pixcir_stop(ts);
> - if (ret < 0)
> - goto err_input_register;
> -
> - ret = input_register_device(input_dev);
> - if (ret) {
> - dev_err(&client->dev, "could not register input device\n");
> - goto err_input_register;
> + error = devm_gpio_request_one(&client->dev, pdata->gpio_rst,
> + GPIOF_DIR_OUT | GPIOF_INIT_HIGH,
> + "auo_pixcir_ts_int");
^^ auo_pixcir_ts_rst
> + if (error) {
> + dev_err(&client->dev, "request of gpio %d failed, %d\n",
> + pdata->gpio_rst, error);
> + return error;
>
> }
>
> - i2c_set_clientdata(client, ts);
> -
> - return 0;
> -
> -err_input_register:
> - free_irq(client->irq, ts);
> -err_fw_vers:
> - input_free_device(input_dev);
> -err_input_alloc:
> - gpio_set_value(pdata->gpio_rst, 0);
> -err_gpio_rst:
> - gpio_free(pdata->gpio_rst);
> -err_gpio_dir:
> - gpio_free(pdata->gpio_int);
> -err_gpio_int:
> - kfree(ts);
> + error = devm_add_action(&client->dev, auo_pixcir_reset, ts);
> + if (error) {
> + auo_pixcir_reset(ts);
> + dev_err(&client->dev, "failed to register xxx action,
> %d\n", + error);
> + return error;
> + }
>
> - return ret;
> -}
> + msleep(200);
>
> -static int auo_pixcir_remove(struct i2c_client *client)
> -{
> - struct auo_pixcir_ts *ts = i2c_get_clientdata(client);
> - const struct auo_pixcir_ts_platdata *pdata = ts->pdata;
> + version = i2c_smbus_read_byte_data(client, AUO_PIXCIR_REG_VERSION);
> + if (version < 0) {
> + error = version;
> + return error;
> + }
>
> - free_irq(client->irq, ts);
> + dev_info(&client->dev, "firmware version 0x%X\n", version);
>
> - input_unregister_device(ts->input);
> + error = auo_pixcir_int_config(ts, pdata->int_setting);
> + if (error)
> + return error;
>
> - gpio_set_value(pdata->gpio_rst, 0);
> - gpio_free(pdata->gpio_rst);
> + error = request_threaded_irq(client->irq, NULL,
+ error = devm_request_threaded_irq(&client->dev, client->irq, NULL,
> auo_pixcir_interrupt, +
> IRQF_TRIGGER_RISING | IRQF_ONESHOT, +
> input_dev->name, ts);
> + if (error) {
> + dev_err(&client->dev, "irq %d requested failed, %d\n",
> + client->irq, error);
> + return error;
> + }
>
> - gpio_free(pdata->gpio_int);
> + /* stop device and put it into deep sleep until it is opened */
> + error = auo_pixcir_stop(ts);
> + if (error)
> + return error;
> +
> + error = input_register_device(input_dev);
> + if (error) {
> + dev_err(&client->dev, "could not register input device,
> %d\n", + error);
> + return error;
> + }
>
> - kfree(ts);
> + i2c_set_clientdata(client, ts);
>
> return 0;
>
> }
>
> @@ -718,7 +693,6 @@ static struct i2c_driver auo_pixcir_driver = {
>
> .of_match_table = of_match_ptr(auo_pixcir_ts_dt_idtable),
>
> },
> .probe = auo_pixcir_probe,
>
> - .remove = auo_pixcir_remove,
>
> .id_table = auo_pixcir_idtable,
>
> };
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
next prev parent reply other threads:[~2013-02-25 13:47 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-02-23 11:55 [PATCH 0/4] Input: auo_pixcir_ts - add devicetree support Heiko Stübner
[not found] ` <201302231255.23772.heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org>
2013-02-23 11:56 ` [PATCH 1/4] Input: auo-pixcir-ts - set input direction for interrupt gpio Heiko Stübner
2013-02-23 11:56 ` [PATCH 2/4] Input: auo-pixcir-ts - handle reset gpio directly Heiko Stübner
2013-02-23 11:57 ` [PATCH 3/4] Input: auo_pixcir_ts - keep own pointer to platform_data Heiko Stübner
2013-02-23 11:58 ` [PATCH 4/4] Input: auo_pixcir_ts - add devicetree support Heiko Stübner
2013-02-24 8:00 ` Dmitry Torokhov
[not found] ` <20130224080015.GA20590-WlK9ik9hQGAhIp7JRqBPierSzoNAToWh@public.gmane.org>
2013-02-24 9:58 ` Heiko Stübner
2013-02-25 3:06 ` Dmitry Torokhov
2013-02-25 13:47 ` Heiko Stübner [this message]
2013-02-26 7:05 ` Dmitry Torokhov
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=201302251447.18385.heiko@sntech.de \
--to=heiko@sntech.de \
--cc=devicetree-discuss@lists.ozlabs.org \
--cc=dmitry.torokhov@gmail.com \
--cc=grant.likely@secretlab.ca \
--cc=linux-input@vger.kernel.org \
--cc=rob.herring@calxeda.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.