| Message ID | 20221130150857.67113-3-tmaimon77@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | MMC: add NPCM SDHCI driver support | expand |
On Wed, Nov 30, 2022 at 5:09 PM Tomer Maimon <tmaimon77@gmail.com> wrote: > > Add Nuvoton NPCM BMC sdhci-pltfm controller driver. ... > obj-$(CONFIG_MMC_S3C) += s3cmci.o > obj-$(CONFIG_MMC_SDRICOH_CS) += sdricoh_cs.o > obj-$(CONFIG_MMC_TMIO) += tmio_mmc.o > +obj-$(CONFIG_MMC_SDHCI_NPCM) += sdhci-npcm.o Keep it ordered by module name. > obj-$(CONFIG_MMC_TMIO_CORE) += tmio_mmc_core.o > obj-$(CONFIG_MMC_SDHI) += renesas_sdhi_core.o > obj-$(CONFIG_MMC_SDHI_SYS_DMAC) += renesas_sdhi_sys_dmac.o ... > +/* > + * NPCM SDHC MMC host controller driver. > + * > + */ Too many lines for seems to be oneliner comment. ... > +#include <linux/of.h> I don't see how it's being used. But it seems the mod_devicetable.h is missing. ... > +static const struct sdhci_pltfm_data npcm_sdhci_pdata = { > + .quirks = SDHCI_QUIRK_DELAY_AFTER_POWER, > + .quirks2 = SDHCI_QUIRK2_STOP_WITH_TC | > + SDHCI_QUIRK2_NO_1_8_V, > +}; Why? Can't you use the sdhci as a library? ... > +static int npcm_sdhci_probe(struct platform_device *pdev) > +{ > + struct sdhci_pltfm_host *pltfm_host; > + struct sdhci_host *host; > + u32 caps; > + int ret; > + > + host = sdhci_pltfm_init(pdev, &npcm_sdhci_pdata, 0); > + if (IS_ERR(host)) > + return PTR_ERR(host); > + > + pltfm_host = sdhci_priv(host); > + pltfm_host->clk = devm_clk_get(&pdev->dev, NULL); > + Blank line in a wrong position, should be before devm_clk_get(). > + if (!IS_ERR(pltfm_host->clk)) > + clk_prepare_enable(pltfm_host->clk); Why not use a specific helper that gets the clock enabled? > + caps = sdhci_readl(host, SDHCI_CAPABILITIES); > + if (caps & SDHCI_CAN_DO_8BIT) > + host->mmc->caps |= MMC_CAP_8_BIT_DATA; > + > + ret = mmc_of_parse(host->mmc); > + if (ret) > + goto err_sdhci_add; > + > + ret = sdhci_add_host(host); > + if (ret) > + goto err_sdhci_add; > + > + return 0; > + > +err_sdhci_add: > + clk_disable_unprepare(pltfm_host->clk); > + sdhci_pltfm_free(pdev); > + return ret; > +} ... > + Redundant blank line. > +module_platform_driver(npcm_sdhci_driver);
Hi Andy, Thanks for your comments. On Wed, 30 Nov 2022 at 17:50, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Wed, Nov 30, 2022 at 5:09 PM Tomer Maimon <tmaimon77@gmail.com> wrote: > > > > Add Nuvoton NPCM BMC sdhci-pltfm controller driver. > > ... > > > obj-$(CONFIG_MMC_S3C) += s3cmci.o > > obj-$(CONFIG_MMC_SDRICOH_CS) += sdricoh_cs.o > > obj-$(CONFIG_MMC_TMIO) += tmio_mmc.o > > > +obj-$(CONFIG_MMC_SDHCI_NPCM) += sdhci-npcm.o > > Keep it ordered by module name. > > > obj-$(CONFIG_MMC_TMIO_CORE) += tmio_mmc_core.o > > obj-$(CONFIG_MMC_SDHI) += renesas_sdhi_core.o > > obj-$(CONFIG_MMC_SDHI_SYS_DMAC) += renesas_sdhi_sys_dmac.o > > ... > > > +/* > > + * NPCM SDHC MMC host controller driver. > > + * > > + */ > > Too many lines for seems to be oneliner comment. > > ... > > > +#include <linux/of.h> > > I don't see how it's being used. > But it seems the mod_devicetable.h is missing. > > ... > > > +static const struct sdhci_pltfm_data npcm_sdhci_pdata = { > > + .quirks = SDHCI_QUIRK_DELAY_AFTER_POWER, > > + .quirks2 = SDHCI_QUIRK2_STOP_WITH_TC | > > + SDHCI_QUIRK2_NO_1_8_V, > > +}; > > Why? Can't you use the sdhci as a library? Can you explain what you mean by using sdhci as a library? is it not to use the pltfm_data structure and only to set the quirks directly through the host? > ... > > > +static int npcm_sdhci_probe(struct platform_device *pdev) > > +{ > > + struct sdhci_pltfm_host *pltfm_host; > > + struct sdhci_host *host; > > + u32 caps; > > + int ret; > > + > > + host = sdhci_pltfm_init(pdev, &npcm_sdhci_pdata, 0); > > + if (IS_ERR(host)) > > + return PTR_ERR(host); > > + > > + pltfm_host = sdhci_priv(host); > > > + pltfm_host->clk = devm_clk_get(&pdev->dev, NULL); > > + > > Blank line in a wrong position, should be before devm_clk_get(). > > > + if (!IS_ERR(pltfm_host->clk)) > > + clk_prepare_enable(pltfm_host->clk); > > Why not use a specific helper that gets the clock enabled? which specific helper? can you give me more specific details? > > > + caps = sdhci_readl(host, SDHCI_CAPABILITIES); > > + if (caps & SDHCI_CAN_DO_8BIT) > > + host->mmc->caps |= MMC_CAP_8_BIT_DATA; > > + > > + ret = mmc_of_parse(host->mmc); > > + if (ret) > > + goto err_sdhci_add; > > + > > + ret = sdhci_add_host(host); > > + if (ret) > > + goto err_sdhci_add; > > + > > + return 0; > > + > > +err_sdhci_add: > > + clk_disable_unprepare(pltfm_host->clk); > > + sdhci_pltfm_free(pdev); > > + return ret; > > +} > > ... > > > + > > Redundant blank line. > > > +module_platform_driver(npcm_sdhci_driver); > > -- > With Best Regards, > Andy Shevchenko Best regards, Tomer
On 30/11/22 17:08, Tomer Maimon wrote: > Add Nuvoton NPCM BMC sdhci-pltfm controller driver. > > Signed-off-by: Tomer Maimon <tmaimon77@gmail.com> > --- > drivers/mmc/host/Kconfig | 8 ++++ > drivers/mmc/host/Makefile | 1 + > drivers/mmc/host/sdhci-npcm.c | 81 +++++++++++++++++++++++++++++++++++ > 3 files changed, 90 insertions(+) > create mode 100644 drivers/mmc/host/sdhci-npcm.c > > diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig > index fb1062a6394c..4b2d9ce4308c 100644 > --- a/drivers/mmc/host/Kconfig > +++ b/drivers/mmc/host/Kconfig > @@ -709,6 +709,14 @@ config MMC_TMIO > This provides support for the SD/MMC cell found in TC6393XB, > T7L66XB and also HTC ASIC3 > > +config MMC_SDHCI_NPCM > + tristate "Secure Digital Host Controller Interface support for NPCM" > + depends on ARCH_NPCM || COMPILE_TEST > + depends on MMC_SDHCI_PLTFM > + help > + This provides support for the SD/eMMC controller found in > + NPCM BMC family SoCs. > + > config MMC_SDHI > tristate "Renesas SDHI SD/SDIO controller support" > depends on SUPERH || ARCH_RENESAS || COMPILE_TEST > diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile > index 4e4ceb32c4b4..801086613d7f 100644 > --- a/drivers/mmc/host/Makefile > +++ b/drivers/mmc/host/Makefile > @@ -37,6 +37,7 @@ obj-$(CONFIG_MMC_SPI) += of_mmc_spi.o > obj-$(CONFIG_MMC_S3C) += s3cmci.o > obj-$(CONFIG_MMC_SDRICOH_CS) += sdricoh_cs.o > obj-$(CONFIG_MMC_TMIO) += tmio_mmc.o > +obj-$(CONFIG_MMC_SDHCI_NPCM) += sdhci-npcm.o > obj-$(CONFIG_MMC_TMIO_CORE) += tmio_mmc_core.o > obj-$(CONFIG_MMC_SDHI) += renesas_sdhi_core.o > obj-$(CONFIG_MMC_SDHI_SYS_DMAC) += renesas_sdhi_sys_dmac.o > diff --git a/drivers/mmc/host/sdhci-npcm.c b/drivers/mmc/host/sdhci-npcm.c > new file mode 100644 > index 000000000000..298c5f3e7c2b > --- /dev/null > +++ b/drivers/mmc/host/sdhci-npcm.c > @@ -0,0 +1,81 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* > + * NPCM SDHC MMC host controller driver. > + * > + */ > + > +#include <linux/clk.h> > +#include <linux/err.h> > +#include <linux/io.h> > +#include <linux/mmc/host.h> > +#include <linux/mmc/mmc.h> > +#include <linux/module.h> > +#include <linux/of.h> > + > +#include "sdhci-pltfm.h" > + > +static const struct sdhci_pltfm_data npcm_sdhci_pdata = { > + .quirks = SDHCI_QUIRK_DELAY_AFTER_POWER, > + .quirks2 = SDHCI_QUIRK2_STOP_WITH_TC | > + SDHCI_QUIRK2_NO_1_8_V, > +}; > + > +static int npcm_sdhci_probe(struct platform_device *pdev) > +{ > + struct sdhci_pltfm_host *pltfm_host; > + struct sdhci_host *host; > + u32 caps; > + int ret; > + > + host = sdhci_pltfm_init(pdev, &npcm_sdhci_pdata, 0); > + if (IS_ERR(host)) > + return PTR_ERR(host); > + > + pltfm_host = sdhci_priv(host); > + pltfm_host->clk = devm_clk_get(&pdev->dev, NULL); For an optional clock, something like: pltfm_host->clk = devm_clk_get_optional(&pdev->dev, NULL); if (IS_ERR(pltfm_host->clk)) return PTR_ERR(pltfm_host->clk); will handle -EPROBE_DEFER > + > + if (!IS_ERR(pltfm_host->clk)) > + clk_prepare_enable(pltfm_host->clk); > + > + caps = sdhci_readl(host, SDHCI_CAPABILITIES); > + if (caps & SDHCI_CAN_DO_8BIT) > + host->mmc->caps |= MMC_CAP_8_BIT_DATA; > + > + ret = mmc_of_parse(host->mmc); > + if (ret) > + goto err_sdhci_add; > + > + ret = sdhci_add_host(host); > + if (ret) > + goto err_sdhci_add; > + > + return 0; > + > +err_sdhci_add: > + clk_disable_unprepare(pltfm_host->clk); > + sdhci_pltfm_free(pdev); > + return ret; > +} > + > +static const struct of_device_id npcm_sdhci_of_match[] = { > + { .compatible = "nuvoton,npcm750-sdhci" }, > + { .compatible = "nuvoton,npcm845-sdhci" }, > + { } > +}; > +MODULE_DEVICE_TABLE(of, npcm_sdhci_of_match); > + > +static struct platform_driver npcm_sdhci_driver = { > + .driver = { > + .name = "npcm-sdhci", > + .of_match_table = npcm_sdhci_of_match, > + .pm = &sdhci_pltfm_pmops, > + }, > + .probe = npcm_sdhci_probe, > + .remove = sdhci_pltfm_unregister, > +}; > + > +module_platform_driver(npcm_sdhci_driver); > + > +MODULE_DESCRIPTION("NPCM Secure Digital Host Controller Interface driver"); > +MODULE_AUTHOR("Tomer Maimon <tomer.maimon@nuvoton.com>"); > +MODULE_LICENSE("GPL v2"); WARNING: Prefer "GPL" over "GPL v2" - see commit bf7fbeeae6db ("module: Cure the MODULE_LICENSE "GPL" vs. "GPL v2" bogosity") #133: FILE: drivers/mmc/host/sdhci-npcm.c:81: +MODULE_LICENSE("GPL v2");
Hi Adrian, Thanks for your comments. Your comments will be addressed next version. On Thu, 1 Dec 2022 at 18:28, Adrian Hunter <adrian.hunter@intel.com> wrote: > > On 30/11/22 17:08, Tomer Maimon wrote: > > Add Nuvoton NPCM BMC sdhci-pltfm controller driver. > > > > Signed-off-by: Tomer Maimon <tmaimon77@gmail.com> > > --- > > drivers/mmc/host/Kconfig | 8 ++++ > > drivers/mmc/host/Makefile | 1 + > > drivers/mmc/host/sdhci-npcm.c | 81 +++++++++++++++++++++++++++++++++++ > > 3 files changed, 90 insertions(+) > > create mode 100644 drivers/mmc/host/sdhci-npcm.c > > > > diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig > > index fb1062a6394c..4b2d9ce4308c 100644 > > --- a/drivers/mmc/host/Kconfig > > +++ b/drivers/mmc/host/Kconfig > > @@ -709,6 +709,14 @@ config MMC_TMIO > > This provides support for the SD/MMC cell found in TC6393XB, > > T7L66XB and also HTC ASIC3 > > > > +config MMC_SDHCI_NPCM > > + tristate "Secure Digital Host Controller Interface support for NPCM" > > + depends on ARCH_NPCM || COMPILE_TEST > > + depends on MMC_SDHCI_PLTFM > > + help > > + This provides support for the SD/eMMC controller found in > > + NPCM BMC family SoCs. > > + > > config MMC_SDHI > > tristate "Renesas SDHI SD/SDIO controller support" > > depends on SUPERH || ARCH_RENESAS || COMPILE_TEST > > diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile > > index 4e4ceb32c4b4..801086613d7f 100644 > > --- a/drivers/mmc/host/Makefile > > +++ b/drivers/mmc/host/Makefile > > @@ -37,6 +37,7 @@ obj-$(CONFIG_MMC_SPI) += of_mmc_spi.o > > obj-$(CONFIG_MMC_S3C) += s3cmci.o > > obj-$(CONFIG_MMC_SDRICOH_CS) += sdricoh_cs.o > > obj-$(CONFIG_MMC_TMIO) += tmio_mmc.o > > +obj-$(CONFIG_MMC_SDHCI_NPCM) += sdhci-npcm.o > > obj-$(CONFIG_MMC_TMIO_CORE) += tmio_mmc_core.o > > obj-$(CONFIG_MMC_SDHI) += renesas_sdhi_core.o > > obj-$(CONFIG_MMC_SDHI_SYS_DMAC) += renesas_sdhi_sys_dmac.o > > diff --git a/drivers/mmc/host/sdhci-npcm.c b/drivers/mmc/host/sdhci-npcm.c > > new file mode 100644 > > index 000000000000..298c5f3e7c2b > > --- /dev/null > > +++ b/drivers/mmc/host/sdhci-npcm.c > > @@ -0,0 +1,81 @@ > > +// SPDX-License-Identifier: GPL-2.0+ > > +/* > > + * NPCM SDHC MMC host controller driver. > > + * > > + */ > > + > > +#include <linux/clk.h> > > +#include <linux/err.h> > > +#include <linux/io.h> > > +#include <linux/mmc/host.h> > > +#include <linux/mmc/mmc.h> > > +#include <linux/module.h> > > +#include <linux/of.h> > > + > > +#include "sdhci-pltfm.h" > > + > > +static const struct sdhci_pltfm_data npcm_sdhci_pdata = { > > + .quirks = SDHCI_QUIRK_DELAY_AFTER_POWER, > > + .quirks2 = SDHCI_QUIRK2_STOP_WITH_TC | > > + SDHCI_QUIRK2_NO_1_8_V, > > +}; > > + > > +static int npcm_sdhci_probe(struct platform_device *pdev) > > +{ > > + struct sdhci_pltfm_host *pltfm_host; > > + struct sdhci_host *host; > > + u32 caps; > > + int ret; > > + > > + host = sdhci_pltfm_init(pdev, &npcm_sdhci_pdata, 0); > > + if (IS_ERR(host)) > > + return PTR_ERR(host); > > + > > + pltfm_host = sdhci_priv(host); > > + pltfm_host->clk = devm_clk_get(&pdev->dev, NULL); > > For an optional clock, something like: > > pltfm_host->clk = devm_clk_get_optional(&pdev->dev, NULL); > if (IS_ERR(pltfm_host->clk)) > return PTR_ERR(pltfm_host->clk); > > will handle -EPROBE_DEFER > > > + > > + if (!IS_ERR(pltfm_host->clk)) > > + clk_prepare_enable(pltfm_host->clk); > > + > > + caps = sdhci_readl(host, SDHCI_CAPABILITIES); > > + if (caps & SDHCI_CAN_DO_8BIT) > > + host->mmc->caps |= MMC_CAP_8_BIT_DATA; > > + > > + ret = mmc_of_parse(host->mmc); > > + if (ret) > > + goto err_sdhci_add; > > + > > + ret = sdhci_add_host(host); > > + if (ret) > > + goto err_sdhci_add; > > + > > + return 0; > > + > > +err_sdhci_add: > > + clk_disable_unprepare(pltfm_host->clk); > > + sdhci_pltfm_free(pdev); > > + return ret; > > +} > > + > > +static const struct of_device_id npcm_sdhci_of_match[] = { > > + { .compatible = "nuvoton,npcm750-sdhci" }, > > + { .compatible = "nuvoton,npcm845-sdhci" }, > > + { } > > +}; > > +MODULE_DEVICE_TABLE(of, npcm_sdhci_of_match); > > + > > +static struct platform_driver npcm_sdhci_driver = { > > + .driver = { > > + .name = "npcm-sdhci", > > + .of_match_table = npcm_sdhci_of_match, > > + .pm = &sdhci_pltfm_pmops, > > + }, > > + .probe = npcm_sdhci_probe, > > + .remove = sdhci_pltfm_unregister, > > +}; > > + > > +module_platform_driver(npcm_sdhci_driver); > > + > > +MODULE_DESCRIPTION("NPCM Secure Digital Host Controller Interface driver"); > > +MODULE_AUTHOR("Tomer Maimon <tomer.maimon@nuvoton.com>"); > > +MODULE_LICENSE("GPL v2"); > > WARNING: Prefer "GPL" over "GPL v2" - see commit bf7fbeeae6db ("module: Cure the MODULE_LICENSE "GPL" vs. "GPL v2" bogosity") > #133: FILE: drivers/mmc/host/sdhci-npcm.c:81: > +MODULE_LICENSE("GPL v2"); > > Best regards, Tomer
diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig index fb1062a6394c..4b2d9ce4308c 100644 --- a/drivers/mmc/host/Kconfig +++ b/drivers/mmc/host/Kconfig @@ -709,6 +709,14 @@ config MMC_TMIO This provides support for the SD/MMC cell found in TC6393XB, T7L66XB and also HTC ASIC3 +config MMC_SDHCI_NPCM + tristate "Secure Digital Host Controller Interface support for NPCM" + depends on ARCH_NPCM || COMPILE_TEST + depends on MMC_SDHCI_PLTFM + help + This provides support for the SD/eMMC controller found in + NPCM BMC family SoCs. + config MMC_SDHI tristate "Renesas SDHI SD/SDIO controller support" depends on SUPERH || ARCH_RENESAS || COMPILE_TEST diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile index 4e4ceb32c4b4..801086613d7f 100644 --- a/drivers/mmc/host/Makefile +++ b/drivers/mmc/host/Makefile @@ -37,6 +37,7 @@ obj-$(CONFIG_MMC_SPI) += of_mmc_spi.o obj-$(CONFIG_MMC_S3C) += s3cmci.o obj-$(CONFIG_MMC_SDRICOH_CS) += sdricoh_cs.o obj-$(CONFIG_MMC_TMIO) += tmio_mmc.o +obj-$(CONFIG_MMC_SDHCI_NPCM) += sdhci-npcm.o obj-$(CONFIG_MMC_TMIO_CORE) += tmio_mmc_core.o obj-$(CONFIG_MMC_SDHI) += renesas_sdhi_core.o obj-$(CONFIG_MMC_SDHI_SYS_DMAC) += renesas_sdhi_sys_dmac.o diff --git a/drivers/mmc/host/sdhci-npcm.c b/drivers/mmc/host/sdhci-npcm.c new file mode 100644 index 000000000000..298c5f3e7c2b --- /dev/null +++ b/drivers/mmc/host/sdhci-npcm.c @@ -0,0 +1,81 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * NPCM SDHC MMC host controller driver. + * + */ + +#include <linux/clk.h> +#include <linux/err.h> +#include <linux/io.h> +#include <linux/mmc/host.h> +#include <linux/mmc/mmc.h> +#include <linux/module.h> +#include <linux/of.h> + +#include "sdhci-pltfm.h" + +static const struct sdhci_pltfm_data npcm_sdhci_pdata = { + .quirks = SDHCI_QUIRK_DELAY_AFTER_POWER, + .quirks2 = SDHCI_QUIRK2_STOP_WITH_TC | + SDHCI_QUIRK2_NO_1_8_V, +}; + +static int npcm_sdhci_probe(struct platform_device *pdev) +{ + struct sdhci_pltfm_host *pltfm_host; + struct sdhci_host *host; + u32 caps; + int ret; + + host = sdhci_pltfm_init(pdev, &npcm_sdhci_pdata, 0); + if (IS_ERR(host)) + return PTR_ERR(host); + + pltfm_host = sdhci_priv(host); + pltfm_host->clk = devm_clk_get(&pdev->dev, NULL); + + if (!IS_ERR(pltfm_host->clk)) + clk_prepare_enable(pltfm_host->clk); + + caps = sdhci_readl(host, SDHCI_CAPABILITIES); + if (caps & SDHCI_CAN_DO_8BIT) + host->mmc->caps |= MMC_CAP_8_BIT_DATA; + + ret = mmc_of_parse(host->mmc); + if (ret) + goto err_sdhci_add; + + ret = sdhci_add_host(host); + if (ret) + goto err_sdhci_add; + + return 0; + +err_sdhci_add: + clk_disable_unprepare(pltfm_host->clk); + sdhci_pltfm_free(pdev); + return ret; +} + +static const struct of_device_id npcm_sdhci_of_match[] = { + { .compatible = "nuvoton,npcm750-sdhci" }, + { .compatible = "nuvoton,npcm845-sdhci" }, + { } +}; +MODULE_DEVICE_TABLE(of, npcm_sdhci_of_match); + +static struct platform_driver npcm_sdhci_driver = { + .driver = { + .name = "npcm-sdhci", + .of_match_table = npcm_sdhci_of_match, + .pm = &sdhci_pltfm_pmops, + }, + .probe = npcm_sdhci_probe, + .remove = sdhci_pltfm_unregister, +}; + +module_platform_driver(npcm_sdhci_driver); + +MODULE_DESCRIPTION("NPCM Secure Digital Host Controller Interface driver"); +MODULE_AUTHOR("Tomer Maimon <tomer.maimon@nuvoton.com>"); +MODULE_LICENSE("GPL v2");
Add Nuvoton NPCM BMC sdhci-pltfm controller driver. Signed-off-by: Tomer Maimon <tmaimon77@gmail.com> --- drivers/mmc/host/Kconfig | 8 ++++ drivers/mmc/host/Makefile | 1 + drivers/mmc/host/sdhci-npcm.c | 81 +++++++++++++++++++++++++++++++++++ 3 files changed, 90 insertions(+) create mode 100644 drivers/mmc/host/sdhci-npcm.c