From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 08F26C27C4F for ; Fri, 21 Jun 2024 08:07:12 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:List-Subscribe:List-Help :List-Post:List-Archive:List-Unsubscribe:List-Id:Content-Transfer-Encoding: Content-Type:In-Reply-To:References:Cc:To:Subject:From:MIME-Version:Date: Message-ID:Reply-To:Content-ID:Content-Description:Resent-Date:Resent-From: Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=AvWL0wj3A/i0gS74gMUgMtb/mv9EjdRvYDNJx0i61hc=; b=RbMTH3YRgcKTpDgLv0bNlLHEn9 6aOpdZYx1T2gQSwdEs43yJswls9HIFbqpoiLaMBAUL0q9usjXcwJl6agd4XWCsqoEKiX+klu+gy99 ofWumOQ4PBzi6tMuuFsNrr7uLRZAqpbrRIRllNLV0EeZYK4NJff++dZWfBkujZMCUtrnsa3p0tWCr Gu+DZmKufyOCxd3xD3zYIMDKXo9n74YKVX2c2mLo+iIGpbi4V1f/q1yK04ekuqvv8ZOyAMP4ddK7k fHB0up29IVZaiOzjTaUjF1DO1bOoZH7uUEhDjZOdWPO3Ny4xIUWQEEMXnv07R1DSEw5Qbh+MSt1Sj /y7mqnfg==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1sKZIR-00000008GJd-374C; Fri, 21 Jun 2024 08:06:59 +0000 Received: from mail-ot1-x333.google.com ([2607:f8b0:4864:20::333]) by bombadil.infradead.org with esmtps (Exim 4.97.1 #2 (Red Hat Linux)) id 1sKZIO-00000008GHx-0gV4 for linux-arm-kernel@lists.infradead.org; Fri, 21 Jun 2024 08:06:57 +0000 Received: by mail-ot1-x333.google.com with SMTP id 46e09a7af769-6fa0d077694so804886a34.0 for ; Fri, 21 Jun 2024 01:06:55 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1718957215; x=1719562015; darn=lists.infradead.org; h=content-transfer-encoding:in-reply-to:content-language:references :cc:to:subject:from:user-agent:mime-version:date:message-id:from:to :cc:subject:date:message-id:reply-to; bh=AvWL0wj3A/i0gS74gMUgMtb/mv9EjdRvYDNJx0i61hc=; b=a4UDWZ7Ym85Moap6S/oYKeejZ/sA13DQSxTMNJFLKX3Ei9G6N+VTfIN+McbBcFl3rg gB1iXR5YjMm5wttXZ046XF2W8aEM8JEv32T3+/P9GEw6A3Y+xDU2c4rjvoEOMYZQGpfj a6DrqR7QmNcTjxVNitIn5kEItgv8dmgAOObQTPT8rfR6crMOoARkFqQ1oULV6vwyZiBN s152252g5bZ/FdOHDPcAX8IIaXl+EDFh7m5+TnAvkEKcgTMVW5p6KYeaJ5Ks2WY7lenH cRl0/6O0mpRMhAu+E7Y+GCEq8XGFHQdF2XV64CWFRGIY0Eslq2NBTnkosyULHskFwjTL l2NQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1718957215; x=1719562015; h=content-transfer-encoding:in-reply-to:content-language:references :cc:to:subject:from:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=AvWL0wj3A/i0gS74gMUgMtb/mv9EjdRvYDNJx0i61hc=; b=N79ZI5L/0STEkPlkenwjlXwczsFqIjKtR0VUt8V9+1vpVbTdo+HJZcU/7ZJuaDHigk SRbF8/z72GV2kV/VpaFgcGBwktCunmZhD84qVApgbk14vfpQfNFw0rma7V7j1mMwldEr 2kF1qv881YWaNcxaMb632n8jG2rnD7N7rGT7j9Fe/LLU9J7x93epibT93r+sWnALNNbL LRB2u7EigV/A+ldLdSttofbAn7+rWHfpwR8Wu0QDFpCOqKheGOceOuSPxa4qhSVyfrjp osmOBexpXGROrj4I+HNThIGO5jYn6XYlqhi2UnOUOZkMUUgBU8/AZnJr6LnF3VRF704O 4ZZA== X-Forwarded-Encrypted: i=1; AJvYcCWYhKiFAiX5lGyMkVKx5Zxlt6+xdMC4936B72TB5mHBd6pYT33imu1l5p+pEPN85IVc+DqdaGuVT/87frpEGitIPqPAKU414TCYOpUQMDvXLHD2mng= X-Gm-Message-State: AOJu0YwO7U8cWaiFjLCObwDxsw4Q3mFiZa08/lyb+od1VAKOOdd/K6d+ 5W9ojHKXemznsiHyHwEb4A06ZhGh1neL7lU4LSThO3gzcw1n3kyQ X-Google-Smtp-Source: AGHT+IGNxn3y66ounzSsDGs7VdmSG+BnCQ7CjXvUV2vuZifwOgwlSEySSHcFYeSH0KDy0+wtbHUz3A== X-Received: by 2002:a05:6870:c14f:b0:258:42c1:2523 with SMTP id 586e51a60fabf-25c94a67c23mr8415683fac.18.1718957214669; Fri, 21 Jun 2024 01:06:54 -0700 (PDT) Received: from [172.19.1.51] (60-250-192-107.hinet-ip.hinet.net. [60.250.192.107]) by smtp.gmail.com with ESMTPSA id d2e1a72fcca58-70651192a4csm824304b3a.54.2024.06.21.01.06.51 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 21 Jun 2024 01:06:54 -0700 (PDT) Message-ID: <1e249c77-def1-4ffc-bbd6-d64f7e95b0ac@gmail.com> Date: Fri, 21 Jun 2024 16:06:49 +0800 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird From: Shan-Chun Hung Subject: Re: [PATCH 2/2] mmc: sdhci-of-ma35d1: Add Novoton MA35D1 SDHCI driver To: Andy Shevchenko Cc: ulf.hansson@linaro.org, robh@kernel.org, krzk+dt@kernel.org, conor+dt@kernel.org, adrian.hunter@intel.com, p.zabel@pengutronix.de, pbrobinson@gmail.com, serghox@gmail.com, mcgrof@kernel.org, prabhakar.mahadev-lad.rj@bp.renesas.com, forbidden405@outlook.com, tmaimon77@gmail.com, linux-arm-kernel@lists.infradead.org, linux-mmc@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, ychuang3@nuvoton.com, schung@nuvoton.com References: <20240619054641.277062-1-shanchun1218@gmail.com> <20240619054641.277062-3-shanchun1218@gmail.com> Content-Language: en-US In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240621_010656_236913_F95EF273 X-CRM114-Status: GOOD ( 40.29 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org Dear Andy, Thanks for your review. On 2024/6/20 上午 03:09, Andy Shevchenko wrote: > On Wed, Jun 19, 2024 at 7:47 AM Shan-Chun Hung 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 > Even I agree with Markus' remarks, so please correct your SoB by using > something similar to the From line. I will fix it. > > ... > >> +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. I am not familiar with IWYU yet, but I will learn it and use it for checks later on. For new, I am adding these header files. > + array_size.h > + bits.h > >> +#include > + device.h > >> +#include > + err.h > >> +#include > + math.h > + mod_devicetable.h > >> +#include >> +#include >> +#include > + platform_device.h > >> +#include >> +#include >> +#include > + 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. I will add sizes.h and directly apply globally defined  ALIGN() macro instead > ... > >> + /* 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. > */ > > ... I will fix it. >> +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() I will use fsleep() instead of usleep_range(). >> +} >> + >> +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. I will fix it. >> + 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) ? Your idea is better, I will change it. >> + val[idx] = sdhci_readl(host, restore_data[idx].reg); >> + else if (restore_data[idx].width == 8) > sizeof(u8) ? I will fix it. >> + 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? I will fix it. >> + } >> + >> + 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! I will use "dev" instead of "&pdev->dev". >> + 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? I will fix it. >> + if (IS_ERR(host)) >> + return PTR_ERR(host); >> + >> + /* >> + * extra adma table cnt for cross 128M boundary handling. >> + */ > Wrong comment style. I will fix it. >> + extra = DIV_ROUND_UP_ULL(dma_get_required_mask(&pdev->dev), SZ_128M); >> + if (extra > SDHCI_MAX_SEGS) >> + extra = SDHCI_MAX_SEGS; > min() ? clamp() ? I will use min() macro to fix it >> + host->adma_table_cnt += extra; >> + pltfm_host = sdhci_priv(host); >> + priv = sdhci_pltfm_priv(pltfm_host); >> + if (dev->of_node) { > Why? I will remove the "if ..." >> + 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(...); I will use dev_err_probe() instead of dev_err() >> + 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? I have tested it, there is no error messages during driver initial process. My thought is that sdhci_pltfm_init and sdhci_pltfm_free are used together. If there's any error after the successful execution of sdhci_pltfm_init, I'll use sdhci_pltfm_free. I am not entirely sure if this answers your question. >> + } >> + err = clk_prepare_enable(pltfm_host->clk); >> + if (err) >> + goto free_pltfm; > Use _enabled variant of devm_clk_get() instead. I will use devm_clk_get_optional_enabled() instead. >> + } >> + >> + err = mmc_of_parse(host->mmc); >> + if (err) >> + goto err_clk; >> + >> + priv->rst = devm_reset_control_get(&pdev->dev, NULL); > No error check?! I will add an 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) I will fix it. >> + >> + 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. I will use devm_clk_get_optional_enabled, so I will remove it. >> +free_pltfm: >> + sdhci_pltfm_free(pdev); > This should go to be correct in ordering. I am not entirely sure if it is a similar to "goto free_pltfm;" issue. >> + return err; >> +} >> + >> +static int ma35_remove(struct platform_device *pdev) > Use remove_new callback. I will fix it. >> +{ >> + 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. I will use sdhci_pltfm_remove instead of  the ma35_remove. >> + return 0; >> +} > ... > >> +MODULE_AUTHOR("shanchun1218@google.com"); > Needs to be fixed as Markus said. I will fix it. Best Regards, Shan-Chun