On 10.02.2021 17:47, Michael Walle wrote:
> Unfortunately, the IP101A and IP101G share the same PHY identifier.
> While most of the functions are somewhat backwards compatible, there is
> for example the APS_EN bit on the IP101A but on the IP101G this bit
> reserved. Also, the IP101G has many more functionalities.
>
> Deduce the model by accessing the page select register which - according
> to the datasheet - is not available on the IP101A. If this register is
> writable, assume we have an IP101G.
>
> Split the combined IP101A/G driver into two separate drivers.
>
> Signed-off-by: Michael Walle <mich...@walle.cc>
> ---
> Changes since v1:
> - use match_phy_device() as suggested by Heiner
>
> Andrew, I've dropped your Reviewed-by because of this.
>
> drivers/net/phy/icplus.c | 69 ++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 67 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/phy/icplus.c b/drivers/net/phy/icplus.c
> index 036bac628b11..1bc9baa9048f 100644
> --- a/drivers/net/phy/icplus.c
> +++ b/drivers/net/phy/icplus.c
> @@ -44,6 +44,8 @@ MODULE_LICENSE("GPL");
> #define IP101A_G_IRQ_DUPLEX_CHANGE BIT(1)
> #define IP101A_G_IRQ_LINK_CHANGE BIT(0)
>
> +#define IP101G_PAGE_CONTROL 0x14
> +#define IP101G_PAGE_CONTROL_MASK GENMASK(4, 0)
> #define IP101G_DIGITAL_IO_SPEC_CTRL 0x1d
> #define IP101G_DIGITAL_IO_SPEC_CTRL_SEL_INTR32 BIT(2)
>
> @@ -301,6 +303,58 @@ static irqreturn_t ip101a_g_handle_interrupt(struct
> phy_device *phydev)
> return IRQ_HANDLED;
> }
>
> +static int ip101a_g_has_page_register(struct phy_device *phydev)
> +{
> + int oldval, val, ret;
> +
> + oldval = phy_read(phydev, IP101G_PAGE_CONTROL);
> + if (oldval < 0)
> + return oldval;
> +
> + ret = phy_write(phydev, IP101G_PAGE_CONTROL, 0xffff);
> + if (ret)
> + return ret;
> +
> + val = phy_read(phydev, IP101G_PAGE_CONTROL);
> + if (val < 0)
> + return val;
> +
> + ret = phy_write(phydev, IP101G_PAGE_CONTROL, oldval);
> + if (ret)
> + return ret;
> +
> + return val == IP101G_PAGE_CONTROL_MASK;
> +}
> +
> +static int ip101a_g_match_phy_device(struct phy_device *phydev, bool ip101a)
> +{
> + int ret;
> +
> + if (phydev->phy_id != IP101A_PHY_ID)
> + return 0;
> +
> + /* The IP101A and the IP101G share the same PHY identifier.The IP101G
> + * seems to be a successor of the IP101A and implements more functions.
> + * Amongst other things there is a page select register, which is not
> + * available on the IP101A. Use this to distinguish these two.
> + */
> + ret = ip101a_g_has_page_register(phydev);
> + if (ret < 0)
> + return ret;
> +
> + return (ip101a) ? !ret : ret;
Simpler would be: return ip101a == !ret;
> +}
> +
> +static int ip101a_match_phy_device(struct phy_device *phydev)
> +{
> + return ip101a_g_match_phy_device(phydev, true);
> +}
> +
> +static int ip101g_match_phy_device(struct phy_device *phydev)
> +{
> + return ip101a_g_match_phy_device(phydev, false);
> +}
> +
> static struct phy_driver icplus_driver[] = {
> {
> PHY_ID_MATCH_MODEL(IP175C_PHY_ID),
> @@ -320,8 +374,19 @@ static struct phy_driver icplus_driver[] = {
> .suspend = genphy_suspend,
> .resume = genphy_resume,
> }, {
> - PHY_ID_MATCH_EXACT(IP101A_PHY_ID),
> - .name = "ICPlus IP101A/G",
> + .name = "ICPlus IP101A",
> + .match_phy_device = ip101a_match_phy_device,
> + /* PHY_BASIC_FEATURES */
These comments have once been automatically added as part of
a change in phy_driver structure. For new drivers they don't
make too much sense.
> + .probe = ip101a_g_probe,
> + .config_intr = ip101a_g_config_intr,
> + .handle_interrupt = ip101a_g_handle_interrupt,
> + .config_init = ip101a_g_config_init,
> + .soft_reset = genphy_soft_reset,
> + .suspend = genphy_suspend,
> + .resume = genphy_resume,
> +}, {
> + .name = "ICPlus IP101G",
> + .match_phy_device = ip101g_match_phy_device,
> /* PHY_BASIC_FEATURES */
> .probe = ip101a_g_probe,
> .config_intr = ip101a_g_config_intr,
>
