linux-kernel - Re: [PATCH v2 1/2] drm/panel: add the orisetech ota5601a

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CACRpkdag1ZDyHRu5FqLWrsiqbmVuX2Wj5z67yhkg_=ooFqsboQ@mail.gmail.com>
Date:   Thu, 15 Dec 2022 09:41:48 +0100
From:   Linus Walleij <linus.walleij@...aro.org>
To:     Christophe Branchereau <cbranchereau@...il.com>
Cc:     thierry.reding@...il.com, sam@...nborg.org, airlied@...il.com,
        daniel@...ll.ch, robh+dt@...nel.org,
        krzysztof.kozlowski+dt@...aro.org, dri-devel@...ts.freedesktop.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/2] drm/panel: add the orisetech ota5601a

Hi Christophe,

thanks for your patch!

On Wed, Dec 14, 2022 at 12:01 PM Christophe Branchereau
<cbranchereau@...il.com> wrote:

> Add the orisetech ota5601a ic driver
>
> For now it only supports the focaltech gpt3 3" 640x480 ips panel
> found in the ylm rg300x handheld.
>
> Signed-off-by: Christophe Branchereau <cbranchereau@...il.com>
(...)
> +config DRM_PANEL_ORISETECH_OTA5601A
> +       tristate "Orise Technology ota5601a RGB/SPI panel"
> +       depends on OF && SPI
> +       depends on BACKLIGHT_CLASS_DEVICE
> +       select REGMAP_SPI

Nice use of regmap in this driver.

> +static const struct reg_sequence ota5601a_panel_regs[] = {
> +       { 0xfd, 0x00 },
> +       { 0x02, 0x00 },
> +
> +       { 0x18, 0x00 },
> +       { 0x34, 0x20 },
> +
> +       { 0x0c, 0x01 },
> +       { 0x0d, 0x48 },
> +       { 0x0e, 0x48 },
> +       { 0x0f, 0x48 },
> +       { 0x07, 0x40 },
> +       { 0x08, 0x33 },
> +       { 0x09, 0x3a },
> +
> +       { 0x16, 0x01 },
> +       { 0x19, 0x8d },
> +       { 0x1a, 0x28 },
> +       { 0x1c, 0x00 },
> +
> +       { 0xfd, 0xc5 },
> +       { 0x82, 0x0c },
> +       { 0xa2, 0xb4 },
> +
> +       { 0xfd, 0xc4 },
> +       { 0x82, 0x45 },
> +
> +       { 0xfd, 0xc1 },
> +       { 0x91, 0x02 },
> +
> +       { 0xfd, 0xc0 },
> +       { 0xa1, 0x01 },
> +       { 0xa2, 0x1f },
> +       { 0xa3, 0x0b },
> +       { 0xa4, 0x38 },
> +       { 0xa5, 0x00 },
> +       { 0xa6, 0x0a },
> +       { 0xa7, 0x38 },
> +       { 0xa8, 0x00 },
> +       { 0xa9, 0x0a },
> +       { 0xaa, 0x37 },
> +
> +       { 0xfd, 0xce },
> +       { 0x81, 0x18 },
> +       { 0x82, 0x43 },
> +       { 0x83, 0x43 },
> +       { 0x91, 0x06 },
> +       { 0x93, 0x38 },
> +       { 0x94, 0x02 },
> +       { 0x95, 0x06 },
> +       { 0x97, 0x38 },
> +       { 0x98, 0x02 },
> +       { 0x99, 0x06 },
> +       { 0x9b, 0x38 },
> +       { 0x9c, 0x02 },
> +
> +       { 0xfd, 0x00 },
> +};

If these are registers, why is register 0xfd written 7 times with different
values?

This is rather a jam table or intializations sequence, so it should be
names something like that and a comment about undocumented
registers added probably as well.

> +static int ota5601a_enable(struct drm_panel *drm_panel)
> +{
> +       struct ota5601a *panel = to_ota5601a(drm_panel);
> +       int err;
> +
> +       err = regmap_write(panel->map, 0x01, 0x01);

Since you know what this register does, what about:

#include <linux/bits.h>

#define OTA5601A_CTL 0x01
#define OTA5601A_CTL_OFF 0x00
#define OTA5601A_CTL_ON BIT(0)

> +static int ota5601a_get_modes(struct drm_panel *drm_panel,
> +                            struct drm_connector *connector)
> +{
> +       struct ota5601a *panel = to_ota5601a(drm_panel);
> +       const struct ota5601a_panel_info *panel_info = panel->panel_info;
> +       struct drm_display_mode *mode;
> +       unsigned int i;
> +
> +       for (i = 0; i < panel_info->num_modes; i++) {
> +               mode = drm_mode_duplicate(connector->dev,
> +                                         &panel_info->display_modes[i]);
> +               if (!mode)
> +                       return -ENOMEM;
> +
> +               drm_mode_set_name(mode);
> +
> +               mode->type = DRM_MODE_TYPE_DRIVER;
> +               if (panel_info->num_modes == 1)

But ... you have just added an array that contains 2 elements, you
KNOW it will not be 1.

> +                       mode->type |= DRM_MODE_TYPE_PREFERRED;

I think you should probably set this on the preferred resolution
anyways, take the one you actually use.

> +static const struct of_device_id ota5601a_of_match[] = {
> +       { .compatible = "focaltech,gpt3", .data = &gpt3_info },
> +       { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, ota5601a_of_match);

Does this mean the display controller is ota5601a and the display
manufacturer and product is focaltech gpt3? Then it's fine.

Overall the driver looks very good, just the above little details.

Yours,
Linus Walleij