Re: [PATCH v2 06/11] drm/panel: s6e3ha8: Correct the polarity logic within

[David Heidelberg <david@ixit.cz>]

On 29/09/2026 12:36, Krzysztof Kozlowski wrote:
> On 29/09/2026 11:59, David Heidelberg wrote:
>> On 29/09/2026 11:51, Krzysztof Kozlowski wrote:
>>> On 29/09/2026 11:41, David Heidelberg wrote:
>>>> On 29/09/2026 09:45, Krzysztof Kozlowski wrote:
>>>>> On Thu, Sep 24, 2026 at 04:01:34PM +0200, David Heidelberg wrote:
>>>>>> The reset was introduced with wrong polarity. Correct for the future
>>>>>> compatibles and keep current with reverted logic.
>>>>>>
>>>>>> Old DTs keep GPIO_ACTIVE_HIGH and are fixed up via
>>>>>> gpiod_toggle_active_low() on the deprecated compatible.
>>>>>>
>>>>>> Assisted-by: LLM
>>>>>> Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>
>>>>>> Signed-off-by: David Heidelberg <david@ixit.cz>
>>>>>> ---
>>>>>>     drivers/gpu/drm/panel/panel-samsung-s6e3ha8.c | 23 +++++++++++++++++++----
>>>>>>     1 file changed, 19 insertions(+), 4 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/panel/panel-samsung-s6e3ha8.c b/drivers/gpu/drm/panel/panel-samsung-s6e3ha8.c
>>>>>> index 5e1e997b83b36..99290913de69a 100644
>>>>>> --- a/drivers/gpu/drm/panel/panel-samsung-s6e3ha8.c
>>>>>> +++ b/drivers/gpu/drm/panel/panel-samsung-s6e3ha8.c
>>>>>> @@ -20,16 +20,17 @@
>>>>>>     #include "panel-samsung-dsi.h"
>>>>>>     
>>>>>>     struct s6e3ha8_desc {
>>>>>>     	const struct drm_panel_funcs *funcs;
>>>>>>     	const struct drm_display_mode *mode;
>>>>>>     	unsigned long mode_flags;
>>>>>>     	const struct regulator_bulk_data *supplies;
>>>>>>     	unsigned int num_supplies;
>>>>>> +	bool broken_reset_polarity;
>>>>>>     };
>>>>>>     
>>>>>>     struct s6e3ha8 {
>>>>>>     	struct drm_panel panel;
>>>>>>     	struct mipi_dsi_device *dsi;
>>>>>>     	const struct s6e3ha8_desc *desc;
>>>>>>     	struct drm_dsc_config dsc;
>>>>>>     	struct gpio_desc *reset_gpio;
>>>>>> @@ -62,22 +63,22 @@ static int s6e3ha8_unprepare(struct drm_panel *panel)
>>>>>>     {
>>>>>>     	struct s6e3ha8 *priv = to_s6e3ha8(panel);
>>>>>>     
>>>>>>     	return regulator_bulk_disable(priv->desc->num_supplies, priv->supplies);
>>>>>>     }
>>>>>>     
>>>>>>     static void s6e3ha8_amb577px01_wqhd_reset(struct s6e3ha8 *priv)
>>>>>>     {
>>>>>> -	gpiod_set_value_cansleep(priv->reset_gpio, 1);
>>>>>> -	usleep_range(5000, 6000);
>>>>>>     	gpiod_set_value_cansleep(priv->reset_gpio, 0);
>>>>>>     	usleep_range(5000, 6000);
>>>>>>     	gpiod_set_value_cansleep(priv->reset_gpio, 1);
>>>>>>     	usleep_range(5000, 6000);
>>>>>> +	gpiod_set_value_cansleep(priv->reset_gpio, 0);
>>>>>
>>>>> This breaks all users and this usage of ABI was already released.
>>>>
>>>> See the gpiod_toggle_active_low() usage later in the patch which keep the logic
>>>> for the original compatible as intended.
>>>>
>>>
>>> OK, I went way too fast, that's correct part. But splitting fix is still
>>> just confusing. Backporting to stable is a different thing than fixing
>>> issues.
>>
>> Sure, I already droped the previous commit changing it for stable.
>>
>> Btw. looking at gpiod_toggle_active_low(), would it make sense to do a series
>> correcting panel reset logic? I see many panels keep "reset asserted" in the
>> driver (but ofc not in the reality).
> 
> 
> To my knowledge it is impossible task to do, without breaking something.

I would do the partial change (only driver, not full DT).

So we end up with drivers having the correct logic with extra 2-line DT quirk 
using gpiod_toggle_active_low() for given compatible.

This could solve people implementing new panel on top of existing DDIC and allow 
them to use the right polarity for new compatible.

David

> Either you break users of ABI (so the DTS) or break existing users of
> DTS. One could try to avoid both by using your approach here with
> compatibles having fallback. But then what polarity actually would be in
> such DTS node? If you know your users, like for some SoC components, you
> could argue that none of then will be affected. But both the driver and
> DTS here can be used externally.
> 
> Best regards,
> Krzysztof