[PATCH 1/2] isp1704_charger: Add DT support

[PATCH 1/2] isp1704_charger: Add DT support

[Sebastian Reichel]

This patch introduces device tree support to the isp1704
charger driver. Adding support involved moving the handling
of the enable GPIO from board code into the driver.

Signed-off-by: Sebastian Reichel <sre@debian.org>
---
 arch/arm/mach-omap2/board-rx51-peripherals.c | 10 +----
 drivers/power/isp1704_charger.c              | 56 +++++++++++++++++++++++++---
 include/linux/power/isp1704_charger.h        |  2 +-
 3 files changed, 52 insertions(+), 16 deletions(-)

diff --git a/arch/arm/mach-omap2/board-rx51-peripherals.c b/arch/arm/mach-omap2/board-rx51-peripherals.c
index a791fef..2986d16 100644
--- a/arch/arm/mach-omap2/board-rx51-peripherals.c
+++ b/arch/arm/mach-omap2/board-rx51-peripherals.c
@@ -273,13 +273,8 @@ static struct platform_device rx51_battery_device = {
 	.id	= -1,
 };
 
-static void rx51_charger_set_power(bool on)
-{
-	gpio_set_value(RX51_USB_TRANSCEIVER_RST_GPIO, on);
-}
-
 static struct isp1704_charger_data rx51_charger_data = {
-	.set_power	= rx51_charger_set_power,
+	.enable_gpio	= RX51_USB_TRANSCEIVER_RST_GPIO,
 };
 
 static struct platform_device rx51_charger_device = {
@@ -291,9 +286,6 @@ static struct platform_device rx51_charger_device = {
 
 static void __init rx51_charger_init(void)
 {
-	WARN_ON(gpio_request_one(RX51_USB_TRANSCEIVER_RST_GPIO,
-		GPIOF_OUT_INIT_HIGH, "isp1704_reset"));
-
 	platform_device_register(&rx51_battery_device);
 	platform_device_register(&rx51_charger_device);
 }
diff --git a/drivers/power/isp1704_charger.c b/drivers/power/isp1704_charger.c
index fc04d19..db96778 100644
--- a/drivers/power/isp1704_charger.c
+++ b/drivers/power/isp1704_charger.c
@@ -28,6 +28,8 @@
 #include <linux/platform_device.h>
 #include <linux/power_supply.h>
 #include <linux/delay.h>
+#include <linux/of.h>
+#include <linux/of_gpio.h>
 
 #include <linux/usb/otg.h>
 #include <linux/usb/ulpi.h>
@@ -89,8 +91,8 @@ static void isp1704_charger_set_power(struct isp1704_charger *isp, bool on)
 {
 	struct isp1704_charger_data	*board = isp->dev->platform_data;
 
-	if (board && board->set_power)
-		board->set_power(on);
+	if (board)
+		gpio_set_value(board->enable_gpio, on);
 }
 
 /*
@@ -411,12 +413,46 @@ static int isp1704_charger_probe(struct platform_device *pdev)
 	struct isp1704_charger	*isp;
 	int			ret = -ENODEV;
 
+	struct isp1704_charger_data *pdata = dev_get_platdata(&pdev->dev);
+	struct device_node *np = pdev->dev.of_node;
+
+	if (np) {
+		int gpio = of_get_named_gpio(np, "nxp,enable-gpio", 0);
+
+		if (gpio < 0)
+			return gpio;
+
+		pdata = devm_kzalloc(&pdev->dev,
+			sizeof(struct isp1704_charger_data), GFP_KERNEL);
+		pdata->enable_gpio = gpio;
+	}
+
+	if (!pdata) {
+		dev_err(&pdev->dev, "missing platform data!\n");
+		return -ENODEV;
+	}
+
+	dev_info(&pdev->dev, "init gpio %d\n", pdata->enable_gpio);
+
+	ret = devm_gpio_request_one(&pdev->dev, pdata->enable_gpio,
+					GPIOF_OUT_INIT_HIGH, "isp1704_reset");
+	if (ret)
+		goto fail0;
+
 	isp = devm_kzalloc(&pdev->dev, sizeof(*isp), GFP_KERNEL);
 	if (!isp)
 		return -ENOMEM;
 
-	isp->phy = usb_get_phy(USB_PHY_TYPE_USB2);
-	if (IS_ERR_OR_NULL(isp->phy))
+	if (np)
+		isp->phy = devm_usb_get_phy_by_phandle(&pdev->dev, "usb-phy", 0);
+	else
+		isp->phy = devm_usb_get_phy(&pdev->dev, USB_PHY_TYPE_USB2);
+
+	if (IS_ERR(isp->phy)) {
+		ret = PTR_ERR(isp->phy);
+		goto fail0;
+	}
+	if (!isp->phy)
 		goto fail0;
 
 	isp->dev = &pdev->dev;
@@ -475,7 +511,6 @@ fail2:
 	power_supply_unregister(&isp->psy);
 fail1:
 	isp1704_charger_set_power(isp, 0);
-	usb_put_phy(isp->phy);
 fail0:
 	dev_err(&pdev->dev, "failed to register isp1704 with error %d\n", ret);
 
@@ -488,15 +523,24 @@ static int isp1704_charger_remove(struct platform_device *pdev)
 
 	usb_unregister_notifier(isp->phy, &isp->nb);
 	power_supply_unregister(&isp->psy);
-	usb_put_phy(isp->phy);
 	isp1704_charger_set_power(isp, 0);
 
 	return 0;
 }
 
+
+#ifdef CONFIG_OF
+static const struct of_device_id omap_isp1704_of_match[] = {
+	{ .compatible = "nxp,isp1704", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, omap_isp1704_of_match);
+#endif
+
 static struct platform_driver isp1704_charger_driver = {
 	.driver = {
 		.name = "isp1704_charger",
+		.of_match_table = of_match_ptr(omap_isp1704_of_match),
 	},
 	.probe = isp1704_charger_probe,
 	.remove = isp1704_charger_remove,
diff --git a/include/linux/power/isp1704_charger.h b/include/linux/power/isp1704_charger.h
index 68096a6..05081a9 100644
--- a/include/linux/power/isp1704_charger.h
+++ b/include/linux/power/isp1704_charger.h
@@ -23,7 +23,7 @@
 #define __ISP1704_CHARGER_H
 
 struct isp1704_charger_data {
-	void		(*set_power)(bool on);
+	int		enable_gpio;
 };
 
 #endif
-- 
1.8.4.2

[PATCH 2/2] dt: binding documentation for isp1704 charger

[Sebastian Reichel]

Add devicetree binding documentation for isp1704-charger.

Signed-off-by: Sebastian Reichel <sre@debian.org>
---
 Documentation/devicetree/bindings/power/isp1704.txt | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/power/isp1704.txt

diff --git a/Documentation/devicetree/bindings/power/isp1704.txt b/Documentation/devicetree/bindings/power/isp1704.txt
new file mode 100644
index 0000000..bbeec11
--- /dev/null
+++ b/Documentation/devicetree/bindings/power/isp1704.txt
@@ -0,0 +1,17 @@
+Binding for NXP ISP1704 USB Charger Detection
+
+Required properties:
+- compatible: Should contain one of the following:
+ * "nxp,isp1704"
+- nxp,enable-gpio: Should contain a phandle to the
+  GPIO pin connected to the chip's enable pin.
+- usb-phy: Should contain a phandle to the USB PHY
+  the ISP1704 is connected to.
+
+Example:
+
+isp1704 {
+	compatible = "nxp,isp1704";
+	nxp,enable-gpio = <&gpio3 3 GPIO_ACTIVE_LOW>;
+	usb-phy = <&usb2_phy>;
+};
-- 
1.8.4.2

Re: [PATCH 1/2] isp1704_charger: Add DT support

[Tony Lindgren]

* Sebastian Reichel <sre@debian.org> [131114 04:40]:
> This patch introduces device tree support to the isp1704
> charger driver. Adding support involved moving the handling
> of the enable GPIO from board code into the driver.
> 
> Signed-off-by: Sebastian Reichel <sre@debian.org>
> ---
>  arch/arm/mach-omap2/board-rx51-peripherals.c | 10 +----
>  drivers/power/isp1704_charger.c              | 56 +++++++++++++++++++++++++---
>  include/linux/power/isp1704_charger.h        |  2 +-
>  3 files changed, 52 insertions(+), 16 deletions(-)
> 
> diff --git a/arch/arm/mach-omap2/board-rx51-peripherals.c b/arch/arm/mach-omap2/board-rx51-peripherals.c
> index a791fef..2986d16 100644
> --- a/arch/arm/mach-omap2/board-rx51-peripherals.c
> +++ b/arch/arm/mach-omap2/board-rx51-peripherals.c
> @@ -273,13 +273,8 @@ static struct platform_device rx51_battery_device = {
>  	.id	= -1,
>  };
>  
> -static void rx51_charger_set_power(bool on)
> -{
> -	gpio_set_value(RX51_USB_TRANSCEIVER_RST_GPIO, on);
> -}
> -
>  static struct isp1704_charger_data rx51_charger_data = {
> -	.set_power	= rx51_charger_set_power,
> +	.enable_gpio	= RX51_USB_TRANSCEIVER_RST_GPIO,
>  };
>  
>  static struct platform_device rx51_charger_device = {
> @@ -291,9 +286,6 @@ static struct platform_device rx51_charger_device = {
>  
>  static void __init rx51_charger_init(void)
>  {
> -	WARN_ON(gpio_request_one(RX51_USB_TRANSCEIVER_RST_GPIO,
> -		GPIOF_OUT_INIT_HIGH, "isp1704_reset"));
> -
>  	platform_device_register(&rx51_battery_device);
>  	platform_device_register(&rx51_charger_device);
>  }

Can you please separate out the changes to board-rx51-peripherals.c
as they have a huge chance of merge conflicts for v3.14 when
we're planning to flip omap3 to be device tree only?

To remove the dependency, how about add .enable_gpio to the
isp1704_charger.h and to the board-rx51-peripherals.c without
removing .set_power in the first patch?

Then we can set up an immutable branch that can be merged both
to linux-omap tree and Anton's tree to avoid merge conflicts.

Then the rest of the patches can go via Anton's tree and you
can just have .set_power do nothing, and then it can be
removed later on when no longer used at all.

Regards,

Tony

Re: [PATCH 1/2] isp1704_charger: Add DT support

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 937 bytes --]

On Thursday 14 November 2013 13:38:53 Sebastian Reichel wrote:
> diff --git a/drivers/power/isp1704_charger.c
> b/drivers/power/isp1704_charger.c index fc04d19..db96778
> 100644
> --- a/drivers/power/isp1704_charger.c
> +++ b/drivers/power/isp1704_charger.c
> @@ -28,6 +28,8 @@
>  #include <linux/platform_device.h>
>  #include <linux/power_supply.h>
>  #include <linux/delay.h>
> +#include <linux/of.h>
> +#include <linux/of_gpio.h>
> 
>  #include <linux/usb/otg.h>
>  #include <linux/usb/ulpi.h>
> @@ -89,8 +91,8 @@ static void isp1704_charger_set_power(struct
> isp1704_charger *isp, bool on) {
>  	struct isp1704_charger_data	*board =
> isp->dev->platform_data;
> 
> -	if (board && board->set_power)
> -		board->set_power(on);
> +	if (board)
> +		gpio_set_value(board->enable_gpio, on);
>  }
> 

You need to check if enable_gpio in board data was defined or not.

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 1/2] isp1704_charger: Add DT support

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 1094 bytes --]

On Thu, Nov 14, 2013 at 07:05:06PM +0100, Pali Rohár wrote:
> On Thursday 14 November 2013 13:38:53 Sebastian Reichel wrote:
> > diff --git a/drivers/power/isp1704_charger.c
> > b/drivers/power/isp1704_charger.c index fc04d19..db96778
> > 100644
> > --- a/drivers/power/isp1704_charger.c
> > +++ b/drivers/power/isp1704_charger.c
> > @@ -28,6 +28,8 @@
> >  #include <linux/platform_device.h>
> >  #include <linux/power_supply.h>
> >  #include <linux/delay.h>
> > +#include <linux/of.h>
> > +#include <linux/of_gpio.h>
> > 
> >  #include <linux/usb/otg.h>
> >  #include <linux/usb/ulpi.h>
> > @@ -89,8 +91,8 @@ static void isp1704_charger_set_power(struct
> > isp1704_charger *isp, bool on) {
> >  	struct isp1704_charger_data	*board =
> > isp->dev->platform_data;
> > 
> > -	if (board && board->set_power)
> > -		board->set_power(on);
> > +	if (board)
> > +		gpio_set_value(board->enable_gpio, on);
> >  }
> > 
> 
> You need to check if enable_gpio in board data was defined or not.

The device is not successful probed without valid enable_gpio.

-- Sebastian

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 1/2] isp1704_charger: Add DT support

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 1334 bytes --]

On Thursday 14 November 2013 23:34:19 Sebastian Reichel wrote:
> On Thu, Nov 14, 2013 at 07:05:06PM +0100, Pali Rohár wrote:
> > On Thursday 14 November 2013 13:38:53 Sebastian Reichel 
wrote:
> > > diff --git a/drivers/power/isp1704_charger.c
> > > b/drivers/power/isp1704_charger.c index fc04d19..db96778
> > > 100644
> > > --- a/drivers/power/isp1704_charger.c
> > > +++ b/drivers/power/isp1704_charger.c
> > > @@ -28,6 +28,8 @@
> > > 
> > >  #include <linux/platform_device.h>
> > >  #include <linux/power_supply.h>
> > >  #include <linux/delay.h>
> > > 
> > > +#include <linux/of.h>
> > > +#include <linux/of_gpio.h>
> > > 
> > >  #include <linux/usb/otg.h>
> > >  #include <linux/usb/ulpi.h>
> > > 
> > > @@ -89,8 +91,8 @@ static void
> > > isp1704_charger_set_power(struct isp1704_charger *isp,
> > > bool on) {
> > > 
> > >  	struct isp1704_charger_data	*board =
> > > 
> > > isp->dev->platform_data;
> > > 
> > > -	if (board && board->set_power)
> > > -		board->set_power(on);
> > > +	if (board)
> > > +		gpio_set_value(board->enable_gpio, on);
> > > 
> > >  }
> > 
> > You need to check if enable_gpio in board data was defined
> > or not.
> 
> The device is not successful probed without valid enable_gpio.
> 
> -- Sebastian

Then, OK.

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 2/2] dt: binding documentation for isp1704 charger

[Mark Rutland]

On Thu, Nov 14, 2013 at 12:38:54PM +0000, Sebastian Reichel wrote:
> Add devicetree binding documentation for isp1704-charger.
> 
> Signed-off-by: Sebastian Reichel <sre@debian.org>
> ---
>  Documentation/devicetree/bindings/power/isp1704.txt | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/power/isp1704.txt
> 
> diff --git a/Documentation/devicetree/bindings/power/isp1704.txt b/Documentation/devicetree/bindings/power/isp1704.txt
> new file mode 100644
> index 0000000..bbeec11
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/isp1704.txt
> @@ -0,0 +1,17 @@
> +Binding for NXP ISP1704 USB Charger Detection
> +
> +Required properties:
> +- compatible: Should contain one of the following:
> + * "nxp,isp1704"
> +- nxp,enable-gpio: Should contain a phandle to the

Nit: phandle + gpio-specifier

Otherwise this looks fine.

Mark.

[PATCHv2 0/2] DT support for isp1704-charger

[Sebastian Reichel]

Hi,

This is the second iteration of the isp1704 DT patches.

Changes since v1:
 * reword the binding documentation slightly according
   to the suggestions of Mark Rutland
 * keep supporting the set_power callback and leave the
   board code in its current state. This solves potential
   merge problems. The additional code can be removed in
   the next merge window.

-- Sebastian

Sebastian Reichel (2):
  isp1704_charger: Add DT support
  dt: binding documentation for isp1704 charger

 .../devicetree/bindings/power/isp1704.txt          | 17 +++++++
 drivers/power/isp1704_charger.c                    | 55 ++++++++++++++++++++--
 include/linux/power/isp1704_charger.h              |  1 +
 3 files changed, 69 insertions(+), 4 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/power/isp1704.txt

-- 
1.8.4.3

[PATCHv2 1/2] isp1704_charger: Add DT support

[Sebastian Reichel]

This patch introduces device tree support to the isp1704
charger driver. Adding support involved moving the handling
of the enable GPIO from board code into the driver.

Signed-off-by: Sebastian Reichel <sre@debian.org>
---
 drivers/power/isp1704_charger.c       | 55 ++++++++++++++++++++++++++++++++---
 include/linux/power/isp1704_charger.h |  1 +
 2 files changed, 52 insertions(+), 4 deletions(-)

diff --git a/drivers/power/isp1704_charger.c b/drivers/power/isp1704_charger.c
index 1bb3a91..2535989 100644
--- a/drivers/power/isp1704_charger.c
+++ b/drivers/power/isp1704_charger.c
@@ -29,6 +29,8 @@
 #include <linux/platform_device.h>
 #include <linux/power_supply.h>
 #include <linux/delay.h>
+#include <linux/of.h>
+#include <linux/of_gpio.h>
 
 #include <linux/usb/otg.h>
 #include <linux/usb/ulpi.h>
@@ -88,6 +90,8 @@ static void isp1704_charger_set_power(struct isp1704_charger *isp, bool on)
 
 	if (board && board->set_power)
 		board->set_power(on);
+	else if (board)
+		gpio_set_value(board->enable_gpio, on);
 }
 
 /*
@@ -400,12 +404,47 @@ static int isp1704_charger_probe(struct platform_device *pdev)
 	struct isp1704_charger	*isp;
 	int			ret = -ENODEV;
 
+	struct isp1704_charger_data *pdata = dev_get_platdata(&pdev->dev);
+	struct device_node *np = pdev->dev.of_node;
+
+	if (np) {
+		int gpio = of_get_named_gpio(np, "nxp,enable-gpio", 0);
+
+		if (gpio < 0)
+			return gpio;
+
+		pdata = devm_kzalloc(&pdev->dev,
+			sizeof(struct isp1704_charger_data), GFP_KERNEL);
+		pdata->enable_gpio = gpio;
+
+		dev_info(&pdev->dev, "init gpio %d\n", pdata->enable_gpio);
+
+		ret = devm_gpio_request_one(&pdev->dev, pdata->enable_gpio,
+					GPIOF_OUT_INIT_HIGH, "isp1704_reset");
+		if (ret)
+			goto fail0;
+	}
+
+	if (!pdata) {
+		dev_err(&pdev->dev, "missing platform data!\n");
+		return -ENODEV;
+	}
+
+
 	isp = devm_kzalloc(&pdev->dev, sizeof(*isp), GFP_KERNEL);
 	if (!isp)
 		return -ENOMEM;
 
-	isp->phy = usb_get_phy(USB_PHY_TYPE_USB2);
-	if (IS_ERR_OR_NULL(isp->phy))
+	if (np)
+		isp->phy = devm_usb_get_phy_by_phandle(&pdev->dev, "usb-phy", 0);
+	else
+		isp->phy = devm_usb_get_phy(&pdev->dev, USB_PHY_TYPE_USB2);
+
+	if (IS_ERR(isp->phy)) {
+		ret = PTR_ERR(isp->phy);
+		goto fail0;
+	}
+	if (!isp->phy)
 		goto fail0;
 
 	isp->dev = &pdev->dev;
@@ -464,7 +503,6 @@ fail2:
 	power_supply_unregister(&isp->psy);
 fail1:
 	isp1704_charger_set_power(isp, 0);
-	usb_put_phy(isp->phy);
 fail0:
 	dev_err(&pdev->dev, "failed to register isp1704 with error %d\n", ret);
 
@@ -477,15 +515,24 @@ static int isp1704_charger_remove(struct platform_device *pdev)
 
 	usb_unregister_notifier(isp->phy, &isp->nb);
 	power_supply_unregister(&isp->psy);
-	usb_put_phy(isp->phy);
 	isp1704_charger_set_power(isp, 0);
 
 	return 0;
 }
 
+
+#ifdef CONFIG_OF
+static const struct of_device_id omap_isp1704_of_match[] = {
+	{ .compatible = "nxp,isp1704", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, omap_isp1704_of_match);
+#endif
+
 static struct platform_driver isp1704_charger_driver = {
 	.driver = {
 		.name = "isp1704_charger",
+		.of_match_table = of_match_ptr(omap_isp1704_of_match),
 	},
 	.probe = isp1704_charger_probe,
 	.remove = isp1704_charger_remove,
diff --git a/include/linux/power/isp1704_charger.h b/include/linux/power/isp1704_charger.h
index 68096a6..0105d9e 100644
--- a/include/linux/power/isp1704_charger.h
+++ b/include/linux/power/isp1704_charger.h
@@ -24,6 +24,7 @@
 
 struct isp1704_charger_data {
 	void		(*set_power)(bool on);
+	int		enable_gpio;
 };
 
 #endif
-- 
1.8.4.3

[PATCHv2 2/2] dt: binding documentation for isp1704 charger

[Sebastian Reichel]

Add devicetree binding documentation for isp1704-charger.

Signed-off-by: Sebastian Reichel <sre@debian.org>
---
 Documentation/devicetree/bindings/power/isp1704.txt | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/power/isp1704.txt

diff --git a/Documentation/devicetree/bindings/power/isp1704.txt b/Documentation/devicetree/bindings/power/isp1704.txt
new file mode 100644
index 0000000..fa35969
--- /dev/null
+++ b/Documentation/devicetree/bindings/power/isp1704.txt
@@ -0,0 +1,17 @@
+Binding for NXP ISP1704 USB Charger Detection
+
+Required properties:
+- compatible: Should contain one of the following:
+ * "nxp,isp1704"
+- nxp,enable-gpio: Should contain a phandle + gpio-specifier
+  to the GPIO pin connected to the chip's enable pin.
+- usb-phy: Should contain a phandle to the USB PHY
+  the ISP1704 is connected to.
+
+Example:
+
+isp1704 {
+	compatible = "nxp,isp1704";
+	nxp,enable-gpio = <&gpio3 3 GPIO_ACTIVE_LOW>;
+	usb-phy = <&usb2_phy>;
+};
-- 
1.8.4.3

Re: [PATCHv2 0/2] DT support for isp1704-charger

[Anton Vorontsov]

On Sun, Dec 01, 2013 at 02:00:09AM +0100, Sebastian Reichel wrote:
> This is the second iteration of the isp1704 DT patches.
> 
> Changes since v1:
>  * reword the binding documentation slightly according
>    to the suggestions of Mark Rutland
>  * keep supporting the set_power callback and leave the
>    board code in its current state. This solves potential
>    merge problems. The additional code can be removed in
>    the next merge window.

Applied, thanks a lot!

Anton