On Fri, Oct 30, 2015 at 05:38:26PM -0700, bjorn@xxxxxxx wrote:
> From: Werner Johansson <werner.johansson@xxxxxxxxxxxxxx>
>
> The MIPI_DSI_TURN_ON_PERIPHERAL and MIPI_DSI_SHUTDOWN_PERIPHERAL
> packets are required for some panels, one example being the
> Panasonic VVX10F034N00 panel.
>
> Signed-off-by: Werner Johansson <werner.johansson@xxxxxxxxxxxxxx>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@xxxxxxxxxxxxxx>
> ---
> drivers/gpu/drm/drm_mipi_dsi.c | 47 ++++++++++++++++++++++++++++++++++++++++++
> include/drm/drm_mipi_dsi.h | 3 +++
> 2 files changed, 50 insertions(+)
This being a dependency on 3/3 it would've been good to send this To: me
as well. I hadn't seen this in my inbox and had simply discarded it as
being independent. Of course if I had properly checked the build I
would've noticed...
Anyway, I've applied this with slight modifications. I removed the raw
short write helper. I don't think it buys us much and we already have an
open-coded variant in mipi_dsi_set_maximum_return_packet_size(). I've
also moved these functions closer to the Set Maximum Return Packet Size
command implementation because they are part of the same command set. I
also made the functions return int instead of ssize_t to avoid potential
confusion about their return value (ssize_t implies "number of bytes
transferred, or a negative error code on failure).
One minor comment below...
> diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
> index 2d5ca8ee..13b4a9c 100644
> --- a/drivers/gpu/drm/drm_mipi_dsi.c
> +++ b/drivers/gpu/drm/drm_mipi_dsi.c
> @@ -862,6 +862,53 @@ int mipi_dsi_dcs_set_pixel_format(struct mipi_dsi_device *dsi, u8 format)
> }
> EXPORT_SYMBOL(mipi_dsi_dcs_set_pixel_format);
>
> +/**
> + * mipi_dsi_raw_short_write() - Sends a data-less short DSI packet
> + * @dsi: DSI peripheral device
> + * @type: Data Type of packet to send
> + *
> + * Return: 0 on success or a negative error code on failure.
> + */
> +static ssize_t mipi_dsi_raw_short_write(struct mipi_dsi_device *dsi, u8 type)
> +{
> + u8 dummy[2] = { 0, 0 };
> + struct mipi_dsi_msg msg = {
> + .channel = dsi->channel,
> + .tx_buf = dummy,
> + .tx_len = sizeof(dummy),
> + .type = type
> + };
> +
> + if (mipi_dsi_packet_format_is_short(type))
> + return mipi_dsi_device_transfer(dsi, &msg);
> + else
> + return -1;
> +}
When the kerneldoc comment says "negative error code", developers will
expect one of the -E* constants. -1 maps to -EPERM (operation not
permitted), which isn't a good error code for this case. This is also an
internal function, so these errors really should never happen.
Anyway, since I removed this helper this isn't an issue anymore, just
wanted to mention it.
Thanks,
Thierry
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
