Bug in mipi_dbi_hw_reset() causes incorrect DT entries

Bug in mipi_dbi_hw_reset() causes incorrect DT entries

[Alex Lanzano]

Hi all,

There is a bug in the mipi_dbi_hw_reset() function that handles the
reset logic of the controller. Currently, it will set the reset gpio
value to 0, wait a specified time, then set the reset gpio value to 1.

The issue with this implementation is that the MIPI DBI spec states that
the reset signal is active low. So, in order to correct for this logic,
the developer needs to incorrectly define the reset gpio as active high
in the DT.

Fixing the logic in the driver would cause all the displays using this
driver downstream to stop working. To mitigate this, Josef and I were
thinking about adding an additional boolean property to the DT that when
present would use the correct reset logic in the driver. And if it's not
present use the current reset logic and print out a warning that this
reset logic is deprecated.

The overall plan would be to have this temporary fix for a few release
cycles so downstream has time to be aware of the issue and update their
DT. Eventually, we would remove the incorrect reset logic in the driver
and this addtional boolean property.

Let me know what you think of this approach and if there might be
something better we can do here.

Best regards,
Alex

Re: Bug in mipi_dbi_hw_reset() causes incorrect DT entries

[Krzysztof Kozlowski]

On 15/03/2025 16:20, Alex Lanzano wrote:
> Hi all,
> 
> There is a bug in the mipi_dbi_hw_reset() function that handles the
> reset logic of the controller. Currently, it will set the reset gpio
> value to 0, wait a specified time, then set the reset gpio value to 1.
> 
> The issue with this implementation is that the MIPI DBI spec states that
> the reset signal is active low. So, in order to correct for this logic,
> the developer needs to incorrectly define the reset gpio as active high
> in the DT.

You should address it to driver and subsystem maintainers. Instead you
skipped all maintainers but Cc-ed people who have nothing to do with
MIPI DBI drivers in the kernel.

> 
> Fixing the logic in the driver would cause all the displays using this
> driver downstream to stop working. To mitigate this, Josef and I were
> thinking about adding an additional boolean property to the DT that when
> present would use the correct reset logic in the driver. And if it's not
> present use the current reset logic and print out a warning that this
> reset logic is deprecated.
> 
> The overall plan would be to have this temporary fix for a few release
> cycles so downstream has time to be aware of the issue and update their
> DT. Eventually, we would remove the incorrect reset logic in the driver
> and this addtional boolean property.

That's an ABI now, so you cannot change it without affecting users. Code
is from 2018 so you have plenty of users now.

I did some workaround for wsa88xx speakers but that's an exception and
it should not necessarily be a pattern to follow.

Not sure if it the issue is worth changing considering the impact.


Best regards,
Krzysztof

Re: Bug in mipi_dbi_hw_reset() causes incorrect DT entries

[Alex Lanzano]

On Sun, Mar 16, 2025 at 12:51:29PM +0100, Krzysztof Kozlowski wrote:
> On 15/03/2025 16:20, Alex Lanzano wrote:
> > Hi all,
> > 
> > There is a bug in the mipi_dbi_hw_reset() function that handles the
> > reset logic of the controller. Currently, it will set the reset gpio
> > value to 0, wait a specified time, then set the reset gpio value to 1.
> > 
> > The issue with this implementation is that the MIPI DBI spec states that
> > the reset signal is active low. So, in order to correct for this logic,
> > the developer needs to incorrectly define the reset gpio as active high
> > in the DT.
> 
> You should address it to driver and subsystem maintainers. Instead you
> skipped all maintainers but Cc-ed people who have nothing to do with
> MIPI DBI drivers in the kernel.
> 

My apologies. I wanted to get the advice from you and the other device
tree maintainers since I was proposing a new device property to address
this issue. CC'ing them on the thread now.

> > 
> > Fixing the logic in the driver would cause all the displays using this
> > driver downstream to stop working. To mitigate this, Josef and I were
> > thinking about adding an additional boolean property to the DT that when
> > present would use the correct reset logic in the driver. And if it's not
> > present use the current reset logic and print out a warning that this
> > reset logic is deprecated.
> > 
> > The overall plan would be to have this temporary fix for a few release
> > cycles so downstream has time to be aware of the issue and update their
> > DT. Eventually, we would remove the incorrect reset logic in the driver
> > and this addtional boolean property.
> 
> That's an ABI now, so you cannot change it without affecting users. Code
> is from 2018 so you have plenty of users now.
> 
> I did some workaround for wsa88xx speakers but that's an exception and
> it should not necessarily be a pattern to follow.
> 
> Not sure if it the issue is worth changing considering the impact.
> 

Makes sense, I'm just a bit concerned about the device tree definitions
not being compatible across different platforms.

Best regards,
Alex