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
