Re: [PATCH v1 3/4] iio: adc: ltc2378: Enable high-speed data capture

[Marcelo Schmitt <marcelo.schmitt1@gmail.com>]

> > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> > index 70fec8e3e891..b5368ee783f7 100644
> > --- a/drivers/iio/adc/Kconfig
> > +++ b/drivers/iio/adc/Kconfig
> > @@ -944,6 +944,7 @@ config LTC2378
> >  	depends on SPI
> >  	depends on GPIOLIB || PWM
> >  	select IIO_BUFFER
> > +	imply LTC2378_OFFLOAD_BUFFER
> >  	help
> >  	  Say yes here to build support for Analog Devices LTC2378-20 and
> >  	  similar analog to digital converters.
> > @@ -951,6 +952,17 @@ config LTC2378
> >  	  This driver can also be built as a module. If so, the module will
> >  	  be called ltc2378.
> >  
> > +config LTC2378_OFFLOAD_BUFFER
> > +	bool "Offloaded data capture with LTC2378"
> > +	depends on SPI && LTC2378
> > +	depends on SPI_OFFLOAD=y
> > +	depends on PWM=y
> > +	depends on SPI_OFFLOAD_TRIGGER_PWM=y
> > +	depends on IIO_BUFFER_DMA=y
> > +	depends on IIO_BUFFER_DMAENGINE=y
> 
> why do all these have to be built in?  In general I think we need to jusitfy
> why this driver needs to work on systems with out this stuff being built.
> I.e. why do we need the separate optional file at all?

Hi Jonathan, thank you for your review.

I've applied fixes to all issues reported by sashiko, except for Kconfig
configuration (see below) and the is_visible() thing I'm still working on.

My intent with this design choice was to keep chip intrinsic dependencies
separate from SPI offload dependencies. The indented benefit is to enable using
the device without having to enable SPI_OFFLOAD (which is boolean and might not
be supported on all platforms). The offload feature is indeed highly desirable
hence the imply on the main driver Kconfig entry. The other symbols are tristate
so, if we keep this approach, will drop =y.

The separate optional file is part of the push for dependency separation. It
helps avoid #ifdef in c code. If the proposal is not far from something
acceptable, I'd like to have another try of making it work. Still, it's arguable
that the real interest is only on supporting the offload thing. Also, since I
don't have any request to support these without offload, I think it will also be
okay to have everything combined into a single driver. 

Will follow all the other suggestions.

> 
> > +	help
> > +	  Say yes here to build support for high speed data capture with LTC2378
> > +
> >  config LTC2471
> >  	tristate "Linear Technology LTC2471 and LTC2473 ADC driver"
> >  	depends on I2C
> > diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> > index 1814fb78dde3..2fa5dce0ceea 100644
> > --- a/drivers/iio/adc/Makefile
> > +++ b/drivers/iio/adc/Makefile
> > @@ -82,6 +82,7 @@ obj-$(CONFIG_LPC18XX_ADC) += lpc18xx_adc.o
> >  obj-$(CONFIG_LPC32XX_ADC) += lpc32xx_adc.o
> >  obj-$(CONFIG_LTC2309) += ltc2309.o
> >  obj-$(CONFIG_LTC2378) += ltc2378.o
> > +obj-$(CONFIG_LTC2378_OFFLOAD_BUFFER) += ltc2378-offload-buffer.o
> 
> Hmm. This is odd. The driver can be modular but not this and it's build seperately
> rather than as part of the module. I think these bit needs a redesign.
> We definitely want this extra support to be part of the module.  See how something
> like the ADIS IMU library in iio/imu/Makefile is done.

Will check this out too.

> 
> >  obj-$(CONFIG_LTC2471) += ltc2471.o
> >  obj-$(CONFIG_LTC2485) += ltc2485.o
> >  obj-$(CONFIG_LTC2496) += ltc2496.o ltc2497-core.o