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

[Tzu-Hao Wei <twei@axiado.com>]

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

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy