Re: [PATCH v1 1/5] net: phy: Add driver for Motorcomm yt8531 gigabit ethernet phy

[yanhong wang <yanhong.wang@starfivetech.com>]

On 2023/3/19 4:20, Simon Glass wrote:
> Hi Yanhong,
> 
> On Thu, 16 Mar 2023 at 19:06, Yanhong Wang
> <yanhong.wang@starfivetech.com> wrote:
>>
>> Add a driver for the motorcomm yt8531 gigabit ethernet phy. We have
>> verified the driver on StarFive VisionFive2 board.
>>
>> Signed-off-by: Yanhong Wang <yanhong.wang@starfivetech.com>
>> ---
>>  drivers/net/phy/Kconfig     |   6 +
>>  drivers/net/phy/Makefile    |   1 +
>>  drivers/net/phy/motorcomm.c | 409 ++++++++++++++++++++++++++++++++++++
>>  drivers/net/phy/phy.c       |   4 +-
>>  include/phy.h               |   1 +
>>  5 files changed, 420 insertions(+), 1 deletion(-)
>>  create mode 100644 drivers/net/phy/motorcomm.c
>>
>> diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
>> index 5eaff053a0..aba718566a 100644
>> --- a/drivers/net/phy/Kconfig
>> +++ b/drivers/net/phy/Kconfig
>> @@ -212,6 +212,12 @@ config PHY_MICREL_KSZ8XXX
>>
>>  endif # PHY_MICREL
>>
>> +config PHY_MOTORCOMM
>> +       tristate "Motorcomm PHYs"
>> +       help
>> +         Enables support for Motorcomm network PHYs.
>> +         Currently supports the YT8531 Gigabit Ethernet PHYs.
>> +
>>  config PHY_MSCC
>>         bool "Microsemi Corp Ethernet PHYs support"
>>
>> diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
>> index d38e99e717..e9523fed2e 100644
>> --- a/drivers/net/phy/Makefile
>> +++ b/drivers/net/phy/Makefile
>> @@ -23,6 +23,7 @@ obj-$(CONFIG_PHY_MARVELL) += marvell.o
>>  obj-$(CONFIG_PHY_MICREL_KSZ8XXX) += micrel_ksz8xxx.o
>>  obj-$(CONFIG_PHY_MICREL_KSZ90X1) += micrel_ksz90x1.o
>>  obj-$(CONFIG_PHY_MESON_GXL) += meson-gxl.o
>> +obj-$(CONFIG_PHY_MOTORCOMM) += motorcomm.o
>>  obj-$(CONFIG_PHY_NATSEMI) += natsemi.o
>>  obj-$(CONFIG_PHY_NXP_C45_TJA11XX) += nxp-c45-tja11xx.o
>>  obj-$(CONFIG_PHY_NXP_TJA11XX) += nxp-tja11xx.o
>> diff --git a/drivers/net/phy/motorcomm.c b/drivers/net/phy/motorcomm.c
>> new file mode 100644
>> index 0000000000..c7e44cfb63
>> --- /dev/null
>> +++ b/drivers/net/phy/motorcomm.c
>> @@ -0,0 +1,409 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +/*
> 
> [..]
> 
>> +static u32 ytphy_get_delay_reg_value(struct phy_device *phydev,
>> +                                    const char *prop_name,
>> +                                    const struct ytphy_cfg_reg_map *tbl,
>> +                                    int tb_size,
>> +                                    u16 *rxc_dly_en,
>> +                                    u32 dflt)
>> +{
>> +       int tb_size_half = tb_size / 2;
>> +       u32 val;
>> +       int i;
>> +
>> +       if (ofnode_read_u32(phydev->node, prop_name, &val))
>> +               goto err_dts_val;
> 
> Please move to your ofdata_to_plat() method.
> 
> Also, use dev_read_u32() when you have a device.
> 
> [.]
> 

Thanks. I will define a platform data structure for saving device-related configurations in the next version.

>> +static int yt8531_startup(struct phy_device *phydev)
>> +{
>> +       bool tx_clk_adj_enabled = false;
>> +       bool tx_clk_1000_inverted = false;
>> +       bool tx_clk_100_inverted = false;
>> +       bool tx_clk_10_inverted = false;
>> +       u16 val = 0;
>> +       int ret;
>> +
>> +       ret = genphy_update_link(phydev);
>> +       if (ret)
>> +               return ret;
>> +
>> +       ret = yt8531_parse_status(phydev);
>> +       if (ret)
>> +               return ret;
>> +
>> +       if (ofnode_read_bool(phydev->node, "motorcomm,tx-clk-adj-enabled"))
>> +               tx_clk_adj_enabled = true;
> 
> priv->tx_clk_adj_enabled = ofnode_read_bool(...)
> 
> Please fix globally
> 

Ok. I will fixed.

> Regards,
> Simon