On 2022/12/9 5:09, Linus Walleij wrote:
> 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.
>
Will fix.
>> +#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.
>
Will fix.
>> +#include <linux/regmap.h>
>> +#include <linux/regulator/consumer.h>
>
> Or this.
>
Will fix.
>> +#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?
>
Will fix.
>> + int found = 0;
>
> This looks like a bool.
>
Will update.
>> + 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.
>
I'll update it in next version.
>> +
>> + 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.
>
Will update.
> (...)
>> + 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.
>
Will update.
>> +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?
>
Will fix.
> Other than these things, the driver looks good!
>
Hi Linus,
Thank you for taking time to review and provide helpful comments for this patch.
I will take all of your suggestions and update this driver in next version.
Best Regards
William Qiu
> Yours,
> Linus Walleij
