RE: [PATCH 1/2] iio: frequency: adrf6780: add support for ADRF6780

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> 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á




[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Input]     [Linux Kernel]     [Linux SCSI]     [X.org]

  Powered by Linux