Re: [PATCH 3/4] mfd: s2mps11: Add device tree support

[Yadwinder Singh Brar]

On Wed, Jul 3, 2013 at 11:35 PM, Mark Brown <broonie@xxxxxxxxxx> wrote:
> On Mon, Jun 24, 2013 at 04:50:57PM +0530, Yadwinder Singh Brar wrote:
>
>> + regulator-ramp-delay for BUCKs = [6250/12500/25000(default)/50000] uV/us
>
>> + Optional property for BUCK[2/3/4/6] only:
>> + regulator-ramp-disable: boolean, disables ramp delay on hardware.
>
> If these were device specific bindings they ought to be namespaced as
> samsung, or similar.  However I think they are generic enough that they
> could just be added to the standard regulator bindings, ramp rates are
> common enough and units of time are a simple way to express them.  I'd
> also suggest just using a ramp delay of zero for no ramp, it's simpler
> and will probably fall naturally out of the bindings.
>

Yes, I was also expecting to see regulator-ramp-disable as generic binding.
Before preparing this patch I thought of using regulator-ramp-delay = 0,
but I found it inappropriate because :
-  Currently ramp-delay (= 0) if not defined in DT, leaves the
hardware with default
ramp settings, and uses default ramp-rate defined in rdev->desc . So we need
an extra mechanism(either special(-ve) value of ramp-delay or an extra flag
(ramp_disable) in regulator constraints) to figure out whether ramp-rate is
actually set to zero or its left (by default) zero.
- As ramp time is inversely propositional to ramp-rate(i.e. ramp-delay ,
its wrongly named, my mistake :( ) so it may look weired to use ramp-rate =0
as no ramp(ramp_time = 0).

> Please just send a separate patch adding these to the generic regulator
> binding document instead of this one, we can add generic code/ops for
> this later.

I had below idea in my mind, I dropped it because I thought it may be
to early to put common code as no other driver in mainline appeared
with this requirement yet but probably may come.

8<-----------------------------------------------------------------------

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 815d6df..cf73e7c 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -2424,7 +2424,8 @@ static int _regulator_do_set_voltage(struct regulator_dev

        /* Call set_voltage_time_sel if successfully obtained old_selector */
        if (ret == 0 && _regulator_is_enabled(rdev) && old_selector >= 0 &&
-           old_selector != selector && rdev->desc->ops->set_voltage_time_sel)
+           old_selector != selector && rdev->desc->ops->set_voltage_time_sel
+           && !rdev->constraints->ramp_disable) {

                delay = rdev->desc->ops->set_voltage_time_sel(rdev,
                                                old_selector, selector);
diff --git a/drivers/regulator/of_regulator.c b/drivers/regulator/of_regulator.
index 66ca769..c579aca 100644
--- a/drivers/regulator/of_regulator.c
+++ b/drivers/regulator/of_regulator.c
@@ -64,6 +64,9 @@ static void of_get_regulation_constraints(struct device_node
        ramp_delay = of_get_property(np, "regulator-ramp-delay", NULL);
        if (ramp_delay)
                constraints->ramp_delay = be32_to_cpu(*ramp_delay);
+
+       if (of_find_property(np, "regulator-ramp-disable", NULL))
+               constraints->ramp_disable = true;
 }

 /**
diff --git a/include/linux/regulator/machine.h b/include/linux/regulator/machin
index 36adbc8..02592da 100644
--- a/include/linux/regulator/machine.h
+++ b/include/linux/regulator/machine.h
@@ -134,6 +134,7 @@ struct regulation_constraints {
        unsigned always_on:1;   /* regulator never off when system is on */
        unsigned boot_on:1;     /* bootloader/firmware enabled regulator */
        unsigned apply_uV:1;    /* apply uV constraint if min == max */
+       unsigned int ramp_disable:1;
 };

 /**


Regards,
Yadwinder
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/