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 >
