| 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: <CAHp75VdzVzNV1k8RqG6Rxsg06Oqu_p1o-4QFeT10xBjrFOEZHA@mail.gmail.com>
Date: Tue, 15 Apr 2025 22:05:29 +0300
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: victor.duicu@...rochip.com
Cc: jic23@...nel.org, andy@...nel.org, dlechner@...libre.com,
nuno.sa@...log.com, marius.cristea@...rochip.com, linux-iio@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 2/2] iio: temperature: add support for MCP998X
On Tue, Apr 15, 2025 at 4:27 PM <victor.duicu@...rochip.com> wrote:
>
> This is the driver for Microchip MCP998X/33 and MCP998XD/33D
> Multichannel Automotive Monitor Family.
...
> +KernelVersion: 6.14
This boat is already sailing, it should be v6.16 at bare minimum.
> +Contact: linux-iio@...r.kernel.org
> +Description:
> + This attribute controls the number of samples for the
> + running average window applied to External Channel 1.
> + Using this method the temperature spikes are reduced.
> + X is the IIO index of the device.
...
> +KernelVersion: 6.14
Ditto.
> +Contact: linux-iio@...r.kernel.org
> +Description:
> + Reading returns a list with the possible number of samples used
> + in the running average window. The window can be composed of 1,
> + 4 or 8 previous samples. X is the IIO index of the device.
...
> +/*
> + * IIO driver for MCP998X/33 and MCP998XD/33D Multichannel Automotive Temperature Monitor Family
> + *
> + * Copyright (C) 2025 Microchip Technology Inc. and its subsidiaries
> + *
> + * Author: Victor Duicu <victor.duicu@...rochip.com>
> + *
> + * Datasheet can be found here:
> + * https://ww1.microchip.com/downloads/aemDocuments/documents/MSLD/ProductDocuments/DataSheets/MCP998X-Family-Data-Sheet-DS20006827.pdf
> + *
Redundant blank line
> + */
...
> +#include <linux/bitfield.h>
+ bits.h
+ err.h
> +#include <linux/i2c.h>
> +#include <linux/iio/events.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/irq.h>
> +#include <linux/limits.h>
> +#include <linux/property.h>
> +#include <linux/regmap.h>
> +#include <linux/string.h>
> +#include <linux/unaligned.h>
> +#include <linux/units.h>
Please, make sure you follow the IWYU principle.
...
> +#define MCP9982_INT_HIGH_BYTE_ADDR(index) (2 * (index))
> +#define MCP9982_INT_LOW_BYTE_ADDR(index) (2 * (index) + 1)
Why? Can't you use __be16 everywhere and bulk IO?
...
> +#define MCP9982_DEV_ATTR(name) (&iio_dev_attr_##name.dev_attr.attr)
Useless macro that makes code harder to read.
...
> +#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), \
> + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ), \
> + .channel = index, \
> + .address = __address, \
> + .scan_index = si, \
> + .scan_type = { \
> + .sign = 'u', \
> + .realbits = 8, \
> + .storagebits = 8, \
> + .endianness = IIO_CPU \
> + }, \
> + .indexed = 1, \
> + }; \
> + __chan; \
Why in this form and not as a compound literal?
> +})
...
> +/*
> + * struct mcp9982_features - features of a mcp9982 instance
> + * @phys_channels: number of physical channels supported by the chip
> + * @name: chip's name
> + */
> +struct mcp9982_features {
> + u8 phys_channels;
> + const char *name;
Have you run `pahole`? Has it found a room to improve?
> +};
...
> +/**
> + * struct mcp9992_priv - information about chip parameters
> + * @client: the i2c-client attached to the device
> + * @regmap: device register map
> + * @iio_info iio_info
> + * @iio_chan specifications of channels
> + * @num_channels number of physical channels
> + * @lock synchronize access to driver's state members
> + * @running_avg number of samples in the running average window
> + * @hysteresis value of temperature hysteresis
> + * @temp_range_code coded value representing the set temperature range
> + * @labels labels of the channels
> + * @chip_name name of the chip present
> + * @beta_user_value value given by the user for beta on channel 1 and 2
> + * @apdd state of anti-parallel diode mode
> + * @recd12 state of REC on channels 1 and 2
> + * @recd34 state of REC on channels 3 and 4
> + * @ideality_user_value values given by user to ideality factor for all channels
> + */
> +
Redundant blank line.
...
> +struct mcp9982_priv {
> + struct i2c_client *client;
For what do you need a client? Wouldn't struct device *dev suffice?
And if so, can't it be retrieved from regmap when needed?
> + struct regmap *regmap;
> + struct iio_info iio_info;
> +
> + struct iio_chan_spec *iio_chan;
> + u8 num_channels;
> +
> + /*
> + * Synchronize access to private members, and ensure
> + * atomicity of consecutive regmap operations.
> + */
> + struct mutex lock;
> +
> + int running_avg;
> + int hysteresis;
> + int temp_range_code;
> + char *labels[MCP9982_MAX_NUM_CHANNELS];
> + char *chip_name;
> + int beta_user_value[2];
> + int apdd;
> + int recd12;
> + int recd34;
> + int ideality_user_value[4];
> +};
...
> +static int mcp9982_read_raw(struct iio_dev *indio_dev, struct iio_chan_spec const *chan,
> + int *val, int *val2, long mask)
> +{
> + struct mcp9982_priv *priv = iio_priv(indio_dev);
> + int ret, val3, HIGH_BYTE, LOW_BYTE;
No way we name variables in the capital letters. And use __be16.
> +
> + /* Write in ONESHOT register to take a new reading */
> + ret = regmap_write(priv->regmap, MCP9982_ONE_SHOT_ADDR, 1);
> + if (ret)
> + return ret;
> +
> + guard(mutex)(&priv->lock);
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
> + HIGH_BYTE = MCP9982_INT_HIGH_BYTE_ADDR(chan->channel);
> + LOW_BYTE = MCP9982_INT_LOW_BYTE_ADDR(chan->channel);
> +
> + ret = regmap_read(priv->regmap, HIGH_BYTE, val);
> + if (ret)
> + return ret;
> +
> + if (priv->temp_range_code)
> + *val -= MCP9982_TEMP_OFFSET;
> +
> + ret = regmap_read(priv->regmap, LOW_BYTE, val2);
> + if (ret)
> + return ret;
> +
> + *val2 = mcp9982_fractional_values[*val2 >> 5];
> +
> + return IIO_VAL_INT_PLUS_MICRO;
> + case IIO_CHAN_INFO_SAMP_FREQ:
> + ret = regmap_read(priv->regmap, MCP9982_CONV_ADDR, &val3);
> + if (ret)
> + return ret;
> +
> + *val = mcp9982_conv_rate[val3][0];
> + *val2 = mcp9982_conv_rate[val3][1];
> +
> + return IIO_VAL_INT_PLUS_MICRO;
> + default:
> + return -EINVAL;
> + }
> +}
...
> + return sprintf(label, "%s\n", priv->labels[chan->channel]);
+ sprintf.h
...
> +static int mcp9982_write_raw(struct iio_dev *indio_dev, struct iio_chan_spec const *chan,
> + int val, int val2, long mask)
> +{
> + struct mcp9982_priv *priv = iio_priv(indio_dev);
> + struct device *dev = &priv->client->dev;
> + int i;
Why signed?
> + int status = 0;
Why not boolean?
> + guard(mutex)(&priv->lock);
> + switch (mask) {
> + case IIO_CHAN_INFO_SAMP_FREQ:
> + for (i = 0; i < ARRAY_SIZE(mcp9982_conv_rate); i++) {
+ array_size.h
> + if (val == mcp9982_conv_rate[i][0] &&
> + val2 == mcp9982_conv_rate[i][1]){
> + status = 1;
> + break;
> + }
> + }
> + if (!status)
> + return dev_err_probe(dev, -EINVAL, "Sampling Frequency is invalid\n");
> +
> + return regmap_write(priv->regmap, MCP9982_CONV_ADDR, i);
> + default:
> + return -EINVAL;
> + }
> +}
...
> +static ssize_t mcp9982_running_average_window_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> + struct mcp9982_priv *priv = iio_priv(indio_dev);
> +
> + return sprintf(buf, "%u sample(s)\n", priv->running_avg);
Please, read the documentation about this. s*printf() must not be used
here (in ->show() callbacks). There are special APIs.
...
> +static ssize_t mcp9982_running_average_window_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> + struct mcp9982_priv *priv = iio_priv(indio_dev);
> + int val, ret, reg_val;
> +
> + if (kstrtouint(buf, 10, &val)) {
Do not shadow the actuall error code.
> + dev_err(dev, "Value is not a number\n");
> + return -EINVAL;
> + }
> +
> + switch (val) {
> + case 1:
> + reg_val = 0;
> + break;
> + case 4:
> + reg_val = 1;
> + break;
> + case 8:
> + reg_val = 3;
> + break;
> + default:
> + dev_err(dev, "Value is invalid\n");
> + return -EINVAL;
> + }
> +
> + guard(mutex)(&priv->lock);
> +
> + ret = regmap_write(priv->regmap, MCP9982_RUNNING_AVG_ADDR, reg_val);
How ret is being used?
I think I have a déjà vu about this code... I think I commented this
or something like this already and there seems to be no reaction...
> + priv->running_avg = val;
> +
> + return count;
> +}
...
> +static int mcp9982_prep_custom_attributes(struct mcp9982_priv *priv, struct iio_dev *indio_dev)
> +{
> + struct attribute **mcp9982_custom_attr;
> + struct attribute_group *mcp9982_group;
> + struct device *dev = &priv->client->dev;
> +
> + mcp9982_group = devm_kzalloc(dev, sizeof(*mcp9982_group), GFP_KERNEL);
+ device/devres.h
> + if (!mcp9982_group)
> + return -ENOMEM;
> +
> + mcp9982_custom_attr = devm_kzalloc(dev, MCP9982_NR_CUSTOM_ATTR *
> + sizeof(*mcp9982_group) + 1, GFP_KERNEL);
No, use devm_kcalloc() and I'm not sure you have got the size correctly here.
> + if (!mcp9982_custom_attr)
> + return -ENOMEM;
> +
> + mcp9982_custom_attr[0] = MCP9982_DEV_ATTR(running_average_window);
> + mcp9982_custom_attr[1] = MCP9982_DEV_ATTR(running_average_window_available);
> +
> + mcp9982_group->attrs = mcp9982_custom_attr;
> + priv->iio_info.attrs = mcp9982_group;
> +
> + return 0;
> +}
> + if (device_property_present(dev, "microchip,beta-channel1")) {
...
> + return dev_err_probe(dev, -EINVAL, "Beta 1 value is higher than max\n");
+ dev_printk.h
...
> + priv->iio_chan = devm_kzalloc(dev, priv->num_channels * sizeof(*priv->iio_chan),
> + GFP_KERNEL);
kcalloc()
> + if (!priv->iio_chan)
> + return -ENOMEM;
...
> + device_for_each_child_node_scoped(dev, child) {
> + ret = fwnode_property_read_u32(child, "reg", ®_nr);
How is ret being used?
> + if (reg_nr >= mcp9982_chip_config[i].phys_channels)
> + return dev_err_probe(dev, -EINVAL,
> + "The index of the channels does not match the chip\n");
> +
> + if (fwnode_property_present(child, "microchip,ideality-factor")) {
> + ret = fwnode_property_read_u32(child, "microchip,ideality-factor",
> + &priv->ideality_user_value[reg_nr - 1]);
> + if (priv->ideality_user_value[reg_nr - 1] > MCP9982_IDEALITY_MAX_VALUE)
> + return dev_err_probe(dev, -EINVAL,
> + "The ideality value is higher than maximum\n");
> + } else {
> + priv->ideality_user_value[reg_nr - 1] = MCP9982_IDEALITY_FACTOR_DEFAULT;
> + }
> +
> + ret = fwnode_property_read_string(child, "label",
> + (const char **)&priv->labels[reg_nr]);
Ditto.
Why casting?
> + priv->iio_chan[iio_idx++] = MCP9982_CHAN(reg_nr, reg_nr,
> + MCP9982_INT_HIGH_BYTE_ADDR(reg_nr));
> + }
> +
> + return 0;
> +}
...
> + i2c_set_clientdata(client, indio_dev);
How is this being used?
...
> +static struct i2c_driver mcp9982_driver = {
> + .driver = {
> + .name = "mcp9982",
> + .of_match_table = mcp9982_of_match,
> + },
> + .probe = mcp9982_probe,
> + .id_table = mcp9982_id,
> +};
> +
Redundant blank line.
> +module_i2c_driver(mcp9982_driver);
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists