Re: [PATCH 2/2] input: Add support for Azoteq IQS626A

[Jeff LaBundy <jeff@labundy.com>]

Hi Dmitry,

On Mon, Nov 30, 2020 at 11:01:06PM -0800, Dmitry Torokhov wrote:
> On Mon, Nov 23, 2020 at 06:15:16PM -0600, Jeff LaBundy wrote:
> > Hi Dmitry,
> > 
> > Thank you for taking a look.
> > 
> > On Sun, Nov 22, 2020 at 11:03:07PM -0800, Dmitry Torokhov wrote:
> > > Hi Jeff,
> > > 
> > > On Sun, Nov 22, 2020 at 04:39:08PM -0600, Jeff LaBundy wrote:
> > > > +
> > > > +		if ((sys_reg->active & tp_mask) == tp_mask)
> > > > +			input_set_abs_params(iqs626->trackpad,
> > > > +					     ABS_X, 0, 255, 0, 0);
> > > > +		else
> > > > +			input_set_abs_params(iqs626->trackpad,
> > > > +					     ABS_X, 0, 128, 0, 0);
> > > > +#ifdef CONFIG_TOUCHSCREEN_PROPERTIES
> > > > +		touchscreen_parse_properties(iqs626->trackpad, false,
> > > > +					     &iqs626->prop);
> > > > +#endif /* CONFIG_TOUCHSCREEN_PROPERTIES */
> > > 
> > > This should not be separately selectable from CONFIG_INPUT, so there is
> > > not need to have this guard.
> > > 
> > > The reason it is a separate symbol is historical - it used to depend on
> > > OF in addition to INPUT. I suppose I can drop it now.
> > 
> > Without these guards, the build fails if CONFIG_INPUT_TOUCHSCREEN=n and
> > I felt it too heavy-handed to add a 'depends on' for what is ultimately
> > a corner-case of sorts for this device.
> 
> Ah, I missed the fat that we got outside of the
> drivers/input/toucscreen.
> 
> > 
> > The touchscreen helpers are useful for more than just touchscreens, and
> > we can extend them to all of input with something like the patch below.
> > If it looks OK to you, I can insert it into v2 after I collect feedback
> > from Rob for the binding.
> 
> Yes, I guess we should move into core. Can you move the file into
> drivers/input and maybe we should rename it into touch-properties.c? And
> start renaming the API form touchscreen_*() to touch_()?

Sure thing, I can move it. I guess we want to do the same for the binding
too? There are only a handful of other bindings that will need references
to touchscreen.yaml updated with a new relative path.

I'm hesitant to rename the API because we still need to support bindings
that start with touchscreen-* and having an API with different namespace
seems inconsistent. How about I volunteer the following for this series:

1. Move of_touchscreen.c to drivers/input, and rename it to touchscreen.c
   since it is not actually related to OF at this point. This would match
   the header file too.
2. Update its introductory comments from:
   "Generic DT helper functions for touchscreen devices"
   to:
   "Generic helper functions for touchscreens and other two-dimensional
    pointing devices"
3. Move touchscreen.* from bindings/input/touchscreen to bindings/input,
   and update the handful of touchscreen bindings that assume a relative
   path to touchscreen.yaml.

Let me know if this seems like a reasonable compromise.

> 
> Thanks.
> 
> -- 
> Dmitry

Kind regards,
Jeff LaBundy