* [PATCH v3 0/2] iio: dac: mcp47a1: add support for new device
@ 2026-08-03 17:32 Joshua Crofts
2026-08-03 17:32 ` [PATCH v3 1/2] dt-bindings: iio: dac: add support for mcp47a1 Joshua Crofts
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Joshua Crofts @ 2026-08-03 17:32 UTC (permalink / raw)
To: Jonathan Cameron, David Lechner, Nuno Sá, Andy Shevchenko,
Rob Herring, Krzysztof Kozlowski, Conor Dooley
Cc: linux-iio, devicetree, linux-kernel, Joshua Crofts, Conor Dooley
This patch series adds support for the MCP47A1 DAC.
The Microchip MCP47A1 is a volatile 6-bit Digital-to-Analog converter
which communicates via I2C. It is a low-power, low-cost DAC commonly
used in embedded systems.
Reasons for adding a new driver:
- Existing Microchip DACs use a more complicated set of commands
whereas the MCP47A1 communicates via SMBus using only one command.
- The closest Microchip DAC already in the tree - the MCP4725 - uses
an EEPROM while the MCP47A1 is volatile, meaning that altering
the driver of the former would worsen the code flow.
- The MCP47A1 maps cleanly to a standard regmap.
The driver was tested on a Raspberry Pi 4 using a breakout board with
the MCP47A1.
The datasheet can be viewed here:
https://ww1.microchip.com/downloads/aemDocuments/documents/OTH/ProductDocuments/DataSheets/25154A.pdf
Signed-off-by: Joshua Crofts <joshua.crofts1@gmail.com>
---
Changes in v3:
- remove iio_chan_spec array and define a separate single channel as
only one is used
- change scale denominator from 63 to 64 as former would produce scaling
errors
- Link to v2: https://lore.kernel.org/r/20260727-mcp47a1-add-support-v2-0-d4591a2ce69e@gmail.com
Changes in v2:
- dt-bindings: add slave address enum to dt-binding
- dt-bindings: remove unnecessary example
- move driver from regmap to i2c_smbus_* functions to reduce overhead
- remove read test on probe as it's too unstable
- add 20us delay on startup
- style changes
- remove excess mailing list entry from MAINTAINERS
- Link to v1: https://lore.kernel.org/r/20260721-mcp47a1-add-support-v1-0-da045a2567e3@gmail.com
---
Joshua Crofts (2):
dt-bindings: iio: dac: add support for mcp47a1
iio: dac: mcp47a1: add support for new device
.../bindings/iio/dac/microchip,mcp47a1.yaml | 50 +++++++
MAINTAINERS | 6 +
drivers/iio/dac/Kconfig | 10 ++
drivers/iio/dac/Makefile | 1 +
drivers/iio/dac/mcp47a1.c | 166 +++++++++++++++++++++
5 files changed, 233 insertions(+)
---
base-commit: e0484d62e8e1cff75b210938be835ea6221bda59
change-id: 20260711-mcp47a1-add-support-3af1248f13a5
Best regards,
--
Kind regards,
Joshua Crofts
^ permalink raw reply [flat|nested] 8+ messages in thread* [PATCH v3 1/2] dt-bindings: iio: dac: add support for mcp47a1 2026-08-03 17:32 [PATCH v3 0/2] iio: dac: mcp47a1: add support for new device Joshua Crofts @ 2026-08-03 17:32 ` Joshua Crofts 2026-08-03 18:07 ` sashiko-bot 2026-08-03 17:32 ` [PATCH v3 2/2] iio: dac: mcp47a1: add support for new device Joshua Crofts 2026-08-05 0:14 ` [PATCH v3 0/2] " Jonathan Cameron 2 siblings, 1 reply; 8+ messages in thread From: Joshua Crofts @ 2026-08-03 17:32 UTC (permalink / raw) To: Jonathan Cameron, David Lechner, Nuno Sá, Andy Shevchenko, Rob Herring, Krzysztof Kozlowski, Conor Dooley Cc: linux-iio, devicetree, linux-kernel, Joshua Crofts, Conor Dooley The Microchip MCP47A1 is a 6-bit volatile Digital-to-Analog converter which communicates via I2C. Reviewed-by: Conor Dooley <conor.dooley@microchip.com> Signed-off-by: Joshua Crofts <joshua.crofts1@gmail.com> --- .../bindings/iio/dac/microchip,mcp47a1.yaml | 50 ++++++++++++++++++++++ MAINTAINERS | 5 +++ 2 files changed, 55 insertions(+) diff --git a/Documentation/devicetree/bindings/iio/dac/microchip,mcp47a1.yaml b/Documentation/devicetree/bindings/iio/dac/microchip,mcp47a1.yaml new file mode 100644 index 000000000000..b181fe73dce8 --- /dev/null +++ b/Documentation/devicetree/bindings/iio/dac/microchip,mcp47a1.yaml @@ -0,0 +1,50 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- + +$id: http://devicetree.org/schemas/iio/dac/microchip,mcp47a1.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Microchip MCP47A1 DAC + +maintainers: + - Joshua Crofts <joshua.crofts1@gmail.com> + +description: | + The Microchip MCP47A1 is a 6-bit single output volatile DAC. + This device can have different IDs (0x2e and 0x3e). + https://ww1.microchip.com/downloads/aemDocuments/documents/OTH/ProductDocuments/DataSheets/25154A.pdf + +properties: + compatible: + const: microchip,mcp47a1 + + reg: + items: + - enum: [0x2e, 0x3e] + + vref-supply: true + + vdd-supply: true + +required: + - compatible + - reg + - vref-supply + - vdd-supply + +additionalProperties: false + +examples: + - | + i2c { + #address-cells = <1>; + #size-cells = <0>; + + dac@2e { + compatible = "microchip,mcp47a1"; + reg = <0x2e>; + vref-supply = <&vref_regulator>; + vdd-supply = <&vdd_regulator>; + }; + }; diff --git a/MAINTAINERS b/MAINTAINERS index ce020ded5fd3..8c8118eb4141 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -17699,6 +17699,11 @@ S: Maintained F: Documentation/devicetree/bindings/iio/adc/microchip,mcp3911.yaml F: drivers/iio/adc/mcp3911.c +MICROCHIP MCP47A1 DAC DRIVER +M: Joshua Crofts <joshua.crofts1@gmail.com> +S: Maintained +F: Documentation/devicetree/bindings/iio/dac/microchip,mcp47a1.yaml + MICROCHIP MCP9982 TEMPERATURE DRIVER M: Victor Duicu <victor.duicu@microchip.com> L: linux-hwmon@vger.kernel.org -- 2.55.0 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v3 1/2] dt-bindings: iio: dac: add support for mcp47a1 2026-08-03 17:32 ` [PATCH v3 1/2] dt-bindings: iio: dac: add support for mcp47a1 Joshua Crofts @ 2026-08-03 18:07 ` sashiko-bot 2026-08-03 19:35 ` Joshua Crofts 0 siblings, 1 reply; 8+ messages in thread From: sashiko-bot @ 2026-08-03 18:07 UTC (permalink / raw) To: Joshua Crofts; +Cc: devicetree, robh, conor+dt Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Medium] The I2C addresses specified in the schema and example appear to be 8-bit write addresses instead of the 7-bit addresses required by the DT specification. - [Low] The `reg` property schema applies an integer `enum` directly to a matrix array element, which will cause `dt_binding_check` validation to always fail. -- --- Patch [1]: [PATCH v3 1/2] dt-bindings: iio: dac: add support for mcp47a1 --- commit 441e840b5a2a3f037c3520c49013a55b7dc4c1bd Author: Joshua Crofts <joshua.crofts1@gmail.com> dt-bindings: iio: dac: add support for mcp47a1 The Microchip MCP47A1 is a 6-bit volatile Digital-to-Analog converter which communicates via I2C. > diff --git a/Documentation/devicetree/bindings/iio/dac/microchip,mcp47a1.yaml b/Documentation/devicetree/bindings/iio/dac/microchip,mcp47a1.yaml > new file mode 100644 > index 0000000000000..b181fe73dce84 > --- /dev/null > +++ b/Documentation/devicetree/bindings/iio/dac/microchip,mcp47a1.yaml [ ... ] > +properties: > + compatible: > + const: microchip,mcp47a1 > + > + reg: > + items: > + - enum: [0x2e, 0x3e] [Severity: Low] Will this cause dt_binding_check validation to fail? Since dt-schema parses reg values into a uint32-matrix (an array of arrays), the items array applied to it defines a tuple schema for the outer array's elements. This means the inner array [0x2e] will be matched against the integer enum [0x2e, 0x3e], which will mathematically fail since an array is not an integer. [Severity: Medium] Are these 8-bit write addresses instead of the 7-bit addresses required by the Device Tree specification? If these are indeed 8-bit addresses (as indicated by the 0 LSB and typical Microchip datasheet conventions), they must be shifted right by 1. That would make the correct 7-bit DT addresses 0x17 and 0x1f. > + > + vref-supply: true [ ... ] > +examples: > + - | > + i2c { > + #address-cells = <1>; > + #size-cells = <0>; > + > + dac@2e { > + compatible = "microchip,mcp47a1"; > + reg = <0x2e>; [Severity: Medium] Does this example propagate the DT specification violation? Using an 8-bit write address directly in the reg property and unit name (dac@2e) will cause the device to fail to probe or communicate, as the I2C subsystem expects a 7-bit address. > + vref-supply = <&vref_regulator>; > + vdd-supply = <&vdd_regulator>; > + }; > + }; -- Sashiko AI review · https://sashiko.dev/#/patchset/20260803-mcp47a1-add-support-v3-0-540e71b4fd2e@gmail.com?part=1 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3 1/2] dt-bindings: iio: dac: add support for mcp47a1 2026-08-03 18:07 ` sashiko-bot @ 2026-08-03 19:35 ` Joshua Crofts 0 siblings, 0 replies; 8+ messages in thread From: Joshua Crofts @ 2026-08-03 19:35 UTC (permalink / raw) To: sashiko-bot Cc: sashiko-reviews, devicetree, robh, conor+dt, linux-iio, jic23, dlechner, nuno.sa, andy On Mon, 03 Aug 2026 18:07:16 +0000 sashiko-bot@kernel.org wrote: > Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: > - [Medium] The I2C addresses specified in the schema and example appear to be 8-bit write addresses instead of the 7-bit addresses required by the DT specification. > - [Low] The `reg` property schema applies an integer `enum` directly to a matrix array element, which will cause `dt_binding_check` validation to always fail. > -- > > --- Patch [1]: [PATCH v3 1/2] dt-bindings: iio: dac: add support for mcp47a1 --- > commit 441e840b5a2a3f037c3520c49013a55b7dc4c1bd > Author: Joshua Crofts <joshua.crofts1@gmail.com> > > dt-bindings: iio: dac: add support for mcp47a1 > > The Microchip MCP47A1 is a 6-bit volatile Digital-to-Analog converter > which communicates via I2C. > > > diff --git a/Documentation/devicetree/bindings/iio/dac/microchip,mcp47a1.yaml b/Documentation/devicetree/bindings/iio/dac/microchip,mcp47a1.yaml > > new file mode 100644 > > index 0000000000000..b181fe73dce84 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/iio/dac/microchip,mcp47a1.yaml > [ ... ] > > +properties: > > + compatible: > > + const: microchip,mcp47a1 > > + > > + reg: > > + items: > > + - enum: [0x2e, 0x3e] > > [Severity: Low] > Will this cause dt_binding_check validation to fail? No, this is a perfectly valid dt-schema which passes dt_binding_check. > > Since dt-schema parses reg values into a uint32-matrix (an array of arrays), > the items array applied to it defines a tuple schema for the outer array's > elements. This means the inner array [0x2e] will be matched against the > integer enum [0x2e, 0x3e], which will mathematically fail since an array > is not an integer. > > [Severity: Medium] > Are these 8-bit write addresses instead of the 7-bit addresses required by > the Device Tree specification? > > If these are indeed 8-bit addresses (as indicated by the 0 LSB and typical > Microchip datasheet conventions), they must be shifted right by 1. That would > make the correct 7-bit DT addresses 0x17 and 0x1f. No clue what Sashiko is on about since these are indeed 7-bit addresses which do work when tested on the actual hardware. -- Kind regards, Joshua Crofts ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v3 2/2] iio: dac: mcp47a1: add support for new device 2026-08-03 17:32 [PATCH v3 0/2] iio: dac: mcp47a1: add support for new device Joshua Crofts 2026-08-03 17:32 ` [PATCH v3 1/2] dt-bindings: iio: dac: add support for mcp47a1 Joshua Crofts @ 2026-08-03 17:32 ` Joshua Crofts 2026-08-03 17:44 ` sashiko-bot 2026-08-05 0:14 ` [PATCH v3 0/2] " Jonathan Cameron 2 siblings, 1 reply; 8+ messages in thread From: Joshua Crofts @ 2026-08-03 17:32 UTC (permalink / raw) To: Jonathan Cameron, David Lechner, Nuno Sá, Andy Shevchenko, Rob Herring, Krzysztof Kozlowski, Conor Dooley Cc: linux-iio, devicetree, linux-kernel, Joshua Crofts The Microchip MCP47A1 is a 6-bit volatile Digital-to-Analog converter which communicates via I2C. Signed-off-by: Joshua Crofts <joshua.crofts1@gmail.com> --- MAINTAINERS | 1 + drivers/iio/dac/Kconfig | 10 +++ drivers/iio/dac/Makefile | 1 + drivers/iio/dac/mcp47a1.c | 166 ++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 178 insertions(+) diff --git a/MAINTAINERS b/MAINTAINERS index 8c8118eb4141..fc76a2159e1f 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -17703,6 +17703,7 @@ MICROCHIP MCP47A1 DAC DRIVER M: Joshua Crofts <joshua.crofts1@gmail.com> S: Maintained F: Documentation/devicetree/bindings/iio/dac/microchip,mcp47a1.yaml +F: drivers/iio/dac/mcp47a1.c MICROCHIP MCP9982 TEMPERATURE DRIVER M: Victor Duicu <victor.duicu@microchip.com> diff --git a/drivers/iio/dac/Kconfig b/drivers/iio/dac/Kconfig index 18a27785e28b..04c90d98f86f 100644 --- a/drivers/iio/dac/Kconfig +++ b/drivers/iio/dac/Kconfig @@ -582,6 +582,16 @@ config MCP4728 To compile this driver as a module, choose M here: the module will be called mcp4728. +config MCP47A1 + tristate "MCP47A1 DAC driver" + depends on I2C + help + Say Y here if you want to build a driver for the Microchip + MCP47A1 digital-to-analog converter with an I2C interface. + + To compile this driver as a module, choose M here: the module + will be called mcp47a1. + config MCP47FEB02 tristate "MCP47F(E/V)B01/02/04/08/11/12/14/18/21/22/24/28 DAC driver" depends on I2C diff --git a/drivers/iio/dac/Makefile b/drivers/iio/dac/Makefile index 5d20d37e44ce..992f8930f95c 100644 --- a/drivers/iio/dac/Makefile +++ b/drivers/iio/dac/Makefile @@ -55,6 +55,7 @@ obj-$(CONFIG_MAX5821) += max5821.o obj-$(CONFIG_MCF54415_DAC) += mcf54415_dac.o obj-$(CONFIG_MCP4725) += mcp4725.o obj-$(CONFIG_MCP4728) += mcp4728.o +obj-$(CONFIG_MCP47A1) += mcp47a1.o obj-$(CONFIG_MCP47FEB02) += mcp47feb02.o obj-$(CONFIG_MCP4821) += mcp4821.o obj-$(CONFIG_MCP4922) += mcp4922.o diff --git a/drivers/iio/dac/mcp47a1.c b/drivers/iio/dac/mcp47a1.c new file mode 100644 index 000000000000..0bf994aa0e4f --- /dev/null +++ b/drivers/iio/dac/mcp47a1.c @@ -0,0 +1,166 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Microchip MCP47A1 DAC driver + * + * Copyright (c) 2026 Joshua Crofts <joshua.crofts1@gmail.com> + * + * Datasheet: https://ww1.microchip.com/downloads/aemDocuments/documents/OTH/ProductDocuments/DataSheets/25154A.pdf + */ + +#include <linux/delay.h> +#include <linux/err.h> +#include <linux/i2c.h> +#include <linux/minmax.h> +#include <linux/module.h> +#include <linux/regulator/consumer.h> +#include <linux/types.h> +#include <linux/units.h> + +#include <linux/iio/iio.h> + +#define MCP47A1_CMD_CODE 0x00 +#define MCP47A1_MAX_STEPS 64 + +struct mcp47a1_data { + struct i2c_client *client; + int vref_mV; +}; + +static const int mcp47a1_raw_avail[] = { 0, 1, MCP47A1_MAX_STEPS - 1 }; + +static const struct iio_chan_spec mcp47a1_channel = { + .type = IIO_VOLTAGE, + .indexed = 1, + .output = 1, + .channel = 0, + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), + .info_mask_separate_available = BIT(IIO_CHAN_INFO_RAW), + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), +}; + +static int mcp47a1_write(struct iio_dev *indio_dev, + struct iio_chan_spec const *chan, + int val, int val2, long mask) +{ + struct mcp47a1_data *data = iio_priv(indio_dev); + + switch (mask) { + case IIO_CHAN_INFO_RAW: + if (!in_range(val, 0, MCP47A1_MAX_STEPS)) + return -EINVAL; + + return i2c_smbus_write_byte_data(data->client, MCP47A1_CMD_CODE, + val); + default: + return -EINVAL; + } +} + +static int mcp47a1_read(struct iio_dev *indio_dev, + struct iio_chan_spec const *chan, + int *val, int *val2, long mask) +{ + struct mcp47a1_data *data = iio_priv(indio_dev); + int ret; + + switch (mask) { + case IIO_CHAN_INFO_RAW: + ret = i2c_smbus_read_byte_data(data->client, MCP47A1_CMD_CODE); + if (ret < 0) + return ret; + + *val = ret; + + return IIO_VAL_INT; + case IIO_CHAN_INFO_SCALE: + *val = data->vref_mV; + *val2 = MCP47A1_MAX_STEPS; + + return IIO_VAL_FRACTIONAL; + default: + return -EINVAL; + } +} + +static int mcp47a1_read_avail(struct iio_dev *indio_dev, + struct iio_chan_spec const *chan, + const int **vals, int *type, int *length, + long mask) +{ + switch (mask) { + case IIO_CHAN_INFO_RAW: + *vals = mcp47a1_raw_avail; + *type = IIO_VAL_INT; + return IIO_AVAIL_RANGE; + default: + return -EINVAL; + } +} + +static const struct iio_info mcp47a1_info = { + .write_raw = mcp47a1_write, + .read_raw = mcp47a1_read, + .read_avail = mcp47a1_read_avail, +}; + +static int mcp47a1_probe(struct i2c_client *client) +{ + struct device *dev = &client->dev; + struct mcp47a1_data *data; + struct iio_dev *indio_dev; + int ret; + + indio_dev = devm_iio_device_alloc(dev, sizeof(*data)); + if (!indio_dev) + return -ENOMEM; + + data = iio_priv(indio_dev); + data->client = client; + + ret = devm_regulator_get_enable(dev, "vdd"); + if (ret) + return dev_err_probe(dev, ret, "Failed to enable regulator\n"); + + /* Delay after device exits reset state (see AC/DC characteristics) */ + fsleep(20); + + ret = devm_regulator_get_enable_read_voltage(dev, "vref"); + if (ret < 0) + return dev_err_probe(dev, ret, "Failed to read vref\n"); + + data->vref_mV = ret / (MICRO / MILLI); + + indio_dev->name = "mcp47a1"; + indio_dev->modes = INDIO_DIRECT_MODE; + indio_dev->info = &mcp47a1_info; + indio_dev->channels = &mcp47a1_channel; + indio_dev->num_channels = 1; + + return devm_iio_device_register(dev, indio_dev); +} + +static const struct of_device_id mcp47a1_of_match[] = { + { .compatible = "microchip,mcp47a1" }, + { } +}; +MODULE_DEVICE_TABLE(of, mcp47a1_of_match); + +static const struct i2c_device_id mcp47a1_id[] = { + { .name = "mcp47a1" }, + { } +}; +MODULE_DEVICE_TABLE(i2c, mcp47a1_id); + +static struct i2c_driver mcp47a1_driver = { + .driver = { + .name = "mcp47a1", + .of_match_table = mcp47a1_of_match, + }, + .probe = mcp47a1_probe, + .id_table = mcp47a1_id, +}; +module_i2c_driver(mcp47a1_driver); + +MODULE_AUTHOR("Joshua Crofts <joshua.crofts1@gmail.com>"); +MODULE_DESCRIPTION("Microchip MCP47A1 DAC Driver"); +MODULE_LICENSE("GPL"); -- 2.55.0 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v3 2/2] iio: dac: mcp47a1: add support for new device 2026-08-03 17:32 ` [PATCH v3 2/2] iio: dac: mcp47a1: add support for new device Joshua Crofts @ 2026-08-03 17:44 ` sashiko-bot 2026-08-03 19:42 ` Joshua Crofts 0 siblings, 1 reply; 8+ messages in thread From: sashiko-bot @ 2026-08-03 17:44 UTC (permalink / raw) To: Joshua Crofts; +Cc: robh, conor+dt, devicetree Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] Incorrect SMBus commands send extra bytes and corrupt the DAC hardware state. -- --- Patch [2]: [PATCH v3 2/2] iio: dac: mcp47a1: add support for new device --- commit f73263d5ea24798a693c0c4cfa738627794988ba Author: Joshua Crofts <joshua.crofts1@gmail.com> iio: dac: mcp47a1: add support for new device This commit introduces a new driver for the Microchip MCP47A1 6-bit volatile DAC. It supports writing and reading the raw values and exposing the scale using I2C. > diff --git a/drivers/iio/dac/mcp47a1.c b/drivers/iio/dac/mcp47a1.c > new file mode 100644 > index 0000000000000..0bf994aa0e4f5 > --- /dev/null > +++ b/drivers/iio/dac/mcp47a1.c [ ... ] > +static int mcp47a1_write(struct iio_dev *indio_dev, > + struct iio_chan_spec const *chan, > + int val, int val2, long mask) > +{ > + struct mcp47a1_data *data = iio_priv(indio_dev); > + > + switch (mask) { > + case IIO_CHAN_INFO_RAW: > + if (!in_range(val, 0, MCP47A1_MAX_STEPS)) > + return -EINVAL; > + > + return i2c_smbus_write_byte_data(data->client, MCP47A1_CMD_CODE, > + val); [Severity: Medium] Could this corrupt the DAC hardware state by sending an extra byte? If the MCP47A1 expects a single byte without a command code, using i2c_smbus_write_byte_data() will send a 2-byte payload, writing 0x00 to the DAC before the actual value. This could potentially cause an output glitch. > + default: > + return -EINVAL; > + } > +} > + > +static int mcp47a1_read(struct iio_dev *indio_dev, > + struct iio_chan_spec const *chan, > + int *val, int *val2, long mask) > +{ > + struct mcp47a1_data *data = iio_priv(indio_dev); > + int ret; > + > + switch (mask) { > + case IIO_CHAN_INFO_RAW: > + ret = i2c_smbus_read_byte_data(data->client, MCP47A1_CMD_CODE); [Severity: Medium] Does this destructively reset the DAC on every read? If the device does not expect a command code and interprets it as a data byte, calling i2c_smbus_read_byte_data() will perform a write of 0x00 before reading, destructively resetting the DAC output to 0. > + if (ret < 0) > + return ret; -- Sashiko AI review · https://sashiko.dev/#/patchset/20260803-mcp47a1-add-support-v3-0-540e71b4fd2e@gmail.com?part=2 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3 2/2] iio: dac: mcp47a1: add support for new device 2026-08-03 17:44 ` sashiko-bot @ 2026-08-03 19:42 ` Joshua Crofts 0 siblings, 0 replies; 8+ messages in thread From: Joshua Crofts @ 2026-08-03 19:42 UTC (permalink / raw) To: sashiko-bot Cc: sashiko-reviews, robh, conor+dt, devicetree, jic23, dlechner, andy, nuno.sa, linux-iio On Mon, 03 Aug 2026 17:44:40 +0000 sashiko-bot@kernel.org wrote: > Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: > - [Medium] Incorrect SMBus commands send extra bytes and corrupt the DAC hardware state. > -- > > --- Patch [2]: [PATCH v3 2/2] iio: dac: mcp47a1: add support for new device --- > commit f73263d5ea24798a693c0c4cfa738627794988ba > Author: Joshua Crofts <joshua.crofts1@gmail.com> > > iio: dac: mcp47a1: add support for new device > > This commit introduces a new driver for the Microchip MCP47A1 6-bit volatile > DAC. It supports writing and reading the raw values and exposing the scale > using I2C. > > > diff --git a/drivers/iio/dac/mcp47a1.c b/drivers/iio/dac/mcp47a1.c > > new file mode 100644 > > index 0000000000000..0bf994aa0e4f5 > > --- /dev/null > > +++ b/drivers/iio/dac/mcp47a1.c > > [ ... ] > > > +static int mcp47a1_write(struct iio_dev *indio_dev, > > + struct iio_chan_spec const *chan, > > + int val, int val2, long mask) > > +{ > > + struct mcp47a1_data *data = iio_priv(indio_dev); > > + > > + switch (mask) { > > + case IIO_CHAN_INFO_RAW: > > + if (!in_range(val, 0, MCP47A1_MAX_STEPS)) > > + return -EINVAL; > > + > > + return i2c_smbus_write_byte_data(data->client, MCP47A1_CMD_CODE, > > + val); > > [Severity: Medium] > Could this corrupt the DAC hardware state by sending an extra byte? > > If the MCP47A1 expects a single byte without a command code, using > i2c_smbus_write_byte_data() will send a 2-byte payload, writing 0x00 to the > DAC before the actual value. This could potentially cause an output glitch. > > > + default: > > + return -EINVAL; > > + } > > +} > > + > > +static int mcp47a1_read(struct iio_dev *indio_dev, > > + struct iio_chan_spec const *chan, > > + int *val, int *val2, long mask) > > +{ > > + struct mcp47a1_data *data = iio_priv(indio_dev); > > + int ret; > > + > > + switch (mask) { > > + case IIO_CHAN_INFO_RAW: > > + ret = i2c_smbus_read_byte_data(data->client, MCP47A1_CMD_CODE); > > [Severity: Medium] > Does this destructively reset the DAC on every read? > > If the device does not expect a command code and interprets it as a data byte, > calling i2c_smbus_read_byte_data() will perform a write of 0x00 before reading, > destructively resetting the DAC output to 0. No, this is a false positive. The MCP47A1 is SMBus compatible, it will interpret the command as an instruction, not a data byte. -- Kind regards, Joshua Crofts ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3 0/2] iio: dac: mcp47a1: add support for new device 2026-08-03 17:32 [PATCH v3 0/2] iio: dac: mcp47a1: add support for new device Joshua Crofts 2026-08-03 17:32 ` [PATCH v3 1/2] dt-bindings: iio: dac: add support for mcp47a1 Joshua Crofts 2026-08-03 17:32 ` [PATCH v3 2/2] iio: dac: mcp47a1: add support for new device Joshua Crofts @ 2026-08-05 0:14 ` Jonathan Cameron 2 siblings, 0 replies; 8+ messages in thread From: Jonathan Cameron @ 2026-08-05 0:14 UTC (permalink / raw) To: Joshua Crofts Cc: David Lechner, Nuno Sá, Andy Shevchenko, Rob Herring, Krzysztof Kozlowski, Conor Dooley, linux-iio, devicetree, linux-kernel, Conor Dooley On Mon, 03 Aug 2026 19:32:24 +0200 Joshua Crofts <joshua.crofts1@gmail.com> wrote: > This patch series adds support for the MCP47A1 DAC. > > The Microchip MCP47A1 is a volatile 6-bit Digital-to-Analog converter > which communicates via I2C. It is a low-power, low-cost DAC commonly > used in embedded systems. > > Reasons for adding a new driver: > - Existing Microchip DACs use a more complicated set of commands > whereas the MCP47A1 communicates via SMBus using only one command. > - The closest Microchip DAC already in the tree - the MCP4725 - uses > an EEPROM while the MCP47A1 is volatile, meaning that altering > the driver of the former would worsen the code flow. > - The MCP47A1 maps cleanly to a standard regmap. > > The driver was tested on a Raspberry Pi 4 using a breakout board with > the MCP47A1. > > The datasheet can be viewed here: > https://ww1.microchip.com/downloads/aemDocuments/documents/OTH/ProductDocuments/DataSheets/25154A.pdf > > Signed-off-by: Joshua Crofts <joshua.crofts1@gmail.com> Applied to the testing branch of iio.git Thanks, Jonathan ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2026-08-05 0:14 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2026-08-03 17:32 [PATCH v3 0/2] iio: dac: mcp47a1: add support for new device Joshua Crofts 2026-08-03 17:32 ` [PATCH v3 1/2] dt-bindings: iio: dac: add support for mcp47a1 Joshua Crofts 2026-08-03 18:07 ` sashiko-bot 2026-08-03 19:35 ` Joshua Crofts 2026-08-03 17:32 ` [PATCH v3 2/2] iio: dac: mcp47a1: add support for new device Joshua Crofts 2026-08-03 17:44 ` sashiko-bot 2026-08-03 19:42 ` Joshua Crofts 2026-08-05 0:14 ` [PATCH v3 0/2] " Jonathan Cameron
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox