Re: [PATCH v5 2/3] iio: adc: ad7625: add driver

[Jonathan Cameron]

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;
> +