Re: [PATCH 2/2] regulator: core: Ensure we are at least in bounds for our constraints

[Mark Brown]

On Wed, Mar 30, 2016 at 09:07:21AM +0000, Haibo Chen wrote:

Please fix your mail client to word wrap within paragraphs at something
substantially less than 80 columns.  Doing this makes your messages much
easier to read and reply to.

> In our sdhci.c, we call the function 
> regulator_set_voltage ---> regulator_set_voltage_unlocked(struct regulator *regulator, int min_uV, int max_uV)
> here, the parameter min_uV is 3300000, and the max_uV is 3400000
> 
> currently with your patch (the upper operator is &&), when insert a SD3.0 card, 
> it will do the sanity check, and return -EINVAL

> but when I change the upper operator from && to !=, 
> before the sanity check, it will first get the current_uV, and then go to out.
> 
> I'm not familiar with regulator common code. Hope the upper describe can help you debug this issue.

No, that's not helping clarify anything.  To repeat what I said to Geert 
this patch changes code that is only called when the regulator is
probed.  This means that the changed code is not running when the SDHCI
code is running.  You need to investigate what exactly is causing the
error.

Just randomly thrashing around with a separate bit of code with no
coharent explanation for what you think is happening is not helping
here, we need some analysis of what is going on.  The change you are
both proposing is guaranteed to break other boards since it means that
the case we supported originally where we set a specific voltage that is
specified by setting equal minimum and maximum constraints is no longer
going have the voltage applied.

Having taken another look I *suspect* that the SDHCI code is broken in
the way it enumerates the set of voltages that can be set and that this
will probably trigger in other situations where the set of voltages that
can be set is limited but I can't put my finger on it, someone with the
ability to run the code will need to investigate.

The following patch *might* help the SDHCI but I'm just guessing and
like I say it looks like this is flagging up a problem in the SDHCI
code.

diff --git a/drivers/regulator/of_regulator.c b/drivers/regulator/of_regulator.c
index f45106a44635..cd828dbf9d52 100644
--- a/drivers/regulator/of_regulator.c
+++ b/drivers/regulator/of_regulator.c
@@ -43,10 +43,12 @@ static void of_get_regulation_constraints(struct device_node *np,
 		constraints->max_uV = pval;
 
 	/* Voltage change possible? */
-	if (constraints->min_uV && constraints->max_uV) {
+	if (constraints->min_uV != constraints->max_uV)
 		constraints->valid_ops_mask |= REGULATOR_CHANGE_VOLTAGE;
+
+	/* Do we have a voltage range, if so try to apply it? */
+	if (constraints->min_uV && constraints->max_uV)
 		constraints->apply_uV = true;
-	}
 
 	if (!of_property_read_u32(np, "regulator-microvolt-offset", &pval))
 		constraints->uV_offset = pval;

> > -----Original Message-----
> > From: linux-mmc-owner@xxxxxxxxxxxxxxx [mailto:linux-mmc-
> > owner@xxxxxxxxxxxxxxx] On Behalf Of Mark Brown

Please don't top post, reply in line with needed context.  This allows
readers to readily follow the flow of conversation and understand what
you are talking about and also helps ensure that everything in the
discussion is being addressed.

Attachment: signature.asc
Description: PGP signature