| Message ID | 20241004-ad7606_add_iio_backend_support-v3-0-38757012ce82@baylibre.com |
|---|---|
| Headers | show |
| Series | Add iio backend compatibility for ad7606 | expand |
On Fri, 04 Oct 2024 21:48:35 +0000 Guillaume Stols <gstols@baylibre.com> wrote: > The parallel driver's name is ad7606_par and not ad7606_parallel. > > Fixes: 0046a46a8f93 ("staging/ad7606: Actually build the interface modules") > Signed-off-by: Guillaume Stols <gstols@baylibre.com> I'm going to nibble away at series where I can today because there are more patches under revision than I'd like. Merging subsets of series where there are simple ones like this makes that a little easier to manage. So applied this one to the togreg branch of iio.git and pushed out as testing so 0-day can fail to notice it ;) Thanks, Jonathan > --- > drivers/iio/adc/Kconfig | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig > index 97ece1a4b7e3..4ab1a3092d88 100644 > --- a/drivers/iio/adc/Kconfig > +++ b/drivers/iio/adc/Kconfig > @@ -229,7 +229,7 @@ config AD7606_IFACE_PARALLEL > ad7605-4, ad7606, ad7606-6, ad7606-4 analog to digital converters (ADC). > > To compile this driver as a module, choose M here: the > - module will be called ad7606_parallel. > + module will be called ad7606_par. > > config AD7606_IFACE_SPI > tristate "Analog Devices AD7606 ADC driver with spi interface support" >
On Fri, 04 Oct 2024 21:48:39 +0000 Guillaume Stols <gstols@baylibre.com> wrote: > Some of the includes were not in alphabetical order, this commit fixes > it. > > Signed-off-by: Guillaume Stols <gstols@baylibre.com> Applied this patch to the togreg branch of iio.git and pushed out for now as testing for 0-day to take a look. Thanks, Jonathan
On Fri, 04 Oct 2024 21:48:43 +0000 Guillaume Stols <gstols@baylibre.com> wrote: > - Basic support for iio backend. > - Supports IIO_CHAN_INFO_SAMP_FREQ R/W. > - Only hardware mode is available, and that IIO_CHAN_INFO_RAW is not > supported if iio-backend mode is selected. I don't much like the trivial window between this patch and the next where the emulated mode is still there but the sleeps aren't adapting with sampling frequency. Maybe it's worth a dance of leaving the write_raw support until after this one so the frequency remains fixed until after the fsleep(2) calls are gone? There is another bit that I'm unsure is technically correct until after the next patch. Maybe I'm reading the diff wrong though! Thanks, J > > Signed-off-by: Guillaume Stols <gstols@baylibre.com> > --- > drivers/iio/adc/Kconfig | 2 + > drivers/iio/adc/ad7606.c | 124 +++++++++++++++++++++++++++++++++++++------ > drivers/iio/adc/ad7606.h | 15 ++++++ > drivers/iio/adc/ad7606_par.c | 94 +++++++++++++++++++++++++++++++- > 4 files changed, 219 insertions(+), 16 deletions(-) > > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig > index 4ab1a3092d88..9b52d5b2c592 100644 > --- a/drivers/iio/adc/Kconfig > +++ b/drivers/iio/adc/Kconfig > @@ -224,9 +224,11 @@ config AD7606_IFACE_PARALLEL > tristate "Analog Devices AD7606 ADC driver with parallel interface support" > depends on HAS_IOPORT > select AD7606 > + select IIO_BACKEND > help > Say yes here to build parallel interface support for Analog Devices: > ad7605-4, ad7606, ad7606-6, ad7606-4 analog to digital converters (ADC). > + It also support iio_backended devices for AD7606B. > > To compile this driver as a module, choose M here: the > module will be called ad7606_par. > diff --git a/drivers/iio/adc/ad7606.c b/drivers/iio/adc/ad7606.c > index 3666a58f8a6f..d86eb7c3e4f7 100644 > --- a/drivers/iio/adc/ad7606.c > +++ b/drivers/iio/adc/ad7606.c > @@ -21,6 +21,7 @@ > @@ -737,6 +773,10 @@ static int ad7606_write_raw(struct iio_dev *indio_dev, > return ret; > > return 0; > + case IIO_CHAN_INFO_SAMP_FREQ: > + if (val < 0 && val2 != 0) > + return -EINVAL; > + return ad7606_set_sampling_freq(st, val); Currently I think for the !backend + pwm case this can go out of range for which that code works (fsleep removed in next patch). Perhaps delay adding this until after that patch. > default: > return -EINVAL; > } > @@ -1108,7 +1186,24 @@ int ad7606_probe(struct device *dev, int irq, void __iomem *base_address, > st->cnvst_pwm); > if (ret) > return ret; > + } > + > + if (st->bops->iio_backend_config) { > + /* > + * If there is a backend, the PWM should not overpass the maximum sampling > + * frequency the chip supports. > + */ > + ret = ad7606_set_sampling_freq(st, > + chip_info->max_samplerate ? : 2 * KILO); > + if (ret) > + return ret; > + > + ret = st->bops->iio_backend_config(dev, indio_dev); > + if (ret) > + return ret; > + indio_dev->setup_ops = &ad7606_pwm_buffer_ops; > } else { > + init_completion(&st->completion); > st->trig = devm_iio_trigger_alloc(dev, "%s-dev%d", > indio_dev->name, > iio_device_id(indio_dev)); It's a little hard to unwind the patches, but this was previously in the !pwm case. At this point in the series we still allow the pwm case to work with ! backend. So is this now running in that case? Do we need a temporary additional check on !pwm > @@ -1126,15 +1221,14 @@ int ad7606_probe(struct device *dev, int irq, void __iomem *base_address, > &ad7606_buffer_ops); > if (ret) > return ret; > + ret = devm_request_threaded_irq(dev, irq, > + NULL, > + &ad7606_interrupt, > + IRQF_TRIGGER_FALLING | IRQF_ONESHOT, > + chip_info->name, indio_dev); > + if (ret) > + return ret; > } > - ret = devm_request_threaded_irq(dev, irq, > - NULL, > - &ad7606_interrupt, > - IRQF_TRIGGER_FALLING | IRQF_ONESHOT, > - chip_info->name, indio_dev); > - if (ret) > - return ret; > - > return devm_iio_device_register(dev, indio_dev); > } > EXPORT_SYMBOL_NS_GPL(ad7606_probe, IIO_AD7606); > diff --git a/drivers/iio/adc/ad7606_par.c b/drivers/iio/adc/ad7606_par.c > index b87be2f1ca04..6042f6799272 100644 > --- a/drivers/iio/adc/ad7606_par.c > +++ b/drivers/iio/adc/ad7606_par.c > @@ -2,7 +2,8 @@ > + > +static int ad7606_bi_setup_iio_backend(struct device *dev, struct iio_dev *indio_dev) > +{ > + struct ad7606_state *st = iio_priv(indio_dev); > + unsigned int ret, c; > + struct iio_backend_data_fmt data = { > + .sign_extend = true, > + .enable = true, > + }; > + > + st->back = devm_iio_backend_get(dev, NULL); > + if (IS_ERR(st->back)) > + return PTR_ERR(st->back); > + > + /* If the device is iio_backend powered the PWM is mandatory */ > + if (!st->cnvst_pwm) > + return dev_err_probe(st->dev, -EINVAL, > + "A PWM is mandatory when using backend.\n"); > + > + ret = devm_iio_backend_request_buffer(dev, st->back, indio_dev); > + if (ret) > + return ret; > + > + ret = devm_iio_backend_enable(dev, st->back); > + if (ret) > + return ret; > + > + for (c = 0; c < indio_dev->num_channels; c++) { > + ret = iio_backend_data_format_set(st->back, c, &data); > + if (ret) > + return ret; > + } > + > + indio_dev->channels = ad7606b_bi_channels; Ultimately this may want to move into the chip_info structures as more devices are added but this is fine for now I suppose. > + indio_dev->num_channels = 8; > + > + return 0; > +}
On Fri, Oct 04, 2024 at 09:48:37PM +0000, Guillaume Stols wrote: > Add the required properties for iio-backend support, as well as an > example and the conditions to mutually exclude interruption and > conversion trigger with iio-backend. > The iio-backend's function is to controls the communication, and thus the > interruption pin won't be available anymore. > As a consequence, the conversion pin must be controlled externally since > we will miss information about when every single conversion cycle (i.e > conversion + data transfer) ends, hence a PWM is introduced to trigger > the conversions. > > Signed-off-by: Guillaume Stols <gstols@baylibre.com> > --- > .../devicetree/bindings/iio/adc/adi,ad7606.yaml | 64 +++++++++++++++++++++- > 1 file changed, 62 insertions(+), 2 deletions(-) > > diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml > index 47081c79a1cf..a389cfda824d 100644 > --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml > +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml > @@ -129,6 +129,29 @@ properties: > assumed that the pins are hardwired to VDD. > type: boolean > > + pwms: > + description: > + In case the conversion is triggered by a PWM instead of a GPIO plugged to > + the CONVST pin, the PWM must be referenced. > + The first is the PWM connected to CONVST or CONVST1 for the chips with 2 s/2/2nd/ Otherwise, Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
On 10/5/24 13:53, Jonathan Cameron wrote: > On Fri, 04 Oct 2024 21:48:43 +0000 > Guillaume Stols <gstols@baylibre.com> wrote: > >> - Basic support for iio backend. >> - Supports IIO_CHAN_INFO_SAMP_FREQ R/W. >> - Only hardware mode is available, and that IIO_CHAN_INFO_RAW is not >> supported if iio-backend mode is selected. > I don't much like the trivial window between this patch and the next > where the emulated mode is still there but the sleeps aren't adapting with sampling frequency. > > Maybe it's worth a dance of leaving the write_raw support > until after this one so the frequency remains fixed until after > the fsleep(2) calls are gone? > > There is another bit that I'm unsure is technically correct until after > the next patch. Maybe I'm reading the diff wrong though! > > Thanks, > > J > >> Signed-off-by: Guillaume Stols <gstols@baylibre.com> >> --- >> drivers/iio/adc/Kconfig | 2 + >> drivers/iio/adc/ad7606.c | 124 +++++++++++++++++++++++++++++++++++++------ >> drivers/iio/adc/ad7606.h | 15 ++++++ >> drivers/iio/adc/ad7606_par.c | 94 +++++++++++++++++++++++++++++++- >> 4 files changed, 219 insertions(+), 16 deletions(-) >> >> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig >> index 4ab1a3092d88..9b52d5b2c592 100644 >> --- a/drivers/iio/adc/Kconfig >> +++ b/drivers/iio/adc/Kconfig >> @@ -224,9 +224,11 @@ config AD7606_IFACE_PARALLEL >> tristate "Analog Devices AD7606 ADC driver with parallel interface support" >> depends on HAS_IOPORT >> select AD7606 >> + select IIO_BACKEND >> help >> Say yes here to build parallel interface support for Analog Devices: >> ad7605-4, ad7606, ad7606-6, ad7606-4 analog to digital converters (ADC). >> + It also support iio_backended devices for AD7606B. >> >> To compile this driver as a module, choose M here: the >> module will be called ad7606_par. >> diff --git a/drivers/iio/adc/ad7606.c b/drivers/iio/adc/ad7606.c >> index 3666a58f8a6f..d86eb7c3e4f7 100644 >> --- a/drivers/iio/adc/ad7606.c >> +++ b/drivers/iio/adc/ad7606.c >> @@ -21,6 +21,7 @@ >> @@ -737,6 +773,10 @@ static int ad7606_write_raw(struct iio_dev *indio_dev, >> return ret; >> >> return 0; >> + case IIO_CHAN_INFO_SAMP_FREQ: >> + if (val < 0 && val2 != 0) >> + return -EINVAL; >> + return ad7606_set_sampling_freq(st, val); > Currently I think for the !backend + pwm case this can go out of > range for which that code works (fsleep removed in next patch). > Perhaps delay adding this until after that patch. Hi Jonathan, The sampling frequency can be adjusted only for the backend version, otherwise (including pwm+interrupt), there is no sysfs access to the sampling frequency (only available for AD7606_BI_CHANNEL). >> default: >> return -EINVAL; >> } >> @@ -1108,7 +1186,24 @@ int ad7606_probe(struct device *dev, int irq, void __iomem *base_address, >> st->cnvst_pwm); >> if (ret) >> return ret; >> + } >> + >> + if (st->bops->iio_backend_config) { >> + /* >> + * If there is a backend, the PWM should not overpass the maximum sampling >> + * frequency the chip supports. >> + */ >> + ret = ad7606_set_sampling_freq(st, >> + chip_info->max_samplerate ? : 2 * KILO); >> + if (ret) >> + return ret; >> + >> + ret = st->bops->iio_backend_config(dev, indio_dev); >> + if (ret) >> + return ret; >> + indio_dev->setup_ops = &ad7606_pwm_buffer_ops; >> } else { >> + init_completion(&st->completion); >> st->trig = devm_iio_trigger_alloc(dev, "%s-dev%d", >> indio_dev->name, >> iio_device_id(indio_dev)); > It's a little hard to unwind the patches, but this was previously in the !pwm case. > At this point in the series we still allow the pwm case to work with ! backend. > So is this now running in that case? Do we need a temporary additional check > on !pwm mmm actually this should not be in a condition in the PWM patch. Will fix this directly there. > > >> @@ -1126,15 +1221,14 @@ int ad7606_probe(struct device *dev, int irq, void __iomem *base_address, >> &ad7606_buffer_ops); >> if (ret) >> return ret; >> + ret = devm_request_threaded_irq(dev, irq, >> + NULL, >> + &ad7606_interrupt, >> + IRQF_TRIGGER_FALLING | IRQF_ONESHOT, >> + chip_info->name, indio_dev); >> + if (ret) >> + return ret; >> } >> - ret = devm_request_threaded_irq(dev, irq, >> - NULL, >> - &ad7606_interrupt, >> - IRQF_TRIGGER_FALLING | IRQF_ONESHOT, >> - chip_info->name, indio_dev); >> - if (ret) >> - return ret; >> - >> return devm_iio_device_register(dev, indio_dev); >> } >> EXPORT_SYMBOL_NS_GPL(ad7606_probe, IIO_AD7606); >> diff --git a/drivers/iio/adc/ad7606_par.c b/drivers/iio/adc/ad7606_par.c >> index b87be2f1ca04..6042f6799272 100644 >> --- a/drivers/iio/adc/ad7606_par.c >> +++ b/drivers/iio/adc/ad7606_par.c >> @@ -2,7 +2,8 @@ >> + >> +static int ad7606_bi_setup_iio_backend(struct device *dev, struct iio_dev *indio_dev) >> +{ >> + struct ad7606_state *st = iio_priv(indio_dev); >> + unsigned int ret, c; >> + struct iio_backend_data_fmt data = { >> + .sign_extend = true, >> + .enable = true, >> + }; >> + >> + st->back = devm_iio_backend_get(dev, NULL); >> + if (IS_ERR(st->back)) >> + return PTR_ERR(st->back); >> + >> + /* If the device is iio_backend powered the PWM is mandatory */ >> + if (!st->cnvst_pwm) >> + return dev_err_probe(st->dev, -EINVAL, >> + "A PWM is mandatory when using backend.\n"); >> + >> + ret = devm_iio_backend_request_buffer(dev, st->back, indio_dev); >> + if (ret) >> + return ret; >> + >> + ret = devm_iio_backend_enable(dev, st->back); >> + if (ret) >> + return ret; >> + >> + for (c = 0; c < indio_dev->num_channels; c++) { >> + ret = iio_backend_data_format_set(st->back, c, &data); >> + if (ret) >> + return ret; >> + } >> + >> + indio_dev->channels = ad7606b_bi_channels; > Ultimately this may want to move into the chip_info structures as more devices are added > but this is fine for now I suppose. Will do this in a next series where support is added for the other chips. > >> + indio_dev->num_channels = 8; >> + >> + return 0; >> +}
> >> diff --git a/drivers/iio/adc/ad7606.c b/drivers/iio/adc/ad7606.c > >> index 3666a58f8a6f..d86eb7c3e4f7 100644 > >> --- a/drivers/iio/adc/ad7606.c > >> +++ b/drivers/iio/adc/ad7606.c > >> @@ -21,6 +21,7 @@ > >> @@ -737,6 +773,10 @@ static int ad7606_write_raw(struct iio_dev *indio_dev, > >> return ret; > >> > >> return 0; > >> + case IIO_CHAN_INFO_SAMP_FREQ: > >> + if (val < 0 && val2 != 0) > >> + return -EINVAL; > >> + return ad7606_set_sampling_freq(st, val); > > Currently I think for the !backend + pwm case this can go out of > > range for which that code works (fsleep removed in next patch). > > Perhaps delay adding this until after that patch. > > Hi Jonathan, > > The sampling frequency can be adjusted only for the backend version, > otherwise (including pwm+interrupt), there is no sysfs access to the > sampling frequency (only available for AD7606_BI_CHANNEL). Ah! That makes sense. Thanks, J
This series aims to add iio backend support for AD7606X ADCs. In a nutshell, iio backend is a paradigm to shift the logic establishing the connexion between iio buffers and backend buffers into the backend's driver. This provides a more stable programming interface to the driver developers, and give more flexibility in the way the hardware communicates. The support will be first added on AD7606B, and on next patches AD7606C16 and AD7606C18 will be added. The series have been tested on a Zedboard, using the latest HDL available, i.e https://github.com/analogdevicesinc/hdl/commit/7d0a4cee1b5fa403f175af513d7eb804c3bd75d0 and an AD7606B FMCZ EKV. This HDL handles both the conversion trigger (through a PWM), and the end of conversion interruption, and is compatible with axi-adc, which is "iio-backendable". More information about this HDL design can be found at: https://wiki.analog.com/resources/eval/user-guides/ad7606x-fmc/hdl The support is thus separated in two parts: - PWM support was first added. My first intention was to make it available for any version of the driver, but the time required to handle the interruption is not neglectable, and I saw drifts that would eventually cause an overlapping SPI read with a new conversion trigger, whith catastrphic consequences. To mitigate this, CRC check must be implemented, but indeed increasing the samplerate causes more sample to be lost. Therefore, I decided to only allow PWM for iio-backend powered device as a first intention, leaving open the possibility to add the general compatibility afterwards. - IIO backend support was added: Once the PWM support was ready, the driver can be extended to iio-backend. The iio-backend powered version of the driver is a platform driver, and an exemple devicetree node is available in the bindings. The following features will be added in subsequent patch series: - software mode for iio backend - 18 bits mode (AD7606C18) - single read (IIO_CHAN_READ_RAW) Signed-off-by: Guillaume Stols <gstols@baylibre.com> --- Changes in v3: - Rebase on top of the series adding ad7606C16 and AD7606C18 support. - Addition of pwm-names actual values and improvement in the description. - Introduction of .num_adc_channels field in ad7606_chip_info that defines the number of hardware inputs. - Introduction of ad7606_bus_info which couples hardware and wiring informations. - Addition of a delay in the scan_direct function for the backend. - Link to v2: https://lore.kernel.org/r/20240920-ad7606_add_iio_backend_support-v2-0-0e78782ae7d0@baylibre.com Changes in v2: - Logical change in dt-bindings, using a flag for the interface instead of infering it from the value of the "reg" property. - Removal of get_platform_match_data addition, instead the logic is directly used in the file. - Removal of use and export of pwm_get_state_hw, returning the configured frequency instead of the running one. - Correction on various typos, whitespaces, bad order of includes. - Separation of SPI conditions and PWM disabling for no backend in other commits. - Link to v1: https://lore.kernel.org/r/20240815-ad7606_add_iio_backend_support-v1-0-cea3e11b1aa4@baylibre.com --- Guillaume Stols (10): iio: adc: ad7606: Fix typo in the driver name dt-bindings: iio: adc: ad7606: Remove spi-cpha from required dt-bindings: iio: adc: ad7606: Add iio backend bindings Documentation: iio: Document ad7606 driver iio: adc: ad7606: Sort includes in alphabetical order iio: adc: ad7606: Add PWM support for conversion trigger iio: adc: ad7606: Add compatibility to fw_nodes iio: adc: ad7606: Introduce num_adc_channels iio: adc: ad7606: Add iio-backend support iio: adc: ad7606: Disable PWM usage for non backend version .../devicetree/bindings/iio/adc/adi,ad7606.yaml | 72 ++- Documentation/iio/ad7606.rst | 145 ++++++ Documentation/iio/index.rst | 1 + MAINTAINERS | 1 + drivers/iio/adc/Kconfig | 4 +- drivers/iio/adc/ad7606.c | 576 +++++++++++++++------ drivers/iio/adc/ad7606.h | 51 +- drivers/iio/adc/ad7606_par.c | 130 ++++- drivers/iio/adc/ad7606_spi.c | 104 ++-- 9 files changed, 847 insertions(+), 237 deletions(-) --- base-commit: 35307f34d6fef8f9d41a1e8f4f532e4b0a7ee422 change-id: 20240725-ad7606_add_iio_backend_support-c401305a6924 Best regards, -- Guillaume Stols <gstols@baylibre.com>