On Sun, 27 Oct 2024 14:52:17 -0500
David Lechner <dlechner@xxxxxxxxxxxx> wrote:
> On 10/27/24 4:12 AM, Jonathan Cameron wrote:
> > On Sat, 26 Oct 2024 19:01:53 -0500
> > David Lechner <dlechner@xxxxxxxxxxxx> wrote:
> >
> >> On 10/26/24 11:00 AM, Jonathan Cameron wrote:
> >>> On Wed, 23 Oct 2024 15:59:22 -0500
> >>> David Lechner <dlechner@xxxxxxxxxxxx> 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.
>
>
