Hello Jonathan, Would you like me to reintroduce the locked/unlocked version for the "adrf6780_spi_update_bits" function too? This was present in the first version of the driver but removed afterwards based on the review. Regards, -- Antoniu Miclăuş > -----Original Message----- > From: Jonathan Cameron <jic23@xxxxxxxxxx> > Sent: Wednesday, October 20, 2021 11:24 PM > To: Miclaus, Antoniu <Antoniu.Miclaus@xxxxxxxxxx> > Cc: linux-kernel@xxxxxxxxxxxxxxx; linux-iio@xxxxxxxxxxxxxxx > Subject: Re: [PATCH v8 1/2] iio: frequency: adrf6780: add support for > ADRF6780 > > [External] > > On Mon, 18 Oct 2021 14:09:30 +0300 > Antoniu Miclaus <antoniu.miclaus@xxxxxxxxxx> wrote: > > > The ADRF6780 is a silicon germanium (SiGe) design, wideband, > > microwave upconverter optimized for point to point microwave > > radio designs operating in the 5.9 GHz to 23.6 GHz frequency > > range. > > > > Datasheet: > > https://www.analog.com/media/en/technical-documentation/data- > sheets/ADRF6780.pdf > > > > Signed-off-by: Antoniu Miclaus <antoniu.miclaus@xxxxxxxxxx> > > The 'problem' with rereading drivers lots of times is sometimes > you notice a new issue that you've missed before. Unfortunately > the locking in here is inconsistent and there is not protection against > various functions using the dev->data[] array at the same time. > Sorry I missed this in earlier reviews! > > It's a little bit too fiddly for me to simply make the changes > whilst applying, so please add the missing mutex_lock/unlock > and send out a v9. I would suggest doing it by renaming the existing > unlocked versions to > __XXX_write() /read() > and then adding functions which take the mutex called after their > original names. > > I made a small additional comment on naming that would also be good to > clear up. > > Plus please run scripts/checkpatch.pl over the file and fix the warnings. > Preferably with --strict > > Thanks, > > Jonathan > > > --- > > changes in v8: > > - condense lines that are very long > > - set ADC channel as input > > - add `dev_err_probe` > > - handle device powerdown via `devm_add_action_or_reset` > > drivers/iio/frequency/Kconfig | 12 + > > drivers/iio/frequency/Makefile | 1 + > > drivers/iio/frequency/adrf6780.c | 496 > +++++++++++++++++++++++++++++++ > > 3 files changed, 509 insertions(+) > > create mode 100644 drivers/iio/frequency/adrf6780.c > > > > diff --git a/drivers/iio/frequency/Kconfig b/drivers/iio/frequency/Kconfig > > index 240b81502512..2c9e0559e8a4 100644 > > --- a/drivers/iio/frequency/Kconfig > > +++ b/drivers/iio/frequency/Kconfig > > @@ -49,5 +49,17 @@ config ADF4371 > > > > To compile this driver as a module, choose M here: the > > module will be called adf4371. > > + > > +config ADRF6780 > > + tristate "Analog Devices ADRF6780 Microwave Upconverter" > > + depends on SPI > > + depends on COMMON_CLK > > + help > > + Say yes here to build support for Analog Devices ADRF6780 > > + 5.9 GHz to 23.6 GHz, Wideband, Microwave Upconverter. > > + > > + To compile this driver as a module, choose M here: the > > + module will be called adrf6780. > > + > > endmenu > > endmenu > > diff --git a/drivers/iio/frequency/Makefile > b/drivers/iio/frequency/Makefile > > index 518b1e50caef..ae3136c79202 100644 > > --- a/drivers/iio/frequency/Makefile > > +++ b/drivers/iio/frequency/Makefile > > @@ -7,3 +7,4 @@ > > obj-$(CONFIG_AD9523) += ad9523.o > > obj-$(CONFIG_ADF4350) += adf4350.o > > obj-$(CONFIG_ADF4371) += adf4371.o > > +obj-$(CONFIG_ADRF6780) += adrf6780.o > > diff --git a/drivers/iio/frequency/adrf6780.c > b/drivers/iio/frequency/adrf6780.c > > new file mode 100644 > > index 000000000000..4097b31bdf0b > > --- /dev/null > > +++ b/drivers/iio/frequency/adrf6780.c > > > + > > +static int adrf6780_spi_read(struct adrf6780_dev *dev, unsigned int reg, > > + unsigned int *val) > > I've highlighted a few paths inline where this isn't protected by a > mutex. As such we will be racing on the content of dev->data[] > and results are going to be very unpredicatable. > > > +{ > > + int ret; > > + struct spi_transfer t = {0}; > > + > > + dev->data[0] = 0x80 | (reg << 1); > > + dev->data[1] = 0x0; > > + dev->data[2] = 0x0; > > + > > + t.rx_buf = &dev->data[0]; > > + t.tx_buf = &dev->data[0]; > > + t.len = 3; > > + > > + ret = spi_sync_transfer(dev->spi, &t, 1); > > + if (ret) > > + return ret; > > + > > + *val = (get_unaligned_be24(&dev->data[0]) >> 1) & GENMASK(15, > 0); > > + > > + return ret; > > +} > > + > > +static int adrf6780_spi_write(struct adrf6780_dev *dev, > > + unsigned int reg, > > + unsigned int val) > > +{ > > + put_unaligned_be24((val << 1) | (reg << 17), &dev->data[0]); > > + > > + return spi_write(dev->spi, &dev->data[0], 3); > > +} > > + > ... > > > +static int adrf6780_read_raw(struct iio_dev *indio_dev, > > + struct iio_chan_spec const *chan, > > + int *val, int *val2, long info) > > +{ > > + struct adrf6780_dev *dev = iio_priv(indio_dev); > > + unsigned int data; > > + int ret; > > + > > + switch (info) { > > + case IIO_CHAN_INFO_RAW: > > + ret = adrf6780_read_adc_raw(dev, &data); > > This takes the mutex inside the call which is good. > > > + if (ret) > > + return ret; > > + > > + *val = data & ADRF6780_ADC_VALUE_MSK; > > + > > + return IIO_VAL_INT; > > + > > + case IIO_CHAN_INFO_SCALE: > > But need locks here as well (see below) > > > + ret = adrf6780_spi_read(dev, ADRF6780_REG_LINEARIZE, > &data); > > + if (ret) > > + return ret; > > + > > + *val = data & ADRF6780_RDAC_LINEARIZE_MSK; > > + > > + return IIO_VAL_INT; > > + case IIO_CHAN_INFO_PHASE: > > And here. > > > + ret = adrf6780_spi_read(dev, ADRF6780_REG_LO_PATH, > &data); > > + if (ret) > > + return ret; > > + > > + switch (chan->channel2) { > > + case IIO_MOD_I: > > + *val = data & > ADRF6780_I_PATH_PHASE_ACCURACY_MSK; > > + > > + return IIO_VAL_INT; > > + case IIO_MOD_Q: > > + *val = > FIELD_GET(ADRF6780_Q_PATH_PHASE_ACCURACY_MSK, data); > > + > > + return IIO_VAL_INT; > > + default: > > + return -EINVAL; > > + } > > + default: > > + return -EINVAL; > > + } > > +} > > + > > +static int adrf6780_write_raw(struct iio_dev *indio_dev, > > + struct iio_chan_spec const *chan, > > + int val, int val2, long info) > > +{ > > + struct adrf6780_dev *dev = iio_priv(indio_dev); > > Minor point but there is a fairly strong expectation than anything > called simply 'dev' is a struct device. Consider renaming. > > > + int ret; > > + > > + switch (info) { > > + case IIO_CHAN_INFO_SCALE: > > This needs to take the lock or you can have this using dev->data at the > same time as other calls which have taken the mutex. > > > + return adrf6780_spi_write(dev, ADRF6780_REG_LINEARIZE, > val); > > + case IIO_CHAN_INFO_PHASE: > > + switch (chan->channel2) { > > + case IIO_MOD_I: > > + mutex_lock(&dev->lock); > > + ret = adrf6780_spi_update_bits(dev, > ADRF6780_REG_LO_PATH, > > + > ADRF6780_I_PATH_PHASE_ACCURACY_MSK, > > + > FIELD_PREP(ADRF6780_I_PATH_PHASE_ACCURACY_MSK, val)); > > + mutex_unlock(&dev->lock); > > + > > + return ret; > > + case IIO_MOD_Q: > > + mutex_lock(&dev->lock); > > + ret = adrf6780_spi_update_bits(dev, > ADRF6780_REG_LO_PATH, > > + > ADRF6780_Q_PATH_PHASE_ACCURACY_MSK, > > + > FIELD_PREP(ADRF6780_Q_PATH_PHASE_ACCURACY_MSK, val)); > > + mutex_unlock(&dev->lock); > > + > > + return ret; > > + default: > > + return -EINVAL; > > + } > > + default: > > + return -EINVAL; > > + } > > +} > > + > > +static int adrf6780_reg_access(struct iio_dev *indio_dev, > > + unsigned int reg, > > + unsigned int write_val, > > + unsigned int *read_val) > > +{ > > + struct adrf6780_dev *dev = iio_priv(indio_dev); > > + > > + if (read_val) > > Nothing prevents these being concurrent with other accesses > using the st->buf either. > > > + return adrf6780_spi_read(dev, reg, read_val); > > + else > > + return adrf6780_spi_write(dev, reg, write_val); > > +} > > + > ...
