[PATCH v2 0/2] rcar-vin: Add support for R-Car V4M

[PATCH v2 0/2] rcar-vin: Add support for R-Car V4M

[Niklas Söderlund]

Hello,

This small series adds bindings and support to rcar-vin for R-Car V4M.

The V4M capture pipeline is similar to the other Gen4 SoC supported 
upstream already V4H. Currently all futures supported for VIN on V4M are 
also supported by V4H and the driver code can be shared. But as done for 
other R-Car IP bindings a new dedicated binding for V4M is created.  
This have proved prudent in the past where quirks are found even within 
the same generation as more advance use-cases are enabled.

The two patches where previously posted separately as v1, but are now 
collected in a single series. I wold be happy if the bindings could go 
thru the DT-tree.

See individual patches for changes since previous version.

Niklas Söderlund (2):
  dt-bindings: media: renesas,vin: Add binding for V4M
  media: rcar-vin: Add support for R-Car V4M

 Documentation/devicetree/bindings/media/renesas,vin.yaml | 1 +
 drivers/media/platform/renesas/rcar-vin/rcar-core.c      | 8 ++++++--
 2 files changed, 7 insertions(+), 2 deletions(-)

-- 
2.45.2

[PATCH v2 1/2] dt-bindings: media: renesas,vin: Add binding for V4M

[Niklas Söderlund]

Document support for the VIN module in the Renesas V4M (r8a779h0) SoC.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 Documentation/devicetree/bindings/media/renesas,vin.yaml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/media/renesas,vin.yaml b/Documentation/devicetree/bindings/media/renesas,vin.yaml
index 5539d0f8e74d..168cb02f8abe 100644
--- a/Documentation/devicetree/bindings/media/renesas,vin.yaml
+++ b/Documentation/devicetree/bindings/media/renesas,vin.yaml
@@ -54,6 +54,7 @@ properties:
               - renesas,vin-r8a77995 # R-Car D3
               - renesas,vin-r8a779a0 # R-Car V3U
               - renesas,vin-r8a779g0 # R-Car V4H
+              - renesas,vin-r8a779h0 # R-Car V4M
 
   reg:
     maxItems: 1
-- 
2.45.2

[PATCH v2 2/2] media: rcar-vin: Add support for R-Car V4M

[Niklas Söderlund]

Add support for R-Car V4M. The V4M is similar to V4H and uses the ISP
Channel Selector as its only possible video input source. Reuse and
rename the info structure from V4H to cover all current Gen4 SoCs.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
* Changes since v1
- Create a shared Gen4 info strucutre.
---
 drivers/media/platform/renesas/rcar-vin/rcar-core.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/renesas/rcar-vin/rcar-core.c b/drivers/media/platform/renesas/rcar-vin/rcar-core.c
index 809c3a38cc4a..6992b61f0d48 100644
--- a/drivers/media/platform/renesas/rcar-vin/rcar-core.c
+++ b/drivers/media/platform/renesas/rcar-vin/rcar-core.c
@@ -1283,7 +1283,7 @@ static const struct rvin_info rcar_info_r8a779a0 = {
 	.max_height = 4096,
 };
 
-static const struct rvin_info rcar_info_r8a779g0 = {
+static const struct rvin_info rcar_info_gen4 = {
 	.model = RCAR_GEN3,
 	.use_mc = true,
 	.use_isp = true,
@@ -1359,7 +1359,11 @@ static const struct of_device_id rvin_of_id_table[] = {
 	},
 	{
 		.compatible = "renesas,vin-r8a779g0",
-		.data = &rcar_info_r8a779g0,
+		.data = &rcar_info_gen4,
+	},
+	{
+		.compatible = "renesas,vin-r8a779h0",
+		.data = &rcar_info_gen4,
 	},
 	{ /* Sentinel */ },
 };
-- 
2.45.2

Re: [PATCH v2 2/2] media: rcar-vin: Add support for R-Car V4M

[Geert Uytterhoeven]

Hi Niklas,

On Mon, Jun 10, 2024 at 1:32 PM Niklas Söderlund
<niklas.soderlund+renesas@ragnatech.se> wrote:
> Add support for R-Car V4M. The V4M is similar to V4H and uses the ISP
> Channel Selector as its only possible video input source. Reuse and
> rename the info structure from V4H to cover all current Gen4 SoCs.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
> * Changes since v1
> - Create a shared Gen4 info strucutre.

Thanks for the update!

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

> --- a/drivers/media/platform/renesas/rcar-vin/rcar-core.c
> +++ b/drivers/media/platform/renesas/rcar-vin/rcar-core.c
> @@ -1283,7 +1283,7 @@ static const struct rvin_info rcar_info_r8a779a0 = {
>         .max_height = 4096,
>  };
>
> -static const struct rvin_info rcar_info_r8a779g0 = {
> +static const struct rvin_info rcar_info_gen4 = {
>         .model = RCAR_GEN3,
>         .use_mc = true,
>         .use_isp = true,

FTR, rcar_info_gen4 is (still) identical to rcar_info_r8a779a0.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 1/2] dt-bindings: media: renesas,vin: Add binding for V4M

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1019 bytes --]

On Mon, Jun 10, 2024 at 01:31:23PM +0200, Niklas Söderlund wrote:
> Document support for the VIN module in the Renesas V4M (r8a779h0) SoC.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
>  Documentation/devicetree/bindings/media/renesas,vin.yaml | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Documentation/devicetree/bindings/media/renesas,vin.yaml b/Documentation/devicetree/bindings/media/renesas,vin.yaml
> index 5539d0f8e74d..168cb02f8abe 100644
> --- a/Documentation/devicetree/bindings/media/renesas,vin.yaml
> +++ b/Documentation/devicetree/bindings/media/renesas,vin.yaml
> @@ -54,6 +54,7 @@ properties:
>                - renesas,vin-r8a77995 # R-Car D3
>                - renesas,vin-r8a779a0 # R-Car V3U
>                - renesas,vin-r8a779g0 # R-Car V4H
> +              - renesas,vin-r8a779h0 # R-Car V4M

Your driver patch suggests that this is compatible with the g variant.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2 1/2] dt-bindings: media: renesas,vin: Add binding for V4M

[Niklas Söderlund]

Hi Conor,

Thanks for your feedback.

On 2024-06-10 17:03:49 +0100, Conor Dooley wrote:
> On Mon, Jun 10, 2024 at 01:31:23PM +0200, Niklas Söderlund wrote:
> > Document support for the VIN module in the Renesas V4M (r8a779h0) SoC.
> > 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > ---
> >  Documentation/devicetree/bindings/media/renesas,vin.yaml | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/media/renesas,vin.yaml b/Documentation/devicetree/bindings/media/renesas,vin.yaml
> > index 5539d0f8e74d..168cb02f8abe 100644
> > --- a/Documentation/devicetree/bindings/media/renesas,vin.yaml
> > +++ b/Documentation/devicetree/bindings/media/renesas,vin.yaml
> > @@ -54,6 +54,7 @@ properties:
> >                - renesas,vin-r8a77995 # R-Car D3
> >                - renesas,vin-r8a779a0 # R-Car V3U
> >                - renesas,vin-r8a779g0 # R-Car V4H
> > +              - renesas,vin-r8a779h0 # R-Car V4M
> 
> Your driver patch suggests that this is compatible with the g variant.

Currently it is. But that not always be true, I tried to outline this in 
to cover letter.

    The V4M capture pipeline is similar to the other Gen4 SoC supported
    upstream already V4H. Currently all futures supported for VIN on V4M are
    also supported by V4H and the driver code can be shared. But as done for
    other R-Car IP bindings a new dedicated binding for V4M is created.
    This have proved prudent in the past where quirks are found even within
    the same generation as more advance use-cases are enabled.

-- 
Kind Regards,
Niklas Söderlund

Re: [PATCH v2 1/2] dt-bindings: media: renesas,vin: Add binding for V4M

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 2168 bytes --]

On Mon, Jun 10, 2024 at 06:59:35PM +0200, Niklas Söderlund wrote:
> Hi Conor,
> 
> Thanks for your feedback.
> 
> On 2024-06-10 17:03:49 +0100, Conor Dooley wrote:
> > On Mon, Jun 10, 2024 at 01:31:23PM +0200, Niklas Söderlund wrote:
> > > Document support for the VIN module in the Renesas V4M (r8a779h0) SoC.
> > > 
> > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > > ---
> > >  Documentation/devicetree/bindings/media/renesas,vin.yaml | 1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/media/renesas,vin.yaml b/Documentation/devicetree/bindings/media/renesas,vin.yaml
> > > index 5539d0f8e74d..168cb02f8abe 100644
> > > --- a/Documentation/devicetree/bindings/media/renesas,vin.yaml
> > > +++ b/Documentation/devicetree/bindings/media/renesas,vin.yaml
> > > @@ -54,6 +54,7 @@ properties:
> > >                - renesas,vin-r8a77995 # R-Car D3
> > >                - renesas,vin-r8a779a0 # R-Car V3U
> > >                - renesas,vin-r8a779g0 # R-Car V4H
> > > +              - renesas,vin-r8a779h0 # R-Car V4M
> > 
> > Your driver patch suggests that this is compatible with the g variant.
> 
> Currently it is. But that not always be true, I tried to outline this in 
> to cover letter.

To be honest, I don't usually read cover letters when reviewing bindings.
Information about why things are/are not compatible should be in a
commit itself.

>     The V4M capture pipeline is similar to the other Gen4 SoC supported
>     upstream already V4H. Currently all futures supported for VIN on V4M are
>     also supported by V4H and the driver code can be shared. But as done for
>     other R-Car IP bindings a new dedicated binding for V4M is created.
>     This have proved prudent in the past where quirks are found even within
>     the same generation as more advance use-cases are enabled.

I don't understand how this precludes using the g variant as a fallback
compatible. I'm not suggesting that you don't add a specific one for the
h variant.

Thanks,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2 1/2] dt-bindings: media: renesas,vin: Add binding for V4M

[Niklas Söderlund]

On 2024-06-10 22:32:29 +0100, Conor Dooley wrote:
> On Mon, Jun 10, 2024 at 06:59:35PM +0200, Niklas Söderlund wrote:
> > Hi Conor,
> > 
> > Thanks for your feedback.
> > 
> > On 2024-06-10 17:03:49 +0100, Conor Dooley wrote:
> > > On Mon, Jun 10, 2024 at 01:31:23PM +0200, Niklas Söderlund wrote:
> > > > Document support for the VIN module in the Renesas V4M (r8a779h0) SoC.
> > > > 
> > > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > > > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > > > ---
> > > >  Documentation/devicetree/bindings/media/renesas,vin.yaml | 1 +
> > > >  1 file changed, 1 insertion(+)
> > > > 
> > > > diff --git a/Documentation/devicetree/bindings/media/renesas,vin.yaml b/Documentation/devicetree/bindings/media/renesas,vin.yaml
> > > > index 5539d0f8e74d..168cb02f8abe 100644
> > > > --- a/Documentation/devicetree/bindings/media/renesas,vin.yaml
> > > > +++ b/Documentation/devicetree/bindings/media/renesas,vin.yaml
> > > > @@ -54,6 +54,7 @@ properties:
> > > >                - renesas,vin-r8a77995 # R-Car D3
> > > >                - renesas,vin-r8a779a0 # R-Car V3U
> > > >                - renesas,vin-r8a779g0 # R-Car V4H
> > > > +              - renesas,vin-r8a779h0 # R-Car V4M
> > > 
> > > Your driver patch suggests that this is compatible with the g variant.
> > 
> > Currently it is. But that not always be true, I tried to outline this in 
> > to cover letter.
> 
> To be honest, I don't usually read cover letters when reviewing bindings.
> Information about why things are/are not compatible should be in a
> commit itself.
> 
> >     The V4M capture pipeline is similar to the other Gen4 SoC supported
> >     upstream already V4H. Currently all futures supported for VIN on V4M are
> >     also supported by V4H and the driver code can be shared. But as done for
> >     other R-Car IP bindings a new dedicated binding for V4M is created.
> >     This have proved prudent in the past where quirks are found even within
> >     the same generation as more advance use-cases are enabled.
> 
> I don't understand how this precludes using the g variant as a fallback
> compatible. I'm not suggesting that you don't add a specific one for the
> h variant.

The bindings have been around for a while and currently there are 25 SoC 
specific compatibles, one for each SoC supported. Each compatible 
consist of the SoC model number, not the VIN IP model/version number as 
no such versioning schema exist.

The datasheets are specific for each SoC and there are differences 
between almost every SoC. There are of course lots of similarities 
between the SoCs and the similarities are cluster around the 3 
generations (Gen{2,3,4}) supported.

Using the g variant as fallback in DTS for h variant even if we also add 
a specific one for h is confusing. As g and h are two different SoC.

The g variant is r8a779g0 which is the SoC name/number for V4H.
The h variant is r8a779h0 which is the SoC name/number for V4M.

I think the core of the problem is that there are no versioning schema 
for the individual IP blocks used on each SoC. For better or worse the 
bindings for lots of Renesas IPs are centred around SoC name/number and 
not the individual IP implementations.

-- 
Kind Regards,
Niklas Söderlund

Re: [PATCH v2 1/2] dt-bindings: media: renesas,vin: Add binding for V4M

[Rob Herring]

On Tue, Jun 11, 2024 at 01:06:17PM +0200, Niklas Söderlund wrote:
> On 2024-06-10 22:32:29 +0100, Conor Dooley wrote:
> > On Mon, Jun 10, 2024 at 06:59:35PM +0200, Niklas Söderlund wrote:
> > > Hi Conor,
> > > 
> > > Thanks for your feedback.
> > > 
> > > On 2024-06-10 17:03:49 +0100, Conor Dooley wrote:
> > > > On Mon, Jun 10, 2024 at 01:31:23PM +0200, Niklas Söderlund wrote:
> > > > > Document support for the VIN module in the Renesas V4M (r8a779h0) SoC.
> > > > > 
> > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > > > > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > > > > ---
> > > > >  Documentation/devicetree/bindings/media/renesas,vin.yaml | 1 +
> > > > >  1 file changed, 1 insertion(+)
> > > > > 
> > > > > diff --git a/Documentation/devicetree/bindings/media/renesas,vin.yaml b/Documentation/devicetree/bindings/media/renesas,vin.yaml
> > > > > index 5539d0f8e74d..168cb02f8abe 100644
> > > > > --- a/Documentation/devicetree/bindings/media/renesas,vin.yaml
> > > > > +++ b/Documentation/devicetree/bindings/media/renesas,vin.yaml
> > > > > @@ -54,6 +54,7 @@ properties:
> > > > >                - renesas,vin-r8a77995 # R-Car D3
> > > > >                - renesas,vin-r8a779a0 # R-Car V3U
> > > > >                - renesas,vin-r8a779g0 # R-Car V4H
> > > > > +              - renesas,vin-r8a779h0 # R-Car V4M
> > > > 
> > > > Your driver patch suggests that this is compatible with the g variant.
> > > 
> > > Currently it is. But that not always be true, I tried to outline this in 
> > > to cover letter.
> > 
> > To be honest, I don't usually read cover letters when reviewing bindings.
> > Information about why things are/are not compatible should be in a
> > commit itself.
> > 
> > >     The V4M capture pipeline is similar to the other Gen4 SoC supported
> > >     upstream already V4H. Currently all futures supported for VIN on V4M are
> > >     also supported by V4H and the driver code can be shared. But as done for
> > >     other R-Car IP bindings a new dedicated binding for V4M is created.
> > >     This have proved prudent in the past where quirks are found even within
> > >     the same generation as more advance use-cases are enabled.
> > 
> > I don't understand how this precludes using the g variant as a fallback
> > compatible. I'm not suggesting that you don't add a specific one for the
> > h variant.
> 
> The bindings have been around for a while and currently there are 25 SoC 
> specific compatibles, one for each SoC supported. Each compatible 
> consist of the SoC model number, not the VIN IP model/version number as 
> no such versioning schema exist.
> 
> The datasheets are specific for each SoC and there are differences 
> between almost every SoC. There are of course lots of similarities 
> between the SoCs and the similarities are cluster around the 3 
> generations (Gen{2,3,4}) supported.
> 
> Using the g variant as fallback in DTS for h variant even if we also add 
> a specific one for h is confusing. As g and h are two different SoC.

Why? That is the very definition of how "compatible" is supposed to 
work.

> The g variant is r8a779g0 which is the SoC name/number for V4H.
> The h variant is r8a779h0 which is the SoC name/number for V4M.
> 
> I think the core of the problem is that there are no versioning schema 
> for the individual IP blocks used on each SoC. For better or worse the 
> bindings for lots of Renesas IPs are centred around SoC name/number and 
> not the individual IP implementations.

We've tried IP version based compatibles before. It doesn't work. Guess 
what, the IP version changes with nearly every SoC. Chip designers have 
no discipline.

Rob

Re: [PATCH v2 1/2] dt-bindings: media: renesas,vin: Add binding for V4M

[Geert Uytterhoeven]

Hi Rob, Conor,

On Thu, Jun 13, 2024 at 6:51 PM Rob Herring <robh@kernel.org> wrote:
> On Tue, Jun 11, 2024 at 01:06:17PM +0200, Niklas Söderlund wrote:
> > On 2024-06-10 22:32:29 +0100, Conor Dooley wrote:
> > > On Mon, Jun 10, 2024 at 06:59:35PM +0200, Niklas Söderlund wrote:
> > > > On 2024-06-10 17:03:49 +0100, Conor Dooley wrote:
> > > > > On Mon, Jun 10, 2024 at 01:31:23PM +0200, Niklas Söderlund wrote:
> > > > > > Document support for the VIN module in the Renesas V4M (r8a779h0) SoC.
> > > > > >
> > > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > > > > > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > > > > > ---
> > > > > >  Documentation/devicetree/bindings/media/renesas,vin.yaml | 1 +
> > > > > >  1 file changed, 1 insertion(+)
> > > > > >
> > > > > > diff --git a/Documentation/devicetree/bindings/media/renesas,vin.yaml b/Documentation/devicetree/bindings/media/renesas,vin.yaml
> > > > > > index 5539d0f8e74d..168cb02f8abe 100644
> > > > > > --- a/Documentation/devicetree/bindings/media/renesas,vin.yaml
> > > > > > +++ b/Documentation/devicetree/bindings/media/renesas,vin.yaml
> > > > > > @@ -54,6 +54,7 @@ properties:
> > > > > >                - renesas,vin-r8a77995 # R-Car D3
> > > > > >                - renesas,vin-r8a779a0 # R-Car V3U
> > > > > >                - renesas,vin-r8a779g0 # R-Car V4H
> > > > > > +              - renesas,vin-r8a779h0 # R-Car V4M
> > > > >
> > > > > Your driver patch suggests that this is compatible with the g variant.
> > > >
> > > > Currently it is. But that not always be true, I tried to outline this in
> > > > to cover letter.
> > >
> > > To be honest, I don't usually read cover letters when reviewing bindings.
> > > Information about why things are/are not compatible should be in a
> > > commit itself.
> > >
> > > >     The V4M capture pipeline is similar to the other Gen4 SoC supported
> > > >     upstream already V4H. Currently all futures supported for VIN on V4M are
> > > >     also supported by V4H and the driver code can be shared. But as done for
> > > >     other R-Car IP bindings a new dedicated binding for V4M is created.
> > > >     This have proved prudent in the past where quirks are found even within
> > > >     the same generation as more advance use-cases are enabled.
> > >
> > > I don't understand how this precludes using the g variant as a fallback
> > > compatible. I'm not suggesting that you don't add a specific one for the
> > > h variant.
> >
> > The bindings have been around for a while and currently there are 25 SoC
> > specific compatibles, one for each SoC supported. Each compatible
> > consist of the SoC model number, not the VIN IP model/version number as
> > no such versioning schema exist.
> >
> > The datasheets are specific for each SoC and there are differences
> > between almost every SoC. There are of course lots of similarities
> > between the SoCs and the similarities are cluster around the 3
> > generations (Gen{2,3,4}) supported.
> >
> > Using the g variant as fallback in DTS for h variant even if we also add
> > a specific one for h is confusing. As g and h are two different SoC.
>
> Why? That is the very definition of how "compatible" is supposed to
> work.
>
> > The g variant is r8a779g0 which is the SoC name/number for V4H.
> > The h variant is r8a779h0 which is the SoC name/number for V4M.
> >
> > I think the core of the problem is that there are no versioning schema
> > for the individual IP blocks used on each SoC. For better or worse the
> > bindings for lots of Renesas IPs are centred around SoC name/number and
> > not the individual IP implementations.
>
> We've tried IP version based compatibles before. It doesn't work. Guess
> what, the IP version changes with nearly every SoC. Chip designers have
> no discipline.

The R-Car V4M capture pipeline is similar to e.g. the R-Car V4H capture
pipeline. But it is not identical, hence the different compatible values.
AFAIU, for the current feature-set, the driver does not need to handle
the differences.  But that may change later...

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 1/2] dt-bindings: media: renesas,vin: Add binding for V4M

[Niklas Söderlund]

Hi All,

On 2024-06-13 21:35:13 +0200, Geert Uytterhoeven wrote:
> Hi Rob, Conor,
> 
> On Thu, Jun 13, 2024 at 6:51 PM Rob Herring <robh@kernel.org> wrote:
> > On Tue, Jun 11, 2024 at 01:06:17PM +0200, Niklas Söderlund wrote:
> > > On 2024-06-10 22:32:29 +0100, Conor Dooley wrote:
> > > > On Mon, Jun 10, 2024 at 06:59:35PM +0200, Niklas Söderlund wrote:
> > > > > On 2024-06-10 17:03:49 +0100, Conor Dooley wrote:
> > > > > > On Mon, Jun 10, 2024 at 01:31:23PM +0200, Niklas Söderlund wrote:
> > > > > > > Document support for the VIN module in the Renesas V4M (r8a779h0) SoC.
> > > > > > >
> > > > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > > > > > > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > > > > > > ---
> > > > > > >  Documentation/devicetree/bindings/media/renesas,vin.yaml | 1 +
> > > > > > >  1 file changed, 1 insertion(+)
> > > > > > >
> > > > > > > diff --git a/Documentation/devicetree/bindings/media/renesas,vin.yaml b/Documentation/devicetree/bindings/media/renesas,vin.yaml
> > > > > > > index 5539d0f8e74d..168cb02f8abe 100644
> > > > > > > --- a/Documentation/devicetree/bindings/media/renesas,vin.yaml
> > > > > > > +++ b/Documentation/devicetree/bindings/media/renesas,vin.yaml
> > > > > > > @@ -54,6 +54,7 @@ properties:
> > > > > > >                - renesas,vin-r8a77995 # R-Car D3
> > > > > > >                - renesas,vin-r8a779a0 # R-Car V3U
> > > > > > >                - renesas,vin-r8a779g0 # R-Car V4H
> > > > > > > +              - renesas,vin-r8a779h0 # R-Car V4M
> > > > > >
> > > > > > Your driver patch suggests that this is compatible with the g variant.
> > > > >
> > > > > Currently it is. But that not always be true, I tried to outline this in
> > > > > to cover letter.
> > > >
> > > > To be honest, I don't usually read cover letters when reviewing bindings.
> > > > Information about why things are/are not compatible should be in a
> > > > commit itself.
> > > >
> > > > >     The V4M capture pipeline is similar to the other Gen4 SoC supported
> > > > >     upstream already V4H. Currently all futures supported for VIN on V4M are
> > > > >     also supported by V4H and the driver code can be shared. But as done for
> > > > >     other R-Car IP bindings a new dedicated binding for V4M is created.
> > > > >     This have proved prudent in the past where quirks are found even within
> > > > >     the same generation as more advance use-cases are enabled.
> > > >
> > > > I don't understand how this precludes using the g variant as a fallback
> > > > compatible. I'm not suggesting that you don't add a specific one for the
> > > > h variant.
> > >
> > > The bindings have been around for a while and currently there are 25 SoC
> > > specific compatibles, one for each SoC supported. Each compatible
> > > consist of the SoC model number, not the VIN IP model/version number as
> > > no such versioning schema exist.
> > >
> > > The datasheets are specific for each SoC and there are differences
> > > between almost every SoC. There are of course lots of similarities
> > > between the SoCs and the similarities are cluster around the 3
> > > generations (Gen{2,3,4}) supported.
> > >
> > > Using the g variant as fallback in DTS for h variant even if we also add
> > > a specific one for h is confusing. As g and h are two different SoC.
> >
> > Why? That is the very definition of how "compatible" is supposed to
> > work.
> >
> > > The g variant is r8a779g0 which is the SoC name/number for V4H.
> > > The h variant is r8a779h0 which is the SoC name/number for V4M.
> > >
> > > I think the core of the problem is that there are no versioning schema
> > > for the individual IP blocks used on each SoC. For better or worse the
> > > bindings for lots of Renesas IPs are centred around SoC name/number and
> > > not the individual IP implementations.
> >
> > We've tried IP version based compatibles before. It doesn't work. Guess
> > what, the IP version changes with nearly every SoC. Chip designers have
> > no discipline.
> 
> The R-Car V4M capture pipeline is similar to e.g. the R-Car V4H capture
> pipeline. But it is not identical, hence the different compatible values.
> AFAIU, for the current feature-set, the driver does not need to handle
> the differences.  But that may change later...

How can I best move forward here? The proposed compatible is not a IP 
block specific one, but a SoC specific one. This is the design used for 
the R-Car video capture pipeline and we already use 25 of them to 
support different SoCs in the R-Car Gen1, Gen2, Gen3 and Gen4 families 
using the schema proposed in this patch.

If I understand the feedback correctly there is not so much an issue 
with adding this new compatible. Rather that the driver do not use a 
fallback compatible as currently the compatible added by this patch, as 
the driver currently treats it the same as another SoC in the R-Car Gen4 
family. Have I understand the issue correctly?

If so, then yes the driver currently treats it the same as another Gen4 
SoC. But we already know there are differences between the video capture 
pipeline in these SoCs, however the driver do not yet cover these parts.  
So going the fallback compatible route now could create comp ability 
issues down the road. Is it not better to do the specific thing now and 
avoid that issue all together?

-- 
Kind Regards,
Niklas Söderlund

Re: [PATCH v2 1/2] dt-bindings: media: renesas,vin: Add binding for V4M

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 6373 bytes --]

On Tue, Jun 18, 2024 at 03:57:53PM +0200, Niklas Söderlund wrote:
> Hi All,
> 
> On 2024-06-13 21:35:13 +0200, Geert Uytterhoeven wrote:
> > Hi Rob, Conor,
> > 
> > On Thu, Jun 13, 2024 at 6:51 PM Rob Herring <robh@kernel.org> wrote:
> > > On Tue, Jun 11, 2024 at 01:06:17PM +0200, Niklas Söderlund wrote:
> > > > On 2024-06-10 22:32:29 +0100, Conor Dooley wrote:
> > > > > On Mon, Jun 10, 2024 at 06:59:35PM +0200, Niklas Söderlund wrote:
> > > > > > On 2024-06-10 17:03:49 +0100, Conor Dooley wrote:
> > > > > > > On Mon, Jun 10, 2024 at 01:31:23PM +0200, Niklas Söderlund wrote:
> > > > > > > > Document support for the VIN module in the Renesas V4M (r8a779h0) SoC.
> > > > > > > >
> > > > > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > > > > > > > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > > > > > > > ---
> > > > > > > >  Documentation/devicetree/bindings/media/renesas,vin.yaml | 1 +
> > > > > > > >  1 file changed, 1 insertion(+)
> > > > > > > >
> > > > > > > > diff --git a/Documentation/devicetree/bindings/media/renesas,vin.yaml b/Documentation/devicetree/bindings/media/renesas,vin.yaml
> > > > > > > > index 5539d0f8e74d..168cb02f8abe 100644
> > > > > > > > --- a/Documentation/devicetree/bindings/media/renesas,vin.yaml
> > > > > > > > +++ b/Documentation/devicetree/bindings/media/renesas,vin.yaml
> > > > > > > > @@ -54,6 +54,7 @@ properties:
> > > > > > > >                - renesas,vin-r8a77995 # R-Car D3
> > > > > > > >                - renesas,vin-r8a779a0 # R-Car V3U
> > > > > > > >                - renesas,vin-r8a779g0 # R-Car V4H
> > > > > > > > +              - renesas,vin-r8a779h0 # R-Car V4M
> > > > > > >
> > > > > > > Your driver patch suggests that this is compatible with the g variant.
> > > > > >
> > > > > > Currently it is. But that not always be true, I tried to outline this in
> > > > > > to cover letter.
> > > > >
> > > > > To be honest, I don't usually read cover letters when reviewing bindings.
> > > > > Information about why things are/are not compatible should be in a
> > > > > commit itself.
> > > > >
> > > > > >     The V4M capture pipeline is similar to the other Gen4 SoC supported
> > > > > >     upstream already V4H. Currently all futures supported for VIN on V4M are
> > > > > >     also supported by V4H and the driver code can be shared. But as done for
> > > > > >     other R-Car IP bindings a new dedicated binding for V4M is created.
> > > > > >     This have proved prudent in the past where quirks are found even within
> > > > > >     the same generation as more advance use-cases are enabled.
> > > > >
> > > > > I don't understand how this precludes using the g variant as a fallback
> > > > > compatible. I'm not suggesting that you don't add a specific one for the
> > > > > h variant.
> > > >
> > > > The bindings have been around for a while and currently there are 25 SoC
> > > > specific compatibles, one for each SoC supported. Each compatible
> > > > consist of the SoC model number, not the VIN IP model/version number as
> > > > no such versioning schema exist.
> > > >
> > > > The datasheets are specific for each SoC and there are differences
> > > > between almost every SoC. There are of course lots of similarities
> > > > between the SoCs and the similarities are cluster around the 3
> > > > generations (Gen{2,3,4}) supported.
> > > >
> > > > Using the g variant as fallback in DTS for h variant even if we also add
> > > > a specific one for h is confusing. As g and h are two different SoC.
> > >
> > > Why? That is the very definition of how "compatible" is supposed to
> > > work.
> > >
> > > > The g variant is r8a779g0 which is the SoC name/number for V4H.
> > > > The h variant is r8a779h0 which is the SoC name/number for V4M.
> > > >
> > > > I think the core of the problem is that there are no versioning schema
> > > > for the individual IP blocks used on each SoC. For better or worse the
> > > > bindings for lots of Renesas IPs are centred around SoC name/number and
> > > > not the individual IP implementations.
> > >
> > > We've tried IP version based compatibles before. It doesn't work. Guess
> > > what, the IP version changes with nearly every SoC. Chip designers have
> > > no discipline.
> > 
> > The R-Car V4M capture pipeline is similar to e.g. the R-Car V4H capture
> > pipeline. But it is not identical, hence the different compatible values.
> > AFAIU, for the current feature-set, the driver does not need to handle
> > the differences.  But that may change later...
> 
> How can I best move forward here? The proposed compatible is not a IP 
> block specific one, but a SoC specific one. This is the design used for 
> the R-Car video capture pipeline and we already use 25 of them to 
> support different SoCs in the R-Car Gen1, Gen2, Gen3 and Gen4 families 
> using the schema proposed in this patch.
> 
> If I understand the feedback correctly there is not so much an issue 
> with adding this new compatible. Rather that the driver do not use a 
> fallback compatible as currently the compatible added by this patch, as 
> the driver currently treats it the same as another SoC in the R-Car Gen4 
> family. Have I understand the issue correctly?

Yes, there's __nothing__ wrong with a specific compatible (in fact they
are encouraged), it's the adding of a specific compatible without a
fallback when the driver change indicates that a fallback is suitable.

> If so, then yes the driver currently treats it the same as another Gen4 
> SoC. But we already know there are differences between the video capture 
> pipeline in these SoCs, however the driver do not yet cover these parts.  
> So going the fallback compatible route now could create comp ability 
> issues down the road. Is it not better to do the specific thing now and 
> avoid that issue all together?

It entirely depends on what the variances are. If the H variant adds
some new features that the G one does not have, then there's not gonna
be a compatibility issue down the road. If it is the other way around,
or both variants have unique elements, then the fallback wouldn't be
suitable. The onus would be is on you to explain why in your commit
message for the binding if it one of the latter two cases.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2 2/2] media: rcar-vin: Add support for R-Car V4M

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Mon, Jun 10, 2024 at 01:31:24PM +0200, Niklas Söderlund wrote:
> Add support for R-Car V4M. The V4M is similar to V4H and uses the ISP
> Channel Selector as its only possible video input source. Reuse and
> rename the info structure from V4H to cover all current Gen4 SoCs.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

I would normally take this in my tree and send a pull request, but a
decision on the DT bindings is needed first. I may miss the outcome if I
don't get CC'ed on v2, but I'm sure Hans or Sakari can also take the
patches.

> ---
> * Changes since v1
> - Create a shared Gen4 info strucutre.
> ---
>  drivers/media/platform/renesas/rcar-vin/rcar-core.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/renesas/rcar-vin/rcar-core.c b/drivers/media/platform/renesas/rcar-vin/rcar-core.c
> index 809c3a38cc4a..6992b61f0d48 100644
> --- a/drivers/media/platform/renesas/rcar-vin/rcar-core.c
> +++ b/drivers/media/platform/renesas/rcar-vin/rcar-core.c
> @@ -1283,7 +1283,7 @@ static const struct rvin_info rcar_info_r8a779a0 = {
>  	.max_height = 4096,
>  };
>  
> -static const struct rvin_info rcar_info_r8a779g0 = {
> +static const struct rvin_info rcar_info_gen4 = {
>  	.model = RCAR_GEN3,
>  	.use_mc = true,
>  	.use_isp = true,
> @@ -1359,7 +1359,11 @@ static const struct of_device_id rvin_of_id_table[] = {
>  	},
>  	{
>  		.compatible = "renesas,vin-r8a779g0",
> -		.data = &rcar_info_r8a779g0,
> +		.data = &rcar_info_gen4,
> +	},
> +	{
> +		.compatible = "renesas,vin-r8a779h0",
> +		.data = &rcar_info_gen4,
>  	},
>  	{ /* Sentinel */ },
>  };

-- 
Regards,

Laurent Pinchart