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
