Re: [PATCH v1 2/2] iio: adc: add ADC driver for the TI TSC2046 controller

[Andy Shevchenko]

On Tue, Mar 9, 2021 at 2:18 PM Oleksij Rempel <o.rempel@xxxxxxxxxxxxxx> wrote:
>
> On Tue, Mar 09, 2021 at 01:46:55PM +0200, Andy Shevchenko wrote:
> > On Tue, Mar 9, 2021 at 1:42 PM Oleksij Rempel <o.rempel@xxxxxxxxxxxxxx> wrote:
> > > On Tue, Mar 09, 2021 at 01:05:27PM +0200, Andy Shevchenko wrote:
> > > > On Fri, Mar 5, 2021 at 9:05 PM Jonathan Cameron
> > > > <Jonathan.Cameron@xxxxxxxxxx> wrote:
> > > > >
> > > > > On Fri, 5 Mar 2021 14:38:13 +0100
> > > > > Oleksij Rempel <o.rempel@xxxxxxxxxxxxxx> wrote:
> > > > >
> > > > > > Basically the TI TSC2046 touchscreen controller is 8 channel ADC optimized for
> > > > > > the touchscreen use case. By implementing it as IIO ADC device, we can
> > > > > > make use of resistive-adc-touch and iio-hwmon drivers.
> > > > > >
> > > > > > So far, this driver was tested with custom version of resistive-adc-touch driver,
> > > > > > since it need to be extended to make use of Z1 and Z2 channels. The X/Y
> > > > > > are working without additional changes.
> > > > > >
> > > > > > Signed-off-by: Oleksij Rempel <o.rempel@xxxxxxxxxxxxxx>
> > > > >
> > > > > Hi Oleksij,
> > > > >
> > > > > To consider this as a possible long term route instead of just making this
> > > > > a touchscreen driver, we'll want to see those mods to the resistive-adc-touch.
> > > > > Of course that doesn't stop review of this in the meantime.
> > > > >
> > > > > There are quite a few things in here that feel pretty specific to the touchscreen
> > > > > usecase. That makes me wonder if this is a sensible approach or not.
> > > >
> > > > I'm wondering if this has any similarities with existing drivers under
> > > > drivers/input/touchscreen.
> > >
> > > Yes, for example: drivers/input/touchscreen/ads7846.c
> >
> > Then I have a few questions here:
> > 1/ why the above mentioned driver can't be extended to cover this?
>
> It is not possible to keep old device tree binding compatible with the
> new driver at least not for currently existing abstraction: ADC +
> touchscreen node.
>
> It is too expensive to overwrite the old driver, we do not have enough time and
> resource to do it. I lardy spend some weeks to do it and I would need a
> many more weeks to make it by tiny slices without solving actual
> problem. Many resistive touchscreen driver should share a lot of code.
>
> Since there is already existing IIO based components, it seems to me
> better to spend available resource and making it properly in a way,
> which reflect modern best practices.
>
> > 2/ or why is the proposed driver named after the touchscreen instead
> > of the real AD/C chip behind it?
>
> I do not understand this question. The proposed driver is named after
> the chip which provides ADC functionality, In this case, it is TSC2046.
> The touchscreen is a separate physical module.
>
> The idea of this proposition is to keep physically separate components
> separately on the kernel side.
>
> > 3/ maybe we can introduce a simple AD/C driver under IIO for that?
>
> There are already simple ADC drivers for that:
> iio-hwmon: drivers/hwmon/iio_hwmon.c
> resistive-adc-touch: drivers/input/touchscreen/resistive-adc-touch.c
>
> This two driver + the proposed one, will replace functionality of ads7846.c

Okay, then maybe you can elaborate all this in the cover letter to
make sure that maintainers will know why the new driver appeared
instead of modifications to the old one.

-- 
With Best Regards,
Andy Shevchenko