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

On 15.12.2018 18:01, Andrew Lunn wrote:

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).

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).

Florian, Heiner, do we want to allow this?

	 Andrew