Re: [PATCH 1/2] regulator: mt6360: Add optional mediatek.power-off-sequence in bindings document

From: Mark Brown <broonie@kernel.org>
To: Rob Herring <robh@kernel.org>
Cc: cy_huang <u0084500@gmail.com>,
	lgirdwood@gmail.com, matthias.bgg@gmail.com,
	gene_chen@richtek.com, linux-kernel@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	linux-mediatek@lists.infradead.org, devicetree@vger.kernel.org,
	cy_huang@richtek.com, gene.chen.richtek@gmail.com
Subject: Re: [PATCH 1/2] regulator: mt6360: Add optional mediatek.power-off-sequence in bindings document
Date: Thu, 17 Jun 2021 17:24:28 +0100	[thread overview]
Message-ID: <20210617162428.GG5067@sirena.org.uk> (raw)
In-Reply-To: <20210611201643.GA1583875@robh.at.kernel.org>

[-- Attachment #1.1: Type: text/plain, Size: 1955 bytes --]

On Fri, Jun 11, 2021 at 02:16:43PM -0600, Rob Herring wrote:
> On Wed, Jun 02, 2021 at 02:54:34PM +0800, cy_huang wrote:

> > Originally, we think it must write in platform dependent code like as bootloader.
> > But after the evaluation, it must write only when system normal HALT or POWER_OFF.
> > For the other cases, just follow HW immediate off by default.

> Wouldn't this be handled by PSCI implementation?

Ideally I think...

> > +  mediatek,power-off-sequence:
> > +    description: |
> > +      Power off sequence time selection for BUCK1/BUCK2/LDO7/LDO6, respetively.
> > +      Cause these regulators are all default-on power. Each value from 0 to 63,
> > +      and step is 1. Each step means 2 millisecond delay.
> > +      Therefore, the power off sequence delay time range is from 0ms to 126ms.
> > +    $ref: "/schemas/types.yaml#/definitions/uint8-array"
> > +    minItems: 4
> > +    maxItems: 4

> So this is the delay between BUCK1 and BUCK2, then BUCK2 to LDO7, etcc? 
> If we wanted to express this in DT, we'd made this generic which would 
> need to be more flexible. A poweroff delay in each regulator (similar to 
> the existing power on delay) would be sufficient for what you need I 
> think.

It's not exactly a delay that's being described there - it's a series of
timeslots, each regulator getting assigned to a timeslot.  You could
possibly do a general binding by specifying a delay from the start of
the power off sequence and then (for this device) having the driver work
out a mapping of those times to timeslots.  That feels genericish, you
might also have things like mode changes but it'd cover a lot of the
cases.

On the other hand this is the sort of thing that is often just not
configurable and where people often make weird and inflexible hardware
so things that do implement it are likely to end up wanting to add a
bunch of constraints which might be a lot of hassle.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel