This patchset adds mfd, regulator and adc driver and related bindings.The patches
are required to be applied in sequence.
Okan Sahin (5):
staging: drivers: mfd: Add MAX77541/MAX77540 PMIC Support
staging: dt-bindings: mfd: adi,max77541.yaml Add MAX77541 bindings
staging: drivers: regulator: Add MAX77541 Regulator Support
staging: dt-bindings: regulator: adi,max77541.yaml Add MAX77541
Regulator bindings
staging: drivers: iio: adc: Adc MAX77541 ADC Support
.../devicetree/bindings/mfd/adi,max77541.yaml | 134 ++++++++++
.../bindings/regulator/adi,max77541.yaml | 44 ++++
MAINTAINERS | 11 +
drivers/iio/adc/Kconfig | 11 +
drivers/iio/adc/Makefile | 1 +
drivers/iio/adc/max77541-adc.c | 224 +++++++++++++++++
drivers/mfd/Kconfig | 13 +
drivers/mfd/Makefile | 1 +
drivers/mfd/max77541.c | 236 ++++++++++++++++++
drivers/regulator/Kconfig | 9 +
drivers/regulator/Makefile | 1 +
drivers/regulator/max77541-regulator.c | 181 ++++++++++++++
include/linux/mfd/max77541.h | 150 +++++++++++
13 files changed, 1016 insertions(+)
create mode 100644 Documentation/devicetree/bindings/mfd/adi,max77541.yaml
create mode 100644 Documentation/devicetree/bindings/regulator/adi,max77541.yaml
create mode 100644 drivers/iio/adc/max77541-adc.c
create mode 100644 drivers/mfd/max77541.c
create mode 100644 drivers/regulator/max77541-regulator.c
create mode 100644 include/linux/mfd/max77541.h
--
2.30.2
This patch adds document the bindings for MAX77541 MFD driver. It also
includes MAX77540 driver whose regmap is covered by MAX77541.
Signed-off-by: Okan Sahin <[email protected]>
---
.../devicetree/bindings/mfd/adi,max77541.yaml | 134 ++++++++++++++++++
MAINTAINERS | 1 +
2 files changed, 135 insertions(+)
create mode 100644 Documentation/devicetree/bindings/mfd/adi,max77541.yaml
diff --git a/Documentation/devicetree/bindings/mfd/adi,max77541.yaml b/Documentation/devicetree/bindings/mfd/adi,max77541.yaml
new file mode 100644
index 000000000000..205953e6dd15
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/adi,max77541.yaml
@@ -0,0 +1,134 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/mfd/adi,max77541.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: MAX77540/MAX77541 PMIC from ADI.
+
+maintainers:
+ - Okan Sahin <[email protected]>
+
+description: |
+ MAX77540 is a Power Management IC with 2 buck regulators.
+
+ MAX77541 is a Power Management IC with 2 buck regulators and 1 ADC.
+
+properties:
+ compatible:
+ enum:
+ - adi,max77540
+ - adi,max77541
+
+ reg:
+ maxItems: 1
+
+ interrupts:
+ maxItems: 1
+
+ regulators:
+ $ref: /schemas/regulator/adi,max77541.yaml#
+
+ adc:
+ type: object
+ additionalProperties: false
+ properties:
+ compatible:
+ const: adi,max77541-adc
+ required:
+ -compatible
+
+required:
+ - compatible
+ - reg
+ - interrupts
+
+allOf:
+ - if:
+ properties:
+ compatible:
+ contains:
+ const: adi,max77540
+ then:
+ properties:
+ regulator:
+ properties:
+ compatible:
+ const: adi,max77540-regulator
+ else:
+ properties:
+ regulator:
+ properties:
+ compatible:
+ const: adi,max77541-regulator
+ adc:
+ properties:
+ compatible:
+ const: adi,max77541-adc
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/interrupt-controller/irq.h>
+
+ i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ pmic@69 {
+ compatible = "adi,max77540";
+ reg = <0x69>;
+ interrupt-parent = <&gpio>;
+ interrupts = <16 IRQ_TYPE_EDGE_FALLING>;
+
+ regulators {
+ buck1 {
+ regulator-min-microvolt = <500000>;
+ regulator-max-microvolt = <5200000>;
+ regulator-boot-on;
+ regulator-always-on;
+ };
+ buck2 {
+ regulator-min-microvolt = <500000>;
+ regulator-max-microvolt = <5200000>;
+ regulator-boot-on;
+ regulator-always-on;
+ };
+ };
+ };
+ };
+
+ - |
+ #include <dt-bindings/interrupt-controller/irq.h>
+
+ i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ pmic@69 {
+ compatible = "adi,max77541";
+ reg = <0x63>;
+ interrupt-parent = <&gpio>;
+ interrupts = <16 IRQ_TYPE_EDGE_FALLING>;
+
+ regulators {
+ buck1 {
+ regulator-min-microvolt = <500000>;
+ regulator-max-microvolt = <5200000>;
+ regulator-boot-on;
+ regulator-always-on;
+ };
+ buck2 {
+ regulator-min-microvolt = <500000>;
+ regulator-max-microvolt = <5200000>;
+ regulator-boot-on;
+ regulator-always-on;
+ };
+ };
+
+ adc {
+ compatible = "adi,max77541-adc";
+ }
+ };
+ };
\ No newline at end of file
diff --git a/MAINTAINERS b/MAINTAINERS
index af94d06bb9f0..22f5a9c490e3 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12501,6 +12501,7 @@ MAXIM MAX77541 PMIC MFD DRIVER
M: Okan Sahin <[email protected]>
L: [email protected]
S: Maintained
+F: Documentation/devicetree/bindings/mfd/adi,max77541.yaml
F: drivers/mfd/max77541.c
F: include/linux/mfd/max77541.h
--
2.30.2
This patch adds MFD driver for MAX77541/MAX77540 to enable its sub
devices.
The MAX77541 is a multi-function devices. It includes
buck converter and ADC.
The MAX77540 is a high-efficiency buck converter
with two 3A switching phases.
They have same regmap except for ADC part of MAX77541.
Signed-off-by: Okan Sahin <[email protected]>
---
MAINTAINERS | 7 ++
drivers/mfd/Kconfig | 13 ++
drivers/mfd/Makefile | 1 +
drivers/mfd/max77541.c | 236 +++++++++++++++++++++++++++++++++++
include/linux/mfd/max77541.h | 150 ++++++++++++++++++++++
5 files changed, 407 insertions(+)
create mode 100644 drivers/mfd/max77541.c
create mode 100644 include/linux/mfd/max77541.h
diff --git a/MAINTAINERS b/MAINTAINERS
index cf0f18502372..af94d06bb9f0 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12497,6 +12497,13 @@ S: Maintained
F: Documentation/devicetree/bindings/regulator/maxim,max20086.yaml
F: drivers/regulator/max20086-regulator.c
+MAXIM MAX77541 PMIC MFD DRIVER
+M: Okan Sahin <[email protected]>
+L: [email protected]
+S: Maintained
+F: drivers/mfd/max77541.c
+F: include/linux/mfd/max77541.h
+
MAXIM MAX77650 PMIC MFD DRIVER
M: Bartosz Golaszewski <[email protected]>
L: [email protected]
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 8b93856de432..153e8d6757b0 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -791,6 +791,19 @@ config MFD_MAX14577
additional drivers must be enabled in order to use the functionality
of the device.
+config MFD_MAX77541
+ tristate "Analog Devices MAX77541/77540 PMIC Support"
+ depends on I2C=y
+ select MFD_CORE
+ select REGMAP_I2C
+ select REGMAP_IRQ
+ help
+ Say yes here to add support for Analog Devices
+ MAX77541 and MAX77540 Power Management ICs.
+ This driver provides common support for accessing the device;
+ additional drivers must be enabled in order to use the functionality
+ of the device.
+
config MFD_MAX77620
bool "Maxim Semiconductor MAX77620 and MAX20024 PMIC Support"
depends on I2C=y
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index 7ed3ef4a698c..bf21228f5742 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_MAX77541) += max77541.o
obj-$(CONFIG_MFD_MAX77620) += max77620.o
obj-$(CONFIG_MFD_MAX77650) += max77650.o
obj-$(CONFIG_MFD_MAX77686) += max77686.o
diff --git a/drivers/mfd/max77541.c b/drivers/mfd/max77541.c
new file mode 100644
index 000000000000..97a2df3ce0b6
--- /dev/null
+++ b/drivers/mfd/max77541.c
@@ -0,0 +1,236 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2022 Analog Devices, Inc.
+ * Mfd core driver for the MAX77540 and MAX77541
+ */
+
+ #include <linux/i2c.h>
+ #include <linux/interrupt.h>
+ #include <linux/mfd/core.h>
+ #include <linux/mfd/max77541.h>
+ #include <linux/regmap.h>
+
+static const struct regmap_config max77541_regmap_config = {
+ .reg_bits = 8,
+ .val_bits = 8,
+};
+
+static const struct regmap_irq max77541_src_irqs[] = {
+ MAX77541_REGMAP_IRQ_REG(MAX77541_BIT_INT_SRC_TOPSYS),
+ MAX77541_REGMAP_IRQ_REG(MAX77541_BIT_INT_SRC_BUCK),
+};
+
+static const struct regmap_irq_chip max77541_src_irq_chip = {
+ .name = "max77541-src",
+ .status_base = MAX77541_REG_INT_SRC,
+ .mask_base = MAX77541_REG_INT_SRC,
+ .num_regs = 1,
+ .irqs = max77541_src_irqs,
+ .num_irqs = ARRAY_SIZE(max77541_src_irqs),
+};
+
+static const struct regmap_irq max77541_topsys_irqs[] = {
+ MAX77541_REGMAP_IRQ_REG(MAX77541_BIT_TOPSYS_INT_TJ_120C),
+ MAX77541_REGMAP_IRQ_REG(MAX77541_BIT_TOPSYS_INT_TJ_140C),
+ MAX77541_REGMAP_IRQ_REG(MAX77541_BIT_TOPSYS_INT_TSHDN),
+ MAX77541_REGMAP_IRQ_REG(MAX77541_BIT_TOPSYS_INT_UVLO),
+ MAX77541_REGMAP_IRQ_REG(MAX77541_BIT_TOPSYS_INT_ALT_SWO),
+ MAX77541_REGMAP_IRQ_REG(MAX77541_BIT_TOPSYS_INT_EXT_FREQ_DET),
+};
+
+static const struct regmap_irq_chip max77541_topsys_irq_chip = {
+ .name = "max77541-topsys",
+ .status_base = MAX77541_REG_TOPSYS_INT,
+ .mask_base = MAX77541_REG_TOPSYS_INT_M,
+ .num_regs = 1,
+ .irqs = max77541_topsys_irqs,
+ .num_irqs = ARRAY_SIZE(max77541_topsys_irqs),
+};
+
+static const struct regmap_irq max77541_buck_irqs[] = {
+ MAX77541_REGMAP_IRQ_REG(MAX77541_BIT_BUCK_INT_M1_POK_FLT),
+ MAX77541_REGMAP_IRQ_REG(MAX77541_BIT_BUCK_INT_M2_POK_FLT),
+ MAX77541_REGMAP_IRQ_REG(MAX77541_BIT_BUCK_INT_M1_SCFLT),
+ MAX77541_REGMAP_IRQ_REG(MAX77541_BIT_BUCK_INT_M2_SCFLT),
+};
+
+static const struct regmap_irq_chip max77541_buck_irq_chip = {
+ .name = "max77541-buck",
+ .status_base = MAX77541_REG_BUCK_INT,
+ .mask_base = MAX77541_REG_BUCK_INT_M,
+ .num_regs = 1,
+ .irqs = max77541_buck_irqs,
+ .num_irqs = ARRAY_SIZE(max77541_buck_irqs),
+};
+
+static const struct regmap_irq max77541_adc_irqs[] = {
+ MAX77541_REGMAP_IRQ_REG(MAX77541_BIT_ADC_INT_CH1_I),
+ MAX77541_REGMAP_IRQ_REG(MAX77541_BIT_ADC_INT_CH2_I),
+ MAX77541_REGMAP_IRQ_REG(MAX77541_BIT_ADC_INT_CH3_I),
+ MAX77541_REGMAP_IRQ_REG(MAX77541_BIT_ADC_INT_CH6_I),
+};
+
+static const struct regmap_irq_chip max77541_adc_irq_chip = {
+ .name = "max77541-adc",
+ .status_base = MAX77541_REG_ADC_INT,
+ .mask_base = MAX77541_REG_ADC_MSK,
+ .num_regs = 1,
+ .irqs = max77541_adc_irqs,
+ .num_irqs = ARRAY_SIZE(max77541_adc_irqs),
+};
+
+static const struct mfd_cell max77540_devs[] = {
+ MFD_CELL_OF("max77540-regulator", NULL, NULL, 0, 0,
+ "adi,max77540-regulator"),
+};
+
+static const struct mfd_cell max77541_devs[] = {
+ MFD_CELL_OF("max77541-regulator", NULL, NULL, 0, 0,
+ "adi,max77541-regulator"),
+ MFD_CELL_OF("max77541-adc", NULL, NULL, 0, 0,
+ NULL),
+};
+
+static int max77541_pmic_irq_init(struct max77541_dev *me)
+{
+ struct regmap *regmap = me->regmap;
+ struct device *dev = me->dev;
+ int irq = me->i2c->irq;
+ int ret;
+
+ ret = devm_regmap_add_irq_chip(dev, regmap, irq,
+ IRQF_ONESHOT | IRQF_SHARED, 0,
+ &max77541_src_irq_chip, &me->irq_data);
+ if (ret)
+ return ret;
+
+ ret = devm_regmap_add_irq_chip(dev, regmap, irq,
+ IRQF_ONESHOT | IRQF_SHARED, 0,
+ &max77541_topsys_irq_chip, &me->irq_topsys);
+ if (ret)
+ return ret;
+
+ ret = devm_regmap_add_irq_chip(dev, regmap, irq,
+ IRQF_ONESHOT | IRQF_SHARED, 0,
+ &max77541_buck_irq_chip, &me->irq_buck);
+ if (ret)
+ return ret;
+
+ if (me->type == MAX77541) {
+ ret = devm_regmap_add_irq_chip(dev, regmap, irq,
+ IRQF_ONESHOT | IRQF_SHARED, 0,
+ &max77541_adc_irq_chip,
+ &me->irq_adc);
+ if (ret)
+ return ret;
+ }
+
+ return ret;
+}
+
+static int max77541_pmic_setup(struct max77541_dev *me)
+{
+ struct regmap *regmap = me->regmap;
+ struct device *dev = me->dev;
+ unsigned int val;
+ int ret;
+
+ ret = max77541_pmic_irq_init(me);
+ if (ret)
+ return dev_err_probe(dev, ret, "Failed to initialize IRQ\n");
+
+ ret = regmap_read(regmap, MAX77541_REG_INT_SRC, &val);
+ if (ret)
+ return ret;
+
+ ret = regmap_read(regmap, MAX77541_REG_TOPSYS_INT, &val);
+ if (ret)
+ return ret;
+
+ ret = regmap_read(regmap, MAX77541_REG_BUCK_INT, &val);
+ if (ret)
+ return ret;
+
+ ret = device_init_wakeup(dev, true);
+ if (ret)
+ return dev_err_probe(dev, ret, "Unable to init wakeup\n");
+
+ switch (me->type) {
+ case MAX77540:
+ return devm_mfd_add_devices(dev, -1, max77540_devs, ARRAY_SIZE(max77540_devs),
+ NULL, 0, NULL);
+ case MAX77541:
+ return devm_mfd_add_devices(dev, -1, max77541_devs, ARRAY_SIZE(max77541_devs),
+ NULL, 0, NULL);
+ default:
+ return -EINVAL;
+ }
+}
+
+static int max77541_i2c_probe(struct i2c_client *client)
+{
+ const struct i2c_device_id *chip_id;
+ struct max77541_dev *me;
+ const void *chip_data;
+
+ me = devm_kzalloc(&client->dev, sizeof(*me), GFP_KERNEL);
+ if (!me)
+ return -ENOMEM;
+
+ i2c_set_clientdata(client, me);
+ me->dev = &client->dev;
+ me->i2c = client;
+
+ chip_id = to_i2c_driver(client->dev.driver)->id_table;
+
+ chip_data = device_get_match_data(me->dev);
+ if (!chip_data) {
+ chip_data = (void *)i2c_match_id(chip_id, client)->driver_data;
+ if (!chip_data)
+ return dev_err_probe(me->dev, -ENODEV, "Unable to find device\n");
+ }
+
+ me->type = (enum dev_type)chip_data;
+ me->regmap = devm_regmap_init_i2c(client, &max77541_regmap_config);
+ if (IS_ERR(me->regmap))
+ return dev_err_probe(me->dev, PTR_ERR(me->regmap),
+ "Failed to allocate register map\n");
+
+ return max77541_pmic_setup(me);
+}
+
+static const struct of_device_id max77541_of_id[] = {
+ {
+ .compatible = "adi,max77540",
+ .data = (void *)MAX77540,
+ },
+ {
+ .compatible = "adi,max77541",
+ .data = (void *)MAX77541,
+ },
+ { /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, max77541_of_id);
+
+static const struct i2c_device_id max77541_i2c_id[] = {
+ { "max77540", MAX77540 },
+ { "max77541", MAX77541 },
+ { /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(i2c, max77541_i2c_id);
+
+static struct i2c_driver max77541_i2c_driver = {
+ .driver = {
+ .name = "max77541",
+ .of_match_table = max77541_of_id,
+ },
+ .probe_new = max77541_i2c_probe,
+ .id_table = max77541_i2c_id,
+};
+
+module_i2c_driver(max77541_i2c_driver);
+
+MODULE_DESCRIPTION("MAX7740/MAX7741 MFD Driver");
+MODULE_AUTHOR("Okan Sahin <[email protected]>");
+MODULE_LICENSE("GPL");
+MODULE_VERSION("1.0");
diff --git a/include/linux/mfd/max77541.h b/include/linux/mfd/max77541.h
new file mode 100644
index 000000000000..6f2753300227
--- /dev/null
+++ b/include/linux/mfd/max77541.h
@@ -0,0 +1,150 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+
+#ifndef __MAX77541_MFD_H__
+#define __MAX77541_MFD_H__
+
+/* REGISTERS */
+
+/* GLOBAL CONFIG1 */
+#define MAX77541_REG_INT_SRC 0x00
+#define MAX77541_REG_INT_SRC_M 0x01
+#define MAX77541_REG_TOPSYS_INT 0x02
+#define MAX77541_REG_TOPSYS_INT_M 0x03
+#define MAX77541_REG_TOPSYS_STAT 0x04
+#define MAX77541_REG_DEVICE_CFG1 0x06
+#define MAX77541_REG_DEVICE_CFG2 0x07
+#define MAX77541_REG_TOPSYS_CFG 0x08
+#define MAX77541_REG_PROT_CFG 0x09
+#define MAX77541_REG_EN_CTRL 0x0B
+
+/* GLOBAL CONFIG2 */
+#define MAX77541_REG_GLB_CFG1 0x11
+
+/* BUCK CONFIG */
+#define MAX77541_REG_BUCK_INT 0x20
+#define MAX77541_REG_BUCK_INT_M 0x21
+#define MAX77541_REG_BUCK_STAT 0x22
+
+/* BUCK1 */
+#define MAX77541_REG_M1_VOUT 0x23
+#define MAX77541_REG_M1_CFG1 0x25
+#define MAX77541_REG_M1_CFG2 0x26
+#define MAX77541_REG_M1_CFG3 0x27
+
+/* BUCK2 */
+#define MAX77541_REG_M2_VOUT 0x33
+#define MAX77541_REG_M2_CFG1 0x35
+#define MAX77541_REG_M2_CFG2 0x36
+#define MAX77541_REG_M2_CFG3 0x37
+
+#define MAX77541_REG_NOT_AVAILABLE 0xFF
+
+/* INTERRUPT MASKS*/
+#define MAX77541_REG_INT_SRC_MASK 0x00
+#define MAX77541_REG_TOPSYS_INT_MASK 0x00
+#define MAX77541_REG_BUCK_INT_MASK 0x00
+
+/*BITS OF REGISTERS*/
+
+#define MAX77541_BIT_INT_SRC_TOPSYS BIT(0)
+#define MAX77541_BIT_INT_SRC_BUCK BIT(1)
+
+#define MAX77541_BIT_TOPSYS_INT_TJ_120C BIT(0)
+#define MAX77541_BIT_TOPSYS_INT_TJ_140C BIT(1)
+#define MAX77541_BIT_TOPSYS_INT_TSHDN BIT(2)
+#define MAX77541_BIT_TOPSYS_INT_UVLO BIT(3)
+#define MAX77541_BIT_TOPSYS_INT_ALT_SWO BIT(4)
+#define MAX77541_BIT_TOPSYS_INT_EXT_FREQ_DET BIT(5)
+
+#define MAX77541_BIT_BUCK_INT_M1_POK_FLT BIT(0)
+#define MAX77541_BIT_BUCK_INT_M2_POK_FLT BIT(1)
+#define MAX77541_BIT_BUCK_INT_M1_SCFLT BIT(4)
+#define MAX77541_BIT_BUCK_INT_M2_SCFLT BIT(5)
+
+#define MAX77541_BITS_DEVICE_CFG1_SEL1_LATCH GENMASK(4, 0)
+#define MAX77541_BITS_DEVICE_CFG2_SEL2_LATCH GENMASK(4, 0)
+
+#define MAX77541_BIT_TOPSYS_CFG_DIS_ALT_IN BIT(0)
+
+#define MAX77541_BITS_PROT_CFG_POK_TO GENMASK(1, 0)
+#define MAX77541_BIT_PROT_CFG_EN_FTMON BIT(2)
+
+#define MAX77541_BIT_M1_EN BIT(0)
+#define MAX77541_BIT_M2_EN BIT(1)
+#define MAX77541_BIT_M1_LPM BIT(4)
+#define MAX77541_BIT_M2_LPM BIT(5)
+
+#define MAX77541_BITS_GLB_CFG1_SSTOP_SR GENMASK(2, 0)
+#define MAX77541_BITS_GLB_CFG1_SSTRT_SR GENMASK(5, 3)
+
+#define MAX77541_BITS_MX_VOUT GENMASK(7, 0)
+
+#define MAX77541_BITS_MX_CFG1_RU_SR GENMASK(2, 0)
+#define MAX77541_BITS_MX_CFG1_RD_SR GENMASK(5, 3)
+#define MAX77541_BITS_MX_CFG1_RNG GENMASK(7, 6)
+
+#define MAX77541_BIT_MX_CFG2_FPWM BIT(0)
+#define MAX77541_BIT_MX_CFG2_FSREN BIT(1)
+#define MAX77541_BITS_MX_CFG2_SS_PAT GENMASK(3, 2)
+#define MAX77541_BITS_MX_CFG2_SS_FREQ GENMASK(5, 4)
+#define MAX77541_BITS_MX_CFG2_SS_ENV GENMASK(7, 6)
+
+#define MAX77541_BITS_MX_CFG3_ILIM GENMASK(1, 0)
+#define MAX77541_BITS_MX_CFG3_FREQ GENMASK(3, 2)
+#define MAX77541_BIT_MX_CFG3_FTRAK BIT(4)
+#define MAX77541_BIT_MX_CFG3_RESRESH BIT(5)
+#define MAX77541_BIT_MX_CFG3_ADIS1 BIT(6)
+#define MAX77541_BIT_MX_CFG3_ADIS100 BIT(7)
+
+#define MAX77541_MAX_REGULATORS 2
+
+/* ADC */
+#define MAX77541_REG_ADC_INT 0x70
+#define MAX77541_REG_ADC_MSK 0x71
+
+#define MAX77541_REG_ADC_DATA_CH1 0x72
+#define MAX77541_REG_ADC_DATA_CH2 0x73
+#define MAX77541_REG_ADC_DATA_CH3 0x74
+#define MAX77541_REG_ADC_DATA_CH6 0x77
+
+#define MAX77541_BIT_ADC_INT_CH1_I BIT(0)
+#define MAX77541_BIT_ADC_INT_CH2_I BIT(1)
+#define MAX77541_BIT_ADC_INT_CH3_I BIT(2)
+#define MAX77541_BIT_ADC_INT_CH6_I BIT(5)
+
+#define MAX77541_REGMAP_IRQ_REG(_mask) \
+ { .mask = (_mask), }
+
+enum dev_type {
+ MAX77540 = 1,
+ MAX77541 = 2,
+};
+
+enum max77541_regulators {
+ MAX77541_BUCK1 = 1,
+ MAX77541_BUCK2,
+};
+
+enum mx_range {
+ LOW_RANGE,
+ MID_RANGE,
+ HIGH_RANGE,
+ RESERVED
+};
+
+struct max77541_dev {
+ void *pdata;
+ struct device *dev;
+
+ struct regmap_irq_chip_data *irq_data;
+ struct regmap_irq_chip_data *irq_buck;
+ struct regmap_irq_chip_data *irq_topsys;
+ struct regmap_irq_chip_data *irq_adc;
+
+ struct i2c_client *i2c;
+ struct regmap *regmap;
+
+ u8 type;
+};
+
+#endif /* __MAX77541_MFD_H__ */
--
2.30.2
On 07/12/2022 10:08, Okan Sahin wrote:
> This patch adds document the bindings for MAX77541 MFD driver. It also
Do not use "This commit/patch".
https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95
> includes MAX77540 driver whose regmap is covered by MAX77541.
>
> Signed-off-by: Okan Sahin <[email protected]>
> ---
> .../devicetree/bindings/mfd/adi,max77541.yaml | 134 ++++++++++++++++++
> MAINTAINERS | 1 +
> 2 files changed, 135 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/mfd/adi,max77541.yaml
>
> diff --git a/Documentation/devicetree/bindings/mfd/adi,max77541.yaml b/Documentation/devicetree/bindings/mfd/adi,max77541.yaml
> new file mode 100644
> index 000000000000..205953e6dd15
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/adi,max77541.yaml
> @@ -0,0 +1,134 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/mfd/adi,max77541.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: MAX77540/MAX77541 PMIC from ADI.
Drop trailing space.
> +
> +maintainers:
> + - Okan Sahin <[email protected]>
> +
> +description: |
> + MAX77540 is a Power Management IC with 2 buck regulators.
> +
> + MAX77541 is a Power Management IC with 2 buck regulators and 1 ADC.
> +
> +properties:
> + compatible:
> + enum:
> + - adi,max77540
> + - adi,max77541
> +
> + reg:
> + maxItems: 1
> +
> + interrupts:
> + maxItems: 1
> +
> + regulators:
> + $ref: /schemas/regulator/adi,max77541.yaml#
I don't think you tested this patch. There is no such file.
> +
> + adc:
> + type: object
> + additionalProperties: false
> + properties:
> + compatible:
> + const: adi,max77541-adc
Why having a child without any resources? It does not look like needed
and instead your parent driver should instantiate the child device.
> + required:
> + -compatible
> +
> +required:
> + - compatible
> + - reg
> + - interrupts
> +
> +allOf:
> + - if:
> + properties:
> + compatible:
> + contains:
> + const: adi,max77540
> + then:
> + properties:
> + regulator:
> + properties:
> + compatible:
> + const: adi,max77540-regulator
> + else:
> + properties:
> + regulator:
> + properties:
> + compatible:
> + const: adi,max77541-regulator
> + adc:
> + properties:
> + compatible:
> + const: adi,max77541-adc
The adc part is not needed anyway - duplicating what's in top-level
properties.
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + #include <dt-bindings/interrupt-controller/irq.h>
> +
> + i2c {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + pmic@69 {
> + compatible = "adi,max77540";
> + reg = <0x69>;
> + interrupt-parent = <&gpio>;
> + interrupts = <16 IRQ_TYPE_EDGE_FALLING>;
> +
> + regulators {
> + buck1 {
> + regulator-min-microvolt = <500000>;
> + regulator-max-microvolt = <5200000>;
> + regulator-boot-on;
> + regulator-always-on;
> + };
> + buck2 {
> + regulator-min-microvolt = <500000>;
> + regulator-max-microvolt = <5200000>;
> + regulator-boot-on;
> + regulator-always-on;
> + };
> + };
> + };
> + };
> +
> + - |
> + #include <dt-bindings/interrupt-controller/irq.h>
> +
> + i2c {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + pmic@69 {
> + compatible = "adi,max77541";
Keep only one example (more complex one) - they are almost the same.
> + reg = <0x63>;
> + interrupt-parent = <&gpio>;
> + interrupts = <16 IRQ_TYPE_EDGE_FALLING>;
> +
> + regulators {
> + buck1 {
> + regulator-min-microvolt = <500000>;
> + regulator-max-microvolt = <5200000>;
> + regulator-boot-on;
> + regulator-always-on;
> + };
> + buck2 {
> + regulator-min-microvolt = <500000>;
> + regulator-max-microvolt = <5200000>;
> + regulator-boot-on;
> + regulator-always-on;
> + };
> + };
> +
> + adc {
> + compatible = "adi,max77541-adc";
> + }
> + };
> + };
> \ No newline at end of file
Error here.
> diff --git a/MAINTAINERS b/MAINTAINERS
> index af94d06bb9f0..22f5a9c490e3 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -12501,6 +12501,7 @@ MAXIM MAX77541 PMIC MFD DRIVER
> M: Okan Sahin <[email protected]>
> L: [email protected]
> S: Maintained
> +F: Documentation/devicetree/bindings/mfd/adi,max77541.yaml
> F: drivers/mfd/max77541.c
> F: include/linux/mfd/max77541.h
>
Best regards,
Krzysztof
This patch add adc support for MAX77541.
The MAX77541 has an 8-bit Successive Approximation Register (SAR) ADC
with four multiplexers for supporting the telemetry feature
Signed-off-by: Okan Sahin <[email protected]>
---
MAINTAINERS | 1 +
drivers/iio/adc/Kconfig | 11 ++
drivers/iio/adc/Makefile | 1 +
drivers/iio/adc/max77541-adc.c | 224 +++++++++++++++++++++++++++++++++
4 files changed, 237 insertions(+)
create mode 100644 drivers/iio/adc/max77541-adc.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 8e5572b28a8c..18ce4644cc75 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12503,6 +12503,7 @@ L: [email protected]
S: Maintained
F: Documentation/devicetree/bindings/mfd/adi,max77541.yaml
F: Documentation/devicetree/bindings/regulator/adi,max77541.yaml
+F: drivers/iio/adc/max77541-adc.c
F: drivers/mfd/max77541.c
F: drivers/regulator/max77541-regulator.c
F: include/linux/mfd/max77541.h
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 791612ca6012..2e7833b33f12 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -696,6 +696,17 @@ config MAX1363
To compile this driver as a module, choose M here: the module will be
called max1363.
+config MAX77541_ADC
+ tristate "Analog Devices MAX77541 ADC driver"
+ depends on MFD_MAX77541
+ help
+ This driver controls a Analog Devices MAX77541 adc
+ via I2C bus. This device has one adc. Say yes here to build
+ support for Analog Devices MAX77541 ADC interface.
+
+ To compile this driver as a module, choose M here: the module will be
+ called max77541-adc.
+
config MAX9611
tristate "Maxim max9611/max9612 ADC driver"
depends on I2C
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index 46caba7a010c..03774cccbb4b 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -64,6 +64,7 @@ obj-$(CONFIG_MAX1118) += max1118.o
obj-$(CONFIG_MAX11205) += max11205.o
obj-$(CONFIG_MAX1241) += max1241.o
obj-$(CONFIG_MAX1363) += max1363.o
+obj-$(CONFIG_MAX77541_ADC) += max77541-adc.o
obj-$(CONFIG_MAX9611) += max9611.o
obj-$(CONFIG_MCP320X) += mcp320x.o
obj-$(CONFIG_MCP3422) += mcp3422.o
diff --git a/drivers/iio/adc/max77541-adc.c b/drivers/iio/adc/max77541-adc.c
new file mode 100644
index 000000000000..7ca8576bd4e1
--- /dev/null
+++ b/drivers/iio/adc/max77541-adc.c
@@ -0,0 +1,224 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2022 Analog Devices, Inc.
+ * ADI MAX77541 ADC Driver with IIO interface
+ */
+
+#include <linux/bitfield.h>
+#include <linux/iio/iio.h>
+#include <include/linux/mfd/max77541.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/regulator/driver.h>
+#include <linux/regulator/of_regulator.h>
+
+#define MAX77541_ADC_CHANNEL(_channel, _name, _type, _reg) \
+ { \
+ .type = _type, \
+ .indexed = 1, \
+ .channel = _channel, \
+ .address = _reg, \
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
+ BIT(IIO_CHAN_INFO_SCALE) |\
+ BIT(IIO_CHAN_INFO_OFFSET),\
+ .datasheet_name = _name, \
+ }
+
+enum {
+ MAX77541_ADC_CH1_I = 0,
+ MAX77541_ADC_CH2_I,
+ MAX77541_ADC_CH3_I,
+ MAX77541_ADC_CH6_I,
+
+ MAX77541_ADC_IRQMAX_I,
+};
+
+struct max77541_adc_iio {
+ struct regmap *regmap;
+ int irq;
+ int irq_arr[MAX77541_ADC_IRQMAX_I];
+};
+
+enum max77541_adc_channel {
+ MAX77541_ADC_VSYS_V = 0,
+ MAX77541_ADC_VOUT1_V,
+ MAX77541_ADC_VOUT2_V,
+ MAX77541_ADC_TEMP,
+};
+
+static int max77541_adc_offset(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ int *val, int *val2)
+{
+ switch (chan->channel) {
+ case MAX77541_ADC_VSYS_V:
+ case MAX77541_ADC_VOUT1_V:
+ case MAX77541_ADC_VOUT2_V:
+ *val = 0;
+ *val2 = 0;
+ return IIO_VAL_INT_PLUS_MICRO;
+ case MAX77541_ADC_TEMP:
+ *val = -273;
+ *val2 = 0;
+ return IIO_VAL_INT_PLUS_MICRO;
+ default:
+ return -EINVAL;
+ }
+}
+
+static int max77541_adc_scale(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ int *val, int *val2)
+{
+ struct max77541_adc_iio *info = iio_priv(indio_dev);
+ unsigned int reg_val;
+ int ret;
+
+ switch (chan->channel) {
+ case MAX77541_ADC_VSYS_V:
+ *val = 0;
+ *val2 = 25000;
+ return IIO_VAL_INT_PLUS_MICRO;
+ case MAX77541_ADC_VOUT1_V:
+ ret = regmap_read(info->regmap, MAX77541_REG_M2_CFG1, ®_val);
+ if (ret)
+ return ret;
+
+ reg_val = FIELD_GET(MAX77541_BITS_MX_CFG1_RNG, reg_val);
+
+ *val = 0;
+
+ if (reg_val == LOW_RANGE)
+ *val2 = 6250;
+ else if (reg_val == MID_RANGE)
+ *val2 = 12500;
+ else if (reg_val == HIGH_RANGE)
+ *val2 = 25000;
+ else
+ return -EINVAL;
+
+ return IIO_VAL_INT_PLUS_MICRO;
+ case MAX77541_ADC_VOUT2_V:
+ ret = regmap_read(info->regmap, MAX77541_REG_M2_CFG1, ®_val);
+ if (ret)
+ return ret;
+ reg_val = FIELD_GET(MAX77541_BITS_MX_CFG1_RNG, reg_val);
+
+ *val = 0;
+
+ if (reg_val == LOW_RANGE)
+ *val2 = 6250;
+ else if (reg_val == MID_RANGE)
+ *val2 = 12500;
+ else if (reg_val == HIGH_RANGE)
+ *val2 = 25000;
+ else
+ return -EINVAL;
+
+ return IIO_VAL_INT_PLUS_MICRO;
+ case MAX77541_ADC_TEMP:
+ *val = 1;
+ *val2 = 725000;
+ return IIO_VAL_INT_PLUS_MICRO;
+ default:
+ return -EINVAL;
+ }
+}
+
+static int max77541_adc_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ int *val)
+{
+ struct max77541_adc_iio *info = iio_priv(indio_dev);
+ int ret;
+
+ ret = regmap_read(info->regmap, chan->address, val);
+ if (ret)
+ return ret;
+
+ return IIO_VAL_INT;
+}
+
+static const struct iio_chan_spec max77541_adc_channels[] = {
+ MAX77541_ADC_CHANNEL(MAX77541_ADC_VSYS_V, "vsys_v", IIO_VOLTAGE,
+ MAX77541_REG_ADC_DATA_CH1),
+ MAX77541_ADC_CHANNEL(MAX77541_ADC_VOUT1_V, "vout1_v", IIO_VOLTAGE,
+ MAX77541_REG_ADC_DATA_CH2),
+ MAX77541_ADC_CHANNEL(MAX77541_ADC_VOUT2_V, "vout2_v", IIO_VOLTAGE,
+ MAX77541_REG_ADC_DATA_CH3),
+ MAX77541_ADC_CHANNEL(MAX77541_ADC_TEMP, "temp", IIO_TEMP,
+ MAX77541_REG_ADC_DATA_CH6),
+};
+
+static int max77541_adc_read_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ int *val, int *val2, long mask)
+{
+ switch (mask) {
+ case IIO_CHAN_INFO_OFFSET:
+ return max77541_adc_offset(indio_dev, chan, val, val2);
+ case IIO_CHAN_INFO_SCALE:
+ return max77541_adc_scale(indio_dev, chan, val, val2);
+ case IIO_CHAN_INFO_RAW:
+ return max77541_adc_raw(indio_dev, chan, val);
+ default:
+ return -EINVAL;
+ }
+}
+
+static const struct iio_info max77541_adc_info = {
+ .read_raw = max77541_adc_read_raw,
+};
+
+static int max77541_adc_probe(struct platform_device *pdev)
+{
+ struct max77541_dev *max77541;
+ struct max77541_adc_iio *info;
+ struct iio_dev *indio_dev;
+
+ indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*info));
+ if (!indio_dev)
+ return -ENOMEM;
+
+ info = iio_priv(indio_dev);
+
+ info->regmap = max77541->regmap;
+ indio_dev->modes = INDIO_DIRECT_MODE;
+
+ indio_dev->name = platform_get_device_id(pdev)->name;
+ indio_dev->info = &max77541_adc_info;
+ indio_dev->channels = max77541_adc_channels;
+ indio_dev->num_channels = ARRAY_SIZE(max77541_adc_channels);
+
+ return devm_iio_device_register(&pdev->dev, indio_dev);
+}
+
+static const struct platform_device_id max77541_adc_platform_id[] = {
+ { "max77541-adc", MAX77541, },
+ { /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(platform, max77541_adc_platform_id);
+
+static const struct of_device_id max77541_adc_of_id[] = {
+ {
+ .compatible = "adi,max77541-adc",
+ .data = (void *)MAX77541,
+ },
+ { /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, max77541_adc_of_id);
+
+static struct platform_driver max77541_adc_driver = {
+ .driver = {
+ .name = "max77541-adc",
+ .of_match_table = max77541_adc_of_id,
+ },
+ .probe = max77541_adc_probe,
+ .id_table = max77541_adc_platform_id,
+};
+
+module_platform_driver(max77541_adc_driver);
+
+MODULE_AUTHOR("Okan Sahin <[email protected]>");
+MODULE_DESCRIPTION("MAX77541 ADC driver");
+MODULE_LICENSE("GPL");
--
2.30.2
On Wed, Dec 07, 2022 at 12:08:39PM +0300, Okan Sahin wrote:
> This patchset adds mfd, regulator and adc driver and related bindings.The patches
> are required to be applied in sequence.
You have an indentation / wrapping issues in the above text.
Nevertheless, why staging? What does it mean?
--
With Best Regards,
Andy Shevchenko
On Wed, Dec 07, 2022 at 12:08:44PM +0300, Okan Sahin wrote:
> This patch add adc support for MAX77541.
>
> The MAX77541 has an 8-bit Successive Approximation Register (SAR) ADC
> with four multiplexers for supporting the telemetry feature
Same comment as per patch 2.
...
> +#include <linux/bitfield.h>
> +#include <linux/iio/iio.h>
> +#include <include/linux/mfd/max77541.h>
Hmm...
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/driver.h>
> +#include <linux/regulator/of_regulator.h>
...
> +enum {
> + MAX77541_ADC_CH1_I = 0,
> + MAX77541_ADC_CH2_I,
> + MAX77541_ADC_CH3_I,
> + MAX77541_ADC_CH6_I,
> +
> + MAX77541_ADC_IRQMAX_I,
If it's a terminator, drop the trailing comma.
> +};
...
> + case MAX77541_ADC_TEMP:
> + *val = -273;
I believe we have definition for this in units.h. Can you use it?
> + *val2 = 0;
> + return IIO_VAL_INT_PLUS_MICRO;
> + }
> +}
...
> + *val = 0;
> +
> + if (reg_val == LOW_RANGE)
> + *val2 = 6250;
> + else if (reg_val == MID_RANGE)
> + *val2 = 12500;
> + else if (reg_val == HIGH_RANGE)
> + *val2 = 25000;
> + else
> + return -EINVAL;
Can it be provided as a table?
...
> + *val = 0;
> +
> + if (reg_val == LOW_RANGE)
> + *val2 = 6250;
> + else if (reg_val == MID_RANGE)
> + *val2 = 12500;
> + else if (reg_val == HIGH_RANGE)
> + *val2 = 25000;
> + else
> + return -EINVAL;
Ditto.
...
> +
Redundant blank line.
> +module_platform_driver(max77541_adc_driver);
--
With Best Regards,
Andy Shevchenko
On Wed, Dec 07, 2022 at 12:08:40PM +0300, Okan Sahin wrote:
> This patch adds MFD driver for MAX77541/MAX77540 to enable its sub
> devices.
>
> The MAX77541 is a multi-function devices. It includes
> buck converter and ADC.
>
> The MAX77540 is a high-efficiency buck converter
> with two 3A switching phases.
>
> They have same regmap except for ADC part of MAX77541.
Same comment as per patch 2.
...
> + help
> + Say yes here to add support for Analog Devices
> + MAX77541 and MAX77540 Power Management ICs.
> + This driver provides common support for accessing the device;
> + additional drivers must be enabled in order to use the functionality
> + of the device.
Arbitrary line lengths.
...
> + return devm_mfd_add_devices(dev, -1, max77540_devs, ARRAY_SIZE(max77540_devs),
There is a definition for -1, use it.
> + NULL, 0, NULL);
...
> + return devm_mfd_add_devices(dev, -1, max77541_devs, ARRAY_SIZE(max77541_devs),
Ditto.
> + NULL, 0, NULL);
...
> + chip_id = to_i2c_driver(client->dev.driver)->id_table;
> + if (!chip_data) {
> + chip_data = (void *)i2c_match_id(chip_id, client)->driver_data;
> + }
We have a new helper for this.
...
> + return dev_err_probe(me->dev, PTR_ERR(me->regmap),
> + "Failed to allocate register map\n");
Wrong indentation.
...
> +
Redundant blank line.
> +module_i2c_driver(max77541_i2c_driver);
...
> +MODULE_VERSION("1.0");
Why?
...
Missing inclusions:
- bits.h
- types.h
Missing forward declarations for:
struct device
struct regmap
struct regmap_irq_chip_data
...
> +/* REGISTERS */
...
> +#define MAX77541_REGMAP_IRQ_REG(_mask) \
> + { .mask = (_mask), }
When {} are on one line, the trailing comma inside is not needed.
...
> +enum mx_range {
> + LOW_RANGE,
> + MID_RANGE,
> + HIGH_RANGE,
> + RESERVED
Is it terminator?
> +};
...
> +struct max77541_dev {
> + void *pdata;
Why do you need this?
> + struct device *dev;
Isn't it the same as dev inside regmap?
> + struct regmap_irq_chip_data *irq_data;
> + struct regmap_irq_chip_data *irq_buck;
> + struct regmap_irq_chip_data *irq_topsys;
> + struct regmap_irq_chip_data *irq_adc;
> + struct i2c_client *i2c;
Is this is really needed? Perhaps regmap below provides what you need, no?
> + struct regmap *regmap;
> +
> + u8 type;
> +};
--
With Best Regards,
Andy Shevchenko
Hi Okan,
I love your patch! Yet something to improve:
[auto build test ERROR on staging/staging-testing]
url: https://github.com/intel-lab-lkp/linux/commits/Okan-Sahin/staging-drivers-mfd-Add-MAX77541-MFD-and-related-device-drivers/20221207-171559
patch link: https://lore.kernel.org/r/20221207090906.5896-6-okan.sahin%40analog.com
patch subject: [PATCH 5/5] staging: drivers: iio: adc: Adc MAX77541 ADC Support
config: s390-allyesconfig
compiler: s390-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/37b9db52db8439588f267900b71bf61c294e8e6f
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Okan-Sahin/staging-drivers-mfd-Add-MAX77541-MFD-and-related-device-drivers/20221207-171559
git checkout 37b9db52db8439588f267900b71bf61c294e8e6f
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=s390 SHELL=/bin/bash
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
All errors (new ones prefixed by >>):
>> drivers/iio/adc/max77541-adc.c:9:10: fatal error: include/linux/mfd/max77541.h: No such file or directory
9 | #include <include/linux/mfd/max77541.h>
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
compilation terminated.
vim +9 drivers/iio/adc/max77541-adc.c
> 9 #include <include/linux/mfd/max77541.h>
10 #include <linux/platform_device.h>
11 #include <linux/regmap.h>
12 #include <linux/regulator/driver.h>
13 #include <linux/regulator/of_regulator.h>
14
--
0-DAY CI Kernel Test Service
https://01.org/lkp
On Wed, 07 Dec 2022 12:08:41 +0300, Okan Sahin wrote:
> This patch adds document the bindings for MAX77541 MFD driver. It also
> includes MAX77540 driver whose regmap is covered by MAX77541.
>
> Signed-off-by: Okan Sahin <[email protected]>
> ---
> .../devicetree/bindings/mfd/adi,max77541.yaml | 134 ++++++++++++++++++
> MAINTAINERS | 1 +
> 2 files changed, 135 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/mfd/adi,max77541.yaml
>
My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):
yamllint warnings/errors:
./Documentation/devicetree/bindings/mfd/adi,max77541.yaml:134:7: [error] no new line character at the end of file (new-line-at-end-of-file)
dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/mfd/adi,max77541.yaml: properties:adc:required: '-compatible' is not of type 'array'
from schema $id: http://json-schema.org/draft-07/schema#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/mfd/adi,max77541.yaml: properties:adc:required: '-compatible' is not of type 'array'
from schema $id: http://devicetree.org/meta-schemas/keywords.yaml#
./Documentation/devicetree/bindings/mfd/adi,max77541.yaml: Unable to find schema file matching $id: http://devicetree.org/schemas/regulator/adi,max77541.yaml
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/mfd/adi,max77541.yaml: ignoring, error in schema: properties: adc: required
Error: Documentation/devicetree/bindings/mfd/adi,max77541.example.dts:99.13-14 syntax error
FATAL ERROR: Unable to parse input tree
make[1]: *** [scripts/Makefile.lib:406: Documentation/devicetree/bindings/mfd/adi,max77541.example.dtb] Error 1
make[1]: *** Waiting for unfinished jobs....
make: *** [Makefile:1492: dt_binding_check] Error 2
doc reference errors (make refcheckdocs):
See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/[email protected]
The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.
If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:
pip3 install dtschema --upgrade
Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.
On Wed, 7 Dec 2022 13:09:34 +0200
Andy Shevchenko <[email protected]> wrote:
> On Wed, Dec 07, 2022 at 12:08:39PM +0300, Okan Sahin wrote:
> > This patchset adds mfd, regulator and adc driver and related bindings.The patches
> > are required to be applied in sequence.
>
> You have an indentation / wrapping issues in the above text.
>
> Nevertheless, why staging? What does it mean?
>
The main reason to go via staging is because a driver is sitting out
of tree and it is useful to bring it in on the basis that it can then be
cleaned up in tree before moving out of staging.
For a relatively small driver like this, that's hard to argue. Just
clean it up in response to review feedback and then we can take it
directly into relevant subsystems in the main tree.
Jonathan
On Wed, 7 Dec 2022 12:08:40 +0300
Okan Sahin <[email protected]> wrote:
> This patch adds MFD driver for MAX77541/MAX77540 to enable its sub
> devices.
>
> The MAX77541 is a multi-function devices. It includes
> buck converter and ADC.
>
> The MAX77540 is a high-efficiency buck converter
> with two 3A switching phases.
>
> They have same regmap except for ADC part of MAX77541.
>
> Signed-off-by: Okan Sahin <[email protected]>
I'll try not to duplicate the points Andy has already made, but there
may be some overlap!
> ---
> MAINTAINERS | 7 ++
> drivers/mfd/Kconfig | 13 ++
> drivers/mfd/Makefile | 1 +
> drivers/mfd/max77541.c | 236 +++++++++++++++++++++++++++++++++++
> include/linux/mfd/max77541.h | 150 ++++++++++++++++++++++
Huh? It's not in staging, so just change the patch titles!
> 5 files changed, 407 insertions(+)
> create mode 100644 drivers/mfd/max77541.c
> create mode 100644 include/linux/mfd/max77541.h
>
or MAX77620 and MAX20024 PMIC Support"
> depends on I2C=y
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 7ed3ef4a698c..bf21228f5742 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_MAX77541) += max77541.o
> obj-$(CONFIG_MFD_MAX77620) += max77620.o
> obj-$(CONFIG_MFD_MAX77650) += max77650.o
> obj-$(CONFIG_MFD_MAX77686) += max77686.o
> diff --git a/drivers/mfd/max77541.c b/drivers/mfd/max77541.c
> new file mode 100644
> index 000000000000..97a2df3ce0b6
> --- /dev/null
> +++ b/drivers/mfd/max77541.c
> @@ -0,0 +1,236 @@
...
> +
> +static int max77541_pmic_setup(struct max77541_dev *me)
> +{
> + struct regmap *regmap = me->regmap;
> + struct device *dev = me->dev;
> + unsigned int val;
> + int ret;
> +
> + ret = max77541_pmic_irq_init(me);
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to initialize IRQ\n");
> +
> + ret = regmap_read(regmap, MAX77541_REG_INT_SRC, &val);
> + if (ret)
> + return ret;
> +
> + ret = regmap_read(regmap, MAX77541_REG_TOPSYS_INT, &val);
> + if (ret)
> + return ret;
> +
> + ret = regmap_read(regmap, MAX77541_REG_BUCK_INT, &val);
> + if (ret)
> + return ret;
> +
> + ret = device_init_wakeup(dev, true);
> + if (ret)
> + return dev_err_probe(dev, ret, "Unable to init wakeup\n");
> +
> + switch (me->type) {
> + case MAX77540:
once you've switched to using a chip_info structure instead this block just becomes
return devm_mfd_add_devices(dev, -1, me->chip_info.devs, me->chip_info.num_devs,
NULL, 0, NULL);
> + return devm_mfd_add_devices(dev, -1, max77540_devs, ARRAY_SIZE(max77540_devs),
> + NULL, 0, NULL);
> + case MAX77541:
> + return devm_mfd_add_devices(dev, -1, max77541_devs, ARRAY_SIZE(max77541_devs),
> + NULL, 0, NULL);
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int max77541_i2c_probe(struct i2c_client *client)
> +{
> + const struct i2c_device_id *chip_id;
> + struct max77541_dev *me;
> + const void *chip_data;
> +
> + me = devm_kzalloc(&client->dev, sizeof(*me), GFP_KERNEL);
> + if (!me)
> + return -ENOMEM;
> +
> + i2c_set_clientdata(client, me);
> + me->dev = &client->dev;
> + me->i2c = client;
> +
> + chip_id = to_i2c_driver(client->dev.driver)->id_table;
> +
> + chip_data = device_get_match_data(me->dev);
> + if (!chip_data) {
> + chip_data = (void *)i2c_match_id(chip_id, client)->driver_data;
Ah. This is why your enum doesn't have 0 in it. Solve that by using a poitner
to a chip_info structure rather than an enum value
> + if (!chip_data)
> + return dev_err_probe(me->dev, -ENODEV, "Unable to find device\n");
Message is rather misleading. More a case of unknown device.
> + }
> +
> + me->type = (enum dev_type)chip_data;
> + me->regmap = devm_regmap_init_i2c(client, &max77541_regmap_config);
> + if (IS_ERR(me->regmap))
> + return dev_err_probe(me->dev, PTR_ERR(me->regmap),
> + "Failed to allocate register map\n");
> +
> + return max77541_pmic_setup(me);
> +}
> +
> +static const struct of_device_id max77541_of_id[] = {
> + {
> + .compatible = "adi,max77540",
> + .data = (void *)MAX77540,
> + },
> + {
> + .compatible = "adi,max77541",
> + .data = (void *)MAX77541,
> + },
> + { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, max77541_of_id);
> +
> +static const struct i2c_device_id max77541_i2c_id[] = {
> + { "max77540", MAX77540 },
> + { "max77541", MAX77541 },
> + { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(i2c, max77541_i2c_id);
> +
> +static struct i2c_driver max77541_i2c_driver = {
> + .driver = {
> + .name = "max77541",
> + .of_match_table = max77541_of_id,
> + },
> + .probe_new = max77541_i2c_probe,
> + .id_table = max77541_i2c_id,
> +};
> +
> +module_i2c_driver(max77541_i2c_driver);
> +
> +MODULE_DESCRIPTION("MAX7740/MAX7741 MFD Driver");
> +MODULE_AUTHOR("Okan Sahin <[email protected]>");
> +MODULE_LICENSE("GPL");
> +MODULE_VERSION("1.0");
We almost never assign module versions any more. They aren't useful
given we must maintain backwards compatibility of userspace interfaces anyway.
> diff --git a/include/linux/mfd/max77541.h b/include/linux/mfd/max77541.h
> new file mode 100644
> index 000000000000..6f2753300227
> --- /dev/null
> +++ b/include/linux/mfd/max77541.h
> @@ -0,0 +1,150 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +
> +#ifndef __MAX77541_MFD_H__
> +#define __MAX77541_MFD_H__
> +
> +/* REGISTERS */
> +
> +/* GLOBAL CONFIG1 */
> +#define MAX77541_REG_INT_SRC 0x00
> +#define MAX77541_REG_INT_SRC_M 0x01
> +#define MAX77541_REG_TOPSYS_INT 0x02
> +#define MAX77541_REG_TOPSYS_INT_M 0x03
> +#define MAX77541_REG_TOPSYS_STAT 0x04
> +#define MAX77541_REG_DEVICE_CFG1 0x06
> +#define MAX77541_REG_DEVICE_CFG2 0x07
> +#define MAX77541_REG_TOPSYS_CFG 0x08
> +#define MAX77541_REG_PROT_CFG 0x09
> +#define MAX77541_REG_EN_CTRL 0x0B
> +
> +/* GLOBAL CONFIG2 */
Hmm. A register called CFG1 is CONFIG2?
That's novel ;)
> +#define MAX77541_REG_GLB_CFG1 0x11
> +
> +/* BUCK CONFIG */
> +#define MAX77541_REG_BUCK_INT 0x20
> +#define MAX77541_REG_BUCK_INT_M 0x21
> +#define MAX77541_REG_BUCK_STAT 0x22
> +
> +/* BUCK1 */
> +#define MAX77541_REG_M1_VOUT 0x23
> +#define MAX77541_REG_M1_CFG1 0x25
> +#define MAX77541_REG_M1_CFG2 0x26
> +#define MAX77541_REG_M1_CFG3 0x27
> +
> +/* BUCK2 */
> +#define MAX77541_REG_M2_VOUT 0x33
> +#define MAX77541_REG_M2_CFG1 0x35
> +#define MAX77541_REG_M2_CFG2 0x36
> +#define MAX77541_REG_M2_CFG3 0x37
> +
> +#define MAX77541_REG_NOT_AVAILABLE 0xFF
> +
> +/* INTERRUPT MASKS*/
> +#define MAX77541_REG_INT_SRC_MASK 0x00
> +#define MAX77541_REG_TOPSYS_INT_MASK 0x00
> +#define MAX77541_REG_BUCK_INT_MASK 0x00
> +
> +/*BITS OF REGISTERS*/
> +
> +#define MAX77541_BIT_INT_SRC_TOPSYS BIT(0)
> +#define MAX77541_BIT_INT_SRC_BUCK BIT(1)
> +
> +#define MAX77541_BIT_TOPSYS_INT_TJ_120C BIT(0)
> +#define MAX77541_BIT_TOPSYS_INT_TJ_140C BIT(1)
> +#define MAX77541_BIT_TOPSYS_INT_TSHDN BIT(2)
> +#define MAX77541_BIT_TOPSYS_INT_UVLO BIT(3)
> +#define MAX77541_BIT_TOPSYS_INT_ALT_SWO BIT(4)
> +#define MAX77541_BIT_TOPSYS_INT_EXT_FREQ_DET BIT(5)
> +
> +#define MAX77541_BIT_BUCK_INT_M1_POK_FLT BIT(0)
> +#define MAX77541_BIT_BUCK_INT_M2_POK_FLT BIT(1)
> +#define MAX77541_BIT_BUCK_INT_M1_SCFLT BIT(4)
> +#define MAX77541_BIT_BUCK_INT_M2_SCFLT BIT(5)
> +
> +#define MAX77541_BITS_DEVICE_CFG1_SEL1_LATCH GENMASK(4, 0)
> +#define MAX77541_BITS_DEVICE_CFG2_SEL2_LATCH GENMASK(4, 0)
> +
> +#define MAX77541_BIT_TOPSYS_CFG_DIS_ALT_IN BIT(0)
> +
> +#define MAX77541_BITS_PROT_CFG_POK_TO GENMASK(1, 0)
> +#define MAX77541_BIT_PROT_CFG_EN_FTMON BIT(2)
> +
> +#define MAX77541_BIT_M1_EN BIT(0)
> +#define MAX77541_BIT_M2_EN BIT(1)
> +#define MAX77541_BIT_M1_LPM BIT(4)
> +#define MAX77541_BIT_M2_LPM BIT(5)
> +
> +#define MAX77541_BITS_GLB_CFG1_SSTOP_SR GENMASK(2, 0)
Naming of BITS is unusual for what I think is a field mask.
MSK or MASK is more common. Cn use that for the single bit
fields as well.
> +#define MAX77541_BITS_GLB_CFG1_SSTRT_SR GENMASK(5, 3)
> +
> +#define MAX77541_BITS_MX_VOUT GENMASK(7, 0)
> +
> +#define MAX77541_BITS_MX_CFG1_RU_SR GENMASK(2, 0)
> +#define MAX77541_BITS_MX_CFG1_RD_SR GENMASK(5, 3)
> +#define MAX77541_BITS_MX_CFG1_RNG GENMASK(7, 6)
> +
> +#define MAX77541_BIT_MX_CFG2_FPWM BIT(0)
> +#define MAX77541_BIT_MX_CFG2_FSREN BIT(1)
> +#define MAX77541_BITS_MX_CFG2_SS_PAT GENMASK(3, 2)
> +#define MAX77541_BITS_MX_CFG2_SS_FREQ GENMASK(5, 4)
> +#define MAX77541_BITS_MX_CFG2_SS_ENV GENMASK(7, 6)
> +
> +#define MAX77541_BITS_MX_CFG3_ILIM GENMASK(1, 0)
> +#define MAX77541_BITS_MX_CFG3_FREQ GENMASK(3, 2)
> +#define MAX77541_BIT_MX_CFG3_FTRAK BIT(4)
> +#define MAX77541_BIT_MX_CFG3_RESRESH BIT(5)
> +#define MAX77541_BIT_MX_CFG3_ADIS1 BIT(6)
> +#define MAX77541_BIT_MX_CFG3_ADIS100 BIT(7)
> +
> +#define MAX77541_MAX_REGULATORS 2
> +
> +/* ADC */
> +#define MAX77541_REG_ADC_INT 0x70
> +#define MAX77541_REG_ADC_MSK 0x71
> +
> +#define MAX77541_REG_ADC_DATA_CH1 0x72
> +#define MAX77541_REG_ADC_DATA_CH2 0x73
> +#define MAX77541_REG_ADC_DATA_CH3 0x74
> +#define MAX77541_REG_ADC_DATA_CH6 0x77
> +
> +#define MAX77541_BIT_ADC_INT_CH1_I BIT(0)
> +#define MAX77541_BIT_ADC_INT_CH2_I BIT(1)
> +#define MAX77541_BIT_ADC_INT_CH3_I BIT(2)
> +#define MAX77541_BIT_ADC_INT_CH6_I BIT(5)
> +
> +#define MAX77541_REGMAP_IRQ_REG(_mask) \
> + { .mask = (_mask), }
Define is longer than the code. Just use the code everywhere
you current use this macro.
> +
> +enum dev_type {
> + MAX77540 = 1,
If you need to index from 1, add a comment on why.
> + MAX77541 = 2,
> +};
> +
> +enum max77541_regulators {
> + MAX77541_BUCK1 = 1,
> + MAX77541_BUCK2,
> +};
> +
> +enum mx_range {
Prefix this with max77541
Also, probably better to introduce it in the patch where it is used.
For now I've no idea why we need a reserved value for example!
> + LOW_RANGE,
> + MID_RANGE,
> + HIGH_RANGE,
> + RESERVED
> +};
> +
> +struct max77541_dev {
> + u8 type;
Rather than using an enum (incidentally the type of this should be a
named enum, not a u8) much better to define a
'chip info' structure that describes the different specific features of
each chip. That approach ends up much more extensible as new devices
are added to the driver than a constant increase in switch statements.
> +};
> +
> +#endif /* __MAX77541_MFD_H__ */
On Wed, 7 Dec 2022 12:08:44 +0300
Okan Sahin <[email protected]> wrote:
> This patch add adc support for MAX77541.
>
> The MAX77541 has an 8-bit Successive Approximation Register (SAR) ADC
> with four multiplexers for supporting the telemetry feature
Good to add a little detail on what features the driver currently supports.
>
> Signed-off-by: Okan Sahin <[email protected]>
...
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 791612ca6012..2e7833b33f12 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -696,6 +696,17 @@ config MAX1363
> To compile this driver as a module, choose M here: the module will be
> called max1363.
>
> +config MAX77541_ADC
> + tristate "Analog Devices MAX77541 ADC driver"
> + depends on MFD_MAX77541
> + help
> + This driver controls a Analog Devices MAX77541 adc
ADC
> + via I2C bus. This device has one adc. Say yes here to build
> + support for Analog Devices MAX77541 ADC interface.
> +
> + To compile this driver as a module, choose M here: the module will be
> + called max77541-adc.
> +
...
> diff --git a/drivers/iio/adc/max77541-adc.c b/drivers/iio/adc/max77541-adc.c
> new file mode 100644
> index 000000000000..7ca8576bd4e1
> --- /dev/null
> +++ b/drivers/iio/adc/max77541-adc.c
> @@ -0,0 +1,224 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) 2022 Analog Devices, Inc.
> + * ADI MAX77541 ADC Driver with IIO interface
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/iio/iio.h>
> +#include <include/linux/mfd/max77541.h>
Move this driver specific header include to below the main
block. We want to make it clear it is special.
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/driver.h>
> +#include <linux/regulator/of_regulator.h>
Would be very very surprised to see off_regulator.h correctly included
in an IIO driver. Check all of these for whether they are necessary
(rather than cut and paste from another driver).
> +
> +#define MAX77541_ADC_CHANNEL(_channel, _name, _type, _reg) \
> + { \
> + .type = _type, \
> + .indexed = 1, \
> + .channel = _channel, \
> + .address = _reg, \
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
> + BIT(IIO_CHAN_INFO_SCALE) |\
> + BIT(IIO_CHAN_INFO_OFFSET),\
> + .datasheet_name = _name, \
> + }
> +
> +enum {
> + MAX77541_ADC_CH1_I = 0,
> + MAX77541_ADC_CH2_I,
> + MAX77541_ADC_CH3_I,
> + MAX77541_ADC_CH6_I,
> +
> + MAX77541_ADC_IRQMAX_I,
> +};
> +
> +struct max77541_adc_iio {
> + struct regmap *regmap;
> + int irq;
> + int irq_arr[MAX77541_ADC_IRQMAX_I];
Clear out unused elements until the follow up patch that actually makes
us of them. For now they are just distracting noise.
> +};
> +
> +enum max77541_adc_channel {
> + MAX77541_ADC_VSYS_V = 0,
> + MAX77541_ADC_VOUT1_V,
> + MAX77541_ADC_VOUT2_V,
> + MAX77541_ADC_TEMP,
> +};
> +
> +static int max77541_adc_offset(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int *val, int *val2)
> +{
> + switch (chan->channel) {
> + case MAX77541_ADC_VSYS_V:
> + case MAX77541_ADC_VOUT1_V:
> + case MAX77541_ADC_VOUT2_V:
> + *val = 0;
> + *val2 = 0;
> + return IIO_VAL_INT_PLUS_MICRO;
If the offset is zero, then don't have the interface. Default
assumption if OFFSET is not provided is that the offset is 0.
> + case MAX77541_ADC_TEMP:
> + *val = -273;
> + *val2 = 0;
> + return IIO_VAL_INT_PLUS_MICRO;
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int max77541_adc_scale(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int *val, int *val2)
> +{
> + struct max77541_adc_iio *info = iio_priv(indio_dev);
> + unsigned int reg_val;
> + int ret;
> +
> + switch (chan->channel) {
> + case MAX77541_ADC_VSYS_V:
> + *val = 0;
> + *val2 = 25000;
> + return IIO_VAL_INT_PLUS_MICRO;
> + case MAX77541_ADC_VOUT1_V:
> + ret = regmap_read(info->regmap, MAX77541_REG_M2_CFG1, ®_val);
> + if (ret)
> + return ret;
> +
> + reg_val = FIELD_GET(MAX77541_BITS_MX_CFG1_RNG, reg_val);
> +
> + *val = 0;
> +
> + if (reg_val == LOW_RANGE)
Ah. Here is the mysterious macro from patch 1. Bring that definition forwards
to this patch. Also switch statement would be more readable here.
> + *val2 = 6250;
> + else if (reg_val == MID_RANGE)
> + *val2 = 12500;
> + else if (reg_val == HIGH_RANGE)
> + *val2 = 25000;
> + else
> + return -EINVAL;
> +
> + return IIO_VAL_INT_PLUS_MICRO;
> + case MAX77541_ADC_VOUT2_V:
> + ret = regmap_read(info->regmap, MAX77541_REG_M2_CFG1, ®_val);
Umm. Is this identical to the above case? If so, just have one block for
both.
> + if (ret)
> + return ret;
> + reg_val = FIELD_GET(MAX77541_BITS_MX_CFG1_RNG, reg_val);
> +
> + *val = 0;
> +
> + if (reg_val == LOW_RANGE)
> + *val2 = 6250;
> + else if (reg_val == MID_RANGE)
> + *val2 = 12500;
> + else if (reg_val == HIGH_RANGE)
> + *val2 = 25000;
> + else
> + return -EINVAL;
> +
> + return IIO_VAL_INT_PLUS_MICRO;
> + case MAX77541_ADC_TEMP:
> + *val = 1;
> + *val2 = 725000;
> + return IIO_VAL_INT_PLUS_MICRO;
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +
> +static const struct iio_chan_spec max77541_adc_channels[] = {
Bring the macro definition down to just above this array. That will
make it easier to review as we can see the right parameters are being
passed in. For 4 channels, I'm not sure I'd bother wrapping it in
a macro at all (particularly as that macro needs to be more
complex - see above), but I don't mind if you want to keep it that way.
I would also move this down below the iio_info definition because
that will keep all the code relevant to that in one place rather
than throwing this in the middle.
> + MAX77541_ADC_CHANNEL(MAX77541_ADC_VSYS_V, "vsys_v", IIO_VOLTAGE,
> + MAX77541_REG_ADC_DATA_CH1),
> + MAX77541_ADC_CHANNEL(MAX77541_ADC_VOUT1_V, "vout1_v", IIO_VOLTAGE,
> + MAX77541_REG_ADC_DATA_CH2),
> + MAX77541_ADC_CHANNEL(MAX77541_ADC_VOUT2_V, "vout2_v", IIO_VOLTAGE,
> + MAX77541_REG_ADC_DATA_CH3),
> + MAX77541_ADC_CHANNEL(MAX77541_ADC_TEMP, "temp", IIO_TEMP,
> + MAX77541_REG_ADC_DATA_CH6),
> +};
> +
> +static int max77541_adc_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int *val, int *val2, long mask)
> +{
> + switch (mask) {
> + case IIO_CHAN_INFO_OFFSET:
> + return max77541_adc_offset(indio_dev, chan, val, val2);
> + case IIO_CHAN_INFO_SCALE:
> + return max77541_adc_scale(indio_dev, chan, val, val2);
> + case IIO_CHAN_INFO_RAW:
> + return max77541_adc_raw(indio_dev, chan, val);
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static const struct iio_info max77541_adc_info = {
> + .read_raw = max77541_adc_read_raw,
> +};
> +
> +static int max77541_adc_probe(struct platform_device *pdev)
> +{
> + struct max77541_dev *max77541;
> + struct max77541_adc_iio *info;
> + struct iio_dev *indio_dev;
> +
> + indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*info));
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + info = iio_priv(indio_dev);
> +
> + info->regmap = max77541->regmap;
> + indio_dev->modes = INDIO_DIRECT_MODE;
> +
> + indio_dev->name = platform_get_device_id(pdev)->name;
> + indio_dev->info = &max77541_adc_info;
> + indio_dev->channels = max77541_adc_channels;
> + indio_dev->num_channels = ARRAY_SIZE(max77541_adc_channels);
> +
> + return devm_iio_device_register(&pdev->dev, indio_dev);
> +}
> +
> +static const struct platform_device_id max77541_adc_platform_id[] = {
> + { "max77541-adc", MAX77541, },
Better to introduce the complexity given by the type only when it is needed.
So for now drop the .data values here and below.
If / when that does happen I'll be asking you to use a chip_info structure
anyway - right now that structure would be empty so no point in adding it
yet.
> + { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(platform, max77541_adc_platform_id);
> +
> +static const struct of_device_id max77541_adc_of_id[] = {
> + {
> + .compatible = "adi,max77541-adc",
> + .data = (void *)MAX77541,
> + },
> + { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, max77541_adc_of_id);
> +
> +static struct platform_driver max77541_adc_driver = {
> + .driver = {
> + .name = "max77541-adc",
> + .of_match_table = max77541_adc_of_id,
> + },
> + .probe = max77541_adc_probe,
> + .id_table = max77541_adc_platform_id,
> +};
> +
Drop blank line. Macro and structure are very closely associated
so it's good to make that obvious by not separating them.
> +module_platform_driver(max77541_adc_driver);
> +
> +MODULE_AUTHOR("Okan Sahin <[email protected]>");
> +MODULE_DESCRIPTION("MAX77541 ADC driver");
> +MODULE_LICENSE("GPL");
On Sun, Dec 11, 2022 at 12:20:43PM +0000, Jonathan Cameron wrote:
> On Wed, 7 Dec 2022 13:09:34 +0200
> Andy Shevchenko <[email protected]> wrote:
> > On Wed, Dec 07, 2022 at 12:08:39PM +0300, Okan Sahin wrote:
> > > This patchset adds mfd, regulator and adc driver and related bindings.The patches
> > > are required to be applied in sequence.
> >
> > You have an indentation / wrapping issues in the above text.
> >
> > Nevertheless, why staging? What does it mean?
>
> The main reason to go via staging is because a driver is sitting out
> of tree and it is useful to bring it in on the basis that it can then be
> cleaned up in tree before moving out of staging.
But files are not in staging. Me being confused.
> For a relatively small driver like this, that's hard to argue. Just
> clean it up in response to review feedback and then we can take it
> directly into relevant subsystems in the main tree.
--
With Best Regards,
Andy Shevchenko
On Sun, 11 Dec 2022 15:34:10 +0200
Andy Shevchenko <[email protected]> wrote:
> On Sun, Dec 11, 2022 at 12:20:43PM +0000, Jonathan Cameron wrote:
> > On Wed, 7 Dec 2022 13:09:34 +0200
> > Andy Shevchenko <[email protected]> wrote:
> > > On Wed, Dec 07, 2022 at 12:08:39PM +0300, Okan Sahin wrote:
> > > > This patchset adds mfd, regulator and adc driver and related bindings.The patches
> > > > are required to be applied in sequence.
> > >
> > > You have an indentation / wrapping issues in the above text.
> > >
> > > Nevertheless, why staging? What does it mean?
> >
> > The main reason to go via staging is because a driver is sitting out
> > of tree and it is useful to bring it in on the basis that it can then be
> > cleaned up in tree before moving out of staging.
>
> But files are not in staging. Me being confused.
I noticed that when I got to the patches :)
Odd indeed - I'm guessing some cut and paste gone wrong.
Jonathan
>
> > For a relatively small driver like this, that's hard to argue. Just
> > clean it up in response to review feedback and then we can take it
> > directly into relevant subsystems in the main tree.
>