Hi Artur,
thanks for your patch!
On Sun, Apr 16, 2023 at 3:16 PM Artur Weber <aweber.kernel@xxxxxxxxx> 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@xxxxxxxxx>
> ---
> 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
