Re: [PATCH v2 4/5] media: i2c: adv748x: Register only enabled inputs

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Jacopo,

Thankyou for the patch,

On 05/09/18 16:27, Jacopo Mondi wrote:
> The adv748x assumes input endpoints are always enabled, and registers
> a subdevice for each of them everytime the corresponding output subdevice
> is registered.
> 

s/everytime/every time/

Although this sounds like something that happens repeatedly in the
lifetime of the driver when it only actually happens once I believe?

In which case perhaps:

s/everytime/when/

> Fix this by conditionally register the input subdevice only if it is

s/register/registering/

> actually described in device tree.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@xxxxxxxxxx>

Otherwise,

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@xxxxxxxxxxxxxxxx>

> ---
>  drivers/media/i2c/adv748x/adv748x-csi2.c |  6 +++---
>  drivers/media/i2c/adv748x/adv748x.h      | 10 ++++++++++
>  2 files changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c b/drivers/media/i2c/adv748x/adv748x-csi2.c
> index 034fd93..9e9df51 100644
> --- a/drivers/media/i2c/adv748x/adv748x-csi2.c
> +++ b/drivers/media/i2c/adv748x/adv748x-csi2.c
> @@ -82,15 +82,15 @@ static int adv748x_csi2_registered(struct v4l2_subdev *sd)
>  	 *
>  	 * Link HDMI->TXA, and AFE->TXB directly.
>  	 */
> -	if (is_txa(tx)) {
> +	if (is_txa(tx) && is_hdmi_enabled(state))
>  		return adv748x_csi2_register_link(tx, sd->v4l2_dev,
>  						  &state->hdmi.sd,
>  						  ADV748X_HDMI_SOURCE);
> -	} else {
> +	if (!is_txa(tx) && is_afe_enabled(state))
>  		return adv748x_csi2_register_link(tx, sd->v4l2_dev,
>  						  &state->afe.sd,
>  						  ADV748X_AFE_SOURCE);
> -	}
> +	return 0;
>  }
>  
>  static const struct v4l2_subdev_internal_ops adv748x_csi2_internal_ops = {
> diff --git a/drivers/media/i2c/adv748x/adv748x.h b/drivers/media/i2c/adv748x/adv748x.h
> index eeadf05..a34004e 100644
> --- a/drivers/media/i2c/adv748x/adv748x.h
> +++ b/drivers/media/i2c/adv748x/adv748x.h
> @@ -94,6 +94,16 @@ struct adv748x_csi2 {
>  #define adv748x_sd_to_csi2(sd) container_of(sd, struct adv748x_csi2, sd)
>  #define is_tx_enabled(_tx) ((_tx)->state->endpoints[(_tx)->port] != NULL)
>  #define is_txa(_tx) ((_tx) == &(_tx)->state->txa)
> +#define is_afe_enabled(_state)					\
> +	((_state)->endpoints[ADV748X_PORT_AIN0] != NULL ||	\
> +	 (_state)->endpoints[ADV748X_PORT_AIN1] != NULL ||	\
> +	 (_state)->endpoints[ADV748X_PORT_AIN2] != NULL ||	\
> +	 (_state)->endpoints[ADV748X_PORT_AIN3] != NULL ||	\
> +	 (_state)->endpoints[ADV748X_PORT_AIN4] != NULL ||	\
> +	 (_state)->endpoints[ADV748X_PORT_AIN5] != NULL ||	\
> +	 (_state)->endpoints[ADV748X_PORT_AIN6] != NULL ||	\
> +	 (_state)->endpoints[ADV748X_PORT_AIN7] != NULL)
> +#define is_hdmi_enabled(_state) ((_state)->endpoints[ADV748X_PORT_HDMI] != NULL)

Wow that's quite some conditionals on the AFE :)

We could introduce a flag instead... but it's just implementation
detail. I'll leave that up to you, if you prefer this then that's fine.
We're not on a hot-path here so it's not a big deal.


>  
>  enum adv748x_hdmi_pads {
>  	ADV748X_HDMI_SINK,
> 




[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux