diff mbox series

[V3] net: phy: tja11xx: Add TJA11xx PHY driver

Message ID 20181221233552.3741-1-marex@denx.de
State Changes Requested, archived
Delegated to: David Miller
Headers show
Series [V3] net: phy: tja11xx: Add TJA11xx PHY driver | expand

Commit Message

Marek Vasut Dec. 21, 2018, 11:35 p.m. UTC 
Add driver for the NXP TJA1100 and TJA1101 PHYs. These PHYs are special
BroadRReach 100BaseT1 PHYs used in automotive.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: Heiner Kallweit <hkallweit1@gmail.com>
---
V2: - Use phy_modify(), phy_{set,clear}_bits()
    - Drop enable argument of tja11xx_enable_link_control()
    - Use PHY_BASIC_T1_FEATURES and dont modify supported/advertised
      features in config_init callback
    - Use genphy_soft_reset() instead of opencoding the reset sequence.
    - Drop the aneg parts, since the PHY datasheet claims it does not
      support aneg
V3: - Replace clr with mask
    - Add hwmon support
    - Check commstat in tja11xx_read_status() only if link is up
    - Use PHY_ID_MATCH_MODEL()
---
 drivers/net/phy/Kconfig       |   6 +
 drivers/net/phy/Makefile      |   1 +
 drivers/net/phy/nxp-tja11xx.c | 424 ++++++++++++++++++++++++++++++++++
 3 files changed, 431 insertions(+)
 create mode 100644 drivers/net/phy/nxp-tja11xx.c

Comments

Heiner Kallweit Dec. 22, 2018, 5:39 p.m. UTC | #1 
On 22.12.2018 00:35, Marek Vasut wrote:
> Add driver for the NXP TJA1100 and TJA1101 PHYs. These PHYs are special
> BroadRReach 100BaseT1 PHYs used in automotive.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Andrew Lunn <andrew@lunn.ch>
> Cc: Florian Fainelli <f.fainelli@gmail.com>
> Cc: Heiner Kallweit <hkallweit1@gmail.com>
> ---
> V2: - Use phy_modify(), phy_{set,clear}_bits()
>     - Drop enable argument of tja11xx_enable_link_control()
>     - Use PHY_BASIC_T1_FEATURES and dont modify supported/advertised
>       features in config_init callback
>     - Use genphy_soft_reset() instead of opencoding the reset sequence.
>     - Drop the aneg parts, since the PHY datasheet claims it does not
>       support aneg
> V3: - Replace clr with mask
>     - Add hwmon support
>     - Check commstat in tja11xx_read_status() only if link is up
>     - Use PHY_ID_MATCH_MODEL()
> ---
>  drivers/net/phy/Kconfig       |   6 +
>  drivers/net/phy/Makefile      |   1 +
>  drivers/net/phy/nxp-tja11xx.c | 424 ++++++++++++++++++++++++++++++++++
>  3 files changed, 431 insertions(+)
>  create mode 100644 drivers/net/phy/nxp-tja11xx.c
> 
[...]
> +
> +struct tja11xx_phy_stats {
> +	const char	*string;
> +	u8		reg;
> +	u8		off;
> +	u16		mask;
> +};
> +
As written in my other mail, you could think of using
FIELD_GET() again. Things like
... n, BIT(n), 
... m, BIT(m), 
are simply redundant.

> +static struct tja11xx_phy_stats tja11xx_hw_stats[] = {
> +	{ "phy_symbol_error_count", 20, 0, 0xffff },
> +	{ "phy_polarity_detect", 25, 6, BIT(6) },
> +	{ "phy_open_detect", 25, 7, BIT(7) },
> +	{ "phy_short_detect", 25, 8, BIT(8) },
> +	{ "phy_rem_rcvr_count", 26, 0, 0xff },
> +	{ "phy_loc_rcvr_count", 26, 8, 0xff },

Shouldn't mask in the last line be 0xff00 ?
In the relevant code you do: val = (reg & mask) >> off

> +static int tja11xx_probe(struct phy_device *phydev)
> +{
> +	struct device *dev = &phydev->mdio.dev;
> +	struct tja11xx_priv *priv;
> +	int i;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->hwmon_name = devm_kstrdup(dev, dev_name(dev), GFP_KERNEL);
> +	if (!priv->hwmon_name)
> +		return -ENODEV;

Do you really need to make a copy of the device name?
Why not simply priv->hwmon_name = dev_name(dev) ?
And if devm_kstrdup fails, then most likely you have an out-of-memory
error, so why not return -ENOMEM as usual?

> +
> +	for (i = 0; priv->hwmon_name[i]; i++)
> +		if (hwmon_is_bad_char(priv->hwmon_name[i]))
> +			priv->hwmon_name[i] = '_';
> +
> +	priv->hwmon_dev =
> +		devm_hwmon_device_register_with_info(dev, priv->hwmon_name,
> +						     phydev,
> +						     &tja11xx_hwmon_chip_info,
> +						     NULL);
> +
Prerequisite for this call is that HWMON is configured in the kernel and
it's reachable. Something like "IS_REACHABLE(CONFIG_HWMON)" would be
needed. You can see driver rtc-ds1307 for an example.
Heiner Kallweit Dec. 22, 2018, 8:51 p.m. UTC | #2 
On 22.12.2018 00:35, Marek Vasut wrote:
> Add driver for the NXP TJA1100 and TJA1101 PHYs. These PHYs are special
> BroadRReach 100BaseT1 PHYs used in automotive.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Andrew Lunn <andrew@lunn.ch>
> Cc: Florian Fainelli <f.fainelli@gmail.com>
> Cc: Heiner Kallweit <hkallweit1@gmail.com>
> ---
> V2: - Use phy_modify(), phy_{set,clear}_bits()
>     - Drop enable argument of tja11xx_enable_link_control()
>     - Use PHY_BASIC_T1_FEATURES and dont modify supported/advertised
>       features in config_init callback
>     - Use genphy_soft_reset() instead of opencoding the reset sequence.
>     - Drop the aneg parts, since the PHY datasheet claims it does not
>       support aneg
> V3: - Replace clr with mask
>     - Add hwmon support
>     - Check commstat in tja11xx_read_status() only if link is up
>     - Use PHY_ID_MATCH_MODEL()
> ---
>  drivers/net/phy/Kconfig       |   6 +
>  drivers/net/phy/Makefile      |   1 +
>  drivers/net/phy/nxp-tja11xx.c | 424 ++++++++++++++++++++++++++++++++++
>  3 files changed, 431 insertions(+)
>  create mode 100644 drivers/net/phy/nxp-tja11xx.c
[...]
> +static int tja11xx_hwmon_read(struct device *dev,
> +			      enum hwmon_sensor_types type,
> +			      u32 attr, int channel, long *value)
> +{
> +	struct phy_device *phydev = dev_get_drvdata(dev);
> +	int ret;
> +
> +	switch (attr) {
> +	case hwmon_in_lcrit_alarm:
> +		ret = phy_read(phydev, MII_INTSRC);
> +		if (ret < 0)
> +			return ret;
> +
> +		*value = !!(ret & MII_INTSRC_TEMP_ERR);
> +		return 0;
> +	case hwmon_temp_crit_alarm:
> +		ret = phy_read(phydev, MII_INTSRC);
> +		if (ret < 0)
> +			return ret;
> +
> +		*value = !!(ret & MII_INTSRC_TEMP_ERR);
> +		return 0;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
This looks like a copy & paste error, in both cases you're doing the same.
Marek Vasut Dec. 23, 2018, 9:13 a.m. UTC | #3 
On 12/22/18 9:51 PM, Heiner Kallweit wrote:
> On 22.12.2018 00:35, Marek Vasut wrote:
>> Add driver for the NXP TJA1100 and TJA1101 PHYs. These PHYs are special
>> BroadRReach 100BaseT1 PHYs used in automotive.
>>
>> Signed-off-by: Marek Vasut <marex@denx.de>
>> Cc: Andrew Lunn <andrew@lunn.ch>
>> Cc: Florian Fainelli <f.fainelli@gmail.com>
>> Cc: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>> V2: - Use phy_modify(), phy_{set,clear}_bits()
>>     - Drop enable argument of tja11xx_enable_link_control()
>>     - Use PHY_BASIC_T1_FEATURES and dont modify supported/advertised
>>       features in config_init callback
>>     - Use genphy_soft_reset() instead of opencoding the reset sequence.
>>     - Drop the aneg parts, since the PHY datasheet claims it does not
>>       support aneg
>> V3: - Replace clr with mask
>>     - Add hwmon support
>>     - Check commstat in tja11xx_read_status() only if link is up
>>     - Use PHY_ID_MATCH_MODEL()
>> ---
>>  drivers/net/phy/Kconfig       |   6 +
>>  drivers/net/phy/Makefile      |   1 +
>>  drivers/net/phy/nxp-tja11xx.c | 424 ++++++++++++++++++++++++++++++++++
>>  3 files changed, 431 insertions(+)
>>  create mode 100644 drivers/net/phy/nxp-tja11xx.c
> [...]
>> +static int tja11xx_hwmon_read(struct device *dev,
>> +			      enum hwmon_sensor_types type,
>> +			      u32 attr, int channel, long *value)
>> +{
>> +	struct phy_device *phydev = dev_get_drvdata(dev);
>> +	int ret;
>> +
>> +	switch (attr) {
>> +	case hwmon_in_lcrit_alarm:
>> +		ret = phy_read(phydev, MII_INTSRC);
>> +		if (ret < 0)
>> +			return ret;
>> +
>> +		*value = !!(ret & MII_INTSRC_TEMP_ERR);
>> +		return 0;
>> +	case hwmon_temp_crit_alarm:
>> +		ret = phy_read(phydev, MII_INTSRC);
>> +		if (ret < 0)
>> +			return ret;
>> +
>> +		*value = !!(ret & MII_INTSRC_TEMP_ERR);
>> +		return 0;
>> +	default:
>> +		return -EOPNOTSUPP;
>> +	}
>> +}
> This looks like a copy & paste error, in both cases you're doing the same.

Fixed, thanks. I'll send V4 once I can actually test this stuff.
Marek Vasut Dec. 23, 2018, 9:16 a.m. UTC | #4 
On 12/22/18 6:39 PM, Heiner Kallweit wrote:
> On 22.12.2018 00:35, Marek Vasut wrote:
>> Add driver for the NXP TJA1100 and TJA1101 PHYs. These PHYs are special
>> BroadRReach 100BaseT1 PHYs used in automotive.
>>
>> Signed-off-by: Marek Vasut <marex@denx.de>
>> Cc: Andrew Lunn <andrew@lunn.ch>
>> Cc: Florian Fainelli <f.fainelli@gmail.com>
>> Cc: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>> V2: - Use phy_modify(), phy_{set,clear}_bits()
>>     - Drop enable argument of tja11xx_enable_link_control()
>>     - Use PHY_BASIC_T1_FEATURES and dont modify supported/advertised
>>       features in config_init callback
>>     - Use genphy_soft_reset() instead of opencoding the reset sequence.
>>     - Drop the aneg parts, since the PHY datasheet claims it does not
>>       support aneg
>> V3: - Replace clr with mask
>>     - Add hwmon support
>>     - Check commstat in tja11xx_read_status() only if link is up
>>     - Use PHY_ID_MATCH_MODEL()
>> ---
>>  drivers/net/phy/Kconfig       |   6 +
>>  drivers/net/phy/Makefile      |   1 +
>>  drivers/net/phy/nxp-tja11xx.c | 424 ++++++++++++++++++++++++++++++++++
>>  3 files changed, 431 insertions(+)
>>  create mode 100644 drivers/net/phy/nxp-tja11xx.c
>>
> [...]
>> +
>> +struct tja11xx_phy_stats {
>> +	const char	*string;
>> +	u8		reg;
>> +	u8		off;
>> +	u16		mask;
>> +};
>> +
> As written in my other mail, you could think of using
> FIELD_GET() again. Things like
> ... n, BIT(n), 
> ... m, BIT(m), 
> are simply redundant.

Done

>> +static struct tja11xx_phy_stats tja11xx_hw_stats[] = {
>> +	{ "phy_symbol_error_count", 20, 0, 0xffff },
>> +	{ "phy_polarity_detect", 25, 6, BIT(6) },
>> +	{ "phy_open_detect", 25, 7, BIT(7) },
>> +	{ "phy_short_detect", 25, 8, BIT(8) },
>> +	{ "phy_rem_rcvr_count", 26, 0, 0xff },
>> +	{ "phy_loc_rcvr_count", 26, 8, 0xff },
> 
> Shouldn't mask in the last line be 0xff00 ?
> In the relevant code you do: val = (reg & mask) >> off

Yes, fixed, thanks

>> +static int tja11xx_probe(struct phy_device *phydev)
>> +{
>> +	struct device *dev = &phydev->mdio.dev;
>> +	struct tja11xx_priv *priv;
>> +	int i;
>> +
>> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>> +	if (!priv)
>> +		return -ENOMEM;
>> +
>> +	priv->hwmon_name = devm_kstrdup(dev, dev_name(dev), GFP_KERNEL);
>> +	if (!priv->hwmon_name)
>> +		return -ENODEV;
> 
> Do you really need to make a copy of the device name?
> Why not simply priv->hwmon_name = dev_name(dev) ?

Fine by me, but then maybe I don't quite understand why the other
drivers duplicate the name, eg. the sfp.c one.

> And if devm_kstrdup fails, then most likely you have an out-of-memory
> error, so why not return -ENOMEM as usual?

Fixed

>> +
>> +	for (i = 0; priv->hwmon_name[i]; i++)
>> +		if (hwmon_is_bad_char(priv->hwmon_name[i]))
>> +			priv->hwmon_name[i] = '_';
>> +
>> +	priv->hwmon_dev =
>> +		devm_hwmon_device_register_with_info(dev, priv->hwmon_name,
>> +						     phydev,
>> +						     &tja11xx_hwmon_chip_info,
>> +						     NULL);
>> +
> Prerequisite for this call is that HWMON is configured in the kernel and
> it's reachable. Something like "IS_REACHABLE(CONFIG_HWMON)" would be
> needed. You can see driver rtc-ds1307 for an example.

The driver depends on HWMON, so that should be sufficient ?
Heiner Kallweit Dec. 23, 2018, 9:41 a.m. UTC | #5 
On 23.12.2018 10:16, Marek Vasut wrote:
> On 12/22/18 6:39 PM, Heiner Kallweit wrote:
>> On 22.12.2018 00:35, Marek Vasut wrote:
>>> Add driver for the NXP TJA1100 and TJA1101 PHYs. These PHYs are special
>>> BroadRReach 100BaseT1 PHYs used in automotive.
>>>
>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>> Cc: Andrew Lunn <andrew@lunn.ch>
>>> Cc: Florian Fainelli <f.fainelli@gmail.com>
>>> Cc: Heiner Kallweit <hkallweit1@gmail.com>
>>> ---
>>> V2: - Use phy_modify(), phy_{set,clear}_bits()
>>>     - Drop enable argument of tja11xx_enable_link_control()
>>>     - Use PHY_BASIC_T1_FEATURES and dont modify supported/advertised
>>>       features in config_init callback
>>>     - Use genphy_soft_reset() instead of opencoding the reset sequence.
>>>     - Drop the aneg parts, since the PHY datasheet claims it does not
>>>       support aneg
>>> V3: - Replace clr with mask
>>>     - Add hwmon support
>>>     - Check commstat in tja11xx_read_status() only if link is up
>>>     - Use PHY_ID_MATCH_MODEL()
>>> ---
>>>  drivers/net/phy/Kconfig       |   6 +
>>>  drivers/net/phy/Makefile      |   1 +
>>>  drivers/net/phy/nxp-tja11xx.c | 424 ++++++++++++++++++++++++++++++++++
>>>  3 files changed, 431 insertions(+)
>>>  create mode 100644 drivers/net/phy/nxp-tja11xx.c
>>>
>> [...]
>>> +
>>> +struct tja11xx_phy_stats {
>>> +	const char	*string;
>>> +	u8		reg;
>>> +	u8		off;
>>> +	u16		mask;
>>> +};
>>> +
>> As written in my other mail, you could think of using
>> FIELD_GET() again. Things like
>> ... n, BIT(n), 
>> ... m, BIT(m), 
>> are simply redundant.
> 
> Done
> 
>>> +static struct tja11xx_phy_stats tja11xx_hw_stats[] = {
>>> +	{ "phy_symbol_error_count", 20, 0, 0xffff },
>>> +	{ "phy_polarity_detect", 25, 6, BIT(6) },
>>> +	{ "phy_open_detect", 25, 7, BIT(7) },
>>> +	{ "phy_short_detect", 25, 8, BIT(8) },
>>> +	{ "phy_rem_rcvr_count", 26, 0, 0xff },
>>> +	{ "phy_loc_rcvr_count", 26, 8, 0xff },
>>
>> Shouldn't mask in the last line be 0xff00 ?
>> In the relevant code you do: val = (reg & mask) >> off
> 
> Yes, fixed, thanks
> 
>>> +static int tja11xx_probe(struct phy_device *phydev)
>>> +{
>>> +	struct device *dev = &phydev->mdio.dev;
>>> +	struct tja11xx_priv *priv;
>>> +	int i;
>>> +
>>> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>>> +	if (!priv)
>>> +		return -ENOMEM;
>>> +
>>> +	priv->hwmon_name = devm_kstrdup(dev, dev_name(dev), GFP_KERNEL);
>>> +	if (!priv->hwmon_name)
>>> +		return -ENODEV;
>>
>> Do you really need to make a copy of the device name?
>> Why not simply priv->hwmon_name = dev_name(dev) ?
> 
> Fine by me, but then maybe I don't quite understand why the other
> drivers duplicate the name, eg. the sfp.c one.
> 
It's a question of object lifetime. If the original object can go away
before your object, then you need to make a copy of the name.
However in our case I don't think priv can live longer than dev.

>> And if devm_kstrdup fails, then most likely you have an out-of-memory
>> error, so why not return -ENOMEM as usual?
> 
> Fixed
> 
>>> +
>>> +	for (i = 0; priv->hwmon_name[i]; i++)
>>> +		if (hwmon_is_bad_char(priv->hwmon_name[i]))
>>> +			priv->hwmon_name[i] = '_';
>>> +
>>> +	priv->hwmon_dev =
>>> +		devm_hwmon_device_register_with_info(dev, priv->hwmon_name,
>>> +						     phydev,
>>> +						     &tja11xx_hwmon_chip_info,
>>> +						     NULL);
>>> +
>> Prerequisite for this call is that HWMON is configured in the kernel and
>> it's reachable. Something like "IS_REACHABLE(CONFIG_HWMON)" would be
>> needed. You can see driver rtc-ds1307 for an example.
> 
> The driver depends on HWMON, so that should be sufficient ?
> 
Missed that, that's sufficient. Just something to think about:
Often HWMON is seen as an optional add-on feature. The driver itself would
work perfectly fine also w/o HWMON. In this case you don't want the hard
dependency. So it's up to you whether you want to allow that the driver is
used on systems w/o HWMON support.
Andrew Lunn Dec. 23, 2018, 9:59 a.m. UTC | #6 
> >>> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> >>> +	if (!priv)
> >>> +		return -ENOMEM;
> >>> +
> >>> +	priv->hwmon_name = devm_kstrdup(dev, dev_name(dev), GFP_KERNEL);
> >>> +	if (!priv->hwmon_name)
> >>> +		return -ENODEV;
> >>
> >> Do you really need to make a copy of the device name?
> >> Why not simply priv->hwmon_name = dev_name(dev) ?
> > 
> > Fine by me, but then maybe I don't quite understand why the other
> > drivers duplicate the name, eg. the sfp.c one.
> > 
> It's a question of object lifetime. If the original object can go away
> before your object, then you need to make a copy of the name.
> However in our case I don't think priv can live longer than dev.
> 
> >> And if devm_kstrdup fails, then most likely you have an out-of-memory
> >> error, so why not return -ENOMEM as usual?
> > 
> > Fixed
> > 
> >>> +
> >>> +	for (i = 0; priv->hwmon_name[i]; i++)
> >>> +		if (hwmon_is_bad_char(priv->hwmon_name[i]))
> >>> +			priv->hwmon_name[i] = '_';

This is one reason to make a copy. You don't want to apply that to
main name of the device.

> >>> +
> >>> +	priv->hwmon_dev =
> >>> +		devm_hwmon_device_register_with_info(dev, priv->hwmon_name,
> >>> +						     phydev,
> >>> +						     &tja11xx_hwmon_chip_info,
> >>> +						     NULL);
> >>> +

The second reason is priv is released before dev, but what about
hwmon, especially if somebody has one of the files open? Is the
unregister synchronous?

	 Andrew
Andrew Lunn Dec. 23, 2018, 10:06 a.m. UTC | #7 
> > +	switch (attr) {
> > +	case hwmon_in_lcrit_alarm:
> > +		ret = phy_read(phydev, MII_INTSRC);
> > +		if (ret < 0)
> > +			return ret;
> > +
> > +		*value = !!(ret & MII_INTSRC_TEMP_ERR);
> > +		return 0;
> > +	case hwmon_temp_crit_alarm:
> > +		ret = phy_read(phydev, MII_INTSRC);
> > +		if (ret < 0)
> > +			return ret;
> > +
> > +		*value = !!(ret & MII_INTSRC_TEMP_ERR);
> > +		return 0;
> > +	default:
> > +		return -EOPNOTSUPP;
> > +	}
> > +}
> This looks like a copy & paste error, in both cases you're doing the same.

You also should not do it like this. hwmon_temp_crit_alarm is in a
different set of enum's as hwmon_in_lcrit_alarm. hwmon_in_lcrit_alarm
= 14.  hwmon_temp_max_alarm also is 14. You should have two different
switch statements to take account of this.

       Andrew
Marek Vasut Dec. 23, 2018, 10:21 a.m. UTC | #8 
On 12/23/18 10:41 AM, Heiner Kallweit wrote:
> On 23.12.2018 10:16, Marek Vasut wrote:
>> On 12/22/18 6:39 PM, Heiner Kallweit wrote:
>>> On 22.12.2018 00:35, Marek Vasut wrote:
>>>> Add driver for the NXP TJA1100 and TJA1101 PHYs. These PHYs are special
>>>> BroadRReach 100BaseT1 PHYs used in automotive.
>>>>
>>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>>> Cc: Andrew Lunn <andrew@lunn.ch>
>>>> Cc: Florian Fainelli <f.fainelli@gmail.com>
>>>> Cc: Heiner Kallweit <hkallweit1@gmail.com>
>>>> ---
>>>> V2: - Use phy_modify(), phy_{set,clear}_bits()
>>>>     - Drop enable argument of tja11xx_enable_link_control()
>>>>     - Use PHY_BASIC_T1_FEATURES and dont modify supported/advertised
>>>>       features in config_init callback
>>>>     - Use genphy_soft_reset() instead of opencoding the reset sequence.
>>>>     - Drop the aneg parts, since the PHY datasheet claims it does not
>>>>       support aneg
>>>> V3: - Replace clr with mask
>>>>     - Add hwmon support
>>>>     - Check commstat in tja11xx_read_status() only if link is up
>>>>     - Use PHY_ID_MATCH_MODEL()
>>>> ---
>>>>  drivers/net/phy/Kconfig       |   6 +
>>>>  drivers/net/phy/Makefile      |   1 +
>>>>  drivers/net/phy/nxp-tja11xx.c | 424 ++++++++++++++++++++++++++++++++++
>>>>  3 files changed, 431 insertions(+)
>>>>  create mode 100644 drivers/net/phy/nxp-tja11xx.c
>>>>
>>> [...]
>>>> +
>>>> +struct tja11xx_phy_stats {
>>>> +	const char	*string;
>>>> +	u8		reg;
>>>> +	u8		off;
>>>> +	u16		mask;
>>>> +};
>>>> +
>>> As written in my other mail, you could think of using
>>> FIELD_GET() again. Things like
>>> ... n, BIT(n), 
>>> ... m, BIT(m), 
>>> are simply redundant.
>>
>> Done
>>
>>>> +static struct tja11xx_phy_stats tja11xx_hw_stats[] = {
>>>> +	{ "phy_symbol_error_count", 20, 0, 0xffff },
>>>> +	{ "phy_polarity_detect", 25, 6, BIT(6) },
>>>> +	{ "phy_open_detect", 25, 7, BIT(7) },
>>>> +	{ "phy_short_detect", 25, 8, BIT(8) },
>>>> +	{ "phy_rem_rcvr_count", 26, 0, 0xff },
>>>> +	{ "phy_loc_rcvr_count", 26, 8, 0xff },
>>>
>>> Shouldn't mask in the last line be 0xff00 ?
>>> In the relevant code you do: val = (reg & mask) >> off
>>
>> Yes, fixed, thanks
>>
>>>> +static int tja11xx_probe(struct phy_device *phydev)
>>>> +{
>>>> +	struct device *dev = &phydev->mdio.dev;
>>>> +	struct tja11xx_priv *priv;
>>>> +	int i;
>>>> +
>>>> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>>>> +	if (!priv)
>>>> +		return -ENOMEM;
>>>> +
>>>> +	priv->hwmon_name = devm_kstrdup(dev, dev_name(dev), GFP_KERNEL);
>>>> +	if (!priv->hwmon_name)
>>>> +		return -ENODEV;
>>>
>>> Do you really need to make a copy of the device name?
>>> Why not simply priv->hwmon_name = dev_name(dev) ?
>>
>> Fine by me, but then maybe I don't quite understand why the other
>> drivers duplicate the name, eg. the sfp.c one.
>>
> It's a question of object lifetime. If the original object can go away
> before your object, then you need to make a copy of the name.
> However in our case I don't think priv can live longer than dev.
> 
>>> And if devm_kstrdup fails, then most likely you have an out-of-memory
>>> error, so why not return -ENOMEM as usual?
>>
>> Fixed
>>
>>>> +
>>>> +	for (i = 0; priv->hwmon_name[i]; i++)
>>>> +		if (hwmon_is_bad_char(priv->hwmon_name[i]))
>>>> +			priv->hwmon_name[i] = '_';
>>>> +
>>>> +	priv->hwmon_dev =
>>>> +		devm_hwmon_device_register_with_info(dev, priv->hwmon_name,
>>>> +						     phydev,
>>>> +						     &tja11xx_hwmon_chip_info,
>>>> +						     NULL);
>>>> +
>>> Prerequisite for this call is that HWMON is configured in the kernel and
>>> it's reachable. Something like "IS_REACHABLE(CONFIG_HWMON)" would be
>>> needed. You can see driver rtc-ds1307 for an example.
>>
>> The driver depends on HWMON, so that should be sufficient ?
>>
> Missed that, that's sufficient. Just something to think about:
> Often HWMON is seen as an optional add-on feature. The driver itself would
> work perfectly fine also w/o HWMON. In this case you don't want the hard
> dependency. So it's up to you whether you want to allow that the driver is
> used on systems w/o HWMON support.

Given that the HWMON indicates that the automotive device either
overheated or suffered undervolt, I presume it'd be safer not to make it
optional ?
Andrew Lunn Dec. 23, 2018, 10:35 a.m. UTC | #9 
> Given that the HWMON indicates that the automotive device either
> overheated or suffered undervolt, I presume it'd be safer not to make it
> optional ?

Hi Marek

It is a niche device, where i expect a custom kernel configuration. Do
any of these devices exist on standard PCIe cards? Maybe a test PC
using a standard Linux Distribution? But then again, such
distributions turn on HWMON anyway.

      Andrew
Marek Vasut Dec. 23, 2018, 10:48 a.m. UTC | #10 
On 12/23/18 11:35 AM, Andrew Lunn wrote:
>> Given that the HWMON indicates that the automotive device either
>> overheated or suffered undervolt, I presume it'd be safer not to make it
>> optional ?
> 
> Hi Marek
> 
> It is a niche device, where i expect a custom kernel configuration. Do
> any of these devices exist on standard PCIe cards? Maybe a test PC
> using a standard Linux Distribution? But then again, such
> distributions turn on HWMON anyway.

Not to my knowledge. I have two on various embedded systems and one on
SMSC95xx USB-ethernet device (although, the SMSC95xx driver needs to be
fixed to support that, cfr. the other thread).
Marek Vasut Dec. 23, 2018, 10:49 a.m. UTC | #11 
On 12/23/18 11:06 AM, Andrew Lunn wrote:
>>> +	switch (attr) {
>>> +	case hwmon_in_lcrit_alarm:
>>> +		ret = phy_read(phydev, MII_INTSRC);
>>> +		if (ret < 0)
>>> +			return ret;
>>> +
>>> +		*value = !!(ret & MII_INTSRC_TEMP_ERR);
>>> +		return 0;
>>> +	case hwmon_temp_crit_alarm:
>>> +		ret = phy_read(phydev, MII_INTSRC);
>>> +		if (ret < 0)
>>> +			return ret;
>>> +
>>> +		*value = !!(ret & MII_INTSRC_TEMP_ERR);
>>> +		return 0;
>>> +	default:
>>> +		return -EOPNOTSUPP;
>>> +	}
>>> +}
>> This looks like a copy & paste error, in both cases you're doing the same.
> 
> You also should not do it like this. hwmon_temp_crit_alarm is in a
> different set of enum's as hwmon_in_lcrit_alarm. hwmon_in_lcrit_alarm
> = 14.  hwmon_temp_max_alarm also is 14. You should have two different
> switch statements to take account of this.

I can also use a simple conditional, since I don't expect the number of
HWMON properties to grow, eg.

if (type == hwmon_in && attr == hwmon_in_lcrit_alarm) {...}
if (type == hwmon_temp && attr == hwmon_temp_crit_alarm) {...}

I think that's a bit more readable.
Andrew Lunn Dec. 23, 2018, 10:58 a.m. UTC | #12 
> I can also use a simple conditional, since I don't expect the number of
> HWMON properties to grow, eg.
> 
> if (type == hwmon_in && attr == hwmon_in_lcrit_alarm) {...}
> if (type == hwmon_temp && attr == hwmon_temp_crit_alarm) {...}

Yes, that is fine.

Please make sure you Cc: the HWMON maintainer on this patch.

       Andrew
Marek Vasut Dec. 23, 2018, 11 a.m. UTC | #13 
On 12/23/18 11:58 AM, Andrew Lunn wrote:
>> I can also use a simple conditional, since I don't expect the number of
>> HWMON properties to grow, eg.
>>
>> if (type == hwmon_in && attr == hwmon_in_lcrit_alarm) {...}
>> if (type == hwmon_temp && attr == hwmon_temp_crit_alarm) {...}
> 
> Yes, that is fine.
> 
> Please make sure you Cc: the HWMON maintainer on this patch.

Fine
Marek Vasut Jan. 3, 2019, 2:09 a.m. UTC | #14 
On 12/23/18 10:16 AM, Marek Vasut wrote:
> On 12/22/18 6:39 PM, Heiner Kallweit wrote:
>> On 22.12.2018 00:35, Marek Vasut wrote:
>>> Add driver for the NXP TJA1100 and TJA1101 PHYs. These PHYs are special
>>> BroadRReach 100BaseT1 PHYs used in automotive.
>>>
>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>> Cc: Andrew Lunn <andrew@lunn.ch>
>>> Cc: Florian Fainelli <f.fainelli@gmail.com>
>>> Cc: Heiner Kallweit <hkallweit1@gmail.com>
>>> ---
>>> V2: - Use phy_modify(), phy_{set,clear}_bits()
>>>     - Drop enable argument of tja11xx_enable_link_control()
>>>     - Use PHY_BASIC_T1_FEATURES and dont modify supported/advertised
>>>       features in config_init callback
>>>     - Use genphy_soft_reset() instead of opencoding the reset sequence.
>>>     - Drop the aneg parts, since the PHY datasheet claims it does not
>>>       support aneg
>>> V3: - Replace clr with mask
>>>     - Add hwmon support
>>>     - Check commstat in tja11xx_read_status() only if link is up
>>>     - Use PHY_ID_MATCH_MODEL()
>>> ---
>>>  drivers/net/phy/Kconfig       |   6 +
>>>  drivers/net/phy/Makefile      |   1 +
>>>  drivers/net/phy/nxp-tja11xx.c | 424 ++++++++++++++++++++++++++++++++++
>>>  3 files changed, 431 insertions(+)
>>>  create mode 100644 drivers/net/phy/nxp-tja11xx.c
>>>
>> [...]
>>> +
>>> +struct tja11xx_phy_stats {
>>> +	const char	*string;
>>> +	u8		reg;
>>> +	u8		off;
>>> +	u16		mask;
>>> +};
>>> +
>> As written in my other mail, you could think of using
>> FIELD_GET() again. Things like
>> ... n, BIT(n), 
>> ... m, BIT(m), 
>> are simply redundant.
> 
> Done

And undone, FIELD_GET() requires a mask that is compile-time constant.
That's not the case here.

Replicating what FIELD_GET() does at runtime is inefficient, so we can
as well encode the field and offset back into the table. Or is there a
better suggestion ?
Heiner Kallweit Jan. 3, 2019, 6:23 a.m. UTC | #15 
On 03.01.2019 03:09, Marek Vasut wrote:
> On 12/23/18 10:16 AM, Marek Vasut wrote:
>> On 12/22/18 6:39 PM, Heiner Kallweit wrote:
>>> On 22.12.2018 00:35, Marek Vasut wrote:
>>>> Add driver for the NXP TJA1100 and TJA1101 PHYs. These PHYs are special
>>>> BroadRReach 100BaseT1 PHYs used in automotive.
>>>>
>>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>>> Cc: Andrew Lunn <andrew@lunn.ch>
>>>> Cc: Florian Fainelli <f.fainelli@gmail.com>
>>>> Cc: Heiner Kallweit <hkallweit1@gmail.com>
>>>> ---
>>>> V2: - Use phy_modify(), phy_{set,clear}_bits()
>>>>     - Drop enable argument of tja11xx_enable_link_control()
>>>>     - Use PHY_BASIC_T1_FEATURES and dont modify supported/advertised
>>>>       features in config_init callback
>>>>     - Use genphy_soft_reset() instead of opencoding the reset sequence.
>>>>     - Drop the aneg parts, since the PHY datasheet claims it does not
>>>>       support aneg
>>>> V3: - Replace clr with mask
>>>>     - Add hwmon support
>>>>     - Check commstat in tja11xx_read_status() only if link is up
>>>>     - Use PHY_ID_MATCH_MODEL()
>>>> ---
>>>>  drivers/net/phy/Kconfig       |   6 +
>>>>  drivers/net/phy/Makefile      |   1 +
>>>>  drivers/net/phy/nxp-tja11xx.c | 424 ++++++++++++++++++++++++++++++++++
>>>>  3 files changed, 431 insertions(+)
>>>>  create mode 100644 drivers/net/phy/nxp-tja11xx.c
>>>>
>>> [...]
>>>> +
>>>> +struct tja11xx_phy_stats {
>>>> +	const char	*string;
>>>> +	u8		reg;
>>>> +	u8		off;
>>>> +	u16		mask;
>>>> +};
>>>> +
>>> As written in my other mail, you could think of using
>>> FIELD_GET() again. Things like
>>> ... n, BIT(n), 
>>> ... m, BIT(m), 
>>> are simply redundant.
>>
>> Done
> 
> And undone, FIELD_GET() requires a mask that is compile-time constant.
> That's not the case here.
> 
> Replicating what FIELD_GET() does at runtime is inefficient, so we can
> as well encode the field and offset back into the table. Or is there a
> better suggestion ?
> 
You're right, FIELD_GET() requires a constant as mask. Then for the
case here I'm not aware of a better solution than having also the mask
in the table.
Marek Vasut Jan. 4, 2019, 2:19 a.m. UTC | #16 
On 1/3/19 7:23 AM, Heiner Kallweit wrote:
> On 03.01.2019 03:09, Marek Vasut wrote:
>> On 12/23/18 10:16 AM, Marek Vasut wrote:
>>> On 12/22/18 6:39 PM, Heiner Kallweit wrote:
>>>> On 22.12.2018 00:35, Marek Vasut wrote:
>>>>> Add driver for the NXP TJA1100 and TJA1101 PHYs. These PHYs are special
>>>>> BroadRReach 100BaseT1 PHYs used in automotive.
>>>>>
>>>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>>>> Cc: Andrew Lunn <andrew@lunn.ch>
>>>>> Cc: Florian Fainelli <f.fainelli@gmail.com>
>>>>> Cc: Heiner Kallweit <hkallweit1@gmail.com>
>>>>> ---
>>>>> V2: - Use phy_modify(), phy_{set,clear}_bits()
>>>>>     - Drop enable argument of tja11xx_enable_link_control()
>>>>>     - Use PHY_BASIC_T1_FEATURES and dont modify supported/advertised
>>>>>       features in config_init callback
>>>>>     - Use genphy_soft_reset() instead of opencoding the reset sequence.
>>>>>     - Drop the aneg parts, since the PHY datasheet claims it does not
>>>>>       support aneg
>>>>> V3: - Replace clr with mask
>>>>>     - Add hwmon support
>>>>>     - Check commstat in tja11xx_read_status() only if link is up
>>>>>     - Use PHY_ID_MATCH_MODEL()
>>>>> ---
>>>>>  drivers/net/phy/Kconfig       |   6 +
>>>>>  drivers/net/phy/Makefile      |   1 +
>>>>>  drivers/net/phy/nxp-tja11xx.c | 424 ++++++++++++++++++++++++++++++++++
>>>>>  3 files changed, 431 insertions(+)
>>>>>  create mode 100644 drivers/net/phy/nxp-tja11xx.c
>>>>>
>>>> [...]
>>>>> +
>>>>> +struct tja11xx_phy_stats {
>>>>> +	const char	*string;
>>>>> +	u8		reg;
>>>>> +	u8		off;
>>>>> +	u16		mask;
>>>>> +};
>>>>> +
>>>> As written in my other mail, you could think of using
>>>> FIELD_GET() again. Things like
>>>> ... n, BIT(n), 
>>>> ... m, BIT(m), 
>>>> are simply redundant.
>>>
>>> Done
>>
>> And undone, FIELD_GET() requires a mask that is compile-time constant.
>> That's not the case here.
>>
>> Replicating what FIELD_GET() does at runtime is inefficient, so we can
>> as well encode the field and offset back into the table. Or is there a
>> better suggestion ?
>>
> You're right, FIELD_GET() requires a constant as mask. Then for the
> case here I'm not aware of a better solution than having also the mask
> in the table.

V4 is out then.
Marek Vasut Jan. 4, 2019, 2:53 a.m. UTC | #17 
On 1/4/19 3:19 AM, Marek Vasut wrote:

[...]

On a separate note, I was pondering about the master/slave mode of the
TJA11xx PHY. Right now, the driver only operates in Automatic mode,
however the PHY can be force-configured as either master or slave.
Would using .set_tunable with a new tunable type be a good way to expose
this to userspace ? The ethtool can then configure the PHY mode.
Florian Fainelli Jan. 4, 2019, 2:57 a.m. UTC | #18 
On 1/3/2019 6:53 PM, Marek Vasut wrote:
> On 1/4/19 3:19 AM, Marek Vasut wrote:
> 
> [...]
> 
> On a separate note, I was pondering about the master/slave mode of the
> TJA11xx PHY. Right now, the driver only operates in Automatic mode,
> however the PHY can be force-configured as either master or slave.
> Would using .set_tunable with a new tunable type be a good way to expose
> this to userspace ? The ethtool can then configure the PHY mode.

Using an ethtool u8 tunable would be fine, with something like:

0 -> automatic
1 -> force master
2 -> force slave
Marek Vasut Jan. 4, 2019, 5:40 a.m. UTC | #19 
On 1/4/19 3:57 AM, Florian Fainelli wrote:
> 
> 
> On 1/3/2019 6:53 PM, Marek Vasut wrote:
>> On 1/4/19 3:19 AM, Marek Vasut wrote:
>>
>> [...]
>>
>> On a separate note, I was pondering about the master/slave mode of the
>> TJA11xx PHY. Right now, the driver only operates in Automatic mode,
>> however the PHY can be force-configured as either master or slave.
>> Would using .set_tunable with a new tunable type be a good way to expose
>> this to userspace ? The ethtool can then configure the PHY mode.
> 
> Using an ethtool u8 tunable would be fine, with something like:
> 
> 0 -> automatic
> 1 -> force master
> 2 -> force slave

Fine, however I'll send it as a separate patch later.
How does this name/macro sound for the tunable ?

[ETHTOOL_PHY_BRR_MODE]  = "phy-broadrreach-mode",
diff mbox series

Patch

diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index 3d187cd50eb0..88b462ef1274 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -389,6 +389,12 @@  config NATIONAL_PHY
 	---help---
 	  Currently supports the DP83865 PHY.
 
+config NXP_TJA11XX_PHY
+	tristate "NXP TJA11xx PHYs support"
+	depends on HWMON
+	---help---
+	  Currently supports the NXP TJA1100 and TJA1101 PHY.
+
 config QSEMI_PHY
 	tristate "Quality Semiconductor PHYs"
 	---help---
diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
index 5805c0b7d60e..023e5db8c7cc 100644
--- a/drivers/net/phy/Makefile
+++ b/drivers/net/phy/Makefile
@@ -76,6 +76,7 @@  obj-$(CONFIG_MICROCHIP_PHY)	+= microchip.o
 obj-$(CONFIG_MICROCHIP_T1_PHY)	+= microchip_t1.o
 obj-$(CONFIG_MICROSEMI_PHY)	+= mscc.o
 obj-$(CONFIG_NATIONAL_PHY)	+= national.o
+obj-$(CONFIG_NXP_TJA11XX_PHY)	+= nxp-tja11xx.o
 obj-$(CONFIG_QSEMI_PHY)		+= qsemi.o
 obj-$(CONFIG_REALTEK_PHY)	+= realtek.o
 obj-$(CONFIG_RENESAS_PHY)	+= uPD60620.o
diff --git a/drivers/net/phy/nxp-tja11xx.c b/drivers/net/phy/nxp-tja11xx.c
new file mode 100644
index 000000000000..3b8d0917e2b0
--- /dev/null
+++ b/drivers/net/phy/nxp-tja11xx.c
@@ -0,0 +1,424 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/* NXP TJA1100 BroadRReach PHY driver
+ *
+ * Copyright (C) 2018 Marek Vasut <marex@denx.de>
+ */
+#include <linux/delay.h>
+#include <linux/ethtool.h>
+#include <linux/kernel.h>
+#include <linux/mii.h>
+#include <linux/module.h>
+#include <linux/phy.h>
+#include <linux/hwmon.h>
+
+#define PHY_ID_MASK			0xfffffff0
+#define PHY_ID_TJA1100			0x0180dc40
+#define PHY_ID_TJA1101			0x0180dd00
+
+#define MII_ECTRL			17
+#define MII_ECTRL_LINK_CONTROL		BIT(15)
+#define MII_ECTRL_POWER_MODE_MASK	GENMASK(14, 11)
+#define MII_ECTRL_POWER_MODE_NO_CHANGE	(0x0 << 11)
+#define MII_ECTRL_POWER_MODE_NORMAL	(0x3 << 11)
+#define MII_ECTRL_POWER_MODE_STANDBY	(0xc << 11)
+#define MII_ECTRL_CONFIG_EN		BIT(2)
+#define MII_ECTRL_WAKE_REQUEST		BIT(0)
+
+#define MII_CFG1			18
+#define MII_CFG1_AUTO_OP		BIT(14)
+#define MII_CFG1_SLEEP_CONFIRM		BIT(6)
+#define MII_CFG1_LED_MODE_MASK		GENMASK(5, 4)
+#define MII_CFG1_LED_MODE_LINKUP	0
+#define MII_CFG1_LED_ENABLE		BIT(3)
+
+#define MII_CFG2			19
+#define MII_CFG2_SLEEP_REQUEST_TO	GENMASK(1, 0)
+#define MII_CFG2_SLEEP_REQUEST_TO_16MS	0x3
+
+#define MII_INTSRC			21
+#define MII_INTSRC_TEMP_ERR		BIT(1)
+#define MII_INTSRC_UV_ERR		BIT(3)
+
+#define MII_COMMSTAT			23
+#define MII_COMMSTAT_LINK_UP		BIT(15)
+
+#define MII_GENSTAT			24
+#define MII_GENSTAT_PLL_LOCKED		BIT(14)
+
+#define MII_COMMCFG			27
+#define MII_COMMCFG_AUTO_OP		BIT(15)
+
+struct tja11xx_priv {
+	char		*hwmon_name;
+	struct device	*hwmon_dev;
+};
+
+struct tja11xx_phy_stats {
+	const char	*string;
+	u8		reg;
+	u8		off;
+	u16		mask;
+};
+
+static struct tja11xx_phy_stats tja11xx_hw_stats[] = {
+	{ "phy_symbol_error_count", 20, 0, 0xffff },
+	{ "phy_polarity_detect", 25, 6, BIT(6) },
+	{ "phy_open_detect", 25, 7, BIT(7) },
+	{ "phy_short_detect", 25, 8, BIT(8) },
+	{ "phy_rem_rcvr_count", 26, 0, 0xff },
+	{ "phy_loc_rcvr_count", 26, 8, 0xff },
+};
+
+static int tja11xx_check(struct phy_device *phydev, u8 reg, u16 mask, u16 set)
+{
+	int i, ret;
+
+	for (i = 0; i < 200; i++) {
+		ret = phy_read(phydev, reg);
+		if (ret < 0)
+			return ret;
+
+		if ((ret & mask) == set)
+			return 0;
+
+		usleep_range(100, 150);
+	}
+
+	return -ETIMEDOUT;
+}
+
+static int phy_modify_check(struct phy_device *phydev, u8 reg,
+			    u16 mask, u16 set)
+{
+	int ret;
+
+	ret = phy_modify(phydev, reg, mask, set);
+	if (ret)
+		return ret;
+
+	return tja11xx_check(phydev, reg, mask, set);
+}
+
+static int tja11xx_enable_reg_write(struct phy_device *phydev)
+{
+	return phy_set_bits(phydev, MII_ECTRL, MII_ECTRL_CONFIG_EN);
+}
+
+static int tja11xx_enable_link_control(struct phy_device *phydev)
+{
+	return phy_set_bits(phydev, MII_ECTRL, MII_ECTRL_LINK_CONTROL);
+}
+
+static int tja11xx_wakeup(struct phy_device *phydev)
+{
+	int ret;
+
+	ret = phy_read(phydev, MII_ECTRL);
+	if (ret < 0)
+		return ret;
+
+	switch (ret & MII_ECTRL_POWER_MODE_MASK) {
+	case MII_ECTRL_POWER_MODE_NO_CHANGE:
+		break;
+	case MII_ECTRL_POWER_MODE_NORMAL:
+		ret = phy_set_bits(phydev, MII_ECTRL, MII_ECTRL_WAKE_REQUEST);
+		if (ret)
+			return ret;
+
+		ret = phy_clear_bits(phydev, MII_ECTRL, MII_ECTRL_WAKE_REQUEST);
+		if (ret)
+			return ret;
+		break;
+	case MII_ECTRL_POWER_MODE_STANDBY:
+		ret = phy_modify_check(phydev, MII_ECTRL,
+				       MII_ECTRL_POWER_MODE_MASK,
+				       MII_ECTRL_POWER_MODE_STANDBY);
+		if (ret)
+			return ret;
+
+		ret = phy_modify(phydev, MII_ECTRL, MII_ECTRL_POWER_MODE_MASK,
+				 MII_ECTRL_POWER_MODE_NORMAL);
+		if (ret)
+			return ret;
+
+		ret = phy_modify_check(phydev, MII_GENSTAT,
+				       MII_GENSTAT_PLL_LOCKED,
+				       MII_GENSTAT_PLL_LOCKED);
+		if (ret)
+			return ret;
+
+		return tja11xx_enable_link_control(phydev);
+	default:
+		break;
+	}
+
+	return 0;
+}
+
+static int tja11xx_soft_reset(struct phy_device *phydev)
+{
+	int ret;
+
+	ret = tja11xx_enable_reg_write(phydev);
+	if (ret)
+		return ret;
+
+	return genphy_soft_reset(phydev);
+}
+
+static int tja11xx_config_init(struct phy_device *phydev)
+{
+	int ret;
+
+	ret = tja11xx_enable_reg_write(phydev);
+	if (ret)
+		return ret;
+
+	phydev->irq = PHY_POLL;
+	phydev->autoneg = AUTONEG_DISABLE;
+	phydev->speed = SPEED_100;
+	phydev->duplex = DUPLEX_FULL;
+	phydev->pause = 0;
+	phydev->asym_pause = 0;
+
+	switch (phydev->phy_id & PHY_ID_MASK) {
+	case PHY_ID_TJA1100:
+		ret = phy_modify(phydev, MII_CFG1,
+				 MII_CFG1_AUTO_OP | MII_CFG1_LED_MODE_MASK |
+				 MII_CFG1_LED_ENABLE,
+				 MII_CFG1_AUTO_OP | MII_CFG1_LED_MODE_LINKUP |
+				 MII_CFG1_LED_ENABLE);
+		if (ret)
+			return ret;
+		break;
+	case PHY_ID_TJA1101:
+		ret = phy_set_bits(phydev, MII_COMMCFG, MII_COMMCFG_AUTO_OP);
+		if (ret)
+			return ret;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	ret = phy_clear_bits(phydev, MII_CFG1, MII_CFG1_SLEEP_CONFIRM);
+	if (ret)
+		return ret;
+
+	ret = phy_modify(phydev, MII_CFG2, MII_CFG2_SLEEP_REQUEST_TO,
+			 MII_CFG2_SLEEP_REQUEST_TO_16MS);
+	if (ret)
+		return ret;
+
+	ret = tja11xx_wakeup(phydev);
+	if (ret < 0)
+		return ret;
+
+	/* ACK interrupts by reading the status register */
+	ret = phy_read(phydev, MII_INTSRC);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
+static int tja11xx_read_status(struct phy_device *phydev)
+{
+	int ret;
+
+	ret = genphy_update_link(phydev);
+	if (ret)
+		return ret;
+
+	if (phydev->link) {
+		ret = phy_read(phydev, MII_COMMSTAT);
+		if (ret < 0)
+			return ret;
+
+		if (!(ret & MII_COMMSTAT_LINK_UP))
+			phydev->link = 0;
+	}
+
+	return 0;
+}
+
+static int tja11xx_get_sset_count(struct phy_device *phydev)
+{
+	return ARRAY_SIZE(tja11xx_hw_stats);
+}
+
+static void tja11xx_get_strings(struct phy_device *phydev, u8 *data)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(tja11xx_hw_stats); i++) {
+		strncpy(data + i * ETH_GSTRING_LEN,
+			tja11xx_hw_stats[i].string, ETH_GSTRING_LEN);
+	}
+}
+
+static void tja11xx_get_stats(struct phy_device *phydev,
+			      struct ethtool_stats *stats, u64 *data)
+{
+	int i, ret;
+
+	for (i = 0; i < ARRAY_SIZE(tja11xx_hw_stats); i++) {
+		ret = phy_read(phydev, tja11xx_hw_stats[i].reg);
+		if (ret < 0) {
+			data[i] = U64_MAX;
+		} else {
+			data[i] = ret & tja11xx_hw_stats[i].mask;
+			data[i] >>= tja11xx_hw_stats[i].off;
+		}
+	}
+}
+
+static int tja11xx_hwmon_read(struct device *dev,
+			      enum hwmon_sensor_types type,
+			      u32 attr, int channel, long *value)
+{
+	struct phy_device *phydev = dev_get_drvdata(dev);
+	int ret;
+
+	switch (attr) {
+	case hwmon_in_lcrit_alarm:
+		ret = phy_read(phydev, MII_INTSRC);
+		if (ret < 0)
+			return ret;
+
+		*value = !!(ret & MII_INTSRC_TEMP_ERR);
+		return 0;
+	case hwmon_temp_crit_alarm:
+		ret = phy_read(phydev, MII_INTSRC);
+		if (ret < 0)
+			return ret;
+
+		*value = !!(ret & MII_INTSRC_TEMP_ERR);
+		return 0;
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static umode_t tja11xx_hwmon_is_visible(const void *data,
+					enum hwmon_sensor_types type,
+					u32 attr, int channel)
+{
+	if (type == hwmon_in && attr == hwmon_in_lcrit_alarm)
+		return 0444;
+
+	if (type == hwmon_temp && attr == hwmon_temp_crit_alarm)
+		return 0444;
+
+	return 0;
+}
+
+static u32 tja11xx_hwmon_in_config[] = {
+	HWMON_I_LCRIT_ALARM,
+	0
+};
+
+static const struct hwmon_channel_info tja11xx_hwmon_in = {
+	.type		= hwmon_in,
+	.config		= tja11xx_hwmon_in_config,
+};
+
+static u32 tja11xx_hwmon_temp_config[] = {
+	HWMON_T_CRIT_ALARM,
+	0
+};
+
+static const struct hwmon_channel_info tja11xx_hwmon_temp = {
+	.type		= hwmon_temp,
+	.config		= tja11xx_hwmon_temp_config,
+};
+
+static const struct hwmon_channel_info *tja11xx_hwmon_info[] = {
+	&tja11xx_hwmon_in,
+	&tja11xx_hwmon_temp,
+	NULL
+};
+
+static const struct hwmon_ops tja11xx_hwmon_hwmon_ops = {
+	.is_visible	= tja11xx_hwmon_is_visible,
+	.read		= tja11xx_hwmon_read,
+};
+
+static const struct hwmon_chip_info tja11xx_hwmon_chip_info = {
+	.ops		= &tja11xx_hwmon_hwmon_ops,
+	.info		= tja11xx_hwmon_info,
+};
+
+static int tja11xx_probe(struct phy_device *phydev)
+{
+	struct device *dev = &phydev->mdio.dev;
+	struct tja11xx_priv *priv;
+	int i;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->hwmon_name = devm_kstrdup(dev, dev_name(dev), GFP_KERNEL);
+	if (!priv->hwmon_name)
+		return -ENODEV;
+
+	for (i = 0; priv->hwmon_name[i]; i++)
+		if (hwmon_is_bad_char(priv->hwmon_name[i]))
+			priv->hwmon_name[i] = '_';
+
+	priv->hwmon_dev =
+		devm_hwmon_device_register_with_info(dev, priv->hwmon_name,
+						     phydev,
+						     &tja11xx_hwmon_chip_info,
+						     NULL);
+
+	return PTR_ERR_OR_ZERO(priv->hwmon_dev);
+}
+
+static struct phy_driver tja11xx_driver[] = {
+	{
+		PHY_ID_MATCH_MODEL(PHY_ID_TJA1100),
+		.name		= "NXP TJA1100",
+		.features       = PHY_BASIC_T1_FEATURES,
+		.probe		= tja11xx_probe,
+		.soft_reset	= tja11xx_soft_reset,
+		.config_init	= tja11xx_config_init,
+		.read_status	= tja11xx_read_status,
+		.suspend	= genphy_suspend,
+		.resume		= genphy_resume,
+		.set_loopback   = genphy_loopback,
+		/* Statistics */
+		.get_sset_count = tja11xx_get_sset_count,
+		.get_strings	= tja11xx_get_strings,
+		.get_stats	= tja11xx_get_stats,
+	}, {
+		PHY_ID_MATCH_MODEL(PHY_ID_TJA1101),
+		.name		= "NXP TJA1101",
+		.features       = PHY_BASIC_T1_FEATURES,
+		.probe		= tja11xx_probe,
+		.soft_reset	= tja11xx_soft_reset,
+		.config_init	= tja11xx_config_init,
+		.read_status	= tja11xx_read_status,
+		.suspend	= genphy_suspend,
+		.resume		= genphy_resume,
+		.set_loopback   = genphy_loopback,
+		/* Statistics */
+		.get_sset_count = tja11xx_get_sset_count,
+		.get_strings	= tja11xx_get_strings,
+		.get_stats	= tja11xx_get_stats,
+	}
+};
+
+module_phy_driver(tja11xx_driver);
+
+static struct mdio_device_id __maybe_unused tja11xx_tbl[] = {
+	{ PHY_ID_MATCH_MODEL(PHY_ID_TJA1100) },
+	{ PHY_ID_MATCH_MODEL(PHY_ID_TJA1101) },
+	{ }
+};
+
+MODULE_DEVICE_TABLE(mdio, tja11xx_tbl);
+
+MODULE_AUTHOR("Marek Vasut <marex@denx.de>");
+MODULE_DESCRIPTION("NXP TJA11xx BoardR-Reach PHY driver");
+MODULE_LICENSE("GPL");