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

[Krzysztof Kozlowski <krzk@kernel.org>]

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.
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