Hi Sakari,
On Monday 14 Aug 2017 13:53:27 Sakari Ailus wrote:
> On Fri, Aug 11, 2017 at 02:32:00PM +0300, Laurent Pinchart wrote:
> > On Friday 11 Aug 2017 12:57:09 Sakari Ailus wrote:
> >> If the CSI-2 receiver isn't part of the pipeline (or isn't there to
> >> begin with), skip its initialisation.
> >
> > I don't think the commit message really describes the patch.
> >
> >> Signed-off-by: Sakari Ailus <[email protected]>
> >> Tested-by: Laurent Pinchart <[email protected]> # on
> >> Beagleboard-xM + MPT9P031 Acked-by: Pavel Machek <[email protected]>
> >> ---
> >> since v1.1:
> >>
> >> - Assign phy->entity before calling omap3isp_csiphy_config(), for
> >>
> >> phy->entity is used by omap3isp_csiphy_config(). (Thanks to Pavel for
> >> spotting this.)
> >>
> >> drivers/media/platform/omap3isp/ispccp2.c | 2 +-
> >> drivers/media/platform/omap3isp/ispcsi2.c | 4 +--
> >> drivers/media/platform/omap3isp/ispcsiphy.c | 38 ++++++++++------------
> >> drivers/media/platform/omap3isp/ispcsiphy.h | 6 +++--
> >> 4 files changed, 27 insertions(+), 23 deletions(-)
> >>
[snip]
> >> diff --git a/drivers/media/platform/omap3isp/ispcsiphy.c
> >> b/drivers/media/platform/omap3isp/ispcsiphy.c index
> >> 2028bb519108..aedd88fa8246 100644
> >> --- a/drivers/media/platform/omap3isp/ispcsiphy.c
> >> +++ b/drivers/media/platform/omap3isp/ispcsiphy.c
> >> @@ -164,15 +164,18 @@ static int csiphy_set_power(struct isp_csiphy
> >> *phy, u32 power)
> >>
> >> static int omap3isp_csiphy_config(struct isp_csiphy *phy)
> >> {
> >> - struct isp_csi2_device *csi2 = phy->csi2;
> >> - struct isp_pipeline *pipe = to_isp_pipeline(&csi2->subdev.entity);
> >> - struct isp_bus_cfg *buscfg = pipe->external->host_priv;
> >> + struct isp_pipeline *pipe = to_isp_pipeline(phy->entity);
> >> + struct isp_bus_cfg *buscfg;
> >> struct isp_csiphy_lanes_cfg *lanes;
> >> int csi2_ddrclk_khz;
> >> unsigned int num_data_lanes, used_lanes = 0;
> >> unsigned int i;
> >> u32 reg;
> >>
> >> + if (!pipe)
> >> + return -EBUSY;
> >
> > When can this happen ?
>
> It shouldn't. Just in case, it'd be a driver bug if it did. What would you
> think of adding WARN_ON() here?
I throw WARN_ON()s in when I believe that driver could get it wrong. In this
particular case, given that this function is called from .s_stream() handlers
only, I wonder if the check makes sense at all.
> >> + buscfg = pipe->external->host_priv;
> >> if (!buscfg) {
> >> struct isp_async_subdev *isd =
> >> container_of(pipe->external->asd,
[snip]
--
Regards,
Laurent Pinchart
