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