Hi Laurent,
On Fri, Aug 11, 2017 at 02:32:00PM +0300, Laurent Pinchart wrote:
> Hi Sakari,
>
> Thank you for the patch.
>
> 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 <sakari.ailus@xxxxxxxxxxxxxxx>
> > Tested-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> # on
> > Beagleboard-xM + MPT9P031 Acked-by: Pavel Machek <pavel@xxxxxx>
> > ---
> > 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(-)
> >
> > diff --git a/drivers/media/platform/omap3isp/ispccp2.c
> > b/drivers/media/platform/omap3isp/ispccp2.c index
> > 47210b102bcb..3db8df09cd9a 100644
> > --- a/drivers/media/platform/omap3isp/ispccp2.c
> > +++ b/drivers/media/platform/omap3isp/ispccp2.c
> > @@ -841,7 +841,7 @@ static int ccp2_s_stream(struct v4l2_subdev *sd, int
> > enable) switch (enable) {
> > case ISP_PIPELINE_STREAM_CONTINUOUS:
> > if (ccp2->phy) {
> > - ret = omap3isp_csiphy_acquire(ccp2->phy);
> > + ret = omap3isp_csiphy_acquire(ccp2->phy, &sd->entity);
> > if (ret < 0)
> > return ret;
> > }
> > diff --git a/drivers/media/platform/omap3isp/ispcsi2.c
> > b/drivers/media/platform/omap3isp/ispcsi2.c index
> > 7dae2fe0d42d..3ec37fed710b 100644
> > --- a/drivers/media/platform/omap3isp/ispcsi2.c
> > +++ b/drivers/media/platform/omap3isp/ispcsi2.c
> > @@ -490,7 +490,7 @@ int omap3isp_csi2_reset(struct isp_csi2_device *csi2)
> > if (!csi2->available)
> > return -ENODEV;
> >
> > - if (csi2->phy->phy_in_use)
> > + if (csi2->phy->entity)
> > return -EBUSY;
> >
> > isp_reg_set(isp, csi2->regs1, ISPCSI2_SYSCONFIG,
> > @@ -1053,7 +1053,7 @@ static int csi2_set_stream(struct v4l2_subdev *sd, int
> > enable)
> >
> > switch (enable) {
> > case ISP_PIPELINE_STREAM_CONTINUOUS:
> > - if (omap3isp_csiphy_acquire(csi2->phy) < 0)
> > + if (omap3isp_csiphy_acquire(csi2->phy, &sd->entity) < 0)
> > return -ENODEV;
> > if (csi2->output & CSI2_OUTPUT_MEMORY)
> > omap3isp_sbl_enable(isp, OMAP3_ISP_SBL_CSI2A_WRITE);
> > 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?
>
> > + buscfg = pipe->external->host_priv;
> > if (!buscfg) {
> > struct isp_async_subdev *isd =
> > container_of(pipe->external->asd,
> > @@ -222,7 +225,7 @@ static int omap3isp_csiphy_config(struct isp_csiphy
> > *phy) csi2_ddrclk_khz = pipe->external_rate / 1000
> > / (2 * hweight32(used_lanes)) * pipe->external_width;
> >
> > - reg = isp_reg_readl(csi2->isp, phy->phy_regs, ISPCSIPHY_REG0);
> > + reg = isp_reg_readl(phy->isp, phy->phy_regs, ISPCSIPHY_REG0);
> >
> > reg &= ~(ISPCSIPHY_REG0_THS_TERM_MASK |
> > ISPCSIPHY_REG0_THS_SETTLE_MASK);
> > @@ -233,9 +236,9 @@ static int omap3isp_csiphy_config(struct isp_csiphy
> > *phy) reg |= (DIV_ROUND_UP(90 * csi2_ddrclk_khz, 1000000) + 3)
> > << ISPCSIPHY_REG0_THS_SETTLE_SHIFT;
> >
> > - isp_reg_writel(csi2->isp, reg, phy->phy_regs, ISPCSIPHY_REG0);
> > + isp_reg_writel(phy->isp, reg, phy->phy_regs, ISPCSIPHY_REG0);
> >
> > - reg = isp_reg_readl(csi2->isp, phy->phy_regs, ISPCSIPHY_REG1);
> > + reg = isp_reg_readl(phy->isp, phy->phy_regs, ISPCSIPHY_REG1);
> >
> > reg &= ~(ISPCSIPHY_REG1_TCLK_TERM_MASK |
> > ISPCSIPHY_REG1_TCLK_MISS_MASK |
> > @@ -244,10 +247,10 @@ static int omap3isp_csiphy_config(struct isp_csiphy
> > *phy) reg |= TCLK_MISS << ISPCSIPHY_REG1_TCLK_MISS_SHIFT;
> > reg |= TCLK_SETTLE << ISPCSIPHY_REG1_TCLK_SETTLE_SHIFT;
> >
> > - isp_reg_writel(csi2->isp, reg, phy->phy_regs, ISPCSIPHY_REG1);
> > + isp_reg_writel(phy->isp, reg, phy->phy_regs, ISPCSIPHY_REG1);
> >
> > /* DPHY lane configuration */
> > - reg = isp_reg_readl(csi2->isp, phy->cfg_regs, ISPCSI2_PHY_CFG);
> > + reg = isp_reg_readl(phy->isp, phy->cfg_regs, ISPCSI2_PHY_CFG);
> >
> > for (i = 0; i < num_data_lanes; i++) {
> > reg &= ~(ISPCSI2_PHY_CFG_DATA_POL_MASK(i + 1) |
> > @@ -263,12 +266,12 @@ static int omap3isp_csiphy_config(struct isp_csiphy
> > *phy) reg |= lanes->clk.pol << ISPCSI2_PHY_CFG_CLOCK_POL_SHIFT;
> > reg |= lanes->clk.pos << ISPCSI2_PHY_CFG_CLOCK_POSITION_SHIFT;
> >
> > - isp_reg_writel(csi2->isp, reg, phy->cfg_regs, ISPCSI2_PHY_CFG);
> > + isp_reg_writel(phy->isp, reg, phy->cfg_regs, ISPCSI2_PHY_CFG);
> >
> > return 0;
> > }
> >
> > -int omap3isp_csiphy_acquire(struct isp_csiphy *phy)
> > +int omap3isp_csiphy_acquire(struct isp_csiphy *phy, struct media_entity
> > *entity) {
> > int rval;
> >
> > @@ -288,6 +291,8 @@ int omap3isp_csiphy_acquire(struct isp_csiphy *phy)
> > if (rval < 0)
> > goto done;
> >
> > + phy->entity = entity;
> > +
> > rval = omap3isp_csiphy_config(phy);
> > if (rval < 0)
> > goto done;
> > @@ -301,10 +306,9 @@ int omap3isp_csiphy_acquire(struct isp_csiphy *phy)
> >
> > csiphy_power_autoswitch_enable(phy, true);
> > }
> > -
> > - phy->phy_in_use = 1;
> > -
> > done:
> > + phy->entity = NULL;
> > +
>
> Did you really mean to release the PHY here ?
Yes, indeed. Fixed.
>
> > mutex_unlock(&phy->mutex);
> > return rval;
> > }
> > @@ -312,10 +316,8 @@ int omap3isp_csiphy_acquire(struct isp_csiphy *phy)
> > void omap3isp_csiphy_release(struct isp_csiphy *phy)
> > {
> > mutex_lock(&phy->mutex);
> > - if (phy->phy_in_use) {
> > - struct isp_csi2_device *csi2 = phy->csi2;
> > - struct isp_pipeline *pipe =
> > - to_isp_pipeline(&csi2->subdev.entity);
> > + if (phy->entity && phy->entity->pipe) {
> > + struct isp_pipeline *pipe = to_isp_pipeline(phy->entity);
> > struct isp_bus_cfg *buscfg = pipe->external->host_priv;
> >
> > csiphy_routing_cfg(phy, buscfg->interface, false,
> > @@ -325,7 +327,7 @@ void omap3isp_csiphy_release(struct isp_csiphy *phy)
> > csiphy_set_power(phy, ISPCSI2_PHY_CFG_PWR_CMD_OFF);
> > }
> > regulator_disable(phy->vdd);
> > - phy->phy_in_use = 0;
> > + phy->entity = NULL;
> > }
> > mutex_unlock(&phy->mutex);
> > }
> > diff --git a/drivers/media/platform/omap3isp/ispcsiphy.h
> > b/drivers/media/platform/omap3isp/ispcsiphy.h index
> > 978ca5c80a6c..cffda0265767 100644
> > --- a/drivers/media/platform/omap3isp/ispcsiphy.h
> > +++ b/drivers/media/platform/omap3isp/ispcsiphy.h
> > @@ -25,9 +25,10 @@ struct regulator;
> > struct isp_csiphy {
> > struct isp_device *isp;
> > struct mutex mutex; /* serialize csiphy configuration */
> > - u8 phy_in_use;
> > struct isp_csi2_device *csi2;
> > struct regulator *vdd;
> > + /* the entity for which the phy was acquired for */
>
> Either "the entity that the phy was acquired for", or "the entity for which
> the phy was acquired". Or you could rename the field to owner and document it
> as "the entity owning the PHY", I think that would be more explicit in the
> code.
Yes, there's one "for" too much. I'll drop the latter.
>
> > + struct media_entity *entity;
> >
> > /* mem resources - enums as defined in enum isp_mem_resources */
> > unsigned int cfg_regs;
> > @@ -36,7 +37,8 @@ struct isp_csiphy {
> > u8 num_data_lanes; /* number of CSI2 Data Lanes supported */
> > };
> >
> > -int omap3isp_csiphy_acquire(struct isp_csiphy *phy);
> > +int omap3isp_csiphy_acquire(struct isp_csiphy *phy,
> > + struct media_entity *entity);
> > void omap3isp_csiphy_release(struct isp_csiphy *phy);
> > int omap3isp_csiphy_init(struct isp_device *isp);
> > void omap3isp_csiphy_cleanup(struct isp_device *isp);
>
--
Sakari Ailus
e-mail: sakari.ailus@xxxxxx
