| 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 |
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
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", ®)) > + 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 >
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 --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", ®)) + 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);
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(+)