[4/4] iio: dac: mcp4922: add support for mcp48xx series chips
Commit Message
The MCP4801, MCP4802, MCP4811, MCP4812, MCP4821 and MCP4822
DACs are analogous to their MCP49XX counterparts, but contain
an internal 2.048V voltage reference. Add support for them by
refactoring the driver to allow for chips without a vref supply,
and add the necessary device IDs.
Signed-off-by: Nicolas Frattaroli <frattaroli.nicolas@gmail.com>
---
drivers/iio/dac/mcp4922.c | 112 ++++++++++++++++++++++++++++----------
1 file changed, 84 insertions(+), 28 deletions(-)
Comments
Hi,
Looks very good. A few small comments inline.
[...]
> @@ -50,6 +58,35 @@ struct mcp4922_state {
> }, \
> }
>
> +static bool mcp4922_needs_vref(int device_id)
`enum mcp4922_supported_device_ids` instead of `int`. Same for num_channels() below.
> +{
> + switch (device_id) {
> + case ID_MCP4902:
> + case ID_MCP4912:
> + case ID_MCP4921:
> + case ID_MCP4922:
> + return true;
> + default:
> + return false;
> + }
> +}
> [...]
> static int mcp4922_spi_write(struct mcp4922_state *state, u8 addr, u32 val)
> {
> state->mosi[1] = val & 0xff;
> @@ -108,11 +145,17 @@ static int mcp4922_write_raw(struct iio_dev *indio_dev,
> }
> }
>
> -static const struct iio_chan_spec mcp4922_channels[4][MCP4922_NUM_CHANNELS] = {
> +static const struct iio_chan_spec mcp4922_channels[10][MCP4922_NUM_CHANNELS] = {
mcp4922_channels[][MCP4922_NUM_CHANNELS]
So it does not have to be changed again when adding additional devices
in the future.
> [...]
> @@ -197,11 +244,14 @@ static void mcp4922_remove(struct spi_device *spi)
> {
> struct iio_dev *indio_dev = spi_get_drvdata(spi);
> struct mcp4922_state *state;
> + const struct spi_device_id *id = spi_get_device_id(spi);
>
> iio_device_unregister(indio_dev);
> state = iio_priv(indio_dev);
> regulator_disable(state->vdd_reg);
> - regulator_disable(state->vref_reg);
> + if (mcp4922_needs_vref(id->driver_data)) {
Could be `if (state->vref_reg)`, this way you don't need to lookup the
spi_device_id. But either way is fine.
> + regulator_disable(state->vref_reg);
> + }
> }
>
> [...]
On Wed, 5 Apr 2023 07:18:46 -0700
Lars-Peter Clausen <lars@metafoo.de> wrote:
> Hi,
>
> Looks very good. A few small comments inline.
> [...]
> > @@ -50,6 +58,35 @@ struct mcp4922_state {
> > }, \
> > }
> >
> > +static bool mcp4922_needs_vref(int device_id)
>
> `enum mcp4922_supported_device_ids` instead of `int`. Same for num_channels() below.
>
> > +{
> > + switch (device_id) {
> > + case ID_MCP4902:
> > + case ID_MCP4912:
> > + case ID_MCP4921:
> > + case ID_MCP4922:
> > + return true;
> > + default:
> > + return false;
> > + }
> > +}
> > [...]
> > static int mcp4922_spi_write(struct mcp4922_state *state, u8 addr, u32 val)
> > {
> > state->mosi[1] = val & 0xff;
> > @@ -108,11 +145,17 @@ static int mcp4922_write_raw(struct iio_dev *indio_dev,
> > }
> > }
> >
> > -static const struct iio_chan_spec mcp4922_channels[4][MCP4922_NUM_CHANNELS] = {
> > +static const struct iio_chan_spec mcp4922_channels[10][MCP4922_NUM_CHANNELS] = {
>
> mcp4922_channels[][MCP4922_NUM_CHANNELS]
>
> So it does not have to be changed again when adding additional devices
> in the future.
>
> > [...]
> > @@ -197,11 +244,14 @@ static void mcp4922_remove(struct spi_device *spi)
> > {
> > struct iio_dev *indio_dev = spi_get_drvdata(spi);
> > struct mcp4922_state *state;
> > + const struct spi_device_id *id = spi_get_device_id(spi);
> >
> > iio_device_unregister(indio_dev);
> > state = iio_priv(indio_dev);
> > regulator_disable(state->vdd_reg);
> > - regulator_disable(state->vref_reg);
> > + if (mcp4922_needs_vref(id->driver_data)) {
> Could be `if (state->vref_reg)`, this way you don't need to lookup the
> spi_device_id. But either way is fine.
Even better would be to make this driver use devm_ for all unwinding.
That way the regulator_disable() callback registered via
devm_add_action_or_reset() will only be registered if you enabled such
a regulator in the first place.
The rest of this remove is trivial so it would be a nice addition to this
series to get rid of the need to have it at all.
> > + regulator_disable(state->vref_reg);
> > + }
> > }
> >
> > [...]
>
>
On Wed, 5 Apr 2023 16:01:14 +0200
Nicolas Frattaroli <frattaroli.nicolas@gmail.com> wrote:
> The MCP4801, MCP4802, MCP4811, MCP4812, MCP4821 and MCP4822
> DACs are analogous to their MCP49XX counterparts, but contain
> an internal 2.048V voltage reference. Add support for them by
> refactoring the driver to allow for chips without a vref supply,
> and add the necessary device IDs.
>
> Signed-off-by: Nicolas Frattaroli <frattaroli.nicolas@gmail.com>
> ---
> drivers/iio/dac/mcp4922.c | 112 ++++++++++++++++++++++++++++----------
> 1 file changed, 84 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/iio/dac/mcp4922.c b/drivers/iio/dac/mcp4922.c
> index 0b9458cbbcff..56dcb90c8464 100644
> --- a/drivers/iio/dac/mcp4922.c
> +++ b/drivers/iio/dac/mcp4922.c
> @@ -3,7 +3,8 @@
> * mcp4922.c
> *
> * Driver for Microchip Digital to Analog Converters.
> - * Supports MCP4902, MCP4912, and MCP4922.
> + * Supports MCP4902, MCP4912, MCP4921, MCP4922, MCP4801, MCP4802, MCP4811,
> + * MCP4812, MCP4821, and MCP4822.
> *
> * Copyright (c) 2014 EMAC Inc.
> */
> @@ -16,14 +17,21 @@
> #include <linux/regulator/consumer.h>
> #include <linux/bitops.h>
>
> -#define MCP4922_NUM_CHANNELS 2
> -#define MCP4921_NUM_CHANNELS 1
> +#define MCP4922_NUM_CHANNELS 2
> +#define MCP4921_NUM_CHANNELS 1
> +#define MCP48XX_INTERNAL_VREF_MV 2048
>
> enum mcp4922_supported_device_ids {
> ID_MCP4902,
> ID_MCP4912,
> ID_MCP4921,
> ID_MCP4922,
> + ID_MCP4801,
Numeric order preferred.
> + ID_MCP4802,
> + ID_MCP4811,
> + ID_MCP4812,
> + ID_MCP4821,
> + ID_MCP4822,
> };
>
> struct mcp4922_state {
> @@ -50,6 +58,35 @@ struct mcp4922_state {
> }, \
> }
>
> +static bool mcp4922_needs_vref(int device_id)
> +{
> + switch (device_id) {
> + case ID_MCP4902:
> + case ID_MCP4912:
> + case ID_MCP4921:
> + case ID_MCP4922:
> + return true;
> + default:
> + return false;
> + }
> +}
> +
> +static int mcp4922_num_channels(int device_id)
> +{
> + switch (device_id) {
> + case ID_MCP4902:
> + case ID_MCP4912:
> + case ID_MCP4922:
> + case ID_MCP4802:
> + case ID_MCP4812:
> + case ID_MCP4822:
> + return MCP4922_NUM_CHANNELS;
I'd rather see this as an actual number as the define is just reducing
readability in this particular case.
> + default:
> + return MCP4921_NUM_CHANNELS;
> + }
> +
> +}
> +
> static int mcp4922_spi_write(struct mcp4922_state *state, u8 addr, u32 val)
> {
> state->mosi[1] = val & 0xff;
> @@ -108,11 +145,17 @@ static int mcp4922_write_raw(struct iio_dev *indio_dev,
> }
> }
>
> -static const struct iio_chan_spec mcp4922_channels[4][MCP4922_NUM_CHANNELS] = {
> +static const struct iio_chan_spec mcp4922_channels[10][MCP4922_NUM_CHANNELS] = {
> [ID_MCP4902] = { MCP4922_CHAN(0, 8), MCP4922_CHAN(1, 8) },
> [ID_MCP4912] = { MCP4922_CHAN(0, 10), MCP4922_CHAN(1, 10) },
> [ID_MCP4921] = { MCP4922_CHAN(0, 12), {} },
> [ID_MCP4922] = { MCP4922_CHAN(0, 12), MCP4922_CHAN(1, 12) },
> + [ID_MCP4801] = { MCP4922_CHAN(0, 8), {} },
Numeric order preferred.
> + [ID_MCP4802] = { MCP4922_CHAN(0, 8), MCP4922_CHAN(1, 8) },
> + [ID_MCP4811] = { MCP4922_CHAN(0, 10), {} },
> + [ID_MCP4812] = { MCP4922_CHAN(0, 10), MCP4922_CHAN(1, 10) },
> + [ID_MCP4821] = { MCP4922_CHAN(0, 12), {} },
> + [ID_MCP4822] = { MCP4922_CHAN(0, 12), MCP4922_CHAN(1, 12) },
> };
>
> static const struct iio_info mcp4922_info = {
> @@ -133,25 +176,31 @@ static int mcp4922_probe(struct spi_device *spi)
>
> state = iio_priv(indio_dev);
> state->spi = spi;
> - state->vref_reg = devm_regulator_get(&spi->dev, "vref");
> - if (IS_ERR(state->vref_reg))
> - return dev_err_probe(&spi->dev, PTR_ERR(state->vref_reg),
> - "Vref regulator not specified\n");
> + id = spi_get_device_id(spi);
> + if (mcp4922_needs_vref(id->driver_data)) {
> + state->vref_reg = devm_regulator_get(&spi->dev, "vref");
> + if (IS_ERR(state->vref_reg))
> + return dev_err_probe(&spi->dev, PTR_ERR(state->vref_reg),
> + "Vref regulator not specified\n");
>
> - ret = regulator_enable(state->vref_reg);
> - if (ret) {
> - dev_err(&spi->dev, "Failed to enable vref regulator: %d\n",
> - ret);
> - return ret;
> - }
> + ret = regulator_enable(state->vref_reg);
> + if (ret) {
> + dev_err(&spi->dev, "Failed to enable vref regulator: %d\n",
> + ret);
> + return ret;
> + }
If you add a devm_add_action_or_reset() and a suitable callback function to
turn the regulator off again, you can avoid any need for special handing in
remove path. The callback will only get called if were in this block of
code anyway and hence there is something to turn off.
>
> - ret = regulator_get_voltage(state->vref_reg);
> - if (ret < 0) {
> - dev_err(&spi->dev, "Failed to read vref regulator: %d\n",
> - ret);
> - goto error_disable_vref_reg;
> + ret = regulator_get_voltage(state->vref_reg);
> + if (ret < 0) {
> + dev_err(&spi->dev, "Failed to read vref regulator: %d\n",
> + ret);
> + goto error_disable_vref_reg;
> + }
> + state->vref_mv = ret / 1000;
> + } else {
> + state->vref_mv = MCP48XX_INTERNAL_VREF_MV;
> }
> - state->vref_mv = ret / 1000;
> +
Avoid unrelated white space changes
>
> state->vdd_reg = devm_regulator_get(&spi->dev, "vdd");
>
> static const struct spi_device_id mcp4922_id[] = {
> @@ -209,6 +259,12 @@ static const struct spi_device_id mcp4922_id[] = {
> {"mcp4912", ID_MCP4912},
> {"mcp4921", ID_MCP4921},
> {"mcp4922", ID_MCP4922},
> + {"mcp4801", ID_MCP4801},
I'd prefer this table to be in numeric order.
> + {"mcp4802", ID_MCP4802},
> + {"mcp4811", ID_MCP4811},
> + {"mcp4812", ID_MCP4812},
> + {"mcp4821", ID_MCP4821},
> + {"mcp4822", ID_MCP4822},
> {}
> };
> MODULE_DEVICE_TABLE(spi, mcp4922_id);
> @@ -224,5 +280,5 @@ static struct spi_driver mcp4922_driver = {
> module_spi_driver(mcp4922_driver);
>
> MODULE_AUTHOR("Michael Welling <mwelling@ieee.org>");
> -MODULE_DESCRIPTION("Microchip MCP4902, MCP4912, MCP4922 DAC");
> +MODULE_DESCRIPTION("Microchip MCP49XX and MCP48XX DAC");
> MODULE_LICENSE("GPL v2");
@@ -3,7 +3,8 @@
* mcp4922.c
*
* Driver for Microchip Digital to Analog Converters.
- * Supports MCP4902, MCP4912, and MCP4922.
+ * Supports MCP4902, MCP4912, MCP4921, MCP4922, MCP4801, MCP4802, MCP4811,
+ * MCP4812, MCP4821, and MCP4822.
*
* Copyright (c) 2014 EMAC Inc.
*/
@@ -16,14 +17,21 @@
#include <linux/regulator/consumer.h>
#include <linux/bitops.h>
-#define MCP4922_NUM_CHANNELS 2
-#define MCP4921_NUM_CHANNELS 1
+#define MCP4922_NUM_CHANNELS 2
+#define MCP4921_NUM_CHANNELS 1
+#define MCP48XX_INTERNAL_VREF_MV 2048
enum mcp4922_supported_device_ids {
ID_MCP4902,
ID_MCP4912,
ID_MCP4921,
ID_MCP4922,
+ ID_MCP4801,
+ ID_MCP4802,
+ ID_MCP4811,
+ ID_MCP4812,
+ ID_MCP4821,
+ ID_MCP4822,
};
struct mcp4922_state {
@@ -50,6 +58,35 @@ struct mcp4922_state {
}, \
}
+static bool mcp4922_needs_vref(int device_id)
+{
+ switch (device_id) {
+ case ID_MCP4902:
+ case ID_MCP4912:
+ case ID_MCP4921:
+ case ID_MCP4922:
+ return true;
+ default:
+ return false;
+ }
+}
+
+static int mcp4922_num_channels(int device_id)
+{
+ switch (device_id) {
+ case ID_MCP4902:
+ case ID_MCP4912:
+ case ID_MCP4922:
+ case ID_MCP4802:
+ case ID_MCP4812:
+ case ID_MCP4822:
+ return MCP4922_NUM_CHANNELS;
+ default:
+ return MCP4921_NUM_CHANNELS;
+ }
+
+}
+
static int mcp4922_spi_write(struct mcp4922_state *state, u8 addr, u32 val)
{
state->mosi[1] = val & 0xff;
@@ -108,11 +145,17 @@ static int mcp4922_write_raw(struct iio_dev *indio_dev,
}
}
-static const struct iio_chan_spec mcp4922_channels[4][MCP4922_NUM_CHANNELS] = {
+static const struct iio_chan_spec mcp4922_channels[10][MCP4922_NUM_CHANNELS] = {
[ID_MCP4902] = { MCP4922_CHAN(0, 8), MCP4922_CHAN(1, 8) },
[ID_MCP4912] = { MCP4922_CHAN(0, 10), MCP4922_CHAN(1, 10) },
[ID_MCP4921] = { MCP4922_CHAN(0, 12), {} },
[ID_MCP4922] = { MCP4922_CHAN(0, 12), MCP4922_CHAN(1, 12) },
+ [ID_MCP4801] = { MCP4922_CHAN(0, 8), {} },
+ [ID_MCP4802] = { MCP4922_CHAN(0, 8), MCP4922_CHAN(1, 8) },
+ [ID_MCP4811] = { MCP4922_CHAN(0, 10), {} },
+ [ID_MCP4812] = { MCP4922_CHAN(0, 10), MCP4922_CHAN(1, 10) },
+ [ID_MCP4821] = { MCP4922_CHAN(0, 12), {} },
+ [ID_MCP4822] = { MCP4922_CHAN(0, 12), MCP4922_CHAN(1, 12) },
};
static const struct iio_info mcp4922_info = {
@@ -133,25 +176,31 @@ static int mcp4922_probe(struct spi_device *spi)
state = iio_priv(indio_dev);
state->spi = spi;
- state->vref_reg = devm_regulator_get(&spi->dev, "vref");
- if (IS_ERR(state->vref_reg))
- return dev_err_probe(&spi->dev, PTR_ERR(state->vref_reg),
- "Vref regulator not specified\n");
+ id = spi_get_device_id(spi);
+ if (mcp4922_needs_vref(id->driver_data)) {
+ state->vref_reg = devm_regulator_get(&spi->dev, "vref");
+ if (IS_ERR(state->vref_reg))
+ return dev_err_probe(&spi->dev, PTR_ERR(state->vref_reg),
+ "Vref regulator not specified\n");
- ret = regulator_enable(state->vref_reg);
- if (ret) {
- dev_err(&spi->dev, "Failed to enable vref regulator: %d\n",
- ret);
- return ret;
- }
+ ret = regulator_enable(state->vref_reg);
+ if (ret) {
+ dev_err(&spi->dev, "Failed to enable vref regulator: %d\n",
+ ret);
+ return ret;
+ }
- ret = regulator_get_voltage(state->vref_reg);
- if (ret < 0) {
- dev_err(&spi->dev, "Failed to read vref regulator: %d\n",
- ret);
- goto error_disable_vref_reg;
+ ret = regulator_get_voltage(state->vref_reg);
+ if (ret < 0) {
+ dev_err(&spi->dev, "Failed to read vref regulator: %d\n",
+ ret);
+ goto error_disable_vref_reg;
+ }
+ state->vref_mv = ret / 1000;
+ } else {
+ state->vref_mv = MCP48XX_INTERNAL_VREF_MV;
}
- state->vref_mv = ret / 1000;
+
state->vdd_reg = devm_regulator_get(&spi->dev, "vdd");
if (IS_ERR(state->vdd_reg)) {
@@ -167,14 +216,10 @@ static int mcp4922_probe(struct spi_device *spi)
}
spi_set_drvdata(spi, indio_dev);
- id = spi_get_device_id(spi);
indio_dev->info = &mcp4922_info;
indio_dev->modes = INDIO_DIRECT_MODE;
indio_dev->channels = mcp4922_channels[id->driver_data];
- if (id->driver_data == ID_MCP4921)
- indio_dev->num_channels = MCP4921_NUM_CHANNELS;
- else
- indio_dev->num_channels = MCP4922_NUM_CHANNELS;
+ indio_dev->num_channels = mcp4922_num_channels(id->driver_data);
indio_dev->name = id->name;
ret = iio_device_register(indio_dev);
@@ -188,7 +233,9 @@ static int mcp4922_probe(struct spi_device *spi)
error_disable_vdd_reg:
regulator_disable(state->vdd_reg);
error_disable_vref_reg:
- regulator_disable(state->vref_reg);
+ if (mcp4922_needs_vref(id->driver_data))
+ regulator_disable(state->vref_reg);
+
return ret;
}
@@ -197,11 +244,14 @@ static void mcp4922_remove(struct spi_device *spi)
{
struct iio_dev *indio_dev = spi_get_drvdata(spi);
struct mcp4922_state *state;
+ const struct spi_device_id *id = spi_get_device_id(spi);
iio_device_unregister(indio_dev);
state = iio_priv(indio_dev);
regulator_disable(state->vdd_reg);
- regulator_disable(state->vref_reg);
+ if (mcp4922_needs_vref(id->driver_data)) {
+ regulator_disable(state->vref_reg);
+ }
}
static const struct spi_device_id mcp4922_id[] = {
@@ -209,6 +259,12 @@ static const struct spi_device_id mcp4922_id[] = {
{"mcp4912", ID_MCP4912},
{"mcp4921", ID_MCP4921},
{"mcp4922", ID_MCP4922},
+ {"mcp4801", ID_MCP4801},
+ {"mcp4802", ID_MCP4802},
+ {"mcp4811", ID_MCP4811},
+ {"mcp4812", ID_MCP4812},
+ {"mcp4821", ID_MCP4821},
+ {"mcp4822", ID_MCP4822},
{}
};
MODULE_DEVICE_TABLE(spi, mcp4922_id);
@@ -224,5 +280,5 @@ static struct spi_driver mcp4922_driver = {
module_spi_driver(mcp4922_driver);
MODULE_AUTHOR("Michael Welling <mwelling@ieee.org>");
-MODULE_DESCRIPTION("Microchip MCP4902, MCP4912, MCP4922 DAC");
+MODULE_DESCRIPTION("Microchip MCP49XX and MCP48XX DAC");
MODULE_LICENSE("GPL v2");