linux-kernel - Re: [PATCH 4/5] input: touchscreen: support Allwinner SoCs' touchscreen

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ca39a2e7-6116-6f6e-3f61-78abfdefcbb7@free-electrons.com>
Date:   Sat, 24 Sep 2016 20:26:08 +0200
From:   Quentin Schulz <quentin.schulz@...e-electrons.com>
To:     Dmitry Torokhov <dmitry.torokhov@...il.com>
Cc:     jic23@...nel.org, knaack.h@....de, lars@...afoo.de,
        pmeerw@...erw.net, maxime.ripard@...e-electrons.com, wens@...e.org,
        lee.jones@...aro.org, antoine.tenart@...e-electrons.com,
        thomas.petazzoni@...e-electrons.com, linux-kernel@...r.kernel.org,
        linux-iio@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linux-input@...r.kernel.org
Subject: Re: [PATCH 4/5] input: touchscreen: support Allwinner SoCs'
 touchscreen

Hi Dimitry,

Sorry for the (long) delay, I did not have time to work on it. I'll
mainly work in my free time now.

On 20/07/2016 19:25, Dmitry Torokhov wrote:
> Hi Quentin,
> 
> On Wed, Jul 20, 2016 at 10:29:10AM +0200, Quentin Schulz wrote:
>> This adds support for Allwinner SoCs' (A10, A13 and A31) resistive
>> touchscreen. This driver is probed by the MFD sunxi-gpadc-mfd.
>>
>> This driver uses ADC channels exposed through the IIO framework by
>> sunxi-gpadc-iio to get its data. When opening this input device, it will
>> start buffering in the ADC driver and enable a TP_UP_PENDING irq. The ADC
>> driver will fill in a buffer with all data and call the callback the input
>> device associated with this buffer. The input device will then read the
>> buffer two by two and send X and Y coordinates to the input framework based
>> on what it received from the ADC's buffer. When closing this input device,
>> the buffering is stopped.
>>
>> Note that locations in the first received buffer after an TP_UP_PENDING irq
>> occurred are unreliable, thus dropped.
>>
>> Signed-off-by: Quentin Schulz <quentin.schulz@...e-electrons.com>
>> ---
[...]
>> +	info->buffer = iio_channel_get_all_cb(&pdev->dev,
>> +					      &sunxi_gpadc_ts_callback,
>> +					      (void *)info);
> 
> Any chance we could introduce devm-variant here? If you do not want to
> wait for IIO to add it you can temporarily add call
> devm_add_action_or_reset() after getting channels and remove it when IIO
> API catches up.
> 

Something like:

release_iio_channels(void* data)
{
	struct sunxi_gpadc_ts *info = data;
	iio_channel_release_all_cb(info->buffer);
}

[...]
	info->buffer = iio_channel_get_all_cb(&pdev->dev,
					      &sunxi_gpadc_ts_callback,
					      (void *)info);
	ret = devm_add_action_or_reset(&pdev->dev,
				       release_iio_channels,
				       (void *)info);
	if (ret)
		return ret;

?

May I know why you prefer that way instead of explicit removing in
remove function of the platform device? I understand for devm-variant
already in the framework but I am curious for this one.

[...]
>> +static int sunxi_gpadc_ts_remove(struct platform_device *pdev)
>> +{
>> +	struct sunxi_gpadc_ts *info = platform_get_drvdata(pdev);
>> +
>> +	iio_channel_stop_all_cb(info->buffer);
>> +	iio_channel_release_all_cb(info->buffer);
>> +
>> +	disable_irq(info->tp_up_irq);
> 
> You are mixing devm and non-devm so your unwind order is completely out
> of wack. If input device is opened while you are unloading (or
> unbinding) the dirver, then you'll release channels, then input device's
> close() will be called, which will try to stop the IIO channels again
> and disable IRQ yet again.
> 

Do you mean that I should be using exclusively devm or non-devm functions?
Do you mean input device's close will always be called when
unloading/unbinding the driver?

[...]
Thanks,
Quentin

-- 
Quentin Schulz, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com