Re: [RESEND 3/4] regulator: mt6359: Add support for MT6359 regulator

From: Mark Brown <broonie@kernel.org>
To: Wen Su <Wen.Su@mediatek.com>
Cc: Mark Rutland <mark.rutland@arm.com>,
	devicetree@vger.kernel.org, wsd_upstream@mediatek.com,
	linux-kernel@vger.kernel.org, Liam Girdwood <lgirdwood@gmail.com>,
	Rob Herring <robh+dt@kernel.org>,
	linux-mediatek@lists.infradead.org,
	Matthias Brugger <matthias.bgg@gmail.com>,
	Lee Jones <lee.jones@linaro.org>,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [RESEND 3/4] regulator: mt6359: Add support for MT6359 regulator
Date: Wed, 22 Jan 2020 12:34:01 +0000	[thread overview]
Message-ID: <20200122123401.GD3833@sirena.org.uk> (raw)
In-Reply-To: <1579659806.6612.12.camel@mtkswgap22>

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

On Wed, Jan 22, 2020 at 10:23:26AM +0800, Wen Su wrote:
> On Mon, 2020-01-20 at 19:04 +0000, Mark Brown wrote:

> > This looks like you should be using regulator_list_voltage_table() and
> > associated functions, probably map_voltage_ascend() or _iterate() and
> > just a simple set_voltage_sel_regmap().

> Thanks for your suggestion.
> Currently it's using regulator_list_voltage_table() and
> regulator_map_voltage_iterate() as below:

> The reason to use mt6359_set_voltage_sel() is to convert selector value
> to hardware register index value:
> 	idx = pvol[selector];

The whole idea behind regulator_list_voltage_table() is that it does the
selector to voltage conversion for you, you shouldn't need to do any
additional mapping.

> To avoid using mt6359_set_voltage_sel(), the *_voltages array need to be
> filled with zeros as below: 
> Current:
> static const u32 vemc_voltages[] = {
> 	2900000, 3000000, 3300000,
> };
> static const u32 vemc_idx[] = {
> 	10, 11, 13,
> };

> change to:
> static const u32 vxo22_voltages[] = {
> 	0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 2900000, 3000000, 0, 3300000,
> };

That's fine, the table is small and it only needs to be iterated in
contexts where we're doing I2C I/O.  If it's really a problem introduce
generic helpers for this rather than open coding.

> > > +	switch (mode) {
> > > +	case REGULATOR_MODE_FAST:
> > > +		if (curr_mode == REGULATOR_MODE_IDLE) {
> > > +			WARN_ON(1);
> > > +			dev_notice(&rdev->dev,
> > > +				   "BUCK %s is LP mode, can't FPWM\n",
> > > +				   rdev->desc->name);
> > > +			return -EIO;

> > I'd expect the device to go out of low power mode then into force PWM
> > mode if it has to do that rather than reject the operation.

> The device low power mode may control by hardware pad, so that the
> reason to reject the operation is the device low power mode can not go
> out by software.

If this is being forced by hardware you need to check for that directly
rather than just rejecting it without even trying (but hopefully the
user set their constraints such that this doesn't happen).

> Another scenario is one user set the device to low power mode, we think
> it's not suitable to change device mode to _FAST mode by another user.

It's not your driver's problem to worry about arbitrating between higher
level users, let the framework deal with that (and notice that we have
no code in kernel that actually sets the mode directly at runtime).
That way we will have consistent behaviour between devices rather than
devices trying to enforce their own policies.

[-- 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