From: Christophe Branchereau <cbranchereau@gmail.com> To: Linus Walleij <linus.walleij@linaro.org> Cc: thierry.reding@gmail.com, sam@ravnborg.org, airlied@gmail.com, daniel@ffwll.ch, robh+dt@kernel.org, krzysztof.kozlowski+dt@linaro.org, dri-devel@lists.freedesktop.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v2 1/2] drm/panel: add the orisetech ota5601a Date: Thu, 15 Dec 2022 20:22:01 +0100 [thread overview] Message-ID: <CAFsFa86mQt5hMAftcpqj77nraF82CjfXv=m_Zt5quTv7O8Jf9A@mail.gmail.com> (raw) In-Reply-To: <CACRpkdag1ZDyHRu5FqLWrsiqbmVuX2Wj5z67yhkg_=ooFqsboQ@mail.gmail.com> Hello Linus On Thu, Dec 15, 2022 at 9:42 AM Linus Walleij <linus.walleij@linaro.org> wrote: > > Hi Christophe, > > thanks for your patch! > > On Wed, Dec 14, 2022 at 12:01 PM Christophe Branchereau > <cbranchereau@gmail.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@gmail.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? It initiates a page shift > 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. Honestly I don't remember how I got the panel specsheet, thought it was widely available but I can't find it anymore online, so yes I guess at least documenting what each page does could be good for future reference > > +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) I can definitely do that if it improves clarity > > +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. I know that for the specific panel supported by this driver there are 2 modes, however in the future someone could want to add a panel using the same IC, which could have any number of modes > > > + 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. Yes > Overall the driver looks very good, just the above little details. Ok, will do the v3 next week, holidays etc :) > Yours, > Linus Walleij
WARNING: multiple messages have this Message-ID (diff)
From: Christophe Branchereau <cbranchereau@gmail.com> To: Linus Walleij <linus.walleij@linaro.org> Cc: devicetree@vger.kernel.org, sam@ravnborg.org, linux-kernel@vger.kernel.org, robh+dt@kernel.org, thierry.reding@gmail.com, dri-devel@lists.freedesktop.org, krzysztof.kozlowski+dt@linaro.org Subject: Re: [PATCH v2 1/2] drm/panel: add the orisetech ota5601a Date: Thu, 15 Dec 2022 20:22:01 +0100 [thread overview] Message-ID: <CAFsFa86mQt5hMAftcpqj77nraF82CjfXv=m_Zt5quTv7O8Jf9A@mail.gmail.com> (raw) In-Reply-To: <CACRpkdag1ZDyHRu5FqLWrsiqbmVuX2Wj5z67yhkg_=ooFqsboQ@mail.gmail.com> Hello Linus On Thu, Dec 15, 2022 at 9:42 AM Linus Walleij <linus.walleij@linaro.org> wrote: > > Hi Christophe, > > thanks for your patch! > > On Wed, Dec 14, 2022 at 12:01 PM Christophe Branchereau > <cbranchereau@gmail.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@gmail.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? It initiates a page shift > 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. Honestly I don't remember how I got the panel specsheet, thought it was widely available but I can't find it anymore online, so yes I guess at least documenting what each page does could be good for future reference > > +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) I can definitely do that if it improves clarity > > +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. I know that for the specific panel supported by this driver there are 2 modes, however in the future someone could want to add a panel using the same IC, which could have any number of modes > > > + 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. Yes > Overall the driver looks very good, just the above little details. Ok, will do the v3 next week, holidays etc :) > Yours, > Linus Walleij
next prev parent reply other threads:[~2022-12-15 19:22 UTC|newest] Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-12-14 11:00 [PATCH v2 0/2] Add support for the AUO A030JTN01 TFT LCD Christophe Branchereau 2022-12-14 11:00 ` [PATCH v2 1/2] drm/panel: add the orisetech ota5601a Christophe Branchereau 2022-12-15 8:41 ` Linus Walleij 2022-12-15 8:41 ` Linus Walleij 2022-12-15 19:22 ` Christophe Branchereau [this message] 2022-12-15 19:22 ` Christophe Branchereau 2022-12-14 11:00 ` [PATCH v2 2/2] dt-bindings: display/panel: Add the Focaltech gpt3 Christophe Branchereau 2022-12-14 11:32 ` Krzysztof Kozlowski 2022-12-14 11:31 ` [PATCH v2 0/2] Add support for the AUO A030JTN01 TFT LCD Christophe Branchereau
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to='CAFsFa86mQt5hMAftcpqj77nraF82CjfXv=m_Zt5quTv7O8Jf9A@mail.gmail.com' \ --to=cbranchereau@gmail.com \ --cc=airlied@gmail.com \ --cc=daniel@ffwll.ch \ --cc=devicetree@vger.kernel.org \ --cc=dri-devel@lists.freedesktop.org \ --cc=krzysztof.kozlowski+dt@linaro.org \ --cc=linus.walleij@linaro.org \ --cc=linux-kernel@vger.kernel.org \ --cc=robh+dt@kernel.org \ --cc=sam@ravnborg.org \ --cc=thierry.reding@gmail.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.