Hi William,
thanks for your patch!
On Wed, Dec 7, 2022 at 2:17 PM William Qiu <william.qiu@xxxxxxxxxxxxxxxx> wrote:
> Add sdio/emmc driver support for StarFive JH7110 soc.
>
> Signed-off-by: William Qiu <william.qiu@xxxxxxxxxxxxxxxx>
(...)
> +#include <linux/gpio.h>
Never include this legacy header in new code. Also: you don't use it.
> +#include <linux/mfd/syscon.h>
> +#include <linux/mmc/host.h>
> +#include <linux/module.h>
> +#include <linux/of_address.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
You're not using this include either.
> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
Or this.
> +#define ALL_INT_CLR 0x1ffff
> +#define MAX_DELAY_CHAIN 32
> +
> +struct starfive_priv {
> + struct device *dev;
> + struct regmap *reg_syscon;
> + u32 syscon_offset;
> + u32 syscon_shift;
> + u32 syscon_mask;
> +};
> +
> +static unsigned long dw_mci_starfive_caps[] = {
> + MMC_CAP_CMD23,
> + MMC_CAP_CMD23,
> + MMC_CAP_CMD23
> +};
> +
> +static void dw_mci_starfive_set_ios(struct dw_mci *host, struct mmc_ios *ios)
> +{
> + int ret;
> + unsigned int clock;
> +
> + if (ios->timing == MMC_TIMING_MMC_DDR52 || ios->timing == MMC_TIMING_UHS_DDR50) {
> + clock = (ios->clock > 50000000 && ios->clock <= 52000000) ? 100000000 : ios->clock;
> + ret = clk_set_rate(host->ciu_clk, clock);
> + if (ret)
> + dev_dbg(host->dev, "Use an external frequency divider %uHz\n", ios->clock);
> + host->bus_hz = clk_get_rate(host->ciu_clk);
> + } else {
> + dev_dbg(host->dev, "Using the internal divider\n");
> + }
> +}
> +
> +static int dw_mci_starfive_execute_tuning(struct dw_mci_slot *slot,
> + u32 opcode)
> +{
> + static const int grade = MAX_DELAY_CHAIN;
> + struct dw_mci *host = slot->host;
> + struct starfive_priv *priv = host->priv;
> + int raise_point = -1, fall_point = -1;
> + int err, prev_err = -1;
I don't like these default-init to -1. Can you just skip it and assign it
where it makes most sense instead?
> + int found = 0;
This looks like a bool.
> + int i;
> + u32 regval;
> +
> + for (i = 0; i < grade; i++) {
> + regval = i << priv->syscon_shift;
> + err = regmap_update_bits(priv->reg_syscon, priv->syscon_offset,
> + priv->syscon_mask, regval);
> + if (err)
> + return err;
> + mci_writel(host, RINTSTS, ALL_INT_CLR);
> +
> + err = mmc_send_tuning(slot->mmc, opcode, NULL);
> + if (!err)
> + found = 1;
> +
> + if (i > 0) {
> + if (err && !prev_err)
> + fall_point = i - 1;
> + if (!err && prev_err)
> + raise_point = i;
> + }
> +
> + if (raise_point != -1 && fall_point != -1)
> + goto tuning_out;
There are just these raise point (shouldn't this be "rise_point" in proper
english?) and fall point, this misses some comments explaining what is
going on, the code is not intuitively eviden. Rise and fall of *what* for
example.
> +
> + prev_err = err;
> + err = 0;
> + }
> +
> +tuning_out:
> + if (found) {
> + if (raise_point == -1)
> + raise_point = 0;
> + if (fall_point == -1)
> + fall_point = grade - 1;
> + if (fall_point < raise_point) {
> + if ((raise_point + fall_point) >
> + (grade - 1))
> + i = fall_point / 2;
> + else
> + i = (raise_point + grade - 1) / 2;
> + } else {
> + i = (raise_point + fall_point) / 2;
> + }
Likewise here, explain what grade is, refer to the eMMC spec if necessary.
(...)
> + ret = of_parse_phandle_with_fixed_args(host->dev->of_node,
> + "starfive,sys-syscon", 3, 0, &args);
> + if (ret) {
> + dev_err(host->dev, "Failed to parse starfive,sys-syscon\n");
> + return -EINVAL;
> + }
> +
> + priv->reg_syscon = syscon_node_to_regmap(args.np);
> + of_node_put(args.np);
> + if (IS_ERR(priv->reg_syscon))
> + return PTR_ERR(priv->reg_syscon);
> +
> + priv->syscon_offset = args.args[0];
> + priv->syscon_shift = args.args[1];
> + priv->syscon_mask = args.args[2];
Why should these three things be in the device tree instead of being derived
from the compatible-string or just plain hard-coded as #defines?
I don't get it.
> +static int dw_mci_starfive_probe(struct platform_device *pdev)
> +{
> + return dw_mci_pltfm_register(pdev, &starfive_data);
> +}
> +
> +static int dw_mci_starfive_remove(struct platform_device *pdev)
> +{
> + return dw_mci_pltfm_remove(pdev);
> +}
Can't you just assign dw_mci_pltfm_remove() to .remove?
Other than these things, the driver looks good!
Yours,
Linus Walleij
