On 3/23/23 05:01, Naresh Solanki wrote:
Hi,
On 22-03-2023 09:28 pm, Lars-Peter Clausen wrote:
Hi,
This looks really good. A few minor comments inline.
On 3/22/23 05:43, Naresh Solanki wrote:
[...]
+static int max597x_iio_read_raw(struct iio_dev *iio_dev,
+ struct iio_chan_spec const *chan,
+ int *val, int *val2, long info)
+{
+ int ret;
+ struct max597x_iio *data = iio_priv(iio_dev);
+ unsigned int reg_l, reg_h;
+
+ switch (info) {
+ case IIO_CHAN_INFO_RAW:
+ ret = regmap_read(data->regmap, chan->address, ®_l);
+ if (ret < 0)
+ return ret;
+ ret = regmap_read(data->regmap, chan->address - 1, ®_h);
+ if (ret < 0)
+ return ret;
Is there any chance of a race condition of getting inconsistent data
when splitting this over two reads? I.e. registers being updated with
new values in between the two reads.
yes, reg_l holds lower 2 bits. due to latency in reads, value may differ.
+ *val = (reg_h << 2) | (reg_l & 3);
+
+ return IIO_VAL_INT;
+ case IIO_CHAN_INFO_SCALE:
+
+ switch (chan->address) {
+ case MAX5970_REG_CURRENT_L(0):
+ fallthrough;
`fallthrough` should not be needed for multiple case statements right
on top of each other with no code in between. Same below
Sure.
+ case MAX5970_REG_CURRENT_L(1):
+ /* in A, convert to mA */
+ *val = data->irng[chan->channel] * 1000;
+ *val2 =
+ data->shunt_micro_ohms[chan->channel] * ADC_MASK;
ADC_MASK should really have a MAX5970_ prefix, but I guess it is
defined in max597x.h
Yes its taken from max597x.h
+ return IIO_VAL_FRACTIONAL;
+
+ case MAX5970_REG_VOLTAGE_L(0):
+ fallthrough;
+ case MAX5970_REG_VOLTAGE_L(1):
+ /* in uV, convert to mV */
+ *val = data->mon_rng[chan->channel];
+ *val2 = ADC_MASK * 1000;
+ return IIO_VAL_FRACTIONAL;
+ }
+
+ break;
+ }
+ return -EINVAL;
+}
[..]
+static int max597x_iio_probe(struct platform_device *pdev)
+{
+ struct max597x_data *max597x = dev_get_drvdata(pdev->dev.parent);
+ struct i2c_client *i2c = to_i2c_client(pdev->dev.parent);
+ struct regmap *regmap = dev_get_regmap(pdev->dev.parent, NULL);
+ struct iio_dev *indio_dev;
+ struct max597x_iio *priv;
+ int ret, i;
+
+ if (!regmap)
+ return -EPROBE_DEFER;
+
+ if (!max597x || !max597x->num_switches)
+ return -EPROBE_DEFER;
+
+ /* registering iio */
+ indio_dev = devm_iio_device_alloc(&i2c->dev, sizeof(*priv));
For the devm allocations we should be using &pdev->dev and not the
I2C device, since this is the device to which the allocations belong
and where they should be freed when the device is removed.
Sure. Will use &pdev->dev
+ if (!indio_dev) {
+ dev_err(&i2c->dev, "failed allocating iio device\n");
Consider using dev_err_probe() for error message printing. This will
give a consistent formatting of the messages. Also again use
&pdev->dev instead of I2C device to get the right device listed in
the error messages.
Sure. Will use
dev_err_probe(&pdev->dev, ret, "could not register iio device");
+ return -ENOMEM;
+ }
+ indio_dev->name = dev_name(&i2c->dev);
The IIO ABI wants the type of the chip for the name. E.g. "max5970",
using dev_name() of the parent I2C device will result in something else.
Sure. Will make it:
indio_dev->name = dev_name(&pdev->dev);
dev_name() in general should not be used for indio_dev->name, it does
not meet the ABI requirements for the IIO ABI. Move this into the switch
block below and then assign "max5970" or "max5978" depending on the
device type.
