lkml.org 
[lkml]   [2023]   [Mar]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH 1/2] iio: max597x: Add support for max597x
From
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, &reg_l);
>> +        if (ret < 0)
>> +            return ret;
>> +        ret = regmap_read(data->regmap, chan->address - 1, &reg_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

\
 
 \ /
  Last update: 2023-03-27 01:14    [W:0.065 / U:0.448 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site