Hi Alexey,
I had some gmail problems and replied to the very old driver
by Iskren, sorry for the mess.
I overall like this driver a lot. Some of Sam's comments could
be addressed especially for backlight.
I think the driver should indeed handle both the physical
displays like you do here.
On Sun, Jul 25, 2021 at 4:05 PM Alexey Minnekhanov
<alexeymin@xxxxxxxxxxxxxxxx> wrote:
> Samsung S6E3FA2 panel is amoled 1080x1920 command mode DSI
> panel used in Samsung Galaxy S5 phone. There are 2 known
> variations of panel that were shipped in this phone, and
> this driver handles both of them.
>
> Panel has built-in backlight (like all other AMOLED panels),
> controlled over DSI by some vendor specific commands, some
> of them include sending long byte sequences of what seems
> to be called "smart dimming".
>
> Signed-off-by: Alexey Minnekhanov <alexeymin@xxxxxxxxxxxxxxxx>
(...)
> +#define dsi_generic_write_seq(dsi, seq...) do { \
> + static const u8 d[] = { seq }; \
> + int ret; \
> + ret = mipi_dsi_generic_write(dsi, d, ARRAY_SIZE(d)); \
> + if (ret < 0) \
> + return ret; \
> + } while (0)
> +
> +#define dsi_dcs_write_seq(dsi, seq...) do { \
> + static const u8 d[] = { seq }; \
> + int ret; \
> + ret = mipi_dsi_dcs_write_buffer(dsi, d, ARRAY_SIZE(d)); \
> + if (ret < 0) \
> + return ret; \
> + } while (0)
These look generic as pointed out in other mail.
> +static int s6e3fa2_dsi_dcs_read1(struct mipi_dsi_device *dsi, const u8 cmd,
> + u8 *data)
> +{
> + int ret;
> +
> + ret = mipi_dsi_dcs_read(dsi, cmd, data, 1);
> + if (ret < 0) {
> + dev_err(&dsi->dev, "could not read DCS CMD %02x\n", cmd);
> + return ret;
> + }
> + return 0;
> +}
I don't think this needs a wrapper, just call mipi_dsi_dcs_read() directly.
> +/* Panel variants */
> +#define LCD_ID_S6E3FA2 0x602813
> +#define LCD_ID_EA8064G 0x622872
Interesting use of the "vendor" byte by Samsung here. It seems they are
repurposing the non-standard MTP bytes as they seem fit.
> +/*
> + * Which AID sequence to use for each candela level.
> + * This lookup table is same for both panels.
> + */
> +static const u8 map_candela_to_aid[S6E3FA2_NUM_GAMMA_LEVELS] = {
> + 0, 2, 3, 4, 6, 7, 8, 10, 11, 13, 14, 15,
> + 16, 17, 18, 20, 21, 22, 23, 24, 25, 26, 27, 28,
> + 29, 30, 31, 32, 33, 34, 35, 36, 36, 36, 36, 36,
> + 36, 36, 36, 36, 36, 36, 36, 36, 36, 36, 37, 38,
> + 39, 40, 41, 42, 43, 44, 44, 44, 44, 44, 44, 44
> +};
This and other things hints that we are dealing with the same display
controller.
> +/* Other panel drivers call these commands test_key_enable/disable */
> +static const u8 seq_s6e3fa2_test_key_en[6] = {
> + 0xf0, 0x5a, 0x5a,
> + 0xfc, 0x5a, 0x5a
> +};
0xf0 and 0xfc is obviously some "level 2 unlock" commands.
Maybe #define them as pointed out in other comments.
> +static const u8 seq_s6e3fa2_test_key_dis[6] = {
> + 0xf0, 0xa5, 0xa5,
> + 0xfc, 0xa5, 0xa5
> +};
> +static const u8 seq_ea8064g_test_key_en[6] = {
> + 0xf0, 0x5a, 0x5a,
> + 0xf1, 0x5a, 0x5a
> +};
> +static const u8 seq_ea8064g_test_key_dis[6] = {
> + 0xf1, 0xa5, 0xa5,
> + 0xf0, 0xa5, 0xa5
> +};
The use of two different registers for locking is suspicious, that
may point to different display controllers. :/
This is an icky panel, but it seems they are close enough to
be handled by the same driver IMO.
Yours,
Linus Walleij
