> From: Sa, Nuno <Nuno.Sa@xxxxxxxxxx>
> Sent: Wednesday, June 30, 2021 2:04 PM
> To: Alexandru Ardelean <ardeleanalex@xxxxxxxxx>; Miclaus, Antoniu
> <Antoniu.Miclaus@xxxxxxxxxx>
> Cc: linux-iio <linux-iio@xxxxxxxxxxxxxxx>; LKML <linux-
> kernel@xxxxxxxxxxxxxxx>; Jonathan Cameron <jic23@xxxxxxxxxx>;
> devicetree <devicetree@xxxxxxxxxxxxxxx>; Rob Herring
> <robh+dt@xxxxxxxxxx>
> Subject: RE: [PATCH 1/2] iio: frequency: adrf6780: add support for
> ADRF6780
>
> [External]
>
> Hey Alex,
>
> Thanks for the review,
>
> > From: Alexandru Ardelean <ardeleanalex@xxxxxxxxx>
> > Sent: Wednesday, June 30, 2021 10:38 AM
> > To: Miclaus, Antoniu <Antoniu.Miclaus@xxxxxxxxxx>
> > Cc: linux-iio <linux-iio@xxxxxxxxxxxxxxx>; LKML <linux-
> > kernel@xxxxxxxxxxxxxxx>; Jonathan Cameron <jic23@xxxxxxxxxx>;
> > devicetree <devicetree@xxxxxxxxxxxxxxx>; Rob Herring
> > <robh+dt@xxxxxxxxxx>
> > Subject: Re: [PATCH 1/2] iio: frequency: adrf6780: add support for
> > ADRF6780
> >
> > On Tue, Jun 29, 2021 at 5:25 PM Antoniu Miclaus
> > <antoniu.miclaus@xxxxxxxxxx> wrote:
> > >
> > > Add support for the ADRF6780 microwave upconverter.
> >
> > Hey,
> >
> > A few comments inline.
> > The description of the commit could have a bit more information.
> > Maybe a short description of the chip (typically I'd adapt something
> > from the datasheet).
> > And maybe a link to the datasheet.
> >
> > >
> > > Signed-off-by: Antoniu Miclaus <antoniu.miclaus@xxxxxxxxxx>
> > > ---
> > > drivers/iio/frequency/Kconfig | 13 +
> > > drivers/iio/frequency/Makefile | 1 +
> > > drivers/iio/frequency/adrf6780.c | 534
> > +++++++++++++++++++++++++++++++
> > > 3 files changed, 548 insertions(+)
> > > create mode 100644 drivers/iio/frequency/adrf6780.c
> > >
> > > diff --git a/drivers/iio/frequency/Kconfig
> > b/drivers/iio/frequency/Kconfig
> > > index 240b81502512..fc9751c48f59 100644
> > > --- a/drivers/iio/frequency/Kconfig
> > > +++ b/drivers/iio/frequency/Kconfig
> > > @@ -49,5 +49,18 @@ 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
> > > + depends on OF
> > > + 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..c492c4e4adf1
> > > --- /dev/null
> > > +++ b/drivers/iio/frequency/adrf6780.c
> > > @@ -0,0 +1,534 @@
> > > +// SPDX-License-Identifier: GPL-2.0+
> >
> > about the licensing;
> >
> > SPDX-License-Identifier: GPL-2.0+ == MODULE_LICENSE("GPL
> v2");
> > SPDX-License-Identifier: GPL-2.0 == MODULE_LICENSE("GPL v2");
> >
> > I usually don't care about this licensing details, but it seems to be
> > important elsewhere.
> >
> > > +/*
> > > + * ADRF6780 driver
> >
> > This could be "Analog Devices ADRF6780 driver"
> >
> > > + *
> > > + * Copyright 2021 Analog Devices Inc.
> > > + */
> > > +
> > > +#include <linux/bitfield.h>
> > > +#include <linux/bits.h>
> > > +#include <linux/clk.h>
> > > +#include <linux/clkdev.h>
> > > +#include <linux/clk-provider.h>
> > > +#include <linux/delay.h>
> > > +#include <linux/device.h>
> > > +#include <linux/iio/iio.h>
> > > +#include <linux/module.h>
> > > +#include <linux/regmap.h>
> > > +#include <linux/spi/spi.h>
> >
> > Not all these headers look used.
> > For one thing, regmap.h doesn't look used at all.
> > Maybe trim the list.
> >
> > > +
> > > +/* ADRF6780 Register Map */
> > > +#define ADRF6780_REG_CONTROL 0x00
> > > +#define ADRF6780_REG_ALARM_READBACK 0x01
> > > +#define ADRF6780_REG_ALARM_MASKS 0x02
> > > +#define ADRF6780_REG_ENABLE 0x03
> > > +#define ADRF6780_REG_LINEARIZE 0x04
> > > +#define ADRF6780_REG_LO_PATH 0x05
> > > +#define ADRF6780_REG_ADC_CONTROL 0x06
> > > +#define ADRF6780_REG_ADC_OUTPUT 0x0C
> > > +
> > > +/* ADRF6780_REG_CONTROL Map */
> > > +#define ADRF6780_PARITY_EN_MSK BIT(15)
> > > +#define ADRF6780_PARITY_EN(x)
> > FIELD_PREP(ADRF6780_PARITY_EN_MSK, x)
> > > +#define ADRF6780_SOFT_RESET_MSK BIT(14)
> > > +#define ADRF6780_SOFT_RESET(x)
> > FIELD_PREP(ADRF6780_SOFT_RESET_MSK, x)
> > > +#define ADRF6780_CHIP_ID_MSK GENMASK(11, 4)
> > > +#define ADRF6780_CHIP_ID 0xA
> > > +#define ADRF6780_CHIP_REVISION_MSK GENMASK(3, 0)
> > > +#define ADRF6780_CHIP_REVISION(x)
> > FIELD_PREP(ADRF6780_CHIP_REVISION_MSK, x)
> > > +
> > > +/* ADRF6780_REG_ALARM_READBACK Map */
> > > +#define ADRF6780_PARITY_ERROR_MSK BIT(15)
> > > +#define ADRF6780_PARITY_ERROR(x)
> > FIELD_PREP(ADRF6780_PARITY_ERROR_MSK, x)
> > > +#define ADRF6780_TOO_FEW_ERRORS_MSK BIT(14)
> > > +#define ADRF6780_TOO_FEW_ERRORS(x)
> > FIELD_PREP(ADRF6780_TOO_FEW_ERRORS_MSK, x)
> > > +#define ADRF6780_TOO_MANY_ERRORS_MSK BIT(13)
> > > +#define ADRF6780_TOO_MANY_ERRORS(x)
> > FIELD_PREP(ADRF6780_TOO_MANY_ERRORS_MSK, x)
> > > +#define ADRF6780_ADDRESS_RANGE_ERROR_MSK BIT(12)
> > > +#define ADRF6780_ADDRESS_RANGE_ERROR(x)
> > FIELD_PREP(ADRF6780_ADDRESS_RANGE_ERROR_MSK, x)
> > > +
> > > +/* ADRF6780_REG_ENABLE Map */
> > > +#define ADRF6780_VGA_BUFFER_EN_MSK BIT(8)
> > > +#define ADRF6780_VGA_BUFFER_EN(x)
> > FIELD_PREP(ADRF6780_VGA_BUFFER_EN_MSK, x)
> > > +#define ADRF6780_DETECTOR_EN_MSK BIT(7)
> > > +#define ADRF6780_DETECTOR_EN(x)
> > FIELD_PREP(ADRF6780_DETECTOR_EN_MSK, x)
> > > +#define ADRF6780_LO_BUFFER_EN_MSK BIT(6)
> > > +#define ADRF6780_LO_BUFFER_EN(x)
> > FIELD_PREP(ADRF6780_LO_BUFFER_EN_MSK, x)
> > > +#define ADRF6780_IF_MODE_EN_MSK BIT(5)
> > > +#define ADRF6780_IF_MODE_EN(x)
> > FIELD_PREP(ADRF6780_IF_MODE_EN_MSK, x)
> > > +#define ADRF6780_IQ_MODE_EN_MSK BIT(4)
> > > +#define ADRF6780_IQ_MODE_EN(x)
> > FIELD_PREP(ADRF6780_IQ_MODE_EN_MSK, x)
> > > +#define ADRF6780_LO_X2_EN_MSK BIT(3)
> > > +#define ADRF6780_LO_X2_EN(x)
> > FIELD_PREP(ADRF6780_LO_X2_EN_MSK, x)
> > > +#define ADRF6780_LO_PPF_EN_MSK BIT(2)
> > > +#define ADRF6780_LO_PPF_EN(x)
> > FIELD_PREP(ADRF6780_LO_PPF_EN_MSK, x)
> > > +#define ADRF6780_LO_EN_MSK BIT(1)
> > > +#define ADRF6780_LO_EN(x)
> > FIELD_PREP(ADRF6780_LO_EN_MSK, x)
> > > +#define ADRF6780_UC_BIAS_EN_MSK BIT(0)
> > > +#define ADRF6780_UC_BIAS_EN(x)
> > FIELD_PREP(ADRF6780_UC_BIAS_EN_MSK, x)
> > > +
> > > +/* ADRF6780_REG_LINEARIZE Map */
> > > +#define ADRF6780_RDAC_LINEARIZE_MSK GENMASK(7, 0)
> > > +#define ADRF6780_RDAC_LINEARIZE(x)
> > FIELD_PREP(ADRF6780_RDAC_LINEARIZE_MSK, x)
> > > +
> > > +/* ADRF6780_REG_LO_PATH Map */
> > > +#define ADRF6780_LO_SIDEBAND_MSK BIT(10)
> > > +#define ADRF6780_LO_SIDEBAND(x)
> > FIELD_PREP(ADRF6780_LO_SIDEBAND_MSK, x)
> > > +#define ADRF6780_Q_PATH_PHASE_ACCURACY_MSK
> > GENMASK(7, 4)
> > > +#define ADRF6780_Q_PATH_PHASE_ACCURACY(x)
> > FIELD_PREP(ADRF6780_Q_PATH_PHASE_ACCURACY_MSK, x)
> > > +#define ADRF6780_I_PATH_PHASE_ACCURACY_MSK
> > GENMASK(3, 0)
> > > +#define ADRF6780_I_PATH_PHASE_ACCURACY(x)
> > FIELD_PREP(ADRF6780_I_PATH_PHASE_ACCURACY_MSK, x)
> > > +
> > > +/* ADRF6780_REG_ADC_CONTROL Map */
> > > +#define ADRF6780_VDET_OUTPUT_SELECT_MSK BIT(3)
> > > +#define ADRF6780_VDET_OUTPUT_SELECT(x)
> > FIELD_PREP(ADRF6780_VDET_OUTPUT_SELECT_MSK, x)
> > > +#define ADRF6780_ADC_START_MSK BIT(2)
> > > +#define ADRF6780_ADC_START(x)
> > FIELD_PREP(ADRF6780_ADC_START_MSK, x)
> > > +#define ADRF6780_ADC_EN_MSK BIT(1)
> > > +#define ADRF6780_ADC_EN(x)
> > FIELD_PREP(ADRF6780_ADC_EN_MSK, x)
> > > +#define ADRF6780_ADC_CLOCK_EN_MSK BIT(0)
> > > +#define ADRF6780_ADC_CLOCK_EN(x)
> > FIELD_PREP(ADRF6780_ADC_CLOCK_EN_MSK, x)
> > > +
> > > +/* ADRF6780_REG_ADC_OUTPUT Map */
> > > +#define ADRF6780_ADC_STATUS_MSK BIT(8)
> > > +#define ADRF6780_ADC_STATUS(x)
> > FIELD_PREP(ADRF6780_ADC_STATUS_MSK, x)
> > > +#define ADRF6780_ADC_VALUE_MSK GENMASK(7, 0)
> > > +#define ADRF6780_ADC_VALUE(x)
> > FIELD_PREP(ADRF6780_ADC_VALUE_MSK, x)
> >
> > The indentation for the bit-values doesn't look consistent in all
> places.
> >
> > > +
> > > +enum supported_parts {
> > > + ADRF6780,
> > > +};
> >
> > This enum doesn't seem used anywhere
> >
> > > +
> > > +struct adrf6780_dev {
> > > + struct spi_device *spi;
> > > + struct clk *clkin;
> > > + /* Protect against concurrent accesses to the device */
> > > + struct mutex lock;
> > > + bool parity_en;
> >
> > Maybe remove this parity check.
> > There are many drivers that support some form of simple error
> > checking, but in the kernel this is typically left up to the SPI
> > framework.
> > So, I'd just disable the error checking entirely.
>
> I would just argue here... Though parity check is not the most reliable
> one
> out there, it does give some extra checking on top of what the SPI
> framework
> can give (AFAICT). As this part might be used is some sensitive
> applications, I
> can imagine someone wanting this extra feature that does not really
> cost that
> much (if something). However it looks like that on the write side, the
> only way
> to know about parity issues (or other errors) is to make use of ALM
> pin.
>
> Anyways, I do agree that having this as a devicetree parameter is, at
> least, arguable
> (I don't think it is terribly wrong though). But I guess that, at least, as a
> module
> parameter would be something more acceptable.
Nice that we did not went (removed in v2) with the module parameter. Just
found out yesterday [1] that they really not much appreciated in device drivers
(which actually makes sense).
[1]: https://lore.kernel.org/lkml/YN2MtVFOC+yd%2FrRZ@xxxxxxxxx/
- Nuno Sá
