On Mon, 09 Sep 2024 10:30:48 -0400
Trevor Gamblin <tgamblin@xxxxxxxxxxxx> 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.
>
> Reviewed-by: Nuno Sa <nuno.sa@xxxxxxxxxx>
> Signed-off-by: Trevor Gamblin <tgamblin@xxxxxxxxxxxx>
0-day found an issue. I've fixed up as:
diff --git a/drivers/iio/adc/ad7625.c b/drivers/iio/adc/ad7625.c
index 19c15002d96c..ddd1e4a26429 100644
--- a/drivers/iio/adc/ad7625.c
+++ b/drivers/iio/adc/ad7625.c
@@ -452,9 +452,10 @@ static const struct iio_buffer_setup_ops ad7625_buffer_setup_ops = {
.postdisable = &ad7625_buffer_postdisable,
};
-static int devm_ad7625_pwm_get(struct device *dev, struct clk *ref_clk,
+static int devm_ad7625_pwm_get(struct device *dev,
struct ad7625_state *st)
{
+ struct clk *ref_clk;
u32 ref_clk_rate_hz;
st->cnv_pwm = devm_pwm_get(dev, "cnv");
@@ -556,7 +557,6 @@ static int ad7625_probe(struct platform_device *pdev)
struct device *dev = &pdev->dev;
struct iio_dev *indio_dev;
struct ad7625_state *st;
- struct clk *ref_clk;
int ret;
u32 default_sample_freq;
@@ -610,7 +610,7 @@ static int ad7625_probe(struct platform_device *pdev)
"failed to set EN pins\n");
}
- ret = devm_ad7625_pwm_get(dev, ref_clk, st);
+ ret = devm_ad7625_pwm_get(dev, st);
if (ret)
return ret;
Let me know if I've missed something.
I'd guess this is a case of code evolving into a dead end ;)
The bots win again.
Jonathan
> +static int devm_ad7625_pwm_get(struct device *dev, struct clk *ref_clk,
> + struct ad7625_state *st)
Here ref_clk is passed in but then overwritten in here.
> +{
> + u32 ref_clk_rate_hz;
> +
> + st->cnv_pwm = devm_pwm_get(dev, "cnv");
> + if (IS_ERR(st->cnv_pwm))
> + return dev_err_probe(dev, PTR_ERR(st->cnv_pwm),
> + "failed to get cnv pwm\n");
> +
> + /* Preemptively disable the PWM in case it was enabled at boot */
> + pwm_disable(st->cnv_pwm);
> +
> + st->clk_gate_pwm = devm_pwm_get(dev, "clk_gate");
> + if (IS_ERR(st->clk_gate_pwm))
> + return dev_err_probe(dev, PTR_ERR(st->clk_gate_pwm),
> + "failed to get clk_gate pwm\n");
> +
> + /* Preemptively disable the PWM in case it was enabled at boot */
> + pwm_disable(st->clk_gate_pwm);
> +
> + ref_clk = devm_clk_get_enabled(dev, NULL);
> + if (IS_ERR(ref_clk))
> + return dev_err_probe(dev, PTR_ERR(ref_clk),
> + "failed to get ref_clk");
> +
> + ref_clk_rate_hz = clk_get_rate(ref_clk);
> + if (!ref_clk_rate_hz)
> + return dev_err_probe(dev, -EINVAL,
> + "failed to get ref_clk rate");
> +
> + st->ref_clk_rate_hz = ref_clk_rate_hz;
> +
> + return 0;
> +}
> +
> +static int ad7625_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct iio_dev *indio_dev;
> + struct ad7625_state *st;
> + struct clk *ref_clk;
> + int ret;
> + u32 default_sample_freq;
> +
> + indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + st = iio_priv(indio_dev);
> +
> + st->info = device_get_match_data(dev);
> + if (!st->info)
> + return dev_err_probe(dev, -EINVAL, "no chip info\n");
> +
> + if (device_property_read_bool(dev, "adi,no-dco"))
> + return dev_err_probe(dev, -EINVAL,
> + "self-clocked mode not supported\n");
> +
> + if (st->info->has_bandwidth_control)
> + ret = ad7625_parse_mode(dev, st, 4);
> + else
> + ret = ad7625_parse_mode(dev, st, 2);
> +
> + if (ret)
> + return ret;
> +
> + ret = devm_ad7625_regulator_setup(dev, st);
> + if (ret)
> + return ret;
> +
> + /* Set the device mode based on detected EN configuration. */
> + if (!st->info->has_bandwidth_control) {
> + ad7625_set_en_gpios_for_vref(st, st->have_refin, st->vref_mv);
> + } else {
> + /*
> + * If neither sampling mode is available, then report an error,
> + * since the other modes are not useful defaults.
> + */
> + if (st->can_wide_bandwidth) {
> + ret = ad7960_set_mode(st, AD7960_MODE_WIDE_BANDWIDTH,
> + st->have_refin, st->vref_mv);
> + } else if (st->can_narrow_bandwidth) {
> + ret = ad7960_set_mode(st, AD7960_MODE_NARROW_BANDWIDTH,
> + st->have_refin, st->vref_mv);
> + } else {
> + return dev_err_probe(dev, -EINVAL,
> + "couldn't set device to wide or narrow bandwidth modes\n");
> + }
> +
> + if (ret)
> + return dev_err_probe(dev, -EINVAL,
> + "failed to set EN pins\n");
> + }
> +
> + ret = devm_ad7625_pwm_get(dev, ref_clk, st);
We pass ref_clk in but don't update the underlying pointer (just a copy of
it) and don't need it out here anyway.
I've dropped the parameter and moved the definition into pwm_get()
> + if (ret)
> + return ret;
> +
