Re: [PATCH RFC v4 15/15] iio: adc: ad4695: Add support for SPI offload

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Sun, 27 Oct 2024 14:52:17 -0500
David Lechner <dlechner@baylibre.com> wrote:

> On 10/27/24 4:12 AM, Jonathan Cameron wrote:
> > On Sat, 26 Oct 2024 19:01:53 -0500
> > David Lechner <dlechner@baylibre.com> wrote:
> >   
> >> On 10/26/24 11:00 AM, Jonathan Cameron wrote:  
> >>> On Wed, 23 Oct 2024 15:59:22 -0500
> >>> David Lechner <dlechner@baylibre.com> wrote:
> >>>     
> 
> ...
> 
> >>>     
> >>>>  static int ad4695_probe(struct spi_device *spi)
> >>>>  {
> >>>>  	struct device *dev = &spi->dev;
> >>>>  	struct ad4695_state *st;
> >>>>  	struct iio_dev *indio_dev;
> >>>> -	struct gpio_desc *cnv_gpio;
> >>>>  	bool use_internal_ldo_supply;
> >>>>  	bool use_internal_ref_buffer;
> >>>>  	int ret;
> >>>>  
> >>>> -	cnv_gpio = devm_gpiod_get_optional(dev, "cnv", GPIOD_OUT_LOW);
> >>>> -	if (IS_ERR(cnv_gpio))
> >>>> -		return dev_err_probe(dev, PTR_ERR(cnv_gpio),
> >>>> -				     "Failed to get CNV GPIO\n");
> >>>> -
> >>>> -	/* Driver currently requires CNV pin to be connected to SPI CS */
> >>>> -	if (cnv_gpio)
> >>>> -		return dev_err_probe(dev, -ENODEV,
> >>>> -				     "CNV GPIO is not supported\n");
> >>>> -
> >>>>  	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
> >>>>  	if (!indio_dev)
> >>>>  		return -ENOMEM;
> >>>> @@ -1002,8 +1374,13 @@ static int ad4695_probe(struct spi_device *spi)
> >>>>  		return -EINVAL;
> >>>>  
> >>>>  	/* Registers cannot be read at the max allowable speed */
> >>>> +	st->spi_max_speed_hz = spi->max_speed_hz;
> >>>>  	spi->max_speed_hz = AD4695_REG_ACCESS_SCLK_HZ;
> >>>>  
> >>>> +	ret = devm_add_action_or_reset(dev, ad4695_restore_spi_max_speed_hz, st);    
> >>>
> >>> Why do you need to put it back in devm? What happens after this but without
> >>> a driver restart that uses that faster rate?
> >>>     
> >> I should have added a comment here as this was a weird bug to trace.
> >>
> >> The core SPI framework sets the initial value of spi->max_speed_hz
> >> to the minimum of the controller max rate and the max rate specified
> >> by the devicetree.
> >>
> >> The SPI device lives beyond this driver, so if we bind the driver
> >> and set spi->max_speed_hz to something other than what the SPI core
> >> set it, then the next time we bind the driver, we don't get the
> >> the max rate from the SPI core, but rather we changed it to when
> >> the driver unbound.
> >>
> >> So on the second bind, the max rate would be the slow register
> >> read rate instead of the actual max allowable rate.
> >>
> >> So we need to reset spi->max_speed_hz to what it was originally
> >> on driver unbind so that everything works as expected on the
> >> next bind.
> >>
> >> (Or we call this a SPI core bug and fix it there instead).  
> > Definitely a question to ask.  Directly accessing spi_max_speed_hz may
> > be the fundamental issue as I don't think the driver is generally
> > expected to touch that in a dynamic fashion.  Should we be instead setting it
> > per transfer for the ones that need it controlled?
> > 
> > Jonathan
> >   
> 
> The problem is that we are using regmap and that doesn't have
> a way to specify the max frequency for register reads that is
> different from other uses of the SPI bus (i.e. reading sample
> data). So we could fix it in the generic SPI regmap (not exactly
> trivial) or we could write our own regmap read/write callbacks
> in this driver that properly sets the per-transfer max speed.

Custom read / write callbacks seems the best approach at first
glance, given this is pretty rare thing to do. 

> 
>