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

Tzu-Hao Wei
2026-02-05T03:36:39Z

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