Re: [PATCH V2] net: phy: tja11xx: Add TJA11xx PHY driver

On 12/15/2018 06:38 PM, Heiner Kallweit wrote:

On 15.12.2018 18:01, Andrew Lunn wrote:

quoted

quoted

+static struct tja11xx_phy_stats tja11xx_hw_stats[] = {
+	{ "phy_symbol_error_count", 20, 0, 0xffff },
+	{ "phy_overtemp_error", 21, 1, BIT(1) },
+	{ "phy_undervolt_error", 21, 3, BIT(3) },
+	{ "phy_polarity_detect", 25, 6, BIT(6) },
+	{ "phy_open_detect", 25, 7, BIT(7) },
+	{ "phy_short_detect", 25, 8, BIT(8) },

Hi Marek

You have a number of one bit counters here, which is pretty unusual.
The names also don't really suggest they are counters.

Apart from few counters the values seem to be flags. I just think
it could be done in a little bit more readable form, e.g. instead of
{ "phy_short_detect", 25, 8, BIT(8) } use
{ "phy_short_detect", 25, BIT(8) } and in tja11xx_get_stats() then
use FIELD_GET (see linux/bitfield.h).

This doesn't work with the counters, it only works with flags. The array
contains both.

The idea of HWMON attributes sounds good to me because it allows
to use the flags to trigger actions in a structured way. And I
assume in case of e.g. "PHY undervolt" some monitoring system
would like to be informed (especially because we talk about
automotive here).

I added HWMON_T_CRIT_ALARM and HWMON_I_LCRIT_ALARM for
phy_overtemp_error and phy_undervolt_error respectively.
Are there any other hwmon attributes I can use to replace the flags in
the tja11xx_hw_stats array ?

-- 
Best regards,
Marek Vasut