* [PATCH v4 0/2] add MCP4728 I2C DAC driver
@ 2023-08-03 12:56 Andrea Collamati
2023-08-03 12:56 ` [PATCH v4 1/2] dt-bindings: iio: dac: add mcp4728.yaml Andrea Collamati
2023-08-03 12:56 ` [PATCH v4 2/2] iio: add MCP4728 I2C DAC driver Andrea Collamati
0 siblings, 2 replies; 8+ messages in thread
From: Andrea Collamati @ 2023-08-03 12:56 UTC (permalink / raw)
To: Jonathan Cameron, Lars-Peter Clausen, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Andrea Collamati
Cc: linux-iio, devicetree, linux-kernel
Changes v3->v4:
- sorted includes
- driver name inlined
- used FIELD_PREP() / FIELD_GET() to define/access regs
- fix comment style
- removed infrastructur form multiple support devices
- wrapped up custom ABI in the standard ABI of _scale
- used dev_get_drvdata(dev) to cleanup code
- used devm_add_action_or_reset to cleanup code
Changes v2->v3:
- fix wrong i2c_device_id array indentation
- removed double blank line in Kconfig
- added description in dt-bindings
- use uppercase letters for device name
Changes v1->v2:
- fix mcp4728_remove prototype
- improve indentation
- various fixes suggested by checkscript.pl
- removed unused of_device_id.data field
- removed unuseful mcp4728_data.id field
- various fixes suggested by dt_binding_check
Andrea Collamati (2):
dt-bindings: iio: dac: add mcp4728.yaml
iio: add MCP4728 I2C DAC driver
.../bindings/iio/dac/microchip,mcp4728.yaml | 49 ++
drivers/iio/dac/Kconfig | 11 +
drivers/iio/dac/Makefile | 1 +
drivers/iio/dac/mcp4728.c | 626 ++++++++++++++++++
4 files changed, 687 insertions(+)
create mode 100644 Documentation/devicetree/bindings/iio/dac/microchip,mcp4728.yaml
create mode 100644 drivers/iio/dac/mcp4728.c
--
2.34.1
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v4 1/2] dt-bindings: iio: dac: add mcp4728.yaml
2023-08-03 12:56 [PATCH v4 0/2] add MCP4728 I2C DAC driver Andrea Collamati
@ 2023-08-03 12:56 ` Andrea Collamati
2023-08-03 15:21 ` Conor Dooley
2023-08-03 12:56 ` [PATCH v4 2/2] iio: add MCP4728 I2C DAC driver Andrea Collamati
1 sibling, 1 reply; 8+ messages in thread
From: Andrea Collamati @ 2023-08-03 12:56 UTC (permalink / raw)
To: Jonathan Cameron, Lars-Peter Clausen, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Andrea Collamati
Cc: linux-iio, devicetree, linux-kernel
Add documentation for MCP4728
Signed-off-by: Andrea Collamati <andrea.collamati@gmail.com>
---
.../bindings/iio/dac/microchip,mcp4728.yaml | 49 +++++++++++++++++++
1 file changed, 49 insertions(+)
create mode 100644 Documentation/devicetree/bindings/iio/dac/microchip,mcp4728.yaml
diff --git a/Documentation/devicetree/bindings/iio/dac/microchip,mcp4728.yaml b/Documentation/devicetree/bindings/iio/dac/microchip,mcp4728.yaml
new file mode 100644
index 000000000000..99831d7f1c16
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/dac/microchip,mcp4728.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,mcp4728.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Microchip MCP4728 DAC
+
+maintainers:
+ - Andrea Collamati <andrea.collamati@gmail.com>
+
+description: |
+ MCP4728 is a quad channel, 12-bit voltage output
+ Digital-to-Analog Converter with non-volatile
+ memory and I2C compatible Serial Interface.
+ https://www.microchip.com/en-us/product/mcp4728
+
+properties:
+ compatible:
+ const: microchip,mcp4728
+
+ reg:
+ maxItems: 1
+
+ vdd-supply:
+ description: |
+ Provides both power and acts as the reference supply on the MCP4728
+ when Internal Vref is not selected.
+
+required:
+ - compatible
+ - reg
+ - vdd-supply
+
+additionalProperties: false
+
+examples:
+ - |
+ i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ dac@60 {
+ compatible = "microchip,mcp4728";
+ reg = <0x60>;
+ vdd-supply = <&vdac_vdd>;
+ };
+ };
--
2.34.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH v4 2/2] iio: add MCP4728 I2C DAC driver
2023-08-03 12:56 [PATCH v4 0/2] add MCP4728 I2C DAC driver Andrea Collamati
2023-08-03 12:56 ` [PATCH v4 1/2] dt-bindings: iio: dac: add mcp4728.yaml Andrea Collamati
@ 2023-08-03 12:56 ` Andrea Collamati
2023-08-05 18:28 ` Jonathan Cameron
1 sibling, 1 reply; 8+ messages in thread
From: Andrea Collamati @ 2023-08-03 12:56 UTC (permalink / raw)
To: Jonathan Cameron, Lars-Peter Clausen, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Andrea Collamati
Cc: linux-iio, devicetree, linux-kernel
MCP4728 is a 12-bit quad channel DAC with I2C interface.
support for:
* per-channel gain
* per-channel power state
* per-channel power down mode control
* per-channel vref selection internal/vdd
* store current state to on-chip EEPROM
Signed-off-by: Andrea Collamati <andrea.collamati@gmail.com>
---
drivers/iio/dac/Kconfig | 11 +
drivers/iio/dac/Makefile | 1 +
drivers/iio/dac/mcp4728.c | 626 ++++++++++++++++++++++++++++++++++++++
3 files changed, 638 insertions(+)
create mode 100644 drivers/iio/dac/mcp4728.c
diff --git a/drivers/iio/dac/Kconfig b/drivers/iio/dac/Kconfig
index 3acd9c3f388e..93b8be183de6 100644
--- a/drivers/iio/dac/Kconfig
+++ b/drivers/iio/dac/Kconfig
@@ -389,6 +389,17 @@ config MCP4725
To compile this driver as a module, choose M here: the module
will be called mcp4725.
+config MCP4728
+ tristate "MCP4728 DAC driver"
+ depends on I2C
+ help
+ Say Y here if you want to build a driver for the Microchip
+ MCP4728 quad channel, 12-bit digital-to-analog converter (DAC)
+ with I2C interface.
+
+ To compile this driver as a module, choose M here: the module
+ will be called mcp4728.
+
config MCP4922
tristate "MCP4902, MCP4912, MCP4922 DAC driver"
depends on SPI
diff --git a/drivers/iio/dac/Makefile b/drivers/iio/dac/Makefile
index addd97a78838..5b2bac900d5a 100644
--- a/drivers/iio/dac/Makefile
+++ b/drivers/iio/dac/Makefile
@@ -41,6 +41,7 @@ obj-$(CONFIG_MAX517) += max517.o
obj-$(CONFIG_MAX5522) += max5522.o
obj-$(CONFIG_MAX5821) += max5821.o
obj-$(CONFIG_MCP4725) += mcp4725.o
+obj-$(CONFIG_MCP4728) += mcp4728.o
obj-$(CONFIG_MCP4922) += mcp4922.o
obj-$(CONFIG_STM32_DAC_CORE) += stm32-dac-core.o
obj-$(CONFIG_STM32_DAC) += stm32-dac.o
diff --git a/drivers/iio/dac/mcp4728.c b/drivers/iio/dac/mcp4728.c
new file mode 100644
index 000000000000..ba3eab349b0a
--- /dev/null
+++ b/drivers/iio/dac/mcp4728.c
@@ -0,0 +1,626 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Support for Microchip MCP4728
+ *
+ * Copyright (C) 2023 Andrea Collamati <andrea.collamati@gmail.com>
+ *
+ * Based on mcp4725 by Peter Meerwald <pmeerw@pmeerw.net>
+ *
+ * Driver for the Microchip I2C 12-bit digital-to-analog quad channels
+ * converter (DAC).
+ *
+ * (7-bit I2C slave address 0x60, the three LSBs can be configured in
+ * hardware)
+ */
+
+#include <linux/bitfield.h>
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/i2c.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+#include <linux/module.h>
+#include <linux/mod_devicetable.h>
+#include <linux/property.h>
+#include <linux/regulator/consumer.h>
+
+#define MCP4728_RESOLUTION 12
+#define MCP4728_N_CHANNELS 4
+
+#define MCP4728_CMD_MASK GENMASK(7, 3)
+#define MCP4728_CHSEL_MASK GENMASK(2, 1)
+#define MCP4728_UDAC_MASK BIT(0)
+
+#define MCP4728_VREF_MASK BIT(7)
+#define MCP4728_PDMODE_MASK GENMASK(6, 5)
+#define MCP4728_GAIN_MASK BIT(4)
+
+#define MCP4728_DAC_H_MASK GENMASK(3, 0)
+#define MCP4728_DAC_L_MASK GENMASK(7, 0)
+
+#define MCP4728_RDY_MASK BIT(7)
+
+#define MCP4728_MW_CMD 0x08 /* Multiwrite Command */
+#define MCP4728_SW_CMD 0x0A /* Sequential Write Command with EEPROM */
+
+#define MCP4728_READ_RESPONSE_LEN (MCP4728_N_CHANNELS * 3 * 2)
+#define MCP4728_WRITE_EEPROM_LEN (1 + MCP4728_N_CHANNELS * 2)
+
+enum vref_mode {
+ MCP4728_VREF_EXTERNAL_VDD = 0,
+ MCP4728_VREF_INTERNAL_2048mV = 1,
+};
+
+enum gain_mode {
+ MCP4728_GAIN_X1 = 0,
+ MCP4728_GAIN_X2 = 1,
+};
+
+enum iio_powerdown_mode {
+ MCP4728_IIO_1K,
+ MCP4728_IIO_100K,
+ MCP4728_IIO_500K,
+};
+
+struct mcp4728_channel_data {
+ enum vref_mode ref_mode;
+ enum iio_powerdown_mode pd_mode;
+ enum gain_mode g_mode;
+ u16 dac_value;
+};
+
+/* MCP4728 Full Scale Ranges
+ * the device available ranges are
+ * - VREF = VDD FSR = from 0.0V to VDD
+ * - VREF = Internal Gain = 1 FSR = from 0.0V to VREF
+ * - VREF = Internal Gain = 2 FSR = from 0.0V to 2*VREF
+ */
+enum mcp4728_scale {
+ MCP4728_SCALE_VDD,
+ MCP4728_SCALE_VINT_NO_GAIN,
+ MCP4728_SCALE_VINT_GAIN_X2,
+ MCP4728_N_SCALES
+};
+
+struct mcp4728_data {
+ struct i2c_client *client;
+ struct regulator *vdd_reg;
+ bool powerdown;
+ int scales_avail[MCP4728_N_SCALES * 2];
+ struct mcp4728_channel_data chdata[MCP4728_N_CHANNELS];
+};
+
+#define MCP4728_CHAN(chan) { \
+ .type = IIO_VOLTAGE, \
+ .output = 1, \
+ .indexed = 1, \
+ .channel = chan, \
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
+ BIT(IIO_CHAN_INFO_SCALE), \
+ .info_mask_shared_by_type_available = BIT(IIO_CHAN_INFO_SCALE), \
+ .ext_info = mcp4728_ext_info, \
+}
+
+static int mcp4728_suspend(struct device *dev);
+static int mcp4728_resume(struct device *dev);
+
+static ssize_t mcp4728_store_eeprom(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t len)
+{
+ struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+ struct mcp4728_data *data = iio_priv(indio_dev);
+ u8 outbuf[MCP4728_WRITE_EEPROM_LEN];
+ int tries = 20;
+ u8 inbuf[3];
+ bool state;
+ int ret;
+ unsigned int i;
+
+ ret = kstrtobool(buf, &state);
+ if (ret < 0)
+ return ret;
+
+ if (!state)
+ return 0;
+
+ outbuf[0] = FIELD_PREP(MCP4728_CMD_MASK, MCP4728_SW_CMD);
+
+ for (i = 0; i < MCP4728_N_CHANNELS; i++) {
+ struct mcp4728_channel_data *ch = &data->chdata[i];
+ int offset = 1 + i * 2;
+
+ outbuf[offset] = FIELD_PREP(MCP4728_VREF_MASK, ch->ref_mode);
+
+ if (data->powerdown) {
+ u8 mcp4728_pd_mode = ch->pd_mode + 1;
+
+ outbuf[offset] |= FIELD_PREP(MCP4728_PDMODE_MASK,
+ mcp4728_pd_mode);
+ }
+
+ outbuf[offset] |= FIELD_PREP(MCP4728_GAIN_MASK, ch->g_mode);
+ outbuf[offset] |=
+ FIELD_PREP(MCP4728_DAC_H_MASK, ch->dac_value >> 8);
+ outbuf[offset + 1] =
+ FIELD_PREP(MCP4728_DAC_L_MASK, ch->dac_value);
+ }
+
+ ret = i2c_master_send(data->client, outbuf, MCP4728_WRITE_EEPROM_LEN);
+ if (ret < 0)
+ return ret;
+ else if (ret != MCP4728_WRITE_EEPROM_LEN)
+ return -EIO;
+
+ /* wait RDY signal for write complete, takes up to 50ms */
+ while (tries--) {
+ msleep(20);
+ ret = i2c_master_recv(data->client, inbuf, 3);
+ if (ret < 0)
+ return ret;
+ else if (ret != 3)
+ return -EIO;
+
+ if (FIELD_GET(MCP4728_RDY_MASK, inbuf[0]))
+ break;
+ }
+
+ if (tries < 0) {
+ dev_err(&data->client->dev, "%s failed, incomplete\n",
+ __func__);
+ return -EIO;
+ }
+ return len;
+}
+
+static IIO_DEVICE_ATTR(store_eeprom, 0200, NULL, mcp4728_store_eeprom, 0);
+
+static struct attribute *mcp4728_attributes[] = {
+ &iio_dev_attr_store_eeprom.dev_attr.attr,
+ NULL,
+};
+
+static const struct attribute_group mcp4728_attribute_group = {
+ .attrs = mcp4728_attributes,
+};
+
+static int mcp4728_program_channel_cfg(int channel, struct iio_dev *indio_dev)
+{
+ struct mcp4728_data *data = iio_priv(indio_dev);
+ struct mcp4728_channel_data *ch = &data->chdata[channel];
+ u8 outbuf[3];
+ int ret;
+
+ outbuf[0] = FIELD_PREP(MCP4728_CMD_MASK, MCP4728_MW_CMD);
+ outbuf[0] |= FIELD_PREP(MCP4728_CHSEL_MASK, channel);
+ outbuf[0] |= FIELD_PREP(MCP4728_UDAC_MASK, 0);
+
+ outbuf[1] = FIELD_PREP(MCP4728_VREF_MASK, ch->ref_mode);
+
+ if (data->powerdown)
+ outbuf[1] |= FIELD_PREP(MCP4728_PDMODE_MASK, ch->pd_mode + 1);
+
+ outbuf[1] |= FIELD_PREP(MCP4728_GAIN_MASK, ch->g_mode);
+ outbuf[1] |= FIELD_PREP(MCP4728_DAC_H_MASK, ch->dac_value >> 8);
+ outbuf[2] = FIELD_PREP(MCP4728_DAC_L_MASK, ch->dac_value);
+
+ ret = i2c_master_send(data->client, outbuf, 3);
+ if (ret < 0)
+ return ret;
+ else if (ret != 3)
+ return -EIO;
+
+ return 0;
+}
+
+static const char *const mcp4728_powerdown_modes[] = { "1kohm_to_gnd",
+ "100kohm_to_gnd",
+ "500kohm_to_gnd" };
+
+static int mcp4728_get_powerdown_mode(struct iio_dev *indio_dev,
+ const struct iio_chan_spec *chan)
+{
+ struct mcp4728_data *data = iio_priv(indio_dev);
+
+ return data->chdata[chan->channel].pd_mode;
+}
+
+static int mcp4728_set_powerdown_mode(struct iio_dev *indio_dev,
+ const struct iio_chan_spec *chan,
+ unsigned int mode)
+{
+ struct mcp4728_data *data = iio_priv(indio_dev);
+
+ data->chdata[chan->channel].pd_mode = mode;
+
+ return 0;
+}
+
+static ssize_t mcp4728_read_powerdown(struct iio_dev *indio_dev,
+ uintptr_t private,
+ const struct iio_chan_spec *chan,
+ char *buf)
+{
+ struct mcp4728_data *data = iio_priv(indio_dev);
+
+ return sysfs_emit(buf, "%d\n", data->powerdown);
+}
+
+static ssize_t mcp4728_write_powerdown(struct iio_dev *indio_dev,
+ uintptr_t private,
+ const struct iio_chan_spec *chan,
+ const char *buf, size_t len)
+{
+ struct mcp4728_data *data = iio_priv(indio_dev);
+ bool state;
+ int ret;
+
+ ret = kstrtobool(buf, &state);
+ if (ret)
+ return ret;
+
+ if (state)
+ ret = mcp4728_suspend(&data->client->dev);
+ else
+ ret = mcp4728_resume(&data->client->dev);
+
+ if (ret < 0)
+ return ret;
+
+ return len;
+}
+
+static const struct iio_enum mcp4728_powerdown_mode_enum = {
+ .items = mcp4728_powerdown_modes,
+ .num_items = ARRAY_SIZE(mcp4728_powerdown_modes),
+ .get = mcp4728_get_powerdown_mode,
+ .set = mcp4728_set_powerdown_mode,
+};
+
+static const struct iio_chan_spec_ext_info mcp4728_ext_info[] = {
+ {
+ .name = "powerdown",
+ .read = mcp4728_read_powerdown,
+ .write = mcp4728_write_powerdown,
+ .shared = IIO_SEPARATE,
+ },
+ IIO_ENUM("powerdown_mode", IIO_SEPARATE, &mcp4728_powerdown_mode_enum),
+ IIO_ENUM_AVAILABLE("powerdown_mode", IIO_SHARED_BY_TYPE,
+ &mcp4728_powerdown_mode_enum),
+ {},
+};
+
+static const struct iio_chan_spec mcp4728_channels[MCP4728_N_CHANNELS] = {
+ MCP4728_CHAN(0),
+ MCP4728_CHAN(1),
+ MCP4728_CHAN(2),
+ MCP4728_CHAN(3),
+};
+
+static void mcp4728_get_scale_avail(enum mcp4728_scale scale,
+ struct mcp4728_data *data, int *val,
+ int *val2)
+{
+ *val = data->scales_avail[scale * 2];
+ *val2 = data->scales_avail[scale * 2 + 1];
+}
+
+static void mcp4728_get_scale(int channel, struct mcp4728_data *data, int *val,
+ int *val2)
+{
+ int ref_mode = data->chdata[channel].ref_mode;
+ int g_mode = data->chdata[channel].g_mode;
+
+ if (ref_mode == MCP4728_VREF_EXTERNAL_VDD) {
+ mcp4728_get_scale_avail(MCP4728_SCALE_VDD, data, val, val2);
+ } else {
+ if (g_mode == MCP4728_GAIN_X1) {
+ mcp4728_get_scale_avail(MCP4728_SCALE_VINT_NO_GAIN,
+ data, val, val2);
+ } else {
+ mcp4728_get_scale_avail(MCP4728_SCALE_VINT_GAIN_X2,
+ data, val, val2);
+ }
+ }
+}
+
+static int mcp4728_find_matching_scale(struct mcp4728_data *data, int val,
+ int val2)
+{
+ for (int i = 0; i < MCP4728_N_SCALES; i++) {
+ if (data->scales_avail[i * 2] == val &&
+ data->scales_avail[i * 2 + 1] == val2)
+ return i;
+ }
+ return -EINVAL;
+}
+
+static int mcp4728_set_scale(int channel, struct mcp4728_data *data, int val,
+ int val2)
+{
+ int scale = mcp4728_find_matching_scale(data, val, val2);
+
+ if (scale < 0)
+ return scale;
+
+ switch (scale) {
+ case MCP4728_SCALE_VDD:
+ data->chdata[channel].ref_mode = MCP4728_VREF_EXTERNAL_VDD;
+ break;
+ case MCP4728_SCALE_VINT_NO_GAIN:
+ data->chdata[channel].ref_mode = MCP4728_VREF_INTERNAL_2048mV;
+ data->chdata[channel].g_mode = MCP4728_GAIN_X1;
+ break;
+ case MCP4728_SCALE_VINT_GAIN_X2:
+ data->chdata[channel].ref_mode = MCP4728_VREF_INTERNAL_2048mV;
+ data->chdata[channel].g_mode = MCP4728_GAIN_X2;
+ break;
+ default:
+ return -EINVAL;
+ }
+ return 0;
+}
+
+static int mcp4728_read_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan, int *val,
+ int *val2, long mask)
+{
+ struct mcp4728_data *data = iio_priv(indio_dev);
+
+ switch (mask) {
+ case IIO_CHAN_INFO_RAW:
+ *val = data->chdata[chan->channel].dac_value;
+ return IIO_VAL_INT;
+ case IIO_CHAN_INFO_SCALE:
+ mcp4728_get_scale(chan->channel, data, val, val2);
+ return IIO_VAL_INT_PLUS_MICRO;
+ }
+ return -EINVAL;
+}
+
+static int mcp4728_write_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan, int val,
+ int val2, long mask)
+{
+ struct mcp4728_data *data = iio_priv(indio_dev);
+ int ret;
+
+ switch (mask) {
+ case IIO_CHAN_INFO_RAW:
+ if (val < 0 || val > GENMASK(MCP4728_RESOLUTION - 1, 0))
+ return -EINVAL;
+ data->chdata[chan->channel].dac_value = val;
+ ret = mcp4728_program_channel_cfg(chan->channel, indio_dev);
+ break;
+ case IIO_CHAN_INFO_SCALE:
+ ret = mcp4728_set_scale(chan->channel, data, val, val2);
+ if (ret)
+ break;
+
+ ret = mcp4728_program_channel_cfg(chan->channel, indio_dev);
+ break;
+ default:
+ ret = -EINVAL;
+ break;
+ }
+
+ return ret;
+}
+
+static void mcp4728_init_scale_avail(enum mcp4728_scale scale, int vref_mv,
+ struct mcp4728_data *data)
+{
+ s64 tmp;
+ int value_micro;
+ int value_int;
+
+ tmp = (s64)vref_mv * 1000000LL >> MCP4728_RESOLUTION;
+ value_int = div_s64_rem(tmp, 1000000LL, &value_micro);
+
+ data->scales_avail[scale * 2] = value_int;
+ data->scales_avail[scale * 2 + 1] = value_micro;
+}
+
+static int mcp4728_init_scales_avail(struct mcp4728_data *data)
+{
+ int ret;
+
+ ret = regulator_get_voltage(data->vdd_reg);
+ if (ret < 0)
+ return ret;
+
+ mcp4728_init_scale_avail(MCP4728_SCALE_VDD, ret / 1000, data);
+ mcp4728_init_scale_avail(MCP4728_SCALE_VINT_NO_GAIN, 2048, data);
+ mcp4728_init_scale_avail(MCP4728_SCALE_VINT_GAIN_X2, 4096, data);
+
+ return 0;
+}
+
+static int mcp4728_read_avail(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ const int **vals, int *type, int *length,
+ long info)
+{
+ struct mcp4728_data *data = iio_priv(indio_dev);
+
+ switch (info) {
+ case IIO_CHAN_INFO_SCALE:
+ *type = IIO_VAL_INT_PLUS_MICRO;
+
+ switch (chan->type) {
+ case IIO_VOLTAGE:
+ *vals = data->scales_avail;
+ *length = MCP4728_N_SCALES * 2;
+ return IIO_AVAIL_LIST;
+ default:
+ return -EINVAL;
+ }
+ default:
+ return -EINVAL;
+ }
+}
+
+static const struct iio_info mcp4728_info = {
+ .read_raw = mcp4728_read_raw,
+ .write_raw = mcp4728_write_raw,
+ .read_avail = &mcp4728_read_avail,
+ .attrs = &mcp4728_attribute_group,
+};
+
+static int mcp4728_suspend(struct device *dev)
+{
+ struct iio_dev *indio_dev = dev_get_drvdata(dev);
+ struct mcp4728_data *data = iio_priv(indio_dev);
+ unsigned int i;
+
+ data->powerdown = true;
+
+ for (i = 0; i < MCP4728_N_CHANNELS; i++) {
+ int err = mcp4728_program_channel_cfg(i, indio_dev);
+
+ if (err)
+ return err;
+ }
+ return 0;
+}
+
+static int mcp4728_resume(struct device *dev)
+{
+ struct iio_dev *indio_dev = dev_get_drvdata(dev);
+ struct mcp4728_data *data = iio_priv(indio_dev);
+ int err = 0;
+ unsigned int i;
+
+ data->powerdown = false;
+
+ for (i = 0; i < MCP4728_N_CHANNELS; i++) {
+ int ret = mcp4728_program_channel_cfg(i, indio_dev);
+
+ if (ret)
+ err = ret;
+ }
+ return err;
+}
+
+static DEFINE_SIMPLE_DEV_PM_OPS(mcp4728_pm_ops, mcp4728_suspend,
+ mcp4728_resume);
+
+static int mcp4728_init_channels_data(struct mcp4728_data *data)
+{
+ u8 inbuf[MCP4728_READ_RESPONSE_LEN];
+ int ret;
+ unsigned int i;
+
+ ret = i2c_master_recv(data->client, inbuf, MCP4728_READ_RESPONSE_LEN);
+ if (ret < 0) {
+ dev_err(&data->client->dev,
+ "failed to read mcp4728 conf. Err=%d\n", ret);
+ return ret;
+ } else if (ret != MCP4728_READ_RESPONSE_LEN) {
+ dev_err(&data->client->dev,
+ "failed to read mcp4728 conf. Wrong Response Len ret=%d\n",
+ ret);
+ return -EIO;
+ }
+
+ for (i = 0; i < MCP4728_N_CHANNELS; i++) {
+ struct mcp4728_channel_data *ch = &data->chdata[i];
+ u8 r2 = inbuf[i * 6 + 1];
+ u8 r3 = inbuf[i * 6 + 2];
+
+ ch->dac_value = FIELD_GET(MCP4728_DAC_H_MASK, r2) << 8 |
+ FIELD_GET(MCP4728_DAC_L_MASK, r3);
+ ch->ref_mode = FIELD_GET(MCP4728_VREF_MASK, r2);
+ ch->pd_mode = FIELD_GET(MCP4728_PDMODE_MASK, r2);
+ ch->g_mode = FIELD_GET(MCP4728_GAIN_MASK, r2);
+ }
+
+ return 0;
+}
+
+static void mcp4728_reg_disable(void *reg)
+{
+ regulator_disable(reg);
+}
+
+static int mcp4728_probe(struct i2c_client *client)
+{
+ const struct i2c_device_id *id = i2c_client_get_device_id(client);
+ struct mcp4728_data *data;
+ struct iio_dev *indio_dev;
+ int err;
+
+ indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
+ if (!indio_dev)
+ return -ENOMEM;
+
+ data = iio_priv(indio_dev);
+ i2c_set_clientdata(client, indio_dev);
+ data->client = client;
+
+ data->vdd_reg = devm_regulator_get(&client->dev, "vdd");
+ if (IS_ERR(data->vdd_reg))
+ return PTR_ERR(data->vdd_reg);
+
+ err = regulator_enable(data->vdd_reg);
+ if (err)
+ return err;
+
+ err = devm_add_action_or_reset(&client->dev, mcp4728_reg_disable,
+ data->vdd_reg);
+ if (err)
+ return err;
+
+ /* MCP4728 has internal EEPROM that save each channel boot configuration.
+ * It means that device configuration is unknown to the driver at kernel boot.
+ * mcp4728_init_channels_data reads back DAC settings and stores them in data
+ * structure.
+ */
+ err = mcp4728_init_channels_data(data);
+ if (err) {
+ dev_err(&client->dev,
+ "failed to read mcp4728 current configuration\n");
+ return err;
+ }
+
+ err = mcp4728_init_scales_avail(data);
+ if (err) {
+ dev_err(&client->dev, "failed to init scales\n");
+ return err;
+ }
+
+ indio_dev->name = id->name;
+ indio_dev->info = &mcp4728_info;
+ indio_dev->channels = mcp4728_channels;
+ indio_dev->num_channels = MCP4728_N_CHANNELS;
+ indio_dev->modes = INDIO_DIRECT_MODE;
+
+ return devm_iio_device_register(&client->dev, indio_dev);
+}
+
+static const struct i2c_device_id mcp4728_id[] = {
+ { "mcp4728", 0 },
+ {}
+};
+MODULE_DEVICE_TABLE(i2c, mcp4728_id);
+
+static const struct of_device_id mcp4728_of_match[] = {
+ { .compatible = "microchip,mcp4728" },
+ {}
+};
+MODULE_DEVICE_TABLE(of, mcp4728_of_match);
+
+static struct i2c_driver mcp4728_driver = {
+ .driver = {
+ .name = "mcp4728",
+ .of_match_table = mcp4728_of_match,
+ .pm = pm_sleep_ptr(&mcp4728_pm_ops),
+ },
+ .probe = mcp4728_probe,
+ .id_table = mcp4728_id,
+};
+module_i2c_driver(mcp4728_driver);
+
+MODULE_AUTHOR("Andrea Collamati <andrea.collamati@gmail.com>");
+MODULE_DESCRIPTION("MCP4728 12-bit DAC");
+MODULE_LICENSE("GPL");
--
2.34.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v4 1/2] dt-bindings: iio: dac: add mcp4728.yaml
2023-08-03 12:56 ` [PATCH v4 1/2] dt-bindings: iio: dac: add mcp4728.yaml Andrea Collamati
@ 2023-08-03 15:21 ` Conor Dooley
2023-08-03 18:39 ` Andrea Collamati
0 siblings, 1 reply; 8+ messages in thread
From: Conor Dooley @ 2023-08-03 15:21 UTC (permalink / raw)
To: Andrea Collamati
Cc: Jonathan Cameron, Lars-Peter Clausen, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, linux-iio, devicetree,
linux-kernel
[-- Attachment #1: Type: text/plain, Size: 2043 bytes --]
On Thu, Aug 03, 2023 at 02:56:34PM +0200, Andrea Collamati wrote:
> Add documentation for MCP4728
>
> Signed-off-by: Andrea Collamati <andrea.collamati@gmail.com>
I gave you a reviewed-by on v3, is there a reason that you dropped it?
Thanks,
Conor.
> ---
> .../bindings/iio/dac/microchip,mcp4728.yaml | 49 +++++++++++++++++++
> 1 file changed, 49 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/iio/dac/microchip,mcp4728.yaml
>
> diff --git a/Documentation/devicetree/bindings/iio/dac/microchip,mcp4728.yaml b/Documentation/devicetree/bindings/iio/dac/microchip,mcp4728.yaml
> new file mode 100644
> index 000000000000..99831d7f1c16
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/dac/microchip,mcp4728.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,mcp4728.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Microchip MCP4728 DAC
> +
> +maintainers:
> + - Andrea Collamati <andrea.collamati@gmail.com>
> +
> +description: |
> + MCP4728 is a quad channel, 12-bit voltage output
> + Digital-to-Analog Converter with non-volatile
> + memory and I2C compatible Serial Interface.
> + https://www.microchip.com/en-us/product/mcp4728
> +
> +properties:
> + compatible:
> + const: microchip,mcp4728
> +
> + reg:
> + maxItems: 1
> +
> + vdd-supply:
> + description: |
> + Provides both power and acts as the reference supply on the MCP4728
> + when Internal Vref is not selected.
> +
> +required:
> + - compatible
> + - reg
> + - vdd-supply
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + i2c {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + dac@60 {
> + compatible = "microchip,mcp4728";
> + reg = <0x60>;
> + vdd-supply = <&vdac_vdd>;
> + };
> + };
> --
> 2.34.1
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v4 1/2] dt-bindings: iio: dac: add mcp4728.yaml
2023-08-03 15:21 ` Conor Dooley
@ 2023-08-03 18:39 ` Andrea Collamati
2023-08-03 18:46 ` Conor Dooley
0 siblings, 1 reply; 8+ messages in thread
From: Andrea Collamati @ 2023-08-03 18:39 UTC (permalink / raw)
To: Conor Dooley
Cc: Jonathan Cameron, Lars-Peter Clausen, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, linux-iio, devicetree,
linux-kernel
Sorry it's the first time I try to submit a driver.
So after your positive review I should add...
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
I will do in v5..
Andrea
On 8/3/23 17:21, Conor Dooley wrote:
> On Thu, Aug 03, 2023 at 02:56:34PM +0200, Andrea Collamati wrote:
>> Add documentation for MCP4728
>>
>> Signed-off-by: Andrea Collamati <andrea.collamati@gmail.com>
> I gave you a reviewed-by on v3, is there a reason that you dropped it?
>
> Thanks,
> Conor.
>
>> ---
>> .../bindings/iio/dac/microchip,mcp4728.yaml | 49 +++++++++++++++++++
>> 1 file changed, 49 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/iio/dac/microchip,mcp4728.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/iio/dac/microchip,mcp4728.yaml b/Documentation/devicetree/bindings/iio/dac/microchip,mcp4728.yaml
>> new file mode 100644
>> index 000000000000..99831d7f1c16
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/iio/dac/microchip,mcp4728.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,mcp4728.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Microchip MCP4728 DAC
>> +
>> +maintainers:
>> + - Andrea Collamati <andrea.collamati@gmail.com>
>> +
>> +description: |
>> + MCP4728 is a quad channel, 12-bit voltage output
>> + Digital-to-Analog Converter with non-volatile
>> + memory and I2C compatible Serial Interface.
>> + https://www.microchip.com/en-us/product/mcp4728
>> +
>> +properties:
>> + compatible:
>> + const: microchip,mcp4728
>> +
>> + reg:
>> + maxItems: 1
>> +
>> + vdd-supply:
>> + description: |
>> + Provides both power and acts as the reference supply on the MCP4728
>> + when Internal Vref is not selected.
>> +
>> +required:
>> + - compatible
>> + - reg
>> + - vdd-supply
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> + - |
>> + i2c {
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> +
>> + dac@60 {
>> + compatible = "microchip,mcp4728";
>> + reg = <0x60>;
>> + vdd-supply = <&vdac_vdd>;
>> + };
>> + };
>> --
>> 2.34.1
>>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v4 1/2] dt-bindings: iio: dac: add mcp4728.yaml
2023-08-03 18:39 ` Andrea Collamati
@ 2023-08-03 18:46 ` Conor Dooley
0 siblings, 0 replies; 8+ messages in thread
From: Conor Dooley @ 2023-08-03 18:46 UTC (permalink / raw)
To: Andrea Collamati
Cc: Jonathan Cameron, Lars-Peter Clausen, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, linux-iio, devicetree,
linux-kernel
[-- Attachment #1: Type: text/plain, Size: 743 bytes --]
On Thu, Aug 03, 2023 at 08:39:30PM +0200, Andrea Collamati wrote:
> On 8/3/23 17:21, Conor Dooley wrote:
> > On Thu, Aug 03, 2023 at 02:56:34PM +0200, Andrea Collamati wrote:
> >> Add documentation for MCP4728
> >>
> >> Signed-off-by: Andrea Collamati <andrea.collamati@gmail.com>
> > I gave you a reviewed-by on v3, is there a reason that you dropped it?
> Sorry it's the first time I try to submit a driver.
No worries. It's just hard to know if people do things intentionally or
not!
> So after your positive review I should add...
>
> Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
>
> I will do in v5.
But don't post a v5 for that alone, if this version is fine then it can
be picked up on application.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v4 2/2] iio: add MCP4728 I2C DAC driver
2023-08-03 12:56 ` [PATCH v4 2/2] iio: add MCP4728 I2C DAC driver Andrea Collamati
@ 2023-08-05 18:28 ` Jonathan Cameron
2023-08-06 13:40 ` Andrea Collamati
0 siblings, 1 reply; 8+ messages in thread
From: Jonathan Cameron @ 2023-08-05 18:28 UTC (permalink / raw)
To: Andrea Collamati
Cc: Lars-Peter Clausen, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, linux-iio, devicetree, linux-kernel
On Thu, 3 Aug 2023 14:56:35 +0200
Andrea Collamati <andrea.collamati@gmail.com> wrote:
> MCP4728 is a 12-bit quad channel DAC with I2C interface.
>
> support for:
> * per-channel gain
> * per-channel power state
> * per-channel power down mode control
> * per-channel vref selection internal/vdd
> * store current state to on-chip EEPROM
>
> Signed-off-by: Andrea Collamati <andrea.collamati@gmail.com>
Hi Andrea,
On this final read through I noticed a few minor things. Rather than
get you to do a v5 I've made the changes whilst applying. Please
take a sanity check at
https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git/log/?h=testing
to make sure I didn't break anything and let me know if I did!
Applied with the below mentioned changes to the togreg branch of iio.git
which is initially pushed out as testing to let 0-day poke at it and see
if it can find any problems.
Thanks,
Jonathan
> diff --git a/drivers/iio/dac/mcp4728.c b/drivers/iio/dac/mcp4728.c
> new file mode 100644
> index 000000000000..ba3eab349b0a
> --- /dev/null
> +++ b/drivers/iio/dac/mcp4728.c
> @@ -0,0 +1,626 @@
...
> +
> +static int mcp4728_set_scale(int channel, struct mcp4728_data *data, int val,
> + int val2)
> +{
> + int scale = mcp4728_find_matching_scale(data, val, val2);
> +
> + if (scale < 0)
> + return scale;
> +
> + switch (scale) {
> + case MCP4728_SCALE_VDD:
> + data->chdata[channel].ref_mode = MCP4728_VREF_EXTERNAL_VDD;
> + break;
> + case MCP4728_SCALE_VINT_NO_GAIN:
> + data->chdata[channel].ref_mode = MCP4728_VREF_INTERNAL_2048mV;
> + data->chdata[channel].g_mode = MCP4728_GAIN_X1;
> + break;
> + case MCP4728_SCALE_VINT_GAIN_X2:
> + data->chdata[channel].ref_mode = MCP4728_VREF_INTERNAL_2048mV;
> + data->chdata[channel].g_mode = MCP4728_GAIN_X2;
> + break;
> + default:
> + return -EINVAL;
> + }
> + return 0;
return instead of breaking above as nothing to be done after the switch.
Side effect of that is this return 0 not needed.
> +}
...
> +static int mcp4728_write_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan, int val,
> + int val2, long mask)
> +{
> + struct mcp4728_data *data = iio_priv(indio_dev);
> + int ret;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
> + if (val < 0 || val > GENMASK(MCP4728_RESOLUTION - 1, 0))
> + return -EINVAL;
> + data->chdata[chan->channel].dac_value = val;
> + ret = mcp4728_program_channel_cfg(chan->channel, indio_dev);
> + break;
Direct returns preferred if no cleanup to do.
> + case IIO_CHAN_INFO_SCALE:
> + ret = mcp4728_set_scale(chan->channel, data, val, val2);
> + if (ret)
> + break;
> +
> + ret = mcp4728_program_channel_cfg(chan->channel, indio_dev);
> + break;
> + default:
> + ret = -EINVAL;
> + break;
> + }
> +
> + return ret;
> +}
> +static int mcp4728_init_channels_data(struct mcp4728_data *data)
> +{
> + u8 inbuf[MCP4728_READ_RESPONSE_LEN];
> + int ret;
> + unsigned int i;
> +
> + ret = i2c_master_recv(data->client, inbuf, MCP4728_READ_RESPONSE_LEN);
> + if (ret < 0) {
> + dev_err(&data->client->dev,
> + "failed to read mcp4728 conf. Err=%d\n", ret);
> + return ret;
As this is only called from probe, we should still use dev_err_probe() in here
as mentioned below.
> + } else if (ret != MCP4728_READ_RESPONSE_LEN) {
> + dev_err(&data->client->dev,
> + "failed to read mcp4728 conf. Wrong Response Len ret=%d\n",
> + ret);
> + return -EIO;
> + }
> +
> + for (i = 0; i < MCP4728_N_CHANNELS; i++) {
> + struct mcp4728_channel_data *ch = &data->chdata[i];
> + u8 r2 = inbuf[i * 6 + 1];
> + u8 r3 = inbuf[i * 6 + 2];
> +
> + ch->dac_value = FIELD_GET(MCP4728_DAC_H_MASK, r2) << 8 |
> + FIELD_GET(MCP4728_DAC_L_MASK, r3);
> + ch->ref_mode = FIELD_GET(MCP4728_VREF_MASK, r2);
> + ch->pd_mode = FIELD_GET(MCP4728_PDMODE_MASK, r2);
> + ch->g_mode = FIELD_GET(MCP4728_GAIN_MASK, r2);
> + }
> +
> + return 0;
> +}
...
> +static int mcp4728_probe(struct i2c_client *client)
> +{
> + const struct i2c_device_id *id = i2c_client_get_device_id(client);
> + struct mcp4728_data *data;
> + struct iio_dev *indio_dev;
> + int err;
> +
> + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + data = iio_priv(indio_dev);
> + i2c_set_clientdata(client, indio_dev);
> + data->client = client;
> +
> + data->vdd_reg = devm_regulator_get(&client->dev, "vdd");
> + if (IS_ERR(data->vdd_reg))
> + return PTR_ERR(data->vdd_reg);
> +
> + err = regulator_enable(data->vdd_reg);
> + if (err)
> + return err;
> +
> + err = devm_add_action_or_reset(&client->dev, mcp4728_reg_disable,
> + data->vdd_reg);
> + if (err)
> + return err;
> +
> + /* MCP4728 has internal EEPROM that save each channel boot configuration.
Trivial but comment syntax in IIO (there are some differences in a few other subsystems)
is
/*
* MCP...
> + * It means that device configuration is unknown to the driver at kernel boot.
> + * mcp4728_init_channels_data reads back DAC settings and stores them in data
> + * structure.
> + */
> + err = mcp4728_init_channels_data(data);
> + if (err) {
I'd missed this previously but dev_err_probe() has several advantages in that
it handles deferring neatly and also ends up with less code.
> + dev_err(&client->dev,
> + "failed to read mcp4728 current configuration\n");
> + return err;
> + }
> +
> + err = mcp4728_init_scales_avail(data);
> + if (err) {
> + dev_err(&client->dev, "failed to init scales\n");
> + return err;
> + }
> +
> + indio_dev->name = id->name;
> + indio_dev->info = &mcp4728_info;
> + indio_dev->channels = mcp4728_channels;
> + indio_dev->num_channels = MCP4728_N_CHANNELS;
> + indio_dev->modes = INDIO_DIRECT_MODE;
> +
> + return devm_iio_device_register(&client->dev, indio_dev);
> +}
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v4 2/2] iio: add MCP4728 I2C DAC driver
2023-08-05 18:28 ` Jonathan Cameron
@ 2023-08-06 13:40 ` Andrea Collamati
0 siblings, 0 replies; 8+ messages in thread
From: Andrea Collamati @ 2023-08-06 13:40 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Lars-Peter Clausen, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, linux-iio, devicetree, linux-kernel
Hi Jonathan,
I tested the driver and I didn't find any issue.
These are the tests I performed:
Test Configuration:
SW:
git://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git
commit: ee812337ee74cc403b4fbd85840dbe1b21d9a60f
HW:
- Aster board from Toradex hosting imx7d processor
- Adafruit MCP4728 https://www.adafruit.com/product/4470
TEST:
Probe Test OK
#> insmod mcp4728.ko
Read Scales OK
~# cat /sys/devices/platform/soc/30800000.bus/30a50000.i2c/i2c-3/3-0064/iio:device1/out_voltage_scale_available
0.805664 0.500000 1.000000
Write DAC CH0 OK
#>iio_attr -c mcp4728 voltage0 scale 1.000000
#>iio_attr -c mcp4728 voltage0 raw 750
Voltage Monitor Reading: 749 mV
Write DAC CH1 OK
#>iio_attr -c mcp4728 voltage1 scale 0.5
#>iio_attr -c mcp4728 voltage1 raw 750
Voltage Monitor Reading: 378 mV
Write DAC CH2 OK
#>iio_attr -c mcp4728 voltage2 scale 0.805664
#>iio_attr -c mcp4728 voltage2 raw 750
Voltage Monitor Reading: 603 mV ~ 750*0.805664 ~ 604 mV
Write DAC CH3 OK
#>iio_attr -c mcp4728 voltage3 scale 1.000000
#>iio_attr -c mcp4728 voltage3 raw 350
Voltage Monitor Reading: 354 mV
Store EEPROM OK
#>echo y > /sys/devices/platform/soc/30800000.bus/30a50000.i2c/i2c-3/3-0064/iio:device1/store_eeprom
#>power cycle board
Voltage Monitor Reading CH0 749 mV
Voltage Monitor Reading CH1 378 mV
Voltage Monitor Reading CH2 605 mV
Voltage Monitor Reading CH3 355 mV
Power Down CH0
#>iio_attr -c mcp4728 voltage0 powerdown_mode 1kohm_to_gnd
#>iio_attr -c mcp4728 voltage0 powerdown 1
Voltage Monitor Reading CH0 0 mV
Power Down CH1
#>iio_attr -c mcp4728 voltage1 powerdown_mode 1kohm_to_gnd
#>iio_attr -c mcp4728 voltage1 powerdown 1
Voltage Monitor Reading CH1 0 mV
Power Down CH2
#>iio_attr -c mcp4728 voltage2 powerdown_mode 1kohm_to_gnd
#>iio_attr -c mcp4728 voltage2 powerdown 1
Voltage Monitor Reading CH2 0 mV
Power Down CH3
#>iio_attr -c mcp4728 voltage3 powerdown_mode 1kohm_to_gnd
#>iio_attr -c mcp4728 voltage3 powerdown 1
Voltage Monitor Reading CH3 0 mV
Regards
Andrea
On 8/5/23 20:28, Jonathan Cameron wrote:
> On Thu, 3 Aug 2023 14:56:35 +0200
> Andrea Collamati <andrea.collamati@gmail.com> wrote:
>
>> MCP4728 is a 12-bit quad channel DAC with I2C interface.
>>
>> support for:
>> * per-channel gain
>> * per-channel power state
>> * per-channel power down mode control
>> * per-channel vref selection internal/vdd
>> * store current state to on-chip EEPROM
>>
>> Signed-off-by: Andrea Collamati <andrea.collamati@gmail.com>
> Hi Andrea,
>
> On this final read through I noticed a few minor things. Rather than
> get you to do a v5 I've made the changes whilst applying. Please
> take a sanity check at
> https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git/log/?h=testing
> to make sure I didn't break anything and let me know if I did!
>
> Applied with the below mentioned changes to the togreg branch of iio.git
> which is initially pushed out as testing to let 0-day poke at it and see
> if it can find any problems.
>
> Thanks,
>
> Jonathan
>
>> diff --git a/drivers/iio/dac/mcp4728.c b/drivers/iio/dac/mcp4728.c
>> new file mode 100644
>> index 000000000000..ba3eab349b0a
>> --- /dev/null
>> +++ b/drivers/iio/dac/mcp4728.c
>> @@ -0,0 +1,626 @@
> ...
>
>
>
>> +
>> +static int mcp4728_set_scale(int channel, struct mcp4728_data *data, int val,
>> + int val2)
>> +{
>> + int scale = mcp4728_find_matching_scale(data, val, val2);
>> +
>> + if (scale < 0)
>> + return scale;
>> +
>> + switch (scale) {
>> + case MCP4728_SCALE_VDD:
>> + data->chdata[channel].ref_mode = MCP4728_VREF_EXTERNAL_VDD;
>> + break;
>> + case MCP4728_SCALE_VINT_NO_GAIN:
>> + data->chdata[channel].ref_mode = MCP4728_VREF_INTERNAL_2048mV;
>> + data->chdata[channel].g_mode = MCP4728_GAIN_X1;
>> + break;
>> + case MCP4728_SCALE_VINT_GAIN_X2:
>> + data->chdata[channel].ref_mode = MCP4728_VREF_INTERNAL_2048mV;
>> + data->chdata[channel].g_mode = MCP4728_GAIN_X2;
>> + break;
>> + default:
>> + return -EINVAL;
>> + }
>> + return 0;
> return instead of breaking above as nothing to be done after the switch.
> Side effect of that is this return 0 not needed.
>
>> +}
> ...
>
>> +static int mcp4728_write_raw(struct iio_dev *indio_dev,
>> + struct iio_chan_spec const *chan, int val,
>> + int val2, long mask)
>> +{
>> + struct mcp4728_data *data = iio_priv(indio_dev);
>> + int ret;
>> +
>> + switch (mask) {
>> + case IIO_CHAN_INFO_RAW:
>> + if (val < 0 || val > GENMASK(MCP4728_RESOLUTION - 1, 0))
>> + return -EINVAL;
>> + data->chdata[chan->channel].dac_value = val;
>> + ret = mcp4728_program_channel_cfg(chan->channel, indio_dev);
>> + break;
> Direct returns preferred if no cleanup to do.
>
>> + case IIO_CHAN_INFO_SCALE:
>> + ret = mcp4728_set_scale(chan->channel, data, val, val2);
>> + if (ret)
>> + break;
>> +
>> + ret = mcp4728_program_channel_cfg(chan->channel, indio_dev);
>> + break;
>> + default:
>> + ret = -EINVAL;
>> + break;
>> + }
>> +
>> + return ret;
>> +}
>
>
>> +static int mcp4728_init_channels_data(struct mcp4728_data *data)
>> +{
>> + u8 inbuf[MCP4728_READ_RESPONSE_LEN];
>> + int ret;
>> + unsigned int i;
>> +
>> + ret = i2c_master_recv(data->client, inbuf, MCP4728_READ_RESPONSE_LEN);
>> + if (ret < 0) {
>> + dev_err(&data->client->dev,
>> + "failed to read mcp4728 conf. Err=%d\n", ret);
>> + return ret;
> As this is only called from probe, we should still use dev_err_probe() in here
> as mentioned below.
>
>
>> + } else if (ret != MCP4728_READ_RESPONSE_LEN) {
>> + dev_err(&data->client->dev,
>> + "failed to read mcp4728 conf. Wrong Response Len ret=%d\n",
>> + ret);
>> + return -EIO;
>> + }
>> +
>> + for (i = 0; i < MCP4728_N_CHANNELS; i++) {
>> + struct mcp4728_channel_data *ch = &data->chdata[i];
>> + u8 r2 = inbuf[i * 6 + 1];
>> + u8 r3 = inbuf[i * 6 + 2];
>> +
>> + ch->dac_value = FIELD_GET(MCP4728_DAC_H_MASK, r2) << 8 |
>> + FIELD_GET(MCP4728_DAC_L_MASK, r3);
>> + ch->ref_mode = FIELD_GET(MCP4728_VREF_MASK, r2);
>> + ch->pd_mode = FIELD_GET(MCP4728_PDMODE_MASK, r2);
>> + ch->g_mode = FIELD_GET(MCP4728_GAIN_MASK, r2);
>> + }
>> +
>> + return 0;
>> +}
> ...
>
>> +static int mcp4728_probe(struct i2c_client *client)
>> +{
>> + const struct i2c_device_id *id = i2c_client_get_device_id(client);
>> + struct mcp4728_data *data;
>> + struct iio_dev *indio_dev;
>> + int err;
>> +
>> + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
>> + if (!indio_dev)
>> + return -ENOMEM;
>> +
>> + data = iio_priv(indio_dev);
>> + i2c_set_clientdata(client, indio_dev);
>> + data->client = client;
>> +
>> + data->vdd_reg = devm_regulator_get(&client->dev, "vdd");
>> + if (IS_ERR(data->vdd_reg))
>> + return PTR_ERR(data->vdd_reg);
>> +
>> + err = regulator_enable(data->vdd_reg);
>> + if (err)
>> + return err;
>> +
>> + err = devm_add_action_or_reset(&client->dev, mcp4728_reg_disable,
>> + data->vdd_reg);
>> + if (err)
>> + return err;
>> +
>> + /* MCP4728 has internal EEPROM that save each channel boot configuration.
> Trivial but comment syntax in IIO (there are some differences in a few other subsystems)
> is
> /*
> * MCP...
>
>> + * It means that device configuration is unknown to the driver at kernel boot.
>> + * mcp4728_init_channels_data reads back DAC settings and stores them in data
>> + * structure.
>> + */
>> + err = mcp4728_init_channels_data(data);
>> + if (err) {
> I'd missed this previously but dev_err_probe() has several advantages in that
> it handles deferring neatly and also ends up with less code.
>
>> + dev_err(&client->dev,
>> + "failed to read mcp4728 current configuration\n");
>> + return err;
>> + }
>> +
>> + err = mcp4728_init_scales_avail(data);
>> + if (err) {
>> + dev_err(&client->dev, "failed to init scales\n");
>> + return err;
>> + }
>> +
>> + indio_dev->name = id->name;
>> + indio_dev->info = &mcp4728_info;
>> + indio_dev->channels = mcp4728_channels;
>> + indio_dev->num_channels = MCP4728_N_CHANNELS;
>> + indio_dev->modes = INDIO_DIRECT_MODE;
>> +
>> + return devm_iio_device_register(&client->dev, indio_dev);
>> +}
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2023-08-06 13:41 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-08-03 12:56 [PATCH v4 0/2] add MCP4728 I2C DAC driver Andrea Collamati
2023-08-03 12:56 ` [PATCH v4 1/2] dt-bindings: iio: dac: add mcp4728.yaml Andrea Collamati
2023-08-03 15:21 ` Conor Dooley
2023-08-03 18:39 ` Andrea Collamati
2023-08-03 18:46 ` Conor Dooley
2023-08-03 12:56 ` [PATCH v4 2/2] iio: add MCP4728 I2C DAC driver Andrea Collamati
2023-08-05 18:28 ` Jonathan Cameron
2023-08-06 13:40 ` Andrea Collamati
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).