Messages in this thread | ![/](/images/icornerl.gif) | | Date | Thu, 23 Mar 2023 19:17:48 +0530 | Subject | Re: [PATCH 1/2] iio: max597x: Add support for max597x | From | Naresh Solanki <> |
| |
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
| ![\](/images/icornerr.gif) |