| Message ID | 1425915402-10012-2-git-send-email-eliad@wizery.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
On Monday 09 March 2015 17:36:41 Eliad Peller wrote: > @@ -323,11 +388,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); > } > > The third patch looks ok, but now you should remove the wl12xx_platform_data from the wlcore code, since it's not used any more, it was broken to start with (as it supports only one instance) and we want to prevent others from adding new users of that. Since the only thing you need is the irq number, you can directly add the irq number to struct wlcore_platdev_data and remove the pdata pointer there. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello Eliad, On Mon, Mar 9, 2015 at 4:36 PM, Eliad Peller <eliad@wizery.com> 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 see this is a v5 but I don't know what was changed from prior revisions. It would be good if the patches had a versions history. > drivers/net/wireless/ti/wlcore/sdio.c | 80 ++++++++++++++++++++++++++++++++--- > 1 file changed, 74 insertions(+), 6 deletions(-) > > diff --git a/drivers/net/wireless/ti/wlcore/sdio.c b/drivers/net/wireless/ti/wlcore/sdio.c > index d3dd7bf..ee556ac 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,69 @@ static struct wl1271_if_operations sdio_ops = { > .set_block_size = wl1271_sdio_set_block_size, > }; > > +#ifdef CONFIG_OF > +static const struct of_device_id wlcore_sdio_of_match_table[] = { > + { .compatible = "ti,wl1801" }, > + { .compatible = "ti,wl1805" }, > + { .compatible = "ti,wl1807" }, > + { .compatible = "ti,wl1831" }, > + { .compatible = "ti,wl1835" }, > + { .compatible = "ti,wl1837" }, > + { } > +}; > + > +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_match_node(wlcore_sdio_of_match_table, np)) > + 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; > + > + /* first, look for DT data */ I thought it was the opposite, that platform data should over-rule DT. That way you can still use the data filled in arch/arm/mach-omap2/pdata-quirks.c even after the driver supports your new DT binding. > + pdata = wlcore_probe_of(dev); > + if (pdata) > + return pdata; > + > + /* if not found - fallback to static platform data */ > + pdata = wl12xx_get_platform_data(); > + if (!IS_ERR(pdata)) > + return kmemdup(pdata, sizeof(*pdata), GFP_KERNEL); > + > + dev_err(dev, "No platform data set\n"); > + return NULL; > +} > + > +static void wlcore_del_platform_data(struct wl12xx_platform_data *pdata) > +{ > + kfree(pdata); > +} > + This function seems to be an unnecessary, why not just call kfree() directly? Or better, maybe the resource-managed devm_*() functions can be used so the data doesn't have to be explicitly freed? Best regards, Javier -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday 11 March 2015 01:34:19 Javier Martinez Canillas wrote: > > + > > +static struct wl12xx_platform_data * > > +wlcore_get_platform_data(struct device *dev) > > +{ > > + struct wl12xx_platform_data *pdata; > > + > > + /* first, look for DT data */ > > I thought it was the opposite, that platform data should over-rule DT. > That way you can still use the data filled in > arch/arm/mach-omap2/pdata-quirks.c even after the driver supports your > new DT binding. No, the pdata-quirks stuff for this driver must die, it was a hack that only exists because we previously could not attach data to an sdio function. > > + pdata = wlcore_probe_of(dev); > > + if (pdata) > > + return pdata; > > + > > + /* if not found - fallback to static platform data */ > > + pdata = wl12xx_get_platform_data(); > > + if (!IS_ERR(pdata)) > > + return kmemdup(pdata, sizeof(*pdata), GFP_KERNEL); > > + > > + dev_err(dev, "No platform data set\n"); > > + return NULL; > > +} > > + > > +static void wlcore_del_platform_data(struct wl12xx_platform_data *pdata) > > +{ > > + kfree(pdata); > > +} > > + > > This function seems to be an unnecessary, why not just call kfree() directly? > > Or better, maybe the resource-managed devm_*() functions can be used > so the data doesn't have to be explicitly freed? As I said earlier, I think it would be best not to dynamically allocate anything here at all. As Eliad explained, the data is used by two different drivers: wl12xx and wl18xx, and only the latter is converted for now, but after the conversion, it should not need the platform data structure any more, only the irq number that gets passed in from DT. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello Arnd, On Wed, Mar 11, 2015 at 10:51 AM, Arnd Bergmann <arnd@arndb.de> wrote: > On Wednesday 11 March 2015 01:34:19 Javier Martinez Canillas wrote: >> > + >> > +static struct wl12xx_platform_data * >> > +wlcore_get_platform_data(struct device *dev) >> > +{ >> > + struct wl12xx_platform_data *pdata; >> > + >> > + /* first, look for DT data */ >> >> I thought it was the opposite, that platform data should over-rule DT. >> That way you can still use the data filled in >> arch/arm/mach-omap2/pdata-quirks.c even after the driver supports your >> new DT binding. > > No, the pdata-quirks stuff for this driver must die, it was a hack > that only exists because we previously could not attach data to an > sdio function. > Ok sorry, I misunderstood and thought that the output from the discussion in patch 3/3 was that the pdata could not still be removed due not having a way to configure the clocks for wl12xx. I totally agree with removing the pdata on this driver, in fact I would go an just remove all the remaining OMAP2+ board files and pdata-quirks completely. Most OMAP2+ boards have been converted to DT years ago and if someone really cares about mainline support for these boards, they can add a DTS and DT bindings for the drivers that still needed like this one. Best regards, Javier -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday 11 March 2015 11:05:47 Javier Martinez Canillas wrote: > > On Wed, Mar 11, 2015 at 10:51 AM, Arnd Bergmann <arnd@arndb.de> wrote: > > On Wednesday 11 March 2015 01:34:19 Javier Martinez Canillas wrote: > >> > + > >> > +static struct wl12xx_platform_data * > >> > +wlcore_get_platform_data(struct device *dev) > >> > +{ > >> > + struct wl12xx_platform_data *pdata; > >> > + > >> > + /* first, look for DT data */ > >> > >> I thought it was the opposite, that platform data should over-rule DT. > >> That way you can still use the data filled in > >> arch/arm/mach-omap2/pdata-quirks.c even after the driver supports your > >> new DT binding. > > > > No, the pdata-quirks stuff for this driver must die, it was a hack > > that only exists because we previously could not attach data to an > > sdio function. > > > > Ok sorry, I misunderstood and thought that the output from the > discussion in patch 3/3 was that the pdata could not still be removed > due not having a way to configure the clocks for wl12xx. I think that is still the case, but we should never have both pdata and DT, and if we ever did, I think the DT should take precedence so we can be sure that the pdata is not used and can be removed. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Mar 11, 2015 at 11:51 AM, Arnd Bergmann <arnd@arndb.de> wrote: >> > +static void wlcore_del_platform_data(struct wl12xx_platform_data *pdata) >> > +{ >> > + kfree(pdata); >> > +} >> > + >> >> This function seems to be an unnecessary, why not just call kfree() directly? >> >> Or better, maybe the resource-managed devm_*() functions can be used >> so the data doesn't have to be explicitly freed? > > As I said earlier, I think it would be best not to dynamically allocate anything > here at all. As Eliad explained, the data is used by two different drivers: > wl12xx and wl18xx, and only the latter is converted for now, but after the > conversion, it should not need the platform data structure any more, only > the irq number that gets passed in from DT. > sure, i'll try taking care of it (probably with additional patch after the conversion) Eliad. -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" 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/drivers/net/wireless/ti/wlcore/sdio.c b/drivers/net/wireless/ti/wlcore/sdio.c index d3dd7bf..ee556ac 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,69 @@ static struct wl1271_if_operations sdio_ops = { .set_block_size = wl1271_sdio_set_block_size, }; +#ifdef CONFIG_OF +static const struct of_device_id wlcore_sdio_of_match_table[] = { + { .compatible = "ti,wl1801" }, + { .compatible = "ti,wl1805" }, + { .compatible = "ti,wl1807" }, + { .compatible = "ti,wl1831" }, + { .compatible = "ti,wl1835" }, + { .compatible = "ti,wl1837" }, + { } +}; + +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_match_node(wlcore_sdio_of_match_table, np)) + 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; + + /* first, look for DT data */ + pdata = wlcore_probe_of(dev); + if (pdata) + return pdata; + + /* if not found - fallback to static platform data */ + pdata = wl12xx_get_platform_data(); + if (!IS_ERR(pdata)) + return kmemdup(pdata, sizeof(*pdata), GFP_KERNEL); + + dev_err(dev, "No platform data set\n"); + return NULL; +} + +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 +310,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 +341,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 +375,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 +388,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); }