lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Sun, 17 Oct 2021 16:00:47 +0100 From: Jonathan Cameron <jic23@...nel.org> To: Antoniu Miclaus <antoniu.miclaus@...log.com> Cc: <linux-iio@...r.kernel.org>, <devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org> Subject: Re: [PATCH v7 1/2] iio: frequency: adrf6780: add support for ADRF6780 On Mon, 11 Oct 2021 14:00:10 +0300 Antoniu Miclaus <antoniu.miclaus@...log.com> 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@...log.com> Hi Antoniu, A few things went wrong with the addition of the remove path changes that you will want to clean up. Also just noticed your ADC channel is an output which seems unlikely... See comments inline. Thanks, Jonathan > --- > changes in v7: > - add device remove path and disable all circuitry controlled via > Enable Register. > - rename `adrf6780_dt_parse` -> `adrf6780_properties_parse` > > drivers/iio/frequency/Kconfig | 12 + > drivers/iio/frequency/Makefile | 1 + > drivers/iio/frequency/adrf6780.c | 503 +++++++++++++++++++++++++++++++ > 3 files changed, 516 insertions(+) > create mode 100644 drivers/iio/frequency/adrf6780.c > ... > diff --git a/drivers/iio/frequency/adrf6780.c b/drivers/iio/frequency/adrf6780.c > new file mode 100644 > index 000000000000..2daa728d2ff4 > --- /dev/null > +++ b/drivers/iio/frequency/adrf6780.c > @@ -0,0 +1,503 @@ ... > +#define ADRF6780_CHAN_ADC(_channel) { \ > + .type = IIO_ALTVOLTAGE, \ > + .output = 1, \ > + .indexed = 1, \ > + .channel = _channel, \ > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) \ > +} > + > +#define ADRF6780_CHAN_RDAC(_channel) { \ > + .type = IIO_ALTVOLTAGE, \ > + .output = 1, \ > + .indexed = 1, \ > + .channel = _channel, \ > + .info_mask_separate = BIT(IIO_CHAN_INFO_SCALE) \ > +} Just noticed, but as you use the same index for ADC(0) and RDAC(0) and both are output channels(odd for something called ADC?) you might as well combine the info_masks and have just one entry. I'm guessing that ADC channel might not be an output one though... > + > +#define ADRF6780_CHAN_IQ_PHASE(_channel, rf_comp) { \ > + .type = IIO_ALTVOLTAGE, \ > + .modified = 1, \ > + .output = 1, \ > + .indexed = 1, \ > + .channel2 = IIO_MOD_##rf_comp, \ > + .channel = _channel, \ > + .info_mask_separate = BIT(IIO_CHAN_INFO_PHASE) \ > +} > + > +static const struct iio_chan_spec adrf6780_channels[] = { > + ADRF6780_CHAN_ADC(0), > + ADRF6780_CHAN_RDAC(0), > + ADRF6780_CHAN_IQ_PHASE(0, I), > + ADRF6780_CHAN_IQ_PHASE(0, Q), > +}; > + > ... > +static int adrf6780_init(struct adrf6780_dev *dev) > +{ > + int ret; > + unsigned int chip_id, enable_reg, enable_reg_msk; > + struct spi_device *spi = dev->spi; > + > + /* Perform a software reset */ > + ret = adrf6780_reset(dev); > + if (ret) > + return ret; > + > + ret = adrf6780_spi_read(dev, ADRF6780_REG_CONTROL, &chip_id); > + if (ret) > + return ret; > + > + chip_id = FIELD_GET(ADRF6780_CHIP_ID_MSK, chip_id); > + if (chip_id != ADRF6780_CHIP_ID) { > + dev_err(&spi->dev, "ADRF6780 Invalid Chip ID.\n"); > + return -EINVAL; > + } > + > + enable_reg_msk = ADRF6780_VGA_BUFFER_EN_MSK | > + ADRF6780_DETECTOR_EN_MSK | > + ADRF6780_LO_BUFFER_EN_MSK | > + ADRF6780_IF_MODE_EN_MSK | > + ADRF6780_IQ_MODE_EN_MSK | > + ADRF6780_LO_X2_EN_MSK | > + ADRF6780_LO_PPF_EN_MSK | > + ADRF6780_LO_EN_MSK | > + ADRF6780_UC_BIAS_EN_MSK; > + > + enable_reg = FIELD_PREP(ADRF6780_VGA_BUFFER_EN_MSK, dev->vga_buff_en) | > + FIELD_PREP(ADRF6780_DETECTOR_EN_MSK, 1) | > + FIELD_PREP(ADRF6780_LO_BUFFER_EN_MSK, dev->lo_buff_en) | > + FIELD_PREP(ADRF6780_IF_MODE_EN_MSK, dev->if_mode_en) | > + FIELD_PREP(ADRF6780_IQ_MODE_EN_MSK, dev->iq_mode_en) | > + FIELD_PREP(ADRF6780_LO_X2_EN_MSK, dev->lo_x2_en) | > + FIELD_PREP(ADRF6780_LO_PPF_EN_MSK, dev->lo_ppf_en) | > + FIELD_PREP(ADRF6780_LO_EN_MSK, dev->lo_en) | > + FIELD_PREP(ADRF6780_UC_BIAS_EN_MSK, dev->uc_bias_en); > + > + ret = adrf6780_spi_update_bits(dev, ADRF6780_REG_ENABLE, enable_reg_msk, enable_reg); > + if (ret) > + return ret; > + > + ret = adrf6780_spi_update_bits(dev, ADRF6780_REG_LO_PATH, > + ADRF6780_LO_SIDEBAND_MSK, > + FIELD_PREP(ADRF6780_LO_SIDEBAND_MSK, dev->lo_sideband)); > + if (ret) > + return ret; > + > + return adrf6780_spi_update_bits(dev, ADRF6780_REG_ADC_CONTROL, > + ADRF6780_VDET_OUTPUT_SELECT_MSK, > + FIELD_PREP(ADRF6780_VDET_OUTPUT_SELECT_MSK, dev->vdet_out_en)); Very long line so perhaps add an additional line break after the , > +} > + ... > + > +static int adrf6780_probe(struct spi_device *spi) > +{ > + struct iio_dev *indio_dev; > + struct adrf6780_dev *dev; > + int ret; > + > + indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*dev)); > + if (!indio_dev) > + return -ENOMEM; > + > + dev = iio_priv(indio_dev); > + > + indio_dev->info = &adrf6780_info; > + indio_dev->name = "adrf6780"; > + indio_dev->channels = adrf6780_channels; > + indio_dev->num_channels = ARRAY_SIZE(adrf6780_channels); > + > + dev->spi = spi; > + > + adrf6780_properties_parse(dev); > + > + dev->clkin = devm_clk_get(&spi->dev, "lo_in"); Please add an error message to things that might result in a deferred probe via dev_err_probe() as it stores more debugging information for those trying to chase down why a probe was deferred. Note this is a new thing, but as you are going to need to do another version anyway, nice to tidy it up in the initial driver rather than as a follow up patch. > + if (IS_ERR(dev->clkin)) > + return PTR_ERR(dev->clkin); > + > + ret = clk_prepare_enable(dev->clkin); > + if (ret < 0) > + goto error_disable_clk; > + > + mutex_init(&dev->lock); > + > + ret = adrf6780_init(dev); > + if (ret) > + goto error_disable_clk;; > + > + ret = iio_device_register(indio_dev); > + if (ret) > + goto error_disable_clk; > + > + return 0; > + > +error_disable_clk: An easy way to spot inconsistencies is to compare what is done in the error path in probe() with what occurs in remove(). They should only differ in that the first element in remove() often won't appear in the probe error handling. So why am I not seeing the disable write to the device here? > + clk_disable_unprepare(dev->clkin); > + > + return ret; > +} > + > +static int adrf6780_remove(struct spi_device *spi) > +{ > + int ret; > + struct iio_dev *indio_dev = spi_get_drvdata(spi); > + struct adrf6780_dev *dev = iio_priv(indio_dev); > + > + /* Disable all components in the Enable Register */ > + ret = adrf6780_spi_write(dev, ADRF6780_REG_ENABLE, 0x0); What call in probe() is this matched against? Looks to be the _init() call. In which case this should be after the iio_device_unregister(). Particularly obvious here as turning off the device 'before' we remove the userspace interface is never a good plan. Also, there is no point in returning an error code from probe so just carry on if this fails and we'll disable as much as possible. Note that there are lots of patches removing returns from spi driver remove functions at the moment. Everything you are doing in this remove could be easily handled automatically with a few devm_add_action_or_reset() calls in probe. See the many patches Alexandru Ardelean has sent converting driver to that approach. One side effect is that the ordering is automatically correct. > + if(ret) > + return ret; > + > + iio_device_unregister(indio_dev); > + > + clk_disable_unprepare(dev->clkin); > + > + return 0; > +} > + > + One blank line is almost always enough. > +static const struct spi_device_id adrf6780_id[] = { > + { "adrf6780", 0 }, > + {} > +};
Powered by blists - more mailing lists