linux-kernel - Re: [PATCH v4 10/11] iio: dac: ad3552r: add high-speed platform driver

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: <20241006145812.3d6679fa@jic23-huawei>
Date: Sun, 6 Oct 2024 14:58:12 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Angelo Dureghello <adureghello@...libre.com>
Cc: Lars-Peter Clausen <lars@...afoo.de>, Michael Hennerich
 <Michael.Hennerich@...log.com>, Nuno Sa <nuno.sa@...log.com>, Rob Herring
 <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Mihail Chindris <mihail.chindris@...log.com>,
 Olivier Moysan <olivier.moysan@...s.st.com>, linux-iio@...r.kernel.org,
 linux-kernel@...r.kernel.org, Jonathan Cameron
 <Jonathan.Cameron@...wei.com>, devicetree@...r.kernel.org,
 dlechner@...libre.com, Mark Brown <broonie@...nel.org>
Subject: Re: [PATCH v4 10/11] iio: dac: ad3552r: add high-speed platform
 driver

On Thu, 03 Oct 2024 19:29:07 +0200
Angelo Dureghello <adureghello@...libre.com> wrote:

> From: Angelo Dureghello <adureghello@...libre.com>
> 
> Add High Speed ad3552r platform driver.
> 
> The ad3552r DAC is controlled by a custom (fpga-based) DAC IP
> through the current AXI backend, or similar alternative IIO backend.
> 
> Compared to the existing driver (ad3552r.c), that is a simple SPI
> driver, this driver is coupled with a DAC IIO backend that finally
> controls the ad3552r by a fpga-based "QSPI+DDR" interface, to reach
> maximum transfer rate of 33MUPS using dma stream capabilities.
> 
> All commands involving QSPI bus read/write are delegated to the backend
> through the provided APIs for bus read/write.
> 
> Signed-off-by: Angelo Dureghello <adureghello@...libre.com>
Hi Angelo

A few trivial things inline.

Jonathan

>  obj-$(CONFIG_AD5380) += ad5380.o
> diff --git a/drivers/iio/dac/ad3552r-hs.c b/drivers/iio/dac/ad3552r-hs.c
> new file mode 100644
> index 000000000000..1e141d573d76
> --- /dev/null
> +++ b/drivers/iio/dac/ad3552r-hs.c

> +static int ad3552r_hs_setup(struct ad3552r_hs_state *st)
> +{
> +	u8 gs_p, gs_n;
> +	s16 goffs;
> +	u16 id, rfb;
> +	u16 gain = 0, offset = 0;
> +	u32 val, range;
> +	int err;
...

> +	err = ad3552r_get_custom_gain(st->dev, child, &gs_p, &gs_n, &rfb,
> +				      &goffs);
> +	if (err)
> +		return err;
> +
> +	gain = ad3552r_calc_custom_gain(gs_p, gs_n, goffs);
> +	offset = abs((s32)goffs);

Why the cast?  abs is special cased for short which should work with s16 I think.

> +
> +	return ad3552r_hs_setup_custom_gain(st, gain, offset);
> +}


> +static int ad3552r_hs_probe(struct platform_device *pdev)
> +{
> +	struct ad3552r_hs_state *st;
> +	struct iio_dev *indio_dev;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*st));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	st = iio_priv(indio_dev);
> +	st->dev = &pdev->dev;
> +
> +	st->data = pdev->dev.platform_data;
> +	if (!st->data) {
> +		dev_err(&pdev->dev, "no platform data!\n");
> +		return -ENODEV;

Trivial preference for return dev_err_probe() mostly because
we can then drop the brackets.

> +	}

> diff --git a/drivers/iio/dac/ad3552r.h b/drivers/iio/dac/ad3552r.h
> index 088eb8ecfac6..d9da4794fcb3 100644
> --- a/drivers/iio/dac/ad3552r.h
> +++ b/drivers/iio/dac/ad3552r.h

>  #define AD3552R_GAIN_SCALE				1000
>  #define AD3552R_LDAC_PULSE_US				100
>  
> +#define AD3552R_CH0_ACTIVE				BIT(0)
> +#define AD3552R_CH1_ACTIVE				BIT(1)
> +#define AD3552R_CH0_CH1_ACTIVE				(AD3552R_CH0_ACTIVE | \
> +							AD3552R_CH1_ACTIVE)
Add a space before the above. I think it's aligned with the (
but should be just after that.

> +
>  #define AD3552R_MAX_RANGES	5
>  #define AD3542R_MAX_RANGES	6