On 06/29, Jonathan Cameron wrote:
> On Tue, 25 Jun 2024 18:55:27 -0300
> Marcelo Schmitt <marcelo.schmitt@xxxxxxxxxx> wrote:
>
> > Add support for AD4000 series of low noise, low power, high speed,
> > successive approximation register (SAR) ADCs.
> >
> > Signed-off-by: Marcelo Schmitt <marcelo.schmitt@xxxxxxxxxx>
> Hi Marcelo,
>
> You've clearly gotten some good review for this version so I only
> had a quick scan through. One thing did jump out at me though.
>
> > +
> > +static int ad4000_write_raw_get_fmt(struct iio_dev *indio_dev,
> > + struct iio_chan_spec const *chan, long mask)
> > +{
> > + switch (mask) {
> > + case IIO_CHAN_INFO_SCALE:
> > + return IIO_VAL_INT_PLUS_NANO;
> > + default:
> > + return IIO_VAL_INT_PLUS_MICRO;
> > + }
> > +}
> > +
> > +static int ad4000_write_raw(struct iio_dev *indio_dev,
> > + struct iio_chan_spec const *chan, int val, int val2,
> > + long mask)
> > +{
> > + struct ad4000_state *st = iio_priv(indio_dev);
> > + unsigned int reg_val;
> > + bool span_comp_en;
> > + int ret;
> > +
> > + switch (mask) {
> > + case IIO_CHAN_INFO_SCALE:
> > + ret = iio_device_claim_direct_mode(indio_dev);
> > + if (ret < 0)
> > + return ret;
> > +
> > + mutex_lock(&st->lock);
> > + ret = ad4000_read_reg(st, ®_val);
> > + if (ret < 0)
> > + goto err_unlock;
> > +
> > + span_comp_en = val2 == st->scale_tbl[1][1];
> > + reg_val &= ~AD4000_CFG_SPAN_COMP;
> > + reg_val |= FIELD_PREP(AD4000_CFG_SPAN_COMP, span_comp_en);
> > +
> > + ret = ad4000_write_reg(st, reg_val);
> > + if (ret < 0)
> > + goto err_unlock;
> > +
> > + st->span_comp = span_comp_en;
> > +err_unlock:
> > + iio_device_release_direct_mode(indio_dev);
> > + mutex_unlock(&st->lock);
>
> Lock ordering needs another look. I'm not sure we an trigger
> a deadlock but it definitely looks problematic.
Oops. Oh, that's inddeed back lock release ordering.
I've changed to scoped and guard for v6 and will send the updated version soon.
Anyway, thanks for having a look at it.
Marcelo
>
> J
>
>
