Hi
On 23-03-2023 06:37 pm, Lars-Peter Clausen wrote:
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.
Sure.
Thanks,
Naresh
