linux-kernel - Re: [PATCH v1 03/10] v4l: platform: Add Renesas CEU driver

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]

Date:   Wed, 15 Nov 2017 15:25:11 +0100
From:   jacopo mondi <jacopo@...ndi.org>
To:     Sakari Ailus <sakari.ailus@....fi>
Cc:     Jacopo Mondi <jacopo+renesas@...ndi.org>,
        laurent.pinchart@...asonboard.com, magnus.damm@...il.com,
        geert@...der.be, mchehab@...nel.org, hverkuil@...all.nl,
        linux-renesas-soc@...r.kernel.org, linux-media@...r.kernel.org,
        linux-sh@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 03/10] v4l: platform: Add Renesas CEU driver

Hi Sakari,
   thanks for review!

On Wed, Nov 15, 2017 at 02:45:51PM +0200, Sakari Ailus wrote:
> Hi Jacopo,
>
> Could you remove the original driver and send the patch using git
> send-email -C ? That way a single patch would address converting it to a
> proper V4L2 driver as well as move it to the correct location. The changes
> would be easier to review that way since then, well, it'd be easier to see
> the changes. :-)

Actually I prefer not to remove the existing driver at the moment. See
the cover letter for reasons why not to do so right now...

Also, there's not that much code from the old driver in here, surely
less than the default 50% -C and -M options of 'git format-patch' use
as a threshold for detecting copies iirc..

I would prefer this to be reviewed as new driver, I know it's a bit
more painful, but irq handler and a couple of other routines apart,
there's not that much code shared between the two...

>
> The same goes for the two V4L2 SoC camera sensor / video decoder drivers at
> the end of the set.
>

Also in this case I prefer not to remove existing code, as long as
there are platforms using it..

> On Wed, Nov 15, 2017 at 11:55:56AM +0100, Jacopo Mondi wrote:
> > Add driver for Renesas Capture Engine Unit (CEU).
> >
> > The CEU interface supports capturing 'data' (YUV422) and 'images'
> > (NV[12|21|16|61]).
> >
> > This driver aims to replace the soc_camera based sh_mobile_ceu one.
> >
> > Tested with ov7670 camera sensor, providing YUYV_2X8 data on Renesas RZ
> > platform GR-Peach.
> >
> > Tested with ov7725 camera sensor on SH4 platform Migo-R.
>
> Nice!
>
> >
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@...ndi.org>
> > ---
> > +#include <linux/completion.h>
>
> Do you need this header? There would seem some that I wouldn't expect to be
> needed below, such as linux/init.h.

It's probably a leftover, I'll remove it...

[snip]
>
> > +#if IS_ENABLED(CONFIG_OF)
> > +static const struct of_device_id ceu_of_match[] = {
> > +	{ .compatible = "renesas,renesas-ceu" },
>
> Even if you add support for new hardware, shouldn't you maintain support
> for renesas,sh-mobile-ceu?
>

As you noticed already, the old driver did not support OF, so there
are no compatibility issues here

Thanks
   j