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

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

Hi Jeff,

On Tue, Dec 01, 2020 at 11:53:50PM -0600, Jeff LaBundy wrote:
> 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.i

I was thinking about allowing both "touch-" and "touchscreen-" for the
allowed property names.

> 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.

That is fine, we can rename the file later if we decide to change the
namespace.

> 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.

Yep, that sounds great, thank you.

-- 
Dmitry