Re: [PATCH V2 1/2] mux: adgs1408: new driver for Analog Devices ADGS1408/1409 mux …
On Wed, 2018-07-18 at 14:49 +0200, Peter Rosin wrote:
> On 2018-07-17 15:42, Mircea Caprioru wrote:
> > This patch adds basic support for Analog Device ADGS1408/09 SPI mux
> > controller.
> >
> > The device is probed and set to a disabled state. It uses the new
> > mux
> > controller framework.
> >
> > Signed-off-by: Mircea Caprioru
> > ---
> > Changelog V1 -> V2
> > - removed adgs140x wildcard
> > - removed cells verification since only 0 configuration supported
>
> Ahhh, the two muxes in 1409 cannot be operated independently, as this
> is a
> "differential" mux. That simplifies things quite a bit, and good
> thing you
> caught this early!
Yeah, I understood the mux framework a bit better the second time I
looked at it (after your first review).
>
> > - added id enum for ADGS1408 and ADGS1409
> > - sorted includes
> >
> > MAINTAINERS| 5 ++
> > drivers/mux/Kconfig| 12 +
> > drivers/mux/Makefile | 2 +
> > drivers/mux/adgs1408.c | 120
> > +
> > 4 files changed, 139 insertions(+)
> > create mode 100644 drivers/mux/adgs1408.c
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 192d7f73fd01..458d42d6f4f3 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -810,6 +810,11 @@ L: linux-me...@vger.kernel.org
> > S: Maintained
> > F: drivers/media/i2c/ad9389b*
> >
> > +ANALOG DEVICES INC ADGS1408 DRIVER
> > +M: Mircea Caprioru
> > +S: Supported
> > +F: drivers/mux/adgs1408.c
> > +
> > ANALOG DEVICES INC ADV7180 DRIVER
> > M: Lars-Peter Clausen
> > L: linux-me...@vger.kernel.org
> > diff --git a/drivers/mux/Kconfig b/drivers/mux/Kconfig
> > index 6241678e99af..cf825e9f47ef 100644
> > --- a/drivers/mux/Kconfig
> > +++ b/drivers/mux/Kconfig
> > @@ -21,6 +21,18 @@ config MUX_ADG792A
> > To compile the driver as a module, choose M here: the
> > module will
> > be called mux-adg792a.
> >
> > +config MUX_ADGS1408
> > + tristate "Analog Devices ADGS1408/ADGS1409 Multiplexers"
> > + depends on SPI
> > + help
> > + ADGS1408 8:1 multiplexer and ADGS1409 double 4:1
> > multiplexer
> > + switches.
> > +
> > + The driver supports driving each switch independently.
>
> Remove this line. It is confusing since 1408 only have one mux and
> the two muxes in the dual 1409 cannot be operated independently.
Ack.
>
> > +
> > + To compile the driver as a module, choose M here: the
> > module will
> > + be called mux-adgs1408.
> > +
> > config MUX_GPIO
> > tristate "GPIO-controlled Multiplexer"
> > depends on GPIOLIB || COMPILE_TEST
> > diff --git a/drivers/mux/Makefile b/drivers/mux/Makefile
> > index c3d883955fd5..6e9fa47daf56 100644
> > --- a/drivers/mux/Makefile
> > +++ b/drivers/mux/Makefile
> > @@ -5,10 +5,12 @@
> >
> > mux-core-objs := core.o
> > mux-adg792a-objs := adg792a.o
> > +mux-adgs1408-objs := adgs1408.o
> > mux-gpio-objs := gpio.o
> > mux-mmio-objs := mmio.o
> >
> > obj-$(CONFIG_MULTIPLEXER) += mux-core.o
> > obj-$(CONFIG_MUX_ADG792A) += mux-adg792a.o
> > +obj-$(CONFIG_MUX_ADGS1408) += mux-adgs1408.o
> > obj-$(CONFIG_MUX_GPIO) += mux-gpio.o
> > obj-$(CONFIG_MUX_MMIO) += mux-mmio.o
> > diff --git a/drivers/mux/adgs1408.c b/drivers/mux/adgs1408.c
> > new file mode 100644
> > index ..a5192c5e484b
> > --- /dev/null
> > +++ b/drivers/mux/adgs1408.c
> > @@ -0,0 +1,120 @@
> > +// SPDX-License-Identifier: GPL-2.0+
>
> The preferred spelling is GPL-2.0-or-later
Hmmm...
This seems to be new. There are quite a few places where GPL-2.0+ is
used.
Will use GPL-2.0-or-later here.
>
> > +/*
> > + * ADG1408 SPI MUX driver
>
> ADGS1408; you are missing the S.
>
> And I think you could/should mention ADGS1409 here.
Ack.
>
> > + *
> > + * Copyright 2018 Analog Devices Inc.
> > + */
> > +
> > +#include
> > +#include
>
> This #include sorts after linux/property.h.
>
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> > +#define ADGS1408_SW_DATA (0x01)
> > +#define ADGS1408_REG_READ(reg) ((reg) | 0x80)
> > +#define ADGS1408_DISABLE (0x00)
> > +#define ADGS1408_MUX(state)(((state) << 1) | 1)
> > +
> > +enum {
> > + ADGS1408,
> > + ADGS1409
>
> Add a trailing comma here, to make future changes less intrusive.
Ack.
>
> > +};
> > +
> > +static int adgs1408_spi_reg_write(struct spi_device *spi,
> > + u8 reg_addr, u8 reg_data)
> > +{
> > + u8 tx_buf[2];
> > +
> > + tx_buf[0] = reg_addr;
> > + tx_buf[1] = reg_data;
> > +
> > + return spi_write_then_read(spi, tx_buf, sizeof(tx_buf),
> > NULL, 0);
> > +}
> > +
> > +static int adgs1408_set(struct mux_control *mux, int state)
> > +{
> > + struct spi_device *spi = to_spi_device(mux->chip-
> > >dev.parent);
> > + u8 reg;
> > +
> > + if (state == MUX_IDLE_DISCONNECT)
> > + reg = ADGS1408_DISABLE;
> > + else
> > + re
RE: [PATCH] mux: adgs1408: use the correct SPDX license identifier
Ok now I understand. We can go with GPL-2.0-or-later and
MODULE_LICENSE("GPL").
We are pretty flexible regarding what license we should use upstream wise. If
you feel we should change it in some other way we can do that too.
Regards,
Mircea
-Original Message-
From: Peter Rosin [mailto:p...@axentia.se]
Sent: Monday, August 20, 2018 1:38 PM
To: linux-kernel@vger.kernel.org
Cc: Peter Rosin ; Caprioru, Mircea
Subject: [PATCH] mux: adgs1408: use the correct SPDX license identifier
The file is GPL v2 only.
Signed-off-by: Peter Rosin
---
drivers/mux/adgs1408.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
On 2018-08-20 12:16, Caprioru, Mircea wrote:
> Peter Rosin wrote:
>> I just noticed a discrepancy in the license annotations in your
>> driver. The SPDX license identifier says GPL-2.0-or-later, and I
>> assume that is correct and that MODULE_LICENSE("GPL") without the v2
>> specifier, as in this patch, is the correct thing to do?
>
> The file should be v2. It might of slipped from the final version.
Ok, so I read that as if it should be v2 *only* and not v2-or-later.
I.e. the reverse of the original patch. But since these things can be
sensitive, I'd like confirmation that this new patch is what is correct...
Note that MODULE_LICENSE("GPL") means GPL v2 *or later* (GPL v1 is not on the
table), while MODULE_LICENSE("GPL v2") means GPL v2 *only*.
See include/linux/module.h for details.
Cheers,
Peter
diff --git a/drivers/mux/adgs1408.c b/drivers/mux/adgs1408.c index
0f7cf54e3234..fe0377bb83eb 100644
--- a/drivers/mux/adgs1408.c
+++ b/drivers/mux/adgs1408.c
@@ -1,4 +1,4 @@
-// SPDX-License-Identifier: GPL-2.0-or-later
+// SPDX-License-Identifier: GPL-2.0
/*
* ADGS1408/ADGS1409 SPI MUX driver
*
--
2.11.0
Re: [PATCH V3 4/5] dt-bindings: iio: adc: Convert ad7124 documentation to YAML
On Mon, 2019-06-24 at 07:50 -0600, Rob Herring wrote: > [External] > > > On Mon, Jun 24, 2019 at 2:09 AM Mircea Caprioru > wrote: > > > > Convert AD7124 bindings documentation to YAML format. > > > > Signed-off-by: Mircea Caprioru > > --- > > > > Changelog v2: > > - modified SPDX license to GPL-2.0 OR BSD-2-Clause > > - added regex for a range from 0 to 15 > > - added minimum and maximum constraints for reg property > > - set type and range of values for adi,reference-select property > > - used items for diff-channels property > > - set bipolar, adi,buffered-positive and negative to type: boolean > > > > Changelog v3: > > - moved adi,buffered-positive and negative properties to own commit > > > > .../bindings/iio/adc/adi,ad7124.yaml | 144 > > ++ > > 1 file changed, 144 insertions(+) > > create mode 100644 > > Documentation/devicetree/bindings/iio/adc/adi,ad7124.yaml > > The subject says 'Convert', but where's the removal of the old > binding? > > Rob You are right I was not sure if I should also remove the old binding. I'll do a v4 to delete it in this patch. Thanks, Mircea
Re: [PATCH 1/4] iio: adc: ad7124: Remove input number limitation
On Thu, 2019-06-20 at 12:19 +0300, Mircea Caprioru wrote:
> The driver limits the user to use only 4/8 differential inputs, but
> this
> device has the option to use pseudo-differential channels. This will
> increase the number of channels to be equal with the number of inputs
> so 8
> channels for ad7124-4 and 16 for ad7124-8.
>
> This patch removes the check between channel nodes and num_inputs
> value.
>
> Signed-off-by: Mircea Caprioru
> ---
> drivers/iio/adc/ad7124.c | 7 ---
> 1 file changed, 7 deletions(-)
>
> diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c
> index 659ef37d5fe8..810234db9c0d 100644
> --- a/drivers/iio/adc/ad7124.c
> +++ b/drivers/iio/adc/ad7124.c
> @@ -462,13 +462,6 @@ static int ad7124_of_parse_channel_config(struct
> iio_dev *indio_dev,
> if (ret)
> goto err;
>
> - if (ain[0] >= st->chip_info->num_inputs ||
> - ain[1] >= st->chip_info->num_inputs) {
> - dev_err(indio_dev->dev.parent,
> - "Input pin number out of range.\n");
> - ret = -EINVAL;
> - goto err;
> - }
> st->channel_config[channel].ain =
> AD7124_CHANNEL_AINP(ain[0]) |
> AD7124_CHANNEL_AINM(a
> in[1]);
> st->channel_config[channel].bipolar =
Please ignore this patch serie, just did a RESEND.
Thanks,
Mircea
RE: [PATCH] iio: dac: ad5758: Modifications for new revision
Hi Jonathan,
1. Any changes to functionality that might break users of the older revisions?
>From what i have found out there are no users of the older revision since it
>has not been available for purchasing. Only this new revision.
2. Any userspace visible changes?
There are no userspace visible changes.
3. Do we want to think about back porting to older kernels?
This could be a benefit, but i don't think the impact will be significant.
4. Should we support this explicitly by detecting or describing the
hardware revision in some fashion?
Yes I could use the CHIP_ID register to check the revision and do a V2. Just
wondering if it is needed if there are no older revision chips in use.
Thanks,
Mircea
-Original Message-
From: Jonathan Cameron [mailto:ji...@kernel.org]
Sent: Sunday, April 14, 2019 4:02 PM
To: Caprioru, Mircea
Cc: Hennerich, Michael ; Popa, Stefan Serban
; l...@metafoo.de; gre...@linuxfoundation.org;
linux-kernel@vger.kernel.org; linux-...@vger.kernel.org
Subject: Re: [PATCH] iio: dac: ad5758: Modifications for new revision
[External]
On Tue, 9 Apr 2019 16:35:21 +0300
Mircea Caprioru wrote:
> This patch will ensure compatibility with the new revision of the
> AD5758 dac converter. The modifications consist of removing the
> fault_prot_switch function since this option is no longer available,
> and enabling the ENABLE_PPC_BUFFERS bit in ADC_CONFIG register before
> setting the PPC current mode.
>
> Signed-off-by: Mircea Caprioru
Hi Mircea,
Talk me through the implications of this change.
1. Any changes to functionality that might break users of the older revisions?
2. Any userspace visible changes?
3. Do we want to think about back porting to older kernels?
4. Should we support this explicitly by detecting or describing the
hardware revision in some fashion?
Thanks,
Jonathan
> ---
> drivers/iio/dac/ad5758.c | 55
> +---
> 1 file changed, 18 insertions(+), 37 deletions(-)
>
> diff --git a/drivers/iio/dac/ad5758.c b/drivers/iio/dac/ad5758.c index
> 2bdf1b0aee06..a513c70faefa 100644
> --- a/drivers/iio/dac/ad5758.c
> +++ b/drivers/iio/dac/ad5758.c
> @@ -72,8 +72,6 @@
> #define AD5758_DCDC_CONFIG1_DCDC_VPROG_MODE(x) (((x) & 0x1F) << 0)
> #define AD5758_DCDC_CONFIG1_DCDC_MODE_MSKGENMASK(6, 5)
> #define AD5758_DCDC_CONFIG1_DCDC_MODE_MODE(x)(((x) & 0x3) << 5)
> -#define AD5758_DCDC_CONFIG1_PROT_SW_EN_MSK BIT(7)
> -#define AD5758_DCDC_CONFIG1_PROT_SW_EN_MODE(x) (((x) & 0x1) << 7)
>
> /* AD5758_DCDC_CONFIG2 */
> #define AD5758_DCDC_CONFIG2_ILIMIT_MSK GENMASK(3, 1)
> @@ -84,6 +82,10 @@
> /* AD5758_DIGITAL_DIAG_RESULTS */
> #define AD5758_CAL_MEM_UNREFRESHED_MSK BIT(15)
>
> +/* AD5758_ADC_CONFIG */
> +#define AD5758_ADC_CONFIG_PPC_BUF_EN(x) (((x) & 0x1) << 11)
> +#define AD5758_ADC_CONFIG_PPC_BUF_MSKBIT(11)
> +
> #define AD5758_WR_FLAG_MSK(x)(0x80 | ((x) & 0x1F))
>
> #define AD5758_FULL_SCALE_MICRO 6553500ULL
> @@ -315,6 +317,18 @@ static int ad5758_set_dc_dc_conv_mode(struct
> ad5758_state *st, {
> int ret;
>
> + /*
> + * The ENABLE_PPC_BUFFERS bit must be set prior to enabling PPC current
> + * mode.
> + */
> + if (mode == AD5758_DCDC_MODE_PPC_CURRENT) {
> + ret = ad5758_spi_write_mask(st, AD5758_ADC_CONFIG,
> + AD5758_ADC_CONFIG_PPC_BUF_MSK,
> + AD5758_ADC_CONFIG_PPC_BUF_EN(1));
> + if (ret < 0)
> + return ret;
> + }
> +
> ret = ad5758_spi_write_mask(st, AD5758_DCDC_CONFIG1,
> AD5758_DCDC_CONFIG1_DCDC_MODE_MSK,
>
> AD5758_DCDC_CONFIG1_DCDC_MODE_MODE(mode));
> @@ -444,23 +458,6 @@ static int ad5758_set_out_range(struct ad5758_state *st,
> int range)
>
> AD5758_CAL_MEM_UNREFRESHED_MSK); }
>
> -static int ad5758_fault_prot_switch_en(struct ad5758_state *st, bool
> enable) -{
> - int ret;
> -
> - ret = ad5758_spi_write_mask(st, AD5758_DCDC_CONFIG1,
> - AD5758_DCDC_CONFIG1_PROT_SW_EN_MSK,
> - AD5758_DCDC_CONFIG1_PROT_SW_EN_MODE(enable));
> - if (ret < 0)
> - return ret;
> - /*
> - * Poll the BUSY_3WI bit in the DCDC_CONFIG2 register until it is 0.
> - * This allows the 3-wire interface communication to complete.
> - */
> - return ad5758_wait_for_task_complete(st, AD5758_DCDC_CONFIG2,
> - AD5758_DCDC_CONFIG2_BUSY_3WI_MSK);
> -}
>
