Re: [PATCH v2 1/3] Input: ili210x - use resolution from ili251x firmware

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

Hi Marek,

On Fri, Aug 27, 2021 at 11:12:56PM +0200, Marek Vasut wrote:
> The ili251x firmware protocol permits readout of panel resolution,
> implement this, but make it possible to override this value using
> DT bindings. This way, older DTs which contain touchscreen-size-x
> and touchscreen-size-y properties will behave just like before and
> new DTs may avoid specifying these for ILI251x.
> 
> Note that the command format is different on other controllers, so
> this functionality is isolated to ILI251x.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Cc: Joe Hung <joe_hung@ilitek.com>
> Cc: Luca Hsu <luca_hsu@ilitek.com>
> ---
> V2: New patch
> ---
>  drivers/input/touchscreen/ili210x.c | 31 +++++++++++++++++++++++++++++
>  1 file changed, 31 insertions(+)
> 
> diff --git a/drivers/input/touchscreen/ili210x.c b/drivers/input/touchscreen/ili210x.c
> index 30576a5f2f04..c46553ecabe6 100644
> --- a/drivers/input/touchscreen/ili210x.c
> +++ b/drivers/input/touchscreen/ili210x.c
> @@ -323,6 +323,34 @@ static irqreturn_t ili210x_irq(int irq, void *irq_data)
>  	return IRQ_HANDLED;
>  }
>  
> +static int ili251x_firmware_update_resolution(struct device *dev)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct ili210x *priv = i2c_get_clientdata(client);
> +	u16 resx, resy;
> +	u8 rs[10];
> +	int ret;
> +
> +	/* The firmware update blob might have changed the resolution. */
> +	ret = priv->chip->read_reg(client, REG_PANEL_INFO, &rs, sizeof(rs));
> +	if (ret)
> +		return ret;
> +
> +	resx = (rs[1] << 8U) | rs[0];
> +	resy = (rs[3] << 8U) | rs[2];

Why do we need the 'U' specifier here? Actually, let's use
le16_to_cpup() or get_unaligned_le16().

> +
> +	/* The value reported by the firmware is invalid. */
> +	if (!resx || resx == 0xffff || !resy || resy == 0xfff)

Not 0xffff for resy?

> +		return -EINVAL;
> +
> +	priv->input->absinfo[ABS_X].maximum = resx - 1;
> +	priv->input->absinfo[ABS_Y].maximum = resy - 1;
> +	priv->input->absinfo[ABS_MT_POSITION_X].maximum = resx - 1;
> +	priv->input->absinfo[ABS_MT_POSITION_Y].maximum = resy - 1;

There is

	input_set_abs_max(priv->input, ABS_X, resx - 1);

> +
> +	return 0;
> +}
> +
>  static ssize_t ili210x_calibrate(struct device *dev,
>  				 struct device_attribute *attr,
>  				 const char *buf, size_t count)
> @@ -449,6 +477,9 @@ static int ili210x_i2c_probe(struct i2c_client *client,
>  	input_set_abs_params(input, ABS_MT_POSITION_Y, 0, max_xy, 0, 0);
>  	if (priv->chip->has_pressure_reg)
>  		input_set_abs_params(input, ABS_MT_PRESSURE, 0, 0xa, 0, 0);
> +	/* ILI251x does report valid resolution information, try it. */
> +	if (priv->chip == &ili251x_chip)
> +		ili251x_firmware_update_resolution(dev);

Would prefer a flag in chip features vs matching on chip.

>  	touchscreen_parse_properties(input, true, &priv->prop);
>  
>  	error = input_mt_init_slots(input, priv->chip->max_touches,
> -- 
> 2.32.0
> 

Thanks.

-- 
Dmitry