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" > In order to support the Titan architecture, make the ISPIF an optional > subdevice. > > Signed-off-by: Robert Foss <[email protected]> > --- > .../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; > [..] > @@ -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
