* [PATCH 1/1] microchip-csi2dc: Remove VC support for now @ 2022-02-02 15:36 Sakari Ailus 2022-02-08 5:11 ` Mauro Carvalho Chehab 0 siblings, 1 reply; 12+ messages in thread From: Sakari Ailus @ 2022-02-02 15:36 UTC (permalink / raw) To: linux-media; +Cc: laurent.pinchart As part of removing mbus config flags, remove VC flag use in the microchip-csi2dc driver. The support can be reintroduced later on as part of the streams patches. Cc: Eugen Hristev <eugen.hristev@microchip.com> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> --- .../media/platform/atmel/microchip-csi2dc.c | 18 ++---------------- 1 file changed, 2 insertions(+), 16 deletions(-) diff --git a/drivers/media/platform/atmel/microchip-csi2dc.c b/drivers/media/platform/atmel/microchip-csi2dc.c index 6bc549c28e05..6a7f5b4b0e3b 100644 --- a/drivers/media/platform/atmel/microchip-csi2dc.c +++ b/drivers/media/platform/atmel/microchip-csi2dc.c @@ -348,24 +348,15 @@ static int csi2dc_get_mbus_config(struct csi2dc_device *csi2dc) if (ret == -ENOIOCTLCMD) { dev_dbg(csi2dc->dev, "no remote mbus configuration available\n"); - goto csi2dc_get_mbus_config_defaults; + return 0; } if (ret) { dev_err(csi2dc->dev, "failed to get remote mbus configuration\n"); - goto csi2dc_get_mbus_config_defaults; + return 0; } - if (mbus_config.flags & V4L2_MBUS_CSI2_CHANNEL_0) - csi2dc->vc = 0; - else if (mbus_config.flags & V4L2_MBUS_CSI2_CHANNEL_1) - csi2dc->vc = 1; - else if (mbus_config.flags & V4L2_MBUS_CSI2_CHANNEL_2) - csi2dc->vc = 2; - else if (mbus_config.flags & V4L2_MBUS_CSI2_CHANNEL_3) - csi2dc->vc = 3; - dev_dbg(csi2dc->dev, "subdev sending on channel %d\n", csi2dc->vc); csi2dc->clk_gated = mbus_config.flags & @@ -375,11 +366,6 @@ static int csi2dc_get_mbus_config(struct csi2dc_device *csi2dc) csi2dc->clk_gated ? "gated" : "free running"); return 0; - -csi2dc_get_mbus_config_defaults: - csi2dc->vc = 0; /* Virtual ID 0 by default */ - - return 0; } static void csi2dc_vp_update(struct csi2dc_device *csi2dc) -- 2.30.2 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 1/1] microchip-csi2dc: Remove VC support for now 2022-02-02 15:36 [PATCH 1/1] microchip-csi2dc: Remove VC support for now Sakari Ailus @ 2022-02-08 5:11 ` Mauro Carvalho Chehab 2022-02-08 8:00 ` Eugen.Hristev 0 siblings, 1 reply; 12+ messages in thread From: Mauro Carvalho Chehab @ 2022-02-08 5:11 UTC (permalink / raw) To: Sakari Ailus; +Cc: linux-media, laurent.pinchart, Eugen Hristev Em Wed, 2 Feb 2022 17:36:09 +0200 Sakari Ailus <sakari.ailus@linux.intel.com> escreveu: > As part of removing mbus config flags, remove VC flag use in the > microchip-csi2dc driver. The support can be reintroduced later on as part > of the streams patches. > > Cc: Eugen Hristev <eugen.hristev@microchip.com> Hmm... that sounds a regression to me. What effects this will cause at the driver? Eugen, any comments? > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > --- > .../media/platform/atmel/microchip-csi2dc.c | 18 ++---------------- > 1 file changed, 2 insertions(+), 16 deletions(-) > > diff --git a/drivers/media/platform/atmel/microchip-csi2dc.c b/drivers/media/platform/atmel/microchip-csi2dc.c > index 6bc549c28e05..6a7f5b4b0e3b 100644 > --- a/drivers/media/platform/atmel/microchip-csi2dc.c > +++ b/drivers/media/platform/atmel/microchip-csi2dc.c > @@ -348,24 +348,15 @@ static int csi2dc_get_mbus_config(struct csi2dc_device *csi2dc) > if (ret == -ENOIOCTLCMD) { > dev_dbg(csi2dc->dev, > "no remote mbus configuration available\n"); > - goto csi2dc_get_mbus_config_defaults; > + return 0; > } > > if (ret) { > dev_err(csi2dc->dev, > "failed to get remote mbus configuration\n"); > - goto csi2dc_get_mbus_config_defaults; > + return 0; > } > > - if (mbus_config.flags & V4L2_MBUS_CSI2_CHANNEL_0) > - csi2dc->vc = 0; > - else if (mbus_config.flags & V4L2_MBUS_CSI2_CHANNEL_1) > - csi2dc->vc = 1; > - else if (mbus_config.flags & V4L2_MBUS_CSI2_CHANNEL_2) > - csi2dc->vc = 2; > - else if (mbus_config.flags & V4L2_MBUS_CSI2_CHANNEL_3) > - csi2dc->vc = 3; > - > dev_dbg(csi2dc->dev, "subdev sending on channel %d\n", csi2dc->vc); > > csi2dc->clk_gated = mbus_config.flags & > @@ -375,11 +366,6 @@ static int csi2dc_get_mbus_config(struct csi2dc_device *csi2dc) > csi2dc->clk_gated ? "gated" : "free running"); > > return 0; > - > -csi2dc_get_mbus_config_defaults: > - csi2dc->vc = 0; /* Virtual ID 0 by default */ > - > - return 0; > } > > static void csi2dc_vp_update(struct csi2dc_device *csi2dc) ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/1] microchip-csi2dc: Remove VC support for now 2022-02-08 5:11 ` Mauro Carvalho Chehab @ 2022-02-08 8:00 ` Eugen.Hristev 2022-02-08 8:05 ` Eugen.Hristev 2022-02-08 11:06 ` Laurent Pinchart 0 siblings, 2 replies; 12+ messages in thread From: Eugen.Hristev @ 2022-02-08 8:00 UTC (permalink / raw) To: mchehab, sakari.ailus; +Cc: linux-media, laurent.pinchart On 2/8/22 7:11 AM, Mauro Carvalho Chehab wrote: > Em Wed, 2 Feb 2022 17:36:09 +0200 > Sakari Ailus <sakari.ailus@linux.intel.com> escreveu: > >> As part of removing mbus config flags, remove VC flag use in the >> microchip-csi2dc driver. The support can be reintroduced later on as part >> of the streams patches. >> >> Cc: Eugen Hristev <eugen.hristev@microchip.com> > > Hmm... that sounds a regression to me. What effects this will cause at > the driver? > > Eugen, any comments? Hi , I am not happy with this change. It looks like I wasn't even CC-ed on the original patch e-mail. The effect on the driver will be that everything will be treated as virtual channel=0 . I do not yet understand why we are about to remove V4L2_MBUS_CSI2_CHANNEL_* as I remember this was just introduced. Is there any alternative in place ? My opinion is that if we want to replace something existing with a new API or something else, we should first add the new support, block any new adopters for the old API such that everyone uses the new API, and only after that convert the old API clients to the new API. So 'can be reintroduced later on' is not okay. We can't remove things in the hope that it would be reintroduced later. Just my personal take on this, feel free to have a different opinion. In the end you guys are the maintainers for the subsystem and can have this change if you like, I am more unhappy about the fact that changes happen suddenly and without notice. Eugen > >> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> >> --- >> .../media/platform/atmel/microchip-csi2dc.c | 18 ++---------------- >> 1 file changed, 2 insertions(+), 16 deletions(-) >> >> diff --git a/drivers/media/platform/atmel/microchip-csi2dc.c b/drivers/media/platform/atmel/microchip-csi2dc.c >> index 6bc549c28e05..6a7f5b4b0e3b 100644 >> --- a/drivers/media/platform/atmel/microchip-csi2dc.c >> +++ b/drivers/media/platform/atmel/microchip-csi2dc.c >> @@ -348,24 +348,15 @@ static int csi2dc_get_mbus_config(struct csi2dc_device *csi2dc) >> if (ret == -ENOIOCTLCMD) { >> dev_dbg(csi2dc->dev, >> "no remote mbus configuration available\n"); >> - goto csi2dc_get_mbus_config_defaults; >> + return 0; >> } >> >> if (ret) { >> dev_err(csi2dc->dev, >> "failed to get remote mbus configuration\n"); >> - goto csi2dc_get_mbus_config_defaults; >> + return 0; >> } >> >> - if (mbus_config.flags & V4L2_MBUS_CSI2_CHANNEL_0) >> - csi2dc->vc = 0; >> - else if (mbus_config.flags & V4L2_MBUS_CSI2_CHANNEL_1) >> - csi2dc->vc = 1; >> - else if (mbus_config.flags & V4L2_MBUS_CSI2_CHANNEL_2) >> - csi2dc->vc = 2; >> - else if (mbus_config.flags & V4L2_MBUS_CSI2_CHANNEL_3) >> - csi2dc->vc = 3; >> - >> dev_dbg(csi2dc->dev, "subdev sending on channel %d\n", csi2dc->vc); >> >> csi2dc->clk_gated = mbus_config.flags & >> @@ -375,11 +366,6 @@ static int csi2dc_get_mbus_config(struct csi2dc_device *csi2dc) >> csi2dc->clk_gated ? "gated" : "free running"); >> >> return 0; >> - >> -csi2dc_get_mbus_config_defaults: >> - csi2dc->vc = 0; /* Virtual ID 0 by default */ >> - >> - return 0; >> } >> >> static void csi2dc_vp_update(struct csi2dc_device *csi2dc) ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/1] microchip-csi2dc: Remove VC support for now 2022-02-08 8:00 ` Eugen.Hristev @ 2022-02-08 8:05 ` Eugen.Hristev 2022-02-08 11:06 ` Laurent Pinchart 1 sibling, 0 replies; 12+ messages in thread From: Eugen.Hristev @ 2022-02-08 8:05 UTC (permalink / raw) To: mchehab, sakari.ailus; +Cc: linux-media, laurent.pinchart On 2/8/22 10:00 AM, Eugen Hristev - M18282 wrote: > On 2/8/22 7:11 AM, Mauro Carvalho Chehab wrote: >> Em Wed, 2 Feb 2022 17:36:09 +0200 >> Sakari Ailus <sakari.ailus@linux.intel.com> escreveu: >> >>> As part of removing mbus config flags, remove VC flag use in the >>> microchip-csi2dc driver. The support can be reintroduced later on as part >>> of the streams patches. >>> >>> Cc: Eugen Hristev <eugen.hristev@microchip.com> Having a closer look, I am added here to the Cc: list but your git send-email did not send it to me. Perhaps you have a CC-suppress activated. >> >> Hmm... that sounds a regression to me. What effects this will cause at >> the driver? >> >> Eugen, any comments? > > Hi , > > I am not happy with this change. It looks like I wasn't even CC-ed on > the original patch e-mail. > > The effect on the driver will be that everything will be treated as > virtual channel=0 . > I do not yet understand why we are about to remove > V4L2_MBUS_CSI2_CHANNEL_* as I remember this was just introduced. > Is there any alternative in place ? > > My opinion is that if we want to replace something existing with a new > API or something else, we should first add the new support, block any > new adopters for the old API such that everyone uses the new API, and > only after that convert the old API clients to the new API. > So 'can be reintroduced later on' is not okay. We can't remove things in > the hope that it would be reintroduced later. Just my personal take on > this, feel free to have a different opinion. > > In the end you guys are the maintainers for the subsystem and can have > this change if you like, I am more unhappy about the fact that changes > happen suddenly and without notice. > > Eugen > >> >>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> >>> --- >>> .../media/platform/atmel/microchip-csi2dc.c | 18 ++---------------- >>> 1 file changed, 2 insertions(+), 16 deletions(-) >>> >>> diff --git a/drivers/media/platform/atmel/microchip-csi2dc.c b/drivers/media/platform/atmel/microchip-csi2dc.c >>> index 6bc549c28e05..6a7f5b4b0e3b 100644 >>> --- a/drivers/media/platform/atmel/microchip-csi2dc.c >>> +++ b/drivers/media/platform/atmel/microchip-csi2dc.c >>> @@ -348,24 +348,15 @@ static int csi2dc_get_mbus_config(struct csi2dc_device *csi2dc) >>> if (ret == -ENOIOCTLCMD) { >>> dev_dbg(csi2dc->dev, >>> "no remote mbus configuration available\n"); >>> - goto csi2dc_get_mbus_config_defaults; >>> + return 0; >>> } >>> >>> if (ret) { >>> dev_err(csi2dc->dev, >>> "failed to get remote mbus configuration\n"); >>> - goto csi2dc_get_mbus_config_defaults; >>> + return 0; >>> } >>> >>> - if (mbus_config.flags & V4L2_MBUS_CSI2_CHANNEL_0) >>> - csi2dc->vc = 0; >>> - else if (mbus_config.flags & V4L2_MBUS_CSI2_CHANNEL_1) >>> - csi2dc->vc = 1; >>> - else if (mbus_config.flags & V4L2_MBUS_CSI2_CHANNEL_2) >>> - csi2dc->vc = 2; >>> - else if (mbus_config.flags & V4L2_MBUS_CSI2_CHANNEL_3) >>> - csi2dc->vc = 3; >>> - >>> dev_dbg(csi2dc->dev, "subdev sending on channel %d\n", csi2dc->vc); >>> >>> csi2dc->clk_gated = mbus_config.flags & >>> @@ -375,11 +366,6 @@ static int csi2dc_get_mbus_config(struct csi2dc_device *csi2dc) >>> csi2dc->clk_gated ? "gated" : "free running"); >>> >>> return 0; >>> - >>> -csi2dc_get_mbus_config_defaults: >>> - csi2dc->vc = 0; /* Virtual ID 0 by default */ >>> - >>> - return 0; >>> } >>> >>> static void csi2dc_vp_update(struct csi2dc_device *csi2dc) > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/1] microchip-csi2dc: Remove VC support for now 2022-02-08 8:00 ` Eugen.Hristev 2022-02-08 8:05 ` Eugen.Hristev @ 2022-02-08 11:06 ` Laurent Pinchart 2022-02-08 11:16 ` Sakari Ailus 2022-02-08 11:36 ` Eugen.Hristev 1 sibling, 2 replies; 12+ messages in thread From: Laurent Pinchart @ 2022-02-08 11:06 UTC (permalink / raw) To: Eugen.Hristev; +Cc: mchehab, sakari.ailus, linux-media Hi Eugen, On Tue, Feb 08, 2022 at 08:00:19AM +0000, Eugen.Hristev@microchip.com wrote: > On 2/8/22 7:11 AM, Mauro Carvalho Chehab wrote: > > Em Wed, 2 Feb 2022 17:36:09 +0200 Sakari Ailus escreveu: > > > >> As part of removing mbus config flags, remove VC flag use in the > >> microchip-csi2dc driver. The support can be reintroduced later on as part > >> of the streams patches. > >> > >> Cc: Eugen Hristev <eugen.hristev@microchip.com> > > > > Hmm... that sounds a regression to me. What effects this will cause at > > the driver? > > > > Eugen, any comments? > > Hi , > > I am not happy with this change. It looks like I wasn't even CC-ed on > the original patch e-mail. > > The effect on the driver will be that everything will be treated as > virtual channel=0 . I don't think there's any risk of regression, as we have no driver setting any of the V4L2_MBUS_CSI2_CHANNEL_[123] flags in the kernel. > I do not yet understand why we are about to remove > V4L2_MBUS_CSI2_CHANNEL_* as I remember this was just introduced. Those flags were added in 2011. If you think of that as "just introduced" then I understand why you would be unhappy about "sudden changes" mentioned below ;-) > Is there any alternative in place ? Virtual channels have never been properly supported in V4L2. This is going to change with "[PATCH v10 00/38] v4l: subdev internal routing and streams" ([1]). [1] https://lore.kernel.org/all/20211130141536.891878-1-tomi.valkeinen@ideasonboard.com > My opinion is that if we want to replace something existing with a new > API or something else, we should first add the new support, block any > new adopters for the old API such that everyone uses the new API, and > only after that convert the old API clients to the new API. > So 'can be reintroduced later on' is not okay. We can't remove things in > the hope that it would be reintroduced later. Just my personal take on > this, feel free to have a different opinion. When regressions are introduced this makes sense, but here we're dropping a feature that isn't used as no kernel driver selects a VC different than 0. > In the end you guys are the maintainers for the subsystem and can have > this change if you like, I am more unhappy about the fact that changes > happen suddenly and without notice. > > >> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > >> --- > >> .../media/platform/atmel/microchip-csi2dc.c | 18 ++---------------- > >> 1 file changed, 2 insertions(+), 16 deletions(-) > >> > >> diff --git a/drivers/media/platform/atmel/microchip-csi2dc.c b/drivers/media/platform/atmel/microchip-csi2dc.c > >> index 6bc549c28e05..6a7f5b4b0e3b 100644 > >> --- a/drivers/media/platform/atmel/microchip-csi2dc.c > >> +++ b/drivers/media/platform/atmel/microchip-csi2dc.c > >> @@ -348,24 +348,15 @@ static int csi2dc_get_mbus_config(struct csi2dc_device *csi2dc) > >> if (ret == -ENOIOCTLCMD) { > >> dev_dbg(csi2dc->dev, > >> "no remote mbus configuration available\n"); > >> - goto csi2dc_get_mbus_config_defaults; > >> + return 0; > >> } > >> > >> if (ret) { > >> dev_err(csi2dc->dev, > >> "failed to get remote mbus configuration\n"); > >> - goto csi2dc_get_mbus_config_defaults; > >> + return 0; > >> } > >> > >> - if (mbus_config.flags & V4L2_MBUS_CSI2_CHANNEL_0) > >> - csi2dc->vc = 0; > >> - else if (mbus_config.flags & V4L2_MBUS_CSI2_CHANNEL_1) > >> - csi2dc->vc = 1; > >> - else if (mbus_config.flags & V4L2_MBUS_CSI2_CHANNEL_2) > >> - csi2dc->vc = 2; > >> - else if (mbus_config.flags & V4L2_MBUS_CSI2_CHANNEL_3) > >> - csi2dc->vc = 3; > >> - > >> dev_dbg(csi2dc->dev, "subdev sending on channel %d\n", csi2dc->vc); > >> > >> csi2dc->clk_gated = mbus_config.flags & > >> @@ -375,11 +366,6 @@ static int csi2dc_get_mbus_config(struct csi2dc_device *csi2dc) > >> csi2dc->clk_gated ? "gated" : "free running"); > >> > >> return 0; > >> - > >> -csi2dc_get_mbus_config_defaults: > >> - csi2dc->vc = 0; /* Virtual ID 0 by default */ > >> - > >> - return 0; > >> } > >> > >> static void csi2dc_vp_update(struct csi2dc_device *csi2dc) -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/1] microchip-csi2dc: Remove VC support for now 2022-02-08 11:06 ` Laurent Pinchart @ 2022-02-08 11:16 ` Sakari Ailus 2022-02-08 11:37 ` Eugen.Hristev 2022-02-08 11:36 ` Eugen.Hristev 1 sibling, 1 reply; 12+ messages in thread From: Sakari Ailus @ 2022-02-08 11:16 UTC (permalink / raw) To: Laurent Pinchart; +Cc: Eugen.Hristev, mchehab, linux-media Hello everyone, On Tue, Feb 08, 2022 at 01:06:17PM +0200, Laurent Pinchart wrote: > > My opinion is that if we want to replace something existing with a new > > API or something else, we should first add the new support, block any > > new adopters for the old API such that everyone uses the new API, and > > only after that convert the old API clients to the new API. > > So 'can be reintroduced later on' is not okay. We can't remove things in > > the hope that it would be reintroduced later. Just my personal take on > > this, feel free to have a different opinion. > > When regressions are introduced this makes sense, but here we're > dropping a feature that isn't used as no kernel driver selects a VC > different than 0. I'd like to add that the purpose of virtual channels in CSI-2 is to be able to transport multiple logically separate streams over a single link. That has never been supported in V4L2 so these flags have been unusable to begin with. The streams set a lot of people have been working on on their turn is the way to support virtual channels (as well as other features such as data types), and it does not involve using mbus flags (as the virtual channel isn't bound to the bus). The intent was to cc the patch to Eugen but it seems my gitconfig prevented that. -- Kind regards, Sakari Ailus ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/1] microchip-csi2dc: Remove VC support for now 2022-02-08 11:16 ` Sakari Ailus @ 2022-02-08 11:37 ` Eugen.Hristev 0 siblings, 0 replies; 12+ messages in thread From: Eugen.Hristev @ 2022-02-08 11:37 UTC (permalink / raw) To: sakari.ailus, laurent.pinchart; +Cc: mchehab, linux-media On 2/8/22 1:16 PM, Sakari Ailus wrote: > Hello everyone, > > On Tue, Feb 08, 2022 at 01:06:17PM +0200, Laurent Pinchart wrote: >>> My opinion is that if we want to replace something existing with a new >>> API or something else, we should first add the new support, block any >>> new adopters for the old API such that everyone uses the new API, and >>> only after that convert the old API clients to the new API. >>> So 'can be reintroduced later on' is not okay. We can't remove things in >>> the hope that it would be reintroduced later. Just my personal take on >>> this, feel free to have a different opinion. >> >> When regressions are introduced this makes sense, but here we're >> dropping a feature that isn't used as no kernel driver selects a VC >> different than 0. > > I'd like to add that the purpose of virtual channels in CSI-2 is to be able > to transport multiple logically separate streams over a single link. That > has never been supported in V4L2 so these flags have been unusable to begin > with. > > The streams set a lot of people have been working on on their turn is the > way to support virtual channels (as well as other features such as data > types), and it does not involve using mbus flags (as the virtual channel > isn't bound to the bus). > > The intent was to cc the patch to Eugen but it seems my gitconfig prevented > that. Hello Sakari, I appreciate you taking time to explain this and the intent to cc me on the patch. Thanks, Eugen > > -- > Kind regards, > > Sakari Ailus > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/1] microchip-csi2dc: Remove VC support for now 2022-02-08 11:06 ` Laurent Pinchart 2022-02-08 11:16 ` Sakari Ailus @ 2022-02-08 11:36 ` Eugen.Hristev 2022-02-08 12:19 ` Laurent Pinchart 2022-02-15 19:21 ` Sakari Ailus 1 sibling, 2 replies; 12+ messages in thread From: Eugen.Hristev @ 2022-02-08 11:36 UTC (permalink / raw) To: laurent.pinchart; +Cc: mchehab, sakari.ailus, linux-media On 2/8/22 1:06 PM, Laurent Pinchart wrote: > Hi Eugen, > > On Tue, Feb 08, 2022 at 08:00:19AM +0000, Eugen.Hristev@microchip.com wrote: >> On 2/8/22 7:11 AM, Mauro Carvalho Chehab wrote: >>> Em Wed, 2 Feb 2022 17:36:09 +0200 Sakari Ailus escreveu: >>> >>>> As part of removing mbus config flags, remove VC flag use in the >>>> microchip-csi2dc driver. The support can be reintroduced later on as part >>>> of the streams patches. >>>> >>>> Cc: Eugen Hristev <eugen.hristev@microchip.com> >>> >>> Hmm... that sounds a regression to me. What effects this will cause at >>> the driver? >>> >>> Eugen, any comments? >> >> Hi , >> >> I am not happy with this change. It looks like I wasn't even CC-ed on >> the original patch e-mail. >> >> The effect on the driver will be that everything will be treated as >> virtual channel=0 . > > I don't think there's any risk of regression, as we have no driver > setting any of the V4L2_MBUS_CSI2_CHANNEL_[123] flags in the kernel. > >> I do not yet understand why we are about to remove >> V4L2_MBUS_CSI2_CHANNEL_* as I remember this was just introduced. > > Those flags were added in 2011. If you think of that as "just > introduced" then I understand why you would be unhappy about "sudden > changes" mentioned below ;-) Hello Laurent, I am sorry, I was referring to the get_mbus_config which was recently added, which in my case uses V4L2_MBUS_CSI2_CHANNEL_* . Without the get_mbus_config, I wasn't supposed to use the V4L2_MBUS_CSI2_CHANNEL_* at all . Hence my confusion in adding them into the same bucket. My driver uses these flags because my initial implementation to get the VC from the devicetree was rejected and this was suggested instead. > >> Is there any alternative in place ? > > Virtual channels have never been properly supported in V4L2. This is > going to change with "[PATCH v10 00/38] v4l: subdev internal routing and > streams" ([1]). > > [1] https://lore.kernel.org/all/20211130141536.891878-1-tomi.valkeinen@ideasonboard.com In that case then I suppose the support in the csi2dc driver has to be rewritten > >> My opinion is that if we want to replace something existing with a new >> API or something else, we should first add the new support, block any >> new adopters for the old API such that everyone uses the new API, and >> only after that convert the old API clients to the new API. >> So 'can be reintroduced later on' is not okay. We can't remove things in >> the hope that it would be reintroduced later. Just my personal take on >> this, feel free to have a different opinion. > > When regressions are introduced this makes sense, but here we're > dropping a feature that isn't used as no kernel driver selects a VC > different than 0. I still disagree that these should be removed before the patch you mentioned earlier about adding 'subdev internal routing and streams' is applied. I see things in the way that after the new support is available, old flags , even if unused, can be removed. You can disagree, of course. Thank you for sharing your thoughts, Eugen > >> In the end you guys are the maintainers for the subsystem and can have >> this change if you like, I am more unhappy about the fact that changes >> happen suddenly and without notice. >> >>>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> >>>> --- >>>> .../media/platform/atmel/microchip-csi2dc.c | 18 ++---------------- >>>> 1 file changed, 2 insertions(+), 16 deletions(-) >>>> >>>> diff --git a/drivers/media/platform/atmel/microchip-csi2dc.c b/drivers/media/platform/atmel/microchip-csi2dc.c >>>> index 6bc549c28e05..6a7f5b4b0e3b 100644 >>>> --- a/drivers/media/platform/atmel/microchip-csi2dc.c >>>> +++ b/drivers/media/platform/atmel/microchip-csi2dc.c >>>> @@ -348,24 +348,15 @@ static int csi2dc_get_mbus_config(struct csi2dc_device *csi2dc) >>>> if (ret == -ENOIOCTLCMD) { >>>> dev_dbg(csi2dc->dev, >>>> "no remote mbus configuration available\n"); >>>> - goto csi2dc_get_mbus_config_defaults; >>>> + return 0; >>>> } >>>> >>>> if (ret) { >>>> dev_err(csi2dc->dev, >>>> "failed to get remote mbus configuration\n"); >>>> - goto csi2dc_get_mbus_config_defaults; >>>> + return 0; >>>> } >>>> >>>> - if (mbus_config.flags & V4L2_MBUS_CSI2_CHANNEL_0) >>>> - csi2dc->vc = 0; >>>> - else if (mbus_config.flags & V4L2_MBUS_CSI2_CHANNEL_1) >>>> - csi2dc->vc = 1; >>>> - else if (mbus_config.flags & V4L2_MBUS_CSI2_CHANNEL_2) >>>> - csi2dc->vc = 2; >>>> - else if (mbus_config.flags & V4L2_MBUS_CSI2_CHANNEL_3) >>>> - csi2dc->vc = 3; >>>> - >>>> dev_dbg(csi2dc->dev, "subdev sending on channel %d\n", csi2dc->vc); >>>> >>>> csi2dc->clk_gated = mbus_config.flags & >>>> @@ -375,11 +366,6 @@ static int csi2dc_get_mbus_config(struct csi2dc_device *csi2dc) >>>> csi2dc->clk_gated ? "gated" : "free running"); >>>> >>>> return 0; >>>> - >>>> -csi2dc_get_mbus_config_defaults: >>>> - csi2dc->vc = 0; /* Virtual ID 0 by default */ >>>> - >>>> - return 0; >>>> } >>>> >>>> static void csi2dc_vp_update(struct csi2dc_device *csi2dc) > > -- > Regards, > > Laurent Pinchart > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/1] microchip-csi2dc: Remove VC support for now 2022-02-08 11:36 ` Eugen.Hristev @ 2022-02-08 12:19 ` Laurent Pinchart 2022-02-15 19:21 ` Sakari Ailus 1 sibling, 0 replies; 12+ messages in thread From: Laurent Pinchart @ 2022-02-08 12:19 UTC (permalink / raw) To: Eugen.Hristev; +Cc: mchehab, sakari.ailus, linux-media Hi Eugen, On Tue, Feb 08, 2022 at 11:36:16AM +0000, Eugen.Hristev@microchip.com wrote: > On 2/8/22 1:06 PM, Laurent Pinchart wrote: > > On Tue, Feb 08, 2022 at 08:00:19AM +0000, Eugen.Hristev@microchip.com wrote: > >> On 2/8/22 7:11 AM, Mauro Carvalho Chehab wrote: > >>> Em Wed, 2 Feb 2022 17:36:09 +0200 Sakari Ailus escreveu: > >>> > >>>> As part of removing mbus config flags, remove VC flag use in the > >>>> microchip-csi2dc driver. The support can be reintroduced later on as part > >>>> of the streams patches. > >>>> > >>>> Cc: Eugen Hristev <eugen.hristev@microchip.com> > >>> > >>> Hmm... that sounds a regression to me. What effects this will cause at > >>> the driver? > >>> > >>> Eugen, any comments? > >> > >> Hi , > >> > >> I am not happy with this change. It looks like I wasn't even CC-ed on > >> the original patch e-mail. > >> > >> The effect on the driver will be that everything will be treated as > >> virtual channel=0 . > > > > I don't think there's any risk of regression, as we have no driver > > setting any of the V4L2_MBUS_CSI2_CHANNEL_[123] flags in the kernel. > > > >> I do not yet understand why we are about to remove > >> V4L2_MBUS_CSI2_CHANNEL_* as I remember this was just introduced. > > > > Those flags were added in 2011. If you think of that as "just > > introduced" then I understand why you would be unhappy about "sudden > > changes" mentioned below ;-) > > Hello Laurent, > > I am sorry, I was referring to the get_mbus_config which was recently > added, which in my case uses V4L2_MBUS_CSI2_CHANNEL_* . > Without the get_mbus_config, I wasn't supposed to use the > V4L2_MBUS_CSI2_CHANNEL_* at all . > Hence my confusion in adding them into the same bucket. > > My driver uses these flags because my initial implementation to get the > VC from the devicetree was rejected and this was suggested instead. > > >> Is there any alternative in place ? > > > > Virtual channels have never been properly supported in V4L2. This is > > going to change with "[PATCH v10 00/38] v4l: subdev internal routing and > > streams" ([1]). > > > > [1] https://lore.kernel.org/all/20211130141536.891878-1-tomi.valkeinen@ideasonboard.com > > In that case then I suppose the support in the csi2dc driver has to be > rewritten Only when you'll need to support virtual channels other than VC 0. It's a non-negligible effort if you need to capture multiple streams from different VCs simultaneously, but if it's only about selecting the correct VC number for one stream, it's much easier. As far as I understand, your hardware supports the latter, not the former, right ? Do you have any particular CSI-2 source in mind that produces different VCs by the way ? > >> My opinion is that if we want to replace something existing with a new > >> API or something else, we should first add the new support, block any > >> new adopters for the old API such that everyone uses the new API, and > >> only after that convert the old API clients to the new API. > >> So 'can be reintroduced later on' is not okay. We can't remove things in > >> the hope that it would be reintroduced later. Just my personal take on > >> this, feel free to have a different opinion. > > > > When regressions are introduced this makes sense, but here we're > > dropping a feature that isn't used as no kernel driver selects a VC > > different than 0. > > I still disagree that these should be removed before the patch you > mentioned earlier about adding 'subdev internal routing and streams' is > applied. I see things in the way that after the new support is > available, old flags , even if unused, can be removed. > You can disagree, of course. I agree it would be nice, but the multiplexed streams series will still need time to land (it's been out there for ages, we've resumed work on it about a year ago, and Tomi has done a really amazing job moving this forward). I'd like to avoid blocking the V4L2_MBUS_CSI2_CHANNEL_* removal until multiplexed streams get merged if possible. If there was an urgent need to support different VCs in your driver the situation would of course be different, but if that's not foreseen until we get proper support for multiplexed streams, I think moving forward with all the rework in parallel is best. V4L2 has few active contributors for core code, it's really hard modernizing the internals of the subsystem with the resource shortage, I'd like to avoid making it even harder :-) > Thank you for sharing your thoughts, > Eugen > > >> In the end you guys are the maintainers for the subsystem and can have > >> this change if you like, I am more unhappy about the fact that changes > >> happen suddenly and without notice. > >> > >>>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > >>>> --- > >>>> .../media/platform/atmel/microchip-csi2dc.c | 18 ++---------------- > >>>> 1 file changed, 2 insertions(+), 16 deletions(-) > >>>> > >>>> diff --git a/drivers/media/platform/atmel/microchip-csi2dc.c b/drivers/media/platform/atmel/microchip-csi2dc.c > >>>> index 6bc549c28e05..6a7f5b4b0e3b 100644 > >>>> --- a/drivers/media/platform/atmel/microchip-csi2dc.c > >>>> +++ b/drivers/media/platform/atmel/microchip-csi2dc.c > >>>> @@ -348,24 +348,15 @@ static int csi2dc_get_mbus_config(struct csi2dc_device *csi2dc) > >>>> if (ret == -ENOIOCTLCMD) { > >>>> dev_dbg(csi2dc->dev, > >>>> "no remote mbus configuration available\n"); > >>>> - goto csi2dc_get_mbus_config_defaults; > >>>> + return 0; > >>>> } > >>>> > >>>> if (ret) { > >>>> dev_err(csi2dc->dev, > >>>> "failed to get remote mbus configuration\n"); > >>>> - goto csi2dc_get_mbus_config_defaults; > >>>> + return 0; > >>>> } > >>>> > >>>> - if (mbus_config.flags & V4L2_MBUS_CSI2_CHANNEL_0) > >>>> - csi2dc->vc = 0; > >>>> - else if (mbus_config.flags & V4L2_MBUS_CSI2_CHANNEL_1) > >>>> - csi2dc->vc = 1; > >>>> - else if (mbus_config.flags & V4L2_MBUS_CSI2_CHANNEL_2) > >>>> - csi2dc->vc = 2; > >>>> - else if (mbus_config.flags & V4L2_MBUS_CSI2_CHANNEL_3) > >>>> - csi2dc->vc = 3; > >>>> - > >>>> dev_dbg(csi2dc->dev, "subdev sending on channel %d\n", csi2dc->vc); > >>>> > >>>> csi2dc->clk_gated = mbus_config.flags & > >>>> @@ -375,11 +366,6 @@ static int csi2dc_get_mbus_config(struct csi2dc_device *csi2dc) > >>>> csi2dc->clk_gated ? "gated" : "free running"); > >>>> > >>>> return 0; > >>>> - > >>>> -csi2dc_get_mbus_config_defaults: > >>>> - csi2dc->vc = 0; /* Virtual ID 0 by default */ > >>>> - > >>>> - return 0; > >>>> } > >>>> > >>>> static void csi2dc_vp_update(struct csi2dc_device *csi2dc) -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/1] microchip-csi2dc: Remove VC support for now 2022-02-08 11:36 ` Eugen.Hristev 2022-02-08 12:19 ` Laurent Pinchart @ 2022-02-15 19:21 ` Sakari Ailus 2022-02-15 20:10 ` Eugen.Hristev 1 sibling, 1 reply; 12+ messages in thread From: Sakari Ailus @ 2022-02-15 19:21 UTC (permalink / raw) To: Eugen.Hristev; +Cc: laurent.pinchart, mchehab, linux-media Hi Eugen, On Tue, Feb 08, 2022 at 11:36:16AM +0000, Eugen.Hristev@microchip.com wrote: > I am sorry, I was referring to the get_mbus_config which was recently > added, which in my case uses V4L2_MBUS_CSI2_CHANNEL_* . > Without the get_mbus_config, I wasn't supposed to use the > V4L2_MBUS_CSI2_CHANNEL_* at all . > Hence my confusion in adding them into the same bucket. > > My driver uses these flags because my initial implementation to get the > VC from the devicetree was rejected and this was suggested instead. As noted by Laurent, this feature is not supported in mainline currently. Beyond that, no transmitter driver uses virtual channel different from zero, so the case for setting non-zero virtual channel in absence of multiple streams does not exist. Also the interfaces required to make use of multiple streams do not exist in mainline at the moment. They will be added by the streams patchset eventually, and adding support for them requires extra driver work. Thus, we're not losing any functionality by dropping the defines and potentially associated driver code. Therefore I see no reason to postpone these patches. -- Kind regards, Sakari Ailus ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/1] microchip-csi2dc: Remove VC support for now 2022-02-15 19:21 ` Sakari Ailus @ 2022-02-15 20:10 ` Eugen.Hristev 2022-02-17 11:10 ` Sakari Ailus 0 siblings, 1 reply; 12+ messages in thread From: Eugen.Hristev @ 2022-02-15 20:10 UTC (permalink / raw) To: sakari.ailus; +Cc: laurent.pinchart, mchehab, linux-media On 2/15/22 9:21 PM, Sakari Ailus wrote: > Hi Eugen, > > On Tue, Feb 08, 2022 at 11:36:16AM +0000, Eugen.Hristev@microchip.com wrote: >> I am sorry, I was referring to the get_mbus_config which was recently >> added, which in my case uses V4L2_MBUS_CSI2_CHANNEL_* . >> Without the get_mbus_config, I wasn't supposed to use the >> V4L2_MBUS_CSI2_CHANNEL_* at all . >> Hence my confusion in adding them into the same bucket. >> >> My driver uses these flags because my initial implementation to get the >> VC from the devicetree was rejected and this was suggested instead. > > As noted by Laurent, this feature is not supported in mainline currently. > Beyond that, no transmitter driver uses virtual channel different from > zero, so the case for setting non-zero virtual channel in absence of > multiple streams does not exist. > > Also the interfaces required to make use of multiple streams do not exist > in mainline at the moment. They will be added by the streams patchset > eventually, and adding support for them requires extra driver work. > > Thus, we're not losing any functionality by dropping the defines and > potentially associated driver code. Therefore I see no reason to postpone > these patches. Hi Sakari, I understand. I am fine with the patch. Eugen > > -- > Kind regards, > > Sakari Ailus > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/1] microchip-csi2dc: Remove VC support for now 2022-02-15 20:10 ` Eugen.Hristev @ 2022-02-17 11:10 ` Sakari Ailus 0 siblings, 0 replies; 12+ messages in thread From: Sakari Ailus @ 2022-02-17 11:10 UTC (permalink / raw) To: Eugen.Hristev; +Cc: laurent.pinchart, mchehab, linux-media On Tue, Feb 15, 2022 at 08:10:18PM +0000, Eugen.Hristev@microchip.com wrote: > On 2/15/22 9:21 PM, Sakari Ailus wrote: > > Hi Eugen, > > > > On Tue, Feb 08, 2022 at 11:36:16AM +0000, Eugen.Hristev@microchip.com wrote: > >> I am sorry, I was referring to the get_mbus_config which was recently > >> added, which in my case uses V4L2_MBUS_CSI2_CHANNEL_* . > >> Without the get_mbus_config, I wasn't supposed to use the > >> V4L2_MBUS_CSI2_CHANNEL_* at all . > >> Hence my confusion in adding them into the same bucket. > >> > >> My driver uses these flags because my initial implementation to get the > >> VC from the devicetree was rejected and this was suggested instead. > > > > As noted by Laurent, this feature is not supported in mainline currently. > > Beyond that, no transmitter driver uses virtual channel different from > > zero, so the case for setting non-zero virtual channel in absence of > > multiple streams does not exist. > > > > Also the interfaces required to make use of multiple streams do not exist > > in mainline at the moment. They will be added by the streams patchset > > eventually, and adding support for them requires extra driver work. > > > > Thus, we're not losing any functionality by dropping the defines and > > potentially associated driver code. Therefore I see no reason to postpone > > these patches. > > Hi Sakari, > > I understand. I am fine with the patch. Thank you, Eugen. -- Sakari Ailus ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2022-02-17 11:10 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-02-02 15:36 [PATCH 1/1] microchip-csi2dc: Remove VC support for now Sakari Ailus 2022-02-08 5:11 ` Mauro Carvalho Chehab 2022-02-08 8:00 ` Eugen.Hristev 2022-02-08 8:05 ` Eugen.Hristev 2022-02-08 11:06 ` Laurent Pinchart 2022-02-08 11:16 ` Sakari Ailus 2022-02-08 11:37 ` Eugen.Hristev 2022-02-08 11:36 ` Eugen.Hristev 2022-02-08 12:19 ` Laurent Pinchart 2022-02-15 19:21 ` Sakari Ailus 2022-02-15 20:10 ` Eugen.Hristev 2022-02-17 11:10 ` 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).