[v2] wlcore: add basic device-tree support

Commit Message

Eliad Peller Feb. 15, 2015, 11:09 a.m. UTC

When running with device-tree, we no longer have a board file
that can set up the platform data for wlcore.
Allow this data to be passed from DT.

For now, parse only the irq used. Other (optional) properties
can be added later on.

Signed-off-by: Ido Yariv <[e-mail]>
Signed-off-by: Eliad Peller <[e-mail]>
---
previous submission of this patch got lost somewhere.

this patch follows the expected way to pass SDIO function DT
information, as was recently added by this patchset:
http://www.spinics.net/lists/linux-mmc/msg27353.html

 .../devicetree/bindings/net/wireless/ti,wlcore.txt | 31 +++++++++++
 drivers/net/wireless/ti/wlcore/sdio.c              | 65 ++++++++++++++++++++--
 2 files changed, 90 insertions(+), 6 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/net/wireless/ti,wlcore.txt

Comments

Arnd Bergmann Feb. 16, 2015, 10:06 a.m. UTC | #1

On Sunday 15 February 2015 13:09:10 Eliad Peller wrote:
s
> +
> +This node provides properties for controlling the wilink wireless device. The
> +node is expected to be specified as a child node to the SDIO controller that
> +connects the device to the system.
> +
> +Required properties:
> +
> + - compatible : Should be "ti,wlcore".

I think you should use the specific model number here. If I understand
correctly, wlcore is the name of the driver that is used for multiple
device implementation.

> + - interrupt-parent : the phandle for the interrupt controller to which the
> +	device interrupts are connected.

interrupt-parent should not be required

> +&mmc3 {
> +	status = "okay";
> +	vmmc-supply = <&wlan_en_reg>;
> +	bus-width = <4>;
> +	cap-power-off-card;
> +	keep-power-in-suspend;
> +
> +	#address-cells = <1>;
> +	#size-cells = <0>;
> +	wlcore: wlcore@0 {
> +		compatible = "ti,wlcore";
> +		reg = <2>;
> +		interrupt-parent = <&gpio0>;
> +		interrupts = <19 IRQ_TYPE_NONE>;
> +	};
> +};

It could make sense to specify a few extra properties here:

- The platform data lists two clocks. How about adding them
  here as optional clocks so we don't need to change the binding
  again.

> diff --git a/drivers/net/wireless/ti/wlcore/sdio.c b/drivers/net/wireless/ti/wlcore/sdio.c
> index d3dd7bf..317796b 100644
> --- a/drivers/net/wireless/ti/wlcore/sdio.c
> +++ b/drivers/net/wireless/ti/wlcore/sdio.c

Please make this a two-patch series and keep the dt binding in a separate
patch from the driver change.
  
> +#ifdef CONFIG_OF
> +static struct wl12xx_platform_data *wlcore_probe_of(struct device *dev)
> +{
> +	struct device_node *np = dev->of_node;
> +	struct wl12xx_platform_data *pdata;
> +
> +	if (!np || !of_device_is_compatible(np, "ti,wlcore")) {
> +		dev_err(dev, "No platform data set\n");
> +		return NULL;
> +	}
> +
> +	pdata = kzalloc(sizeof(*pdata), GFP_KERNEL);
> +	if (!pdata)
> +		return NULL;

Your method seems overly complicated. While a lot of drivers do the same
thing, allocating a platform_data structure at probe time is really
just extra work compared to making the platform_data optional and
adding the required fields into the driver-private structure.

	Arnd

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Eliad Peller Feb. 17, 2015, 3:39 p.m. UTC | #2

On Mon, Feb 16, 2015 at 12:06 PM, Arnd Bergmann <[e-mail]> wrote:
> On Sunday 15 February 2015 13:09:10 Eliad Peller wrote:
> s
>> +
>> +This node provides properties for controlling the wilink wireless device. The
>> +node is expected to be specified as a child node to the SDIO controller that
>> +connects the device to the system.
>> +
>> +Required properties:
>> +
>> + - compatible : Should be "ti,wlcore".
>
> I think you should use the specific model number here. If I understand
> correctly, wlcore is the name of the driver that is used for multiple
> device implementation.
>
right, wlcore is the common driver part of wl12xx and wl18xx device drivers.
these DT properties are common for both.
can't we use a common binding as well in this case?

>> + - interrupt-parent : the phandle for the interrupt controller to which the
>> +     device interrupts are connected.
>
> interrupt-parent should not be required
>
sure. i'll make it optional.

>> +&mmc3 {
>> +     status = "okay";
>> +     vmmc-supply = <&wlan_en_reg>;
>> +     bus-width = <4>;
>> +     cap-power-off-card;
>> +     keep-power-in-suspend;
>> +
>> +     #address-cells = <1>;
>> +     #size-cells = <0>;
>> +     wlcore: wlcore@0 {
>> +             compatible = "ti,wlcore";
>> +             reg = <2>;
>> +             interrupt-parent = <&gpio0>;
>> +             interrupts = <19 IRQ_TYPE_NONE>;
>> +     };
>> +};
>
> It could make sense to specify a few extra properties here:
>
> - The platform data lists two clocks. How about adding them
>   here as optional clocks so we don't need to change the binding
>   again.
>
There were some very long threads previously regarding the correct way
to describe these clocks.
I prefer starting a working basic implementation and add the
controversial parts later on, as needed.

>> diff --git a/drivers/net/wireless/ti/wlcore/sdio.c b/drivers/net/wireless/ti/wlcore/sdio.c
>> index d3dd7bf..317796b 100644
>> --- a/drivers/net/wireless/ti/wlcore/sdio.c
>> +++ b/drivers/net/wireless/ti/wlcore/sdio.c
>
> Please make this a two-patch series and keep the dt binding in a separate
> patch from the driver change.
>
sure.

>> +#ifdef CONFIG_OF
>> +static struct wl12xx_platform_data *wlcore_probe_of(struct device *dev)
>> +{
>> +     struct device_node *np = dev->of_node;
>> +     struct wl12xx_platform_data *pdata;
>> +
>> +     if (!np || !of_device_is_compatible(np, "ti,wlcore")) {
>> +             dev_err(dev, "No platform data set\n");
>> +             return NULL;
>> +     }
>> +
>> +     pdata = kzalloc(sizeof(*pdata), GFP_KERNEL);
>> +     if (!pdata)
>> +             return NULL;
>
> Your method seems overly complicated. While a lot of drivers do the same
> thing, allocating a platform_data structure at probe time is really
> just extra work compared to making the platform_data optional and
> adding the required fields into the driver-private structure.
i see your point here.
however, the driver already holds (and uses) a pointer describing the
platform_data (in the non-dt case), so this patch simply takes
leverage of the current behavior (and returns a similar pointer).

thanks for the review!

Eliad.
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Mark Rutland Feb. 17, 2015, 3:45 p.m. UTC | #3

On Tue, Feb 17, 2015 at 03:39:03PM +0000, Eliad Peller wrote:
> On Mon, Feb 16, 2015 at 12:06 PM, Arnd Bergmann <[e-mail]> wrote:
> > On Sunday 15 February 2015 13:09:10 Eliad Peller wrote:
> > s
> >> +
> >> +This node provides properties for controlling the wilink wireless device. The
> >> +node is expected to be specified as a child node to the SDIO controller that
> >> +connects the device to the system.
> >> +
> >> +Required properties:
> >> +
> >> + - compatible : Should be "ti,wlcore".
> >
> > I think you should use the specific model number here. If I understand
> > correctly, wlcore is the name of the driver that is used for multiple
> > device implementation.
> >
> right, wlcore is the common driver part of wl12xx and wl18xx device drivers.
> these DT properties are common for both.
> can't we use a common binding as well in this case?

Just have a string for each; the driver can easily match them all and
handle them identically for now until we decide/realise we need to
handle them differently later.

Regardless, the compatible string should match some real part number
and/or standard rather than the Linux driver name.

> 
> >> + - interrupt-parent : the phandle for the interrupt controller to which the
> >> +     device interrupts are connected.
> >
> > interrupt-parent should not be required
> >
> sure. i'll make it optional.
> 
> >> +&mmc3 {
> >> +     status = "okay";
> >> +     vmmc-supply = <&wlan_en_reg>;
> >> +     bus-width = <4>;
> >> +     cap-power-off-card;
> >> +     keep-power-in-suspend;
> >> +
> >> +     #address-cells = <1>;
> >> +     #size-cells = <0>;
> >> +     wlcore: wlcore@0 {
> >> +             compatible = "ti,wlcore";
> >> +             reg = <2>;
> >> +             interrupt-parent = <&gpio0>;
> >> +             interrupts = <19 IRQ_TYPE_NONE>;
> >> +     };
> >> +};
> >
> > It could make sense to specify a few extra properties here:
> >
> > - The platform data lists two clocks. How about adding them
> >   here as optional clocks so we don't need to change the binding
> >   again.
> >
> There were some very long threads previously regarding the correct way
> to describe these clocks.
> I prefer starting a working basic implementation and add the
> controversial parts later on, as needed.

This could be problematic. Elsewhere we've later added properties that
should have been mandatory from the start but were masked by some
platform data somewhere. It would be very good to avoid that.

What is problematic about describing the clock inputs to this block? Is
the problem specific to this block or to the specific clock
controller(s) it happens to be wired to?

Thanks,
Mark.
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Mark Rutland Feb. 17, 2015, 3:48 p.m. UTC | #4

[...]

> +	wlcore: wlcore@0 {
> +		compatible = "ti,wlcore";
> +		reg = <2>;

Nit: the reg and unit-address (the bit after the '@' in the node name)
should match.

Mark.
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Eliad Peller Feb. 18, 2015, 8:14 a.m. UTC | #5

On Tue, Feb 17, 2015 at 5:45 PM, Mark Rutland <[e-mail]> wrote:
> On Tue, Feb 17, 2015 at 03:39:03PM +0000, Eliad Peller wrote:
>> On Mon, Feb 16, 2015 at 12:06 PM, Arnd Bergmann <[e-mail]> wrote:
>> > On Sunday 15 February 2015 13:09:10 Eliad Peller wrote:
>> > s
>> >> +
>> >> +This node provides properties for controlling the wilink wireless device. The
>> >> +node is expected to be specified as a child node to the SDIO controller that
>> >> +connects the device to the system.
>> >> +
>> >> +Required properties:
>> >> +
>> >> + - compatible : Should be "ti,wlcore".
>> >
>> > I think you should use the specific model number here. If I understand
>> > correctly, wlcore is the name of the driver that is used for multiple
>> > device implementation.
>> >
>> right, wlcore is the common driver part of wl12xx and wl18xx device drivers.
>> these DT properties are common for both.
>> can't we use a common binding as well in this case?
>
> Just have a string for each; the driver can easily match them all and
> handle them identically for now until we decide/realise we need to
> handle them differently later.
>
> Regardless, the compatible string should match some real part number
> and/or standard rather than the Linux driver name.
>
ok, makes sense.

>> >> +&mmc3 {
>> >> +     status = "okay";
>> >> +     vmmc-supply = <&wlan_en_reg>;
>> >> +     bus-width = <4>;
>> >> +     cap-power-off-card;
>> >> +     keep-power-in-suspend;
>> >> +
>> >> +     #address-cells = <1>;
>> >> +     #size-cells = <0>;
>> >> +     wlcore: wlcore@0 {
>> >> +             compatible = "ti,wlcore";
>> >> +             reg = <2>;
>> >> +             interrupt-parent = <&gpio0>;
>> >> +             interrupts = <19 IRQ_TYPE_NONE>;
>> >> +     };
>> >> +};
>> >
>> > It could make sense to specify a few extra properties here:
>> >
>> > - The platform data lists two clocks. How about adding them
>> >   here as optional clocks so we don't need to change the binding
>> >   again.
>> >
>> There were some very long threads previously regarding the correct way
>> to describe these clocks.
>> I prefer starting a working basic implementation and add the
>> controversial parts later on, as needed.
>
> This could be problematic. Elsewhere we've later added properties that
> should have been mandatory from the start but were masked by some
> platform data somewhere. It would be very good to avoid that.
>
that's a good argument.
in this case, i'll start with submitting a binding only for wl18xx,
which doesn't need the clock bindings (they are needed only for
wl12xx).

> What is problematic about describing the clock inputs to this block? Is
> the problem specific to this block or to the specific clock
> controller(s) it happens to be wired to?
i'm referring to patches like this one:
http://thread.gmane.org/gmane.linux.kernel/1520752

Eliad.
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Luca Coelho Feb. 27, 2015, 7:31 a.m. UTC | #6

Hi,

On Sun, 2015-02-15 at 13:09 +0200, Eliad Peller wrote:
> When running with device-tree, we no longer have a board file
> that can set up the platform data for wlcore.
> Allow this data to be passed from DT.
> 
> For now, parse only the irq used. Other (optional) properties
> can be added later on.
> 
> Signed-off-by: Ido Yariv <[e-mail]>
> Signed-off-by: Eliad Peller <[e-mail]>
> ---

I don't really care much, but why not just finalize the work I did a
couple of years ago?

http://mid.gmane.org/[e-mail]
http://mid.gmane.org/[e-mail]

IIRC there were just some minor comments there, but I never really got
to it because I left TI.

--
Luca.

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Eliad Peller March 8, 2015, 11:04 a.m. UTC | #7

hi Luca,

On Fri, Feb 27, 2015 at 9:31 AM, Luca Coelho <[e-mail]> wrote:
> Hi,
>
> On Sun, 2015-02-15 at 13:09 +0200, Eliad Peller wrote:
>> When running with device-tree, we no longer have a board file
>> that can set up the platform data for wlcore.
>> Allow this data to be passed from DT.
>>
>> For now, parse only the irq used. Other (optional) properties
>> can be added later on.
>>
>> Signed-off-by: Ido Yariv <[e-mail]>
>> Signed-off-by: Eliad Peller <[e-mail]>
>> ---
>
> I don't really care much, but why not just finalize the work I did a
> couple of years ago?
>
> http://mid.gmane.org/[e-mail]
> http://mid.gmane.org/[e-mail]
>
> IIRC there were just some minor comments there, but I never really got
> to it because I left TI.

I just wanted a small patch to add DT support for wl18xx.
as there were some issues remaining wrt the correct way to specify the
clock data, i preferred taking the simpler route (for now), and only
add the wl18xx part, which doesn't require the clocks definitions.

i guess the patches will still be useful (and used) when wl12xx DT
support will be added.

Eliad.
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/Documentation/devicetree/bindings/net/wireless/ti,wlcore.txt b/Documentation/devicetree/bindings/net/wireless/ti,wlcore.txt
new file mode 100644
index 0000000..df9af48
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/wireless/ti,wlcore.txt
@@ -0,0 +1,31 @@ 
+TI Wilink (wlcore) SDIO devices
+
+This node provides properties for controlling the wilink wireless device. The
+node is expected to be specified as a child node to the SDIO controller that
+connects the device to the system.
+
+Required properties:
+
+ - compatible : Should be "ti,wlcore".
+ - interrupt-parent : the phandle for the interrupt controller to which the
+	device interrupts are connected.
+ - interrupts : specifies attributes for the out-of-band interrupt.
+
+Example:
+
+&mmc3 {
+	status = "okay";
+	vmmc-supply = <&wlan_en_reg>;
+	bus-width = <4>;
+	cap-power-off-card;
+	keep-power-in-suspend;
+
+	#address-cells = <1>;
+	#size-cells = <0>;
+	wlcore: wlcore@0 {
+		compatible = "ti,wlcore";
+		reg = <2>;
+		interrupt-parent = <&gpio0>;
+		interrupts = <19 IRQ_TYPE_NONE>;
+	};
+};
diff --git a/drivers/net/wireless/ti/wlcore/sdio.c b/drivers/net/wireless/ti/wlcore/sdio.c
index d3dd7bf..317796b 100644
--- a/drivers/net/wireless/ti/wlcore/sdio.c
+++ b/drivers/net/wireless/ti/wlcore/sdio.c
@@ -34,6 +34,8 @@ 
 #include <linux/wl12xx.h>
 #include <linux/pm_runtime.h>
 #include <linux/printk.h>
+#include <linux/of.h>
+#include <linux/of_irq.h>
 
 #include "wlcore.h"
 #include "wl12xx_80211.h"
@@ -214,6 +216,54 @@  static struct wl1271_if_operations sdio_ops = {
 	.set_block_size = wl1271_sdio_set_block_size,
 };
 
+#ifdef CONFIG_OF
+static struct wl12xx_platform_data *wlcore_probe_of(struct device *dev)
+{
+	struct device_node *np = dev->of_node;
+	struct wl12xx_platform_data *pdata;
+
+	if (!np || !of_device_is_compatible(np, "ti,wlcore")) {
+		dev_err(dev, "No platform data set\n");
+		return NULL;
+	}
+
+	pdata = kzalloc(sizeof(*pdata), GFP_KERNEL);
+	if (!pdata)
+		return NULL;
+
+	pdata->irq = irq_of_parse_and_map(np, 0);
+	if (!pdata->irq) {
+		dev_err(dev, "No irq in platform data\n");
+		kfree(pdata);
+		return NULL;
+	}
+
+	return pdata;
+}
+#else
+static struct wl12xx_platform_data *wlcore_probe_of(struct device *dev)
+{
+	return NULL;
+}
+#endif
+
+static struct wl12xx_platform_data *
+wlcore_get_platform_data(struct device *dev)
+{
+	struct wl12xx_platform_data *pdata;
+
+	pdata = wl12xx_get_platform_data();
+	if (!IS_ERR(pdata))
+		return kmemdup(pdata, sizeof(*pdata), GFP_KERNEL);
+
+	return wlcore_probe_of(dev);
+}
+
+static void wlcore_del_platform_data(struct wl12xx_platform_data *pdata)
+{
+	kfree(pdata);
+}
+
 static int wl1271_probe(struct sdio_func *func,
 				  const struct sdio_device_id *id)
 {
@@ -245,12 +295,9 @@  static int wl1271_probe(struct sdio_func *func,
 	/* Use block mode for transferring over one block size of data */
 	func->card->quirks |= MMC_QUIRK_BLKSZ_FOR_BYTE_MODE;
 
-	pdev_data.pdata = wl12xx_get_platform_data();
-	if (IS_ERR(pdev_data.pdata)) {
-		ret = PTR_ERR(pdev_data.pdata);
-		dev_err(glue->dev, "missing wlan platform data: %d\n", ret);
+	pdev_data.pdata = wlcore_get_platform_data(&func->dev);
+	if (!pdev_data.pdata)
 		goto out_free_glue;
-	}
 
 	/* if sdio can keep power while host is suspended, enable wow */
 	mmcflags = sdio_get_host_pm_caps(func);
@@ -279,7 +326,7 @@  static int wl1271_probe(struct sdio_func *func,
 	if (!glue->core) {
 		dev_err(glue->dev, "can't allocate platform_device");
 		ret = -ENOMEM;
-		goto out_free_glue;
+		goto out_free_pdata;
 	}
 
 	glue->core->dev.parent = &func->dev;
@@ -313,6 +360,9 @@  static int wl1271_probe(struct sdio_func *func,
 out_dev_put:
 	platform_device_put(glue->core);
 
+out_free_pdata:
+	wlcore_del_platform_data(pdev_data.pdata);
+
 out_free_glue:
 	kfree(glue);
 
@@ -323,11 +373,14 @@  out:
 static void wl1271_remove(struct sdio_func *func)
 {
 	struct wl12xx_sdio_glue *glue = sdio_get_drvdata(func);
+	struct wlcore_platdev_data *pdev_data = glue->core->dev.platform_data;
+	struct wl12xx_platform_data *pdata = pdev_data->pdata;
 
 	/* Undo decrement done above in wl1271_probe */
 	pm_runtime_get_noresume(&func->dev);
 
 	platform_device_unregister(glue->core);
+	wlcore_del_platform_data(pdata);
 	kfree(glue);
 }