Re: [PATCH 2/3] tps6507x-ts: add DT support

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

On Tue, Feb 28, 2017 at 03:36:26PM +0530, Sekhar Nori wrote:
> On Tuesday 28 February 2017 07:46 AM, Kevin Hilman wrote:
> > + Sekhar for the board-da850-evm questions.
> > 
> > Yegor Yefremov <yegorslists@googlemail.com> writes:
> > 
> >> Hi Dmitry,
> >>
> >> On Sat, Feb 25, 2017 at 8:08 PM, Dmitry Torokhov
> >> <dmitry.torokhov@gmail.com> wrote:
> >>> Hi Yegor,
> >>>
> >>> On Fri, Feb 24, 2017 at 04:42:25PM +0100, yegorslists@googlemail.com wrote:
> >>>> From: Yegor Yefremov <yegorslists@googlemail.com>
> >>>>
> >>>> This patch adds support for DT to tps6507x-ts driver.
> >>>>
> >>>> Signed-off-by: Yegor Yefremov <yegorslists@googlemail.com>
> >>>> ---
> >>>>  drivers/input/touchscreen/tps6507x-ts.c | 70 ++++++++++++++++++++-------------
> >>>>  1 file changed, 42 insertions(+), 28 deletions(-)
> >>>>
> >>>> diff --git a/drivers/input/touchscreen/tps6507x-ts.c b/drivers/input/touchscreen/tps6507x-ts.c
> >>>> index 5bf1ec6..2d61220 100644
> >>>> --- a/drivers/input/touchscreen/tps6507x-ts.c
> >>>> +++ b/drivers/input/touchscreen/tps6507x-ts.c
> >>>> @@ -22,6 +22,8 @@
> >>>>  #include <linux/mfd/tps6507x.h>
> >>>>  #include <linux/input/tps6507x-ts.h>
> >>>>  #include <linux/delay.h>
> >>>> +#include <linux/of.h>
> >>>> +#include <linux/of_device.h>
> >>>>
> >>>>  #define TSC_DEFAULT_POLL_PERIOD 30 /* ms */
> >>>>  #define TPS_DEFAULT_MIN_PRESSURE 0x30
> >>>> @@ -199,33 +201,52 @@ static void tps6507x_ts_poll(struct input_polled_dev *poll_dev)
> >>>>       tps6507x_adc_standby(tsc);
> >>>>  }
> >>>>
> >>>> +static void tsc_init_data(struct tps6507x_ts *tsc,
> >>>> +             struct input_dev *input_dev)
> >>>> +{
> >>>> +     struct device_node *node = tsc->dev->of_node;
> >>>> +     const struct tps6507x_board *tps_board =
> >>>> +                     dev_get_platdata(tsc->dev);
> >>>> +     const struct touchscreen_init_data *init_data = NULL;
> >>>> +
> >>>> +     if (tps_board)
> >>>> +             /*
> >>>> +              * init_data points to array of touchscreen_init structure
> >>>> +              * coming from the board-evm file.
> >>>> +              */
> >>>> +             init_data = tps_board->tps6507x_ts_init_data;
> >>>> +
> >>>> +     if (node == NULL && init_data == NULL) {
> >>>> +             tsc->poll_dev->poll_interval = TSC_DEFAULT_POLL_PERIOD;
> >>>> +             tsc->min_pressure = TPS_DEFAULT_MIN_PRESSURE;
> >>>
> >>> I think you want to always init with defaults, then override with
> >>> property data.
> >>
> >> ACK
> >>
> >>>> +     } else if (init_data) {
> >>>> +             tsc->poll_dev->poll_interval = init_data->poll_period;
> >>>> +             tsc->min_pressure = init_data->min_pressure;
> >>>> +             input_dev->id.vendor = init_data->vendor;
> >>>> +             input_dev->id.product = init_data->product;
> >>>> +             input_dev->id.version = init_data->version;
> >>>> +     } else if (node) {
> >>>> +             of_property_read_u32(node, "ti,poll-period",
> >>>> +                             &tsc->poll_dev->poll_interval);
> >>>> +             of_property_read_u16(node, "ti,min-pressure",
> >>>> +                             &tsc->min_pressure);
> >>>
> >>> Anything but u32 is better avoided in DT as it requires special
> >>> annotation which is easily forgotten.
> >>
> >> Will convert to 32-bit values.
> >>
> >>> I also would prefer if we switched to generic device properties and
> >>> retired the static init data.
> >>
> >> do you mean converting the driver to DT-only variant?

Not to DT only. device_property_read_* API works on DT, ACPI and legacy
boards that can be converted to use property sets (see
device_add_properties() and struct property_entry API).

> >>
> >>>> +             of_property_read_u16(node, "ti,vendor",
> >>>> +                             &input_dev->id.vendor);
> >>>> +             of_property_read_u16(node, "ti,product",
> >>>> +                             &input_dev->id.product);
> >>>
> >>> Would we not know vendor and product from the compatible string?
> >>>
> >>>> +             of_property_read_u16(node, "ti,version",
> >>>> +                             &input_dev->id.version);
> >>>
> >>> I wonder if anyone cares about this.
> >>
> >> I've looked at other touchscreen drivers and this structure seems to
> >> be filled mostly for the RS232 connected drivers. So I'd suggest just
> >> to leave vendor, product and version at 0 and not provide these values
> >> via DT.  At least tslib and hence Qt is working without these values
> >> set.
> >>
> >> @Kevin: so far the only user of this touch driver is da850-evm board.
> >> Can we drop the values for the id structure? Btw. what else is needed
> 
> It seems like it was mostly added so it can be read through sysfs. But
> since it has been exposed to userspace, I am not sure if it can simply
> be dropped. May be used hardcoded values for these in the driver based
> on compatible string ?

That's what I said. We do know the product and vendor. Version could be
dropped.

-- 
Dmitry