| Message ID | 1423998550-8829-1-git-send-email-eliad@wizery.com |
|---|---|
| State | Needs Review / ACK, archived |
| Headers | show |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | warning | total: 1 errors, 0 warnings, 0 lines checked |
| robh/patch-applied | success |
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 devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Feb 16, 2015 at 12:06 PM, Arnd Bergmann <arnd@arndb.de> 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 devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Feb 17, 2015 at 03:39:03PM +0000, Eliad Peller wrote: > On Mon, Feb 16, 2015 at 12:06 PM, Arnd Bergmann <arnd@arndb.de> 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 devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[...] > + 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 devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Feb 17, 2015 at 5:45 PM, Mark Rutland <mark.rutland@arm.com> 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 <arnd@arndb.de> 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 devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 <ido@wizery.com> > Signed-off-by: Eliad Peller <eliad@wizery.com> > --- I don't really care much, but why not just finalize the work I did a couple of years ago? http://mid.gmane.org/1375189476-21557-1-git-send-email-coelho@ti.com http://mid.gmane.org/1375215668-29171-1-git-send-email-coelho@ti.com 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 devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
hi Luca, On Fri, Feb 27, 2015 at 9:31 AM, Luca Coelho <luca@coelho.fi> 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 <ido@wizery.com> >> Signed-off-by: Eliad Peller <eliad@wizery.com> >> --- > > I don't really care much, but why not just finalize the work I did a > couple of years ago? > > http://mid.gmane.org/1375189476-21557-1-git-send-email-coelho@ti.com > http://mid.gmane.org/1375215668-29171-1-git-send-email-coelho@ti.com > > 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 devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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); }