Hi,
On Mon, Jul 3, 2023 at 10:07 PM Cong Yang
<[email protected]> 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.
-Doug
Hi,
On Fri, Jul 7, 2023 at 3:32 AM Doug Anderson <[email protected]> wrote:
>
> Hi,
>
> On Mon, Jul 3, 2023 at 10:07 PM Cong Yang
> <[email protected]> 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.
Thanks Doug.
>
> -Doug
Hi,
On Thu, Jul 6, 2023 at 6:20 PM cong yang
<[email protected]> wrote:
>
> Hi,
>
> On Fri, Jul 7, 2023 at 3:32 AM Doug Anderson <[email protected]> wrote:
> >
> > Hi,
> >
> > On Mon, Jul 3, 2023 at 10:07 PM Cong Yang
> > <[email protected]> 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?
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
Hi,
On Fri, Jul 7, 2023 at 11:15 PM Doug Anderson <[email protected]> wrote:
>
> Hi,
>
> On Thu, Jul 6, 2023 at 6:20 PM cong yang
> <[email protected]> wrote:
> >
> > Hi,
> >
> > On Fri, Jul 7, 2023 at 3:32 AM Doug Anderson <[email protected]> wrote:
> > >
> > > Hi,
> > >
> > > On Mon, Jul 3, 2023 at 10:07 PM Cong Yang
> > > <[email protected]> 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?
Different boards use the same panel, and the delay should be the same,
Unless use different pmic on the boards.
It should be noted that my current situation is due to the replacement
of the pmic
Before this we used the TPS65132 PMIC, which can control the output timing of
AVDD/AVEE by setting the internal register and then store it in EEPROM.
After replacing it with PMIC AW37503, this power ic has no EEPROM,
and its AVDD/AVEE output timing is controlled by regulator gpio (ENN/ENP).
>
> 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?
>
Hmmm, sorry, I will let the vendor upload the timing of power off.
If the power off does not have this 1 ms, the panel IC may not be able
to enter sleep normally when system suspend.
> 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?
PMIC has "active discharge", but as explained above,
VSP/VSN are discharged at the same time. For the panel vendor
they want more is to discharge the VSP after the discharge of the VSN
is completed.
>
> 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?
I think the delay of power off does not affect the work of the power IC itself,
and some panel can accept VSP/VSN to be powered on or off at the same time.
So this part is just to meet the needs of the panel.
>
> 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.
Yes,I confirmed with the vendor, they think that if the VSN/VSP is
powered off at the same time,
the panel ic may not be able to enter sleep mode in system suspend.
>
> 2. Please provide the power sequence diagram for disable. If there's a
> 1 ms delay between regulators being disabled then please confirm.
Okay,I will push the vendor to update the power off timing on git hub.
>
> 3. If the 1 ms delay isn't truly needed then we can just drop this patch, right?
Yes.
>
> 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.
Bind the delay in the panel 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?
There is no problem with the ramp time of VSN/VSP, but the two rise or
fall at the same time.
>
>
> -Doug
Hi,
On Fri, Jul 7, 2023 at 11:15 PM Doug Anderson <[email protected]> wrote:
>
> Hi,
>
> On Thu, Jul 6, 2023 at 6:20 PM cong yang
> <[email protected]> wrote:
> >
> > Hi,
> >
> > On Fri, Jul 7, 2023 at 3:32 AM Doug Anderson <[email protected]> wrote:
> > >
> > > Hi,
> > >
> > > On Mon, Jul 3, 2023 at 10:07 PM Cong Yang
> > > <[email protected]> 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