add MCP4728 I2C DAC driver
Subsystems (2)
v3 cover add MCP4728 I2C DAC driver
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 | 48 ++ drivers/iio/dac/Kconfig | 11 + drivers/iio/dac/Makefile | 1 + drivers/iio/dac/mcp4728.c | 638 ++++++++++++++++++ 4 files changed, 698 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/dac/microchip,mcp4728.yaml create mode 100644 drivers/iio/dac/mcp4728.c
Signature · 1 line
2.34.1
PATCH v3 1/2 dt-bindings: iio: dac: add mcp4728.yaml
Add documentation for MCP4728
--- .../bindings/iio/dac/microchip,mcp4728.yaml | 48 +++++++++++++++++++ 1 file changed, 48 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/dac/microchip,mcp4728.yaml
Documentation/devicetree/bindings/iio/dac/microchip,mcp4728.yaml new file +48 -0
@@ -0,0 +1,48 @@ +# 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 + +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 + +maintainers: + - Andrea Collamati <[email protected]> + +properties: + compatible: + enum: + - 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>; + + mcp4728@60 { + compatible = "microchip,mcp4728"; + reg = <0x60>; + vdd-supply = <&vdac_vdd>; + }; + };
Signature · 1 line
2.34.1
PATCH v3 2/2 iio: add MCP4728 I2C DAC driver
In reply to Andrea CollamatiMCP4728 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
--- drivers/iio/dac/Kconfig | 11 + drivers/iio/dac/Makefile | 1 + drivers/iio/dac/mcp4728.c | 638 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 650 insertions(+) create mode 100644 drivers/iio/dac/mcp4728.c
drivers/iio/dac/Kconfig +11 -0
@@ -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
drivers/iio/dac/Makefile +1 -0
@@ -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
drivers/iio/dac/mcp4728.c new file +638 -0
638 lines not on this page · raw message
Signature · 1 line
2.34.1
Re: [PATCH v3 1/2] dt-bindings: iio: dac: add mcp4728.yaml
In reply to Andrea CollamatiHey Andrea,
Quoted 25 lines · Andrea Collamati
Add documentation for MCP4728
Signed-off-by Andrea Collamati <[email protected]>--- .../bindings/iio/dac/microchip,mcp4728.yaml | 48 +++++++++++++++++++ 1 file changed, 48 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/dac/microchip,mcp4728.yamlDocumentation/devicetree/bindings/iio/dac/microchip,mcp4728.yaml new file +11
-0@@ -0,0 +1,48 @@ +# 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 + +description: + MCP4728 is a quad channel, 12-bit voltage output + Digital-to-Analog Converter with non-volatile
Documentation/devicetree/bindings/iio/dac/microchip,mcp4728.yaml line 21
Quoted 10 lines
Documentation/devicetree/bindings/iio/dac/microchip,mcp4728.yaml new file +10
-0+ memory and I2C compatible Serial Interface. + https://www.microchip.com/en-us/product/mcp4728 + +maintainers: + - Andrea Collamati <[email protected]> + +properties: + compatible: + enum: + - microchip,mcp4728
This can just be
compatible:
const: microchip,mcp47288
since you only have a single item in your enum.
Otherwise, this looks good to me.
Despite the email address, I have no knowledge of the hardware in question, I'm just reviewing the binding.
Thanks,
Conor.
Quoted 30 lines
Documentation/devicetree/bindings/iio/dac/microchip,mcp4728.yaml new file +27
-0+ 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>; + + mcp4728@60 { + compatible = "microchip,mcp4728"; + reg = <0x60>; + vdd-supply = <&vdac_vdd>; + }; + };Signature · 1 line
2.34.1
Jonathan Cameron
maintainerRe: [PATCH v3 2/2] iio: add MCP4728 I2C DAC driver
In reply to Andrea CollamatiQuoted 10 lines · On Thu, 20 Jul 2023 17:40:03 +0200 Andrea Collamati…
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 EEPROMSigned-off-by Andrea Collamati <[email protected]>
Hi Andrea,
Given the question was raised about similar parts, I took a quick look and to me this look different enough from parts already supported that a new driver is reasonable.
Biggest thing to address here is the custom ABI.
1) I tend to resist that wherever possible - it will be little used (because it's custom) and rather defeats the point of a unfied subsystem.
2) It's not documented ;) Without docs, very low chance of acceptance or even good review.
Comments inline.
Quoted 27 lines
--- drivers/iio/dac/Kconfig | 11 + drivers/iio/dac/Makefile | 1 + drivers/iio/dac/mcp4728.c | 638 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 650 insertions(+) create mode 100644 drivers/iio/dac/mcp4728.cdrivers/iio/dac/mcp4728.c new file +12
-0@@ -0,0 +1,638 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* +* Support for Microchip MCP4728 +* +* Copyright (C) 2023 Andrea Collamati <[email protected]> +* +* Based on mcp4725 by Peter Meerwald <[email protected]> +* +* 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
drivers/iio/dac/mcp4728.c line 22
Quoted 10 lines
drivers/iio/dac/mcp4728.c new file +10
-0+* hardware) +*/ + +#include <linux/module.h> +#include <linux/i2c.h> +#include <linux/err.h> +#include <linux/delay.h> +#include <linux/regulator/consumer.h> +#include <linux/mod_devicetable.h> +#include <linux/property.h>
Prefer alphabetical order. The groups you have are fine.
drivers/iio/dac/mcp4728.c line 27
Quoted 5 lines
drivers/iio/dac/mcp4728.c new file +5
-0+ +#include <linux/iio/iio.h> +#include <linux/iio/sysfs.h> + +#define MCP4728_DRV_NAME "mcp4728"
Normally better to just put this inline. It's not repeated much.
Quoted 6 lines
drivers/iio/dac/mcp4728.c new file +6
-0+ +#define MCP4728_RESOLUTION 12 +#define MCP4728_N_CHANNELS 4 + +#define MCP4728_CMD_POS 3 +#define MCP4728_CMD_UDAC_POS 0
drivers/iio/dac/mcp4728.c line 43
Quoted 10 lines
drivers/iio/dac/mcp4728.c new file +10
-0+#define MCP4728_CMD_CH_SEL_POS 1 + +#define MCP4728_CMD_VREF_MASK 0x80 +#define MCP4728_CMD_VREF_POS 7 + +#define MCP4728_CMD_PDMODE_MASK 0x60 +#define MCP4728_CMD_PDMODE_POS 5 + +#define MCP4728_CMD_GAIN_MASK 0x10 +#define MCP4728_CMD_GAIN_POS 4
All these should be just the masks then use FIELD_PREP() / FIELD_GET()
drivers/iio/dac/mcp4728.c line 46
Quoted 3 lines
drivers/iio/dac/mcp4728.c new file +3
-0+ +#define MCP4728_MW_CMD 0x08 // Multiwrite Command +#define MCP4728_SW_CMD 0x0A // Sequential Write Command (include eeprom)
/* */ comments only for consistency with rest of the drivers in IIO
Quoted 17 lines
drivers/iio/dac/mcp4728.c new file +15
-0+ +#define MCP4728_READ_RESPONSE_LEN (MCP4728_N_CHANNELS * 3 * 2) +#define MCP4728_WRITE_EEPROM_LEN (1 + MCP4728_N_CHANNELS * 2) + +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;
drivers/iio/dac/mcp4728.c line 114
Quoted 10 lines
drivers/iio/dac/mcp4728.c new file +10
-0+unsigned int i; + + ret =kstrtobool (buf, &state); +if (ret <0 ) +return ret; + +if (!state) +return 0 ; + + outbuf[0 ] =MCP4728_SW_CMD <<MCP4728_CMD_POS ;// Command ID
FIELD_PREP()
drivers/iio/dac/mcp4728.c line 120
Quoted 6 lines
drivers/iio/dac/mcp4728.c new file +6
-0+ +for (i =0 ; i <MCP4728_N_CHANNELS ; i++) { +struct mcp4728_channel_data *ch = &data->channel_data[i]; +int offset =1 + i *2 ; + + outbuf[offset] = ch->ref_mode <<MCP4728_CMD_VREF_POS ;
FIELD_PREP() etc in similar places throughout.
Quoted 42 lines
drivers/iio/dac/mcp4728.c new file +42
-0+if (data->powerdown) { +u8 mcp4728_pd_mode = ch->pd_mode +1 ; + + outbuf[offset] |= mcp4728_pd_mode + <<MCP4728_CMD_PDMODE_POS ; + } + + outbuf[offset] |= ch->g_mode <<MCP4728_CMD_GAIN_POS ; + outbuf[offset] |= ch->dac_value >>8 ; + outbuf[offset +1 ] = ch->dac_value &0xff ; + } + + 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 (inbuf[0 ] &0x80 )// check RDY flag +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[] = {
drivers/iio/dac/mcp4728.c line 172
Quoted 10 lines
drivers/iio/dac/mcp4728.c new file +10
-0+ &iio_dev_attr_store_eeprom.dev_attr.attr, + NULL, +}; + +static const struct attribute_group mcp4728_attribute_group = { + .attrs = mcp4728_attributes, +}; + +enum chip_id { + MCP4728,
Don't introduce any infrastructure for multiple support devices until you add support for a second device. Right now it is just nose.
drivers/iio/dac/mcp4728.c line 183
Quoted 11 lines
drivers/iio/dac/mcp4728.c new file +11
-0+}; + +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->channel_data[channel]; +u8 outbuf[3 ]; +int ret; + + outbuf[0 ] =MCP4728_MW_CMD <<MCP4728_CMD_POS ;// Command ID + outbuf[0 ] |= channel <<MCP4728_CMD_CH_SEL_POS ;// Channel Selector
FIELD_PREP()
drivers/iio/dac/mcp4728.c line 194
Quoted 11 lines
drivers/iio/dac/mcp4728.c new file +11
-0+ outbuf[0 ] |=0 ;// UDAC = 0 + + outbuf[1 ] = ch->ref_mode <<MCP4728_CMD_VREF_POS ; + +if (data->powerdown) { +u8 mcp4728_pd_mode = ch->pd_mode +1 ; + + outbuf[1 ] |= mcp4728_pd_mode <<MCP4728_CMD_PDMODE_POS ; + } + + outbuf[1 ] |= ch->g_mode <<MCP4728_CMD_GAIN_POS ;
FIELD_PREP()
drivers/iio/dac/mcp4728.c line 196
Quoted 2 lines
drivers/iio/dac/mcp4728.c new file +2
-0+ outbuf[1 ] |= ch->dac_value >>8 ; + outbuf[2 ] = ch->dac_value &0xff ;
put_unaligned_be16()
drivers/iio/dac/mcp4728.c line 203
Quoted 7 lines
drivers/iio/dac/mcp4728.c new file +7
-0+ + ret =i2c_master_send (data->client, outbuf,3 ); +if (ret <0 ) +return ret; +else if (ret !=3 ) +return -EIO ; +else
Drop the else - this is the non error condition.
Quoted 49 lines
drivers/iio/dac/mcp4728.c new file +49
-0+return 0 ; +} + +// powerdown mode +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->channel_data[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->channel_data[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;
drivers/iio/dac/mcp4728.c line 262
Quoted 10 lines
drivers/iio/dac/mcp4728.c new file +10
-0+ +if (state) + ret =mcp4728_suspend (&data->client->dev); +else + ret =mcp4728_resume (&data->client->dev); + +if (ret <0 ) +return ret; + +return len;
Don't support custom powering down. If this is needed it should be using the existing power management flows.
Might have been more interesting if it were per channel, but it's not. (and I have no idea off the top of my head on how we would deal with it if it were per channel).
drivers/iio/dac/mcp4728.c line 272
Quoted 10 lines
drivers/iio/dac/mcp4728.c new file +10
-0+} + +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, +}; + +// vref mode
No c++ comments in IIO (other than for the spdx thing)
Quoted 17 lines
drivers/iio/dac/mcp4728.c new file +17
-0+static const char *const mcp4728_vref_modes[] = { +"vdd_ext" , +"internal" , +}; + +static int mcp4728_get_vref_mode (struct iio_dev *indio_dev, +const struct iio_chan_spec *chan) +{ +struct mcp4728_data *data =iio_priv (indio_dev); + +return data->channel_data[chan->channel].ref_mode; +} + +static int mcp4728_set_vref_mode (struct iio_dev *indio_dev, +const struct iio_chan_spec *chan, +unsigned int mode) +{
drivers/iio/dac/mcp4728.c line 299
Quoted 10 lines
drivers/iio/dac/mcp4728.c new file +10
-0+struct mcp4728_data *data =iio_priv (indio_dev); +int ret; + + data->channel_data[chan->channel].ref_mode = mode; + +if (mode ==MCP4728_VREF_EXTERNAL_VDD && + data->channel_data[chan->channel].g_mode ==MCP4728_GAIN_X2 ) { +dev_warn (&data->client->dev, +"CH%d: Gain x2 not effective when vref is vdd, force to x1" , + chan->channel);
Even better if you don't present the option at all and wrap it up in the standard ABI of _scale
drivers/iio/dac/mcp4728.c line 305
Quoted 6 lines
drivers/iio/dac/mcp4728.c new file +6
-0+ data->channel_data[chan->channel].g_mode =MCP4728_GAIN_X1 ; + } + + ret =mcp4728_program_channel_cfg (chan->channel, indio_dev); + +return ret;
return mcp...
drivers/iio/dac/mcp4728.c line 318
Quoted 13 lines
drivers/iio/dac/mcp4728.c new file +13
-0+} + +static const struct iio_enum mcp4728_vref_mode_enum = { + .items = mcp4728_vref_modes, + .num_items =ARRAY_SIZE (mcp4728_vref_modes), + .get = mcp4728_get_vref_mode, + .set = mcp4728_set_vref_mode, +}; + +// gain +static const char *const mcp4728_gain_modes[] = { +"x1" , +"x2" ,
Map this to _SCALE probably as a set of values combined with the reference select.
Also, use numbers for ABI where possible. x2 is a pain for software library to mess around with.
Quoted 46 lines
drivers/iio/dac/mcp4728.c new file +46
-0+}; + +static int mcp4728_get_gain_mode (struct iio_dev *indio_dev, +const struct iio_chan_spec *chan) +{ +struct mcp4728_data *data =iio_priv (indio_dev); + +return data->channel_data[chan->channel].g_mode; +} + +static int mcp4728_set_gain_mode (struct iio_dev *indio_dev, +const struct iio_chan_spec *chan, +unsigned int mode) +{ +struct mcp4728_data *data =iio_priv (indio_dev); +int ret; + +if (mode ==MCP4728_GAIN_X2 && + data->channel_data[chan->channel].ref_mode == +MCP4728_VREF_EXTERNAL_VDD ) { +dev_err (&data->client->dev, +"CH%d: Gain x2 not effective when vref is vdd" , + chan->channel); +return -EINVAL ; + } + + data->channel_data[chan->channel].g_mode = mode; + + ret =mcp4728_program_channel_cfg (chan->channel, indio_dev); + +return ret; +} + +static const struct iio_enum mcp4728_gain_mode_enum = { + .items = mcp4728_gain_modes, + .num_items =ARRAY_SIZE (mcp4728_gain_modes), + .get = mcp4728_get_gain_mode, + .set = mcp4728_set_gain_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 ,
drivers/iio/dac/mcp4728.c line 374
Quoted 10 lines
drivers/iio/dac/mcp4728.c new file +10
-0+ }, +IIO_ENUM ("powerdown_mode" ,IIO_SEPARATE , &mcp4728_powerdown_mode_enum), +IIO_ENUM_AVAILABLE ("powerdown_mode" ,IIO_SHARED_BY_TYPE , + &mcp4728_powerdown_mode_enum), +IIO_ENUM ("vref_mode" ,IIO_SEPARATE , &mcp4728_vref_mode_enum), +IIO_ENUM_AVAILABLE ("vref_mode" ,IIO_SHARED_BY_TYPE , + &mcp4728_vref_mode_enum), +IIO_ENUM ("gain_mode" ,IIO_SEPARATE , &mcp4728_gain_mode_enum), +IIO_ENUM_AVAILABLE ("gain_mode" ,IIO_SHARED_BY_TYPE , + &mcp4728_gain_mode_enum),
Custom ABI is basically ABI no software will use. Also to even start attempting to get that accepted for a driver, we need documentation in Documentation/ABI/testing/sysfs-bus-iio-*
Quoted 30 lines
drivers/iio/dac/mcp4728.c new file +30
-0+ {}, +}; + +static const struct iio_chan_spec mcp4728_channels[MCP4728_N_CHANNELS ] = { +MCP4728_CHAN (0 ), +MCP4728_CHAN (1 ), +MCP4728_CHAN (2 ), +MCP4728_CHAN (3 ), +}; + +static int mcp4728_full_scale_mV (u32 *full_scale_mV,int channel, +struct mcp4728_data *data) +{ +int ret; + +if (data->channel_data[channel].ref_mode ==MCP4728_VREF_EXTERNAL_VDD ) + ret =regulator_get_voltage (data->vdd_reg); +else + ret =2048000 ; + +if (ret <0 ) +return ret; + +if (ret ==0 ) +return -EINVAL ; + + *full_scale_mV = ret /1000 ; +return 0 ; +} +
drivers/iio/dac/mcp4728.c line 414
Quoted 10 lines
drivers/iio/dac/mcp4728.c new file +10
-0+static u32 mcp4728_raw_to_mV (u32 raw,int channel,struct mcp4728_data *data) +{ +int ret; +u32 full_scale_mV; + + ret =mcp4728_full_scale_mV (&full_scale_mV, channel, data); +if (ret) +return ret; + +return (((raw +1 ) * full_scale_mV) >>MCP4728_RESOLUTION );
This looks like a linear transformation. If so that's a job for userspace and you should expose the raw channel values + the scaling.
Quoted 69 lines
drivers/iio/dac/mcp4728.c new file +69
-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); +int ret; + +switch (mask) { +case IIO_CHAN_INFO_RAW : + *val = data->channel_data[chan->channel].dac_value; +return IIO_VAL_INT ; +case IIO_CHAN_INFO_SCALE : +if (data->channel_data[chan->channel].ref_mode == +MCP4728_VREF_EXTERNAL_VDD ) + ret =regulator_get_voltage (data->vdd_reg); +else + ret =2048000 ; + +if (ret <0 ) +return ret; + + *val = ret /1000 ; + *val2 =MCP4728_RESOLUTION ; +return IIO_VAL_FRACTIONAL_LOG2 ; + } +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->channel_data[chan->channel].dac_value = val; + ret =mcp4728_program_channel_cfg (chan->channel, indio_dev); +break ; +default : + ret = -EINVAL ; +break ; + } + +return ret; +} + +static const struct iio_info mcp4728_info = { + .read_raw = mcp4728_read_raw, + .write_raw = mcp4728_write_raw, + .attrs = &mcp4728_attribute_group, +}; + +static int mcp4728_suspend (struct device *dev) +{ +struct iio_dev *indio_dev =i2c_get_clientdata (to_i2c_client (dev)); +struct mcp4728_data *data =iio_priv (indio_dev); +int err =0 ; +unsigned int i; + + data->powerdown = true; + +for (i =0 ; i <MCP4728_N_CHANNELS ; i++) { +int ret =mcp4728_program_channel_cfg (i, indio_dev);
drivers/iio/dac/mcp4728.c line 493
Quoted 10 lines
drivers/iio/dac/mcp4728.c new file +10
-0+ +if (ret) + err = ret; + } +return err; +} + +static int mcp4728_resume (struct device *dev) +{ +struct iio_dev *indio_dev =i2c_get_clientdata (to_i2c_client (dev));
I don't feel strongly about this, but chances are this will get simplified to not bounce from dev to i2c client and back again. dev_get_drvdata(dev)
drivers/iio/dac/mcp4728.c line 504
Quoted 11 lines
drivers/iio/dac/mcp4728.c new file +11
-0+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;
Generally if an error occured, stop poking the device. So error out immediately. I can see the logic in trying to continue, but an error on a device like this normally indicates something is going very wrong so muddling on won't succeed.
Quoted 16 lines
drivers/iio/dac/mcp4728.c new file +16
-0+ } +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,
drivers/iio/dac/mcp4728.c line 530
Quoted 10 lines
drivers/iio/dac/mcp4728.c new file +10
-0+ "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++) {
Unusual to read back existing values from the device rather than resetting it into a clean state. What is your reasoning?
drivers/iio/dac/mcp4728.c line 539
Quoted 9 lines
drivers/iio/dac/mcp4728.c new file +9
-0+struct mcp4728_channel_data *ch = &data->channel_data[i]; +u8 r2 = inbuf[i *6 +1 ]; +u8 r3 = inbuf[i *6 +2 ]; +u32 dac_mv; + + ch->dac_value = (r2 &0x0F ) <<8 | r3; + ch->ref_mode = (r2 &MCP4728_CMD_VREF_MASK ) >>MCP4728_CMD_VREF_POS ; + ch->pd_mode = (r2 &MCP4728_CMD_PDMODE_MASK ) >>MCP4728_CMD_PDMODE_POS ; + ch->g_mode = (r2 &MCP4728_CMD_GAIN_MASK ) >>MCP4728_CMD_GAIN_POS ;
Use FIELD_GET()
drivers/iio/dac/mcp4728.c line 549
Quoted 10 lines
drivers/iio/dac/mcp4728.c new file +10
-0+ +if (ch->g_mode ==MCP4728_GAIN_X2 && ch->ref_mode ==MCP4728_VREF_EXTERNAL_VDD ) +dev_warn (&data->client->dev, +"CH%d: Gain x2 not effective when vref is vdd" , + i); + + dac_mv =mcp4728_raw_to_mV (ch->dac_value, i, data); +dev_info (&data->client->dev, +"CH%d: Voltage=%dmV VRef=%d PowerDown=%d Gain=%d\n" , i, + dac_mv, ch->ref_mode, ch->pd_mode, ch->g_mode);
That's noise that you should be able to get by directly reading the relevant attributes or is detail that userspace should not need to know about.
You can rely on dynamic debug though and make that dev_dbg() so it is available if needed.
Quoted 17 lines
drivers/iio/dac/mcp4728.c new file +17
-0+ } + +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);
drivers/iio/dac/mcp4728.c line 576
Quoted 10 lines
drivers/iio/dac/mcp4728.c new file +10
-0+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) + goto err_disable_vdd_reg;
As mentioned below - a devm_add_action_or_reset() here to deal with the regulator would be good. Also, regulator_enable() is side effect free on error so you should not be disabling in this error path.
Quoted 23 lines
drivers/iio/dac/mcp4728.c new file +23
-0+ + err =mcp4728_init_channels_data (data); +if (err) { +dev_err (&client->dev, +"failed to read mcp4728 current configuration\n" ); + goto err_disable_vdd_reg; + } + + 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 ; + + err =iio_device_register (indio_dev); +if (err) + goto err_disable_vdd_reg; + +return 0 ; + +err_disable_vdd_reg: +regulator_disable (data->vdd_reg); +
drivers/iio/dac/mcp4728.c line 609
Quoted 10 lines
drivers/iio/dac/mcp4728.c new file +10
-0+return err; +} + +static void mcp4728_remove (struct i2c_client *client) +{ +struct iio_dev *indio_dev =i2c_get_clientdata (client); +struct mcp4728_data *data =iio_priv (indio_dev); + +iio_device_unregister (indio_dev); +regulator_disable (data->vdd_reg);
Use devm_add_action_or_reset() + a suitable callback to turn of the regulator, then you can use devm for the device register as well and no need for a remove function or error handlers in probe()
Quoted 29 lines
drivers/iio/dac/mcp4728.c new file +29
-0+} + +static const struct i2c_device_id mcp4728_id[] = { + {"mcp4728" ,MCP4728 }, + {} +}; +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_DRV_NAME , + .of_match_table = mcp4728_of_match, + .pm =pm_sleep_ptr (&mcp4728_pm_ops), + }, + .probe = mcp4728_probe, + .remove = mcp4728_remove, + .id_table = mcp4728_id, +}; +module_i2c_driver (mcp4728_driver); + +MODULE_AUTHOR ("Andrea Collamati <[email protected]>" ); +MODULE_DESCRIPTION ("MCP4728 12-bit DAC" ); +MODULE_LICENSE ("GPL" );
Re: [PATCH v3 1/2] dt-bindings: iio: dac: add mcp4728.yaml
In reply to Andrea CollamatiQuoted 25 lines · Andrea Collamati
Add documentation for MCP4728
Signed-off-by Andrea Collamati <[email protected]>--- .../bindings/iio/dac/microchip,mcp4728.yaml | 48 +++++++++++++++++++ 1 file changed, 48 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/dac/microchip,mcp4728.yamlDocumentation/devicetree/bindings/iio/dac/microchip,mcp4728.yaml new file +11
-0@@ -0,0 +1,48 @@ +# 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 + +description: + MCP4728 is a quad channel, 12-bit voltage output + Digital-to-Analog Converter with non-volatile
Documentation/devicetree/bindings/iio/dac/microchip,mcp4728.yaml line 21
Quoted 10 lines
Documentation/devicetree/bindings/iio/dac/microchip,mcp4728.yaml new file +10
-0+ memory and I2C compatible Serial Interface. + https://www.microchip.com/en-us/product/mcp4728 + +maintainers: + - Andrea Collamati <[email protected]> + +properties: + compatible: + enum: + - microchip,mcp4728
This is a friendly reminder during the review process.
It seems my previous comments were not fully addressed. Maybe my feedback got lost between the quotes, maybe you just forgot to apply it. Please go back to the previous discussion and either implement all requested changes or keep discussing them.
Thank you.
Quoted 12 lines
Documentation/devicetree/bindings/iio/dac/microchip,mcp4728.yaml new file +12
-0+ 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
Documentation/devicetree/bindings/iio/dac/microchip,mcp4728.yaml line 43
Quoted 10 lines
Documentation/devicetree/bindings/iio/dac/microchip,mcp4728.yaml new file +10
-0+ +additionalProperties: false + +examples: + - | + i2c { + #address-cells = <1>; + #size-cells = <0>; + + mcp4728@60 {
The same... Probably more comments were ignored, so:
This is a friendly reminder during the review process.
It seems my previous comments were not fully addressed. Maybe my feedback got lost between the quotes, maybe you just forgot to apply it. Please go back to the previous discussion and either implement all requested changes or keep discussing them.
Thank you.
Best regards,
Krzysztof
Re: [PATCH v3 1/2] dt-bindings: iio: dac: add mcp4728.yaml
Quoted 7 lines · Krzysztof Kozlowski
Documentation/devicetree/bindings/iio/dac/microchip,mcp4728.yaml line 43
Quoted 6 lines
Documentation/devicetree/bindings/iio/dac/microchip,mcp4728.yaml new file +6
-0+ - | + i2c { + #address-cells = <1>; + #size-cells = <0>; + + mcp4728@60 {
Quoted 8 lines
The same... Probably more comments were ignored, so:
This is a friendly reminder during the review process.
It seems my previous comments were not fully addressed. Maybe my feedback got lost between the quotes, maybe you just forgot to apply it. Please go back to the previous discussion and either implement all requested changes or keep discussing them.
Damn, this is my third comment about the same. Here was second:
https://lore.kernel.org/all/[email protected]/
so you nicely ignore feedback. NAK.
Best regards,
Krzysztof
Re: [PATCH v3 1/2] dt-bindings: iio: dac: add mcp4728.yaml
In reply to Krzysztof KozlowskiHi Krzysztof,
Quoted 28 lines · Krzysztof Kozlowski
Quoted 28 lines
Add documentation for MCP4728
Signed-off-by Andrea Collamati <[email protected]>--- .../bindings/iio/dac/microchip,mcp4728.yaml | 48 +++++++++++++++++++ 1 file changed, 48 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/dac/microchip,mcp4728.yamlDocumentation/devicetree/bindings/iio/dac/microchip,mcp4728.yaml new file +14
-0@@ -0,0 +1,48 @@ +# 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 + +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 +
Quoted 8 lines
Documentation/devicetree/bindings/iio/dac/microchip,mcp4728.yaml line 21
Quoted 7 lines
Documentation/devicetree/bindings/iio/dac/microchip,mcp4728.yaml new file +7
-0+maintainers: + - Andrea Collamati <[email protected]> + +properties: + compatible: + enum: + - microchip,mcp4728This is a friendly reminder during the review process.
Sorry but I didn't understand all your requests:
- I changed in the title mcp4728 with MCP4728
- I added description
but I don't know which blank line or whitespaces should be removed.
Can you tell me please?
Re: [PATCH v3 1/2] dt-bindings: iio: dac: add mcp4728.yaml
In reply to Krzysztof KozlowskiQuoted 7 lines · Krzysztof Kozlowski
Quoted 6 lines · Krzysztof Kozlowski
Quoted 6 lines
Documentation/devicetree/bindings/iio/dac/microchip,mcp4728.yaml new file +6
-0+ - | + i2c { + #address-cells = <1>; + #size-cells = <0>; + + mcp4728@60 {
Quoted 8 lines
Quoted 8 lines
The same... Probably more comments were ignored, so:
This is a friendly reminder during the review process.
It seems my previous comments were not fully addressed. Maybe my feedback got lost between the quotes, maybe you just forgot to apply it. Please go back to the previous discussion and either implement all requested changes or keep discussing them.
Sorry, you are right. I missed to change the node name.
{
#address-cells = <1>;
#size-cells = <0>;
dac@60 {could be ok?
Thank you
Andrea
Re: [PATCH v3 1/2] dt-bindings: iio: dac: add mcp4728.yaml
In reply to Andrea CollamatiQuoted 42 lines · Andrea Collamati
Hi Krzysztof,
Quoted 28 lines · Krzysztof Kozlowski
Quoted 28 lines
Add documentation for MCP4728
Signed-off-by Andrea Collamati <[email protected]>--- .../bindings/iio/dac/microchip,mcp4728.yaml | 48 +++++++++++++++++++ 1 file changed, 48 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/dac/microchip,mcp4728.yamlDocumentation/devicetree/bindings/iio/dac/microchip,mcp4728.yaml new file +14
-0@@ -0,0 +1,48 @@ +# 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 + +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 +Quoted 8 lines
Documentation/devicetree/bindings/iio/dac/microchip,mcp4728.yaml line 21
Quoted 7 lines
Documentation/devicetree/bindings/iio/dac/microchip,mcp4728.yaml new file +7
-0+maintainers: + - Andrea Collamati <[email protected]> + +properties: + compatible: + enum: + - microchip,mcp4728This is a friendly reminder during the review process.
Sorry but I didn't understand all your requests:
Quoted 7 lines
- I changed in the title mcp4728 with MCP4728
- I added description
but I don't know which blank line or whitespaces should be removed.
Can you tell me please?
You forgot to add blank line. Open example-schema and compare.
Also, you had white-space errors. Editors should show it to you. Git maybe as well.
Best regards,
Krzysztof
Re: [PATCH v3 2/2] iio: add MCP4728 I2C DAC driver
In reply to Jonathan CameronHi Jonathan,
thank you for your first review.
See below.
Quoted 12 lines · Jonathan Cameron
drivers/iio/dac/mcp4728.c line 194
Quoted 10 lines
drivers/iio/dac/mcp4728.c new file +10
-0+ + outbuf[1 ] = ch->ref_mode <<MCP4728_CMD_VREF_POS ; + +if (data->powerdown) { +u8 mcp4728_pd_mode = ch->pd_mode +1 ; + + outbuf[1 ] |= mcp4728_pd_mode <<MCP4728_CMD_PDMODE_POS ; + } + + outbuf[1 ] |= ch->g_mode <<MCP4728_CMD_GAIN_POS ;
FIELD_PREP()
Quoted 4 lines
drivers/iio/dac/mcp4728.c line 196
Quoted 2 lines
drivers/iio/dac/mcp4728.c new file +2
-0+ outbuf[1 ] |= ch->dac_value >>8 ; + outbuf[2 ] = ch->dac_value &0xff ;
put_unaligned_be16()
outbuf[1] contains other data (gain mode, powerdown mode, etc) in addition to dac value. Using put_unaligned_be16 will probably reset them.
Something like this could be the solution?
#defineMCP4728_DAC_H_FIELD GENMASK(3, 0)
#defineMCP4728_DAC_L_FIELD GENMASK(7, 0)
...
outbuf[1] |= FIELD_PREP(MCP4728_DAC_H_FIELD, ch->dac_value>> 8);
outbuf[2] = FIELD_PREP(MCP4728_DAC_L_FIELD, ch->dac_value);
Quoted 53 lines
Quoted 53 lines
drivers/iio/dac/mcp4728.c new file +53
-0+return 0 ; +} + +// powerdown mode +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->channel_data[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->channel_data[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
Quoted 8 lines
drivers/iio/dac/mcp4728.c line 262
Quoted 6 lines
drivers/iio/dac/mcp4728.c new file +6
-0+ ret =mcp4728_resume (&data->client->dev); + +if (ret <0 ) +return ret; + +return len;Don't support custom powering down. If this is needed it should be using the existing power management flows.
Could you explain better? I have extended customized powering down because I took as reference the driver mcp4725.c and I extended to multichannel.
How should I change it?
Quoted 3 lines
Might have been more interesting if it were per channel, but it's not. (and I have no idea off the top of my head on how we would deal with it if it were per channel).
MCP4728 can handle power down per channel...
There are two bits per each channel the could be
00 => No Power Down
01 => 1kohm_to_gnd
10 => 100kohm_to_gnd
11 => 500kohm_to_gnd
Quoted 16 lines
Quoted 16 lines
drivers/iio/dac/mcp4728.c new file +15
-0+ 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,
Quoted 8 lines
drivers/iio/dac/mcp4728.c line 530
Quoted 6 lines
drivers/iio/dac/mcp4728.c new file +6
-0+"failed to read mcp4728 conf. Wrong Response Len ret=%d\n" , + ret); +return -EIO ; + } + +for (i =0 ; i <MCP4728_N_CHANNELS ; i++) {Unusual to read back existing values from the device rather than resetting it into a clean state. What is your reasoning?
MCP4728 has an EEPROM memory.
Under the reset conditions, the device uploads the EEPROM data into both of the DAC input and output registers simultaneously.
My reasoning was that the driver syncs with device at probe time and let the user change each channel parameters via iio_chan_spec_ext_info.
Thank you again.
Andrea
Re: [PATCH v3 1/2] dt-bindings: iio: dac: add mcp4728.yaml
In reply to Conor DooleyHi Conor,
Quoted 31 lines · Conor Dooley
Quoted 31 lines
Add documentation for MCP4728
Signed-off-by Andrea Collamati <[email protected]>--- .../bindings/iio/dac/microchip,mcp4728.yaml | 48 +++++++++++++++++++ 1 file changed, 48 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/dac/microchip,mcp4728.yamlDocumentation/devicetree/bindings/iio/dac/microchip,mcp4728.yaml new file +17
-0@@ -0,0 +1,48 @@ +# 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 + +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 + +maintainers: + - Andrea Collamati <[email protected]> +
Quoted 8 lines
Documentation/devicetree/bindings/iio/dac/microchip,mcp4728.yaml line 21
Quoted 4 lines
Documentation/devicetree/bindings/iio/dac/microchip,mcp4728.yaml new file +4
-0+properties: + compatible: + enum: + - microchip,mcp4728This can just be
compatible:
const: microchip,mcp47288
since you only have a single item in your enum.
I will in include in v4.
Thank you.
Andrea
Re: [PATCH v3 2/2] iio: add MCP4728 I2C DAC driver
In reply to Jonathan CameronHi Jonathan,
Quoted 21 lines · Jonathan Cameron
Quoted 21 lines
drivers/iio/dac/mcp4728.c new file +21
-0+static const char *const mcp4728_vref_modes[] = { +"vdd_ext" , +"internal" , +}; + +static int mcp4728_get_vref_mode (struct iio_dev *indio_dev, +const struct iio_chan_spec *chan) +{ +struct mcp4728_data *data =iio_priv (indio_dev); + +return data->channel_data[chan->channel].ref_mode; +} + +static int mcp4728_set_vref_mode (struct iio_dev *indio_dev, +const struct iio_chan_spec *chan, +unsigned int mode) +{ +struct mcp4728_data *data =iio_priv (indio_dev); +int ret; + + data->channel_data[chan->channel].ref_mode = mode;
Quoted 9 lines
drivers/iio/dac/mcp4728.c line 299
Quoted 6 lines
drivers/iio/dac/mcp4728.c new file +6
-0+ +if (mode ==MCP4728_VREF_EXTERNAL_VDD && + data->channel_data[chan->channel].g_mode ==MCP4728_GAIN_X2 ) { +dev_warn (&data->client->dev, +"CH%d: Gain x2 not effective when vref is vdd, force to x1" , + chan->channel);Even better if you don't present the option at all and wrap it up in the standard ABI of
_scale
I think that the solution could be:
- Removing custom ABI (vref/gain)
- Initialize them at device tree level using two 4-elements arrays.
- Finally using the same approach of https://github.com/torvalds/linux/blob/c2782531397f5cb19ca3f8f9c17727f1cdf5bee8/drivers/iio/dac/mcp4725.c#L462 where after having synced current parameters stored in EEPROM they are updated with the ones specified in dts.
Best regards
Andrea
Jonathan Cameron
maintainerRe: [PATCH v3 2/2] iio: add MCP4728 I2C DAC driver
In reply to Andrea CollamatiQuoted 20 lines · On Fri, 21 Jul 2023 21:47:37 +0200 Andrea Collamati…
Hi Jonathan,
thank you for your first review.
See below.
Quoted 12 lines · Jonathan Cameron
drivers/iio/dac/mcp4728.c line 194
Quoted 10 lines
drivers/iio/dac/mcp4728.c new file +10
-0+ + outbuf[1 ] = ch->ref_mode <<MCP4728_CMD_VREF_POS ; + +if (data->powerdown) { +u8 mcp4728_pd_mode = ch->pd_mode +1 ; + + outbuf[1 ] |= mcp4728_pd_mode <<MCP4728_CMD_PDMODE_POS ; + } + + outbuf[1 ] |= ch->g_mode <<MCP4728_CMD_GAIN_POS ;
FIELD_PREP()
Quoted 5 lines
Quoted 4 lines
drivers/iio/dac/mcp4728.c line 196
Quoted 2 lines
drivers/iio/dac/mcp4728.c new file +2
-0+ outbuf[1 ] |= ch->dac_value >>8 ; + outbuf[2 ] = ch->dac_value &0xff ;
put_unaligned_be16()outbuf[1] contains other data (gain mode, powerdown mode, etc) in addition to dac value. Using
put_unaligned_be16will probably reset them.
Ah. I'd missed the |= because you then didn't mask the dac_value.
Quoted 6 lines
Something like this could be the solution?
#defineMCP4728_DAC_H_FIELD
GENMASK(3, 0)#defineMCP4728_DAC_L_FIELD
GENMASK(7, 0)
Call them _MASK or _MSK rather than field.
I think that is much more common naming.
Quoted 6 lines
...
outbuf[1] |=
FIELD_PREP(MCP4728_DAC_H_FIELD, ch->dac_value>> 8);outbuf[2] =
FIELD_PREP(MCP4728_DAC_L_FIELD, ch->dac_value);
That's better.
Quoted 64 lines
Quoted 55 lines
Quoted 55 lines
drivers/iio/dac/mcp4728.c new file +53
-0+return 0 ; +} + +// powerdown mode +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->channel_data[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->channel_data[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 Quoted 8 lines
drivers/iio/dac/mcp4728.c line 262
Quoted 6 lines
drivers/iio/dac/mcp4728.c new file +6
-0+ ret =mcp4728_resume (&data->client->dev); + +if (ret <0 ) +return ret; + +return len;Don't support custom powering down. If this is needed it should be using the existing power management flows.
Quoted 3 lines
Could you explain better? I have extended customized powering down because I took as reference the driver
mcp4725.cand I extended to multichannel.How should I change it?
Ok. I'd missed that we had an existing driver doing this and indeed we have documented it. Fair enough - I must have been persuaded in the past and then forgotten about it :)
Quoted 9 lines
Quoted 4 lines
Might have been more interesting if it were per channel, but it's not. (and I have no idea off the top of my head on how we would deal with it if it were per channel).
MCP4728 can handle power down per channel...
There are two bits per each channel the could be
Quoted 8 lines
00 => No Power Down
01 => 1kohm_to_gnd
10 => 100kohm_to_gnd
11 => 500kohm_to_gnd
Ok. Good to support that rather than a full power down.
Quoted 25 lines
Quoted 16 lines
Quoted 16 lines
drivers/iio/dac/mcp4728.c new file +15
-0+ 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,Quoted 8 lines
drivers/iio/dac/mcp4728.c line 530
Quoted 6 lines
drivers/iio/dac/mcp4728.c new file +6
-0+"failed to read mcp4728 conf. Wrong Response Len ret=%d\n" , + ret); +return -EIO ; + } + +for (i =0 ; i <MCP4728_N_CHANNELS ; i++) {Unusual to read back existing values from the device rather than resetting it into a clean state. What is your reasoning?
Quoted 7 lines
MCP4728 has an EEPROM memory.
Under the reset conditions, the device uploads the EEPROM data into both of the DAC input and output registers simultaneously.
My reasoning was that the driver syncs with device at probe time and let the user change each channel parameters via
iio_chan_spec_ext_info.
Ok - this is fine if it's reading back from EEPROM.
Please add a comment on that though because as demonstrated above I'm forgetful and may wonder why it was done like this in the future.
Jonathan
Quoted 7 lines
Thank you again.
Andrea
Jonathan Cameron
maintainerRe: [PATCH v3 2/2] iio: add MCP4728 I2C DAC driver
In reply to Andrea CollamatiQuoted 33 lines · On Sun, 23 Jul 2023 07:09:11 +0200 Andrea Collamati…
Hi Jonathan,
Quoted 21 lines · Jonathan Cameron
Quoted 21 lines
drivers/iio/dac/mcp4728.c new file +21
-0+static const char *const mcp4728_vref_modes[] = { +"vdd_ext" , +"internal" , +}; + +static int mcp4728_get_vref_mode (struct iio_dev *indio_dev, +const struct iio_chan_spec *chan) +{ +struct mcp4728_data *data =iio_priv (indio_dev); + +return data->channel_data[chan->channel].ref_mode; +} + +static int mcp4728_set_vref_mode (struct iio_dev *indio_dev, +const struct iio_chan_spec *chan, +unsigned int mode) +{ +struct mcp4728_data *data =iio_priv (indio_dev); +int ret; + + data->channel_data[chan->channel].ref_mode = mode;Quoted 9 lines
drivers/iio/dac/mcp4728.c line 299
Quoted 6 lines
drivers/iio/dac/mcp4728.c new file +6
-0+ +if (mode ==MCP4728_VREF_EXTERNAL_VDD && + data->channel_data[chan->channel].g_mode ==MCP4728_GAIN_X2 ) { +dev_warn (&data->client->dev, +"CH%d: Gain x2 not effective when vref is vdd, force to x1" , + chan->channel);Even better if you don't present the option at all and wrap it up in the standard ABI of
_scale
Quoted 5 lines
I think that the solution could be:
- Removing custom ABI (vref/gain)
- Initialize them at device tree level using two 4-elements arrays.
If doing with device tree, they should reflect something that is a characteristic of how the chips is wired up. So you would need to explain why that is the case here.
However, I'm still not understanding why _SCALE is not appropriate here. We have a small set of options with well defined scales.
Quoted 3 lines
- Finally using the same approach of https://github.com/torvalds/linux/blob/c2782531397f5cb19ca3f8f9c17727f1cdf5bee8/drivers/iio/dac/mcp4725.c#L462 where after having synced current parameters stored in EEPROM they are updated with the ones specified in dts.
From a quick look those are not coming from DTS and are not in the dt-binding for that device. The parameters only come from legacy platform data (or the wild west of early IIO).
Only the presence of vref and whether to buffer it are in DT binding
https://elixir.bootlin.com/linux/latest/source/drivers/iio/dac/mcp4725.c#L373
We should probably rip the remaining platform data based probe paths out of drivers sometime soon. No one uses them any more (there are better paths even if people are using non DT based board files) and they can mislead because the rules weren't as tight as they are for what goes in a DT binding.
Jonathan
Quoted 6 lines
Best regards
Andrea
Re: [PATCH v3 2/2] iio: add MCP4728 I2C DAC driver
Quoted 11 lines · Jonathan Cameron
Quoted 11 lines
Quoted 9 lines
drivers/iio/dac/mcp4728.c line 299
Quoted 6 lines
drivers/iio/dac/mcp4728.c new file +6
-0+ +if (mode ==MCP4728_VREF_EXTERNAL_VDD && + data->channel_data[chan->channel].g_mode ==MCP4728_GAIN_X2 ) { +dev_warn (&data->client->dev, +"CH%d: Gain x2 not effective when vref is vdd, force to x1" , + chan->channel);Even better if you don't present the option at all and wrap it up in the standard ABI of
_scaleI think that the solution could be:
Quoted 8 lines
Quoted 3 lines
- Removing custom ABI (vref/gain)
- Initialize them at device tree level using two 4-elements arrays.
If doing with device tree, they should reflect something that is a characteristic of how the chips is wired up. So you would need to explain why that is the case here.
However, I'm still not understanding why
_SCALEis not appropriate here. We have a small set of options with well defined scales.
SCALE is appropriate. I didn't know that scale_available was a standard ABI.
I will follow the implementation done n https://github.com/torvalds/linux/blob/c2782531397f5cb19ca3f8f9c17727f1cdf5bee8/drivers/iio/dac/ad5592r-base.c#L487
Jonathan Cameron
maintainerRe: [PATCH v3 2/2] iio: add MCP4728 I2C DAC driver
Quoted 18 lines · On Sun, 23 Jul 2023 14:15:10 +0200 Andrea Collamati…
Quoted 17 lines · Jonathan Cameron
Quoted 9 lines
Quoted 9 lines
drivers/iio/dac/mcp4728.c line 299
Quoted 6 lines
drivers/iio/dac/mcp4728.c new file +6
-0+ +if (mode ==MCP4728_VREF_EXTERNAL_VDD && + data->channel_data[chan->channel].g_mode ==MCP4728_GAIN_X2 ) { +dev_warn (&data->client->dev, +"CH%d: Gain x2 not effective when vref is vdd, force to x1" , + chan->channel);Even better if you don't present the option at all and wrap it up in the standard ABI of
_scaleQuoted 5 lines
I think that the solution could be:
- Removing custom ABI (vref/gain)
- Initialize them at device tree level using two 4-elements arrays.
If doing with device tree, they should reflect something that is a characteristic of how the chips is wired up. So you would need to explain why that is the case here.
Quoted 6 lines
Quoted 2 lines
However, I'm still not understanding why
_SCALEis not appropriate here. We have a small set of options with well defined scales.SCALE is appropriate. I didn't know that
scale_availablewas a standard ABI.I will follow the implementation done n https://github.com/torvalds/linux/blob/c2782531397f5cb19ca3f8f9c17727f1cdf5bee8/drivers/iio/dac/ad5592r-base.c#L487
That's long out of date wrt to best practice. Instead follow. drivers/iio/dac/ad7293.c or any of the other drivers that set .info_mask_shared_by_*_available = BIT(IIO_CHAN_INFO_SCALE) and provide the read_avail() callback.
The advantage that brings is that consumer drivers can then access this information much more simply.
One day we will hopefully finish converting old drivers over to newer interfaces, but it's slow progress!
Jonathan