All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Niklas Söderlund" <niklas.soderlund@ragnatech.se>
To: Jacopo Mondi <jacopo+renesas@jmondi.org>
Cc: laurent.pinchart@ideasonboard.com,
	kieran.bingham@ideasonboard.com, linux-media@vger.kernel.org,
	linux-renesas-soc@vger.kernel.org,
	Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
Subject: Re: [PATCH v3 3/6] media: adv748x: csi2: Link AFE with TXA and TXB
Date: Wed, 16 Jan 2019 14:44:25 +0100	[thread overview]
Message-ID: <20190116134424.GP7393@bigcity.dyn.berto.se> (raw)
In-Reply-To: <20190114145533.GK30160@bigcity.dyn.berto.se>

Hi (again) Jacopo,

I found something else in this patch unfortunately :-(

On 2019-01-14 15:55:33 +0100, Niklas Söderlund wrote:
> Hi Jacopo,
> 
> Thanks for your patch.
> 
> On 2019-01-10 15:02:10 +0100, Jacopo Mondi wrote:
> > The ADV748x chip supports routing AFE output to either TXA or TXB.
> > In order to support run-time configuration of video stream path, create an
> > additional (not enabled) "AFE:8->TXA:0" link, and remove the IMMUTABLE flag
> > from existing ones.
> > 
> > Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> >  drivers/media/i2c/adv748x/adv748x-csi2.c | 44 +++++++++++++-----------
> >  1 file changed, 23 insertions(+), 21 deletions(-)
> > 
> > diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c b/drivers/media/i2c/adv748x/adv748x-csi2.c
> > index b6b5d8c7ea7c..8c3714495e11 100644
> > --- a/drivers/media/i2c/adv748x/adv748x-csi2.c
> > +++ b/drivers/media/i2c/adv748x/adv748x-csi2.c
> > @@ -27,6 +27,7 @@ static int adv748x_csi2_set_virtual_channel(struct adv748x_csi2 *tx,
> >   * @v4l2_dev: Video registration device
> >   * @src: Source subdevice to establish link
> >   * @src_pad: Pad number of source to link to this @tx
> > + * @enable: Link enabled flag
> >   *
> >   * Ensure that the subdevice is registered against the v4l2_device, and link the
> >   * source pad to the sink pad of the CSI2 bus entity.
> > @@ -34,17 +35,11 @@ static int adv748x_csi2_set_virtual_channel(struct adv748x_csi2 *tx,
> >  static int adv748x_csi2_register_link(struct adv748x_csi2 *tx,
> >  				      struct v4l2_device *v4l2_dev,
> >  				      struct v4l2_subdev *src,
> > -				      unsigned int src_pad)
> > +				      unsigned int src_pad,
> > +				      bool enable)
> >  {
> > -	int enabled = MEDIA_LNK_FL_ENABLED;
> >  	int ret;
> >  
> > -	/*
> > -	 * Dynamic linking of the AFE is not supported.
> > -	 * Register the links as immutable.
> > -	 */
> > -	enabled |= MEDIA_LNK_FL_IMMUTABLE;
> > -
> >  	if (!src->v4l2_dev) {
> >  		ret = v4l2_device_register_subdev(v4l2_dev, src);
> >  		if (ret)
> > @@ -53,7 +48,7 @@ static int adv748x_csi2_register_link(struct adv748x_csi2 *tx,
> >  
> >  	return media_create_pad_link(&src->entity, src_pad,
> >  				     &tx->sd.entity, ADV748X_CSI2_SINK,
> > -				     enabled);
> > +				     enable ? MEDIA_LNK_FL_ENABLED : 0);
> >  }
> >  
> >  /* -----------------------------------------------------------------------------
> > @@ -68,25 +63,32 @@ static int adv748x_csi2_registered(struct v4l2_subdev *sd)
> >  {
> >  	struct adv748x_csi2 *tx = adv748x_sd_to_csi2(sd);
> >  	struct adv748x_state *state = tx->state;
> > +	int ret;
> >  
> >  	adv_dbg(state, "Registered %s (%s)", is_txa(tx) ? "TXA":"TXB",
> >  			sd->name);
> >  
> >  	/*
> > -	 * The adv748x hardware allows the AFE to route through the TXA, however
> > -	 * this is not currently supported in this driver.
> > +	 * Link TXA to AFE and HDMI, and TXB to AFE only as TXB cannot output
> > +	 * HDMI.
> >  	 *
> > -	 * Link HDMI->TXA, and AFE->TXB directly.
> > +	 * The HDMI->TXA link is enabled by default, as is the AFE->TXB one.
> >  	 */
> > -	if (is_txa(tx) && is_hdmi_enabled(state))
> > -		return adv748x_csi2_register_link(tx, sd->v4l2_dev,
> > -						  &state->hdmi.sd,
> > -						  ADV748X_HDMI_SOURCE);
> > -	if (is_txb(tx) && is_afe_enabled(state))
> > -		return adv748x_csi2_register_link(tx, sd->v4l2_dev,
> > -						  &state->afe.sd,
> > -						  ADV748X_AFE_SOURCE);
> > -	return 0;
> > +	if (is_afe_enabled(state)) {
> > +		ret = adv748x_csi2_register_link(tx, sd->v4l2_dev,
> > +						 &state->afe.sd,
> > +						 ADV748X_AFE_SOURCE,
> > +						 is_txb(tx));
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> > +	/* Register link to HDMI for TXA only. */
> > +	if (is_txb(tx) || !is_hdmi_enabled(state))
> 
> Small nit, I would s/is_txb(tx)/!is_txa(tx)/ here as to me it becomes 
> easier to read. With or without this change,
> 
> Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> 
> > +		return 0;
> > +
> > +	return adv748x_csi2_register_link(tx, sd->v4l2_dev, &state->hdmi.sd,
> > +					  ADV748X_HDMI_SOURCE, true);

If the call to adv748x_csi2_register_link() fails should not the 
(possible) link to the AFE be removed?

> >  }
> >  
> >  static const struct v4l2_subdev_internal_ops adv748x_csi2_internal_ops = {
> > -- 
> > 2.20.1
> > 
> 
> -- 
> Regards,
> Niklas Söderlund

-- 
Regards,
Niklas Söderlund

  parent reply	other threads:[~2019-01-16 13:44 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-01-10 14:02 [PATCH v3 0/6] media: adv748x: Implement dynamic routing support Jacopo Mondi
2019-01-10 14:02 ` [PATCH v3 1/6] media: adv748x: Add is_txb() Jacopo Mondi
2019-01-14 14:47   ` Niklas Söderlund
2019-01-10 14:02 ` [PATCH v3 2/6] media: adv748x: Rename reset procedures Jacopo Mondi
2019-01-14 14:49   ` Niklas Söderlund
2019-01-10 14:02 ` [PATCH v3 3/6] media: adv748x: csi2: Link AFE with TXA and TXB Jacopo Mondi
2019-01-14 14:55   ` Niklas Söderlund
2019-01-16  9:10     ` Jacopo Mondi
2019-01-16 12:28       ` Niklas Söderlund
2019-01-16 13:44     ` Niklas Söderlund [this message]
2019-01-28 14:47       ` Jacopo Mondi
2019-02-07 15:57         ` Niklas Söderlund
2019-01-10 14:02 ` [PATCH v3 4/6] media: adv748x: Store the source subdevice in TX Jacopo Mondi
2019-01-16 13:33   ` Niklas Söderlund
2019-01-10 14:02 ` [PATCH v3 5/6] media: adv748x: Store the TX sink in HDMI/AFE Jacopo Mondi
2019-01-16 13:45   ` Niklas Söderlund
2019-01-10 14:02 ` [PATCH v3 6/6] media: adv748x: Implement TX link_setup callback Jacopo Mondi
2019-01-10 14:36   ` Kieran Bingham
2019-01-16 13:46   ` Niklas Söderlund
2019-01-14 14:22 ` [PATCH v3 0/6] media: adv748x: Implement dynamic routing support Kieran Bingham

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190116134424.GP7393@bigcity.dyn.berto.se \
    --to=niklas.soderlund@ragnatech.se \
    --cc=jacopo+renesas@jmondi.org \
    --cc=kieran.bingham+renesas@ideasonboard.com \
    --cc=kieran.bingham@ideasonboard.com \
    --cc=laurent.pinchart@ideasonboard.com \
    --cc=linux-media@vger.kernel.org \
    --cc=linux-renesas-soc@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.