Re: [PATCH v3 2/3] drm/panel: Add Samsung S6D7AA0 panel controller driver

[Linus Walleij <linus.walleij@linaro.org>]

Hi Artur,

thanks for your patch!

On Sun, Apr 16, 2023 at 3:16 PM Artur Weber <aweber.kernel@gmail.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@gmail.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