| Message ID | 1526293892.12966.21.camel@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 05/14/2018 12:31 PM, Silvan Murer wrote: > This patch adds support for external reference voltage through the regulator framework. > The patch add also the remove function to the device driver. > > Signed-off-by: Silvan Murer <silvan.murer@gmail.com> Hi, Thanks for the patch. A few comments. > --- > .../devicetree/bindings/iio/dac/ltc2632.txt | 9 +++ > drivers/iio/dac/ltc2632.c | 86 +++++++++++++++++----- > 2 files changed, 78 insertions(+), 17 deletions(-) > > diff --git a/Documentation/devicetree/bindings/iio/dac/ltc2632.txt b/Documentation/devicetree/bindings/iio/dac/ltc2632.txt > index eb911e5..d369a4b 100644 > --- a/Documentation/devicetree/bindings/iio/dac/ltc2632.txt > +++ b/Documentation/devicetree/bindings/iio/dac/ltc2632.txt > @@ -14,10 +14,19 @@ apply. In particular, "reg" and "spi-max-frequency" properties must be given. > > Example: > > + vref: regulator-vref { > + compatible = "regulator-fixed"; > + regulator-name = "vref-ltc2632"; > + regulator-min-microvolt = <1250000>; > + regulator-max-microvolt = <1250000>; > + regulator-always-on; > + }; > + > spi_master { > dac: ltc2632@0 { > compatible = "lltc,ltc2632-l12"; > reg = <0>; /* CS0 */ > spi-max-frequency = <1000000>; > + vref-supply = <&vref>; /* optional */ This should not only update the example, but also add a 'optional properties' section where the property is documented. > }; > }; > diff --git a/drivers/iio/dac/ltc2632.c b/drivers/iio/dac/ltc2632.c > index ac5e05f..4a5c5bd 100644 > --- a/drivers/iio/dac/ltc2632.c > +++ b/drivers/iio/dac/ltc2632.c [... > enum ltc2632_supported_device_ids { > @@ -90,7 +96,7 @@ static int ltc2632_read_raw(struct iio_dev *indio_dev, > > switch (m) { > case IIO_CHAN_INFO_SCALE: > - *val = chip_info->vref_mv; > + *val = st->vref_mv; > *val2 = chan->scan_type.realbits; > return IIO_VAL_FRACTIONAL_LOG2; > } > @@ -247,6 +253,41 @@ static int ltc2632_probe(struct spi_device *spi) > chip_info = (struct ltc2632_chip_info *) > spi_get_device_id(spi)->driver_data; > > + st->vref_reg = devm_regulator_get_optional(&spi->dev, "vref"); > + if (IS_ERR(st->vref_reg)) { There are two error cases that should be handled. One is no regulator is specified and the other is a regulator is specified, but something went wrong. In the later case the error should be reported and not ignored. Have a look at e.g. ad5592r-base.c as an example. > + /* use internal reference voltage */ > + st->vref_reg = NULL; > + st->vref_mv = chip_info->vref_mv; > + > + ret = ltc2632_spi_write(spi, LTC2632_CMD_INTERNAL_REFER, > + 0, 0, 0); > + if (ret) { > + dev_err(&spi->dev, > + "Set internal reference command failed, %d\n", > + ret); > + return ret; > + } > + } else { > + /* use external reference voltage */ > + ret = regulator_enable(st->vref_reg); > + if (ret) { > + dev_err(&spi->dev, > + "enable reference regulator failed, %d\n", > + ret); > + return ret; > + } > + st->vref_mv = regulator_get_voltage(st->vref_reg)/1000; Should be space around '/'. > + > + ret = ltc2632_spi_write(spi, LTC2632_CMD_EXTERNAL_REFER, > + 0, 0, 0); > + if (ret) { > + dev_err(&spi->dev, > + "Set external reference command failed, %d\n", > + ret); > + return ret; > + } > + } > + > indio_dev->dev.parent = &spi->dev; > indio_dev->name = dev_of_node(&spi->dev) ? dev_of_node(&spi->dev)->name > : spi_get_device_id(spi)->name; > @@ -255,14 +296,23 @@ static int ltc2632_probe(struct spi_device *spi) > indio_dev->channels = chip_info->channels; > indio_dev->num_channels = LTC2632_DAC_CHANNELS; > > - ret = ltc2632_spi_write(spi, LTC2632_CMD_INTERNAL_REFER, 0, 0, 0); > - if (ret) { > - dev_err(&spi->dev, > - "Set internal reference command failed, %d\n", ret); > - return ret; > + return devm_iio_device_register(&spi->dev, indio_dev); > +} > + > +static int ltc2632_remove(struct spi_device *spi) > +{ > + struct iio_dev *indio_dev = spi_get_drvdata(spi); > + struct ltc2632_state *st = iio_priv(indio_dev); > + > + devm_iio_device_unregister(&spi->dev, indio_dev); > + > + if (st->vref_reg != NULL) { > + regulator_disable(st->vref_reg); > + devm_regulator_put(st->vref_reg); > } > > - return devm_iio_device_register(&spi->dev, indio_dev); > + devm_iio_device_free(&spi->dev, indio_dev); The idea behind the devm_* interface is that you do not explicitly call it in the remove() callback. It will automatically run after the remove function. This means in this case you can remove the devm_regulator_put() and devm_iio_device_free(). The devm_iio_device_unregister() still needs to say since we have to unregister the device before we disable the regulator. But you can simplify this by using the non-managed API (iio_device_unregister()/iio_device_unregister()). > + return 0; > } > > static const struct spi_device_id ltc2632_id[] = { > @@ -276,15 +326,6 @@ static const struct spi_device_id ltc2632_id[] = { > }; > MODULE_DEVICE_TABLE(spi, ltc2632_id); > > -static struct spi_driver ltc2632_driver = { > - .driver = { > - .name = "ltc2632", > - }, > - .probe = ltc2632_probe, > - .id_table = ltc2632_id, > -}; > -module_spi_driver(ltc2632_driver); > - > static const struct of_device_id ltc2632_of_match[] = { > { > .compatible = "lltc,ltc2632-l12", > @@ -309,6 +350,17 @@ static const struct of_device_id ltc2632_of_match[] = { > }; > MODULE_DEVICE_TABLE(of, ltc2632_of_match); > > +static struct spi_driver ltc2632_driver = { > + .driver = { > + .name = "ltc2632", > + .of_match_table = of_match_ptr(ltc2632_of_match), It's a bit strange that of_match_table was not assigned in the first place. I think this should be a separate change and be declared as a fix. > + }, > + .probe = ltc2632_probe, > + .remove = ltc2632_remove, This line uses tabs for alignment, while all the other lines use tabs. > + .id_table = ltc2632_id, > +}; > +module_spi_driver(ltc2632_driver); -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Thanks Lars for the feedback. I created just now, a new patch which includes just the of_match_table fix. For the other stuffe i will create a new version of the patch (v2) soon. On Mon, 2018-05-14 at 13:14 +0200, Lars-Peter Clausen wrote: > On 05/14/2018 12:31 PM, Silvan Murer wrote: > > > > This patch adds support for external reference voltage through the > > regulator framework. > > The patch add also the remove function to the device driver. > > > > Signed-off-by: Silvan Murer <silvan.murer@gmail.com> > Hi, > > Thanks for the patch. A few comments. > > > > > --- > > .../devicetree/bindings/iio/dac/ltc2632.txt | 9 +++ > > drivers/iio/dac/ltc2632.c | 86 > > +++++++++++++++++----- > > 2 files changed, 78 insertions(+), 17 deletions(-) > > > > diff --git a/Documentation/devicetree/bindings/iio/dac/ltc2632.txt > > b/Documentation/devicetree/bindings/iio/dac/ltc2632.txt > > index eb911e5..d369a4b 100644 > > --- a/Documentation/devicetree/bindings/iio/dac/ltc2632.txt > > +++ b/Documentation/devicetree/bindings/iio/dac/ltc2632.txt > > @@ -14,10 +14,19 @@ apply. In particular, "reg" and "spi-max- > > frequency" properties must be given. > > > > Example: > > > > + vref: regulator-vref { > > + compatible = "regulator-fixed"; > > + regulator-name = "vref-ltc2632"; > > + regulator-min-microvolt = <1250000>; > > + regulator-max-microvolt = <1250000>; > > + regulator-always-on; > > + }; > > + > > spi_master { > > dac: ltc2632@0 { > > compatible = "lltc,ltc2632-l12"; > > reg = <0>; /* CS0 */ > > spi-max-frequency = <1000000>; > > + vref-supply = <&vref>; /* optional */ > This should not only update the example, but also add a 'optional > properties' section where the property is documented. > > > > > }; > > }; > > diff --git a/drivers/iio/dac/ltc2632.c b/drivers/iio/dac/ltc2632.c > > index ac5e05f..4a5c5bd 100644 > > --- a/drivers/iio/dac/ltc2632.c > > +++ b/drivers/iio/dac/ltc2632.c > [... > > > > enum ltc2632_supported_device_ids { > > @@ -90,7 +96,7 @@ static int ltc2632_read_raw(struct iio_dev > > *indio_dev, > > > > switch (m) { > > case IIO_CHAN_INFO_SCALE: > > - *val = chip_info->vref_mv; > > + *val = st->vref_mv; > > *val2 = chan->scan_type.realbits; > > return IIO_VAL_FRACTIONAL_LOG2; > > } > > @@ -247,6 +253,41 @@ static int ltc2632_probe(struct spi_device > > *spi) > > chip_info = (struct ltc2632_chip_info *) > > spi_get_device_id(spi)->driver_data; > > > > + st->vref_reg = devm_regulator_get_optional(&spi->dev, > > "vref"); > > + if (IS_ERR(st->vref_reg)) { > There are two error cases that should be handled. One is no regulator > is > specified and the other is a regulator is specified, but something > went > wrong. In the later case the error should be reported and not > ignored. Have > a look at e.g. ad5592r-base.c as an example. > > > > > + /* use internal reference voltage */ > > + st->vref_reg = NULL; > > + st->vref_mv = chip_info->vref_mv; > > + > > + ret = ltc2632_spi_write(spi, > > LTC2632_CMD_INTERNAL_REFER, > > + 0, 0, 0); > > + if (ret) { > > + dev_err(&spi->dev, > > + "Set internal reference command > > failed, %d\n", > > + ret); > > + return ret; > > + } > > + } else { > > + /* use external reference voltage */ > > + ret = regulator_enable(st->vref_reg); > > + if (ret) { > > + dev_err(&spi->dev, > > + "enable reference regulator > > failed, %d\n", > > + ret); > > + return ret; > > + } > > + st->vref_mv = regulator_get_voltage(st- > > >vref_reg)/1000; > Should be space around '/'. > > > > > + > > + ret = ltc2632_spi_write(spi, > > LTC2632_CMD_EXTERNAL_REFER, > > + 0, 0, 0); > > + if (ret) { > > + dev_err(&spi->dev, > > + "Set external reference command > > failed, %d\n", > > + ret); > > + return ret; > > + } > > + } > > + > > indio_dev->dev.parent = &spi->dev; > > indio_dev->name = dev_of_node(&spi->dev) ? > > dev_of_node(&spi->dev)->name > > : > > spi_get_device_id(spi)->name; > > @@ -255,14 +296,23 @@ static int ltc2632_probe(struct spi_device > > *spi) > > indio_dev->channels = chip_info->channels; > > indio_dev->num_channels = LTC2632_DAC_CHANNELS; > > > > - ret = ltc2632_spi_write(spi, LTC2632_CMD_INTERNAL_REFER, > > 0, 0, 0); > > - if (ret) { > > - dev_err(&spi->dev, > > - "Set internal reference command failed, > > %d\n", ret); > > - return ret; > > + return devm_iio_device_register(&spi->dev, indio_dev); > > +} > > + > > +static int ltc2632_remove(struct spi_device *spi) > > +{ > > + struct iio_dev *indio_dev = spi_get_drvdata(spi); > > + struct ltc2632_state *st = iio_priv(indio_dev); > > + > > + devm_iio_device_unregister(&spi->dev, indio_dev); > > + > > + if (st->vref_reg != NULL) { > > + regulator_disable(st->vref_reg); > > + devm_regulator_put(st->vref_reg); > > } > > > > - return devm_iio_device_register(&spi->dev, indio_dev); > > + devm_iio_device_free(&spi->dev, indio_dev); > The idea behind the devm_* interface is that you do not explicitly > call it > in the remove() callback. It will automatically run after the remove > function. > > This means in this case you can remove the devm_regulator_put() and > devm_iio_device_free(). > > The devm_iio_device_unregister() still needs to say since we have to > unregister the device before we disable the regulator. But you can > simplify > this by using the non-managed API > (iio_device_unregister()/iio_device_unregister()). > > > > > + return 0; > > } > > > > static const struct spi_device_id ltc2632_id[] = { > > @@ -276,15 +326,6 @@ static const struct spi_device_id ltc2632_id[] > > = { > > }; > > MODULE_DEVICE_TABLE(spi, ltc2632_id); > > > > -static struct spi_driver ltc2632_driver = { > > - .driver = { > > - .name = "ltc2632", > > - }, > > - .probe = ltc2632_probe, > > - .id_table = ltc2632_id, > > -}; > > -module_spi_driver(ltc2632_driver); > > - > > static const struct of_device_id ltc2632_of_match[] = { > > { > > .compatible = "lltc,ltc2632-l12", > > @@ -309,6 +350,17 @@ static const struct of_device_id > > ltc2632_of_match[] = { > > }; > > MODULE_DEVICE_TABLE(of, ltc2632_of_match); > > > > +static struct spi_driver ltc2632_driver = { > > + .driver = { > > + .name = "ltc2632", > > + .of_match_table = of_match_ptr(ltc2632_of_match), > It's a bit strange that of_match_table was not assigned in the first > place. > I think this should be a separate change and be declared as a fix. > > > > > + }, > > + .probe = ltc2632_probe, > > + .remove = ltc2632_remove, > This line uses tabs for alignment, while all the other lines use > tabs. > > > > > + .id_table = ltc2632_id, > > +}; > > +module_spi_driver(ltc2632_driver); -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/iio/dac/ltc2632.txt b/Documentation/devicetree/bindings/iio/dac/ltc2632.txt index eb911e5..d369a4b 100644 --- a/Documentation/devicetree/bindings/iio/dac/ltc2632.txt +++ b/Documentation/devicetree/bindings/iio/dac/ltc2632.txt @@ -14,10 +14,19 @@ apply. In particular, "reg" and "spi-max-frequency" properties must be given. Example: + vref: regulator-vref { + compatible = "regulator-fixed"; + regulator-name = "vref-ltc2632"; + regulator-min-microvolt = <1250000>; + regulator-max-microvolt = <1250000>; + regulator-always-on; + }; + spi_master { dac: ltc2632@0 { compatible = "lltc,ltc2632-l12"; reg = <0>; /* CS0 */ spi-max-frequency = <1000000>; + vref-supply = <&vref>; /* optional */ }; }; diff --git a/drivers/iio/dac/ltc2632.c b/drivers/iio/dac/ltc2632.c index ac5e05f..4a5c5bd 100644 --- a/drivers/iio/dac/ltc2632.c +++ b/drivers/iio/dac/ltc2632.c @@ -2,6 +2,7 @@ * LTC2632 Digital to analog convertors spi driver * * Copyright 2017 Maxime Roussin-Bélanger + * expanded by Silvan Murer <silvan.murer@gmail.com> * * Licensed under the GPL-2. */ @@ -10,6 +11,7 @@ #include <linux/spi/spi.h> #include <linux/module.h> #include <linux/iio/iio.h> +#include <linux/regulator/consumer.h> #define LTC2632_DAC_CHANNELS 2 @@ -28,7 +30,7 @@ /** * struct ltc2632_chip_info - chip specific information * @channels: channel spec for the DAC - * @vref_mv: reference voltage + * @vref_mv: internal reference voltage */ struct ltc2632_chip_info { const struct iio_chan_spec *channels; @@ -39,10 +41,14 @@ struct ltc2632_chip_info { * struct ltc2632_state - driver instance specific data * @spi_dev: pointer to the spi_device struct * @powerdown_cache_mask used to show current channel powerdown state + * @vref_mv used reference voltage (internal or external) + * @vref_reg regulator for the reference voltage */ struct ltc2632_state { struct spi_device *spi_dev; unsigned int powerdown_cache_mask; + int vref_mv; + struct regulator *vref_reg; }; enum ltc2632_supported_device_ids { @@ -90,7 +96,7 @@ static int ltc2632_read_raw(struct iio_dev *indio_dev, switch (m) { case IIO_CHAN_INFO_SCALE: - *val = chip_info->vref_mv; + *val = st->vref_mv; *val2 = chan->scan_type.realbits; return IIO_VAL_FRACTIONAL_LOG2; } @@ -247,6 +253,41 @@ static int ltc2632_probe(struct spi_device *spi) chip_info = (struct ltc2632_chip_info *) spi_get_device_id(spi)->driver_data; + st->vref_reg = devm_regulator_get_optional(&spi->dev, "vref"); + if (IS_ERR(st->vref_reg)) { + /* use internal reference voltage */ + st->vref_reg = NULL; + st->vref_mv = chip_info->vref_mv; + + ret = ltc2632_spi_write(spi, LTC2632_CMD_INTERNAL_REFER, + 0, 0, 0); + if (ret) { + dev_err(&spi->dev, + "Set internal reference command failed, %d\n", + ret); + return ret; + } + } else { + /* use external reference voltage */ + ret = regulator_enable(st->vref_reg); + if (ret) { + dev_err(&spi->dev, + "enable reference regulator failed, %d\n", + ret); + return ret; + } + st->vref_mv = regulator_get_voltage(st->vref_reg)/1000; + + ret = ltc2632_spi_write(spi, LTC2632_CMD_EXTERNAL_REFER, + 0, 0, 0); + if (ret) { + dev_err(&spi->dev, + "Set external reference command failed, %d\n", + ret); + return ret; + } + } + indio_dev->dev.parent = &spi->dev; indio_dev->name = dev_of_node(&spi->dev) ? dev_of_node(&spi->dev)->name : spi_get_device_id(spi)->name; @@ -255,14 +296,23 @@ static int ltc2632_probe(struct spi_device *spi) indio_dev->channels = chip_info->channels; indio_dev->num_channels = LTC2632_DAC_CHANNELS; - ret = ltc2632_spi_write(spi, LTC2632_CMD_INTERNAL_REFER, 0, 0, 0); - if (ret) { - dev_err(&spi->dev, - "Set internal reference command failed, %d\n", ret); - return ret; + return devm_iio_device_register(&spi->dev, indio_dev); +} + +static int ltc2632_remove(struct spi_device *spi) +{ + struct iio_dev *indio_dev = spi_get_drvdata(spi); + struct ltc2632_state *st = iio_priv(indio_dev); + + devm_iio_device_unregister(&spi->dev, indio_dev); + + if (st->vref_reg != NULL) { + regulator_disable(st->vref_reg); + devm_regulator_put(st->vref_reg); } - return devm_iio_device_register(&spi->dev, indio_dev); + devm_iio_device_free(&spi->dev, indio_dev); + return 0; } static const struct spi_device_id ltc2632_id[] = { @@ -276,15 +326,6 @@ static const struct spi_device_id ltc2632_id[] = { }; MODULE_DEVICE_TABLE(spi, ltc2632_id); -static struct spi_driver ltc2632_driver = { - .driver = { - .name = "ltc2632", - }, - .probe = ltc2632_probe, - .id_table = ltc2632_id, -}; -module_spi_driver(ltc2632_driver); - static const struct of_device_id ltc2632_of_match[] = { { .compatible = "lltc,ltc2632-l12", @@ -309,6 +350,17 @@ static const struct of_device_id ltc2632_of_match[] = { }; MODULE_DEVICE_TABLE(of, ltc2632_of_match); +static struct spi_driver ltc2632_driver = { + .driver = { + .name = "ltc2632", + .of_match_table = of_match_ptr(ltc2632_of_match), + }, + .probe = ltc2632_probe, + .remove = ltc2632_remove, + .id_table = ltc2632_id, +}; +module_spi_driver(ltc2632_driver); + MODULE_AUTHOR("Maxime Roussin-Belanger <maxime.roussinbelanger@gmail.com>"); MODULE_DESCRIPTION("LTC2632 DAC SPI driver"); MODULE_LICENSE("GPL v2");
This patch adds support for external reference voltage through the regulator framework. The patch add also the remove function to the device driver. Signed-off-by: Silvan Murer <silvan.murer@gmail.com> --- .../devicetree/bindings/iio/dac/ltc2632.txt | 9 +++ drivers/iio/dac/ltc2632.c | 86 +++++++++++++++++----- 2 files changed, 78 insertions(+), 17 deletions(-) -- 2.7.4 -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html