| Message ID | 20180419101812.30688-2-rui.silva@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | media: staging/imx7: add i.MX7 media driver | expand |
On Thu, Apr 19, 2018 at 11:17:58AM +0100, Rui Miguel Silva wrote: > Some i.MX SoC do not have IPU, like the i.MX7, add to the the media device > infrastructure support to be used in this type of systems that do not have > internal subdevices besides the CSI. > > Signed-off-by: Rui Miguel Silva <rui.silva@linaro.org> > --- > drivers/staging/media/imx/imx-media-dev.c | 16 +++++++++++----- > .../staging/media/imx/imx-media-internal-sd.c | 3 +++ > drivers/staging/media/imx/imx-media.h | 3 +++ > 3 files changed, 17 insertions(+), 5 deletions(-) > > diff --git a/drivers/staging/media/imx/imx-media-dev.c b/drivers/staging/media/imx/imx-media-dev.c > index f67ec8e27093..a8afe0ec4134 100644 > --- a/drivers/staging/media/imx/imx-media-dev.c > +++ b/drivers/staging/media/imx/imx-media-dev.c > @@ -92,6 +92,9 @@ static int imx_media_get_ipu(struct imx_media_dev *imxmd, > struct ipu_soc *ipu; > int ipu_id; > > + if (imxmd->no_ipu_present) It's sort of nicer if variables don't have a negative built in because otherwise you get confusing double negatives like "if (!no_ipu) {". It's not hard to invert the varible in this case, because the only thing we need to change is imx_media_probe() to set: + imxmd->ipu_present = true; regards, dan carpenter -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Dan, Thanks for this and the other reviews. On Thu 19 Apr 2018 at 12:06, Dan Carpenter wrote: > On Thu, Apr 19, 2018 at 11:17:58AM +0100, Rui Miguel Silva > wrote: >> Some i.MX SoC do not have IPU, like the i.MX7, add to the the >> media device >> infrastructure support to be used in this type of systems that >> do not have >> internal subdevices besides the CSI. >> >> Signed-off-by: Rui Miguel Silva <rui.silva@linaro.org> >> --- >> drivers/staging/media/imx/imx-media-dev.c | 16 >> +++++++++++----- >> .../staging/media/imx/imx-media-internal-sd.c | 3 +++ >> drivers/staging/media/imx/imx-media.h | 3 +++ >> 3 files changed, 17 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/staging/media/imx/imx-media-dev.c >> b/drivers/staging/media/imx/imx-media-dev.c >> index f67ec8e27093..a8afe0ec4134 100644 >> --- a/drivers/staging/media/imx/imx-media-dev.c >> +++ b/drivers/staging/media/imx/imx-media-dev.c >> @@ -92,6 +92,9 @@ static int imx_media_get_ipu(struct >> imx_media_dev *imxmd, >> struct ipu_soc *ipu; >> int ipu_id; >> >> + if (imxmd->no_ipu_present) > > It's sort of nicer if variables don't have a negative built in > because > otherwise you get confusing double negatives like "if (!no_ipu) > {". > It's not hard to invert the varible in this case, because the > only thing > we need to change is imx_media_probe() to set: > > + imxmd->ipu_present = true; Yeah, my code was like this till last minute, and I also dislike the double negatives... but since the logic that reset the variable would only be done in a later patch I switched the logic. But You are right I could just had the initialization here to true. Will take this in account in v2. --- Cheers, Rui -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/staging/media/imx/imx-media-dev.c b/drivers/staging/media/imx/imx-media-dev.c index f67ec8e27093..a8afe0ec4134 100644 --- a/drivers/staging/media/imx/imx-media-dev.c +++ b/drivers/staging/media/imx/imx-media-dev.c @@ -92,6 +92,9 @@ static int imx_media_get_ipu(struct imx_media_dev *imxmd, struct ipu_soc *ipu; int ipu_id; + if (imxmd->no_ipu_present) + return 0; + ipu = dev_get_drvdata(csi_sd->dev->parent); if (!ipu) { v4l2_err(&imxmd->v4l2_dev, @@ -481,16 +484,19 @@ static int imx_media_probe(struct platform_device *pdev) goto notifier_cleanup; } - ret = imx_media_add_internal_subdevs(imxmd); - if (ret) { - v4l2_err(&imxmd->v4l2_dev, - "add_internal_subdevs failed with %d\n", ret); - goto notifier_cleanup; + if (!imxmd->no_ipu_present) { + ret = imx_media_add_internal_subdevs(imxmd); + if (ret) { + v4l2_err(&imxmd->v4l2_dev, + "add_internal_subdevs failed with %d\n", ret); + goto notifier_cleanup; + } } /* no subdevs? just bail */ if (imxmd->notifier.num_subdevs == 0) { ret = -ENODEV; + v4l2_err(&imxmd->v4l2_dev, "no subdevs\n"); goto notifier_cleanup; } diff --git a/drivers/staging/media/imx/imx-media-internal-sd.c b/drivers/staging/media/imx/imx-media-internal-sd.c index 0fdc45dbfb76..4a246813b4e1 100644 --- a/drivers/staging/media/imx/imx-media-internal-sd.c +++ b/drivers/staging/media/imx/imx-media-internal-sd.c @@ -238,6 +238,9 @@ int imx_media_create_internal_links(struct imx_media_dev *imxmd, struct media_pad *pad; int i, j, ret; + if (imxmd->no_ipu_present) + return 0; + intsd = find_intsd_by_grp_id(sd->grp_id); if (!intsd) return -ENODEV; diff --git a/drivers/staging/media/imx/imx-media.h b/drivers/staging/media/imx/imx-media.h index 44532cd5b812..0c63132861a0 100644 --- a/drivers/staging/media/imx/imx-media.h +++ b/drivers/staging/media/imx/imx-media.h @@ -147,6 +147,9 @@ struct imx_media_dev { /* for async subdev registration */ struct v4l2_async_notifier notifier; + + /* indicator to if the system lack IPU */ + bool no_ipu_present; }; enum codespace_sel {
Some i.MX SoC do not have IPU, like the i.MX7, add to the the media device infrastructure support to be used in this type of systems that do not have internal subdevices besides the CSI. Signed-off-by: Rui Miguel Silva <rui.silva@linaro.org> --- drivers/staging/media/imx/imx-media-dev.c | 16 +++++++++++----- .../staging/media/imx/imx-media-internal-sd.c | 3 +++ drivers/staging/media/imx/imx-media.h | 3 +++ 3 files changed, 17 insertions(+), 5 deletions(-) -- 2.17.0 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html