Re: [PATCH] hwmon: da9052: Synchronize access with mfd

[Sebastian Reichel]

Hi Guenter,

On Mon, May 11, 2020 at 09:51:25AM -0700, Guenter Roeck wrote:
> On 5/11/20 4:02 AM, Sebastian Reichel wrote:
> > From: Samu Nuutamo <samu.nuutamo@xxxxxxxxx>
> > 
> > When tsi-as-adc is configured it is possible for in7[0123]_input read to
> > return an incorrect value if a concurrent read to in[456]_input is
> > performed. This is caused by a concurrent manipulation of the mux
> > channel without proper locking as hwmon and mfd use different locks for
> > synchronization.
> > 
> > Switch hwmon to use the same lock as mfd when accessing the TSI channel.
> > 
> > Fixes: 4f16cab19a3d5 ("hwmon: da9052: Add support for TSI channel")
> > Signed-off-by: Samu Nuutamo <samu.nuutamo@xxxxxxxxx>
> > [rebase to current master, reword commit message slightly]
> > Signed-off-by: Sebastian Reichel <sebastian.reichel@xxxxxxxxxxxxx>
> 
> Have you explored calling da9052_adc_manual_read() instead ?
> At the very least we should have a comment explaining why that
> isn't feasible.

da9052_adc_manual_read() writes to DA9052_ADC_MAN_REG, waits for an
IRQ and then reads DA9052_ADC_RES_H_REG/DA9052_ADC_RES_L_REG. The
function called here works with the TSI registers instead. So
calling da9052_adc_manual_read() is not an option.

The reason for the locking problem is, that the same ADC is used
internally by the devices and muxed to different pins in the
background. In continuous touchscreen mode, the device is supposed
to schedule the ADC slots automatically and I assumed this would
also work here when I wrote the original support. Turns out the
device is not smart enough for that :(

-- Sebastian

> Thanks,
> Guenter
> 
> > ---
> >  drivers/hwmon/da9052-hwmon.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/hwmon/da9052-hwmon.c b/drivers/hwmon/da9052-hwmon.c
> > index 53b517dbe7e6..4af2fc309c28 100644
> > --- a/drivers/hwmon/da9052-hwmon.c
> > +++ b/drivers/hwmon/da9052-hwmon.c
> > @@ -244,9 +244,9 @@ static ssize_t da9052_tsi_show(struct device *dev,
> >  	int channel = to_sensor_dev_attr(devattr)->index;
> >  	int ret;
> >  
> > -	mutex_lock(&hwmon->hwmon_lock);
> > +	mutex_lock(&hwmon->da9052->auxadc_lock);
> >  	ret = __da9052_read_tsi(dev, channel);
> > -	mutex_unlock(&hwmon->hwmon_lock);
> > +	mutex_unlock(&hwmon->da9052->auxadc_lock);
> >  
> >  	if (ret < 0)
> >  		return ret;
> > 
>

Attachment: signature.asc
Description: PGP signature