Hi Geert,
Thanks for the feedback.
> Subject: Re: [PATCH 3/3] media: vsp1: Add support for RZ/G2L VSPD
>
> Hi Biju,
>
> On Wed, Mar 9, 2022 at 8:45 PM Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> wrote:
> > The RZ/G2L VSPD provides a single VSPD instance. It has the following
> > sub modules MAU, CTU, RPF, DPR, LUT, BRS, WPF and LIF.
> >
> > The VSPD block on RZ/G2L does not have a version register, so added a
> > new compatible string "renesas,vsp2-rzg2l" with a data pointer
> > containing the info structure. Also the reset line is shared with the
> > DU module so devm_reset_control_get_shared() call is used in case of
> RZ/G2L.
> >
> > Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> > Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> > ---
> > RFC->v1:
> > * Used data pointer containing info structure to retrieve version
> > information
> > RFC:
> > *
>
> Thanks for the update!
>
> > --- a/drivers/media/platform/vsp1/vsp1_drv.c
> > +++ b/drivers/media/platform/vsp1/vsp1_drv.c
>
> > @@ -841,7 +849,14 @@ static int vsp1_probe(struct platform_device *pdev)
> > if (irq < 0)
> > return irq;
> >
> > - vsp1->rstc = devm_reset_control_get_exclusive(&pdev->dev, NULL);
> > + vsp1->info = of_device_get_match_data(&pdev->dev);
> > + if (vsp1->info) {
> > + vsp1->version = vsp1->info->version;
> > + vsp1->rstc = devm_reset_control_get_shared(&pdev->dev,
> NULL);
> > + } else {
> > + vsp1->rstc =
> > + devm_reset_control_get_exclusive(&pdev->dev, NULL);
>
> Making the reset control shared or exclusive dependent on the presence of
> match data looks fragile to me. I think you want to check the IP version
> instead (ideally, the SoC, as this is an integration feature).
> Or just make it shared unconditionally (in the previous patch)?
Agreed.
>
> > + }
> > +
> > if (IS_ERR(vsp1->rstc))
> > return dev_err_probe(&pdev->dev, PTR_ERR(vsp1->rstc),
> > "failed to get reset ctrl\n"); @@
> > -874,13 +889,15 @@ static int vsp1_probe(struct platform_device *pdev)
> > if (ret < 0)
> > goto done;
> >
> > - vsp1->version = vsp1_read(vsp1, VI6_IP_VERSION);
> > + if (!vsp1->info) {
> > + vsp1->version = vsp1_read(vsp1, VI6_IP_VERSION);
> >
> > - for (i = 0; i < ARRAY_SIZE(vsp1_device_infos); ++i) {
> > - if ((vsp1->version & VI6_IP_VERSION_MODEL_MASK) ==
> > - vsp1_device_infos[i].version) {
> > - vsp1->info = &vsp1_device_infos[i];
> > - break;
> > + for (i = 0; i < ARRAY_SIZE(vsp1_device_infos); ++i) {
> > + if ((vsp1->version & VI6_IP_VERSION_MODEL_MASK)
> ==
> > + vsp1_device_infos[i].version) {
> > + vsp1->info = &vsp1_device_infos[i];
> > + break;
> > + }
> > }
> > }
> >
> > @@ -943,6 +960,7 @@ static int vsp1_remove(struct platform_device
> > *pdev) static const struct of_device_id vsp1_of_match[] = {
> > { .compatible = "renesas,vsp1" },
> > { .compatible = "renesas,vsp2" },
> > + { .compatible = "renesas,vsp2-rzg2l", .data =
> > + &vsp1_device_infos[14] },
> > { },
>
> Is VI6_IP_VERSION_MODEL_VSPD_RZG2L = 0x1b an official number?
> If yes, it might make sense to change the compatible value to
> "renesas,vsp2-0x1b".
No, it is not official one. I just use 0x1b as no one claimed it.
Cheers,
Biju
