From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753040AbbKXQCY (ORCPT ); Tue, 24 Nov 2015 11:02:24 -0500 Received: from mail-lf0-f46.google.com ([209.85.215.46]:33974 "EHLO mail-lf0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751518AbbKXQCW (ORCPT ); Tue, 24 Nov 2015 11:02:22 -0500 MIME-Version: 1.0 In-Reply-To: <20151124093356.GA6149@ulmo> References: <1446251908-2603-1-git-send-email-bjorn@kryo.se> <20151124093356.GA6149@ulmo> Date: Tue, 24 Nov 2015 08:02:20 -0800 Message-ID: Subject: Re: [PATCH 1/3] drm/dsi: Add support for Turn on/Shutdown peripheral packets From: Bjorn Andersson To: Thierry Reding Cc: David Airlie , "linux-kernel@vger.kernel.org" , "dri-devel@lists.freedesktop.org" Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Nov 24, 2015 at 1:33 AM, Thierry Reding wrote: > On Fri, Oct 30, 2015 at 05:38:26PM -0700, bjorn@kryo.se wrote: >> From: Werner Johansson >> >> 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 >> Signed-off-by: Bjorn Andersson >> --- >> 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