[v2,6/6] media: adv748x: Implement TX link_setup callback
diff mbox series

Message ID 20190106155413.30666-7-jacopo+renesas@jmondi.org
State New
Headers show
Series
  • media: adv748x: Implement dynamic routing support
Related show

Commit Message

Jacopo Mondi Jan. 6, 2019, 3:54 p.m. UTC
When the adv748x driver is informed about a link being created from HDMI or
AFE to a CSI-2 TX output, the 'link_setup()' callback is invoked. Make
sure to implement proper routing management at link setup time, to route
the selected video stream to the desired TX output.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/i2c/adv748x/adv748x-core.c | 57 +++++++++++++++++++++++-
 drivers/media/i2c/adv748x/adv748x.h      |  2 +
 2 files changed, 58 insertions(+), 1 deletion(-)

Comments

Kieran Bingham Jan. 7, 2019, 12:36 p.m. UTC | #1
Hi Jacopo,

On 06/01/2019 15:54, Jacopo Mondi wrote:
> When the adv748x driver is informed about a link being created from HDMI or
> AFE to a CSI-2 TX output, the 'link_setup()' callback is invoked. Make
> sure to implement proper routing management at link setup time, to route
> the selected video stream to the desired TX output.

Overall this looks like the right approach - but I feel like the
handling of the io10 register might need some consideration, because
it's value depends on the condition of both CSI2 transmitters, not just
the currently parsed link.

I had a go at some pseudo - uncompiled/untested code inline as a suggestion.

If you think it's better - feel free to rework it in ... or not as you
see fit.

Regards

Kieran




> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/media/i2c/adv748x/adv748x-core.c | 57 +++++++++++++++++++++++-
>  drivers/media/i2c/adv748x/adv748x.h      |  2 +
>  2 files changed, 58 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c
> index 200e00f93546..a586bf393558 100644
> --- a/drivers/media/i2c/adv748x/adv748x-core.c
> +++ b/drivers/media/i2c/adv748x/adv748x-core.c
> @@ -335,6 +335,60 @@ int adv748x_tx_power(struct adv748x_csi2 *tx, bool on)
>  /* -----------------------------------------------------------------------------
>   * Media Operations
>   */
> +static int adv748x_link_setup(struct media_entity *entity,
> +			      const struct media_pad *local,
> +			      const struct media_pad *remote, u32 flags)
> +{
> +	struct v4l2_subdev *rsd = media_entity_to_v4l2_subdev(remote->entity);
> +	struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(entity);
> +	struct adv748x_state *state = v4l2_get_subdevdata(sd);
> +	struct adv748x_csi2 *tx = adv748x_sd_to_csi2(sd);
> +	bool enable = flags & MEDIA_LNK_FL_ENABLED;
> +	u8 io10;
> +
> +	/* Refuse to enable multiple links to the same TX at the same time. */
> +	if (enable && tx->src)
> +		return -EINVAL;
> +
> +	/* Set or clear the source (HDMI or AFE) and the current TX. */
> +	if (rsd == &state->afe.sd)
> +		state->afe.tx = enable ? tx : NULL;
> +	else
> +		state->hdmi.tx = enable ? tx : NULL;
> +
> +	tx->src = enable ? rsd : NULL;
> +
> +	if (!enable)
> +		return 0;

Don't we potentially want to take any action on disable to power down
links below ?

> +
> +	/* Change video stream routing, according to the newly enabled link. */
> +	io10 = io_read(state, ADV748X_IO_10);
> +	if (rsd == &state->afe.sd) {
> +		/*
> +		 * Set AFE->TXA routing and power off TXB if AFE goes to TXA.
> +		 * if AFE goes to TXB, we need both TXA and TXB powered on.
> +		 */
> +		io10 &= ~ADV748X_IO_10_CSI1_EN;
> +		io10 &= ~ADV748X_IO_10_CSI4_IN_SEL_AFE;
> +		if (is_txa(tx))
> +			io10 |= ADV748X_IO_10_CSI4_IN_SEL_AFE;

Shouldn't the CSI4 be enabled here too? or are we assuming it's already
(/always) enabled?
		io10 |= ADV748X_IO_10_CSI4_EN;

> +		else
> +			io10 |= ADV748X_IO_10_CSI4_EN |
> +				ADV748X_IO_10_CSI1_EN;
> +	} else {
> +		/* Clear AFE->TXA routing and power up TXA. */
> +		io10 &= ~ADV748X_IO_10_CSI4_IN_SEL_AFE;
> +		io10 |= ADV748X_IO_10_CSI4_EN;

But if we assume it's already enabled ... do we need this?
Perhaps it might be better to be explicit on this?

> +	}
> +	io_write(state, ADV748X_IO_10, io10);


Would it be any cleaner to use io_clrset() here?

Hrm ... also it feels like this register really should be set depending
upon the complete state of ... &state->...

So perhaps it deserves it's own function which should be called after
csi_registered() callback and any link change.

/me has a quick go at some psuedo codeishness...:

int adv74x_io_10(struct adv748x_state *state);
	u8 bits = 0;
	u8 mask = ADV748X_IO_10_CSI1_EN
		| ADV748X_IO_10_CSI4_EN
		| ADV748X_IO_10_CSI4_IN_SEL_AFE;

	if (state->afe.tx) {
		/* AFE Requires TXA enabled, even when output to TXB */
		bits |= ADV748X_IO_10_CSI4_EN;

		if (is_txa(state->afe.tx))
			bits |= ADV748X_IO_10_CSI4_IN_SEL_AFE
		else
			bits |= ADV748X_IO_10_CSI1_EN;
	}

	if (state->hdmi.tx) {
		bits |= ADV748X_IO_10_CSI4_EN;
	}

	return io_clrset(state, ADV748X_IO_10, mask, bits);
}

How does that look ? (is it even correct first?)

> +
> +	return 0;
> +}
> +
> +static const struct media_entity_operations adv748x_tx_media_ops = {
> +	.link_setup	= adv748x_link_setup,
> +	.link_validate	= v4l2_subdev_link_validate,
> +};
>  
>  static const struct media_entity_operations adv748x_media_ops = {
>  	.link_validate = v4l2_subdev_link_validate,
> @@ -516,7 +570,8 @@ void adv748x_subdev_init(struct v4l2_subdev *sd, struct adv748x_state *state,
>  		state->client->addr, ident);
>  
>  	sd->entity.function = function;
> -	sd->entity.ops = &adv748x_media_ops;
> +	sd->entity.ops = is_tx(adv748x_sd_to_csi2(sd)) ?
> +			 &adv748x_tx_media_ops : &adv748x_media_ops;

Aha - yes that's a neat solution to ensure that only the TX links
generate link_setup calls :)



>  }
>  
>  static int adv748x_parse_csi2_lanes(struct adv748x_state *state,
> diff --git a/drivers/media/i2c/adv748x/adv748x.h b/drivers/media/i2c/adv748x/adv748x.h
> index 6eb2e4a95eed..eb19c6cbbb4e 100644
> --- a/drivers/media/i2c/adv748x/adv748x.h
> +++ b/drivers/media/i2c/adv748x/adv748x.h
> @@ -93,6 +93,7 @@ struct adv748x_csi2 {
>  
>  #define is_tx_enabled(_tx) ((_tx)->state->endpoints[(_tx)->port] != NULL)
>  #define __is_tx(_tx, _ab) ((_tx) == &(_tx)->state->tx##_ab)
> +#define is_tx(_tx) (is_txa(_tx) || is_txb(_tx))
>  #define is_txa(_tx) __is_tx(_tx, a)
>  #define is_txb(_tx) __is_tx(_tx, b)
>  
> @@ -224,6 +225,7 @@ struct adv748x_state {
>  #define ADV748X_IO_10_CSI4_EN		BIT(7)
>  #define ADV748X_IO_10_CSI1_EN		BIT(6)
>  #define ADV748X_IO_10_PIX_OUT_EN	BIT(5)
> +#define ADV748X_IO_10_CSI4_IN_SEL_AFE	BIT(3)



>  
>  #define ADV748X_IO_CHIP_REV_ID_1	0xdf
>  #define ADV748X_IO_CHIP_REV_ID_2	0xe0
>
Laurent Pinchart Jan. 9, 2019, 12:15 a.m. UTC | #2
Hello,

On Monday, 7 January 2019 14:36:28 EET Kieran Bingham wrote:
> On 06/01/2019 15:54, Jacopo Mondi wrote:
> > When the adv748x driver is informed about a link being created from HDMI
> > or AFE to a CSI-2 TX output, the 'link_setup()' callback is invoked. Make
> > sure to implement proper routing management at link setup time, to route
> > the selected video stream to the desired TX output.
> 
> Overall this looks like the right approach - but I feel like the
> handling of the io10 register might need some consideration, because
> it's value depends on the condition of both CSI2 transmitters, not just
> the currently parsed link.
> 
> I had a go at some pseudo - uncompiled/untested code inline as a suggestion.
> 
> If you think it's better - feel free to rework it in ... or not as you
> see fit.
> 
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> > 
> >  drivers/media/i2c/adv748x/adv748x-core.c | 57 +++++++++++++++++++++++-
> >  drivers/media/i2c/adv748x/adv748x.h      |  2 +
> >  2 files changed, 58 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/media/i2c/adv748x/adv748x-core.c
> > b/drivers/media/i2c/adv748x/adv748x-core.c index
> > 200e00f93546..a586bf393558 100644
> > --- a/drivers/media/i2c/adv748x/adv748x-core.c
> > +++ b/drivers/media/i2c/adv748x/adv748x-core.c
> > @@ -335,6 +335,60 @@ int adv748x_tx_power(struct adv748x_csi2 *tx, bool
> > on)
> >  /* ----------------------------------------------------------------------
> >   * Media Operations
> >   */
> > +static int adv748x_link_setup(struct media_entity *entity,
> > +			      const struct media_pad *local,
> > +			      const struct media_pad *remote, u32 flags)
> > +{
> > +	struct v4l2_subdev *rsd = media_entity_to_v4l2_subdev(remote->entity);
> > +	struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(entity);
> > +	struct adv748x_state *state = v4l2_get_subdevdata(sd);
> > +	struct adv748x_csi2 *tx = adv748x_sd_to_csi2(sd);
> > +	bool enable = flags & MEDIA_LNK_FL_ENABLED;
> > +	u8 io10;
> > +
> > +	/* Refuse to enable multiple links to the same TX at the same time. */
> > +	if (enable && tx->src)
> > +		return -EINVAL;
> > +
> > +	/* Set or clear the source (HDMI or AFE) and the current TX. */
> > +	if (rsd == &state->afe.sd)
> > +		state->afe.tx = enable ? tx : NULL;
> > +	else
> > +		state->hdmi.tx = enable ? tx : NULL;
> > +
> > +	tx->src = enable ? rsd : NULL;
> > +
> > +	if (!enable)
> > +		return 0;
> 
> Don't we potentially want to take any action on disable to power down
> links below ?
> 
> > +
> > +	/* Change video stream routing, according to the newly enabled link. */
> > +	io10 = io_read(state, ADV748X_IO_10);
> > +	if (rsd == &state->afe.sd) {
> > +		/*
> > +		 * Set AFE->TXA routing and power off TXB if AFE goes to TXA.
> > +		 * if AFE goes to TXB, we need both TXA and TXB powered on.
> > +		 */
> > +		io10 &= ~ADV748X_IO_10_CSI1_EN;
> > +		io10 &= ~ADV748X_IO_10_CSI4_IN_SEL_AFE;
> > +		if (is_txa(tx))
> > +			io10 |= ADV748X_IO_10_CSI4_IN_SEL_AFE;
> 
> Shouldn't the CSI4 be enabled here too? or are we assuming it's already
> (/always) enabled?
> 		io10 |= ADV748X_IO_10_CSI4_EN;
> 
> > +		else
> > +			io10 |= ADV748X_IO_10_CSI4_EN |
> > +				ADV748X_IO_10_CSI1_EN;
> > +	} else {
> > +		/* Clear AFE->TXA routing and power up TXA. */
> > +		io10 &= ~ADV748X_IO_10_CSI4_IN_SEL_AFE;
> > +		io10 |= ADV748X_IO_10_CSI4_EN;
> 
> But if we assume it's already enabled ... do we need this?
> Perhaps it might be better to be explicit on this?
> 
> > +	}
> > +	io_write(state, ADV748X_IO_10, io10);
> 
> Would it be any cleaner to use io_clrset() here?
> 
> Hrm ... also it feels like this register really should be set depending
> upon the complete state of ... &state->...
> 
> So perhaps it deserves it's own function which should be called after
> csi_registered() callback and any link change.
> 
> /me has a quick go at some psuedo codeishness...:
> 
> int adv74x_io_10(struct adv748x_state *state);
> 	u8 bits = 0;
> 	u8 mask = ADV748X_IO_10_CSI1_EN
> 
> 		| ADV748X_IO_10_CSI4_EN
> 		| ADV748X_IO_10_CSI4_IN_SEL_AFE;
> 
> 	if (state->afe.tx) {
> 		/* AFE Requires TXA enabled, even when output to TXB */
> 		bits |= ADV748X_IO_10_CSI4_EN;
> 
> 		if (is_txa(state->afe.tx))
> 			bits |= ADV748X_IO_10_CSI4_IN_SEL_AFE
> 		else
> 			bits |= ADV748X_IO_10_CSI1_EN;
> 	}
> 
> 	if (state->hdmi.tx) {
> 		bits |= ADV748X_IO_10_CSI4_EN;
> 	}
> 
> 	return io_clrset(state, ADV748X_IO_10, mask, bits);
> }
> 
> How does that look ? (is it even correct first?)
> 
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct media_entity_operations adv748x_tx_media_ops = {
> > +	.link_setup	= adv748x_link_setup,
> > +	.link_validate	= v4l2_subdev_link_validate,
> > +};
> > 
> >  static const struct media_entity_operations adv748x_media_ops = {
> >  	.link_validate = v4l2_subdev_link_validate,
> > @@ -516,7 +570,8 @@ void adv748x_subdev_init(struct v4l2_subdev *sd,
> > struct adv748x_state *state,
> >  		state->client->addr, ident);
> >  	
> >  	sd->entity.function = function;
> > -	sd->entity.ops = &adv748x_media_ops;
> > +	sd->entity.ops = is_tx(adv748x_sd_to_csi2(sd)) ?
> > +			 &adv748x_tx_media_ops : &adv748x_media_ops;
> 
> Aha - yes that's a neat solution to ensure that only the TX links
> generate link_setup calls :)

Another option would be to bail out from adv748x_link_setup() if the entity is 
not a TX*.

> >  }

[snip]
Kieran Bingham Jan. 9, 2019, 2:15 p.m. UTC | #3
On 09/01/2019 00:15, Laurent Pinchart wrote:
> Hello,
> 
> On Monday, 7 January 2019 14:36:28 EET Kieran Bingham wrote:
>> On 06/01/2019 15:54, Jacopo Mondi wrote:
>>> When the adv748x driver is informed about a link being created from HDMI
>>> or AFE to a CSI-2 TX output, the 'link_setup()' callback is invoked. Make
>>> sure to implement proper routing management at link setup time, to route
>>> the selected video stream to the desired TX output.
>>
>> Overall this looks like the right approach - but I feel like the
>> handling of the io10 register might need some consideration, because
>> it's value depends on the condition of both CSI2 transmitters, not just
>> the currently parsed link.
>>
>> I had a go at some pseudo - uncompiled/untested code inline as a suggestion.
>>
>> If you think it's better - feel free to rework it in ... or not as you
>> see fit.
>>
>>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
>>> ---
>>>
>>>  drivers/media/i2c/adv748x/adv748x-core.c | 57 +++++++++++++++++++++++-
>>>  drivers/media/i2c/adv748x/adv748x.h      |  2 +
>>>  2 files changed, 58 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/media/i2c/adv748x/adv748x-core.c
>>> b/drivers/media/i2c/adv748x/adv748x-core.c index
>>> 200e00f93546..a586bf393558 100644
>>> --- a/drivers/media/i2c/adv748x/adv748x-core.c
>>> +++ b/drivers/media/i2c/adv748x/adv748x-core.c
>>> @@ -335,6 +335,60 @@ int adv748x_tx_power(struct adv748x_csi2 *tx, bool
>>> on)
>>>  /* ----------------------------------------------------------------------
>>>   * Media Operations
>>>   */
>>> +static int adv748x_link_setup(struct media_entity *entity,
>>> +			      const struct media_pad *local,
>>> +			      const struct media_pad *remote, u32 flags)
>>> +{
>>> +	struct v4l2_subdev *rsd = media_entity_to_v4l2_subdev(remote->entity);
>>> +	struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(entity);
>>> +	struct adv748x_state *state = v4l2_get_subdevdata(sd);
>>> +	struct adv748x_csi2 *tx = adv748x_sd_to_csi2(sd);
>>> +	bool enable = flags & MEDIA_LNK_FL_ENABLED;
>>> +	u8 io10;
>>> +
>>> +	/* Refuse to enable multiple links to the same TX at the same time. */
>>> +	if (enable && tx->src)
>>> +		return -EINVAL;
>>> +
>>> +	/* Set or clear the source (HDMI or AFE) and the current TX. */
>>> +	if (rsd == &state->afe.sd)
>>> +		state->afe.tx = enable ? tx : NULL;
>>> +	else
>>> +		state->hdmi.tx = enable ? tx : NULL;
>>> +
>>> +	tx->src = enable ? rsd : NULL;
>>> +
>>> +	if (!enable)
>>> +		return 0;
>>
>> Don't we potentially want to take any action on disable to power down
>> links below ?
>>
>>> +
>>> +	/* Change video stream routing, according to the newly enabled link. */
>>> +	io10 = io_read(state, ADV748X_IO_10);
>>> +	if (rsd == &state->afe.sd) {
>>> +		/*
>>> +		 * Set AFE->TXA routing and power off TXB if AFE goes to TXA.
>>> +		 * if AFE goes to TXB, we need both TXA and TXB powered on.
>>> +		 */
>>> +		io10 &= ~ADV748X_IO_10_CSI1_EN;
>>> +		io10 &= ~ADV748X_IO_10_CSI4_IN_SEL_AFE;
>>> +		if (is_txa(tx))
>>> +			io10 |= ADV748X_IO_10_CSI4_IN_SEL_AFE;
>>
>> Shouldn't the CSI4 be enabled here too? or are we assuming it's already
>> (/always) enabled?
>> 		io10 |= ADV748X_IO_10_CSI4_EN;
>>
>>> +		else
>>> +			io10 |= ADV748X_IO_10_CSI4_EN |
>>> +				ADV748X_IO_10_CSI1_EN;
>>> +	} else {
>>> +		/* Clear AFE->TXA routing and power up TXA. */
>>> +		io10 &= ~ADV748X_IO_10_CSI4_IN_SEL_AFE;
>>> +		io10 |= ADV748X_IO_10_CSI4_EN;
>>
>> But if we assume it's already enabled ... do we need this?
>> Perhaps it might be better to be explicit on this?
>>
>>> +	}
>>> +	io_write(state, ADV748X_IO_10, io10);
>>
>> Would it be any cleaner to use io_clrset() here?
>>
>> Hrm ... also it feels like this register really should be set depending
>> upon the complete state of ... &state->...
>>
>> So perhaps it deserves it's own function which should be called after
>> csi_registered() callback and any link change.
>>
>> /me has a quick go at some psuedo codeishness...:
>>
>> int adv74x_io_10(struct adv748x_state *state);
>> 	u8 bits = 0;
>> 	u8 mask = ADV748X_IO_10_CSI1_EN
>>
>> 		| ADV748X_IO_10_CSI4_EN
>> 		| ADV748X_IO_10_CSI4_IN_SEL_AFE;
>>
>> 	if (state->afe.tx) {
>> 		/* AFE Requires TXA enabled, even when output to TXB */
>> 		bits |= ADV748X_IO_10_CSI4_EN;
>>
>> 		if (is_txa(state->afe.tx))
>> 			bits |= ADV748X_IO_10_CSI4_IN_SEL_AFE
>> 		else
>> 			bits |= ADV748X_IO_10_CSI1_EN;
>> 	}
>>
>> 	if (state->hdmi.tx) {
>> 		bits |= ADV748X_IO_10_CSI4_EN;
>> 	}
>>
>> 	return io_clrset(state, ADV748X_IO_10, mask, bits);
>> }
>>
>> How does that look ? (is it even correct first?)
>>
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static const struct media_entity_operations adv748x_tx_media_ops = {
>>> +	.link_setup	= adv748x_link_setup,
>>> +	.link_validate	= v4l2_subdev_link_validate,
>>> +};
>>>
>>>  static const struct media_entity_operations adv748x_media_ops = {
>>>  	.link_validate = v4l2_subdev_link_validate,
>>> @@ -516,7 +570,8 @@ void adv748x_subdev_init(struct v4l2_subdev *sd,
>>> struct adv748x_state *state,
>>>  		state->client->addr, ident);
>>>  	
>>>  	sd->entity.function = function;
>>> -	sd->entity.ops = &adv748x_media_ops;
>>> +	sd->entity.ops = is_tx(adv748x_sd_to_csi2(sd)) ?
>>> +			 &adv748x_tx_media_ops : &adv748x_media_ops;
>>
>> Aha - yes that's a neat solution to ensure that only the TX links
>> generate link_setup calls :)
> 
> Another option would be to bail out from adv748x_link_setup() if the entity is 
> not a TX*.
> 

I suggested this in v1 - but Jacopo objected with the following:

> Checking for is_txa() and is_txb() would require to call
> 'adv_sd_to_csi2(sd)' before having made sure the 'sd' actually
> represent a csi2_tx. I would keep it as it is.

Now I look at the implementation here, I see this is precisely what it
is doing anyway .... still converting through adv748x_sd_to_csi2(sd) on
an unknown pointer type
 (which I still believe is a valid thing to do in this instance)

So yes, I think this would be simpler having the check at the top of the
adv748x_link_setup() call, and thus then there is no need to add a
second adv_media_ops structure.


>>>  }
> 
> [snip]
>
Kieran Bingham Jan. 9, 2019, 2:17 p.m. UTC | #4
Hi Jacopo,

One more comment below:

On 07/01/2019 12:36, Kieran Bingham wrote:
> Hi Jacopo,
> 
> On 06/01/2019 15:54, Jacopo Mondi wrote:
>> When the adv748x driver is informed about a link being created from HDMI or
>> AFE to a CSI-2 TX output, the 'link_setup()' callback is invoked. Make
>> sure to implement proper routing management at link setup time, to route
>> the selected video stream to the desired TX output.>

<snip>

>>  static int adv748x_parse_csi2_lanes(struct adv748x_state *state,
>> diff --git a/drivers/media/i2c/adv748x/adv748x.h b/drivers/media/i2c/adv748x/adv748x.h
>> index 6eb2e4a95eed..eb19c6cbbb4e 100644
>> --- a/drivers/media/i2c/adv748x/adv748x.h
>> +++ b/drivers/media/i2c/adv748x/adv748x.h
>> @@ -93,6 +93,7 @@ struct adv748x_csi2 {
>>  
>>  #define is_tx_enabled(_tx) ((_tx)->state->endpoints[(_tx)->port] != NULL)
>>  #define __is_tx(_tx, _ab) ((_tx) == &(_tx)->state->tx##_ab)
>> +#define is_tx(_tx) (is_txa(_tx) || is_txb(_tx))

I'd put this /after/ is_txa/is_txb so that the use is after the
declarations.
--
KB


>>  #define is_txa(_tx) __is_tx(_tx, a)
>>  #define is_txb(_tx) __is_tx(_tx, b)
>>  
>> @@ -224,6 +225,7 @@ struct adv748x_state {
>>  #define ADV748X_IO_10_CSI4_EN		BIT(7)
>>  #define ADV748X_IO_10_CSI1_EN		BIT(6)
>>  #define ADV748X_IO_10_PIX_OUT_EN	BIT(5)
>> +#define ADV748X_IO_10_CSI4_IN_SEL_AFE	BIT(3)
>>  
>>  #define ADV748X_IO_CHIP_REV_ID_1	0xdf
>>  #define ADV748X_IO_CHIP_REV_ID_2	0xe0
>>
>
Jacopo Mondi Jan. 10, 2019, 8:51 a.m. UTC | #5
Hi Laurent,

On Wed, Jan 09, 2019 at 02:15:04AM +0200, Laurent Pinchart wrote:
> Hello,
>
> On Monday, 7 January 2019 14:36:28 EET Kieran Bingham wrote:
> > On 06/01/2019 15:54, Jacopo Mondi wrote:
> > > When the adv748x driver is informed about a link being created from HDMI
> > > or AFE to a CSI-2 TX output, the 'link_setup()' callback is invoked. Make
> > > sure to implement proper routing management at link setup time, to route
> > > the selected video stream to the desired TX output.
> >
> > Overall this looks like the right approach - but I feel like the
> > handling of the io10 register might need some consideration, because
> > it's value depends on the condition of both CSI2 transmitters, not just
> > the currently parsed link.
> >
> > I had a go at some pseudo - uncompiled/untested code inline as a suggestion.
> >
> > If you think it's better - feel free to rework it in ... or not as you
> > see fit.
> >
> > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > > ---
> > >
> > >  drivers/media/i2c/adv748x/adv748x-core.c | 57 +++++++++++++++++++++++-
> > >  drivers/media/i2c/adv748x/adv748x.h      |  2 +
> > >  2 files changed, 58 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/media/i2c/adv748x/adv748x-core.c
> > > b/drivers/media/i2c/adv748x/adv748x-core.c index
> > > 200e00f93546..a586bf393558 100644
> > > --- a/drivers/media/i2c/adv748x/adv748x-core.c
> > > +++ b/drivers/media/i2c/adv748x/adv748x-core.c
> > > @@ -335,6 +335,60 @@ int adv748x_tx_power(struct adv748x_csi2 *tx, bool
> > > on)
> > >  /* ----------------------------------------------------------------------
> > >   * Media Operations
> > >   */
> > > +static int adv748x_link_setup(struct media_entity *entity,
> > > +			      const struct media_pad *local,
> > > +			      const struct media_pad *remote, u32 flags)
> > > +{
> > > +	struct v4l2_subdev *rsd = media_entity_to_v4l2_subdev(remote->entity);
> > > +	struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(entity);
> > > +	struct adv748x_state *state = v4l2_get_subdevdata(sd);
> > > +	struct adv748x_csi2 *tx = adv748x_sd_to_csi2(sd);
> > > +	bool enable = flags & MEDIA_LNK_FL_ENABLED;
> > > +	u8 io10;
> > > +
> > > +	/* Refuse to enable multiple links to the same TX at the same time. */
> > > +	if (enable && tx->src)
> > > +		return -EINVAL;
> > > +
> > > +	/* Set or clear the source (HDMI or AFE) and the current TX. */
> > > +	if (rsd == &state->afe.sd)
> > > +		state->afe.tx = enable ? tx : NULL;
> > > +	else
> > > +		state->hdmi.tx = enable ? tx : NULL;
> > > +
> > > +	tx->src = enable ? rsd : NULL;
> > > +
> > > +	if (!enable)
> > > +		return 0;
> >
> > Don't we potentially want to take any action on disable to power down
> > links below ?
> >
> > > +
> > > +	/* Change video stream routing, according to the newly enabled link. */
> > > +	io10 = io_read(state, ADV748X_IO_10);
> > > +	if (rsd == &state->afe.sd) {
> > > +		/*
> > > +		 * Set AFE->TXA routing and power off TXB if AFE goes to TXA.
> > > +		 * if AFE goes to TXB, we need both TXA and TXB powered on.
> > > +		 */
> > > +		io10 &= ~ADV748X_IO_10_CSI1_EN;
> > > +		io10 &= ~ADV748X_IO_10_CSI4_IN_SEL_AFE;
> > > +		if (is_txa(tx))
> > > +			io10 |= ADV748X_IO_10_CSI4_IN_SEL_AFE;
> >
> > Shouldn't the CSI4 be enabled here too? or are we assuming it's already
> > (/always) enabled?
> > 		io10 |= ADV748X_IO_10_CSI4_EN;
> >
> > > +		else
> > > +			io10 |= ADV748X_IO_10_CSI4_EN |
> > > +				ADV748X_IO_10_CSI1_EN;
> > > +	} else {
> > > +		/* Clear AFE->TXA routing and power up TXA. */
> > > +		io10 &= ~ADV748X_IO_10_CSI4_IN_SEL_AFE;
> > > +		io10 |= ADV748X_IO_10_CSI4_EN;
> >
> > But if we assume it's already enabled ... do we need this?
> > Perhaps it might be better to be explicit on this?
> >
> > > +	}
> > > +	io_write(state, ADV748X_IO_10, io10);
> >
> > Would it be any cleaner to use io_clrset() here?
> >
> > Hrm ... also it feels like this register really should be set depending
> > upon the complete state of ... &state->...
> >
> > So perhaps it deserves it's own function which should be called after
> > csi_registered() callback and any link change.
> >
> > /me has a quick go at some psuedo codeishness...:
> >
> > int adv74x_io_10(struct adv748x_state *state);
> > 	u8 bits = 0;
> > 	u8 mask = ADV748X_IO_10_CSI1_EN
> >
> > 		| ADV748X_IO_10_CSI4_EN
> > 		| ADV748X_IO_10_CSI4_IN_SEL_AFE;
> >
> > 	if (state->afe.tx) {
> > 		/* AFE Requires TXA enabled, even when output to TXB */
> > 		bits |= ADV748X_IO_10_CSI4_EN;
> >
> > 		if (is_txa(state->afe.tx))
> > 			bits |= ADV748X_IO_10_CSI4_IN_SEL_AFE
> > 		else
> > 			bits |= ADV748X_IO_10_CSI1_EN;
> > 	}
> >
> > 	if (state->hdmi.tx) {
> > 		bits |= ADV748X_IO_10_CSI4_EN;
> > 	}
> >
> > 	return io_clrset(state, ADV748X_IO_10, mask, bits);
> > }
> >
> > How does that look ? (is it even correct first?)
> >
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static const struct media_entity_operations adv748x_tx_media_ops = {
> > > +	.link_setup	= adv748x_link_setup,
> > > +	.link_validate	= v4l2_subdev_link_validate,
> > > +};
> > >
> > >  static const struct media_entity_operations adv748x_media_ops = {
> > >  	.link_validate = v4l2_subdev_link_validate,
> > > @@ -516,7 +570,8 @@ void adv748x_subdev_init(struct v4l2_subdev *sd,
> > > struct adv748x_state *state,
> > >  		state->client->addr, ident);
> > >
> > >  	sd->entity.function = function;
> > > -	sd->entity.ops = &adv748x_media_ops;
> > > +	sd->entity.ops = is_tx(adv748x_sd_to_csi2(sd)) ?
> > > +			 &adv748x_tx_media_ops : &adv748x_media_ops;
> >
> > Aha - yes that's a neat solution to ensure that only the TX links
> > generate link_setup calls :)
>
> Another option would be to bail out from adv748x_link_setup() if the entity is
> not a TX*.
>

If I'm not wrong you suggested me to register a set of operations with
the .link_setup callback only for TX entities, and I agree it is much
better, so I'm leaning to leave it as it is in this series.

Thanks
  j

> > >  }
>
> [snip]
>
> --
> Regards,
>
> Laurent Pinchart
>
>
>
Jacopo Mondi Jan. 10, 2019, 8:58 a.m. UTC | #6
On Wed, Jan 09, 2019 at 02:15:33PM +0000, Kieran Bingham wrote:
> On 09/01/2019 00:15, Laurent Pinchart wrote:
> > Hello,
> >
> > On Monday, 7 January 2019 14:36:28 EET Kieran Bingham wrote:
> >> On 06/01/2019 15:54, Jacopo Mondi wrote:
> >>> When the adv748x driver is informed about a link being created from HDMI
> >>> or AFE to a CSI-2 TX output, the 'link_setup()' callback is invoked. Make
> >>> sure to implement proper routing management at link setup time, to route
> >>> the selected video stream to the desired TX output.
> >>
> >> Overall this looks like the right approach - but I feel like the
> >> handling of the io10 register might need some consideration, because
> >> it's value depends on the condition of both CSI2 transmitters, not just
> >> the currently parsed link.
> >>
> >> I had a go at some pseudo - uncompiled/untested code inline as a suggestion.
> >>
> >> If you think it's better - feel free to rework it in ... or not as you
> >> see fit.
> >>
> >>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> >>> ---
> >>>
> >>>  drivers/media/i2c/adv748x/adv748x-core.c | 57 +++++++++++++++++++++++-
> >>>  drivers/media/i2c/adv748x/adv748x.h      |  2 +
> >>>  2 files changed, 58 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/media/i2c/adv748x/adv748x-core.c
> >>> b/drivers/media/i2c/adv748x/adv748x-core.c index
> >>> 200e00f93546..a586bf393558 100644
> >>> --- a/drivers/media/i2c/adv748x/adv748x-core.c
> >>> +++ b/drivers/media/i2c/adv748x/adv748x-core.c
> >>> @@ -335,6 +335,60 @@ int adv748x_tx_power(struct adv748x_csi2 *tx, bool
> >>> on)
> >>>  /* ----------------------------------------------------------------------
> >>>   * Media Operations
> >>>   */
> >>> +static int adv748x_link_setup(struct media_entity *entity,
> >>> +			      const struct media_pad *local,
> >>> +			      const struct media_pad *remote, u32 flags)
> >>> +{
> >>> +	struct v4l2_subdev *rsd = media_entity_to_v4l2_subdev(remote->entity);
> >>> +	struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(entity);
> >>> +	struct adv748x_state *state = v4l2_get_subdevdata(sd);
> >>> +	struct adv748x_csi2 *tx = adv748x_sd_to_csi2(sd);
> >>> +	bool enable = flags & MEDIA_LNK_FL_ENABLED;
> >>> +	u8 io10;
> >>> +
> >>> +	/* Refuse to enable multiple links to the same TX at the same time. */
> >>> +	if (enable && tx->src)
> >>> +		return -EINVAL;
> >>> +
> >>> +	/* Set or clear the source (HDMI or AFE) and the current TX. */
> >>> +	if (rsd == &state->afe.sd)
> >>> +		state->afe.tx = enable ? tx : NULL;
> >>> +	else
> >>> +		state->hdmi.tx = enable ? tx : NULL;
> >>> +
> >>> +	tx->src = enable ? rsd : NULL;
> >>> +
> >>> +	if (!enable)
> >>> +		return 0;
> >>
> >> Don't we potentially want to take any action on disable to power down
> >> links below ?
> >>
> >>> +
> >>> +	/* Change video stream routing, according to the newly enabled link. */
> >>> +	io10 = io_read(state, ADV748X_IO_10);
> >>> +	if (rsd == &state->afe.sd) {
> >>> +		/*
> >>> +		 * Set AFE->TXA routing and power off TXB if AFE goes to TXA.
> >>> +		 * if AFE goes to TXB, we need both TXA and TXB powered on.
> >>> +		 */
> >>> +		io10 &= ~ADV748X_IO_10_CSI1_EN;
> >>> +		io10 &= ~ADV748X_IO_10_CSI4_IN_SEL_AFE;
> >>> +		if (is_txa(tx))
> >>> +			io10 |= ADV748X_IO_10_CSI4_IN_SEL_AFE;
> >>
> >> Shouldn't the CSI4 be enabled here too? or are we assuming it's already
> >> (/always) enabled?
> >> 		io10 |= ADV748X_IO_10_CSI4_EN;
> >>
> >>> +		else
> >>> +			io10 |= ADV748X_IO_10_CSI4_EN |
> >>> +				ADV748X_IO_10_CSI1_EN;
> >>> +	} else {
> >>> +		/* Clear AFE->TXA routing and power up TXA. */
> >>> +		io10 &= ~ADV748X_IO_10_CSI4_IN_SEL_AFE;
> >>> +		io10 |= ADV748X_IO_10_CSI4_EN;
> >>
> >> But if we assume it's already enabled ... do we need this?
> >> Perhaps it might be better to be explicit on this?
> >>
> >>> +	}
> >>> +	io_write(state, ADV748X_IO_10, io10);
> >>
> >> Would it be any cleaner to use io_clrset() here?
> >>
> >> Hrm ... also it feels like this register really should be set depending
> >> upon the complete state of ... &state->...
> >>
> >> So perhaps it deserves it's own function which should be called after
> >> csi_registered() callback and any link change.
> >>
> >> /me has a quick go at some psuedo codeishness...:
> >>
> >> int adv74x_io_10(struct adv748x_state *state);
> >> 	u8 bits = 0;
> >> 	u8 mask = ADV748X_IO_10_CSI1_EN
> >>
> >> 		| ADV748X_IO_10_CSI4_EN
> >> 		| ADV748X_IO_10_CSI4_IN_SEL_AFE;
> >>
> >> 	if (state->afe.tx) {
> >> 		/* AFE Requires TXA enabled, even when output to TXB */
> >> 		bits |= ADV748X_IO_10_CSI4_EN;
> >>
> >> 		if (is_txa(state->afe.tx))
> >> 			bits |= ADV748X_IO_10_CSI4_IN_SEL_AFE
> >> 		else
> >> 			bits |= ADV748X_IO_10_CSI1_EN;
> >> 	}
> >>
> >> 	if (state->hdmi.tx) {
> >> 		bits |= ADV748X_IO_10_CSI4_EN;
> >> 	}
> >>
> >> 	return io_clrset(state, ADV748X_IO_10, mask, bits);
> >> }
> >>
> >> How does that look ? (is it even correct first?)
> >>
> >>> +
> >>> +	return 0;
> >>> +}
> >>> +
> >>> +static const struct media_entity_operations adv748x_tx_media_ops = {
> >>> +	.link_setup	= adv748x_link_setup,
> >>> +	.link_validate	= v4l2_subdev_link_validate,
> >>> +};
> >>>
> >>>  static const struct media_entity_operations adv748x_media_ops = {
> >>>  	.link_validate = v4l2_subdev_link_validate,
> >>> @@ -516,7 +570,8 @@ void adv748x_subdev_init(struct v4l2_subdev *sd,
> >>> struct adv748x_state *state,
> >>>  		state->client->addr, ident);
> >>>
> >>>  	sd->entity.function = function;
> >>> -	sd->entity.ops = &adv748x_media_ops;
> >>> +	sd->entity.ops = is_tx(adv748x_sd_to_csi2(sd)) ?
> >>> +			 &adv748x_tx_media_ops : &adv748x_media_ops;
> >>
> >> Aha - yes that's a neat solution to ensure that only the TX links
> >> generate link_setup calls :)
> >
> > Another option would be to bail out from adv748x_link_setup() if the entity is
> > not a TX*.
> >
>
> I suggested this in v1 - but Jacopo objected with the following:
>
> > Checking for is_txa() and is_txb() would require to call
> > 'adv_sd_to_csi2(sd)' before having made sure the 'sd' actually
> > represent a csi2_tx. I would keep it as it is.
>

That was at the time where the .link_setup() callback was called for
TXs and non-TXs. What you proposed was to call:

#define adv748x_sd_to_csi2(sd) container_of(sd, struct adv748x_csi2, sd)

on variables that we don't have any guarantee that are of type 'struct
adv748x_csi2'. I still think it is dangerous and should be avoided and
I worked it around in v1 as:

+	if ((sd != &state->txa.sd && sd != &state->txb.sd) ||

> Now I look at the implementation here, I see this is precisely what it
> is doing anyway .... still converting through adv748x_sd_to_csi2(sd) on
> an unknown pointer type
>  (which I still believe is a valid thing to do in this instance)

It's not unknown, .link_setup() is only registered for TXs. If it gets
called, we know we're dealing with a TX.

>
> So yes, I think this would be simpler having the check at the top of the
> adv748x_link_setup() call, and thus then there is no need to add a
> second adv_media_ops structure.

That was what I did in v1, didn't I ?

The current implementation looks better imho, but if the both of you
prefer something similar to v1 I will consider that.

Thanks
   j
>
>
> >>>  }
> >
> > [snip]
> >
>
> --
> Regards
> --
> Kieran
Laurent Pinchart Jan. 10, 2019, 9:27 a.m. UTC | #7
Hi Jacopo,

On Thursday, 10 January 2019 10:51:00 EET Jacopo Mondi wrote:
> On Wed, Jan 09, 2019 at 02:15:04AM +0200, Laurent Pinchart wrote:
> > On Monday, 7 January 2019 14:36:28 EET Kieran Bingham wrote:
> >> On 06/01/2019 15:54, Jacopo Mondi wrote:
> >>> When the adv748x driver is informed about a link being created from
> >>> HDMI or AFE to a CSI-2 TX output, the 'link_setup()' callback is
> >>> invoked. Make sure to implement proper routing management at link setup
> >>> time, to route the selected video stream to the desired TX output.
> >> 
> >> Overall this looks like the right approach - but I feel like the
> >> handling of the io10 register might need some consideration, because
> >> it's value depends on the condition of both CSI2 transmitters, not just
> >> the currently parsed link.
> >> 
> >> I had a go at some pseudo - uncompiled/untested code inline as a
> >> suggestion.
> >> 
> >> If you think it's better - feel free to rework it in ... or not as you
> >> see fit.
> >> 
> >>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> >>> ---
> >>> 
> >>>  drivers/media/i2c/adv748x/adv748x-core.c | 57 +++++++++++++++++++++++-
> >>>  drivers/media/i2c/adv748x/adv748x.h      |  2 +
> >>>  2 files changed, 58 insertions(+), 1 deletion(-)
> >>> 
> >>> diff --git a/drivers/media/i2c/adv748x/adv748x-core.c
> >>> b/drivers/media/i2c/adv748x/adv748x-core.c index
> >>> 200e00f93546..a586bf393558 100644
> >>> --- a/drivers/media/i2c/adv748x/adv748x-core.c
> >>> +++ b/drivers/media/i2c/adv748x/adv748x-core.c

[snip]

> >>> +static const struct media_entity_operations adv748x_tx_media_ops = {
> >>> +	.link_setup	= adv748x_link_setup,
> >>> +	.link_validate	= v4l2_subdev_link_validate,
> >>> +};
> >>> 
> >>>  static const struct media_entity_operations adv748x_media_ops = {
> >>>  	.link_validate = v4l2_subdev_link_validate,
> >>> @@ -516,7 +570,8 @@ void adv748x_subdev_init(struct v4l2_subdev *sd,
> >>> struct adv748x_state *state,
> >>>  		state->client->addr, ident);
> >>>  	
> >>>  	sd->entity.function = function;
> >>> -	sd->entity.ops = &adv748x_media_ops;
> >>> +	sd->entity.ops = is_tx(adv748x_sd_to_csi2(sd)) ?
> >>> +			 &adv748x_tx_media_ops : &adv748x_media_ops;
> >> 
> >> Aha - yes that's a neat solution to ensure that only the TX links
> >> generate link_setup calls :)
> > 
> > Another option would be to bail out from adv748x_link_setup() if the
> > entity is not a TX*.
> 
> If I'm not wrong you suggested me to register a set of operations with
> the .link_setup callback only for TX entities, and I agree it is much
> better, so I'm leaning to leave it as it is in this series.

Sorry, I should have made it clear that this wasn't a request for a change, 
just pointing out another potential option. Your implementation is fine with 
me.

> >>>  }
> > 
> > [snip]
Jacopo Mondi Jan. 10, 2019, 10:01 a.m. UTC | #8
Hi Kieran,

On Mon, Jan 07, 2019 at 12:36:28PM +0000, Kieran Bingham wrote:
> Hi Jacopo,
>
> On 06/01/2019 15:54, Jacopo Mondi wrote:
> > When the adv748x driver is informed about a link being created from HDMI or
> > AFE to a CSI-2 TX output, the 'link_setup()' callback is invoked. Make
> > sure to implement proper routing management at link setup time, to route
> > the selected video stream to the desired TX output.
>
> Overall this looks like the right approach - but I feel like the
> handling of the io10 register might need some consideration, because
> it's value depends on the condition of both CSI2 transmitters, not just
> the currently parsed link.
>
> I had a go at some pseudo - uncompiled/untested code inline as a suggestion.
>
> If you think it's better - feel free to rework it in ... or not as you
> see fit.
>
> Regards
>
> Kieran
>
>
>
>
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> >  drivers/media/i2c/adv748x/adv748x-core.c | 57 +++++++++++++++++++++++-
> >  drivers/media/i2c/adv748x/adv748x.h      |  2 +
> >  2 files changed, 58 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c
> > index 200e00f93546..a586bf393558 100644
> > --- a/drivers/media/i2c/adv748x/adv748x-core.c
> > +++ b/drivers/media/i2c/adv748x/adv748x-core.c
> > @@ -335,6 +335,60 @@ int adv748x_tx_power(struct adv748x_csi2 *tx, bool on)
> >  /* -----------------------------------------------------------------------------
> >   * Media Operations
> >   */
> > +static int adv748x_link_setup(struct media_entity *entity,
> > +			      const struct media_pad *local,
> > +			      const struct media_pad *remote, u32 flags)
> > +{
> > +	struct v4l2_subdev *rsd = media_entity_to_v4l2_subdev(remote->entity);
> > +	struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(entity);
> > +	struct adv748x_state *state = v4l2_get_subdevdata(sd);
> > +	struct adv748x_csi2 *tx = adv748x_sd_to_csi2(sd);
> > +	bool enable = flags & MEDIA_LNK_FL_ENABLED;
> > +	u8 io10;
> > +
> > +	/* Refuse to enable multiple links to the same TX at the same time. */
> > +	if (enable && tx->src)
> > +		return -EINVAL;
> > +
> > +	/* Set or clear the source (HDMI or AFE) and the current TX. */
> > +	if (rsd == &state->afe.sd)
> > +		state->afe.tx = enable ? tx : NULL;
> > +	else
> > +		state->hdmi.tx = enable ? tx : NULL;
> > +
> > +	tx->src = enable ? rsd : NULL;
> > +
> > +	if (!enable)
> > +		return 0;
>
> Don't we potentially want to take any action on disable to power down
> links below ?
>

You know, I had a thought about it, and I was not sure it was worth
it. The chip stays powered, and even if a TX stays active when it is
not linked, this is not worse than what it was before, and
considering...


> > +
> > +	/* Change video stream routing, according to the newly enabled link. */
> > +	io10 = io_read(state, ADV748X_IO_10);
> > +	if (rsd == &state->afe.sd) {
> > +		/*
> > +		 * Set AFE->TXA routing and power off TXB if AFE goes to TXA.
> > +		 * if AFE goes to TXB, we need both TXA and TXB powered on.
> > +		 */

... this
I should not simply disable, say TXA, because the HDMI->TXA link has
been disabled, but I should inspect the global state everytime.

> > +		io10 &= ~ADV748X_IO_10_CSI1_EN;
> > +		io10 &= ~ADV748X_IO_10_CSI4_IN_SEL_AFE;
> > +		if (is_txa(tx))
> > +			io10 |= ADV748X_IO_10_CSI4_IN_SEL_AFE;
>
> Shouldn't the CSI4 be enabled here too? or are we assuming it's already
> (/always) enabled?
> 		io10 |= ADV748X_IO_10_CSI4_EN;

Correct, I wrongly assumed it stayed on, but it could be not.

>
> > +		else
> > +			io10 |= ADV748X_IO_10_CSI4_EN |
> > +				ADV748X_IO_10_CSI1_EN;
> > +	} else {
> > +		/* Clear AFE->TXA routing and power up TXA. */
> > +		io10 &= ~ADV748X_IO_10_CSI4_IN_SEL_AFE;
> > +		io10 |= ADV748X_IO_10_CSI4_EN;
>
> But if we assume it's already enabled ... do we need this?
> Perhaps it might be better to be explicit on this?
>
> > +	}
> > +	io_write(state, ADV748X_IO_10, io10);
>
>
> Would it be any cleaner to use io_clrset() here?

Not sure if it is cleaner, but I checked if it was safe to write the
whole IO_10 register, and the only fields not touched by this are the
ones controlling output redirection to the TTL port, which should stay
zeros. Bits [1:0] are not documented though..
>
> Hrm ... also it feels like this register really should be set depending
> upon the complete state of ... &state->...
>

Possibly, that would help catching cases where a TX (or both TXs) could be
shut down, if we consider it worthy.

> So perhaps it deserves it's own function which should be called after
> csi_registered() callback and any link change.
>

Not sure if it makes sense to touch this register when a subdevice
gets registered... we create links at that time, but they're not
enabled, so the only place where this configuration changes is
actually this function. I'm not sure it is worth breaking this out,
but I agree the full state should probably be inspected, and not just
the last changed route.

I'll run some tests and see how it works.
Thanks for the comments
   j


> /me has a quick go at some psuedo codeishness...:
>
> int adv74x_io_10(struct adv748x_state *state);
> 	u8 bits = 0;
> 	u8 mask = ADV748X_IO_10_CSI1_EN
> 		| ADV748X_IO_10_CSI4_EN
> 		| ADV748X_IO_10_CSI4_IN_SEL_AFE;
>
> 	if (state->afe.tx) {
> 		/* AFE Requires TXA enabled, even when output to TXB */
> 		bits |= ADV748X_IO_10_CSI4_EN;
>
> 		if (is_txa(state->afe.tx))
> 			bits |= ADV748X_IO_10_CSI4_IN_SEL_AFE
> 		else
> 			bits |= ADV748X_IO_10_CSI1_EN;
> 	}
>
> 	if (state->hdmi.tx) {
> 		bits |= ADV748X_IO_10_CSI4_EN;
> 	}
>
> 	return io_clrset(state, ADV748X_IO_10, mask, bits);
> }
>
> How does that look ? (is it even correct first?)
>
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct media_entity_operations adv748x_tx_media_ops = {
> > +	.link_setup	= adv748x_link_setup,
> > +	.link_validate	= v4l2_subdev_link_validate,
> > +};
> >
> >  static const struct media_entity_operations adv748x_media_ops = {
> >  	.link_validate = v4l2_subdev_link_validate,
> > @@ -516,7 +570,8 @@ void adv748x_subdev_init(struct v4l2_subdev *sd, struct adv748x_state *state,
> >  		state->client->addr, ident);
> >
> >  	sd->entity.function = function;
> > -	sd->entity.ops = &adv748x_media_ops;
> > +	sd->entity.ops = is_tx(adv748x_sd_to_csi2(sd)) ?
> > +			 &adv748x_tx_media_ops : &adv748x_media_ops;
>
> Aha - yes that's a neat solution to ensure that only the TX links
> generate link_setup calls :)
>
>
>
> >  }
> >
> >  static int adv748x_parse_csi2_lanes(struct adv748x_state *state,
> > diff --git a/drivers/media/i2c/adv748x/adv748x.h b/drivers/media/i2c/adv748x/adv748x.h
> > index 6eb2e4a95eed..eb19c6cbbb4e 100644
> > --- a/drivers/media/i2c/adv748x/adv748x.h
> > +++ b/drivers/media/i2c/adv748x/adv748x.h
> > @@ -93,6 +93,7 @@ struct adv748x_csi2 {
> >
> >  #define is_tx_enabled(_tx) ((_tx)->state->endpoints[(_tx)->port] != NULL)
> >  #define __is_tx(_tx, _ab) ((_tx) == &(_tx)->state->tx##_ab)
> > +#define is_tx(_tx) (is_txa(_tx) || is_txb(_tx))
> >  #define is_txa(_tx) __is_tx(_tx, a)
> >  #define is_txb(_tx) __is_tx(_tx, b)
> >
> > @@ -224,6 +225,7 @@ struct adv748x_state {
> >  #define ADV748X_IO_10_CSI4_EN		BIT(7)
> >  #define ADV748X_IO_10_CSI1_EN		BIT(6)
> >  #define ADV748X_IO_10_PIX_OUT_EN	BIT(5)
> > +#define ADV748X_IO_10_CSI4_IN_SEL_AFE	BIT(3)
>
>
>
> >
> >  #define ADV748X_IO_CHIP_REV_ID_1	0xdf
> >  #define ADV748X_IO_CHIP_REV_ID_2	0xe0
> >
>
> --
> Regards
> --
> Kieran
Kieran Bingham Jan. 10, 2019, 10:05 a.m. UTC | #9
Hi Jacopo,

On 10/01/2019 08:58, Jacopo Mondi wrote:
> On Wed, Jan 09, 2019 at 02:15:33PM +0000, Kieran Bingham wrote:
>> On 09/01/2019 00:15, Laurent Pinchart wrote:
>>> Hello,
>>>
>>> On Monday, 7 January 2019 14:36:28 EET Kieran Bingham wrote:
>>>> On 06/01/2019 15:54, Jacopo Mondi wrote:
>>>>> When the adv748x driver is informed about a link being created from HDMI
>>>>> or AFE to a CSI-2 TX output, the 'link_setup()' callback is invoked. Make
>>>>> sure to implement proper routing management at link setup time, to route
>>>>> the selected video stream to the desired TX output.
>>>>
>>>> Overall this looks like the right approach - but I feel like the
>>>> handling of the io10 register might need some consideration, because
>>>> it's value depends on the condition of both CSI2 transmitters, not just
>>>> the currently parsed link.
>>>>
>>>> I had a go at some pseudo - uncompiled/untested code inline as a suggestion.
>>>>
>>>> If you think it's better - feel free to rework it in ... or not as you
>>>> see fit.
>>>>
>>>>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
>>>>> ---
>>>>>
>>>>>  drivers/media/i2c/adv748x/adv748x-core.c | 57 +++++++++++++++++++++++-
>>>>>  drivers/media/i2c/adv748x/adv748x.h      |  2 +
>>>>>  2 files changed, 58 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/media/i2c/adv748x/adv748x-core.c
>>>>> b/drivers/media/i2c/adv748x/adv748x-core.c index
>>>>> 200e00f93546..a586bf393558 100644
>>>>> --- a/drivers/media/i2c/adv748x/adv748x-core.c
>>>>> +++ b/drivers/media/i2c/adv748x/adv748x-core.c
>>>>> @@ -335,6 +335,60 @@ int adv748x_tx_power(struct adv748x_csi2 *tx, bool
>>>>> on)
>>>>>  /* ----------------------------------------------------------------------
>>>>>   * Media Operations
>>>>>   */
>>>>> +static int adv748x_link_setup(struct media_entity *entity,
>>>>> +			      const struct media_pad *local,
>>>>> +			      const struct media_pad *remote, u32 flags)
>>>>> +{
>>>>> +	struct v4l2_subdev *rsd = media_entity_to_v4l2_subdev(remote->entity);
>>>>> +	struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(entity);
>>>>> +	struct adv748x_state *state = v4l2_get_subdevdata(sd);
>>>>> +	struct adv748x_csi2 *tx = adv748x_sd_to_csi2(sd);
>>>>> +	bool enable = flags & MEDIA_LNK_FL_ENABLED;
>>>>> +	u8 io10;
>>>>> +
>>>>> +	/* Refuse to enable multiple links to the same TX at the same time. */
>>>>> +	if (enable && tx->src)
>>>>> +		return -EINVAL;
>>>>> +
>>>>> +	/* Set or clear the source (HDMI or AFE) and the current TX. */
>>>>> +	if (rsd == &state->afe.sd)
>>>>> +		state->afe.tx = enable ? tx : NULL;
>>>>> +	else
>>>>> +		state->hdmi.tx = enable ? tx : NULL;
>>>>> +
>>>>> +	tx->src = enable ? rsd : NULL;
>>>>> +
>>>>> +	if (!enable)
>>>>> +		return 0;
>>>>
>>>> Don't we potentially want to take any action on disable to power down
>>>> links below ?
>>>>
>>>>> +
>>>>> +	/* Change video stream routing, according to the newly enabled link. */
>>>>> +	io10 = io_read(state, ADV748X_IO_10);
>>>>> +	if (rsd == &state->afe.sd) {
>>>>> +		/*
>>>>> +		 * Set AFE->TXA routing and power off TXB if AFE goes to TXA.
>>>>> +		 * if AFE goes to TXB, we need both TXA and TXB powered on.
>>>>> +		 */
>>>>> +		io10 &= ~ADV748X_IO_10_CSI1_EN;
>>>>> +		io10 &= ~ADV748X_IO_10_CSI4_IN_SEL_AFE;
>>>>> +		if (is_txa(tx))
>>>>> +			io10 |= ADV748X_IO_10_CSI4_IN_SEL_AFE;
>>>>
>>>> Shouldn't the CSI4 be enabled here too? or are we assuming it's already
>>>> (/always) enabled?
>>>> 		io10 |= ADV748X_IO_10_CSI4_EN;
>>>>
>>>>> +		else
>>>>> +			io10 |= ADV748X_IO_10_CSI4_EN |
>>>>> +				ADV748X_IO_10_CSI1_EN;
>>>>> +	} else {
>>>>> +		/* Clear AFE->TXA routing and power up TXA. */
>>>>> +		io10 &= ~ADV748X_IO_10_CSI4_IN_SEL_AFE;
>>>>> +		io10 |= ADV748X_IO_10_CSI4_EN;
>>>>
>>>> But if we assume it's already enabled ... do we need this?
>>>> Perhaps it might be better to be explicit on this?
>>>>
>>>>> +	}
>>>>> +	io_write(state, ADV748X_IO_10, io10);
>>>>
>>>> Would it be any cleaner to use io_clrset() here?
>>>>
>>>> Hrm ... also it feels like this register really should be set depending
>>>> upon the complete state of ... &state->...
>>>>
>>>> So perhaps it deserves it's own function which should be called after
>>>> csi_registered() callback and any link change.
>>>>
>>>> /me has a quick go at some psuedo codeishness...:
>>>>
>>>> int adv74x_io_10(struct adv748x_state *state);
>>>> 	u8 bits = 0;
>>>> 	u8 mask = ADV748X_IO_10_CSI1_EN
>>>>
>>>> 		| ADV748X_IO_10_CSI4_EN
>>>> 		| ADV748X_IO_10_CSI4_IN_SEL_AFE;
>>>>
>>>> 	if (state->afe.tx) {
>>>> 		/* AFE Requires TXA enabled, even when output to TXB */
>>>> 		bits |= ADV748X_IO_10_CSI4_EN;
>>>>
>>>> 		if (is_txa(state->afe.tx))
>>>> 			bits |= ADV748X_IO_10_CSI4_IN_SEL_AFE
>>>> 		else
>>>> 			bits |= ADV748X_IO_10_CSI1_EN;
>>>> 	}
>>>>
>>>> 	if (state->hdmi.tx) {
>>>> 		bits |= ADV748X_IO_10_CSI4_EN;
>>>> 	}
>>>>
>>>> 	return io_clrset(state, ADV748X_IO_10, mask, bits);
>>>> }
>>>>
>>>> How does that look ? (is it even correct first?)
>>>>
>>>>> +
>>>>> +	return 0;
>>>>> +}
>>>>> +
>>>>> +static const struct media_entity_operations adv748x_tx_media_ops = {
>>>>> +	.link_setup	= adv748x_link_setup,
>>>>> +	.link_validate	= v4l2_subdev_link_validate,
>>>>> +};
>>>>>
>>>>>  static const struct media_entity_operations adv748x_media_ops = {
>>>>>  	.link_validate = v4l2_subdev_link_validate,
>>>>> @@ -516,7 +570,8 @@ void adv748x_subdev_init(struct v4l2_subdev *sd,
>>>>> struct adv748x_state *state,
>>>>>  		state->client->addr, ident);
>>>>>
>>>>>  	sd->entity.function = function;
>>>>> -	sd->entity.ops = &adv748x_media_ops;
>>>>> +	sd->entity.ops = is_tx(adv748x_sd_to_csi2(sd)) ?
>>>>> +			 &adv748x_tx_media_ops : &adv748x_media_ops;
>>>>
>>>> Aha - yes that's a neat solution to ensure that only the TX links
>>>> generate link_setup calls :)
>>>
>>> Another option would be to bail out from adv748x_link_setup() if the entity is
>>> not a TX*.
>>>
>>
>> I suggested this in v1 - but Jacopo objected with the following:
>>
>>> Checking for is_txa() and is_txb() would require to call
>>> 'adv_sd_to_csi2(sd)' before having made sure the 'sd' actually
>>> represent a csi2_tx. I would keep it as it is.
>>
> 
> That was at the time where the .link_setup() callback was called for
> TXs and non-TXs. What you proposed was to call:
> 
> #define adv748x_sd_to_csi2(sd) container_of(sd, struct adv748x_csi2, sd)
> 
> on variables that we don't have any guarantee that are of type 'struct
> adv748x_csi2'. I still think it is dangerous and should be avoided and
> I worked it around in v1 as:
> 
> +	if ((sd != &state->txa.sd && sd != &state->txb.sd) ||
> 
>> Now I look at the implementation here, I see this is precisely what it
>> is doing anyway .... still converting through adv748x_sd_to_csi2(sd) on
>> an unknown pointer type
>>  (which I still believe is a valid thing to do in this instance)
> 
> It's not unknown, .link_setup() is only registered for TXs. If it gets
> called, we know we're dealing with a TX.
> 
>>
>> So yes, I think this would be simpler having the check at the top of the
>> adv748x_link_setup() call, and thus then there is no need to add a
>> second adv_media_ops structure.
> 
> That was what I did in v1, didn't I ?
> 
> The current implementation looks better imho, but if the both of you
> prefer something similar to v1 I will consider that.

Given the extra clarification above, I'll not object to keeping it this
way. I still think it's fine to use container of and then check the
pointers for failure. The is_tx() would perform the type-validation :) -
but lets stick with the one that you prefer. Its your patch :)
--
Kieran
Jacopo Mondi Jan. 10, 2019, 1:40 p.m. UTC | #10
Hi Kieran,
On Mon, Jan 07, 2019 at 12:36:28PM +0000, Kieran Bingham wrote:
> Hi Jacopo,
>
> On 06/01/2019 15:54, Jacopo Mondi wrote:
> > When the adv748x driver is informed about a link being created from HDMI or
> > AFE to a CSI-2 TX output, the 'link_setup()' callback is invoked. Make
> > sure to implement proper routing management at link setup time, to route
> > the selected video stream to the desired TX output.
>
> Overall this looks like the right approach - but I feel like the
> handling of the io10 register might need some consideration, because
> it's value depends on the condition of both CSI2 transmitters, not just
> the currently parsed link.
>
> I had a go at some pseudo - uncompiled/untested code inline as a suggestion.
>
> If you think it's better - feel free to rework it in ... or not as you
> see fit.
>
> Regards
>
> Kieran
>
>
>
>
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> >  drivers/media/i2c/adv748x/adv748x-core.c | 57 +++++++++++++++++++++++-
> >  drivers/media/i2c/adv748x/adv748x.h      |  2 +
> >  2 files changed, 58 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c
> > index 200e00f93546..a586bf393558 100644
> > --- a/drivers/media/i2c/adv748x/adv748x-core.c
> > +++ b/drivers/media/i2c/adv748x/adv748x-core.c
> > @@ -335,6 +335,60 @@ int adv748x_tx_power(struct adv748x_csi2 *tx, bool on)
> >  /* -----------------------------------------------------------------------------
> >   * Media Operations
> >   */
> > +static int adv748x_link_setup(struct media_entity *entity,
> > +			      const struct media_pad *local,
> > +			      const struct media_pad *remote, u32 flags)
> > +{
> > +	struct v4l2_subdev *rsd = media_entity_to_v4l2_subdev(remote->entity);
> > +	struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(entity);
> > +	struct adv748x_state *state = v4l2_get_subdevdata(sd);
> > +	struct adv748x_csi2 *tx = adv748x_sd_to_csi2(sd);
> > +	bool enable = flags & MEDIA_LNK_FL_ENABLED;
> > +	u8 io10;
> > +
> > +	/* Refuse to enable multiple links to the same TX at the same time. */
> > +	if (enable && tx->src)
> > +		return -EINVAL;
> > +
> > +	/* Set or clear the source (HDMI or AFE) and the current TX. */
> > +	if (rsd == &state->afe.sd)
> > +		state->afe.tx = enable ? tx : NULL;
> > +	else
> > +		state->hdmi.tx = enable ? tx : NULL;
> > +
> > +	tx->src = enable ? rsd : NULL;
> > +
> > +	if (!enable)
> > +		return 0;
>
> Don't we potentially want to take any action on disable to power down
> links below ?
>
> > +
> > +	/* Change video stream routing, according to the newly enabled link. */
> > +	io10 = io_read(state, ADV748X_IO_10);
> > +	if (rsd == &state->afe.sd) {
> > +		/*
> > +		 * Set AFE->TXA routing and power off TXB if AFE goes to TXA.
> > +		 * if AFE goes to TXB, we need both TXA and TXB powered on.
> > +		 */
> > +		io10 &= ~ADV748X_IO_10_CSI1_EN;
> > +		io10 &= ~ADV748X_IO_10_CSI4_IN_SEL_AFE;
> > +		if (is_txa(tx))
> > +			io10 |= ADV748X_IO_10_CSI4_IN_SEL_AFE;
>
> Shouldn't the CSI4 be enabled here too? or are we assuming it's already
> (/always) enabled?
> 		io10 |= ADV748X_IO_10_CSI4_EN;
>
> > +		else
> > +			io10 |= ADV748X_IO_10_CSI4_EN |
> > +				ADV748X_IO_10_CSI1_EN;
> > +	} else {
> > +		/* Clear AFE->TXA routing and power up TXA. */
> > +		io10 &= ~ADV748X_IO_10_CSI4_IN_SEL_AFE;
> > +		io10 |= ADV748X_IO_10_CSI4_EN;
>
> But if we assume it's already enabled ... do we need this?
> Perhaps it might be better to be explicit on this?
>
> > +	}
> > +	io_write(state, ADV748X_IO_10, io10);
>
>
> Would it be any cleaner to use io_clrset() here?
>
> Hrm ... also it feels like this register really should be set depending
> upon the complete state of ... &state->...
>
> So perhaps it deserves it's own function which should be called after
> csi_registered() callback and any link change.
>
> /me has a quick go at some psuedo codeishness...:
>
> int adv74x_io_10(struct adv748x_state *state);
> 	u8 bits = 0;
> 	u8 mask = ADV748X_IO_10_CSI1_EN
> 		| ADV748X_IO_10_CSI4_EN
> 		| ADV748X_IO_10_CSI4_IN_SEL_AFE;
>
> 	if (state->afe.tx) {
> 		/* AFE Requires TXA enabled, even when output to TXB */
> 		bits |= ADV748X_IO_10_CSI4_EN;
>
> 		if (is_txa(state->afe.tx))
> 			bits |= ADV748X_IO_10_CSI4_IN_SEL_AFE
> 		else
> 			bits |= ADV748X_IO_10_CSI1_EN;
> 	}
>
> 	if (state->hdmi.tx) {
> 		bits |= ADV748X_IO_10_CSI4_EN;
> 	}
>
> 	return io_clrset(state, ADV748X_IO_10, mask, bits);
> }
>
> How does that look ? (is it even correct first?)
>

Thanks, I've now updated the implementation to use something very
similar to what you proposed here!

Thanks
  j

> > +
> > +	return 0;
> > +}
> > +
> > +static const struct media_entity_operations adv748x_tx_media_ops = {
> > +	.link_setup	= adv748x_link_setup,
> > +	.link_validate	= v4l2_subdev_link_validate,
> > +};
> >
> >  static const struct media_entity_operations adv748x_media_ops = {
> >  	.link_validate = v4l2_subdev_link_validate,
> > @@ -516,7 +570,8 @@ void adv748x_subdev_init(struct v4l2_subdev *sd, struct adv748x_state *state,
> >  		state->client->addr, ident);
> >
> >  	sd->entity.function = function;
> > -	sd->entity.ops = &adv748x_media_ops;
> > +	sd->entity.ops = is_tx(adv748x_sd_to_csi2(sd)) ?
> > +			 &adv748x_tx_media_ops : &adv748x_media_ops;
>
> Aha - yes that's a neat solution to ensure that only the TX links
> generate link_setup calls :)
>
>
>
> >  }
> >
> >  static int adv748x_parse_csi2_lanes(struct adv748x_state *state,
> > diff --git a/drivers/media/i2c/adv748x/adv748x.h b/drivers/media/i2c/adv748x/adv748x.h
> > index 6eb2e4a95eed..eb19c6cbbb4e 100644
> > --- a/drivers/media/i2c/adv748x/adv748x.h
> > +++ b/drivers/media/i2c/adv748x/adv748x.h
> > @@ -93,6 +93,7 @@ struct adv748x_csi2 {
> >
> >  #define is_tx_enabled(_tx) ((_tx)->state->endpoints[(_tx)->port] != NULL)
> >  #define __is_tx(_tx, _ab) ((_tx) == &(_tx)->state->tx##_ab)
> > +#define is_tx(_tx) (is_txa(_tx) || is_txb(_tx))
> >  #define is_txa(_tx) __is_tx(_tx, a)
> >  #define is_txb(_tx) __is_tx(_tx, b)
> >
> > @@ -224,6 +225,7 @@ struct adv748x_state {
> >  #define ADV748X_IO_10_CSI4_EN		BIT(7)
> >  #define ADV748X_IO_10_CSI1_EN		BIT(6)
> >  #define ADV748X_IO_10_PIX_OUT_EN	BIT(5)
> > +#define ADV748X_IO_10_CSI4_IN_SEL_AFE	BIT(3)
>
>
>
> >
> >  #define ADV748X_IO_CHIP_REV_ID_1	0xdf
> >  #define ADV748X_IO_CHIP_REV_ID_2	0xe0
> >
>
> --
> Regards
> --
> Kieran

Patch
diff mbox series

diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c
index 200e00f93546..a586bf393558 100644
--- a/drivers/media/i2c/adv748x/adv748x-core.c
+++ b/drivers/media/i2c/adv748x/adv748x-core.c
@@ -335,6 +335,60 @@  int adv748x_tx_power(struct adv748x_csi2 *tx, bool on)
 /* -----------------------------------------------------------------------------
  * Media Operations
  */
+static int adv748x_link_setup(struct media_entity *entity,
+			      const struct media_pad *local,
+			      const struct media_pad *remote, u32 flags)
+{
+	struct v4l2_subdev *rsd = media_entity_to_v4l2_subdev(remote->entity);
+	struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(entity);
+	struct adv748x_state *state = v4l2_get_subdevdata(sd);
+	struct adv748x_csi2 *tx = adv748x_sd_to_csi2(sd);
+	bool enable = flags & MEDIA_LNK_FL_ENABLED;
+	u8 io10;
+
+	/* Refuse to enable multiple links to the same TX at the same time. */
+	if (enable && tx->src)
+		return -EINVAL;
+
+	/* Set or clear the source (HDMI or AFE) and the current TX. */
+	if (rsd == &state->afe.sd)
+		state->afe.tx = enable ? tx : NULL;
+	else
+		state->hdmi.tx = enable ? tx : NULL;
+
+	tx->src = enable ? rsd : NULL;
+
+	if (!enable)
+		return 0;
+
+	/* Change video stream routing, according to the newly enabled link. */
+	io10 = io_read(state, ADV748X_IO_10);
+	if (rsd == &state->afe.sd) {
+		/*
+		 * Set AFE->TXA routing and power off TXB if AFE goes to TXA.
+		 * if AFE goes to TXB, we need both TXA and TXB powered on.
+		 */
+		io10 &= ~ADV748X_IO_10_CSI1_EN;
+		io10 &= ~ADV748X_IO_10_CSI4_IN_SEL_AFE;
+		if (is_txa(tx))
+			io10 |= ADV748X_IO_10_CSI4_IN_SEL_AFE;
+		else
+			io10 |= ADV748X_IO_10_CSI4_EN |
+				ADV748X_IO_10_CSI1_EN;
+	} else {
+		/* Clear AFE->TXA routing and power up TXA. */
+		io10 &= ~ADV748X_IO_10_CSI4_IN_SEL_AFE;
+		io10 |= ADV748X_IO_10_CSI4_EN;
+	}
+	io_write(state, ADV748X_IO_10, io10);
+
+	return 0;
+}
+
+static const struct media_entity_operations adv748x_tx_media_ops = {
+	.link_setup	= adv748x_link_setup,
+	.link_validate	= v4l2_subdev_link_validate,
+};
 
 static const struct media_entity_operations adv748x_media_ops = {
 	.link_validate = v4l2_subdev_link_validate,
@@ -516,7 +570,8 @@  void adv748x_subdev_init(struct v4l2_subdev *sd, struct adv748x_state *state,
 		state->client->addr, ident);
 
 	sd->entity.function = function;
-	sd->entity.ops = &adv748x_media_ops;
+	sd->entity.ops = is_tx(adv748x_sd_to_csi2(sd)) ?
+			 &adv748x_tx_media_ops : &adv748x_media_ops;
 }
 
 static int adv748x_parse_csi2_lanes(struct adv748x_state *state,
diff --git a/drivers/media/i2c/adv748x/adv748x.h b/drivers/media/i2c/adv748x/adv748x.h
index 6eb2e4a95eed..eb19c6cbbb4e 100644
--- a/drivers/media/i2c/adv748x/adv748x.h
+++ b/drivers/media/i2c/adv748x/adv748x.h
@@ -93,6 +93,7 @@  struct adv748x_csi2 {
 
 #define is_tx_enabled(_tx) ((_tx)->state->endpoints[(_tx)->port] != NULL)
 #define __is_tx(_tx, _ab) ((_tx) == &(_tx)->state->tx##_ab)
+#define is_tx(_tx) (is_txa(_tx) || is_txb(_tx))
 #define is_txa(_tx) __is_tx(_tx, a)
 #define is_txb(_tx) __is_tx(_tx, b)
 
@@ -224,6 +225,7 @@  struct adv748x_state {
 #define ADV748X_IO_10_CSI4_EN		BIT(7)
 #define ADV748X_IO_10_CSI1_EN		BIT(6)
 #define ADV748X_IO_10_PIX_OUT_EN	BIT(5)
+#define ADV748X_IO_10_CSI4_IN_SEL_AFE	BIT(3)
 
 #define ADV748X_IO_CHIP_REV_ID_1	0xdf
 #define ADV748X_IO_CHIP_REV_ID_2	0xe0