From mboxrd@z Thu Jan  1 00:00:00 1970
From: Heiner Kallweit <hkallweit1@gmail.com>
Subject: Re: [PATCH V2] net: phy: tja11xx: Add TJA11xx PHY driver
Date: Sat, 15 Dec 2018 18:38:08 +0100
Message-ID: <42aad282-2100-87bc-7145-a19f8f46afa6@gmail.com>
References: <20181214161149.6493-1-marex@denx.de>
 <20181215170153.GA5922@lunn.ch>
Mime-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit
Cc: netdev@vger.kernel.org, f.fainelli@gmail.com
To: Andrew Lunn <andrew@lunn.ch>, Marek Vasut <marex@denx.de>
Return-path: <netdev-owner@vger.kernel.org>
Received: from mail-wr1-f65.google.com ([209.85.221.65]:42369 "EHLO
        mail-wr1-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1726494AbeLORiO (ORCPT
        <rfc822;netdev@vger.kernel.org>); Sat, 15 Dec 2018 12:38:14 -0500
Received: by mail-wr1-f65.google.com with SMTP id q18so8355017wrx.9
        for <netdev@vger.kernel.org>; Sat, 15 Dec 2018 09:38:13 -0800 (PST)
In-Reply-To: <20181215170153.GA5922@lunn.ch>
Content-Language: en-US
Sender: netdev-owner@vger.kernel.org
List-ID: <netdev.vger.kernel.org>

On 15.12.2018 18:01, Andrew Lunn wrote:
>> +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
>