* [PATCH] media: ov8865: move mode_configure out of state_configure @ 2025-07-22 20:35 Allen Ballway 2025-07-23 15:47 ` Laurent Pinchart 0 siblings, 1 reply; 7+ messages in thread From: Allen Ballway @ 2025-07-22 20:35 UTC (permalink / raw) To: Hans de Goede, Sakari Ailus, Mauro Carvalho Chehab Cc: linux-media, linux-kernel, Allen Ballway ov8865_mode_configure() only needs to be called on sensor init, but it can be called multiple times from ov8865_state_configure(). Move ov8865_mode_configure() to ov8865_sensor_init(). Signed-off-by: Allen Ballway <ballway@chromium.org> --- drivers/media/i2c/ov8865.c | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/drivers/media/i2c/ov8865.c b/drivers/media/i2c/ov8865.c index 95ffe7536aa6aba814f4e5c3d12e7279470b2f07..1d1a1f261bf4ab5c09848402dc057e2f572504e7 100644 --- a/drivers/media/i2c/ov8865.c +++ b/drivers/media/i2c/ov8865.c @@ -2304,14 +2304,6 @@ static int ov8865_state_configure(struct ov8865_sensor *sensor, if (sensor->state.streaming) return -EBUSY; - /* State will be configured at first power on otherwise. */ - if (pm_runtime_enabled(sensor->dev) && - !pm_runtime_suspended(sensor->dev)) { - ret = ov8865_mode_configure(sensor, mode, mbus_code); - if (ret) - return ret; - } - ret = ov8865_state_mipi_configure(sensor, mode, mbus_code); if (ret) return ret; @@ -2384,6 +2376,13 @@ static int ov8865_sensor_init(struct ov8865_sensor *sensor) } /* Configure current mode. */ + ret = ov8865_mode_configure(sensor, sensor->state.mode, + sensor->state.mbus_code); + if (ret) { + dev_err(sensor->dev, "failed to configure mode\n"); + return ret; + } + ret = ov8865_state_configure(sensor, sensor->state.mode, sensor->state.mbus_code); if (ret) { --- base-commit: 6832a9317eee280117cd695fa885b2b7a7a38daf change-id: 20250722-mode_configure-80105fbd835d Best regards, -- Allen Ballway <ballway@chromium.org> ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] media: ov8865: move mode_configure out of state_configure 2025-07-22 20:35 [PATCH] media: ov8865: move mode_configure out of state_configure Allen Ballway @ 2025-07-23 15:47 ` Laurent Pinchart 2025-07-23 16:40 ` Allen Ballway 0 siblings, 1 reply; 7+ messages in thread From: Laurent Pinchart @ 2025-07-23 15:47 UTC (permalink / raw) To: Allen Ballway Cc: Hans de Goede, Sakari Ailus, Mauro Carvalho Chehab, linux-media, linux-kernel On Tue, Jul 22, 2025 at 01:35:43PM -0700, Allen Ballway wrote: > ov8865_mode_configure() only needs to be called on sensor init, but it can > be called multiple times from ov8865_state_configure(). Move > ov8865_mode_configure() to ov8865_sensor_init(). > > Signed-off-by: Allen Ballway <ballway@chromium.org> > --- > drivers/media/i2c/ov8865.c | 15 +++++++-------- > 1 file changed, 7 insertions(+), 8 deletions(-) > > diff --git a/drivers/media/i2c/ov8865.c b/drivers/media/i2c/ov8865.c > index 95ffe7536aa6aba814f4e5c3d12e7279470b2f07..1d1a1f261bf4ab5c09848402dc057e2f572504e7 100644 > --- a/drivers/media/i2c/ov8865.c > +++ b/drivers/media/i2c/ov8865.c > @@ -2304,14 +2304,6 @@ static int ov8865_state_configure(struct ov8865_sensor *sensor, > if (sensor->state.streaming) > return -EBUSY; > > - /* State will be configured at first power on otherwise. */ > - if (pm_runtime_enabled(sensor->dev) && > - !pm_runtime_suspended(sensor->dev)) { > - ret = ov8865_mode_configure(sensor, mode, mbus_code); > - if (ret) > - return ret; > - } > - > ret = ov8865_state_mipi_configure(sensor, mode, mbus_code); > if (ret) > return ret; > @@ -2384,6 +2376,13 @@ static int ov8865_sensor_init(struct ov8865_sensor *sensor) > } > > /* Configure current mode. */ > + ret = ov8865_mode_configure(sensor, sensor->state.mode, > + sensor->state.mbus_code); How about the implication on ov8865_set_fmt() that will not update the link freq and pixel rate controls anymore ? > + if (ret) { > + dev_err(sensor->dev, "failed to configure mode\n"); > + return ret; > + } > + > ret = ov8865_state_configure(sensor, sensor->state.mode, > sensor->state.mbus_code); > if (ret) { > > --- > base-commit: 6832a9317eee280117cd695fa885b2b7a7a38daf > change-id: 20250722-mode_configure-80105fbd835d -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] media: ov8865: move mode_configure out of state_configure 2025-07-23 15:47 ` Laurent Pinchart @ 2025-07-23 16:40 ` Allen Ballway 2025-07-23 17:02 ` Laurent Pinchart 0 siblings, 1 reply; 7+ messages in thread From: Allen Ballway @ 2025-07-23 16:40 UTC (permalink / raw) To: Laurent Pinchart Cc: Hans de Goede, Sakari Ailus, Mauro Carvalho Chehab, linux-media, linux-kernel On Wed, Jul 23, 2025 at 8:47 AM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > On Tue, Jul 22, 2025 at 01:35:43PM -0700, Allen Ballway wrote: > > ov8865_mode_configure() only needs to be called on sensor init, but it can > > be called multiple times from ov8865_state_configure(). Move > > ov8865_mode_configure() to ov8865_sensor_init(). > > > > Signed-off-by: Allen Ballway <ballway@chromium.org> > > --- > > drivers/media/i2c/ov8865.c | 15 +++++++-------- > > 1 file changed, 7 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/media/i2c/ov8865.c b/drivers/media/i2c/ov8865.c > > index 95ffe7536aa6aba814f4e5c3d12e7279470b2f07..1d1a1f261bf4ab5c09848402dc057e2f572504e7 100644 > > --- a/drivers/media/i2c/ov8865.c > > +++ b/drivers/media/i2c/ov8865.c > > @@ -2304,14 +2304,6 @@ static int ov8865_state_configure(struct ov8865_sensor *sensor, > > if (sensor->state.streaming) > > return -EBUSY; > > > > - /* State will be configured at first power on otherwise. */ > > - if (pm_runtime_enabled(sensor->dev) && > > - !pm_runtime_suspended(sensor->dev)) { > > - ret = ov8865_mode_configure(sensor, mode, mbus_code); > > - if (ret) > > - return ret; > > - } > > - > > ret = ov8865_state_mipi_configure(sensor, mode, mbus_code); > > if (ret) > > return ret; > > @@ -2384,6 +2376,13 @@ static int ov8865_sensor_init(struct ov8865_sensor *sensor) > > } > > > > /* Configure current mode. */ > > + ret = ov8865_mode_configure(sensor, sensor->state.mode, > > + sensor->state.mbus_code); > > How about the implication on ov8865_set_fmt() that will not update the > link freq and pixel rate controls anymore ? > I believe those will be unaffected by this change, they are updated in ov8865_state_mipi_configure() which is still called from ov8865_set_fmt() via ov8865_state_configure(). Thanks, Allen > > + if (ret) { > > + dev_err(sensor->dev, "failed to configure mode\n"); > > + return ret; > > + } > > + > > ret = ov8865_state_configure(sensor, sensor->state.mode, > > sensor->state.mbus_code); > > if (ret) { > > > > --- > > base-commit: 6832a9317eee280117cd695fa885b2b7a7a38daf > > change-id: 20250722-mode_configure-80105fbd835d > > -- > Regards, > > Laurent Pinchart ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] media: ov8865: move mode_configure out of state_configure 2025-07-23 16:40 ` Allen Ballway @ 2025-07-23 17:02 ` Laurent Pinchart 2025-08-25 7:54 ` Sakari Ailus 0 siblings, 1 reply; 7+ messages in thread From: Laurent Pinchart @ 2025-07-23 17:02 UTC (permalink / raw) To: Allen Ballway Cc: Hans de Goede, Sakari Ailus, Mauro Carvalho Chehab, linux-media, linux-kernel On Wed, Jul 23, 2025 at 09:40:42AM -0700, Allen Ballway wrote: > On Wed, Jul 23, 2025 at 8:47 AM Laurent Pinchart wrote: > > On Tue, Jul 22, 2025 at 01:35:43PM -0700, Allen Ballway wrote: > > > ov8865_mode_configure() only needs to be called on sensor init, but it can > > > be called multiple times from ov8865_state_configure(). Move > > > ov8865_mode_configure() to ov8865_sensor_init(). > > > > > > Signed-off-by: Allen Ballway <ballway@chromium.org> > > > --- > > > drivers/media/i2c/ov8865.c | 15 +++++++-------- > > > 1 file changed, 7 insertions(+), 8 deletions(-) > > > > > > diff --git a/drivers/media/i2c/ov8865.c b/drivers/media/i2c/ov8865.c > > > index 95ffe7536aa6aba814f4e5c3d12e7279470b2f07..1d1a1f261bf4ab5c09848402dc057e2f572504e7 100644 > > > --- a/drivers/media/i2c/ov8865.c > > > +++ b/drivers/media/i2c/ov8865.c > > > @@ -2304,14 +2304,6 @@ static int ov8865_state_configure(struct ov8865_sensor *sensor, > > > if (sensor->state.streaming) > > > return -EBUSY; > > > > > > - /* State will be configured at first power on otherwise. */ > > > - if (pm_runtime_enabled(sensor->dev) && > > > - !pm_runtime_suspended(sensor->dev)) { > > > - ret = ov8865_mode_configure(sensor, mode, mbus_code); > > > - if (ret) > > > - return ret; > > > - } > > > - > > > ret = ov8865_state_mipi_configure(sensor, mode, mbus_code); > > > if (ret) > > > return ret; > > > @@ -2384,6 +2376,13 @@ static int ov8865_sensor_init(struct ov8865_sensor *sensor) > > > } > > > > > > /* Configure current mode. */ > > > + ret = ov8865_mode_configure(sensor, sensor->state.mode, > > > + sensor->state.mbus_code); > > > > How about the implication on ov8865_set_fmt() that will not update the > > link freq and pixel rate controls anymore ? > > I believe those will be unaffected by this change, they are updated in > ov8865_state_mipi_configure() which is still called from > ov8865_set_fmt() via ov8865_state_configure(). You're right, my bad. > > > + if (ret) { > > > + dev_err(sensor->dev, "failed to configure mode\n"); > > > + return ret; > > > + } > > > + > > > ret = ov8865_state_configure(sensor, sensor->state.mode, > > > sensor->state.mbus_code); Can't we drop this now ? The remaining code in ov8865_state_configure() updates the link frequency and pixel rate controls, and sets sensor->state.mode and sensor->state.mbus_code. The latter is a no-op here as they're set to their current value, and the controls shouldn't need an update in this function as it's only called from ov8865_resume(). > > > if (ret) { > > > > > > --- > > > base-commit: 6832a9317eee280117cd695fa885b2b7a7a38daf > > > change-id: 20250722-mode_configure-80105fbd835d -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] media: ov8865: move mode_configure out of state_configure 2025-07-23 17:02 ` Laurent Pinchart @ 2025-08-25 7:54 ` Sakari Ailus 2025-08-25 15:08 ` Allen Ballway 0 siblings, 1 reply; 7+ messages in thread From: Sakari Ailus @ 2025-08-25 7:54 UTC (permalink / raw) To: Laurent Pinchart Cc: Allen Ballway, Hans de Goede, Mauro Carvalho Chehab, linux-media, linux-kernel Hi Allen, Laurent, On Wed, Jul 23, 2025 at 08:02:37PM +0300, Laurent Pinchart wrote: > On Wed, Jul 23, 2025 at 09:40:42AM -0700, Allen Ballway wrote: > > On Wed, Jul 23, 2025 at 8:47 AM Laurent Pinchart wrote: > > > On Tue, Jul 22, 2025 at 01:35:43PM -0700, Allen Ballway wrote: > > > > ov8865_mode_configure() only needs to be called on sensor init, but it can > > > > be called multiple times from ov8865_state_configure(). Move > > > > ov8865_mode_configure() to ov8865_sensor_init(). > > > > > > > > Signed-off-by: Allen Ballway <ballway@chromium.org> > > > > --- > > > > drivers/media/i2c/ov8865.c | 15 +++++++-------- > > > > 1 file changed, 7 insertions(+), 8 deletions(-) > > > > > > > > diff --git a/drivers/media/i2c/ov8865.c b/drivers/media/i2c/ov8865.c > > > > index 95ffe7536aa6aba814f4e5c3d12e7279470b2f07..1d1a1f261bf4ab5c09848402dc057e2f572504e7 100644 > > > > --- a/drivers/media/i2c/ov8865.c > > > > +++ b/drivers/media/i2c/ov8865.c > > > > @@ -2304,14 +2304,6 @@ static int ov8865_state_configure(struct ov8865_sensor *sensor, > > > > if (sensor->state.streaming) > > > > return -EBUSY; > > > > > > > > - /* State will be configured at first power on otherwise. */ > > > > - if (pm_runtime_enabled(sensor->dev) && > > > > - !pm_runtime_suspended(sensor->dev)) { > > > > - ret = ov8865_mode_configure(sensor, mode, mbus_code); > > > > - if (ret) > > > > - return ret; > > > > - } > > > > - > > > > ret = ov8865_state_mipi_configure(sensor, mode, mbus_code); > > > > if (ret) > > > > return ret; > > > > @@ -2384,6 +2376,13 @@ static int ov8865_sensor_init(struct ov8865_sensor *sensor) > > > > } > > > > > > > > /* Configure current mode. */ > > > > + ret = ov8865_mode_configure(sensor, sensor->state.mode, > > > > + sensor->state.mbus_code); > > > > > > How about the implication on ov8865_set_fmt() that will not update the > > > link freq and pixel rate controls anymore ? > > > > I believe those will be unaffected by this change, they are updated in > > ov8865_state_mipi_configure() which is still called from > > ov8865_set_fmt() via ov8865_state_configure(). > > You're right, my bad. > > > > > + if (ret) { > > > > + dev_err(sensor->dev, "failed to configure mode\n"); > > > > + return ret; > > > > + } > > > > + > > > > ret = ov8865_state_configure(sensor, sensor->state.mode, > > > > sensor->state.mbus_code); > > Can't we drop this now ? The remaining code in ov8865_state_configure() > updates the link frequency and pixel rate controls, and sets > sensor->state.mode and sensor->state.mbus_code. The latter is a no-op > here as they're set to their current value, and the controls shouldn't > need an update in this function as it's only called from > ov8865_resume(). Any comments on this? Will there be v2? -- Sakari Ailus ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] media: ov8865: move mode_configure out of state_configure 2025-08-25 7:54 ` Sakari Ailus @ 2025-08-25 15:08 ` Allen Ballway 2025-08-25 20:13 ` Sakari Ailus 0 siblings, 1 reply; 7+ messages in thread From: Allen Ballway @ 2025-08-25 15:08 UTC (permalink / raw) To: Sakari Ailus Cc: Laurent Pinchart, Hans de Goede, Mauro Carvalho Chehab, linux-media, linux-kernel Hi Sakari, I sent out a v2 some time ago (https://lore.kernel.org/all/20250723-mode_configure-v2-1-7fb0f6ba1194@chromium.org). It seems I used b4 incorrectly and it didn't add to this chain as I expected, apologies for the confusion. Thanks, Allen On Mon, Aug 25, 2025 at 12:55 AM Sakari Ailus <sakari.ailus@linux.intel.com> wrote: > > Hi Allen, Laurent, > > On Wed, Jul 23, 2025 at 08:02:37PM +0300, Laurent Pinchart wrote: > > On Wed, Jul 23, 2025 at 09:40:42AM -0700, Allen Ballway wrote: > > > On Wed, Jul 23, 2025 at 8:47 AM Laurent Pinchart wrote: > > > > On Tue, Jul 22, 2025 at 01:35:43PM -0700, Allen Ballway wrote: > > > > > ov8865_mode_configure() only needs to be called on sensor init, but it can > > > > > be called multiple times from ov8865_state_configure(). Move > > > > > ov8865_mode_configure() to ov8865_sensor_init(). > > > > > > > > > > Signed-off-by: Allen Ballway <ballway@chromium.org> > > > > > --- > > > > > drivers/media/i2c/ov8865.c | 15 +++++++-------- > > > > > 1 file changed, 7 insertions(+), 8 deletions(-) > > > > > > > > > > diff --git a/drivers/media/i2c/ov8865.c b/drivers/media/i2c/ov8865.c > > > > > index 95ffe7536aa6aba814f4e5c3d12e7279470b2f07..1d1a1f261bf4ab5c09848402dc057e2f572504e7 100644 > > > > > --- a/drivers/media/i2c/ov8865.c > > > > > +++ b/drivers/media/i2c/ov8865.c > > > > > @@ -2304,14 +2304,6 @@ static int ov8865_state_configure(struct ov8865_sensor *sensor, > > > > > if (sensor->state.streaming) > > > > > return -EBUSY; > > > > > > > > > > - /* State will be configured at first power on otherwise. */ > > > > > - if (pm_runtime_enabled(sensor->dev) && > > > > > - !pm_runtime_suspended(sensor->dev)) { > > > > > - ret = ov8865_mode_configure(sensor, mode, mbus_code); > > > > > - if (ret) > > > > > - return ret; > > > > > - } > > > > > - > > > > > ret = ov8865_state_mipi_configure(sensor, mode, mbus_code); > > > > > if (ret) > > > > > return ret; > > > > > @@ -2384,6 +2376,13 @@ static int ov8865_sensor_init(struct ov8865_sensor *sensor) > > > > > } > > > > > > > > > > /* Configure current mode. */ > > > > > + ret = ov8865_mode_configure(sensor, sensor->state.mode, > > > > > + sensor->state.mbus_code); > > > > > > > > How about the implication on ov8865_set_fmt() that will not update the > > > > link freq and pixel rate controls anymore ? > > > > > > I believe those will be unaffected by this change, they are updated in > > > ov8865_state_mipi_configure() which is still called from > > > ov8865_set_fmt() via ov8865_state_configure(). > > > > You're right, my bad. > > > > > > > + if (ret) { > > > > > + dev_err(sensor->dev, "failed to configure mode\n"); > > > > > + return ret; > > > > > + } > > > > > + > > > > > ret = ov8865_state_configure(sensor, sensor->state.mode, > > > > > sensor->state.mbus_code); > > > > Can't we drop this now ? The remaining code in ov8865_state_configure() > > updates the link frequency and pixel rate controls, and sets > > sensor->state.mode and sensor->state.mbus_code. The latter is a no-op > > here as they're set to their current value, and the controls shouldn't > > need an update in this function as it's only called from > > ov8865_resume(). > > Any comments on this? Will there be v2? > > -- > Sakari Ailus ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] media: ov8865: move mode_configure out of state_configure 2025-08-25 15:08 ` Allen Ballway @ 2025-08-25 20:13 ` Sakari Ailus 0 siblings, 0 replies; 7+ messages in thread From: Sakari Ailus @ 2025-08-25 20:13 UTC (permalink / raw) To: Allen Ballway Cc: Laurent Pinchart, Hans de Goede, Mauro Carvalho Chehab, linux-media, linux-kernel Hi Allen, On Mon, Aug 25, 2025 at 08:08:09AM -0700, Allen Ballway wrote: > Hi Sakari, > > I sent out a v2 some time ago > (https://lore.kernel.org/all/20250723-mode_configure-v2-1-7fb0f6ba1194@chromium.org). > It seems I used b4 incorrectly and it didn't add to this chain as I > expected, apologies for the confusion. Oops, I had missed v2. Posting it separately from previous versions is in fact preferred. -- Regards, Sakari Ailus ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2025-08-25 20:13 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-07-22 20:35 [PATCH] media: ov8865: move mode_configure out of state_configure Allen Ballway 2025-07-23 15:47 ` Laurent Pinchart 2025-07-23 16:40 ` Allen Ballway 2025-07-23 17:02 ` Laurent Pinchart 2025-08-25 7:54 ` Sakari Ailus 2025-08-25 15:08 ` Allen Ballway 2025-08-25 20:13 ` Sakari Ailus
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).