Hi Jakob, thanks for your patch! On Thu, Oct 24, 2024 at 5:18 AM Jakob Hauser <jahau@xxxxxxxxxxxxxx> wrote: > This initial part of the panel driver was mostly generated by the > "linux-mdss-dsi-panel-driver-generator" tool [1], reading downstream > Android kernel file "dsi_panel_S6E88A0_AMS427AP24_qhd_octa_video.dtsi" [2]. > > On top of the generic output of the tool, there were a couple of changes > applied: > - Added mipi_dsi_dcs_set_display_on() to function s6e88a0_ams427ap24_on(), > otherwise the display does not show up. > - In functions s6e88a0_ams427ap24_on() and s6e88a0_ams427ap24_off() > changed DSI commands to multi context and used "accum_err" returns. > - In functions s6e88a0_ams427ap24_on() and s6e88a0_ams427ap24_off() replaced > msleep() by mipi_dsi_msleep(). > - The function s6e88a0_ams427ap24_get_modes() was changed to make use of > drm_connector_helper_get_modes_fixed(). This also required to include > drm/drm_probe_helper.h. > - In function s6e88a0_ams427ap24_probe() registring the regulators was changed > to devm_regulator_bulk_get_const(). This required to change supplies in struct > s6e88a0_ams427ap24 to a pointer. > - Removed bool "prepared" from struct s6e88a0_ams427ap24 and according parts in > functions s6e88a0_ams427ap24_prepare() and s6e88a0_ams427ap24_unprepare(). > > [1] https://github.com/msm8916-mainline/linux-mdss-dsi-panel-driver-generator > [2] https://github.com/msm8916-mainline/linux-downstream/blob/GT-I9195I/drivers/video/msm/mdss/samsung/S6E88A0_AMS427AP24/dsi_panel_S6E88A0_AMS427AP24_qhd_octa_video.dtsi > > Signed-off-by: Jakob Hauser <jahau@xxxxxxxxxxxxxx> Nice job on this driver so far! > +#include <linux/delay.h> > +#include <linux/gpio/consumer.h> > +#include <linux/module.h> > +#include <linux/of.h> Why do you need this include? .of_match_table is part of <linux/driver.h> > +static int s6e88a0_ams427ap24_on(struct s6e88a0_ams427ap24 *ctx) > +{ > + struct mipi_dsi_device *dsi = ctx->dsi; > + struct mipi_dsi_multi_context dsi_ctx = { .dsi = dsi }; > + > + dsi->mode_flags |= MIPI_DSI_MODE_LPM; > + > + mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xf0, 0x5a, 0x5a); Can we provide #defines for at least some of this magic? See other drivers for a very good idea of what some of them mean. panel-samsung-s6d27a1.c:#define S6D27A1_PASSWD_L2 0xF0 /* Password Command for Level 2 Control */ panel-samsung-s6d7aa0.c:#define MCS_PASSWD1 0xf0 > + mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xfc, 0x5a, 0x5a); panel-samsung-s6d7aa0.c:#define MCS_PASSWD3 0xfc > + mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xb0, 0x11); > + mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xfd, 0x11); > + mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xb0, 0x13); > + mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xfd, 0x18); > + mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xb0, 0x02); > + mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xb8, 0x30); (...) > + mipi_dsi_dcs_exit_sleep_mode_multi(&dsi_ctx); > + mipi_dsi_msleep(&dsi_ctx, 20); > + > + mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xf1, 0x5a, 0x5a); panel-samsung-s6d7aa0.c:#define MCS_PASSWD2 0xf1 > + mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xcc, 0x4c); > + mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xf2, 0x03, 0x0d); > + mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xf1, 0xa5, 0xa5); panel-samsung-s6d7aa0.c:#define MCS_PASSWD2 0xf1 Send in the reverse password: disable access. > + mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xca, > + 0x01, 0x00, 0x01, 0x00, 0x01, 0x00, 0x80, > + 0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80, > + 0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80, > + 0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80, > + 0x80, 0x80, 0x00, 0x00, 0x00); > + mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xb2, > + 0x40, 0x08, 0x20, 0x00, 0x08); > + mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xb6, 0x28, 0x0b); > + mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xf7, 0x03); > + mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0x55, 0x00); > + mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xf0, 0xa5, 0xa5); > + mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xfc, 0xa5, 0xa5); Send in the reverse password: disable access. A bit of #defines and comments would make it much more clear what is going on. Yours, Linus Walleij
