linux-kernel - Re: [PATCH v4 2/3] iio: adc: ad7625: add 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: <23c81d6735075c0b9d98833641606a661fab7194.camel@gmail.com>
Date: Thu, 05 Sep 2024 09:58:22 +0200
From: Nuno Sá <noname.nuno@...il.com>
To: Trevor Gamblin <tgamblin@...libre.com>, Lars-Peter Clausen
 <lars@...afoo.de>,  Michael Hennerich <Michael.Hennerich@...log.com>, Nuno
 Sá <nuno.sa@...log.com>, Jonathan Cameron
 <jic23@...nel.org>, Rob Herring <robh@...nel.org>, Krzysztof Kozlowski
 <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, Jonathan Corbet
 <corbet@....net>, David Lechner <dlechner@...libre.com>, Uwe Kleine-Konig
 <u.kleine-koenig@...libre.com>
Cc: linux-iio@...r.kernel.org, devicetree@...r.kernel.org, 
	linux-kernel@...r.kernel.org, linux-doc@...r.kernel.org
Subject: Re: [PATCH v4 2/3] iio: adc: ad7625: add driver

On Wed, 2024-09-04 at 15:14 -0400, Trevor Gamblin wrote:
> Add a driver for the AD762x and AD796x family of ADCs. These are
> pin-compatible devices using an LVDS interface for data transfer,
> capable of sampling at rates of 6 (AD7625), 10 (AD7626), and 5
> (AD7960/AD7961) MSPS, respectively. They also feature multiple voltage
> reference options based on the configuration of the EN1/EN0 pins, which
> can be set in the devicetree.
> 
> Signed-off-by: Trevor Gamblin <tgamblin@...libre.com>
> ---

Hi Trevor,

It LGTM, just some minor stuff from me...

With that,

Reviewed-by: Nuno Sa <nuno.sa@...log.com>

>  MAINTAINERS              |   1 +
>  drivers/iio/adc/Kconfig  |  16 ++
>  drivers/iio/adc/Makefile |   1 +
>  drivers/iio/adc/ad7625.c | 684
> +++++++++++++++++++++++++++++++++++++++++++++++
> 

...

> +
> +static int ad7625_set_sampling_freq(struct ad7625_state *st, int freq)
> +{
> +	u64 target;
> +	struct pwm_waveform clk_gate_wf = { }, cnv_wf = { };
> +	int ret;
> +
> +	target = DIV_ROUND_UP_ULL(NSEC_PER_SEC, freq);

Not seeing any reason why it can't be DIV_ROUND_UP()?

> +	cnv_wf.period_length_ns = clamp(target, 100, 10 * KILO);
> +
> +	/*
> +	 * Use the maximum conversion time t_CNVH from the datasheet as
> +	 * the duty_cycle for ref_clk, cnv, and clk_gate
> +	 */
> +	cnv_wf.duty_length_ns = st->info->timing_spec->conv_high_ns;
> +
> +	ret = pwm_round_waveform_might_sleep(st->cnv_pwm, &cnv_wf);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * Set up the burst signal for transferring data. period and
> +	 * offset should mirror the CNV signal
> +	 */
> +	clk_gate_wf.period_length_ns = cnv_wf.period_length_ns;
> +
> +	clk_gate_wf.duty_length_ns = DIV_ROUND_UP_ULL((u64)NSEC_PER_SEC *
> +		st->info->chan_spec.scan_type.realbits,
> +		st->ref_clk_rate_hz);
> +
> +	/* max t_MSB from datasheet */
> +	clk_gate_wf.duty_offset_ns = st->info->timing_spec->conv_msb_ns;
> +
> +	ret = pwm_round_waveform_might_sleep(st->clk_gate_pwm, &clk_gate_wf);
> +	if (ret)
> +		return ret;
> +
> +	st->cnv_wf = cnv_wf;
> +	st->clk_gate_wf = clk_gate_wf;
> +
> +	/* TODO: Add a rounding API for PWMs that can simplify this */
> +	target = DIV_ROUND_CLOSEST_ULL(st->ref_clk_rate_hz, freq);

ditto...

> +	st->sampling_freq_hz = DIV_ROUND_CLOSEST_ULL(st->ref_clk_rate_hz,
> +						     target);
> +
> +	return 0;
> +}
> +
> 

...

> +
> +static int ad7625_buffer_preenable(struct iio_dev *indio_dev)
> +{
> +	struct ad7625_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	ret = pwm_set_waveform_might_sleep(st->cnv_pwm, &st->cnv_wf, false);
> +	if (ret)
> +		return ret;
> +
> +	ret = pwm_set_waveform_might_sleep(st->clk_gate_pwm,
> +					   &st->clk_gate_wf, false);
> +	if (ret) {
> +		/* Disable cnv PWM if clk_gate setup failed */
> +		pwm_disable(st->cnv_pwm);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int ad7625_buffer_postdisable(struct iio_dev *indio_dev)
> +{
> +	struct ad7625_state *st = iio_priv(indio_dev);
> +
> +	pwm_disable(st->cnv_pwm);
> +	pwm_disable(st->clk_gate_pwm);
> +
> +	return 0;
> +}
> +

Might not matter but it is a good practise to disable things in the reverse
order.

- Nuno Sá