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:33:10AM +0200, Marek Vasut wrote:
> On 8/31/21 1:20 AM, Dmitry Torokhov wrote:
> > 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.
> 
> I don't think this touch controller has any "disable interrupts" bit.
> So the only option here would be some disable_irq() on the IRQ line itself ?

Yes I believe so.

-- 
Dmitry