Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Sun, 4 Sep 2022 17:15:59 +0100
From:   Jonathan Cameron <jic23@kernel.org>
To:     Andy Shevchenko <andy.shevchenko@gmail.com>
Cc:     Vincent Whitchurch <vincent.whitchurch@axis.com>,
        kernel <kernel@axis.com>, Lars-Peter Clausen <lars@metafoo.de>,
        linux-iio <linux-iio@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2 1/5] iio: adc: mcp320x: use callbacks for RX
 conversion
Message-ID: <20220904171559.1bf77d42@jic23-huawei>
In-Reply-To: <CAHp75Vek8tLnwYWZO91HHr1_i7G_F9tErnRsq-GwfQcpR0FaTw@mail.gmail.com>
References: <20220831100506.3368103-1-vincent.whitchurch@axis.com>
        <20220831100506.3368103-2-vincent.whitchurch@axis.com>
        <CAHp75Vek8tLnwYWZO91HHr1_i7G_F9tErnRsq-GwfQcpR0FaTw@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Wed, 31 Aug 2022 15:40:49 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Wed, Aug 31, 2022 at 1:05 PM Vincent Whitchurch
> <vincent.whitchurch@axis.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) */;

This made me look a bit closer at the code.  
rx_buf isn't necessarily aligned...
This may be a good usecase for an anonymous union.

Which would be fine except one of the callbacks (and original
code) uses be32_to_cpup() which is not unaligned safe.

I'm not keen to push unrelated work onto someone doing good stuff
on a driver, but in this particular case it does seem reasonable to
tidy all this up given you are moving the code anyway.

Jonathan

> 
> ...
> 
> > +       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.
> 
> >         },  
>