Messages in this thread | | | Date | Fri, 8 Jan 2021 13:07:17 -0600 | From | Bjorn Andersson <> | Subject | Re: [PATCH v1 04/17] media: camss: Make ISPIF subdevice optional |
| |
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 <robert.foss@linaro.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.
> 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
| |