Re: [PATCH net-next 2/2] net: phy: aquantia: add hwmon support

[Heiner Kallweit <hkallweit1@gmail.com>]

On 24.02.2019 21:29, Andrew Lunn wrote:
>> diff --git a/drivers/net/phy/aquantia_hwmon.c b/drivers/net/phy/aquantia_hwmon.c
>> new file mode 100644
>> index 000000000..c0dd695f6
>> --- /dev/null
>> +++ b/drivers/net/phy/aquantia_hwmon.c
>> @@ -0,0 +1,263 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/* HWMON driver for Aquantia PHY
>> + *
>> + * Author: Nikita Yushchenko <nikita.yoush@cogentembedded.com>
>> + * Author: Andrew Lunn <andrew@lunn.ch>
>> + * Author: Heiner Kallweit <hkallweit1@gmail.com>
>> + */
>> +
>> +#include <linux/phy.h>
>> +#include <linux/device.h>
>> +#include <linux/ctype.h>
>> +#include <linux/hwmon.h>
>> +
>> +#include "aquantia_hwmon.h"
>> +
>> +/* Vendor specific 1, MDIO_MMD_VEND1 */
>> +#define VEND1_THERMAL_PROV_HIGH_TEMP_FAIL	0xc421
>> +#define VEND1_THERMAL_PROV_LOW_TEMP_FAIL	0xc422
>> +#define VEND1_THERMAL_PROV_HIGH_TEMP_WARN	0xc423
>> +#define VEND1_THERMAL_PROV_LOW_TEMP_WARN	0xc424
>> +#define VEND1_THERMAL_STAT1			0xc820
>> +#define VEND1_THERMAL_STAT2			0xc821
>> +#define VEND1_THERMAL_STAT2_VALID		BIT(0)
>> +#define VEND1_GENERAL_STAT1			0xc830
>> +#define VEND1_GENERAL_STAT1_HIGH_TEMP_FAIL	BIT(14)
>> +#define VEND1_GENERAL_STAT1_LOW_TEMP_FAIL	BIT(13)
>> +#define VEND1_GENERAL_STAT1_HIGH_TEMP_WARN	BIT(12)
>> +#define VEND1_GENERAL_STAT1_LOW_TEMP_WARN	BIT(11)
>> +
>> +struct aqr_priv {
>> +	struct device *hwmon_dev;
>> +	char *hwmon_name;
>> +};
> 
> Hi Heiner
> 
> It seems a bit odd having the driver private structure here. I expect
> with time we are going to need other things in it which are not
> HWMON. e.g many of the statistics counters are clear on read. So we
> need to keep the running totals somewhere.
> 
> I would keep the probe code and the allocation of this structure in
> the main driver file.
> 
I just see that we don't need struct aqr_priv at all for hwmon
because it uses device-managed versions of the relevant functions.
So we can add such a struct when it's actually needed.

>     Andrew
> 
Heiner