diff mbox series

[v2] hwmon: tps23861: add support for initializing the chip

Message ID 20220825141043.75354-1-dev@aboehler.at (mailing list archive)
State Changes Requested
Headers show
Series [v2] hwmon: tps23861: add support for initializing the chip | expand

Commit Message

Andreas Böhler Aug. 25, 2022, 2:10 p.m. UTC
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@aboehler.at>
---
 .../bindings/hwmon/ti,tps23861.yaml           | 76 +++++++++++++++++
 drivers/hwmon/tps23861.c                      | 81 +++++++++++++++++++
 2 files changed, 157 insertions(+)

Comments

Krzysztof Kozlowski Aug. 25, 2022, 2:17 p.m. UTC | #1
On 25/08/2022 17:10, 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@aboehler.at>
> ---
>  .../bindings/hwmon/ti,tps23861.yaml           | 76 +++++++++++++++++

Please run scripts/checkpatch.pl and fix the warnings.

Best regards,
Krzysztof
Guenter Roeck Aug. 25, 2022, 3 p.m. UTC | #2
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@aboehler.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", &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
>
Rob Herring (Arm) Aug. 25, 2022, 6:51 p.m. UTC | #3
On Thu, 25 Aug 2022 16:10:42 +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@aboehler.at>
> ---
>  .../bindings/hwmon/ti,tps23861.yaml           | 76 +++++++++++++++++
>  drivers/hwmon/tps23861.c                      | 81 +++++++++++++++++++
>  2 files changed, 157 insertions(+)
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Documentation/devicetree/bindings/hwmon/ti,tps23861.example.dts:28.23-33: Warning (reg_format): /example-0/i2c/tps23861@30/port@0:reg: property has invalid length (4 bytes) (#address-cells == 2, #size-cells == 1)
Documentation/devicetree/bindings/hwmon/ti,tps23861.example.dts:36.23-33: Warning (reg_format): /example-0/i2c/tps23861@30/port@1:reg: property has invalid length (4 bytes) (#address-cells == 2, #size-cells == 1)
Documentation/devicetree/bindings/hwmon/ti,tps23861.example.dts:44.23-33: Warning (reg_format): /example-0/i2c/tps23861@30/port@2:reg: property has invalid length (4 bytes) (#address-cells == 2, #size-cells == 1)
Documentation/devicetree/bindings/hwmon/ti,tps23861.example.dts:52.23-33: Warning (reg_format): /example-0/i2c/tps23861@30/port@3:reg: property has invalid length (4 bytes) (#address-cells == 2, #size-cells == 1)
Documentation/devicetree/bindings/hwmon/ti,tps23861.example.dtb: Warning (pci_device_reg): Failed prerequisite 'reg_format'
Documentation/devicetree/bindings/hwmon/ti,tps23861.example.dtb: Warning (pci_device_bus_num): Failed prerequisite 'reg_format'
Documentation/devicetree/bindings/hwmon/ti,tps23861.example.dtb: Warning (simple_bus_reg): Failed prerequisite 'reg_format'
Documentation/devicetree/bindings/hwmon/ti,tps23861.example.dtb: Warning (i2c_bus_reg): Failed prerequisite 'reg_format'
Documentation/devicetree/bindings/hwmon/ti,tps23861.example.dtb: Warning (spi_bus_reg): Failed prerequisite 'reg_format'
Documentation/devicetree/bindings/hwmon/ti,tps23861.example.dts:27.26-33.21: Warning (avoid_default_addr_size): /example-0/i2c/tps23861@30/port@0: Relying on default #address-cells value
Documentation/devicetree/bindings/hwmon/ti,tps23861.example.dts:27.26-33.21: Warning (avoid_default_addr_size): /example-0/i2c/tps23861@30/port@0: Relying on default #size-cells value
Documentation/devicetree/bindings/hwmon/ti,tps23861.example.dts:35.26-41.21: Warning (avoid_default_addr_size): /example-0/i2c/tps23861@30/port@1: Relying on default #address-cells value
Documentation/devicetree/bindings/hwmon/ti,tps23861.example.dts:35.26-41.21: Warning (avoid_default_addr_size): /example-0/i2c/tps23861@30/port@1: Relying on default #size-cells value
Documentation/devicetree/bindings/hwmon/ti,tps23861.example.dts:43.26-49.21: Warning (avoid_default_addr_size): /example-0/i2c/tps23861@30/port@2: Relying on default #address-cells value
Documentation/devicetree/bindings/hwmon/ti,tps23861.example.dts:43.26-49.21: Warning (avoid_default_addr_size): /example-0/i2c/tps23861@30/port@2: Relying on default #size-cells value
Documentation/devicetree/bindings/hwmon/ti,tps23861.example.dts:51.26-57.21: Warning (avoid_default_addr_size): /example-0/i2c/tps23861@30/port@3: Relying on default #address-cells value
Documentation/devicetree/bindings/hwmon/ti,tps23861.example.dts:51.26-57.21: Warning (avoid_default_addr_size): /example-0/i2c/tps23861@30/port@3: Relying on default #size-cells value
Documentation/devicetree/bindings/hwmon/ti,tps23861.example.dtb: Warning (unique_unit_address_if_enabled): Failed prerequisite 'avoid_default_addr_size'

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/patch/

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.
diff mbox series

Patch

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
@@ -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
+
+    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", &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);