| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 18 Jul 2013 19:17:44 +0200
From: rydberg@...omail.se
To: Nick Dyer <nick.dyer@...ev.co.uk>
Cc: Dmitry Torokhov <dmitry.torokhov@...il.com>,
Daniel Kurtz <djkurtz@...omium.org>,
Joonyoung Shim <jy0922.shim@...sung.com>,
Alan Bowens <Alan.Bowens@...el.com>,
linux-input@...r.kernel.org, linux-kernel@...r.kernel.org,
Peter Meerwald <pmeerw@...erw.net>,
Benson Leung <bleung@...omium.org>,
Olof Johansson <olofj@...omium.org>,
Yufeng Shen <miletus@...omium.org>
Subject: Re: [PATCH 20/51] Input: atmel_mxt_ts - Set default irqflags when
there is no pdata
Hi Nick,
> From: Yufeng Shen <miletus@...omium.org>
>
> This is the preparation for supporting the code path when there is
> platform data provided and still boot the device into a sane state
> with backup NVRAM config.
>
> Make the irqflags default to be IRQF_TRIGGER_FALLING if no platform data is
> provided.
>
> Signed-off-by: Yufeng Shen <miletus@...omium.org>
> Signed-off-by: Daniel Kurtz <djkurtz@...omium.org>
> Signed-off-by: Nick Dyer <nick.dyer@...ev.co.uk>
> Acked-by: Benson Leung <bleung@...omium.org>
> ---
> drivers/input/touchscreen/atmel_mxt_ts.c | 54 +++++++++++++++++++++---------
> 1 file changed, 39 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
> index 1334e5b..2645d36 100644
> --- a/drivers/input/touchscreen/atmel_mxt_ts.c
> +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
> @@ -232,7 +232,7 @@ struct mxt_data {
> struct i2c_client *client;
> struct input_dev *input_dev;
> char phys[64]; /* device physical location */
> - const struct mxt_platform_data *pdata;
> + struct mxt_platform_data *pdata;
> struct mxt_object *object_table;
> struct mxt_info info;
> unsigned int irq;
> @@ -1640,10 +1640,29 @@ static void mxt_input_close(struct input_dev *dev)
> mxt_stop(data);
> }
>
> +static int mxt_handle_pdata(struct mxt_data *data)
> +{
> + data->pdata = dev_get_platdata(&data->client->dev);
> +
> + /* Use provided platform data if present */
> + if (data->pdata)
> + return 0;
> +
> + data->pdata = kzalloc(sizeof(*data->pdata), GFP_KERNEL);
> + if (!data->pdata) {
> + dev_err(&data->client->dev, "Failed to allocate pdata\n");
> + return -ENOMEM;
> + }
Any chance this could be a static instead?
> +
> + /* Set default parameters */
> + data->pdata->irqflags = IRQF_TRIGGER_FALLING;
> +
> + return 0;
> +}
> +
Opencode instead?
> static int mxt_probe(struct i2c_client *client,
> const struct i2c_device_id *id)
> {
> - const struct mxt_platform_data *pdata = client->dev.platform_data;
This line keeps reappearing in various versions of this
function. Perhaps it should simply stay as is instead?
> struct mxt_data *data;
> struct input_dev *input_dev;
> int error;
> @@ -1651,9 +1670,6 @@ static int mxt_probe(struct i2c_client *client,
> unsigned int mt_flags = 0;
> int i;
>
> - if (!pdata)
Why not just initialize the default here instead?
> - return -EINVAL;
> -
> data = kzalloc(sizeof(struct mxt_data), GFP_KERNEL);
> input_dev = input_allocate_device();
> if (!data || !input_dev) {
> @@ -1675,19 +1691,23 @@ static int mxt_probe(struct i2c_client *client,
>
> data->client = client;
> data->input_dev = input_dev;
> - data->pdata = pdata;
> data->irq = client->irq;
> + i2c_set_clientdata(client, data);
> +
> + error = mxt_handle_pdata(data);
> + if (error)
> + goto err_free_mem;
then this would go away
>
> init_completion(&data->bl_completion);
> init_completion(&data->reset_completion);
> init_completion(&data->crc_completion);
>
> - error = request_threaded_irq(client->irq, NULL, mxt_interrupt,
> - pdata->irqflags | IRQF_ONESHOT,
> + error = request_threaded_irq(data->irq, NULL, mxt_interrupt,
> + data->pdata->irqflags | IRQF_ONESHOT,
> client->name, data);
and this hunk
> if (error) {
> dev_err(&client->dev, "Failed to register interrupt\n");
> - goto err_free_mem;
> + goto err_free_pdata;
> }
>
> disable_irq(client->irq);
> @@ -1700,13 +1720,13 @@ static int mxt_probe(struct i2c_client *client,
> __set_bit(EV_KEY, input_dev->evbit);
> __set_bit(BTN_TOUCH, input_dev->keybit);
>
> - if (pdata->t19_num_keys) {
> + if (data->pdata->t19_num_keys) {
and this hunk
> __set_bit(INPUT_PROP_BUTTONPAD, input_dev->propbit);
>
> - for (i = 0; i < pdata->t19_num_keys; i++)
> - if (pdata->t19_keymap[i] != KEY_RESERVED)
> + for (i = 0; i < data->pdata->t19_num_keys; i++)
> + if (data->pdata->t19_keymap[i] != KEY_RESERVED)
> input_set_capability(input_dev, EV_KEY,
> - pdata->t19_keymap[i]);
> + data->pdata->t19_keymap[i]);
>
> mt_flags |= INPUT_MT_POINTER;
>
> @@ -1743,7 +1763,6 @@ static int mxt_probe(struct i2c_client *client,
> 0, 255, 0, 0);
>
> input_set_drvdata(input_dev, data);
> - i2c_set_clientdata(client, data);
>
> error = input_register_device(input_dev);
> if (error) {
> @@ -1767,7 +1786,10 @@ err_unregister_device:
> err_free_object:
> kfree(data->object_table);
> err_free_irq:
> - free_irq(client->irq, data);
> + free_irq(data->irq, data);
> +err_free_pdata:
> + if (!dev_get_platdata(&data->client->dev))
> + kfree(data->pdata);
> err_free_mem:
> input_free_device(input_dev);
> kfree(data);
> @@ -1782,6 +1804,8 @@ static int mxt_remove(struct i2c_client *client)
> free_irq(data->irq, data);
> input_unregister_device(data->input_dev);
> kfree(data->object_table);
> + if (!dev_get_platdata(&data->client->dev))
> + kfree(data->pdata);
Shared ownership should perhaps be signalled in a more robust way?
> kfree(data);
>
> return 0;
> --
> 1.7.10.4
>
Thanks,
Henrik
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists