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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAG3jFyvVBFLX_1vobyBHtEnVkN0QFSRX3MdwtUnBCXbVwDQ4DQ@mail.gmail.com>
Date:   Wed, 13 Jan 2021 16:02:31 +0100
From:   Robert Foss <robert.foss@...aro.org>
To:     Bjorn Andersson <bjorn.andersson@...aro.org>
Cc:     Andy Gross <agross@...nel.org>, Todor Tomov <todor.too@...il.com>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Rob Herring <robh+dt@...nel.org>, catalin.marinas@....com,
        will@...nel.org, shawnguo@...nel.org, leoyang.li@....com,
        geert+renesas@...der.be, arnd@...db.de, Anson.Huang@....com,
        michael@...le.cc, agx@...xcpu.org, max.oss.09@...il.com,
        linux-arm-msm@...r.kernel.org,
        linux-media <linux-media@...r.kernel.org>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@...r.kernel.org>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        "moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE" 
        <linux-arm-kernel@...ts.infradead.org>,
        AngeloGioacchino Del Regno <kholk11@...il.com>,
        Andrey Konovalov <andrey.konovalov@...aro.org>,
        Tomasz Figa <tfiga@...omium.org>,
        Azam Sadiq Pasha Kapatrala Syed <akapatra@...cinc.com>,
        Sarvesh Sridutt <Sarvesh.Sridutt@...rtwirelesscompute.com>,
        Laurent Pinchart <laurent.pinchart@...asonboard.com>
Subject: Re: [PATCH v1 04/17] media: camss: Make ISPIF subdevice optional

Thanks Björn!

On Fri, 8 Jan 2021 at 20:07, Bjorn Andersson <bjorn.andersson@...aro.org> 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@...aro.org>
> > ---
> >  .../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.