Received-SPF: pass (google.com: domain of linux-kernel+bounces-65412-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Wed, 14 Feb 2024 16:24:24 +0100
From: Tommaso Merciai <tomm.merciai@gmail.com>
To: Sakari Ailus <sakari.ailus@linux.intel.com>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
	linuxfancy@googlegroups.com, martin.hecht@avnet.eu,
	Mauro Carvalho Chehab <mchehab@kernel.org>,
	linux-media@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 5/5] media: i2c: alvium: fix req_fr check into
 alvium_s_frame_interval()
Message-ID: <ZczbKDGXF1PXaBXr@tom-HP-ZBook-Fury-15-G7-Mobile-Workstation>
References: <20231220124023.2801417-1-tomm.merciai@gmail.com>
 <20231220124023.2801417-6-tomm.merciai@gmail.com>
 <20231220130236.GN29638@pendragon.ideasonboard.com>
 <ZYLxtTRQF0sWJLiu@tom-HP-ZBook-Fury-15-G7-Mobile-Workstation>
 <ZcygmyT1ZaKsbSxX@kekkonen.localdomain>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ZcygmyT1ZaKsbSxX@kekkonen.localdomain>

Hi Sakari,
Sorry for delay.

On Wed, Feb 14, 2024 at 11:14:35AM +0000, Sakari Ailus wrote:
> Hi Tommaso,
> 
> On Wed, Dec 20, 2023 at 02:52:53PM +0100, Tommaso Merciai wrote:
> > Hi Laurent,
> > 
> > On Wed, Dec 20, 2023 at 03:02:36PM +0200, Laurent Pinchart wrote:
> > > Hi Tommaso,
> > > 
> > > Thank you for the patch.
> > > 
> > > On Wed, Dec 20, 2023 at 01:40:23PM +0100, Tommaso Merciai wrote:
> > > > Actually req_fr check into alvium_s_frame_interval() is wrong.
> > > > In particular req_fr can't be >=max and <= min at the same time.
> > > > Fix this using clamp and remove dft_fr parameter from
> > > > alvium_get_frame_interval() not more used.
> > > 
> > > The commit message should have explained why clamping is better than
> > > picking a default value, as that's a functional change. If you propose
> > > an updated commit message in a reply, I think Sakari can update the
> > > patch when applying the series to his tree, there's no need for a v4.
> > 
> > What about:
> > 
> > Actually req_fr check into alvium_s_frame_interval() is wrong.
> > In particular req_fr can't be >=max and <= min at the same time.
> > Fix this using clamp and remove dft_fr parameter from
> > alvium_get_frame_interval() not more used.
> > 
> > Clamp function make sure that if the setted value exceeds the limits is
> > replaced with min_fr/max_fr instead of setting the value readed back
> > from the hw.
> > 
> > What do you think?
> 
> I used this, hopefully it's ok:
> 
> media: i2c: alvium: fix req_fr check in alvium_s_frame_interval()
> 
> req_fr check in alvium_s_frame_interval() is incorrect. In particular
> req_fr can't be >=max and <= min at the same time. Ensure the requested
> frame rate remains within the supported range between min_fr and max_fr by
> clamping it.
> 
> Also remove the unused dft_fr argument of alvium_get_frame_interval().

Looks good to me! :)
Thanks for this.

Regards,
Tommaso


> 
> -- 
> Kind regards,
> 
> Sakari Ailus