Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1225471 > unrolled thread

[PATCH 0/3] mfd: tps65912: Driver rewrite with DT support

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/3] mfd: tps65912: Driver rewrite with DT support "Andrew F. Davis" <afd@ti.com> - 2015-09-15 20:00 +0200
    [PATCH 1/3] Documentation: tps65912: Add DT bindings for the TPS65912 PMIC "Andrew F. Davis" <afd@ti.com> - 2015-09-15 20:00 +0200
      Re: [PATCH 1/3] Documentation: tps65912: Add DT bindings for the  TPS65912 PMIC Lee Jones <lee.jones@linaro.org> - 2015-09-20 06:20 +0200
        Re: [PATCH 1/3] Documentation: tps65912: Add DT bindings for the  TPS65912 PMIC "Andrew F. Davis" <afd@ti.com> - 2015-09-21 18:40 +0200
          Re: [PATCH 1/3] Documentation: tps65912: Add DT bindings for the  TPS65912 PMIC Lee Jones <lee.jones@linaro.org> - 2015-09-22 01:10 +0200
    [PATCH 3/3] tps65912: Cleanup TPS65912 subdevice configuration dependencies "Andrew F. Davis" <afd@ti.com> - 2015-09-15 20:00 +0200
      Re: [PATCH 3/3] tps65912: Cleanup TPS65912 subdevice configuration  dependencies Mark Brown <broonie@kernel.org> - 2015-09-16 22:00 +0200
    Re: [PATCH 2/3] mfd: tps65912: Rewrite driver adding DT support and  using regmap Mark Brown <broonie@kernel.org> - 2015-09-20 02:40 +0200
      Re: [PATCH 2/3] mfd: tps65912: Rewrite driver adding DT support and  using regmap "Andrew F. Davis" <afd@ti.com> - 2015-09-21 18:50 +0200
        Re: [PATCH 2/3] mfd: tps65912: Rewrite driver adding DT support and  using regmap Mark Brown <broonie@kernel.org> - 2015-09-21 21:30 +0200
          Re: [PATCH 2/3] mfd: tps65912: Rewrite driver adding DT support and  using regmap "Andrew F. Davis" <afd@ti.com> - 2015-09-21 21:50 +0200
            Re: [PATCH 2/3] mfd: tps65912: Rewrite driver adding DT support and  using regmap Mark Brown <broonie@kernel.org> - 2015-09-21 22:00 +0200
              Re: [PATCH 2/3] mfd: tps65912: Rewrite driver adding DT support and  using regmap "Andrew F. Davis" <afd@ti.com> - 2015-09-21 22:10 +0200
    Re: [PATCH 2/3] mfd: tps65912: Rewrite driver adding DT support and  using regmap Lee Jones <lee.jones@linaro.org> - 2015-09-22 01:10 +0200

#1225471 — [PATCH 0/3] mfd: tps65912: Driver rewrite with DT support

In an effort to cleanup this driver and add Device Tree support
the driver has been rewritten based on new driver styles and
modern kernel driver helpers. This has nearly halved the lines
of code while keeping all previous functionality.

Platform file based initialization has been dropped as there is
no examples of this use in the kernel.

Andrew F. Davis (3):
  Documentation: tps65912: Add DT bindings for the TPS65912 PMIC
  mfd: tps65912: Rewrite driver adding DT support and using regmap
  tps65912: Cleanup TPS65912 subdevice configuration dependencies

 .../devicetree/bindings/gpio/gpio-tps65912.txt     |  17 +
 Documentation/devicetree/bindings/mfd/tps65912.txt |  43 ++
 .../bindings/regulator/tps65912-regulator.txt      |  32 +
 drivers/gpio/Kconfig                               |   2 +-
 drivers/gpio/gpio-tps65912.c                       | 291 ++++----
 drivers/mfd/Kconfig                                |  20 +-
 drivers/mfd/Makefile                               |   3 +-
 drivers/mfd/tps65912-core.c                        | 288 +++-----
 drivers/mfd/tps65912-i2c.c                         | 233 +++---
 drivers/mfd/tps65912-irq.c                         | 217 ------
 drivers/mfd/tps65912-spi.c                         | 236 +++----
 drivers/regulator/Kconfig                          |   2 +-
 drivers/regulator/tps65912-regulator.c             | 783 +++++++--------------
 include/linux/mfd/tps65912.h                       | 256 ++++---
 14 files changed, 948 insertions(+), 1475 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/gpio/gpio-tps65912.txt
 create mode 100644 Documentation/devicetree/bindings/mfd/tps65912.txt
 create mode 100644 Documentation/devicetree/bindings/regulator/tps65912-regulator.txt
 rewrite drivers/gpio/gpio-tps65912.c (68%)
 rewrite drivers/mfd/tps65912-core.c (96%)
 rewrite drivers/mfd/tps65912-i2c.c (92%)
 delete mode 100644 drivers/mfd/tps65912-irq.c
 rewrite drivers/mfd/tps65912-spi.c (91%)
 rewrite drivers/regulator/tps65912-regulator.c (94%)

-- 
1.9.1

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

[toc] | [next] | [standalone]

#1225472 — [PATCH 1/3] Documentation: tps65912: Add DT bindings for the TPS65912 PMIC

The TPS65912 PMIC contains several regulators and a GPIO controller.
Add bindings for the TPS65912 PMIC.

Signed-off-by: Andrew F. Davis <afd@ti.com>
---
 .../devicetree/bindings/gpio/gpio-tps65912.txt     | 17 +++++++++
 Documentation/devicetree/bindings/mfd/tps65912.txt | 43 ++++++++++++++++++++++
 .../bindings/regulator/tps65912-regulator.txt      | 32 ++++++++++++++++
 3 files changed, 92 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/gpio-tps65912.txt
 create mode 100644 Documentation/devicetree/bindings/mfd/tps65912.txt
 create mode 100644 Documentation/devicetree/bindings/regulator/tps65912-regulator.txt

diff --git a/Documentation/devicetree/bindings/gpio/gpio-tps65912.txt b/Documentation/devicetree/bindings/gpio/gpio-tps65912.txt
new file mode 100644
index 0000000..f65370b
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/gpio-tps65912.txt
@@ -0,0 +1,17 @@
+* TPS65912 GPIO controller bindings
+
+Required properties:
+ - compatible : Should be "ti,tps65912-gpio".
+ - gpio-controller : Marks the device node as a gpio controller.
+ - #gpio-cells : Should be two.  The first cell is the pin number and
+     the second cell is used to specify the gpio polarity:
+       0 = active high
+       1 = active low
+
+Example:
+
+	gpio4: tps65912_gpio {
+		compatible = "ti,tps65912-gpio";
+		gpio-controller;
+		#gpio-cells = <2>;
+	};
diff --git a/Documentation/devicetree/bindings/mfd/tps65912.txt b/Documentation/devicetree/bindings/mfd/tps65912.txt
new file mode 100644
index 0000000..081af66
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/tps65912.txt
@@ -0,0 +1,43 @@
+* TPS65912 Power Management Integrated Circuit bindings
+
+Required properties:
+ - compatible : Should be "ti,tps65912".
+ - reg : Slave address or chip select number (I2C / SPI).
+ - interrupt-parent : The parent interrupt controller.
+ - interrupts : The interrupt line the device is connected to.
+ - interrupt-controller : Marks the device node as an interrupt controller.
+ - #interrupt-cells: the number of cells to describe an IRQ, this should be 2.
+     The first cell is the IRQ number.
+     The second cell is the flags, encoded as the trigger masks from
+     Documentation/devicetree/bindings/interrupts.txt
+
+Optional nodes:
+ - Regulators: Documentation/devicetree/bindings/regulator/tps65912-regulator.txt
+ - GPIO: Documentation/devicetree/bindings/gpio/gpio-tps65912.txt.
+
+Example:
+
+	pmic: tps65912@2d {
+		compatible = "ti,tps65912";
+		reg = <0x2d>;
+		interrupt-parent = <&gpio1>;
+		interrupts = <28 IRQ_TYPE_LEVEL_LOW>;
+		interrupt-controller;
+		#interrupt-cells = <2>;
+
+		dcdc1: regulator-dcdc1 {
+			compatible = "ti,tps65912-dcdc1";
+			regulator-name = "vdd_core";
+			regulator-min-microvolt = <912000>;
+			regulator-max-microvolt = <1144000>;
+			regulator-boot-on;
+			regulator-always-on;
+		};
+		...
+
+		gpio4: tps65912_gpio {
+			compatible = "ti,tps65912-gpio";
+			gpio-controller;
+			#gpio-cells = <2>;
+		};
+	};
diff --git a/Documentation/devicetree/bindings/regulator/tps65912-regulator.txt b/Documentation/devicetree/bindings/regulator/tps65912-regulator.txt
new file mode 100644
index 0000000..a417ff7
--- /dev/null
+++ b/Documentation/devicetree/bindings/regulator/tps65912-regulator.txt
@@ -0,0 +1,32 @@
+* TPS65912 regulator bindings
+
+Required properties:
+ - compatible: Should be:
+   - "ti,tps65912-dcdc1" for DCDC1
+   - "ti,tps65912-dcdc2" for DCDC2
+   - "ti,tps65912-dcdc3" for DCDC3
+   - "ti,tps65912-dcdc4" for DCDC4
+   - "ti,tps65912-ldo1" for LDO1
+   - "ti,tps65912-ldo2" for LDO2
+   - "ti,tps65912-ldo3" for LDO3
+   - "ti,tps65912-ldo4" for LDO4
+   - "ti,tps65912-ldo5" for LDO5
+   - "ti,tps65912-ldo6" for LDO6
+   - "ti,tps65912-ldo7" for LDO7
+   - "ti,tps65912-ldo8" for LDO8
+   - "ti,tps65912-ldo9" for LDO9
+   - "ti,tps65912-ldo10" for LDO10
+
+Optional properties:
+ - Any optional property defined in bindings/regulator/regulator.txt
+
+Example:
+
+	xyz: regulator@0 {
+		compatible = "ti,tps65912-dcdc1";
+		regulator-name = "vdd_core";
+		regulator-min-microvolt = <912000>;
+		regulator-max-microvolt = <1144000>;
+		regulator-boot-on;
+		regulator-always-on;
+	};
-- 
1.9.1

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

[toc] | [prev] | [next] | [standalone]

#1228820 — Re: [PATCH 1/3] Documentation: tps65912: Add DT bindings for the TPS65912 PMIC

On Tue, 15 Sep 2015, Andrew F. Davis wrote:

> The TPS65912 PMIC contains several regulators and a GPIO controller.
> Add bindings for the TPS65912 PMIC.
> 
> Signed-off-by: Andrew F. Davis <afd@ti.com>
> ---
>  .../devicetree/bindings/gpio/gpio-tps65912.txt     | 17 +++++++++
>  Documentation/devicetree/bindings/mfd/tps65912.txt | 43 ++++++++++++++++++++++
>  .../bindings/regulator/tps65912-regulator.txt      | 32 ++++++++++++++++
>  3 files changed, 92 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/gpio/gpio-tps65912.txt
>  create mode 100644 Documentation/devicetree/bindings/mfd/tps65912.txt
>  create mode 100644 Documentation/devicetree/bindings/regulator/tps65912-regulator.txt
> 
> diff --git a/Documentation/devicetree/bindings/gpio/gpio-tps65912.txt b/Documentation/devicetree/bindings/gpio/gpio-tps65912.txt
> new file mode 100644
> index 0000000..f65370b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/gpio-tps65912.txt
> @@ -0,0 +1,17 @@
> +* TPS65912 GPIO controller bindings

Suggest s/controller/Controller/

> +Required properties:
> + - compatible : Should be "ti,tps65912-gpio".
> + - gpio-controller : Marks the device node as a gpio controller.

s/gpio/GPIO/

As above for controller.

> + - #gpio-cells : Should be two.  The first cell is the pin number and
> +     the second cell is used to specify the gpio polarity:
> +       0 = active high
> +       1 = active low

Best to use the #defines in include/dt-bindings/gpio.

> +Example:
> +
> +	gpio4: tps65912_gpio {
> +		compatible = "ti,tps65912-gpio";
> +		gpio-controller;
> +		#gpio-cells = <2>;
> +	};
> diff --git a/Documentation/devicetree/bindings/mfd/tps65912.txt b/Documentation/devicetree/bindings/mfd/tps65912.txt
> new file mode 100644
> index 0000000..081af66
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/tps65912.txt
> @@ -0,0 +1,43 @@
> +* TPS65912 Power Management Integrated Circuit bindings
> +
> +Required properties:
> + - compatible : Should be "ti,tps65912".
> + - reg : Slave address or chip select number (I2C / SPI).
> + - interrupt-parent : The parent interrupt controller.
> + - interrupts : The interrupt line the device is connected to.
> + - interrupt-controller : Marks the device node as an interrupt controller.
> + - #interrupt-cells: the number of cells to describe an IRQ, this should be 2.

s/the/The/

> +     The first cell is the IRQ number.
> +     The second cell is the flags, encoded as the trigger masks from
> +     Documentation/devicetree/bindings/interrupts.txt

No such file.

> +Optional nodes:
> + - Regulators: Documentation/devicetree/bindings/regulator/tps65912-regulator.txt
> + - GPIO: Documentation/devicetree/bindings/gpio/gpio-tps65912.txt.

Better to use ../gpio, ../regulator, etc.

"Regulators" and "GPIO" aren't valid node names.

Please be more specific.

> +Example:
> +
> +	pmic: tps65912@2d {
> +		compatible = "ti,tps65912";
> +		reg = <0x2d>;
> +		interrupt-parent = <&gpio1>;
> +		interrupts = <28 IRQ_TYPE_LEVEL_LOW>;
> +		interrupt-controller;
> +		#interrupt-cells = <2>;
> +
> +		dcdc1: regulator-dcdc1 {
> +			compatible = "ti,tps65912-dcdc1";
> +			regulator-name = "vdd_core";
> +			regulator-min-microvolt = <912000>;
> +			regulator-max-microvolt = <1144000>;
> +			regulator-boot-on;
> +			regulator-always-on;
> +		};
> +		...

No need for this.

> +		gpio4: tps65912_gpio {
> +			compatible = "ti,tps65912-gpio";
> +			gpio-controller;
> +			#gpio-cells = <2>;
> +		};
> +	};
> diff --git a/Documentation/devicetree/bindings/regulator/tps65912-regulator.txt b/Documentation/devicetree/bindings/regulator/tps65912-regulator.txt
> new file mode 100644
> index 0000000..a417ff7
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/regulator/tps65912-regulator.txt
> @@ -0,0 +1,32 @@
> +* TPS65912 regulator bindings
> +
> +Required properties:
> + - compatible: Should be:
> +   - "ti,tps65912-dcdc1" for DCDC1
> +   - "ti,tps65912-dcdc2" for DCDC2
> +   - "ti,tps65912-dcdc3" for DCDC3
> +   - "ti,tps65912-dcdc4" for DCDC4
> +   - "ti,tps65912-ldo1" for LDO1
> +   - "ti,tps65912-ldo2" for LDO2
> +   - "ti,tps65912-ldo3" for LDO3
> +   - "ti,tps65912-ldo4" for LDO4
> +   - "ti,tps65912-ldo5" for LDO5
> +   - "ti,tps65912-ldo6" for LDO6
> +   - "ti,tps65912-ldo7" for LDO7
> +   - "ti,tps65912-ldo8" for LDO8
> +   - "ti,tps65912-ldo9" for LDO9
> +   - "ti,tps65912-ldo10" for LDO10
> +
> +Optional properties:
> + - Any optional property defined in bindings/regulator/regulator.txt

../regulator/...

> +Example:
> +
> +	xyz: regulator@0 {
> +		compatible = "ti,tps65912-dcdc1";
> +		regulator-name = "vdd_core";
> +		regulator-min-microvolt = <912000>;
> +		regulator-max-microvolt = <1144000>;
> +		regulator-boot-on;
> +		regulator-always-on;
> +	};

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1229545 — Re: [PATCH 1/3] Documentation: tps65912: Add DT bindings for the TPS65912 PMIC

On 09/19/2015 11:16 PM, Lee Jones wrote:
> On Tue, 15 Sep 2015, Andrew F. Davis wrote:
>
>> The TPS65912 PMIC contains several regulators and a GPIO controller.
>> Add bindings for the TPS65912 PMIC.
>>
>> Signed-off-by: Andrew F. Davis <afd@ti.com>
>> ---
>>   .../devicetree/bindings/gpio/gpio-tps65912.txt     | 17 +++++++++
>>   Documentation/devicetree/bindings/mfd/tps65912.txt | 43 ++++++++++++++++++++++
>>   .../bindings/regulator/tps65912-regulator.txt      | 32 ++++++++++++++++
>>   3 files changed, 92 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/gpio/gpio-tps65912.txt
>>   create mode 100644 Documentation/devicetree/bindings/mfd/tps65912.txt
>>   create mode 100644 Documentation/devicetree/bindings/regulator/tps65912-regulator.txt
>>
>> diff --git a/Documentation/devicetree/bindings/gpio/gpio-tps65912.txt b/Documentation/devicetree/bindings/gpio/gpio-tps65912.txt
>> new file mode 100644
>> index 0000000..f65370b
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/gpio/gpio-tps65912.txt
>> @@ -0,0 +1,17 @@
>> +* TPS65912 GPIO controller bindings
>
> Suggest s/controller/Controller/
>

ACK

>> +Required properties:
>> + - compatible : Should be "ti,tps65912-gpio".
>> + - gpio-controller : Marks the device node as a gpio controller.
>
> s/gpio/GPIO/
>
> As above for controller.
>

ACK

>> + - #gpio-cells : Should be two.  The first cell is the pin number and
>> +     the second cell is used to specify the gpio polarity:
>> +       0 = active high
>> +       1 = active low
>
> Best to use the #defines in include/dt-bindings/gpio.
>

ACK

>> +Example:
>> +
>> +	gpio4: tps65912_gpio {
>> +		compatible = "ti,tps65912-gpio";
>> +		gpio-controller;
>> +		#gpio-cells = <2>;
>> +	};
>> diff --git a/Documentation/devicetree/bindings/mfd/tps65912.txt b/Documentation/devicetree/bindings/mfd/tps65912.txt
>> new file mode 100644
>> index 0000000..081af66
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/mfd/tps65912.txt
>> @@ -0,0 +1,43 @@
>> +* TPS65912 Power Management Integrated Circuit bindings
>> +
>> +Required properties:
>> + - compatible : Should be "ti,tps65912".
>> + - reg : Slave address or chip select number (I2C / SPI).
>> + - interrupt-parent : The parent interrupt controller.
>> + - interrupts : The interrupt line the device is connected to.
>> + - interrupt-controller : Marks the device node as an interrupt controller.
>> + - #interrupt-cells: the number of cells to describe an IRQ, this should be 2.
>
> s/the/The/
>

ACK

>> +     The first cell is the IRQ number.
>> +     The second cell is the flags, encoded as the trigger masks from
>> +     Documentation/devicetree/bindings/interrupts.txt
>
> No such file.
>

Yeah, this is an unchecked copy/paste on my part, other bindings did it this also, so
I just submitted a patch to fix them too.

>> +Optional nodes:
>> + - Regulators: Documentation/devicetree/bindings/regulator/tps65912-regulator.txt
>> + - GPIO: Documentation/devicetree/bindings/gpio/gpio-tps65912.txt.
>
> Better to use ../gpio, ../regulator, etc.
>
> "Regulators" and "GPIO" aren't valid node names.
>
> Please be more specific.
>

OK, I'll see if I can clear this up.

>> +Example:
>> +
>> +	pmic: tps65912@2d {
>> +		compatible = "ti,tps65912";
>> +		reg = <0x2d>;
>> +		interrupt-parent = <&gpio1>;
>> +		interrupts = <28 IRQ_TYPE_LEVEL_LOW>;
>> +		interrupt-controller;
>> +		#interrupt-cells = <2>;
>> +
>> +		dcdc1: regulator-dcdc1 {
>> +			compatible = "ti,tps65912-dcdc1";
>> +			regulator-name = "vdd_core";
>> +			regulator-min-microvolt = <912000>;
>> +			regulator-max-microvolt = <1144000>;
>> +			regulator-boot-on;
>> +			regulator-always-on;
>> +		};
>> +		...
>
> No need for this.
>

ACK

>> +		gpio4: tps65912_gpio {
>> +			compatible = "ti,tps65912-gpio";
>> +			gpio-controller;
>> +			#gpio-cells = <2>;
>> +		};
>> +	};
>> diff --git a/Documentation/devicetree/bindings/regulator/tps65912-regulator.txt b/Documentation/devicetree/bindings/regulator/tps65912-regulator.txt
>> new file mode 100644
>> index 0000000..a417ff7
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/regulator/tps65912-regulator.txt
>> @@ -0,0 +1,32 @@
>> +* TPS65912 regulator bindings
>> +
>> +Required properties:
>> + - compatible: Should be:
>> +   - "ti,tps65912-dcdc1" for DCDC1
>> +   - "ti,tps65912-dcdc2" for DCDC2
>> +   - "ti,tps65912-dcdc3" for DCDC3
>> +   - "ti,tps65912-dcdc4" for DCDC4
>> +   - "ti,tps65912-ldo1" for LDO1
>> +   - "ti,tps65912-ldo2" for LDO2
>> +   - "ti,tps65912-ldo3" for LDO3
>> +   - "ti,tps65912-ldo4" for LDO4
>> +   - "ti,tps65912-ldo5" for LDO5
>> +   - "ti,tps65912-ldo6" for LDO6
>> +   - "ti,tps65912-ldo7" for LDO7
>> +   - "ti,tps65912-ldo8" for LDO8
>> +   - "ti,tps65912-ldo9" for LDO9
>> +   - "ti,tps65912-ldo10" for LDO10
>> +
>> +Optional properties:
>> + - Any optional property defined in bindings/regulator/regulator.txt
>
> ../regulator/...
>

Not really sure what you mean here?

Thanks,
Andrew

>> +Example:
>> +
>> +	xyz: regulator@0 {
>> +		compatible = "ti,tps65912-dcdc1";
>> +		regulator-name = "vdd_core";
>> +		regulator-min-microvolt = <912000>;
>> +		regulator-max-microvolt = <1144000>;
>> +		regulator-boot-on;
>> +		regulator-always-on;
>> +	};
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1229813 — Re: [PATCH 1/3] Documentation: tps65912: Add DT bindings for the TPS65912 PMIC

On Mon, 21 Sep 2015, Andrew F. Davis wrote:
> On 09/19/2015 11:16 PM, Lee Jones wrote:
> >On Tue, 15 Sep 2015, Andrew F. Davis wrote:
> >
> >>The TPS65912 PMIC contains several regulators and a GPIO controller.
> >>Add bindings for the TPS65912 PMIC.
> >>
> >>Signed-off-by: Andrew F. Davis <afd@ti.com>
> >>---
> >>  .../devicetree/bindings/gpio/gpio-tps65912.txt     | 17 +++++++++
> >>  Documentation/devicetree/bindings/mfd/tps65912.txt | 43 ++++++++++++++++++++++
> >>  .../bindings/regulator/tps65912-regulator.txt      | 32 ++++++++++++++++
> >>  3 files changed, 92 insertions(+)
> >>  create mode 100644 Documentation/devicetree/bindings/gpio/gpio-tps65912.txt
> >>  create mode 100644 Documentation/devicetree/bindings/mfd/tps65912.txt
> >>  create mode 100644 Documentation/devicetree/bindings/regulator/tps65912-regulator.txt

[...]

> >>+Optional nodes:
> >>+ - Regulators: Documentation/devicetree/bindings/regulator/tps65912-regulator.txt
> >>+ - GPIO: Documentation/devicetree/bindings/gpio/gpio-tps65912.txt.
> >
> >Better to use ../gpio, ../regulator, etc.
> >
> >"Regulators" and "GPIO" aren't valid node names.
> >
> >Please be more specific.
> >
> 
> OK, I'll see if I can clear this up.

[...]

> >>+Optional properties:
> >>+ - Any optional property defined in bindings/regulator/regulator.txt
> >
> >../regulator/...
> >
> 
> Not really sure what you mean here?

Same as above.  Use "../regulator/regulator.txt" instead.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1225474 — [PATCH 3/3] tps65912: Cleanup TPS65912 subdevice configuration dependencies

The TPS65912 subdevices depend on the mfd core and not any particular
interface. The core is only enabled by enabling an interface so this
is safe.

Signed-off-by: Andrew F. Davis <afd@ti.com>
---
 drivers/gpio/Kconfig      | 2 +-
 drivers/regulator/Kconfig | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index b4fc9e4..82218fa 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -840,7 +840,7 @@ config GPIO_TPS65910
 
 config GPIO_TPS65912
 	tristate "TI TPS65912 GPIO"
-	depends on (MFD_TPS65912_I2C || MFD_TPS65912_SPI)
+	depends on MFD_TPS65912
 	help
 	  This driver supports TPS65912 gpio chip
 
diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index 64bccff..1dec96a 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -729,7 +729,7 @@ config REGULATOR_TPS65910
 
 config REGULATOR_TPS65912
 	tristate "TI TPS65912 Power regulator"
-	depends on (MFD_TPS65912_I2C || MFD_TPS65912_SPI)
+	depends on MFD_TPS65912
 	help
 	    This driver supports TPS65912 voltage regulator chip.
 
-- 
1.9.1

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

[toc] | [prev] | [next] | [standalone]

#1226433 — Re: [PATCH 3/3] tps65912: Cleanup TPS65912 subdevice configuration dependencies

[Multipart message — attachments visible in raw view] — view raw

On Tue, Sep 15, 2015 at 12:57:41PM -0500, Andrew F. Davis wrote:
> The TPS65912 subdevices depend on the mfd core and not any particular
> interface. The core is only enabled by enabling an interface so this
> is safe.

Acked-by: Mark Brown <broonie@kernel.org>

[toc] | [prev] | [next] | [standalone]

#1228798 — Re: [PATCH 2/3] mfd: tps65912: Rewrite driver adding DT support and using regmap

[Multipart message — attachments visible in raw view] — view raw

On Tue, Sep 15, 2015 at 12:57:40PM -0500, Andrew F. Davis wrote:
> The old driver does not support DT. Rewrite the driver adding DT support
> and use modern kernel features such as regmap and related helpers.
> 
> Signed-off-by: Andrew F. Davis <afd@ti.com>
> ---
>  drivers/gpio/gpio-tps65912.c           | 291 ++++++------
>  drivers/mfd/Kconfig                    |  20 +-
>  drivers/mfd/Makefile                   |   3 +-
>  drivers/mfd/tps65912-core.c            | 288 +++++-------
>  drivers/mfd/tps65912-i2c.c             | 233 ++++------
>  drivers/mfd/tps65912-irq.c             | 217 ---------
>  drivers/mfd/tps65912-spi.c             | 236 ++++------
>  drivers/regulator/tps65912-regulator.c | 783 ++++++++++-----------------------
>  include/linux/mfd/tps65912.h           | 256 +++++++----
>  9 files changed, 854 insertions(+), 1473 deletions(-)

It's not OK to have a single commit that rewrites multiple drivers over
many subsystems, that's really not something that can be sensibly
reviewed.  You should split this into a patch series which makes one
specific change at a time as covered in SubmittingPatches.  That will
allow the changes to be reviewed much more sensibly.

[toc] | [prev] | [next] | [standalone]

#1229560 — Re: [PATCH 2/3] mfd: tps65912: Rewrite driver adding DT support and using regmap

On 09/19/2015 01:40 PM, Mark Brown wrote:
> On Tue, Sep 15, 2015 at 12:57:40PM -0500, Andrew F. Davis wrote:
>> The old driver does not support DT. Rewrite the driver adding DT support
>> and use modern kernel features such as regmap and related helpers.
>>
>> Signed-off-by: Andrew F. Davis <afd@ti.com>
>> ---
>>   drivers/gpio/gpio-tps65912.c           | 291 ++++++------
>>   drivers/mfd/Kconfig                    |  20 +-
>>   drivers/mfd/Makefile                   |   3 +-
>>   drivers/mfd/tps65912-core.c            | 288 +++++-------
>>   drivers/mfd/tps65912-i2c.c             | 233 ++++------
>>   drivers/mfd/tps65912-irq.c             | 217 ---------
>>   drivers/mfd/tps65912-spi.c             | 236 ++++------
>>   drivers/regulator/tps65912-regulator.c | 783 ++++++++++-----------------------
>>   include/linux/mfd/tps65912.h           | 256 +++++++----
>>   9 files changed, 854 insertions(+), 1473 deletions(-)
>
> It's not OK to have a single commit that rewrites multiple drivers over
> many subsystems, that's really not something that can be sensibly
> reviewed.  You should split this into a patch series which makes one
> specific change at a time as covered in SubmittingPatches.  That will
> allow the changes to be reviewed much more sensibly.
>

I know this is hard to review, and so I would like to apologize in advance, but
the regulator and GPIO changes depend on the new driver core, as do the i2c/spi
components. I really don't know how to split this up without leaving some part
in a non-working state in-between patches (which I've heard is also not OK).
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1229684 — Re: [PATCH 2/3] mfd: tps65912: Rewrite driver adding DT support and using regmap

[Multipart message — attachments visible in raw view] — view raw

On Mon, Sep 21, 2015 at 11:42:13AM -0500, Andrew F. Davis wrote:

> I know this is hard to review, and so I would like to apologize in advance, but
> the regulator and GPIO changes depend on the new driver core, as do the i2c/spi
> components. I really don't know how to split this up without leaving some part
> in a non-working state in-between patches (which I've heard is also not OK).

So split that interface change between the MFD and the function drivers
out into a separate change - you've got way more than just an interface
change in here (and you've not enumerated the various things that are
being changed in the changelog).

[toc] | [prev] | [next] | [standalone]

#1229699 — Re: [PATCH 2/3] mfd: tps65912: Rewrite driver adding DT support and using regmap

On 09/21/2015 02:26 PM, Mark Brown wrote:
> On Mon, Sep 21, 2015 at 11:42:13AM -0500, Andrew F. Davis wrote:
>
>> I know this is hard to review, and so I would like to apologize in advance, but
>> the regulator and GPIO changes depend on the new driver core, as do the i2c/spi
>> components. I really don't know how to split this up without leaving some part
>> in a non-working state in-between patches (which I've heard is also not OK).
>
> So split that interface change between the MFD and the function drivers
> out into a separate change - you've got way more than just an interface
> change in here (and you've not enumerated the various things that are
> being changed in the changelog).
>

I'm not really "changing" anything, this is a ground-up re-write. This
fundamentally changes the way the MFD core and the function drivers interact,
I would consider this a whole new driver that just happens to replace an older
driver, not a bunch of incremental changes although I kept the same filenames.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1229705 — Re: [PATCH 2/3] mfd: tps65912: Rewrite driver adding DT support and using regmap

[Multipart message — attachments visible in raw view] — view raw

On Mon, Sep 21, 2015 at 02:46:50PM -0500, Andrew F. Davis wrote:
> On 09/21/2015 02:26 PM, Mark Brown wrote:

> >So split that interface change between the MFD and the function drivers
> >out into a separate change - you've got way more than just an interface
> >change in here (and you've not enumerated the various things that are
> >being changed in the changelog).

> I'm not really "changing" anything, this is a ground-up re-write. This
> fundamentally changes the way the MFD core and the function drivers interact,
> I would consider this a whole new driver that just happens to replace an older
> driver, not a bunch of incremental changes although I kept the same filenames.

So split out the deletion then add the new driver like you'd add any
other new driver.  If there were in tree users even that would be really
problematic but it does seem like we don't have any so perhaps people
will be OK with this.

[toc] | [prev] | [next] | [standalone]

#1229707 — Re: [PATCH 2/3] mfd: tps65912: Rewrite driver adding DT support and using regmap

On 09/21/2015 02:54 PM, Mark Brown wrote:
> On Mon, Sep 21, 2015 at 02:46:50PM -0500, Andrew F. Davis wrote:
>> On 09/21/2015 02:26 PM, Mark Brown wrote:
>
>>> So split that interface change between the MFD and the function drivers
>>> out into a separate change - you've got way more than just an interface
>>> change in here (and you've not enumerated the various things that are
>>> being changed in the changelog).
>
>> I'm not really "changing" anything, this is a ground-up re-write. This
>> fundamentally changes the way the MFD core and the function drivers interact,
>> I would consider this a whole new driver that just happens to replace an older
>> driver, not a bunch of incremental changes although I kept the same filenames.
>
> So split out the deletion then add the new driver like you'd add any
> other new driver.  If there were in tree users even that would be really
> problematic but it does seem like we don't have any so perhaps people
> will be OK with this.
>

OK, I'll split that in v2.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1229817 — Re: [PATCH 2/3] mfd: tps65912: Rewrite driver adding DT support and using regmap

On Mon, 21 Sep 2015, Andrew F. Davis wrote:
> On 09/19/2015 11:16 PM, Lee Jones wrote:
> >On Tue, 15 Sep 2015, Andrew F. Davis wrote:
> >
> >>The old driver does not support DT. Rewrite the driver adding DT support
> >>and use modern kernel features such as regmap and related helpers.
> >>
> >>Signed-off-by: Andrew F. Davis <afd@ti.com>
> >>---
> >>  drivers/gpio/gpio-tps65912.c           | 291 ++++++------
> >>  drivers/mfd/Kconfig                    |  20 +-
> >>  drivers/mfd/Makefile                   |   3 +-
> >>  drivers/mfd/tps65912-core.c            | 288 +++++-------
> >>  drivers/mfd/tps65912-i2c.c             | 233 ++++------
> >>  drivers/mfd/tps65912-irq.c             | 217 ---------
> >>  drivers/mfd/tps65912-spi.c             | 236 ++++------
> >>  drivers/regulator/tps65912-regulator.c | 783 ++++++++++-----------------------
> >>  include/linux/mfd/tps65912.h           | 256 +++++++----
> >>  9 files changed, 854 insertions(+), 1473 deletions(-)
> >>  rewrite drivers/gpio/gpio-tps65912.c (68%)
> >>  rewrite drivers/mfd/tps65912-core.c (96%)
> >>  rewrite drivers/mfd/tps65912-i2c.c (92%)
> >>  delete mode 100644 drivers/mfd/tps65912-irq.c
> >>  rewrite drivers/mfd/tps65912-spi.c (91%)
> >>  rewrite drivers/regulator/tps65912-regulator.c (94%)
> >
> >Is there really no other way to split this up?
> 
> I don't think so, not without breaking stuff, all the files are strongly
> connected.

I believe you've already had this conversation with Mark, so I'll not
labour it.

[...]

> >>+#include <linux/interrupt.h>
> >>+#include <linux/module.h>
> >>+#include <linux/of_device.h>
> >>+
> >
> >No need for the '\n'.
> >
> 
> Is it prohibited? It seems a lot over files do it this way, I personally
> think it makes it cleaner to see what headers are driver local. If you
> really don't like it have have no problem removing it though.

It's not a blocker.

> >>+#include <linux/mfd/tps65912.h>
> >>+
> >>+#define TPS65912_IRQ(_name, _reg, _offset)	\
> >>+	[TPS65912_IRQ_ ## _name] = {	\
> >>+		.mask = TPS65912_ ## _reg ## _ ## _name,	\
> >>+		.reg_offset = _offset,	\
> >>+	}
> >
> >If you find this useful, then others will too.
> >
> >Please submit this to Mark Brown.
> 
> This is a really horrible macro hack I made so I didn't have to type
> out the whole register name each time, I'm not sure anyone else
> could use this, a better solution for most would be to use less
> verbose #defines for register names.

http://www.gossamer-threads.com/lists/linux/kernel/2261088

[...]

> >>+	.name = "tps65912",
> >>+	.irqs = tps65912_irqs,
> >>+	.num_irqs = ARRAY_SIZE(tps65912_irqs),
> >>+
> >>+	.num_regs = 4,
> >
> >Why do you have 2 num_regs entries?
> 
> Not sure what you mean, the other is num_irqs?

That's what no sleep on a plane gets you.

Please ignore.

[...]

> >>+#include <linux/mfd/tps65912.h>
> >>+
> >>+static const struct of_device_id tps65912_i2c_of_match_table[] = {
> >>+	{ .compatible = "ti,tps65912", },
> >>+	{ /*sentinel*/ }
> >
> >No need for this comment.  We know what { } does.
> 
> We do, but I didn't when I first saw this kind of thing, everything is
> obvious to someone, I'm not sure trying to keep the kernel as comment
> bare as it is is a good idea. Plus, how many chances do you get to
> use the word "sentinel" :). But I'll remove it if you have a strong
> opposition to it.

Again, it's not a blocker, but I believe there are enough of these now
and they have been around long enough that any non-noob will know what
they mean.  But granted, "sentinel" is a cool word.  Either remove it
completely, or at least conform to the Kernel's single line
commentating standards (HINT: Whitespace).

> >>+};
> >>+
> >>+static int tps65912_i2c_probe(struct i2c_client *client,
> >>+			      const struct i2c_device_id *ids)
> >>+{
> >>+	struct tps65912 *tps;
> >>+	const struct of_device_id *match;
> >>+
> >>+	match = of_match_device(tps65912_i2c_of_match_table, &client->dev);
> >>+	if (!match) {
> >>+		dev_err(&client->dev, "Failed to find matching DT id\n");
> >>+		return -EINVAL;
> >>+	}
> >
> >Why are you matching?
> >
> 
> It looks like other drivers do this so they will fail early if they were
> not instantiated with DT. Here we don't need the match, so I'll just
> drop this check.

They should not be doing that either.

> >>+	tps = devm_kzalloc(&client->dev, sizeof(*tps), GFP_KERNEL);
> >>+	if (!tps)
> >>+		return -ENOMEM;
> >>+
> >>+	i2c_set_clientdata(client, tps);
> >>+	tps->dev = &client->dev;
> >>+	tps->irq = client->irq;
> >>+
> >>+	tps->regmap = devm_regmap_init_i2c(client, &tps65912_regmap_config);
> >>+	if (IS_ERR(tps->regmap)) {
> >>+		int ret = PTR_ERR(tps->regmap);
> >
> >Neater just to declare 'int ret' up the top like we always do.
> 
> Linus has decreed we should not mix code and declerations outside of C89,
> and so I will not, but ret is at the top of its scope and so is conforment
> to C89. Moving it further up might be overly overly pedantic.
> 
> But if you think it would be really neater I can move it :)

Again, it's not a 'standard' per say and not a blocker, but it would
be conforming to the way we normally do things.

[...]

> >>+	{ "tps65912", TPS65912 },
> >
> >This doesn't appear to be used?
> >
> 
> The TPS65912 part is not, but it may someday get an extra device ID,
> some use just 0 for the second field, but I have TPS65912 defined so
> might as well use it.

I'm not into adding code for 'may some day's.  If it's not used,
please remove it.

> >>+	{ /*sentinel*/ },
> >
> >As before.
> >
> >>+};
> >>+MODULE_DEVICE_TABLE(i2c, tps65912_i2c_id_table);
> >>+
> >>+static struct i2c_driver tps65912_i2c_driver = {
> >>+	.driver		= {
> >>+		.name	= "tps65912",
> >>+		.of_match_table = tps65912_i2c_of_match_table,
> >>+	},
> >>+	.probe		= tps65912_i2c_probe,
> >>+	.remove		= tps65912_i2c_remove,
> >>+	.id_table       = tps65912_i2c_id_table,
> >>+};
> >>+
> >>+module_i2c_driver(tps65912_i2c_driver);
> >>+
> >>+MODULE_AUTHOR("Andrew F. Davis <afd@ti.com>");
> >>+MODULE_DESCRIPTION("TPS65912x I2C Interface Driver");
> >>+MODULE_LICENSE("GPL v2");
> >>diff --git a/drivers/mfd/tps65912-irq.c b/drivers/mfd/tps65912-irq.c
> >>deleted file mode 100644
> >>index db2c29c..0000000
> >>--- a/drivers/mfd/tps65912-irq.c
> >>+++ /dev/null
> >>@@ -1,217 +0,0 @@
> >>-/*
> >>- * tps65912-irq.c  --  TI TPS6591x
> >>- *
> >>- * Copyright 2011 Texas Instruments Inc.
> >>- *
> >>- * Author: Margarita Olaya <magi@slimlogic.co.uk>
> >>- *
> >>- *  This program is free software; you can redistribute it and/or modify it
> >>- *  under  the terms of the GNU General  Public License as published by the
> >>- *  Free Software Foundation;  either version 2 of the License, or (at your
> >>- *  option) any later version.
> >>- *
> >>- * This driver is based on wm8350 implementation.
> >>- */
> >>-
> >>-#include <linux/kernel.h>
> >>-#include <linux/module.h>
> >>-#include <linux/bug.h>
> >>-#include <linux/device.h>
> >>-#include <linux/interrupt.h>
> >>-#include <linux/irq.h>
> >>-#include <linux/gpio.h>
> >>-#include <linux/mfd/tps65912.h>
> >>-
> >>-static inline int irq_to_tps65912_irq(struct tps65912 *tps65912,
> >>-							int irq)
> >>-{
> >>-	return irq - tps65912->irq_base;
> >>-}
> >>-
> >>-/*
> >>- * This is a threaded IRQ handler so can access I2C/SPI.  Since the
> >>- * IRQ handler explicitly clears the IRQ it handles the IRQ line
> >>- * will be reasserted and the physical IRQ will be handled again if
> >>- * another interrupt is asserted while we run - in the normal course
> >>- * of events this is a rare occurrence so we save I2C/SPI reads. We're
> >>- * also assuming that it's rare to get lots of interrupts firing
> >>- * simultaneously so try to minimise I/O.
> >>- */
> >>-static irqreturn_t tps65912_irq(int irq, void *irq_data)
> >>-{
> >>-	struct tps65912 *tps65912 = irq_data;
> >>-	u32 irq_sts;
> >>-	u32 irq_mask;
> >>-	u8 reg;
> >>-	int i;
> >>-
> >>-
> >>-	tps65912->read(tps65912, TPS65912_INT_STS, 1, &reg);
> >>-	irq_sts = reg;
> >>-	tps65912->read(tps65912, TPS65912_INT_STS2, 1, &reg);
> >>-	irq_sts |= reg << 8;
> >>-	tps65912->read(tps65912, TPS65912_INT_STS3, 1, &reg);
> >>-	irq_sts |= reg << 16;
> >>-	tps65912->read(tps65912, TPS65912_INT_STS4, 1, &reg);
> >>-	irq_sts |= reg << 24;
> >>-
> >>-	tps65912->read(tps65912, TPS65912_INT_MSK, 1, &reg);
> >>-	irq_mask = reg;
> >>-	tps65912->read(tps65912, TPS65912_INT_MSK2, 1, &reg);
> >>-	irq_mask |= reg << 8;
> >>-	tps65912->read(tps65912, TPS65912_INT_MSK3, 1, &reg);
> >>-	irq_mask |= reg << 16;
> >>-	tps65912->read(tps65912, TPS65912_INT_MSK4, 1, &reg);
> >>-	irq_mask |= reg << 24;
> >>-
> >>-	irq_sts &= ~irq_mask;
> >>-	if (!irq_sts)
> >>-		return IRQ_NONE;
> >>-
> >>-	for (i = 0; i < tps65912->irq_num; i++) {
> >>-		if (!(irq_sts & (1 << i)))
> >>-			continue;
> >>-
> >>-		handle_nested_irq(tps65912->irq_base + i);
> >>-	}
> >>-
> >>-	/* Write the STS register back to clear IRQs we handled */
> >>-	reg = irq_sts & 0xFF;
> >>-	irq_sts >>= 8;
> >>-	if (reg)
> >>-		tps65912->write(tps65912, TPS65912_INT_STS, 1, &reg);
> >>-	reg = irq_sts & 0xFF;
> >>-	irq_sts >>= 8;
> >>-	if (reg)
> >>-		tps65912->write(tps65912, TPS65912_INT_STS2, 1, &reg);
> >>-	reg = irq_sts & 0xFF;
> >>-	irq_sts >>= 8;
> >>-	if (reg)
> >>-		tps65912->write(tps65912, TPS65912_INT_STS3, 1, &reg);
> >>-	reg = irq_sts & 0xFF;
> >>-	if (reg)
> >>-		tps65912->write(tps65912, TPS65912_INT_STS4, 1, &reg);
> >>-
> >>-	return IRQ_HANDLED;
> >>-}
> >>-
> >>-static void tps65912_irq_lock(struct irq_data *data)
> >>-{
> >>-	struct tps65912 *tps65912 = irq_data_get_irq_chip_data(data);
> >>-
> >>-	mutex_lock(&tps65912->irq_lock);
> >>-}
> >>-
> >>-static void tps65912_irq_sync_unlock(struct irq_data *data)
> >>-{
> >>-	struct tps65912 *tps65912 = irq_data_get_irq_chip_data(data);
> >>-	u32 reg_mask;
> >>-	u8 reg;
> >>-
> >>-	tps65912->read(tps65912, TPS65912_INT_MSK, 1, &reg);
> >>-	reg_mask = reg;
> >>-	tps65912->read(tps65912, TPS65912_INT_MSK2, 1, &reg);
> >>-	reg_mask |= reg << 8;
> >>-	tps65912->read(tps65912, TPS65912_INT_MSK3, 1, &reg);
> >>-	reg_mask |= reg << 16;
> >>-	tps65912->read(tps65912, TPS65912_INT_MSK4, 1, &reg);
> >>-	reg_mask |= reg << 24;
> >>-
> >>-	if (tps65912->irq_mask != reg_mask) {
> >>-		reg = tps65912->irq_mask & 0xFF;
> >>-		tps65912->write(tps65912, TPS65912_INT_MSK, 1, &reg);
> >>-		reg = tps65912->irq_mask >> 8 & 0xFF;
> >>-		tps65912->write(tps65912, TPS65912_INT_MSK2, 1, &reg);
> >>-		reg = tps65912->irq_mask >> 16 & 0xFF;
> >>-		tps65912->write(tps65912, TPS65912_INT_MSK3, 1, &reg);
> >>-		reg = tps65912->irq_mask >> 24 & 0xFF;
> >>-		tps65912->write(tps65912, TPS65912_INT_MSK4, 1, &reg);
> >>-	}
> >>-
> >>-	mutex_unlock(&tps65912->irq_lock);
> >>-}
> >>-
> >>-static void tps65912_irq_enable(struct irq_data *data)
> >>-{
> >>-	struct tps65912 *tps65912 = irq_data_get_irq_chip_data(data);
> >>-
> >>-	tps65912->irq_mask &= ~(1 << irq_to_tps65912_irq(tps65912, data->irq));
> >>-}
> >>-
> >>-static void tps65912_irq_disable(struct irq_data *data)
> >>-{
> >>-	struct tps65912 *tps65912 = irq_data_get_irq_chip_data(data);
> >>-
> >>-	tps65912->irq_mask |= (1 << irq_to_tps65912_irq(tps65912, data->irq));
> >>-}
> >>-
> >>-static struct irq_chip tps65912_irq_chip = {
> >>-	.name = "tps65912",
> >>-	.irq_bus_lock = tps65912_irq_lock,
> >>-	.irq_bus_sync_unlock = tps65912_irq_sync_unlock,
> >>-	.irq_disable = tps65912_irq_disable,
> >>-	.irq_enable = tps65912_irq_enable,
> >>-};
> >>-
> >>-int tps65912_irq_init(struct tps65912 *tps65912, int irq,
> >>-			    struct tps65912_platform_data *pdata)
> >>-{
> >>-	int ret, cur_irq;
> >>-	int flags = IRQF_ONESHOT;
> >>-	u8 reg;
> >>-
> >>-	if (!irq) {
> >>-		dev_warn(tps65912->dev, "No interrupt support, no core IRQ\n");
> >>-		return 0;
> >>-	}
> >>-
> >>-	if (!pdata || !pdata->irq_base) {
> >>-		dev_warn(tps65912->dev, "No interrupt support, no IRQ base\n");
> >>-		return 0;
> >>-	}
> >>-
> >>-	/* Clear unattended interrupts */
> >>-	tps65912->read(tps65912, TPS65912_INT_STS, 1, &reg);
> >>-	tps65912->write(tps65912, TPS65912_INT_STS, 1, &reg);
> >>-	tps65912->read(tps65912, TPS65912_INT_STS2, 1, &reg);
> >>-	tps65912->write(tps65912, TPS65912_INT_STS2, 1, &reg);
> >>-	tps65912->read(tps65912, TPS65912_INT_STS3, 1, &reg);
> >>-	tps65912->write(tps65912, TPS65912_INT_STS3, 1, &reg);
> >>-	tps65912->read(tps65912, TPS65912_INT_STS4, 1, &reg);
> >>-	tps65912->write(tps65912, TPS65912_INT_STS4, 1, &reg);
> >>-
> >>-	/* Mask top level interrupts */
> >>-	tps65912->irq_mask = 0xFFFFFFFF;
> >>-
> >>-	mutex_init(&tps65912->irq_lock);
> >>-	tps65912->chip_irq = irq;
> >>-	tps65912->irq_base = pdata->irq_base;
> >>-
> >>-	tps65912->irq_num = TPS65912_NUM_IRQ;
> >>-
> >>-	/* Register with genirq */
> >>-	for (cur_irq = tps65912->irq_base;
> >>-	     cur_irq < tps65912->irq_num + tps65912->irq_base;
> >>-	     cur_irq++) {
> >>-		irq_set_chip_data(cur_irq, tps65912);
> >>-		irq_set_chip_and_handler(cur_irq, &tps65912_irq_chip,
> >>-					 handle_edge_irq);
> >>-		irq_set_nested_thread(cur_irq, 1);
> >>-		irq_clear_status_flags(cur_irq, IRQ_NOREQUEST | IRQ_NOPROBE);
> >>-	}
> >>-
> >>-	ret = request_threaded_irq(irq, NULL, tps65912_irq, flags,
> >>-				   "tps65912", tps65912);
> >>-
> >>-	irq_set_irq_type(irq, IRQ_TYPE_LEVEL_LOW);
> >>-	if (ret != 0)
> >>-		dev_err(tps65912->dev, "Failed to request IRQ: %d\n", ret);
> >>-
> >>-	return ret;
> >>-}
> >>-
> >>-int tps65912_irq_exit(struct tps65912 *tps65912)
> >>-{
> >>-	free_irq(tps65912->chip_irq, tps65912);
> >>-	return 0;
> >>-}
> >>diff --git a/drivers/mfd/tps65912-spi.c b/drivers/mfd/tps65912-spi.c
> >>dissimilarity index 91%
> >>index de60ad9..d6ab929 100644
> >>--- a/drivers/mfd/tps65912-spi.c
> >>+++ b/drivers/mfd/tps65912-spi.c
> >>@@ -1,141 +1,95 @@
> >>-/*
> >>- * tps65912-spi.c  --  SPI access for TI TPS65912x PMIC
> >>- *
> >>- * Copyright 2011 Texas Instruments Inc.
> >>- *
> >>- * Author: Margarita Olaya Cabrera <magi@slimlogic.co.uk>
> >>- *
> >>- *  This program is free software; you can redistribute it and/or modify it
> >>- *  under  the terms of the GNU General  Public License as published by the
> >>- *  Free Software Foundation;  either version 2 of the License, or (at your
> >>- *  option) any later version.
> >>- *
> >>- *  This driver is based on wm8350 implementation.
> >>- */
> >>-
> >>-#include <linux/module.h>
> >>-#include <linux/moduleparam.h>
> >>-#include <linux/init.h>
> >>-#include <linux/slab.h>
> >>-#include <linux/gpio.h>
> >>-#include <linux/spi/spi.h>
> >>-#include <linux/mfd/core.h>
> >>-#include <linux/mfd/tps65912.h>
> >>-
> >>-static int tps65912_spi_write(struct tps65912 *tps65912, u8 addr,
> >>-							int bytes, void *src)
> >>-{
> >>-	struct spi_device *spi = tps65912->control_data;
> >>-	u8 *data = (u8 *) src;
> >>-	int ret;
> >>-	/* bit 23 is the read/write bit */
> >>-	unsigned long spi_data = 1 << 23 | addr << 15 | *data;
> >>-	struct spi_transfer xfer;
> >>-	struct spi_message msg;
> >>-	u32 tx_buf;
> >>-
> >>-	tx_buf = spi_data;
> >>-
> >>-	xfer.tx_buf	= &tx_buf;
> >>-	xfer.rx_buf	= NULL;
> >>-	xfer.len	= sizeof(unsigned long);
> >>-	xfer.bits_per_word = 24;
> >>-
> >>-	spi_message_init(&msg);
> >>-	spi_message_add_tail(&xfer, &msg);
> >>-
> >>-	ret = spi_sync(spi, &msg);
> >>-	return ret;
> >>-}
> >>-
> >>-static int tps65912_spi_read(struct tps65912 *tps65912, u8 addr,
> >>-							int bytes, void *dest)
> >>-{
> >>-	struct spi_device *spi = tps65912->control_data;
> >>-	/* bit 23 is the read/write bit */
> >>-	unsigned long spi_data = 0 << 23 | addr << 15;
> >>-	struct spi_transfer xfer;
> >>-	struct spi_message msg;
> >>-	int ret;
> >>-	u8 *data = (u8 *) dest;
> >>-	u32 tx_buf, rx_buf;
> >>-
> >>-	tx_buf = spi_data;
> >>-	rx_buf = 0;
> >>-
> >>-	xfer.tx_buf	= &tx_buf;
> >>-	xfer.rx_buf	= &rx_buf;
> >>-	xfer.len	= sizeof(unsigned long);
> >>-	xfer.bits_per_word = 24;
> >>-
> >>-	spi_message_init(&msg);
> >>-	spi_message_add_tail(&xfer, &msg);
> >>-
> >>-	if (spi == NULL)
> >>-		return 0;
> >>-
> >>-	ret = spi_sync(spi, &msg);
> >>-	if (ret == 0)
> >>-		*data = (u8) (rx_buf & 0xFF);
> >>-	return ret;
> >>-}
> >>-
> >>-static int tps65912_spi_probe(struct spi_device *spi)
> >>-{
> >>-	struct tps65912 *tps65912;
> >>-
> >>-	tps65912 = devm_kzalloc(&spi->dev,
> >>-				sizeof(struct tps65912), GFP_KERNEL);
> >>-	if (tps65912 == NULL)
> >>-		return -ENOMEM;
> >>-
> >>-	tps65912->dev = &spi->dev;
> >>-	tps65912->control_data = spi;
> >>-	tps65912->read = tps65912_spi_read;
> >>-	tps65912->write = tps65912_spi_write;
> >>-
> >>-	spi_set_drvdata(spi, tps65912);
> >>-
> >>-	return tps65912_device_init(tps65912);
> >>-}
> >>-
> >>-static int tps65912_spi_remove(struct spi_device *spi)
> >>-{
> >>-	struct tps65912 *tps65912 = spi_get_drvdata(spi);
> >>-
> >>-	tps65912_device_exit(tps65912);
> >>-
> >>-	return 0;
> >>-}
> >>-
> >>-static struct spi_driver tps65912_spi_driver = {
> >>-	.driver = {
> >>-		.name = "tps65912",
> >>-		.owner = THIS_MODULE,
> >>-	},
> >>-	.probe	= tps65912_spi_probe,
> >>-	.remove = tps65912_spi_remove,
> >>-};
> >>-
> >>-static int __init tps65912_spi_init(void)
> >>-{
> >>-	int ret;
> >>-
> >>-	ret = spi_register_driver(&tps65912_spi_driver);
> >>-	if (ret != 0)
> >>-		pr_err("Failed to register TPS65912 SPI driver: %d\n", ret);
> >>-
> >>-	return 0;
> >>-}
> >>-/* init early so consumer devices can complete system boot */
> >>-subsys_initcall(tps65912_spi_init);
> >>-
> >>-static void __exit tps65912_spi_exit(void)
> >>-{
> >>-	spi_unregister_driver(&tps65912_spi_driver);
> >>-}
> >>-module_exit(tps65912_spi_exit);
> >>-
> >>-MODULE_AUTHOR("Margarita Olaya	<magi@slimlogic.co.uk>");
> >>-MODULE_DESCRIPTION("SPI support for TPS65912 chip family mfd");
> >>-MODULE_LICENSE("GPL");
> >>+/*
> >>+ * tps65912-spi.c -- SPI access driver for TI TPS65912x PMIC
> >
> >All the same comments as the other files -- I'll not labour them.
> >
> >Same goes for the rest of the file.
> >
> 
> ACK
> 
> >>+ * Copyright (C) 2015 Texas Instruments Incorporated - http://www.ti.com/
> >>+ *
> >>+ * Author: Andrew F. Davis <afd@ti.com>
> >>+ *
> >>+ * This program is free software; you can redistribute it and/or
> >>+ * modify it under the terms of the GNU General Public License version 2 as
> >>+ * published by the Free Software Foundation.
> >>+ *
> >>+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any
> >>+ * kind, whether expressed or implied; without even the implied warranty
> >>+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> >>+ * GNU General Public License version 2 for more details.
> >>+ *
> >>+ * Based on the TPS65218 driver and the previous TPS65912 driver by
> >>+ * Margarita Olaya Cabrera <magi@slimlogic.co.uk>
> >>+ */
> >>+
> >>+#include <linux/spi/spi.h>
> >>+#include <linux/module.h>
> >>+#include <linux/of_device.h>
> >>+#include <linux/regmap.h>
> >
> >Alphabetical.
> >
> 
> ACK
> 
> >>+#include <linux/mfd/tps65912.h>
> >>+
> >>+static const struct of_device_id tps65912_spi_of_match_table[] = {
> >>+	{ .compatible = "ti,tps65912", },
> >>+	{ /*sentinel*/ }
> >>+};
> >>+
> >>+static int tps65912_spi_probe(struct spi_device *spi)
> >>+{
> >>+	struct tps65912 *tps;
> >>+	const struct of_device_id *match;
> >>+
> >>+	match = of_match_device(tps65912_spi_of_match_table, &spi->dev);
> >>+	if (!match) {
> >>+		dev_err(&spi->dev, "Failed to find matching DT id\n");
> >>+		return -EINVAL;
> >>+	}
> >
> >Why are you matching?
> >
> 
> Same response as above.
> 
> >>+	tps = devm_kzalloc(&spi->dev, sizeof(*tps), GFP_KERNEL);
> >>+	if (!tps)
> >>+		return -ENOMEM;
> >>+
> >>+	spi_set_drvdata(spi, tps);
> >>+	tps->dev = &spi->dev;
> >>+	tps->irq = spi->irq;
> >>+
> >>+	tps->regmap = devm_regmap_init_spi(spi, &tps65912_regmap_config);
> >>+	if (IS_ERR(tps->regmap)) {
> >>+		int ret = PTR_ERR(tps->regmap);
> >>+
> >>+		dev_err(tps->dev, "Failed to allocate register map: %d\n",
> >>+			ret);
> >>+
> >>+		return ret;
> >>+	}
> >>+
> >>+	return tps65912_device_init(tps);
> >>+}
> >>+
> >>+static int tps65912_spi_remove(struct spi_device *client)
> >>+{
> >>+	struct tps65912 *tps = spi_get_drvdata(client);
> >>+
> >>+	tps65912_device_exit(tps);
> >>+
> >>+	return 0;
> >>+}
> >>+
> >>+static const struct spi_device_id tps65912_spi_id_table[] = {
> >>+	{ "tps65912", TPS65912 },
> >>+	{ /*sentinel*/ },
> >>+};
> >>+MODULE_DEVICE_TABLE(spi, tps65912_spi_id_table);
> >>+
> >>+static struct spi_driver tps65912_spi_driver = {
> >>+	.driver		= {
> >>+		.name	= "tps65912",
> >>+		.owner	= THIS_MODULE,
> >
> >Remove this line.
> >
> 
> I wasn't sure about this one, all the SPI drivers I looked at still have
> this, but if you are sure this is not needed then I'll remove it.
> 
> (also if it is not needed someone could probably remove this from all
> these drivers like 816c44c36901 did in power)

Yes, they should all be removed.  It's taken care of elsewhere.  Some
effort has been put in and I believe a Coccinelle script even exists
now.

Fill your boots.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web