Re: [PATCH v3 07/15] drm/bridge: tc358767: Simplify AUX data write

[Andrzej Hajda <a.hajda@samsung.com>]

On 05.06.2019 09:04, Andrey Smirnov wrote:
> Simplify AUX data write by dropping index arithmetic and shifting and
> replacing it with a call to a helper function that does three things:
>
>     1. Copies user-provided data into a write buffer
>     2. Optionally fixes the endianness of the write buffer (not needed
>        on LE hosts)
>     3. Transfers contenst of the write buffer to up to 4 32-bit
>        registers on the chip
>
> Note that separate data endianness fix:
>
>     tmp = (tmp << 8) | buf[i];
>
> that was reserved for DP_AUX_I2C_WRITE looks really strange, since it
> will place data differently depending on the passed user-data
> size. E.g. for a write of 1 byte, data transferred to the chip would
> look like:
>
> [byte0] [dummy1] [dummy2] [dummy3]
>
> whereas for a write of 4 bytes we'd get:
>
> [byte3] [byte2] [byte1] [byte0]
>
> Since there's no indication in the datasheet that I2C write buffer
> should be treated differently than AUX write buffer and no comment in
> the original code explaining why it was done this way, that special
> I2C write buffer transformation was dropped in this patch.
>
> 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: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Cc: Andrey Gusakov <andrey.gusakov@cogentembedded.com>
> Cc: Philipp Zabel <p.zabel@pengutronix.de>
> Cc: Cory Tusar <cory.tusar@zii.aero>
> 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 | 59 +++++++++++++++++++------------
>  1 file changed, 37 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> index da47d81e7109..260fbcd0271e 100644
> --- a/drivers/gpu/drm/bridge/tc358767.c
> +++ b/drivers/gpu/drm/bridge/tc358767.c
> @@ -321,6 +321,32 @@ static int tc_aux_get_status(struct tc_data *tc, u8 *reply)
>  	return 0;
>  }
>  
> +static int tc_aux_write_data(struct tc_data *tc, const void *data,
> +			     size_t size)
> +{
> +	u32 auxwdata[DP_AUX_MAX_PAYLOAD_BYTES / sizeof(u32)] = { 0 };
> +	int ret, i, count = DIV_ROUND_UP(size, sizeof(u32));
> +
> +	memcpy(auxwdata, data, size);
> +
> +	for (i = 0; i < count; i++) {
> +		/*
> +		 * Our regmap is configured as LE
> +		 * for register data, so we need
> +		 * undo any byte swapping that will
> +		 * happened to preserve original
> +		 * byte order.
> +		 */
> +		cpu_to_le32s(&auxwdata[i]);
> +	}
> +
> +	ret = regmap_bulk_write(tc->regmap, DP0_AUXWDATA(0), auxwdata, count);
> +	if (ret)
> +		return ret;
> +
> +	return size;
> +}


As prevoiusly maybe regmap_raw_write.

Beside this:

Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>

 --
Regards
Andrzej


> +
>  static int tc_aux_read_data(struct tc_data *tc, void *data, size_t size)
>  {
>  	u32 auxrdata[DP_AUX_MAX_PAYLOAD_BYTES / sizeof(u32)];
> @@ -350,9 +376,6 @@ static ssize_t tc_aux_transfer(struct drm_dp_aux *aux,
>  	struct tc_data *tc = aux_to_tc(aux);
>  	size_t size = min_t(size_t, 8, msg->size);
>  	u8 request = msg->request & ~DP_AUX_I2C_MOT;
> -	u8 *buf = msg->buffer;
> -	u32 tmp = 0;
> -	int i = 0;
>  	int ret;
>  
>  	if (size == 0)
> @@ -362,25 +385,17 @@ static ssize_t tc_aux_transfer(struct drm_dp_aux *aux,
>  	if (ret)
>  		return ret;
>  
> -	if (request == DP_AUX_I2C_WRITE || request == DP_AUX_NATIVE_WRITE) {
> -		/* Store data */
> -		while (i < size) {
> -			if (request == DP_AUX_NATIVE_WRITE)
> -				tmp = tmp | (buf[i] << (8 * (i & 0x3)));
> -			else
> -				tmp = (tmp << 8) | buf[i];
> -			i++;
> -			if (((i % 4) == 0) || (i == size)) {
> -				ret = regmap_write(tc->regmap,
> -						   DP0_AUXWDATA((i - 1) >> 2),
> -						   tmp);
> -				if (ret)
> -					return ret;
> -				tmp = 0;
> -			}
> -		}
> -	} else if (request != DP_AUX_I2C_READ &&
> -		   request != DP_AUX_NATIVE_READ) {
> +	switch (request) {
> +	case DP_AUX_NATIVE_READ:
> +	case DP_AUX_I2C_READ:
> +		break;
> +	case DP_AUX_NATIVE_WRITE:
> +	case DP_AUX_I2C_WRITE:
> +		ret = tc_aux_write_data(tc, msg->buffer, size);
> +		if (ret < 0)
> +			return ret;
> +		break;
> +	default:
>  		return -EINVAL;
>  	}
>