[PATCH] drm/panel: ws2401: Add driver for WideChips WS2401

[Doug Anderson]

Hi,

On Thu, Jun 24, 2021 at 3:47 PM Linus Walleij <linus.walleij at linaro.org> wrote:
>
> @@ -5946,6 +5946,13 @@ S:       Maintained
>  T:     git git://anongit.freedesktop.org/drm/drm-misc
>  F:     drivers/gpu/drm/vboxvideo/
>
> +DRM DRIVER FOR WIDECHIPS WS2401 PANELS
> +M:     Linus Walleij <linus.walleij at linaro.org>
> +S:     Maintained
> +T:     git git://anongit.freedesktop.org/drm/drm-misc
> +F:     Documentation/devicetree/bindings/display/panel/samsung,lms380kf01.yaml
> +F:     drivers/gpu/drm/panel/panel-widechips-ws2401.c
> +
>  DRM DRIVER FOR VMWARE VIRTUAL GPU

nit: I assume this is supposed to be alphabetized? If so, [W]IDECHIPS
comes after [V]MWARE


> @@ -0,0 +1,404 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Panel driver for the WideChips WS2401 480x800 DPI RGB panel, used in
> + * the Samsung Mobile Display (SMD) LMS380KF01.
> + * Found in the Samsung Galaxy Ace 2 GT-I8160 mobile phone.
> + * Linus Walleij <linus.walleij at linaro.org>
> + * Inspired by code and know-how in the vendor driver by Gareth Phillips.
> + */
> +#include <drm/drm_modes.h>
> +#include <drm/drm_mipi_dbi.h>

nit: m[o]des sorts after m[i]pi


> +#define ws2401_command(ws, cmd, seq...) \
> +({ \
> +       struct mipi_dbi *dbi = &ws->dbi; \
> +       int ret; \
> +       ret = mipi_dbi_command(dbi, cmd, seq);  \
> +       if (ret) { \
> +               dev_err(ws->dev, "failure in writing command %02x\n", cmd); \
> +       } \

nit: don't need braces for the "if", right?

optional nit: use %#02x instead of %02x


> +})
> +
> +static void ws2401_read_mtp_id(struct ws2401 *ws)
> +{
> +       struct mipi_dbi *dbi = &ws->dbi;
> +       u8 id1, id2, id3;
> +       int ret;
> +
> +       ret = mipi_dbi_command_read(dbi, WS2401_READ_ID1, &id1);
> +       if (ret) {
> +               dev_err(ws->dev, "unable to read MTP ID 1\n");
> +               return;
> +       }
> +       ret = mipi_dbi_command_read(dbi, WS2401_READ_ID2, &id1);
> +       if (ret) {
> +               dev_err(ws->dev, "unable to read MTP ID 2\n");
> +               return;
> +       }
> +       ret = mipi_dbi_command_read(dbi, WS2401_READ_ID3, &id1);
> +       if (ret) {
> +               dev_err(ws->dev, "unable to read MTP ID 3\n");
> +               return;
> +       }
> +       dev_info(ws->dev, "MTP ID: %02x %02x %02x\n", id1, id2, id3);

Does this need to be printed every time you power on the panel? Seems
like it's going to spam up the logs... I'm not sure what it's used
for.


> +static int ws2401_power_off(struct ws2401 *ws)
> +{
> +       /* Disable backlight */
> +       if (ws->bl)
> +               ws2401_command(ws, WS2401_WRCTRLD, 0x00);

I don't have any real knowledge here, but the location of this seems a
little odd. Just based on inspection of the rest of the driver, I
almost would have thought it would need to be sent _before_ entering
sleep mode, but I certainly could be wrong.


> +static int ws2401_disable(struct drm_panel *panel)
> +{
> +       struct ws2401 *ws = to_ws2401(panel);
> +
> +       ws2401_command(ws, MIPI_DCS_SET_DISPLAY_OFF);
> +       msleep(25);

It feels weird / arbitrary the split between "disable" and "unprepare"
on this panel driver compared to the "db7430.c" one. In the other
driver you put the sleep mode here and in this driver you put the
sleep mode un "unpreapre". Is that for a reason, or just arbitrary?
Can it be consistent between the two drivers?

I guess maybe this is because in "db7430" the power up order was
slightly different?


> +static const struct backlight_ops ws2401_bl_ops = {
> +       .update_status = ws2401_set_brightness,
> +};
> +
> +const struct backlight_properties ws2401_bl_props = {

"static const" instead of "const"?


> +       ret = drm_panel_of_backlight(&ws->panel);
> +       if (ret) {
> +               dev_info(dev, "no external backlight, using internal backlight\n");
> +               ws->bl = devm_backlight_device_register(dev, "ws2401", dev, ws,
> +                                                       &ws2401_bl_ops, &ws2401_bl_props);
> +               if (IS_ERR(ws->bl)) {
> +                       ret = PTR_ERR(ws->bl);
> +                       return dev_err_probe(dev, ret,
> +                                            "failed to register backlight device\n");

nit: probably didn't need the separate assignment to "ret". Just pass
"PTR_ERR(ws->bl)" to the function. Then no need for braces for your
"if" too.

> +               }
> +               ws->panel.backlight = ws->bl;
> +       } else {
> +               dev_info(dev, "using external backlight\n");

This (and the other "no extenal backlight") feels a bit chatty to me.
If you really want them and want them at "info" level then I won't
object, but I guess I like short logs even with "info" enabled.

-Doug