| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20220825150025.GA240395@roeck-us.net>
Date: Thu, 25 Aug 2022 08:00:25 -0700
From: Guenter Roeck <linux@...ck-us.net>
To: Andreas Böhler <dev@...ehler.at>
Cc: Robert Marko <robert.marko@...tura.hr>,
Luka Perkov <luka.perkov@...tura.hr>,
Jean Delvare <jdelvare@...e.com>,
Rob Herring <robh+dt@...nel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
linux-hwmon@...r.kernel.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] hwmon: tps23861: add support for initializing the chip
On Thu, Aug 25, 2022 at 04:10:42PM +0200, Andreas Böhler wrote:
> The tps23861 driver does not initialize the chip and relies on it being
> in auto-mode by default. On some devices, these controllers default to
> OFF-Mode and hence cannot be used at all.
>
> This brings minimal support for initializing the controller in a user-
> defined mode.
>
> Tested on a TP-Link TL-SG2452P with 12x TI TPS23861 controllers.
>
> Signed-off-by: Andreas Böhler <dev@...ehler.at>
> ---
> .../bindings/hwmon/ti,tps23861.yaml | 76 +++++++++++++++++
> drivers/hwmon/tps23861.c | 81 +++++++++++++++++++
> 2 files changed, 157 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/hwmon/ti,tps23861.yaml b/Documentation/devicetree/bindings/hwmon/ti,tps23861.yaml
> index 3bc8e73dfbf0..ed3a703478fb 100644
> --- a/Documentation/devicetree/bindings/hwmon/ti,tps23861.yaml
> +++ b/Documentation/devicetree/bindings/hwmon/ti,tps23861.yaml
Devicetree bindings need to be a separate patch.
> @@ -35,6 +35,50 @@ required:
> - compatible
> - reg
>
> +patternProperties:
> + "^port@([0-3])$":
> + type: object
> + description: Represents ports of the device and their specific configuration.
> +
> + properties:
> + reg:
> + description: The port number
> + items:
> + minimum: 0
> + maximum: 3
> +
> + mode:
> + description: The operating mode the device should be initialized with
> + items:
> + - enum:
> + - auto
> + - semiauto
> + - manual
> + - off
> +
> + enable:
> + description: Whether the port should be enabled
> + items:
> + minimum: 0
> + maximum: 1
> +
> + power:
> + description: Whether the port should be powered on
> + items:
> + minimum: 0
> + maximum: 1
> +
> + poe_plus:
> + description: Whether the port should support PoE+
> + items:
> + minimum: 0
> + maximum: 1
> +
The above properties are out of scope for a hardware monitoring driver.
A hardware monitoring driver should only affect hardware monitoring
functionality, not chip operation. Port control functionality should
be implemented in a phy driver.
Having said this, I notice that the driver's 'enable' attribute is
misused (abused). It should enable or disable port monitoring, not
port functionality. That attribute should be removed from the driver.
Guenter
> + required:
> + - reg
> +
> + additionalProperties: false
> +
> additionalProperties: false
>
> examples:
> @@ -47,5 +91,37 @@ examples:
> compatible = "ti,tps23861";
> reg = <0x30>;
> shunt-resistor-micro-ohms = <255000>;
> +
> + port@0 {
> + reg = <0>;
> + mode = "auto";
> + enable = <1>;
> + power = <1>;
> + poe_plus = <1>;
> + };
> +
> + port@1 {
> + reg = <1>;
> + mode = "semiauto";
> + enable = <1>;
> + power = <0>;
> + poe_plus = <1>;
> + };
> +
> + port@2 {
> + reg = <2>;
> + mode = "manual";
> + enable = <0>;
> + power = <0>;
> + poe_plus = <0>;
> + };
> +
> + port@3 {
> + reg = <3>;
> + mode = "off";
> + enable = <0>;
> + power = <0>;
> + poe_plus = <1>;
> + };
> };
> };
> diff --git a/drivers/hwmon/tps23861.c b/drivers/hwmon/tps23861.c
> index 42762e87b014..27bf8716cf12 100644
> --- a/drivers/hwmon/tps23861.c
> +++ b/drivers/hwmon/tps23861.c
> @@ -85,6 +85,8 @@
> #define PORT_DETECT_CAPACITANCE_INVALID_DELTA 11
> #define PORT_DETECT_CAPACITANCE_OUT_OF_RANGE 12
> #define POE_PLUS 0x40
> +#define POE_PLUS_MASK(_port) \
> + GENMASK(_port + 4, _port + 4)
> #define OPERATING_MODE 0x12
> #define OPERATING_MODE_OFF 0
> #define OPERATING_MODE_MANUAL 1
> @@ -94,9 +96,22 @@
> #define OPERATING_MODE_PORT_2_MASK GENMASK(3, 2)
> #define OPERATING_MODE_PORT_3_MASK GENMASK(5, 4)
> #define OPERATING_MODE_PORT_4_MASK GENMASK(7, 6)
> +#define OPERATING_MODE_PORT(_mode, _port) \
> + (_mode << (_port * 2))
>
> +#define DISCONNECT_ENABLE 0x13
> +#define DISCONNECT_ENABLE_MASK(_port) \
> + GENMASK(_port, _port)
> +#define DISCONNECT_MASK(_port) \
> + (GENMASK(_port, _port) | GENMASK(_port + 4, _port + 4))
> +
> +#define DETECT_CLASS_ENABLE 0x14
> #define DETECT_CLASS_RESTART 0x18
> #define POWER_ENABLE 0x19
> +#define POWER_ENABLE_ON_MASK(_port) \
> + GENMASK(_port, _port)
> +#define POWER_ENABLE_OFF_MASK(_port) \
> + GENMASK(_port + 4, _port + 4)
> #define TPS23861_NUM_PORTS 4
>
> #define TPS23861_GENERAL_MASK_1 0x17
> @@ -548,7 +563,16 @@ static int tps23861_probe(struct i2c_client *client)
> struct device *dev = &client->dev;
> struct tps23861_data *data;
> struct device *hwmon_dev;
> + struct device_node *child;
> u32 shunt_resistor;
> + u32 reg;
> + u32 temp;
> + const char *mode;
> + unsigned int poe_plusval;
> + unsigned int mode_val;
> + unsigned int power_val;
> + unsigned int enable_val;
> + unsigned int disconnect_enable_val;
>
> data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> if (!data)
> @@ -577,6 +601,63 @@ static int tps23861_probe(struct i2c_client *client)
> TPS23861_GENERAL_MASK_1,
> TPS23861_CURRENT_SHUNT_MASK);
>
> + regmap_read(data->regmap, POE_PLUS, &poe_plusval);
> + regmap_read(data->regmap, POWER_ENABLE, &power_val);
> + regmap_read(data->regmap, OPERATING_MODE, &mode_val);
> + regmap_read(data->regmap, DETECT_CLASS_ENABLE, &enable_val);
> + regmap_read(data->regmap, DISCONNECT_ENABLE, &disconnect_enable_val);
> +
> + for_each_child_of_node(dev->of_node, child) {
> + if (of_property_read_u32(child, "reg", ®))
> + continue;
> +
> + if (reg > (TPS23861_NUM_PORTS - 1) || reg < 0)
> + continue;
> +
> + if (!of_property_read_string(child, "mode", &mode)) {
> + if (!strncmp(mode, "manual", 6)) {
> + mode_val &= ~OPERATING_MODE_PORT(OPERATING_MODE_AUTO, reg);
> + mode_val |= OPERATING_MODE_PORT(OPERATING_MODE_MANUAL, reg);
> + } else if (!strncmp(mode, "semiauto", 8)) {
> + mode_val &= ~OPERATING_MODE_PORT(OPERATING_MODE_AUTO, reg);
> + mode_val |= OPERATING_MODE_PORT(OPERATING_MODE_SEMI, reg);
> + } else if (!strncmp(mode, "auto", 4))
> + mode_val |= OPERATING_MODE_PORT(OPERATING_MODE_AUTO, reg);
> + else
> + mode_val &= ~OPERATING_MODE_PORT(OPERATING_MODE_AUTO, reg);
> + }
> +
> + if (!of_property_read_u32(child, "enable", &temp)) {
> + if (temp) {
> + enable_val |= DISCONNECT_MASK(reg);
> + disconnect_enable_val |= DISCONNECT_ENABLE_MASK(reg);
> + } else {
> + enable_val &= ~DISCONNECT_MASK(reg);
> + disconnect_enable_val &= ~DISCONNECT_ENABLE_MASK(reg);
> + }
> + }
> +
> + if (!of_property_read_u32(child, "power", &temp)) {
> + if (temp)
> + power_val |= POWER_ENABLE_ON_MASK(reg);
> + else
> + power_val |= POWER_ENABLE_OFF_MASK(reg);
> + }
> +
> + if (!of_property_read_u32(child, "poe_plus", &temp)) {
> + if (temp)
> + poe_plusval |= POE_PLUS_MASK(reg);
> + else
> + poe_plusval &= ~POE_PLUS_MASK(reg);
> + }
> + }
> +
> + regmap_write(data->regmap, POE_PLUS, poe_plusval);
> + regmap_write(data->regmap, POWER_ENABLE, power_val);
> + regmap_write(data->regmap, OPERATING_MODE, mode_val);
> + regmap_write(data->regmap, DETECT_CLASS_ENABLE, enable_val);
> + regmap_write(data->regmap, DISCONNECT_ENABLE, disconnect_enable_val);
> +
> hwmon_dev = devm_hwmon_device_register_with_info(dev, client->name,
> data, &tps23861_chip_info,
> NULL);
> --
> 2.37.2
>
Powered by blists - more mailing lists