linux-kernel - Re: [PATCH 1/2] iio: max597x: Add support for max597x

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <0d7c5b82-e48f-4331-07a3-8bfd51f3a1e6@9elements.com>
Date:   Thu, 23 Mar 2023 19:17:48 +0530
From:   Naresh Solanki <naresh.solanki@...ements.com>
To:     Lars-Peter Clausen <lars@...afoo.de>, Lee Jones <lee@...nel.org>,
        Jonathan Cameron <jic23@...nel.org>
Cc:     Patrick Rudolph <patrick.rudolph@...ements.com>,
        linux-kernel@...r.kernel.org, linux-iio@...r.kernel.org
Subject: Re: [PATCH 1/2] iio: max597x: Add support for max597x

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, &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);
>>
> 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