Re: [PATCH v2 1/2] net: stmmac: Add OXNAS Glue Driver
Hi Neil,
On 31 October 2016 at 11:54, Neil Armstrong <narmstr...@baylibre.com> wrote:
> Add Synopsys Designware MAC Glue layer for the Oxford Semiconductor OX820.
>
> Acked-by: Joachim Eastwood <manab...@gmail.com>
> Signed-off-by: Neil Armstrong <narmstr...@baylibre.com>
> ---
> +static int oxnas_dwmac_init(struct oxnas_dwmac *dwmac)
> +{
> + unsigned int value;
> + int ret;
> +
> + /* Reset HW here before changing the glue configuration */
> + ret = device_reset(dwmac->dev);
> + if (ret)
> + return ret;
> +
> + ret = clk_prepare_enable(dwmac->clk);
> + if (ret)
> + return ret;
> +
> + ret = regmap_read(dwmac->regmap, OXNAS_DWMAC_CTRL_REGOFFSET, );
> + if (ret < 0)
> + return ret;
If regmap reading fails here, the clock will be left on as probe fails.
> +
> + /* Enable GMII_GTXCLK to follow GMII_REFCLK, required for gigabit PHY
> */
> + value |= BIT(DWMAC_CKEN_GTX)|
> +/* Use simple mux for 25/125 Mhz clock switching */
> +BIT(DWMAC_SIMPLE_MUX) |
> +/* set auto switch tx clock source */
> +BIT(DWMAC_AUTO_TX_SOURCE) |
> +/* enable tx & rx vardelay */
> +BIT(DWMAC_CKEN_TX_OUT) |
> +BIT(DWMAC_CKEN_TXN_OUT)|
> +BIT(DWMAC_CKEN_TX_IN) |
> +BIT(DWMAC_CKEN_RX_OUT) |
> +BIT(DWMAC_CKEN_RXN_OUT)|
> +BIT(DWMAC_CKEN_RX_IN);
> + regmap_write(dwmac->regmap, OXNAS_DWMAC_CTRL_REGOFFSET, value);
> +
> + /* set tx & rx vardelay */
> + value = DWMAC_TX_VARDELAY(4)|
> + DWMAC_TXN_VARDELAY(2) |
> + DWMAC_RX_VARDELAY(10) |
> + DWMAC_RXN_VARDELAY(8);
> + regmap_write(dwmac->regmap, OXNAS_DWMAC_DELAY_REGOFFSET, value);
> +
> + return 0;
> +}
regards,
Joachim Eastwood
Re: [PATCH v2 1/2] net: stmmac: Add OXNAS Glue Driver
Hi Neil,
On 31 October 2016 at 11:54, Neil Armstrong wrote:
> Add Synopsys Designware MAC Glue layer for the Oxford Semiconductor OX820.
>
> Acked-by: Joachim Eastwood
> Signed-off-by: Neil Armstrong
> ---
> +static int oxnas_dwmac_init(struct oxnas_dwmac *dwmac)
> +{
> + unsigned int value;
> + int ret;
> +
> + /* Reset HW here before changing the glue configuration */
> + ret = device_reset(dwmac->dev);
> + if (ret)
> + return ret;
> +
> + ret = clk_prepare_enable(dwmac->clk);
> + if (ret)
> + return ret;
> +
> + ret = regmap_read(dwmac->regmap, OXNAS_DWMAC_CTRL_REGOFFSET, );
> + if (ret < 0)
> + return ret;
If regmap reading fails here, the clock will be left on as probe fails.
> +
> + /* Enable GMII_GTXCLK to follow GMII_REFCLK, required for gigabit PHY
> */
> + value |= BIT(DWMAC_CKEN_GTX)|
> +/* Use simple mux for 25/125 Mhz clock switching */
> +BIT(DWMAC_SIMPLE_MUX) |
> +/* set auto switch tx clock source */
> +BIT(DWMAC_AUTO_TX_SOURCE) |
> +/* enable tx & rx vardelay */
> +BIT(DWMAC_CKEN_TX_OUT) |
> +BIT(DWMAC_CKEN_TXN_OUT)|
> +BIT(DWMAC_CKEN_TX_IN) |
> +BIT(DWMAC_CKEN_RX_OUT) |
> +BIT(DWMAC_CKEN_RXN_OUT)|
> +BIT(DWMAC_CKEN_RX_IN);
> + regmap_write(dwmac->regmap, OXNAS_DWMAC_CTRL_REGOFFSET, value);
> +
> + /* set tx & rx vardelay */
> + value = DWMAC_TX_VARDELAY(4)|
> + DWMAC_TXN_VARDELAY(2) |
> + DWMAC_RX_VARDELAY(10) |
> + DWMAC_RXN_VARDELAY(8);
> + regmap_write(dwmac->regmap, OXNAS_DWMAC_DELAY_REGOFFSET, value);
> +
> + return 0;
> +}
regards,
Joachim Eastwood
Re: [PATCH] net: stmmac: Add OXNAS Glue Driver
Hi Neil, On 31 October 2016 at 10:55, Neil Armstrong <narmstr...@baylibre.com> wrote: > On 10/30/2016 09:41 PM, Rob Herring wrote: >> On Fri, Oct 21, 2016 at 10:44:45AM +0200, Neil Armstrong wrote: >>> Add Synopsys Designware MAC Glue layer for the Oxford Semiconductor OX820. >>> >>> Signed-off-by: Neil Armstrong <narmstr...@baylibre.com> >>> --- >>> .../devicetree/bindings/net/oxnas-dwmac.txt| 44 + >> >> It's preferred that bindings are a separate patch. > > OK > >> >>> drivers/net/ethernet/stmicro/stmmac/Kconfig| 11 ++ >>> drivers/net/ethernet/stmicro/stmmac/Makefile | 1 + >>> drivers/net/ethernet/stmicro/stmmac/dwmac-oxnas.c | 219 >>> + >>> 4 files changed, 275 insertions(+) >>> create mode 100644 Documentation/devicetree/bindings/net/oxnas-dwmac.txt >>> create mode 100644 drivers/net/ethernet/stmicro/stmmac/dwmac-oxnas.c >>> >>> Changes since RFC at https://patchwork.kernel.org/patch/9387257 : >>> - Drop init/exit callbacks >>> - Implement proper remove and PM callback >>> - Call init from probe >>> - Disable/Unprepare clock if stmmac probe fails >>> >>> diff --git a/Documentation/devicetree/bindings/net/oxnas-dwmac.txt >>> b/Documentation/devicetree/bindings/net/oxnas-dwmac.txt >>> new file mode 100644 >>> index 000..5d2696c >>> --- /dev/null >>> +++ b/Documentation/devicetree/bindings/net/oxnas-dwmac.txt >>> @@ -0,0 +1,44 @@ >>> +* Oxford Semiconductor OXNAS DWMAC Ethernet controller >>> + >>> +The device inherits all the properties of the dwmac/stmmac devices >>> +described in the file stmmac.txt in the current directory with the >>> +following changes. >>> + >>> +Required properties on all platforms: >>> + >>> +- compatible: Depending on the platform this should be one of: >>> +- "oxsemi,ox820-dwmac" >>> +Additionally "snps,dwmac" and any applicable more >>> +detailed version number described in net/stmmac.txt >>> +should be used. >> >> You should be explicit what version applies to ox820. "snps,dwmac" >> should probably be deprecated IMO. There are so many variations of DW >> h/w. > > Well, to be honest I have absolutely no idea ! But I will try to find out... You can see in the boot log: >From lpc18xx boot: [3.242253] stmmac - user ID: 0x11, Synopsys ID: 0x36 [3.247653] Ring mode enabled [3.251491] DMA HW capability register supported [3.256336] Enhanced/Alternate descriptors [3.261537] Enabled extended descriptors [3.265968] RX Checksum Offload Engine supported (type 2) [3.272249] TX Checksum insertion supported [3.276874] Wake-Up On Lan supported [3.283743] Enable RX Mitigation via HW Watchdog Timer [3.326701] libphy: stmmac: probed Synopsys ID: 0x36 and user UD: 0x11, gives us DWMAC version 3.611 regards, Joachim Eastwood
Re: [PATCH] net: stmmac: Add OXNAS Glue Driver
Hi Neil, On 31 October 2016 at 10:55, Neil Armstrong wrote: > On 10/30/2016 09:41 PM, Rob Herring wrote: >> On Fri, Oct 21, 2016 at 10:44:45AM +0200, Neil Armstrong wrote: >>> Add Synopsys Designware MAC Glue layer for the Oxford Semiconductor OX820. >>> >>> Signed-off-by: Neil Armstrong >>> --- >>> .../devicetree/bindings/net/oxnas-dwmac.txt| 44 + >> >> It's preferred that bindings are a separate patch. > > OK > >> >>> drivers/net/ethernet/stmicro/stmmac/Kconfig| 11 ++ >>> drivers/net/ethernet/stmicro/stmmac/Makefile | 1 + >>> drivers/net/ethernet/stmicro/stmmac/dwmac-oxnas.c | 219 >>> + >>> 4 files changed, 275 insertions(+) >>> create mode 100644 Documentation/devicetree/bindings/net/oxnas-dwmac.txt >>> create mode 100644 drivers/net/ethernet/stmicro/stmmac/dwmac-oxnas.c >>> >>> Changes since RFC at https://patchwork.kernel.org/patch/9387257 : >>> - Drop init/exit callbacks >>> - Implement proper remove and PM callback >>> - Call init from probe >>> - Disable/Unprepare clock if stmmac probe fails >>> >>> diff --git a/Documentation/devicetree/bindings/net/oxnas-dwmac.txt >>> b/Documentation/devicetree/bindings/net/oxnas-dwmac.txt >>> new file mode 100644 >>> index 000..5d2696c >>> --- /dev/null >>> +++ b/Documentation/devicetree/bindings/net/oxnas-dwmac.txt >>> @@ -0,0 +1,44 @@ >>> +* Oxford Semiconductor OXNAS DWMAC Ethernet controller >>> + >>> +The device inherits all the properties of the dwmac/stmmac devices >>> +described in the file stmmac.txt in the current directory with the >>> +following changes. >>> + >>> +Required properties on all platforms: >>> + >>> +- compatible: Depending on the platform this should be one of: >>> +- "oxsemi,ox820-dwmac" >>> +Additionally "snps,dwmac" and any applicable more >>> +detailed version number described in net/stmmac.txt >>> +should be used. >> >> You should be explicit what version applies to ox820. "snps,dwmac" >> should probably be deprecated IMO. There are so many variations of DW >> h/w. > > Well, to be honest I have absolutely no idea ! But I will try to find out... You can see in the boot log: >From lpc18xx boot: [3.242253] stmmac - user ID: 0x11, Synopsys ID: 0x36 [3.247653] Ring mode enabled [3.251491] DMA HW capability register supported [3.256336] Enhanced/Alternate descriptors [3.261537] Enabled extended descriptors [3.265968] RX Checksum Offload Engine supported (type 2) [3.272249] TX Checksum insertion supported [3.276874] Wake-Up On Lan supported [3.283743] Enable RX Mitigation via HW Watchdog Timer [3.326701] libphy: stmmac: probed Synopsys ID: 0x36 and user UD: 0x11, gives us DWMAC version 3.611 regards, Joachim Eastwood
Re: [PATCH v2] rtc: add support for rtc NXP pca21125
Hi Venkat, On 19 October 2016 at 19:41, VENKAT PRASHANTH B U <venkat.prashanth2...@gmail.com> wrote: > This is a patch to add support for > NXP rtc pca21125 The register layout PCA21125 looks similar to PCF8563 which already has a driver. Would it be possible to support the PCA21125 device with the rtc-pcf8563 driver? You will need to add a regmap layer since the PCA21125 is SPI while the PCF8563 is I2C. regards, Joachim Eastwood
Re: [PATCH v2] rtc: add support for rtc NXP pca21125
Hi Venkat, On 19 October 2016 at 19:41, VENKAT PRASHANTH B U wrote: > This is a patch to add support for > NXP rtc pca21125 The register layout PCA21125 looks similar to PCF8563 which already has a driver. Would it be possible to support the PCA21125 device with the rtc-pcf8563 driver? You will need to add a regmap layer since the PCA21125 is SPI while the PCF8563 is I2C. regards, Joachim Eastwood
Re: [PATCH] rtc: add support for rtc NXP pca8565
Hi Venkat, On 30 October 2016 at 09:03, VENKAT PRASHANTH B U <venkat.prashanth2...@gmail.com> wrote: > This is a patch to add support for > NXP rtc pca8565 >From a quick look the PCA8565 register layout looks very similar to PCF8563 which already has a driver (rtc-pcf8563). Can you use that driver instead of creating a new one? If so; send a patch for rtc-pcf8563 that adds "pca8565" to the set of i2c ids. regards, Joachim Eastwood
Re: [PATCH] rtc: add support for rtc NXP pca8565
Hi Venkat, On 30 October 2016 at 09:03, VENKAT PRASHANTH B U wrote: > This is a patch to add support for > NXP rtc pca8565 >From a quick look the PCA8565 register layout looks very similar to PCF8563 which already has a driver (rtc-pcf8563). Can you use that driver instead of creating a new one? If so; send a patch for rtc-pcf8563 that adds "pca8565" to the set of i2c ids. regards, Joachim Eastwood
Re: [PATCH] net: stmmac: Add OXNAS Glue Driver
AC_RXN_VARDELAY(8);
> + regmap_write(dwmac->regmap, OXNAS_DWMAC_DELAY_REGOFFSET, value);
> +
> + return 0;
> +}
> +
> +static int oxnas_dwmac_probe(struct platform_device *pdev)
> +{
> + struct plat_stmmacenet_data *plat_dat;
> + struct stmmac_resources stmmac_res;
> + struct device_node *sysctrl;
> + struct oxnas_dwmac *dwmac;
> + int ret;
> +
> + sysctrl = of_parse_phandle(pdev->dev.of_node, "oxsemi,sys-ctrl", 0);
> + if (!sysctrl) {
> + dev_err(>dev, "failed to get sys-ctrl node\n");
> + return -EINVAL;
> + }
> +
> + ret = stmmac_get_platform_resources(pdev, _res);
> + if (ret)
> + return ret;
> +
> + plat_dat = stmmac_probe_config_dt(pdev, _res.mac);
> + if (IS_ERR(plat_dat))
> + return PTR_ERR(plat_dat);
> +
> + dwmac = devm_kzalloc(>dev, sizeof(*dwmac), GFP_KERNEL);
> + if (!dwmac)
> + return -ENOMEM;
> +
> + dwmac->dev = >dev;
> + plat_dat->bsp_priv = dwmac;
> +
> + dwmac->regmap = syscon_node_to_regmap(sysctrl);
> + if (IS_ERR(dwmac->regmap)) {
> + dev_err(>dev, "failed to have sysctrl regmap\n");
> + return PTR_ERR(dwmac->regmap);
> + }
> +
> + dwmac->clk = devm_clk_get(>dev, "gmac");
> + if (IS_ERR(dwmac->clk))
> + return PTR_ERR(dwmac->clk);
> +
> + ret = oxnas_dwmac_init(dwmac);
> + if (ret)
> + return ret;
> +
> + ret = stmmac_dvr_probe(>dev, plat_dat, _res);
> + if (ret)
> + clk_disable_unprepare(dwmac->clk);
> +
> + return ret;
> +}
> +
> +static int oxnas_dwmac_remove(struct platform_device *pdev)
> +{
> + struct net_device *ndev = platform_get_drvdata(pdev);
> + struct stmmac_priv *priv = netdev_priv(ndev);
> + struct oxnas_dwmac *dwmac = priv->plat->bsp_priv;
Instead of this long dance of variables use the get_stmmac_bsp_priv()-helper.
You can take a look at dwmac-meson8b.c for reference.
> + int ret = stmmac_dvr_remove(>dev);
> +
> + clk_disable_unprepare(dwmac->clk);
> +
> + return ret;
> +}
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int oxnas_dwmac_suspend(struct device *dev)
> +{
> + struct net_device *ndev = dev_get_drvdata(dev);
> + struct stmmac_priv *priv = netdev_priv(ndev);
> + struct oxnas_dwmac *dwmac = priv->plat->bsp_priv;
get_stmmac_bsp_priv()
> + int ret;
> +
> + ret = stmmac_suspend(dev);
> + clk_disable_unprepare(dwmac->clk);
> +
> + return ret;
> +}
> +
> +static int oxnas_dwmac_resume(struct device *dev)
> +{
> + struct net_device *ndev = dev_get_drvdata(dev);
> + struct stmmac_priv *priv = netdev_priv(ndev);
> + struct oxnas_dwmac *dwmac = priv->plat->bsp_priv;
get_stmmac_bsp_priv()
> + int ret;
> +
> + ret = oxnas_dwmac_init(dwmac);
> + if (ret)
> + return ret;
> +
> + ret = stmmac_resume(dev);
> +
> + return ret;
> +}
> +#endif /* CONFIG_PM_SLEEP */
With these changes:
Acked-by: Joachim Eastwood <manab...@gmail.com>
best regards,
Joachim Eastwood
Re: [PATCH] net: stmmac: Add OXNAS Glue Driver
ac_probe(struct platform_device *pdev)
> +{
> + struct plat_stmmacenet_data *plat_dat;
> + struct stmmac_resources stmmac_res;
> + struct device_node *sysctrl;
> + struct oxnas_dwmac *dwmac;
> + int ret;
> +
> + sysctrl = of_parse_phandle(pdev->dev.of_node, "oxsemi,sys-ctrl", 0);
> + if (!sysctrl) {
> + dev_err(>dev, "failed to get sys-ctrl node\n");
> + return -EINVAL;
> + }
> +
> + ret = stmmac_get_platform_resources(pdev, _res);
> + if (ret)
> + return ret;
> +
> + plat_dat = stmmac_probe_config_dt(pdev, _res.mac);
> + if (IS_ERR(plat_dat))
> + return PTR_ERR(plat_dat);
> +
> + dwmac = devm_kzalloc(>dev, sizeof(*dwmac), GFP_KERNEL);
> + if (!dwmac)
> + return -ENOMEM;
> +
> + dwmac->dev = >dev;
> + plat_dat->bsp_priv = dwmac;
> +
> + dwmac->regmap = syscon_node_to_regmap(sysctrl);
> + if (IS_ERR(dwmac->regmap)) {
> + dev_err(>dev, "failed to have sysctrl regmap\n");
> + return PTR_ERR(dwmac->regmap);
> + }
> +
> + dwmac->clk = devm_clk_get(>dev, "gmac");
> + if (IS_ERR(dwmac->clk))
> + return PTR_ERR(dwmac->clk);
> +
> + ret = oxnas_dwmac_init(dwmac);
> + if (ret)
> + return ret;
> +
> + ret = stmmac_dvr_probe(>dev, plat_dat, _res);
> + if (ret)
> + clk_disable_unprepare(dwmac->clk);
> +
> + return ret;
> +}
> +
> +static int oxnas_dwmac_remove(struct platform_device *pdev)
> +{
> + struct net_device *ndev = platform_get_drvdata(pdev);
> + struct stmmac_priv *priv = netdev_priv(ndev);
> + struct oxnas_dwmac *dwmac = priv->plat->bsp_priv;
Instead of this long dance of variables use the get_stmmac_bsp_priv()-helper.
You can take a look at dwmac-meson8b.c for reference.
> + int ret = stmmac_dvr_remove(>dev);
> +
> + clk_disable_unprepare(dwmac->clk);
> +
> + return ret;
> +}
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int oxnas_dwmac_suspend(struct device *dev)
> +{
> + struct net_device *ndev = dev_get_drvdata(dev);
> + struct stmmac_priv *priv = netdev_priv(ndev);
> + struct oxnas_dwmac *dwmac = priv->plat->bsp_priv;
get_stmmac_bsp_priv()
> + int ret;
> +
> + ret = stmmac_suspend(dev);
> + clk_disable_unprepare(dwmac->clk);
> +
> + return ret;
> +}
> +
> +static int oxnas_dwmac_resume(struct device *dev)
> +{
> + struct net_device *ndev = dev_get_drvdata(dev);
> + struct stmmac_priv *priv = netdev_priv(ndev);
> + struct oxnas_dwmac *dwmac = priv->plat->bsp_priv;
get_stmmac_bsp_priv()
> + int ret;
> +
> + ret = oxnas_dwmac_init(dwmac);
> + if (ret)
> + return ret;
> +
> + ret = stmmac_resume(dev);
> +
> + return ret;
> +}
> +#endif /* CONFIG_PM_SLEEP */
With these changes:
Acked-by: Joachim Eastwood
best regards,
Joachim Eastwood
Re: [RFC PATCH] net: stmmac: Add OXNAS Glue Driver
Hi Neil,
On 20 October 2016 at 17:54, Neil Armstrong <narmstr...@baylibre.com> wrote:
> Add Synopsys Designware MAC Glue layer for the Oxford Semiconductor OX820.
>
> Signed-off-by: Neil Armstrong <narmstr...@baylibre.com>
> ---
> .../devicetree/bindings/net/oxnas-dwmac.txt| 44 ++
> drivers/net/ethernet/stmicro/stmmac/Kconfig| 11 ++
> drivers/net/ethernet/stmicro/stmmac/Makefile | 1 +
> drivers/net/ethernet/stmicro/stmmac/dwmac-oxnas.c | 173
> +
> 4 files changed, 229 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/net/oxnas-dwmac.txt
> create mode 100644 drivers/net/ethernet/stmicro/stmmac/dwmac-oxnas.c
> +
> +static int oxnas_dwmac_probe(struct platform_device *pdev)
> +{
> + struct plat_stmmacenet_data *plat_dat;
> + struct stmmac_resources stmmac_res;
> + struct device_node *sysctrl;
> + struct oxnas_dwmac *dwmac;
> + int ret;
> +
> + sysctrl = of_parse_phandle(pdev->dev.of_node, "oxsemi,sys-ctrl", 0);
> + if (!sysctrl) {
> + dev_err(>dev, "failed to get sys-ctrl node\n");
> + return -EINVAL;
> + }
> +
> + ret = stmmac_get_platform_resources(pdev, _res);
> + if (ret)
> + return ret;
> +
> + plat_dat = stmmac_probe_config_dt(pdev, _res.mac);
> + if (IS_ERR(plat_dat))
> + return PTR_ERR(plat_dat);
> +
> + dwmac = devm_kzalloc(>dev, sizeof(*dwmac), GFP_KERNEL);
> + if (!dwmac)
> + return -ENOMEM;
> +
> + dwmac->regmap = syscon_node_to_regmap(sysctrl);
> + if (IS_ERR(dwmac->regmap)) {
> + dev_err(>dev, "failed to have sysctrl regmap\n");
> + return PTR_ERR(dwmac->regmap);
> + }
> +
> + dwmac->clk = devm_clk_get(>dev, "gmac");
> + if (IS_ERR(dwmac->clk))
> + return PTR_ERR(dwmac->clk);
> +
> + plat_dat->bsp_priv = dwmac;
> + plat_dat->init = oxnas_dwmac_init;
> + plat_dat->exit = oxnas_dwmac_exit;
Please do not use the init/exit callbacks. Implement proper driver
callbacks instead. I.e: PM resume/suspend and driver remove.
Shouldn't you call oxnas_dwmac_init() from probe as well?
As it is now it will only be called during PM resume and that can't be right.
> +
> + return stmmac_dvr_probe(>dev, plat_dat, _res);
If stmmac_dvr_probe() fails you should disable your clocks.
regards,
Joachim Eastwood
Re: [RFC PATCH] net: stmmac: Add OXNAS Glue Driver
Hi Neil,
On 20 October 2016 at 17:54, Neil Armstrong wrote:
> Add Synopsys Designware MAC Glue layer for the Oxford Semiconductor OX820.
>
> Signed-off-by: Neil Armstrong
> ---
> .../devicetree/bindings/net/oxnas-dwmac.txt| 44 ++
> drivers/net/ethernet/stmicro/stmmac/Kconfig| 11 ++
> drivers/net/ethernet/stmicro/stmmac/Makefile | 1 +
> drivers/net/ethernet/stmicro/stmmac/dwmac-oxnas.c | 173
> +
> 4 files changed, 229 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/net/oxnas-dwmac.txt
> create mode 100644 drivers/net/ethernet/stmicro/stmmac/dwmac-oxnas.c
> +
> +static int oxnas_dwmac_probe(struct platform_device *pdev)
> +{
> + struct plat_stmmacenet_data *plat_dat;
> + struct stmmac_resources stmmac_res;
> + struct device_node *sysctrl;
> + struct oxnas_dwmac *dwmac;
> + int ret;
> +
> + sysctrl = of_parse_phandle(pdev->dev.of_node, "oxsemi,sys-ctrl", 0);
> + if (!sysctrl) {
> + dev_err(>dev, "failed to get sys-ctrl node\n");
> + return -EINVAL;
> + }
> +
> + ret = stmmac_get_platform_resources(pdev, _res);
> + if (ret)
> + return ret;
> +
> + plat_dat = stmmac_probe_config_dt(pdev, _res.mac);
> + if (IS_ERR(plat_dat))
> + return PTR_ERR(plat_dat);
> +
> + dwmac = devm_kzalloc(>dev, sizeof(*dwmac), GFP_KERNEL);
> + if (!dwmac)
> + return -ENOMEM;
> +
> + dwmac->regmap = syscon_node_to_regmap(sysctrl);
> + if (IS_ERR(dwmac->regmap)) {
> + dev_err(>dev, "failed to have sysctrl regmap\n");
> + return PTR_ERR(dwmac->regmap);
> + }
> +
> + dwmac->clk = devm_clk_get(>dev, "gmac");
> + if (IS_ERR(dwmac->clk))
> + return PTR_ERR(dwmac->clk);
> +
> + plat_dat->bsp_priv = dwmac;
> + plat_dat->init = oxnas_dwmac_init;
> + plat_dat->exit = oxnas_dwmac_exit;
Please do not use the init/exit callbacks. Implement proper driver
callbacks instead. I.e: PM resume/suspend and driver remove.
Shouldn't you call oxnas_dwmac_init() from probe as well?
As it is now it will only be called during PM resume and that can't be right.
> +
> + return stmmac_dvr_probe(>dev, plat_dat, _res);
If stmmac_dvr_probe() fails you should disable your clocks.
regards,
Joachim Eastwood
Re: [PATCH] clk: nxp: clk-lpc18xx-ccu: Unmap region obtained by of_iomap
Hi Arvind,
On 20 September 2016 at 12:39, Arvind Yadav <arvind.yadav...@gmail.com> wrote:
> From: Arvind Yadav <arvind.yadav...@gmail.com>
>
> Free memory mapping, if lpc18xx_ccu_init is not successful.
>
> Signed-off-by: Arvind Yadav <arvind.yadav...@gmail.com>
Acked-by: Joachim Eastwood <manab...@gmail.com>
One comment below:
> ---
> drivers/clk/nxp/clk-lpc18xx-ccu.c |5 -
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/clk/nxp/clk-lpc18xx-ccu.c
> b/drivers/clk/nxp/clk-lpc18xx-ccu.c
> index f7136b9..27781b4 100644
> --- a/drivers/clk/nxp/clk-lpc18xx-ccu.c
> +++ b/drivers/clk/nxp/clk-lpc18xx-ccu.c
> @@ -277,12 +277,15 @@ static void __init lpc18xx_ccu_init(struct device_node
> *np)
> }
>
> clk_data = kzalloc(sizeof(*clk_data), GFP_KERNEL);
> - if (!clk_data)
> + if (!clk_data) {
> + iounmap(reg_base);
> return;
> + }
You could also move the kzalloc() at the beginning of the function to
save a few lines. I am fine either way.
>
> clk_data->num = of_property_count_strings(np, "clock-names");
> clk_data->name = kcalloc(clk_data->num, sizeof(char *), GFP_KERNEL);
> if (!clk_data->name) {
> + iounmap(reg_base);
> kfree(clk_data);
> return;
> }
regards,
Joachim Eastwood
Re: [PATCH] clk: nxp: clk-lpc18xx-ccu: Unmap region obtained by of_iomap
Hi Arvind,
On 20 September 2016 at 12:39, Arvind Yadav wrote:
> From: Arvind Yadav
>
> Free memory mapping, if lpc18xx_ccu_init is not successful.
>
> Signed-off-by: Arvind Yadav
Acked-by: Joachim Eastwood
One comment below:
> ---
> drivers/clk/nxp/clk-lpc18xx-ccu.c |5 -
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/clk/nxp/clk-lpc18xx-ccu.c
> b/drivers/clk/nxp/clk-lpc18xx-ccu.c
> index f7136b9..27781b4 100644
> --- a/drivers/clk/nxp/clk-lpc18xx-ccu.c
> +++ b/drivers/clk/nxp/clk-lpc18xx-ccu.c
> @@ -277,12 +277,15 @@ static void __init lpc18xx_ccu_init(struct device_node
> *np)
> }
>
> clk_data = kzalloc(sizeof(*clk_data), GFP_KERNEL);
> - if (!clk_data)
> + if (!clk_data) {
> + iounmap(reg_base);
> return;
> + }
You could also move the kzalloc() at the beginning of the function to
save a few lines. I am fine either way.
>
> clk_data->num = of_property_count_strings(np, "clock-names");
> clk_data->name = kcalloc(clk_data->num, sizeof(char *), GFP_KERNEL);
> if (!clk_data->name) {
> + iounmap(reg_base);
> kfree(clk_data);
> return;
> }
regards,
Joachim Eastwood
Re: [PATCH 1/6] gpio: constify gpio_chip structures
Hi Julia,
On 11 September 2016 at 14:14, Julia Lawall <julia.law...@lip6.fr> wrote:
> These structures are only used to copy into other structures, so declare
> them as const.
>
> The semantic patch that makes this change is as follows:
> (http://coccinelle.lip6.fr/)
>
> //
> @r disable optional_qualifier@
> identifier i;
> position p;
> @@
> static struct gpio_chip i@p = { ... };
>
> @ok@
> identifier r.i;
> expression e;
> position p;
> @@
> e = i@p;
>
> @bad@
> position p != {r.p,ok.p};
> identifier r.i;
> struct gpio_chip e;
> @@
> e@i@p
>
> @depends on !bad disable optional_qualifier@
> identifier r.i;
> @@
> static
> +const
> struct gpio_chip i = { ... };
> //
>
> Signed-off-by: Julia Lawall <julia.law...@lip6.fr>
>
> ---
> drivers/gpio/gpio-arizona.c |2 +-
> drivers/gpio/gpio-bcm-kona.c |2 +-
> drivers/gpio/gpio-da9052.c |2 +-
> drivers/gpio/gpio-da9055.c |2 +-
> drivers/gpio/gpio-it87.c |2 +-
> drivers/gpio/gpio-lp873x.c |2 +-
> drivers/gpio/gpio-lpc18xx.c |2 +-
> drivers/gpio/gpio-pisosr.c |2 +-
> drivers/gpio/gpio-sch.c |2 +-
> drivers/gpio/gpio-stmpe.c|2 +-
> drivers/gpio/gpio-tc3589x.c |2 +-
> drivers/gpio/gpio-tpic2810.c |2 +-
> drivers/gpio/gpio-tps65086.c |2 +-
> drivers/gpio/gpio-tps65218.c |2 +-
> drivers/gpio/gpio-tps65912.c |2 +-
> drivers/gpio/gpio-ts4900.c |2 +-
> drivers/gpio/gpio-twl4030.c |2 +-
> drivers/gpio/gpio-wm831x.c |2 +-
> drivers/gpio/gpio-wm8350.c |2 +-
> drivers/gpio/gpio-wm8994.c |2 +-
> 20 files changed, 20 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/gpio/gpio-lpc18xx.c b/drivers/gpio/gpio-lpc18xx.c
> index 98832c9..f12e02e 100644
> --- a/drivers/gpio/gpio-lpc18xx.c
> +++ b/drivers/gpio/gpio-lpc18xx.c
> @@ -78,7 +78,7 @@ static int lpc18xx_gpio_direction_output(struct gpio_chip
> *chip,
> return lpc18xx_gpio_direction(chip, offset, true);
> }
>
> -static struct gpio_chip lpc18xx_chip = {
> +static const struct gpio_chip lpc18xx_chip = {
> .label = "lpc18xx/43xx-gpio",
> .request= gpiochip_generic_request,
> .free = gpiochip_generic_free,
For lpc18xx:
Acked-by: Joachim Eastwood <manab...@gmail.com>
regards,
Joachim Eastwood
Re: [PATCH 1/6] gpio: constify gpio_chip structures
Hi Julia,
On 11 September 2016 at 14:14, Julia Lawall wrote:
> These structures are only used to copy into other structures, so declare
> them as const.
>
> The semantic patch that makes this change is as follows:
> (http://coccinelle.lip6.fr/)
>
> //
> @r disable optional_qualifier@
> identifier i;
> position p;
> @@
> static struct gpio_chip i@p = { ... };
>
> @ok@
> identifier r.i;
> expression e;
> position p;
> @@
> e = i@p;
>
> @bad@
> position p != {r.p,ok.p};
> identifier r.i;
> struct gpio_chip e;
> @@
> e@i@p
>
> @depends on !bad disable optional_qualifier@
> identifier r.i;
> @@
> static
> +const
> struct gpio_chip i = { ... };
> //
>
> Signed-off-by: Julia Lawall
>
> ---
> drivers/gpio/gpio-arizona.c |2 +-
> drivers/gpio/gpio-bcm-kona.c |2 +-
> drivers/gpio/gpio-da9052.c |2 +-
> drivers/gpio/gpio-da9055.c |2 +-
> drivers/gpio/gpio-it87.c |2 +-
> drivers/gpio/gpio-lp873x.c |2 +-
> drivers/gpio/gpio-lpc18xx.c |2 +-
> drivers/gpio/gpio-pisosr.c |2 +-
> drivers/gpio/gpio-sch.c |2 +-
> drivers/gpio/gpio-stmpe.c|2 +-
> drivers/gpio/gpio-tc3589x.c |2 +-
> drivers/gpio/gpio-tpic2810.c |2 +-
> drivers/gpio/gpio-tps65086.c |2 +-
> drivers/gpio/gpio-tps65218.c |2 +-
> drivers/gpio/gpio-tps65912.c |2 +-
> drivers/gpio/gpio-ts4900.c |2 +-
> drivers/gpio/gpio-twl4030.c |2 +-
> drivers/gpio/gpio-wm831x.c |2 +-
> drivers/gpio/gpio-wm8350.c |2 +-
> drivers/gpio/gpio-wm8994.c |2 +-
> 20 files changed, 20 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/gpio/gpio-lpc18xx.c b/drivers/gpio/gpio-lpc18xx.c
> index 98832c9..f12e02e 100644
> --- a/drivers/gpio/gpio-lpc18xx.c
> +++ b/drivers/gpio/gpio-lpc18xx.c
> @@ -78,7 +78,7 @@ static int lpc18xx_gpio_direction_output(struct gpio_chip
> *chip,
> return lpc18xx_gpio_direction(chip, offset, true);
> }
>
> -static struct gpio_chip lpc18xx_chip = {
> +static const struct gpio_chip lpc18xx_chip = {
> .label = "lpc18xx/43xx-gpio",
> .request= gpiochip_generic_request,
> .free = gpiochip_generic_free,
For lpc18xx:
Acked-by: Joachim Eastwood
regards,
Joachim Eastwood
Re: [PATCH 03/10] reset: lpc18xx: add driver Kconfig option
Hi Philipp, On 24 August 2016 at 15:28, Philipp Zabel <p.za...@pengutronix.de> wrote: > Visible only if COMPILE_TEST is enabled, this allows to include the > driver in build tests. > > Cc: Joachim Eastwood <manab...@gmail.com> > Signed-off-by: Philipp Zabel <p.za...@pengutronix.de> > --- > drivers/reset/Kconfig | 7 +++ > drivers/reset/Makefile | 2 +- > 2 files changed, 8 insertions(+), 1 deletion(-) > > diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig > index 1194cbe..8e33de2 100644 > --- a/drivers/reset/Kconfig > +++ b/drivers/reset/Kconfig > @@ -27,6 +27,13 @@ config RESET_BERLIN > help > This enables the reset controller driver for Marvell Berlin SoCs. > > +config RESET_LPC18XX > + bool "LPC18xx/43xx Reset Driver" if COMPILE_TEST > + default ARCH_LPC18XX > + help > + This enables the LPC18xx/43 reset driver that supports the reset > + controllers on AR71xx SoCs. Don't know where you got the "AR71xx SoCs" from. This reset controller is found on NXP LPC18xx/43xx SoCs. Other than that it looks fine to me. Acked-by: Joachim Eastwood <manab...@gmail.com> > + > config RESET_OXNAS > bool > > diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile > index 34c0b23..25aa05a 100644 > --- a/drivers/reset/Makefile > +++ b/drivers/reset/Makefile > @@ -1,5 +1,4 @@ > obj-y += core.o > -obj-$(CONFIG_ARCH_LPC18XX) += reset-lpc18xx.o > obj-$(CONFIG_ARCH_SOCFPGA) += reset-socfpga.o > obj-$(CONFIG_MACH_PISTACHIO) += reset-pistachio.o > obj-$(CONFIG_ARCH_MESON) += reset-meson.o > @@ -10,6 +9,7 @@ obj-$(CONFIG_ARCH_HISI) += hisilicon/ > obj-$(CONFIG_ARCH_ZYNQ) += reset-zynq.o > obj-$(CONFIG_RESET_ATH79) += reset-ath79.o > obj-$(CONFIG_RESET_BERLIN) += reset-berlin.o > +obj-$(CONFIG_RESET_LPC18XX) += reset-lpc18xx.o > obj-$(CONFIG_RESET_OXNAS) += reset-oxnas.o > obj-$(CONFIG_TI_SYSCON_RESET) += reset-ti-syscon.o > obj-$(CONFIG_RESET_UNIPHIER) += reset-uniphier.o > -- > 2.8.1 regards, Joachim Eastwood
Re: [PATCH 03/10] reset: lpc18xx: add driver Kconfig option
Hi Philipp, On 24 August 2016 at 15:28, Philipp Zabel wrote: > Visible only if COMPILE_TEST is enabled, this allows to include the > driver in build tests. > > Cc: Joachim Eastwood > Signed-off-by: Philipp Zabel > --- > drivers/reset/Kconfig | 7 +++ > drivers/reset/Makefile | 2 +- > 2 files changed, 8 insertions(+), 1 deletion(-) > > diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig > index 1194cbe..8e33de2 100644 > --- a/drivers/reset/Kconfig > +++ b/drivers/reset/Kconfig > @@ -27,6 +27,13 @@ config RESET_BERLIN > help > This enables the reset controller driver for Marvell Berlin SoCs. > > +config RESET_LPC18XX > + bool "LPC18xx/43xx Reset Driver" if COMPILE_TEST > + default ARCH_LPC18XX > + help > + This enables the LPC18xx/43 reset driver that supports the reset > + controllers on AR71xx SoCs. Don't know where you got the "AR71xx SoCs" from. This reset controller is found on NXP LPC18xx/43xx SoCs. Other than that it looks fine to me. Acked-by: Joachim Eastwood > + > config RESET_OXNAS > bool > > diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile > index 34c0b23..25aa05a 100644 > --- a/drivers/reset/Makefile > +++ b/drivers/reset/Makefile > @@ -1,5 +1,4 @@ > obj-y += core.o > -obj-$(CONFIG_ARCH_LPC18XX) += reset-lpc18xx.o > obj-$(CONFIG_ARCH_SOCFPGA) += reset-socfpga.o > obj-$(CONFIG_MACH_PISTACHIO) += reset-pistachio.o > obj-$(CONFIG_ARCH_MESON) += reset-meson.o > @@ -10,6 +9,7 @@ obj-$(CONFIG_ARCH_HISI) += hisilicon/ > obj-$(CONFIG_ARCH_ZYNQ) += reset-zynq.o > obj-$(CONFIG_RESET_ATH79) += reset-ath79.o > obj-$(CONFIG_RESET_BERLIN) += reset-berlin.o > +obj-$(CONFIG_RESET_LPC18XX) += reset-lpc18xx.o > obj-$(CONFIG_RESET_OXNAS) += reset-oxnas.o > obj-$(CONFIG_TI_SYSCON_RESET) += reset-ti-syscon.o > obj-$(CONFIG_RESET_UNIPHIER) += reset-uniphier.o > -- > 2.8.1 regards, Joachim Eastwood
Re: [PATCH] i2c: don't print error when adding adapter fails
On 9 August 2016 at 13:36, Wolfram Sang <wsa-...@sang-engineering.com> wrote: > Since v4.8-rc1, the I2C core will print detailed information when adding an > I2C > adapter fails. So, drivers can skip this now. > > I am still undecided if I apply this as a single patch or break it out. But > for > reviewing, avoiding the patch bomb is probably a good thing. > > Should go via subsystem tree, I'd think. > > Wolfram Sang (1): > i2c: don't print error when adding adapter fails For > drivers/i2c/busses/i2c-lpc2k.c | 4 +--- Acked-by: Joachim Eastwood <manab...@gmail.com> regards, Joachim Eastwood
Re: [PATCH] i2c: don't print error when adding adapter fails
On 9 August 2016 at 13:36, Wolfram Sang wrote: > Since v4.8-rc1, the I2C core will print detailed information when adding an > I2C > adapter fails. So, drivers can skip this now. > > I am still undecided if I apply this as a single patch or break it out. But > for > reviewing, avoiding the patch bomb is probably a good thing. > > Should go via subsystem tree, I'd think. > > Wolfram Sang (1): > i2c: don't print error when adding adapter fails For > drivers/i2c/busses/i2c-lpc2k.c | 4 +--- Acked-by: Joachim Eastwood regards, Joachim Eastwood
Re: [PATCH v8 2/3] gpio: mmio: add DT support for memory-mapped GPIOs
Hi Christian,
On 8 May 2016 at 15:08, Christian Lamparter <chunk...@googlemail.com> wrote:
> From: Álvaro Fernández Rojas <nolt...@gmail.com>
>
> This patch adds support for defining memory-mapped GPIOs which
> are compatible with the existing gpio-mmio interface. The generic
> library provides support for many memory-mapped GPIO controllers
> that are found in various on-board FPGA and ASIC solutions that
> are used to control board's switches, LEDs, chip-selects,
> Ethernet/USB PHY power, etc.
>
> For setting GPIO's there are three configurations:
> 1. single input/output register resource (named "dat"),
> 2. set/clear pair (named "set" and "clr"),
> 3. single output register resource and single input resource
>("set" and dat").
>
> The configuration is detected by which resources are present.
> For the single output register, this drives a 1 by setting a bit
> and a zero by clearing a bit. For the set clr pair, this drives
> a 1 by setting a bit in the set register and clears it by setting
> a bit in the clear register. The configuration is detected by
> which resources are present.
>
> For setting the GPIO direction, there are three configurations:
> a. simple bidirectional GPIOs that requires no configuration.
> b. an output direction register (named "dirout")
>where a 1 bit indicates the GPIO is an output.
> c. an input direction register (named "dirin")
>where a 1 bit indicates the GPIO is an input.
>
> The first user for this binding is "wd,mbl-gpio".
>
> Signed-off-by: Álvaro Fernández Rojas <nolt...@gmail.com>
> Signed-off-by: Christian Lamparter <chunk...@googlemail.com>
> ---
> +#define ADD(_name, _func) { .compatible = _name, .data = _func }
I don't see the point in having a macro for such a simple data
structure, but since this v8 and Linus hasn't complained I guess it's
fine.
Using a macro here makes it impossible to grep for 'compatible'. Doing
'git grep compatible drivers/gpio/' is sometimes very useful to see
which hardware the driver actually supports.
> +static const struct of_device_id bgpio_of_match[] = {
> + ADD("wd,mbl-gpio", bgpio_basic_mmio_parse_dt),
> + { }
> +};
> +#undef ADD
> +MODULE_DEVICE_TABLE(of, bgpio_of_match);
> +
> +static struct bgpio_pdata *bgpio_parse_dt(struct platform_device *pdev,
> + unsigned long *flags)
> +{
> + const int (*parse_dt)(struct platform_device *,
> + struct bgpio_pdata *, unsigned long *);
> + const struct device_node *node = pdev->dev.of_node;
> + const struct of_device_id *of_id;
> + struct bgpio_pdata *pdata;
> + int err = -ENODEV;
> +
> + of_id = of_match_node(bgpio_of_match, node);
> + if (!of_id)
> + return NULL;
> +
> + pdata = devm_kzalloc(>dev, sizeof(struct bgpio_pdata),
> +GFP_KERNEL);
> + if (!pdata)
> + return ERR_PTR(-ENOMEM);
> +
> + parse_dt = (const void *)of_id->data;
You can retrieve OF match data using of_device_get_match_data(). Saves
you a couple of lines and better explains what your doing.
> + if (parse_dt)
> + err = parse_dt(pdev, pdata, flags);
> + if (err)
> + return ERR_PTR(err);
> +
> + return pdata;
> +}
regards,
Joachim Eastwood
Re: [PATCH v8 2/3] gpio: mmio: add DT support for memory-mapped GPIOs
Hi Christian,
On 8 May 2016 at 15:08, Christian Lamparter wrote:
> From: Álvaro Fernández Rojas
>
> This patch adds support for defining memory-mapped GPIOs which
> are compatible with the existing gpio-mmio interface. The generic
> library provides support for many memory-mapped GPIO controllers
> that are found in various on-board FPGA and ASIC solutions that
> are used to control board's switches, LEDs, chip-selects,
> Ethernet/USB PHY power, etc.
>
> For setting GPIO's there are three configurations:
> 1. single input/output register resource (named "dat"),
> 2. set/clear pair (named "set" and "clr"),
> 3. single output register resource and single input resource
>("set" and dat").
>
> The configuration is detected by which resources are present.
> For the single output register, this drives a 1 by setting a bit
> and a zero by clearing a bit. For the set clr pair, this drives
> a 1 by setting a bit in the set register and clears it by setting
> a bit in the clear register. The configuration is detected by
> which resources are present.
>
> For setting the GPIO direction, there are three configurations:
> a. simple bidirectional GPIOs that requires no configuration.
> b. an output direction register (named "dirout")
>where a 1 bit indicates the GPIO is an output.
> c. an input direction register (named "dirin")
>where a 1 bit indicates the GPIO is an input.
>
> The first user for this binding is "wd,mbl-gpio".
>
> Signed-off-by: Álvaro Fernández Rojas
> Signed-off-by: Christian Lamparter
> ---
> +#define ADD(_name, _func) { .compatible = _name, .data = _func }
I don't see the point in having a macro for such a simple data
structure, but since this v8 and Linus hasn't complained I guess it's
fine.
Using a macro here makes it impossible to grep for 'compatible'. Doing
'git grep compatible drivers/gpio/' is sometimes very useful to see
which hardware the driver actually supports.
> +static const struct of_device_id bgpio_of_match[] = {
> + ADD("wd,mbl-gpio", bgpio_basic_mmio_parse_dt),
> + { }
> +};
> +#undef ADD
> +MODULE_DEVICE_TABLE(of, bgpio_of_match);
> +
> +static struct bgpio_pdata *bgpio_parse_dt(struct platform_device *pdev,
> + unsigned long *flags)
> +{
> + const int (*parse_dt)(struct platform_device *,
> + struct bgpio_pdata *, unsigned long *);
> + const struct device_node *node = pdev->dev.of_node;
> + const struct of_device_id *of_id;
> + struct bgpio_pdata *pdata;
> + int err = -ENODEV;
> +
> + of_id = of_match_node(bgpio_of_match, node);
> + if (!of_id)
> + return NULL;
> +
> + pdata = devm_kzalloc(>dev, sizeof(struct bgpio_pdata),
> +GFP_KERNEL);
> + if (!pdata)
> + return ERR_PTR(-ENOMEM);
> +
> + parse_dt = (const void *)of_id->data;
You can retrieve OF match data using of_device_get_match_data(). Saves
you a couple of lines and better explains what your doing.
> + if (parse_dt)
> + err = parse_dt(pdev, pdata, flags);
> + if (err)
> + return ERR_PTR(err);
> +
> + return pdata;
> +}
regards,
Joachim Eastwood
Re: [PATCH 3/7] reset: lpc18xx: use devm_reset_controller_register()
On 1 May 2016 at 12:36, Masahiro Yamada <yamada.masah...@socionext.com> wrote: > Use devm_reset_controller_register() for the reset controller > registration and remove the unregister call from the .remove callback. > > Signed-off-by: Masahiro Yamada <yamada.masah...@socionext.com> > --- Acked-by: Joachim Eastwood <manab...@gmail.com> regards, Joachim Eastwood
Re: [PATCH 3/7] reset: lpc18xx: use devm_reset_controller_register()
On 1 May 2016 at 12:36, Masahiro Yamada wrote: > Use devm_reset_controller_register() for the reset controller > registration and remove the unregister call from the .remove callback. > > Signed-off-by: Masahiro Yamada > --- Acked-by: Joachim Eastwood regards, Joachim Eastwood
Re: [PATCH v5 02/24] pwm: use pwm_get/set_xxx() helpers where appropriate
On 14 April 2016 at 21:17, Boris Brezillon
<boris.brezil...@free-electrons.com> wrote:
> Use pwm_get/set_xxx() helpers instead of directly accessing the pwm->xxx
> field. Doing that will ease adaptation of the PWM framework to support
> atomic update.
>
> Signed-off-by: Boris Brezillon <boris.brezil...@free-electrons.com>
> ---
> Patch generated with the following coccinelle script:
>
> --->8---
> virtual patch
>
> @@
> struct pwm_device *p;
> expression e;
> @@
> (
> -(p)->polarity = e;
> +pwm_set_polarity(p, e);
> |
> -(p)->polarity
> +pwm_get_polarity(p)
> |
> -(p)->period = e;
> +pwm_set_period(p, e);
> |
> -(p)->period
> +pwm_get_period(p)
> |
> -(p)->duty_cycle = e;
> +pwm_set_duty_cycle(p, e);
> |
> -(p)->duty_cycle
> +pwm_get_duty_cycle(p)
> )
> --->8---
> ---
> drivers/pwm/pwm-crc.c | 2 +-
> drivers/pwm/pwm-lpc18xx-sct.c | 2 +-
> diff --git a/drivers/pwm/pwm-lpc18xx-sct.c b/drivers/pwm/pwm-lpc18xx-sct.c
> index 9861fed..19dc64c 100644
> --- a/drivers/pwm/pwm-lpc18xx-sct.c
> +++ b/drivers/pwm/pwm-lpc18xx-sct.c
> @@ -249,7 +249,7 @@ static int lpc18xx_pwm_enable(struct pwm_chip *chip,
> struct pwm_device *pwm)
>LPC18XX_PWM_EVSTATEMSK(lpc18xx_data->duty_event),
>LPC18XX_PWM_EVSTATEMSK_ALL);
>
> - if (pwm->polarity == PWM_POLARITY_NORMAL) {
> + if (pwm_get_polarity(pwm) == PWM_POLARITY_NORMAL) {
> set_event = lpc18xx_pwm->period_event;
> clear_event = lpc18xx_data->duty_event;
> res_action = LPC18XX_PWM_RES_SET;
For the lpc18xx-sct part:
Acked-by: Joachim Eastwood <manab...@gmail.com>
regards,
Joachim Eastwood
Re: [PATCH v5 02/24] pwm: use pwm_get/set_xxx() helpers where appropriate
On 14 April 2016 at 21:17, Boris Brezillon
wrote:
> Use pwm_get/set_xxx() helpers instead of directly accessing the pwm->xxx
> field. Doing that will ease adaptation of the PWM framework to support
> atomic update.
>
> Signed-off-by: Boris Brezillon
> ---
> Patch generated with the following coccinelle script:
>
> --->8---
> virtual patch
>
> @@
> struct pwm_device *p;
> expression e;
> @@
> (
> -(p)->polarity = e;
> +pwm_set_polarity(p, e);
> |
> -(p)->polarity
> +pwm_get_polarity(p)
> |
> -(p)->period = e;
> +pwm_set_period(p, e);
> |
> -(p)->period
> +pwm_get_period(p)
> |
> -(p)->duty_cycle = e;
> +pwm_set_duty_cycle(p, e);
> |
> -(p)->duty_cycle
> +pwm_get_duty_cycle(p)
> )
> --->8---
> ---
> drivers/pwm/pwm-crc.c | 2 +-
> drivers/pwm/pwm-lpc18xx-sct.c | 2 +-
> diff --git a/drivers/pwm/pwm-lpc18xx-sct.c b/drivers/pwm/pwm-lpc18xx-sct.c
> index 9861fed..19dc64c 100644
> --- a/drivers/pwm/pwm-lpc18xx-sct.c
> +++ b/drivers/pwm/pwm-lpc18xx-sct.c
> @@ -249,7 +249,7 @@ static int lpc18xx_pwm_enable(struct pwm_chip *chip,
> struct pwm_device *pwm)
>LPC18XX_PWM_EVSTATEMSK(lpc18xx_data->duty_event),
>LPC18XX_PWM_EVSTATEMSK_ALL);
>
> - if (pwm->polarity == PWM_POLARITY_NORMAL) {
> + if (pwm_get_polarity(pwm) == PWM_POLARITY_NORMAL) {
> set_event = lpc18xx_pwm->period_event;
> clear_event = lpc18xx_data->duty_event;
> res_action = LPC18XX_PWM_RES_SET;
For the lpc18xx-sct part:
Acked-by: Joachim Eastwood
regards,
Joachim Eastwood
Re: [PATCH v2 1/2] irqchip: add lpc18xx gpio pin interrupt driver
Hi Marc,
On 11 April 2016 at 17:15, Marc Zyngier <marc.zyng...@arm.com> wrote:
> Hi Joachim,
>
> On 02/04/16 17:35, Joachim Eastwood wrote:
>> Signed-off-by: Joachim Eastwood <manab...@gmail.com>
>
> As a start, a commit message would be appreciated.
Ops! I wonder where that disappeared to. The previous version did have one:
https://www.marc.info/?l=devicetree=145643797630859=3
I'll add it back in the next version.
>> ---
>> drivers/irqchip/Kconfig | 5 +
>> drivers/irqchip/Makefile| 1 +
>> drivers/irqchip/irq-lpc18xx-gpio-pint.c | 219
>>
>> 3 files changed, 225 insertions(+)
>> create mode 100644 drivers/irqchip/irq-lpc18xx-gpio-pint.c
>>
>> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
>> index 3e12479..0278837e 100644
>> --- a/drivers/irqchip/Kconfig
>> +++ b/drivers/irqchip/Kconfig
>> @@ -211,6 +211,11 @@ config KEYSTONE_IRQ
>> Support for Texas Instruments Keystone 2 IRQ controller IP
>> which
>> is part of the Keystone 2 IPC mechanism
>>
>> +config LPC18XX_GPIO_PINT
>> + bool
>> + select IRQ_DOMAIN
>> + select GENERIC_IRQ_CHIP
>> +
>> config MIPS_GIC
>> bool
>> select GENERIC_IRQ_IPI
>> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
>> index b03cfcb..bf60e0c 100644
>> --- a/drivers/irqchip/Makefile
>> +++ b/drivers/irqchip/Makefile
>> @@ -55,6 +55,7 @@ obj-$(CONFIG_BCM7038_L1_IRQ)+=
>> irq-bcm7038-l1.o
>> obj-$(CONFIG_BCM7120_L2_IRQ) += irq-bcm7120-l2.o
>> obj-$(CONFIG_BRCMSTB_L2_IRQ) += irq-brcmstb-l2.o
>> obj-$(CONFIG_KEYSTONE_IRQ) += irq-keystone.o
>> +obj-$(CONFIG_LPC18XX_GPIO_PINT) += irq-lpc18xx-gpio-pint.o
>> obj-$(CONFIG_MIPS_GIC) += irq-mips-gic.o
>> obj-$(CONFIG_ARCH_MEDIATEK) += irq-mtk-sysirq.o
>> obj-$(CONFIG_ARCH_DIGICOLOR) += irq-digicolor.o
>> diff --git a/drivers/irqchip/irq-lpc18xx-gpio-pint.c
>> b/drivers/irqchip/irq-lpc18xx-gpio-pint.c
>> new file mode 100644
>> index 000..d53e99b
>> --- /dev/null
>> +++ b/drivers/irqchip/irq-lpc18xx-gpio-pint.c
>> @@ -0,0 +1,219 @@
>> +/*
>> + * Irqchip driver for GPIO Pin Interrupt (PINT) on NXP LPC18xx/43xx.
>> + *
>> + * Copyright (C) 2016 Joachim Eastwood <manab...@gmail.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.
>> + *
>> + */
>> +
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +
>> +/* LPC18xx GPIO pin interrupt register offsets */
>> +#define LPC18XX_GPIO_PINT_ISEL 0x000
>> +#define LPC18XX_GPIO_PINT_SIENR 0x008
>> +#define LPC18XX_GPIO_PINT_CIENR 0x00c
>> +#define LPC18XX_GPIO_PINT_SIENF 0x014
>> +#define LPC18XX_GPIO_PINT_CIENF 0x018
>> +#define LPC18XX_GPIO_PINT_IST0x024
>> +
>> +#define PINT_MAX_IRQS32
>> +
>> +struct lpc18xx_gpio_pint_chip {
>> + struct irq_domain *domain;
>> + void __iomem *base;
>> + struct clk*clk;
>> + unsigned int revmap[];
>> +};
>> +
>> +static void lpc18xx_gpio_pint_handler(struct irq_desc *desc)
>> +{
>> + struct lpc18xx_gpio_pint_chip *pint = irq_desc_get_handler_data(desc);
>> + unsigned int irq = irq_desc_get_irq(desc);
>> + unsigned int hwirq = pint->revmap[irq];
>> + unsigned int virq;
>> +
>> + virq = irq_find_mapping(pint->domain, hwirq);
>> + generic_handle_irq(virq);
>
> Two things here:
> - It feels a bit weird that you are indirecting everything through a
> cascade interrupt. As you have a 1:1 relationship between the PINT
> interrupts and their NVIC counterparts, this would be better described
> as a hierarchical irqchip (see drivers/irqchip/irq-vf610-mscm-ir.c for
> an example)
Okey, I'll have a look at how irq-vf610-mscm-ir handels it.
> - If you decide to stick with the current approach, you're then missing
> the usual chained_irq_{enter,exit}.
Indeed.
Thanks for looking through.
regards,
Joachim Eastwood
Re: [PATCH v2 1/2] irqchip: add lpc18xx gpio pin interrupt driver
Hi Marc,
On 11 April 2016 at 17:15, Marc Zyngier wrote:
> Hi Joachim,
>
> On 02/04/16 17:35, Joachim Eastwood wrote:
>> Signed-off-by: Joachim Eastwood
>
> As a start, a commit message would be appreciated.
Ops! I wonder where that disappeared to. The previous version did have one:
https://www.marc.info/?l=devicetree=145643797630859=3
I'll add it back in the next version.
>> ---
>> drivers/irqchip/Kconfig | 5 +
>> drivers/irqchip/Makefile| 1 +
>> drivers/irqchip/irq-lpc18xx-gpio-pint.c | 219
>>
>> 3 files changed, 225 insertions(+)
>> create mode 100644 drivers/irqchip/irq-lpc18xx-gpio-pint.c
>>
>> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
>> index 3e12479..0278837e 100644
>> --- a/drivers/irqchip/Kconfig
>> +++ b/drivers/irqchip/Kconfig
>> @@ -211,6 +211,11 @@ config KEYSTONE_IRQ
>> Support for Texas Instruments Keystone 2 IRQ controller IP
>> which
>> is part of the Keystone 2 IPC mechanism
>>
>> +config LPC18XX_GPIO_PINT
>> + bool
>> + select IRQ_DOMAIN
>> + select GENERIC_IRQ_CHIP
>> +
>> config MIPS_GIC
>> bool
>> select GENERIC_IRQ_IPI
>> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
>> index b03cfcb..bf60e0c 100644
>> --- a/drivers/irqchip/Makefile
>> +++ b/drivers/irqchip/Makefile
>> @@ -55,6 +55,7 @@ obj-$(CONFIG_BCM7038_L1_IRQ)+=
>> irq-bcm7038-l1.o
>> obj-$(CONFIG_BCM7120_L2_IRQ) += irq-bcm7120-l2.o
>> obj-$(CONFIG_BRCMSTB_L2_IRQ) += irq-brcmstb-l2.o
>> obj-$(CONFIG_KEYSTONE_IRQ) += irq-keystone.o
>> +obj-$(CONFIG_LPC18XX_GPIO_PINT) += irq-lpc18xx-gpio-pint.o
>> obj-$(CONFIG_MIPS_GIC) += irq-mips-gic.o
>> obj-$(CONFIG_ARCH_MEDIATEK) += irq-mtk-sysirq.o
>> obj-$(CONFIG_ARCH_DIGICOLOR) += irq-digicolor.o
>> diff --git a/drivers/irqchip/irq-lpc18xx-gpio-pint.c
>> b/drivers/irqchip/irq-lpc18xx-gpio-pint.c
>> new file mode 100644
>> index 000..d53e99b
>> --- /dev/null
>> +++ b/drivers/irqchip/irq-lpc18xx-gpio-pint.c
>> @@ -0,0 +1,219 @@
>> +/*
>> + * Irqchip driver for GPIO Pin Interrupt (PINT) on NXP LPC18xx/43xx.
>> + *
>> + * Copyright (C) 2016 Joachim Eastwood
>> + *
>> + * 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.
>> + *
>> + */
>> +
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +
>> +/* LPC18xx GPIO pin interrupt register offsets */
>> +#define LPC18XX_GPIO_PINT_ISEL 0x000
>> +#define LPC18XX_GPIO_PINT_SIENR 0x008
>> +#define LPC18XX_GPIO_PINT_CIENR 0x00c
>> +#define LPC18XX_GPIO_PINT_SIENF 0x014
>> +#define LPC18XX_GPIO_PINT_CIENF 0x018
>> +#define LPC18XX_GPIO_PINT_IST0x024
>> +
>> +#define PINT_MAX_IRQS32
>> +
>> +struct lpc18xx_gpio_pint_chip {
>> + struct irq_domain *domain;
>> + void __iomem *base;
>> + struct clk*clk;
>> + unsigned int revmap[];
>> +};
>> +
>> +static void lpc18xx_gpio_pint_handler(struct irq_desc *desc)
>> +{
>> + struct lpc18xx_gpio_pint_chip *pint = irq_desc_get_handler_data(desc);
>> + unsigned int irq = irq_desc_get_irq(desc);
>> + unsigned int hwirq = pint->revmap[irq];
>> + unsigned int virq;
>> +
>> + virq = irq_find_mapping(pint->domain, hwirq);
>> + generic_handle_irq(virq);
>
> Two things here:
> - It feels a bit weird that you are indirecting everything through a
> cascade interrupt. As you have a 1:1 relationship between the PINT
> interrupts and their NVIC counterparts, this would be better described
> as a hierarchical irqchip (see drivers/irqchip/irq-vf610-mscm-ir.c for
> an example)
Okey, I'll have a look at how irq-vf610-mscm-ir handels it.
> - If you decide to stick with the current approach, you're then missing
> the usual chained_irq_{enter,exit}.
Indeed.
Thanks for looking through.
regards,
Joachim Eastwood
Re: [PATCH v2] iio: max5487: Add support for Maxim digital potentiometers
On 10 April 2016 at 14:47, Joachim Eastwood <manab...@gmail.com> wrote:
> Hi Cristina,
>
> On 9 April 2016 at 10:24, Cristina Moraru <cristina.morar...@gmail.com> wrote:
>> Add implementation for Maxim MAX5487, MAX5488, MAX5489
>> digital potentiometers.
>>
>> Datasheet:
>> http://datasheets.maximintegrated.com/en/ds/MAX5487-MAX5489.pdf
>>
>> Signed-off-by: Cristina Moraru <cristina.morar...@gmail.com>
>> CC: Daniel Baluta <daniel.bal...@intel.com>
>> ---
> ...
>> +static int max5487_read_raw(struct iio_dev *indio_dev,
>> + struct iio_chan_spec const *chan,
>> + int *val, int *val2, long mask)
>> +{
>> + struct max5487_data *data = iio_priv(indio_dev);
>> +
>> + if (mask != IIO_CHAN_INFO_SCALE)
>> + return -EINVAL;
>> +
>> + *val = 1000 * data->kohms;
>> + *val2 = MAX5487_MAX_POS;
>
> Newline before return.
>
>> + return IIO_VAL_FRACTIONAL;
>> +}
>> +
>> +static int max5487_write_raw(struct iio_dev *indio_dev,
>> +struct iio_chan_spec const *chan,
>> +int val, int val2, long mask)
>> +{
>> + struct max5487_data *data = iio_priv(indio_dev);
>> +
>> + switch (mask) {
>> + case IIO_CHAN_INFO_RAW:
>> + if (val < 0 || val > MAX5487_MAX_POS)
>> + return -EINVAL;
>> + return regmap_write(data->regmap, chan->address, val);
>> + default:
>> + return -EINVAL;
>> + }
>> + return -EINVAL;
>
> To be consistent with your max5487_read_raw() function you could do a:
>if (mask != IIO_CHAN_INFO_RAW)
>return -EINVAL;
>
>
>> +static const struct iio_info max5487_info = {
>> + .read_raw = _read_raw,
>> + .write_raw = _write_raw,
>
> Address operator should be unnecessary on functions.
>
>
>> + data->regmap = devm_regmap_init_spi(spi, _regmap_config);
>> + if (IS_ERR(data->regmap))
>> + return PTR_ERR(data->regmap);
>
> Nothing wrong with using regmap here, but since you are only using
> simple regmap_write()'s you might as well have used spi_write()
> directly. I am not telling you to switch, but I don't see the point of
> using regmap here.
Looking again: it seem that spi.h doesn't have a function that do
write(cmd, data) which regmap does. So I guess that is one reason for
using regmap. But it wouldn't be hard to create a write(cmd,
data)-function for spi either. Just wrap spi_write() and have a local
buf var. I am a bit surprised that spi.h doesn't have such a function
as it should be quite a common pattern for spi chips.
>
> Which reminds me; for regmap you need to select REGMAP_SPI in your
> Kconfig entry.
>
>
> regards,
> Joachim Eastwood
Re: [PATCH v2] iio: max5487: Add support for Maxim digital potentiometers
On 10 April 2016 at 14:47, Joachim Eastwood wrote:
> Hi Cristina,
>
> On 9 April 2016 at 10:24, Cristina Moraru wrote:
>> Add implementation for Maxim MAX5487, MAX5488, MAX5489
>> digital potentiometers.
>>
>> Datasheet:
>> http://datasheets.maximintegrated.com/en/ds/MAX5487-MAX5489.pdf
>>
>> Signed-off-by: Cristina Moraru
>> CC: Daniel Baluta
>> ---
> ...
>> +static int max5487_read_raw(struct iio_dev *indio_dev,
>> + struct iio_chan_spec const *chan,
>> + int *val, int *val2, long mask)
>> +{
>> + struct max5487_data *data = iio_priv(indio_dev);
>> +
>> + if (mask != IIO_CHAN_INFO_SCALE)
>> + return -EINVAL;
>> +
>> + *val = 1000 * data->kohms;
>> + *val2 = MAX5487_MAX_POS;
>
> Newline before return.
>
>> + return IIO_VAL_FRACTIONAL;
>> +}
>> +
>> +static int max5487_write_raw(struct iio_dev *indio_dev,
>> +struct iio_chan_spec const *chan,
>> +int val, int val2, long mask)
>> +{
>> + struct max5487_data *data = iio_priv(indio_dev);
>> +
>> + switch (mask) {
>> + case IIO_CHAN_INFO_RAW:
>> + if (val < 0 || val > MAX5487_MAX_POS)
>> + return -EINVAL;
>> + return regmap_write(data->regmap, chan->address, val);
>> + default:
>> + return -EINVAL;
>> + }
>> + return -EINVAL;
>
> To be consistent with your max5487_read_raw() function you could do a:
>if (mask != IIO_CHAN_INFO_RAW)
>return -EINVAL;
>
>
>> +static const struct iio_info max5487_info = {
>> + .read_raw = _read_raw,
>> + .write_raw = _write_raw,
>
> Address operator should be unnecessary on functions.
>
>
>> + data->regmap = devm_regmap_init_spi(spi, _regmap_config);
>> + if (IS_ERR(data->regmap))
>> + return PTR_ERR(data->regmap);
>
> Nothing wrong with using regmap here, but since you are only using
> simple regmap_write()'s you might as well have used spi_write()
> directly. I am not telling you to switch, but I don't see the point of
> using regmap here.
Looking again: it seem that spi.h doesn't have a function that do
write(cmd, data) which regmap does. So I guess that is one reason for
using regmap. But it wouldn't be hard to create a write(cmd,
data)-function for spi either. Just wrap spi_write() and have a local
buf var. I am a bit surprised that spi.h doesn't have such a function
as it should be quite a common pattern for spi chips.
>
> Which reminds me; for regmap you need to select REGMAP_SPI in your
> Kconfig entry.
>
>
> regards,
> Joachim Eastwood
Re: [PATCH v2] iio: max5487: Add support for Maxim digital potentiometers
Hi Cristina,
On 9 April 2016 at 10:24, Cristina Moraru <cristina.morar...@gmail.com> wrote:
> Add implementation for Maxim MAX5487, MAX5488, MAX5489
> digital potentiometers.
>
> Datasheet:
> http://datasheets.maximintegrated.com/en/ds/MAX5487-MAX5489.pdf
>
> Signed-off-by: Cristina Moraru <cristina.morar...@gmail.com>
> CC: Daniel Baluta <daniel.bal...@intel.com>
> ---
...
> +static int max5487_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int *val, int *val2, long mask)
> +{
> + struct max5487_data *data = iio_priv(indio_dev);
> +
> + if (mask != IIO_CHAN_INFO_SCALE)
> + return -EINVAL;
> +
> + *val = 1000 * data->kohms;
> + *val2 = MAX5487_MAX_POS;
Newline before return.
> + return IIO_VAL_FRACTIONAL;
> +}
> +
> +static int max5487_write_raw(struct iio_dev *indio_dev,
> +struct iio_chan_spec const *chan,
> +int val, int val2, long mask)
> +{
> + struct max5487_data *data = iio_priv(indio_dev);
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
> + if (val < 0 || val > MAX5487_MAX_POS)
> + return -EINVAL;
> + return regmap_write(data->regmap, chan->address, val);
> + default:
> + return -EINVAL;
> + }
> + return -EINVAL;
To be consistent with your max5487_read_raw() function you could do a:
if (mask != IIO_CHAN_INFO_RAW)
return -EINVAL;
> +static const struct iio_info max5487_info = {
> + .read_raw = _read_raw,
> + .write_raw = _write_raw,
Address operator should be unnecessary on functions.
> + data->regmap = devm_regmap_init_spi(spi, _regmap_config);
> + if (IS_ERR(data->regmap))
> + return PTR_ERR(data->regmap);
Nothing wrong with using regmap here, but since you are only using
simple regmap_write()'s you might as well have used spi_write()
directly. I am not telling you to switch, but I don't see the point of
using regmap here.
Which reminds me; for regmap you need to select REGMAP_SPI in your
Kconfig entry.
regards,
Joachim Eastwood
Re: [PATCH v2] iio: max5487: Add support for Maxim digital potentiometers
Hi Cristina,
On 9 April 2016 at 10:24, Cristina Moraru wrote:
> Add implementation for Maxim MAX5487, MAX5488, MAX5489
> digital potentiometers.
>
> Datasheet:
> http://datasheets.maximintegrated.com/en/ds/MAX5487-MAX5489.pdf
>
> Signed-off-by: Cristina Moraru
> CC: Daniel Baluta
> ---
...
> +static int max5487_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int *val, int *val2, long mask)
> +{
> + struct max5487_data *data = iio_priv(indio_dev);
> +
> + if (mask != IIO_CHAN_INFO_SCALE)
> + return -EINVAL;
> +
> + *val = 1000 * data->kohms;
> + *val2 = MAX5487_MAX_POS;
Newline before return.
> + return IIO_VAL_FRACTIONAL;
> +}
> +
> +static int max5487_write_raw(struct iio_dev *indio_dev,
> +struct iio_chan_spec const *chan,
> +int val, int val2, long mask)
> +{
> + struct max5487_data *data = iio_priv(indio_dev);
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
> + if (val < 0 || val > MAX5487_MAX_POS)
> + return -EINVAL;
> + return regmap_write(data->regmap, chan->address, val);
> + default:
> + return -EINVAL;
> + }
> + return -EINVAL;
To be consistent with your max5487_read_raw() function you could do a:
if (mask != IIO_CHAN_INFO_RAW)
return -EINVAL;
> +static const struct iio_info max5487_info = {
> + .read_raw = _read_raw,
> + .write_raw = _write_raw,
Address operator should be unnecessary on functions.
> + data->regmap = devm_regmap_init_spi(spi, _regmap_config);
> + if (IS_ERR(data->regmap))
> + return PTR_ERR(data->regmap);
Nothing wrong with using regmap here, but since you are only using
simple regmap_write()'s you might as well have used spi_write()
directly. I am not telling you to switch, but I don't see the point of
using regmap here.
Which reminds me; for regmap you need to select REGMAP_SPI in your
Kconfig entry.
regards,
Joachim Eastwood
Re: [PATCH 5/9] iio: accel: mma7455: use regmap to retrieve struct device
Hi Alison,
On 6 April 2016 at 07:18, Alison Schofield <amsfiel...@gmail.com> wrote:
> Driver includes struct regmap and struct device in its global data.
> Remove the struct device and use regmap API to retrieve device info.
>
> Patch created using Coccinelle plus manual edits.
>
> Signed-off-by: Alison Schofield <amsfiel...@gmail.com>
> ---
> drivers/iio/accel/mma7455_core.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/iio/accel/mma7455_core.c
> b/drivers/iio/accel/mma7455_core.c
> index c633cc2..c902f54 100644
> --- a/drivers/iio/accel/mma7455_core.c
> +++ b/drivers/iio/accel/mma7455_core.c
> @@ -55,11 +55,11 @@
>
> struct mma7455_data {
> struct regmap *regmap;
> - struct device *dev;
> };
>
> static int mma7455_drdy(struct mma7455_data *mma7455)
> {
> + struct device *dev = regmap_get_device(mma7455->regmap);
ah, nice!
Acked-by: Joachim Eastwood <manab...@gmail.com>
regards,
Joachim Eastwood
Re: [PATCH 5/9] iio: accel: mma7455: use regmap to retrieve struct device
Hi Alison,
On 6 April 2016 at 07:18, Alison Schofield wrote:
> Driver includes struct regmap and struct device in its global data.
> Remove the struct device and use regmap API to retrieve device info.
>
> Patch created using Coccinelle plus manual edits.
>
> Signed-off-by: Alison Schofield
> ---
> drivers/iio/accel/mma7455_core.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/iio/accel/mma7455_core.c
> b/drivers/iio/accel/mma7455_core.c
> index c633cc2..c902f54 100644
> --- a/drivers/iio/accel/mma7455_core.c
> +++ b/drivers/iio/accel/mma7455_core.c
> @@ -55,11 +55,11 @@
>
> struct mma7455_data {
> struct regmap *regmap;
> - struct device *dev;
> };
>
> static int mma7455_drdy(struct mma7455_data *mma7455)
> {
> + struct device *dev = regmap_get_device(mma7455->regmap);
ah, nice!
Acked-by: Joachim Eastwood
regards,
Joachim Eastwood
[PATCH v2 2/2] devicetree: document NXP LPC1850 PINT irq controller binding
Add binding documentation for NXP LPC1850 GPIO Pin Interrupt (PINT)
controller.
Signed-off-by: Joachim Eastwood <manab...@gmail.com>
---
.../interrupt-controller/nxp,lpc1850-gpio-pint.txt | 26 ++
1 file changed, 26 insertions(+)
create mode 100644
Documentation/devicetree/bindings/interrupt-controller/nxp,lpc1850-gpio-pint.txt
diff --git
a/Documentation/devicetree/bindings/interrupt-controller/nxp,lpc1850-gpio-pint.txt
b/Documentation/devicetree/bindings/interrupt-controller/nxp,lpc1850-gpio-pint.txt
new file mode 100644
index 000..0d59d31
--- /dev/null
+++
b/Documentation/devicetree/bindings/interrupt-controller/nxp,lpc1850-gpio-pint.txt
@@ -0,0 +1,26 @@
+NXP LPC18xx/43xx GPIO Pin Interrupt (PINT) controller
+
+Required properties:
+
+- compatible : should be "nxp,lpc1850-gpio-pint".
+- reg : Specifies base physical address and size of the registers.
+- interrupt-controller : Identifies the node as an interrupt controller
+- #interrupt-cells : Specifies the number of cells needed to encode an
+ interrupt source. The value shall be 2.
+- interrupts : Must contain the interrupt numbers from the parent controller.
+ This defines the mapping between the controllers where the first
+ interrupt is mapped to pint hwirq 0 and so on. Which interrupts
+ that are connected to where and the number of is device specific.
+ For lpc1850 the mapping is 32 33 34 35 36 37 38 39 (8 in total).
+- clocks: Must contain a reference to the functional clock.
+
+Example:
+
+pint: interrupt-controller@40087000 {
+ compatible = "nxp,lpc1850-gpio-pint";
+ reg = <0x40087000 0x1000>;
+ interrupt-controller;
+ #interrupt-cells = <2>;
+ interrupts = <32 33 34 35 36 37 38 39>;
+ clocks = < CLK_CPU_GPIO>;
+};
--
2.8.0
[PATCH v2 2/2] devicetree: document NXP LPC1850 PINT irq controller binding
Add binding documentation for NXP LPC1850 GPIO Pin Interrupt (PINT)
controller.
Signed-off-by: Joachim Eastwood
---
.../interrupt-controller/nxp,lpc1850-gpio-pint.txt | 26 ++
1 file changed, 26 insertions(+)
create mode 100644
Documentation/devicetree/bindings/interrupt-controller/nxp,lpc1850-gpio-pint.txt
diff --git
a/Documentation/devicetree/bindings/interrupt-controller/nxp,lpc1850-gpio-pint.txt
b/Documentation/devicetree/bindings/interrupt-controller/nxp,lpc1850-gpio-pint.txt
new file mode 100644
index 000..0d59d31
--- /dev/null
+++
b/Documentation/devicetree/bindings/interrupt-controller/nxp,lpc1850-gpio-pint.txt
@@ -0,0 +1,26 @@
+NXP LPC18xx/43xx GPIO Pin Interrupt (PINT) controller
+
+Required properties:
+
+- compatible : should be "nxp,lpc1850-gpio-pint".
+- reg : Specifies base physical address and size of the registers.
+- interrupt-controller : Identifies the node as an interrupt controller
+- #interrupt-cells : Specifies the number of cells needed to encode an
+ interrupt source. The value shall be 2.
+- interrupts : Must contain the interrupt numbers from the parent controller.
+ This defines the mapping between the controllers where the first
+ interrupt is mapped to pint hwirq 0 and so on. Which interrupts
+ that are connected to where and the number of is device specific.
+ For lpc1850 the mapping is 32 33 34 35 36 37 38 39 (8 in total).
+- clocks: Must contain a reference to the functional clock.
+
+Example:
+
+pint: interrupt-controller@40087000 {
+ compatible = "nxp,lpc1850-gpio-pint";
+ reg = <0x40087000 0x1000>;
+ interrupt-controller;
+ #interrupt-cells = <2>;
+ interrupts = <32 33 34 35 36 37 38 39>;
+ clocks = < CLK_CPU_GPIO>;
+};
--
2.8.0
[PATCH v2 1/2] irqchip: add lpc18xx gpio pin interrupt driver
Signed-off-by: Joachim Eastwood <manab...@gmail.com>
---
drivers/irqchip/Kconfig | 5 +
drivers/irqchip/Makefile| 1 +
drivers/irqchip/irq-lpc18xx-gpio-pint.c | 219
3 files changed, 225 insertions(+)
create mode 100644 drivers/irqchip/irq-lpc18xx-gpio-pint.c
diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index 3e12479..0278837e 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -211,6 +211,11 @@ config KEYSTONE_IRQ
Support for Texas Instruments Keystone 2 IRQ controller IP which
is part of the Keystone 2 IPC mechanism
+config LPC18XX_GPIO_PINT
+ bool
+ select IRQ_DOMAIN
+ select GENERIC_IRQ_CHIP
+
config MIPS_GIC
bool
select GENERIC_IRQ_IPI
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index b03cfcb..bf60e0c 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -55,6 +55,7 @@ obj-$(CONFIG_BCM7038_L1_IRQ) += irq-bcm7038-l1.o
obj-$(CONFIG_BCM7120_L2_IRQ) += irq-bcm7120-l2.o
obj-$(CONFIG_BRCMSTB_L2_IRQ) += irq-brcmstb-l2.o
obj-$(CONFIG_KEYSTONE_IRQ) += irq-keystone.o
+obj-$(CONFIG_LPC18XX_GPIO_PINT)+= irq-lpc18xx-gpio-pint.o
obj-$(CONFIG_MIPS_GIC) += irq-mips-gic.o
obj-$(CONFIG_ARCH_MEDIATEK)+= irq-mtk-sysirq.o
obj-$(CONFIG_ARCH_DIGICOLOR) += irq-digicolor.o
diff --git a/drivers/irqchip/irq-lpc18xx-gpio-pint.c
b/drivers/irqchip/irq-lpc18xx-gpio-pint.c
new file mode 100644
index 000..d53e99b
--- /dev/null
+++ b/drivers/irqchip/irq-lpc18xx-gpio-pint.c
@@ -0,0 +1,219 @@
+/*
+ * Irqchip driver for GPIO Pin Interrupt (PINT) on NXP LPC18xx/43xx.
+ *
+ * Copyright (C) 2016 Joachim Eastwood <manab...@gmail.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.
+ *
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+/* LPC18xx GPIO pin interrupt register offsets */
+#define LPC18XX_GPIO_PINT_ISEL 0x000
+#define LPC18XX_GPIO_PINT_SIENR0x008
+#define LPC18XX_GPIO_PINT_CIENR0x00c
+#define LPC18XX_GPIO_PINT_SIENF0x014
+#define LPC18XX_GPIO_PINT_CIENF0x018
+#define LPC18XX_GPIO_PINT_IST 0x024
+
+#define PINT_MAX_IRQS 32
+
+struct lpc18xx_gpio_pint_chip {
+ struct irq_domain *domain;
+ void __iomem *base;
+ struct clk*clk;
+ unsigned int revmap[];
+};
+
+static void lpc18xx_gpio_pint_handler(struct irq_desc *desc)
+{
+ struct lpc18xx_gpio_pint_chip *pint = irq_desc_get_handler_data(desc);
+ unsigned int irq = irq_desc_get_irq(desc);
+ unsigned int hwirq = pint->revmap[irq];
+ unsigned int virq;
+
+ virq = irq_find_mapping(pint->domain, hwirq);
+ generic_handle_irq(virq);
+}
+
+static void lpc18xx_gpio_pint_edge_mask(struct irq_data *d)
+{
+ struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
+ u32 mask = d->mask;
+
+ irq_gc_lock(gc);
+ irq_reg_writel(gc, mask, LPC18XX_GPIO_PINT_CIENR);
+ irq_reg_writel(gc, mask, LPC18XX_GPIO_PINT_CIENF);
+ irq_gc_unlock(gc);
+}
+
+static void lpc18xx_gpio_pint_edge_unmask(struct irq_data *d)
+{
+ struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
+ u32 type, mask = d->mask;
+
+ irq_gc_lock(gc);
+ type = irqd_get_trigger_type(d);
+ if (type & IRQ_TYPE_EDGE_RISING)
+ irq_reg_writel(gc, mask, LPC18XX_GPIO_PINT_SIENR);
+ if (type & IRQ_TYPE_EDGE_FALLING)
+ irq_reg_writel(gc, mask, LPC18XX_GPIO_PINT_SIENF);
+ irq_gc_unlock(gc);
+}
+
+static int lpc18xx_gpio_pint_type(struct irq_data *data, unsigned int type)
+{
+ struct irq_chip_generic *gc = irq_data_get_irq_chip_data(data);
+ u32 mask = data->mask;
+
+ irq_gc_lock(gc);
+ if (type & IRQ_TYPE_LEVEL_MASK)
+ gc->type_cache |= mask;
+ else
+ gc->type_cache &= ~mask;
+ irq_reg_writel(gc, gc->type_cache, LPC18XX_GPIO_PINT_ISEL);
+
+ switch (type) {
+ case IRQ_TYPE_LEVEL_HIGH:
+ irq_reg_writel(gc, mask, LPC18XX_GPIO_PINT_SIENF);
+ break;
+
+ case IRQ_TYPE_LEVEL_LOW:
+ irq_reg_writel(gc, mask, LPC18XX_GPIO_PINT_CIENF);
+ break;
+
+ /* IRQ_TYPE_EDGE_* is set in lpc18xx_gpio_pint_edge_unmask */
+ }
+
+ irqd_set_trigger_type(data, type);
+ irq_setup_alt_chip(data, type);
+ irq_gc_unlock(gc);
+
+ return 0;
+}
+
+static int lpc18xx_gpio_pint_probe(struct platform_device *pdev)
+{
+ struct device_node *np = pdev->dev.of_node;
+
[PATCH v2 0/2] PINT irqchip driver for NXP LPC18xx family
The LPC1850 has no less than 3 GPIO interrupt blocks. One of these blocks is called 'gpio pin interrupt' or just PINT. LPC1850 PINT can handle up to 8 interrupts and these have a one-to-one relationship with the main interrupt controller (NVIC). The interrupts on PINT can be either level or edge trigger and supports any polarity. This patch set adds a irqchip driver for PINT on LPC18xx. This version address the comments from Thomas and Rob. Changes since v1: - use irq_gc_ack_set_bit for edge ack - switch to generic functions on level mask/unmask - use revmap to look up hwirq in handler - describe the interrupts property better in dt doc Joachim Eastwood (2): irqchip: add lpc18xx gpio pin interrupt driver devicetree: document NXP LPC1850 PINT irq controller binding .../interrupt-controller/nxp,lpc1850-gpio-pint.txt | 26 +++ drivers/irqchip/Kconfig| 5 + drivers/irqchip/Makefile | 1 + drivers/irqchip/irq-lpc18xx-gpio-pint.c| 219 + 4 files changed, 251 insertions(+) create mode 100644 Documentation/devicetree/bindings/interrupt-controller/nxp,lpc1850-gpio-pint.txt create mode 100644 drivers/irqchip/irq-lpc18xx-gpio-pint.c -- 2.8.0
[PATCH v2 1/2] irqchip: add lpc18xx gpio pin interrupt driver
Signed-off-by: Joachim Eastwood
---
drivers/irqchip/Kconfig | 5 +
drivers/irqchip/Makefile| 1 +
drivers/irqchip/irq-lpc18xx-gpio-pint.c | 219
3 files changed, 225 insertions(+)
create mode 100644 drivers/irqchip/irq-lpc18xx-gpio-pint.c
diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index 3e12479..0278837e 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -211,6 +211,11 @@ config KEYSTONE_IRQ
Support for Texas Instruments Keystone 2 IRQ controller IP which
is part of the Keystone 2 IPC mechanism
+config LPC18XX_GPIO_PINT
+ bool
+ select IRQ_DOMAIN
+ select GENERIC_IRQ_CHIP
+
config MIPS_GIC
bool
select GENERIC_IRQ_IPI
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index b03cfcb..bf60e0c 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -55,6 +55,7 @@ obj-$(CONFIG_BCM7038_L1_IRQ) += irq-bcm7038-l1.o
obj-$(CONFIG_BCM7120_L2_IRQ) += irq-bcm7120-l2.o
obj-$(CONFIG_BRCMSTB_L2_IRQ) += irq-brcmstb-l2.o
obj-$(CONFIG_KEYSTONE_IRQ) += irq-keystone.o
+obj-$(CONFIG_LPC18XX_GPIO_PINT)+= irq-lpc18xx-gpio-pint.o
obj-$(CONFIG_MIPS_GIC) += irq-mips-gic.o
obj-$(CONFIG_ARCH_MEDIATEK)+= irq-mtk-sysirq.o
obj-$(CONFIG_ARCH_DIGICOLOR) += irq-digicolor.o
diff --git a/drivers/irqchip/irq-lpc18xx-gpio-pint.c
b/drivers/irqchip/irq-lpc18xx-gpio-pint.c
new file mode 100644
index 000..d53e99b
--- /dev/null
+++ b/drivers/irqchip/irq-lpc18xx-gpio-pint.c
@@ -0,0 +1,219 @@
+/*
+ * Irqchip driver for GPIO Pin Interrupt (PINT) on NXP LPC18xx/43xx.
+ *
+ * Copyright (C) 2016 Joachim Eastwood
+ *
+ * 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.
+ *
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+/* LPC18xx GPIO pin interrupt register offsets */
+#define LPC18XX_GPIO_PINT_ISEL 0x000
+#define LPC18XX_GPIO_PINT_SIENR0x008
+#define LPC18XX_GPIO_PINT_CIENR0x00c
+#define LPC18XX_GPIO_PINT_SIENF0x014
+#define LPC18XX_GPIO_PINT_CIENF0x018
+#define LPC18XX_GPIO_PINT_IST 0x024
+
+#define PINT_MAX_IRQS 32
+
+struct lpc18xx_gpio_pint_chip {
+ struct irq_domain *domain;
+ void __iomem *base;
+ struct clk*clk;
+ unsigned int revmap[];
+};
+
+static void lpc18xx_gpio_pint_handler(struct irq_desc *desc)
+{
+ struct lpc18xx_gpio_pint_chip *pint = irq_desc_get_handler_data(desc);
+ unsigned int irq = irq_desc_get_irq(desc);
+ unsigned int hwirq = pint->revmap[irq];
+ unsigned int virq;
+
+ virq = irq_find_mapping(pint->domain, hwirq);
+ generic_handle_irq(virq);
+}
+
+static void lpc18xx_gpio_pint_edge_mask(struct irq_data *d)
+{
+ struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
+ u32 mask = d->mask;
+
+ irq_gc_lock(gc);
+ irq_reg_writel(gc, mask, LPC18XX_GPIO_PINT_CIENR);
+ irq_reg_writel(gc, mask, LPC18XX_GPIO_PINT_CIENF);
+ irq_gc_unlock(gc);
+}
+
+static void lpc18xx_gpio_pint_edge_unmask(struct irq_data *d)
+{
+ struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
+ u32 type, mask = d->mask;
+
+ irq_gc_lock(gc);
+ type = irqd_get_trigger_type(d);
+ if (type & IRQ_TYPE_EDGE_RISING)
+ irq_reg_writel(gc, mask, LPC18XX_GPIO_PINT_SIENR);
+ if (type & IRQ_TYPE_EDGE_FALLING)
+ irq_reg_writel(gc, mask, LPC18XX_GPIO_PINT_SIENF);
+ irq_gc_unlock(gc);
+}
+
+static int lpc18xx_gpio_pint_type(struct irq_data *data, unsigned int type)
+{
+ struct irq_chip_generic *gc = irq_data_get_irq_chip_data(data);
+ u32 mask = data->mask;
+
+ irq_gc_lock(gc);
+ if (type & IRQ_TYPE_LEVEL_MASK)
+ gc->type_cache |= mask;
+ else
+ gc->type_cache &= ~mask;
+ irq_reg_writel(gc, gc->type_cache, LPC18XX_GPIO_PINT_ISEL);
+
+ switch (type) {
+ case IRQ_TYPE_LEVEL_HIGH:
+ irq_reg_writel(gc, mask, LPC18XX_GPIO_PINT_SIENF);
+ break;
+
+ case IRQ_TYPE_LEVEL_LOW:
+ irq_reg_writel(gc, mask, LPC18XX_GPIO_PINT_CIENF);
+ break;
+
+ /* IRQ_TYPE_EDGE_* is set in lpc18xx_gpio_pint_edge_unmask */
+ }
+
+ irqd_set_trigger_type(data, type);
+ irq_setup_alt_chip(data, type);
+ irq_gc_unlock(gc);
+
+ return 0;
+}
+
+static int lpc18xx_gpio_pint_probe(struct platform_device *pdev)
+{
+ struct device_node *np = pdev->dev.of_node;
+ struct lpc18xx_gpio_pint_chip *pint;
+ st
[PATCH v2 0/2] PINT irqchip driver for NXP LPC18xx family
The LPC1850 has no less than 3 GPIO interrupt blocks. One of these blocks is called 'gpio pin interrupt' or just PINT. LPC1850 PINT can handle up to 8 interrupts and these have a one-to-one relationship with the main interrupt controller (NVIC). The interrupts on PINT can be either level or edge trigger and supports any polarity. This patch set adds a irqchip driver for PINT on LPC18xx. This version address the comments from Thomas and Rob. Changes since v1: - use irq_gc_ack_set_bit for edge ack - switch to generic functions on level mask/unmask - use revmap to look up hwirq in handler - describe the interrupts property better in dt doc Joachim Eastwood (2): irqchip: add lpc18xx gpio pin interrupt driver devicetree: document NXP LPC1850 PINT irq controller binding .../interrupt-controller/nxp,lpc1850-gpio-pint.txt | 26 +++ drivers/irqchip/Kconfig| 5 + drivers/irqchip/Makefile | 1 + drivers/irqchip/irq-lpc18xx-gpio-pint.c| 219 + 4 files changed, 251 insertions(+) create mode 100644 Documentation/devicetree/bindings/interrupt-controller/nxp,lpc1850-gpio-pint.txt create mode 100644 drivers/irqchip/irq-lpc18xx-gpio-pint.c -- 2.8.0
Re: [PATCH] ARM: dts: armv7-m: add address to unit name
On 2 April 2016 at 08:08, Stefan Agner <ste...@agner.ch> wrote:
> Add address to unit name to remove the following warning:
> Warning (unit_address_vs_reg): Node /nv-interrupt-controller has a
> reg or ranges property, but no unit name
>
> Signed-off-by: Stefan Agner <ste...@agner.ch>
> ---
> Hi Arnd,
>
> Not sure through which tree this should go but an earlier patch
> seemd to be applied directly by you...
>
> --
> Stefan
>
> arch/arm/boot/dts/armv7-m.dtsi | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/arm/boot/dts/armv7-m.dtsi b/arch/arm/boot/dts/armv7-m.dtsi
> index b1ad7cf..8126bec 100644
> --- a/arch/arm/boot/dts/armv7-m.dtsi
> +++ b/arch/arm/boot/dts/armv7-m.dtsi
> @@ -1,7 +1,7 @@
> #include "skeleton.dtsi"
>
> / {
> - nvic: nv-interrupt-controller {
> + nvic: nv-interrupt-controller@0xe000e100 {
While changing the line it might be good idea to use the standard
'interrupt-controller' name instead.
I posted the same patch couple of days ago, btw.
http://marc.info/?l=devicetree=145929088915714=2
But I don't care which one that is applied.
regards,
Joachim Eastwood
Re: [PATCH] ARM: dts: armv7-m: add address to unit name
On 2 April 2016 at 08:08, Stefan Agner wrote:
> Add address to unit name to remove the following warning:
> Warning (unit_address_vs_reg): Node /nv-interrupt-controller has a
> reg or ranges property, but no unit name
>
> Signed-off-by: Stefan Agner
> ---
> Hi Arnd,
>
> Not sure through which tree this should go but an earlier patch
> seemd to be applied directly by you...
>
> --
> Stefan
>
> arch/arm/boot/dts/armv7-m.dtsi | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/arm/boot/dts/armv7-m.dtsi b/arch/arm/boot/dts/armv7-m.dtsi
> index b1ad7cf..8126bec 100644
> --- a/arch/arm/boot/dts/armv7-m.dtsi
> +++ b/arch/arm/boot/dts/armv7-m.dtsi
> @@ -1,7 +1,7 @@
> #include "skeleton.dtsi"
>
> / {
> - nvic: nv-interrupt-controller {
> + nvic: nv-interrupt-controller@0xe000e100 {
While changing the line it might be good idea to use the standard
'interrupt-controller' name instead.
I posted the same patch couple of days ago, btw.
http://marc.info/?l=devicetree=145929088915714=2
But I don't care which one that is applied.
regards,
Joachim Eastwood
Re: [PATCH] ARM: dts: s3c: Fix DTC unit name warnings in S3C2416 and S3C6410
On 1 April 2016 at 09:11, Krzysztof Kozlowski <k.kozlow...@samsung.com> wrote:
> Fix following DTC warnings in S3C2416 and S3C6410 boards:
>
> Warning (unit_address_vs_reg): Node /memory has a reg or ranges property, but
> no unit name
>
> Signed-off-by: Krzysztof Kozlowski <k.kozlow...@samsung.com>
> ---
> arch/arm/boot/dts/s3c2416-smdk2416.dts | 2 +-
> arch/arm/boot/dts/s3c6410-mini6410.dts | 2 +-
> arch/arm/boot/dts/s3c6410-smdk6410.dts | 2 +-
> 3 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/arch/arm/boot/dts/s3c2416-smdk2416.dts
> b/arch/arm/boot/dts/s3c2416-smdk2416.dts
> index f257926c13b7..15f3d1c1bb80 100644
> --- a/arch/arm/boot/dts/s3c2416-smdk2416.dts
> +++ b/arch/arm/boot/dts/s3c2416-smdk2416.dts
> @@ -15,7 +15,7 @@
> model = "SMDK2416";
> compatible = "samsung,s3c2416";
>
> - memory {
> + memory@3000 {
> reg = <0x3000 0x400>;
> };
This change will cause duplicated memory node entries in the DT as
noted by Vladimir Zapolskiy [1]. Same goes for all the other patch
where you make this change.
$ scripts/dtc/dtc -I dtb -O dts arch/arm/boot/dts/s3c2416-smdk2416.dtb
| grep -A2 memory
memory {
device_type = "memory";
reg = <0x0 0x0>;
};
--
memory@3000 {
reg = <0x3000 0x400>;
};
regards,
Joachim Eastwood
[1] http://marc.info/?l=linux-arm-kernel=145933287125938=2
Re: [PATCH] ARM: dts: s3c: Fix DTC unit name warnings in S3C2416 and S3C6410
On 1 April 2016 at 09:11, Krzysztof Kozlowski wrote:
> Fix following DTC warnings in S3C2416 and S3C6410 boards:
>
> Warning (unit_address_vs_reg): Node /memory has a reg or ranges property, but
> no unit name
>
> Signed-off-by: Krzysztof Kozlowski
> ---
> arch/arm/boot/dts/s3c2416-smdk2416.dts | 2 +-
> arch/arm/boot/dts/s3c6410-mini6410.dts | 2 +-
> arch/arm/boot/dts/s3c6410-smdk6410.dts | 2 +-
> 3 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/arch/arm/boot/dts/s3c2416-smdk2416.dts
> b/arch/arm/boot/dts/s3c2416-smdk2416.dts
> index f257926c13b7..15f3d1c1bb80 100644
> --- a/arch/arm/boot/dts/s3c2416-smdk2416.dts
> +++ b/arch/arm/boot/dts/s3c2416-smdk2416.dts
> @@ -15,7 +15,7 @@
> model = "SMDK2416";
> compatible = "samsung,s3c2416";
>
> - memory {
> + memory@3000 {
> reg = <0x3000 0x400>;
> };
This change will cause duplicated memory node entries in the DT as
noted by Vladimir Zapolskiy [1]. Same goes for all the other patch
where you make this change.
$ scripts/dtc/dtc -I dtb -O dts arch/arm/boot/dts/s3c2416-smdk2416.dtb
| grep -A2 memory
memory {
device_type = "memory";
reg = <0x0 0x0>;
};
--
memory@3000 {
reg = <0x3000 0x400>;
};
regards,
Joachim Eastwood
[1] http://marc.info/?l=linux-arm-kernel=145933287125938=2
Re: [PATCH v5] iio: potentiometer: add driver for Microchip MCP413X/414X/415X/416X/423X/424X/425X/426X
On 28 March 2016 at 16:58, Jonathan Cameron <ji...@kernel.org> wrote: > On 23/03/16 08:57, Slawomir Stepien wrote: >> The following functionalities are supported: >> - write, read from volatile memory >> >> Datasheet: http://ww1.microchip.com/downloads/en/DeviceDoc/22060b.pdf >> >> Signed-off-by: Slawomir Stepien <s...@poczta.fm> > One process comment below... And git actually copes fine with what you > did (I wondered which way it would go until I tried it :) > > Applied to the togreg branch of iio.git - initially pushed out as > testing for the autobuilders to play pingpong with it. > > A very nice, clean driver. Thanks for your hard work on this. > > Note it is in a branch I happy to rebase for the next week, so if Joachim > in particular would like to add a reviewed by tag, I'd be delighted to append > it. Often reviewers don't get enough credit in my opinion! Sure; Reviewed-by: Joachim Eastwood <manab...@gmail.com> regards, Joachim Eastwood
Re: [PATCH v5] iio: potentiometer: add driver for Microchip MCP413X/414X/415X/416X/423X/424X/425X/426X
On 28 March 2016 at 16:58, Jonathan Cameron wrote: > On 23/03/16 08:57, Slawomir Stepien wrote: >> The following functionalities are supported: >> - write, read from volatile memory >> >> Datasheet: http://ww1.microchip.com/downloads/en/DeviceDoc/22060b.pdf >> >> Signed-off-by: Slawomir Stepien > One process comment below... And git actually copes fine with what you > did (I wondered which way it would go until I tried it :) > > Applied to the togreg branch of iio.git - initially pushed out as > testing for the autobuilders to play pingpong with it. > > A very nice, clean driver. Thanks for your hard work on this. > > Note it is in a branch I happy to rebase for the next week, so if Joachim > in particular would like to add a reviewed by tag, I'd be delighted to append > it. Often reviewers don't get enough credit in my opinion! Sure; Reviewed-by: Joachim Eastwood regards, Joachim Eastwood
Re: [PATCH v4] iio: potentiometer: add driver for Microchip MCP413X/414X/415X/416X/423X/424X/425X/426X
Hi Slawomir,
On 22 March 2016 at 16:44, Slawomir Stepien <s...@poczta.fm> wrote:
> The following functionalities are supported:
> - write, read from volatile memory
>
> Datasheet: http://ww1.microchip.com/downloads/en/DeviceDoc/22060b.pdf
>
> Signed-off-by: Slawomir Stepien <s...@poczta.fm>
> ---
> +#include
> +#include
> +#include
> +
> +#include
Give that you use that you have a some OF stuff in your driver you
should also include . Same goes for .
I am sure this builds fine without those includes, but you should
explicitly include stuff that you use.
While you're at it you could also put the includes in alphabetic order.
> +static int mcp4131_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int *val, int *val2, long mask)
> +{
> + int err;
> + struct mcp4131_data *data = iio_priv(indio_dev);
> + int address = chan->channel;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
> + mutex_lock(>lock);
> +
> + data->buf[0] = (address << MCP4131_WIPER_SHIFT) |
> MCP4131_READ;
> + data->buf[1] = 0;
> +
> + err = mcp4131_read(data->spi, data->buf, 2);
> + if (err) {
> + mutex_unlock(>lock);
> + return err;
> + }
> +
> + /* Error, bad address/command combination */
> + if (!MCP4131_CMDERR(data->buf))
> + return -EIO;
Missing mutex unlock here.
> +
> + *val = MCP4131_RAW(data->buf);
> + mutex_unlock(>lock);
> +
> + return IIO_VAL_INT;
> +
> + case IIO_CHAN_INFO_SCALE:
> + *val = 1000 * data->cfg->kohms;
> + *val2 = data->cfg->max_pos;
> + return IIO_VAL_FRACTIONAL;
> + }
> +
> + return -EINVAL;
> +}
> +
> +static int mcp4131_write_raw(struct iio_dev *indio_dev,
> +struct iio_chan_spec const *chan,
> +int val, int val2, long mask)
> +{
> + int err;
> + struct mcp4131_data *data = iio_priv(indio_dev);
> + int address = chan->channel << MCP4131_WIPER_SHIFT;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
> + if (val > data->cfg->max_pos || val < 0)
> + return -EINVAL;
> + break;
> +
> + default:
> + return -EINVAL;
> + }
> +
> + mutex_lock(>lock);
> +
> + data->buf[0] = address << MCP4131_WIPER_SHIFT;
> + data->buf[0] |= MCP4131_WRITE | (val >> 8);
> + data->buf[1] = val & 0xFF; /* 8 bits here */
> +
> + err = spi_write(data->spi, data->buf, 2);
> + if (err) {
> + mutex_unlock(>lock);
> + return err;
> + }
> + mutex_unlock(>lock);
> +
> + return 0;
This last part could be written as:
err = spi_write(data->spi, data->buf, 2);
mutex_unlock(>lock);
return err;
Other than the things I noted driver looks good to me.
regards,
Joachim Eastwood
Re: [PATCH v4] iio: potentiometer: add driver for Microchip MCP413X/414X/415X/416X/423X/424X/425X/426X
Hi Slawomir,
On 22 March 2016 at 16:44, Slawomir Stepien wrote:
> The following functionalities are supported:
> - write, read from volatile memory
>
> Datasheet: http://ww1.microchip.com/downloads/en/DeviceDoc/22060b.pdf
>
> Signed-off-by: Slawomir Stepien
> ---
> +#include
> +#include
> +#include
> +
> +#include
Give that you use that you have a some OF stuff in your driver you
should also include . Same goes for .
I am sure this builds fine without those includes, but you should
explicitly include stuff that you use.
While you're at it you could also put the includes in alphabetic order.
> +static int mcp4131_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int *val, int *val2, long mask)
> +{
> + int err;
> + struct mcp4131_data *data = iio_priv(indio_dev);
> + int address = chan->channel;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
> + mutex_lock(>lock);
> +
> + data->buf[0] = (address << MCP4131_WIPER_SHIFT) |
> MCP4131_READ;
> + data->buf[1] = 0;
> +
> + err = mcp4131_read(data->spi, data->buf, 2);
> + if (err) {
> + mutex_unlock(>lock);
> + return err;
> + }
> +
> + /* Error, bad address/command combination */
> + if (!MCP4131_CMDERR(data->buf))
> + return -EIO;
Missing mutex unlock here.
> +
> + *val = MCP4131_RAW(data->buf);
> + mutex_unlock(>lock);
> +
> + return IIO_VAL_INT;
> +
> + case IIO_CHAN_INFO_SCALE:
> + *val = 1000 * data->cfg->kohms;
> + *val2 = data->cfg->max_pos;
> + return IIO_VAL_FRACTIONAL;
> + }
> +
> + return -EINVAL;
> +}
> +
> +static int mcp4131_write_raw(struct iio_dev *indio_dev,
> +struct iio_chan_spec const *chan,
> +int val, int val2, long mask)
> +{
> + int err;
> + struct mcp4131_data *data = iio_priv(indio_dev);
> + int address = chan->channel << MCP4131_WIPER_SHIFT;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
> + if (val > data->cfg->max_pos || val < 0)
> + return -EINVAL;
> + break;
> +
> + default:
> + return -EINVAL;
> + }
> +
> + mutex_lock(>lock);
> +
> + data->buf[0] = address << MCP4131_WIPER_SHIFT;
> + data->buf[0] |= MCP4131_WRITE | (val >> 8);
> + data->buf[1] = val & 0xFF; /* 8 bits here */
> +
> + err = spi_write(data->spi, data->buf, 2);
> + if (err) {
> + mutex_unlock(>lock);
> + return err;
> + }
> + mutex_unlock(>lock);
> +
> + return 0;
This last part could be written as:
err = spi_write(data->spi, data->buf, 2);
mutex_unlock(>lock);
return err;
Other than the things I noted driver looks good to me.
regards,
Joachim Eastwood
Re: [PATCH v5 2/4] Documentation: Bindings: Add STM32 DWMAC glue
On 21 March 2016 at 13:40, Rob Herring <r...@kernel.org> wrote: > On Sat, Mar 19, 2016 at 12:00:22AM +0800, Chen-Yu Tsai wrote: >> Hi, >> >> On Fri, Mar 18, 2016 at 11:37 PM, Alexandre TORGUE >> <alexandre.tor...@gmail.com> wrote: >> > +- clocks: Must contain a phandle for each entry in clock-names. >> > +- clock-names: Should be "stmmaceth" for the host clock. > > This doesn't sound like the clock input signal name... > >> > + Should be "tx-clk" for the MAC TX clock. >> > + Should be "rx-clk" for the MAC RX clock. > > How can other DWMAC blocks not have these clocks? The glue can't really > add these clocks. It could combine them into one or a new version of > DWMAC could have a different number of clock inputs. So if there is > variation here, then some of the bindings are probably wrong. I guess > the only change I'm suggesting is possibly moving these into common > binding doc. The LPC18xx implementation probably have these clocks as well but the LPC1850 user manual only documents the main clock. Someone with access to the IP block doc from Synopsys should be able to check which clocks the MAC really needs. Rockchip bindings have two clocks named "mac_clk_rx" and "mac_clk_tx". These are probably the same as stm32 needs so maybe use these names and move them into the main doc and update the rockchip binding. regards, Joachim Eastwood
Re: [PATCH v5 2/4] Documentation: Bindings: Add STM32 DWMAC glue
On 21 March 2016 at 13:40, Rob Herring wrote: > On Sat, Mar 19, 2016 at 12:00:22AM +0800, Chen-Yu Tsai wrote: >> Hi, >> >> On Fri, Mar 18, 2016 at 11:37 PM, Alexandre TORGUE >> wrote: >> > +- clocks: Must contain a phandle for each entry in clock-names. >> > +- clock-names: Should be "stmmaceth" for the host clock. > > This doesn't sound like the clock input signal name... > >> > + Should be "tx-clk" for the MAC TX clock. >> > + Should be "rx-clk" for the MAC RX clock. > > How can other DWMAC blocks not have these clocks? The glue can't really > add these clocks. It could combine them into one or a new version of > DWMAC could have a different number of clock inputs. So if there is > variation here, then some of the bindings are probably wrong. I guess > the only change I'm suggesting is possibly moving these into common > binding doc. The LPC18xx implementation probably have these clocks as well but the LPC1850 user manual only documents the main clock. Someone with access to the IP block doc from Synopsys should be able to check which clocks the MAC really needs. Rockchip bindings have two clocks named "mac_clk_rx" and "mac_clk_tx". These are probably the same as stm32 needs so maybe use these names and move them into the main doc and update the rockchip binding. regards, Joachim Eastwood
Re: [PATCH v5 2/4] Documentation: Bindings: Add STM32 DWMAC glue
Hi Chen-Yu, On 21 March 2016 at 12:08, Chen-Yu Tsai <w...@csie.org> wrote: > On Mon, Mar 21, 2016 at 6:45 PM, Alexandre Torgue > <alexandre.tor...@gmail.com> wrote: >> Hi, >> >> 2016-03-18 17:00 GMT+01:00 Chen-Yu Tsai <w...@csie.org>: >>> Hi, >>> >>> On Fri, Mar 18, 2016 at 11:37 PM, Alexandre TORGUE >>> <alexandre.tor...@gmail.com> wrote: >>>> Signed-off-by: Alexandre TORGUE <alexandre.tor...@gmail.com> >>>> >>>> diff --git a/Documentation/devicetree/bindings/net/stm32-dwmac.txt >>>> b/Documentation/devicetree/bindings/net/stm32-dwmac.txt >>>> new file mode 100644 >>>> index 000..ada2aa4 >>>> --- /dev/null >>>> +++ b/Documentation/devicetree/bindings/net/stm32-dwmac.txt >>>> @@ -0,0 +1,32 @@ >>>> +STMicroelectronics STM32 / MCU DWMAC glue layer controller >>>> + >>>> +This file documents platform glue layer for stmmac. >>>> +Please see stmmac.txt for the other unchanged properties. >>>> + >>>> +The device node has following properties. >>>> + >>>> +Required properties: >>>> +- compatible: Should be "st,stm32-dwmac" to select glue, and >>>> + "snps,dwmac-3.50a" to select IP vesrion. s/vesrion/version >>> >>> If you need have sort of hardware glue, then it is not compatible. >>> >> >> We could have the case where the glue is set by a bootloader. >> In this case, we will select IP version in compatible and we will use >> generic dwmac glue to probe stmmac driver. > > It seems most platforms using DWMAC follow this design set by > the original stmmac bindings. I'm arguing that the requirement > of setting up the glue makes them incompatible. > > What happens when the bootloader didn't setup the glue? And one > forgets to build the STM32 driver, only the generic one? The > generic driver even matches to some, but not all, version > strings. > > Maybe it would've been better if the versioned strings were > only used to indicate functionality, and not used to bind > the drivers. But the bindings were set some time ago. Since Alexandre has not added "snps,dwmac-3.50a" to dwmac-generic doesn't he use it as you suggest here? Note that we can not remove all the generic compatible strings from dwmac-generic because there is one platform that depend on one of them. (see arch/arm/boot/dts/exynos5440.dtsi:190) So we can not remove "snps,dwmac-3.70a" from the dwmac-generic driver if we want to keep backwards compatibility with exynos5440. But I guess we could remove the others if we want to. regards, Joachim Eastwood
Re: [PATCH v5 2/4] Documentation: Bindings: Add STM32 DWMAC glue
Hi Chen-Yu, On 21 March 2016 at 12:08, Chen-Yu Tsai wrote: > On Mon, Mar 21, 2016 at 6:45 PM, Alexandre Torgue > wrote: >> Hi, >> >> 2016-03-18 17:00 GMT+01:00 Chen-Yu Tsai : >>> Hi, >>> >>> On Fri, Mar 18, 2016 at 11:37 PM, Alexandre TORGUE >>> wrote: >>>> Signed-off-by: Alexandre TORGUE >>>> >>>> diff --git a/Documentation/devicetree/bindings/net/stm32-dwmac.txt >>>> b/Documentation/devicetree/bindings/net/stm32-dwmac.txt >>>> new file mode 100644 >>>> index 000..ada2aa4 >>>> --- /dev/null >>>> +++ b/Documentation/devicetree/bindings/net/stm32-dwmac.txt >>>> @@ -0,0 +1,32 @@ >>>> +STMicroelectronics STM32 / MCU DWMAC glue layer controller >>>> + >>>> +This file documents platform glue layer for stmmac. >>>> +Please see stmmac.txt for the other unchanged properties. >>>> + >>>> +The device node has following properties. >>>> + >>>> +Required properties: >>>> +- compatible: Should be "st,stm32-dwmac" to select glue, and >>>> + "snps,dwmac-3.50a" to select IP vesrion. s/vesrion/version >>> >>> If you need have sort of hardware glue, then it is not compatible. >>> >> >> We could have the case where the glue is set by a bootloader. >> In this case, we will select IP version in compatible and we will use >> generic dwmac glue to probe stmmac driver. > > It seems most platforms using DWMAC follow this design set by > the original stmmac bindings. I'm arguing that the requirement > of setting up the glue makes them incompatible. > > What happens when the bootloader didn't setup the glue? And one > forgets to build the STM32 driver, only the generic one? The > generic driver even matches to some, but not all, version > strings. > > Maybe it would've been better if the versioned strings were > only used to indicate functionality, and not used to bind > the drivers. But the bindings were set some time ago. Since Alexandre has not added "snps,dwmac-3.50a" to dwmac-generic doesn't he use it as you suggest here? Note that we can not remove all the generic compatible strings from dwmac-generic because there is one platform that depend on one of them. (see arch/arm/boot/dts/exynos5440.dtsi:190) So we can not remove "snps,dwmac-3.70a" from the dwmac-generic driver if we want to keep backwards compatibility with exynos5440. But I guess we could remove the others if we want to. regards, Joachim Eastwood
Re: [PATCH v3] iio: add driver for Microchip MCP413X/414X/415X/416X/423X/424X/425X/426X
Hi Jonathan,
On 20 March 2016 at 18:25, Jonathan Cameron <ji...@kernel.org> wrote:
> On 20/03/16 16:12, Joachim Eastwood wrote:
>>> +static int mcp4131_exec(struct mcp4131_data *data,
>>> + u8 addr, u8 cmd,
>>> + u16 val)
>>> +{
>>> + int err;
>>> + struct spi_device *spi = data->spi;
>>> +
>>> + data->xfer.tx_buf = data->buf;
>>> + data->xfer.rx_buf = data->buf;
>>> +
>>> + switch (cmd) {
>>> + case MCP4131_READ:
>>> + data->xfer.len = 2; /* Two bytes transfer for this command
>>> */
>>> + data->buf[0] = (addr << MCP4131_WIPER_SHIFT) | MCP4131_READ;
>>> + data->buf[1] = 0;
>>> + break;
>>> +
>>> + case MCP4131_WRITE:
>>> + data->xfer.len = 2;
>>> + data->buf[0] = (addr << MCP4131_WIPER_SHIFT) |
>>> + MCP4131_WRITE | (val >> 8);
>>> + data->buf[1] = val & 0xFF; /* 8 bits here */
>>> + break;
>>> +
>>> + default:
>>> + return -EINVAL;
>>> + }
>>> +
>>> + dev_dbg(>dev, "mcp4131_exec: tx0: 0x%x tx1: 0x%x\n",
>>> + data->buf[0], data->buf[1]);
>>> +
>>> + spi_message_init(>msg);
>>> + spi_message_add_tail(>xfer, >msg);
>>> +
>>> + err = spi_sync(spi, >msg);
>>> + if (err) {
>>> + dev_err(>dev, "spi_sync(): %d\n", err);
>>> + return err;
>>> + }
>>
>> Isn't this init, add, sync sequence basically open coding of what
>> spi_write/spi_read does?
>> If you could use those you could also get rid transfer/message structs
>> in priv data.
> I initially wrote the same comment, then realised it's more nuanced than
> that. Whilst this initially looks like an w8r8 type cycle it's actually
> something like w4r12 in the read case anyway. The write case could indeed
> be done with spi_write.
Indeed. I didn't notice that for the read case.
The read case could almost be copy of spi_read, though. One would only
need to add ".tx_buf = buf" when setting up the transfer struct, I
think. Having it in its a own function with a comment would make it
easier to spot the difference.
regards,
Joachim Eastwood
Re: [PATCH v3] iio: add driver for Microchip MCP413X/414X/415X/416X/423X/424X/425X/426X
Hi Jonathan,
On 20 March 2016 at 18:25, Jonathan Cameron wrote:
> On 20/03/16 16:12, Joachim Eastwood wrote:
>>> +static int mcp4131_exec(struct mcp4131_data *data,
>>> + u8 addr, u8 cmd,
>>> + u16 val)
>>> +{
>>> + int err;
>>> + struct spi_device *spi = data->spi;
>>> +
>>> + data->xfer.tx_buf = data->buf;
>>> + data->xfer.rx_buf = data->buf;
>>> +
>>> + switch (cmd) {
>>> + case MCP4131_READ:
>>> + data->xfer.len = 2; /* Two bytes transfer for this command
>>> */
>>> + data->buf[0] = (addr << MCP4131_WIPER_SHIFT) | MCP4131_READ;
>>> + data->buf[1] = 0;
>>> + break;
>>> +
>>> + case MCP4131_WRITE:
>>> + data->xfer.len = 2;
>>> + data->buf[0] = (addr << MCP4131_WIPER_SHIFT) |
>>> + MCP4131_WRITE | (val >> 8);
>>> + data->buf[1] = val & 0xFF; /* 8 bits here */
>>> + break;
>>> +
>>> + default:
>>> + return -EINVAL;
>>> + }
>>> +
>>> + dev_dbg(>dev, "mcp4131_exec: tx0: 0x%x tx1: 0x%x\n",
>>> + data->buf[0], data->buf[1]);
>>> +
>>> + spi_message_init(>msg);
>>> + spi_message_add_tail(>xfer, >msg);
>>> +
>>> + err = spi_sync(spi, >msg);
>>> + if (err) {
>>> + dev_err(>dev, "spi_sync(): %d\n", err);
>>> + return err;
>>> + }
>>
>> Isn't this init, add, sync sequence basically open coding of what
>> spi_write/spi_read does?
>> If you could use those you could also get rid transfer/message structs
>> in priv data.
> I initially wrote the same comment, then realised it's more nuanced than
> that. Whilst this initially looks like an w8r8 type cycle it's actually
> something like w4r12 in the read case anyway. The write case could indeed
> be done with spi_write.
Indeed. I didn't notice that for the read case.
The read case could almost be copy of spi_read, though. One would only
need to add ".tx_buf = buf" when setting up the transfer struct, I
think. Having it in its a own function with a comment would make it
easier to spot the difference.
regards,
Joachim Eastwood
Re: [PATCH v3] iio: add driver for Microchip MCP413X/414X/415X/416X/423X/424X/425X/426X
return -EINVAL;
> +}
> +
> +static int mcp4131_write_raw(struct iio_dev *indio_dev,
> +struct iio_chan_spec const *chan,
> +int val, int val2, long mask)
> +{
> + int err;
> + struct mcp4131_data *data = iio_priv(indio_dev);
> + int address = chan->channel << MCP4131_WIPER_SHIFT;
> +
> + mutex_lock(>lock);
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
> + if (val > data->cfg->max_pos || val < 0) {
> + mutex_unlock(>lock);
> + return -EINVAL;
> + }
> + break;
> + default:
> + mutex_unlock(>lock);
> + return -EINVAL;
> + }
> +
> + err = mcp4131_exec(data, address, MCP4131_WRITE, val);
> + mutex_unlock(>lock);
While this is not a huge function it is usually good practice to keep
the locking scope as small as possible.
So wouldn't this be sufficient here.
mutex_lock(>lock);
err = mcp4131_exec(data, address, MCP4131_WRITE, val);
mutex_unlock(>lock);
Of course if you are able move the lock into mcp4131_exec this will go away.
regards,
Joachim Eastwood
Re: [PATCH v3] iio: add driver for Microchip MCP413X/414X/415X/416X/423X/424X/425X/426X
ite_raw(struct iio_dev *indio_dev,
> +struct iio_chan_spec const *chan,
> +int val, int val2, long mask)
> +{
> + int err;
> + struct mcp4131_data *data = iio_priv(indio_dev);
> + int address = chan->channel << MCP4131_WIPER_SHIFT;
> +
> + mutex_lock(>lock);
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
> + if (val > data->cfg->max_pos || val < 0) {
> + mutex_unlock(>lock);
> + return -EINVAL;
> + }
> + break;
> + default:
> + mutex_unlock(>lock);
> + return -EINVAL;
> + }
> +
> + err = mcp4131_exec(data, address, MCP4131_WRITE, val);
> + mutex_unlock(>lock);
While this is not a huge function it is usually good practice to keep
the locking scope as small as possible.
So wouldn't this be sufficient here.
mutex_lock(>lock);
err = mcp4131_exec(data, address, MCP4131_WRITE, val);
mutex_unlock(>lock);
Of course if you are able move the lock into mcp4131_exec this will go away.
regards,
Joachim Eastwood
Re: [PATCH v2] pwm: pwm-lpc18xx-sct: test clock rate to avoid division by 0
Hi Wolfram,
On 2 March 2016 at 23:57, Wolfram Sang <w...@the-dreams.de> wrote:
> From: Wolfram Sang <wsa+rene...@sang-engineering.com>
>
> The clk API may return 0 on clk_get_rate, so we should check the result before
> using it as a divisor.
>
> Signed-off-by: Wolfram Sang <wsa+rene...@sang-engineering.com>
> ---
> drivers/pwm/pwm-lpc18xx-sct.c | 5 +
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/pwm/pwm-lpc18xx-sct.c b/drivers/pwm/pwm-lpc18xx-sct.c
> index 9163085101bc94..9861fed4e67d04 100644
> --- a/drivers/pwm/pwm-lpc18xx-sct.c
> +++ b/drivers/pwm/pwm-lpc18xx-sct.c
> @@ -360,6 +360,11 @@ static int lpc18xx_pwm_probe(struct platform_device
> *pdev)
> }
>
> lpc18xx_pwm->clk_rate = clk_get_rate(lpc18xx_pwm->pwm_clk);
> + if (!lpc18xx_pwm->clk_rate) {
> + dev_err(>dev, "pwm clock has no frequency\n");
> + ret = -EINVAL;
> + goto disable_pwmclk;
> + }
Acked-by: Joachim Eastwood <manab...@gmail.com>
Thanks for fixing this.
regards,
Joachim Eastwood
Re: [PATCH v2] pwm: pwm-lpc18xx-sct: test clock rate to avoid division by 0
Hi Wolfram,
On 2 March 2016 at 23:57, Wolfram Sang wrote:
> From: Wolfram Sang
>
> The clk API may return 0 on clk_get_rate, so we should check the result before
> using it as a divisor.
>
> Signed-off-by: Wolfram Sang
> ---
> drivers/pwm/pwm-lpc18xx-sct.c | 5 +
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/pwm/pwm-lpc18xx-sct.c b/drivers/pwm/pwm-lpc18xx-sct.c
> index 9163085101bc94..9861fed4e67d04 100644
> --- a/drivers/pwm/pwm-lpc18xx-sct.c
> +++ b/drivers/pwm/pwm-lpc18xx-sct.c
> @@ -360,6 +360,11 @@ static int lpc18xx_pwm_probe(struct platform_device
> *pdev)
> }
>
> lpc18xx_pwm->clk_rate = clk_get_rate(lpc18xx_pwm->pwm_clk);
> + if (!lpc18xx_pwm->clk_rate) {
> + dev_err(>dev, "pwm clock has no frequency\n");
> + ret = -EINVAL;
> + goto disable_pwmclk;
> + }
Acked-by: Joachim Eastwood
Thanks for fixing this.
regards,
Joachim Eastwood
Re: [PATCH 4/6] pwm: pwm-lpc18xx-sct: test clock rate to avoid division by 0
Hi Wolfram,
On 2 March 2016 at 23:33, Wolfram Sang <w...@the-dreams.de> wrote:
> From: Wolfram Sang <wsa+rene...@sang-engineering.com>
>
> The clk API may return 0 on clk_get_rate, so we should check the result before
> using it as a divisor.
>
> Signed-off-by: Wolfram Sang <wsa+rene...@sang-engineering.com>
> ---
>
> Should go individually via subsystem tree.
>
> drivers/pwm/pwm-lpc18xx-sct.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/pwm/pwm-lpc18xx-sct.c b/drivers/pwm/pwm-lpc18xx-sct.c
> index 9163085101bc94..6487962c355c03 100644
> --- a/drivers/pwm/pwm-lpc18xx-sct.c
> +++ b/drivers/pwm/pwm-lpc18xx-sct.c
> @@ -360,6 +360,8 @@ static int lpc18xx_pwm_probe(struct platform_device *pdev)
> }
>
> lpc18xx_pwm->clk_rate = clk_get_rate(lpc18xx_pwm->pwm_clk);
> + if (!lpc18xx_pwm->clk_rate)
> + return -EINVAL;
This needs to be:
if (!lpc18xx_pwm->clk_rate) {
ret = -EINVAL;
goto disable_pwmclk;
}
I would also prefer an explicit check against 0 here. ie.:
'lpc18xx_pwm->clk_rate == 0'
A dev_err() message would also be nice to have.
regards,
Joachim Eastwood
Re: [PATCH 4/6] pwm: pwm-lpc18xx-sct: test clock rate to avoid division by 0
Hi Wolfram,
On 2 March 2016 at 23:33, Wolfram Sang wrote:
> From: Wolfram Sang
>
> The clk API may return 0 on clk_get_rate, so we should check the result before
> using it as a divisor.
>
> Signed-off-by: Wolfram Sang
> ---
>
> Should go individually via subsystem tree.
>
> drivers/pwm/pwm-lpc18xx-sct.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/pwm/pwm-lpc18xx-sct.c b/drivers/pwm/pwm-lpc18xx-sct.c
> index 9163085101bc94..6487962c355c03 100644
> --- a/drivers/pwm/pwm-lpc18xx-sct.c
> +++ b/drivers/pwm/pwm-lpc18xx-sct.c
> @@ -360,6 +360,8 @@ static int lpc18xx_pwm_probe(struct platform_device *pdev)
> }
>
> lpc18xx_pwm->clk_rate = clk_get_rate(lpc18xx_pwm->pwm_clk);
> + if (!lpc18xx_pwm->clk_rate)
> + return -EINVAL;
This needs to be:
if (!lpc18xx_pwm->clk_rate) {
ret = -EINVAL;
goto disable_pwmclk;
}
I would also prefer an explicit check against 0 here. ie.:
'lpc18xx_pwm->clk_rate == 0'
A dev_err() message would also be nice to have.
regards,
Joachim Eastwood
Re: [PATCH 2/2] devicetree: document NXP LPC1850 PINT irq controller binding
On 2 March 2016 at 19:13, Rob Herring <r...@kernel.org> wrote: > On Thu, Feb 25, 2016 at 11:04:47PM +0100, Joachim Eastwood wrote: >> Add binding documentation for NXP LPC1850 GPIO Pin Interrupt (PINT) >> controller. >> >> Signed-off-by: Joachim Eastwood <manab...@gmail.com> >> --- >> .../interrupt-controller/nxp,lpc1850-gpio-pint.txt | 22 >> ++ >> 1 file changed, 22 insertions(+) >> create mode 100644 >> Documentation/devicetree/bindings/interrupt-controller/nxp,lpc1850-gpio-pint.txt >> >> diff --git >> a/Documentation/devicetree/bindings/interrupt-controller/nxp,lpc1850-gpio-pint.txt >> >> b/Documentation/devicetree/bindings/interrupt-controller/nxp,lpc1850-gpio-pint.txt >> new file mode 100644 >> index ..dc43f187ebda >> --- /dev/null >> +++ >> b/Documentation/devicetree/bindings/interrupt-controller/nxp,lpc1850-gpio-pint.txt >> @@ -0,0 +1,22 @@ >> +NXP LPC18xx/43xx GPIO Pin Interrupt (PINT) controller >> + >> +Required properties: >> + >> +- compatible : should be "nxp,lpc1850-gpio-pint". >> +- reg : Specifies base physical address and size of the registers. >> +- interrupt-controller : Identifies the node as an interrupt controller >> +- #interrupt-cells : Specifies the number of cells needed to encode an >> + interrupt source. The value shall be 2. >> +- interrupts : Specifies the CPU interrupts the controller is connected to. > > How many (8?) and what's the ordering? The PINT on has 8 interrupts, but this is device dependent. Up to 32 could be supported. The order is also device dependent. The interrupts on PINT are merely mapped onto the main interrupt controller. I tried to keep the wording in the binding generic so it could support the PINT on different devices. I'll update the text with some more details. regards, Joachim Eastwood
Re: [PATCH 2/2] devicetree: document NXP LPC1850 PINT irq controller binding
On 2 March 2016 at 19:13, Rob Herring wrote: > On Thu, Feb 25, 2016 at 11:04:47PM +0100, Joachim Eastwood wrote: >> Add binding documentation for NXP LPC1850 GPIO Pin Interrupt (PINT) >> controller. >> >> Signed-off-by: Joachim Eastwood >> --- >> .../interrupt-controller/nxp,lpc1850-gpio-pint.txt | 22 >> ++ >> 1 file changed, 22 insertions(+) >> create mode 100644 >> Documentation/devicetree/bindings/interrupt-controller/nxp,lpc1850-gpio-pint.txt >> >> diff --git >> a/Documentation/devicetree/bindings/interrupt-controller/nxp,lpc1850-gpio-pint.txt >> >> b/Documentation/devicetree/bindings/interrupt-controller/nxp,lpc1850-gpio-pint.txt >> new file mode 100644 >> index ..dc43f187ebda >> --- /dev/null >> +++ >> b/Documentation/devicetree/bindings/interrupt-controller/nxp,lpc1850-gpio-pint.txt >> @@ -0,0 +1,22 @@ >> +NXP LPC18xx/43xx GPIO Pin Interrupt (PINT) controller >> + >> +Required properties: >> + >> +- compatible : should be "nxp,lpc1850-gpio-pint". >> +- reg : Specifies base physical address and size of the registers. >> +- interrupt-controller : Identifies the node as an interrupt controller >> +- #interrupt-cells : Specifies the number of cells needed to encode an >> + interrupt source. The value shall be 2. >> +- interrupts : Specifies the CPU interrupts the controller is connected to. > > How many (8?) and what's the ordering? The PINT on has 8 interrupts, but this is device dependent. Up to 32 could be supported. The order is also device dependent. The interrupts on PINT are merely mapped onto the main interrupt controller. I tried to keep the wording in the binding generic so it could support the PINT on different devices. I'll update the text with some more details. regards, Joachim Eastwood
Re: [PATCH 10/41] clk: nxp: Remove CLK_IS_ROOT
Hi Stephen, On 1 March 2016 at 19:59, Stephen Boyd <sb...@codeaurora.org> wrote: > This flag is a no-op now. Remove usage of the flag. > > Cc: Joachim Eastwood <manab...@gmail.com> > Signed-off-by: Stephen Boyd <sb...@codeaurora.org> > --- Acked-by: Joachim Eastwood <manab...@gmail.com>
Re: [PATCH 10/41] clk: nxp: Remove CLK_IS_ROOT
Hi Stephen, On 1 March 2016 at 19:59, Stephen Boyd wrote: > This flag is a no-op now. Remove usage of the flag. > > Cc: Joachim Eastwood > Signed-off-by: Stephen Boyd > --- Acked-by: Joachim Eastwood
Re: [PATCH 1/2] irqchip: add lpc18xx gpio pin interrupt driver
Hi Thomas,
On 26 February 2016 at 11:26, Thomas Gleixner <t...@linutronix.de> wrote:
> On Thu, 25 Feb 2016, Joachim Eastwood wrote:
>> +static void lpc18xx_gpio_pint_handler(struct irq_desc *desc)
>> +{
>> + struct lpc18xx_gpio_pint_chip *pint = irq_desc_get_handler_data(desc);
>> + unsigned int irq = irq_desc_get_irq(desc);
>> + int irq_no, i;
>> +
>> + /* Find the interrupt */
>> + for (i = 0; i < pint->nrirqs; i++) {
>> + if (pint->irqmap[i] == irq) {
>
> Why don't you have a reverse map for this?
Is there any thing it the irq subsystem for this that I can use?
I have now implement one based on linear_revmap in the irq domain code
and it seems to work. Slightly more code and I needed two loops in
probe. First one to determine the max virq number from the main irq
controller and then another one to create the mapping/register the irq
themselves.
Looked at radix tree also, but I think it might be overkill here.
What do you think?
>> + irq_no = irq_find_mapping(pint->domain, i);
>> + generic_handle_irq(irq_no);
>> + }
>> + }
>> +}
>> +
>> +static void lpc18xx_gpio_pint_edge_ack(struct irq_data *d)
>> +{
>> + struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
>> + u32 mask = d->mask;
>> +
>> + irq_gc_lock(gc);
>> + irq_reg_writel(gc, mask, LPC18XX_GPIO_PINT_IST);
>> + irq_gc_unlock(gc);
>> +}
>
> How is that different from irq_gc_ack_set_bit ?
No, the generic one is same. I'll fix it.
>> +static void lpc18xx_gpio_pint_edge_mask(struct irq_data *d)
>> +{
>> + struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
>> + u32 mask = d->mask;
>> +
>> + irq_gc_lock(gc);
>> + irq_reg_writel(gc, mask, LPC18XX_GPIO_PINT_CIENR);
>> + irq_reg_writel(gc, mask, LPC18XX_GPIO_PINT_CIENF);
>> + irq_gc_unlock(gc);
>> +}
>
> If you use seperate irq types, then you can use the generic chip functions and
> be done with it.
Do you mean one type for IRQ_TYPE_EDGE_RISING and one for IRQ_TYPE_EDGE_FALLING?
Will those two handle the EDGE_BOTH case too? or do I need a type for that also?
>> +static void lpc18xx_gpio_pint_edge_unmask(struct irq_data *d)
>> +{
>> + struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
>> + u32 type, mask = d->mask;
>> +
>> + irq_gc_lock(gc);
>> + type = irqd_get_trigger_type(d);
>> + if (type & IRQ_TYPE_EDGE_RISING)
>> + irq_reg_writel(gc, mask, LPC18XX_GPIO_PINT_SIENR);
>> + if (type & IRQ_TYPE_EDGE_FALLING)
>> + irq_reg_writel(gc, mask, LPC18XX_GPIO_PINT_SIENF);
>> + irq_gc_unlock(gc);
>> +}
>> +
>> +static void lpc18xx_gpio_pint_level_mask(struct irq_data *d)
>> +{
>> + struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
>> + u32 mask = d->mask;
>> +
>> + irq_gc_lock(gc);
>> + irq_reg_writel(gc, mask, LPC18XX_GPIO_PINT_CIENR);
>> + irq_gc_unlock(gc);
>> +}
>> +
>> +static void lpc18xx_gpio_pint_level_unmask(struct irq_data *d)
>> +{
>> + struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
>> + u32 mask = d->mask;
>> +
>> + irq_gc_lock(gc);
>> + irq_reg_writel(gc, mask, LPC18XX_GPIO_PINT_SIENR);
>> + irq_gc_unlock(gc);
>> +}
>
> All the callbacks can go away.
I have now replaced lpc18xx_gpio_pint_edge_ack,
lpc18xx_gpio_pint_level_mask and lpc18xx_gpio_pint_level_unmask with
the equivalent generic versions.
Thanks for taking the time to look at this, Thomas.
regards,
Joachim Eastwood
Re: [PATCH 1/2] irqchip: add lpc18xx gpio pin interrupt driver
Hi Thomas,
On 26 February 2016 at 11:26, Thomas Gleixner wrote:
> On Thu, 25 Feb 2016, Joachim Eastwood wrote:
>> +static void lpc18xx_gpio_pint_handler(struct irq_desc *desc)
>> +{
>> + struct lpc18xx_gpio_pint_chip *pint = irq_desc_get_handler_data(desc);
>> + unsigned int irq = irq_desc_get_irq(desc);
>> + int irq_no, i;
>> +
>> + /* Find the interrupt */
>> + for (i = 0; i < pint->nrirqs; i++) {
>> + if (pint->irqmap[i] == irq) {
>
> Why don't you have a reverse map for this?
Is there any thing it the irq subsystem for this that I can use?
I have now implement one based on linear_revmap in the irq domain code
and it seems to work. Slightly more code and I needed two loops in
probe. First one to determine the max virq number from the main irq
controller and then another one to create the mapping/register the irq
themselves.
Looked at radix tree also, but I think it might be overkill here.
What do you think?
>> + irq_no = irq_find_mapping(pint->domain, i);
>> + generic_handle_irq(irq_no);
>> + }
>> + }
>> +}
>> +
>> +static void lpc18xx_gpio_pint_edge_ack(struct irq_data *d)
>> +{
>> + struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
>> + u32 mask = d->mask;
>> +
>> + irq_gc_lock(gc);
>> + irq_reg_writel(gc, mask, LPC18XX_GPIO_PINT_IST);
>> + irq_gc_unlock(gc);
>> +}
>
> How is that different from irq_gc_ack_set_bit ?
No, the generic one is same. I'll fix it.
>> +static void lpc18xx_gpio_pint_edge_mask(struct irq_data *d)
>> +{
>> + struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
>> + u32 mask = d->mask;
>> +
>> + irq_gc_lock(gc);
>> + irq_reg_writel(gc, mask, LPC18XX_GPIO_PINT_CIENR);
>> + irq_reg_writel(gc, mask, LPC18XX_GPIO_PINT_CIENF);
>> + irq_gc_unlock(gc);
>> +}
>
> If you use seperate irq types, then you can use the generic chip functions and
> be done with it.
Do you mean one type for IRQ_TYPE_EDGE_RISING and one for IRQ_TYPE_EDGE_FALLING?
Will those two handle the EDGE_BOTH case too? or do I need a type for that also?
>> +static void lpc18xx_gpio_pint_edge_unmask(struct irq_data *d)
>> +{
>> + struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
>> + u32 type, mask = d->mask;
>> +
>> + irq_gc_lock(gc);
>> + type = irqd_get_trigger_type(d);
>> + if (type & IRQ_TYPE_EDGE_RISING)
>> + irq_reg_writel(gc, mask, LPC18XX_GPIO_PINT_SIENR);
>> + if (type & IRQ_TYPE_EDGE_FALLING)
>> + irq_reg_writel(gc, mask, LPC18XX_GPIO_PINT_SIENF);
>> + irq_gc_unlock(gc);
>> +}
>> +
>> +static void lpc18xx_gpio_pint_level_mask(struct irq_data *d)
>> +{
>> + struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
>> + u32 mask = d->mask;
>> +
>> + irq_gc_lock(gc);
>> + irq_reg_writel(gc, mask, LPC18XX_GPIO_PINT_CIENR);
>> + irq_gc_unlock(gc);
>> +}
>> +
>> +static void lpc18xx_gpio_pint_level_unmask(struct irq_data *d)
>> +{
>> + struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
>> + u32 mask = d->mask;
>> +
>> + irq_gc_lock(gc);
>> + irq_reg_writel(gc, mask, LPC18XX_GPIO_PINT_SIENR);
>> + irq_gc_unlock(gc);
>> +}
>
> All the callbacks can go away.
I have now replaced lpc18xx_gpio_pint_edge_ack,
lpc18xx_gpio_pint_level_mask and lpc18xx_gpio_pint_level_unmask with
the equivalent generic versions.
Thanks for taking the time to look at this, Thomas.
regards,
Joachim Eastwood
Re: [PATCH v3 1/4] net: ethernet: dwmac: add Ethernet glue logic for stm32 chip
Hi Alexandre, When people comment on your patch please CC them on the next version. On 26 February 2016 at 11:51, Alexandre TORGUE <alexandre.tor...@gmail.com> wrote: > stm324xx family chips support Synopsys MAC 3.510 IP. > This patch adds settings for logical glue logic: > -clocks > -mode selection MII or RMII. > > Signed-off-by: Alexandre TORGUE <alexandre.tor...@gmail.com> Driver looks good now, thanks. Reviewed-by: Joachim Eastwood <manab...@gmail.com> regards, Joachim Eastwood
Re: [PATCH v3 1/4] net: ethernet: dwmac: add Ethernet glue logic for stm32 chip
Hi Alexandre, When people comment on your patch please CC them on the next version. On 26 February 2016 at 11:51, Alexandre TORGUE wrote: > stm324xx family chips support Synopsys MAC 3.510 IP. > This patch adds settings for logical glue logic: > -clocks > -mode selection MII or RMII. > > Signed-off-by: Alexandre TORGUE Driver looks good now, thanks. Reviewed-by: Joachim Eastwood regards, Joachim Eastwood
Re: [PATCH v3 2/4] Documentation: Bindings: Add STM32 DWMAC glue
Hi Alexandre,
On 26 February 2016 at 11:51, Alexandre TORGUE
<alexandre.tor...@gmail.com> wrote:
> Signed-off-by: Alexandre TORGUE <alexandre.tor...@gmail.com>
>
> diff --git a/Documentation/devicetree/bindings/net/stm32-dwmac.txt
> b/Documentation/devicetree/bindings/net/stm32-dwmac.txt
> new file mode 100644
> index 000..67fceda
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/stm32-dwmac.txt
> @@ -0,0 +1,40 @@
> +STMicroelectronics STM32 / MCU DWMAC glue layer controller
> +
> +This file documents platform glue layer for stmmac.
> +Please see stmmac.txt for the other unchanged properties.
> +
> +The device node has following properties.
> +
> +Required properties:
> +- compatible: Should be "st,stm32-dwmac" to select glue, and
> + "snps,dwmac-3.50a" to select IP vesrion.
> +- clocks: Should contain the GMAC main clock, and tx clock
> +- compatible: Should be "st,stm32-dwmac" to select glue and
> + "snps,dwmac-3.50a" to select IP version.
> +- clocks: Should contain the MAC main clock
> +- clock-names: Should contain the clock names "stmmaceth".
> +- st,syscon : Should be phandle/offset pair. The phandle to the syscon node
> which
> + encompases the glue register, and the offset of the control
> register.
> +
> +Optional properties:
> +- clocks: Could contain:
> + - the tx clock,
> + - the rx clock
> +- clock-names: Could contain the clock names "tx-clk", "rx-clk"
> +
> +Example:
> +
> + ethernet0: dwmac@40028000 {
> + compatible = "st,stm32-dwmac", "snps,dwmac-3.50a";
> + status = "disabled";
> + reg = <0x40028000 0x8000>;
> + reg-names = "stmmaceth";
> + interrupts = <0 61 0>, <0 62 0>;
> + interrupt-names = "macirq", "eth_wake_irq";
> + clock-names = "stmmaceth", "tx-clk", "rx-clk";
> + clocks = < 0 25>, < 0 26>, < 0 27>;
> + st,syscon = < 0x4>;
> + snps,pbl = <8>;
> + snps,mixed-burst;
> + dma-ranges;
> + };
Looks just like any other dwmac-driver binding so:
Acked-by: Joachim Eastwood <manab...@gmail.com>
regards,
Joachim Eastwood
Re: [PATCH v3 2/4] Documentation: Bindings: Add STM32 DWMAC glue
Hi Alexandre,
On 26 February 2016 at 11:51, Alexandre TORGUE
wrote:
> Signed-off-by: Alexandre TORGUE
>
> diff --git a/Documentation/devicetree/bindings/net/stm32-dwmac.txt
> b/Documentation/devicetree/bindings/net/stm32-dwmac.txt
> new file mode 100644
> index 000..67fceda
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/stm32-dwmac.txt
> @@ -0,0 +1,40 @@
> +STMicroelectronics STM32 / MCU DWMAC glue layer controller
> +
> +This file documents platform glue layer for stmmac.
> +Please see stmmac.txt for the other unchanged properties.
> +
> +The device node has following properties.
> +
> +Required properties:
> +- compatible: Should be "st,stm32-dwmac" to select glue, and
> + "snps,dwmac-3.50a" to select IP vesrion.
> +- clocks: Should contain the GMAC main clock, and tx clock
> +- compatible: Should be "st,stm32-dwmac" to select glue and
> + "snps,dwmac-3.50a" to select IP version.
> +- clocks: Should contain the MAC main clock
> +- clock-names: Should contain the clock names "stmmaceth".
> +- st,syscon : Should be phandle/offset pair. The phandle to the syscon node
> which
> + encompases the glue register, and the offset of the control
> register.
> +
> +Optional properties:
> +- clocks: Could contain:
> + - the tx clock,
> + - the rx clock
> +- clock-names: Could contain the clock names "tx-clk", "rx-clk"
> +
> +Example:
> +
> + ethernet0: dwmac@40028000 {
> + compatible = "st,stm32-dwmac", "snps,dwmac-3.50a";
> + status = "disabled";
> + reg = <0x40028000 0x8000>;
> + reg-names = "stmmaceth";
> + interrupts = <0 61 0>, <0 62 0>;
> + interrupt-names = "macirq", "eth_wake_irq";
> + clock-names = "stmmaceth", "tx-clk", "rx-clk";
> + clocks = < 0 25>, < 0 26>, < 0 27>;
> + st,syscon = < 0x4>;
> + snps,pbl = <8>;
> + snps,mixed-burst;
> + dma-ranges;
> + };
Looks just like any other dwmac-driver binding so:
Acked-by: Joachim Eastwood
regards,
Joachim Eastwood
[PATCH 0/2] PINT irqchip driver for NXP LPC18xx family
The LPC1850 has no less than 3 GPIO interrupt blocks. One of these blocks is called 'gpio pin interrupt' or just PINT. LPC1850 PINT can handle up to 8 interrupts and these have a one-to-one relationship with the main interrupt controller (NVIC). The interrupts on PINT can be either level or edge trigger and supports any polarity. This patch set adds an irqchip driver for the PINT found on lpc18xx. Joachim Eastwood (2): irqchip: add lpc18xx gpio pin interrupt driver devicetree: document NXP LPC1850 PINT irq controller binding .../interrupt-controller/nxp,lpc1850-gpio-pint.txt | 22 ++ drivers/irqchip/Kconfig| 5 + drivers/irqchip/Makefile | 1 + drivers/irqchip/irq-lpc18xx-gpio-pint.c| 238 + 4 files changed, 266 insertions(+) create mode 100644 Documentation/devicetree/bindings/interrupt-controller/nxp,lpc1850-gpio-pint.txt create mode 100644 drivers/irqchip/irq-lpc18xx-gpio-pint.c -- 1.8.0
[PATCH 0/2] PINT irqchip driver for NXP LPC18xx family
The LPC1850 has no less than 3 GPIO interrupt blocks. One of these blocks is called 'gpio pin interrupt' or just PINT. LPC1850 PINT can handle up to 8 interrupts and these have a one-to-one relationship with the main interrupt controller (NVIC). The interrupts on PINT can be either level or edge trigger and supports any polarity. This patch set adds an irqchip driver for the PINT found on lpc18xx. Joachim Eastwood (2): irqchip: add lpc18xx gpio pin interrupt driver devicetree: document NXP LPC1850 PINT irq controller binding .../interrupt-controller/nxp,lpc1850-gpio-pint.txt | 22 ++ drivers/irqchip/Kconfig| 5 + drivers/irqchip/Makefile | 1 + drivers/irqchip/irq-lpc18xx-gpio-pint.c| 238 + 4 files changed, 266 insertions(+) create mode 100644 Documentation/devicetree/bindings/interrupt-controller/nxp,lpc1850-gpio-pint.txt create mode 100644 drivers/irqchip/irq-lpc18xx-gpio-pint.c -- 1.8.0
[PATCH 2/2] devicetree: document NXP LPC1850 PINT irq controller binding
Add binding documentation for NXP LPC1850 GPIO Pin Interrupt (PINT)
controller.
Signed-off-by: Joachim Eastwood <manab...@gmail.com>
---
.../interrupt-controller/nxp,lpc1850-gpio-pint.txt | 22 ++
1 file changed, 22 insertions(+)
create mode 100644
Documentation/devicetree/bindings/interrupt-controller/nxp,lpc1850-gpio-pint.txt
diff --git
a/Documentation/devicetree/bindings/interrupt-controller/nxp,lpc1850-gpio-pint.txt
b/Documentation/devicetree/bindings/interrupt-controller/nxp,lpc1850-gpio-pint.txt
new file mode 100644
index ..dc43f187ebda
--- /dev/null
+++
b/Documentation/devicetree/bindings/interrupt-controller/nxp,lpc1850-gpio-pint.txt
@@ -0,0 +1,22 @@
+NXP LPC18xx/43xx GPIO Pin Interrupt (PINT) controller
+
+Required properties:
+
+- compatible : should be "nxp,lpc1850-gpio-pint".
+- reg : Specifies base physical address and size of the registers.
+- interrupt-controller : Identifies the node as an interrupt controller
+- #interrupt-cells : Specifies the number of cells needed to encode an
+ interrupt source. The value shall be 2.
+- interrupts : Specifies the CPU interrupts the controller is connected to.
+- clocks: Must contain a reference to the functional clock.
+
+Example:
+
+pint: interrupt-controller@40087000 {
+ compatible = "nxp,lpc1850-gpio-pint";
+ reg = <0x40087000 0x1000>;
+ interrupt-controller;
+ #interrupt-cells = <2>;
+ interrupts = <32 33 34 35 36 37 38 39>;
+ clocks = < CLK_CPU_GPIO>;
+};
--
1.8.0
[PATCH 2/2] devicetree: document NXP LPC1850 PINT irq controller binding
Add binding documentation for NXP LPC1850 GPIO Pin Interrupt (PINT)
controller.
Signed-off-by: Joachim Eastwood
---
.../interrupt-controller/nxp,lpc1850-gpio-pint.txt | 22 ++
1 file changed, 22 insertions(+)
create mode 100644
Documentation/devicetree/bindings/interrupt-controller/nxp,lpc1850-gpio-pint.txt
diff --git
a/Documentation/devicetree/bindings/interrupt-controller/nxp,lpc1850-gpio-pint.txt
b/Documentation/devicetree/bindings/interrupt-controller/nxp,lpc1850-gpio-pint.txt
new file mode 100644
index ..dc43f187ebda
--- /dev/null
+++
b/Documentation/devicetree/bindings/interrupt-controller/nxp,lpc1850-gpio-pint.txt
@@ -0,0 +1,22 @@
+NXP LPC18xx/43xx GPIO Pin Interrupt (PINT) controller
+
+Required properties:
+
+- compatible : should be "nxp,lpc1850-gpio-pint".
+- reg : Specifies base physical address and size of the registers.
+- interrupt-controller : Identifies the node as an interrupt controller
+- #interrupt-cells : Specifies the number of cells needed to encode an
+ interrupt source. The value shall be 2.
+- interrupts : Specifies the CPU interrupts the controller is connected to.
+- clocks: Must contain a reference to the functional clock.
+
+Example:
+
+pint: interrupt-controller@40087000 {
+ compatible = "nxp,lpc1850-gpio-pint";
+ reg = <0x40087000 0x1000>;
+ interrupt-controller;
+ #interrupt-cells = <2>;
+ interrupts = <32 33 34 35 36 37 38 39>;
+ clocks = < CLK_CPU_GPIO>;
+};
--
1.8.0
[PATCH 1/2] irqchip: add lpc18xx gpio pin interrupt driver
NXP LPC18xx has an interrupt controller called 'gpio pin interrupt'
or just PINT. The PINT can handle up to 8 interrupts and these have
a one-to-one relationship with the main interrupt controller (NVIC).
The interrupts on PINT can be either level or edge trigger and
supports any polarity.
Selection of which GPIOs that are connected to the PINT is done by
the lpc18xx pinctrl driver (SCU).
Signed-off-by: Joachim Eastwood <manab...@gmail.com>
---
drivers/irqchip/Kconfig | 5 +
drivers/irqchip/Makefile| 1 +
drivers/irqchip/irq-lpc18xx-gpio-pint.c | 238
3 files changed, 244 insertions(+)
create mode 100644 drivers/irqchip/irq-lpc18xx-gpio-pint.c
diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index 715923d5236c..4d26b2e82e0c 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -190,6 +190,11 @@ config KEYSTONE_IRQ
Support for Texas Instruments Keystone 2 IRQ controller IP which
is part of the Keystone 2 IPC mechanism
+config LPC18XX_GPIO_PINT
+ bool
+ select IRQ_DOMAIN
+ select GENERIC_IRQ_CHIP
+
config MIPS_GIC
bool
select MIPS_CM
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index 18caacb60d58..ae6e05e4347d 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -50,6 +50,7 @@ obj-$(CONFIG_BCM7038_L1_IRQ) += irq-bcm7038-l1.o
obj-$(CONFIG_BCM7120_L2_IRQ) += irq-bcm7120-l2.o
obj-$(CONFIG_BRCMSTB_L2_IRQ) += irq-brcmstb-l2.o
obj-$(CONFIG_KEYSTONE_IRQ) += irq-keystone.o
+obj-$(CONFIG_LPC18XX_GPIO_PINT)+= irq-lpc18xx-gpio-pint.o
obj-$(CONFIG_MIPS_GIC) += irq-mips-gic.o
obj-$(CONFIG_ARCH_MEDIATEK)+= irq-mtk-sysirq.o
obj-$(CONFIG_ARCH_DIGICOLOR) += irq-digicolor.o
diff --git a/drivers/irqchip/irq-lpc18xx-gpio-pint.c
b/drivers/irqchip/irq-lpc18xx-gpio-pint.c
new file mode 100644
index ..4dc791681016
--- /dev/null
+++ b/drivers/irqchip/irq-lpc18xx-gpio-pint.c
@@ -0,0 +1,238 @@
+/*
+ * Irqchip driver for GPIO Pin Interrupt (PINT) on NXP LPC18xx/43xx.
+ *
+ * Copyright (C) 2015 Joachim Eastwood <manab...@gmail.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.
+ *
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+/* LPC18xx GPIO pin interrupt register offsets */
+#define LPC18XX_GPIO_PINT_ISEL 0x000
+#define LPC18XX_GPIO_PINT_SIENR0x008
+#define LPC18XX_GPIO_PINT_CIENR0x00c
+#define LPC18XX_GPIO_PINT_SIENF0x014
+#define LPC18XX_GPIO_PINT_CIENF0x018
+#define LPC18XX_GPIO_PINT_IST 0x024
+
+struct lpc18xx_gpio_pint_chip {
+ struct irq_domain *domain;
+ void __iomem *base;
+ struct clk*clk;
+ int *irqmap;
+ int nrirqs;
+};
+
+static void lpc18xx_gpio_pint_handler(struct irq_desc *desc)
+{
+ struct lpc18xx_gpio_pint_chip *pint = irq_desc_get_handler_data(desc);
+ unsigned int irq = irq_desc_get_irq(desc);
+ int irq_no, i;
+
+ /* Find the interrupt */
+ for (i = 0; i < pint->nrirqs; i++) {
+ if (pint->irqmap[i] == irq) {
+ irq_no = irq_find_mapping(pint->domain, i);
+ generic_handle_irq(irq_no);
+ }
+ }
+}
+
+static void lpc18xx_gpio_pint_edge_ack(struct irq_data *d)
+{
+ struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
+ u32 mask = d->mask;
+
+ irq_gc_lock(gc);
+ irq_reg_writel(gc, mask, LPC18XX_GPIO_PINT_IST);
+ irq_gc_unlock(gc);
+}
+
+static void lpc18xx_gpio_pint_edge_mask(struct irq_data *d)
+{
+ struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
+ u32 mask = d->mask;
+
+ irq_gc_lock(gc);
+ irq_reg_writel(gc, mask, LPC18XX_GPIO_PINT_CIENR);
+ irq_reg_writel(gc, mask, LPC18XX_GPIO_PINT_CIENF);
+ irq_gc_unlock(gc);
+}
+
+static void lpc18xx_gpio_pint_edge_unmask(struct irq_data *d)
+{
+ struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
+ u32 type, mask = d->mask;
+
+ irq_gc_lock(gc);
+ type = irqd_get_trigger_type(d);
+ if (type & IRQ_TYPE_EDGE_RISING)
+ irq_reg_writel(gc, mask, LPC18XX_GPIO_PINT_SIENR);
+ if (type & IRQ_TYPE_EDGE_FALLING)
+ irq_reg_writel(gc, mask, LPC18XX_GPIO_PINT_SIENF);
+ irq_gc_unlock(gc);
+}
+
+static void lpc18xx_gpio_pint_level_mask(struct irq_data *d)
+{
+ struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
+ u32 mask = d->mask;
+
+ irq_gc_lock(gc);
+ irq_reg_writel(gc, mask, LPC18XX_GPIO_PINT_CIE
[PATCH 1/2] irqchip: add lpc18xx gpio pin interrupt driver
NXP LPC18xx has an interrupt controller called 'gpio pin interrupt'
or just PINT. The PINT can handle up to 8 interrupts and these have
a one-to-one relationship with the main interrupt controller (NVIC).
The interrupts on PINT can be either level or edge trigger and
supports any polarity.
Selection of which GPIOs that are connected to the PINT is done by
the lpc18xx pinctrl driver (SCU).
Signed-off-by: Joachim Eastwood
---
drivers/irqchip/Kconfig | 5 +
drivers/irqchip/Makefile| 1 +
drivers/irqchip/irq-lpc18xx-gpio-pint.c | 238
3 files changed, 244 insertions(+)
create mode 100644 drivers/irqchip/irq-lpc18xx-gpio-pint.c
diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index 715923d5236c..4d26b2e82e0c 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -190,6 +190,11 @@ config KEYSTONE_IRQ
Support for Texas Instruments Keystone 2 IRQ controller IP which
is part of the Keystone 2 IPC mechanism
+config LPC18XX_GPIO_PINT
+ bool
+ select IRQ_DOMAIN
+ select GENERIC_IRQ_CHIP
+
config MIPS_GIC
bool
select MIPS_CM
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index 18caacb60d58..ae6e05e4347d 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -50,6 +50,7 @@ obj-$(CONFIG_BCM7038_L1_IRQ) += irq-bcm7038-l1.o
obj-$(CONFIG_BCM7120_L2_IRQ) += irq-bcm7120-l2.o
obj-$(CONFIG_BRCMSTB_L2_IRQ) += irq-brcmstb-l2.o
obj-$(CONFIG_KEYSTONE_IRQ) += irq-keystone.o
+obj-$(CONFIG_LPC18XX_GPIO_PINT)+= irq-lpc18xx-gpio-pint.o
obj-$(CONFIG_MIPS_GIC) += irq-mips-gic.o
obj-$(CONFIG_ARCH_MEDIATEK)+= irq-mtk-sysirq.o
obj-$(CONFIG_ARCH_DIGICOLOR) += irq-digicolor.o
diff --git a/drivers/irqchip/irq-lpc18xx-gpio-pint.c
b/drivers/irqchip/irq-lpc18xx-gpio-pint.c
new file mode 100644
index ..4dc791681016
--- /dev/null
+++ b/drivers/irqchip/irq-lpc18xx-gpio-pint.c
@@ -0,0 +1,238 @@
+/*
+ * Irqchip driver for GPIO Pin Interrupt (PINT) on NXP LPC18xx/43xx.
+ *
+ * Copyright (C) 2015 Joachim Eastwood
+ *
+ * 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.
+ *
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+/* LPC18xx GPIO pin interrupt register offsets */
+#define LPC18XX_GPIO_PINT_ISEL 0x000
+#define LPC18XX_GPIO_PINT_SIENR0x008
+#define LPC18XX_GPIO_PINT_CIENR0x00c
+#define LPC18XX_GPIO_PINT_SIENF0x014
+#define LPC18XX_GPIO_PINT_CIENF0x018
+#define LPC18XX_GPIO_PINT_IST 0x024
+
+struct lpc18xx_gpio_pint_chip {
+ struct irq_domain *domain;
+ void __iomem *base;
+ struct clk*clk;
+ int *irqmap;
+ int nrirqs;
+};
+
+static void lpc18xx_gpio_pint_handler(struct irq_desc *desc)
+{
+ struct lpc18xx_gpio_pint_chip *pint = irq_desc_get_handler_data(desc);
+ unsigned int irq = irq_desc_get_irq(desc);
+ int irq_no, i;
+
+ /* Find the interrupt */
+ for (i = 0; i < pint->nrirqs; i++) {
+ if (pint->irqmap[i] == irq) {
+ irq_no = irq_find_mapping(pint->domain, i);
+ generic_handle_irq(irq_no);
+ }
+ }
+}
+
+static void lpc18xx_gpio_pint_edge_ack(struct irq_data *d)
+{
+ struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
+ u32 mask = d->mask;
+
+ irq_gc_lock(gc);
+ irq_reg_writel(gc, mask, LPC18XX_GPIO_PINT_IST);
+ irq_gc_unlock(gc);
+}
+
+static void lpc18xx_gpio_pint_edge_mask(struct irq_data *d)
+{
+ struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
+ u32 mask = d->mask;
+
+ irq_gc_lock(gc);
+ irq_reg_writel(gc, mask, LPC18XX_GPIO_PINT_CIENR);
+ irq_reg_writel(gc, mask, LPC18XX_GPIO_PINT_CIENF);
+ irq_gc_unlock(gc);
+}
+
+static void lpc18xx_gpio_pint_edge_unmask(struct irq_data *d)
+{
+ struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
+ u32 type, mask = d->mask;
+
+ irq_gc_lock(gc);
+ type = irqd_get_trigger_type(d);
+ if (type & IRQ_TYPE_EDGE_RISING)
+ irq_reg_writel(gc, mask, LPC18XX_GPIO_PINT_SIENR);
+ if (type & IRQ_TYPE_EDGE_FALLING)
+ irq_reg_writel(gc, mask, LPC18XX_GPIO_PINT_SIENF);
+ irq_gc_unlock(gc);
+}
+
+static void lpc18xx_gpio_pint_level_mask(struct irq_data *d)
+{
+ struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
+ u32 mask = d->mask;
+
+ irq_gc_lock(gc);
+ irq_reg_writel(gc, mask, LPC18XX_GPIO_PINT_CIENR);
+ irq_gc_unlock(gc);
+}
+
+static voi
Re: [PATCH] pinctrl: lpc18xx: ensure ngroups is initialized at correct place
Hi Colin,
On 24 February 2016 at 18:32, Colin King <colin.k...@canonical.com> wrote:
> From: Colin Ian King <colin.k...@canonical.com>
>
> The initialization of ngroups is occurring at the end of the
> first iteration of the outer loop, which means that the
> assignment pins[ngroups++] = i is potentially indexing into
> a region outside of array pins because ngroups is not initialized.
> Instead, initialize ngroups in the inner loop before the first
> inner loop iteration.
>
> Signed-off-by: Colin Ian King <colin.k...@canonical.com>
> ---
> drivers/pinctrl/pinctrl-lpc18xx.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/pinctrl/pinctrl-lpc18xx.c
> b/drivers/pinctrl/pinctrl-lpc18xx.c
> index f0bebbe..ed1cfa7 100644
> --- a/drivers/pinctrl/pinctrl-lpc18xx.c
> +++ b/drivers/pinctrl/pinctrl-lpc18xx.c
> @@ -1170,9 +1170,8 @@ static int lpc18xx_create_group_func_map(struct device
> *dev,
> u16 pins[ARRAY_SIZE(lpc18xx_pins)];
> int func, ngroups, i;
>
> - for (func = 0; func < FUNC_MAX; ngroups = 0, func++) {
> -
> - for (i = 0; i < ARRAY_SIZE(lpc18xx_pins); i++) {
> + for (func = 0; func < FUNC_MAX; func++) {
> + for (ngroups = 0, i = 0; i < ARRAY_SIZE(lpc18xx_pins); i++) {
Good catch!
Reviewed-by: Joachim Eastwood <manab...@gmail.com>
regards,
Joachim Eastwood
Re: [PATCH] pinctrl: lpc18xx: ensure ngroups is initialized at correct place
Hi Colin,
On 24 February 2016 at 18:32, Colin King wrote:
> From: Colin Ian King
>
> The initialization of ngroups is occurring at the end of the
> first iteration of the outer loop, which means that the
> assignment pins[ngroups++] = i is potentially indexing into
> a region outside of array pins because ngroups is not initialized.
> Instead, initialize ngroups in the inner loop before the first
> inner loop iteration.
>
> Signed-off-by: Colin Ian King
> ---
> drivers/pinctrl/pinctrl-lpc18xx.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/pinctrl/pinctrl-lpc18xx.c
> b/drivers/pinctrl/pinctrl-lpc18xx.c
> index f0bebbe..ed1cfa7 100644
> --- a/drivers/pinctrl/pinctrl-lpc18xx.c
> +++ b/drivers/pinctrl/pinctrl-lpc18xx.c
> @@ -1170,9 +1170,8 @@ static int lpc18xx_create_group_func_map(struct device
> *dev,
> u16 pins[ARRAY_SIZE(lpc18xx_pins)];
> int func, ngroups, i;
>
> - for (func = 0; func < FUNC_MAX; ngroups = 0, func++) {
> -
> - for (i = 0; i < ARRAY_SIZE(lpc18xx_pins); i++) {
> + for (func = 0; func < FUNC_MAX; func++) {
> + for (ngroups = 0, i = 0; i < ARRAY_SIZE(lpc18xx_pins); i++) {
Good catch!
Reviewed-by: Joachim Eastwood
regards,
Joachim Eastwood
Re: [PATCH 40/50] pinctrl: lpc18xx: Use devm_pinctrl_register() for pinctrl registration
Hi Laxman, On 24 February 2016 at 14:16, Laxman Dewangan <ldewan...@nvidia.com> wrote: > Use devm_pinctrl_register() for pin control registration. > > Signed-off-by: Laxman Dewangan <ldewan...@nvidia.com> > Cc: Joachim Eastwood <manab...@gmail.com> > --- > drivers/pinctrl/pinctrl-lpc18xx.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) Acked-by: Joachim Eastwood <manab...@gmail.com> regards, Joachim Eastwood
Re: [PATCH 40/50] pinctrl: lpc18xx: Use devm_pinctrl_register() for pinctrl registration
Hi Laxman, On 24 February 2016 at 14:16, Laxman Dewangan wrote: > Use devm_pinctrl_register() for pin control registration. > > Signed-off-by: Laxman Dewangan > Cc: Joachim Eastwood > --- > drivers/pinctrl/pinctrl-lpc18xx.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) Acked-by: Joachim Eastwood regards, Joachim Eastwood
Re: [PATCH v2 2/4] Documentation: Bindings: Add STM32 DWMAC glue
Hi Alexandre,
You should copy 'devicet...@vger.kernel.org' on bindings doc. Adding cc here.
On 23 February 2016 at 16:10, Alexandre TORGUE
<alexandre.tor...@gmail.com> wrote:
> Signed-off-by: Alexandre TORGUE <alexandre.tor...@gmail.com>
>
> diff --git a/Documentation/devicetree/bindings/net/stm32-dwmac.txt
> b/Documentation/devicetree/bindings/net/stm32-dwmac.txt
> new file mode 100644
> index 000..18734b3
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/stm32-dwmac.txt
> @@ -0,0 +1,41 @@
> +STMicroelectronics STM32 / MCU DWMAC glue layer controller
> +
> +This file documents platform glue layer for stmmac.
> +Please see stmmac.txt for the other unchanged properties.
> +
> +The device node has following properties.
> +
> +Required properties:
> +- compatible: Should be "st,stm32-dwmac" to select glue, and
> + "snps,dwmac-3.50a" to select IP vesrion.
> +- clocks: Should contain the GMAC main clock, and tx clock
> +- compatible: Should be "st,stm32-dwmac" to select glue and
> + "snps,dwmac-3.50a" to select IP version.
> +- clocks: Should contain the MAC main clock
> +- clock-names: Should contain the clock names "stmmaceth".
> +- st,syscon : Should be phandle/offset pair. The phandle to the syscon node
> which
> + encompases the glue register, and the offset of the control
> register.
> +
> +Optional properties:
> +- clocks: Could contain:
> + - the tx clock,
> + - the rx clock
> +- clock-names: Could contain the clock names "tx-clk", "rx-clk"
> +
> +Example:
> +
> + ethernet0: dwmac@40028000 {
> + device_type = "network";
What is this 'device_type = "network"' for?
It seems to used in a lot of powerpc DTs, but only a couple of arm DTs.
Maybe Rob could enlighten us?
> + compatible = "st,stm32-dwmac", "snps,dwmac-3.50a";
> + status = "disabled";
> + reg = <0x40028000 0x8000>;
> + reg-names = "stmmaceth";
> + interrupts = <0 61 0>, <0 62 0>;
> + interrupt-names = "macirq", "eth_wake_irq";
> + clock-names = "stmmaceth", "tx-clk", "rx-clk";
> + clocks = < 0 25>, < 0 26>, < 0 27>;
> + st,syscon = < 0x4>;
> + snps,pbl = <32>;
Regarding snps,pbl; using 32 here might not give you what you would except.
See comment in dwmac1000_dma_init().
The driver is hard coded to use PBL4X/PBL8X mode. Just a heads up.
regards,
Joachim Eastwood
Re: [PATCH v2 2/4] Documentation: Bindings: Add STM32 DWMAC glue
Hi Alexandre,
You should copy 'devicet...@vger.kernel.org' on bindings doc. Adding cc here.
On 23 February 2016 at 16:10, Alexandre TORGUE
wrote:
> Signed-off-by: Alexandre TORGUE
>
> diff --git a/Documentation/devicetree/bindings/net/stm32-dwmac.txt
> b/Documentation/devicetree/bindings/net/stm32-dwmac.txt
> new file mode 100644
> index 000..18734b3
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/stm32-dwmac.txt
> @@ -0,0 +1,41 @@
> +STMicroelectronics STM32 / MCU DWMAC glue layer controller
> +
> +This file documents platform glue layer for stmmac.
> +Please see stmmac.txt for the other unchanged properties.
> +
> +The device node has following properties.
> +
> +Required properties:
> +- compatible: Should be "st,stm32-dwmac" to select glue, and
> + "snps,dwmac-3.50a" to select IP vesrion.
> +- clocks: Should contain the GMAC main clock, and tx clock
> +- compatible: Should be "st,stm32-dwmac" to select glue and
> + "snps,dwmac-3.50a" to select IP version.
> +- clocks: Should contain the MAC main clock
> +- clock-names: Should contain the clock names "stmmaceth".
> +- st,syscon : Should be phandle/offset pair. The phandle to the syscon node
> which
> + encompases the glue register, and the offset of the control
> register.
> +
> +Optional properties:
> +- clocks: Could contain:
> + - the tx clock,
> + - the rx clock
> +- clock-names: Could contain the clock names "tx-clk", "rx-clk"
> +
> +Example:
> +
> + ethernet0: dwmac@40028000 {
> + device_type = "network";
What is this 'device_type = "network"' for?
It seems to used in a lot of powerpc DTs, but only a couple of arm DTs.
Maybe Rob could enlighten us?
> + compatible = "st,stm32-dwmac", "snps,dwmac-3.50a";
> + status = "disabled";
> + reg = <0x40028000 0x8000>;
> + reg-names = "stmmaceth";
> + interrupts = <0 61 0>, <0 62 0>;
> + interrupt-names = "macirq", "eth_wake_irq";
> + clock-names = "stmmaceth", "tx-clk", "rx-clk";
> + clocks = < 0 25>, < 0 26>, < 0 27>;
> + st,syscon = < 0x4>;
> + snps,pbl = <32>;
Regarding snps,pbl; using 32 here might not give you what you would except.
See comment in dwmac1000_dma_init().
The driver is hard coded to use PBL4X/PBL8X mode. Just a heads up.
regards,
Joachim Eastwood
Re: [PATCH v2 1/4] net: ethernet: dwmac: add Ethernet glue logic for stm32 chip
;clk_tx)) {
> + dev_warn(dev, "No tx clock provided...\n");
> + dwmac->clk_tx = NULL;
> + }
> + dwmac->clk_rx = devm_clk_get(dev, "rx-clk");
> + if (IS_ERR(dwmac->clk_rx)) {
> + dev_warn(dev, "No rx clock provided...\n");
> + dwmac->clk_rx = NULL;
> + }
> +
> + /* Get mode register */
> + regmap = syscon_regmap_lookup_by_phandle(np, "st,syscon");
> + if (IS_ERR(regmap))
> + return PTR_ERR(regmap);
> +
> + err = of_property_read_u32_index(np, "st,syscon", 1,
> >mode_reg);
> + if (err) {
> + dev_err(dev, "Can't get sysconfig mode offset (%d)\n", err);
> + return err;
> + }
> +
> + dwmac->interface = of_get_phy_mode(np);
> + dwmac->regmap = regmap;
Why the temporary local regmap variable?
Assigning dwmac->regmap with syscon_regmap_lookup_by_phandle() should
not exceed 80 chars if that is what you are worried about.
> + return 0;
> +}
> +
> +static int stm32_dwmac_probe(struct platform_device *pdev)
> +{
> + struct plat_stmmacenet_data *plat_dat;
> + struct stmmac_resources stmmac_res;
> + struct stm32_dwmac *dwmac;
> + int ret;
> +
> + ret = stmmac_get_platform_resources(pdev, _res);
> + if (ret)
> + return ret;
> +
> + plat_dat = stmmac_probe_config_dt(pdev, _res.mac);
> + if (IS_ERR(plat_dat))
> + return PTR_ERR(plat_dat);
> +
> + dwmac = devm_kzalloc(>dev, sizeof(*dwmac), GFP_KERNEL);
> + if (!dwmac)
> + return -ENOMEM;
> +
> + ret = stm32_dwmac_parse_data(dwmac, pdev);
> + if (ret) {
> + dev_err(>dev, "Unable to parse OF data\n");
> + return ret;
> + }
> +
> + plat_dat->bsp_priv = dwmac;
> +
> + ret = stm32_dwmac_init(plat_dat->bsp_priv);
> + if (ret)
> + return ret;
> +
> + return stmmac_dvr_probe(>dev, plat_dat, _res);
Note that stmmac_dvr_probe() can fail and if so you should disable
your tx/rx clks before you return.
Consider putting the clk_prepare_enable() directly here and use goto
labels for the clean up like most other drivers do in probe.
Also if you put regmap_update_bits() for phy mode above the
clk_prepare_enable() calls you remove one of the gotos.
I assume you don't need to enable tx/rx clock before you write to syscon.
> +static int stm32_dwmac_remove(struct platform_device *pdev)
> +{
> + struct net_device *ndev = platform_get_drvdata(pdev);
> + struct stmmac_priv *priv = netdev_priv(ndev);
> + int ret = stmmac_dvr_remove(ndev);
> +
> + stm32_dwmac_exit(priv->plat->bsp_priv);
> +
> + return ret;
> +}
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int stm32_dwmac_suspend(struct device *dev)
> +{
> + struct net_device *ndev = dev_get_drvdata(dev);
> + struct stmmac_priv *priv = netdev_priv(ndev);
> + int ret;
> +
> + ret = stmmac_suspend(ndev);
> + stm32_dwmac_exit(priv->plat->bsp_priv);
> +
> + return ret;
> +}
> +
> +static int stm32_dwmac_resume(struct device *dev)
> +{
> + struct net_device *ndev = dev_get_drvdata(dev);
> + struct stmmac_priv *priv = netdev_priv(ndev);
> + int ret;
> +
> + ret = stm32_dwmac_init(priv->plat->bsp_priv);
> + if (ret)
> + goto out_regmap;
> +
> + ret = stmmac_resume(ndev);
> +
> +out_regmap:
> + return ret;
Why the goto?
This could be written:
ret = stm32_dwmac_init(priv->plat->bsp_priv);
if (ret)
return ret;
return stmmac_resume(ndev);
regards,
Joachim Eastwood
Re: [PATCH v2 1/4] net: ethernet: dwmac: add Ethernet glue logic for stm32 chip
;
> + dwmac->clk_tx = NULL;
> + }
> + dwmac->clk_rx = devm_clk_get(dev, "rx-clk");
> + if (IS_ERR(dwmac->clk_rx)) {
> + dev_warn(dev, "No rx clock provided...\n");
> + dwmac->clk_rx = NULL;
> + }
> +
> + /* Get mode register */
> + regmap = syscon_regmap_lookup_by_phandle(np, "st,syscon");
> + if (IS_ERR(regmap))
> + return PTR_ERR(regmap);
> +
> + err = of_property_read_u32_index(np, "st,syscon", 1,
> >mode_reg);
> + if (err) {
> + dev_err(dev, "Can't get sysconfig mode offset (%d)\n", err);
> + return err;
> + }
> +
> + dwmac->interface = of_get_phy_mode(np);
> + dwmac->regmap = regmap;
Why the temporary local regmap variable?
Assigning dwmac->regmap with syscon_regmap_lookup_by_phandle() should
not exceed 80 chars if that is what you are worried about.
> + return 0;
> +}
> +
> +static int stm32_dwmac_probe(struct platform_device *pdev)
> +{
> + struct plat_stmmacenet_data *plat_dat;
> + struct stmmac_resources stmmac_res;
> + struct stm32_dwmac *dwmac;
> + int ret;
> +
> + ret = stmmac_get_platform_resources(pdev, _res);
> + if (ret)
> + return ret;
> +
> + plat_dat = stmmac_probe_config_dt(pdev, _res.mac);
> + if (IS_ERR(plat_dat))
> + return PTR_ERR(plat_dat);
> +
> + dwmac = devm_kzalloc(>dev, sizeof(*dwmac), GFP_KERNEL);
> + if (!dwmac)
> + return -ENOMEM;
> +
> + ret = stm32_dwmac_parse_data(dwmac, pdev);
> + if (ret) {
> + dev_err(>dev, "Unable to parse OF data\n");
> + return ret;
> + }
> +
> + plat_dat->bsp_priv = dwmac;
> +
> + ret = stm32_dwmac_init(plat_dat->bsp_priv);
> + if (ret)
> + return ret;
> +
> + return stmmac_dvr_probe(>dev, plat_dat, _res);
Note that stmmac_dvr_probe() can fail and if so you should disable
your tx/rx clks before you return.
Consider putting the clk_prepare_enable() directly here and use goto
labels for the clean up like most other drivers do in probe.
Also if you put regmap_update_bits() for phy mode above the
clk_prepare_enable() calls you remove one of the gotos.
I assume you don't need to enable tx/rx clock before you write to syscon.
> +static int stm32_dwmac_remove(struct platform_device *pdev)
> +{
> + struct net_device *ndev = platform_get_drvdata(pdev);
> + struct stmmac_priv *priv = netdev_priv(ndev);
> + int ret = stmmac_dvr_remove(ndev);
> +
> + stm32_dwmac_exit(priv->plat->bsp_priv);
> +
> + return ret;
> +}
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int stm32_dwmac_suspend(struct device *dev)
> +{
> + struct net_device *ndev = dev_get_drvdata(dev);
> + struct stmmac_priv *priv = netdev_priv(ndev);
> + int ret;
> +
> + ret = stmmac_suspend(ndev);
> + stm32_dwmac_exit(priv->plat->bsp_priv);
> +
> + return ret;
> +}
> +
> +static int stm32_dwmac_resume(struct device *dev)
> +{
> + struct net_device *ndev = dev_get_drvdata(dev);
> + struct stmmac_priv *priv = netdev_priv(ndev);
> + int ret;
> +
> + ret = stm32_dwmac_init(priv->plat->bsp_priv);
> + if (ret)
> + goto out_regmap;
> +
> + ret = stmmac_resume(ndev);
> +
> +out_regmap:
> + return ret;
Why the goto?
This could be written:
ret = stm32_dwmac_init(priv->plat->bsp_priv);
if (ret)
return ret;
return stmmac_resume(ndev);
regards,
Joachim Eastwood
Re: [PATCH 1/4] net: ethernet: dwmac: add Ethernet glue logic for stm32 chip
On 23 February 2016 at 10:59, Alexandre Torgue
<alexandre.tor...@gmail.com> wrote:
> 2016-02-22 22:52 GMT+01:00 Joachim Eastwood <manab...@gmail.com>:
>> On 22 February 2016 at 15:50, Alexandre Torgue
>> <alexandre.tor...@gmail.com> wrote:
>>> 2016-02-13 14:48 GMT+01:00 Joachim Eastwood <manab...@gmail.com>:
>>>> On 3 February 2016 at 15:54, Alexandre TORGUE
>>>> <alexandre.tor...@gmail.com> wrote:
>>>>> + plat_dat->bsp_priv = dwmac;
>>>>> + plat_dat->init = stm32_dwmac_init;
>>>>> + plat_dat->exit = stm32_dwmac_exit;
>>>>
>>>> Instead of using these callbacks could you rather implement the PM
>>>> callbacks directly in this driver?
>>>> I don't think it should add much code and it will make it look more
>>>> like standard driver. This will also give you some more control and
>>>> flexibility in your code.
>>>
>>> I prefer to keep the code as it is. Glue layer is directly linked to
>>> stmmac driver and I don't want to brake the link between the glue and
>>> the stmmac driver.
>>
>> What do you mean by break the link?
>>
>
> I thought that you wanted to split stmmac_pltfr_supend (glue part and
> stmamc part), but I well understood it is not the case (sorry for
> mistake).
>
>> There has been numerous of patch sets to make the stmmac "glue"
>> drivers into more standard platform drivers.
>> http://marc.info/?l=linux-netdev=143159850631093=2
>> http://marc.info/?l=linux-netdev=143708560009851=2
>> http://marc.info/?l=linux-netdev=143812136600541=2
>>
>> Do you see any advantage by using the init and exit hooks in your
>> driver instead of using the standard driver PM callbacks and remove
>> function?
>> The only "cost" I see is slightly more boilerplate code. But since you
>> already have init/exit functions you could easily make them into PM
>> resume/suspend so I doubt there would be much increase in code size.
>>
>
> If I well understood you want to continue the stmmac glue driver
> rework by moving stmmac_pltfr_suspend/resume/remove in each glue
> driver (stm32, sun, sti ).
At least I want to avoid the init/exit callbacks for new drivers like
stm32-dwmac.
> Each glue driver will call directly stmmac_suspend/resume/remove and
> their own init/exit function.
> If it is what you meant, I can do it.
Yes, in your stm32 driver's suspend/resume/remove functions call
stmmac_suspend/stmmac_resume/stmmac_dvr_remove directly. Then you
shouldn't need to use the init/exit callbacks. Just put the need code
in the driver's suspend/resume/remove functions instead of init/exit
functions.
For example:
static int stm32_dwmac_resume(struct device *dev)
{
struct net_device *ndev = dev_get_drvdata(dev);
struct plat_stmmacenet_data *plat_dat = get_stmmac_plat_data(ndev)
struct stm32_dwmac *dwmac =plat_dat->bsp_priv;
/* enable clocks */
/* set phy mode */
return stmmac_resume(ndev);
}
If it makes sense to have the enable clk/phy mode stuff in it's own
function that is fine too.
>> One other thing;
>> Do you need to have the PHY mode setup code in the init function which
>> is called each time on resume?
>
> I can't guarantee that after a suspend the sysconfig register will
> contain same data than before suspend.
I see.
regards,
Joachim Eastwood
Re: [PATCH 1/4] net: ethernet: dwmac: add Ethernet glue logic for stm32 chip
On 23 February 2016 at 10:59, Alexandre Torgue
wrote:
> 2016-02-22 22:52 GMT+01:00 Joachim Eastwood :
>> On 22 February 2016 at 15:50, Alexandre Torgue
>> wrote:
>>> 2016-02-13 14:48 GMT+01:00 Joachim Eastwood :
>>>> On 3 February 2016 at 15:54, Alexandre TORGUE
>>>> wrote:
>>>>> + plat_dat->bsp_priv = dwmac;
>>>>> + plat_dat->init = stm32_dwmac_init;
>>>>> + plat_dat->exit = stm32_dwmac_exit;
>>>>
>>>> Instead of using these callbacks could you rather implement the PM
>>>> callbacks directly in this driver?
>>>> I don't think it should add much code and it will make it look more
>>>> like standard driver. This will also give you some more control and
>>>> flexibility in your code.
>>>
>>> I prefer to keep the code as it is. Glue layer is directly linked to
>>> stmmac driver and I don't want to brake the link between the glue and
>>> the stmmac driver.
>>
>> What do you mean by break the link?
>>
>
> I thought that you wanted to split stmmac_pltfr_supend (glue part and
> stmamc part), but I well understood it is not the case (sorry for
> mistake).
>
>> There has been numerous of patch sets to make the stmmac "glue"
>> drivers into more standard platform drivers.
>> http://marc.info/?l=linux-netdev=143159850631093=2
>> http://marc.info/?l=linux-netdev=143708560009851=2
>> http://marc.info/?l=linux-netdev=143812136600541=2
>>
>> Do you see any advantage by using the init and exit hooks in your
>> driver instead of using the standard driver PM callbacks and remove
>> function?
>> The only "cost" I see is slightly more boilerplate code. But since you
>> already have init/exit functions you could easily make them into PM
>> resume/suspend so I doubt there would be much increase in code size.
>>
>
> If I well understood you want to continue the stmmac glue driver
> rework by moving stmmac_pltfr_suspend/resume/remove in each glue
> driver (stm32, sun, sti ).
At least I want to avoid the init/exit callbacks for new drivers like
stm32-dwmac.
> Each glue driver will call directly stmmac_suspend/resume/remove and
> their own init/exit function.
> If it is what you meant, I can do it.
Yes, in your stm32 driver's suspend/resume/remove functions call
stmmac_suspend/stmmac_resume/stmmac_dvr_remove directly. Then you
shouldn't need to use the init/exit callbacks. Just put the need code
in the driver's suspend/resume/remove functions instead of init/exit
functions.
For example:
static int stm32_dwmac_resume(struct device *dev)
{
struct net_device *ndev = dev_get_drvdata(dev);
struct plat_stmmacenet_data *plat_dat = get_stmmac_plat_data(ndev)
struct stm32_dwmac *dwmac =plat_dat->bsp_priv;
/* enable clocks */
/* set phy mode */
return stmmac_resume(ndev);
}
If it makes sense to have the enable clk/phy mode stuff in it's own
function that is fine too.
>> One other thing;
>> Do you need to have the PHY mode setup code in the init function which
>> is called each time on resume?
>
> I can't guarantee that after a suspend the sysconfig register will
> contain same data than before suspend.
I see.
regards,
Joachim Eastwood
Re: [PATCH v2 12/16] clk: avoid circular clock topology
On 22 February 2016 at 03:29, Masahiro Yamada <yamada.masah...@socionext.com> wrote: > Hi Joachim, > > > 2016-02-22 6:39 GMT+09:00 Joachim Eastwood <manab...@gmail.com>: >> Hi everyone, >> >> On 28 December 2015 at 11:10, Masahiro Yamada >> <yamada.masah...@socionext.com> wrote: >>> Currently, clk_register() never checks a circular parent looping, >>> but clock providers could register such an insane clock topology. >>> For example, "clk_a" could have "clk_b" as a parent, and vice versa. >>> In this case, clk_core_reparent() creates a circular parent list >>> and __clk_recalc_accuracies() calls itself recursively forever. >>> >>> The core infrastructure should be kind enough to bail out, showing >>> an appropriate error message in such a case. This helps to easily >>> find a bug in clock providers. (uh, I made such a silly mistake >>> when I was implementing my clock providers first. I was upset >>> because the kernel did not respond, without any error message.) >>> >>> This commit adds a new helper function, __clk_is_ancestor(). It >>> returns true if the second argument is a possible ancestor of the >>> first one. If a clock core is a possible ancestor of itself, it >>> would make a loop when it were registered. That should be detected >>> as an error. >> >> This commit breaks lpc18xx boot in next right now. See >> http://marc.info/?l=linux-arm-kernel=145608597106087=2 >> >> The Clock Generation Unit (CGU) on lpc18xx allow for circular parents >> in hardware. While it is obliviously not a good idea to configure the >> clocks in that manner there is nothing that stops you either. >> >> Please take a look at the second figure on: >> https://github.com/manabian/linux-lpc/wiki/LPC18xx-LPC43xx-clocks >> All PLLs can feed clock into the dividers and the dividers can feed >> clock into the PLLs. >> >> The reason why this is made possible in the CGU is because you can >> then choose where to put your divider; either before the PLL or after. >> > > > Sorry for breaking your board. No worries, that is why we have next so we can catch it before it hits mainline :-) > I am OK with reverting b58f75aa83fb. > > > > I guess your hardware could make clock looping for the best flexibility > but you do not make clock looping in actual use cases. That's right. > Maybe, does it make sense to check the parent looping > in clk_set_parent() or somewhere, not in clk_register()? I think that would be a nice addition. While the CGU can certainly be configured with loops it is indeed something that we should prevent from happening. regards, Joachim Eastwood
Re: [PATCH v2 12/16] clk: avoid circular clock topology
On 22 February 2016 at 03:29, Masahiro Yamada wrote: > Hi Joachim, > > > 2016-02-22 6:39 GMT+09:00 Joachim Eastwood : >> Hi everyone, >> >> On 28 December 2015 at 11:10, Masahiro Yamada >> wrote: >>> Currently, clk_register() never checks a circular parent looping, >>> but clock providers could register such an insane clock topology. >>> For example, "clk_a" could have "clk_b" as a parent, and vice versa. >>> In this case, clk_core_reparent() creates a circular parent list >>> and __clk_recalc_accuracies() calls itself recursively forever. >>> >>> The core infrastructure should be kind enough to bail out, showing >>> an appropriate error message in such a case. This helps to easily >>> find a bug in clock providers. (uh, I made such a silly mistake >>> when I was implementing my clock providers first. I was upset >>> because the kernel did not respond, without any error message.) >>> >>> This commit adds a new helper function, __clk_is_ancestor(). It >>> returns true if the second argument is a possible ancestor of the >>> first one. If a clock core is a possible ancestor of itself, it >>> would make a loop when it were registered. That should be detected >>> as an error. >> >> This commit breaks lpc18xx boot in next right now. See >> http://marc.info/?l=linux-arm-kernel=145608597106087=2 >> >> The Clock Generation Unit (CGU) on lpc18xx allow for circular parents >> in hardware. While it is obliviously not a good idea to configure the >> clocks in that manner there is nothing that stops you either. >> >> Please take a look at the second figure on: >> https://github.com/manabian/linux-lpc/wiki/LPC18xx-LPC43xx-clocks >> All PLLs can feed clock into the dividers and the dividers can feed >> clock into the PLLs. >> >> The reason why this is made possible in the CGU is because you can >> then choose where to put your divider; either before the PLL or after. >> > > > Sorry for breaking your board. No worries, that is why we have next so we can catch it before it hits mainline :-) > I am OK with reverting b58f75aa83fb. > > > > I guess your hardware could make clock looping for the best flexibility > but you do not make clock looping in actual use cases. That's right. > Maybe, does it make sense to check the parent looping > in clk_set_parent() or somewhere, not in clk_register()? I think that would be a nice addition. While the CGU can certainly be configured with loops it is indeed something that we should prevent from happening. regards, Joachim Eastwood
Re: [PATCH 1/4] net: ethernet: dwmac: add Ethernet glue logic for stm32 chip
On 22 February 2016 at 15:50, Alexandre Torgue <alexandre.tor...@gmail.com> wrote: > 2016-02-13 14:48 GMT+01:00 Joachim Eastwood <manab...@gmail.com>: >> On 3 February 2016 at 15:54, Alexandre TORGUE >> <alexandre.tor...@gmail.com> wrote: >>> + plat_dat->bsp_priv = dwmac; >>> + plat_dat->init = stm32_dwmac_init; >>> + plat_dat->exit = stm32_dwmac_exit; >> >> Instead of using these callbacks could you rather implement the PM >> callbacks directly in this driver? >> I don't think it should add much code and it will make it look more >> like standard driver. This will also give you some more control and >> flexibility in your code. > > I prefer to keep the code as it is. Glue layer is directly linked to > stmmac driver and I don't want to brake the link between the glue and > the stmmac driver. What do you mean by break the link? There has been numerous of patch sets to make the stmmac "glue" drivers into more standard platform drivers. http://marc.info/?l=linux-netdev=143159850631093=2 http://marc.info/?l=linux-netdev=143708560009851=2 http://marc.info/?l=linux-netdev=143812136600541=2 Do you see any advantage by using the init and exit hooks in your driver instead of using the standard driver PM callbacks and remove function? The only "cost" I see is slightly more boilerplate code. But since you already have init/exit functions you could easily make them into PM resume/suspend so I doubt there would be much increase in code size. One other thing; Do you need to have the PHY mode setup code in the init function which is called each time on resume? If you could move it to probe you could drop the interface priv data member and use plat_dat->interface as stmmac_probe_config_dt() has already done of_get_phy_mode(). regards, Joachim Eastwood
Re: [PATCH 1/4] net: ethernet: dwmac: add Ethernet glue logic for stm32 chip
On 22 February 2016 at 15:50, Alexandre Torgue wrote: > 2016-02-13 14:48 GMT+01:00 Joachim Eastwood : >> On 3 February 2016 at 15:54, Alexandre TORGUE >> wrote: >>> + plat_dat->bsp_priv = dwmac; >>> + plat_dat->init = stm32_dwmac_init; >>> + plat_dat->exit = stm32_dwmac_exit; >> >> Instead of using these callbacks could you rather implement the PM >> callbacks directly in this driver? >> I don't think it should add much code and it will make it look more >> like standard driver. This will also give you some more control and >> flexibility in your code. > > I prefer to keep the code as it is. Glue layer is directly linked to > stmmac driver and I don't want to brake the link between the glue and > the stmmac driver. What do you mean by break the link? There has been numerous of patch sets to make the stmmac "glue" drivers into more standard platform drivers. http://marc.info/?l=linux-netdev=143159850631093=2 http://marc.info/?l=linux-netdev=143708560009851=2 http://marc.info/?l=linux-netdev=143812136600541=2 Do you see any advantage by using the init and exit hooks in your driver instead of using the standard driver PM callbacks and remove function? The only "cost" I see is slightly more boilerplate code. But since you already have init/exit functions you could easily make them into PM resume/suspend so I doubt there would be much increase in code size. One other thing; Do you need to have the PHY mode setup code in the init function which is called each time on resume? If you could move it to probe you could drop the interface priv data member and use plat_dat->interface as stmmac_probe_config_dt() has already done of_get_phy_mode(). regards, Joachim Eastwood
Re: [PATCH v2 12/16] clk: avoid circular clock topology
Hi everyone, On 28 December 2015 at 11:10, Masahiro Yamada <yamada.masah...@socionext.com> wrote: > Currently, clk_register() never checks a circular parent looping, > but clock providers could register such an insane clock topology. > For example, "clk_a" could have "clk_b" as a parent, and vice versa. > In this case, clk_core_reparent() creates a circular parent list > and __clk_recalc_accuracies() calls itself recursively forever. > > The core infrastructure should be kind enough to bail out, showing > an appropriate error message in such a case. This helps to easily > find a bug in clock providers. (uh, I made such a silly mistake > when I was implementing my clock providers first. I was upset > because the kernel did not respond, without any error message.) > > This commit adds a new helper function, __clk_is_ancestor(). It > returns true if the second argument is a possible ancestor of the > first one. If a clock core is a possible ancestor of itself, it > would make a loop when it were registered. That should be detected > as an error. This commit breaks lpc18xx boot in next right now. See http://marc.info/?l=linux-arm-kernel=145608597106087=2 The Clock Generation Unit (CGU) on lpc18xx allow for circular parents in hardware. While it is obliviously not a good idea to configure the clocks in that manner there is nothing that stops you either. Please take a look at the second figure on: https://github.com/manabian/linux-lpc/wiki/LPC18xx-LPC43xx-clocks All PLLs can feed clock into the dividers and the dividers can feed clock into the PLLs. The reason why this is made possible in the CGU is because you can then choose where to put your divider; either before the PLL or after. regards, Joachim Eastwood
Re: [PATCH v2 12/16] clk: avoid circular clock topology
Hi everyone, On 28 December 2015 at 11:10, Masahiro Yamada wrote: > Currently, clk_register() never checks a circular parent looping, > but clock providers could register such an insane clock topology. > For example, "clk_a" could have "clk_b" as a parent, and vice versa. > In this case, clk_core_reparent() creates a circular parent list > and __clk_recalc_accuracies() calls itself recursively forever. > > The core infrastructure should be kind enough to bail out, showing > an appropriate error message in such a case. This helps to easily > find a bug in clock providers. (uh, I made such a silly mistake > when I was implementing my clock providers first. I was upset > because the kernel did not respond, without any error message.) > > This commit adds a new helper function, __clk_is_ancestor(). It > returns true if the second argument is a possible ancestor of the > first one. If a clock core is a possible ancestor of itself, it > would make a loop when it were registered. That should be detected > as an error. This commit breaks lpc18xx boot in next right now. See http://marc.info/?l=linux-arm-kernel=145608597106087=2 The Clock Generation Unit (CGU) on lpc18xx allow for circular parents in hardware. While it is obliviously not a good idea to configure the clocks in that manner there is nothing that stops you either. Please take a look at the second figure on: https://github.com/manabian/linux-lpc/wiki/LPC18xx-LPC43xx-clocks All PLLs can feed clock into the dividers and the dividers can feed clock into the PLLs. The reason why this is made possible in the CGU is because you can then choose where to put your divider; either before the PLL or after. regards, Joachim Eastwood
Re: [PATCHv5 6/7] eeprom: 93xx46: extend driver to plug into the NVMEM framework
On 17 February 2016 at 23:11, Andrew Lunn <and...@lunn.ch> wrote:
> On Wed, Feb 17, 2016 at 11:02:56PM +0100, Joachim Eastwood wrote:
>> Hi Andrew,
>>
>> On 17 February 2016 at 21:07, Andrew Lunn <and...@lunn.ch> wrote:
>> > Add a regmap for accessing the EEPROM, and then use that with the
>> > NVMEM framework. Enable backward compatibility in the MVMEM config
>>
>> typo: MVMEM
>
> Thanks
>
>> > +/*
>> > + * Provide a regmap interface, which is registered with the NVMEM
>> > + * framework
>> > +*/
>> > +static int eeprom_93xx46_regmap_read(void *context, const void *reg,
>> > +size_t reg_size, void *val,
>> > +size_t val_size)
>> > +{
>> > + struct eeprom_93xx46_dev *eeprom_93xx46 = context;
>> > + off_t offset = *(u32 *)reg;
>> > + int err;
>> > +
>> > + err = eeprom_93xx46_read(eeprom_93xx46, val, offset, val_size);
>> > + if (err)
>> > + return err;
>> > + return 0;
>>
>> Can be:
>> return eeprom_93xx46_read(eeprom_93xx46, val, offset, val_size);
>>
>> Allows you to remove the 'err' variable also.
>
> Nope. regmap wants a return value of 0 or error. eeprom_93xx46_read()
> returns how many bytes where actually read. So we either need this
> code here, or we change the return from eeprom_93xx46_read().
ah, my bad. Sorry about that.
regards,
Joachim Eastwood
Re: [PATCHv5 6/7] eeprom: 93xx46: extend driver to plug into the NVMEM framework
On 17 February 2016 at 23:11, Andrew Lunn wrote:
> On Wed, Feb 17, 2016 at 11:02:56PM +0100, Joachim Eastwood wrote:
>> Hi Andrew,
>>
>> On 17 February 2016 at 21:07, Andrew Lunn wrote:
>> > Add a regmap for accessing the EEPROM, and then use that with the
>> > NVMEM framework. Enable backward compatibility in the MVMEM config
>>
>> typo: MVMEM
>
> Thanks
>
>> > +/*
>> > + * Provide a regmap interface, which is registered with the NVMEM
>> > + * framework
>> > +*/
>> > +static int eeprom_93xx46_regmap_read(void *context, const void *reg,
>> > +size_t reg_size, void *val,
>> > +size_t val_size)
>> > +{
>> > + struct eeprom_93xx46_dev *eeprom_93xx46 = context;
>> > + off_t offset = *(u32 *)reg;
>> > + int err;
>> > +
>> > + err = eeprom_93xx46_read(eeprom_93xx46, val, offset, val_size);
>> > + if (err)
>> > + return err;
>> > + return 0;
>>
>> Can be:
>> return eeprom_93xx46_read(eeprom_93xx46, val, offset, val_size);
>>
>> Allows you to remove the 'err' variable also.
>
> Nope. regmap wants a return value of 0 or error. eeprom_93xx46_read()
> returns how many bytes where actually read. So we either need this
> code here, or we change the return from eeprom_93xx46_read().
ah, my bad. Sorry about that.
regards,
Joachim Eastwood
Re: [PATCHv5 6/7] eeprom: 93xx46: extend driver to plug into the NVMEM framework
On 17 February 2016 at 23:02, Joachim Eastwood <manab...@gmail.com> wrote:
> Hi Andrew,
>
> On 17 February 2016 at 21:07, Andrew Lunn <and...@lunn.ch> wrote:
>> +/*
>> + * Provide a regmap interface, which is registered with the NVMEM
>> + * framework
>> +*/
>> +static int eeprom_93xx46_regmap_read(void *context, const void *reg,
>> +size_t reg_size, void *val,
>> +size_t val_size)
>> +{
>> + struct eeprom_93xx46_dev *eeprom_93xx46 = context;
>> + off_t offset = *(u32 *)reg;
>> + int err;
>> +
>> + err = eeprom_93xx46_read(eeprom_93xx46, val, offset, val_size);
>> + if (err)
>> + return err;
>> + return 0;
>
> Can be:
> return eeprom_93xx46_read(eeprom_93xx46, val, offset, val_size);
>
> Allows you to remove the 'err' variable also.
You seem to do this consistently in the other patches as well. Any
particular reason?
regards,
Joachim Eastwood
Re: [PATCHv5 6/7] eeprom: 93xx46: extend driver to plug into the NVMEM framework
On 17 February 2016 at 23:02, Joachim Eastwood wrote:
> Hi Andrew,
>
> On 17 February 2016 at 21:07, Andrew Lunn wrote:
>> +/*
>> + * Provide a regmap interface, which is registered with the NVMEM
>> + * framework
>> +*/
>> +static int eeprom_93xx46_regmap_read(void *context, const void *reg,
>> +size_t reg_size, void *val,
>> +size_t val_size)
>> +{
>> + struct eeprom_93xx46_dev *eeprom_93xx46 = context;
>> + off_t offset = *(u32 *)reg;
>> + int err;
>> +
>> + err = eeprom_93xx46_read(eeprom_93xx46, val, offset, val_size);
>> + if (err)
>> + return err;
>> + return 0;
>
> Can be:
> return eeprom_93xx46_read(eeprom_93xx46, val, offset, val_size);
>
> Allows you to remove the 'err' variable also.
You seem to do this consistently in the other patches as well. Any
particular reason?
regards,
Joachim Eastwood
Re: [PATCHv5 6/7] eeprom: 93xx46: extend driver to plug into the NVMEM framework
xt;
> + off_t offset = *(u32 *)reg;
> + int err;
> +
> + err = eeprom_93xx46_read(eeprom_93xx46, val, offset, val_size);
> + if (err)
> + return err;
> + return 0;
Can be:
return eeprom_93xx46_read(eeprom_93xx46, val, offset, val_size);
Allows you to remove the 'err' variable also.
> +}
> +
> +static int eeprom_93xx46_regmap_write(void *context, const void *data,
> + size_t count)
> +{
> + struct eeprom_93xx46_dev *eeprom_93xx46 = context;
> + const char *buf;
> + u32 offset;
> + size_t len;
> + int err;
> +
> + memcpy(, data, sizeof(offset));
> + buf = (const char *)data + sizeof(offset);
> + len = count - sizeof(offset);
> +
> + err = eeprom_93xx46_write(eeprom_93xx46, buf, offset, len);
> + if (err)
> + return err;
> + return 0;
Same here.
regards,
Joachim Eastwood
Re: [PATCHv5 6/7] eeprom: 93xx46: extend driver to plug into the NVMEM framework
err = eeprom_93xx46_read(eeprom_93xx46, val, offset, val_size);
> + if (err)
> + return err;
> + return 0;
Can be:
return eeprom_93xx46_read(eeprom_93xx46, val, offset, val_size);
Allows you to remove the 'err' variable also.
> +}
> +
> +static int eeprom_93xx46_regmap_write(void *context, const void *data,
> + size_t count)
> +{
> + struct eeprom_93xx46_dev *eeprom_93xx46 = context;
> + const char *buf;
> + u32 offset;
> + size_t len;
> + int err;
> +
> + memcpy(, data, sizeof(offset));
> + buf = (const char *)data + sizeof(offset);
> + len = count - sizeof(offset);
> +
> + err = eeprom_93xx46_write(eeprom_93xx46, buf, offset, len);
> + if (err)
> + return err;
> + return 0;
Same here.
regards,
Joachim Eastwood
Re: [PATCH 1/4] net: ethernet: dwmac: add Ethernet glue logic for stm32 chip
*pdev)
> +{
> + struct device *dev = >dev;
> + struct device_node *np = dev->of_node;
> + struct regmap *regmap;
> + int err;
> +
> + if (!np)
> + return -EINVAL;
Can this ever happen?
This is a DT only driver, right?
> +
> + /* Get TX/RX clocks */
> + dwmac->clk_tx = devm_clk_get(dev, "tx-clk");
> + if (IS_ERR(dwmac->clk_tx)) {
> + dev_warn(dev, "No tx clock provided...\n");
> + dwmac->clk_tx = NULL;
> + }
> + dwmac->clk_rx = devm_clk_get(dev, "rx-clk");
> + if (IS_ERR(dwmac->clk_rx)) {
> + dev_warn(dev, "No rx clock provided...\n");
> + dwmac->clk_rx = NULL;
> + }
> +
> + /* Get mode register */
> + regmap = syscon_regmap_lookup_by_phandle(np, "st,syscon");
> + if (IS_ERR(regmap))
> + return PTR_ERR(regmap);
> +
> + err = of_property_read_u32_index(np, "st,syscon", 1,
> >mode_reg);
> + if (err) {
> + dev_err(dev, "Can't get sysconfig mode offset (%d)\n", err);
> + return err;
> + }
> +
> + dwmac->dev = dev;
> + dwmac->interface = of_get_phy_mode(np);
> + dwmac->regmap = regmap;
> +
> + return 0;
> +}
> +
> +static int stm32_dwmac_probe(struct platform_device *pdev)
> +{
> + struct plat_stmmacenet_data *plat_dat;
> + struct stmmac_resources stmmac_res;
> + struct stm32_dwmac *dwmac;
> + int ret;
> +
> + ret = stmmac_get_platform_resources(pdev, _res);
> + if (ret)
> + return ret;
> +
> + plat_dat = stmmac_probe_config_dt(pdev, _res.mac);
> + if (IS_ERR(plat_dat))
> + return PTR_ERR(plat_dat);
> +
> + dwmac = devm_kzalloc(>dev, sizeof(*dwmac), GFP_KERNEL);
> + if (!dwmac)
> + return -ENOMEM;
> +
> + ret = stm32_dwmac_parse_data(dwmac, pdev);
> + if (ret) {
> + dev_err(>dev, "Unable to parse OF data\n");
> + return ret;
> + }
> +
> + plat_dat->bsp_priv = dwmac;
> + plat_dat->init = stm32_dwmac_init;
> + plat_dat->exit = stm32_dwmac_exit;
Instead of using these callbacks could you rather implement the PM
callbacks directly in this driver?
I don't think it should add much code and it will make it look more
like standard driver. This will also give you some more control and
flexibility in your code.
> +
> + ret = stm32_dwmac_init(pdev, plat_dat->bsp_priv);
> + if (ret)
> + return ret;
> +
> + return stmmac_dvr_probe(>dev, plat_dat, _res);
> +}
> +
> +static const struct of_device_id stm32_dwmac_match[] = {
> + { .compatible = "st,stm32-dwmac"},
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, stm32_dwmac_match);
> +
> +static struct platform_driver stm32_dwmac_driver = {
> + .probe = stm32_dwmac_probe,
> + .remove = stmmac_pltfr_remove,
Could you implement the .remove callback in your driver instead of
using stmmac_pltfr_remove?
Same reasons as above.
> + .driver = {
> + .name = "stm32-dwmac",
> + .pm = _pltfr_pm_ops,
> + .of_match_table = stm32_dwmac_match,
> + },
> +};
> +module_platform_driver(stm32_dwmac_driver);
> +
> +MODULE_AUTHOR("Alexandre Torgue ");
> +MODULE_DESCRIPTION("STMicroelectronics MCU DWMAC Specific Glue layer");
> +MODULE_LICENSE("GPL");
Since you state:
> + * License terms: GNU General Public License (GPL), version 2
You might want to switch use: MODULE_LICENSE("GPL v2");
regards,
Joachim Eastwood
Re: [PATCH 1/4] net: ethernet: dwmac: add Ethernet glue logic for stm32 chip
(struct stm32_dwmac *dwmac,
> + struct platform_device *pdev)
> +{
> + struct device *dev = >dev;
> + struct device_node *np = dev->of_node;
> + struct regmap *regmap;
> + int err;
> +
> + if (!np)
> + return -EINVAL;
Can this ever happen?
This is a DT only driver, right?
> +
> + /* Get TX/RX clocks */
> + dwmac->clk_tx = devm_clk_get(dev, "tx-clk");
> + if (IS_ERR(dwmac->clk_tx)) {
> + dev_warn(dev, "No tx clock provided...\n");
> + dwmac->clk_tx = NULL;
> + }
> + dwmac->clk_rx = devm_clk_get(dev, "rx-clk");
> + if (IS_ERR(dwmac->clk_rx)) {
> + dev_warn(dev, "No rx clock provided...\n");
> + dwmac->clk_rx = NULL;
> + }
> +
> + /* Get mode register */
> + regmap = syscon_regmap_lookup_by_phandle(np, "st,syscon");
> + if (IS_ERR(regmap))
> + return PTR_ERR(regmap);
> +
> + err = of_property_read_u32_index(np, "st,syscon", 1,
> >mode_reg);
> + if (err) {
> + dev_err(dev, "Can't get sysconfig mode offset (%d)\n", err);
> + return err;
> + }
> +
> + dwmac->dev = dev;
> + dwmac->interface = of_get_phy_mode(np);
> + dwmac->regmap = regmap;
> +
> + return 0;
> +}
> +
> +static int stm32_dwmac_probe(struct platform_device *pdev)
> +{
> + struct plat_stmmacenet_data *plat_dat;
> + struct stmmac_resources stmmac_res;
> + struct stm32_dwmac *dwmac;
> + int ret;
> +
> + ret = stmmac_get_platform_resources(pdev, _res);
> + if (ret)
> + return ret;
> +
> + plat_dat = stmmac_probe_config_dt(pdev, _res.mac);
> + if (IS_ERR(plat_dat))
> + return PTR_ERR(plat_dat);
> +
> + dwmac = devm_kzalloc(>dev, sizeof(*dwmac), GFP_KERNEL);
> + if (!dwmac)
> + return -ENOMEM;
> +
> + ret = stm32_dwmac_parse_data(dwmac, pdev);
> + if (ret) {
> + dev_err(>dev, "Unable to parse OF data\n");
> + return ret;
> + }
> +
> + plat_dat->bsp_priv = dwmac;
> + plat_dat->init = stm32_dwmac_init;
> + plat_dat->exit = stm32_dwmac_exit;
Instead of using these callbacks could you rather implement the PM
callbacks directly in this driver?
I don't think it should add much code and it will make it look more
like standard driver. This will also give you some more control and
flexibility in your code.
> +
> + ret = stm32_dwmac_init(pdev, plat_dat->bsp_priv);
> + if (ret)
> + return ret;
> +
> + return stmmac_dvr_probe(>dev, plat_dat, _res);
> +}
> +
> +static const struct of_device_id stm32_dwmac_match[] = {
> + { .compatible = "st,stm32-dwmac"},
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, stm32_dwmac_match);
> +
> +static struct platform_driver stm32_dwmac_driver = {
> + .probe = stm32_dwmac_probe,
> + .remove = stmmac_pltfr_remove,
Could you implement the .remove callback in your driver instead of
using stmmac_pltfr_remove?
Same reasons as above.
> + .driver = {
> + .name = "stm32-dwmac",
> + .pm = _pltfr_pm_ops,
> + .of_match_table = stm32_dwmac_match,
> + },
> +};
> +module_platform_driver(stm32_dwmac_driver);
> +
> +MODULE_AUTHOR("Alexandre Torgue <alexandre.tor...@gmail.com>");
> +MODULE_DESCRIPTION("STMicroelectronics MCU DWMAC Specific Glue layer");
> +MODULE_LICENSE("GPL");
Since you state:
> + * License terms: GNU General Public License (GPL), version 2
You might want to switch use: MODULE_LICENSE("GPL v2");
regards,
Joachim Eastwood
