Re: [PATCH 05/15] drm/dsi: Implement generic read and write commands

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

On 10/13/2014 12:16 PM, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> Implement generic read and write commands. Selection of the proper data
> type for packets is done automatically based on the number of parameters
> or payload length.
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>  drivers/gpu/drm/drm_mipi_dsi.c | 115 +++++++++++++++++++++++++++++++++++++++++
>  include/drm/drm_mipi_dsi.h     |   6 +++
>  2 files changed, 121 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
> index 27fc6dac5e4a..7cd69273dbad 100644
> --- a/drivers/gpu/drm/drm_mipi_dsi.c
> +++ b/drivers/gpu/drm/drm_mipi_dsi.c
> @@ -229,6 +229,121 @@ int mipi_dsi_set_maximum_return_packet_size(struct mipi_dsi_device *dsi,
>  EXPORT_SYMBOL(mipi_dsi_set_maximum_return_packet_size);
>  
>  /**
> + * mipi_dsi_generic_write() - transmit data using a generic write packet
> + * @dsi: DSI peripheral device
> + * @payload: buffer containing the payload
> + * @size: size of payload buffer
> + *
> + * This function will automatically choose the right data type depending on
> + * the payload length.
> + *
> + * Return: The number of bytes transmitted on success or a negative error code
> + * on failure.
> + */
> +ssize_t mipi_dsi_generic_write(struct mipi_dsi_device *dsi, const void *payload,
> +			       size_t size)
> +{
> +	struct mipi_dsi_msg msg;
> +	ssize_t err;
> +	u8 *tx;
> +
> +	memset(&msg, 0, sizeof(msg));
> +	msg.channel = dsi->channel;

Again, maybe initializer would be better.

> +	msg.flags = MIPI_DSI_MSG_USE_LPM | MIPI_DSI_MSG_REQ_ACK;


You should not hardcode flags here, these flags have nothing to do with
dsi generic write. But there is general problem of encoding flags in
helpers. Possible solutions I see:
1. Translate DSI flags to msg flags as in case of MIPI_DSI_MODE_LPM ->
MIPI_DSI_MSG_USE_LPM.
2. Copy dsi.mode_flags to msg.flags, or to dedicated field of msg.
3. Call transfer callback with dsi pointer instead of host, this
way it will have access to mode_flags.


> +
> +	if (size > 2) {
> +		tx = kmalloc(2 + size, GFP_KERNEL);
> +		if (!tx)
> +			return -ENOMEM;
> +
> +		tx[0] = (size >> 0) & 0xff;
> +		tx[1] = (size >> 8) & 0xff;
> +
> +		memcpy(tx + 2, payload, size);
> +
> +		msg.tx_len = 2 + size;
> +		msg.tx_buf = tx;
> +	} else {
> +		msg.tx_buf = payload;
> +		msg.tx_len = size;
> +	}

Another API breakage, commented already in response to 1st patch.


> +
> +	switch (size) {
> +	case 0:
> +		msg.type = MIPI_DSI_GENERIC_SHORT_WRITE_0_PARAM;
> +		break;
> +
> +	case 1:
> +		msg.type = MIPI_DSI_GENERIC_SHORT_WRITE_1_PARAM;
> +		break;
> +
> +	case 2:
> +		msg.type = MIPI_DSI_GENERIC_SHORT_WRITE_2_PARAM;
> +		break;
> +
> +	default:
> +		msg.type = MIPI_DSI_GENERIC_LONG_WRITE;
> +		break;
> +	}
> +
> +	err = dsi->host->ops->transfer(dsi->host, &msg);
> +
> +	if (size > 2)
> +		kfree(tx);
> +
> +	return err;
> +}
> +EXPORT_SYMBOL(mipi_dsi_generic_write);
> +
> +/**
> + * mipi_dsi_generic_read() - receive data using a generic read packet
> + * @dsi: DSI peripheral device
> + * @params: buffer containing the request parameters
> + * @num_params: number of request parameters
> + * @data: buffer in which to return the received data
> + * @size: size of receive buffer
> + *
> + * This function will automatically choose the right data type depending on
> + * the number of parameters passed in.
> + *
> + * Return: The number of bytes successfully read or a negative error code on
> + * failure.
> + */
> +ssize_t mipi_dsi_generic_read(struct mipi_dsi_device *dsi, const void *params,
> +			      size_t num_params, void *data, size_t size)
> +{
> +	struct mipi_dsi_msg msg;
> +
> +	memset(&msg, 0, sizeof(msg));
> +	msg.channel = dsi->channel;
> +	msg.flags = MIPI_DSI_MSG_USE_LPM;
> +	msg.tx_len = num_params;
> +	msg.tx_buf = params;
> +	msg.rx_len = size;
> +	msg.rx_buf = data;
> +
> +	switch (num_params) {
> +	case 0:
> +		msg.type = MIPI_DSI_GENERIC_READ_REQUEST_0_PARAM;
> +		break;
> +
> +	case 1:
> +		msg.type = MIPI_DSI_GENERIC_READ_REQUEST_1_PARAM;
> +		break;
> +
> +	case 2:
> +		msg.type = MIPI_DSI_GENERIC_READ_REQUEST_2_PARAM;
> +		break;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return dsi->host->ops->transfer(dsi->host, &msg);
> +}
> +EXPORT_SYMBOL(mipi_dsi_generic_read);
> +
> +/**
>   * mipi_dsi_dcs_write_buffer() - transmit a DCS command with payload
>   * @dsi: DSI peripheral device
>   * @data: buffer containing data to be transmitted
> diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h
> index ef50b5d0de57..92ca66306702 100644
> --- a/include/drm/drm_mipi_dsi.h
> +++ b/include/drm/drm_mipi_dsi.h
> @@ -134,6 +134,12 @@ int mipi_dsi_attach(struct mipi_dsi_device *dsi);
>  int mipi_dsi_detach(struct mipi_dsi_device *dsi);
>  int mipi_dsi_set_maximum_return_packet_size(struct mipi_dsi_device *dsi,
>  					    u16 value);
> +
> +ssize_t mipi_dsi_generic_write(struct mipi_dsi_device *dsi, const void *payload,
> +			       size_t size);
> +ssize_t mipi_dsi_generic_read(struct mipi_dsi_device *dsi, const void *params,
> +			      size_t num_params, void *data, size_t size);
> +
>  ssize_t mipi_dsi_dcs_write_buffer(struct mipi_dsi_device *dsi,
>  				  const void *data, size_t len);
>  ssize_t mipi_dsi_dcs_write(struct mipi_dsi_device *dsi, u8 cmd,
>