Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <6803ab6e-d7ed-cce1-013f-221878b4be2c@gmail.com>
Date:   Sat, 22 Apr 2023 16:32:39 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.10.0
From:   Artur Weber <aweber.kernel@gmail.com>
Subject: Re: [PATCH v3 2/3] drm/panel: Add Samsung S6D7AA0 panel controller
 driver
To:     Linus Walleij <linus.walleij@linaro.org>
Cc:     thierry.reding@gmail.com, sam@ravnborg.org, airlied@gmail.com,
        daniel@ffwll.ch, robh+dt@kernel.org,
        krzysztof.kozlowski+dt@linaro.org, dri-devel@lists.freedesktop.org,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
        ~postmarketos/upstreaming@lists.sr.ht
References: <20230416131632.31673-1-aweber.kernel@gmail.com>
 <20230416131632.31673-3-aweber.kernel@gmail.com>
 <CACRpkdaTmHASz25uzDoeZBG2=e7XRLK67DENfAtCbaFp+AYnYA@mail.gmail.com>
Content-Language: en-US
In-Reply-To: <CACRpkdaTmHASz25uzDoeZBG2=e7XRLK67DENfAtCbaFp+AYnYA@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi,

thank you for the review.

On 20/04/2023 09:35, Linus Walleij wrote:
>> +static int s6d7aa0_on(struct s6d7aa0 *ctx)
>> +{
>> +       struct mipi_dsi_device *dsi = ctx->dsi;
>> +       struct device *dev = &dsi->dev;
>> +       int ret;
>> +
>> +       dsi->mode_flags |= MIPI_DSI_MODE_LPM;
> 
> (...)
> 
>> +static int s6d7aa0_off(struct s6d7aa0 *ctx)
>> +{
>> +       struct mipi_dsi_device *dsi = ctx->dsi;
>> +       struct device *dev = &dsi->dev;
>> +       int ret;
>> +
>> +       dsi->mode_flags &= ~MIPI_DSI_MODE_LPM;
> 
> I haven't seen this mode flag MIPI_DSI_MODE_LPM set and
> masked in other DSI panel drivers! Is this something we should
> fix everywhere then? Or even something the core should be
> doing?

These bits were included in a driver for a similar panel with the same
controller in an MSM8916 close-to-mainline kernel fork[1]; that driver
was generated with lmdpdg[2], which adds the LPM mode flag automatically
based on some downstream DTS property. In this case, I left it in, since
it didn't seem to break anything... but I just re-tested without it and
it seems that it might've fixed some odd issues I'd get sometimes when
going out of sleep mode. I'll get rid of it in the next version.

(I based my panel driver off that driver; now that I think about it, it
might be worth mentioning somewhere in the copyright notice...?)

Best regards
Artur Weber

[1]
https://github.com/msm8916-mainline/linux/blob/msm8916/6.3-rc7/drivers/gpu/drm/panel/msm8916-generated/panel-samsung-s6d7aa0-lsl080al03.c
[2]
https://github.com/msm8916-mainline/linux-mdss-dsi-panel-driver-generator