Re: [PATCH v1 04/17] media: camss: Make ISPIF subdevice optional

[Robert Foss]

Thanks Björn!

On Fri, 8 Jan 2021 at 20:07, Bjorn Andersson <bjorn.andersson@xxxxxxxxxx> wrote:
>
> On Fri 08 Jan 06:04 CST 2021, Robert Foss wrote:
>
> > This driver supports multiple architecture versions of the Qualcomm ISP.
> > The CAMSS architecure which this driver is name after, and with the
> > introduction of this series, the Titan architecture.
> >
> > The ISPIF is IP-block that is only present in the CAMSS architecture.
>
> "is an IP-block"

Ack

I cleaned up the message a little bit further in order to be more
clear about camss & titan being the names of architecture generations.

>
> > In order to support the Titan architecture, make the ISPIF an optional
> > subdevice.
> >
> > Signed-off-by: Robert Foss <robert.foss@xxxxxxxxxx>
> > ---
> >  .../media/platform/qcom/camss/camss-ispif.c   | 144 ++++++++++--------
> >  .../media/platform/qcom/camss/camss-ispif.h   |   3 +-
> >  drivers/media/platform/qcom/camss/camss.c     | 113 +++++++++-----
> >  drivers/media/platform/qcom/camss/camss.h     |   2 +-
> >  4 files changed, 160 insertions(+), 102 deletions(-)
> >
> > diff --git a/drivers/media/platform/qcom/camss/camss-ispif.c b/drivers/media/platform/qcom/camss/camss-ispif.c
> [..]
> > -int msm_ispif_subdev_init(struct ispif_device *ispif,
> > +int msm_ispif_subdev_init(struct camss *camss,
> >                         const struct resources_ispif *res)
> >  {
> > -     struct device *dev = to_device(ispif);
> > -     struct platform_device *pdev = to_platform_device(dev);
> > +     struct ispif_device *ispif = camss->ispif;
> > +     struct platform_device *pdev = to_platform_device(camss->dev);
>
> It seems like several of the changes in this function is replacing
> dev with camss->dev. If you retained a struct device *dev = camss->dev;
> you would avoid this.

Ack.

>
> >       struct resource *r;
> >       int i;
> >       int ret;
> >
> > +     if (res == NULL && ispif == NULL)
>
> Afaict this function is called conditional on camss->ispif != NULL, and
> I don't see anything that would cause res to becomes NULL if is hasn't
> been before this change.
>
> So I think this check is unnecessary?

Nice catch, thank you!

>
> > +             return 0;
> > +
> > +     ispif->camss = camss;
> > +
> >       /* Number of ISPIF lines - same as number of CSID hardware modules */
> > -     if (to_camss(ispif)->version == CAMSS_8x16)
> > +     if (camss->version == CAMSS_8x16)
> >               ispif->line_num = 2;
> > -     else if (to_camss(ispif)->version == CAMSS_8x96 ||
> > -              to_camss(ispif)->version == CAMSS_660)
> > +     else if (camss->version == CAMSS_8x96 ||
> > +              camss->version == CAMSS_660)
> >               ispif->line_num = 4;
> >       else
> >               return -EINVAL;
> >
> > -     ispif->line = devm_kcalloc(dev, ispif->line_num, sizeof(*ispif->line),
> > -                                GFP_KERNEL);
> > +     ispif->line = devm_kcalloc(camss->dev, ispif->line_num,
> > +                     sizeof(*ispif->line), GFP_KERNEL);
> >       if (!ispif->line)
> >               return -ENOMEM;
> >
> [..]
> > @@ -1393,6 +1410,9 @@ void msm_ispif_unregister_entities(struct ispif_device *ispif)
> >  {
> >       int i;
> >
> > +     if (!ispif)
> > +             return;
>
> I like this, but later in the patch you make the calls to this function
> conditional on ispif != NULL. You should only need one of the checks.

Ack, removing the external checks and keeping the internal one then.