Re: [PATCH] drm/panel: Add avdd/avee delay for Starry-himax83102-j02 and Starry-ili9882t panel

[cong yang]

Hi,

On Fri, Jul 7, 2023 at 11:15 PM Doug Anderson <dianders@xxxxxxxxxx> wrote:
>
> Hi,
>
> On Thu, Jul 6, 2023 at 6:20 PM cong yang
> <yangcong5@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxx> wrote:
> >
> > Hi,
> >
> > On Fri, Jul 7, 2023 at 3:32 AM Doug Anderson <dianders@xxxxxxxxxx> wrote:
> > >
> > > Hi,
> > >
> > > On Mon, Jul 3, 2023 at 10:07 PM Cong Yang
> > > <yangcong5@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxx> wrote:
> > > >
> > > > From power on/off sequence for panel data sheet[1], T2 timing VSP to VSN
> > > > needs 1ms delay when power on, and VSN to VSP also needs 1ms delay when
> > > > power off. Some pmic may not be able to adjust the delay internally, so
> > > > let's add a delay between avdd/avee regulator gpio to meet the timing of
> > > > panel.
> > >
> > > Unless I'm mistaken, all of this is best handled via regulator
> > > constraints in the device tree. See the file:
> > >
> > > Documentation/devicetree/bindings/regulator/regulator.yaml
> > >
> > > Specifically, any delays related to actually ramping up / down the
> > > regulator can be specified in the device tree. Nominally, you could
> > > argue that the 1 ms delay actually _does_ belong in the driver, but
> > > IMO the 1 ms number there is really just there because someone thought
> > > it was weird to specify a delay of 0 ms. Given that you already need
> > > remp delays in the device tree, it feels OK to me to just include the
> > > 1 ms there.
> >
> > The regulator device tree has only the power on attribute
> > "regulator-enable-ramp-delay",
> > not has power off attribute. The regulator delay looks more like the
> > HW voltage requirement
> > of the power ic itself, and I just want to meet the panel spec
> > requirement. I add regulator-enable-ramp-delay
> > in dts he can also meet my requirement, but I have no way to control
> > the power off delays.
>
> Hmmm, I guess the fact that the delay needed can be different for
> different boards / PMICs still makes me think that the delay doesn't
> belong in the panel driver. Different boards using the same panel
> would need different delays, right?
>
> So, thinking more...
>
> You're saying that you _can_ specify the enable delay in the device
> tree, but not the disable one, right? However, the timing diagram you
> provided doesn't seem to show the "disable" part. Since that's the
> part we're talking about now, could you provide a more complete timing
> diagram? Can you also talk to the panel vendor and confirm that the "1
> ms" actually matters or if they just put that there to ensure
> ordering? In other words, is it simply important that VDD1 gets to
> ~90% before you turn on VSP, or do they truly need a full 1 ms delay?
>
> Can you provide any more details about the power IC you're using? Is
> it just a discrete PMIC with a GPIO enable, or is it something
> fancier? Correct me if I'm confused (entirely possible!), but I think
> some PMICs have a feature where they can turn on "active discharge" so
> that they ramp down more quickly when they're disabled. Any chance
> your PMIC has this?
>
> In general the fact that nobody has added
> "regulator-disable-ramp-delay" to the regulator framework already
> means that the problem you're facing isn't really a common problem.
> There are lots of devices out there that have more than one regulator
> but I don't see examples where drivers need to delay between turning
> all their regulators off. Are you positive that this is something that
> you really need to worry about?
>
> The above is a bit rambling (sorry!), but I guess the summary is:
>
> 1. Please confirm that the panel driver truly needs 1 ms between
> regulators enabled.
>
> 2. Please provide the power sequence diagram for disable. If there's a
> 1 ms delay between regulators being disabled then please confirm.
>
> 3. If the 1 ms delay isn't truly needed then we can just drop this patch, right?

https://github.com/ILITEK-LoganLin/Document/tree/main/ILITEK_Power_Sequence

Ask the vendor to evaluate this 1ms delay again, they think that
current ramp time
does not need 1ms delay, so drop this patch.

>
> 4. IMO if the panel itself truly requires 1 ms between regulators
> being enabled and/or disabled, it would be OK to put the 1 ms delay in
> the driver but it feels wrong to be accounting for ramp time in the
> driver. This should be specified in the device tree.
>
> 5. If we really need to account for the ramp down time, it would at
> least be good to submit a regulator framework patch proposing a way to
> specify this. We'd have to figure out how to make this work since I'd
> imagine that most regulator consumers don't care that much about ramp
> down time. Mark would be the real person to get advice from, but
> perhaps an API call like "regulator_wait_discharged(percent)" that a
> client could call?
>
>
> -Doug