[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ce99d0a4-2a52-43f3-92ea-6e3ed9174956@baylibre.com>
Date: Thu, 5 Sep 2024 12:18:39 -0400
From: Trevor Gamblin <tgamblin@...libre.com>
To: Nuno Sá <noname.nuno@...il.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 2024-09-05 3:58 a.m., Nuno Sá wrote:
> 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()?
Uwe rightly pointed out (internally) that freq probably shouldn't be
signed, so I'll adjust that instead and resubmit.
>
>> + 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.
Good point, I'll change that too.
Thanks!
>
> - Nuno Sá
>
>
Powered by blists - more mailing lists