linux-kernel - Re: [PATCH 2/8] phy: axiado: add Arasan eMMC-PHY for Axiado

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <47ddca19-3856-46e3-bfd3-80df89dc1b54@axiado.com>
Date: Thu, 5 Feb 2026 11:36:31 +0800
From: Tzu-Hao Wei <twei@...ado.com>
To: Krzysztof Kozlowski <krzk@...nel.org>
Cc: SriNavmani A <srinavmani@...ado.com>,
 Prasad Bolisetty <pbolisetty@...ado.com>, Vinod Koul <vkoul@...nel.org>,
 Neil Armstrong <neil.armstrong@...aro.org>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Harshit Shah <hshah@...ado.com>,
 Ulf Hansson <ulf.hansson@...aro.org>, Adrian Hunter
 <adrian.hunter@...el.com>, Michal Simek <michal.simek@....com>,
 linux-phy@...ts.infradead.org, devicetree@...r.kernel.org,
 linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
 linux-mmc@...r.kernel.org
Subject: Re: [PATCH 2/8] phy: axiado: add Arasan eMMC-PHY for Axiado

On 12/23/2025 10:32 PM, Krzysztof Kozlowski wrote:
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.
> 
> 
> On Mon, Dec 22, 2025 at 04:45:01PM +0800, Tzu-Hao Wei wrote:
>> @@ -15,6 +15,7 @@ obj-$(CONFIG_PHY_AIROHA_PCIE)               += phy-airoha-pcie.o
>>  obj-$(CONFIG_PHY_NXP_PTN3222)                += phy-nxp-ptn3222.o
> 
> 
> Where is maintainers file update in this patch? Why shall we take
> unmaintained code?
> 
It's in this series 4/8.

>>  obj-y                                        += allwinner/   \
>>                                          amlogic/     \
>> +                                        axiado/      \
>>                                          broadcom/    \
>>                                          cadence/     \
>>                                          freescale/   \
>> diff --git a/drivers/phy/axiado/Kconfig b/drivers/phy/axiado/Kconfig
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..824114e6068da327308321b9884552ad33db9efc
>> --- /dev/null
>> +++ b/drivers/phy/axiado/Kconfig
>> @@ -0,0 +1,15 @@
>> +#
>> +# PHY drivers for Axiado platforms
>> +#
>> +
> 
> Missing menuconfig or other if-block for groupping this with your ARCH
> and COMPILE_TEST dependency.
> 
Updated in the next version with
depends on OF && (ARCH_AXIADO || COMPILE_TEST)

> Look how other NEW and MAINTAINED platforms did it.
> 
>> +config PHY_AX3000_EMMC
>> +     tristate "Axiado eMMC PHY driver"
>> +     select GENERIC_PHY
>> +     help
>> +       This enables support for the eMMC PHY block found on the
>> +       Axiado AX3000 SoCs. The PHY provides the physical layer
>> +       interface used by the Arasan SDHCI host controller for emmc
>> +       signaling and timing adjustment.
>> +
>> +       If you are building a kernel for AX3000 platform with
>> +       eMMC storage, say Y or N.
> 
> ...
> 
> 
Clean up the description in the new patch.

>> +static void arasan_emmc_phy_write(struct axiado_emmc_phy *ax_phy, u32 offset, u32 data)
>> +{
>> +     writel(data, ax_phy->reg_base + offset);
>> +}
>> +
>> +static int arasan_emmc_phy_read(struct axiado_emmc_phy *ax_phy, u32 offset)
> 
> Useless wrappers. Just use readl/writel directly. You are not making
> code more readable.
> 
Removed wrappers and use readl/writel directly.

>> +
>> +     while (1) {
> 
> You proper read_poll loop.
Removed while() and use readl_poll_timeout()

>> +             bool timedout = ktime_after(ktime_get(), timeout);
>> +
>> +             if (arasan_emmc_phy_read(ax_phy, STATUS) & DLL_RDY_MASK)
>> +                     break;
>> +
>> +             if (timedout) {
>> +                     dev_err(&phy->dev, "DLL_RDY_MASK bit is not cleared.");
>> +                     return -ETIMEDOUT;
>> +             }
>> +             udelay(TIMEOUT_DELAY);
> 
> ...
> 
Removed.

>> +static int axiado_emmc_phy_probe(struct platform_device *pdev)
>> +{
>> +     struct axiado_emmc_phy *ax_phy;
>> +     struct phy_provider *phy_provider;
>> +     struct device *dev = &pdev->dev;
>> +     const struct of_device_id *id;
>> +     struct phy *generic_phy;
>> +     struct resource *res;
>> +
>> +     if (!dev->of_node)
>> +             return -ENODEV;
>> +
>> +     ax_phy = devm_kzalloc(dev, sizeof(*ax_phy), GFP_KERNEL);
>> +     if (!ax_phy)
>> +             return -ENOMEM;
>> +
>> +     res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +
> 
> Use proper wrapper to combine get resource and ioremap.
> 
Fixed.
Calling devm_platform_ioremap_resource(pdev, 0) now.

>> +     ax_phy->reg_base = devm_ioremap_resource(&pdev->dev, res);
>> +
> 
> Drop blank line, there's never such.
> 
Removed
>> +     if (IS_ERR(ax_phy->reg_base))
>> +             return PTR_ERR(ax_phy->reg_base);
>> +
>> +     id = of_match_node(axiado_emmc_phy_of_match, pdev->dev.of_node);
>> +     if (!id) {
>> +             dev_err(dev, "failed to get match_node\n");
> 
> What is the point of this? You do not use this match at all, no other
> devices. How can your device bind and still fail the match?
> 
> Drop
> 
Removed.

>> +             return -EINVAL;
>> +     }
>> +
>> +     generic_phy = devm_phy_create(dev, dev->of_node, &axiado_emmc_phy_ops);
>> +     if (IS_ERR(generic_phy)) {
>> +             dev_err(dev, "failed to create PHY\n");
>> +             return PTR_ERR(generic_phy);
> 
> Syntax is - return dev_err_probe.
> 
Fixed.

> Didn't Axiado receive this feedback before? Are you sure that you have
> procedures set inside to avoid repeating same mistakes?
> 
> Best regards,
> Krzysztof
> 
Thanks Krzysztof.

I have checked the comments one-by-one and make sure every comments
are adopted.

Best regards,
TH