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?
[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?
> +
> + *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?
> +
> +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.
--
Florian
