* [PATCH v8 0/2] mfd: max597x: Add support for max597x @ 2022-11-03 21:34 Naresh Solanki 2022-11-03 21:34 ` [PATCH v8 1/2] dt-bindings: mfd: Add bindings for MAX5970 and MAX5978 Naresh Solanki 2022-11-03 21:34 ` [PATCH v8 2/2] mfd: max597x: Add support " Naresh Solanki 0 siblings, 2 replies; 9+ messages in thread From: Naresh Solanki @ 2022-11-03 21:34 UTC (permalink / raw) To: linux-kernel, devicetree; +Cc: Naresh Solanki 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 -- 2.37.3 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v8 1/2] dt-bindings: mfd: Add bindings for MAX5970 and MAX5978 2022-11-03 21:34 [PATCH v8 0/2] mfd: max597x: Add support for max597x Naresh Solanki @ 2022-11-03 21:34 ` Naresh Solanki 2022-11-03 21:49 ` Krzysztof Kozlowski 2022-11-03 21:34 ` [PATCH v8 2/2] mfd: max597x: Add support " Naresh Solanki 1 sibling, 1 reply; 9+ messages in thread From: Naresh Solanki @ 2022-11-03 21:34 UTC (permalink / raw) To: linux-kernel, devicetree, Lee Jones, Rob Herring, Krzysztof Kozlowski, Patrick Rudolph Cc: Marcello Sylvester Bauer, Naresh Solanki From: Marcello Sylvester Bauer <sylv@sylv.io> 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 <patrick.rudolph@9elements.com> Signed-off-by: Marcello Sylvester Bauer <sylv@sylv.io> Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com> --- .../bindings/mfd/maxim,max5970.yaml | 170 ++++++++++++++++++ 1 file changed, 170 insertions(+) create mode 100644 Documentation/devicetree/bindings/mfd/maxim,max5970.yaml diff --git a/Documentation/devicetree/bindings/mfd/maxim,max5970.yaml b/Documentation/devicetree/bindings/mfd/maxim,max5970.yaml new file mode 100644 index 000000000000..25079c518f68 --- /dev/null +++ b/Documentation/devicetree/bindings/mfd/maxim,max5970.yaml @@ -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 <patrick.rudolph@9elements.com> + +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>; + }; + }; + }; + }; +... -- 2.37.3 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v8 1/2] dt-bindings: mfd: Add bindings for MAX5970 and MAX5978 2022-11-03 21:34 ` [PATCH v8 1/2] dt-bindings: mfd: Add bindings for MAX5970 and MAX5978 Naresh Solanki @ 2022-11-03 21:49 ` Krzysztof Kozlowski 2022-11-11 8:27 ` Naresh Solanki 0 siblings, 1 reply; 9+ messages in thread From: Krzysztof Kozlowski @ 2022-11-03 21:49 UTC (permalink / raw) To: Naresh Solanki, linux-kernel, devicetree, Lee Jones, Rob Herring, Krzysztof Kozlowski, Patrick Rudolph Cc: Marcello Sylvester Bauer On 03/11/2022 17:34, Naresh Solanki wrote: > From: Marcello Sylvester Bauer <sylv@sylv.io> > Subject: drop redundant (second) word "bindings" > 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 <patrick.rudolph@9elements.com> > Signed-off-by: Marcello Sylvester Bauer <sylv@sylv.io> > Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com> Isn't this third version per day? That's too many. Please keep it one per day (usually). > --- > .../bindings/mfd/maxim,max5970.yaml | 170 ++++++++++++++++++ > 1 file changed, 170 insertions(+) > create mode 100644 Documentation/devicetree/bindings/mfd/maxim,max5970.yaml > > diff --git a/Documentation/devicetree/bindings/mfd/maxim,max5970.yaml b/Documentation/devicetree/bindings/mfd/maxim,max5970.yaml > new file mode 100644 > index 000000000000..25079c518f68 > --- /dev/null > +++ b/Documentation/devicetree/bindings/mfd/maxim,max5970.yaml > @@ -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 <patrick.rudolph@9elements.com> > + > +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. This description seems odd. You have here several leds, so what is the 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. That's another odd description. > + > + patternProperties: > + "^(SW[0-1])$": This should be lowercase. You also do not need (). > + $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. > + > + 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. I do not understand the description. Also, add it in the existing example. > + 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. > + > +additionalProperties: false > + Best regards, Krzysztof ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v8 1/2] dt-bindings: mfd: Add bindings for MAX5970 and MAX5978 2022-11-03 21:49 ` Krzysztof Kozlowski @ 2022-11-11 8:27 ` Naresh Solanki 0 siblings, 0 replies; 9+ messages in thread From: Naresh Solanki @ 2022-11-11 8:27 UTC (permalink / raw) To: Krzysztof Kozlowski, linux-kernel, devicetree, Lee Jones, Rob Herring, Krzysztof Kozlowski, Patrick Rudolph Cc: Marcello Sylvester Bauer Hi Krzysztof, On 04-11-2022 03:19 am, Krzysztof Kozlowski wrote: > On 03/11/2022 17:34, Naresh Solanki wrote: >> From: Marcello Sylvester Bauer <sylv@sylv.io> >> > > Subject: drop redundant (second) word "bindings" > >> 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 <patrick.rudolph@9elements.com> >> Signed-off-by: Marcello Sylvester Bauer <sylv@sylv.io> >> Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com> > > Isn't this third version per day? That's too many. Please keep it one > per day (usually). > > >> --- >> .../bindings/mfd/maxim,max5970.yaml | 170 ++++++++++++++++++ >> 1 file changed, 170 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/mfd/maxim,max5970.yaml >> >> diff --git a/Documentation/devicetree/bindings/mfd/maxim,max5970.yaml b/Documentation/devicetree/bindings/mfd/maxim,max5970.yaml >> new file mode 100644 >> index 000000000000..25079c518f68 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/mfd/maxim,max5970.yaml >> @@ -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 <patrick.rudolph@9elements.com> >> + >> +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. > > This description seems odd. You have here several leds, so what is the > single channel? > There are four leds. Will update in next revision. >> + >> + 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. > > That's another odd description. > Update. Willbe part of next version. >> + >> + patternProperties: >> + "^(SW[0-1])$": > > This should be lowercase. You also do not need (). > Done. >> + $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. >> + >> + 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. > > I do not understand the description. > > Also, add it in the existing example. > Have removed them because not being used by the driver. >> + 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. > >> + >> +additionalProperties: false >> + > > Best regards, > Krzysztof > ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v8 2/2] mfd: max597x: Add support for MAX5970 and MAX5978 2022-11-03 21:34 [PATCH v8 0/2] mfd: max597x: Add support for max597x Naresh Solanki 2022-11-03 21:34 ` [PATCH v8 1/2] dt-bindings: mfd: Add bindings for MAX5970 and MAX5978 Naresh Solanki @ 2022-11-03 21:34 ` Naresh Solanki 2022-11-07 9:36 ` Lee Jones 1 sibling, 1 reply; 9+ messages in thread From: Naresh Solanki @ 2022-11-03 21:34 UTC (permalink / raw) To: linux-kernel, devicetree, Lee Jones Cc: Patrick Rudolph, Marcello Sylvester Bauer, Naresh Solanki From: Patrick Rudolph <patrick.rudolph@9elements.com> 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 Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com> Signed-off-by: Marcello Sylvester Bauer <sylv@sylv.io> Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com> --- 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 diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig index 8b93856de432..416fe7986b7b 100644 --- a/drivers/mfd/Kconfig +++ b/drivers/mfd/Kconfig @@ -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 diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile index 7ed3ef4a698c..819d711fa748 100644 --- a/drivers/mfd/Makefile +++ b/drivers/mfd/Makefile @@ -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 diff --git a/drivers/mfd/max597x.c b/drivers/mfd/max597x.c new file mode 100644 index 000000000000..2c64edb6b6dd --- /dev/null +++ b/drivers/mfd/max597x.c @@ -0,0 +1,92 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Maxim MAX5970/MAX5978 MFD Driver + * + * Copyright (c) 2022 9elements GmbH + * + * Author: Patrick Rudolph <patrick.rudolph@9elements.com> + */ + +#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) +{ + 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 + */ + 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"); + return -EINVAL; + } + + /* IRQ used by regulator cell */ + ddata->irq = i2c->irq; + ddata->dev = &i2c->dev; + 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 }, +}; + +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 }, + { /* sentinel */ } +}; + +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), + }, + .probe = max597x_probe, +}; + +module_i2c_driver(max597x_driver); + +MODULE_AUTHOR("Patrick Rudolph <patrick.rudolph@9elements.com>"); +MODULE_DESCRIPTION("MAX597X Power Switch and Monitor"); +MODULE_LICENSE("GPL v2"); diff --git a/include/linux/mfd/max597x.h b/include/linux/mfd/max597x.h new file mode 100644 index 000000000000..f88a57f0e4f2 --- /dev/null +++ b/include/linux/mfd/max597x.h @@ -0,0 +1,103 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* + * Maxim MAX5970/MAX5978 MFD Driver + * + * Copyright (c) 2022 9elements GmbH + * + * Author: Patrick Rudolph <patrick.rudolph@9elements.com> + */ + +#ifndef MFD_MAX597X_H +#define MFD_MAX597X_H + +#include <linux/device.h> +#include <linux/regmap.h> + +/* Number of switch based on chip variant */ +#define MAX5970_NUM_SWITCHES 2 +#define MAX5978_NUM_SWITCHES 1 +/* Both chip variant have 4 indication LEDs used by LED cell */ +#define MAX597X_NUM_LEDS 4 + +enum max597x_chip_type { + MAX597x_TYPE_MAX5978 = 1, + 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 +#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 + +#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]; + u32 shunt_micro_ohms[MAX5970_NUM_SWITCHES]; +}; + +#endif /* _MAX597X_H */ -- 2.37.3 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v8 2/2] mfd: max597x: Add support for MAX5970 and MAX5978 2022-11-03 21:34 ` [PATCH v8 2/2] mfd: max597x: Add support " Naresh Solanki @ 2022-11-07 9:36 ` Lee Jones 2022-11-11 9:14 ` Naresh Solanki 0 siblings, 1 reply; 9+ messages in thread From: Lee Jones @ 2022-11-07 9:36 UTC (permalink / raw) To: Naresh Solanki Cc: linux-kernel, devicetree, Patrick Rudolph, Marcello Sylvester Bauer On Thu, 03 Nov 2022, Naresh Solanki wrote: > From: Patrick Rudolph <patrick.rudolph@9elements.com> > > Implement a regulator driver with IRQ support for fault management. This is not a "regulator driver". > 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 > > Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com> > Signed-off-by: Marcello Sylvester Bauer <sylv@sylv.io> > Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com> Can you explain these tags to me please? Patrick wrote it. Then what happened? > --- > 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 > > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig > index 8b93856de432..416fe7986b7b 100644 > --- a/drivers/mfd/Kconfig > +++ b/drivers/mfd/Kconfig > @@ -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 > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile > index 7ed3ef4a698c..819d711fa748 100644 > --- a/drivers/mfd/Makefile > +++ b/drivers/mfd/Makefile > @@ -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 > diff --git a/drivers/mfd/max597x.c b/drivers/mfd/max597x.c > new file mode 100644 > index 000000000000..2c64edb6b6dd > --- /dev/null > +++ b/drivers/mfd/max597x.c > @@ -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"? > + * Copyright (c) 2022 9elements GmbH > + * > + * Author: Patrick Rudolph <patrick.rudolph@9elements.com> > + */ > + > +#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) > +{ > + 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. > + 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. > + return -EINVAL; Shouldn't you be propagating the error you received? > + } > + > + /* 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? > + 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? > + 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 = ". > +}; > + > +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? > + { /* sentinel */ } Drop the comment. We know what a NULL entry means. > +}; > + > +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. > + .probe = max597x_probe, > +}; > + Remove this line. > +module_i2c_driver(max597x_driver); > + > +MODULE_AUTHOR("Patrick Rudolph <patrick.rudolph@9elements.com>"); > +MODULE_DESCRIPTION("MAX597X Power Switch and Monitor"); > +MODULE_LICENSE("GPL v2"); > diff --git a/include/linux/mfd/max597x.h b/include/linux/mfd/max597x.h > new file mode 100644 > index 000000000000..f88a57f0e4f2 > --- /dev/null > +++ b/include/linux/mfd/max597x.h > @@ -0,0 +1,103 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* > + * Maxim MAX5970/MAX5978 MFD Driver Same here. > + * Copyright (c) 2022 9elements GmbH > + * > + * Author: Patrick Rudolph <patrick.rudolph@9elements.com> > + */ > + > +#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. > +#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. > +#define MAX597X_NUM_LEDS 4 > + > +enum max597x_chip_type { > + MAX597x_TYPE_MAX5978 = 1, Why 1? > + 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 > +#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? > +#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]; > + u32 shunt_micro_ohms[MAX5970_NUM_SWITCHES]; > +}; > + > +#endif /* _MAX597X_H */ This is incorrect and doesn't need to be tabbed out over here. -- Lee Jones [李琼斯] ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v8 2/2] mfd: max597x: Add support for MAX5970 and MAX5978 2022-11-07 9:36 ` Lee Jones @ 2022-11-11 9:14 ` Naresh Solanki 2022-11-14 9:11 ` Lee Jones 0 siblings, 1 reply; 9+ messages in thread From: Naresh Solanki @ 2022-11-11 9:14 UTC (permalink / raw) To: Lee Jones Cc: linux-kernel, devicetree, Patrick Rudolph, Marcello Sylvester Bauer Hi Lee On 07-11-2022 03:06 pm, Lee Jones wrote: > On Thu, 03 Nov 2022, Naresh Solanki wrote: > >> From: Patrick Rudolph <patrick.rudolph@9elements.com> >> >> Implement a regulator driver with IRQ support for fault management. > > This is not a "regulator driver". > >> 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 >> >> Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com> >> Signed-off-by: Marcello Sylvester Bauer <sylv@sylv.io> >> Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com> > > Can you explain these tags to me please? > > Patrick wrote it. Then what happened? > Yes Its written by Patrick & Marcello. >> --- >> 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 >> >> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig >> index 8b93856de432..416fe7986b7b 100644 >> --- a/drivers/mfd/Kconfig >> +++ b/drivers/mfd/Kconfig >> @@ -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 >> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile >> index 7ed3ef4a698c..819d711fa748 100644 >> --- a/drivers/mfd/Makefile >> +++ b/drivers/mfd/Makefile >> @@ -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 >> diff --git a/drivers/mfd/max597x.c b/drivers/mfd/max597x.c >> new file mode 100644 >> index 000000000000..2c64edb6b6dd >> --- /dev/null >> +++ b/drivers/mfd/max597x.c >> @@ -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"? > Yes. that makes more sense. sure. >> + * Copyright (c) 2022 9elements GmbH >> + * >> + * Author: Patrick Rudolph <patrick.rudolph@9elements.com> >> + */ >> + >> +#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) >> +{ >> + 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. > Done. >> + 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. > Sure >> + return -EINVAL; > > Shouldn't you be propagating the error you received? > Yes. Will update in next version. >> + } >> + >> + /* 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? > >> + 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? > >> + 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 = ". > >> +}; >> + >> +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. > >> + { /* sentinel */ } > > Drop the comment. We know what a NULL entry means. Sure. > >> +}; >> + >> +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. > >> + .probe = max597x_probe, >> +}; >> + > > Remove this line. Sure. > >> +module_i2c_driver(max597x_driver); >> + >> +MODULE_AUTHOR("Patrick Rudolph <patrick.rudolph@9elements.com>"); >> +MODULE_DESCRIPTION("MAX597X Power Switch and Monitor"); >> +MODULE_LICENSE("GPL v2"); >> diff --git a/include/linux/mfd/max597x.h b/include/linux/mfd/max597x.h >> new file mode 100644 >> index 000000000000..f88a57f0e4f2 >> --- /dev/null >> +++ b/include/linux/mfd/max597x.h >> @@ -0,0 +1,103 @@ >> +/* SPDX-License-Identifier: GPL-2.0 */ >> +/* >> + * Maxim MAX5970/MAX5978 MFD Driver > > Same here. > >> + * Copyright (c) 2022 9elements GmbH >> + * >> + * Author: Patrick Rudolph <patrick.rudolph@9elements.com> >> + */ >> + >> +#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 ? > >> +#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. > >> +#define MAX597X_NUM_LEDS 4 >> + >> +enum max597x_chip_type { >> + MAX597x_TYPE_MAX5978 = 1, > > Why 1? MAX5978 & single power switch wheres MAX5970 has two. > >> + 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 >> +#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? > Shared reg. >> +#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]; >> + 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. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v8 2/2] mfd: max597x: Add support for MAX5970 and MAX5978 2022-11-11 9:14 ` Naresh Solanki @ 2022-11-14 9:11 ` Lee Jones 2022-11-14 18:20 ` Naresh Solanki 0 siblings, 1 reply; 9+ messages in thread From: Lee Jones @ 2022-11-14 9:11 UTC (permalink / raw) To: Naresh Solanki Cc: linux-kernel, devicetree, Patrick Rudolph, Marcello Sylvester Bauer On Fri, 11 Nov 2022, Naresh Solanki wrote: > On 07-11-2022 03:06 pm, Lee Jones wrote: > > On Thu, 03 Nov 2022, Naresh Solanki wrote: > > > > > From: Patrick Rudolph <patrick.rudolph@9elements.com> > > > > > > Implement a regulator driver with IRQ support for fault management. > > > > This is not a "regulator driver". > > > > > 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 > > > > > > Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com> > > > Signed-off-by: Marcello Sylvester Bauer <sylv@sylv.io> > > > Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com> [...] > > > +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? [...] > > > +#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. > > > +#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. > > > > > +#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? [...] > > > +#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? > > > Shared reg. Where else are they used? -- Lee Jones [李琼斯] ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v8 2/2] mfd: max597x: Add support for MAX5970 and MAX5978 2022-11-14 9:11 ` Lee Jones @ 2022-11-14 18:20 ` Naresh Solanki 0 siblings, 0 replies; 9+ messages in thread From: Naresh Solanki @ 2022-11-14 18:20 UTC (permalink / raw) To: Lee Jones Cc: linux-kernel, devicetree, Patrick Rudolph, Marcello Sylvester Bauer Hi Lee, On 14-11-2022 02:41 pm, Lee Jones wrote: > On Fri, 11 Nov 2022, Naresh Solanki wrote: >> On 07-11-2022 03:06 pm, Lee Jones wrote: >>> On Thu, 03 Nov 2022, Naresh Solanki wrote: >>> >>>> From: Patrick Rudolph <patrick.rudolph@9elements.com> >>>> >>>> Implement a regulator driver with IRQ support for fault management. >>> >>> This is not a "regulator driver". >>> >>>> 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 >>>> >>>> Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com> >>>> Signed-off-by: Marcello Sylvester Bauer <sylv@sylv.io> >>>> Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com> > > [...] > Patrick & Marcello worked on the patch. I'm upstreaming it. Will update it as: Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com> Co-Developed-by: Marcello Sylvester Bauer <sylv@sylv.io> Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com> ok? >>>> +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. > > [...] > >>>> +#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. Sure will remove. > >>>> +#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. >>> >>>> +#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? Yes you are right. And thats how its being used. Setting the first enum value to 1 was to avoid zero. > > [...] > >>>> +#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? >>> >> Shared reg. > > Where else are they used? > The respective cell regulator/iio & led driver. Regards, Naresh ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2022-11-14 18:20 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-11-03 21:34 [PATCH v8 0/2] mfd: max597x: Add support for max597x Naresh Solanki 2022-11-03 21:34 ` [PATCH v8 1/2] dt-bindings: mfd: Add bindings for MAX5970 and MAX5978 Naresh Solanki 2022-11-03 21:49 ` Krzysztof Kozlowski 2022-11-11 8:27 ` Naresh Solanki 2022-11-03 21:34 ` [PATCH v8 2/2] mfd: max597x: Add support " Naresh Solanki 2022-11-07 9:36 ` Lee Jones 2022-11-11 9:14 ` Naresh Solanki 2022-11-14 9:11 ` Lee Jones 2022-11-14 18:20 ` Naresh Solanki
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).