| 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: <aTiYjT1kjOP3xN8x@smile.fi.intel.com>
Date: Tue, 9 Dec 2025 23:45:49 +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 v3 2/2] iio: dac: adding support for Microchip MCP47FEB02
On Tue, Dec 09, 2025 at 06:06:24PM +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.
Thank you for the update, almost good. See my comments below.
...
> +config MCP47FEB02
> + tristate "MCP47F(E/V)B01/02/04/08/11/12/14/18/21/22/24/28 DAC driver"
> + depends on I2C
> + help
> + Say yes here if you want to build the driver for the Microchip:
> + 8-bit DAC: MCP47FEB01, MCP47FEB02, MCP47FEB04, MCP47FEB08,
> + MCP47FVB01, MCP47FVB02, MCP47FVB04, MCP47FVB08
> + 10-bit DAC: MCP47FEB11, MCP47FEB12, MCP47FEB14, MCP47FEB18,
> + MCP47FVB11, MCP47FVB12, MCP47FVB14, MCP47FVB18
> + 12-bit DAC: MCP47FEB21, MCP47FEB22, MCP47FEB24, MCP47FEB28,
> + MCP47FVB21, MCP47FVB22, MCP47FVB24, MCP47FVB28
This is good, but you can do even better, like in cover letter:
- 8-bit DAC:
MCP47FEB01, MCP47FEB02, MCP47FEB04, MCP47FEB08,
MCP47FVB01, MCP47FVB02, MCP47FVB04, MCP47FVB08
- ...
> + having 1 to 8 channels, 8/10/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.
...
> +struct mcp47feb02_features {
> + const char *name;
> + unsigned int phys_channels;
> + unsigned int resolution;
> + bool have_ext_vref1;
> + bool have_eeprom;
Now it's not difference, but maybe in the future we would want more than 4
flags, which will occupy more memory with such an approach. In any case,
currently it is fine.
> +};
...
> + ret = kstrtobool(buf, &state);
+ kstrtox.h
> + if (ret)
> + return ret;
...
> +static ssize_t mcp47feb02_write_powerdown(struct iio_dev *indio_dev, uintptr_t private,
> + const struct iio_chan_spec *ch, const char *buf,
> + size_t len)
> +{
> + struct mcp47feb02_data *data = iio_priv(indio_dev);
> + u8 tmp_pd_mode;
> + bool state;
> + u32 reg;
> + int ret;
> +
> + guard(mutex)(&data->lock);
> +
> + ret = kstrtobool(buf, &state);
> + if (ret < 0)
if (ret)
Be consistent with a style.
> + return ret;
> +
> + reg = ch->address;
> +
> + /*
> + * Set channel to the power-down mode selected. Normal operation mode (0000h)
> + * must be written to register in order to exit power-down mode.
> + */
> + tmp_pd_mode = state ? (data->chdata[reg].powerdown_mode + 1) : MCP47FEB02_NORMAL_OPERATION;
> + ret = regmap_update_bits(data->regmap, MCP47FEB02_POWER_DOWN_REG_ADDR,
> + DAC_CTRL_MASK(reg), DAC_CTRL_VAL(reg, tmp_pd_mode));
> + if (ret)
> + return ret;
> +
> + data->chdata[reg].powerdown = state;
> +
> + return len;
> +}
...
> +static int mcp47feb02_init_scales_avail(struct mcp47feb02_data *data, int vdd_mV,
> + int vref_mV, int vref1_mV)
> +{
> + struct device *dev = &data->client->dev;
Derive it from regmap (here and everywhere else, where no explicit 'client').
> + int tmp_vref;
> +
> + mcp47feb02_init_scale(data, MCP47FEB02_SCALE_VDD, vdd_mV, data->scale);
> +
> + if (data->use_vref)
> + tmp_vref = vref_mV;
> + else
> + tmp_vref = MCP47FEB02_INTERNAL_BAND_GAP_mV;
> +
> + mcp47feb02_init_scale(data, MCP47FEB02_SCALE_GAIN_X1, tmp_vref, data->scale);
> + mcp47feb02_init_scale(data, MCP47FEB02_SCALE_GAIN_X2, tmp_vref * 2, data->scale);
> +
> + if (data->phys_channels >= 4) {
> + mcp47feb02_init_scale(data, MCP47FEB02_SCALE_VDD, vdd_mV, data->scale_1);
> + if (data->use_vref1 && vref1_mV <= 0)
> + return dev_err_probe(dev, -EINVAL, "Invalid voltage for Vref1\n");
Hmm... Does negative vref1_mV contain an actual error code? If so, use it.
> + if (data->use_vref1)
> + tmp_vref = vref1_mV;
> + else
> + tmp_vref = MCP47FEB02_INTERNAL_BAND_GAP_mV;
> +
> + mcp47feb02_init_scale(data, MCP47FEB02_SCALE_GAIN_X1,
> + tmp_vref, data->scale_1);
> + mcp47feb02_init_scale(data, MCP47FEB02_SCALE_GAIN_X2,
> + tmp_vref * 2, data->scale_1);
> + }
> +
> + return 0;
> +}
...
> + ret = fwnode_property_read_string(child, "label", &data->labels[reg]);
> + if (ret)
> + return dev_err_probe(dev, ret, "%s: invalid label\n",
> + fwnode_get_name(child));
%pfw
...
> +static int mcp47feb02_probe(struct i2c_client *client)
> +{
> + const struct mcp47feb02_features *info;
> + struct device *dev = &client->dev;
> + struct mcp47feb02_data *data;
> + struct iio_dev *indio_dev;
> + int vref1_mV = 0;
> + int vref_mV = 0;
> + int vdd_mV = 0;
> + int ret;
> +
> + indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + data = iio_priv(indio_dev);
> + data->client = client;
This is used solely for the getting struct device out of it. Since the struct
mcp47feb02_data contains regmap, this member is duplication and redundancy.
Use regmap_get_device() in the users.
> + info = i2c_get_match_data(client);
> + if (!info)
> + return -EINVAL;
> +
> + data->info = info;
> +
> + if (info->have_eeprom) {
> + data->regmap = devm_regmap_init_i2c(client, &mcp47feb02_regmap_config);
> + indio_dev->info = &mcp47feb02_info;
> + } else {
> + data->regmap = devm_regmap_init_i2c(client, &mcp47fvb02_regmap_config);
> + indio_dev->info = &mcp47fvb02_info;
> + }
> +
Unneeded blank line.
> + if (IS_ERR(data->regmap))
> + return dev_err_probe(dev, PTR_ERR(data->regmap), "Error initializing i2c regmap\n");
> +
> + indio_dev->name = info->name;
> +
> + ret = mcp47feb02_parse_fw(indio_dev, info);
> + if (ret)
> + return dev_err_probe(dev, ret, "Error parsing firmware data\n");
> +
> + ret = devm_mutex_init(dev, &data->lock);
> + if (ret)
> + return ret;
> +
> + ret = devm_regulator_get_enable_read_voltage(dev, "vdd");
> + if (ret < 0)
> + return ret;
> + vdd_mV = ret / 1000;
It might be (MICRO / MILLI)
> + ret = devm_regulator_get_enable_read_voltage(dev, "vref");
> + if (ret > 0) {
> + vref_mV = ret / 1000;
Ditto.
> + data->use_vref = true;
> + } else {
> + dev_dbg(dev, "using internal band gap as voltage reference.\n");
> + dev_dbg(dev, "Vref is unavailable.\n");
> + }
> +
> + if (info->have_ext_vref1) {
> + ret = devm_regulator_get_enable_read_voltage(dev, "vref1");
> + if (ret > 0) {
> + vref1_mV = ret / 1000;
Ditto.
> + data->use_vref1 = true;
> + } else {
> + dev_dbg(dev, "using internal band gap as voltage reference 1.\n");
> + dev_dbg(dev, "Vref1 is unavailable.\n");
> + }
> + }
> +
> + ret = mcp47feb02_init_ctrl_regs(data);
> + if (ret)
> + return dev_err_probe(dev, ret, "Error initialising vref register\n");
> +
> + ret = mcp47feb02_init_ch_scales(data, vdd_mV, vref_mV, vref1_mV);
> + if (ret)
> + return ret;
> +
> + return devm_iio_device_register(dev, indio_dev);
> +}
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists