Re: [PATCH v2 8/8] media: qcom: camss: Add sm8550 support

[Depeng Shao]

Hi Bryan,

On 3/21/2024 12:01 AM, Bryan O'Donoghue wrote:
> On 20/03/2024 14:11, Depeng Shao wrote:
> > Add in functional logic throughout the code to support the SM8550.
> >
> > Signed-off-by: Depeng Shao <quic_depengs@xxxxxxxxxxx>
> > ---
> >   .../media/platform/qcom/camss/camss-csid.c    | 19 +++++++++++++++++++
> >   .../media/platform/qcom/camss/camss-csiphy.c  |  1 +
> >   drivers/media/platform/qcom/camss/camss-vfe.c |  7 +++++++
> >   .../media/platform/qcom/camss/camss-video.c   |  1 +
> >   4 files changed, 28 insertions(+)
> >
> > diff --git a/drivers/media/platform/qcom/camss/camss-csid.c 
> > b/drivers/media/platform/qcom/camss/camss-csid.c
> > index eb27d69e89a1..e9203dc15798 100644
> > --- a/drivers/media/platform/qcom/camss/camss-csid.c
> > +++ b/drivers/media/platform/qcom/camss/camss-csid.c
> > @@ -590,6 +590,25 @@ int msm_csid_subdev_init(struct camss *camss, 
> > struct csid_device *csid,
> >               csid->base = camss->vfe[id].base + 
> > VFE_480_LITE_CSID_OFFSET;
> >           else
> >               csid->base = camss->vfe[id].base + VFE_480_CSID_OFFSET;
> > +    } else if (camss->res->version == CAMSS_8550) {
> > +        /* for titan 780, CSID lite registers are inside the VFE lite 
> > region,
> > +         * between the VFE "top" and "bus" registers. this requires
> > +         * VFE to be initialized before CSID
> > +         */
> > +        if (id >= 2)
> > +            csid->base = camss->vfe[id].base;
>
> Hard-coded magic numbers are definitely out.
>
> If you need to differentiate - please include something in the struct 
> resources so that the flag is always available and we don't have to 
> start doing funky magic index/magic number gymnastics.

Sure, will update this part.

> > +        else {
> > +            csid->base =
> > +                devm_platform_ioremap_resource_byname(pdev, 
> > res->reg[0]);
> > +            if (id != 0)
> > +                csid->top_base = camss->csid[0].top_base;
> > +            else
> > +                csid->top_base =
> > +                    devm_platform_ioremap_resource_byname(pdev, 
> > res->reg[1]);
> > +        }
>
> What is the point of hooking the TOP base just to clear our the status 
> registers ?
>
> We take no meaningful action in the ISR that I can see.

The csid top is a new small hardware block, it controls the output of 
CSID, since the CSID can connect to Sensor Front End(SFE) or original 
VFE in CSID gen3.

> > +
> > +        if (IS_ERR(csid->base))
> > +            return PTR_ERR(csid->base);
> >       } else {
> >           csid->base = devm_platform_ioremap_resource_byname(pdev, 
> > res->reg[0]);
> >           if (IS_ERR(csid->base))
> > diff --git a/drivers/media/platform/qcom/camss/camss-csiphy.c 
> > b/drivers/media/platform/qcom/camss/camss-csiphy.c
> > index 45b3a8e5dea4..f35af0dd2147 100644
> > --- a/drivers/media/platform/qcom/camss/camss-csiphy.c
> > +++ b/drivers/media/platform/qcom/camss/camss-csiphy.c
> > @@ -579,6 +579,7 @@ int msm_csiphy_subdev_init(struct camss *camss,
> >       case CAMSS_845:
> >       case CAMSS_8250:
> >       case CAMSS_8280XP:
> > +    case CAMSS_8550:
> >           csiphy->formats = csiphy_formats_sdm845;
> >           csiphy->nformats = ARRAY_SIZE(csiphy_formats_sdm845);
> >           break;
> > diff --git a/drivers/media/platform/qcom/camss/camss-vfe.c 
> > b/drivers/media/platform/qcom/camss/camss-vfe.c
> > index d875237cf244..ff115c5521c6 100644
> > --- a/drivers/media/platform/qcom/camss/camss-vfe.c
> > +++ b/drivers/media/platform/qcom/camss/camss-vfe.c
> > @@ -226,6 +226,7 @@ static u32 vfe_src_pad_code(struct vfe_line *line, 
> > u32 sink_code,
> >       case CAMSS_845:
> >       case CAMSS_8250:
> >       case CAMSS_8280XP:
> > +    case CAMSS_8550:
> >           switch (sink_code) {
> >           case MEDIA_BUS_FMT_YUYV8_1X16:
> >           {
> > @@ -296,6 +297,10 @@ int vfe_reset(struct vfe_device *vfe)
> >       reinit_completion(&vfe->reset_complete);
> > +    // The reset has been moved to csid in 8550
>
> Please run checkpatch.pl on your code before submission C++ are not 
> allowed.
>
> ---
> bod

Thanks,
Depeng