[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CACRpkdaTmHASz25uzDoeZBG2=e7XRLK67DENfAtCbaFp+AYnYA@mail.gmail.com>
Date: Thu, 20 Apr 2023 09:35:22 +0200
From: Linus Walleij <linus.walleij@...aro.org>
To: Artur Weber <aweber.kernel@...il.com>
Cc: thierry.reding@...il.com, sam@...nborg.org, airlied@...il.com,
daniel@...ll.ch, robh+dt@...nel.org,
krzysztof.kozlowski+dt@...aro.org, dri-devel@...ts.freedesktop.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
~postmarketos/upstreaming@...ts.sr.ht
Subject: Re: [PATCH v3 2/3] drm/panel: Add Samsung S6D7AA0 panel controller driver
Hi Artur,
thanks for your patch!
On Sun, Apr 16, 2023 at 3:16 PM Artur Weber <aweber.kernel@...il.com> wrote:
> Initial driver for S6D7AA0-controlled panels, currently only for the
> LSL080AL02 panel used in the Samsung Galaxy Tab 3 8.0 family of tablets.
>
> It should be possible to extend this driver to work with other panels
> using this IC.
>
> Signed-off-by: Artur Weber <aweber.kernel@...il.com>
> ---
> Changed in v2:
> - Removed unused panel_name property from desc struct
Overall this driver looks very good. I could merge it once the DT bindings
are ACKed by the DT maintainers and some minor stuff fixed.
Some comments below:
> +/* Manufacturer command set */
> +#define CMD_BL_CTL 0xc3
> +#define CMD_OTP_RELOAD 0xd0
> +#define CMD_PASSWD1 0xf0
> +#define CMD_PASSWD2 0xf1
> +#define CMD_PASSWD3 0xfc
Some drivers prefix these commands with "MCS" such as
MCS_BL_CTL.
MCS = Manufacturer Command Set (I think)
Some just name the identifers after the panel such as
s6d27a1 which has S6D27A1_RESCTL etc.
CMD seems a bit general to me and may be mistaken for
the actual DCS commands.
> +struct s6d7aa0 {
> + struct drm_panel panel;
> + struct mipi_dsi_device *dsi;
> + struct gpio_desc *reset_gpio;
> + struct regulator *enable_supply;
> + const struct s6d7aa0_panel_desc *desc;
> + bool prepared;
Skip this state variable, the core keeps track of whether the
panel is enabled or not.
> +static void s6d7aa0_reset(struct s6d7aa0 *ctx)
> +{
> + gpiod_set_value_cansleep(ctx->reset_gpio, 0);
> + msleep(50);
This first de-assertion is unnecessary isn't it?
The reset line will just be asserted longer if it is already asserted.
> + gpiod_set_value_cansleep(ctx->reset_gpio, 1);
> + msleep(50);
> + gpiod_set_value_cansleep(ctx->reset_gpio, 0);
> + msleep(50);
> +}
(...)
> +static int s6d7aa0_on(struct s6d7aa0 *ctx)
> +{
> + struct mipi_dsi_device *dsi = ctx->dsi;
> + struct device *dev = &dsi->dev;
> + int ret;
> +
> + dsi->mode_flags |= MIPI_DSI_MODE_LPM;
(...)
> +static int s6d7aa0_off(struct s6d7aa0 *ctx)
> +{
> + struct mipi_dsi_device *dsi = ctx->dsi;
> + struct device *dev = &dsi->dev;
> + int ret;
> +
> + dsi->mode_flags &= ~MIPI_DSI_MODE_LPM;
I haven't seen this mode flag MIPI_DSI_MODE_LPM set and
masked in other DSI panel drivers! Is this something we should
fix everywhere then? Or even something the core should be
doing?
Yours,
Linus Walleij
Powered by blists - more mailing lists