| Message ID | 20220826184144.605605-6-paul.kocialkowski@bootlin.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Allwinner A31/A83T MIPI CSI-2 and A31 ISP / ISP Driver | expand |
Hi Paul, Thank you for the patch. On Fri, Aug 26, 2022 at 08:41:43PM +0200, Paul Kocialkowski wrote: > Add a helper to detect whether the ISP is available and connected > and store the indication in a driver-wide variable. This sounds like it would be a global variable, while it's stored in the driver-specific device structure. > > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com> > --- > .../platform/sunxi/sun6i-csi/sun6i_csi.c | 33 +++++++++++++++++++ > .../platform/sunxi/sun6i-csi/sun6i_csi.h | 3 ++ > 2 files changed, 36 insertions(+) > > diff --git a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c > index 00521f966cee..b16166cba2ef 100644 > --- a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c > +++ b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c > @@ -24,6 +24,35 @@ > #include "sun6i_csi_capture.h" > #include "sun6i_csi_reg.h" > > +/* ISP */ > + > +static bool sun6i_csi_isp_detect(struct sun6i_csi_device *csi_dev) > +{ > + struct device *dev = csi_dev->dev; > + struct fwnode_handle *handle = NULL; No need to initialize this to NULL. > + > + /* ISP is not available if disabled in kernel config. */ > + if (!IS_ENABLED(CONFIG_VIDEO_SUN6I_ISP)) > + return 0; Hmmm... The ISP driver may be disabled when compiling the sun6i-csi driver, but later enabled and deployed. Disabling ISP support silently like this could be confusing. Could it be better to move this check after the graph check, and print a warning message in this case ? > + > + /* > + * ISP is not available if not connected via fwnode graph. > + * This weill also check that the remote parent node is available. s/weill/will/ * ISP is not available if not connected via fwnode graph. This will * also check that the remote parent node is available. > + */ > + handle = fwnode_graph_get_endpoint_by_id(dev_fwnode(dev), > + SUN6I_CSI_PORT_ISP, 0, > + FWNODE_GRAPH_ENDPOINT_NEXT); > + if (!handle) > + return 0; > + > + fwnode_handle_put(handle); > + > + dev_info(dev, "ISP link is available\n"); You could make that a debug message, it's not crucial information that needs to be printed when the driver is loaded. If you prefer keeping an info message, then I'd move it to the probe function and print that the CSI has been probed, and indicate in that message if the ISP is available. > + csi_dev->isp_available = true; > + > + return 0; > +} > + > /* Media */ > > static const struct media_device_ops sun6i_csi_media_ops = { > @@ -290,6 +319,10 @@ static int sun6i_csi_probe(struct platform_device *platform_dev) > if (ret) > return ret; > > + ret = sun6i_csi_isp_detect(csi_dev); > + if (ret) > + goto error_resources; > + > ret = sun6i_csi_v4l2_setup(csi_dev); > if (ret) > goto error_resources; > diff --git a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.h b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.h > index e611bdd6e9b2..8e232cd91ebe 100644 > --- a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.h > +++ b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.h > @@ -21,6 +21,7 @@ > enum sun6i_csi_port { > SUN6I_CSI_PORT_PARALLEL = 0, > SUN6I_CSI_PORT_MIPI_CSI2 = 1, > + SUN6I_CSI_PORT_ISP = 2, > }; > > struct sun6i_csi_buffer { > @@ -44,6 +45,8 @@ struct sun6i_csi_device { > struct clk *clock_mod; > struct clk *clock_ram; > struct reset_control *reset; > + > + bool isp_available; > }; > > struct sun6i_csi_variant {
Hi Laurent, On Sat 27 Aug 22, 01:39, Laurent Pinchart wrote: > Hi Paul, > > Thank you for the patch. Thanks for the review! > On Fri, Aug 26, 2022 at 08:41:43PM +0200, Paul Kocialkowski wrote: > > Add a helper to detect whether the ISP is available and connected > > and store the indication in a driver-wide variable. > > This sounds like it would be a global variable, while it's stored in the > driver-specific device structure. Okay I can clarify the commit message here. > > > > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com> > > --- > > .../platform/sunxi/sun6i-csi/sun6i_csi.c | 33 +++++++++++++++++++ > > .../platform/sunxi/sun6i-csi/sun6i_csi.h | 3 ++ > > 2 files changed, 36 insertions(+) > > > > diff --git a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c > > index 00521f966cee..b16166cba2ef 100644 > > --- a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c > > +++ b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c > > @@ -24,6 +24,35 @@ > > #include "sun6i_csi_capture.h" > > #include "sun6i_csi_reg.h" > > > > +/* ISP */ > > + > > +static bool sun6i_csi_isp_detect(struct sun6i_csi_device *csi_dev) > > +{ > > + struct device *dev = csi_dev->dev; > > + struct fwnode_handle *handle = NULL; > > No need to initialize this to NULL. Indeed. > > + > > + /* ISP is not available if disabled in kernel config. */ > > + if (!IS_ENABLED(CONFIG_VIDEO_SUN6I_ISP)) > > + return 0; > > Hmmm... The ISP driver may be disabled when compiling the sun6i-csi > driver, but later enabled and deployed. Disabling ISP support silently > like this could be confusing. Could it be better to move this check > after the graph check, and print a warning message in this case ? Yeah I'm not too surprised corner cases like this can exist. Agreed that printing a warning message would be good, but I don't follow the point of moving the check later on. Do you have something in mind there? > > + > > + /* > > + * ISP is not available if not connected via fwnode graph. > > + * This weill also check that the remote parent node is available. > > s/weill/will/ Good catch, thanks! > * ISP is not available if not connected via fwnode graph. This will > * also check that the remote parent node is available. > > > + */ > > + handle = fwnode_graph_get_endpoint_by_id(dev_fwnode(dev), > > + SUN6I_CSI_PORT_ISP, 0, > > + FWNODE_GRAPH_ENDPOINT_NEXT); > > + if (!handle) > > + return 0; > > + > > + fwnode_handle_put(handle); > > + > > + dev_info(dev, "ISP link is available\n"); > > You could make that a debug message, it's not crucial information that > needs to be printed when the driver is loaded. If you prefer keeping an > info message, then I'd move it to the probe function and print that the > CSI has been probed, and indicate in that message if the ISP is > available. You're right, let's make this debug. It's more the opposite case that is worth a warning message. Thanks, Paul > > + csi_dev->isp_available = true; > > + > > + return 0; > > +} > > + > > /* Media */ > > > > static const struct media_device_ops sun6i_csi_media_ops = { > > @@ -290,6 +319,10 @@ static int sun6i_csi_probe(struct platform_device *platform_dev) > > if (ret) > > return ret; > > > > + ret = sun6i_csi_isp_detect(csi_dev); > > + if (ret) > > + goto error_resources; > > + > > ret = sun6i_csi_v4l2_setup(csi_dev); > > if (ret) > > goto error_resources; > > diff --git a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.h b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.h > > index e611bdd6e9b2..8e232cd91ebe 100644 > > --- a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.h > > +++ b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.h > > @@ -21,6 +21,7 @@ > > enum sun6i_csi_port { > > SUN6I_CSI_PORT_PARALLEL = 0, > > SUN6I_CSI_PORT_MIPI_CSI2 = 1, > > + SUN6I_CSI_PORT_ISP = 2, > > }; > > > > struct sun6i_csi_buffer { > > @@ -44,6 +45,8 @@ struct sun6i_csi_device { > > struct clk *clock_mod; > > struct clk *clock_ram; > > struct reset_control *reset; > > + > > + bool isp_available; > > }; > > > > struct sun6i_csi_variant { > > -- > Regards, > > Laurent Pinchart
Hi Paul, On Thu, Sep 01, 2022 at 05:11:00PM +0200, Paul Kocialkowski wrote: > On Sat 27 Aug 22, 01:39, Laurent Pinchart wrote: > > On Fri, Aug 26, 2022 at 08:41:43PM +0200, Paul Kocialkowski wrote: > > > Add a helper to detect whether the ISP is available and connected > > > and store the indication in a driver-wide variable. > > > > This sounds like it would be a global variable, while it's stored in the > > driver-specific device structure. > > Okay I can clarify the commit message here. > > > > > > > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com> > > > --- > > > .../platform/sunxi/sun6i-csi/sun6i_csi.c | 33 +++++++++++++++++++ > > > .../platform/sunxi/sun6i-csi/sun6i_csi.h | 3 ++ > > > 2 files changed, 36 insertions(+) > > > > > > diff --git a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c > > > index 00521f966cee..b16166cba2ef 100644 > > > --- a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c > > > +++ b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c > > > @@ -24,6 +24,35 @@ > > > #include "sun6i_csi_capture.h" > > > #include "sun6i_csi_reg.h" > > > > > > +/* ISP */ > > > + > > > +static bool sun6i_csi_isp_detect(struct sun6i_csi_device *csi_dev) > > > +{ > > > + struct device *dev = csi_dev->dev; > > > + struct fwnode_handle *handle = NULL; > > > > No need to initialize this to NULL. > > Indeed. > > > > + > > > + /* ISP is not available if disabled in kernel config. */ > > > + if (!IS_ENABLED(CONFIG_VIDEO_SUN6I_ISP)) > > > + return 0; > > > > Hmmm... The ISP driver may be disabled when compiling the sun6i-csi > > driver, but later enabled and deployed. Disabling ISP support silently > > like this could be confusing. Could it be better to move this check > > after the graph check, and print a warning message in this case ? > > Yeah I'm not too surprised corner cases like this can exist. > Agreed that printing a warning message would be good, but I don't follow the > point of moving the check later on. Do you have something in mind there? I meant that the warning should only be printed of there's an endpoint connected to the ISP, so I'd first check if the endpoint is present, return 0 if it isn't, and then check if the ISP driver is enabled and print a warning if it isn't. > > > + > > > + /* > > > + * ISP is not available if not connected via fwnode graph. > > > + * This weill also check that the remote parent node is available. > > > > s/weill/will/ > > Good catch, thanks! > > > * ISP is not available if not connected via fwnode graph. This will > > * also check that the remote parent node is available. > > > > > + */ > > > + handle = fwnode_graph_get_endpoint_by_id(dev_fwnode(dev), > > > + SUN6I_CSI_PORT_ISP, 0, > > > + FWNODE_GRAPH_ENDPOINT_NEXT); > > > + if (!handle) > > > + return 0; > > > + > > > + fwnode_handle_put(handle); > > > + > > > + dev_info(dev, "ISP link is available\n"); > > > > You could make that a debug message, it's not crucial information that > > needs to be printed when the driver is loaded. If you prefer keeping an > > info message, then I'd move it to the probe function and print that the > > CSI has been probed, and indicate in that message if the ISP is > > available. > > You're right, let's make this debug. It's more the opposite case that is worth > a warning message. > > > > + csi_dev->isp_available = true; > > > + > > > + return 0; > > > +} > > > + > > > /* Media */ > > > > > > static const struct media_device_ops sun6i_csi_media_ops = { > > > @@ -290,6 +319,10 @@ static int sun6i_csi_probe(struct platform_device *platform_dev) > > > if (ret) > > > return ret; > > > > > > + ret = sun6i_csi_isp_detect(csi_dev); > > > + if (ret) > > > + goto error_resources; > > > + > > > ret = sun6i_csi_v4l2_setup(csi_dev); > > > if (ret) > > > goto error_resources; > > > diff --git a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.h b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.h > > > index e611bdd6e9b2..8e232cd91ebe 100644 > > > --- a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.h > > > +++ b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.h > > > @@ -21,6 +21,7 @@ > > > enum sun6i_csi_port { > > > SUN6I_CSI_PORT_PARALLEL = 0, > > > SUN6I_CSI_PORT_MIPI_CSI2 = 1, > > > + SUN6I_CSI_PORT_ISP = 2, > > > }; > > > > > > struct sun6i_csi_buffer { > > > @@ -44,6 +45,8 @@ struct sun6i_csi_device { > > > struct clk *clock_mod; > > > struct clk *clock_ram; > > > struct reset_control *reset; > > > + > > > + bool isp_available; > > > }; > > > > > > struct sun6i_csi_variant {
diff --git a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c index 00521f966cee..b16166cba2ef 100644 --- a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c +++ b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c @@ -24,6 +24,35 @@ #include "sun6i_csi_capture.h" #include "sun6i_csi_reg.h" +/* ISP */ + +static bool sun6i_csi_isp_detect(struct sun6i_csi_device *csi_dev) +{ + struct device *dev = csi_dev->dev; + struct fwnode_handle *handle = NULL; + + /* ISP is not available if disabled in kernel config. */ + if (!IS_ENABLED(CONFIG_VIDEO_SUN6I_ISP)) + return 0; + + /* + * ISP is not available if not connected via fwnode graph. + * This weill also check that the remote parent node is available. + */ + handle = fwnode_graph_get_endpoint_by_id(dev_fwnode(dev), + SUN6I_CSI_PORT_ISP, 0, + FWNODE_GRAPH_ENDPOINT_NEXT); + if (!handle) + return 0; + + fwnode_handle_put(handle); + + dev_info(dev, "ISP link is available\n"); + csi_dev->isp_available = true; + + return 0; +} + /* Media */ static const struct media_device_ops sun6i_csi_media_ops = { @@ -290,6 +319,10 @@ static int sun6i_csi_probe(struct platform_device *platform_dev) if (ret) return ret; + ret = sun6i_csi_isp_detect(csi_dev); + if (ret) + goto error_resources; + ret = sun6i_csi_v4l2_setup(csi_dev); if (ret) goto error_resources; diff --git a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.h b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.h index e611bdd6e9b2..8e232cd91ebe 100644 --- a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.h +++ b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.h @@ -21,6 +21,7 @@ enum sun6i_csi_port { SUN6I_CSI_PORT_PARALLEL = 0, SUN6I_CSI_PORT_MIPI_CSI2 = 1, + SUN6I_CSI_PORT_ISP = 2, }; struct sun6i_csi_buffer { @@ -44,6 +45,8 @@ struct sun6i_csi_device { struct clk *clock_mod; struct clk *clock_ram; struct reset_control *reset; + + bool isp_available; }; struct sun6i_csi_variant {
Add a helper to detect whether the ISP is available and connected and store the indication in a driver-wide variable. Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com> --- .../platform/sunxi/sun6i-csi/sun6i_csi.c | 33 +++++++++++++++++++ .../platform/sunxi/sun6i-csi/sun6i_csi.h | 3 ++ 2 files changed, 36 insertions(+)