| Message ID | 20190106155413.30666-2-jacopo+renesas@jmondi.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series | media: adv748x: Implement dynamic routing support | expand |
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 || \ >
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
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>
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 --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 || \
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(-)