Messages in this thread | | | Date | Thu, 23 Mar 2023 17:31:18 +0530 | Subject | Re: [PATCH 1/2] iio: max597x: Add support for max597x | From | Naresh Solanki <> |
| |
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); >> [...]
Regards, Naresh
| |