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@xxxxxxxxx>
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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
