linux-kernel - Re: [Patch v2 3/3] media: ov5640: Make 2592x1944 mode only available at 15 fps

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: <20191003120902.wd3w2qmiry5bpa4h@ti.com>
Date:   Thu, 3 Oct 2019 07:09:02 -0500
From:   Benoit Parrot <bparrot@...com>
To:     Jacopo Mondi <jacopo@...ndi.org>
CC:     Hans Verkuil <hverkuil@...all.nl>,
        Sakari Ailus <sakari.ailus@...ux.intel.com>,
        <linux-media@...r.kernel.org>, <devicetree@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [Patch v2 3/3] media: ov5640: Make 2592x1944 mode only available
 at 15 fps

Jacopo Mondi <jacopo@...ndi.org> wrote on Thu [2019-Oct-03 09:31:55 +0200]:
> Hi Benoit,
> 
> On Wed, Oct 02, 2019 at 08:51:34AM -0500, Benoit Parrot wrote:
> > The sensor data sheet clearly state that 2592x1944 only works at 15 fps
> > make sure we don't try to miss configure the pll out of acceptable
> > range.
> 
> The datasheet clearly indicates that 15 fps is the maximum achievable
> rate with that resolution, so I guess preventing it from being set
> to anything faster than that it's acceptable.
> >
> > Signed-off-by: Benoit Parrot <bparrot@...com>
> > ---
> >  drivers/media/i2c/ov5640.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> > index 103a4e8f88e1..d5b0be2c7a0a 100644
> > --- a/drivers/media/i2c/ov5640.c
> > +++ b/drivers/media/i2c/ov5640.c
> > @@ -1613,6 +1613,11 @@ ov5640_find_mode(struct ov5640_dev *sensor, enum ov5640_frame_rate fr,
> >  	    !(mode->hact == 640 && mode->vact == 480))
> >  		return NULL;
> >
> > +	/* 2592x1944 only works at 15fps */
> > +	if (fr != OV5640_15_FPS &&
> 
> As long as 15 fps is the lower framerate declared in
> ov5640_framerates[] this is ok, but I would make this condition a
> check for "fr  > OV5640_15_FPS" so that it's safe for future
> extensions.
> 
> (And I would check for the resolution first then FPS, so you check
> the most unlikely condition first, but that's really a minor
> optimization).

Ah, very good I'll change that.

Benoit

> 
> With the above small details addressed
> Reviewed-by: Jacopo Mondi <jacopo@...ndi.org>
> 
> Thanks
>    j
> 
> > +	    (mode->hact == 2592 && mode->vact == 1944))
> > +		return NULL;
> > +
> >  	return mode;
> >  }
> >
> > --
> > 2.17.1
> >