On 25.02.2019 03:17, Florian Fainelli wrote:
> Le 2/24/19 à 1:36 PM, Heiner Kallweit a écrit :
>> This adds HWMON support for the temperature sensor and the related
>> alarms on the 107/108/109 chips. This patch is based on work from
>> Nikita and Andrew. I added:
>> - support for changing alarm thresholds via sysfs
>> - move HWMON code to a separate source file to improve maintainability
>> - smaller changes like using IS_REACHABLE instead of ifdef
>> (avoids problems if PHY driver is built in and HWMON is a module)
>>
>> v2:
>> - remove struct aqr_priv
>> - rename header file to aquantia.h
>>
>> Signed-off-by: Nikita Yushchenko <nikita.yo...@cogentembedded.com>
>
> Should not Nikita be the author for that patch? Some minor nits below:
>
>> Signed-off-by: Andrew Lunn <and...@lunn.ch>
>> Signed-off-by: Heiner Kallweit <hkallwe...@gmail.com>
>> ---
>> drivers/net/phy/Makefile | 2 +-
>> drivers/net/phy/aquantia.h | 16 ++
>> drivers/net/phy/aquantia_hwmon.c | 245 +++++++++++++++++++++++++++++++
>> drivers/net/phy/aquantia_main.c | 4 +
>> 4 files changed, 266 insertions(+), 1 deletion(-)
>> create mode 100644 drivers/net/phy/aquantia.h
>> create mode 100644 drivers/net/phy/aquantia_hwmon.c
>>
>> diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
>> index b0845adaf..c48596626 100644
>> --- a/drivers/net/phy/Makefile
>> +++ b/drivers/net/phy/Makefile
>> @@ -45,7 +45,7 @@ sfp-obj-$(CONFIG_SFP) += sfp-bus.o
>> obj-y += $(sfp-obj-y) $(sfp-obj-m)
>>
>> obj-$(CONFIG_AMD_PHY) += amd.o
>> -aquantia-objs += aquantia_main.o
>> +aquantia-objs += aquantia_main.o aquantia_hwmon.o
>
> Since you are adding a stub if CONFIG_HWMON is not reachable, does not
> that mean you would want to do something like:
>
> ifdef CONFIG_HWMON
> aquanti-objs += aquantia_hwmon.o
> endif
>
> in this makefile?
>
Yepp, this should be changed as suggested by you.
> [snip]
>
>> +static int aqr_hwmon_get(struct phy_device *phydev, int reg, long *value)
>> +{
>> + int temp = phy_read_mmd(phydev, MDIO_MMD_VEND1, reg);
>> +
>> + if (temp < 0)
>> + return temp;
>> +
>> + /* register value is 2's complement with LSB = 1/256th degree Celsius */
>> + if (temp > 0x8000)
>> + temp -= 0x10000;
>
> Would decimal or BIT() notation be easier to use/read here?
>
For the comparison I agree, we can write it as: if (temp & BIT(15))
And yes, the offset we could write decimal.
An alternative could be to write: temp = (s16)temp;
I tested it and it works, but I'm not sure whether this is covered
by C standard and works with all supported compilers.
>> +
>> + *value = temp * 1000 / 256;
>> +
>> + return 0;
>> +}
>> +
>> +static int aqr_hwmon_set(struct phy_device *phydev, int reg, long value)
>> +{
>> + int temp;
>> +
>> + if (value >= 128000 || value < -128000)
>> + return -ERANGE;
>> +
>> + temp = value * 256 / 1000;
>> +
>> + if (temp < 0)
>> + temp += 0x10000;
>
> Likewise
>
>> +
>> + return phy_write_mmd(phydev, MDIO_MMD_VEND1, reg, temp);
>> +}
>> +
>> +static int aqr_hwmon_status1(struct phy_device *phydev, int bit, long
>> *value)
>> +{
>> + int reg = phy_read_mmd(phydev, MDIO_MMD_VEND1, VEND1_GENERAL_STAT1);
>> +
>> + if (reg < 0)
>> + return reg;
>> +
>> + *value = !!(reg & bit);
>> +
>> + return 0;
>> +}
>
> Can you also make this function take a register offset as parameter such
> that you can eliminate open coding that in the case hwmon_temp_input below?
>
OK
>> +
>> +static int aqr_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
>> + u32 attr, int channel, long *value)
>> +{
>> + struct phy_device *phydev = dev_get_drvdata(dev);
>> + int reg;
>> +
>> + if (type != hwmon_temp)
>> + return -EOPNOTSUPP;
>> +
>> + switch (attr) {
>> + case hwmon_temp_input:
>> + reg = phy_read_mmd(phydev, MDIO_MMD_VEND1,
>> + VEND1_THERMAL_STAT2);
>> + if (reg < 0)
>> + return reg;
>> + if (!(reg & VEND1_THERMAL_STAT2_VALID))
>> + return -EIO;
>
> And use that helper here.
>
>> +
>> + return aqr_hwmon_get(phydev, VEND1_THERMAL_STAT1, value);
>> +
>
> [snip]
>
>> + hwmon_name = devm_kstrdup(dev, dev_name(dev), GFP_KERNEL);
>> + if (!hwmon_name)
>> + return -ENOMEM;
>> +
>> + for (i = j = 0; hwmon_name[i]; i++) {
>> + if (isalnum(hwmon_name[i])) {
>> + if (i != j)
>> + hwmon_name[j] = hwmon_name[i];
>> + j++;
>> + }
>> + }
>> + hwmon_name[j] = '\0';
>
> I am always baffled that drivers need to take care of these details...
> but that seems to be the way to go.
>
IIRC we still have a similar construction site in phylib.
If a PHY driver name includes e.g. a slash then sysfs is broken.
There should be a general helper e.g. in lib/string.c, because we have
this across subsystems wherever a device name is used in a sysfs path.
