drm: mipi_dbi_hw_reset() keeps display in reset

[Josef Luštický]

Ok, I'll implement the change and post it for a review.
About the property naming, I tend to name it something like
"inverted-reset-gpio-fixed" to denote that it is assumed the code
using the "reset-gpios" property was fixed.
What are your thoughts?

Best regards,
Josef

On Tue, Feb 25, 2025 at 2:46 PM Alex Lanzano <lanzano.alex at gmail.com> wrote:
>
> On Tue, Feb 25, 2025 at 12:59:59PM +0100, Josef Luštický wrote:
> > On Mon, Feb 24, 2025 at 12:13 AM Alex Lanzano <lanzano.alex at gmail.com> wrote:
> > >
> > > On Mon, Feb 17, 2025 at 12:39:01PM +0100, Josef Luštický wrote:
> > > > On Sat, Feb 15, 2025 at 8:14 PM Alex Lanzano <lanzano.alex at gmail.com> wrote:
> > > > >
> > > > > On Fri, Feb 14, 2025 at 08:04:41PM -0500, Alex Lanzano wrote:
> > > > > > On Fri, Feb 14, 2025 at 10:29:29AM +0100, Josef Luštický wrote:
> > > > > > > Hello Alex,
> > > > > > > there is a bug in mipi_dbi_hw_reset() function that implements the logic of
> > > > > > > display reset contrary.
> > > > > > > It keeps the reset line activated which keeps displays in reset state.
> > > > > > >
> > > > > > > I reported the bug to
> > > > > > > https://gitlab.freedesktop.org/drm/misc/kernel/-/issues/63
> > > > > > >
> > > > > > > Unfortunately, fixing the bug would mean current DTB-ABI breakage and
> > > > > > > device-trees modification would be needed.
> > > > > > > I mainly write this email to let you and other people know about it, so
> > > > > > > hopefully it can be found easier.
> > > > > > > What are your thoughts?
> > > > > > Thanks for making me aware. I'll dig into over the weekend and get back
> > > > > > to you
> > > > >
> > > > > Alright so I looked into a bit more. Looks like the MIPI Specification
> > > > > says that the reset line is active low. So, if we want dt entries to be
> > > > > correct the logic for setting the reset line in mipi_dbi_hw_reset()
> > > > > should be flipped. However, like you said, this is going to cause a some
> > > > > confused developers to break out their oscilloscopes to figure out
> > > > > why their display isn't working.
> > > > >
> > > > > Best regards,
> > > > > Alex
> > > >
> > > > Thank you Alex for looking into this.
> > > > I think all the supported dts can be changed together with
> > > > mipi_dbi_hw_reset(), however the fix would break existing DTBs and
> > > > third party DTSs.
> > > > So I think it shall be either noted somewhere that due to this bug,
> > > > the reset line needs to be "wrongly" ACTIVE_HIGH in DTS
> > > > or the mipi_dbi_hw_reset() is changed and the compatibility is broken,
> > > > which needs to be announced.
> > > >
> > > > BTW Zephyr fixed the code [1], but they introduced the MIPI DBI
> > > > support just a couple of weeks before the fix, so they avoided the
> > > > compatibility issue.
> > > > I was not able to find users mentioning issues related to the display
> > > > not functioning properly, so I had to dig into the code.
> > > > But afterwards I found a thread on Raspberry PI forums, where one of
> > > > the moderators mentions it [2].
> > > >
> > > > [1] https://github.com/zephyrproject-rtos/zephyr/issues/68562
> > > > [2] https://forums.raspberrypi.com/viewtopic.php?p=2165720#p2165720
> > >
> > > So, here are my thoughts on this after pondering it over for a bit.
> > > Ideally we should eventually reverse the reset logic so the DTS entry
> > > correctly specifies the hardware. However, instead of an abrupt change
> > > maybe we add an additional property to the DTS node that when present
> > > uses the correct reset logic. If the property isn't present we use the
> > > current incorrect reset logic and print out a dev_warn that it will soon
> > > be deprecated.
> > >
> > > Let me know what you think.
> > >
> > > Best regards,
> > > Alex
> > >
> > >
> > I think it's a good idea if the current logic is about to be fixed.
> > Another (probably not as good) idea is to introduce a new property
> > named "nreset-gpios = ..." or something like that, but I realise that
> > "reset-gpios" is the de-facto standard naming.
> >
> > Best regards,
> > Josef
> >
>
> Yeah I think it may be simpler to just add a boolean property like
> 'reverse-reset'. It would make the driver code simpler to implement too.
> Would you like to implement this change and submit the patch or would
> you like me to?
>
> Best regards,
> Alex