linux-kernel - Re: [PATCH v2 4/9] iio: dac: adi-axi-dac: add bus mode setup

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: <50488ccd-8106-4048-9be7-06b2da854f3a@baylibre.com>
Date: Wed, 8 Jan 2025 15:06:19 -0600
From: David Lechner <dlechner@...libre.com>
To: Angelo Dureghello <adureghello@...libre.com>,
 Lars-Peter Clausen <lars@...afoo.de>,
 Michael Hennerich <Michael.Hennerich@...log.com>,
 Jonathan Cameron <jic23@...nel.org>,
 Mihail Chindris <mihail.chindris@...log.com>, Nuno Sa <nuno.sa@...log.com>
Cc: Jonathan Cameron <Jonathan.Cameron@...wei.com>,
 linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 4/9] iio: dac: adi-axi-dac: add bus mode setup

On 1/8/25 11:29 AM, Angelo Dureghello wrote:
> From: Angelo Dureghello <adureghello@...libre.com>
> 
> The ad354xr requires DSPI mode (2 data lanes) to work in buffering
> mode, so backend needs to allow a mode selection between:
>     SPI  (entire ad35xxr family),
>     DSPI (ad354xr),
>     QSPI (ad355xr).
> 

It wouldn't hurt to mention here why AXI_DAC_CUSTOM_CTRL_SYNCED_TRANSFER is
removed. It looks like it was just added by mistake in the original code and
isn't used anywhere.

> Signed-off-by: Angelo Dureghello <adureghello@...libre.com>
> ---
>  drivers/iio/dac/ad3552r-hs.h  |  8 ++++++++
>  drivers/iio/dac/adi-axi-dac.c | 26 +++++++++++++++++++++++++-
>  2 files changed, 33 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/dac/ad3552r-hs.h b/drivers/iio/dac/ad3552r-hs.h
> index 724261d38dea..4a9e35234124 100644
> --- a/drivers/iio/dac/ad3552r-hs.h
> +++ b/drivers/iio/dac/ad3552r-hs.h
> @@ -8,11 +8,19 @@
>  
>  struct iio_backend;
>  
> +enum ad3552r_io_mode {
> +	AD3552R_IO_MODE_SPI,
> +	AD3552R_IO_MODE_DSPI,
> +	AD3552R_IO_MODE_QSPI,
> +};
> +
>  struct ad3552r_hs_platform_data {
>  	int (*bus_reg_read)(struct iio_backend *back, u32 reg, u32 *val,
>  			    size_t data_size);
>  	int (*bus_reg_write)(struct iio_backend *back, u32 reg, u32 val,
>  			     size_t data_size);
> +	int (*bus_set_io_mode)(struct iio_backend *back,
> +			       enum ad3552r_io_mode mode);
>  	u32 bus_sample_data_clock_hz;
>  };
>  
> diff --git a/drivers/iio/dac/adi-axi-dac.c b/drivers/iio/dac/adi-axi-dac.c
> index d02eb535b648..79ca158c1ad9 100644
> --- a/drivers/iio/dac/adi-axi-dac.c
> +++ b/drivers/iio/dac/adi-axi-dac.c
> @@ -64,7 +64,7 @@
>  #define   AXI_DAC_UI_STATUS_IF_BUSY		BIT(4)
>  #define AXI_DAC_CUSTOM_CTRL_REG			0x008C
>  #define   AXI_DAC_CUSTOM_CTRL_ADDRESS		GENMASK(31, 24)
> -#define   AXI_DAC_CUSTOM_CTRL_SYNCED_TRANSFER	BIT(2)
> +#define   AXI_DAC_CUSTOM_CTRL_MULTI_IO_MODE	GENMASK(3, 2)
>  #define   AXI_DAC_CUSTOM_CTRL_STREAM		BIT(1)
>  #define   AXI_DAC_CUSTOM_CTRL_TRANSFER_DATA	BIT(0)
>  
> @@ -725,6 +725,29 @@ static int axi_dac_bus_reg_read(struct iio_backend *back, u32 reg, u32 *val,
>  	return regmap_read(st->regmap, AXI_DAC_CUSTOM_RD_REG, val);
>  }
>  
> +static int axi_dac_bus_set_io_mode(struct iio_backend *back,
> +				   enum ad3552r_io_mode mode)
> +{
> +	struct axi_dac_state *st = iio_backend_get_priv(back);
> +	int ival, ret;

Don't we need to protect against concurrent register access?

	guard(mutex)(&st->lock);

> +
> +	ret = regmap_update_bits(st->regmap, AXI_DAC_CUSTOM_CTRL_REG,
> +				 AXI_DAC_CUSTOM_CTRL_MULTI_IO_MODE,
> +				 FIELD_PREP(AXI_DAC_CUSTOM_CTRL_MULTI_IO_MODE, mode));
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_read_poll_timeout(st->regmap,
> +				       AXI_DAC_UI_STATUS_REG, ival,
> +				       FIELD_GET(AXI_DAC_UI_STATUS_IF_BUSY, ival) == 0,
> +				       10, 100 * KILO);
> +
> +	if (ret == -ETIMEDOUT)
> +		dev_err(st->dev, "AXI read timeout\n");

Same comment here as previous patch, is `dev_err()` really that helpful?

> +
> +	return ret;
> +}
> +
>  static void axi_dac_child_remove(void *data)
>  {
>  	platform_device_unregister(data);
> @@ -736,6 +759,7 @@ static int axi_dac_create_platform_device(struct axi_dac_state *st,
>  	struct ad3552r_hs_platform_data pdata = {
>  		.bus_reg_read = axi_dac_bus_reg_read,
>  		.bus_reg_write = axi_dac_bus_reg_write,
> +		.bus_set_io_mode = axi_dac_bus_set_io_mode,
>  		.bus_sample_data_clock_hz = st->dac_clk_rate,
>  	};
>  	struct platform_device_info pi = {
>