Re: [PATCH v1 04/17] media: camss: Make ISPIF subdevice optional
From: Bjorn Andersson <hidden>
Date: 2021-01-08 19:08:19
Also in:
linux-arm-msm, linux-devicetree, linux-media, lkml
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"
quoted hunk ↗ jump to hunk
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
[..]
-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.
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?
+ 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 hunk ↗ jump to hunk
@@ -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. Regards, Bjorn _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel