linux-kernel - Re: [PATCH v2 1/5] iio: adc: mcp320x: use callbacks for RX conversion

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: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAHp75Vek8tLnwYWZO91HHr1_i7G_F9tErnRsq-GwfQcpR0FaTw@mail.gmail.com>
Date:   Wed, 31 Aug 2022 15:40:49 +0300
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     Vincent Whitchurch <vincent.whitchurch@...s.com>
Cc:     Jonathan Cameron <jic23@...nel.org>, kernel <kernel@...s.com>,
        Lars-Peter Clausen <lars@...afoo.de>,
        linux-iio <linux-iio@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 1/5] iio: adc: mcp320x: use callbacks for RX conversion

On Wed, Aug 31, 2022 at 1:05 PM Vincent Whitchurch
<vincent.whitchurch@...s.com> wrote:
>
> Replace the device_index switch with callbacks from the chip_info
> structure, so that the latter has all the information needed to handle
> the variants.

Below are for the further patches, as I see the original code has the
same disadvantages.

...

>  struct mcp320x_chip_info {
>         const struct iio_chan_spec *channels;
>         unsigned int num_channels;
>         unsigned int resolution;

>         unsigned int conv_time; /* usec */

Instead of a comment, I would rename it to conv_time_us.

> +       int (*convert_rx)(struct mcp320x *adc);
>  };

...

> +       return adc->rx_buf[0] << 5 | adc->rx_buf[1] >> 3;

> +       return adc->rx_buf[0] << 2 | adc->rx_buf[1] >> 6;

> +       return adc->rx_buf[0] << 7 | adc->rx_buf[1] >> 1;

> +       return adc->rx_buf[0] << 4 | adc->rx_buf[1] >> 4;

> +       return sign_extend32((adc->rx_buf[0] & 0x1f) << 8 | adc->rx_buf[1], 12);

All above should really use

u16 buf = be16_to_cpu(&adc->rx_buf[0]);

return buf >> 3 /* 6, 1, 4 (respectively) */;

...

> +       if (raw & BIT(22) && raw & BIT(23))

> +               return -EIO; /* cannot have overrange AND underrange */
> +       else if (raw & BIT(22))

Redundant  'else'.

> +               raw &= ~BIT(22); /* overrange */
> +       else if (raw & BIT(23) || raw & BIT(21))

if (raw & (BIT(23) | BIT(21))) ?

> +               raw |= GENMASK(31, 22); /* underrange or negative */

...

>         [mcp3201] = {
>                 .channels = mcp3201_channels,
>                 .num_channels = ARRAY_SIZE(mcp3201_channels),
> +               .convert_rx = mcp3201_convert_rx,

>                 .resolution = 12

+ Comma in such lines.

>         },

-- 
With Best Regards,
Andy Shevchenko