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@xxxxxxxxxxxx>
> ---
Hi Trevor,
It LGTM, just some minor stuff from me...
With that,
Reviewed-by: Nuno Sa <nuno.sa@xxxxxxxxxx>
> 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á
