* [PATCH net-next v2 0/2] net: phy: aquantia: add hwmon support @ 2019-02-24 21:34 Heiner Kallweit 2019-02-24 21:35 ` [PATCH net-next v2 1/2] net: phy: aquantia: rename aquantia.c to aquantia_main.c Heiner Kallweit 2019-02-24 21:36 ` [PATCH net-next v2 2/2] net: phy: aquantia: add hwmon support Heiner Kallweit 0 siblings, 2 replies; 6+ messages in thread From: Heiner Kallweit @ 2019-02-24 21:34 UTC (permalink / raw) To: Andrew Lunn, Florian Fainelli, David Miller; +Cc: netdev This series adds HWMON support for the temperature sensor and the related alarms on the 107/108/109 chips. v2: - remove struct aqr_priv - rename header file to aquantia.h Heiner Kallweit (2): net: phy: aquantia: rename aquantia.c to aquantia_main.c net: phy: aquantia: add hwmon support drivers/net/phy/Makefile | 1 + drivers/net/phy/aquantia.h | 16 ++ drivers/net/phy/aquantia_hwmon.c | 245 ++++++++++++++++++ .../net/phy/{aquantia.c => aquantia_main.c} | 4 + 4 files changed, 266 insertions(+) create mode 100644 drivers/net/phy/aquantia.h create mode 100644 drivers/net/phy/aquantia_hwmon.c rename drivers/net/phy/{aquantia.c => aquantia_main.c} (99%) -- 2.20.1 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH net-next v2 1/2] net: phy: aquantia: rename aquantia.c to aquantia_main.c 2019-02-24 21:34 [PATCH net-next v2 0/2] net: phy: aquantia: add hwmon support Heiner Kallweit @ 2019-02-24 21:35 ` Heiner Kallweit 2019-02-24 21:36 ` [PATCH net-next v2 2/2] net: phy: aquantia: add hwmon support Heiner Kallweit 1 sibling, 0 replies; 6+ messages in thread From: Heiner Kallweit @ 2019-02-24 21:35 UTC (permalink / raw) To: Andrew Lunn, Florian Fainelli, David Miller; +Cc: netdev Rename aquantia.c to aquantia_main.c to be prepared for adding new functionality to separate source code files. Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> --- drivers/net/phy/Makefile | 1 + drivers/net/phy/{aquantia.c => aquantia_main.c} | 0 2 files changed, 1 insertion(+) rename drivers/net/phy/{aquantia.c => aquantia_main.c} (100%) diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile index 5805c0b7d..b0845adaf 100644 --- a/drivers/net/phy/Makefile +++ b/drivers/net/phy/Makefile @@ -45,6 +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 obj-$(CONFIG_AQUANTIA_PHY) += aquantia.o obj-$(CONFIG_ASIX_PHY) += asix.o obj-$(CONFIG_AT803X_PHY) += at803x.o diff --git a/drivers/net/phy/aquantia.c b/drivers/net/phy/aquantia_main.c similarity index 100% rename from drivers/net/phy/aquantia.c rename to drivers/net/phy/aquantia_main.c -- 2.20.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH net-next v2 2/2] net: phy: aquantia: add hwmon support 2019-02-24 21:34 [PATCH net-next v2 0/2] net: phy: aquantia: add hwmon support Heiner Kallweit 2019-02-24 21:35 ` [PATCH net-next v2 1/2] net: phy: aquantia: rename aquantia.c to aquantia_main.c Heiner Kallweit @ 2019-02-24 21:36 ` Heiner Kallweit 2019-02-25 2:17 ` Florian Fainelli 1 sibling, 1 reply; 6+ messages in thread From: Heiner Kallweit @ 2019-02-24 21:36 UTC (permalink / raw) To: Andrew Lunn, Florian Fainelli, David Miller; +Cc: netdev 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.yoush@cogentembedded.com> Signed-off-by: Andrew Lunn <andrew@lunn.ch> Signed-off-by: Heiner Kallweit <hkallweit1@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 obj-$(CONFIG_AQUANTIA_PHY) += aquantia.o obj-$(CONFIG_ASIX_PHY) += asix.o obj-$(CONFIG_AT803X_PHY) += at803x.o diff --git a/drivers/net/phy/aquantia.h b/drivers/net/phy/aquantia.h new file mode 100644 index 000000000..5a16caab7 --- /dev/null +++ b/drivers/net/phy/aquantia.h @@ -0,0 +1,16 @@ +/* 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/device.h> +#include <linux/phy.h> + +#if IS_REACHABLE(CONFIG_HWMON) +int aqr_hwmon_probe(struct phy_device *phydev); +#else +static inline int aqr_hwmon_probe(struct phy_device *phydev) { return 0; } +#endif diff --git a/drivers/net/phy/aquantia_hwmon.c b/drivers/net/phy/aquantia_hwmon.c new file mode 100644 index 000000000..9c77d149e --- /dev/null +++ b/drivers/net/phy/aquantia_hwmon.c @@ -0,0 +1,245 @@ +// 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.h" + +/* Vendor specific 1, MDIO_MMD_VEND2 */ +#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) + +#if IS_REACHABLE(CONFIG_HWMON) + +static umode_t aqr_hwmon_is_visible(const void *data, + enum hwmon_sensor_types type, + u32 attr, int channel) +{ + if (type != hwmon_temp) + return 0; + + switch (attr) { + case hwmon_temp_input: + case hwmon_temp_min_alarm: + case hwmon_temp_max_alarm: + case hwmon_temp_lcrit_alarm: + case hwmon_temp_crit_alarm: + return 0444; + case hwmon_temp_min: + case hwmon_temp_max: + case hwmon_temp_lcrit: + case hwmon_temp_crit: + return 0644; + default: + return 0; + } +} + +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; + + *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; + + 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; +} + +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; + + return aqr_hwmon_get(phydev, VEND1_THERMAL_STAT1, value); + + case hwmon_temp_lcrit: + return aqr_hwmon_get(phydev, VEND1_THERMAL_PROV_LOW_TEMP_FAIL, + value); + case hwmon_temp_min: + return aqr_hwmon_get(phydev, VEND1_THERMAL_PROV_LOW_TEMP_WARN, + value); + case hwmon_temp_max: + return aqr_hwmon_get(phydev, VEND1_THERMAL_PROV_HIGH_TEMP_WARN, + value); + case hwmon_temp_crit: + return aqr_hwmon_get(phydev, VEND1_THERMAL_PROV_HIGH_TEMP_FAIL, + value); + case hwmon_temp_lcrit_alarm: + return aqr_hwmon_status1(phydev, + VEND1_GENERAL_STAT1_LOW_TEMP_FAIL, + value); + case hwmon_temp_min_alarm: + return aqr_hwmon_status1(phydev, + VEND1_GENERAL_STAT1_LOW_TEMP_WARN, + value); + case hwmon_temp_max_alarm: + return aqr_hwmon_status1(phydev, + VEND1_GENERAL_STAT1_HIGH_TEMP_WARN, + value); + case hwmon_temp_crit_alarm: + return aqr_hwmon_status1(phydev, + VEND1_GENERAL_STAT1_HIGH_TEMP_FAIL, + value); + default: + return -EOPNOTSUPP; + } +} + +static int aqr_hwmon_write(struct device *dev, enum hwmon_sensor_types type, + u32 attr, int channel, long value) +{ + struct phy_device *phydev = dev_get_drvdata(dev); + + if (type != hwmon_temp) + return -EOPNOTSUPP; + + switch (attr) { + case hwmon_temp_lcrit: + return aqr_hwmon_set(phydev, VEND1_THERMAL_PROV_LOW_TEMP_FAIL, + value); + case hwmon_temp_min: + return aqr_hwmon_set(phydev, VEND1_THERMAL_PROV_LOW_TEMP_WARN, + value); + case hwmon_temp_max: + return aqr_hwmon_set(phydev, VEND1_THERMAL_PROV_HIGH_TEMP_WARN, + value); + case hwmon_temp_crit: + return aqr_hwmon_set(phydev, VEND1_THERMAL_PROV_HIGH_TEMP_FAIL, + value); + default: + return -EOPNOTSUPP; + } +} + +static const struct hwmon_ops aqr_hwmon_ops = { + .is_visible = aqr_hwmon_is_visible, + .read = aqr_hwmon_read, + .write = aqr_hwmon_write, +}; + +static u32 aqr_hwmon_chip_config[] = { + HWMON_C_REGISTER_TZ, + 0, +}; + +static const struct hwmon_channel_info aqr_hwmon_chip = { + .type = hwmon_chip, + .config = aqr_hwmon_chip_config, +}; + +static u32 aqr_hwmon_temp_config[] = { + HWMON_T_INPUT | + HWMON_T_MAX | HWMON_T_MIN | + HWMON_T_MAX_ALARM | HWMON_T_MIN_ALARM | + HWMON_T_CRIT | HWMON_T_LCRIT | + HWMON_T_CRIT_ALARM | HWMON_T_LCRIT_ALARM, + 0, +}; + +static const struct hwmon_channel_info aqr_hwmon_temp = { + .type = hwmon_temp, + .config = aqr_hwmon_temp_config, +}; + +static const struct hwmon_channel_info *aqr_hwmon_info[] = { + &aqr_hwmon_chip, + &aqr_hwmon_temp, + NULL, +}; + +static const struct hwmon_chip_info aqr_hwmon_chip_info = { + .ops = &aqr_hwmon_ops, + .info = aqr_hwmon_info, +}; + +int aqr_hwmon_probe(struct phy_device *phydev) +{ + struct device *dev = &phydev->mdio.dev; + struct device *hwmon_dev; + char *hwmon_name; + int i, j; + + 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'; + + hwmon_dev = devm_hwmon_device_register_with_info(dev, hwmon_name, + phydev, &aqr_hwmon_chip_info, NULL); + + return PTR_ERR_OR_ZERO(hwmon_dev); +} + +#endif diff --git a/drivers/net/phy/aquantia_main.c b/drivers/net/phy/aquantia_main.c index 0f0eb5682..37218e5d7 100644 --- a/drivers/net/phy/aquantia_main.c +++ b/drivers/net/phy/aquantia_main.c @@ -12,6 +12,8 @@ #include <linux/delay.h> #include <linux/phy.h> +#include "aquantia.h" + #define PHY_ID_AQ1202 0x03a1b445 #define PHY_ID_AQ2104 0x03a1b460 #define PHY_ID_AQR105 0x03a1b4a2 @@ -231,6 +233,7 @@ static struct phy_driver aqr_driver[] = { .name = "Aquantia AQR107", .aneg_done = genphy_c45_aneg_done, .get_features = genphy_c45_pma_read_abilities, + .probe = aqr_hwmon_probe, .config_aneg = aqr_config_aneg, .config_intr = aqr_config_intr, .ack_interrupt = aqr_ack_interrupt, @@ -241,6 +244,7 @@ static struct phy_driver aqr_driver[] = { .name = "Aquantia AQCS109", .aneg_done = genphy_c45_aneg_done, .get_features = genphy_c45_pma_read_abilities, + .probe = aqr_hwmon_probe, .config_init = aqcs109_config_init, .config_aneg = aqr_config_aneg, .config_intr = aqr_config_intr, -- 2.20.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH net-next v2 2/2] net: phy: aquantia: add hwmon support 2019-02-24 21:36 ` [PATCH net-next v2 2/2] net: phy: aquantia: add hwmon support Heiner Kallweit @ 2019-02-25 2:17 ` Florian Fainelli 2019-02-25 4:02 ` Andrew Lunn 2019-02-25 7:34 ` Heiner Kallweit 0 siblings, 2 replies; 6+ messages in thread From: Florian Fainelli @ 2019-02-25 2:17 UTC (permalink / raw) To: Heiner Kallweit, Andrew Lunn, David Miller; +Cc: netdev 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.yoush@cogentembedded.com> Should not Nikita be the author for that patch? Some minor nits below: > Signed-off-by: Andrew Lunn <andrew@lunn.ch> > Signed-off-by: Heiner Kallweit <hkallweit1@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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net-next v2 2/2] net: phy: aquantia: add hwmon support 2019-02-25 2:17 ` Florian Fainelli @ 2019-02-25 4:02 ` Andrew Lunn 2019-02-25 7:34 ` Heiner Kallweit 1 sibling, 0 replies; 6+ messages in thread From: Andrew Lunn @ 2019-02-25 4:02 UTC (permalink / raw) To: Florian Fainelli; +Cc: Heiner Kallweit, David Miller, netdev On Sun, Feb 24, 2019 at 06:17:49PM -0800, 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.yoush@cogentembedded.com> > > Should not Nikita be the author for that patch? Some minor nits below: > > > Signed-off-by: Andrew Lunn <andrew@lunn.ch> > > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> Hi Florian The history is convoluted. I did the initial implementation, but at the time i did not have the hardware. So it got passed to Nikita who did. He made it actually work, and passed patches back. I then passed it onto Heiner who has done all the work on the driver and core for C45. Andrew ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net-next v2 2/2] net: phy: aquantia: add hwmon support 2019-02-25 2:17 ` Florian Fainelli 2019-02-25 4:02 ` Andrew Lunn @ 2019-02-25 7:34 ` Heiner Kallweit 1 sibling, 0 replies; 6+ messages in thread From: Heiner Kallweit @ 2019-02-25 7:34 UTC (permalink / raw) To: Florian Fainelli, Andrew Lunn, David Miller; +Cc: netdev 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.yoush@cogentembedded.com> > > Should not Nikita be the author for that patch? Some minor nits below: > >> Signed-off-by: Andrew Lunn <andrew@lunn.ch> >> Signed-off-by: Heiner Kallweit <hkallweit1@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. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-02-25 7:36 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-02-24 21:34 [PATCH net-next v2 0/2] net: phy: aquantia: add hwmon support Heiner Kallweit 2019-02-24 21:35 ` [PATCH net-next v2 1/2] net: phy: aquantia: rename aquantia.c to aquantia_main.c Heiner Kallweit 2019-02-24 21:36 ` [PATCH net-next v2 2/2] net: phy: aquantia: add hwmon support Heiner Kallweit 2019-02-25 2:17 ` Florian Fainelli 2019-02-25 4:02 ` Andrew Lunn 2019-02-25 7:34 ` Heiner Kallweit
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.