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á