This series aims to add support for the following DAC chips by Microchip:
- MCP4801 (8 bit, 1 channel)
- MCP4802 (8 bit, 2 channels)
- MCP4811 (10 bit, 1 channel)
- MCP4812 (10 bit, 2 channels)
- MCP4821 (12 bit, 1 channel)
- MCP4822 (12 bit, 2 channels)
The chips' interface is compatible with that of the MCP49XX ones, but they
come with an internal 2.048V voltage reference, meaning there is no need
for an external voltage reference (and no way to use one.)
The first patch rectifies an issue with the mcp4922 bindings where there
was no vdd-supply property modeled. Those chips do have a second supply
aside from vref, the vdd-supply, which powers the digital logic side of
things.
Patch number 2 uses the vdd regulator in the driver. This is just fairly
boring "enable it here, disable it here" type stuff.
Patch number 3 adds the bindings for the MCP48XX chips. We add the whole
family, not just the ones that have an analogous 4922 enum. There is a
need for a separate binding as making vref only required for some in the
mcp4922 binding would make it more complicated than it's worth.
Patch number 4 finally makes all the driver changes to support the
MCP48XX chips.
All changes have been tested on an MCP4821. I don't own the full line of
MCP48XX and MCP49XX chips, so couldn't exhaustively test everything for
all of them, but if Microchip feels like sending some over then I am more
than happy to receive them.
Kind regards,
Nicolas Frattaroli
Nicolas Frattaroli (4):
dt-bindings: iio: dac: mcp4922: add vdd-supply property
iio: dac: mcp4922: get and enable vdd regulator
dt-bindings: iio: dac: add mcp4822
iio: dac: mcp4922: add support for mcp48xx series chips
.../bindings/iio/dac/microchip,mcp4822.yaml | 49 +++++++
.../bindings/iio/dac/microchip,mcp4922.yaml | 4 +
drivers/iio/dac/mcp4922.c | 130 ++++++++++++++----
3 files changed, 154 insertions(+), 29 deletions(-)
create mode 100644 Documentation/devicetree/bindings/iio/dac/microchip,mcp4822.yaml
--
2.40.0
These chips have a vdd supply input, which should be modelled in
the bindings as well. Vref is only the voltage reference supply for
the string DAC.
Signed-off-by: Nicolas Frattaroli <[email protected]>
---
.../devicetree/bindings/iio/dac/microchip,mcp4922.yaml | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/Documentation/devicetree/bindings/iio/dac/microchip,mcp4922.yaml b/Documentation/devicetree/bindings/iio/dac/microchip,mcp4922.yaml
index 19374401e509..a3e80e90eeca 100644
--- a/Documentation/devicetree/bindings/iio/dac/microchip,mcp4922.yaml
+++ b/Documentation/devicetree/bindings/iio/dac/microchip,mcp4922.yaml
@@ -21,11 +21,14 @@ properties:
reg:
maxItems: 1
+ vdd-supply: true
+
vref-supply: true
required:
- compatible
- reg
+ - vdd-supply
- vref-supply
allOf:
@@ -42,6 +45,7 @@ examples:
dac@0 {
compatible = "microchip,mcp4912";
reg = <0>;
+ vdd-supply = <&dac_reg>;
vref-supply = <&dac_vref>;
};
};
--
2.40.0
The MCP4801, MCP4802, MCP4811, MCP4812, MCP4821, and MCP4822 are
SPI digital-to-analog converters by Microchip, which have an
internal voltage reference, in contrast to the MCP49xx series
of DACs which use an external voltage reference.
Thus, these need a separate binding, as to not overcomplicate
the mcp4922 binding.
Signed-off-by: Nicolas Frattaroli <[email protected]>
---
.../bindings/iio/dac/microchip,mcp4822.yaml | 49 +++++++++++++++++++
1 file changed, 49 insertions(+)
create mode 100644 Documentation/devicetree/bindings/iio/dac/microchip,mcp4822.yaml
diff --git a/Documentation/devicetree/bindings/iio/dac/microchip,mcp4822.yaml b/Documentation/devicetree/bindings/iio/dac/microchip,mcp4822.yaml
new file mode 100644
index 000000000000..c6eaf0079add
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/dac/microchip,mcp4822.yaml
@@ -0,0 +1,49 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/dac/microchip,mcp4822.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Microchip MCP4801, MCP4802, MCP4811, MCP4812, MCP4821 and MCP4822 SPI DACs
+
+maintainers:
+ - Nicolas Frattaroli <[email protected]>
+
+properties:
+ compatible:
+ enum:
+ - microchip,mcp4801
+ - microchip,mcp4802
+ - microchip,mcp4811
+ - microchip,mcp4812
+ - microchip,mcp4821
+ - microchip,mcp4822
+
+ reg:
+ maxItems: 1
+
+ vdd-supply: true
+
+required:
+ - compatible
+ - reg
+ - vdd-supply
+
+allOf:
+ - $ref: /schemas/spi/spi-peripheral-props.yaml#
+
+unevaluatedProperties: false
+
+examples:
+ - |
+ spi {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ dac@0 {
+ compatible = "microchip,mcp4821";
+ reg = <0>;
+ vdd-supply = <&dac_reg>;
+ };
+ };
+...
--
2.40.0
The MCP4922 family of chips has a vdd power input, which we
model in our device tree binding for it. The driver should get
and enable the vdd regulator as is appropriate.
Signed-off-by: Nicolas Frattaroli <[email protected]>
---
drivers/iio/dac/mcp4922.c | 24 ++++++++++++++++++++----
1 file changed, 20 insertions(+), 4 deletions(-)
diff --git a/drivers/iio/dac/mcp4922.c b/drivers/iio/dac/mcp4922.c
index da4327624d45..0b9458cbbcff 100644
--- a/drivers/iio/dac/mcp4922.c
+++ b/drivers/iio/dac/mcp4922.c
@@ -31,6 +31,7 @@ struct mcp4922_state {
unsigned int value[MCP4922_NUM_CHANNELS];
unsigned int vref_mv;
struct regulator *vref_reg;
+ struct regulator *vdd_reg;
u8 mosi[2] __aligned(IIO_DMA_MINALIGN);
};
@@ -148,10 +149,23 @@ static int mcp4922_probe(struct spi_device *spi)
if (ret < 0) {
dev_err(&spi->dev, "Failed to read vref regulator: %d\n",
ret);
- goto error_disable_reg;
+ goto error_disable_vref_reg;
}
state->vref_mv = ret / 1000;
+ state->vdd_reg = devm_regulator_get(&spi->dev, "vdd");
+ if (IS_ERR(state->vdd_reg)) {
+ ret = dev_err_probe(&spi->dev, PTR_ERR(state->vdd_reg),
+ "vdd regulator not specified\n");
+ goto error_disable_vref_reg;
+ }
+ ret = regulator_enable(state->vdd_reg);
+ if (ret) {
+ dev_err(&spi->dev, "Failed to enable vdd regulator: %d\n",
+ ret);
+ goto error_disable_vref_reg;
+ }
+
spi_set_drvdata(spi, indio_dev);
id = spi_get_device_id(spi);
indio_dev->info = &mcp4922_info;
@@ -167,12 +181,13 @@ static int mcp4922_probe(struct spi_device *spi)
if (ret) {
dev_err(&spi->dev, "Failed to register iio device: %d\n",
ret);
- goto error_disable_reg;
+ goto error_disable_vdd_reg;
}
return 0;
-
-error_disable_reg:
+error_disable_vdd_reg:
+ regulator_disable(state->vdd_reg);
+error_disable_vref_reg:
regulator_disable(state->vref_reg);
return ret;
@@ -185,6 +200,7 @@ static void mcp4922_remove(struct spi_device *spi)
iio_device_unregister(indio_dev);
state = iio_priv(indio_dev);
+ regulator_disable(state->vdd_reg);
regulator_disable(state->vref_reg);
}
--
2.40.0
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 <[email protected]>
---
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,
+ 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 <[email protected]>");
-MODULE_DESCRIPTION("Microchip MCP4902, MCP4912, MCP4922 DAC");
+MODULE_DESCRIPTION("Microchip MCP49XX and MCP48XX DAC");
MODULE_LICENSE("GPL v2");
--
2.40.0
On 05/04/2023 16:01, Nicolas Frattaroli wrote:
> The MCP4801, MCP4802, MCP4811, MCP4812, MCP4821, and MCP4822 are
> SPI digital-to-analog converters by Microchip, which have an
> internal voltage reference, in contrast to the MCP49xx series
> of DACs which use an external voltage reference.
>
> Thus, these need a separate binding, as to not overcomplicate
> the mcp4922 binding.
The difference is just one property which is very easy to handle - one
allOf:if:then: with
vref-supply: false
Are there any other differences?
Best regards,
Krzysztof
On Mittwoch, 5. April 2023 16:10:10 CEST Krzysztof Kozlowski wrote:
> On 05/04/2023 16:01, Nicolas Frattaroli wrote:
> > The MCP4801, MCP4802, MCP4811, MCP4812, MCP4821, and MCP4822 are
> > SPI digital-to-analog converters by Microchip, which have an
> > internal voltage reference, in contrast to the MCP49xx series
> > of DACs which use an external voltage reference.
> >
> > Thus, these need a separate binding, as to not overcomplicate
> > the mcp4922 binding.
>
> The difference is just one property which is very easy to handle - one
> allOf:if:then: with
> vref-supply: false
>
> Are there any other differences?
>
> Best regards,
> Krzysztof
>
>
In place of the external vref input, the MCP48XX series chips also
have a "SHDN" input, which is an active-low pin to disable the whole
chip and put it in a low power state. Future users of the bindings
may want to model this as being tied to some GPIO, though I haven't
done it here since I don't care about this feature.
Kind regards,
Nicolas Frattaroli
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 4/5/23 07:01, Nicolas Frattaroli wrote:
> [...]
> + state->vdd_reg = devm_regulator_get(&spi->dev, "vdd");
> + if (IS_ERR(state->vdd_reg)) {
> + ret = dev_err_probe(&spi->dev, PTR_ERR(state->vdd_reg),
> + "vdd regulator not specified\n");
> + goto error_disable_vref_reg;
> + }
> + ret = regulator_enable(state->vdd_reg);
> + if (ret) {
> + dev_err(&spi->dev, "Failed to enable vdd regulator: %d\n",
> + ret);
> + goto error_disable_vref_reg;
> + }
The two above can be combined into `devm_regulator_get_enable()`. This
will also take care of automatically disabling the regulator on the
error path and on remove.
> +
> spi_set_drvdata(spi, indio_dev);
> id = spi_get_device_id(spi);
> indio_dev->info = &mcp4922_info;
> [...]
On 05/04/2023 16:17, Nicolas Frattaroli wrote:
> On Mittwoch, 5. April 2023 16:10:10 CEST Krzysztof Kozlowski wrote:
>> On 05/04/2023 16:01, Nicolas Frattaroli wrote:
>>> The MCP4801, MCP4802, MCP4811, MCP4812, MCP4821, and MCP4822 are
>>> SPI digital-to-analog converters by Microchip, which have an
>>> internal voltage reference, in contrast to the MCP49xx series
>>> of DACs which use an external voltage reference.
>>>
>>> Thus, these need a separate binding, as to not overcomplicate
>>> the mcp4922 binding.
>>
>> The difference is just one property which is very easy to handle - one
>> allOf:if:then: with
>> vref-supply: false
>>
>> Are there any other differences?
>>
>> Best regards,
>> Krzysztof
>>
>>
>
> In place of the external vref input, the MCP48XX series chips also
> have a "SHDN" input, which is an active-low pin to disable the whole
> chip and put it in a low power state. Future users of the bindings
> may want to model this as being tied to some GPIO, though I haven't
> done it here since I don't care about this feature.
OK to keep them separate, but then you should add here powerdown-gpios.
Bindings should be complete.
Best regards,
Krzysztof
On Wed, 5 Apr 2023 07:18:46 -0700
Lars-Peter Clausen <[email protected]> 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 <[email protected]> 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 <[email protected]>
> ---
> 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 <[email protected]>");
> -MODULE_DESCRIPTION("Microchip MCP4902, MCP4912, MCP4922 DAC");
> +MODULE_DESCRIPTION("Microchip MCP49XX and MCP48XX DAC");
> MODULE_LICENSE("GPL v2");
On Wed, 5 Apr 2023 16:01:12 +0200
Nicolas Frattaroli <[email protected]> wrote:
> The MCP4922 family of chips has a vdd power input, which we
> model in our device tree binding for it. The driver should get
> and enable the vdd regulator as is appropriate.
>
> Signed-off-by: Nicolas Frattaroli <[email protected]>
If, before doing this you add a patch using devm for all the
unwinding currently being done by hand in remove() you can
simplify this further use devm_regulator_get_enable()
(Note you can only do that for this regulator as we never touch it
after enable - more complex handling needed for the vref one as
described in review of patch 4.)
That conversion patch is pretty simple, so whilst I don't like asking
people to implement extra features, in this case the simplifications
to what you are doing here make that precusor work justified
Jonathan
> ---
> drivers/iio/dac/mcp4922.c | 24 ++++++++++++++++++++----
> 1 file changed, 20 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/iio/dac/mcp4922.c b/drivers/iio/dac/mcp4922.c
> index da4327624d45..0b9458cbbcff 100644
> --- a/drivers/iio/dac/mcp4922.c
> +++ b/drivers/iio/dac/mcp4922.c
> @@ -31,6 +31,7 @@ struct mcp4922_state {
> unsigned int value[MCP4922_NUM_CHANNELS];
> unsigned int vref_mv;
> struct regulator *vref_reg;
> + struct regulator *vdd_reg;
> u8 mosi[2] __aligned(IIO_DMA_MINALIGN);
> };
>
> @@ -148,10 +149,23 @@ static int mcp4922_probe(struct spi_device *spi)
> if (ret < 0) {
> dev_err(&spi->dev, "Failed to read vref regulator: %d\n",
> ret);
> - goto error_disable_reg;
> + goto error_disable_vref_reg;
> }
> state->vref_mv = ret / 1000;
>
> + state->vdd_reg = devm_regulator_get(&spi->dev, "vdd");
> + if (IS_ERR(state->vdd_reg)) {
> + ret = dev_err_probe(&spi->dev, PTR_ERR(state->vdd_reg),
> + "vdd regulator not specified\n");
> + goto error_disable_vref_reg;
> + }
> + ret = regulator_enable(state->vdd_reg);
> + if (ret) {
> + dev_err(&spi->dev, "Failed to enable vdd regulator: %d\n",
> + ret);
> + goto error_disable_vref_reg;
> + }
> +
> spi_set_drvdata(spi, indio_dev);
> id = spi_get_device_id(spi);
> indio_dev->info = &mcp4922_info;
> @@ -167,12 +181,13 @@ static int mcp4922_probe(struct spi_device *spi)
> if (ret) {
> dev_err(&spi->dev, "Failed to register iio device: %d\n",
> ret);
> - goto error_disable_reg;
> + goto error_disable_vdd_reg;
> }
>
> return 0;
> -
> -error_disable_reg:
> +error_disable_vdd_reg:
> + regulator_disable(state->vdd_reg);
> +error_disable_vref_reg:
> regulator_disable(state->vref_reg);
>
> return ret;
> @@ -185,6 +200,7 @@ static void mcp4922_remove(struct spi_device *spi)
>
> iio_device_unregister(indio_dev);
> state = iio_priv(indio_dev);
> + regulator_disable(state->vdd_reg);
> regulator_disable(state->vref_reg);
> }
>
On Wed, 5 Apr 2023 07:21:00 -0700
Lars-Peter Clausen <[email protected]> wrote:
> On 4/5/23 07:01, Nicolas Frattaroli wrote:
> > [...]
> > + state->vdd_reg = devm_regulator_get(&spi->dev, "vdd");
> > + if (IS_ERR(state->vdd_reg)) {
> > + ret = dev_err_probe(&spi->dev, PTR_ERR(state->vdd_reg),
> > + "vdd regulator not specified\n");
> > + goto error_disable_vref_reg;
> > + }
> > + ret = regulator_enable(state->vdd_reg);
> > + if (ret) {
> > + dev_err(&spi->dev, "Failed to enable vdd regulator: %d\n",
> > + ret);
> > + goto error_disable_vref_reg;
> > + }
> The two above can be combined into `devm_regulator_get_enable()`. This
> will also take care of automatically disabling the regulator on the
> error path and on remove.
I'm not keen on the ordering of probe wrt to remove that results from mixing
devm and not. Note that already happens because of the gets vs enables
so another reason to take this driver fully devm_ based.
Jonathan
> > +
> > spi_set_drvdata(spi, indio_dev);
> > id = spi_get_device_id(spi);
> > indio_dev->info = &mcp4922_info;
> > [...]
>
>
On Wed, Apr 05, 2023 at 04:01:11PM +0200, Nicolas Frattaroli wrote:
> These chips have a vdd supply input, which should be modelled in
> the bindings as well. Vref is only the voltage reference supply for
> the string DAC.
>
> Signed-off-by: Nicolas Frattaroli <[email protected]>
Acked-by: Michael Welling <[email protected]>
> ---
> .../devicetree/bindings/iio/dac/microchip,mcp4922.yaml | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/iio/dac/microchip,mcp4922.yaml b/Documentation/devicetree/bindings/iio/dac/microchip,mcp4922.yaml
> index 19374401e509..a3e80e90eeca 100644
> --- a/Documentation/devicetree/bindings/iio/dac/microchip,mcp4922.yaml
> +++ b/Documentation/devicetree/bindings/iio/dac/microchip,mcp4922.yaml
> @@ -21,11 +21,14 @@ properties:
> reg:
> maxItems: 1
>
> + vdd-supply: true
> +
> vref-supply: true
>
> required:
> - compatible
> - reg
> + - vdd-supply
> - vref-supply
>
> allOf:
> @@ -42,6 +45,7 @@ examples:
> dac@0 {
> compatible = "microchip,mcp4912";
> reg = <0>;
> + vdd-supply = <&dac_reg>;
> vref-supply = <&dac_vref>;
> };
> };
> --
> 2.40.0
>