Re: [PATCH v2 04/14] regulator: SY8106A regulator driver

2016-06-28 Thread Ondřej Jirman 
On 27.6.2016 16:54, Mark Brown wrote:
> On Sun, Jun 26, 2016 at 05:07:16PM +0200, Ondřej Jirman wrote:
>> On 26.6.2016 13:26, Mark Brown wrote:
> 
>>> I'm missing almost all of this series, I've just got this and another
>>> patch which look like a standalone driver so it's hard to see any
>>> dependencies.  What's going on here?
> 
>> Sorry, it's this series:
>> http://lists.infradead.org/pipermail/linux-arm-kernel/2016-June/438981.html
> 
>> And here's v1 intro letter:
>> http://lists.infradead.org/pipermail/linux-arm-kernel/2016-June/438794.html
> 
> These aren't really enlightening I'm afraid, there's nothing in there
> about why these are a single series and no dependency information.
> Indeed it looks like something is really confused as the thread is
> missing at least patches 2 and 3.
> 

You're right there's not much about the regulator there. Xulong Orange
Pi PC is ARM SBC, that uses Allwinner H3 Soc, to which this regulator is
connected via I2C bus, to regulate the main CPU supply voltage. It is
used on some other SBC's from Xulong.

It is fairly simple single output, fixed I2C address voltage regulator.
It has just 3 registers, which allow for: slew rate control (not used in
this driver), setting and reading out the voltage, switching between
voltage set via I2C and via external resistor divider and
enabling/disabling output. It also has status reporting for
over-tempertature and over-current conditions. That's all the features
it has.

The patch series uses it to provide dynamic vltage scaling of the ARM
cores in the SoC to be able to achieve higher CPU frequency.

Datasheet is here:

https://01916271185794791722.googlegroups.com/attach/93415fbd5402/SY8106A_datasheet.pdf?part=0.1=ANaJVrHMej8w8XRfd-7A3XoyiryMDDhrHU2SS-tYyHCpOIXRqIaICOIlZTWAUV74vToesmic457zPvODDuvrNnRpomPOPbtV8bMMFV65VX5aYb5NciMkh8E

I'll also include this information in the revised patch.

thank you and regards,
  Ondrej Jirman



signature.asc
Description: OpenPGP digital signature

Re: [PATCH v2 04/14] regulator: SY8106A regulator driver

2016-06-28 Thread Ondřej Jirman 
On 27.6.2016 16:54, Mark Brown wrote:
> On Sun, Jun 26, 2016 at 05:07:16PM +0200, Ondřej Jirman wrote:
>> On 26.6.2016 13:26, Mark Brown wrote:
> 
>>> I'm missing almost all of this series, I've just got this and another
>>> patch which look like a standalone driver so it's hard to see any
>>> dependencies.  What's going on here?
> 
>> Sorry, it's this series:
>> http://lists.infradead.org/pipermail/linux-arm-kernel/2016-June/438981.html
> 
>> And here's v1 intro letter:
>> http://lists.infradead.org/pipermail/linux-arm-kernel/2016-June/438794.html
> 
> These aren't really enlightening I'm afraid, there's nothing in there
> about why these are a single series and no dependency information.
> Indeed it looks like something is really confused as the thread is
> missing at least patches 2 and 3.
> 

You're right there's not much about the regulator there. Xulong Orange
Pi PC is ARM SBC, that uses Allwinner H3 Soc, to which this regulator is
connected via I2C bus, to regulate the main CPU supply voltage. It is
used on some other SBC's from Xulong.

It is fairly simple single output, fixed I2C address voltage regulator.
It has just 3 registers, which allow for: slew rate control (not used in
this driver), setting and reading out the voltage, switching between
voltage set via I2C and via external resistor divider and
enabling/disabling output. It also has status reporting for
over-tempertature and over-current conditions. That's all the features
it has.

The patch series uses it to provide dynamic vltage scaling of the ARM
cores in the SoC to be able to achieve higher CPU frequency.

Datasheet is here:

https://01916271185794791722.googlegroups.com/attach/93415fbd5402/SY8106A_datasheet.pdf?part=0.1=ANaJVrHMej8w8XRfd-7A3XoyiryMDDhrHU2SS-tYyHCpOIXRqIaICOIlZTWAUV74vToesmic457zPvODDuvrNnRpomPOPbtV8bMMFV65VX5aYb5NciMkh8E

I'll also include this information in the revised patch.

thank you and regards,
  Ondrej Jirman



signature.asc
Description: OpenPGP digital signature

Re: [PATCH v2 04/14] regulator: SY8106A regulator driver

2016-06-27 Thread Mark Brown 
On Sat, Jun 25, 2016 at 05:45:01AM +0200, meg...@megous.com wrote:

> + config.init_data = of_get_regulator_init_data(dev, dev->of_node, 
> _reg);
> + if (!config.init_data) {
> + return -EINVAL;
> + }

This is broken, constraints are entirely optional - the driver should
just instantiate no matter what (if anything) the constraints are.  This
means people can read the current state even if there is no need for
runtime configuration.

> + dev_info(>dev, "SY8106A voltage at boot: %u mV\n", SY8106A_MIN_MV 
> + 
> +  SY8106A_STEP_MV * (selector & SY8106A_REG_VOUT1_SEL_MASK));

This is just noise, remove it - if we want to list the voltage for
regulators at probe we should be doing it consistently for all
regulators in the core rather than in individual drivers.

> +/*
> + * This is useless for OF-enabled devices, but it is needed by I2C subsystem
> + */
> +static const struct i2c_device_id sy8106a_i2c_id[] = {
> + { },
> +};
> +MODULE_DEVICE_TABLE(i2c, sy8106a_i2c_id);

No, this is not the case - supporting DT does not mean that other
enumeration mechanisms can't be supported and there's no reason not to
list a sensible ID here.


signature.asc
Description: PGP signature

Re: [PATCH v2 04/14] regulator: SY8106A regulator driver

2016-06-27 Thread Mark Brown 
On Sat, Jun 25, 2016 at 05:45:01AM +0200, meg...@megous.com wrote:

> + config.init_data = of_get_regulator_init_data(dev, dev->of_node, 
> _reg);
> + if (!config.init_data) {
> + return -EINVAL;
> + }

This is broken, constraints are entirely optional - the driver should
just instantiate no matter what (if anything) the constraints are.  This
means people can read the current state even if there is no need for
runtime configuration.

> + dev_info(>dev, "SY8106A voltage at boot: %u mV\n", SY8106A_MIN_MV 
> + 
> +  SY8106A_STEP_MV * (selector & SY8106A_REG_VOUT1_SEL_MASK));

This is just noise, remove it - if we want to list the voltage for
regulators at probe we should be doing it consistently for all
regulators in the core rather than in individual drivers.

> +/*
> + * This is useless for OF-enabled devices, but it is needed by I2C subsystem
> + */
> +static const struct i2c_device_id sy8106a_i2c_id[] = {
> + { },
> +};
> +MODULE_DEVICE_TABLE(i2c, sy8106a_i2c_id);

No, this is not the case - supporting DT does not mean that other
enumeration mechanisms can't be supported and there's no reason not to
list a sensible ID here.


signature.asc
Description: PGP signature

Re: [PATCH v2 04/14] regulator: SY8106A regulator driver

2016-06-27 Thread Mark Brown 
On Sun, Jun 26, 2016 at 05:07:16PM +0200, Ondřej Jirman wrote:
> On 26.6.2016 13:26, Mark Brown wrote:

> > I'm missing almost all of this series, I've just got this and another
> > patch which look like a standalone driver so it's hard to see any
> > dependencies.  What's going on here?

> Sorry, it's this series:
> http://lists.infradead.org/pipermail/linux-arm-kernel/2016-June/438981.html

> And here's v1 intro letter:
> http://lists.infradead.org/pipermail/linux-arm-kernel/2016-June/438794.html

These aren't really enlightening I'm afraid, there's nothing in there
about why these are a single series and no dependency information.
Indeed it looks like something is really confused as the thread is
missing at least patches 2 and 3.


signature.asc
Description: PGP signature

Re: [PATCH v2 04/14] regulator: SY8106A regulator driver

2016-06-27 Thread Mark Brown 
On Sun, Jun 26, 2016 at 05:07:16PM +0200, Ondřej Jirman wrote:
> On 26.6.2016 13:26, Mark Brown wrote:

> > I'm missing almost all of this series, I've just got this and another
> > patch which look like a standalone driver so it's hard to see any
> > dependencies.  What's going on here?

> Sorry, it's this series:
> http://lists.infradead.org/pipermail/linux-arm-kernel/2016-June/438981.html

> And here's v1 intro letter:
> http://lists.infradead.org/pipermail/linux-arm-kernel/2016-June/438794.html

These aren't really enlightening I'm afraid, there's nothing in there
about why these are a single series and no dependency information.
Indeed it looks like something is really confused as the thread is
missing at least patches 2 and 3.


signature.asc
Description: PGP signature

Re: [PATCH v2 04/14] regulator: SY8106A regulator driver

2016-06-26 Thread Ondřej Jirman 
On 26.6.2016 13:26, Mark Brown wrote:
> On Sat, Jun 25, 2016 at 05:45:01AM +0200, meg...@megous.com wrote:
>> From: Ondrej Jirman 
>>
>> SY8106A is I2C attached single output voltage regulator
>> made by Silergy.
> 
> I'm missing almost all of this series, I've just got this and another
> patch which look like a standalone driver so it's hard to see any
> dependencies.  What's going on here?
> 

Sorry, it's this series:
http://lists.infradead.org/pipermail/linux-arm-kernel/2016-June/438981.html

And here's v1 intro letter:
http://lists.infradead.org/pipermail/linux-arm-kernel/2016-June/438794.html

regards,
  Ondrej Jirman

Re: [PATCH v2 04/14] regulator: SY8106A regulator driver

2016-06-26 Thread Ondřej Jirman 
On 26.6.2016 13:26, Mark Brown wrote:
> On Sat, Jun 25, 2016 at 05:45:01AM +0200, meg...@megous.com wrote:
>> From: Ondrej Jirman 
>>
>> SY8106A is I2C attached single output voltage regulator
>> made by Silergy.
> 
> I'm missing almost all of this series, I've just got this and another
> patch which look like a standalone driver so it's hard to see any
> dependencies.  What's going on here?
> 

Sorry, it's this series:
http://lists.infradead.org/pipermail/linux-arm-kernel/2016-June/438981.html

And here's v1 intro letter:
http://lists.infradead.org/pipermail/linux-arm-kernel/2016-June/438794.html

regards,
  Ondrej Jirman

Re: [PATCH v2 04/14] regulator: SY8106A regulator driver

2016-06-26 Thread Mark Brown 
On Sat, Jun 25, 2016 at 05:45:01AM +0200, meg...@megous.com wrote:
> From: Ondrej Jirman 
> 
> SY8106A is I2C attached single output voltage regulator
> made by Silergy.

I'm missing almost all of this series, I've just got this and another
patch which look like a standalone driver so it's hard to see any
dependencies.  What's going on here?


signature.asc
Description: PGP signature

Re: [PATCH v2 04/14] regulator: SY8106A regulator driver

2016-06-26 Thread Mark Brown 
On Sat, Jun 25, 2016 at 05:45:01AM +0200, meg...@megous.com wrote:
> From: Ondrej Jirman 
> 
> SY8106A is I2C attached single output voltage regulator
> made by Silergy.

I'm missing almost all of this series, I've just got this and another
patch which look like a standalone driver so it's hard to see any
dependencies.  What's going on here?


signature.asc
Description: PGP signature

[PATCH v2 04/14] regulator: SY8106A regulator driver

2016-06-24 Thread megous 
From: Ondrej Jirman 

SY8106A is I2C attached single output voltage regulator
made by Silergy.

Signed-off-by: Ondrej Jirman 
---
v2
- added dt-bindings for the regulator
- changed to use of_device_id for probing
- added initialization failure checks
---
 drivers/regulator/Kconfig |   8 +-
 drivers/regulator/Makefile|   2 +-
 drivers/regulator/sy8106a-regulator.c | 171 ++
 3 files changed, 179 insertions(+), 2 deletions(-)
 create mode 100644 drivers/regulator/sy8106a-regulator.c

diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index 144cbf5..93f3fe4f 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -702,6 +702,13 @@ config REGULATOR_STW481X_VMMC
  This driver supports the internal VMMC regulator in the STw481x
  PMIC chips.
 
+config REGULATOR_SY8106A
+   tristate "Silergy SY8106A"
+   depends on I2C && (OF || COMPILE_TEST)
+   select REGMAP_I2C
+   help
+ This driver provides support for SY8106A voltage regulator.
+
 config REGULATOR_TPS51632
tristate "TI TPS51632 Power Regulator"
depends on I2C
@@ -861,4 +868,3 @@ config REGULATOR_WM8994
  WM8994 CODEC.
 
 endif
-
diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index 85a1d44..e3f720f 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -88,6 +88,7 @@ obj-$(CONFIG_REGULATOR_S2MPS11) += s2mps11.o
 obj-$(CONFIG_REGULATOR_S5M8767) += s5m8767.o
 obj-$(CONFIG_REGULATOR_SKY81452) += sky81452-regulator.o
 obj-$(CONFIG_REGULATOR_STW481X_VMMC) += stw481x-vmmc.o
+obj-$(CONFIG_REGULATOR_SY8106A) += sy8106a-regulator.o
 obj-$(CONFIG_REGULATOR_TI_ABB) += ti-abb-regulator.o
 obj-$(CONFIG_REGULATOR_TPS6105X) += tps6105x-regulator.o
 obj-$(CONFIG_REGULATOR_TPS62360) += tps62360-regulator.o
@@ -111,5 +112,4 @@ obj-$(CONFIG_REGULATOR_WM8350) += wm8350-regulator.o
 obj-$(CONFIG_REGULATOR_WM8400) += wm8400-regulator.o
 obj-$(CONFIG_REGULATOR_WM8994) += wm8994-regulator.o
 
-
 ccflags-$(CONFIG_REGULATOR_DEBUG) += -DDEBUG
diff --git a/drivers/regulator/sy8106a-regulator.c 
b/drivers/regulator/sy8106a-regulator.c
new file mode 100644
index 000..98626bc
--- /dev/null
+++ b/drivers/regulator/sy8106a-regulator.c
@@ -0,0 +1,171 @@
+/*
+ * sy8106a-regulator.c - Regulator device driver for SY8106A
+ *
+ * Copyright (C) 2016  Ondřej Jirman 
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Library General Public
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Library General Public License for more details.
+ *
+ * You should have received a copy of the GNU Library General Public
+ * License along with this library; if not, write to the
+ * Free Software Foundation, Inc., 51 Franklin St, Fifth Floor,
+ * Boston, MA  02110-1301, USA.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#define SY8106A_REG_VOUT1_SEL  0x01
+#define SY8106A_REG_VOUT_COM   0x02
+#define SY8106A_REG_VOUT1_SEL_MASK 0x7f
+#define SY8106A_DISABLE_REGBIT(0)
+#define SY8106A_GO_BIT BIT(7)
+
+struct sy8106a {
+   struct regulator_dev *rdev;
+   struct regmap *regmap;
+};
+
+static const struct regmap_config sy8106a_regmap_config = {
+   .reg_bits = 8,
+   .val_bits = 8,
+};
+
+static int sy8106a_set_voltage_sel(struct regulator_dev *rdev, unsigned sel)
+{
+   /* We use our set_voltage_sel in order to avoid unnecessary I2C chatter,
+* because the regulator_get_voltage_sel_regmap using apply_bit
+* would perform 4 unnecessary transfers instead of one, increasing the
+* chance of error.
+*/
+   return regmap_write(rdev->regmap, rdev->desc->vsel_reg,
+   sel | SY8106A_GO_BIT);
+}
+
+static const struct regulator_ops sy8106a_ops = {
+   .is_enabled = regulator_is_enabled_regmap,
+   .set_voltage_sel = sy8106a_set_voltage_sel,
+   .set_voltage_time_sel = regulator_set_voltage_time_sel,
+   .get_voltage_sel = regulator_get_voltage_sel_regmap,
+   .list_voltage = regulator_list_voltage_linear,
+};
+
+/* Default limits measured in millivolts and milliamps */
+#define SY8106A_MIN_MV 680
+#define SY8106A_MAX_MV 1950
+#define SY8106A_STEP_MV10
+
+static const struct regulator_desc sy8106a_reg = {
+   .name = "SY8106A",
+   .id = 0,
+   .ops = _ops,
+   .type = REGULATOR_VOLTAGE,
+   .n_voltages = ((SY8106A_MAX_MV - SY8106A_MIN_MV) / SY8106A_STEP_MV) + 1,
+   .min_uV = (SY8106A_MIN_MV

[PATCH v2 04/14] regulator: SY8106A regulator driver

2016-06-24 Thread megous 
From: Ondrej Jirman 

SY8106A is I2C attached single output voltage regulator
made by Silergy.

Signed-off-by: Ondrej Jirman 
---
v2
- added dt-bindings for the regulator
- changed to use of_device_id for probing
- added initialization failure checks
---
 drivers/regulator/Kconfig |   8 +-
 drivers/regulator/Makefile|   2 +-
 drivers/regulator/sy8106a-regulator.c | 171 ++
 3 files changed, 179 insertions(+), 2 deletions(-)
 create mode 100644 drivers/regulator/sy8106a-regulator.c

diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index 144cbf5..93f3fe4f 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -702,6 +702,13 @@ config REGULATOR_STW481X_VMMC
  This driver supports the internal VMMC regulator in the STw481x
  PMIC chips.
 
+config REGULATOR_SY8106A
+   tristate "Silergy SY8106A"
+   depends on I2C && (OF || COMPILE_TEST)
+   select REGMAP_I2C
+   help
+ This driver provides support for SY8106A voltage regulator.
+
 config REGULATOR_TPS51632
tristate "TI TPS51632 Power Regulator"
depends on I2C
@@ -861,4 +868,3 @@ config REGULATOR_WM8994
  WM8994 CODEC.
 
 endif
-
diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index 85a1d44..e3f720f 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -88,6 +88,7 @@ obj-$(CONFIG_REGULATOR_S2MPS11) += s2mps11.o
 obj-$(CONFIG_REGULATOR_S5M8767) += s5m8767.o
 obj-$(CONFIG_REGULATOR_SKY81452) += sky81452-regulator.o
 obj-$(CONFIG_REGULATOR_STW481X_VMMC) += stw481x-vmmc.o
+obj-$(CONFIG_REGULATOR_SY8106A) += sy8106a-regulator.o
 obj-$(CONFIG_REGULATOR_TI_ABB) += ti-abb-regulator.o
 obj-$(CONFIG_REGULATOR_TPS6105X) += tps6105x-regulator.o
 obj-$(CONFIG_REGULATOR_TPS62360) += tps62360-regulator.o
@@ -111,5 +112,4 @@ obj-$(CONFIG_REGULATOR_WM8350) += wm8350-regulator.o
 obj-$(CONFIG_REGULATOR_WM8400) += wm8400-regulator.o
 obj-$(CONFIG_REGULATOR_WM8994) += wm8994-regulator.o
 
-
 ccflags-$(CONFIG_REGULATOR_DEBUG) += -DDEBUG
diff --git a/drivers/regulator/sy8106a-regulator.c 
b/drivers/regulator/sy8106a-regulator.c
new file mode 100644
index 000..98626bc
--- /dev/null
+++ b/drivers/regulator/sy8106a-regulator.c
@@ -0,0 +1,171 @@
+/*
+ * sy8106a-regulator.c - Regulator device driver for SY8106A
+ *
+ * Copyright (C) 2016  Ondřej Jirman 
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Library General Public
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Library General Public License for more details.
+ *
+ * You should have received a copy of the GNU Library General Public
+ * License along with this library; if not, write to the
+ * Free Software Foundation, Inc., 51 Franklin St, Fifth Floor,
+ * Boston, MA  02110-1301, USA.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#define SY8106A_REG_VOUT1_SEL  0x01
+#define SY8106A_REG_VOUT_COM   0x02
+#define SY8106A_REG_VOUT1_SEL_MASK 0x7f
+#define SY8106A_DISABLE_REGBIT(0)
+#define SY8106A_GO_BIT BIT(7)
+
+struct sy8106a {
+   struct regulator_dev *rdev;
+   struct regmap *regmap;
+};
+
+static const struct regmap_config sy8106a_regmap_config = {
+   .reg_bits = 8,
+   .val_bits = 8,
+};
+
+static int sy8106a_set_voltage_sel(struct regulator_dev *rdev, unsigned sel)
+{
+   /* We use our set_voltage_sel in order to avoid unnecessary I2C chatter,
+* because the regulator_get_voltage_sel_regmap using apply_bit
+* would perform 4 unnecessary transfers instead of one, increasing the
+* chance of error.
+*/
+   return regmap_write(rdev->regmap, rdev->desc->vsel_reg,
+   sel | SY8106A_GO_BIT);
+}
+
+static const struct regulator_ops sy8106a_ops = {
+   .is_enabled = regulator_is_enabled_regmap,
+   .set_voltage_sel = sy8106a_set_voltage_sel,
+   .set_voltage_time_sel = regulator_set_voltage_time_sel,
+   .get_voltage_sel = regulator_get_voltage_sel_regmap,
+   .list_voltage = regulator_list_voltage_linear,
+};
+
+/* Default limits measured in millivolts and milliamps */
+#define SY8106A_MIN_MV 680
+#define SY8106A_MAX_MV 1950
+#define SY8106A_STEP_MV10
+
+static const struct regulator_desc sy8106a_reg = {
+   .name = "SY8106A",
+   .id = 0,
+   .ops = _ops,
+   .type = REGULATOR_VOLTAGE,
+   .n_voltages = ((SY8106A_MAX_MV - SY8106A_MIN_MV) / SY8106A_STEP_MV) + 1,
+   .min_uV = (SY8106A_MIN_MV * 1000),
+   .uV_step = (SY8106A_STEP_MV * 1000),
+