lkml.org 
[lkml]   [2016]   [Sep]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 4/5] input: touchscreen: support Allwinner SoCs' touchscreen
    Hi Quentin,

    On Sat, Sep 24, 2016 at 08:26:08PM +0200, Quentin Schulz wrote:
    > 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@free-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.

    So that you release all resources in the same order they were allocated.
    When mixing devm and non-devm allocation/release order is often
    incorrect.

    >
    > [...]
    > >> +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?

    Yes. Sometimes you can get away with mixing style (you have all devm
    resources allocated first, then non-devm), but it is much clearer and
    safer if you use one style or another exclusively.

    > Do you mean input device's close will always be called when
    > unloading/unbinding the driver?

    If ->open() has been called() then input core will ensure that ->close()
    is called as part of input_unregister_device(). If ->open() has not been
    called, then ->close() will not be called either.

    Thanks.

    --
    Dmitry

    \
     
     \ /
      Last update: 2016-09-24 20:40    [W:2.871 / U:0.084 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site