Re: [PATCH v2 2/3] Input: ili210x - export ili251x version details via sysfs

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

On Tue, Aug 31, 2021 at 01:02:57AM +0200, Marek Vasut wrote:
> On 8/30/21 11:01 PM, Dmitry Torokhov wrote:
> 
> [...]
> 
> > > @@ -351,6 +360,108 @@ static int ili251x_firmware_update_resolution(struct device *dev)
> > >   	return 0;
> > >   }
> > > +static ssize_t ili251x_firmware_version_show(struct device *dev,
> > > +					     struct device_attribute *attr,
> > > +					     char *buf)
> > > +{
> > > +	struct i2c_client *client = to_i2c_client(dev);
> > > +	struct ili210x *priv = i2c_get_clientdata(client);
> > > +	u8 fw[8];
> > > +	int ret;
> > > +
> > > +	/* Get firmware version */
> > > +	mutex_lock(&priv->lock);
> > > +	ret = priv->chip->read_reg(client, REG_FIRMWARE_VERSION,
> > > +				   &fw, sizeof(fw));
> > > +	mutex_unlock(&priv->lock);
> > 
> > Could we query firmware version and mode at probe time (and also later
> > after firmware update attempt) so that we do not need to introduce
> > locking against interrupt handler?
> 
> This is a threaded interrupt handler and I don't expect much lock contention
> here.
> 
> The sysfs attribute readout would race with the interrupt handler and if it
> wasn't for the firmware update support, we could very well cache all those
> values. Except, the firmware update can change them all. Worse, if the
> interrupt were to fire during an update, it could break that update, and I
> want to prevent that from happening.

Usually we simply disable interrupts from the device when updating
firmware.

Thanks.

-- 
Dmitry