| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <aQnUk4Inip8QQz6u@smile.fi.intel.com>
Date: Tue, 4 Nov 2025 12:25:23 +0200
From: Andy Shevchenko <andriy.shevchenko@...el.com>
To: Ariana Lazar <ariana.lazar@...rochip.com>
Cc: Jonathan Cameron <jic23@...nel.org>,
David Lechner <dlechner@...libre.com>,
Nuno Sá <nuno.sa@...log.com>,
Andy Shevchenko <andy@...nel.org>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>, linux-iio@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/2] iio: dac: adding support for Microchip MCP47FEB02
On Mon, Nov 03, 2025 at 05:50:30PM +0200, Ariana Lazar wrote:
> This is the iio driver for Microchip MCP47F(E/V)B(0/1/2)1,
> MCP47F(E/V)B(0/1/2)2, MCP47F(E/V)B(0/1/2)4 and MCP47F(E/V)B(0/1/2)8 series
> of buffered voltage output Digital-to-Analog Converters with nonvolatile or
> volatile memory and an I2C Interface.
>
> The families support up to 8 output channels.
>
> The devices can be 8-bit, 10-bit and 12-bit.
...
> +config MCP47FEB02
> + tristate "MCP47F(E/V)B|(0/1/2)(1/2/4/8)DAC driver"
This is unreadable cryptic title. Make it more human-readable, like:
"Microchip MCP47F family of DAC driver"
> + depends on I2C
> + help
> + Say yes here if you want to build a driver for the Microchip
Too many spaces, see how it's done elsewhere.
> + MCP47FEB01, MCP47FEB11, MCP47FEB21, MCP47FEB02, MCP47FEB12,
> + MCP47FEB22, MCP47FVB01, MCP47FVB11, MCP47FVB21, MCP47FVB02,
> + MCP47FVB12, MCP47FVB02, MCP47FVB12, MCP47FVB22, MCP47FVB04,
> + MCP47FVB14, MCP47FVB24, MCP47FVB04, MCP47FVB08, MCP47FVB18,
> + MCP47FVB28, MCP47FEB04, MCP47FEB14 and MCP47FEB24 having up to 8
This is also unreadable, please split to groups (by family species and/or bits)
and sort each group accordingly, like
- E-group (8-bit): MCP47FEB01, MCP47FEB11, MCP47FEB21
- E-group (10-bit): MCP47FEB02, MCP47FEB12, MCP47FEB22
...
Note, I put a hypothetical text there, I haven't check this for the correctness!
> + channels, 8-bit, 10-bit or 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 mcp47feb02.
...
> + * Datasheet for MCP47FEBXX can be found here:
> + * https://ww1.microchip.com/downloads/aemDocuments/documents/OTH/ProductDocuments/DataSheets/20005375A.pdf
> + *
> + * Datasheet for MCP47FVBXX can be found here:
> + * https://ww1.microchip.com/downloads/aemDocuments/documents/OTH/ProductDocuments/DataSheets/20005405A.pdf
> + *
> + * Datasheet for MCP47FXBX4/8 can be found here:
> + * https://ww1.microchip.com/downloads/aemDocuments/documents/MSLD/ProductDocuments/DataSheets/MCP47FXBX48-Data-Sheet-DS200006368A.pdf
Avoid duplicating information, so far it can be just listed as
* Datasheet links:
*
* [MCP47FEBxx] https://ww1.microchip.com/downloads/aemDocuments/documents/OTH/ProductDocuments/DataSheets/20005375A.pdf
* [MCP47FVBxx] https://ww1.microchip.com/downloads/aemDocuments/documents/OTH/ProductDocuments/DataSheets/20005405A.pdf
* [MCP47FxBx4/8] https://ww1.microchip.com/downloads/aemDocuments/documents/MSLD/ProductDocuments/DataSheets/MCP47FXBX48-Data-Sheet-DS200006368A.pdf
(also note xx instead of XX).
Or propose a better style.
...
> +#include <linux/bits.h>
> +#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/mutex.h>
> +#include <linux/property.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
Missing includes. E.g., for 'bool', for 'ARRAY_SIZE()', for 'guard()()'.
Follow IWYU (Include What You Use) principle.
...
> +#define MCP47FEB02_DAC0_REG_ADDR (0x00 << 3)
I assume it's the similar case as for below 0x10-0x17 range. Perhaps do both as
a macro with a parameter?
> +#define MCP47FEB02_VREF_REG_ADDR (0x08 << 3)
> +#define MCP47FEB02_POWER_DOWN_REG_ADDR (0x09 << 3)
> +#define MCP47FEB02_GAIN_BIT_STATUS_REG_ADDR (0x0A << 3)
> +#define MCP47FEB02_WIPERLOCK_STATUS_REG_ADDR (0x0B << 3)
> +
> +#define MCP47FEB02_NV_DAC0_REG_ADDR (0x10 << 3)
> +#define MCP47FEB02_NV_DAC1_REG_ADDR (0x11 << 3)
> +#define MCP47FEB02_NV_DAC2_REG_ADDR (0x12 << 3)
> +#define MCP47FEB02_NV_DAC3_REG_ADDR (0x13 << 3)
> +#define MCP47FEB02_NV_DAC4_REG_ADDR (0x14 << 3)
> +#define MCP47FEB02_NV_DAC5_REG_ADDR (0x15 << 3)
> +#define MCP47FEB02_NV_DAC6_REG_ADDR (0x16 << 3)
> +#define MCP47FEB02_NV_DAC7_REG_ADDR (0x17 << 3)
> +#define MCP47FEB02_NV_VREF_REG_ADDR (0x18 << 3)
> +#define MCP47FEB02_NV_POWER_DOWN_REG_ADDR (0x19 << 3)
> +#define MCP47FEB02_NV_GAIN_BIT_I2C_SLAVE_REG_ADDR (0x1A << 3)
Drop this << 3 part, just do it at run-time. Or embed them, by providing
shifted values. Also, there is special formats for regmap, perhaps that's what
you wanted to begin with? *Yes, it might need some code to be added into
drivers/base/regmap.c.
...
> +#define MCP47FEB02_INTERNAL_BAND_GAP_MV 2440
_MV --> _mV
...
> +#define MCP47FEB02_DELAY_1_MS 1000
Drop '_1' and use (1 * USEC_PER_MSEC) as value.
But looking at the code this makes the definition useless, just use values
directly there.
...
> +struct mcp47feb02_features {
> + const char *name;
> + unsigned int phys_channels;
> + unsigned int resolution;
> + bool have_ext_vref1;
> + bool have_eeprom;
Inconsistent style. Be consistent.
> +};
...
> +struct mcp47feb02_channel_data {
> + enum vref_mode ref_mode;
> + u8 powerdown_mode;
> + bool use_2x_gain;
> + bool powerdown;
> + u16 dac_data;
Have you ran `pahole`? Please do, and amend the data types accordingly.
> +};
...
> +struct mcp47feb02_data {
> + struct mcp47feb02_channel_data chdata[MCP47FEBXX_MAX_CH];
> + int scale_1[MCP47FEB02_MAX_VALS_SCALES_CH];
> + int scale[MCP47FEB02_MAX_VALS_SCALES_CH];
> + const struct mcp47feb02_features *info;
> + const char *labels[MCP47FEBXX_MAX_CH];
> + unsigned long active_channels_mask;
> + struct i2c_client *client;
> + struct regmap *regmap;
Why both are needed?
> + bool vref1_buffered;
> + bool vref_buffered;
> + u16 phys_channels;
> + struct mutex lock; /* synchronize access to driver's state members */
> + bool use_vref1;
> + bool use_vref;
> +};
...
> +static ssize_t mcp47feb02_store_eeprom(struct device *dev, struct device_attribute *attr,
> + const char *buf, size_t len)
> +{
> + struct mcp47feb02_data *data = iio_priv(dev_to_iio_dev(dev));
> + int ret, i, val, val1, eewa_val;
Do you expect i to hold the signed value?
> + bool state;
> +
> + ret = kstrtobool(buf, &state);
> + if (ret < 0)
Why ' < 0'?
> + return ret;
> +
> + if (!state)
> + return 0;
> +
> + /*
> + * Verify DAC Wiper and DAC Configuratioin are unlocked. If both are disabled,
> + * writing to EEPROM is available.
> + */
> + ret = regmap_read(data->regmap, MCP47FEB02_WIPERLOCK_STATUS_REG_ADDR, &val);
> + if (ret)
> + return ret;
> +
> + if (val) {
> + dev_err(dev, "DAC Wiper and DAC Configuration not are unlocked.\n");
> + return -EINVAL;
> + }
> +
> + for_each_set_bit(i, &data->active_channels_mask, data->phys_channels) {
> + ret = mcp47feb02_write_to_eeprom(data, i << 3, data->chdata[i].dac_data);
> + if (ret)
> + return ret;
> + }
> +
> + ret = regmap_read(data->regmap, MCP47FEB02_VREF_REG_ADDR, &val);
> + if (ret)
> + return ret;
> +
> + ret = mcp47feb02_write_to_eeprom(data, MCP47FEB02_NV_VREF_REG_ADDR, val);
> + if (ret)
> + return ret;
> +
> + ret = regmap_read(data->regmap, MCP47FEB02_POWER_DOWN_REG_ADDR, &val);
> + if (ret)
> + return ret;
> +
> + ret = mcp47feb02_write_to_eeprom(data, MCP47FEB02_NV_POWER_DOWN_REG_ADDR, val);
> + if (ret)
> + return ret;
> +
> + ret = regmap_read_poll_timeout(data->regmap, MCP47FEB02_GAIN_BIT_STATUS_REG_ADDR, eewa_val,
> + !(eewa_val & MCP47FEB02_GAIN_BIT_STATUS_EEWA_MASK),
> + MCP47FEB02_DELAY_1_MS, MCP47FEB02_DELAY_1_MS * 5);
> + if (ret)
> + return ret;
> +
> + ret = regmap_read(data->regmap, MCP47FEB02_NV_GAIN_BIT_I2C_SLAVE_REG_ADDR, &val);
> + if (ret)
> + return ret;
> +
> + ret = regmap_read(data->regmap, MCP47FEB02_GAIN_BIT_STATUS_REG_ADDR, &val1);
> + if (ret)
> + return ret;
> +
> + ret = mcp47feb02_write_to_eeprom(data, MCP47FEB02_NV_GAIN_BIT_I2C_SLAVE_REG_ADDR,
> + (val1 & MCP47FEB02_VOLATILE_GAIN_BIT_MASK) |
> + (val & MCP47FEB02_NV_I2C_SLAVE_ADDR_MASK));
> + if (ret)
> + return ret;
> +
> + return len;
> +}
> +
Blank line should go after IIO_DEVICE_ATTR()...
> +static IIO_DEVICE_ATTR(store_eeprom, 0200, NULL, mcp47feb02_store_eeprom, 0);
...here, also Why not IIO_DEVICE_ATTR_WO()?
> +static struct attribute *mcp47feb02_attributes[] = {
> + &iio_dev_attr_store_eeprom.dev_attr.attr,
> + NULL
> +};
...
> +static int mcp47feb02_suspend(struct device *dev)
> +{
> + struct iio_dev *indio_dev = dev_get_drvdata(dev);
> + struct mcp47feb02_data *data = iio_priv(indio_dev);
> + int ret, ch;
Why ch is signed?
> + u8 pd_mode;
> +
> + guard(mutex)(&data->lock);
> +
> + for_each_set_bit(ch, &data->active_channels_mask, data->phys_channels) {
> + data->chdata[ch].powerdown = true;
> + pd_mode = data->chdata[ch].powerdown_mode + 1;
> + regmap_update_bits(data->regmap, MCP47FEB02_POWER_DOWN_REG_ADDR,
> + DAC_CTRL_MASK(ch), DAC_CTRL_VAL(ch, pd_mode));
Missed error check.
> + if (ret)
> + return ret;
> +
> + ret = regmap_write(data->regmap, ch << 3, data->chdata[ch].dac_data);
> + if (ret)
> + return ret;
> + }
> +
> + return 0;
> +}
I stopped here, I think it's already warrants a new version.
...
> + tmp = (s64)vref_mv * 1000000LL >> data->info->resolution;
vref_mV
MICRO, MEGA, ... from units.h
> + value_int = div_s64_rem(tmp, 1000000LL, &value_micro);
...
> + set_bit(reg, &data->active_channels_mask);
Is the atomic op required here?
...
> + if (fwnode_property_present(child, "label"))
Useless check as you don't handle an error code from below anyway (means
optional property).
> + fwnode_property_read_string(child, "label", &data->labels[reg]);
...
May you split this to add the main functionality with the subset of the
supported chips and add, for example, FxBx later in a separate patch?
This will help a lot with reviewing and pushing your patches forward.
Note, considering my comments above I don't think this will make v6.19-rc1,
so you have plenty of time to polish this and maybe even split more.
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists