| Message ID | 20221111143921.742194-1-linux@rasmusvillemoes.dk |
|---|---|
| Headers | show |
| Series | iio: addac: ad74413r: various fixups | expand |
On Fri, 11 Nov 2022 15:39:17 +0100 Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote: > Silence the run-time warning > > SPI driver ad74413r has no spi_device_id for adi,ad74412r > > Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk> > --- > drivers/iio/addac/ad74413r.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/drivers/iio/addac/ad74413r.c b/drivers/iio/addac/ad74413r.c > index 899bcd83f40b..37485be88a63 100644 > --- a/drivers/iio/addac/ad74413r.c > +++ b/drivers/iio/addac/ad74413r.c > @@ -1457,12 +1457,20 @@ static const struct of_device_id ad74413r_dt_id[] = { > }; > MODULE_DEVICE_TABLE(of, ad74413r_dt_id); > > +static const struct spi_device_id ad74413r_spi_id[] = { > + { .name = "ad74412r", .driver_data = (kernel_ulong_t)&ad74412r_chip_info_data }, > + { .name = "ad74413r", .driver_data = (kernel_ulong_t)&ad74413r_chip_info_data }, > + {}, Trivial, but prefer not to have a comma after a "NULL" terminator like this. It would never make sense to add anything after it in the array. Now you are matching existing driver style, but I'd still rather not see more instances of this added. Also, driver_data is not currently used. It should be because adding this spi_id table means the driver can be probed via various routes where device_get_match_data() == NULL. Hence, alongside this change you need to have a fallback to cover that case. Something along the lines of... st->chip_info = device_get_match_data(..); if (!st->chip_info) { struct spi_device_id *id = spi_get_device_id(); if (!id) return -EINVAL; st->chip_info = (void *)id->driver_data; //or better yet cast to the correct type I'm just too lazy to look it up ;) if (!st->chip_info) return -EINVAL; } > +}; > +MODULE_DEVICE_TABLE(spi, ad74413r_spi_id); > + > static struct spi_driver ad74413r_driver = { > .driver = { > .name = "ad74413r", > .of_match_table = ad74413r_dt_id, > }, > .probe = ad74413r_probe, > + .id_table = ad74413r_spi_id, > }; > > module_driver(ad74413r_driver,
On Fri, 11 Nov 2022 15:39:19 +0100 Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote: > The ad74412r/ad74413r has an internal 2.5V reference output, which (by > tying the REFOUT pin to the REFIN pin) can be used in lieu of an > external 2.5V input reference. > > Support that case by using devm_regulator_get_optional(), and simply > hardcode the 2500000 uV in ad74413r_get_output_current_scale(). > > I'm not sure this is completely correct, but it's certainly better > than the current behaviour, where when refin-supply is not defined in > device tree, the regulator framework helpfully does its > > supply refin not found, using dummy regulator You could reasonably assume that's a bug in the firmware.. See suggestions in reply to patch 2. Given external wiring is involved, I don't think we can assume absence of a regulator means that loop back is in place. We need to indicate that explicitly in the binding in some way. Jonathan > > thing. When we then do the regulator_get_voltage(), that dummy > regulator of course doesn't support that operation and thus returns > -22 (-EINVAL) which is used without being checked. > > Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk> > --- > drivers/iio/addac/ad74413r.c | 31 ++++++++++++++++++++----------- > 1 file changed, 20 insertions(+), 11 deletions(-) > > diff --git a/drivers/iio/addac/ad74413r.c b/drivers/iio/addac/ad74413r.c > index 37485be88a63..9f77d2f514de 100644 > --- a/drivers/iio/addac/ad74413r.c > +++ b/drivers/iio/addac/ad74413r.c > @@ -608,7 +608,10 @@ static int ad74413r_get_output_voltage_scale(struct ad74413r_state *st, > static int ad74413r_get_output_current_scale(struct ad74413r_state *st, > int *val, int *val2) > { > - *val = regulator_get_voltage(st->refin_reg); > + if (st->refin_reg) > + *val = regulator_get_voltage(st->refin_reg); > + else > + *val = 2500000; > *val2 = st->sense_resistor_ohms * AD74413R_DAC_CODE_MAX * 1000; > > return IIO_VAL_FRACTIONAL; > @@ -1313,19 +1316,25 @@ static int ad74413r_probe(struct spi_device *spi) > if (IS_ERR(st->regmap)) > return PTR_ERR(st->regmap); > > - st->refin_reg = devm_regulator_get(st->dev, "refin"); > - if (IS_ERR(st->refin_reg)) > - return dev_err_probe(st->dev, PTR_ERR(st->refin_reg), > - "Failed to get refin regulator\n"); > + st->refin_reg = devm_regulator_get_optional(st->dev, "refin"); > + if (IS_ERR(st->refin_reg)) { > + ret = PTR_ERR(st->refin_reg); > + if (ret != -ENODEV) > + return dev_err_probe(st->dev, ret, > + "Failed to get refin regulator\n"); > + st->refin_reg = NULL; > + } > > - ret = regulator_enable(st->refin_reg); > - if (ret) > - return ret; > + if (st->refin_reg) { > + ret = regulator_enable(st->refin_reg); > + if (ret) > + return ret; > > - ret = devm_add_action_or_reset(st->dev, ad74413r_regulator_disable, > + ret = devm_add_action_or_reset(st->dev, ad74413r_regulator_disable, > st->refin_reg); > - if (ret) > - return ret; > + if (ret) > + return ret; > + } > > st->sense_resistor_ohms = 100000000; > device_property_read_u32(st->dev, "shunt-resistor-micro-ohms",