Devicetree
 help / color / mirror / Atom feed
* Re: [PATCHv2 net-next 11/16] net: mvpp2: handle misc PPv2.1/PPv2.2 differences
From: Russell King - ARM Linux @ 2017-01-07 20:10 UTC (permalink / raw)
  To: Thomas Petazzoni
  Cc: Mark Rutland, devicetree-u79uwXL29TY76Z2rM5mHXA, Yehuda Yitschak,
	Jason Cooper, Pawel Moll, Ian Campbell,
	netdev-u79uwXL29TY76Z2rM5mHXA, Hanna Hawa, Nadav Haklai,
	Rob Herring, Andrew Lunn, Kumar Gala, Gregory Clement,
	Stefan Chulski, Marcin Wojtas, David S. Miller,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	Sebastian Hesselbarth
In-Reply-To: <20170107093834.GJ14217-l+eeeJia6m9URfEZ8mYm6t73F7V6hmMc@public.gmane.org>

On Sat, Jan 07, 2017 at 09:38:34AM +0000, Russell King - ARM Linux wrote:
> On Wed, Dec 28, 2016 at 05:46:27PM +0100, Thomas Petazzoni wrote:
> > @@ -6511,7 +6515,9 @@ static int mvpp2_port_probe(struct platform_device *pdev,
> >  		dev_err(&pdev->dev, "failed to init port %d\n", id);
> >  		goto err_free_stats;
> >  	}
> > -	mvpp2_port_power_up(port);
> > +
> > +	if (priv->hw_version == MVPP21)
> > +		mvpp21_port_power_up(port);
> 
> This has the side effect that nothing clears the port reset bit in the
> GMAC, which means there's no hope of the interface working - with the
> reset bit set, the port is well and truely held in "link down" state.
> 
> In any case, the GMAC part is much the same as mvneta, and I think
> that code should be shared rather than writing new versions of it.
> There are some subtle differences between neta, pp2.1 and pp2.2, but
> it's entirely doable (I have an implementation here as I wasn't going
> to duplicate this code for my phylink conversion.)

In addition to comphy configuration and the above, I also need the
following to have working SGMII.  The change of MACMODE is needed
because uboot has configured the port for 10Gbase-R mode (it has a
10G PHY on it, but the PHY switches to SGMII in <10G modes.)  The
GMAC control register 4 is needed to properly configure for SGMII
mode.  I also included RGMII mode as well in there, as I expect you'd
need it to have GMAC properly configured for RGMII.

With this in place (and the other bits mentioned above), I can ping
the clearfog switch on the other end of eth0's cable:

# ping6 -I eth0 fe80::250:43ff:fe02:302
PING fe80::250:43ff:fe02:302(fe80::250:43ff:fe02:302) from fe80::200:ff:fe00:1 eth0: 56 data bytes
64 bytes from fe80::250:43ff:fe02:302: icmp_seq=1 ttl=64 time=0.297 ms

diff --git a/drivers/net/ethernet/marvell/mvpp2.c b/drivers/net/ethernet/marvell/mvpp2.c
index bc97eebf7eee..4b6ec6213e9c 100644
--- a/drivers/net/ethernet/marvell/mvpp2.c
+++ b/drivers/net/ethernet/marvell/mvpp2.c
@@ -345,7 +345,17 @@
 #define      MVPP2_GMAC_TX_FIFO_MIN_TH_ALL_MASK	0x1fc0
 #define      MVPP2_GMAC_TX_FIFO_MIN_TH_MASK(v)	(((v) << 6) & \
 					MVPP2_GMAC_TX_FIFO_MIN_TH_ALL_MASK)
+#define MVPP22_GMAC_CTRL_4_REG			0x90
+#define      MVPP22_CTRL4_EXT_PIN_GMII_SEL	BIT(0)
+#define      MVPP22_CTRL4_DP_CLK_SEL		BIT(5)
+#define      MVPP22_CTRL4_SYNC_BYPASS		BIT(6)
+#define      MVPP22_CTRL4_QSGMII_BYPASS_ACTIVE	BIT(7)
+
+#define MVPP22_XLG_CTRL3_REG			0x11c
+#define      MVPP22_XLG_CTRL3_MACMODESELECT_MASK	(7 << 13)
+#define      MVPP22_XLG_CTRL3_MACMODESELECT_GMAC	(0 << 13)
 
+/* offsets from iface_base */
 #define MVPP22_SMI_MISC_CFG_REG			0x2a204
 #define      MVPP22_SMI_POLLING_EN		BIT(10)
 
@@ -4171,6 +4181,23 @@ static void mvpp2_port_mii_set(struct mvpp2_port *port)
 {
 	u32 val;
 
+	if (port->priv->hw_version == MVPP22) {
+		val = readl(port->base + MVPP22_XLG_CTRL3_REG);
+		val &= ~MVPP22_XLG_CTRL3_MACMODESELECT_MASK;
+		val |= MVPP22_XLG_CTRL3_MACMODESELECT_GMAC;
+		writel(val, port->base + MVPP22_XLG_CTRL3_REG);
+
+		val = readl(port->base + MVPP22_GMAC_CTRL_4_REG);
+		if (port->phy_interface == PHY_INTERFACE_MODE_RGMII)
+			val |= MVPP22_CTRL4_EXT_PIN_GMII_SEL;
+		else
+			val &= ~MVPP22_CTRL4_EXT_PIN_GMII_SEL;
+		val &= ~MVPP22_CTRL4_DP_CLK_SEL;
+		val |= MVPP22_CTRL4_SYNC_BYPASS;
+		val |= MVPP22_CTRL4_QSGMII_BYPASS_ACTIVE;
+		writel(val, port->base + MVPP22_GMAC_CTRL_4_REG);
+	}
+
 	val = readl(port->base + MVPP2_GMAC_CTRL_2_REG);
 
 	switch (port->phy_interface) {


-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply related

* [PATCH v5 2/2] ARM: dts: vf610-zii-dev: Add .dts file for rev. C
From: Andrey Smirnov @ 2017-01-07 20:06 UTC (permalink / raw)
  To: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
  Cc: Andrey Smirnov, Shawn Guo, Rob Herring, Mark Rutland,
	Russell King, Sascha Hauer, Stefan Agner,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA, andrew-g2DYL2Zd6BY,
	Vivien Didelot, Nikita Yushchenko, cphealy-Re5JQEeQqe8AvxtiuMwx3w
In-Reply-To: <20170107200654.26056-1-andrew.smirnov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

Add .dts file for rev. C of the board by factoring out commonalities
into a shared include file (vf610-zii-dev-rev-b-c.dtsi) and deriving
revision specific file from it (vf610-zii-dev-rev-b.dts and
vf610-zii-dev-reb-c.dts).

Cc: Shawn Guo <shawnguo-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>
Cc: Russell King <linux-I+IVW8TIWO2tmTQ+vhA3Yw@public.gmane.org>
Cc: Sascha Hauer <kernel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
Cc: Stefan Agner <stefan-XLVq0VzYD2Y@public.gmane.org>
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: andrew-g2DYL2Zd6BY@public.gmane.org
Cc: Vivien Didelot <vivien.didelot-4ysUXcep3aM1wj+D4I0NRVaTQe2KTcn/@public.gmane.org>
Cc: Nikita Yushchenko <nikita.yoush-M4DtvfQ/ZS1MRgGoP+s0PdBPR1lH4CV8@public.gmane.org>
Cc: cphealy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org
Signed-off-by: Andrey Smirnov <andrew.smirnov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---

Changes since v3:

        - Added node for AT86RF233 chip on SPI0

Changes since v4:

	- Renamed switch0@0 and switch1@0 to just switch@0 (switch ID
          is still retained in those nodes' labels)

	- Added spacing between children and properties of nodes


 arch/arm/boot/dts/Makefile                |   3 +-
 arch/arm/boot/dts/vf610-zii-dev-rev-b.dts | 315 +----------------------
 arch/arm/boot/dts/vf610-zii-dev-rev-c.dts | 414 ++++++++++++++++++++++++++++++
 arch/arm/boot/dts/vf610-zii-dev.dtsi      | 383 +++++++++++++++++++++++++++
 4 files changed, 813 insertions(+), 302 deletions(-)
 create mode 100644 arch/arm/boot/dts/vf610-zii-dev-rev-c.dts
 create mode 100644 arch/arm/boot/dts/vf610-zii-dev.dtsi

diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
index befcd26..9f0d2a1 100644
--- a/arch/arm/boot/dts/Makefile
+++ b/arch/arm/boot/dts/Makefile
@@ -442,7 +442,8 @@ dtb-$(CONFIG_SOC_VF610) += \
 	vf610-cosmic.dtb \
 	vf610m4-cosmic.dtb \
 	vf610-twr.dtb \
-	vf610-zii-dev-rev-b.dtb
+	vf610-zii-dev-rev-b.dtb \
+	vf610-zii-dev-rev-c.dtb
 dtb-$(CONFIG_ARCH_MXS) += \
 	imx23-evk.dtb \
 	imx23-olinuxino.dtb \
diff --git a/arch/arm/boot/dts/vf610-zii-dev-rev-b.dts b/arch/arm/boot/dts/vf610-zii-dev-rev-b.dts
index 2210811..a6e9275 100644
--- a/arch/arm/boot/dts/vf610-zii-dev-rev-b.dts
+++ b/arch/arm/boot/dts/vf610-zii-dev-rev-b.dts
@@ -43,32 +43,12 @@
  */
 
 /dts-v1/;
-#include "vf610.dtsi"
+#include "vf610-zii-dev.dtsi"
 
 / {
 	model = "ZII VF610 Development Board, Rev B";
 	compatible = "zii,vf610dev-b", "zii,vf610dev", "fsl,vf610";
 
-	chosen {
-		stdout-path = "serial0:115200n8";
-	};
-
-	memory {
-		reg = <0x80000000 0x20000000>;
-	};
-
-	gpio-leds {
-		compatible = "gpio-leds";
-		pinctrl-0 = <&pinctrl_leds_debug>;
-		pinctrl-names = "default";
-
-		debug {
-			label = "zii:green:debug1";
-			gpios = <&gpio2 10 GPIO_ACTIVE_HIGH>;
-			linux,default-trigger = "heartbeat";
-		};
-	};
-
 	mdio-mux {
 		compatible = "mdio-mux-gpio";
 		pinctrl-0 = <&pinctrl_mdio_mux>;
@@ -86,7 +66,7 @@
 			#address-cells = <1>;
 			#size-cells = <0>;
 
-			switch0: switch0@0 {
+			switch0: switch@0 {
 				compatible = "marvell,mv88e6085";
 				#address-cells = <1>;
 				#size-cells = <0>;
@@ -96,6 +76,7 @@
 				ports {
 					#address-cells = <1>;
 					#size-cells = <0>;
+
 					port@0 {
 						reg = <0>;
 						label = "lan0";
@@ -127,6 +108,7 @@
 						reg = <6>;
 						label = "cpu";
 						ethernet = <&fec1>;
+
 						fixed-link {
 							speed = <100>;
 							full-duplex;
@@ -141,7 +123,7 @@
 			#address-cells = <1>;
 			#size-cells = <0>;
 
-			switch1: switch1@0 {
+			switch1: switch@0 {
 				compatible = "marvell,mv88e6085";
 				#address-cells = <1>;
 				#size-cells = <0>;
@@ -151,6 +133,7 @@
 				ports {
 					#address-cells = <1>;
 					#size-cells = <0>;
+
 					port@0 {
 						reg = <0>;
 						label = "lan3";
@@ -174,6 +157,7 @@
 						label = "dsa";
 						link = <&switch2port9>;
 						phy-mode = "rgmii-txid";
+
 						fixed-link {
 							speed = <1000>;
 							full-duplex;
@@ -194,12 +178,15 @@
 				mdio {
 					#address-cells = <1>;
 					#size-cells = <0>;
+
 					switch1phy0: switch1phy0@0 {
 						reg = <0>;
 					};
+
 					switch1phy1: switch1phy0@1 {
 						reg = <1>;
 					};
+
 					switch1phy2: switch1phy0@2 {
 						reg = <2>;
 					};
@@ -222,6 +209,7 @@
 				ports {
 					#address-cells = <1>;
 					#size-cells = <0>;
+
 					port@0 {
 						reg = <0>;
 						label = "lan6";
@@ -240,6 +228,7 @@
 					port@3 {
 						reg = <3>;
 						label = "optical3";
+
 						fixed-link {
 							speed = <1000>;
 							full-duplex;
@@ -251,6 +240,7 @@
 					port@4 {
 						reg = <4>;
 						label = "optical4";
+
 						fixed-link {
 							speed = <1000>;
 							full-duplex;
@@ -265,6 +255,7 @@
 						phy-mode = "rgmii-txid";
 						link = <&switch1port5
 							&switch0port5>;
+
 						fixed-link {
 							speed = <1000>;
 							full-duplex;
@@ -281,25 +272,6 @@
 		};
 	};
 
-	reg_vcc_3v3_mcu: regulator-vcc-3v3-mcu {
-		compatible = "regulator-fixed";
-		regulator-name = "vcc_3v3_mcu";
-		regulator-min-microvolt = <3300000>;
-		regulator-max-microvolt = <3300000>;
-	};
-
-	usb0_vbus: regulator-usb0-vbus {
-		compatible = "regulator-fixed";
-		pinctrl-0 = <&pinctrl_usb_vbus>;
-		regulator-name = "usb_vbus";
-		regulator-min-microvolt = <5000000>;
-		regulator-max-microvolt = <5000000>;
-		enable-active-high;
-		regulator-always-on;
-		regulator-boot-on;
-		gpio = <&gpio0 6 0>;
-	};
-
 	spi0 {
 		compatible = "spi-gpio";
 		pinctrl-0 = <&pinctrl_gpio_spi0>;
@@ -336,49 +308,6 @@
 	};
 };
 
-&adc0 {
-	pinctrl-names = "default";
-	pinctrl-0 = <&pinctrl_adc0_ad5>;
-	vref-supply = <&reg_vcc_3v3_mcu>;
-	status = "okay";
-};
-
-&edma0 {
-	status = "okay";
-};
-
-&esdhc1 {
-	pinctrl-names = "default";
-	pinctrl-0 = <&pinctrl_esdhc1>;
-	bus-width = <4>;
-	status = "okay";
-};
-
-&fec0 {
-	phy-mode = "rmii";
-	pinctrl-names = "default";
-	pinctrl-0 = <&pinctrl_fec0>;
-	status = "okay";
-};
-
-&fec1 {
-	phy-mode = "rmii";
-	pinctrl-names = "default";
-	pinctrl-0 = <&pinctrl_fec1>;
-	status = "okay";
-
-	fixed-link {
-		   speed = <100>;
-		   full-duplex;
-	};
-
-	mdio1: mdio {
-		#address-cells = <1>;
-		#size-cells = <0>;
-		status = "okay";
-	};
-};
-
 &i2c0 {
 	clock-frequency = <100000>;
 	pinctrl-names = "default";
@@ -403,33 +332,6 @@
 		interrupt-parent = <&gpio2>;
 		interrupts = <2 IRQ_TYPE_LEVEL_LOW>;
 	};
-
-	lm75@48 {
-		compatible = "national,lm75";
-		reg = <0x48>;
-	};
-
-	at24c04@50 {
-		compatible = "atmel,24c04";
-		reg = <0x50>;
-	};
-
-	at24c04@52 {
-		compatible = "atmel,24c04";
-		reg = <0x52>;
-	};
-
-	ds1682@6b {
-		compatible = "dallas,ds1682";
-		reg = <0x6b>;
-	};
-};
-
-&i2c1 {
-	clock-frequency = <100000>;
-	pinctrl-names = "default";
-	pinctrl-0 = <&pinctrl_i2c1>;
-	status = "okay";
 };
 
 &i2c2 {
@@ -499,120 +401,8 @@
 	};
 };
 
-&uart0 {
-	pinctrl-names = "default";
-	pinctrl-0 = <&pinctrl_uart0>;
-	status = "okay";
-};
-
-&uart1 {
-	pinctrl-names = "default";
-	pinctrl-0 = <&pinctrl_uart1>;
-	status = "okay";
-};
-
-&uart2 {
-	pinctrl-names = "default";
-	pinctrl-0 = <&pinctrl_uart2>;
-	status = "okay";
-};
-
-&usbdev0 {
-	disable-over-current;
-	vbus-supply = <&usb0_vbus>;
-	dr_mode = "host";
-	status = "okay";
-};
-
-&usbh1 {
-	disable-over-current;
-	status = "okay";
-};
-
-&usbmisc0 {
-	status = "okay";
-};
-
-&usbmisc1 {
-	status = "okay";
-};
-
-&usbphy0 {
-	status = "okay";
-};
-
-&usbphy1 {
-	status = "okay";
-};
 
 &iomuxc {
-	pinctrl_adc0_ad5: adc0ad5grp {
-		fsl,pins = <
-			VF610_PAD_PTC30__ADC0_SE5	0x00a1
-		>;
-	};
-
-	pinctrl_dspi0: dspi0grp {
-		fsl,pins = <
-			VF610_PAD_PTB18__DSPI0_CS1	0x1182
-			VF610_PAD_PTB19__DSPI0_CS0	0x1182
-			VF610_PAD_PTB20__DSPI0_SIN	0x1181
-			VF610_PAD_PTB21__DSPI0_SOUT	0x1182
-			VF610_PAD_PTB22__DSPI0_SCK	0x1182
-		>;
-	};
-
-	pinctrl_dspi2: dspi2grp {
-		fsl,pins = <
-			VF610_PAD_PTD31__DSPI2_CS1	0x1182
-			VF610_PAD_PTD30__DSPI2_CS0	0x1182
-			VF610_PAD_PTD29__DSPI2_SIN	0x1181
-			VF610_PAD_PTD28__DSPI2_SOUT	0x1182
-			VF610_PAD_PTD27__DSPI2_SCK	0x1182
-		>;
-	};
-
-	pinctrl_esdhc1: esdhc1grp {
-		fsl,pins = <
-			VF610_PAD_PTA24__ESDHC1_CLK	0x31ef
-			VF610_PAD_PTA25__ESDHC1_CMD	0x31ef
-			VF610_PAD_PTA26__ESDHC1_DAT0	0x31ef
-			VF610_PAD_PTA27__ESDHC1_DAT1	0x31ef
-			VF610_PAD_PTA28__ESDHC1_DATA2	0x31ef
-			VF610_PAD_PTA29__ESDHC1_DAT3	0x31ef
-			VF610_PAD_PTA7__GPIO_134	0x219d
-		>;
-	};
-
-	pinctrl_fec0: fec0grp {
-		fsl,pins = <
-			VF610_PAD_PTC0__ENET_RMII0_MDC	0x30d2
-			VF610_PAD_PTC1__ENET_RMII0_MDIO	0x30d3
-			VF610_PAD_PTC2__ENET_RMII0_CRS	0x30d1
-			VF610_PAD_PTC3__ENET_RMII0_RXD1	0x30d1
-			VF610_PAD_PTC4__ENET_RMII0_RXD0	0x30d1
-			VF610_PAD_PTC5__ENET_RMII0_RXER	0x30d1
-			VF610_PAD_PTC6__ENET_RMII0_TXD1	0x30d2
-			VF610_PAD_PTC7__ENET_RMII0_TXD0	0x30d2
-			VF610_PAD_PTC8__ENET_RMII0_TXEN	0x30d2
-		>;
-	};
-
-	pinctrl_fec1: fec1grp {
-		fsl,pins = <
-			VF610_PAD_PTA6__RMII_CLKIN		0x30d1
-			VF610_PAD_PTC9__ENET_RMII1_MDC		0x30d2
-			VF610_PAD_PTC10__ENET_RMII1_MDIO	0x30d3
-			VF610_PAD_PTC11__ENET_RMII1_CRS		0x30d1
-			VF610_PAD_PTC12__ENET_RMII1_RXD1	0x30d1
-			VF610_PAD_PTC13__ENET_RMII1_RXD0	0x30d1
-			VF610_PAD_PTC14__ENET_RMII1_RXER	0x30d1
-			VF610_PAD_PTC15__ENET_RMII1_TXD1	0x30d2
-			VF610_PAD_PTC16__ENET_RMII1_TXD0	0x30d2
-			VF610_PAD_PTC17__ENET_RMII1_TXEN	0x30d2
-		>;
-	};
-
 	pinctrl_gpio_e6185_eeprom_sel: pinctrl-gpio-e6185-eeprom-spi0 {
 		fsl,pins = <
 			VF610_PAD_PTE27__GPIO_132	0x33e2
@@ -629,39 +419,6 @@
 		>;
 	};
 
-	pinctrl_i2c_mux_reset: pinctrl-i2c-mux-reset {
-		fsl,pins = <
-			 VF610_PAD_PTE14__GPIO_119	0x31c2
-			 >;
-	};
-
-	pinctrl_i2c0: i2c0grp {
-		fsl,pins = <
-			VF610_PAD_PTB14__I2C0_SCL	0x37ff
-			VF610_PAD_PTB15__I2C0_SDA	0x37ff
-		>;
-	};
-
-	pinctrl_i2c1: i2c1grp {
-		fsl,pins = <
-			VF610_PAD_PTB16__I2C1_SCL	0x37ff
-			VF610_PAD_PTB17__I2C1_SDA	0x37ff
-		>;
-	};
-
-	pinctrl_i2c2: i2c2grp {
-		fsl,pins = <
-			VF610_PAD_PTA22__I2C2_SCL	0x37ff
-			VF610_PAD_PTA23__I2C2_SDA	0x37ff
-		>;
-	};
-
-	pinctrl_leds_debug: pinctrl-leds-debug {
-		fsl,pins = <
-			 VF610_PAD_PTD20__GPIO_74	0x31c2
-			 >;
-	};
-
 	pinctrl_mdio_mux: pinctrl-mdio-mux {
 		fsl,pins = <
 			VF610_PAD_PTA18__GPIO_8		0x31c2
@@ -676,48 +433,4 @@
 			VF610_PAD_PTB28__GPIO_98	0x219d
 		>;
 	};
-
-	pinctrl_qspi0: qspi0grp {
-		fsl,pins = <
-			VF610_PAD_PTD7__QSPI0_B_QSCK	0x31c3
-			VF610_PAD_PTD8__QSPI0_B_CS0	0x31ff
-			VF610_PAD_PTD9__QSPI0_B_DATA3	0x31c3
-			VF610_PAD_PTD10__QSPI0_B_DATA2	0x31c3
-			VF610_PAD_PTD11__QSPI0_B_DATA1	0x31c3
-			VF610_PAD_PTD12__QSPI0_B_DATA0	0x31c3
-		>;
-	};
-
-	pinctrl_uart0: uart0grp {
-		fsl,pins = <
-			VF610_PAD_PTB10__UART0_TX	0x21a2
-			VF610_PAD_PTB11__UART0_RX	0x21a1
-		>;
-	};
-
-	pinctrl_uart1: uart1grp {
-		fsl,pins = <
-			VF610_PAD_PTB23__UART1_TX	0x21a2
-			VF610_PAD_PTB24__UART1_RX	0x21a1
-		>;
-	};
-
-	pinctrl_uart2: uart2grp {
-		fsl,pins = <
-			VF610_PAD_PTD0__UART2_TX	0x21a2
-			VF610_PAD_PTD1__UART2_RX	0x21a1
-		>;
-	};
-
-	pinctrl_usb_vbus: pinctrl-usb-vbus {
-		fsl,pins = <
-			VF610_PAD_PTA16__GPIO_6	0x31c2
-		>;
-	};
-
-	pinctrl_usb0_host: usb0-host-grp {
-		fsl,pins = <
-			VF610_PAD_PTD6__GPIO_85		0x0062
-		>;
-	};
 };
diff --git a/arch/arm/boot/dts/vf610-zii-dev-rev-c.dts b/arch/arm/boot/dts/vf610-zii-dev-rev-c.dts
new file mode 100644
index 0000000..55c9473
--- /dev/null
+++ b/arch/arm/boot/dts/vf610-zii-dev-rev-c.dts
@@ -0,0 +1,414 @@
+/*
+ * Copyright (C) 2015, 2016 Zodiac Inflight Innovations
+ *
+ * Based on an original 'vf610-twr.dts' which is Copyright 2015,
+ * Freescale Semiconductor, Inc.
+ *
+ * This file is dual-licensed: you can use it either under the terms
+ * of the GPL or the X11 license, at your option. Note that this dual
+ * licensing only applies to this file, and not this project as a
+ * whole.
+ *
+ *  a) This file 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 file 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 General Public License for more details.
+ *
+ * Or, alternatively
+ *
+ *  b) Permission is hereby granted, free of charge, to any person
+ *     obtaining a copy of this software and associated documentation
+ *     files (the "Software"), to deal in the Software without
+ *     restriction, including without limitation the rights to use
+ *     copy, modify, merge, publish, distribute, sublicense, and/or
+ *     sell copies of the Software, and to permit persons to whom the
+ *     Software is furnished to do so, subject to the following
+ *     conditions:
+ *
+ *     The above copyright notice and this permission notice shall be
+ *     included in all copies or substantial portions of the Software.
+ *
+ *     THE SOFTWARE IS PROVIDED , WITHOUT WARRANTY OF ANY KIND
+ *     EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES
+ *     OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+ *     NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT
+ *     HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY
+ *     WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ *     FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
+ *     OTHER DEALINGS IN THE SOFTWARE.
+ */
+
+/dts-v1/;
+#include "vf610-zii-dev.dtsi"
+
+/ {
+	model = "ZII VF610 Development Board, Rev C";
+	compatible = "zii,vf610dev-c", "zii,vf610dev", "fsl,vf610";
+
+	mdio-mux {
+		compatible = "mdio-mux-gpio";
+		pinctrl-0 = <&pinctrl_mdio_mux>;
+		pinctrl-names = "default";
+		gpios = <&gpio0 8  GPIO_ACTIVE_HIGH
+			 &gpio0 9  GPIO_ACTIVE_HIGH
+			 &gpio0 25 GPIO_ACTIVE_HIGH>;
+		mdio-parent-bus = <&mdio1>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		mdio_mux_1: mdio@1 {
+			reg = <1>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			switch0: switch@0 {
+				compatible = "marvell,mv88e6190";
+				#address-cells = <1>;
+				#size-cells = <0>;
+				reg = <0>;
+				dsa,member = <0 0>;
+
+				ports {
+					#address-cells = <1>;
+					#size-cells = <0>;
+
+					port@0 {
+						reg = <0>;
+						label = "cpu";
+						ethernet = <&fec1>;
+
+						fixed-link {
+							speed = <100>;
+							full-duplex;
+						};
+					};
+
+					port@1 {
+						reg = <1>;
+						label = "lan1";
+					};
+
+					port@2 {
+						reg = <2>;
+						label = "lan2";
+					};
+
+					port@3 {
+						reg = <3>;
+						label = "lan3";
+					};
+
+					port@4 {
+						reg = <4>;
+						label = "lan4";
+					};
+
+					switch0port10: port@10 {
+						reg = <10>;
+						label = "dsa";
+						phy-mode = "xgmii";
+						link = <&switch1port10>;
+					};
+				};
+			};
+		};
+
+		mdio_mux_2: mdio@2 {
+			reg = <2>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			switch1: switch@0 {
+				compatible = "marvell,mv88e6190";
+				#address-cells = <1>;
+				#size-cells = <0>;
+				reg = <0>;
+				dsa,member = <0 1>;
+
+				ports {
+					#address-cells = <1>;
+					#size-cells = <0>;
+
+					port@1 {
+						reg = <1>;
+						label = "lan5";
+					};
+
+					port@2 {
+						reg = <2>;
+						label = "lan6";
+					};
+
+					port@3 {
+						reg = <3>;
+						label = "lan7";
+					};
+
+					port@4 {
+						reg = <4>;
+						label = "lan8";
+					};
+
+
+					switch1port10: port@10 {
+						reg = <10>;
+						label = "dsa";
+						phy-mode = "xgmii";
+						link = <&switch0port10>;
+					};
+				};
+			};
+		};
+
+		mdio_mux_4: mdio@4 {
+			reg = <4>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+		};
+	};
+};
+
+&dspi0 {
+	bus-num = <0>;
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_dspi0>;
+	status = "okay";
+	spi-num-chipselects = <2>;
+
+	m25p128@0 {
+		compatible = "m25p128", "jedec,spi-nor";
+		#address-cells = <1>;
+		#size-cells = <1>;
+		reg = <0>;
+		spi-max-frequency = <1000000>;
+	};
+
+	atzb-rf-233@1 {
+		compatible = "atmel,at86rf233";
+
+		pinctrl-names = "default";
+		pinctrl-0 = <&pinctr_atzb_rf_233>;
+
+		spi-max-frequency = <7500000>;
+		reg = <1>;
+		interrupts = <4 IRQ_TYPE_LEVEL_HIGH>;
+		interrupt-parent = <&gpio3>;
+		xtal-trim = /bits/ 8 <0x06>;
+
+		sleep-gpio = <&gpio0 24 GPIO_ACTIVE_HIGH>;
+		reset-gpio = <&gpio6 10 GPIO_ACTIVE_HIGH>;
+
+		fsl,spi-cs-sck-delay = <180>;
+		fsl,spi-sck-cs-delay = <250>;
+	};
+};
+
+&i2c0 {
+	/*
+	 * U712
+	 *
+	 * Exposed signals:
+	 *    P1 - WE2_CMD
+	 *    P2 - WE2_CLK
+	 */
+	gpio5: pca9557@18 {
+		compatible = "nxp,pca9557";
+		reg = <0x18>;
+		gpio-controller;
+		#gpio-cells = <2>;
+	};
+
+	/*
+	 * U121
+	 *
+	 * Exposed signals:
+	 *    I/O0  - ENET_SWR_EN
+	 *    I/O1  - ESW1_RESETn
+	 *    I/O2  - ARINC_RESET
+	 *    I/O3  - DD1_IO_RESET
+	 *    I/O4  - ESW2_RESETn
+	 *    I/O5  - ESW3_RESETn
+	 *    I/O6  - ESW4_RESETn
+	 *    I/O8  - TP909
+	 *    I/O9  - FEM_SEL
+	 *    I/O10 - WIFI_RESETn
+	 *    I/O11 - PHY_RSTn
+	 *    I/O12 - OPT1_SD
+	 *    I/O13 - OPT2_SD
+	 *    I/O14 - OPT1_TX_DIS
+	 *    I/O15 - OPT2_TX_DIS
+	 */
+	gpio6: sx1503@20 {
+		compatible = "semtech,sx1503q";
+
+		pinctrl-names = "default";
+		pinctrl-0 = <&pinctrl_sx1503_20>;
+		#gpio-cells = <2>;
+		#interrupt-cells = <2>;
+		reg = <0x20>;
+		interrupt-parent = <&gpio0>;
+		interrupts = <23 IRQ_TYPE_EDGE_FALLING>;
+		gpio-controller;
+		interrupt-controller;
+
+		enet_swr_en {
+			gpio-hog;
+			gpios = <0 GPIO_ACTIVE_HIGH>;
+			output-high;
+			line-name = "enet-swr-en";
+		};
+	};
+
+	/*
+	 * U715
+	 *
+	 * Exposed signals:
+	 *     IO0 - WE1_CLK
+	 *     IO1 - WE1_CMD
+	 */
+	gpio7: pca9554@22 {
+		compatible = "nxp,pca9554";
+		reg = <0x22>;
+		gpio-controller;
+		#gpio-cells = <2>;
+
+	};
+};
+
+&i2c1 {
+	at24mac602@00 {
+		compatible = "atmel,24c02";
+		reg = <0x50>;
+		read-only;
+	};
+};
+
+&i2c2 {
+	tca9548@70 {
+		compatible = "nxp,pca9548";
+		pinctrl-0 = <&pinctrl_i2c_mux_reset>;
+		pinctrl-names = "default";
+		#address-cells = <1>;
+		#size-cells = <0>;
+		reg = <0x70>;
+		reset-gpios = <&gpio3 23 GPIO_ACTIVE_LOW>;
+
+		i2c@0 {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			reg = <0>;
+		};
+
+		i2c@1 {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			reg = <1>;
+
+			sfp2: at24c04@50 {
+				compatible = "atmel,24c02";
+				reg = <0x50>;
+			};
+		};
+
+		i2c@2 {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			reg = <2>;
+
+			sfp3: at24c04@50 {
+				compatible = "atmel,24c02";
+				reg = <0x50>;
+			};
+		};
+
+		i2c@3 {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			reg = <3>;
+		};
+	};
+};
+
+&uart3 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_uart3>;
+	status = "okay";
+};
+
+&gpio0 {
+	eth0_intrp {
+		gpio-hog;
+		gpios = <23 GPIO_ACTIVE_HIGH>;
+		input;
+		line-name = "sx1503-irq";
+	};
+};
+
+&gpio3 {
+	eth0_intrp {
+		gpio-hog;
+		gpios = <2 GPIO_ACTIVE_HIGH>;
+		input;
+		line-name = "eth0-intrp";
+	};
+};
+
+&fec0 {
+	mdio {
+		#address-cells = <1>;
+		#size-cells = <0>;
+		status = "okay";
+
+		ethernet-phy@0 {
+			compatible = "ethernet-phy-ieee802.3-c22";
+
+			pinctrl-names = "default";
+			pinctrl-0 = <&pinctrl_fec0_phy_int>;
+
+			interrupt-parent = <&gpio3>;
+			interrupts = <2 IRQ_TYPE_LEVEL_LOW>;
+			reg = <0>;
+		};
+	};
+};
+
+&iomuxc {
+	pinctr_atzb_rf_233: pinctrl-atzb-rf-233 {
+		fsl,pins = <
+			VF610_PAD_PTB2__GPIO_24		0x31c2
+			VF610_PAD_PTE27__GPIO_132	0x33e2
+		>;
+	};
+
+
+	pinctrl_sx1503_20: pinctrl-sx1503-20 {
+		fsl,pins = <
+			VF610_PAD_PTB1__GPIO_23		0x219d
+		>;
+	};
+
+	pinctrl_uart3: uart3grp {
+		fsl,pins = <
+			VF610_PAD_PTA20__UART3_TX	0x21a2
+			VF610_PAD_PTA21__UART3_RX	0x21a1
+		>;
+	};
+
+	pinctrl_mdio_mux: pinctrl-mdio-mux {
+		fsl,pins = <
+			VF610_PAD_PTA18__GPIO_8		0x31c2
+			VF610_PAD_PTA19__GPIO_9		0x31c2
+			VF610_PAD_PTB3__GPIO_25		0x31c2
+		>;
+	};
+
+	pinctrl_fec0_phy_int: pinctrl-fec0-phy-int {
+		fsl,pins = <
+			VF610_PAD_PTB28__GPIO_98	0x219d
+		>;
+	};
+};
diff --git a/arch/arm/boot/dts/vf610-zii-dev.dtsi b/arch/arm/boot/dts/vf610-zii-dev.dtsi
new file mode 100644
index 0000000..9f5e2e7
--- /dev/null
+++ b/arch/arm/boot/dts/vf610-zii-dev.dtsi
@@ -0,0 +1,383 @@
+/*
+ * Copyright (C) 2015, 2016 Zodiac Inflight Innovations
+ *
+ * Based on an original 'vf610-twr.dts' which is Copyright 2015,
+ * Freescale Semiconductor, Inc.
+ *
+ * This file is dual-licensed: you can use it either under the terms
+ * of the GPL or the X11 license, at your option. Note that this dual
+ * licensing only applies to this file, and not this project as a
+ * whole.
+ *
+ *  a) This file 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 file 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 General Public License for more details.
+ *
+ * Or, alternatively
+ *
+ *  b) Permission is hereby granted, free of charge, to any person
+ *     obtaining a copy of this software and associated documentation
+ *     files (the "Software"), to deal in the Software without
+ *     restriction, including without limitation the rights to use
+n *     copy, modify, merge, publish, distribute, sublicense, and/or
+ *     sell copies of the Software, and to permit persons to whom the
+ *     Software is furnished to do so, subject to the following
+ *     conditions:
+ *
+ *     The above copyright notice and this permission notice shall be
+ *     included in all copies or substantial portions of the Software.
+ *
+ *     THE SOFTWARE IS PROVIDED , WITHOUT WARRANTY OF ANY KIND
+ *     EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES
+ *     OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+ *     NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT
+ *     HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY
+ *     WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ *     FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
+ *     OTHER DEALINGS IN THE SOFTWARE.
+ */
+
+#include "vf610.dtsi"
+
+/ {
+	chosen {
+		stdout-path = "serial0:115200n8";
+	};
+
+	memory {
+		reg = <0x80000000 0x20000000>;
+	};
+
+	gpio-leds {
+		compatible = "gpio-leds";
+		pinctrl-0 = <&pinctrl_leds_debug>;
+		pinctrl-names = "default";
+
+		debug {
+			label = "zii:green:debug1";
+			gpios = <&gpio2 10 GPIO_ACTIVE_HIGH>;
+			linux,default-trigger = "heartbeat";
+		};
+	};
+
+	reg_vcc_3v3_mcu: regulator-vcc-3v3-mcu {
+		compatible = "regulator-fixed";
+		regulator-name = "vcc_3v3_mcu";
+		regulator-min-microvolt = <3300000>;
+		regulator-max-microvolt = <3300000>;
+	};
+
+	usb0_vbus: regulator-usb0-vbus {
+		compatible = "regulator-fixed";
+		pinctrl-0 = <&pinctrl_usb_vbus>;
+		regulator-name = "usb_vbus";
+		regulator-min-microvolt = <5000000>;
+		regulator-max-microvolt = <5000000>;
+		enable-active-high;
+		regulator-always-on;
+		regulator-boot-on;
+		gpio = <&gpio0 6 0>;
+	};
+};
+
+&adc0 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_adc0_ad5>;
+	vref-supply = <&reg_vcc_3v3_mcu>;
+	status = "okay";
+};
+
+&edma0 {
+	status = "okay";
+};
+
+&esdhc1 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_esdhc1>;
+	bus-width = <4>;
+	status = "okay";
+};
+
+&fec0 {
+	phy-mode = "rmii";
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_fec0>;
+	status = "okay";
+};
+
+&fec1 {
+	phy-mode = "rmii";
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_fec1>;
+	status = "okay";
+
+	fixed-link {
+		   speed = <100>;
+		   full-duplex;
+	};
+
+	mdio1: mdio {
+		#address-cells = <1>;
+		#size-cells = <0>;
+		status = "okay";
+	};
+};
+
+&i2c0 {
+	clock-frequency = <100000>;
+	pinctrl-names = "default", "gpio";
+	pinctrl-0 = <&pinctrl_i2c0>;
+	pinctrl-1 = <&pinctrl_i2c0_gpio>;
+	scl-gpios = <&gpio1 4 GPIO_ACTIVE_HIGH>;
+	sda-gpios = <&gpio1 5 GPIO_ACTIVE_HIGH>;
+	status = "okay";
+
+	lm75@48 {
+		compatible = "national,lm75";
+		reg = <0x48>;
+	};
+
+	at24c04@50 {
+		compatible = "atmel,24c04";
+		reg = <0x50>;
+	};
+
+	at24c04@52 {
+		compatible = "atmel,24c04";
+		reg = <0x52>;
+	};
+
+	ds1682@6b {
+		compatible = "dallas,ds1682";
+		reg = <0x6b>;
+	};
+};
+
+&i2c1 {
+	clock-frequency = <100000>;
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_i2c1>;
+	status = "okay";
+};
+
+&i2c2 {
+	clock-frequency = <100000>;
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_i2c2>;
+	status = "okay";
+};
+
+&uart0 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_uart0>;
+	status = "okay";
+};
+
+&uart1 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_uart1>;
+	status = "okay";
+};
+
+&uart2 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_uart2>;
+	status = "okay";
+};
+
+&usbdev0 {
+	disable-over-current;
+	vbus-supply = <&usb0_vbus>;
+	dr_mode = "host";
+	status = "okay";
+};
+
+&usbh1 {
+	disable-over-current;
+	status = "okay";
+};
+
+&usbmisc0 {
+	status = "okay";
+};
+
+&usbmisc1 {
+	status = "okay";
+};
+
+&usbphy0 {
+	status = "okay";
+};
+
+&usbphy1 {
+	status = "okay";
+};
+
+&iomuxc {
+	pinctrl_adc0_ad5: adc0ad5grp {
+		fsl,pins = <
+			VF610_PAD_PTC30__ADC0_SE5	0x00a1
+		>;
+	};
+
+	pinctrl_dspi0: dspi0grp {
+		fsl,pins = <
+			VF610_PAD_PTB18__DSPI0_CS1	0x1182
+			VF610_PAD_PTB19__DSPI0_CS0	0x1182
+			VF610_PAD_PTB20__DSPI0_SIN	0x1181
+			VF610_PAD_PTB21__DSPI0_SOUT	0x1182
+			VF610_PAD_PTB22__DSPI0_SCK	0x1182
+		>;
+	};
+
+	pinctrl_dspi2: dspi2grp {
+		fsl,pins = <
+			VF610_PAD_PTD31__DSPI2_CS1	0x1182
+			VF610_PAD_PTD30__DSPI2_CS0	0x1182
+			VF610_PAD_PTD29__DSPI2_SIN	0x1181
+			VF610_PAD_PTD28__DSPI2_SOUT	0x1182
+			VF610_PAD_PTD27__DSPI2_SCK	0x1182
+		>;
+	};
+
+	pinctrl_esdhc1: esdhc1grp {
+		fsl,pins = <
+			VF610_PAD_PTA24__ESDHC1_CLK	0x31ef
+			VF610_PAD_PTA25__ESDHC1_CMD	0x31ef
+			VF610_PAD_PTA26__ESDHC1_DAT0	0x31ef
+			VF610_PAD_PTA27__ESDHC1_DAT1	0x31ef
+			VF610_PAD_PTA28__ESDHC1_DATA2	0x31ef
+			VF610_PAD_PTA29__ESDHC1_DAT3	0x31ef
+			VF610_PAD_PTA7__GPIO_134	0x219d
+		>;
+	};
+
+	pinctrl_fec0: fec0grp {
+		fsl,pins = <
+			VF610_PAD_PTC0__ENET_RMII0_MDC	0x30d2
+			VF610_PAD_PTC1__ENET_RMII0_MDIO	0x30d3
+			VF610_PAD_PTC2__ENET_RMII0_CRS	0x30d1
+			VF610_PAD_PTC3__ENET_RMII0_RXD1	0x30d1
+			VF610_PAD_PTC4__ENET_RMII0_RXD0	0x30d1
+			VF610_PAD_PTC5__ENET_RMII0_RXER	0x30d1
+			VF610_PAD_PTC6__ENET_RMII0_TXD1	0x30d2
+			VF610_PAD_PTC7__ENET_RMII0_TXD0	0x30d2
+			VF610_PAD_PTC8__ENET_RMII0_TXEN	0x30d2
+		>;
+	};
+
+	pinctrl_fec1: fec1grp {
+		fsl,pins = <
+			VF610_PAD_PTA6__RMII_CLKIN		0x30d1
+			VF610_PAD_PTC9__ENET_RMII1_MDC		0x30d2
+			VF610_PAD_PTC10__ENET_RMII1_MDIO	0x30d3
+			VF610_PAD_PTC11__ENET_RMII1_CRS		0x30d1
+			VF610_PAD_PTC12__ENET_RMII1_RXD1	0x30d1
+			VF610_PAD_PTC13__ENET_RMII1_RXD0	0x30d1
+			VF610_PAD_PTC14__ENET_RMII1_RXER	0x30d1
+			VF610_PAD_PTC15__ENET_RMII1_TXD1	0x30d2
+			VF610_PAD_PTC16__ENET_RMII1_TXD0	0x30d2
+			VF610_PAD_PTC17__ENET_RMII1_TXEN	0x30d2
+		>;
+	};
+
+	pinctrl_gpio_spi0: pinctrl-gpio-spi0 {
+		fsl,pins = <
+			VF610_PAD_PTB22__GPIO_44	0x33e2
+			VF610_PAD_PTB21__GPIO_43	0x33e2
+			VF610_PAD_PTB20__GPIO_42	0x33e1
+			VF610_PAD_PTB19__GPIO_41	0x33e2
+			VF610_PAD_PTB18__GPIO_40	0x33e2
+		>;
+	};
+
+	pinctrl_i2c_mux_reset: pinctrl-i2c-mux-reset {
+		fsl,pins = <
+			 VF610_PAD_PTE14__GPIO_119	0x31c2
+			 >;
+	};
+
+	pinctrl_i2c0: i2c0grp {
+		fsl,pins = <
+			VF610_PAD_PTB14__I2C0_SCL	0x37ff
+			VF610_PAD_PTB15__I2C0_SDA	0x37ff
+		>;
+	};
+
+	pinctrl_i2c0_gpio: i2c0grp-gpio {
+		fsl,pins = <
+			VF610_PAD_PTB14__GPIO_36	0x31c2
+			VF610_PAD_PTB15__GPIO_37	0x31c2
+		>;
+	};
+
+	
+	pinctrl_i2c1: i2c1grp {
+		fsl,pins = <
+			VF610_PAD_PTB16__I2C1_SCL	0x37ff
+			VF610_PAD_PTB17__I2C1_SDA	0x37ff
+		>;
+	};
+
+	pinctrl_i2c2: i2c2grp {
+		fsl,pins = <
+			VF610_PAD_PTA22__I2C2_SCL	0x37ff
+			VF610_PAD_PTA23__I2C2_SDA	0x37ff
+		>;
+	};
+
+	pinctrl_leds_debug: pinctrl-leds-debug {
+		fsl,pins = <
+			 VF610_PAD_PTD20__GPIO_74	0x31c2
+			 >;
+	};
+
+	pinctrl_qspi0: qspi0grp {
+		fsl,pins = <
+			VF610_PAD_PTD7__QSPI0_B_QSCK	0x31c3
+			VF610_PAD_PTD8__QSPI0_B_CS0	0x31ff
+			VF610_PAD_PTD9__QSPI0_B_DATA3	0x31c3
+			VF610_PAD_PTD10__QSPI0_B_DATA2	0x31c3
+			VF610_PAD_PTD11__QSPI0_B_DATA1	0x31c3
+			VF610_PAD_PTD12__QSPI0_B_DATA0	0x31c3
+		>;
+	};
+
+	pinctrl_uart0: uart0grp {
+		fsl,pins = <
+			VF610_PAD_PTB10__UART0_TX	0x21a2
+			VF610_PAD_PTB11__UART0_RX	0x21a1
+		>;
+	};
+
+	pinctrl_uart1: uart1grp {
+		fsl,pins = <
+			VF610_PAD_PTB23__UART1_TX	0x21a2
+			VF610_PAD_PTB24__UART1_RX	0x21a1
+		>;
+	};
+
+	pinctrl_uart2: uart2grp {
+		fsl,pins = <
+			VF610_PAD_PTD0__UART2_TX	0x21a2
+			VF610_PAD_PTD1__UART2_RX	0x21a1
+		>;
+	};
+
+	pinctrl_usb_vbus: pinctrl-usb-vbus {
+		fsl,pins = <
+			VF610_PAD_PTA16__GPIO_6	0x31c2
+		>;
+	};
+
+	pinctrl_usb0_host: usb0-host-grp {
+		fsl,pins = <
+			VF610_PAD_PTD6__GPIO_85		0x0062
+		>;
+	};
+};
-- 
2.9.3

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply related

* [PATCH v5 1/2] ARM: dts: vf610-zii-dev-rev-b: Remove leftover PWM pingroup
From: Andrey Smirnov @ 2017-01-07 20:06 UTC (permalink / raw)
  To: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
  Cc: Andrey Smirnov, Shawn Guo, Rob Herring, Mark Rutland,
	Russell King, Sascha Hauer, Stefan Agner,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA, andrew-g2DYL2Zd6BY,
	Vivien Didelot, cphealy-Re5JQEeQqe8AvxtiuMwx3w

Remove pwm0grp since it is:

	a) Not referenced anywhere in the DTS file (unlike Tower board it
	is based on, this board does not use/expose FTM0)

	b) Configures PTB2 and PTB3 in a way that contradicts
	pinctrl-mdio-mux

Cc: Shawn Guo <shawnguo-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>
Cc: Russell King <linux-I+IVW8TIWO2tmTQ+vhA3Yw@public.gmane.org>
Cc: Sascha Hauer <kernel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
Cc: Stefan Agner <stefan-XLVq0VzYD2Y@public.gmane.org>
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: andrew-g2DYL2Zd6BY@public.gmane.org
Cc: Vivien Didelot <vivien.didelot-4ysUXcep3aM1wj+D4I0NRVaTQe2KTcn/@public.gmane.org>
Cc: cphealy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org
Tested-by: Nikita Yushchenko <nikita.yoush-M4DtvfQ/ZS1MRgGoP+s0PdBPR1lH4CV8@public.gmane.org>
Signed-off-by: Andrey Smirnov <andrew.smirnov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---

No changes since v4

 arch/arm/boot/dts/vf610-zii-dev-rev-b.dts | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/arch/arm/boot/dts/vf610-zii-dev-rev-b.dts b/arch/arm/boot/dts/vf610-zii-dev-rev-b.dts
index fa19cfd..2210811 100644
--- a/arch/arm/boot/dts/vf610-zii-dev-rev-b.dts
+++ b/arch/arm/boot/dts/vf610-zii-dev-rev-b.dts
@@ -677,15 +677,6 @@
 		>;
 	};
 
-	pinctrl_pwm0: pwm0grp {
-		fsl,pins = <
-			VF610_PAD_PTB0__FTM0_CH0	0x1582
-			VF610_PAD_PTB1__FTM0_CH1	0x1582
-			VF610_PAD_PTB2__FTM0_CH2	0x1582
-			VF610_PAD_PTB3__FTM0_CH3	0x1582
-		>;
-	};
-
 	pinctrl_qspi0: qspi0grp {
 		fsl,pins = <
 			VF610_PAD_PTD7__QSPI0_B_QSCK	0x31c3
-- 
2.9.3

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply related

* Re: [PATCH] iio: accel: st_accel: handle deprecated bindings
From: Jonathan Cameron @ 2017-01-07 19:42 UTC (permalink / raw)
  To: Linus Walleij, linux-iio-u79uwXL29TY76Z2rM5mHXA
  Cc: devicetree-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20170105133233.24237-1-linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

On 05/01/17 08:32, Linus Walleij wrote:
> The earlier deployed LIS3LV02DL driver had already defined a few
> DT bindings that need to be supported by the new more generic
> driver and listed as compatible but deprecated bindings in the
> documentation.
> 
> After this we can start to activate the new driver with the old
> systems where applicable.
> 
> As part of this enablement: make us depend on the old drivers
> not being in use so we don't get a kernel with two competing
> drivers.
> 
> Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Signed-off-by: Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Applied to the togreg branch of iio.git. Will push out as testing sometime soonish.
> ---
>  Documentation/devicetree/bindings/iio/accel/lis302.txt | 2 +-
>  Documentation/devicetree/bindings/iio/st-sensors.txt   | 2 ++
>  drivers/iio/accel/Kconfig                              | 2 ++
>  drivers/iio/accel/st_accel_i2c.c                       | 5 +++++
>  drivers/iio/accel/st_accel_spi.c                       | 9 +++++++++
>  5 files changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/iio/accel/lis302.txt b/Documentation/devicetree/bindings/iio/accel/lis302.txt
> index 2a19bff9693f..dfdce67826ba 100644
> --- a/Documentation/devicetree/bindings/iio/accel/lis302.txt
> +++ b/Documentation/devicetree/bindings/iio/accel/lis302.txt
> @@ -5,7 +5,7 @@ that apply in on the generic device (independent from the bus).
>  
>  
>  Required properties for the SPI bindings:
> - - compatible: 		should be set to "st,lis3lv02d_spi"
> + - compatible: 		should be set to "st,lis3lv02d-spi"
>   - reg:			the chipselect index
>   - spi-max-frequency:	maximal bus speed, should be set to 1000000 unless
>  			constrained by external circuitry
> diff --git a/Documentation/devicetree/bindings/iio/st-sensors.txt b/Documentation/devicetree/bindings/iio/st-sensors.txt
> index c040c9ad1889..eaa8fbba34e2 100644
> --- a/Documentation/devicetree/bindings/iio/st-sensors.txt
> +++ b/Documentation/devicetree/bindings/iio/st-sensors.txt
> @@ -27,6 +27,8 @@ standard bindings from pinctrl/pinctrl-bindings.txt.
>  Valid compatible strings:
>  
>  Accelerometers:
> +- st,lis3lv02d (deprecated, use st,lis3lv02dl-accel)
> +- st,lis302dl-spi (deprecated, use st,lis3lv02dl-accel)
>  - st,lis3lv02dl-accel
>  - st,lsm303dlh-accel
>  - st,lsm303dlhc-accel
> diff --git a/drivers/iio/accel/Kconfig b/drivers/iio/accel/Kconfig
> index c68bdb649005..ea295fe0f561 100644
> --- a/drivers/iio/accel/Kconfig
> +++ b/drivers/iio/accel/Kconfig
> @@ -140,11 +140,13 @@ config IIO_ST_ACCEL_3AXIS
>  
>  config IIO_ST_ACCEL_I2C_3AXIS
>  	tristate
> +	depends on !SENSORS_LIS3_I2C
>  	depends on IIO_ST_ACCEL_3AXIS
>  	depends on IIO_ST_SENSORS_I2C
>  
>  config IIO_ST_ACCEL_SPI_3AXIS
>  	tristate
> +	depends on !SENSORS_LIS3_SPI
>  	depends on IIO_ST_ACCEL_3AXIS
>  	depends on IIO_ST_SENSORS_SPI
>  
> diff --git a/drivers/iio/accel/st_accel_i2c.c b/drivers/iio/accel/st_accel_i2c.c
> index c0f8867aa1ea..99a7cdbe3d5d 100644
> --- a/drivers/iio/accel/st_accel_i2c.c
> +++ b/drivers/iio/accel/st_accel_i2c.c
> @@ -21,6 +21,11 @@
>  #ifdef CONFIG_OF
>  static const struct of_device_id st_accel_of_match[] = {
>  	{
> +		/* An older compatible */
> +		.compatible = "st,lis3lv02d",
> +		.data = LIS3LV02DL_ACCEL_DEV_NAME,
> +	},
> +	{
>  		.compatible = "st,lis3lv02dl-accel",
>  		.data = LIS3LV02DL_ACCEL_DEV_NAME,
>  	},
> diff --git a/drivers/iio/accel/st_accel_spi.c b/drivers/iio/accel/st_accel_spi.c
> index c25ac50d4600..29a15f27a51b 100644
> --- a/drivers/iio/accel/st_accel_spi.c
> +++ b/drivers/iio/accel/st_accel_spi.c
> @@ -65,9 +65,18 @@ static const struct spi_device_id st_accel_id_table[] = {
>  };
>  MODULE_DEVICE_TABLE(spi, st_accel_id_table);
>  
> +#ifdef CONFIG_OF
> +static const struct of_device_id lis302dl_spi_dt_ids[] = {
> +	{ .compatible = "st,lis302dl-spi" },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, lis302dl_spi_dt_ids);
> +#endif
> +
>  static struct spi_driver st_accel_driver = {
>  	.driver = {
>  		.name = "st-accel-spi",
> +		.of_match_table = of_match_ptr(lis302dl_spi_dt_ids),
>  	},
>  	.probe = st_accel_spi_probe,
>  	.remove = st_accel_spi_remove,
> 

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH 14/22] dt-bindings: power: supply: add AXP20X/AXP22X battery DT binding
From: Jonathan Cameron @ 2017-01-07 19:33 UTC (permalink / raw)
  To: Rob Herring, Quentin Schulz
  Cc: mark.rutland, devicetree, lars, linux-pm, linux-iio, linux-kernel,
	sre, linux, wens, icenowy, pmeerw, knaack.h, maxime.ripard,
	bonbons, lee.jones, thomas.petazzoni, linux-arm-kernel
In-Reply-To: <20170104132107.tciklylgqvhftb6f@rob-hp-laptop>

On 04/01/17 08:21, Rob Herring wrote:
> On Mon, Jan 02, 2017 at 05:37:14PM +0100, Quentin Schulz wrote:
>> The X-Powers AXP20X and AXP22X PMICs can have a battery as power supply.
>>
>> This patch adds the DT binding documentation for the battery power
>> supply which gets various data from the PMIC, such as the battery status
>> (charging, discharging, full, dead), current max limit, current current,
>> battery capacity (in percentage), voltage max and min limits, current
>> voltage and battery capacity (in Ah).
>>
>> Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
>> ---
>>  .../bindings/power/supply/axp20x_battery.txt       | 27 ++++++++++++++++++++++
>>  1 file changed, 27 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/power/supply/axp20x_battery.txt
>>
>> diff --git a/Documentation/devicetree/bindings/power/supply/axp20x_battery.txt b/Documentation/devicetree/bindings/power/supply/axp20x_battery.txt
>> new file mode 100644
>> index 0000000..5489d0d
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/power/supply/axp20x_battery.txt
>> @@ -0,0 +1,27 @@
>> +AXP20x and AXP22x battery power supply
>> +
>> +Required Properties:
>> + - compatible, one of:
>> +			"x-powers,axp209-battery-power-supply"
>> +			"x-powers,axp221-battery-power-supply"
>> + - io-channels: phandles to battery voltage, charge and discharge
>> + currents ADC channels
>> + - io-channel-names = "batt_v", "batt_chrg_i", "batt_dischrg_i";
>> +
>> +This node is a subnode of the axp20x/axp22x PMIC.
>> +
>> +The AXP20X and AXP22X can read the battery voltage, charge and discharge
>> +currents of the battery by reading ADC channels from the AXP20X/AXP22X
>> +ADC.
>> +
>> +Example:
>> +
>> +&axp209 {
>> +	battery_power_supply: battery_power_supply {
> 
> Humm, I guess you power-supply is not sufficient, so 
> 'battery-power-supply' and similar for ac.
> 
>> +		compatible = "x-powers,axp209-battery-power-supply";
>> +		io-channels = <&axp209_adc 7>, <&axp209_adc 8>,
>> +			<&axp209_adc 9>;
>> +		io-channel-names = "batt_v", "batt_chrg_i",
>> +			"batt_dischrg_i";
Is this stuff fixed for the device?
>> +	}
>> +};
>> -- 
>> 2.9.3
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

^ permalink raw reply

* Re: [PATCH 08/22] power: supply: add AC power supply driver for AXP20X and AXP22X PMICs
From: Jonathan Cameron @ 2017-01-07 19:31 UTC (permalink / raw)
  To: Quentin Schulz, knaack.h, lars, pmeerw, robh+dt, mark.rutland,
	wens, sre, linux, maxime.ripard, lee.jones
  Cc: thomas.petazzoni, devicetree, linux-pm, linux-iio, linux-kernel,
	bonbons, icenowy, linux-arm-kernel
In-Reply-To: <20170102163723.7939-9-quentin.schulz@free-electrons.com>

On 02/01/17 11:37, Quentin Schulz wrote:
> The X-Powers AXP20X and AXP22X PMICs expose the status of AC power
> supply.
> 
> Moreover, the AXP20X can also expose the current current and voltage
> values of the AC power supply.
> 
> This adds the driver which exposes the status of the AC power supply of
> the AXP20X and AXP22X PMICs.
> 
> Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
For the IIO bits
Acked-by: Jonathan Cameron <jic23@kernel.org>

Trivial comment inline.
> ---
>  drivers/power/supply/Kconfig           |  12 ++
>  drivers/power/supply/Makefile          |   1 +
>  drivers/power/supply/axp20x_ac_power.c | 251 +++++++++++++++++++++++++++++++++
>  3 files changed, 264 insertions(+)
>  create mode 100644 drivers/power/supply/axp20x_ac_power.c
> 
> diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
> index 76806a0..c552b4b 100644
> --- a/drivers/power/supply/Kconfig
> +++ b/drivers/power/supply/Kconfig
> @@ -214,6 +214,18 @@ config BATTERY_DA9150
>  	  This driver can also be built as a module. If so, the module will be
>  	  called da9150-fg.
>  
> +config CHARGER_AXP20X
> +	tristate "X-Powers AXP20X and AXP22X AC power supply driver"
> +	depends on MFD_AXP20X
> +	depends on AXP20X_ADC
> +	depends on IIO
> +	help
> +	  Say Y here to enable support for X-Powers AXP20X and AXP22X PMICs' AC
> +	  power supply.
> +
> +	  This driver can also be built as a module. If so, the module will be
> +	  called axp20x_ac_power.
> +
>  config AXP288_CHARGER
>  	tristate "X-Powers AXP288 Charger"
>  	depends on MFD_AXP20X && EXTCON_AXP288
> diff --git a/drivers/power/supply/Makefile b/drivers/power/supply/Makefile
> index 36c599d..7d22417 100644
> --- a/drivers/power/supply/Makefile
> +++ b/drivers/power/supply/Makefile
> @@ -18,6 +18,7 @@ obj-$(CONFIG_TEST_POWER)	+= test_power.o
>  
>  obj-$(CONFIG_BATTERY_88PM860X)	+= 88pm860x_battery.o
>  obj-$(CONFIG_BATTERY_ACT8945A)	+= act8945a_charger.o
> +obj-$(CONFIG_CHARGER_AXP20X)	+= axp20x_ac_power.o
>  obj-$(CONFIG_BATTERY_DS2760)	+= ds2760_battery.o
>  obj-$(CONFIG_BATTERY_DS2780)	+= ds2780_battery.o
>  obj-$(CONFIG_BATTERY_DS2781)	+= ds2781_battery.o
> diff --git a/drivers/power/supply/axp20x_ac_power.c b/drivers/power/supply/axp20x_ac_power.c
> new file mode 100644
> index 0000000..d7bc25c
> --- /dev/null
> +++ b/drivers/power/supply/axp20x_ac_power.c
> @@ -0,0 +1,251 @@
> +/*
> + * AXP20X and AXP22X PMICs' ACIN power supply driver
> + *
> + * Copyright (C) 2016 Free Electrons
> + *	Quentin Schulz <quentin.schulz@free-electrons.com>
> + *
> + * 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.
> + */
> +
> +#include <linux/device.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/kernel.h>
> +#include <linux/mfd/axp20x.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/power_supply.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
> +#include <linux/iio/consumer.h>
> +
> +#define AXP20X_PWR_STATUS_ACIN_PRESENT	BIT(7)
> +#define AXP20X_PWR_STATUS_ACIN_AVAIL	BIT(6)
> +
> +#define DRVNAME "axp20x-ac-power-supply"
> +
> +struct axp20x_ac_power {
> +	struct device_node *np;
> +	struct regmap *regmap;
> +	struct power_supply *supply;
> +	int axp20x_id;
> +	struct iio_channel *acin_v;
> +	struct iio_channel *acin_i;
> +};
> +
> +static irqreturn_t axp20x_ac_power_irq(int irq, void *devid)
> +{
> +	struct axp20x_ac_power *power = devid;
> +
> +	power_supply_changed(power->supply);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int axp20x_ac_power_get_property(struct power_supply *psy,
> +					enum power_supply_property psp,
> +					union power_supply_propval *val)
> +{
> +	struct axp20x_ac_power *power = power_supply_get_drvdata(psy);
> +	int ret, reg;
> +
> +	switch (psp) {
> +	case POWER_SUPPLY_PROP_HEALTH:
> +		ret = regmap_read(power->regmap, AXP20X_PWR_INPUT_STATUS, &reg);
> +		if (ret)
> +			return ret;
> +
> +		if (reg & AXP20X_PWR_STATUS_ACIN_PRESENT) {
> +			val->intval = POWER_SUPPLY_HEALTH_GOOD;
> +			return 0;
> +		}
> +
> +		val->intval = POWER_SUPPLY_HEALTH_UNKNOWN;
> +		return 0;
> +
> +	case POWER_SUPPLY_PROP_PRESENT:
> +		ret = regmap_read(power->regmap, AXP20X_PWR_INPUT_STATUS, &reg);
> +		if (ret)
> +			return ret;
> +
> +		val->intval = !!(reg & AXP20X_PWR_STATUS_ACIN_PRESENT);
> +		return 0;
> +
> +	case POWER_SUPPLY_PROP_ONLINE:
> +		ret = regmap_read(power->regmap, AXP20X_PWR_INPUT_STATUS, &reg);
> +		if (ret)
> +			return ret;
> +
> +		val->intval = !!(reg & AXP20X_PWR_STATUS_ACIN_AVAIL);
> +		return 0;
> +
> +	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
> +		ret = iio_read_channel_processed(power->acin_v, &val->intval);
> +		if (ret)
> +			return ret;
> +
> +		/*
> +		 * IIO framework gives mV but Power Supply framework gives µV.
> +		 */
single line comment syntax throughout or we'll have to face a patch 'fixing' it.
> +		val->intval *= 1000;
> +
> +		return 0;
> +
> +	case POWER_SUPPLY_PROP_CURRENT_NOW:
> +		ret = iio_read_channel_processed(power->acin_i, &val->intval);
> +		if (ret)
> +			return ret;
> +
> +		/*
> +		 * IIO framework gives mV but Power Supply framework gives µV.
> +		 */
> +		val->intval *= 1000;
> +
> +		return 0;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static enum power_supply_property axp20x_ac_power_properties[] = {
> +	POWER_SUPPLY_PROP_HEALTH,
> +	POWER_SUPPLY_PROP_PRESENT,
> +	POWER_SUPPLY_PROP_ONLINE,
> +	POWER_SUPPLY_PROP_VOLTAGE_NOW,
> +	POWER_SUPPLY_PROP_CURRENT_NOW,
> +};
> +
> +static enum power_supply_property axp22x_ac_power_properties[] = {
> +	POWER_SUPPLY_PROP_HEALTH,
> +	POWER_SUPPLY_PROP_PRESENT,
> +	POWER_SUPPLY_PROP_ONLINE,
> +};
> +
> +static const struct power_supply_desc axp20x_ac_power_desc = {
> +	.name = "axp20x-ac",
> +	.type = POWER_SUPPLY_TYPE_MAINS,
> +	.properties = axp20x_ac_power_properties,
> +	.num_properties = ARRAY_SIZE(axp20x_ac_power_properties),
> +	.get_property = axp20x_ac_power_get_property,
> +};
> +
> +static const struct power_supply_desc axp22x_ac_power_desc = {
> +	.name = "axp22x-ac",
> +	.type = POWER_SUPPLY_TYPE_MAINS,
> +	.properties = axp22x_ac_power_properties,
> +	.num_properties = ARRAY_SIZE(axp22x_ac_power_properties),
> +	.get_property = axp20x_ac_power_get_property,
> +};
> +
> +static int axp20x_ac_power_probe(struct platform_device *pdev)
> +{
> +	struct axp20x_dev *axp20x = dev_get_drvdata(pdev->dev.parent);
> +	struct power_supply_config psy_cfg = {};
> +	struct axp20x_ac_power *power;
> +	static const char * const axp20x_irq_names[] = { "ACIN_PLUGIN",
> +		"ACIN_REMOVAL", NULL };
> +	static const char * const *irq_names;
> +	const struct power_supply_desc *ac_power_desc;
> +	int i, irq, ret;
> +
> +	if (!of_device_is_available(pdev->dev.of_node))
> +		return -ENODEV;
> +
> +	if (!axp20x) {
> +		dev_err(&pdev->dev, "Parent drvdata not set\n");
> +		return -EINVAL;
> +	}
> +
> +	power = devm_kzalloc(&pdev->dev, sizeof(*power), GFP_KERNEL);
> +	if (!power)
> +		return -ENOMEM;
> +
> +	power->axp20x_id = (int)of_device_get_match_data(&pdev->dev);
> +
> +	irq_names = axp20x_irq_names;
> +
> +	if (power->axp20x_id == AXP202_ID) {
> +		ac_power_desc = &axp20x_ac_power_desc;
> +
> +		power->acin_v = devm_iio_channel_get(&pdev->dev, "acin_v");
> +		if (IS_ERR(power->acin_v)) {
> +			if (PTR_ERR(power->acin_v) == -ENODEV)
> +				return -EPROBE_DEFER;
> +			return PTR_ERR(power->acin_v);
> +		}
> +
> +		power->acin_i = devm_iio_channel_get(&pdev->dev, "acin_i");
> +		if (IS_ERR(power->acin_i)) {
> +			if (PTR_ERR(power->acin_i) == -ENODEV)
> +				return -EPROBE_DEFER;
> +			return PTR_ERR(power->acin_i);
> +		}
> +	} else {
> +		ac_power_desc = &axp22x_ac_power_desc;
> +	}
> +
> +	power->np = pdev->dev.of_node;
> +	power->regmap = axp20x->regmap;
> +
> +	platform_set_drvdata(pdev, power);
> +
> +	psy_cfg.of_node = pdev->dev.of_node;
> +	psy_cfg.drv_data = power;
> +
> +	power->supply = devm_power_supply_register(&pdev->dev, ac_power_desc,
> +						   &psy_cfg);
> +	if (IS_ERR(power->supply))
> +		return PTR_ERR(power->supply);
> +
> +	/* Request irqs after registering, as irqs may trigger immediately */
> +	for (i = 0; irq_names[i]; i++) {
> +		irq = platform_get_irq_byname(pdev, irq_names[i]);
> +		if (irq < 0) {
> +			dev_warn(&pdev->dev, "No IRQ for %s: %d\n",
> +				 irq_names[i], irq);
> +			continue;
> +		}
> +		irq = regmap_irq_get_virq(axp20x->regmap_irqc, irq);
> +		ret = devm_request_any_context_irq(&pdev->dev, irq,
> +						   axp20x_ac_power_irq, 0,
> +						   DRVNAME, power);
> +		if (ret < 0)
> +			dev_warn(&pdev->dev, "Error requesting %s IRQ: %d\n",
> +				 irq_names[i], ret);
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id axp20x_ac_power_match[] = {
> +	{
> +		.compatible = "x-powers,axp202-ac-power-supply",
> +		.data = (void *)AXP202_ID,
> +	}, {
> +		.compatible = "x-powers,axp221-ac-power-supply",
> +		.data = (void *)AXP221_ID,
> +	}, { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, axp20x_ac_power_match);
> +
> +static struct platform_driver axp20x_ac_power_driver = {
> +	.probe = axp20x_ac_power_probe,
> +	.driver = {
> +		.name = DRVNAME,
> +		.of_match_table = axp20x_ac_power_match,
> +	},
> +};
> +
> +module_platform_driver(axp20x_ac_power_driver);
> +
> +MODULE_AUTHOR("Quentin Schulz <quentin.schulz@free-electrons.com>");
> +MODULE_DESCRIPTION("AXP20X and AXP22X PMICs' AC power supply driver");
> +MODULE_LICENSE("GPL");
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply

* Re: [PATCH 07/22] dt-bindings: power: supply: add AXP20X/AXP22X AC power supply
From: Jonathan Cameron @ 2017-01-07 19:26 UTC (permalink / raw)
  To: Chen-Yu Tsai, Quentin Schulz
  Cc: Mark Rutland, Rob Herring, Lars-Peter Clausen, devicetree,
	linux-iio, linux-kernel, open list:THERMAL, Sebastian Reichel,
	Russell King, Bruno Prémont, Icenowy Zheng,
	Peter Meerwald-Stadler, knaack.h, Maxime Ripard, Lee Jones,
	Thomas Petazzoni, linux-arm-kernel
In-Reply-To: <CAGb2v67V6oGEyYGsBWfp3rVVm_VWQta0szeBiNSKUQR7awm5ng@mail.gmail.com>

On 05/01/17 01:17, Chen-Yu Tsai wrote:
> Hi Quentin,
> 
> On Wed, Jan 4, 2017 at 9:14 PM, Rob Herring <robh@kernel.org> wrote:
>> On Mon, Jan 02, 2017 at 05:37:07PM +0100, Quentin Schulz wrote:
>>> The X-Powers AXP20X and AXP22X PMICs have an AC entry to supply power to
>>> the board. They have a few registers dedicated to the status of the AC
>>> power supply.
>>>
>>> This adds the DT binding documentation for the AC power supply for
>>> AXP20X and AXP22X PMICs.
>>>
>>> Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
>>> ---
>>>  .../bindings/power/supply/axp20x_ac_power.txt      | 28 ++++++++++++++++++++++
>>>  1 file changed, 28 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/power/supply/axp20x_ac_power.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/power/supply/axp20x_ac_power.txt b/Documentation/devicetree/bindings/power/supply/axp20x_ac_power.txt
>>> new file mode 100644
>>> index 0000000..16d0de4
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/power/supply/axp20x_ac_power.txt
>>> @@ -0,0 +1,28 @@
>>> +AXP20X and AXP22X PMICs' AC power supply
>>> +
>>> +Required Properties:
>>> + - compatible: One of:
>>> +                     "x-powers,axp202-ac-power-supply"
>>> +                     "x-powers,axp221-ac-power-supply"
>>> +
>>> +More Required Properties for AXP20X PMICs:
>>> + - io-channels: phandles to ACIN voltage and current ADC channels
>>> + - io-channel-names = "acin_v", "acin_i";
>>> +
>>> +This node is a subnode of the axp20x PMIC.
>>> +
>>> +The AXP20X can read the current current and voltage supplied by AC by
>>> +reading ADC channels from the AXP20X ADC.
>>> +
>>> +The AXP22X is only able to tell if an AC power supply is present and
>>> +usable.
>>> +
>>> +Example:
>>> +
>>> +&axp209 {
>>> +     ac_power_supply: ac_power_supply {
>>
>> power-supply {
>>
>>> +             compatible = "x-powers,axp202-ac-power-supply";
>>> +             io-channels = <&axp209_adc 0>, <&axp209_adc 1>;
>>
>> Is this assignment fixed? If so, then it doesn't need to be in DT.
> 
> Is there any case that we actually need to use the IIO channels
> from the device tree? Seems to me its limited to the other AXP
> sub-devices.
> 
> If so you could use struct iio_map to map the channels to other
> devices by name. See axp288_adc for an example.
Agreed. When we are within a device (so it's not flexible) that
is the way to go.
> 
> Regards
> ChenYu
> 
>>> +             io-channel-names = "acin_v", "acin_i";
>>> +     };
>>> +};
>>> --
>>> 2.9.3
>>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

^ permalink raw reply

* Re: [PATCH 03/22] iio: adc: add support for X-Powers AXP20X and AXP22X PMICs ADCs
From: Jonathan Cameron @ 2017-01-07 19:23 UTC (permalink / raw)
  To: Chen-Yu Tsai, Quentin Schulz
  Cc: Mark Rutland, devicetree, Lars-Peter Clausen, open list:THERMAL,
	linux-iio, linux-kernel, Sebastian Reichel, Russell King,
	Bruno Prémont, Rob Herring, Icenowy Zheng,
	Peter Meerwald-Stadler, knaack.h, Maxime Ripard, Lee Jones,
	Thomas Petazzoni, linux-arm-kernel
In-Reply-To: <CAGb2v654a898jhPL+CJ0K4acTENq+sRo+ZJ4EG1zHqu73o=CGA@mail.gmail.com>

On 05/01/17 05:28, Chen-Yu Tsai wrote:
> On Thu, Jan 5, 2017 at 5:50 PM, Quentin Schulz
> <quentin.schulz@free-electrons.com> wrote:
>> On 05/01/2017 09:27, Chen-Yu Tsai wrote:
>>> On Thu, Jan 5, 2017 at 4:06 PM, Quentin Schulz
>>> <quentin.schulz@free-electrons.com> wrote:
>>>> Hi Chen-Yu,
>>>>
>>>> On 05/01/2017 06:42, Chen-Yu Tsai wrote:
>>>>> On Tue, Jan 3, 2017 at 12:37 AM, Quentin Schulz
>>>>> <quentin.schulz@free-electrons.com> wrote:
>>>> [...]
>>>>>> +
>>>>>> +#define AXP20X_ADC_RATE_MASK                   (3 << 6)
>>>>>> +#define AXP20X_ADC_RATE_25HZ                   (0 << 6)
>>>>>> +#define AXP20X_ADC_RATE_50HZ                   BIT(6)
>>>>>
>>>>> Please be consistent with the format.
>>>>>
>>>>>> +#define AXP20X_ADC_RATE_100HZ                  (2 << 6)
>>>>>> +#define AXP20X_ADC_RATE_200HZ                  (3 << 6)
>>>>>> +
>>>>>> +#define AXP22X_ADC_RATE_100HZ                  (0 << 6)
>>>>>> +#define AXP22X_ADC_RATE_200HZ                  BIT(6)
>>>>>> +#define AXP22X_ADC_RATE_400HZ                  (2 << 6)
>>>>>> +#define AXP22X_ADC_RATE_800HZ                  (3 << 6)
>>>>>
>>>>> These are power-of-2 multiples of some base rate. May I suggest
>>>>> a formula macro instead. Either way, you seem to be using only
>>>>> one value. Will this be made configurable in the future?
>>>>>
>>>>
>>>> Yes, I could use a formula macro instead. No plan to make it
>>>> configurable, should I make it configurable?
>>>
>>> I don't see a use case for that atm.
>>>
>>>>>> +
>>>>>> +#define AXP20X_ADC_CHANNEL(_channel, _name, _type, _reg)       \
>>>>>> +       {                                                       \
>>>>>> +               .type = _type,                                  \
>>>>>> +               .indexed = 1,                                   \
>>>>>> +               .channel = _channel,                            \
>>>>>> +               .address = _reg,                                \
>>>>>> +               .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |  \
>>>>>> +                                     BIT(IIO_CHAN_INFO_SCALE), \
>>>>>> +               .datasheet_name = _name,                        \
>>>>>> +       }
>>>>>> +
>>>>>> +#define AXP20X_ADC_CHANNEL_OFFSET(_channel, _name, _type, _reg) \
>>>>>> +       {                                                       \
>>>>>> +               .type = _type,                                  \
>>>>>> +               .indexed = 1,                                   \
>>>>>> +               .channel = _channel,                            \
>>>>>> +               .address = _reg,                                \
>>>>>> +               .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |  \
>>>>>> +                                     BIT(IIO_CHAN_INFO_SCALE) |\
>>>>>> +                                     BIT(IIO_CHAN_INFO_OFFSET),\
>>>>>> +               .datasheet_name = _name,                        \
>>>>>> +       }
>>>>>> +
>>>>>> +struct axp20x_adc_iio {
>>>>>> +       struct iio_dev          *indio_dev;
>>>>>> +       struct regmap           *regmap;
>>>>>> +};
>>>>>> +
>>>>>> +enum axp20x_adc_channel {
>>>>>> +       AXP20X_ACIN_V = 0,
>>>>>> +       AXP20X_ACIN_I,
>>>>>> +       AXP20X_VBUS_V,
>>>>>> +       AXP20X_VBUS_I,
>>>>>> +       AXP20X_TEMP_ADC,
>>>>>
>>>>> PMIC_TEMP would be better. And please save a slot for TS input.
>>>>>
>>>>
>>>> ACK.
>>>>
>>>> Hum.. I'm wondering what should be the IIO type of the TS input channel
>>>> then? The TS Pin can be used in two modes: either to monitor the
>>>> temperature of the battery or as an external ADC, at least that's what I
>>>> understand from the datasheet.
>>>
>>> AFAIK the battery charge/discharge high/low temperature threshold
>>> registers take values in terms of voltage, not actual temperature.
>>> And the temperature readout kind of depends on the thermoresistor
>>> one is using. So I think "voltage" would be the proper type.
>>>
>>
>> ACK. Should I just add TS_IN in axp20x_adc_channel enum but not add it
>> in the exposed IIO channels ("saving" the slot but not using it)?
> 
> Sure. Or you could skip the number with
> 
>     AXP20X_GPIO0_V = 6,
> 
>>>>
>>>>>> +       AXP20X_GPIO0_V,
>>>>>> +       AXP20X_GPIO1_V,
>>>>>
>>>>> Please skip a slot for "battery instantaneous power".
>>>>>
>>>>>> +       AXP20X_BATT_V,
>>>>>> +       AXP20X_BATT_CHRG_I,
>>>>>> +       AXP20X_BATT_DISCHRG_I,
>>>>>> +       AXP20X_IPSOUT_V,
>>>>>> +};
>>>>>> +
>>>>>> +enum axp22x_adc_channel {
>>>>>> +       AXP22X_TEMP_ADC = 0,
>>>>>
>>>>> Same comments as AXP20X_TEMP_ADC.
>>>>>
>>>>>> +       AXP22X_BATT_V,
>>>>>> +       AXP22X_BATT_CHRG_I,
>>>>>> +       AXP22X_BATT_DISCHRG_I,
>>>>>> +};
>>>>>
>>>>> Shouldn't these channel numbers be exported as part of the device tree
>>>>> bindings? At the very least, they shouldn't be changed.
>>>>>
>>>>
>>>> I don't understand what you mean by that. Do you mean you want a
>>>> consistent numbering between the AXP20X and the AXP22X, so that
>>>> AXP22X_BATT_V would have the same channel number than AXP20X_BATT_V?
>>>>
>>>> Could you explain a bit more your thoughts on the channel numbers being
>>>> exported as part of the device tree bindings?
>>>
>>> What I meant was that, since you are referencing the channels in the
>>> device tree, the numbering scheme would be part of the device tree
>>> binding, and should never be changed. So either these would be macros
>>> in include/dt-bindings/, or a big warning should be put before it.
>>>
>>
>> ACK.
>>
>>> But see my reply on patch 7, about do we actually need to expose this
>>> in the device tree.
>>>
>>
>> I don't know what's the best.
> 
> Then let's not expose it in the device tree for now. It's easier to
> add it later than to remove it.
> 
>>
>>>>> Also please add a comment saying that the channels are numbered
>>>>> in the order of their respective registers, and not the table
>>>>> describing the ADCs in the datasheet (9.7 Signal Capture for AXP209
>>>>> and 9.5 E-Gauge for AXP221).
>>>>>
>>>>
>>>> Yes I can.
>>>>
>>>> What about Rob wanting channel numbers to start at zero for each
>>>> different IIO type (i.e., today we have AXP22X_BATT_CHRG_I being
>>>> exported as in_current1_raw whereas he wants in_current0_raw).
>>>
>>> Hmm... I missed this. Are you talking about IIO or hwmon? IIRC
>>> hwmon numbers things starting at 1.
>>>
>>
>> About IIO.
>>
>> Today, we have exposed:
>> in_voltage0_raw for acin_v
>> in_current1_raw for acin_i
>> in_voltage2_raw for vbus_v
>> in_current3_raw for vbus_i
>> in_temp4_raw for adc_temp
>> in_voltage5_raw for gpio0_v
>> in_voltage6_raw for gpio1_v
>> in_voltage7_raw for batt_v
>> in_current8_raw for batt_chrg_i
>> in_current9_raw for batt_dischrg_i
>> in_voltage10_raw for ipsout_v
>>
>> but I think what Rob wants is:
>>
>> in_voltage0_raw acin_v
>> in_current0_raw for acin_i
>> in_voltage1_raw for vbus_v
>> in_current1_raw for vbus_i
>> in_temp_raw for adc_temp
>> in_voltage2_raw for gpio0_v
>> in_voltage3_raw for gpio1_v
>> in_voltage4_raw for batt_v
>> in_current2_raw for batt_chrg_i
>> in_current3_raw for batt_dischrg_i
>> in_voltage5_raw for ipsout_v
> 
> I think that's doable. If I understand IIO code correctly, the channel
> number is not used anywhere in the core code for dereferencing. It's
> used for sysfs entries (when .indexed is set) and events. However
> the twl4030 and twl6030 drivers use our current exposed scheme.
> I suppose its best to get some input from the IIO maintainer.
If we are doing it to superficially group channels that are measuring
different things about the same physical system then I'm fine with
mappings with holes in them.

It's not actually specified anywhere that we can't allow this and
IIRC there are a few drivers doing exactly this.
> 
> So if we want, we could have the following channel mapping:
> 
>   0 -> acin
>   1 -> vbus
>   2 -> pmic
>   3 -> gpio0
>   4 -> gpio1
>   5 -> ipsout
>   6 -> battery
> 
> Each channel could have mulitple entries in axp20x_adc_channels[],
> one for each type of reading. You might need multiple channels for
> the battery to cover charge and discharge currents.
> 
> Your callback functions would get a bit messier though.
> 
>>
>>>> [...]
>>>>>> +static int axp22x_adc_read_raw(struct iio_dev *indio_dev,
>>>>>> +                              struct iio_chan_spec const *channel, int *val,
>>>>>> +                              int *val2)
>>>>>> +{
>>>>>> +       struct axp20x_adc_iio *info = iio_priv(indio_dev);
>>>>>> +       int size = 12, ret;
>>>>>> +
>>>>>> +       switch (channel->channel) {
>>>>>> +       case AXP22X_BATT_DISCHRG_I:
>>>>>> +               size = 13;
>>>>>> +       case AXP22X_TEMP_ADC:
>>>>>> +       case AXP22X_BATT_V:
>>>>>> +       case AXP22X_BATT_CHRG_I:
>>>>>
>>>>> According to the datasheet, AXP22X_BATT_CHRG_I is also 13 bits wide.
>>>>>
>>>>
>>>> Where did you get that?
>>>>
>>>> Also, the datasheet is inconsistent:
>>>>  - 9.5 E-Gauge Fuel Gauge system => the min value is at 0x0 and the max
>>>> value at 0xfff for all channels, that's 12 bits.
>>>>  - 10.1.4 ADC Data => all channels except battery discharge current are
>>>> on 12 bits (8 high, 4 low).
>>>
>>> My datasheets (AXP221 v1.6, AXP221s v1.2, AXP223 v1.1, all Chinese) say
>>> in 10.1.4:
>>>
>>>   - 7A: battery charge current high 5 bits
>>>   - 7B: battery charge current low 8 bits
>>>   - 7C: battery discharge current high 5 bits
>>>   - 7D: battery discharge current low 8 bits
>>>
>>
>> AXP223 v1.1 in English in 10.1.4[1]:
>>     - 7A: battery charge current high 8 bits
>>     - 7B: battery charge current low 4 bits
>>     - 7C: battery discharge current high 8 bits
>>     - 7D: battery discharge current low 5 bits
>>
>> Note that it's 8 bits for high and 4/5 bits for low while you wrote 4/5
>> bits high and low 8 bits (typos or actually what's written in the
>> datasheet?).
> 
> Typo on my end, sorry. It's high 8bits and low 5/4 bits.
> 
> Apart from that, the Chinese and English versions don't match for the
> battery charge current. :(
> 
> Would it be possible for you to test this? As in, have the module running,
> and charging a battery, and have a multimeter in series with the battery
> to verify the readings.
> 
> Regards
> ChenYu
> 
>>
>> Hum.. from the reg reading function[2] I would say that the correct
>> formula is high on 8 bits and low on 4/5 bits.
>>
>> So hum.. what do we do?
>>
>> [1] http://dl.linux-sunxi.org/AXP/AXP223-en.pdf
>> [2] http://lxr.free-electrons.com/source/include/linux/mfd/axp20x.h#L564
>>
>>>>
>>>> [...]
>>>>>> +static int axp22x_read_raw(struct iio_dev *indio_dev,
>>>>>> +                          struct iio_chan_spec const *chan, int *val,
>>>>>> +                          int *val2, long mask)
>>>>>> +{
>>>>>> +       switch (mask) {
>>>>>> +       case IIO_CHAN_INFO_OFFSET:
>>>>>> +               *val = -2667;
>>>>>
>>>>> Datasheet says -267.7 C, or -2677 here.
>>>>>
>>>>
>>>> The formula in the datasheet is (in milli Celsius):
>>>>  processed = raw * 100 - 266700;
>>>>
>>>> while the IIO framework asks for a scale and an offset which are then
>>>> applied as:
>>>>  processed = (raw + offset) * scale;
>>>>
>>>> Thus by factorizing, we get:
>>>>  processed = (raw - 2667) * 100;
>>>
>>> What I meant was that your lower end value is off by one degree,
>>> -266.7 in your code vs -267.7 in the datasheet.
>>>
>>
>> Indeed. Thanks.
>>
>>>>
>>>> [...]
>>>>>> +static int axp20x_remove(struct platform_device *pdev)
>>>>>> +{
>>>>>> +       struct axp20x_adc_iio *info;
>>>>>> +       struct iio_dev *indio_dev = platform_get_drvdata(pdev);
>>>>>> +
>>>>>> +       info = iio_priv(indio_dev);
>>>>>
>>>>> Nit: you could just reverse the 2 declarations above and join this
>>>>> line after struct axp20x_adc_iio *info;
>>>>>
>>>>>> +       regmap_write(info->regmap, AXP20X_ADC_EN1, 0);
>>>>>> +       regmap_write(info->regmap, AXP20X_ADC_EN2, 0);
>>>>>
>>>>> The existing VBUS power supply driver enables the VBUS ADC bits itself,
>>>>> and does not check them later on. This means if one were to remove this
>>>>> axp20x-adc module, the voltage/current readings in the VBUS power supply
>>>>> would be invalid. Some sort of workaround would be needed here in this
>>>>> driver of the VBUS driver.
>>>>>
>>>>
>>>> That would be one reason to migrate the VBUS driver to use the IIO
>>>> channels, wouldn't it?
>>>
>>> It is, preferably without changing the device tree.
>>>
>>
>> Yes, of course.
>>
>> Thanks,
>> Quentin
>>
>> --
>> Quentin Schulz, Free Electrons
>> Embedded Linux and Kernel engineering
>> http://free-electrons.com
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

^ permalink raw reply

* Re: [PATCH 03/22] iio: adc: add support for X-Powers AXP20X and AXP22X PMICs ADCs
From: Jonathan Cameron @ 2017-01-07 19:20 UTC (permalink / raw)
  To: Quentin Schulz, Chen-Yu Tsai
  Cc: Mark Rutland, devicetree, Lars-Peter Clausen, open list:THERMAL,
	linux-iio, linux-kernel, Sebastian Reichel, Russell King,
	Bruno Prémont, Rob Herring, Icenowy Zheng,
	Peter Meerwald-Stadler, knaack.h, Maxime Ripard, Lee Jones,
	Thomas Petazzoni, linux-arm-kernel
In-Reply-To: <cf4590d1-5381-f1da-7271-e6e7fee0c479@free-electrons.com>

On 05/01/17 04:50, Quentin Schulz wrote:
> On 05/01/2017 09:27, Chen-Yu Tsai wrote:
>> On Thu, Jan 5, 2017 at 4:06 PM, Quentin Schulz
>> <quentin.schulz@free-electrons.com> wrote:
>>> Hi Chen-Yu,
>>>
>>> On 05/01/2017 06:42, Chen-Yu Tsai wrote:
>>>> On Tue, Jan 3, 2017 at 12:37 AM, Quentin Schulz
>>>> <quentin.schulz@free-electrons.com> wrote:
>>> [...]
>>>>> +
>>>>> +#define AXP20X_ADC_RATE_MASK                   (3 << 6)
>>>>> +#define AXP20X_ADC_RATE_25HZ                   (0 << 6)
>>>>> +#define AXP20X_ADC_RATE_50HZ                   BIT(6)
>>>>
>>>> Please be consistent with the format.
>>>>
>>>>> +#define AXP20X_ADC_RATE_100HZ                  (2 << 6)
>>>>> +#define AXP20X_ADC_RATE_200HZ                  (3 << 6)
>>>>> +
>>>>> +#define AXP22X_ADC_RATE_100HZ                  (0 << 6)
>>>>> +#define AXP22X_ADC_RATE_200HZ                  BIT(6)
>>>>> +#define AXP22X_ADC_RATE_400HZ                  (2 << 6)
>>>>> +#define AXP22X_ADC_RATE_800HZ                  (3 << 6)
>>>>
>>>> These are power-of-2 multiples of some base rate. May I suggest
>>>> a formula macro instead. Either way, you seem to be using only
>>>> one value. Will this be made configurable in the future?
>>>>
>>>
>>> Yes, I could use a formula macro instead. No plan to make it
>>> configurable, should I make it configurable?
>>
>> I don't see a use case for that atm.
>>
>>>>> +
>>>>> +#define AXP20X_ADC_CHANNEL(_channel, _name, _type, _reg)       \
>>>>> +       {                                                       \
>>>>> +               .type = _type,                                  \
>>>>> +               .indexed = 1,                                   \
>>>>> +               .channel = _channel,                            \
>>>>> +               .address = _reg,                                \
>>>>> +               .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |  \
>>>>> +                                     BIT(IIO_CHAN_INFO_SCALE), \
>>>>> +               .datasheet_name = _name,                        \
>>>>> +       }
>>>>> +
>>>>> +#define AXP20X_ADC_CHANNEL_OFFSET(_channel, _name, _type, _reg) \
>>>>> +       {                                                       \
>>>>> +               .type = _type,                                  \
>>>>> +               .indexed = 1,                                   \
>>>>> +               .channel = _channel,                            \
>>>>> +               .address = _reg,                                \
>>>>> +               .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |  \
>>>>> +                                     BIT(IIO_CHAN_INFO_SCALE) |\
>>>>> +                                     BIT(IIO_CHAN_INFO_OFFSET),\
>>>>> +               .datasheet_name = _name,                        \
>>>>> +       }
>>>>> +
>>>>> +struct axp20x_adc_iio {
>>>>> +       struct iio_dev          *indio_dev;
>>>>> +       struct regmap           *regmap;
>>>>> +};
>>>>> +
>>>>> +enum axp20x_adc_channel {
>>>>> +       AXP20X_ACIN_V = 0,
>>>>> +       AXP20X_ACIN_I,
>>>>> +       AXP20X_VBUS_V,
>>>>> +       AXP20X_VBUS_I,
>>>>> +       AXP20X_TEMP_ADC,
>>>>
>>>> PMIC_TEMP would be better. And please save a slot for TS input.
>>>>
>>>
>>> ACK.
>>>
>>> Hum.. I'm wondering what should be the IIO type of the TS input channel
>>> then? The TS Pin can be used in two modes: either to monitor the
>>> temperature of the battery or as an external ADC, at least that's what I
>>> understand from the datasheet.
>>
>> AFAIK the battery charge/discharge high/low temperature threshold
>> registers take values in terms of voltage, not actual temperature.
>> And the temperature readout kind of depends on the thermoresistor
>> one is using. So I think "voltage" would be the proper type.
>>
> 
> ACK. Should I just add TS_IN in axp20x_adc_channel enum but not add it
> in the exposed IIO channels ("saving" the slot but not using it)?
Ideally we'd put an IIO consumer driver on the channel that handles the
thermoresistor explicitly.  Would need consumer support for events
though to be much use.
> 
>>>
>>>>> +       AXP20X_GPIO0_V,
>>>>> +       AXP20X_GPIO1_V,
>>>>
>>>> Please skip a slot for "battery instantaneous power".
>>>>
>>>>> +       AXP20X_BATT_V,
>>>>> +       AXP20X_BATT_CHRG_I,
>>>>> +       AXP20X_BATT_DISCHRG_I,
>>>>> +       AXP20X_IPSOUT_V,
>>>>> +};
>>>>> +
>>>>> +enum axp22x_adc_channel {
>>>>> +       AXP22X_TEMP_ADC = 0,
>>>>
>>>> Same comments as AXP20X_TEMP_ADC.
>>>>
>>>>> +       AXP22X_BATT_V,
>>>>> +       AXP22X_BATT_CHRG_I,
>>>>> +       AXP22X_BATT_DISCHRG_I,
>>>>> +};
>>>>
>>>> Shouldn't these channel numbers be exported as part of the device tree
>>>> bindings? At the very least, they shouldn't be changed.
>>>>
>>>
>>> I don't understand what you mean by that. Do you mean you want a
>>> consistent numbering between the AXP20X and the AXP22X, so that
>>> AXP22X_BATT_V would have the same channel number than AXP20X_BATT_V?
>>>
>>> Could you explain a bit more your thoughts on the channel numbers being
>>> exported as part of the device tree bindings?
>>
>> What I meant was that, since you are referencing the channels in the
>> device tree, the numbering scheme would be part of the device tree
>> binding, and should never be changed. So either these would be macros
>> in include/dt-bindings/, or a big warning should be put before it.
>>
> 
> ACK.
> 
>> But see my reply on patch 7, about do we actually need to expose this
>> in the device tree.
>>
> 
> I don't know what's the best.
> 
>>>> Also please add a comment saying that the channels are numbered
>>>> in the order of their respective registers, and not the table
>>>> describing the ADCs in the datasheet (9.7 Signal Capture for AXP209
>>>> and 9.5 E-Gauge for AXP221).
>>>>
>>>
>>> Yes I can.
>>>
>>> What about Rob wanting channel numbers to start at zero for each
>>> different IIO type (i.e., today we have AXP22X_BATT_CHRG_I being
>>> exported as in_current1_raw whereas he wants in_current0_raw).
>>
>> Hmm... I missed this. Are you talking about IIO or hwmon? IIRC
>> hwmon numbers things starting at 1.
>>
> 
> About IIO.
> 
> Today, we have exposed:
> in_voltage0_raw for acin_v
> in_current1_raw for acin_i
> in_voltage2_raw for vbus_v
> in_current3_raw for vbus_i
> in_temp4_raw for adc_temp
> in_voltage5_raw for gpio0_v
> in_voltage6_raw for gpio1_v
> in_voltage7_raw for batt_v
> in_current8_raw for batt_chrg_i
> in_current9_raw for batt_dischrg_i
> in_voltage10_raw for ipsout_v
> 
> but I think what Rob wants is:
> 
> in_voltage0_raw acin_v
> in_current0_raw for acin_i
> in_voltage1_raw for vbus_v
> in_current1_raw for vbus_i
> in_temp_raw for adc_temp
> in_voltage2_raw for gpio0_v
> in_voltage3_raw for gpio1_v
> in_voltage4_raw for batt_v
> in_current2_raw for batt_chrg_i
> in_current3_raw for batt_dischrg_i
> in_voltage5_raw for ipsout_v
That would be standard choice.  It's not disastrous to skip numbers but
it tends to not be what userspace expects.
> 
>>> [...]
>>>>> +static int axp22x_adc_read_raw(struct iio_dev *indio_dev,
>>>>> +                              struct iio_chan_spec const *channel, int *val,
>>>>> +                              int *val2)
>>>>> +{
>>>>> +       struct axp20x_adc_iio *info = iio_priv(indio_dev);
>>>>> +       int size = 12, ret;
>>>>> +
>>>>> +       switch (channel->channel) {
>>>>> +       case AXP22X_BATT_DISCHRG_I:
>>>>> +               size = 13;
>>>>> +       case AXP22X_TEMP_ADC:
>>>>> +       case AXP22X_BATT_V:
>>>>> +       case AXP22X_BATT_CHRG_I:
>>>>
>>>> According to the datasheet, AXP22X_BATT_CHRG_I is also 13 bits wide.
>>>>
>>>
>>> Where did you get that?
>>>
>>> Also, the datasheet is inconsistent:
>>>  - 9.5 E-Gauge Fuel Gauge system => the min value is at 0x0 and the max
>>> value at 0xfff for all channels, that's 12 bits.
>>>  - 10.1.4 ADC Data => all channels except battery discharge current are
>>> on 12 bits (8 high, 4 low).
>>
>> My datasheets (AXP221 v1.6, AXP221s v1.2, AXP223 v1.1, all Chinese) say
>> in 10.1.4:
>>
>>   - 7A: battery charge current high 5 bits
>>   - 7B: battery charge current low 8 bits
>>   - 7C: battery discharge current high 5 bits
>>   - 7D: battery discharge current low 8 bits
>>
> 
> AXP223 v1.1 in English in 10.1.4[1]:
>     - 7A: battery charge current high 8 bits
>     - 7B: battery charge current low 4 bits
>     - 7C: battery discharge current high 8 bits
>     - 7D: battery discharge current low 5 bits
> 
> Note that it's 8 bits for high and 4/5 bits for low while you wrote 4/5
> bits high and low 8 bits (typos or actually what's written in the
> datasheet?).
> 
> Hum.. from the reg reading function[2] I would say that the correct
> formula is high on 8 bits and low on 4/5 bits.
> 
> So hum.. what do we do?
> 
> [1] http://dl.linux-sunxi.org/AXP/AXP223-en.pdf
> [2] http://lxr.free-electrons.com/source/include/linux/mfd/axp20x.h#L564
> 
>>>
>>> [...]
>>>>> +static int axp22x_read_raw(struct iio_dev *indio_dev,
>>>>> +                          struct iio_chan_spec const *chan, int *val,
>>>>> +                          int *val2, long mask)
>>>>> +{
>>>>> +       switch (mask) {
>>>>> +       case IIO_CHAN_INFO_OFFSET:
>>>>> +               *val = -2667;
>>>>
>>>> Datasheet says -267.7 C, or -2677 here.
>>>>
>>>
>>> The formula in the datasheet is (in milli Celsius):
>>>  processed = raw * 100 - 266700;
>>>
>>> while the IIO framework asks for a scale and an offset which are then
>>> applied as:
>>>  processed = (raw + offset) * scale;
>>>
>>> Thus by factorizing, we get:
>>>  processed = (raw - 2667) * 100;
>>
>> What I meant was that your lower end value is off by one degree,
>> -266.7 in your code vs -267.7 in the datasheet.
>>
> 
> Indeed. Thanks.
> 
>>>
>>> [...]
>>>>> +static int axp20x_remove(struct platform_device *pdev)
>>>>> +{
>>>>> +       struct axp20x_adc_iio *info;
>>>>> +       struct iio_dev *indio_dev = platform_get_drvdata(pdev);
>>>>> +
>>>>> +       info = iio_priv(indio_dev);
>>>>
>>>> Nit: you could just reverse the 2 declarations above and join this
>>>> line after struct axp20x_adc_iio *info;
>>>>
>>>>> +       regmap_write(info->regmap, AXP20X_ADC_EN1, 0);
>>>>> +       regmap_write(info->regmap, AXP20X_ADC_EN2, 0);
>>>>
>>>> The existing VBUS power supply driver enables the VBUS ADC bits itself,
>>>> and does not check them later on. This means if one were to remove this
>>>> axp20x-adc module, the voltage/current readings in the VBUS power supply
>>>> would be invalid. Some sort of workaround would be needed here in this
>>>> driver of the VBUS driver.
>>>>
>>>
>>> That would be one reason to migrate the VBUS driver to use the IIO
>>> channels, wouldn't it?
>>
>> It is, preferably without changing the device tree.
>>
> 
> Yes, of course.
> 
> Thanks,
> Quentin
> 

^ permalink raw reply

* Re: [linux-sunxi][PATCH 3/3] ARM: dts: sun6i: Add SPDIF to the Mele I7
From: Chen-Yu Tsai @ 2017-01-07 19:16 UTC (permalink / raw)
  To: Code Kipper; +Cc: Maxime Ripard, linux-arm-kernel, devicetree, linux-sunxi
In-Reply-To: <20161220104038.22532-4-codekipper-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

On Tue, Dec 20, 2016 at 6:40 PM,  <codekipper-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> From: Marcus Cooper <codekipper-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>
> Enable the S/PDIF transmitter that is present on the Mele I7.
>
> Signed-off-by: Marcus Cooper <codekipper-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

Acked-by: Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>

This patch should be ready to be merged. The associated clk
and dtsi changes are already in Maxime's tree.

> ---
>  arch/arm/boot/dts/sun6i-a31-i7.dts | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
>
> diff --git a/arch/arm/boot/dts/sun6i-a31-i7.dts b/arch/arm/boot/dts/sun6i-a31-i7.dts
> index a2193309a199..2bc57d2dcd80 100644
> --- a/arch/arm/boot/dts/sun6i-a31-i7.dts
> +++ b/arch/arm/boot/dts/sun6i-a31-i7.dts
> @@ -69,6 +69,23 @@
>                         gpios = <&pio 7 13 GPIO_ACTIVE_HIGH>;
>                 };
>         };
> +
> +       sound {
> +               compatible = "simple-audio-card";
> +               simple-audio-card,name = "On-board SPDIF";
> +               simple-audio-card,cpu {
> +                       sound-dai = <&spdif>;
> +               };
> +
> +               simple-audio-card,codec {
> +                       sound-dai = <&spdif_out>;
> +               };
> +       };
> +
> +       spdif_out: spdif-out {
> +               #sound-dai-cells = <0>;
> +               compatible = "linux,spdif-dit";
> +       };
>  };
>
>  &codec {
> @@ -138,6 +155,13 @@
>         status = "okay";
>  };
>
> +&spdif {
> +       pinctrl-names = "default";
> +       pinctrl-0 = <&spdif_pins_a>;
> +       spdif-out = "okay";
> +       status = "okay";
> +};
> +
>  &uart0 {
>         pinctrl-names = "default";
>         pinctrl-0 = <&uart0_pins_a>;
> --
> 2.11.0
>
> --
> You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
> For more options, visit https://groups.google.com/d/optout.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH 03/22] iio: adc: add support for X-Powers AXP20X and AXP22X PMICs ADCs
From: Jonathan Cameron @ 2017-01-07 19:13 UTC (permalink / raw)
  To: Quentin Schulz, knaack.h, lars, pmeerw, robh+dt, mark.rutland,
	wens, sre, linux, maxime.ripard, lee.jones
  Cc: thomas.petazzoni, devicetree, linux-pm, linux-iio, linux-kernel,
	bonbons, icenowy, linux-arm-kernel
In-Reply-To: <20170102163723.7939-4-quentin.schulz@free-electrons.com>

On 02/01/17 11:37, Quentin Schulz wrote:
> The X-Powers AXP20X and AXP22X PMICs have multiple ADCs. They expose the
> battery voltage, battery charge and discharge currents, AC-in and VBUS
> voltages and currents, 2 GPIOs muxable in ADC mode and PMIC temperature.
> 
> This adds support for most of AXP20X and AXP22X ADCs.
> 
> Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
Couple of little bits from me. Peter got the bigger stuff.

Jonathan
> ---
>  drivers/iio/adc/Kconfig      |  10 +
>  drivers/iio/adc/Makefile     |   1 +
>  drivers/iio/adc/axp20x_adc.c | 490 +++++++++++++++++++++++++++++++++++++++++++
>  include/linux/mfd/axp20x.h   |   4 +
>  4 files changed, 505 insertions(+)
>  create mode 100644 drivers/iio/adc/axp20x_adc.c
> 
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 38bc319..5c5b51f 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -154,6 +154,16 @@ config AT91_SAMA5D2_ADC
>  	  To compile this driver as a module, choose M here: the module will be
>  	  called at91-sama5d2_adc.
>  
> +config AXP20X_ADC
> +	tristate "X-Powers AXP20X and AXP22X ADC driver"
> +	depends on MFD_AXP20X
> +	help
> +	  Say yes here to have support for X-Powers power management IC (PMIC)
> +	  AXP20X and AXP22X ADC devices.
> +
> +	  To compile this driver as a module, choose M here: the module will be
> +	  called axp20x_adc.
> +
>  config AXP288_ADC
>  	tristate "X-Powers AXP288 ADC driver"
>  	depends on MFD_AXP20X
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index d36c4be..f5c28a5 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -16,6 +16,7 @@ obj-$(CONFIG_AD7887) += ad7887.o
>  obj-$(CONFIG_AD799X) += ad799x.o
>  obj-$(CONFIG_AT91_ADC) += at91_adc.o
>  obj-$(CONFIG_AT91_SAMA5D2_ADC) += at91-sama5d2_adc.o
> +obj-$(CONFIG_AXP20X_ADC) += axp20x_adc.o
>  obj-$(CONFIG_AXP288_ADC) += axp288_adc.o
>  obj-$(CONFIG_BCM_IPROC_ADC) += bcm_iproc_adc.o
>  obj-$(CONFIG_BERLIN2_ADC) += berlin2-adc.o
> diff --git a/drivers/iio/adc/axp20x_adc.c b/drivers/iio/adc/axp20x_adc.c
> new file mode 100644
> index 0000000..8df972a
> --- /dev/null
> +++ b/drivers/iio/adc/axp20x_adc.c
> @@ -0,0 +1,490 @@
> +/* ADC driver for AXP20X and AXP22X PMICs
> + *
> + * Copyright (c) 2016 Free Electrons NextThing Co.
> + *	Quentin Schulz <quentin.schulz@free-electrons.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.
> + *
Pet hate : why a blank line here? ;) I'm grumpy - my flight home is delayed.
> + */
> +
> +#include <linux/completion.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/regmap.h>
> +#include <linux/thermal.h>
> +
> +#include <linux/iio/iio.h>
> +#include <linux/mfd/axp20x.h>
> +
> +#define AXP20X_ADC_EN1_MASK			GENMASK(7, 0)
> +
> +#define AXP20X_ADC_EN2_MASK			(GENMASK(3, 2) | BIT(7))
> +#define AXP22X_ADC_EN1_MASK			(GENMASK(7, 5) | BIT(0))
> +#define AXP20X_ADC_EN2_TEMP_ADC			BIT(7)
> +#define AXP20X_ADC_EN2_GPIO0_ADC		BIT(3)
> +#define AXP20X_ADC_EN2_GPIO1_ADC		BIT(2)
> +
> +#define AXP20X_GPIO10_IN_RANGE_GPIO0		BIT(0)
> +#define AXP20X_GPIO10_IN_RANGE_GPIO1		BIT(1)
> +#define AXP20X_GPIO10_IN_RANGE_GPIO0_VAL(x)	((x) & BIT(0))
> +#define AXP20X_GPIO10_IN_RANGE_GPIO1_VAL(x)	(((x) & BIT(0)) << 1)
> +
> +#define AXP20X_ADC_RATE_MASK			(3 << 6)
> +#define AXP20X_ADC_RATE_25HZ			(0 << 6)
> +#define AXP20X_ADC_RATE_50HZ			BIT(6)
> +#define AXP20X_ADC_RATE_100HZ			(2 << 6)
> +#define AXP20X_ADC_RATE_200HZ			(3 << 6)
> +
> +#define AXP22X_ADC_RATE_100HZ			(0 << 6)
> +#define AXP22X_ADC_RATE_200HZ			BIT(6)
> +#define AXP22X_ADC_RATE_400HZ			(2 << 6)
> +#define AXP22X_ADC_RATE_800HZ			(3 << 6)
> +
> +#define AXP20X_ADC_CHANNEL(_channel, _name, _type, _reg)	\
> +	{							\
> +		.type = _type,					\
> +		.indexed = 1,					\
> +		.channel = _channel,				\
> +		.address = _reg,				\
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |	\
> +				      BIT(IIO_CHAN_INFO_SCALE),	\
> +		.datasheet_name = _name,			\
> +	}
> +
> +#define AXP20X_ADC_CHANNEL_OFFSET(_channel, _name, _type, _reg) \
> +	{							\
> +		.type = _type,					\
> +		.indexed = 1,					\
> +		.channel = _channel,				\
> +		.address = _reg,				\
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |	\
> +				      BIT(IIO_CHAN_INFO_SCALE) |\
> +				      BIT(IIO_CHAN_INFO_OFFSET),\
> +		.datasheet_name = _name,			\
> +	}
> +
> +struct axp20x_adc_iio {
Why?  I'm not seeing this used anywhere.
> +	struct iio_dev		*indio_dev;
> +	struct regmap		*regmap;
> +};
> +
> +enum axp20x_adc_channel {
> +	AXP20X_ACIN_V = 0,
> +	AXP20X_ACIN_I,
> +	AXP20X_VBUS_V,
> +	AXP20X_VBUS_I,
> +	AXP20X_TEMP_ADC,
> +	AXP20X_GPIO0_V,
> +	AXP20X_GPIO1_V,
> +	AXP20X_BATT_V,
> +	AXP20X_BATT_CHRG_I,
> +	AXP20X_BATT_DISCHRG_I,
> +	AXP20X_IPSOUT_V,
> +};
> +
> +enum axp22x_adc_channel {
> +	AXP22X_TEMP_ADC = 0,
> +	AXP22X_BATT_V,
> +	AXP22X_BATT_CHRG_I,
> +	AXP22X_BATT_DISCHRG_I,
> +};
> +
> +static const struct iio_chan_spec axp20x_adc_channels[] = {
> +	AXP20X_ADC_CHANNEL(AXP20X_ACIN_V, "acin_v", IIO_VOLTAGE,
> +			   AXP20X_ACIN_V_ADC_H),
> +	AXP20X_ADC_CHANNEL(AXP20X_ACIN_I, "acin_i", IIO_CURRENT,
> +			   AXP20X_ACIN_I_ADC_H),
> +	AXP20X_ADC_CHANNEL(AXP20X_VBUS_V, "vbus_v", IIO_VOLTAGE,
> +			   AXP20X_VBUS_V_ADC_H),
> +	AXP20X_ADC_CHANNEL(AXP20X_VBUS_I, "vbus_i", IIO_CURRENT,
> +			   AXP20X_VBUS_I_ADC_H),
> +	AXP20X_ADC_CHANNEL_OFFSET(AXP20X_TEMP_ADC, "temp_adc", IIO_TEMP,
> +				  AXP20X_TEMP_ADC_H),
> +	AXP20X_ADC_CHANNEL_OFFSET(AXP20X_GPIO0_V, "gpio0_v", IIO_VOLTAGE,
> +				  AXP20X_GPIO0_V_ADC_H),
> +	AXP20X_ADC_CHANNEL_OFFSET(AXP20X_GPIO1_V, "gpio1_v", IIO_VOLTAGE,
> +				  AXP20X_GPIO1_V_ADC_H),
> +	AXP20X_ADC_CHANNEL(AXP20X_BATT_V, "batt_v", IIO_VOLTAGE,
> +			   AXP20X_BATT_V_H),
> +	AXP20X_ADC_CHANNEL(AXP20X_BATT_CHRG_I, "batt_chrg_i", IIO_CURRENT,
> +			   AXP20X_BATT_CHRG_I_H),
> +	AXP20X_ADC_CHANNEL(AXP20X_BATT_DISCHRG_I, "batt_dischrg_i", IIO_CURRENT,
> +			   AXP20X_BATT_DISCHRG_I_H),
> +	AXP20X_ADC_CHANNEL(AXP20X_IPSOUT_V, "ipsout_v", IIO_VOLTAGE,
> +			   AXP20X_IPSOUT_V_HIGH_H),
> +};
> +
> +static const struct iio_chan_spec axp22x_adc_channels[] = {
> +	AXP20X_ADC_CHANNEL_OFFSET(AXP22X_TEMP_ADC, "temp_adc", IIO_TEMP,
> +				  AXP22X_TEMP_ADC_H),
> +	AXP20X_ADC_CHANNEL(AXP22X_BATT_V, "batt_v", IIO_VOLTAGE,
> +			   AXP20X_BATT_V_H),
> +	AXP20X_ADC_CHANNEL(AXP22X_BATT_CHRG_I, "batt_chrg_i", IIO_CURRENT,
> +			   AXP20X_BATT_CHRG_I_H),
> +	AXP20X_ADC_CHANNEL(AXP22X_BATT_DISCHRG_I, "batt_dischrg_i", IIO_CURRENT,
> +			   AXP20X_BATT_DISCHRG_I_H),
> +};
> +
> +static int axp20x_adc_read_raw(struct iio_dev *indio_dev,
> +			       struct iio_chan_spec const *channel, int *val,
> +			       int *val2)
> +{
> +	struct axp20x_adc_iio *info = iio_priv(indio_dev);
> +	int size = 12, ret;
> +
> +	switch (channel->channel) {
> +	case AXP20X_BATT_DISCHRG_I:
> +		size = 13;
> +	case AXP20X_ACIN_V:
> +	case AXP20X_ACIN_I:
> +	case AXP20X_VBUS_V:
> +	case AXP20X_VBUS_I:
> +	case AXP20X_TEMP_ADC:
> +	case AXP20X_BATT_V:
> +	case AXP20X_BATT_CHRG_I:
> +	case AXP20X_IPSOUT_V:
> +	case AXP20X_GPIO0_V:
> +	case AXP20X_GPIO1_V:
> +		ret = axp20x_read_variable_width(info->regmap, channel->address,
> +						 size);
> +		if (ret < 0)
> +			return ret;
> +		*val = ret;
> +		return IIO_VAL_INT;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static int axp22x_adc_read_raw(struct iio_dev *indio_dev,
> +			       struct iio_chan_spec const *channel, int *val,
> +			       int *val2)
> +{
> +	struct axp20x_adc_iio *info = iio_priv(indio_dev);
> +	int size = 12, ret;
> +
> +	switch (channel->channel) {
> +	case AXP22X_BATT_DISCHRG_I:
> +		size = 13;
> +	case AXP22X_TEMP_ADC:
> +	case AXP22X_BATT_V:
> +	case AXP22X_BATT_CHRG_I:
> +		ret = axp20x_read_variable_width(info->regmap, channel->address,
> +						 size);
> +		if (ret < 0)
> +			return ret;
> +		*val = ret;
> +		return IIO_VAL_INT;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static int axp20x_adc_scale(int channel, int *val, int *val2)
> +{
> +	switch (channel) {
> +	case AXP20X_ACIN_V:
> +	case AXP20X_VBUS_V:
> +		*val = 1;
> +		*val2 = 700000;
> +		return IIO_VAL_INT_PLUS_MICRO;
> +
> +	case AXP20X_ACIN_I:
> +		*val = 0;
> +		*val2 = 625000;
> +		return IIO_VAL_INT_PLUS_MICRO;
> +
> +	case AXP20X_VBUS_I:
> +		*val = 0;
> +		*val2 = 375000;
> +		return IIO_VAL_INT_PLUS_MICRO;
> +
> +	case AXP20X_TEMP_ADC:
> +		*val = 100;
> +		return IIO_VAL_INT;
> +
> +	case AXP20X_GPIO0_V:
> +	case AXP20X_GPIO1_V:
> +		*val = 0;
> +		*val2 = 500000;
> +		return IIO_VAL_INT_PLUS_MICRO;
> +
> +	case AXP20X_BATT_V:
> +		*val = 1;
> +		*val2 = 100000;
> +		return IIO_VAL_INT_PLUS_MICRO;
> +
> +	case AXP20X_BATT_DISCHRG_I:
> +	case AXP20X_BATT_CHRG_I:
> +		*val = 0;
> +		*val2 = 500000;
> +		return IIO_VAL_INT_PLUS_MICRO;
> +
> +	case AXP20X_IPSOUT_V:
> +		*val = 1;
> +		*val2 = 400000;
> +		return IIO_VAL_INT_PLUS_MICRO;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static int axp22x_adc_scale(int channel, int *val, int *val2)
> +{
> +	switch (channel) {
> +	case AXP22X_TEMP_ADC:
> +		*val = 100;
> +		return IIO_VAL_INT;
> +
> +	case AXP22X_BATT_V:
> +		*val = 1;
> +		*val2 = 100000;
> +		return IIO_VAL_INT_PLUS_MICRO;
> +
> +	case AXP22X_BATT_DISCHRG_I:
> +	case AXP22X_BATT_CHRG_I:
> +		*val = 0;
> +		*val2 = 500000;
> +		return IIO_VAL_INT_PLUS_MICRO;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static int axp20x_adc_offset(struct iio_dev *indio_dev, int channel, int *val)
> +{
> +	struct axp20x_adc_iio *info = iio_priv(indio_dev);
> +	int ret, reg;
> +
> +	switch (channel) {
> +	case AXP20X_TEMP_ADC:
> +		*val = -1447;
> +		return IIO_VAL_INT;
> +
> +	case AXP20X_GPIO0_V:
> +	case AXP20X_GPIO1_V:
> +		ret = regmap_read(info->regmap, AXP20X_GPIO10_IN_RANGE, &reg);
> +		if (ret < 0)
> +			return ret;
> +
> +		if (channel == AXP20X_GPIO0_V)
> +			*val = reg & AXP20X_GPIO10_IN_RANGE_GPIO0;
> +		else
> +			*val = reg & AXP20X_GPIO10_IN_RANGE_GPIO1;
> +
> +		*val = !!(*val) * 700000;
> +
> +		return IIO_VAL_INT;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static int axp20x_read_raw(struct iio_dev *indio_dev,
> +			   struct iio_chan_spec const *chan, int *val,
> +			   int *val2, long mask)
> +{
> +	switch (mask) {
> +	case IIO_CHAN_INFO_OFFSET:
> +		return axp20x_adc_offset(indio_dev, chan->channel, val);
> +
> +	case IIO_CHAN_INFO_SCALE:
> +		return axp20x_adc_scale(chan->channel, val, val2);
> +
> +	case IIO_CHAN_INFO_RAW:
> +		return axp20x_adc_read_raw(indio_dev, chan, val, val2);
> +
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static int axp22x_read_raw(struct iio_dev *indio_dev,
> +			   struct iio_chan_spec const *chan, int *val,
> +			   int *val2, long mask)
> +{
> +	switch (mask) {
> +	case IIO_CHAN_INFO_OFFSET:
> +		*val = -2667;
> +		return IIO_VAL_INT;
> +
> +	case IIO_CHAN_INFO_SCALE:
> +		return axp22x_adc_scale(chan->channel, val, val2);
> +
> +	case IIO_CHAN_INFO_RAW:
> +		return axp22x_adc_read_raw(indio_dev, chan, val, val2);
> +
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static int axp20x_write_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *chan, int val, int val2,
> +			    long mask)
> +{
> +	struct axp20x_adc_iio *info = iio_priv(indio_dev);
> +
> +	/*
> +	 * The AXP20X PMIC allows the user to choose between 0V and 0.7V offsets
> +	 * for (independently) GPIO0 and GPIO1 when in ADC mode.
> +	 */
> +	if (mask != IIO_CHAN_INFO_OFFSET)
> +		return -EINVAL;
> +
> +	if (chan->channel != AXP20X_GPIO0_V && chan->channel != AXP20X_GPIO1_V)
> +		return -EINVAL;
> +
> +	if (val != 0 && val != 700000)
> +		return -EINVAL;
> +
> +	if (chan->channel == AXP20X_GPIO0_V)
> +		return regmap_update_bits(info->regmap, AXP20X_GPIO10_IN_RANGE,
> +					  AXP20X_GPIO10_IN_RANGE_GPIO0,
> +					  AXP20X_GPIO10_IN_RANGE_GPIO0_VAL(!!val));
> +
> +	return regmap_update_bits(info->regmap, AXP20X_GPIO10_IN_RANGE,
> +				  AXP20X_GPIO10_IN_RANGE_GPIO1,
> +				  AXP20X_GPIO10_IN_RANGE_GPIO1_VAL(!!val));
> +}
> +
> +static const struct iio_info axp20x_adc_iio_info = {
> +	.read_raw = axp20x_read_raw,
> +	.write_raw = axp20x_write_raw,
> +	.driver_module = THIS_MODULE,
> +};
> +
> +static const struct iio_info axp22x_adc_iio_info = {
> +	.read_raw = axp22x_read_raw,
> +	.driver_module = THIS_MODULE,
> +};
> +
> +static const struct of_device_id axp20x_adc_of_match[] = {
> +	{ .compatible = "x-powers,axp209-adc", .data = (void *)AXP209_ID, },
> +	{ .compatible = "x-powers,axp221-adc", .data = (void *)AXP221_ID, },
> +	{ /* sentinel */ },
> +};
> +
> +static int axp20x_probe(struct platform_device *pdev)
> +{
> +	struct axp20x_adc_iio *info;
> +	struct iio_dev *indio_dev;
> +	struct axp20x_dev *axp20x_dev;
> +	int ret, axp20x_id;
> +
> +	axp20x_dev = dev_get_drvdata(pdev->dev.parent);
> +
> +	indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*info));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	info = iio_priv(indio_dev);
> +	platform_set_drvdata(pdev, indio_dev);
> +
> +	info->regmap = axp20x_dev->regmap;
> +	info->indio_dev = indio_dev;
> +	indio_dev->name = dev_name(&pdev->dev);
> +	indio_dev->dev.parent = &pdev->dev;
> +	indio_dev->dev.of_node = pdev->dev.of_node;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +
> +	axp20x_id = (int)of_device_get_match_data(&pdev->dev);
> +
> +	switch (axp20x_id) {
> +	case AXP209_ID:
> +		indio_dev->info = &axp20x_adc_iio_info;
> +		indio_dev->num_channels = ARRAY_SIZE(axp20x_adc_channels);
> +		indio_dev->channels = axp20x_adc_channels;
> +
> +		/* Enable the ADCs on IP */
> +		regmap_write(info->regmap, AXP20X_ADC_EN1, AXP20X_ADC_EN1_MASK);
> +
> +		/* Enable GPIO0/1 and internal temperature ADCs */
> +		regmap_update_bits(info->regmap, AXP20X_ADC_EN2,
> +				   AXP20X_ADC_EN2_MASK, AXP20X_ADC_EN2_MASK);
> +
> +		/* Configure ADCs rate */
> +		regmap_update_bits(info->regmap, AXP20X_ADC_RATE,
> +				   AXP20X_ADC_RATE_MASK, AXP20X_ADC_RATE_50HZ);
> +		break;
> +
> +	case AXP221_ID:
> +		indio_dev->info = &axp22x_adc_iio_info;
> +		indio_dev->num_channels = ARRAY_SIZE(axp22x_adc_channels);
> +		indio_dev->channels = axp22x_adc_channels;
> +
> +		/* Enable the ADCs on IP */
> +		regmap_write(info->regmap, AXP20X_ADC_EN1, AXP22X_ADC_EN1_MASK);
> +
> +		/* Configure ADCs rate */
> +		regmap_update_bits(info->regmap, AXP20X_ADC_RATE,
> +				   AXP20X_ADC_RATE_MASK, AXP22X_ADC_RATE_200HZ);
> +		break;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	ret = devm_iio_device_register(&pdev->dev, indio_dev);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "could not register the device\n");
> +		regmap_write(info->regmap, AXP20X_ADC_EN1, 0);
> +		regmap_write(info->regmap, AXP20X_ADC_EN2, 0);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int axp20x_remove(struct platform_device *pdev)
> +{
> +	struct axp20x_adc_iio *info;
> +	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> +
> +	info = iio_priv(indio_dev);
> +	regmap_write(info->regmap, AXP20X_ADC_EN1, 0);
> +	regmap_write(info->regmap, AXP20X_ADC_EN2, 0);
> +
> +	return 0;
> +}
> +
> +static struct platform_driver axp20x_adc_driver = {
> +	.driver = {
> +		.name = "axp20x-adc",
> +		.of_match_table = axp20x_adc_of_match,
> +	},
> +	.probe = axp20x_probe,
> +	.remove = axp20x_remove,
> +};
> +
> +module_platform_driver(axp20x_adc_driver);
> +
> +MODULE_DESCRIPTION("ADC driver for AXP20X and AXP22X PMICs");
> +MODULE_AUTHOR("Quentin Schulz <quentin.schulz@free-electrons.com>");
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/mfd/axp20x.h b/include/linux/mfd/axp20x.h
> index a4860bc..650c6f6 100644
> --- a/include/linux/mfd/axp20x.h
> +++ b/include/linux/mfd/axp20x.h
> @@ -150,6 +150,10 @@ enum {
>  #define AXP20X_VBUS_I_ADC_L		0x5d
>  #define AXP20X_TEMP_ADC_H		0x5e
>  #define AXP20X_TEMP_ADC_L		0x5f
> +
> +#define AXP22X_TEMP_ADC_H		0x56
> +#define AXP22X_TEMP_ADC_L		0x57
> +
>  #define AXP20X_TS_IN_H			0x62
>  #define AXP20X_TS_IN_L			0x63
>  #define AXP20X_GPIO0_V_ADC_H		0x64
> 

^ permalink raw reply

* Re: [PATCH v2 0/8] ASoC: sunxi: Add support for audio codec in A23/H3 SoCs
From: Chen-Yu Tsai @ 2017-01-07 19:00 UTC (permalink / raw)
  To: Chen-Yu Tsai; +Cc: Maxime Ripard, linux-arm-kernel, linux-kernel, devicetree
In-Reply-To: <20161125123442.28410-1-wens-jdAy2FN1RRM@public.gmane.org>

Hi Maxime,

On Fri, Nov 25, 2016 at 8:34 PM, Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org> wrote:
> Hi everyone,
>
> This is v2 of my Allwinner A23 and H3 audio codec support series.
>
> Changes since v1:
>
>   - Use DEFINE_RES_MEM for the analog path controls block resources.
>   - Added Rob's ack.
>
> This series adds support for the audio codec found in Allwinner A23 and
> H3 SoCs. The design and data paths are similar to the audio codec found
> in earlier SoCs such as the A31. The analog audio paths are symmetrical
> with left/right channels and down-mix selectors for mono differential
> output.
>
> What deviates from previous SoCs is that the analog path controls have
> been moved to a separate control bus, accessed through a message box
> like register interface in the PRCM block. This necessitates writing
> a separate component driver for it, which is then tied into the sound
> card as an ASoC auxiliary device.
>
> Patch 1 adds the analog path controls block to the sun6i-prcm driver as
> a sub-device, for the A23. The H3 currently does not use the PRCM driver.
>
> Patch 2 adds PCM and card support for the A23 codec to the sun4i-codec
> driver.
>
> Patch 3 adds a device node for the analog path controls block to the A23
> dtsi.
>
> Patch 4 adds a device node for the audio codec, and the phandle for the
> analog path controls block to the A23 dtsi.
>
> Patch 5 enables the audio codec for the A23 Q8 tablets. On these tablets
> the headphone output is driven in DC coupled, or "direct drive", mode.
>
> Patch 6 adds PCM and card support for the H3 codec to the sun4i-codec
> driver.
>
> Patch 7 adds device nodes for the audio codec and analog path controls
> block to the H3 dtsi.
>
> Patch 8 enables the audio codec on the Orange Pi PC. The audio output
> jack on the board is tied to the line out pins on the SoC.

All the driver bits are in. Can you pick up the dts patches?

Thanks
ChenYu

>
>
> Please take a look and let me know what you think.
>
> In addition, the sun4i-codec driver is getting pretty large. Maybe we
> want to split the different parts into different files?
>
>
> Regards
> ChenYu
>
>
> Chen-Yu Tsai (8):
>   mfd: sun6i-prcm: Add codec analog controls sub-device for Allwinner
>     A23
>   ASoC: sun4i-codec: Add support for A23 codec
>   ARM: dts: sun8i: Add codec analog path controls node in PRCM for
>     A23/A33
>   ARM: dts: sun8i-a23: Add device node for internal audio codec
>   ARM: dts: sun8i-a23: q8-tablet: Enable internal audio codec
>   ASoC: sun4i-codec: Add support for H3 codec
>   ARM: dts: sun8i-h3: Add device nodes for audio codec and its analog
>     controls
>   ARM: dts: sun8i-h3: orange-pi-pc: Enable audio codec
>
>  .../devicetree/bindings/sound/sun4i-codec.txt      |  14 +-
>  arch/arm/boot/dts/sun8i-a23-a33.dtsi               |   4 +
>  arch/arm/boot/dts/sun8i-a23-q8-tablet.dts          |  23 +++
>  arch/arm/boot/dts/sun8i-a23.dtsi                   |  16 ++
>  arch/arm/boot/dts/sun8i-h3-orangepi-pc.dts         |   8 +
>  arch/arm/boot/dts/sun8i-h3.dtsi                    |  19 +++
>  drivers/mfd/sun6i-prcm.c                           |  13 ++
>  sound/soc/sunxi/sun4i-codec.c                      | 179 +++++++++++++++++++++
>  8 files changed, 274 insertions(+), 2 deletions(-)
>
> --
> 2.10.2
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH v3 4/4] phy: qcom-qmp: new qmp phy driver for qcom-chipsets
From: vivek.gautam-sgV2jX0FEOL9JmXXK+q4OQ @ 2017-01-07 18:41 UTC (permalink / raw)
  To: Bjorn Andersson
  Cc: robh+dt-DgEjT+Ai2ygdnm+yROfE0A, kishon-l0cyMroinI0,
	sboyd-sgV2jX0FEOL9JmXXK+q4OQ, linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA, mark.rutland-5wv7dgnIgG8,
	srinivas.kandagatla-QSEj5FYQhm4dnm+yROfE0A,
	linux-arm-msm-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-msm-owner-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20170106211722.GT10531@minitux>

On 2017-01-07 02:47, Bjorn Andersson wrote:
> On Fri 06 Jan 01:47 PST 2017, Vivek Gautam wrote:
> 
>> > > +static int qcom_qmp_phy_com_init(struct qcom_qmp_phy *qphy)
>> > > +{
>> > > +	const struct qmp_phy_cfg *cfg = qphy->cfg;
>> > > +	void __iomem *serdes = qphy->serdes;
>> > > +	int ret;
>> > > +
>> > > +	mutex_lock(&qphy->phy_mutex);
>> > > +	if (qphy->init_count++) {
>> > > +		mutex_unlock(&qphy->phy_mutex);
>> > > +		return 0;
>> > > +	}
>> > As far as I can see phy_init() and phy_exit() already keep reference
>> > count on the initialization and you only call this function from
>> > phy_ops->init, so you should be able to drop this.
>> This is an intermediary function that does the common block 
>> initialization.
>> PHYs like PCIe have a separate common block (apart from SerDes)
>> for all phy channels. We shouldn't program this common block
>> multiple times for each channel. That's why this init_count.
>> 
> 
> You're right!
> 
> Unfortunately it took me several minutes to wrap my head around the phy
> vs multi-lane and I have a really hard time keeping "qcom_qmp_phy" and
> "qmp_phy_desc" apart throughout the driver.
> 
> If I understand correctly the qcom_qmp_phy is the context representing 
> a
> "QMP block", while this is a PHY block it's not actually the phy in
> Linux eyes. The qcom_phy_desc represents a "QMP lane", which in Linux
> eyes is the phys, but as we think of QMP as the PHY this confused me.

That's correct. The qcom_qmp_phy structure represents the QMP phy block
as a whole and not the individual phy lane instances. These phy lanes
are represented by qcom_phy_desc, and are the actual PHYs in Linux eyes.

> 
> How about naming them "struct qmp" and "struct qmp_lane" (or possibly
> qmp_phy) instead? That way we remove the confusion of QMP PHY vs Linux
> PHY and we make the lane part explicit.

Sure, this sounds good to me - "struct qmp" and "struct qmp_phy" (will
call the respective variables as qblk for qmp block (struct qmp) and
qphy for struct qmp_phy).
Thanks for pointing out. Will change them.

Regards
Vivek

> 
> Regards,
> Bjorn
> 
> --
> To unsubscribe from this list: send the line "unsubscribe 
> linux-arm-msm" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH] adc: add adc driver for Hisilicon BVT SOCs
From: Jonathan Cameron @ 2017-01-07 17:51 UTC (permalink / raw)
  To: Allen Liu, knaack.h, lars, pmeerw, robh+dt, mark.rutland
  Cc: akinobu.mita, ludovic.desroches, krzk, vilhelm.gray,
	ksenija.stanojevic, zhiyong.tao, daniel.baluta, leonard.crestez,
	ray.jui, raveendra.padasalagi, mranostay, amsfield22, linux-iio,
	devicetree, linux-kernel, xuejiancheng, kevin.lixu
In-Reply-To: <1483784170-228765-1-git-send-email-liurenzhong@hisilicon.com>

On 07/01/17 05:16, Allen Liu wrote:
> Add ADC driver for the ADC controller found on HiSilicon BVT SOCs, like Hi3516CV300, etc.
> The ADC controller is primarily in charge of detecting voltage.
> 
> Reviewed-by: Kevin Li <kevin.lixu@hisilicon.com>
> Signed-off-by: Allen Liu <liurenzhong@hisilicon.com>
Hi Allen,

One quick submission process note first.  It is very important to clearly identify new
versions of a patch and what changes have occurred since the previous posting.

So the email title should have been [PATCH V2] adc...

Also, below the --- please add a brief change log.

The driver is coming together nicely.  A few minor points inline.

Jonathan
> ---
>  .../devicetree/bindings/iio/adc/hibvt-lsadc.txt    |  23 ++
>  drivers/iio/adc/Kconfig                            |  10 +
>  drivers/iio/adc/Makefile                           |   1 +
>  drivers/iio/adc/hibvt_lsadc.c                      | 335 +++++++++++++++++++++
>  4 files changed, 369 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/hibvt-lsadc.txt
>  create mode 100644 drivers/iio/adc/hibvt_lsadc.c
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/hibvt-lsadc.txt b/Documentation/devicetree/bindings/iio/adc/hibvt-lsadc.txt
> new file mode 100644
> index 0000000..fce1ff4
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/hibvt-lsadc.txt
> @@ -0,0 +1,23 @@
> +Hisilicon BVT Low Speed (LS) A/D Converter bindings
> +
> +Required properties:
> +- compatible: should be "hisilicon,<name>-lsadc"
> +   - "hisilicon,hi3516cv300-lsadc": for hi3516cv300
> +
> +- reg: physical base address of the controller and length of memory mapped 
> +	   region.
> +- interrupts: The interrupt number for the ADC device.
Ideally refer to the standard interrupt binding document.
Documentation/devicetree/bindings/interrupt-controller/interrupts.txt

> +
> +Optional properties:
> +- resets: Must contain an entry for each entry in reset-names if need support
> +		  this option. See ../../reset/reset.txt for details.
Don't use a relative path in a binding document. It's far too likely to
be broken by a reorganization of the docs and cannot be grepped for.
> +- reset-names: Must include the name "lsadc-crg".
> +
> +Example:
> +	adc: adc@120e0000 {
> +			compatible = "hisilicon,hi3516cv300-lsadc";
> +			reg = <0x120e0000 0x1000>;
> +			interrupts = <19>;
> +			resets = <&crg 0x7c 3>;
> +			reset-names = "lsadc-crg";
> +	};
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 99c0514..0443f51 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -225,6 +225,16 @@ config HI8435
>  	  This driver can also be built as a module. If so, the module will be
>  	  called hi8435.
>  
> +config HIBVT_LSADC
> +	tristate "HIBVT LSADC driver"
> +	depends on ARCH_HISI || COMPILE_TEST
> +	help
> +	  Say yes here to build support for the LSADC found in SoCs from
> +	  hisilicon BVT chip.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called hibvt_lsadc.
> +
>  config INA2XX_ADC
>  	tristate "Texas Instruments INA2xx Power Monitors IIO driver"
>  	depends on I2C && !SENSORS_INA2XX
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index 7a40c04..6554d92 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -23,6 +23,7 @@ obj-$(CONFIG_DA9150_GPADC) += da9150-gpadc.o
>  obj-$(CONFIG_EXYNOS_ADC) += exynos_adc.o
>  obj-$(CONFIG_FSL_MX25_ADC) += fsl-imx25-gcq.o
>  obj-$(CONFIG_HI8435) += hi8435.o
> +obj-$(CONFIG_HIBVT_LSADC) += hibvt_lsadc.o
>  obj-$(CONFIG_IMX7D_ADC) += imx7d_adc.o
>  obj-$(CONFIG_INA2XX_ADC) += ina2xx-adc.o
>  obj-$(CONFIG_LP8788_ADC) += lp8788_adc.o
> diff --git a/drivers/iio/adc/hibvt_lsadc.c b/drivers/iio/adc/hibvt_lsadc.c
> new file mode 100644
> index 0000000..aaf2024
> --- /dev/null
> +++ b/drivers/iio/adc/hibvt_lsadc.c
> @@ -0,0 +1,335 @@
> +/*
> + * Hisilicon BVT Low Speed (LS) A/D Converter
> + * Copyright (C) 2016 HiSilicon Technologies Co., Ltd.
> + *
> + * 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 program 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 General Public License for more details.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/clk.h>
> +#include <linux/completion.h>
> +#include <linux/delay.h>
> +#include <linux/reset.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/iio/iio.h>
> +
> +/* hisilicon bvt adc registers definitions */
> +#define HIBVT_LSADC_CONFIG		0x00
> +#define HIBVT_CONFIG_DEGLITCH	BIT(17)
> +#define HIBVT_CONFIG_RESET		BIT(15)
> +#define HIBVT_CONFIG_POWERDOWN	BIT(14)
> +#define HIBVT_CONFIG_MODE		BIT(13)
> +#define HIBVT_CONFIG_CHNC		BIT(10)
> +#define HIBVT_CONFIG_CHNB		BIT(9)
> +#define HIBVT_CONFIG_CHNA		BIT(8)
> +
> +#define HIBVT_LSADC_TIMESCAN	0x08
> +#define HIBVT_LSADC_INTEN		0x10
> +#define HIBVT_LSADC_INTSTATUS	0x14
> +#define HIBVT_LSADC_INTCLR		0x18
> +#define HIBVT_LSADC_START		0x1C
> +#define HIBVT_LSADC_STOP		0x20
> +#define HIBVT_LSADC_ACTBIT		0x24
> +#define HIBVT_LSADC_CHNDATA		0x2C
> +
> +#define HIBVT_LSADC_CON_EN		(1u << 0)
> +#define HIBVT_LSADC_CON_DEN		(0u << 0)
> +
> +#define HIBVT_LSADC_NUM_BITS_V1	10
> +#define HIBVT_LSADC_CHN_MASK_v1	0x7
> +
> +/* fix clk:3000000, default tscan set 10ms */
> +#define HIBVT_LSADC_TSCAN_MS	(10*3000)
> +
> +#define HIBVT_LSADC_TIMEOUT		msecs_to_jiffies(100)
> +
> +/* default voltage scale for every channel <mv> */
> +static int g_hibvt_lsadc_voltage[] = {
> +	3300, 3300, 3300
Is default due to an external reference voltage or is there an internal
regulator?  If it is external it should really be described using the
regulator framework.

Const? 
> +};
> +
> +struct hibvt_lsadc {
> +	void __iomem		*regs;
> +	struct completion	completion;
> +	struct reset_control	*reset;
> +	const struct hibvt_lsadc_data	*data;
> +	unsigned int		cur_chn;
> +	unsigned int		value;
> +};
> +
> +struct hibvt_lsadc_data {
> +	int				num_bits;
> +	const struct iio_chan_spec	*channels;
> +	int				num_channels;
> +
> +	void (*clear_irq)(struct hibvt_lsadc *info, int mask);
> +	void (*start_conv)(struct hibvt_lsadc *info);
> +	void (*stop_conv)(struct hibvt_lsadc *info);
> +};
> +
> +static int hibvt_lsadc_read_raw(struct iio_dev *indio_dev,
> +				    struct iio_chan_spec const *chan,
> +				    int *val, int *val2, long mask)
> +{
> +	struct hibvt_lsadc *info = iio_priv(indio_dev);
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		mutex_lock(&indio_dev->mlock);
> +
> +		reinit_completion(&info->completion);
> +
> +		/* Select the channel to be used */
> +		info->cur_chn = chan->channel;
> +
> +		if (info->data->start_conv)
> +			info->data->start_conv(info);
> +
> +		if (!wait_for_completion_timeout(&info->completion,
> +							HIBVT_LSADC_TIMEOUT)) {
> +			if (info->data->stop_conv)
> +				info->data->stop_conv(info);
> +			mutex_unlock(&indio_dev->mlock);
> +			return -ETIMEDOUT;
> +		}
> +
> +		*val = info->value;
> +		mutex_unlock(&indio_dev->mlock);
> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_SCALE:
> +		*val = g_hibvt_lsadc_voltage[chan->channel];
> +		*val2 = info->data->num_bits;
> +		return IIO_VAL_FRACTIONAL_LOG2;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static irqreturn_t hibvt_lsadc_isr(int irq, void *dev_id)
> +{
> +	struct hibvt_lsadc *info = (struct hibvt_lsadc *)dev_id;
> +	int mask;
> +
> +	mask = readl(info->regs + HIBVT_LSADC_INTSTATUS);
> +	if ((mask & HIBVT_LSADC_CHN_MASK_v1) == 0)
> +		return IRQ_NONE;
> +
> +	/* Clear irq */
> +	mask &= HIBVT_LSADC_CHN_MASK_v1;
> +	if (info->data->clear_irq)
> +		info->data->clear_irq(info, mask);
> +
> +	/* Read value */
> +	info->value = readl(info->regs +
> +		HIBVT_LSADC_CHNDATA + (info->cur_chn << 2));
> +	info->value &= GENMASK(info->data->num_bits - 1, 0);
> +
> +	/* stop adc */
> +	if (info->data->stop_conv)
> +		info->data->stop_conv(info);
> +
> +	complete(&info->completion);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static const struct iio_info hibvt_lsadc_iio_info = {
> +	.read_raw = hibvt_lsadc_read_raw,
> +	.driver_module = THIS_MODULE,
> +};
> +
> +#define HIBVT_LSADC_CHANNEL(_index, _id) {      \
> +	.type = IIO_VOLTAGE,                \
> +	.indexed = 1,						\
> +	.channel = _index,					\
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |  \
> +			BIT(IIO_CHAN_INFO_SCALE),   \
> +	.datasheet_name = _id,              \
> +}
> +
> +static const struct iio_chan_spec hibvt_lsadc_iio_channels[] = {
> +	HIBVT_LSADC_CHANNEL(0, "adc0"),
> +	HIBVT_LSADC_CHANNEL(1, "adc1"),
> +	HIBVT_LSADC_CHANNEL(2, "adc2"),
> +};
> +
> +static void hibvt_lsadc_v1_clear_irq(struct hibvt_lsadc *info, int mask)
> +{
> +	writel(mask, info->regs + HIBVT_LSADC_INTCLR);
> +}
> +
> +static void hibvt_lsadc_v1_start_conv(struct hibvt_lsadc *info)
> +{
> +	unsigned int con;
> +
> +	/* set number bit */
set number of bits?
> +	con = GENMASK(info->data->num_bits - 1, 0);
> +	writel(con, (info->regs + HIBVT_LSADC_ACTBIT));
> +
> +	/* config */
> +	con = readl(info->regs + HIBVT_LSADC_CONFIG);
> +	con &= ~HIBVT_CONFIG_RESET;
> +	con |= (HIBVT_CONFIG_POWERDOWN | HIBVT_CONFIG_DEGLITCH |
> +		HIBVT_CONFIG_MODE);
> +	con &= ~(HIBVT_CONFIG_CHNA | HIBVT_CONFIG_CHNB | HIBVT_CONFIG_CHNC);
> +	con |= (HIBVT_CONFIG_CHNA << info->cur_chn);
> +	writel(con, (info->regs + HIBVT_LSADC_CONFIG));
> +
> +	/* set timescan */
> +	writel(HIBVT_LSADC_TSCAN_MS, (info->regs + HIBVT_LSADC_TIMESCAN));
> +
> +	/* clear interrupt */
> +	writel(HIBVT_LSADC_CHN_MASK_v1, info->regs + HIBVT_LSADC_INTCLR);
> +
> +	/* enable interrupt */
> +	writel(HIBVT_LSADC_CON_EN, (info->regs + HIBVT_LSADC_INTEN));
> +
> +	/* start scan */
> +	writel(HIBVT_LSADC_CON_EN, (info->regs + HIBVT_LSADC_START));
> +}
> +
> +static void hibvt_lsadc_v1_stop_conv(struct hibvt_lsadc *info)
> +{
> +	/* reset the timescan */
This isn't a particularly common pice of terminology, perhaps a short
description here of what timescan is and why we should reset it would
make the code easier to follow.

> +	writel(HIBVT_LSADC_CON_DEN, (info->regs + HIBVT_LSADC_TIMESCAN));
> +
> +	/* disable interrupt */
> +	writel(HIBVT_LSADC_CON_DEN, (info->regs + HIBVT_LSADC_INTEN));
> +
> +	/* stop scan */
> +	writel(HIBVT_LSADC_CON_EN, (info->regs + HIBVT_LSADC_STOP));
> +}
> +
> +static const struct hibvt_lsadc_data lsadc_data_v1 = {
> +	.num_bits = HIBVT_LSADC_NUM_BITS_V1,
> +	.channels = hibvt_lsadc_iio_channels,
> +	.num_channels = ARRAY_SIZE(hibvt_lsadc_iio_channels),
> +
> +	.clear_irq = hibvt_lsadc_v1_clear_irq,
> +	.start_conv = hibvt_lsadc_v1_start_conv,
> +	.stop_conv = hibvt_lsadc_v1_stop_conv,
> +};
> +
> +static const struct of_device_id hibvt_lsadc_match[] = {
> +	{
> +		.compatible = "hisilicon,hi3516cv300-lsadc",
> +		.data = &lsadc_data_v1,
The usual convention is to only introduce 'variant' type data as a
precursor patch to a series including the support of new parts.

It is acceptable to post a version with this in if you are shortly to submit
the follow up that adds other device support.  If you are doing this,
please put a note in the patch description to that effect.  Note that if
the additional support doesn't turn up, the driver may we get 'simplified'
by someone else.

I'd also generally expect to see this match table further down - directly
above where it is used.  Makes for ever so slightly easier reviewing!
> +	},
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, hibvt_lsadc_match);
> +
> +/* Reset LSADC Controller */
> +static void hibvt_lsadc_reset_controller(struct reset_control *reset)
> +{
> +	reset_control_assert(reset);
> +	usleep_range(10, 20);
> +	reset_control_deassert(reset);
> +}
> +
> +static int hibvt_lsadc_probe(struct platform_device *pdev)
> +{
> +	struct hibvt_lsadc *info = NULL;
> +	struct device_node *np = pdev->dev.of_node;
> +	struct iio_dev *indio_dev = NULL;
> +	struct resource	*mem;
> +	const struct of_device_id *match;
> +	int ret;
> +	int irq;
> +
> +	if (!np)
> +		return -ENODEV;
> +
> +	indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*info));
> +	if (!indio_dev) {
> +		dev_err(&pdev->dev, "failed allocating iio device\n");
> +		return -ENOMEM;
> +	}
> +	info = iio_priv(indio_dev);
> +
> +	match = of_match_device(hibvt_lsadc_match, &pdev->dev);
> +	info->data = match->data;
> +
> +	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	info->regs = devm_ioremap_resource(&pdev->dev, mem);
> +	if (IS_ERR(info->regs))
> +		return PTR_ERR(info->regs);
> +
> +	/*
> +	 * The reset should be an optional property, as it should work
> +	 * with old devicetrees as well
> +	 */
> +	info->reset = devm_reset_control_get(&pdev->dev, "lsadc-crg");
> +	if (IS_ERR(info->reset)) {
> +		ret = PTR_ERR(info->reset);
> +		if (ret != -ENOENT)
> +			return ret;
> +
> +		dev_dbg(&pdev->dev, "no reset control found\n");
> +		info->reset = NULL;
> +	}
> +
> +	init_completion(&info->completion);
> +
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq < 0) {
> +		dev_err(&pdev->dev, "no irq resource?\n");
> +		return irq;
> +	}
> +
> +	ret = devm_request_irq(&pdev->dev, irq, hibvt_lsadc_isr,
> +			       0, dev_name(&pdev->dev), info);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "failed requesting irq %d\n", irq);
> +		return ret;
> +	}
> +
> +	if (info->reset)
> +		hibvt_lsadc_reset_controller(info->reset);
> +
> +	platform_set_drvdata(pdev, indio_dev);
> +
> +	indio_dev->name = dev_name(&pdev->dev);
> +	indio_dev->dev.parent = &pdev->dev;
> +	indio_dev->dev.of_node = pdev->dev.of_node;
> +	indio_dev->info = &hibvt_lsadc_iio_info;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +
> +	indio_dev->channels = info->data->channels;
> +	indio_dev->num_channels = info->data->num_channels;
> +
> +	ret = devm_iio_device_register(&pdev->dev, indio_dev);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "failed register iio device\n");
> +		return ret;
Drop this return ret and just return ret instead of the return 0 below.
> +	}
> +
> +	return 0;
> +}
> +
> +static struct platform_driver hibvt_lsadc_driver = {
> +	.probe		= hibvt_lsadc_probe,
> +	.driver		= {
> +		.name	= "hibvt-lsadc",
> +		.of_match_table = hibvt_lsadc_match,
> +	},
> +};
> +
> +module_platform_driver(hibvt_lsadc_driver);
> +
> +MODULE_AUTHOR("Allen Liu <liurenzhong@hisilicon.com>");
> +MODULE_DESCRIPTION("hisilicon BVT LSADC driver");
> +MODULE_LICENSE("GPL v2");
> 

^ permalink raw reply

* Re: [PATCH V6 4/3] brcmfmac: use wiphy_read_of_freq_limits to respect extra limits
From: Rafał Miłecki @ 2017-01-07 17:36 UTC (permalink / raw)
  To: Arend Van Spriel
  Cc: Johannes Berg,
	linux-wireless-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	Martin Blumenstingl, Felix Fietkau, Arend van Spriel,
	Arnd Bergmann, devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	Rob Herring, Rafał Miłecki
In-Reply-To: <CACna6rw22benfuw_7BSFw1wedavmMJWTo_hfPLCVa1t0kV+Aqg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>

On 5 January 2017 at 11:02, Rafał Miłecki <zajec5-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On 5 January 2017 at 10:31, Arend Van Spriel
> <arend.vanspriel-dY08KVG/lbpWk0Htik3J/w@public.gmane.org> wrote:
>> On 4-1-2017 22:19, Rafał Miłecki wrote:
>>> I'm not exactly happy with channels management in brcmfmac. Before
>>> calling wiphy_register it already disables channels unavailable for
>>> current country. This results in setting IEEE80211_CHAN_DISABLED in
>>
>> What do you mean by current country. There is none that we are aware off
>> in the driver. So we obtain the channels for the current
>> country/revision in the firmware and enable those before
>> wiphy_register(). This all is within the probe/init sequence so I do not
>> really see an issue. As the wiphy object is not yet registered there is
>> no user-space awareness
>
> It seems I'm terrible as describing my patches/problems/ideas :( Here
> I used 1 inaccurate word and you couldn't understand my point.
>
> By "current country" I meant current region (and so a set of
> regulatory rules) used by the firmware. I believe firmware describes
> it using "ccode" and "regrev".
>
> Now, about the issue I see:
>
> AFAIU if you set IEEE80211_CHAN_DISABLED in "orig_flags" it's meant to
> be there for good. Some reference code that makes me believe so
> (reg.c):
> pr_debug("Disabling freq %d MHz for good\n", chan->center_freq);
> chan->orig_flags |= IEEE80211_CHAN_DISABLED;
>
> This is what happens with brcmfmac right now. If firmware doesn't
> report some channels, you set "flags" to IEEE80211_CHAN_DISABLED for
> them. Then you call wiphy_register which copies that "flags" to the
> "orig_flags". I read it as: we are never going to use these channels.
>
> Now, consider you support regdom change (I do with my local patches).
> You translate alpha2 to a proper firmware request (board specific!),
> you execute it and then firmware may allow you to use channels that
> you marked as disabled for good. You would need to mess with
> orig_flags to recover from this issue.

I was wrong about this. Current notifier implementation in brcmfmac
doesn't care about "orig_flags" and it just sets "flags" as it wants.
This way it can enable even these channels that were believed to be
disabled for good (DISABLED in "orig_flags").

For my test I booted SR400ac with ccode & regrev set to values that
didn't allow me to use channels 149 - 165 (5735 - 5835). It matches my
current country:
country PL: DFS-ETSI
        (2402 - 2482 @ 40), (20)
        (5170 - 5250 @ 80), (20), AUTO-BW
        (5250 - 5330 @ 80), (20), DFS, AUTO-BW
        (5490 - 5710 @ 160), (27), DFS
        # 60 GHz band channels 1-4, ref: Etsi En 302 567
        (57000 - 66000 @ 2160), (40)

Then I used "iw reg set ??" to set country that allows these channels.
My locally modified brcmfmac driver sent proper "country" iovar to the
firmware and queried it for the updated "chanspecs". See my debugging
messages below:
brcmfmac: [brcmf_construct_chaninfo] hw_value:34 orig_flags:0x101 flags:0x001
brcmfmac: [brcmf_construct_chaninfo] hw_value:36 orig_flags:0x1a0 flags:0x0a0
brcmfmac: [brcmf_construct_chaninfo] hw_value:38 orig_flags:0x101 flags:0x001
brcmfmac: [brcmf_construct_chaninfo] hw_value:40 orig_flags:0x190 flags:0x090
brcmfmac: [brcmf_construct_chaninfo] hw_value:42 orig_flags:0x101 flags:0x001
brcmfmac: [brcmf_construct_chaninfo] hw_value:44 orig_flags:0x1a0 flags:0x0a0
brcmfmac: [brcmf_construct_chaninfo] hw_value:46 orig_flags:0x101 flags:0x001
brcmfmac: [brcmf_construct_chaninfo] hw_value:48 orig_flags:0x190 flags:0x090
brcmfmac: [brcmf_construct_chaninfo] hw_value:52 orig_flags:0x1aa flags:0x0aa
brcmfmac: [brcmf_construct_chaninfo] hw_value:56 orig_flags:0x19a flags:0x09a
brcmfmac: [brcmf_construct_chaninfo] hw_value:60 orig_flags:0x1aa flags:0x0aa
brcmfmac: [brcmf_construct_chaninfo] hw_value:64 orig_flags:0x19a flags:0x09a
brcmfmac: [brcmf_construct_chaninfo] hw_value:100 orig_flags:0x1aa flags:0x001
brcmfmac: [brcmf_construct_chaninfo] hw_value:104 orig_flags:0x19a flags:0x001
brcmfmac: [brcmf_construct_chaninfo] hw_value:108 orig_flags:0x1aa flags:0x001
brcmfmac: [brcmf_construct_chaninfo] hw_value:112 orig_flags:0x19a flags:0x001
brcmfmac: [brcmf_construct_chaninfo] hw_value:116 orig_flags:0x1aa flags:0x001
brcmfmac: [brcmf_construct_chaninfo] hw_value:120 orig_flags:0x19a flags:0x001
brcmfmac: [brcmf_construct_chaninfo] hw_value:124 orig_flags:0x1aa flags:0x001
brcmfmac: [brcmf_construct_chaninfo] hw_value:128 orig_flags:0x19a flags:0x001
brcmfmac: [brcmf_construct_chaninfo] hw_value:132 orig_flags:0x1aa flags:0x001
brcmfmac: [brcmf_construct_chaninfo] hw_value:136 orig_flags:0x19a flags:0x001
brcmfmac: [brcmf_construct_chaninfo] hw_value:140 orig_flags:0x1ba flags:0x001
brcmfmac: [brcmf_construct_chaninfo] hw_value:144 orig_flags:0x101 flags:0x001
brcmfmac: [brcmf_construct_chaninfo] hw_value:149 orig_flags:0x101 flags:0x0a0
brcmfmac: [brcmf_construct_chaninfo] hw_value:153 orig_flags:0x101 flags:0x090
brcmfmac: [brcmf_construct_chaninfo] hw_value:157 orig_flags:0x101 flags:0x0a0
brcmfmac: [brcmf_construct_chaninfo] hw_value:161 orig_flags:0x101 flags:0x090
brcmfmac: [brcmf_construct_chaninfo] hw_value:165 orig_flags:0x101 flags:0x0b0

As you can see brcmfmac dropped IEEE80211_CHAN_DISABLED (hint: 0x1)
for channels 149 - 165 even though they were disabled according to the
"orig_flags".

So this patch with its
if (channel->orig_flags & IEEE80211_CHAN_DISABLED)
        continue;
could be considered some kind of regression. My change wouldn't allow
enabling such channels anymore.

Of course noone would notice this as noone uses country_codes table
yet I guess (except for me locally). Anyway, this patch should be
reworked to make sure it still allows implementing support for
country_codes tables in the future.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH linux 2/6] hwmon: occ: Add sysfs interface
From: Guenter Roeck @ 2017-01-07 17:15 UTC (permalink / raw)
  To: Edward James
  Cc: andrew, corbet, devicetree, eajames.ibm, jdelvare, joel,
	linux-doc, linux-hwmon, linux-i2c, linux-kernel, mark.rutland,
	robh+dt, wsa
In-Reply-To: <OFA812992D.D19B1368-ON002580A0.007A2966-862580A0.007A72F0@notes.na.collabserv.com>

On 01/06/2017 02:17 PM, Edward James wrote:

[ ... ]

>> > +}
>> > +
>> > +static DEVICE_ATTR(online, S_IWUSR | S_IRUGO, show_occ_online,
>> > +         store_occ_online);
>> > +
>> > +struct occ_sysfs *occ_sysfs_start(struct device *dev, struct occ *occ,
>> > +              struct occ_sysfs_config *config)
>> > +{
>> > +   struct occ_sysfs *hwmon = devm_kzalloc(dev, sizeof(struct occ_sysfs),
>> > +                      GFP_KERNEL);
>> > +   int rc;
>> > +
>> > +   if (!hwmon)
>> > +      return ERR_PTR(-ENOMEM);
>> > +
>> > +   hwmon->occ = occ;
>> > +   hwmon->num_caps_fields = config->num_caps_fields;
>> > +   hwmon->caps_names = config->caps_names;
>> > +
>> > +   dev_set_drvdata(dev, hwmon);
>> > +
>> > +   rc = device_create_file(dev, &dev_attr_online);
>> > +   if (rc)
>> > +      return ERR_PTR(rc);
>> > +
>> > +   return hwmon;
>> > +}
>> > +EXPORT_SYMBOL(occ_sysfs_start);
>> > +
>> > +int occ_sysfs_stop(struct device *dev, struct occ_sysfs *driver)
>> > +{
>> > +   if (driver->dev) {
>> > +      occ_remove_hwmon_attrs(driver);
>> > +      hwmon_device_unregister(driver->dev);
>> > +   }
>> > +
>> > +   device_remove_file(driver->dev, &dev_attr_online);
>> > +
>> > +   devm_kfree(dev, driver);
>>
>> Thw point of using devm_ functions is not to require remove/free functions.
>> Something is completely wrong here if you need that call.
>>
>> Overall, this is architectually completely wrong. One does not register
>> or instantiate drivers based on writing into sysfs attributes. Please
>> reconsider your approach.
>
> We had some trouble designing this driver because the BMC only has
> access to the OCC once the processor is powered on. This will happen
> sometime after the BMC boots (this driver runs only on the BMC). With
> no access to the OCC, we don't know what sensors are present on the
> system without a large static enumeration. Also any sysfs files created
> before we have OCC access won't be able to return any data.
>
> Instead of the "online" attribute, what do you think about using the
> "bind"/"unbind" API to probe the device from user space once the system
> is powered on? All the hwmon registration would take place in the probe
> function, it would just occur some time after boot.
>

A more common approach would be to have a platform driver. That platform
driver would need a means to detect if the OCC is up and running, and
instantiate everything else once it is.

A trigger from user space is problematic because there is no guarantee
that the OCC is really up (or that it even exists).

An alternative might be to have the hwmon driver poll for the OCC,
but that would be a bit more difficult and might require a kernel thread
or maybe asynchronous probing.

Guenter


^ permalink raw reply

* [PATCH] devicetree: bindings: Add vendor prefix for Shenzhen Xunlong Software
From: Icenowy Zheng @ 2017-01-07 16:13 UTC (permalink / raw)
  To: Rob Herring, Mark Rutland, Maxime Ripard
  Cc: devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-sunxi-/JYPxA39Uh5TLH3MbocFFw, Icenowy Zheng

Shenzhen Xunlong Software CO.,Limited is a SBC vendor, which produces
the "Orange Pi" series of SBCs.

Add a vendor prefix for it. This prefix is already used in many
Allwinner H3 boards.

Signed-off-by: Icenowy Zheng <icenowy-ymACFijhrKM@public.gmane.org>
---
 Documentation/devicetree/bindings/vendor-prefixes.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt b/Documentation/devicetree/bindings/vendor-prefixes.txt
index 4ec84b7a3c56..ddd4c6e5a468 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.txt
+++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
@@ -329,6 +329,7 @@ x-powers	X-Powers
 xes	Extreme Engineering Solutions (X-ES)
 xillybus	Xillybus Ltd.
 xlnx	Xilinx
+xunlong	Shenzhen Xunlong Software CO.,Limited
 zarlink	Zarlink Semiconductor
 zii	Zodiac Inflight Innovations
 zte	ZTE Corp.
-- 
2.11.0

^ permalink raw reply related

* [PATCH v4 2/2] ARM: DT: add power-off support to linkstation lsgl and kuroboxpro
From: Roger Shimizu @ 2017-01-07 15:04 UTC (permalink / raw)
  Cc: Roger Shimizu, Ryan Tandy, linux-pm-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
In-Reply-To: <20170107150451.17912-1-rogershimizu-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

A few models of Linkstation / KuroBox has micro-controller which
controls the power (as well as FAN, but not related here), while
others not. So remove the restart-poweroff driver in linkstation
common dtsi file, and specify the proper power driver in dts
of each device.

Devices need micro-controler to power-off:
  - Linkstation LS-GL
  - KuroBox Pro
Device continues using original restart-poweroff driver:
  - Linkstation LS-WTGL

To: Jason Cooper <jason-NLaQJdtUoK4Be96aLqz0jA@public.gmane.org>
To: Andrew Lunn <andrew-g2DYL2Zd6BY@public.gmane.org>
To: Gregory Clement <gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
To: Sebastian Hesselbarth <sebastian.hesselbarth-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: Ryan Tandy <ryan-pRYBVO4bdZ33fQ9qLvQP4Q@public.gmane.org>
Cc: linux-pm-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org

Reported-by: Ryan Tandy <ryan-pRYBVO4bdZ33fQ9qLvQP4Q@public.gmane.org>
Tested-by: Ryan Tandy <ryan-pRYBVO4bdZ33fQ9qLvQP4Q@public.gmane.org>
Signed-off-by: Roger Shimizu <rogershimizu-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 arch/arm/boot/dts/orion5x-kuroboxpro.dts         |  8 ++++++++
 arch/arm/boot/dts/orion5x-linkstation-lsgl.dts   | 10 ++++++++++
 arch/arm/boot/dts/orion5x-linkstation-lswtgl.dts |  4 ++++
 arch/arm/boot/dts/orion5x-linkstation.dtsi       |  4 ----
 4 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/arch/arm/boot/dts/orion5x-kuroboxpro.dts b/arch/arm/boot/dts/orion5x-kuroboxpro.dts
index 1a672b098d0b..aba38d802bda 100644
--- a/arch/arm/boot/dts/orion5x-kuroboxpro.dts
+++ b/arch/arm/boot/dts/orion5x-kuroboxpro.dts
@@ -60,6 +60,14 @@
 				 <MBUS_ID(0x09, 0x00) 0 0xf2200000 0x800>,
 				 <MBUS_ID(0x01, 0x0f) 0 0xf4000000 0x40000>,
 				 <MBUS_ID(0x01, 0x1e) 0 0xfc000000 0x1000000>;
+
+		internal-regs {
+			power_off {
+				compatible = "linkstation,power-off";
+				reg = <0x12100 0x100>;
+				clocks = <&core_clk 0>;
+			};
+		};
 	};
 
 	memory { /* 128 MB */
diff --git a/arch/arm/boot/dts/orion5x-linkstation-lsgl.dts b/arch/arm/boot/dts/orion5x-linkstation-lsgl.dts
index 51dc734cd5b9..370fc17a6dd9 100644
--- a/arch/arm/boot/dts/orion5x-linkstation-lsgl.dts
+++ b/arch/arm/boot/dts/orion5x-linkstation-lsgl.dts
@@ -56,6 +56,16 @@
 	model = "Buffalo Linkstation Pro/Live";
 	compatible = "buffalo,lsgl", "marvell,orion5x-88f5182", "marvell,orion5x";
 
+	soc {
+		internal-regs {
+			power_off {
+				compatible = "linkstation,power-off";
+				reg = <0x12100 0x100>;
+				clocks = <&core_clk 0>;
+			};
+		};
+	};
+
 	memory { /* 128 MB */
 		device_type = "memory";
 		reg = <0x00000000 0x8000000>;
diff --git a/arch/arm/boot/dts/orion5x-linkstation-lswtgl.dts b/arch/arm/boot/dts/orion5x-linkstation-lswtgl.dts
index 0eead400f427..571a71f5b7ad 100644
--- a/arch/arm/boot/dts/orion5x-linkstation-lswtgl.dts
+++ b/arch/arm/boot/dts/orion5x-linkstation-lswtgl.dts
@@ -59,6 +59,10 @@
 		reg = <0x00000000 0x4000000>;
 	};
 
+	restart_poweroff {
+		compatible = "restart-poweroff";
+	};
+
 	gpio_keys {
 		power-on-switch {
 			gpios = <&gpio0 8 GPIO_ACTIVE_LOW>;
diff --git a/arch/arm/boot/dts/orion5x-linkstation.dtsi b/arch/arm/boot/dts/orion5x-linkstation.dtsi
index ed456ab35fd8..40770a8d4b36 100644
--- a/arch/arm/boot/dts/orion5x-linkstation.dtsi
+++ b/arch/arm/boot/dts/orion5x-linkstation.dtsi
@@ -57,10 +57,6 @@
 				 <MBUS_ID(0x01, 0x0f) 0 0xf4000000 0x40000>;
 	};
 
-	restart_poweroff {
-		compatible = "restart-poweroff";
-	};
-
 	regulators {
 		compatible = "simple-bus";
 		#address-cells = <1>;
-- 
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply related

* [PATCH v4 1/2] power: reset: add linkstation-reset driver
From: Roger Shimizu @ 2017-01-07 15:04 UTC (permalink / raw)
  Cc: Roger Shimizu, Andrew Lunn, Martin Michlmayr, Sylver Bruneau,
	Herbert Valerio Riedel, Ryan Tandy, Florian Fainelli,
	linux-pm-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
In-Reply-To: <20170107150451.17912-1-rogershimizu-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

Buffalo Linkstation / KuroBox and their variants need magic command
sending to UART1 to power-off.

Power driver linkstation-reset implements the magic command and I/O
routine, which come from files listed below:
  - arch/arm/mach-orion5x/kurobox_pro-setup.c
  - arch/arm/mach-orion5x/terastation_pro2-setup.c

To: Sebastian Reichel <sre-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: Andrew Lunn <andrew-g2DYL2Zd6BY@public.gmane.org>
Cc: Martin Michlmayr <tbm-R+vWnYXSFMfQT0dZR+AlfA@public.gmane.org>
Cc: Sylver Bruneau <sylver.bruneau-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org>
Cc: Herbert Valerio Riedel <hvr-mXXj517/zsQ@public.gmane.org>
Cc: Ryan Tandy <ryan-pRYBVO4bdZ33fQ9qLvQP4Q@public.gmane.org>
Cc: Florian Fainelli <f.fainelli-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: linux-pm-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org

Reported-by: Ryan Tandy <ryan-pRYBVO4bdZ33fQ9qLvQP4Q@public.gmane.org>
Tested-by: Ryan Tandy <ryan-pRYBVO4bdZ33fQ9qLvQP4Q@public.gmane.org>
Signed-off-by: Roger Shimizu <rogershimizu-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/power/reset/Kconfig             |  10 ++
 drivers/power/reset/Makefile            |   1 +
 drivers/power/reset/linkstation-reset.c | 270 ++++++++++++++++++++++++++++++++
 3 files changed, 281 insertions(+)
 create mode 100644 drivers/power/reset/linkstation-reset.c

diff --git a/drivers/power/reset/Kconfig b/drivers/power/reset/Kconfig
index c74c3f67b8da..77c44cad7ece 100644
--- a/drivers/power/reset/Kconfig
+++ b/drivers/power/reset/Kconfig
@@ -98,6 +98,16 @@ config POWER_RESET_IMX
 	  say N here or disable in dts to make sure pm_power_off never be
 	  overwrote wrongly by this driver.
 
+config POWER_RESET_LINKSTATION
+	bool "Buffalo Linkstation and its variants reset driver"
+	depends on OF_GPIO && PLAT_ORION
+	help
+	  This driver supports power off Buffalo Linkstation / KuroBox Pro
+	  NAS and their variants by sending commands to the micro-controller
+	  which controls the main power.
+
+	  Say Y if you have a Buffalo Linkstation / KuroBox Pro NAS.
+
 config POWER_RESET_MSM
 	bool "Qualcomm MSM power-off driver"
 	depends on ARCH_QCOM
diff --git a/drivers/power/reset/Makefile b/drivers/power/reset/Makefile
index 1be307c7fc25..692ba6417cfb 100644
--- a/drivers/power/reset/Makefile
+++ b/drivers/power/reset/Makefile
@@ -9,6 +9,7 @@ obj-$(CONFIG_POWER_RESET_GPIO) += gpio-poweroff.o
 obj-$(CONFIG_POWER_RESET_GPIO_RESTART) += gpio-restart.o
 obj-$(CONFIG_POWER_RESET_HISI) += hisi-reboot.o
 obj-$(CONFIG_POWER_RESET_IMX) += imx-snvs-poweroff.o
+obj-$(CONFIG_POWER_RESET_LINKSTATION) += linkstation-reset.o
 obj-$(CONFIG_POWER_RESET_MSM) += msm-poweroff.o
 obj-$(CONFIG_POWER_RESET_LTC2952) += ltc2952-poweroff.o
 obj-$(CONFIG_POWER_RESET_QNAP) += qnap-poweroff.o
diff --git a/drivers/power/reset/linkstation-reset.c b/drivers/power/reset/linkstation-reset.c
new file mode 100644
index 000000000000..c191b7671076
--- /dev/null
+++ b/drivers/power/reset/linkstation-reset.c
@@ -0,0 +1,270 @@
+/*
+ * Buffalo Linkstation power reset driver.
+ * It may also be used on following devices:
+ *  - KuroBox Pro
+ *  - Buffalo Linkstation Pro (LS-GL)
+ *  - Buffalo Terastation Pro II/Live
+ *  - Buffalo Linkstation Duo (LS-WTGL)
+ *  - Buffalo Linkstation Mini (LS-WSGL)
+ *
+ * Copyright (C) 2016  Roger Shimizu <rogershimizu-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
+ *
+ * Based on the code from:
+ *
+ * Copyright (C) 2012  Andrew Lunn <andrew-g2DYL2Zd6BY@public.gmane.org>
+ * Copyright (C) 2009  Martin Michlmayr <tbm-R+vWnYXSFMfQT0dZR+AlfA@public.gmane.org>
+ * Copyright (C) 2008  Byron Bradley <byron.bbradley-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
+ * Copyright (C) 2008  Sylver Bruneau <sylver.bruneau-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org>
+ * Copyright (C) 2007  Herbert Valerio Riedel <hvr-mXXj517/zsQ@public.gmane.org>
+ *
+ * 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.
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/serial_reg.h>
+#include <linux/kallsyms.h>
+#include <linux/of.h>
+#include <linux/io.h>
+#include <linux/clk.h>
+#include <linux/delay.h>
+
+#define UART1_REG(x)	((UART_##x) << 2)
+#define MICON_CMD_SIZE	4
+
+/* 4-byte magic hello command to UART1-attached microcontroller */
+static const unsigned char linkstation_micon_magic[] = {
+	0x1b,
+	0x00,
+	0x07,
+	0x00
+};
+
+/* for each row, first byte is the size of command */
+static const unsigned char linkstation_power_off_cmd[][MICON_CMD_SIZE] = {
+	{ 3,	0x01, 0x35, 0x00},
+	{ 2,	0x00, 0x0c},
+	{ 2,	0x00, 0x06},
+	{}
+};
+
+struct reset_cfg {
+	u32 baud;
+	const unsigned char *magic;
+	const unsigned char (*cmd)[MICON_CMD_SIZE];
+};
+
+struct device_cfg {
+	const struct device *dev;
+	void __iomem *base;
+	unsigned long tclk;
+	const struct reset_cfg *cfg;
+};
+
+static const struct reset_cfg linkstation_power_off_cfg = {
+	.baud = 38400,
+	.magic = linkstation_micon_magic,
+	.cmd = linkstation_power_off_cmd,
+};
+
+static const struct of_device_id linkstation_reset_of_match_table[] = {
+	{ .compatible = "linkstation,power-off",
+	  .data = &linkstation_power_off_cfg,
+	},
+	{}
+};
+MODULE_DEVICE_TABLE(of, linkstation_reset_of_match_table);
+
+static int uart1_micon_read(const struct device_cfg *dev, unsigned char *buf, int count)
+{
+	int i;
+	int timeout;
+
+	for (i = 0; i < count; i++) {
+		timeout = 10;
+
+		while (!(readl(dev->base + UART1_REG(LSR)) & UART_LSR_DR)) {
+			if (--timeout == 0)
+				break;
+			udelay(1000);
+		}
+
+		if (timeout == 0)
+			break;
+		buf[i] = readl(dev->base + UART1_REG(RX));
+	}
+
+	/* return read bytes */
+	return i;
+}
+
+static int uart1_micon_write(const struct device_cfg *dev, const unsigned char *buf, int count)
+{
+	int i = 0;
+
+	while (count--) {
+		while (!(readl(dev->base + UART1_REG(LSR)) & UART_LSR_THRE))
+			barrier();
+		writel(buf[i++], dev->base + UART1_REG(TX));
+	}
+
+	return 0;
+}
+
+int uart1_micon_send(const struct device_cfg *dev, const unsigned char *data, int count)
+{
+	int i;
+	unsigned char checksum = 0;
+	unsigned char recv_buf[40];
+	unsigned char send_buf[40];
+	unsigned char correct_ack[3];
+	int retry = 2;
+
+	/* Generate checksum */
+	for (i = 0; i < count; i++)
+		checksum -=  data[i];
+
+	do {
+		/* Send data */
+		uart1_micon_write(dev, data, count);
+
+		/* send checksum */
+		uart1_micon_write(dev, &checksum, 1);
+
+		if (uart1_micon_read(dev, recv_buf, sizeof(recv_buf)) <= 3) {
+			dev_err(dev->dev, ">%s: receive failed.\n", __func__);
+
+			/* send preamble to clear the receive buffer */
+			memset(&send_buf, 0xff, sizeof(send_buf));
+			uart1_micon_write(dev, send_buf, sizeof(send_buf));
+
+			/* make dummy reads */
+			mdelay(100);
+			uart1_micon_read(dev, recv_buf, sizeof(recv_buf));
+		} else {
+			/* Generate expected ack */
+			correct_ack[0] = 0x01;
+			correct_ack[1] = data[1];
+			correct_ack[2] = 0x00;
+
+			/* checksum Check */
+			if ((recv_buf[0] + recv_buf[1] + recv_buf[2] +
+			     recv_buf[3]) & 0xFF) {
+				dev_err(dev->dev, ">%s: Checksum Error : "
+					"Received data[%02x, %02x, %02x, %02x]"
+					"\n", __func__, recv_buf[0],
+					recv_buf[1], recv_buf[2], recv_buf[3]);
+			} else {
+				/* Check Received Data */
+				if (correct_ack[0] == recv_buf[0] &&
+				    correct_ack[1] == recv_buf[1] &&
+				    correct_ack[2] == recv_buf[2]) {
+					/* Interval for next command */
+					mdelay(10);
+
+					/* Receive ACK */
+					return 0;
+				}
+			}
+			/* Received NAK or illegal Data */
+			dev_err(dev->dev, ">%s: Error : NAK or Illegal Data "
+					"Received\n", __func__);
+		}
+	} while (retry--);
+
+	/* Interval for next command */
+	mdelay(10);
+
+	return -1;
+}
+
+static struct device_cfg reset;
+
+static void linkstation_reset(void)
+{
+	const unsigned divisor = ((reset.tclk + (8 * reset.cfg->baud)) / (16 * reset.cfg->baud));
+	int i;
+
+	pr_err("%s: triggering power-off...\n", __func__);
+
+	/* hijack UART1 and reset into sane state */
+	writel(0x83, reset.base + UART1_REG(LCR));
+	writel(divisor & 0xff, reset.base + UART1_REG(DLL));
+	writel((divisor >> 8) & 0xff, reset.base + UART1_REG(DLM));
+	writel(reset.cfg->magic[0], reset.base + UART1_REG(LCR));
+	writel(reset.cfg->magic[1], reset.base + UART1_REG(IER));
+	writel(reset.cfg->magic[2], reset.base + UART1_REG(FCR));
+	writel(reset.cfg->magic[3], reset.base + UART1_REG(MCR));
+
+	/* send the power-off command to PIC */
+	for(i = 0; reset.cfg->cmd[i][0] > 0; i ++) {
+		/* [0] is size of the command; command starts from [1] */
+		uart1_micon_send(&reset, &(reset.cfg->cmd[i][1]), reset.cfg->cmd[i][0]);
+	}
+}
+
+static int linkstation_reset_probe(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	struct resource *res;
+	struct clk *clk;
+
+	const struct of_device_id *match =
+		of_match_node(linkstation_reset_of_match_table, np);
+	reset.cfg = match->data;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res) {
+		dev_err(&pdev->dev, "Missing resource");
+		return -EINVAL;
+	}
+
+	reset.dev = &pdev->dev;
+	reset.base = devm_ioremap(&pdev->dev, res->start, resource_size(res));
+	if (!reset.base) {
+		dev_err(reset.dev, "Unable to map resource");
+		return -EINVAL;
+	}
+
+	/* We need to know tclk in order to calculate the UART divisor */
+	clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(clk)) {
+		dev_err(reset.dev, "Clk missing");
+		return PTR_ERR(clk);
+	}
+
+	reset.tclk = clk_get_rate(clk);
+
+	/* Check that nothing else has already setup a handler */
+	if (!pm_power_off) {
+		pm_power_off = linkstation_reset;
+	}
+
+	return 0;
+}
+
+static int linkstation_reset_remove(struct platform_device *pdev)
+{
+	if (pm_power_off == linkstation_reset)
+		pm_power_off = NULL;
+	return 0;
+}
+
+static struct platform_driver linkstation_reset_driver = {
+	.probe	= linkstation_reset_probe,
+	.remove	= linkstation_reset_remove,
+	.driver	= {
+		.name	= "linkstation_reset",
+		.of_match_table = of_match_ptr(linkstation_reset_of_match_table),
+	},
+};
+
+module_platform_driver(linkstation_reset_driver);
+
+MODULE_AUTHOR("Roger Shimizu <rogershimizu-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>");
+MODULE_DESCRIPTION("Linkstation Reset driver");
+MODULE_LICENSE("GPL v2");
-- 
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply related

* [PATCH v4 0/2] make kurobox-pro be able to shutdown after device-tree migration
From: Roger Shimizu @ 2017-01-07 15:04 UTC (permalink / raw)
  To: Sebastian Reichel, Rob Herring, Mark Rutland, Jason Cooper,
	Andrew Lunn, Gregory Clement, Sebastian Hesselbarth
  Cc: Roger Shimizu, Ryan Tandy, Florian Fainelli, linux-pm, devicetree,
	linux-arm-kernel
In-Reply-To: <20161227070611.14852-1-rogershimizu@gmail.com>

Dear Kernel Maintainers,

Kurobox-Pro (and variants) need more commands sending to UART1 to shutdown.
So here I make this patch series to let current qnap-poweroff implementation
be able to handle such case.

Here I give 3 patches, for 3 different kernel subsystems respectivelay:
  - Patch 1/3: System reset/shutdown driver
  - Patch 2/3: Open firmware and flattened device tree bindings
  - Patch 3/3: ARM/Marvell Dove/MV78xx0/Orion SOC support

Ryan Tandy, the issue reporter, and I have already tested those changes on
Linkstation/KuroBoxPro devices, and confirmed it's working well.

Merry Xmax and look forward to your feedback!

Changes:
  v0 => v1:
  - Update 0003 to split kuroboxpro related code into kuroboxpro-common.c
  v1 => v2:
  - Split off linkstation/kuroboxpro related code to linkstation-reset.c
    Because linkstation before kuroboxpro also need this driver to power
    off properly. It's more proper to call it linkstation driver.
  v2 => v3:
  - Split off devicetree bindings doc into a separate patch.
  - Add device-tree patch to make Linkstation LS-GL and KuroBox Pro to
    use the newly created linkstation-reset driver.
  - Remove the unused one-byte command sending case in linkstation-reset
    driver.
  v3 => v4:
  - Reduce the global singleton variables by moving to structure, which makes
    the driver easy to support reboot function in the future.  Thanks to
    Florian Fainelli.
  - Simplify the power_off function registration. Thanks to Florian Fainelli.
  - Drop DT binding doc.

Cheers,
Roger Shimizu, GMT +9 Tokyo
PGP/GPG: 4096R/6C6ACD6417B3ACB1

Roger Shimizu (2):
  power: reset: add linkstation-reset driver
  ARM: DT: add power-off support to linkstation lsgl and kuroboxpro

 arch/arm/boot/dts/orion5x-kuroboxpro.dts         |   8 +
 arch/arm/boot/dts/orion5x-linkstation-lsgl.dts   |  10 +
 arch/arm/boot/dts/orion5x-linkstation-lswtgl.dts |   4 +
 arch/arm/boot/dts/orion5x-linkstation.dtsi       |   4 -
 drivers/power/reset/Kconfig                      |  10 +
 drivers/power/reset/Makefile                     |   1 +
 drivers/power/reset/linkstation-reset.c          | 270 +++++++++++++++++++++++
 7 files changed, 303 insertions(+), 4 deletions(-)
 create mode 100644 drivers/power/reset/linkstation-reset.c

-- 
2.11.0


^ permalink raw reply

* Re: [PATCHv2 net-next 11/16] net: mvpp2: handle misc PPv2.1/PPv2.2 differences
From: Russell King - ARM Linux @ 2017-01-07 13:50 UTC (permalink / raw)
  To: Marcin Wojtas
  Cc: Thomas Petazzoni, netdev-u79uwXL29TY76Z2rM5mHXA, David S. Miller,
	devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Rob Herring,
	Ian Campbell, Pawel Moll, Mark Rutland, Kumar Gala, Andrew Lunn,
	Yehuda Yitschak, Jason Cooper, Hanna Hawa, Nadav Haklai,
	Gregory Clement, Stefan Chulski,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
	Sebastian Hesselbarth
In-Reply-To: <CAPv3WKeQ=fj2cKPyJ2NqCaAv55cOyWodujKwj3-v5iCrDYNcmA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>

On Sat, Jan 07, 2017 at 01:12:35PM +0100, Marcin Wojtas wrote:
> In fact there is common SMI bus, but each port has its own register
> set to control it (it's true at least for Neta and PP2). There is also
> an option to use HW polling - every 1s hardware checks PHY over SMI
> and updates MAC registers of each port independently. I was able to
> use those successfully in other implementations.
> 
> However we are supposed to use libphy in Linux and I'm afraid we have
> to use single instance that controls single SMI bus - I think current
> implementation is a compromise between HW and libphy demands.

One of the "DT rules" is that DT only describes the hardware in an
implementation independent way.  It should not describe a particular
implementation's view of the hardware.

"libphy demands" sounds very much like an implementation dictating
the DT description, which sounds to me very wrong and against the
goals and purposes of DT.

Anyway, I don't think it's too bad - a possible solution here would
be to have the existing MDIO driver as a library, which can be
instantiated by the Neta and PP2 drivers.  This could be done in
DT (taking dove as an example) as:

                        eth: ethernet-ctrl@72000 {
                                compatible = "marvell,orion-eth";
				reg = <0x72000 0x4000>;
				ranges = <0 0x72000 0 0x4000>;
				...
				mdio: mdio@4 {
					compatible = "marvell,orion-mdio";
					reg = <4>;
					...
					... phys ...
				};
			};

I don't think that would require that big a change - and it could be
done in a way that compatibility with the existing DT descriptions is
maintained very cheaply.

Now, I'm not suggesting that mdio@4 should be created by a platform
device via marking ethernet-ctrl@72000 with "simple-bus", but it's
something that should be created by the ethernet driver if present.
The compatible string is there so we can identify that this is a
mdio node, and which type of mdio it is (the Armada 8k has this type
and a separate clause-45 xmdio implementation, and we need to
distinguish those.)

What that means is that we no longer have to worry about clocks and
overlapping register regions and the like, and can deal with the
ethernet driver wanting to access the SMI registers as well.

We would need the ethernet driver to be capable of instantiation even
with no ports enabled, so cases where the MDIO interface is used with
other ethernet controllers continues to be supportable (eg, the
Armada 8040 case where the slave CP110 ethernet controller is used
with PHYs connected to the master CP110 ethernet controller's MDIO
buses - which afaik aren't shared between the two CP110 dies.)

However, I'd like to see libphy become more flexible and support
hardware polled mode of operation, since libphy provides a nice
library of functions for accessing various phy features (like setting
the advertisment, EEE modes, flow control, etc.)  Even with hardware
polling, we should still describe the PHY in DT, because PHY is part
of the hardware description, and we need to know where it is in order
to program these phy features.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* [PATCH] arm64: dts: rockchip: add "rockchip,grf" property for RK3399 PMUCRU/CRU
From: Xing Zheng @ 2017-01-07 13:05 UTC (permalink / raw)
  To: heiko-4mtYJXux2i+zQB+pC5nmwQ
  Cc: dianders-hpIqsD4AKlfQT0dZR+AlfA,
	linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Xing Zheng,
	Rob Herring, Mark Rutland, Catalin Marinas, Will Deacon,
	Douglas Anderson, Caesar Wang, Brian Norris, Shawn Lin,
	Jianqun Xu, Elaine Zhang, David Wu, William wu,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA

The structure rockchip_clk_provider needs to refer the GRF regmap
in somewhere, if the CRU node has not "rockchip,grf" property,
calling syscon_regmap_lookup_by_phandle will return an invaild GRF
regmap, and the MUXGRF type clock will be not supported.

Therefore, we need to add them.

Signed-off-by: Xing Zheng <zhengxing-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
---

 arch/arm64/boot/dts/rockchip/rk3399.dtsi | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm64/boot/dts/rockchip/rk3399.dtsi b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
index 92b731f..7790634 100644
--- a/arch/arm64/boot/dts/rockchip/rk3399.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
@@ -1090,6 +1090,7 @@
 	pmucru: pmu-clock-controller@ff750000 {
 		compatible = "rockchip,rk3399-pmucru";
 		reg = <0x0 0xff750000 0x0 0x1000>;
+		rockchip,grf = <&grf>;
 		#clock-cells = <1>;
 		#reset-cells = <1>;
 		assigned-clocks = <&pmucru PLL_PPLL>;
@@ -1099,6 +1100,7 @@
 	cru: clock-controller@ff760000 {
 		compatible = "rockchip,rk3399-cru";
 		reg = <0x0 0xff760000 0x0 0x1000>;
+		rockchip,grf = <&grf>;
 		#clock-cells = <1>;
 		#reset-cells = <1>;
 		assigned-clocks =
-- 
2.7.4


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply related

* Re: [PATCH V6 4/3] brcmfmac: use wiphy_read_of_freq_limits to respect extra limits
From: Rafał Miłecki @ 2017-01-07 12:58 UTC (permalink / raw)
  To: Arend Van Spriel
  Cc: Johannes Berg,
	linux-wireless-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	Martin Blumenstingl, Felix Fietkau, Arend van Spriel,
	Arnd Bergmann, devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	Rob Herring, Rafał Miłecki
In-Reply-To: <36d2dbd1-bcbe-021b-dd7f-068a5b9739ef-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>

On 7 January 2017 at 13:52, Arend Van Spriel
<arend.vanspriel-dY08KVG/lbpWk0Htik3J/w@public.gmane.org> wrote:
> On 5-1-2017 11:02, Rafał Miłecki wrote:
>> On 5 January 2017 at 10:31, Arend Van Spriel
>> <arend.vanspriel-dY08KVG/lbpWk0Htik3J/w@public.gmane.org> wrote:
>>> On 4-1-2017 22:19, Rafał Miłecki wrote:
>>>> On 4 January 2017 at 21:07, Arend Van Spriel
>>>> <arend.vanspriel-dY08KVG/lbpWk0Htik3J/w@public.gmane.org> wrote:
>>>>> On 4-1-2017 18:58, Rafał Miłecki wrote:
>>>>>> From: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>
>>>>>>
>>>>>> There are some devices (e.g. Netgear R8000 home router) with one chipset
>>>>>> model used for different radios, some of them limited to subbands. NVRAM
>>>>>> entries don't contain any extra info on such limitations and firmware
>>>>>> reports full list of channels to us. We need to store extra limitation
>>>>>> info in DT to support such devices properly.
>>>>>>
>>>>>> Now there is a cfg80211 helper for reading such info use it in brcmfmac.
>>>>>> This patch adds check for channel being disabled with orig_flags which
>>>>>> is how this wiphy helper and wiphy_register work.
>>>>>>
>>>>>> Signed-off-by: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>
>>>>>> ---
>>>>>> This patch should probably go through wireless-driver-next which is why
>>>>>> it got weird number 4/3. I'm sending it just as a proof of concept.
>>>>>> It was succesfully tested on SmartRG SR400ac with BCM43602.
>>>>>>
>>>>>> V4: Respect IEEE80211_CHAN_DISABLED in orig_flags
>>>>>> V5: Update commit message
>>>>>> V6: Call wiphy_read_of_freq_limits after brcmf_setup_wiphybands to make it work
>>>>>>     with helper setting "flags" instead of "orig_flags".
>>>>>> ---
>>>>>>  drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c | 9 ++++++++-
>>>>>>  1 file changed, 8 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
>>>>>> index ccae3bb..a008ba5 100644
>>>>>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
>>>>>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
>>>>>> @@ -5886,6 +5886,9 @@ static int brcmf_construct_chaninfo(struct brcmf_cfg80211_info *cfg,
>>>>>>                                                      band->band);
>>>>>>               channel[index].hw_value = ch.control_ch_num;
>>>>>>
>>>>>> +             if (channel->orig_flags & IEEE80211_CHAN_DISABLED)
>>>>>> +                     continue;
>>>>>> +
>>>>>
>>>>> So to be clear this is still needed for subsequent calls to
>>>>> brcmf_setup_wiphybands(). The subsequent calls are done from the
>>>>> regulatory notifier. So I think we have an issue with this approach. Say
>>>>> the device comes up with US. That would set DISABLED flags for channels
>>>>> 12 to 14. With a country update to PL we would want to enable channels
>>>>> 12 and 13, right? The orig_flags are copied from the initial flags
>>>>> during wiphy_register() so it seems we will skip enabling 12 and 13. I
>>>>> think we should remove the check above and call
>>>>> wiphy_read_of_freq_limits() as a last step within
>>>>> brcmf_setup_wiphybands(). It means it is called every time, but it
>>>>> safeguards that the limits in DT are always applied.
>>>>
>>>> I'm not exactly happy with channels management in brcmfmac. Before
>>>> calling wiphy_register it already disables channels unavailable for
>>>> current country. This results in setting IEEE80211_CHAN_DISABLED in
>>>
>>> What do you mean by current country. There is none that we are aware off
>>> in the driver. So we obtain the channels for the current
>>> country/revision in the firmware and enable those before
>>> wiphy_register(). This all is within the probe/init sequence so I do not
>>> really see an issue. As the wiphy object is not yet registered there is
>>> no user-space awareness
>>
>> It seems I'm terrible as describing my patches/problems/ideas :( Here
>> I used 1 inaccurate word and you couldn't understand my point.
>
> Well. Because of our track record of miscommunication and other
> annoyances I want to be sure this time :-p
>
>> By "current country" I meant current region (and so a set of
>> regulatory rules) used by the firmware. I believe firmware describes
>> it using "ccode" and "regrev".
>>
>> Now, about the issue I see:
>>
>> AFAIU if you set IEEE80211_CHAN_DISABLED in "orig_flags" it's meant to
>> be there for good. Some reference code that makes me believe so
>
> Indeed. Admittedly, it is the first time I became aware of it when
> Johannes brought it up.
>
>> (reg.c):
>> pr_debug("Disabling freq %d MHz for good\n", chan->center_freq);
>> chan->orig_flags |= IEEE80211_CHAN_DISABLED;
>>
>> This is what happens with brcmfmac right now. If firmware doesn't
>> report some channels, you set "flags" to IEEE80211_CHAN_DISABLED for
>> them. Then you call wiphy_register which copies that "flags" to the
>> "orig_flags". I read it as: we are never going to use these channels.
>>
>> Now, consider you support regdom change (I do with my local patches).
>> You translate alpha2 to a proper firmware request (board specific!),
>> you execute it and then firmware may allow you to use channels that
>> you marked as disabled for good. You would need to mess with
>> orig_flags to recover from this issue.
>>
>> Does my explanation make more sense of this issue now?
>
> Sure. It seems we should leave all channels enabled and disable them
> after wiphy_register() based on firmware information. Or did you have
> something else in mind. DFS channels may need to pass a feature check in
> firmware and always be disabled otherwise.

I got in mind exactly what you described. I didn't look at DFS channels yet.


>>>> orig_flags of channels that may become available later, after country
>>>> change. Please note it happens even right now, without this patch.
>>>
>>> Nope. As stated earlier the country setting in firmware is not updated
>>> unless you provide a *proper* mapping of user-space country code to
>>> firmware country code/revision. That is the reason, ie. firmware simply
>>> returns the same list of channels as nothing changed from its
>>> perspective. We may actually drop 11d support.
>>
>> I implemented mapping support locally, this is the feature I'm talking about.
>
> Ok. So this is not an upstream feature. Or are you getting the mapping
> from DT?

I'll send patch next week. So far I put mapping table directly in a
driver, but I think it's better to have this in DT.


>>>> Maybe you can workaround this by ignoring orig_flags in custom
>>>> regulatory code, but I'd just prefer to have it nicely handled in the
>>>> first place.
>>>
>>> Please care to explain your ideas before putting any effort in this
>>> "feature". As the author of the code that makes you unhappy and as
>>> driver maintainer I would like to get a clearer picture of your point of
>>> view. What exactly is the issue that makes you unhappy.
>>
>> I meant that problem with "orig_flags" I described in the first
>> paragraph. I wasn't trying to hide whatever issue I'm seeing, I swear
>> ;)
>>
>>
>>>> This is the next feature I'm going to work on. AFAIU this patch won't
>>>> be applied for now (it's for wireless-drivers-next and we first need
>>>> to get wiphy_read_of_freq_limits in that tree). By the time that
>>>> happens I may have another patchset cleaning brcmfmac ready. And FWIW
>>>> this patch wouldn't make things worse *at this point* as we don't
>>>> really support country switching for any device yet.
>>>
>>> Now who is *we*? We as Broadcom can, because we know how to map the ISO
>>> 3166-1 country code to firmware country code/revision for a specific
>>> firmware release. Firmware uses its own regulatory rules which may
>>> differ from what regdb has. Now I know Intel submitted a mechanism to
>>> export firmware rules to regdb so maybe we should consider switching to
>>> that api if that has been upstreamed. Need to check.
>>
>> We as a driver developers. Please read
>> "we don't really support country switching for any device yet"
>> as
>> "brcmfmac doesn't really support country switching for any device yet"
>>
>> Does it help to get the context?
>
> I indeed prefer to talk about the driver instead of we. Indeed it is
> true due to the orig_flags behavior although that only seems to involve
> regulatory code. Could it be that brcmfmac undo that through the notifier?

I guess you could touch orig_flags, but I don't know if it's preferred
way. This is probably question to Johannes & cfg80211 guys.

-- 
Rafał
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH V6 1/3] dt-bindings: document common IEEE 802.11 frequency limit property
From: Rafał Miłecki @ 2017-01-07 12:53 UTC (permalink / raw)
  To: Johannes Berg
  Cc: Rob Herring, linux-wireless, Martin Blumenstingl, Felix Fietkau,
	Arend van Spriel, Arnd Bergmann,
	devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	Rafał Miłecki
In-Reply-To: <1483707597.12677.0.camel-cdvu00un1VgdHxzADdlk8Q@public.gmane.org>

On 6 January 2017 at 13:59, Johannes Berg <johannes-cdvu00un1VgdHxzADdlk8Q@public.gmane.org> wrote:
> On Thu, 2017-01-05 at 10:34 -0600, Rob Herring wrote:
>> On Thu, Jan 5, 2017 at 5:51 AM, Johannes Berg <johannes@sipsolutions.
>> net> wrote:
>> >
>> > > Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>> >
>> > Do I take that to mean that we'll merge it through the subsystem
>> > tree,
>> > and not go through some common DT tree?
>>
>> Yes, that's generally the case when bindings are in a series with
>> driver changes.
>
> Alright, thanks.
>
> I've applied patches 1-3, patch 4 obviously still needs work (and
> probably won't go through my tree anyway.)
>
> Note that I made some documentation fixes in patch 3, Rafał, please
> check.

Looks great, thanks everyone for your help on getting this patchset in shape!

-- 
Rafał
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH V6 4/3] brcmfmac: use wiphy_read_of_freq_limits to respect extra limits
From: Arend Van Spriel @ 2017-01-07 12:52 UTC (permalink / raw)
  To: Rafał Miłecki
  Cc: Johannes Berg,
	linux-wireless-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	Martin Blumenstingl, Felix Fietkau, Arend van Spriel,
	Arnd Bergmann, devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	Rob Herring, Rafał Miłecki
In-Reply-To: <CACna6rw22benfuw_7BSFw1wedavmMJWTo_hfPLCVa1t0kV+Aqg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>

On 5-1-2017 11:02, Rafał Miłecki wrote:
> On 5 January 2017 at 10:31, Arend Van Spriel
> <arend.vanspriel-dY08KVG/lbpWk0Htik3J/w@public.gmane.org> wrote:
>> On 4-1-2017 22:19, Rafał Miłecki wrote:
>>> On 4 January 2017 at 21:07, Arend Van Spriel
>>> <arend.vanspriel-dY08KVG/lbpWk0Htik3J/w@public.gmane.org> wrote:
>>>> On 4-1-2017 18:58, Rafał Miłecki wrote:
>>>>> From: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>
>>>>>
>>>>> There are some devices (e.g. Netgear R8000 home router) with one chipset
>>>>> model used for different radios, some of them limited to subbands. NVRAM
>>>>> entries don't contain any extra info on such limitations and firmware
>>>>> reports full list of channels to us. We need to store extra limitation
>>>>> info in DT to support such devices properly.
>>>>>
>>>>> Now there is a cfg80211 helper for reading such info use it in brcmfmac.
>>>>> This patch adds check for channel being disabled with orig_flags which
>>>>> is how this wiphy helper and wiphy_register work.
>>>>>
>>>>> Signed-off-by: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>
>>>>> ---
>>>>> This patch should probably go through wireless-driver-next which is why
>>>>> it got weird number 4/3. I'm sending it just as a proof of concept.
>>>>> It was succesfully tested on SmartRG SR400ac with BCM43602.
>>>>>
>>>>> V4: Respect IEEE80211_CHAN_DISABLED in orig_flags
>>>>> V5: Update commit message
>>>>> V6: Call wiphy_read_of_freq_limits after brcmf_setup_wiphybands to make it work
>>>>>     with helper setting "flags" instead of "orig_flags".
>>>>> ---
>>>>>  drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c | 9 ++++++++-
>>>>>  1 file changed, 8 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
>>>>> index ccae3bb..a008ba5 100644
>>>>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
>>>>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
>>>>> @@ -5886,6 +5886,9 @@ static int brcmf_construct_chaninfo(struct brcmf_cfg80211_info *cfg,
>>>>>                                                      band->band);
>>>>>               channel[index].hw_value = ch.control_ch_num;
>>>>>
>>>>> +             if (channel->orig_flags & IEEE80211_CHAN_DISABLED)
>>>>> +                     continue;
>>>>> +
>>>>
>>>> So to be clear this is still needed for subsequent calls to
>>>> brcmf_setup_wiphybands(). The subsequent calls are done from the
>>>> regulatory notifier. So I think we have an issue with this approach. Say
>>>> the device comes up with US. That would set DISABLED flags for channels
>>>> 12 to 14. With a country update to PL we would want to enable channels
>>>> 12 and 13, right? The orig_flags are copied from the initial flags
>>>> during wiphy_register() so it seems we will skip enabling 12 and 13. I
>>>> think we should remove the check above and call
>>>> wiphy_read_of_freq_limits() as a last step within
>>>> brcmf_setup_wiphybands(). It means it is called every time, but it
>>>> safeguards that the limits in DT are always applied.
>>>
>>> I'm not exactly happy with channels management in brcmfmac. Before
>>> calling wiphy_register it already disables channels unavailable for
>>> current country. This results in setting IEEE80211_CHAN_DISABLED in
>>
>> What do you mean by current country. There is none that we are aware off
>> in the driver. So we obtain the channels for the current
>> country/revision in the firmware and enable those before
>> wiphy_register(). This all is within the probe/init sequence so I do not
>> really see an issue. As the wiphy object is not yet registered there is
>> no user-space awareness
> 
> It seems I'm terrible as describing my patches/problems/ideas :( Here
> I used 1 inaccurate word and you couldn't understand my point.

Well. Because of our track record of miscommunication and other
annoyances I want to be sure this time :-p

> By "current country" I meant current region (and so a set of
> regulatory rules) used by the firmware. I believe firmware describes
> it using "ccode" and "regrev".
> 
> Now, about the issue I see:
> 
> AFAIU if you set IEEE80211_CHAN_DISABLED in "orig_flags" it's meant to
> be there for good. Some reference code that makes me believe so

Indeed. Admittedly, it is the first time I became aware of it when
Johannes brought it up.

> (reg.c):
> pr_debug("Disabling freq %d MHz for good\n", chan->center_freq);
> chan->orig_flags |= IEEE80211_CHAN_DISABLED;
> 
> This is what happens with brcmfmac right now. If firmware doesn't
> report some channels, you set "flags" to IEEE80211_CHAN_DISABLED for
> them. Then you call wiphy_register which copies that "flags" to the
> "orig_flags". I read it as: we are never going to use these channels.
> 
> Now, consider you support regdom change (I do with my local patches).
> You translate alpha2 to a proper firmware request (board specific!),
> you execute it and then firmware may allow you to use channels that
> you marked as disabled for good. You would need to mess with
> orig_flags to recover from this issue.
> 
> Does my explanation make more sense of this issue now?

Sure. It seems we should leave all channels enabled and disable them
after wiphy_register() based on firmware information. Or did you have
something else in mind. DFS channels may need to pass a feature check in
firmware and always be disabled otherwise.

>>> orig_flags of channels that may become available later, after country
>>> change. Please note it happens even right now, without this patch.
>>
>> Nope. As stated earlier the country setting in firmware is not updated
>> unless you provide a *proper* mapping of user-space country code to
>> firmware country code/revision. That is the reason, ie. firmware simply
>> returns the same list of channels as nothing changed from its
>> perspective. We may actually drop 11d support.
> 
> I implemented mapping support locally, this is the feature I'm talking about.

Ok. So this is not an upstream feature. Or are you getting the mapping
from DT?

>>> Maybe you can workaround this by ignoring orig_flags in custom
>>> regulatory code, but I'd just prefer to have it nicely handled in the
>>> first place.
>>
>> Please care to explain your ideas before putting any effort in this
>> "feature". As the author of the code that makes you unhappy and as
>> driver maintainer I would like to get a clearer picture of your point of
>> view. What exactly is the issue that makes you unhappy.
> 
> I meant that problem with "orig_flags" I described in the first
> paragraph. I wasn't trying to hide whatever issue I'm seeing, I swear
> ;)
> 
> 
>>> This is the next feature I'm going to work on. AFAIU this patch won't
>>> be applied for now (it's for wireless-drivers-next and we first need
>>> to get wiphy_read_of_freq_limits in that tree). By the time that
>>> happens I may have another patchset cleaning brcmfmac ready. And FWIW
>>> this patch wouldn't make things worse *at this point* as we don't
>>> really support country switching for any device yet.
>>
>> Now who is *we*? We as Broadcom can, because we know how to map the ISO
>> 3166-1 country code to firmware country code/revision for a specific
>> firmware release. Firmware uses its own regulatory rules which may
>> differ from what regdb has. Now I know Intel submitted a mechanism to
>> export firmware rules to regdb so maybe we should consider switching to
>> that api if that has been upstreamed. Need to check.
> 
> We as a driver developers. Please read
> "we don't really support country switching for any device yet"
> as
> "brcmfmac doesn't really support country switching for any device yet"
> 
> Does it help to get the context?

I indeed prefer to talk about the driver instead of we. Indeed it is
true due to the orig_flags behavior although that only seems to involve
regulatory code. Could it be that brcmfmac undo that through the notifier?

Regards,
Arend
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox