Re: [PATCH 9/9] drm/bridge: tc358767: Drop tc_read() macro

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Andrey,

Thank you for the patch.

On Tue, Feb 26, 2019 at 11:36:09AM -0800, Andrey Smirnov wrote:
> There's only one place where tc_read() is used, so it doesn't save us
> much. Drop it. No functional change intended.
> 
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> Cc: Archit Taneja <architt@codeaurora.org>
> Cc: Andrzej Hajda <a.hajda@samsung.com>
> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
> Cc: Chris Healy <cphealy@gmail.com>
> Cc: Lucas Stach <l.stach@pengutronix.de>
> Cc: dri-devel@lists.freedesktop.org
> Cc: linux-kernel@vger.kernel.org
> ---
>  drivers/gpu/drm/bridge/tc358767.c | 15 +++++++--------
>  1 file changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> index 239b3aaa255d..3c574f1569aa 100644
> --- a/drivers/gpu/drm/bridge/tc358767.c
> +++ b/drivers/gpu/drm/bridge/tc358767.c
> @@ -240,12 +240,6 @@ static inline struct tc_data *connector_to_tc(struct drm_connector *c)
>  		if (ret)					\
>  			goto err;				\
>  	} while (0)
> -#define tc_read(reg, var)					\
> -	do {							\
> -		ret = regmap_read(tc->regmap, reg, var);	\
> -		if (ret)					\
> -			goto err;				\
> -	} while (0)

While I really like removing the goto from the macro, I think we should
either have accessors for both read and write, or remove them
completely. How about just dropping the goto in this patch, and decide
separately whether to keep accessors or remove them ?

>  static inline int tc_poll_timeout(struct regmap *map, unsigned int addr,
>  				  unsigned int cond_mask,
> @@ -337,8 +331,13 @@ static ssize_t tc_aux_transfer(struct drm_dp_aux *aux,
>  	if (request == DP_AUX_I2C_READ || request == DP_AUX_NATIVE_READ) {
>  		/* Read data */
>  		while (i < size) {
> -			if ((i % 4) == 0)
> -				tc_read(DP0_AUXRDATA(i >> 2), &tmp);
> +			if ((i % 4) == 0) {
> +				ret = regmap_read(tc->regmap,
> +						  DP0_AUXRDATA(i >> 2),
> +						  &tmp);
> +				if (ret)
> +					goto err;

You can return ret directly here.

> +			}
>  			buf[i] = tmp & 0xff;
>  			tmp = tmp >> 8;
>  			i++;

-- 
Regards,

Laurent Pinchart