Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1276215 > unrolled thread

Re: [PATCH 1/3] drm/dsi: Add support for Turn on/Shutdown peripheral packets

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 1/3] drm/dsi: Add support for Turn on/Shutdown peripheral  packets Thierry Reding <thierry.reding@gmail.com> - 2015-11-24 10:40 +0100
    Re: [PATCH 1/3] drm/dsi: Add support for Turn on/Shutdown peripheral packets Bjorn Andersson <bjorn@kryo.se> - 2015-11-24 17:10 +0100

#1276215 — Re: [PATCH 1/3] drm/dsi: Add support for Turn on/Shutdown peripheral packets

[Multipart message — attachments visible in raw view] — view raw

On Fri, Oct 30, 2015 at 05:38:26PM -0700, bjorn@kryo.se wrote:
> From: Werner Johansson <werner.johansson@sonymobile.com>
> 
> 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@sonymobile.com>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
> ---
>  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

[toc] | [next] | [standalone]

#1276597 — Re: [PATCH 1/3] drm/dsi: Add support for Turn on/Shutdown peripheral packets

On Tue, Nov 24, 2015 at 1:33 AM, Thierry Reding
<thierry.reding@gmail.com> wrote:
> On Fri, Oct 30, 2015 at 05:38:26PM -0700, bjorn@kryo.se wrote:
>> From: Werner Johansson <werner.johansson@sonymobile.com>
>>
>> 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@sonymobile.com>
>> Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
>> ---
>>  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...
>

Sorry for not including you as a recipient, I relied too heavily on
get_maintainer. Thanks for fixing my misstake.

> 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).
>

Sounds good, thanks.

> 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.
>

Right. Thanks for fixing it!

Regards,
Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web