| Message ID | 20230607151127.1542024-1-pavacic.p@gmail.com |
|---|---|
| Headers | show |
| Series | drm/panel: add fannal c3004 panel | expand |
Hi Paulo, thanks for your patch! Overall this looks very good. I doubt that the display controller is actually by Fannal, but I guess you tried to find out? We usually try to identify the underlying display controller so the driver can be named after it and reused for more display panels. Some minor questions/nitpicks below. On Wed, Jun 7, 2023 at 5:11 PM Paulo Pavacic <pavacic.p@gmail.com> wrote: > +static int fannal_panel_enable(struct drm_panel *panel) > +{ > + struct mipi_dsi_device *dsi = to_mipi_dsi_device(panel->dev); > + > + mipi_dsi_generic_write_seq(dsi, 0xFF, 0x77, 0x01, 0x00, 0x00, 0x13); > + mipi_dsi_generic_write_seq(dsi, 0xEF, 0x08); Why is everything using mipi_dsi_generic_write_seq() instead of mipi_dsi_dcs_write_seq()? Especially here, because 0x11 is certainly a command: > + mipi_dsi_generic_write_seq(dsi, 0x11); //MIPI_DCS_EXIT_SLEEP_MODE Instead use: ret = mipi_dsi_dcs_exit_sleep_mode(dsi); if (ret) return ret; > + mipi_dsi_generic_write_seq(dsi, 0x29); //MIPI_DCS_SET_DISPLAY_ON Instead use: ret = mipi_dsi_dcs_set_display_on(dsi); if (ret) return ret; Yours, Linus Walleij
Hello Linus, thank you for the comments. čet, 15. lip 2023. u 21:55 Linus Walleij <linus.walleij@linaro.org> napisao je: > > Hi Paulo, > > thanks for your patch! > > Overall this looks very good. > > I doubt that the display controller is actually by Fannal, but I guess > you tried to find out? We usually try to identify the underlying display > controller so the driver can be named after it and reused for more > display panels. Yes, of course, the controller is ST7701S. > > Some minor questions/nitpicks below. > > On Wed, Jun 7, 2023 at 5:11 PM Paulo Pavacic <pavacic.p@gmail.com> wrote: > > > +static int fannal_panel_enable(struct drm_panel *panel) > > +{ > > + struct mipi_dsi_device *dsi = to_mipi_dsi_device(panel->dev); > > + > > + mipi_dsi_generic_write_seq(dsi, 0xFF, 0x77, 0x01, 0x00, 0x00, 0x13); > > + mipi_dsi_generic_write_seq(dsi, 0xEF, 0x08); > > Why is everything using mipi_dsi_generic_write_seq() instead of > mipi_dsi_dcs_write_seq()? Okay, I will replace it. > > Especially here, because 0x11 is certainly a command: > > > + mipi_dsi_generic_write_seq(dsi, 0x11); //MIPI_DCS_EXIT_SLEEP_MODE > > Instead use: > > ret = mipi_dsi_dcs_exit_sleep_mode(dsi); > if (ret) > return ret; > > > > + mipi_dsi_generic_write_seq(dsi, 0x29); //MIPI_DCS_SET_DISPLAY_ON > > Instead use: > > ret = mipi_dsi_dcs_set_display_on(dsi); > if (ret) > return ret; > Okay I will replace all commands with a functions found here: https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/drm_mipi_dsi.c#L995 > Yours, > Linus Walleij Best regards, Paulo
On Fri, Jun 16, 2023 at 11:57 AM Paulo Pavacic <pavacic.p@gmail.com> wrote: > čet, 15. lip 2023. u 21:55 Linus Walleij <linus.walleij@linaro.org> napisao je: > > > > I doubt that the display controller is actually by Fannal, but I guess > > you tried to find out? We usually try to identify the underlying display > > controller so the driver can be named after it and reused for more > > display panels. > > Yes, of course, the controller is ST7701S. Hm did you try to just refactor drivers/gpu/drm/panel/panel-sitronix-st7701.c to support your new panel? One major reason would be that that driver knows what commands actually mean and have #defines for them. Yours, Linus Walleij
pet, 16. lip 2023. u 13:44 Linus Walleij <linus.walleij@linaro.org> napisao je: > > On Fri, Jun 16, 2023 at 11:57 AM Paulo Pavacic <pavacic.p@gmail.com> wrote: > > čet, 15. lip 2023. u 21:55 Linus Walleij <linus.walleij@linaro.org> napisao je: > > > > > > I doubt that the display controller is actually by Fannal, but I guess > > > you tried to find out? We usually try to identify the underlying display > > > controller so the driver can be named after it and reused for more > > > display panels. > > > > Yes, of course, the controller is ST7701S. > > Hm did you try to just refactor > drivers/gpu/drm/panel/panel-sitronix-st7701.c > to support your new panel? > Yes I have tried, but there are too many changes needed and I wasn't sure whether I would be breaking compatibility with st7701 based panels. > One major reason would be that that driver knows what > commands actually mean and have #defines for them. > > Yours, > Linus Walleij Best regards, Paulo
On Fri, Jun 16, 2023 at 2:31 PM Paulo Pavacic <pavacic.p@gmail.com> wrote: > pet, 16. lip 2023. u 13:44 Linus Walleij <linus.walleij@linaro.org> napisao je: > > > > On Fri, Jun 16, 2023 at 11:57 AM Paulo Pavacic <pavacic.p@gmail.com> wrote: > > > čet, 15. lip 2023. u 21:55 Linus Walleij <linus.walleij@linaro.org> napisao je: > > > > > > > > I doubt that the display controller is actually by Fannal, but I guess > > > > you tried to find out? We usually try to identify the underlying display > > > > controller so the driver can be named after it and reused for more > > > > display panels. > > > > > > Yes, of course, the controller is ST7701S. > > > > Hm did you try to just refactor > > drivers/gpu/drm/panel/panel-sitronix-st7701.c > > to support your new panel? > > Yes I have tried, but there are too many changes needed and I wasn't > sure whether I would be breaking compatibility with st7701 based > panels. For the community it creates a problem that now two drivers for similar hardware need to be maintained, and that burden will land on the DRM maintainers. For this reason it would be better if a joint driver could be created. I am sure the users of the old driver will be willing to test patches to make sure their devices keep working. Yours, Linus Walleij
pet, 16. lip 2023. u 14:53 Linus Walleij <linus.walleij@linaro.org> napisao je: > > On Fri, Jun 16, 2023 at 2:31 PM Paulo Pavacic <pavacic.p@gmail.com> wrote: > > pet, 16. lip 2023. u 13:44 Linus Walleij <linus.walleij@linaro.org> napisao je: > > > > > > On Fri, Jun 16, 2023 at 11:57 AM Paulo Pavacic <pavacic.p@gmail.com> wrote: > > > > čet, 15. lip 2023. u 21:55 Linus Walleij <linus.walleij@linaro.org> napisao je: > > > > > > > > > > I doubt that the display controller is actually by Fannal, but I guess > > > > > you tried to find out? We usually try to identify the underlying display > > > > > controller so the driver can be named after it and reused for more > > > > > display panels. > > > > > > > > Yes, of course, the controller is ST7701S. > > > > > > Hm did you try to just refactor > > > drivers/gpu/drm/panel/panel-sitronix-st7701.c > > > to support your new panel? > > > > Yes I have tried, but there are too many changes needed and I wasn't > > sure whether I would be breaking compatibility with st7701 based > > panels. > > For the community it creates a problem that now two drivers for similar > hardware need to be maintained, and that burden will land on the DRM > maintainers. For this reason it would be better if a joint driver could > be created. I will try modifying st7701, but that seems like a big task since currently st7701 crashes kernel (5.15) for me and I have seen suggestions to use raydium driver over st7701. Also I guess I should first read some more documentation and compare st7701 to st7701s. I currently can't reserve a lot of time for that. > > I am sure the users of the old driver will be willing to test patches to > make sure their devices keep working. A lot of modifications to st7701 are required. I believe it would result in a driver that doesn't look or work the same. e.g compare delays between initialization sequences of panel-fannal-c3004 and panel-st7701. I think it would be optimal to create st7701s driver and have special handling for st7701s panels. If there was a flag for whether panel is st7701 or st7701s it would end up looking like a mess. > > Yours, > Linus Walleij Thank you for your time, Paulo
On Wed, Jun 21, 2023 at 5:09 PM Paulo Pavacic <pavacic.p@gmail.com> wrote: > A lot of modifications to st7701 are required. I believe it would > result in a driver that doesn't look or work the same. e.g compare > delays between initialization sequences of panel-fannal-c3004 and > panel-st7701. I think it would be optimal to create st7701s driver and > have special handling for st7701s panels. If there was a flag for > whether panel is st7701 or st7701s it would end up looking like a > mess. What matters is if the original authors of the old st7701 driver are around and reviewing and testing patches at all. What we need is active maintainers. (Added Jagan, Marek & Maya). I buy the reasoning that the st7701s is perhaps substantially different from st7701. If st7701s is very different then I suppose it needs a separate driver, then all we need to to name the driver properly, i.e. panel-sitronix-st7701s.c. Yours, Linus Walleij
Hello Linus, čet, 22. lip 2023. u 10:22 Linus Walleij <linus.walleij@linaro.org> napisao je: > > On Wed, Jun 21, 2023 at 5:09 PM Paulo Pavacic <pavacic.p@gmail.com> wrote: > > > A lot of modifications to st7701 are required. I believe it would > > result in a driver that doesn't look or work the same. e.g compare > > delays between initialization sequences of panel-fannal-c3004 and > > panel-st7701. I think it would be optimal to create st7701s driver and > > have special handling for st7701s panels. If there was a flag for > > whether panel is st7701 or st7701s it would end up looking like a > > mess. > > What matters is if the original authors of the old st7701 driver are > around and reviewing and testing patches at all. What we need is > active maintainers. (Added Jagan, Marek & Maya). > > I buy the reasoning that the st7701s is perhaps substantially different > from st7701. > > If st7701s is very different then I suppose it needs a separate driver, > then all we need to to name the driver properly, i.e. > panel-sitronix-st7701s.c. I had in person talk with Paul Kocialkowski and I have concluded that this is the best solution. I believe I should rename it to st7701s due to the hardware changes. I would like to create V5 patch with driver renamed to st7701s. Please let me know if you agree / disagree. > > Yours, > Linus Walleij Thank you for your time, Paulo Pavačić
On 7/6/23 17:18, Paulo Pavacic wrote: > Hello Linus, > > čet, 22. lip 2023. u 10:22 Linus Walleij <linus.walleij@linaro.org> napisao je: >> >> On Wed, Jun 21, 2023 at 5:09 PM Paulo Pavacic <pavacic.p@gmail.com> wrote: >> >>> A lot of modifications to st7701 are required. I believe it would >>> result in a driver that doesn't look or work the same. e.g compare >>> delays between initialization sequences of panel-fannal-c3004 and >>> panel-st7701. I think it would be optimal to create st7701s driver and >>> have special handling for st7701s panels. If there was a flag for >>> whether panel is st7701 or st7701s it would end up looking like a >>> mess. >> >> What matters is if the original authors of the old st7701 driver are >> around and reviewing and testing patches at all. What we need is >> active maintainers. (Added Jagan, Marek & Maya). >> >> I buy the reasoning that the st7701s is perhaps substantially different >> from st7701. >> >> If st7701s is very different then I suppose it needs a separate driver, >> then all we need to to name the driver properly, i.e. >> panel-sitronix-st7701s.c. > > I had in person talk with Paul Kocialkowski and I have concluded that > this is the best solution. > I believe I should rename it to st7701s due to the hardware changes. I > would like to create V5 patch with driver renamed to st7701s. > Please let me know if you agree / disagree. If I recall it right, the ST7701 and ST7701S are basically the same chip, aren't they ?
Hello Marek, Jul 6, 2023 5:26:15 PM Marek Vasut <marex@denx.de>: > On 7/6/23 17:18, Paulo Pavacic wrote: >> Hello Linus, >> čet, 22. lip 2023. u 10:22 Linus Walleij <linus.walleij@linaro.org> napisao je: >>> >>> On Wed, Jun 21, 2023 at 5:09 PM Paulo Pavacic <pavacic.p@gmail.com> wrote: >>> >>>> A lot of modifications to st7701 are required. I believe it would >>>> result in a driver that doesn't look or work the same. e.g compare >>>> delays between initialization sequences of panel-fannal-c3004 and >>>> panel-st7701. I think it would be optimal to create st7701s driver and >>>> have special handling for st7701s panels. If there was a flag for >>>> whether panel is st7701 or st7701s it would end up looking like a >>>> mess. >>> >>> What matters is if the original authors of the old st7701 driver are >>> around and reviewing and testing patches at all. What we need is >>> active maintainers. (Added Jagan, Marek & Maya). >>> >>> I buy the reasoning that the st7701s is perhaps substantially different >>> from st7701. >>> >>> If st7701s is very different then I suppose it needs a separate driver, >>> then all we need to to name the driver properly, i.e. >>> panel-sitronix-st7701s.c. >> I had in person talk with Paul Kocialkowski and I have concluded that >> this is the best solution. >> I believe I should rename it to st7701s due to the hardware changes. I >> would like to create V5 patch with driver renamed to st7701s. >> Please let me know if you agree / disagree. > > If I recall it right, the ST7701 and ST7701S are basically the same chip, aren't they ? I'm currently exploring all the differences. There aren't a lot of them, but there are some. So far I can see that default register values are different, previously unused registers are now used and there has been some reordering of how info is placed in registers [1] (return value for some commands is different). E.g AJ1N[1:0] has been moved from B102h to B101h [1] Moreover, instructions to some commands have been changed as well as meaning of what data bits mean [2][3]. Also, new features have been added [2]; there is now PCLKS 3 for example. You can see few differences in following images: [1] https://ibb.co/NmgbZmy - GAMACTRL_st7701.png [2] https://ibb.co/G79y235 - PCLKS2.png P.S. this is second time I'm trying to send this e-mail so some of you might have received e-mail with the same text twice Thank you for your time, Paulo
On Thu, 22 Jun 2023 at 13:52, Linus Walleij <linus.walleij@linaro.org> wrote: > > On Wed, Jun 21, 2023 at 5:09 PM Paulo Pavacic <pavacic.p@gmail.com> wrote: > > > A lot of modifications to st7701 are required. I believe it would > > result in a driver that doesn't look or work the same. e.g compare > > delays between initialization sequences of panel-fannal-c3004 and > > panel-st7701. I think it would be optimal to create st7701s driver and > > have special handling for st7701s panels. If there was a flag for > > whether panel is st7701 or st7701s it would end up looking like a > > mess. > > What matters is if the original authors of the old st7701 driver are > around and reviewing and testing patches at all. What we need is > active maintainers. (Added Jagan, Marek & Maya). > > I buy the reasoning that the st7701s is perhaps substantially different > from st7701. > > If st7701s is very different then I suppose it needs a separate driver, > then all we need to to name the driver properly, i.e. > panel-sitronix-st7701s.c. I agree with what Linus mentioned. 1. If the panel is designed on top of ST7701 then add driver data on the existing panel-st7701 driver with this panel. 2. If the panel is designed on top of ST7701S - ST7701 and ST7701S are completely different in terms of the command set and init sequence then add panel-sitronix-st7701s.c 3. If the panel is designed on top ST7701S and if the commands set is the same as ST7701 but the init sequence is different then it is possible to use the existing st7701 driver with the init sequence as in driver data. Thanks, Jagan.
On 7/7/23 17:26, Paulo Pavacic wrote: > Hello Marek, Hi, > čet, 6. srp 2023. u 17:26 Marek Vasut <marex@denx.de> napisao je: >> >> On 7/6/23 17:18, Paulo Pavacic wrote: >>> Hello Linus, >>> >>> čet, 22. lip 2023. u 10:22 Linus Walleij <linus.walleij@linaro.org> napisao je: >>>> >>>> On Wed, Jun 21, 2023 at 5:09 PM Paulo Pavacic <pavacic.p@gmail.com> wrote: >>>> >>>>> A lot of modifications to st7701 are required. I believe it would >>>>> result in a driver that doesn't look or work the same. e.g compare >>>>> delays between initialization sequences of panel-fannal-c3004 and >>>>> panel-st7701. I think it would be optimal to create st7701s driver and >>>>> have special handling for st7701s panels. If there was a flag for >>>>> whether panel is st7701 or st7701s it would end up looking like a >>>>> mess. >>>> >>>> What matters is if the original authors of the old st7701 driver are >>>> around and reviewing and testing patches at all. What we need is >>>> active maintainers. (Added Jagan, Marek & Maya). >>>> >>>> I buy the reasoning that the st7701s is perhaps substantially different >>>> from st7701. >>>> >>>> If st7701s is very different then I suppose it needs a separate driver, >>>> then all we need to to name the driver properly, i.e. >>>> panel-sitronix-st7701s.c. >>> >>> I had in person talk with Paul Kocialkowski and I have concluded that >>> this is the best solution. >>> I believe I should rename it to st7701s due to the hardware changes. I >>> would like to create V5 patch with driver renamed to st7701s. >>> Please let me know if you agree / disagree. >> >> If I recall it right, the ST7701 and ST7701S are basically the same >> chip, aren't they ? > > I'm currently exploring all the differences. There aren't a lot of > differences, but there are some. > So far I can see that default register values are different, new > previously unused registers are now used and there has been some > reordering of how info is placed in registers [1] (data bits are in > different order). Moreover, instructions to some commands have been > changed and meaning of what data bits mean [2][3]. Also, new features > have been added [2]; there is now PCLKS 3 for example. > > You can see few differences in following images. Same images were > attached in this mail: > [1] https://ibb.co/NmgbZmy - GAMACTRL_st7701.png > [2] https://ibb.co/G79y235 - PCLKS2.png Ouch. I wonder if this is still something that can be abstracted out with some helper accessor functions like: if (model == ST7701) write something else write the other layout Or whether it makes sense to outright have a separate driver. The later would introduce duplication, but maybe that much duplication is OK.
Hello all, sub, 8. srp 2023. u 14:53 Marek Vasut <marex@denx.de> napisao je: > > On 7/7/23 17:26, Paulo Pavacic wrote: > > Hello Marek, > > Hi, > > > čet, 6. srp 2023. u 17:26 Marek Vasut <marex@denx.de> napisao je: > >> > >> On 7/6/23 17:18, Paulo Pavacic wrote: > >>> Hello Linus, > >>> > >>> čet, 22. lip 2023. u 10:22 Linus Walleij <linus.walleij@linaro.org> napisao je: > >>>> > >>>> On Wed, Jun 21, 2023 at 5:09 PM Paulo Pavacic <pavacic.p@gmail.com> wrote: > >>>> > >>>>> A lot of modifications to st7701 are required. I believe it would > >>>>> result in a driver that doesn't look or work the same. e.g compare > >>>>> delays between initialization sequences of panel-fannal-c3004 and > >>>>> panel-st7701. I think it would be optimal to create st7701s driver and > >>>>> have special handling for st7701s panels. If there was a flag for > >>>>> whether panel is st7701 or st7701s it would end up looking like a > >>>>> mess. > >>>> > >>>> What matters is if the original authors of the old st7701 driver are > >>>> around and reviewing and testing patches at all. What we need is > >>>> active maintainers. (Added Jagan, Marek & Maya). > >>>> > >>>> I buy the reasoning that the st7701s is perhaps substantially different > >>>> from st7701. > >>>> > >>>> If st7701s is very different then I suppose it needs a separate driver, > >>>> then all we need to to name the driver properly, i.e. > >>>> panel-sitronix-st7701s.c. > >>> > >>> I had in person talk with Paul Kocialkowski and I have concluded that > >>> this is the best solution. > >>> I believe I should rename it to st7701s due to the hardware changes. I > >>> would like to create V5 patch with driver renamed to st7701s. > >>> Please let me know if you agree / disagree. > >> > >> If I recall it right, the ST7701 and ST7701S are basically the same > >> chip, aren't they ? > > > > I'm currently exploring all the differences. There aren't a lot of > > differences, but there are some. > > So far I can see that default register values are different, new > > previously unused registers are now used and there has been some > > reordering of how info is placed in registers [1] (data bits are in > > different order). Moreover, instructions to some commands have been > > changed and meaning of what data bits mean [2][3]. Also, new features > > have been added [2]; there is now PCLKS 3 for example. > > > > You can see few differences in following images. Same images were > > attached in this mail: > > [1] https://ibb.co/NmgbZmy - GAMACTRL_st7701.png > > [2] https://ibb.co/G79y235 - PCLKS2.png > > Ouch. I wonder if this is still something that can be abstracted out > with some helper accessor functions like: > > if (model == ST7701) > write something > else > write the other layout > > Or whether it makes sense to outright have a separate driver. The later > would introduce duplication, but maybe that much duplication is OK. I would like to create new driver because panel-st7701 seems to be outdated and is using non-standard macro (ST7701_WRITE()) and for me it is crashing kernel 5.15. Does anyone have similar issues with it? Br, Paulo
On 7/12/23 14:07, Paulo Pavacic wrote: > Hello all, > > sub, 8. srp 2023. u 14:53 Marek Vasut <marex@denx.de> napisao je: >> >> On 7/7/23 17:26, Paulo Pavacic wrote: >>> Hello Marek, >> >> Hi, >> >>> čet, 6. srp 2023. u 17:26 Marek Vasut <marex@denx.de> napisao je: >>>> >>>> On 7/6/23 17:18, Paulo Pavacic wrote: >>>>> Hello Linus, >>>>> >>>>> čet, 22. lip 2023. u 10:22 Linus Walleij <linus.walleij@linaro.org> napisao je: >>>>>> >>>>>> On Wed, Jun 21, 2023 at 5:09 PM Paulo Pavacic <pavacic.p@gmail.com> wrote: >>>>>> >>>>>>> A lot of modifications to st7701 are required. I believe it would >>>>>>> result in a driver that doesn't look or work the same. e.g compare >>>>>>> delays between initialization sequences of panel-fannal-c3004 and >>>>>>> panel-st7701. I think it would be optimal to create st7701s driver and >>>>>>> have special handling for st7701s panels. If there was a flag for >>>>>>> whether panel is st7701 or st7701s it would end up looking like a >>>>>>> mess. >>>>>> >>>>>> What matters is if the original authors of the old st7701 driver are >>>>>> around and reviewing and testing patches at all. What we need is >>>>>> active maintainers. (Added Jagan, Marek & Maya). >>>>>> >>>>>> I buy the reasoning that the st7701s is perhaps substantially different >>>>>> from st7701. >>>>>> >>>>>> If st7701s is very different then I suppose it needs a separate driver, >>>>>> then all we need to to name the driver properly, i.e. >>>>>> panel-sitronix-st7701s.c. >>>>> >>>>> I had in person talk with Paul Kocialkowski and I have concluded that >>>>> this is the best solution. >>>>> I believe I should rename it to st7701s due to the hardware changes. I >>>>> would like to create V5 patch with driver renamed to st7701s. >>>>> Please let me know if you agree / disagree. >>>> >>>> If I recall it right, the ST7701 and ST7701S are basically the same >>>> chip, aren't they ? >>> >>> I'm currently exploring all the differences. There aren't a lot of >>> differences, but there are some. >>> So far I can see that default register values are different, new >>> previously unused registers are now used and there has been some >>> reordering of how info is placed in registers [1] (data bits are in >>> different order). Moreover, instructions to some commands have been >>> changed and meaning of what data bits mean [2][3]. Also, new features >>> have been added [2]; there is now PCLKS 3 for example. >>> >>> You can see few differences in following images. Same images were >>> attached in this mail: >>> [1] https://ibb.co/NmgbZmy - GAMACTRL_st7701.png >>> [2] https://ibb.co/G79y235 - PCLKS2.png >> >> Ouch. I wonder if this is still something that can be abstracted out >> with some helper accessor functions like: >> >> if (model == ST7701) >> write something >> else >> write the other layout >> >> Or whether it makes sense to outright have a separate driver. The later >> would introduce duplication, but maybe that much duplication is OK. > > I would like to create new driver because panel-st7701 seems to be > outdated and is using non-standard macro (ST7701_WRITE() There is no such macro: $ git grep ST7701_WRITE drivers/gpu/drm/panel/ | wc -l 0 There never was such a macro used in the driver either, are you sure you are not using some hacked up patched downstream fork of the driver ? $ git log -p next/master -- drivers/gpu/drm/panel/panel-sitronix-st7701.c | grep ST7701_WRITE | wc -l 0 > ) and for me > it is crashing kernel 5.15. Have you based all the aforementioned discussion and argumentation on year and half old Linux 5.15.y code base too ? If so, you are missing many patches: $ git log --oneline --no-merges v5.15..next/master -- drivers/gpu/drm/panel/panel-sitronix-st7701.c 5a2854e577dc2 drm: panel: Add orientation support for st7701 e89838968ee44 drm: panel: Add Elida KD50T048A to Sitronix ST7701 driver c62102165dd79 drm/panel/panel-sitronix-st7701: Remove panel on DSI attach failure 49ee766b364ed drm/panel/panel-sitronix-st7701: Clean up CMDnBKx selection c1cdee9b685a1 drm/panel/panel-sitronix-st7701: Fix RTNI calculation 57b2efce45ef5 drm/panel/panel-sitronix-st7701: Add Densitron DMT028VGHMCMI-1A TFT 42542c7904cf2 drm/panel/panel-sitronix-st7701: Split GIP and init sequences 83b7a8e7e88e7 drm/panel/panel-sitronix-st7701: Parametrize voltage and timing de2b4917843cd drm/panel/panel-sitronix-st7701: Infer horizontal pixel count from TFT mode 82f9cee25598a drm/panel/panel-sitronix-st7701: Adjust porch control bitfield name 1ba85119afb5e drm/panel/panel-sitronix-st7701: Infer vertical line count from TFT mode 779c84fea3dbd drm/panel/panel-sitronix-st7701: Make gamma correction TFT specific 7fa8e07128ed6 drm/panel/panel-sitronix-st7701: Make voltage supplies common to ST7701 a6c225be3da7e drm/panel/panel-sitronix-st7701: Enable DSI burst mode, LPM, non-continuous clock 6f481afe220d3 drm/panel/panel-sitronix-st7701: Make DSI mode flags common to ST7701 79abca2b39900 drm/mipi-dsi: Make remove callback return void > Does anyone have similar issues with it? No, I am using it in production.
Hi Marek, sri, 12. srp 2023. u 15:42 Marek Vasut <marex@denx.de> napisao je: > > On 7/12/23 14:07, Paulo Pavacic wrote: > > Hello all, > > > > sub, 8. srp 2023. u 14:53 Marek Vasut <marex@denx.de> napisao je: > >> > >> On 7/7/23 17:26, Paulo Pavacic wrote: > >>> Hello Marek, > >> > >> Hi, > >> > >>> čet, 6. srp 2023. u 17:26 Marek Vasut <marex@denx.de> napisao je: > >>>> > >>>> On 7/6/23 17:18, Paulo Pavacic wrote: > >>>>> Hello Linus, > >>>>> > >>>>> čet, 22. lip 2023. u 10:22 Linus Walleij <linus.walleij@linaro.org> napisao je: > >>>>>> > >>>>>> On Wed, Jun 21, 2023 at 5:09 PM Paulo Pavacic <pavacic.p@gmail.com> wrote: > >>>>>> > >>>>>>> A lot of modifications to st7701 are required. I believe it would > >>>>>>> result in a driver that doesn't look or work the same. e.g compare > >>>>>>> delays between initialization sequences of panel-fannal-c3004 and > >>>>>>> panel-st7701. I think it would be optimal to create st7701s driver and > >>>>>>> have special handling for st7701s panels. If there was a flag for > >>>>>>> whether panel is st7701 or st7701s it would end up looking like a > >>>>>>> mess. > >>>>>> > >>>>>> What matters is if the original authors of the old st7701 driver are > >>>>>> around and reviewing and testing patches at all. What we need is > >>>>>> active maintainers. (Added Jagan, Marek & Maya). > >>>>>> > >>>>>> I buy the reasoning that the st7701s is perhaps substantially different > >>>>>> from st7701. > >>>>>> > >>>>>> If st7701s is very different then I suppose it needs a separate driver, > >>>>>> then all we need to to name the driver properly, i.e. > >>>>>> panel-sitronix-st7701s.c. > >>>>> > >>>>> I had in person talk with Paul Kocialkowski and I have concluded that > >>>>> this is the best solution. > >>>>> I believe I should rename it to st7701s due to the hardware changes. I > >>>>> would like to create V5 patch with driver renamed to st7701s. > >>>>> Please let me know if you agree / disagree. > >>>> > >>>> If I recall it right, the ST7701 and ST7701S are basically the same > >>>> chip, aren't they ? > >>> > >>> I'm currently exploring all the differences. There aren't a lot of > >>> differences, but there are some. > >>> So far I can see that default register values are different, new > >>> previously unused registers are now used and there has been some > >>> reordering of how info is placed in registers [1] (data bits are in > >>> different order). Moreover, instructions to some commands have been > >>> changed and meaning of what data bits mean [2][3]. Also, new features > >>> have been added [2]; there is now PCLKS 3 for example. > >>> > >>> You can see few differences in following images. Same images were > >>> attached in this mail: > >>> [1] https://ibb.co/NmgbZmy - GAMACTRL_st7701.png > >>> [2] https://ibb.co/G79y235 - PCLKS2.png > >> > >> Ouch. I wonder if this is still something that can be abstracted out > >> with some helper accessor functions like: > >> > >> if (model == ST7701) > >> write something > >> else > >> write the other layout > >> > >> Or whether it makes sense to outright have a separate driver. The later > >> would introduce duplication, but maybe that much duplication is OK. > > > > I would like to create new driver because panel-st7701 seems to be > > outdated and is using non-standard macro (ST7701_WRITE() > > There is no such macro: > > $ git grep ST7701_WRITE drivers/gpu/drm/panel/ | wc -l > 0 > > There never was such a macro used in the driver either, are you sure you > are not using some hacked up patched downstream fork of the driver ? I meant ST7701_DSI() macro; It can be replaced with mipi_dsi_generic_write_seq from kernel 6.3. Sorry for the confusion. > > $ git log -p next/master -- > drivers/gpu/drm/panel/panel-sitronix-st7701.c | grep ST7701_WRITE | wc -l > 0 > > > ) and for me > > it is crashing kernel 5.15. > > Have you based all the aforementioned discussion and argumentation on > year and half old Linux 5.15.y code base too ? > > If so, you are missing many patches: > > $ git log --oneline --no-merges v5.15..next/master -- > drivers/gpu/drm/panel/panel-sitronix-st7701.c > 5a2854e577dc2 drm: panel: Add orientation support for st7701 > e89838968ee44 drm: panel: Add Elida KD50T048A to Sitronix ST7701 driver > c62102165dd79 drm/panel/panel-sitronix-st7701: Remove panel on DSI > attach failure > 49ee766b364ed drm/panel/panel-sitronix-st7701: Clean up CMDnBKx selection > c1cdee9b685a1 drm/panel/panel-sitronix-st7701: Fix RTNI calculation > 57b2efce45ef5 drm/panel/panel-sitronix-st7701: Add Densitron > DMT028VGHMCMI-1A TFT > 42542c7904cf2 drm/panel/panel-sitronix-st7701: Split GIP and init sequences > 83b7a8e7e88e7 drm/panel/panel-sitronix-st7701: Parametrize voltage and > timing > de2b4917843cd drm/panel/panel-sitronix-st7701: Infer horizontal pixel > count from TFT mode > 82f9cee25598a drm/panel/panel-sitronix-st7701: Adjust porch control > bitfield name > 1ba85119afb5e drm/panel/panel-sitronix-st7701: Infer vertical line count > from TFT mode > 779c84fea3dbd drm/panel/panel-sitronix-st7701: Make gamma correction TFT > specific > 7fa8e07128ed6 drm/panel/panel-sitronix-st7701: Make voltage supplies > common to ST7701 > a6c225be3da7e drm/panel/panel-sitronix-st7701: Enable DSI burst mode, > LPM, non-continuous clock > 6f481afe220d3 drm/panel/panel-sitronix-st7701: Make DSI mode flags > common to ST7701 > 79abca2b39900 drm/mipi-dsi: Make remove callback return void I will try backporting those patches to 5.15 and applying them to see whether it will then work with initialization sequences provided in this merge request just to be sure not to have duplication. We are still working on transitioning to newer kernel so for the time being I'm using mostly 5.15. On 5.15 kernel I have following kernel panic only with st7701 from the panel drivers I have tried: [ 20.255322] Kernel panic - not syncing: Asynchronous SError Interrupt [ 20.255326] CPU: 1 PID: 36 Comm: kworker/1:1 Tainted: G O 5.15.77-5.15.77-2.1.0 #1 [ 20.255330] Hardware name: XXX i.MX8XX board:XXX (DT) [ 20.255333] Workqueue: events fbcon_register_existing_fbs [ 20.255339] Call trace: [ 20.255340] dump_backtrace+0x0/0x19c [ 20.255349] show_stack+0x18/0x70 [ 20.255354] dump_stack_lvl+0x68/0x84 [ 20.255360] dump_stack+0x18/0x34 [ 20.255365] panic+0x15c/0x308 [ 20.255370] nmi_panic+0x8c/0x90 [ 20.255376] arm64_serror_panic+0x6c/0x7c [ 20.255382] do_serror+0x58/0x5c [ 20.255388] el1h_64_error_handler+0x30/0x50 [ 20.255395] el1h_64_error+0x78/0x7c [ 20.255399] _raw_spin_unlock_irq+0x18/0x50 [ 20.255404] sec_mipi_dsim_host_transfer+0x1a4/0x460 [ 20.255411] mipi_dsi_device_transfer+0x44/0x60 [ 20.255418] mipi_dsi_dcs_write_buffer+0x64/0xa0 [ 20.255424] st7701_init_sequence+0x2b0/0x47c [ 20.255431] st7701_prepare+0x54/0x70 [ 20.255436] drm_panel_prepare+0x28/0x40 [ 20.255431] st7701_prepare+0x54/0x70 [ 20.255436] drm_panel_prepare+0x28/0x40 [ 20.255441] sec_mipi_dsim_bridge_atomic_enable+0x308/0x520 [ 20.255446] drm_atomic_bridge_chain_enable+0x54/0xd0 [ 20.255453] drm_atomic_helper_commit_modeset_enables+0x13c/0x24c [ 20.255458] lcdif_drm_atomic_commit_tail+0x30/0x70 [ 20.255464] commit_tail+0xa0/0x180 [ 20.255468] drm_atomic_helper_commit+0x160/0x370 [ 20.255473] drm_atomic_commit+0x4c/0x60 [ 20.255477] drm_client_modeset_commit_atomic+0x1c8/0x260 [ 20.255485] drm_client_modeset_commit_locked+0x5c/0x1a0 [ 20.255491] drm_client_modeset_commit+0x30/0x60 [ 20.255496] drm_fb_helper_set_par+0xc8/0x120 [ 20.255504] fbcon_init+0x3c8/0x510 [ 20.255508] visual_init+0xb4/0x104 [ 20.255513] do_bind_con_driver.isra.0+0x1c4/0x394 [ 20.255518] do_take_over_console+0x144/0x1fc [ 20.255523] do_fbcon_takeover+0x6c/0xe0 [ 20.255530] fbcon_fb_registered+0xf4/0x140 [ 20.255534] fbcon_register_existing_fbs+0x48/0x64 [ 20.255539] process_one_work+0x1d0/0x354 [ 20.255545] worker_thread+0x13c/0x470 [ 20.255550] kthread+0x150/0x160 [ 20.255556] ret_from_fork+0x10/0x20 [ 20.255563] SMP: stopping secondary CPUs [ 20.255571] Kernel Offset: 0x80000 from 0xffff800008000000 [ 20.255574] PHYS_OFFSET: 0x40000000 [ 20.255563] SMP: stopping secondary CPUs [ 20.255571] Kernel Offset: 0x80000 from 0xffff800008000000 [ 20.255574] PHYS_OFFSET: 0x40000000 [ 20.255575] CPU features: 0x00002401,20000842 [ 20.255579] Memory Limit: none Raydium driver, for example, doesn't crash. It even almost works with few modifications (distorted screen). > > > Does anyone have similar issues with it? > > No, I am using it in production. Okay, then it might be something with my setup, I will check it out. Thank you for your time, Paulo
On 7/12/23 17:10, Paulo Pavacic wrote: Hi, [...] >>>> Or whether it makes sense to outright have a separate driver. The later >>>> would introduce duplication, but maybe that much duplication is OK. >>> >>> I would like to create new driver because panel-st7701 seems to be >>> outdated and is using non-standard macro (ST7701_WRITE() >> >> There is no such macro: >> >> $ git grep ST7701_WRITE drivers/gpu/drm/panel/ | wc -l >> 0 >> >> There never was such a macro used in the driver either, are you sure you >> are not using some hacked up patched downstream fork of the driver ? > > I meant ST7701_DSI() macro; It can be replaced with > mipi_dsi_generic_write_seq from kernel 6.3. Sorry for the confusion. OK >> $ git log -p next/master -- >> drivers/gpu/drm/panel/panel-sitronix-st7701.c | grep ST7701_WRITE | wc -l >> 0 >> >>> ) and for me >>> it is crashing kernel 5.15. >> >> Have you based all the aforementioned discussion and argumentation on >> year and half old Linux 5.15.y code base too ? >> >> If so, you are missing many patches: >> >> $ git log --oneline --no-merges v5.15..next/master -- >> drivers/gpu/drm/panel/panel-sitronix-st7701.c >> 5a2854e577dc2 drm: panel: Add orientation support for st7701 >> e89838968ee44 drm: panel: Add Elida KD50T048A to Sitronix ST7701 driver >> c62102165dd79 drm/panel/panel-sitronix-st7701: Remove panel on DSI >> attach failure >> 49ee766b364ed drm/panel/panel-sitronix-st7701: Clean up CMDnBKx selection >> c1cdee9b685a1 drm/panel/panel-sitronix-st7701: Fix RTNI calculation >> 57b2efce45ef5 drm/panel/panel-sitronix-st7701: Add Densitron >> DMT028VGHMCMI-1A TFT >> 42542c7904cf2 drm/panel/panel-sitronix-st7701: Split GIP and init sequences >> 83b7a8e7e88e7 drm/panel/panel-sitronix-st7701: Parametrize voltage and >> timing >> de2b4917843cd drm/panel/panel-sitronix-st7701: Infer horizontal pixel >> count from TFT mode >> 82f9cee25598a drm/panel/panel-sitronix-st7701: Adjust porch control >> bitfield name >> 1ba85119afb5e drm/panel/panel-sitronix-st7701: Infer vertical line count >> from TFT mode >> 779c84fea3dbd drm/panel/panel-sitronix-st7701: Make gamma correction TFT >> specific >> 7fa8e07128ed6 drm/panel/panel-sitronix-st7701: Make voltage supplies >> common to ST7701 >> a6c225be3da7e drm/panel/panel-sitronix-st7701: Enable DSI burst mode, >> LPM, non-continuous clock >> 6f481afe220d3 drm/panel/panel-sitronix-st7701: Make DSI mode flags >> common to ST7701 >> 79abca2b39900 drm/mipi-dsi: Make remove callback return void > > I will try backporting those patches to 5.15 and applying them to see > whether it will then work with initialization sequences provided in > this merge request just to be sure not to have duplication. We are > still working on transitioning to newer kernel so for the time being > I'm using mostly 5.15. > > On 5.15 kernel I have following kernel panic only with st7701 from the > panel drivers I have tried: > > [ 20.255322] Kernel panic - not syncing: Asynchronous SError Interrupt > [ 20.255326] CPU: 1 PID: 36 Comm: kworker/1:1 Tainted: G O > 5.15.77-5.15.77-2.1.0 #1 The latest 5.15.y is 5.15.120 , can you re-test on that version ? > [ 20.255330] Hardware name: XXX i.MX8XX board:XXX (DT) Is this some NXP downstream kernel fork with thousands of extra patches? The version string 2.1.0 looks very much like NXP versioning scheme ... [...]
Hi, > The latest 5.15.y is 5.15.120 , can you re-test on that version ? > unfortunately it seems that I won't be able to do testing with 5.15.120. I won't have access to hardware any more. > > [ 20.255330] Hardware name: XXX i.MX8XX board:XXX (DT) > > Is this some NXP downstream kernel fork with thousands of extra patches? > The version string 2.1.0 looks very much like NXP versioning scheme ... Yes, this is NXP fork of the kernel. > > [...] Best regards, Paulo