mfd: max597x: Add support for max597x
Subsystems (2)
v8 cover mfd: max597x: Add support for max597x
max597x is multifunction device with hot swap controller, fault protection & upto four indication leds.
max5978 has single hot swap controller whereas max5970 has two hot swap controllers.
Change in V8:
- Set additionalproperties to false
Change in V7:
- Update id
- Remove empty line
Changes in V6:
- Update missing vendor prefix
- Update indentation in example
Changes in V5:
- Fix dt schema error
Changes in V4:
- Add NULL entry for of_device_id
- Memory allocation check
Changes in V3:
- Address code review comment
Changes in V2:
- Update depends in Kconfig.
Marcello Sylvester Bauer (1):
dt-bindings: mfd: Add bindings for MAX5970 and MAX5978
Patrick Rudolph (1):
mfd: max597x: Add support for MAX5970 and MAX5978
.../bindings/mfd/maxim,max5970.yaml | 170 ++++++++++++++++++ drivers/mfd/Kconfig | 12 ++ drivers/mfd/Makefile | 1 + drivers/mfd/max597x.c | 92 ++++++++++ include/linux/mfd/max597x.h | 103 +++++++++++ 5 files changed, 378 insertions(+) create mode 100644 Documentation/devicetree/bindings/mfd/maxim,max5970.yaml create mode 100644 drivers/mfd/max597x.c create mode 100644 include/linux/mfd/max597x.h
base-commit: 6b780408be034213edfb5946889882cb29f8f159
Signature · 1 line
2.37.3
PATCH v8 1/2 dt-bindings: mfd: Add bindings for MAX5970 and MAX5978
From: Marcello Sylvester Bauer <[email protected]>
The MAX597x is a hot swap controller with configurable fault protection. It also has 10bit ADC for current & voltage measurements.
--- .../bindings/mfd/maxim,max5970.yaml | 170 ++++++++++++++++++ 1 file changed, 170 insertions(+) create mode 100644 Documentation/devicetree/bindings/mfd/maxim,max5970.yaml
Documentation/devicetree/bindings/mfd/maxim,max5970.yaml new file +170 -0
@@ -0,0 +1,170 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/mfd/maxim,max5970.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Regulator for MAX5970 smart switch from Maxim Integrated. + +maintainers: + - Patrick Rudolph <[email protected]> + +description: | + The smart switch provides no output regulation, but independent fault protection + and voltage and current sensing. + Programming is done through I2C bus. + + Datasheets: + https://datasheets.maximintegrated.com/en/ds/MAX5970.pdf + https://datasheets.maximintegrated.com/en/ds/MAX5978.pdf + +properties: + compatible: + enum: + - maxim,max5970 + - maxim,max5978 + + reg: + maxItems: 1 + + interrupts: + maxItems: 1 + + leds: + type: object + description: + Properties for single channel. + + properties: + "#address-cells": + const: 1 + + "#size-cells": + const: 0 + + patternProperties: + "^led@[0-3]$": + $ref: /schemas/leds/common.yaml# + type: object + + additionalProperties: false + + vss1-supply: + description: Supply of the first channel. + + vss2-supply: + description: Supply of the first channel. + + "#io-channel-cells": + const: 1 + + regulators: + type: object + description: + Properties for single channel. + + patternProperties: + "^(SW[0-1])$": + $ref: /schemas/regulator/regulator.yaml# + type: object + + shunt-resistor-micro-ohms: + description: | + The value of curent sense resistor in microohms. + Must be specified for each channel. + + additionalProperties: false + +required: + - compatible + - reg + - regulators + - vss1-supply + +allOf: + - if: + properties: + compatible: + enum: + - maxim,max5970 + then: + properties: + io-channels: + items: + - description: voltage first channel + - description: current first channel + - description: voltage second channel + - description: current second channel + description: | + Voltage and current for first and second channel. + required: + - vss2-supply + else: + properties: + io-channels: + items: + - description: voltage first channel + - description: current first channel + description: | + Voltage and current for first channel. + +additionalProperties: false + +examples: + - | + i2c { + #address-cells = <1>; + #size-cells = <0>; + regulator@3a { + compatible = "maxim,max5978"; + reg = <0x3a>; + vss1-supply = <&p3v3>; + + regulators { + sw0_ref_0: SW0 { + regulator-compatible = "SW0"; + shunt-resistor-micro-ohms = <12000>; + }; + }; + + leds { + #address-cells = <1>; + #size-cells = <0>; + led@0 { + reg = <0>; + label = "led0"; + default-state = "on"; + }; + led@1 { + reg = <1>; + label = "led1"; + default-state = "on"; + }; + }; + }; + }; + + - | + i2c { + #address-cells = <1>; + #size-cells = <0>; + + regulator@3a { + compatible = "maxim,max5970"; + reg = <0x3a>; + vss1-supply = <&p3v3>; + vss2-supply = <&p5v>; + + regulators { + sw0_ref_1: SW0 { + regulator-compatible = "SW0"; + shunt-resistor-micro-ohms = <12000>; + }; + sw1_ref_1: SW1 { + regulator-compatible = "SW1"; + shunt-resistor-micro-ohms = <10000>; + }; + }; + }; + }; +...
Signature · 1 line
2.37.3
Re: [PATCH v8 1/2] dt-bindings: mfd: Add bindings for MAX5970 and MAX5978
Quoted 2 lines · Naresh Solanki
From: Marcello Sylvester Bauer <[email protected]>
Subject: drop redundant (second) word "bindings"
Quoted 6 lines
The MAX597x is a hot swap controller with configurable fault protection. It also has 10bit ADC for current & voltage measurements.
Signed-off-by Patrick Rudolph <[email protected]>Signed-off-by Marcello Sylvester Bauer <[email protected]>Signed-off-by Naresh Solanki <[email protected]>
Isn't this third version per day? That's too many. Please keep it one per day (usually).
Quoted 37 lines
--- .../bindings/mfd/maxim,max5970.yaml | 170 ++++++++++++++++++ 1 file changed, 170 insertions(+) create mode 100644 Documentation/devicetree/bindings/mfd/maxim,max5970.yamlDocumentation/devicetree/bindings/mfd/maxim,max5970.yaml new file +26
-0@@ -0,0 +1,170 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/mfd/maxim,max5970.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Regulator for MAX5970 smart switch from Maxim Integrated. + +maintainers: + - Patrick Rudolph <[email protected]> + +description: | + The smart switch provides no output regulation, but independent fault protection + and voltage and current sensing. + Programming is done through I2C bus. + + Datasheets: + https://datasheets.maximintegrated.com/en/ds/MAX5970.pdf + https://datasheets.maximintegrated.com/en/ds/MAX5978.pdf + +properties: + compatible: + enum: + - maxim,max5970 + - maxim,max5978 +
Documentation/devicetree/bindings/mfd/maxim,max5970.yaml line 36
Quoted 10 lines
Documentation/devicetree/bindings/mfd/maxim,max5970.yaml new file +10
-0+ reg: + maxItems: 1 + + interrupts: + maxItems: 1 + + leds: + type: object + description: + Properties for single channel.
This description seems odd. You have here several leds, so what is the single channel?
Quoted 18 lines
Documentation/devicetree/bindings/mfd/maxim,max5970.yaml new file +18
-0+ + properties: + "#address-cells": + const: 1 + + "#size-cells": + const: 0 + + patternProperties: + "^led@[0-3]$": + $ref: /schemas/leds/common.yaml# + type: object + + additionalProperties: false + + vss1-supply: + description: Supply of the first channel. +
Documentation/devicetree/bindings/mfd/maxim,max5970.yaml line 64
Quoted 10 lines
Documentation/devicetree/bindings/mfd/maxim,max5970.yaml new file +10
-0+ vss2-supply: + description: Supply of the first channel. + + "#io-channel-cells": + const: 1 + + regulators: + type: object + description: + Properties for single channel.
That's another odd description.
Documentation/devicetree/bindings/mfd/maxim,max5970.yaml line 67
Quoted 3 lines
Documentation/devicetree/bindings/mfd/maxim,max5970.yaml new file +3
-0+ + patternProperties: + "^(SW[0-1])$":
This should be lowercase. You also do not need ().
Documentation/devicetree/bindings/mfd/maxim,max5970.yaml line 74
Quoted 7 lines
Documentation/devicetree/bindings/mfd/maxim,max5970.yaml new file +7
-0+ $ref: /schemas/regulator/regulator.yaml# + type: object + + shunt-resistor-micro-ohms: + description: | + The value of curent sense resistor in microohms. + Must be specified for each channel.
Drop last sentence and instead add "required:" with proper indent.
Quoted 15 lines
Documentation/devicetree/bindings/mfd/maxim,max5970.yaml new file +15
-0+ + additionalProperties: false + +required: + - compatible + - reg + - regulators + - vss1-supply + +allOf: + - if: + properties: + compatible: + enum: + - maxim,max5970
Documentation/devicetree/bindings/mfd/maxim,max5970.yaml line 99
Quoted 10 lines
Documentation/devicetree/bindings/mfd/maxim,max5970.yaml new file +10
-0+ then: + properties: + io-channels: + items: + - description: voltage first channel + - description: current first channel + - description: voltage second channel + - description: current second channel + description: | + Voltage and current for first and second channel.
I do not understand the description.
Also, add it in the existing example.
Documentation/devicetree/bindings/mfd/maxim,max5970.yaml line 109
Quoted 10 lines
Documentation/devicetree/bindings/mfd/maxim,max5970.yaml new file +10
-0+ required: + - vss2-supply + else: + properties: + io-channels: + items: + - description: voltage first channel + - description: current first channel + description: | + Voltage and current for first channel.
Same question for the description.
Documentation/devicetree/bindings/mfd/maxim,max5970.yaml line 51
Quoted 3 lines
Documentation/devicetree/bindings/mfd/maxim,max5970.yaml new file +3
-0+ +additionalProperties: false +
Best regards,
Krzysztof
Re: [PATCH v8 1/2] dt-bindings: mfd: Add bindings for MAX5970 and MAX5978
Hi Krzysztof,
Quoted 59 lines · Krzysztof Kozlowski
Quoted 2 lines · Naresh Solanki
From: Marcello Sylvester Bauer <[email protected]>
Subject: drop redundant (second) word "bindings"
Quoted 6 lines
The MAX597x is a hot swap controller with configurable fault protection. It also has 10bit ADC for current & voltage measurements.
Signed-off-by Patrick Rudolph <[email protected]>Signed-off-by Marcello Sylvester Bauer <[email protected]>Signed-off-by Naresh Solanki <[email protected]>Isn't this third version per day? That's too many. Please keep it one per day (usually).
Quoted 42 lines
--- .../bindings/mfd/maxim,max5970.yaml | 170 ++++++++++++++++++ 1 file changed, 170 insertions(+)create mode 100644
Documentation/devicetree/bindings/mfd/maxim,max5970.yamlDocumentation/devicetree/bindings/mfd/maxim,max5970.yaml new file +31
-0@@ -0,0 +1,170 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/mfd/maxim,max5970.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Regulator for MAX5970 smart switch from Maxim Integrated. + +maintainers: + - Patrick Rudolph <[email protected]> + +description: | + The smart switch provides no output regulation, but independent fault protection + and voltage and current sensing. + Programming is done through I2C bus. + + Datasheets: + https://datasheets.maximintegrated.com/en/ds/MAX5970.pdf + https://datasheets.maximintegrated.com/en/ds/MAX5978.pdf + +properties: + compatible: + enum: + - maxim,max5970 + - maxim,max5978 + + reg: + maxItems: 1 + + interrupts: + maxItems: 1
Quoted 9 lines
Documentation/devicetree/bindings/mfd/maxim,max5970.yaml line 36
Quoted 5 lines
Documentation/devicetree/bindings/mfd/maxim,max5970.yaml new file +5
-0+ + leds: + type: object + description: + Properties for single channel.This description seems odd. You have here several leds, so what is the single channel?
There are four leds. Will update in next revision.
Quoted 22 lines
Quoted 22 lines
Documentation/devicetree/bindings/mfd/maxim,max5970.yaml new file +22
-0+ + properties: + "#address-cells": + const: 1 + + "#size-cells": + const: 0 + + patternProperties: + "^led@[0-3]$": + $ref: /schemas/leds/common.yaml# + type: object + + additionalProperties: false + + vss1-supply: + description: Supply of the first channel. + + vss2-supply: + description: Supply of the first channel. + + "#io-channel-cells":
Quoted 9 lines
Documentation/devicetree/bindings/mfd/maxim,max5970.yaml line 64
Quoted 6 lines
Documentation/devicetree/bindings/mfd/maxim,max5970.yaml new file +6
-0+ const: 1 + + regulators: + type: object + description: + Properties for single channel.That's another odd description.
Update. Willbe part of next version.
Quoted 6 lines
Documentation/devicetree/bindings/mfd/maxim,max5970.yaml line 67
Quoted 3 lines
Documentation/devicetree/bindings/mfd/maxim,max5970.yaml new file +3
-0+ + patternProperties: + "^(SW[0-1])$":This should be lowercase. You also do not need ().
Done.
Quoted 10 lines
Documentation/devicetree/bindings/mfd/maxim,max5970.yaml line 74
Quoted 7 lines
Documentation/devicetree/bindings/mfd/maxim,max5970.yaml new file +7
-0+ $ref: /schemas/regulator/regulator.yaml# + type: object + + shunt-resistor-micro-ohms: + description: | + The value of curent sense resistor in microohms. + Must be specified for each channel.Drop last sentence and instead add "required:" with proper indent.
Fixed. Will be part of next version.
Quoted 28 lines
Quoted 15 lines
Documentation/devicetree/bindings/mfd/maxim,max5970.yaml new file +15
-0+ + additionalProperties: false + +required: + - compatible + - reg + - regulators + - vss1-supply + +allOf: + - if: + properties: + compatible: + enum: + - maxim,max5970Documentation/devicetree/bindings/mfd/maxim,max5970.yaml line 99
Quoted 10 lines
Documentation/devicetree/bindings/mfd/maxim,max5970.yaml new file +10
-0+ then: + properties: + io-channels: + items: + - description: voltage first channel + - description: current first channel + - description: voltage second channel + - description: current second channel + description: | + Voltage and current for first and second channel.I do not understand the description.
Quoted 2 lines
Also, add it in the existing example.
Have removed them because not being used by the driver.
Quoted 20 lines
Documentation/devicetree/bindings/mfd/maxim,max5970.yaml line 109
Quoted 10 lines
Documentation/devicetree/bindings/mfd/maxim,max5970.yaml new file +10
-0+ required: + - vss2-supply + else: + properties: + io-channels: + items: + - description: voltage first channel + - description: current first channel + description: | + Voltage and current for first channel.Same question for the description.
Documentation/devicetree/bindings/mfd/maxim,max5970.yaml line 51
Quoted 3 lines
Documentation/devicetree/bindings/mfd/maxim,max5970.yaml new file +3
-0+ +additionalProperties: false +Best regards,
Krzysztof
PATCH v8 2/2 mfd: max597x: Add support for MAX5970 and MAX5978
In reply to Naresh SolankiFrom: Patrick Rudolph <[email protected]>
Implement a regulator driver with IRQ support for fault management. Written against documentation [1] and [2] and tested on real hardware.
Every channel has its own regulator supplies nammed 'vss1-supply' and 'vss2-supply'. The regulator supply is used to determine the output voltage, as the smart switch provides no output regulation. The driver requires the 'shunt-resistor-micro-ohms' property to be present in Device Tree to properly calculate current related values.
Datasheet links:
1: https://datasheets.maximintegrated.com/en/ds/MAX5970.pdf
2: https://datasheets.maximintegrated.com/en/ds/MAX5978.pdf
--- drivers/mfd/Kconfig | 12 +++++ drivers/mfd/Makefile | 1 + drivers/mfd/max597x.c | 92 ++++++++++++++++++++++++++++++++ include/linux/mfd/max597x.h | 103 ++++++++++++++++++++++++++++++++++++ 4 files changed, 208 insertions(+) create mode 100644 drivers/mfd/max597x.c create mode 100644 include/linux/mfd/max597x.h
drivers/mfd/Kconfig +12 -0
@@ -253,6 +253,18 @@ config MFD_MADERA_SPI Support for the Cirrus Logic Madera platform audio SoC core functionality controlled via SPI. +config MFD_MAX597X + tristate "Maxim 597x Power Switch and Monitor" + depends on I2C + depends on OF + select MFD_CORE + select REGMAP_I2C + help + This driver controls a Maxim 5970/5978 switch via I2C bus. + The MAX5970/5978 is a smart switch with no output regulation, but + fault protection and voltage and current monitoring capabilities. + Also it supports upto 4 indication LEDs. + config MFD_CS47L15 bool "Cirrus Logic CS47L15" select PINCTRL_CS47L15
drivers/mfd/Makefile +1 -0
@@ -161,6 +161,7 @@ obj-$(CONFIG_MFD_DA9063) += da9063.o obj-$(CONFIG_MFD_DA9150) += da9150-core.o obj-$(CONFIG_MFD_MAX14577) += max14577.o +obj-$(CONFIG_MFD_MAX597X) += max597x.o obj-$(CONFIG_MFD_MAX77620) += max77620.o obj-$(CONFIG_MFD_MAX77650) += max77650.o obj-$(CONFIG_MFD_MAX77686) += max77686.o
drivers/mfd/max597x.c new file +92 -0
92 lines not on this page · raw message
include/linux/mfd/max597x.h new file +103 -0
103 lines not on this page · raw message
Signature · 1 line
2.37.3
Lee Jones
maintainerRe: [PATCH v8 2/2] mfd: max597x: Add support for MAX5970 and MAX5978
Quoted 3 lines · Naresh Solanki
From: Patrick Rudolph <[email protected]>
Implement a regulator driver with IRQ support for fault management.
This is not a "regulator driver".
Quoted 9 lines
Written against documentation [1] and [2] and tested on real hardware.
Every channel has its own regulator supplies nammed 'vss1-supply' and 'vss2-supply'. The regulator supply is used to determine the output voltage, as the smart switch provides no output regulation. The driver requires the 'shunt-resistor-micro-ohms' property to be present in Device Tree to properly calculate current related values.
Quoted 7 lines
Datasheet links:
1: https://datasheets.maximintegrated.com/en/ds/MAX5970.pdf
2: https://datasheets.maximintegrated.com/en/ds/MAX5978.pdfSigned-off-by Patrick Rudolph <[email protected]>Signed-off-by Marcello Sylvester Bauer <[email protected]>Signed-off-by Naresh Solanki <[email protected]>
Can you explain these tags to me please?
Patrick wrote it. Then what happened?
Quoted 50 lines
--- drivers/mfd/Kconfig | 12 +++++ drivers/mfd/Makefile | 1 + drivers/mfd/max597x.c | 92 ++++++++++++++++++++++++++++++++ include/linux/mfd/max597x.h | 103 ++++++++++++++++++++++++++++++++++++ 4 files changed, 208 insertions(+) create mode 100644 drivers/mfd/max597x.c create mode 100644 include/linux/mfd/max597x.hdrivers/mfd/Kconfig +12
-0@@ -253,6 +253,18 @@ config MFD_MADERA_SPI Support for the Cirrus Logic Madera platform audio SoC core functionality controlled via SPI. +config MFD_MAX597X + tristate "Maxim 597x Power Switch and Monitor" + depends on I2C + depends on OF + select MFD_CORE + select REGMAP_I2C + help + This driver controls a Maxim 5970/5978 switch via I2C bus. + The MAX5970/5978 is a smart switch with no output regulation, but + fault protection and voltage and current monitoring capabilities. + Also it supports upto 4 indication LEDs. + config MFD_CS47L15 bool "Cirrus Logic CS47L15" select PINCTRL_CS47L15drivers/mfd/Makefile +1
-0@@ -161,6 +161,7 @@ obj-$(CONFIG_MFD_DA9063) += da9063.o obj-$(CONFIG_MFD_DA9150) += da9150-core.o obj-$(CONFIG_MFD_MAX14577) += max14577.o +obj-$(CONFIG_MFD_MAX597X) += max597x.o obj-$(CONFIG_MFD_MAX77620) += max77620.o obj-$(CONFIG_MFD_MAX77650) += max77650.o obj-$(CONFIG_MFD_MAX77686) += max77686.odrivers/mfd/max597x.c new file +0
-0
drivers/mfd/max597x.c line 3
Quoted 3 lines
drivers/mfd/max597x.c new file +3
-0@@ -0,0 +1,92 @@ +// SPDX-License-Identifier: GPL-2.0 +/* +* Maxim MAX5970/MAX5978 MFD Driver
Please drop "MFD Driver" and replace it with what it does.
"Power Switch and Monitor"?
Quoted 24 lines
drivers/mfd/max597x.c new file +24
-0+* Copyright (c) 2022 9elements GmbH +* +* Author: Patrick Rudolph <[email protected]> +*/ + +#include <linux/i2c.h> +#include <linux/mfd/core.h> +#include <linux/mfd/max597x.h> +#include <linux/regmap.h> + +static const struct regmap_config max597x_regmap_config = { + .reg_bits =8 , + .val_bits =8 , + .max_register =MAX_REGISTERS , +}; + +static const struct mfd_cell max597x_cells[] = { + { .name ="max597x-regulator" , }, + { .name ="max597x-iio" , }, + { .name ="max597x-led" , }, +}; + +static int max597x_probe (struct i2c_client *i2c,const struct i2c_device_id *id) +{
drivers/mfd/max597x.c line 38
Quoted 10 lines
drivers/mfd/max597x.c new file +10
-0+struct max597x_data *ddata; +enum max597x_chip_type chip = id->driver_data; + + ddata =devm_kzalloc (&i2c->dev,sizeof (*ddata),GFP_KERNEL ); +if (!ddata) +return -ENOMEM ; + +/* Initialize num of switch based on chip type for later use by regulator +* & iio cells +*/
Please use proper multi-line comment structure.
Also use proper words, unlike 'num'.
In fact, it looks like this whole comment could do with some love.
drivers/mfd/max597x.c line 50
Quoted 12 lines
drivers/mfd/max597x.c new file +12
-0+switch (chip) { +case MAX597x_TYPE_MAX5970: + ddata->num_switches =MAX5970_NUM_SWITCHES ; +break ; +case MAX597x_TYPE_MAX5978: + ddata->num_switches =MAX5978_NUM_SWITCHES ; +break ; + } + + ddata->regmap =devm_regmap_init_i2c (i2c, &max597x_regmap_config); +if (IS_ERR (ddata->regmap)) { +dev_err (&i2c->dev,"Failed to initialise regmap" );
Are you using American spelling or English?
Please make up your mind and be consistent.
drivers/mfd/max597x.c line 51
Quoted 1 line
drivers/mfd/max597x.c new file +1
-0+return -EINVAL ;
Shouldn't you be propagating the error you received?
drivers/mfd/max597x.c line 54
Quoted 3 lines
drivers/mfd/max597x.c new file +3
-0+ } + +/* IRQ used by regulator cell */
What IRQ is this? Does it have a name?
If it's only used in Regulator, why not fetch it there?
drivers/mfd/max597x.c line 56
Quoted 2 lines
drivers/mfd/max597x.c new file +2
-0+ ddata->irq = i2c->irq; + ddata->dev = &i2c->dev;
You should already have access to the Driver, else how are you going to fetch the data back in the first place?
drivers/mfd/max597x.c line 66
Quoted 10 lines
drivers/mfd/max597x.c new file +10
-0+i2c_set_clientdata (i2c, ddata); + +return devm_mfd_add_devices (ddata->dev,PLATFORM_DEVID_AUTO , + max597x_cells,ARRAY_SIZE (max597x_cells), +NULL ,0 ,NULL ); +} + +static const struct i2c_device_id max597x_table[] = { + { .name ="max5970" , MAX597x_TYPE_MAX5970 }, + { .name ="max5978" , MAX597x_TYPE_MAX5978 },
You don't need ".name = ".
drivers/mfd/max597x.c line 73
Quoted 7 lines
drivers/mfd/max597x.c new file +7
-0+}; + +MODULE_DEVICE_TABLE (i2c, max597x_table); + +static const struct of_device_id max597x_of_match[] = { + { .compatible ="maxim,max5970" , .data = (void *)MAX597x_TYPE_MAX5970 }, + { .compatible ="maxim,max5978" , .data = (void *)MAX597x_TYPE_MAX5978 },
Where is .data used?
drivers/mfd/max597x.c line 74
Quoted 1 line
drivers/mfd/max597x.c new file +1
-0+ {/* sentinel */ }
Drop the comment. We know what a NULL entry means.
drivers/mfd/max597x.c line 84
Quoted 10 lines
drivers/mfd/max597x.c new file +10
-0+}; + +MODULE_DEVICE_TABLE (of, max597x_of_match); + +static struct i2c_driver max597x_driver = { + .id_table = max597x_table, + .driver = { + .name ="max597x" , + .of_match_table =of_match_ptr (max597x_of_match), + },
Tabbing error.
drivers/mfd/max597x.c line 87
Quoted 3 lines
drivers/mfd/max597x.c new file +3
-0+ .probe = max597x_probe, +}; +
Remove this line.
Quoted 11 lines
drivers/mfd/max597x.c new file +5
-0+module_i2c_driver (max597x_driver); + +MODULE_AUTHOR ("Patrick Rudolph <[email protected]>" ); +MODULE_DESCRIPTION ("MAX597X Power Switch and Monitor" ); +MODULE_LICENSE ("GPL v2" );include/linux/mfd/max597x.h new file +0
-0
include/linux/mfd/max597x.h line 3
Quoted 3 lines
include/linux/mfd/max597x.h new file +3
-0@@ -0,0 +1,103 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* +* Maxim MAX5970/MAX5978 MFD Driver
Same here.
include/linux/mfd/max597x.h line 16
Quoted 12 lines
include/linux/mfd/max597x.h new file +12
-0+* Copyright (c) 2022 9elements GmbH +* +* Author: Patrick Rudolph <[email protected]> +*/ + +#ifndef MFD_MAX597X_H +#define MFD_MAX597X_H + +#include <linux/device.h> +#include <linux/regmap.h> + +/* Number of switch based on chip variant */
This comment is superfluous.
include/linux/mfd/max597x.h line 19
Quoted 3 lines
include/linux/mfd/max597x.h new file +3
-0+#define MAX5970_NUM_SWITCHES 2 +#define MAX5978_NUM_SWITCHES 1 +/* Both chip variant have 4 indication LEDs used by LED cell */
Here too I think.
include/linux/mfd/max597x.h line 23
Quoted 4 lines
include/linux/mfd/max597x.h new file +4
-0+#define MAX597X_NUM_LEDS 4 + +enum max597x_chip_type { + MAX597x_TYPE_MAX5978 =1 ,
Why 1?
Quoted 54 lines
include/linux/mfd/max597x.h new file +54
-0+ MAX597x_TYPE_MAX5970, +}; + +#define MAX5970_REG_CURRENT_L (ch) (0x01 + (ch) * 4) +#define MAX5970_REG_CURRENT_H (ch) (0x00 + (ch) * 4) +#define MAX5970_REG_VOLTAGE_L (ch) (0x03 + (ch) * 4) +#define MAX5970_REG_VOLTAGE_H (ch) (0x02 + (ch) * 4) +#define MAX5970_REG_MON_RANGE 0x18 +#define MAX5970_MON_MASK 0x3 +#define MAX5970_MON (reg, ch) (((reg) >> ((ch) * 2)) & MAX5970_MON_MASK) +#define MAX5970_MON_MAX_RANGE_UV 16000000 + +#define MAX5970_REG_CH_UV_WARN_H (ch) (0x1A + (ch) * 10) +#define MAX5970_REG_CH_UV_WARN_L (ch) (0x1B + (ch) * 10) +#define MAX5970_REG_CH_UV_CRIT_H (ch) (0x1C + (ch) * 10) +#define MAX5970_REG_CH_UV_CRIT_L (ch) (0x1D + (ch) * 10) +#define MAX5970_REG_CH_OV_WARN_H (ch) (0x1E + (ch) * 10) +#define MAX5970_REG_CH_OV_WARN_L (ch) (0x1F + (ch) * 10) +#define MAX5970_REG_CH_OV_CRIT_H (ch) (0x20 + (ch) * 10) +#define MAX5970_REG_CH_OV_CRIT_L (ch) (0x21 + (ch) * 10) + +#define MAX5970_VAL2REG_H (x) (((x) >> 2) & 0xFF) +#define MAX5970_VAL2REG_L (x) ((x) & 0x3) + +#define MAX5970_REG_DAC_FAST (ch) (0x2E + (ch)) + +#define MAX5970_FAST2SLOW_RATIO 200 + +#define MAX5970_REG_STATUS0 0x31 +#define MAX5970_CB_IFAULTF (ch) (1 << (ch)) +#define MAX5970_CB_IFAULTS (ch) (1 << ((ch) + 4)) + +#define MAX5970_REG_STATUS1 0x32 +#define STATUS1_PROT_MASK 0x3 +#define STATUS1_PROT (reg) \ + (((reg) >> 6) & STATUS1_PROT_MASK) +#define STATUS1_PROT_SHUTDOWN 0 +#define STATUS1_PROT_CLEAR_PG 1 +#define STATUS1_PROT_ALERT_ONLY 2 + +#define MAX5970_REG_STATUS2 0x33 +#define MAX5970_IRNG_MASK 0x3 +#define MAX5970_IRNG (reg, ch) \ + (((reg) >> ((ch) * 2)) & MAX5970_IRNG_MASK) + +#define MAX5970_REG_STATUS3 0x34 +#define MAX5970_STATUS3_ALERT BIT(4) +#define MAX5970_STATUS3_PG (ch) BIT(ch) + +#define MAX5970_REG_FAULT0 0x35 +#define UV_STATUS_WARN (ch) BIT(ch) +#define UV_STATUS_CRIT (ch) BIT(ch + 4) + +#define MAX5970_REG_FAULT1 0x36
include/linux/mfd/max597x.h line 87
Quoted 10 lines
include/linux/mfd/max597x.h new file +10
-0+#define OV_STATUS_WARN (ch) BIT(ch) +#define OV_STATUS_CRIT (ch) BIT(ch + 4) + +#define MAX5970_REG_FAULT2 0x37 +#define OC_STATUS_WARN (ch) BIT(ch) + +#define MAX5970_REG_CHXEN 0x3b +#define CHXEN (ch) (3 << (ch * 2)) + +#define MAX5970_REG_LED_FLASH 0x43
Do these all need to be shared?
Or can they be isolated into, say, the Regulator driver?
Quoted 5 lines
include/linux/mfd/max597x.h new file +5
-0+#define MAX_REGISTERS 0x49 +#define ADC_MASK 0x3FF + +struct max597x_data { +struct device *dev;
include/linux/mfd/max597x.h line 103
Quoted 10 lines
include/linux/mfd/max597x.h new file +10
-0+int irq; +int num_switches; +struct regmap *regmap; +/* Chip specific parameters needed by regulator & iio cells */ +u32 irng[MAX5970_NUM_SWITCHES ]; +u32 mon_rng[MAX5970_NUM_SWITCHES ]; + u32 shunt_micro_ohms[MAX5970_NUM_SWITCHES]; +}; + +#endif /* _MAX597X_H */
This is incorrect and doesn't need to be tabbed out over here.
Signature · 1 line
Lee Jones [李琼斯]
Re: [PATCH v8 2/2] mfd: max597x: Add support for MAX5970 and MAX5978
Hi Lee
Quoted 21 lines · Lee Jones
Quoted 3 lines · Naresh Solanki
From: Patrick Rudolph <[email protected]>
Implement a regulator driver with IRQ support for fault management.
This is not a "regulator driver".
Quoted 13 lines
Written against documentation [1] and [2] and tested on real hardware.
Every channel has its own regulator supplies nammed 'vss1-supply' and 'vss2-supply'. The regulator supply is used to determine the output voltage, as the smart switch provides no output regulation. The driver requires the 'shunt-resistor-micro-ohms' property to be present in Device Tree to properly calculate current related values.
Datasheet links:
1: https://datasheets.maximintegrated.com/en/ds/MAX5970.pdf
2: https://datasheets.maximintegrated.com/en/ds/MAX5978.pdf
Quoted 8 lines
Quoted 3 lines
Signed-off-by Patrick Rudolph <[email protected]>Signed-off-by Marcello Sylvester Bauer <[email protected]>Signed-off-by Naresh Solanki <[email protected]>Can you explain these tags to me please?
Patrick wrote it. Then what happened?
Yes Its written by Patrick & Marcello.
Quoted 56 lines
Quoted 50 lines
--- drivers/mfd/Kconfig | 12 +++++ drivers/mfd/Makefile | 1 + drivers/mfd/max597x.c | 92 ++++++++++++++++++++++++++++++++ include/linux/mfd/max597x.h | 103 ++++++++++++++++++++++++++++++++++++ 4 files changed, 208 insertions(+)create mode 100644
drivers/mfd/max597x.c
create mode 100644include/linux/mfd/max597x.hdrivers/mfd/Kconfig +12
-0@@ -253,6 +253,18 @@ config MFD_MADERA_SPI Support for the Cirrus Logic Madera platform audio SoC core functionality controlled via SPI. +config MFD_MAX597X + tristate "Maxim 597x Power Switch and Monitor" + depends on I2C + depends on OF + select MFD_CORE + select REGMAP_I2C + help + This driver controls a Maxim 5970/5978 switch via I2C bus. + The MAX5970/5978 is a smart switch with no output regulation, but + fault protection and voltage and current monitoring capabilities. + Also it supports upto 4 indication LEDs. + config MFD_CS47L15 bool "Cirrus Logic CS47L15" select PINCTRL_CS47L15drivers/mfd/Makefile +1
-0@@ -161,6 +161,7 @@ obj-$(CONFIG_MFD_DA9063) += da9063.o obj-$(CONFIG_MFD_DA9150) += da9150-core.o obj-$(CONFIG_MFD_MAX14577) += max14577.o +obj-$(CONFIG_MFD_MAX597X) += max597x.o obj-$(CONFIG_MFD_MAX77620) += max77620.o obj-$(CONFIG_MFD_MAX77650) += max77650.o obj-$(CONFIG_MFD_MAX77686) += max77686.odrivers/mfd/max597x.c new file +0
-0drivers/mfd/max597x.c line 3
Quoted 3 lines
drivers/mfd/max597x.c new file +3
-0@@ -0,0 +1,92 @@ +// SPDX-License-Identifier: GPL-2.0 +/* +* Maxim MAX5970/MAX5978 MFD Driver Please drop "MFD Driver" and replace it with what it does.
Quoted 2 lines
"Power Switch and Monitor"?
Yes. that makes more sense. sure.
Quoted 37 lines
Quoted 24 lines
drivers/mfd/max597x.c new file +24
-0+* Copyright (c) 2022 9elements GmbH +* +* Author: Patrick Rudolph <[email protected]> +*/ + +#include <linux/i2c.h> +#include <linux/mfd/core.h> +#include <linux/mfd/max597x.h> +#include <linux/regmap.h> + +static const struct regmap_config max597x_regmap_config = { + .reg_bits =8 , + .val_bits =8 , + .max_register =MAX_REGISTERS , +}; + +static const struct mfd_cell max597x_cells[] = { + { .name ="max597x-regulator" , }, + { .name ="max597x-iio" , }, + { .name ="max597x-led" , }, +}; + +static int max597x_probe (struct i2c_client *i2c,const struct i2c_device_id *id) +{drivers/mfd/max597x.c line 38
Quoted 10 lines
drivers/mfd/max597x.c new file +10
-0+struct max597x_data *ddata; +enum max597x_chip_type chip = id->driver_data; + + ddata =devm_kzalloc (&i2c->dev,sizeof (*ddata),GFP_KERNEL ); +if (!ddata) +return -ENOMEM ; + +/* Initialize num of switch based on chip type for later use by regulator +* & iio cells +*/ Please use proper multi-line comment structure.
Quoted 4 lines
Also use proper words, unlike 'num'.
In fact, it looks like this whole comment could do with some love.
Done.
Quoted 15 lines
drivers/mfd/max597x.c line 50
Quoted 12 lines
drivers/mfd/max597x.c new file +12
-0+switch (chip) { +case MAX597x_TYPE_MAX5970: + ddata->num_switches =MAX5970_NUM_SWITCHES ; +break ; +case MAX597x_TYPE_MAX5978: + ddata->num_switches =MAX5978_NUM_SWITCHES ; +break ; + } + + ddata->regmap =devm_regmap_init_i2c (i2c, &max597x_regmap_config); +if (IS_ERR (ddata->regmap)) { +dev_err (&i2c->dev,"Failed to initialise regmap" );Are you using American spelling or English?
Quoted 2 lines
Please make up your mind and be consistent.
Sure
Quoted 4 lines
drivers/mfd/max597x.c line 51
Quoted 1 line
drivers/mfd/max597x.c new file +1
-0+return -EINVAL ;Shouldn't you be propagating the error you received?
Yes. Will update in next version.
Quoted 26 lines
drivers/mfd/max597x.c line 54
Quoted 3 lines
drivers/mfd/max597x.c new file +3
-0+ } + +/* IRQ used by regulator cell */ What IRQ is this? Does it have a name?
Its is VR fault status update related irq.
If it's only used in Regulator, why not fetch it there?drivers/mfd/max597x.c line 56
Quoted 2 lines
Quoted 1 line
drivers/mfd/max597x.c new file +1
-0+ ddata->irq = i2c->irq;drivers/mfd/max597x.c new file +1
-0+ ddata->dev = &i2c->dev;You should already have access to the Driver, else how are you going to fetch the data back in the first place?
drivers/mfd/max597x.c line 66
Quoted 10 lines
drivers/mfd/max597x.c new file +10
-0+i2c_set_clientdata (i2c, ddata); + +return devm_mfd_add_devices (ddata->dev,PLATFORM_DEVID_AUTO , + max597x_cells,ARRAY_SIZE (max597x_cells), +NULL ,0 ,NULL ); +} + +static const struct i2c_device_id max597x_table[] = { + { .name ="max5970" , MAX597x_TYPE_MAX5970 }, + { .name ="max5978" , MAX597x_TYPE_MAX5978 },You don't need ".name = ".
Quoted 9 lines
drivers/mfd/max597x.c line 73
Quoted 7 lines
drivers/mfd/max597x.c new file +7
-0+}; + +MODULE_DEVICE_TABLE (i2c, max597x_table); + +static const struct of_device_id max597x_of_match[] = { + { .compatible ="maxim,max5970" , .data = (void *)MAX597x_TYPE_MAX5970 }, + { .compatible ="maxim,max5978" , .data = (void *)MAX597x_TYPE_MAX5978 },Where is .data used?
The .data isn't used.
Quoted 3 lines
Quoted 3 lines
drivers/mfd/max597x.c new file +1
-0+ {/* sentinel */ }Drop the comment. We know what a NULL entry means.
Sure.
Quoted 13 lines
drivers/mfd/max597x.c line 84
Quoted 11 lines
drivers/mfd/max597x.c new file +10
-0+}; + +MODULE_DEVICE_TABLE (of, max597x_of_match); + +static struct i2c_driver max597x_driver = { + .id_table = max597x_table, + .driver = { + .name ="max597x" , + .of_match_table =of_match_ptr (max597x_of_match), + },Tabbing error.
Fixed Will be in next version.
Quoted 6 lines
drivers/mfd/max597x.c line 87
Quoted 4 lines
drivers/mfd/max597x.c new file +3
-0+ .probe = max597x_probe, +}; +Remove this line.
Sure.
Quoted 18 lines
Quoted 12 lines
drivers/mfd/max597x.c new file +5
-0+module_i2c_driver (max597x_driver); + +MODULE_AUTHOR ("Patrick Rudolph <[email protected]>" ); +MODULE_DESCRIPTION ("MAX597X Power Switch and Monitor" ); +MODULE_LICENSE ("GPL v2" );include/linux/mfd/max597x.h new file +0
-0include/linux/mfd/max597x.h line 3
Quoted 3 lines
include/linux/mfd/max597x.h new file +3
-0@@ -0,0 +1,103 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* +* Maxim MAX5970/MAX5978 MFD Driver Same here.
Quoted 14 lines
include/linux/mfd/max597x.h line 16
Quoted 12 lines
include/linux/mfd/max597x.h new file +12
-0+* Copyright (c) 2022 9elements GmbH +* +* Author: Patrick Rudolph <[email protected]> +*/ + +#ifndef MFD_MAX597X_H +#define MFD_MAX597X_H + +#include <linux/device.h> +#include <linux/regmap.h> + +/* Number of switch based on chip variant */ This comment is superfluous.
You mean this comment should be removed ?
Quoted 7 lines
include/linux/mfd/max597x.h line 19
Quoted 4 lines
include/linux/mfd/max597x.h new file +3
-0+#define MAX5970_NUM_SWITCHES 2 +#define MAX5978_NUM_SWITCHES 1 +/* Both chip variant have 4 indication LEDs used by LED cell */ Here too I think.
Quoted 6 lines
include/linux/mfd/max597x.h line 23
Quoted 4 lines
include/linux/mfd/max597x.h new file +4
-0+#define MAX597X_NUM_LEDS 4 + +enum max597x_chip_type { + MAX597x_TYPE_MAX5978 =1 ,Why 1?
MAX5978 & single power switch wheres MAX5970 has two.
Quoted 68 lines
Quoted 55 lines
include/linux/mfd/max597x.h new file +54
-0+ MAX597x_TYPE_MAX5970, +}; + +#define MAX5970_REG_CURRENT_L (ch) (0x01 + (ch) * 4) +#define MAX5970_REG_CURRENT_H (ch) (0x00 + (ch) * 4) +#define MAX5970_REG_VOLTAGE_L (ch) (0x03 + (ch) * 4) +#define MAX5970_REG_VOLTAGE_H (ch) (0x02 + (ch) * 4) +#define MAX5970_REG_MON_RANGE 0x18 +#define MAX5970_MON_MASK 0x3 +#define MAX5970_MON (reg, ch) (((reg) >> ((ch) * 2)) & MAX5970_MON_MASK) +#define MAX5970_MON_MAX_RANGE_UV 16000000 + +#define MAX5970_REG_CH_UV_WARN_H (ch) (0x1A + (ch) * 10) +#define MAX5970_REG_CH_UV_WARN_L (ch) (0x1B + (ch) * 10) +#define MAX5970_REG_CH_UV_CRIT_H (ch) (0x1C + (ch) * 10) +#define MAX5970_REG_CH_UV_CRIT_L (ch) (0x1D + (ch) * 10) +#define MAX5970_REG_CH_OV_WARN_H (ch) (0x1E + (ch) * 10) +#define MAX5970_REG_CH_OV_WARN_L (ch) (0x1F + (ch) * 10) +#define MAX5970_REG_CH_OV_CRIT_H (ch) (0x20 + (ch) * 10) +#define MAX5970_REG_CH_OV_CRIT_L (ch) (0x21 + (ch) * 10) + +#define MAX5970_VAL2REG_H (x) (((x) >> 2) & 0xFF) +#define MAX5970_VAL2REG_L (x) ((x) & 0x3) + +#define MAX5970_REG_DAC_FAST (ch) (0x2E + (ch)) + +#define MAX5970_FAST2SLOW_RATIO 200 + +#define MAX5970_REG_STATUS0 0x31 +#define MAX5970_CB_IFAULTF (ch) (1 << (ch)) +#define MAX5970_CB_IFAULTS (ch) (1 << ((ch) + 4)) + +#define MAX5970_REG_STATUS1 0x32 +#define STATUS1_PROT_MASK 0x3 +#define STATUS1_PROT (reg) \ + (((reg) >> 6) & STATUS1_PROT_MASK) +#define STATUS1_PROT_SHUTDOWN 0 +#define STATUS1_PROT_CLEAR_PG 1 +#define STATUS1_PROT_ALERT_ONLY 2 + +#define MAX5970_REG_STATUS2 0x33 +#define MAX5970_IRNG_MASK 0x3 +#define MAX5970_IRNG (reg, ch) \ + (((reg) >> ((ch) * 2)) & MAX5970_IRNG_MASK) + +#define MAX5970_REG_STATUS3 0x34 +#define MAX5970_STATUS3_ALERT BIT(4) +#define MAX5970_STATUS3_PG (ch) BIT(ch) + +#define MAX5970_REG_FAULT0 0x35 +#define UV_STATUS_WARN (ch) BIT(ch) +#define UV_STATUS_CRIT (ch) BIT(ch + 4) + +#define MAX5970_REG_FAULT1 0x36include/linux/mfd/max597x.h line 87
Quoted 10 lines
include/linux/mfd/max597x.h new file +10
-0+#define OV_STATUS_WARN (ch) BIT(ch) +#define OV_STATUS_CRIT (ch) BIT(ch + 4) + +#define MAX5970_REG_FAULT2 0x37 +#define OC_STATUS_WARN (ch) BIT(ch) + +#define MAX5970_REG_CHXEN 0x3b +#define CHXEN (ch) (3 << (ch * 2)) + +#define MAX5970_REG_LED_FLASH 0x43Do these all need to be shared?
Quoted 2 lines
Or can they be isolated into, say, the Regulator driver?
Shared reg.
Quoted 11 lines
Quoted 11 lines
include/linux/mfd/max597x.h new file +11
-0+#define MAX_REGISTERS 0x49 +#define ADC_MASK 0x3FF + +struct max597x_data { +struct device *dev; +int irq; +int num_switches; +struct regmap *regmap; +/* Chip specific parameters needed by regulator & iio cells */ +u32 irng[MAX5970_NUM_SWITCHES ]; +u32 mon_rng[MAX5970_NUM_SWITCHES ];
Quoted 9 lines
include/linux/mfd/max597x.h line 103
Quoted 4 lines
include/linux/mfd/max597x.h new file +4
-0+ u32 shunt_micro_ohms[MAX5970_NUM_SWITCHES]; +}; + +#endif /* _MAX597X_H */ This is incorrect and doesn't
need to be tabbed out over here.
Will update in next version.
Lee Jones
maintainerRe: [PATCH v8 2/2] mfd: max597x: Add support for MAX5970 and MAX5978
Quoted 25 lines · Naresh Solanki
Quoted 24 lines · Lee Jones
Quoted 3 lines · Naresh Solanki
From: Patrick Rudolph <[email protected]>
Implement a regulator driver with IRQ support for fault management.
This is not a "regulator driver".
Quoted 16 lines
Written against documentation [1] and [2] and tested on real hardware.
Every channel has its own regulator supplies nammed 'vss1-supply' and 'vss2-supply'. The regulator supply is used to determine the output voltage, as the smart switch provides no output regulation. The driver requires the 'shunt-resistor-micro-ohms' property to be present in Device Tree to properly calculate current related values.
Datasheet links:
1: https://datasheets.maximintegrated.com/en/ds/MAX5970.pdf
2: https://datasheets.maximintegrated.com/en/ds/MAX5978.pdfSigned-off-by Patrick Rudolph <[email protected]>Signed-off-by Marcello Sylvester Bauer <[email protected]>Signed-off-by Naresh Solanki <[email protected]>
[...]
Quoted 6 lines
Quoted 5 lines
drivers/mfd/max597x.c line 73
Quoted 3 lines
drivers/mfd/max597x.c new file +3
-0+static const struct of_device_id max597x_of_match[] = { + { .compatible ="maxim,max5970" , .data = (void *)MAX597x_TYPE_MAX5970 }, + { .compatible ="maxim,max5978" , .data = (void *)MAX597x_TYPE_MAX5978 },Where is .data used?
The .data isn't used.
Then why add it?
[...]
Quoted 7 lines
Quoted 6 lines
include/linux/mfd/max597x.h line 16
Quoted 4 lines
include/linux/mfd/max597x.h new file +4
-0+#include <linux/device.h> +#include <linux/regmap.h> + +/* Number of switch based on chip variant */ This comment is superfluous.
You mean this comment should be removed ?
I do.
Quoted 6 lines
Quoted 6 lines
include/linux/mfd/max597x.h line 19
Quoted 3 lines
include/linux/mfd/max597x.h new file +3
-0+#define MAX5970_NUM_SWITCHES 2 +#define MAX5978_NUM_SWITCHES 1 +/* Both chip variant have 4 indication LEDs used by LED cell */ Here too I think.
Quoted 7 lines
Quoted 6 lines
include/linux/mfd/max597x.h line 23
Quoted 4 lines
include/linux/mfd/max597x.h new file +4
-0+#define MAX597X_NUM_LEDS 4 + +enum max597x_chip_type { + MAX597x_TYPE_MAX5978 =1 ,Why 1?
MAX5978 & single power switch wheres MAX5970 has two.
That's not what this enum means.
You are just describing the type to be matched on.
The value should be arbitrary, no?
[...]
Quoted 9 lines
Quoted 9 lines
include/linux/mfd/max597x.h line 87
Quoted 6 lines
include/linux/mfd/max597x.h new file +6
-0+#define OC_STATUS_WARN (ch) BIT(ch) + +#define MAX5970_REG_CHXEN 0x3b +#define CHXEN (ch) (3 << (ch * 2)) + +#define MAX5970_REG_LED_FLASH 0x43Do these all need to be shared?
Quoted 3 lines
Quoted 2 lines
Or can they be isolated into, say, the Regulator driver?
Shared reg.
Where else are they used?
Signature · 1 line
Lee Jones [李琼斯]
Re: [PATCH v8 2/2] mfd: max597x: Add support for MAX5970 and MAX5978
Hi Lee,
Quoted 23 lines · Lee Jones
Quoted 22 lines · Naresh Solanki
Quoted 21 lines · Lee Jones
Quoted 3 lines · Naresh Solanki
From: Patrick Rudolph <[email protected]>
Implement a regulator driver with IRQ support for fault management.
This is not a "regulator driver".
Quoted 13 lines
Written against documentation [1] and [2] and tested on real hardware.
Every channel has its own regulator supplies nammed 'vss1-supply' and 'vss2-supply'. The regulator supply is used to determine the output voltage, as the smart switch provides no output regulation. The driver requires the 'shunt-resistor-micro-ohms' property to be present in Device Tree to properly calculate current related values.
Datasheet links:
1: https://datasheets.maximintegrated.com/en/ds/MAX5970.pdf
2: https://datasheets.maximintegrated.com/en/ds/MAX5978.pdf
Quoted 6 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Signed-off-by Patrick Rudolph <[email protected]>Signed-off-by Marcello Sylvester Bauer <[email protected]>Signed-off-by Naresh Solanki <[email protected]>[...]
Patrick & Marcello worked on the patch. I'm upstreaming it. Will update it as:
ok?
Quoted 8 lines
Quoted 6 lines
Quoted 5 lines
drivers/mfd/max597x.c line 73
Quoted 3 lines
drivers/mfd/max597x.c new file +3
-0+static const struct of_device_id max597x_of_match[] = { + { .compatible ="maxim,max5970" , .data = (void *)MAX597x_TYPE_MAX5970 }, + { .compatible ="maxim,max5978" , .data = (void *)MAX597x_TYPE_MAX5978 },Where is .data used?
The .data isn't used.
Then why add it?
Yes. Will remove that in next version.
Quoted 9 lines
[...]
Quoted 6 lines
Quoted 6 lines
include/linux/mfd/max597x.h line 16
Quoted 4 lines
include/linux/mfd/max597x.h new file +4
-0+#include <linux/device.h> +#include <linux/regmap.h> + +/* Number of switch based on chip variant */ This comment is superfluous.
Quoted 3 lines
Quoted 1 line
You mean this comment should be removed ?
I do.
Sure will remove.
Quoted 13 lines
Quoted 13 lines
Quoted 13 lines
include/linux/mfd/max597x.h line 19
Quoted 4 lines
include/linux/mfd/max597x.h new file +3
-0+#define MAX5970_NUM_SWITCHES 2 +#define MAX5978_NUM_SWITCHES 1 +/* Both chip variant have 4 indication LEDs used by LED cell */ Here too I think.
include/linux/mfd/max597x.h line 23
Quoted 4 lines
include/linux/mfd/max597x.h new file +4
-0+#define MAX597X_NUM_LEDS 4 + +enum max597x_chip_type { + MAX597x_TYPE_MAX5978 =1 ,Why 1?
Quoted 7 lines
Quoted 1 line
MAX5978 & single power switch wheres MAX5970 has two.
That's not what this enum means.
You are just describing the type to be matched on.
The value should be arbitrary, no?
Yes you are right. And thats how its being used. Setting the first enum value to 1 was to avoid zero.
Quoted 12 lines
[...]
Quoted 9 lines
Quoted 9 lines
include/linux/mfd/max597x.h line 87
Quoted 6 lines
include/linux/mfd/max597x.h new file +6
-0+#define OC_STATUS_WARN (ch) BIT(ch) + +#define MAX5970_REG_CHXEN 0x3b +#define CHXEN (ch) (3 << (ch * 2)) + +#define MAX5970_REG_LED_FLASH 0x43Do these all need to be shared?
Quoted 6 lines
Quoted 3 lines
Quoted 2 lines
Or can they be isolated into, say, the Regulator driver?
Shared reg.
Where else are they used?
The respective cell regulator/iio & led driver.
Regards,
Naresh