Re: drm: mipi_dbi_hw_reset() keeps display in reset

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Sat, Mar 15, 2025 at 11:43:54AM +0100, Noralf Trønnes wrote:
> 
> 
> On 15.03.2025 00:50, Alex Lanzano wrote:
> > On Fri, Mar 14, 2025 at 12:57:27PM +0100, Josef Luštický wrote:
> >> On Mon, Mar 10, 2025 at 7:33 PM Alex Lanzano <lanzano.alex@xxxxxxxxx> wrote:
> >>>
> >>> On Fri, Mar 07, 2025 at 10:25:18AM +0100, Josef Luštický wrote:
> >>>> 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?
> >>>>
> >>>
> >>> You probably wnat something more concise and in present tense like
> >>> 'invert-reset-gpio'
> >>
> >> OK, I understand.
> >> It still feels like the 'invert' would mean that the code is supposed
> >> to do something non-standard with the reset-gpios property
> >> specification.
> >> How about 'correct-reset-gpio' or 'proper-reset-gpio' to denote that
> >> the reset-gpio property describes the HW correctly.
> >>
> > 
> > My main concern here is that the device tree properties are supposed to
> > be completely independent of the driver code. So, I'd be hesitant to
> > imply that a property 'fixes' a specific behavior in the driver in the
> > name of the property itself (even though it does).
> > 
> 
> I suggest you ask on the devicetree ML, they probably know how to handle
> bugs like this.
> 

Will do! Thanks!

> Noralf.
> 
> > Best regards,
> > Alex
> > 
> >>>> On Tue, Feb 25, 2025 at 2:46 PM Alex Lanzano <lanzano.alex@xxxxxxxxx> 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@xxxxxxxxx> 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@xxxxxxxxx> 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
> 



[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux