diff mbox series

[v2,1/6] media: adv748x: Add is_txb()

Message ID 20190106155413.30666-2-jacopo+renesas@jmondi.org (mailing list archive)
State New, archived
Headers show
Series media: adv748x: Implement dynamic routing support | expand

Commit Message

Jacopo Mondi Jan. 6, 2019, 3:54 p.m. UTC 
Add small is_txb() macro to the existing is_txa() and use it where
appropriate.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/i2c/adv748x/adv748x-csi2.c | 2 +-
 drivers/media/i2c/adv748x/adv748x.h      | 6 +++++-
 2 files changed, 6 insertions(+), 2 deletions(-)

Comments

Kieran Bingham Jan. 7, 2019, 9:49 a.m. UTC | #1 
Hi Jacopo

On 06/01/2019 15:54, Jacopo Mondi wrote:
> Add small is_txb() macro to the existing is_txa() and use it where
> appropriate.

Thank you.

I think this will make the code much better to read than if (!is_txa).

> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

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


> ---
>  drivers/media/i2c/adv748x/adv748x-csi2.c | 2 +-
>  drivers/media/i2c/adv748x/adv748x.h      | 6 +++++-
>  2 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c b/drivers/media/i2c/adv748x/adv748x-csi2.c
> index 6ce21542ed48..b6b5d8c7ea7c 100644
> --- a/drivers/media/i2c/adv748x/adv748x-csi2.c
> +++ b/drivers/media/i2c/adv748x/adv748x-csi2.c
> @@ -82,7 +82,7 @@ static int adv748x_csi2_registered(struct v4l2_subdev *sd)
>  		return adv748x_csi2_register_link(tx, sd->v4l2_dev,
>  						  &state->hdmi.sd,
>  						  ADV748X_HDMI_SOURCE);
> -	if (!is_txa(tx) && is_afe_enabled(state))
> +	if (is_txb(tx) && is_afe_enabled(state))
>  		return adv748x_csi2_register_link(tx, sd->v4l2_dev,
>  						  &state->afe.sd,
>  						  ADV748X_AFE_SOURCE);
> diff --git a/drivers/media/i2c/adv748x/adv748x.h b/drivers/media/i2c/adv748x/adv748x.h
> index b482c7fe6957..bc2da1b5ce29 100644
> --- a/drivers/media/i2c/adv748x/adv748x.h
> +++ b/drivers/media/i2c/adv748x/adv748x.h
> @@ -89,8 +89,12 @@ struct adv748x_csi2 {
>  
>  #define notifier_to_csi2(n) container_of(n, struct adv748x_csi2, notifier)
>  #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_tx(_tx, _ab) ((_tx) == &(_tx)->state->tx##_ab)
> +#define is_txa(_tx) __is_tx(_tx, a)
> +#define is_txb(_tx) __is_tx(_tx, b)

I would have just duplicated the is_txa() line here... but this is good
too :)


> +
>  #define is_afe_enabled(_state)					\
>  	((_state)->endpoints[ADV748X_PORT_AIN0] != NULL ||	\
>  	 (_state)->endpoints[ADV748X_PORT_AIN1] != NULL ||	\
>
Jacopo Mondi Jan. 7, 2019, 10:05 a.m. UTC | #2 
Hi Kieran,

On Mon, Jan 07, 2019 at 09:49:25AM +0000, Kieran Bingham wrote:
> Hi Jacopo
>
> On 06/01/2019 15:54, Jacopo Mondi wrote:
> > Add small is_txb() macro to the existing is_txa() and use it where
> > appropriate.
>
> Thank you.
>
> I think this will make the code much better to read than if (!is_txa).
>
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
>
> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
>
>
> > ---
> >  drivers/media/i2c/adv748x/adv748x-csi2.c | 2 +-
> >  drivers/media/i2c/adv748x/adv748x.h      | 6 +++++-
> >  2 files changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c b/drivers/media/i2c/adv748x/adv748x-csi2.c
> > index 6ce21542ed48..b6b5d8c7ea7c 100644
> > --- a/drivers/media/i2c/adv748x/adv748x-csi2.c
> > +++ b/drivers/media/i2c/adv748x/adv748x-csi2.c
> > @@ -82,7 +82,7 @@ static int adv748x_csi2_registered(struct v4l2_subdev *sd)
> >  		return adv748x_csi2_register_link(tx, sd->v4l2_dev,
> >  						  &state->hdmi.sd,
> >  						  ADV748X_HDMI_SOURCE);
> > -	if (!is_txa(tx) && is_afe_enabled(state))
> > +	if (is_txb(tx) && is_afe_enabled(state))
> >  		return adv748x_csi2_register_link(tx, sd->v4l2_dev,
> >  						  &state->afe.sd,
> >  						  ADV748X_AFE_SOURCE);
> > diff --git a/drivers/media/i2c/adv748x/adv748x.h b/drivers/media/i2c/adv748x/adv748x.h
> > index b482c7fe6957..bc2da1b5ce29 100644
> > --- a/drivers/media/i2c/adv748x/adv748x.h
> > +++ b/drivers/media/i2c/adv748x/adv748x.h
> > @@ -89,8 +89,12 @@ struct adv748x_csi2 {
> >
> >  #define notifier_to_csi2(n) container_of(n, struct adv748x_csi2, notifier)
> >  #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_tx(_tx, _ab) ((_tx) == &(_tx)->state->tx##_ab)
> > +#define is_txa(_tx) __is_tx(_tx, a)
> > +#define is_txb(_tx) __is_tx(_tx, b)
>
> I would have just duplicated the is_txa() line here... but this is good
> too :)

I agree it might seem more complex than necessary. I initially made it
like this as I started from the 'is_tx()' macro this series adds in
6/6.

If it is easier to have an '((_tx) == &(_tx)->state->txb)' I can
change this.

Thanks
   j

>
>
> > +
> >  #define is_afe_enabled(_state)					\
> >  	((_state)->endpoints[ADV748X_PORT_AIN0] != NULL ||	\
> >  	 (_state)->endpoints[ADV748X_PORT_AIN1] != NULL ||	\
> >
>
> --
> Regards
> --
> Kieran
Kieran Bingham Jan. 7, 2019, 10:38 a.m. UTC | #3 
Hi Jacopo,

On 07/01/2019 10:05, Jacopo Mondi wrote:
> Hi Kieran,

<snip>

>>> diff --git a/drivers/media/i2c/adv748x/adv748x.h b/drivers/media/i2c/adv748x/adv748x.h
>>> index b482c7fe6957..bc2da1b5ce29 100644
>>> --- a/drivers/media/i2c/adv748x/adv748x.h
>>> +++ b/drivers/media/i2c/adv748x/adv748x.h
>>> @@ -89,8 +89,12 @@ struct adv748x_csi2 {
>>>
>>>  #define notifier_to_csi2(n) container_of(n, struct adv748x_csi2, notifier)
>>>  #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_tx(_tx, _ab) ((_tx) == &(_tx)->state->tx##_ab)
>>> +#define is_txa(_tx) __is_tx(_tx, a)
>>> +#define is_txb(_tx) __is_tx(_tx, b)
>>
>> I would have just duplicated the is_txa() line here... but this is good
>> too :)
> 
> I agree it might seem more complex than necessary. I initially made it
> like this as I started from the 'is_tx()' macro this series adds in
> 6/6.
> 
> If it is easier to have an '((_tx) == &(_tx)->state->txb)' I can
> change this.

It's fine for me as you've got it.

It's still clear and readable, and implements the required functionality.

<snip>
Laurent Pinchart Jan. 9, 2019, 12:04 a.m. UTC | #4 
Hello,

On Monday, 7 January 2019 12:38:39 EET Kieran Bingham wrote:
> On 07/01/2019 10:05, Jacopo Mondi wrote:
> > Hi Kieran,
> 
> <snip>
> 
> >>> diff --git a/drivers/media/i2c/adv748x/adv748x.h
> >>> b/drivers/media/i2c/adv748x/adv748x.h index b482c7fe6957..bc2da1b5ce29
> >>> 100644
> >>> --- a/drivers/media/i2c/adv748x/adv748x.h
> >>> +++ b/drivers/media/i2c/adv748x/adv748x.h
> >>> @@ -89,8 +89,12 @@ struct adv748x_csi2 {
> >>> 
> >>>  #define notifier_to_csi2(n) container_of(n, struct adv748x_csi2,
> >>>  notifier)
> >>>  #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_tx(_tx, _ab) ((_tx) == &(_tx)->state->tx##_ab)
> >>> +#define is_txa(_tx) __is_tx(_tx, a)
> >>> +#define is_txb(_tx) __is_tx(_tx, b)
> >> 
> >> I would have just duplicated the is_txa() line here... but this is good
> >> too :)
> > 
> > I agree it might seem more complex than necessary. I initially made it
> > like this as I started from the 'is_tx()' macro this series adds in
> > 6/6.
> > 
> > If it is easier to have an '((_tx) == &(_tx)->state->txb)' I can
> > change this.

I would find it cleaner to write out is_txa and is_txb explicitly instead of 
hiding the implementation behind an __is_tx macro, especially given that we 
won't have to extend this in the future.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> It's fine for me as you've got it.
> 
> It's still clear and readable, and implements the required functionality.
> 
> <snip>
diff mbox series

Patch

diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c b/drivers/media/i2c/adv748x/adv748x-csi2.c
index 6ce21542ed48..b6b5d8c7ea7c 100644
--- a/drivers/media/i2c/adv748x/adv748x-csi2.c
+++ b/drivers/media/i2c/adv748x/adv748x-csi2.c
@@ -82,7 +82,7 @@  static int adv748x_csi2_registered(struct v4l2_subdev *sd)
 		return adv748x_csi2_register_link(tx, sd->v4l2_dev,
 						  &state->hdmi.sd,
 						  ADV748X_HDMI_SOURCE);
-	if (!is_txa(tx) && is_afe_enabled(state))
+	if (is_txb(tx) && is_afe_enabled(state))
 		return adv748x_csi2_register_link(tx, sd->v4l2_dev,
 						  &state->afe.sd,
 						  ADV748X_AFE_SOURCE);
diff --git a/drivers/media/i2c/adv748x/adv748x.h b/drivers/media/i2c/adv748x/adv748x.h
index b482c7fe6957..bc2da1b5ce29 100644
--- a/drivers/media/i2c/adv748x/adv748x.h
+++ b/drivers/media/i2c/adv748x/adv748x.h
@@ -89,8 +89,12 @@  struct adv748x_csi2 {
 
 #define notifier_to_csi2(n) container_of(n, struct adv748x_csi2, notifier)
 #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_tx(_tx, _ab) ((_tx) == &(_tx)->state->tx##_ab)
+#define is_txa(_tx) __is_tx(_tx, a)
+#define is_txb(_tx) __is_tx(_tx, b)
+
 #define is_afe_enabled(_state)					\
 	((_state)->endpoints[ADV748X_PORT_AIN0] != NULL ||	\
 	 (_state)->endpoints[ADV748X_PORT_AIN1] != NULL ||	\