Re: [PATCH 28/57] media: Add ovxxxx_16bit_addr_reg_helpers.h

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Feb 8, 2023 at 6:04 PM Laurent Pinchart
<laurent.pinchart@xxxxxxxxxxxxxxxx> wrote:
> On Wed, Feb 08, 2023 at 05:50:26PM +0200, Andy Shevchenko wrote:
> > On Wed, Feb 8, 2023 at 5:41 PM Laurent Pinchart wrote:
> > > On Wed, Feb 08, 2023 at 01:27:37PM +0200, Andy Shevchenko wrote:
> > > > On Wed, Feb 8, 2023 at 11:52 AM Laurent Pinchart wrote:
> > > > > On Mon, Jan 23, 2023 at 01:51:36PM +0100, Hans de Goede wrote:

...

> > > > > > Add a new ovxxxx_16bit_addr_reg_helpers.h header file with static inline
> > > > > > versions of these register access helpers, so that this code duplication
> > > > > > can be removed.
> > > > >
> > > > > Any reason to hand-roll those instead of using regmap ? Also, may I
> > > > > suggest to have a look at drivers/media/i2c/imx290.c
> > > >
> > > > While this is a bit error prone example, a patch is on its way, ...
> > >
> > > The two cleanups are nice, but they're cleanup, not error fixes :-)
> >
> > It depends on which side you look at it. I admit I haven't dug into
> > the code to see if endianess can be an issue there, but the code
> > itself is not written well, esp. when one offers it as an example. So
> > definitely there is a fix on the upper layer.
>
> Did I miss something ? Your two patches replace a tiny amount of code
> with helper functions that don't change any behaviour. It's nicer with
> those helpers, no question about that, but "not written well" is a bit
> of a stretch and feels quite insulting.

Sorry for your feelings, what I meant is the pure educational purposes
of the example. When one takes the mentioned driver as an example and
uses the code in a slightly different environment the endianess issue
may become a real one. That's why we have helpers in kernel to improve
robustness against blind copy'n'paste approach. It does not mean your
code is broken per se.

> Feel free to submit patches that
> add new "well-written" helpers.



-- 
With Best Regards,
Andy Shevchenko




[Index of Archives]     [Linux Driver Development]     [Linux Driver Backports]     [DMA Engine]     [Linux GPIO]     [Linux SPI]     [Video for Linux]     [Linux USB Devel]     [Linux Coverity]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux