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 <ma...@denx.de>
> Cc: Andrew Lunn <and...@lunn.ch>
> Cc: Florian Fainelli <f.faine...@gmail.com>
> Cc: Heiner Kallweit <hkallwe...@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.
