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
