On Wed, Jun 19, 2024 at 7:47 AM Shan-Chun Hung <shanchun1218@xxxxxxxxx> wrote:
>
> This adds the SDHCI driver for the MA35 series SoC. It is based upon the
> SDHCI interface, but requires some extra initialization.
>
> Signed-off-by: schung <schung@xxxxxxxxxxx>
Even I agree with Markus' remarks, so please correct your SoB by using
something similar to the From line.
...
> +config MMC_SDHCI_OF_MA35D1
> + tristate "SDHCI OF support for the MA35D1 SDHCI controller"
> + depends on ARCH_A35 || COMPILE_TEST
> + depends on MMC_SDHCI_PLTFM
> + depends on OF && COMMON_CLK
OF is not compile dependency AFAICS, if you want make it functional, use
depends on OF || COMPILE_TEST
...
You are missing a lot of header inclusions, please follow IWYU principle.
+ array_size.h
+ bits.h
> +#include <linux/clk.h>
+ device.h
> +#include <linux/dma-mapping.h>
+ err.h
> +#include <linux/mfd/syscon.h>
+ math.h
+ mod_devicetable.h
> +#include <linux/module.h>
> +#include <linux/mmc/mmc.h>
> +#include <linux/pinctrl/consumer.h>
+ platform_device.h
> +#include <linux/regmap.h>
> +#include <linux/reset.h>
> +#include <linux/slab.h>
+ types.h
...
> +#define BOUNDARY_OK(addr, len) \
> + ((addr | (SZ_128M - 1)) == ((addr + len - 1) | (SZ_128M - 1)))
Besides sizes.h being missed, this can be done with help of ALIGN()
macro (or alike). So, kill this and use the globally defined macro
inline.
...
> + /* If the clock frequency exceeds MMC_HIGH_52_MAX_DTR,
> + * disable command conflict check.
> + */
/*
* The comment style is wrong and
* the indentation in the second line.
* Fix it as in this example.
*/
...
> +static void ma35_voltage_switch(struct sdhci_host *host)
> +{
> + /* Wait for 5ms after set 1.8V signal enable bit */
> + usleep_range(5000, 5500);
Use fsleep()
> +}
> +
> +static int ma35_execute_tuning(struct mmc_host *mmc, u32 opcode)
> +{
> + struct sdhci_host *host = mmc_priv(mmc);
> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> + struct ma35_priv *priv = sdhci_pltfm_priv(pltfm_host);
> +
> + /* Limitations require a reset SD/eMMC before tuning. */
> + if (!IS_ERR(priv->rst)) {
It's way too big for indentation, moreover it uses an unusual pattern,
usually we check for an error case first. So, invert the conditional
and this all code will become much better.
> + int idx;
> + u32 *val;
> +
> + val = kmalloc(ARRAY_SIZE(restore_data), GFP_KERNEL);
> + for (idx = 0; idx < ARRAY_SIZE(restore_data); idx++) {
> + if (restore_data[idx].width == 32)
sizeof(u32) ?
> + val[idx] = sdhci_readl(host, restore_data[idx].reg);
> + else if (restore_data[idx].width == 8)
sizeof(u8) ?
> + val[idx] = sdhci_readb(host, restore_data[idx].reg);
> + }
> +
> + reset_control_assert(priv->rst);
> + reset_control_deassert(priv->rst);
> +
> + for (idx = 0; idx < ARRAY_SIZE(restore_data); idx++) {
> + if (restore_data[idx].width == 32)
> + sdhci_writel(host, val[idx], restore_data[idx].reg);
> + else if (restore_data[idx].width == 8)
> + sdhci_writeb(host, val[idx], restore_data[idx].reg);
As per above?
> + }
> +
> + kfree(val);
> + }
> +
> + return sdhci_execute_tuning(mmc, opcode);
> +}
...
> +static int ma35_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
Since you have it, use it!
> + struct sdhci_pltfm_host *pltfm_host;
> + struct sdhci_host *host;
> + struct ma35_priv *priv;
> + int err;
> + u32 extra, ctl;
> +
> + host = sdhci_pltfm_init(pdev, &sdhci_ma35_pdata,
> + sizeof(struct ma35_priv));
One line?
> + if (IS_ERR(host))
> + return PTR_ERR(host);
> +
> + /*
> + * extra adma table cnt for cross 128M boundary handling.
> + */
Wrong comment style.
> + extra = DIV_ROUND_UP_ULL(dma_get_required_mask(&pdev->dev), SZ_128M);
> + if (extra > SDHCI_MAX_SEGS)
> + extra = SDHCI_MAX_SEGS;
min() ? clamp() ?
> + host->adma_table_cnt += extra;
> + pltfm_host = sdhci_priv(host);
> + priv = sdhci_pltfm_priv(pltfm_host);
> + if (dev->of_node) {
Why?
> + pltfm_host->clk = devm_clk_get(&pdev->dev, NULL);
> + if (IS_ERR(pltfm_host->clk)) {
> + err = PTR_ERR(pltfm_host->clk);
> + dev_err(&pdev->dev, "failed to get clk: %d\n", err);
Use
return dev_err_probe(...);
> + goto free_pltfm;
This is wrong. You may not call non-devm before devm ones, otherwise
it makes a room for subtle mistakes on remove-probe or unbind-bind
cycles. Have you tested that?
> + }
> + err = clk_prepare_enable(pltfm_host->clk);
> + if (err)
> + goto free_pltfm;
Use _enabled variant of devm_clk_get() instead.
> + }
> +
> + err = mmc_of_parse(host->mmc);
> + if (err)
> + goto err_clk;
> +
> + priv->rst = devm_reset_control_get(&pdev->dev, NULL);
No error check?!
> + sdhci_get_of_property(pdev);
> +
> + priv->pinctrl = devm_pinctrl_get(&pdev->dev);
> + if (!IS_ERR(priv->pinctrl)) {
> + priv->pins_default = pinctrl_lookup_state(priv->pinctrl, "default");
> + priv->pins_uhs = pinctrl_lookup_state(priv->pinctrl, "state_uhs");
> + pinctrl_select_state(priv->pinctrl, priv->pins_default);
> + }
> +
> + if (!(host->quirks2 & SDHCI_QUIRK2_NO_1_8_V)) {
> + u32 reg;
> +
> + priv->regmap = syscon_regmap_lookup_by_phandle(
> + pdev->dev.of_node, "nuvoton,sys");
dev_of_node(dev)
> +
> + if (!IS_ERR(priv->regmap)) {
> + /* Enable SDHCI voltage stable for 1.8V */
> + regmap_read(priv->regmap, MA35_SYS_MISCFCR0, ®);
> + reg |= BIT(17);
> + regmap_write(priv->regmap, MA35_SYS_MISCFCR0, reg);
> + }
> +
> + host->mmc_host_ops.start_signal_voltage_switch =
> + ma35_start_signal_voltage_switch;
> + }
> +
> + host->mmc_host_ops.execute_tuning = ma35_execute_tuning;
> +
> + err = sdhci_add_host(host);
> + if (err)
> + goto err_clk;
> +
> + /* Enable INCR16 and INCR8 */
> + ctl = sdhci_readw(host, MA35_SDHCI_MBIUCTL);
> + ctl &= ~MA35_SDHCI_INCR_MSK;
> + ctl |= MA35_SDHCI_INCR16|MA35_SDHCI_INCR8;
> + sdhci_writew(host, ctl, MA35_SDHCI_MBIUCTL);
> +
> + return 0;
> +err_clk:
> + clk_disable_unprepare(pltfm_host->clk);
This will go with the _enabled variant being used.
> +free_pltfm:
> + sdhci_pltfm_free(pdev);
This should go to be correct in ordering.
> + return err;
> +}
> +
> +static int ma35_remove(struct platform_device *pdev)
Use remove_new callback.
> +{
> + struct sdhci_host *host = platform_get_drvdata(pdev);
> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +
> + sdhci_remove_host(host, 0);
> + clk_disable_unprepare(pltfm_host->clk);
> + sdhci_pltfm_free(pdev);
At least these two will go away as per probe error path.
> + return 0;
> +}
...
> +MODULE_AUTHOR("shanchun1218@xxxxxxxxxx");
Needs to be fixed as Markus said.
--
With Best Regards,
Andy Shevchenko
