Hi Marco,
On 5/4/20 9:34 AM, Marco Felsch wrote:
> thanks for your patch :)
>
> On 20-04-30 19:53, Daniel Mack wrote:
>> This simplies the code a lot and fixes some potential resource leaks in
>> the error return paths. It also ensures the input device is registered
>> before the interrupt is requested, as the IRQ handler will commit events
>> when it fires.
>
> Why is it necessary to get those events during probe()? Pls, see also my
> inline comments.
My concern was that the IRQ handler starts firing as soon as the
interrupt is requested. I however just learned that this is not a
problem though. So I'll drop that change to keep the patch smaller.
Thanks for noting!
>> ---
>> drivers/input/touchscreen/ads7846.c | 137 +++++++++-------------------
>> 1 file changed, 45 insertions(+), 92 deletions(-)
>>
>> diff --git a/drivers/input/touchscreen/ads7846.c b/drivers/input/touchscreen/ads7846.c
>> index 8fd7fc39c4fd..acf736f5ddab 100644
>> --- a/drivers/input/touchscreen/ads7846.c
>> +++ b/drivers/input/touchscreen/ads7846.c
>> @@ -528,30 +528,19 @@ static int ads784x_hwmon_register(struct spi_device *spi, struct ads7846 *ts)
>> break;
>> }
>>
>> - ts->hwmon = hwmon_device_register_with_groups(&spi->dev, spi->modalias,
>> - ts, ads7846_attr_groups);
>> + ts->hwmon = devm_hwmon_device_register_with_groups(&spi->dev,
>> + spi->modalias, ts,
>> + ads7846_attr_groups);
>
> We don't need the hwmon member anymore if we are switching to the devres
> intializer. Pls, can you drop it completely?
Right. Will drop it.
>> static ssize_t ads7846_pen_down_show(struct device *dev,
>> @@ -944,8 +933,8 @@ static int ads7846_setup_pendown(struct spi_device *spi,
>> ts->get_pendown_state = pdata->get_pendown_state;
>> } else if (gpio_is_valid(pdata->gpio_pendown)) {
>>
>> - err = gpio_request_one(pdata->gpio_pendown, GPIOF_IN,
>> - "ads7846_pendown");
>> + err = devm_gpio_request_one(&spi->dev, pdata->gpio_pendown,
>> + GPIOF_IN, "ads7846_pendown");
>
> I see that you want to keep the changes minimal and I'm fine with this
> but we should considering to move the driver to gpio_desc.
Yes, we should. But I'll keep that out of this patch set.
>> + ts = devm_kzalloc(dev, sizeof(struct ads7846), GFP_KERNEL);
>> + packet = devm_kzalloc(dev, sizeof(struct ads7846_packet), GFP_KERNEL);
>> + input_dev = devm_input_allocate_device(dev);
>> + if (!ts || !packet || !input_dev)
>> + return -ENOMEM;
>
> Pls, can we split that so each alloc get its own error check?
Okay.
>> input_dev->name = ts->name;
>> input_dev->phys = ts->phys;
>> - input_dev->dev.parent = &spi->dev;
>> + input_dev->dev.parent = dev;
>
> I would split the dev usage into another patch since it is unrelated to
> the change you wanna make and keeps the diff smaller.
Agreed.
>> + err = input_register_device(input_dev);
>> + if (err)
>> + goto err_cleanup_filter;
>
> It seems quite common to register the device on the end.
Yes, as stated above. Reverted that change in ordering.
>> err = regulator_enable(ts->reg);
>> if (err) {
>> - dev_err(&spi->dev, "unable to enable regulator: %d\n", err);
>> - goto err_put_regulator;
>> + dev_err(dev, "unable to enable regulator: %d\n", err);
>> + goto err_cleanup_filter;
>> }
>
> From now on the regulator is on an keeps on since you never turn it of.
> So you need to add a devm_add_action_or_reset() here.
Good point. Added.
Will send a v2 soon. Thanks for the review!
Daniel
