Re: [PATCH 01/13] clk: tegra: Add binding for the Tegra124 DFLL clocksource

[Tuomas Tynkkynen <ttynkkynen@nvidia.com>]

On 14/07/14 13:22, Mark Brown wrote:
> * PGP Signed by an unknown key
> 
> On Mon, Jul 14, 2014 at 11:24:35AM +0200, Thierry Reding wrote:
>> On Mon, Jul 14, 2014 at 10:12:33AM +0100, Mark Brown wrote:
> 
>>> The selector value is opaque, it's entirely up to the driver to define
>>> it.  If you could tell me what "this" is I might be able to advise on
>>> how to do it.
> 
>> Tegra124 (and later, also some earlier variants) have this DFLL clock
>> that can program a PMIC automatically depending on the CPU frequency.
>> This DT binding did propose putting this into device tree as a table of
>> <frequency value> pairs where the frequency corresponds to the CPU
>> frequency and the value is the register value to be programmed into the
>> PMIC by the DFLL hardware (there are two additional properties to define
>> the slave address and the register offset).
> 
>> Andrew proposed that this table could instead be built by using
>> regulator_list_voltage() instead. However, due to the fact that the DFLL
>> hardware needs to know the immediate value to write into a register, the
>> requirement would be for a 1:1 mapping between selector and register
>> value. Given that the API needs to cover the general case I don't see
>> how it could practically ensure this.
> 
> Well, if you're going to do that you've already created a private API
> between the regulator driver and the device since you're assuming that
> the device is controlled only by register writes to a single register
> bitfield which isn't always the case.
> 
> As with all these things it would also be better to extend the regulator
> API so that users like this can discover the register address and so on
> too rather than having to replicate that information in the device tree.
> No sense in having to specify this information multiple times.
> 

That sounds indeed useful for this case. How'd the following interface
sound for the register offset / selector-to-register-value conversion?
The I2C address would be a bit trickier to get as it would touch the
regmap stuff as well, but perhaps it would be a good idea to have a
phandle to the I2C device itself, and then parse the reg field for
the address.

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index c563d93..a5efb96 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -2228,6 +2228,63 @@ int regulator_list_voltage(struct regulator *regulator, unsigned selector)
 EXPORT_SYMBOL_GPL(regulator_list_voltage);

 /**
+ * regulator_get_hardware_vsel_register - get the HW voltage selector register
+ * @regulator: regulator source
+ * @vsel_reg: voltage selector register, output parameter
+ * @vsel_mask: mask for voltage selector bitfield, output parameter
+ *
+ * Returns the hardware register offset and bitmask used for setting the
+ * regulator voltage. This might be useful when configuring voltage-scaling
+ * hardware or firmware that can make I2C requests behind the kernel's back,
+ * for example.
+ *
+ * On success, the output parameters @vsel_reg and @vsel_mask are filled in
+ * and 0 is returned, otherwise a negative errno is returned.
+ */
+int regulator_get_hardware_vsel_register(struct regulator *regulator,
+					 unsigned *vsel_reg,
+					 unsigned *vsel_mask)
+{
+	struct regulator_dev	*rdev = regulator->rdev;
+	struct regulator_ops	*ops = rdev->desc->ops;
+
+	if (ops->set_voltage_sel != regulator_set_voltage_sel_regmap)
+		return -EOPNOTSUPP;
+
+	 *vsel_reg = rdev->desc->vsel_reg;
+	 *vsel_mask = rdev->desc->vsel_mask;
+
+	 return 0;
+}
+EXPORT_SYMBOL_GPL(regulator_get_hardware_vsel_register);
+
+/**
+ * regulator_list_hardware_vsel - get the HW-specific register value for a selector
+ * @regulator: regulator source
+ * @selector: identify voltage to list
+ *
+ * Converts the selector to a hardware-specific voltage selector that can be
+ * directly written to the regulator registers. The address of the voltage
+ * register can be determined by calling @regulator_get_hardware_vsel_register.
+ *
+ * On error a negative errno is returned.
+ */
+int regulator_list_hardware_vsel(struct regulator *regulator,
+				 unsigned selector)
+{
+	struct regulator_dev	*rdev = regulator->rdev;
+	struct regulator_ops	*ops = rdev->desc->ops;
+
+	if (selector >= rdev->desc->n_voltages)
+		return -EINVAL;
+	if (ops->set_voltage_sel != regulator_set_voltage_sel_regmap)
+		return -EOPNOTSUPP;
+
+	return selector;
+}
+EXPORT_SYMBOL_GPL(regulator_list_hardware_vsel);
+
+/**
  * regulator_get_linear_step - return the voltage step size between VSEL values
  * @regulator: regulator source
  *
diff --git a/include/linux/regulator/consumer.h b/include/linux/regulator/consumer.h
index 14ec18d..fe4cdb2 100644
--- a/include/linux/regulator/consumer.h
+++ b/include/linux/regulator/consumer.h
@@ -215,6 +215,12 @@ int regulator_set_optimum_mode(struct regulator *regulator, int load_uA);

 int regulator_allow_bypass(struct regulator *regulator, bool allow);

+int regulator_get_hardware_vsel_register(struct regulator *regulator,
+					 unsigned *vsel_reg,
+					 unsigned *vsel_mask);
+int regulator_list_hardware_vsel(struct regulator *regulator,
+				 unsigned selector);
+
 /* regulator notifier block */
 int regulator_register_notifier(struct regulator *regulator,
 			      struct notifier_block *nb);

-- 
nvpublic