From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [213.167.242.64]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id B408742A7A2 for ; Fri, 17 Jul 2026 14:21:25 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=213.167.242.64 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1784298087; cv=none; b=m0GyUyWBM4TIYn7mQ6e3fbfk5WvXUHjeCKHFQlCphqc8AQLUyrJNiYNgbTrx12CwskjPWmpInyt+PFQ5x9nnb6gHnZQOiYFCkU2waznVtU+ga+c0Hd7yBjCHqVxLwvpXwbZCmVqiCSnd38fNFngW5GJF+NhOx0cD9ybPjmfYAQY= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1784298087; c=relaxed/simple; bh=Jt/Bq+FGCDxrc6u1xj55uXHKPEL2jbfC1lIcsU8QY8Y=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=srEQVD7CKaLhRFBB+OqYzg8IN24w3G0ejJf3hJVj/UFdTm6mhs/+Sm4kSZRLN7hIykJSuEMHRpNjoKedBAIP7StaYGmp5oU6TDDzQ4rBKdFzxF32oz7aC1/dXx5LyZbFEGiU7/L8yJQ65zVETkfvoFrGGjYv1+K9cCyMz+9kijo= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=ideasonboard.com; spf=pass smtp.mailfrom=ideasonboard.com; dkim=pass (1024-bit key) header.d=ideasonboard.com header.i=@ideasonboard.com header.b=iwvPx9PW; arc=none smtp.client-ip=213.167.242.64 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=ideasonboard.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=ideasonboard.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=ideasonboard.com header.i=@ideasonboard.com header.b="iwvPx9PW" Received: from killaraus.ideasonboard.com (2001-14ba-70f3-e800--a06.rev.dnainternet.fi [IPv6:2001:14ba:70f3:e800::a06]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id 00CE7267; Fri, 17 Jul 2026 16:20:26 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1784298026; bh=Jt/Bq+FGCDxrc6u1xj55uXHKPEL2jbfC1lIcsU8QY8Y=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=iwvPx9PW6GAMo0EFW399TnDwbqTZhsNRb7pFope9mB/a0C5i3INpri2jEEOKurVBN 9iDWlodYzfz8zCNCEC20+DLvyI+WIcFzIKCyCYiitTf3xLwva80bVLcFbAoBNlhWyf esMycY1mxj6E8X9OTJdk+oOuU9qmF/ZeJVvHeNMo= Date: Fri, 17 Jul 2026 17:21:21 +0300 From: Laurent Pinchart To: Sakari Ailus Cc: linux-media@vger.kernel.org, hans@jjverkuil.nl, Prabhakar , Kate Hsuan , Dave Stevenson , Tommaso Merciai , Benjamin Mugnier , Sylvain Petinot , Christophe JAILLET , Julien Massot , Naushir Patuck , "Yan, Dongcheng" , Stefan Klug , Mirela Rabulea , =?utf-8?B?QW5kcsOp?= Apitzsch , Heimir Thor Sverrisson , Kieran Bingham , Mehdi Djait , Ricardo Ribalda Delgado , Hans de Goede , Jacopo Mondi , Tomi Valkeinen , David Plowman , "Yu, Ong Hock" , "Ng, Khai Wen" , Jai Luthra , Rishikesh Donadkar Subject: Re: [PATCH v6 03/16] media: imx219: Account for rate_factor in control steps Message-ID: <20260717142121.GC1889304@killaraus.ideasonboard.com> References: <20260607215356.842932-1-sakari.ailus@linux.intel.com> <20260701122634.1728782-3-sakari.ailus@linux.intel.com> Precedence: bulk X-Mailing-List: linux-media@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20260701122634.1728782-3-sakari.ailus@linux.intel.com> Hi Sakari, Thank you for the patch. On Wed, Jul 01, 2026 at 03:26:20PM +0300, Sakari Ailus wrote: > The controls that are divided by the rate_factor before writing them to > the registers have the step of the value of the rate_factor. Take this > into account when the control's range is modified. The controls are > created in a configuration where rate_factor is always 1, hence there's no > need to change the code adding new controls. > > Fixes: f513997119f4 ("media: i2c: imx219: Scale the pixel rate for analog binning") > Cc: stable@vger.kernel.org > Signed-off-by: Sakari Ailus > --- > drivers/media/i2c/imx219.c | 18 ++++++++---------- > 1 file changed, 8 insertions(+), 10 deletions(-) > > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c > index 05d9737bdc95..2aab6e7180d4 100644 > --- a/drivers/media/i2c/imx219.c > +++ b/drivers/media/i2c/imx219.c > @@ -319,19 +319,19 @@ static const struct imx219_mode supported_modes[] = { > /* 1080P 30fps cropped */ > .width = 1920, > .height = 1080, > - .fll_def = 1763, > + .fll_def = 1762, This is not a binned mode, an odd default value is not incorrect. Is there a reason to change it ? > }, > { > /* 2x2 binned 60fps mode */ > .width = 1640, > .height = 1232, > - .fll_def = 1707, > + .fll_def = 1706, > }, > { > /* 640x480 60fps mode */ > .width = 640, > .height = 480, > - .fll_def = 1707, > + .fll_def = 1706, Those two are fine as they won't (AFAIU) change register values. > }, > }; > > @@ -458,8 +458,7 @@ static int imx219_set_ctrl(struct v4l2_ctrl *ctrl) > ret = __v4l2_ctrl_modify_range(imx219->exposure, > imx219->exposure->minimum, > exposure_max, > - imx219->exposure->step, > - exposure_def); > + rate_factor, exposure_def); The change is simple so I'm fine with it, but in general I'm not sure this would be worth it if it were to make the code more complex. Userspace can benefit from getting a precise step value, as well as seeing the control value being adjusted when setting controls. However, dealing with limit updates reported through events is quite cumbersome for userspace. It's a big change and not a high priority, but I'd like an API where ioctls such as VIDIOC_SUBDEV_S_FMT would have the ability to report control changes synchronously. With the change to the first fll_def possibly dropped, Reviewed-by: Laurent Pinchart > if (ret) > return ret; > > @@ -887,7 +886,8 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd, > > /* Update limits and set FPS to default */ > ret = __v4l2_ctrl_modify_range(imx219->vblank, IMX219_VBLANK_MIN, > - IMX219_FLL_MAX - mode->height, 1, > + IMX219_FLL_MAX - mode->height, > + rate_factor, > mode->fll_def - mode->height); > if (ret) > return ret; > @@ -905,8 +905,7 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd, > ret = __v4l2_ctrl_modify_range(imx219->exposure, > imx219->exposure->minimum, > exposure_max, > - imx219->exposure->step, > - exposure_def); > + rate_factor, exposure_def); > if (ret) > return ret; > > @@ -931,8 +930,7 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd, > return ret; > > /* Scale the pixel rate based on the mode specific factor */ > - pixel_rate = imx219_get_pixel_rate(imx219) * > - imx219_get_rate_factor(state); > + pixel_rate = imx219_get_pixel_rate(imx219) * rate_factor; > ret = __v4l2_ctrl_modify_range(imx219->pixel_rate, pixel_rate, > pixel_rate, 1, pixel_rate); > if (ret) -- Regards, Laurent Pinchart