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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20241028163907.00007e12@Huawei.com>
Date: Mon, 28 Oct 2024 16:39:07 +0000
From: Jonathan Cameron <Jonathan.Cameron@...wei.com>
To: David Lechner <dlechner@...libre.com>
CC: Jonathan Cameron <jic23@...nel.org>, Mark Brown <broonie@...nel.org>, Rob
 Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor
 Dooley <conor+dt@...nel.org>, Nuno Sá
	<nuno.sa@...log.com>, Uwe Kleine-König
	<ukleinek@...nel.org>, Michael Hennerich <Michael.Hennerich@...log.com>,
	Lars-Peter Clausen <lars@...afoo.de>, David Jander <david@...tonic.nl>,
	Martin Sperl <kernel@...tin.sperl.org>, <linux-spi@...r.kernel.org>,
	<devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
	<linux-iio@...r.kernel.org>, <linux-pwm@...r.kernel.org>
Subject: Re: [PATCH RFC v4 15/15] iio: adc: ad4695: Add support for SPI
 offload

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

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

> 
>