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

Thanks Björn!

On Fri, 8 Jan 2021 at 20:07, Bjorn Andersson [off-list ref] wrote:

On Fri 08 Jan 06:04 CST 2021, Robert Foss wrote:

quoted

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.

quoted

In order to support the Titan architecture, make the ISPIF an optional
subdevice.

Signed-off-by: Robert Foss <redacted>
---
 .../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

[..]

quoted

-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.

quoted

      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!

quoted

+             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;

[..]

quoted

@@ -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.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel