| 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: <CAHp75Vct9BMqFcCjpjgkKr+vNOi5uFsi0FST-Hz5EzqA8UJueQ@mail.gmail.com>
Date: Fri, 29 Aug 2025 21:36:05 +0300
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: victor.duicu@...rochip.com
Cc: jic23@...nel.org, dlechner@...libre.com, nuno.sa@...log.com,
andy@...nel.org, robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org,
marius.cristea@...rochip.com, linux-iio@...r.kernel.org,
linux-kernel@...r.kernel.org, devicetree@...r.kernel.org
Subject: Re: [PATCH v4 2/2] iio: temperature: add support for MCP998X
On Fri, Aug 29, 2025 at 5:35 PM <victor.duicu@...rochip.com> wrote:
>
> This is the driver for Microchip MCP998X/33 and MCP998XD/33D
> Multichannel Automotive Temperature Monitor Family.
...
> +MICROCHIP MCP9982 TEMPERATURE DRIVER
> +M: Victor Duicu <victor.duicu@...rochip.com>
> +L: linux-iio@...r.kernel.org
> +S: Supported
> +F: Documentation/devicetree/bindings/iio/temperature/microchip,mcp9982.yaml
This file is orphaned in accordance with checkpatch, that's why we
usually add MAINTAINERS entry with the first file added to the tree,
i.e. DT bindings in this case.
> +F: drivers/iio/temperature/mcp9982.c
> +config MCP9982
> + tristate "Microchip Technology MCP9982 driver"
> + depends on I2C
> + help
> + Say yes here to build support for Microchip Technology's MCP998X/33
> + and MCP998XD/33D Multichannel Automotive Temperature Monitor Family.
> +
> + This driver can also be built as a module. If so, the module
> + will be called mcp9982.
...
> +#include <linux/array_size.h>
> +#include <linux/bitfield.h>
> +#include <linux/bits.h>
> +#include <linux/delay.h>
> +#include <linux/device/devres.h>
> +#include <linux/dev_printk.h>
> +#include <linux/err.h>
> +#include <linux/i2c.h>
> +#include <linux/iio/iio.h>
> +#include <linux/math64.h>
> +#include <linux/property.h>
> +#include <linux/regmap.h>
> +#include <linux/string.h>
> +#include <linux/units.h>
...
> +#define MCP9982_INTERNAL_HIGH_LIMIT_ADDR 0x0B
> +#define MCP9982_INTERNAL_LOW_LIMIT_ADDR 0x0C
For this and other similar registers, can't we use __be16 and read
properly these data?
...
> +#define MCP9982_CHAN(index, si, __address) ({ \
> + struct iio_chan_spec __chan = { \
> + .type = IIO_TEMP, \
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
> + .info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_SAMP_FREQ) | \
> + BIT(IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY), \
> + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ) | \
> + BIT(IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY) | \
> + BIT(IIO_CHAN_INFO_HYSTERESIS) | \
> + BIT(IIO_CHAN_INFO_OFFSET) | \
> + BIT(IIO_CHAN_INFO_SCALE), \
> + .channel = index, \
> + .address = __address, \
> + .scan_index = si, \
> + .scan_type = { \
> + .sign = 'u', \
> + .realbits = 8, \
> + .storagebits = 8, \
> + }, \
> + .indexed = 1, \
> + }; \
> + __chan; \
> +})
Instead of a GCC expression, please use compound literal. It will make
it shorter and neater.
...
> +struct mcp9982_features {
> + const char *name;
> + u8 phys_channels;
> + bool hw_thermal_shutdown;
> + bool allow_apdd;
> +};
> +
> +static const struct mcp9982_features mcp9933_chip_config = {
> + .name = "mcp9933",
> + .phys_channels = 3,
> + .hw_thermal_shutdown = 0,
> + .allow_apdd = 1,
AFAICS they are booleans, please avoid unneeded type conversions.
Same for all other assignments of these fields.
> +};
...
> +/* (Sampling_Frequency(Hz) * 1000000) / (Window_Size * 2) */
> +static unsigned int mcp9982_calc_all_3db_values(void)
> +{
> + u32 denominator, remainder;
> + unsigned int i, j;
> + u64 numerator;
> +
> + for (i = 0; i < ARRAY_SIZE(mcp9982_window_size); i++) {
> + for (j = 0; j < ARRAY_SIZE(mcp9982_sampl_fr); j++) {
> + numerator = MICRO * mcp9982_sampl_fr[j].integer;
The comment above doesn't really explain this MICRO part. Please,
elaborate more on it.
> + denominator = 2 * mcp9982_window_size[i] *
> + mcp9982_sampl_fr[j].fract;
> + remainder = do_div(numerator, denominator);
> + remainder = do_div(numerator, MICRO);
> + mcp9982_3db_values_map_tbl[j][i][0] = numerator;
> + mcp9982_3db_values_map_tbl[j][i][1] = remainder;
> + }
> + }
> + return 0;
> +}
...
> +struct mcp9982_priv {
> + struct regmap *regmap;
> + const struct mcp9982_features *chip;
> + /*
> + * Synchronize access to private members, and ensure atomicity of
> + * consecutive regmap operations.
> + */
This comment doesn't make clear where the private members are.
> + struct mutex lock;
> + struct iio_chan_spec *iio_chan;
> + const char *labels[MCP9982_MAX_NUM_CHANNELS];
> + unsigned int ideality_value[4];
> + unsigned int sampl_idx;
> + unsigned long time_limit;
> + bool recd34_enable;
> + bool recd12_enable;
> + bool apdd_enable;
> + bool run_state;
> + bool wait_before_read;
Taking into account the above and thinking of what those members, the
booleans might be converted to bit-flags to occupy less space (makes
sense only if 3+ booleans can be combined.
> + u8 num_channels;
> +};
> +
> +static int mcp9982_read_avail(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan, const int **vals,
> + int *type, int *length, long mask)
> +{
> + struct mcp9982_priv *priv = iio_priv(indio_dev);
> + unsigned int idx = 0;
> + unsigned int sub = 0;
Instead of these assignments, make them an 'else' branch. This will be
compact in one place and make code more robust against some subtle
changes.
> + if (priv->chip->hw_thermal_shutdown) {
> + idx = 4;
> + sub = 8;
> + }
> + switch (mask) {
> + case IIO_CHAN_INFO_SAMP_FREQ:
> + *type = IIO_VAL_INT_PLUS_MICRO;
> + *vals = mcp9982_conv_rate[idx];
> + *length = ARRAY_SIZE(mcp9982_conv_rate) * 2 - sub;
> + return IIO_AVAIL_LIST;
> + case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
> + *type = IIO_VAL_INT_PLUS_MICRO;
> + *vals = mcp9982_3db_values_map_tbl[priv->sampl_idx][0];
> + *length = ARRAY_SIZE(mcp9982_3db_values_map_tbl[priv->sampl_idx]) * 2;
> + return IIO_AVAIL_LIST;
> + default:
> + return -EINVAL;
> + }
> +}
...
> + if (!priv->run_state) {
Make it positive conditional, it's slightly easier to read.
> + ret = regmap_write(priv->regmap, MCP9982_ONE_SHOT_ADDR, 1);
> + if (ret)
> + return ret;
> + /*
> + * This delay waits for system start-up, as specified by
> + * time to first conversion from standby
Missing period.
> + */
> + mdelay(125);
The comment poorly explains why we need so long _atomic_ (!) delay.
This needs to be _very well_ justified.
> + ret = regmap_read_poll_timeout(priv->regmap, MCP9982_STATUS_ADDR,
> + reg_status,
> + !(reg_status & MCP9982_STATUS_BUSY),
> + mcp9982_delay_ms[priv->sampl_idx] * 1000,
USEC_PER_MSEC
> + 1000 * mcp9982_delay_ms[priv->sampl_idx] * 1000);
USEC_PER_MSEC and MSEC_PER_SEC
> + if (ret)
> + return ret;
> + } else {
> + /*
> + * When working in Run mode, after modifying a parameter (like sampling
> + * frequency) we have to wait a delay before reading the new values.
> + * We can't determine when the conversion is done based on BUSY bit.
the BUSY
> + */
> + if (priv->wait_before_read) {
> + if (!time_after(jiffies, priv->time_limit))
> + mdelay(jiffies_to_msecs(priv->time_limit - jiffies));
Ditto.
> + priv->wait_before_read = false;
> + }
> + }
...
> +static int mcp9982_read_label(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan, char *label)
> +{
> + struct mcp9982_priv *priv = iio_priv(indio_dev);
> +
> + if (chan->channel < 0 || chan->channel > 4)
Is the channel signed?
> + return -EINVAL;
> +
> + return sysfs_emit(label, "%s\n", priv->labels[chan->channel]);
> +}
...
> + /*
> + * in Run mode, when changing the frequency, wait a delay based
> + * on the previous value to ensure the new value becomes active
> + */
Respect English grammar and punctuation.
...
> + case IIO_CHAN_INFO_HYSTERESIS:
> + if (val < 0 || val > 255)
Do you want to check if it's U8_MAX? Or are these HW related values?
> + return -EINVAL;
> +
> + ret = regmap_write(priv->regmap, MCP9982_HYS_ADDR, val);
> + if (ret)
> + return ret;
> + break;
...
> + /*
> + * Chips with "D" work in Run state and those without work
> + * in Standby state
> + */
Missing period. Just fix all multi-line comments to be the same style,
i.e. with proper English grammar and punctuation.
> + if (priv->chip->hw_thermal_shutdown)
> + priv->run_state = 1;
> + else
> + priv->run_state = 0;
->hw_thermal_shutdown = ->run_state;
No condition needed.
...
> + /* Set auto-detection beta compensation for channels 1 and 2 */
Why only these channels? Comment needs elaboration.
> + for (i = 0; i < 2; i++) {
> + ret = regmap_write(priv->regmap, MCP9982_EXT_BETA_CFG_ADDR(i),
> + MCP9982_BETA_AUTODETECT);
> + if (ret)
> + return ret;
> + }
...
> +static int mcp9982_parse_of_config(struct iio_dev *indio_dev, struct device *dev,
Replace _of part by _fw as this is agnostic.
> + int device_nr_channels)
...
> + device_for_each_child_node_scoped(dev, child) {
> + fwnode_property_read_u32(child, "reg", ®_nr);
I don't see where you assign the default value for reg_nr. And better
to check the return value here as it seems to be mandatory property.
> + if (!reg_nr || reg_nr >= device_nr_channels)
> + return dev_err_probe(dev, -EINVAL,
> + "The index of the channels does not match the chip\n");
> +
> + priv->ideality_value[reg_nr - 1] = 18;
> + if (fwnode_property_present(child, "microchip,ideality-factor")) {
> + fwnode_property_read_u32(child, "microchip,ideality-factor",
> + &priv->ideality_value[reg_nr - 1]);
> + if (priv->ideality_value[reg_nr - 1] > 63)
> + return dev_err_probe(dev, -EOVERFLOW,
> + "The ideality value is higher than maximum\n");
> + }
> +
> + fwnode_property_read_string(child, "label",
> + &priv->labels[reg_nr]);
> +
> + priv->iio_chan[iio_idx++] = MCP9982_CHAN(reg_nr, reg_nr,
> + MCP9982_INT_VALUE_ADDR(reg_nr));
> + }
...
> + chip = i2c_get_match_data(client);
> + if (!chip)
> + return -EINVAL;
> + priv->chip = chip;
You can use priv->chip directly, no local variable is needed, But OTOH
this makes code shorter.
...
> + ret = mcp9982_parse_of_config(indio_dev, &client->dev, chip->phys_channels);
You have dev, use it.
> + if (ret)
> + return dev_err_probe(dev, ret, "Parameter parsing error\n");
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists