MFD, regulator and ADC driver and related bindings for MAX77540/MAX77541.
The patches are requeired to be applied in sequence.
Changes in v3:
* Patch 1: "drivers: mfd: Add ADI MAX77541/MAX77540 PMIC Support"
* Change struct name from max77541_dev to max77541
* Adjust max-line-length lower than 80
* Patch 2: "dt-bindings: mfd: Add ADI MAX77541/MAX77540"
* Remove adc object as we do not need
* Remove adc node from example
* Patch 3: "drivers: regulator: Add ADI MAX77541/MAX77540 Regulator Support"
* Change node name from "BUCK#_id" to "buck#_id" in regulator desc
* Patch 4: "dt-bindings: regulator: Add ADI MAX77541/MAX77540 Regulator"
* Change node name from "BUCK" to "buck" in regulators
* Patch 5: "drivers: iio: adc: Add ADI MAX77541 ADC Support"
* Convert voltage values from V to mV for scaling.
* Convert temperature values from C to miliC for scale and offset
* Do not set offset bit in info_mask_separate for voltage that does not need offset
* Remove unnecessary dev_get_drvdata() instead of it use dev_get_regmap to have regmap.
* Assing hard coded name for adc dev name
Changes in v2:
* Patch 1: "drivers: mfd: Add MAX77541/MAX77540 PMIC Support"
* Drop "this patch adds" from commit message.
* Drop redundant blank lines.
* Drop module version
* Use definition for parameter of devm_mfd_add_devices(.., -1,..)
* Use desc in chip_info to adding desc for different devices.
* Add missing headers and forward declarations.
* Drop unused elements from max77541_dev struct
* Add chip_info into max77541_dev struct to identify different devices.
* Patch 2: "dt-bindings: mfd: adi,max77541.yaml Add MAX77541 bindings"
* Drop "this patch adds" from commit message.
* Fix $ref path
* Drop adc part under allOf
* Keep only one example (more complex one)
* Fix make dt_binding_check errors.(trailing space, No newline)
* Patch 3: "drivers: regulator: Add MAX77541 Regulator Support"
* Drop "this patch adds" from commit message.
* Add trailing comma for required structs.
* Fix wrong indentation.
* Drop redundant blank lines.
* Drop max77541_regulator_dev struct.
* Use "regulator_desc *desc" for both regulator
regarding to "max77541->id"
* Patch 4: "dt-bindings: regulator: max77541-regulator.yaml Add MAX77541
Regulator bindings"
* Drop "this patch adds" from commit message.
* Chance filename (matching compatible), so adi,max77541-regulator.yaml
* Fix make dt_binding_check errors.(trailing space, No newline)
* Patch 5: "drivers: iio: adc: Add MAX77541 ADC Support"
* Drop "this patch adds" from commit message.
* Drop redundant blank lines.
* Fix wrong include path.
* Use switch instead of if-else for range setting in max77541_adc_scale
* Move max77541_adc_range enum from max77541.h to here.
* Use definition from units.h
* Drop unused elements from max77541_adc_iio struct
* Drop the .data from platform_device_id
Okan Sahin (5):
drivers: mfd: Add ADI MAX77541/MAX77540 PMIC Support
dt-bindings: mfd: Add ADI MAX77541/MAX77540
drivers: regulator: Add ADI MAX77541/MAX77540 Regulator Support
dt-bindings: regulator: Add ADI MAX77541/MAX77540 Regulator
drivers: iio: adc: Add ADI MAX77541 ADC Support
.../devicetree/bindings/mfd/adi,max77541.yaml | 87 ++++++
.../regulator/adi,max77541-regulator.yaml | 44 +++
MAINTAINERS | 11 +
drivers/iio/adc/Kconfig | 11 +
drivers/iio/adc/Makefile | 1 +
drivers/iio/adc/max77541-adc.c | 200 ++++++++++++++
drivers/mfd/Kconfig | 13 +
drivers/mfd/Makefile | 1 +
drivers/mfd/max77541.c | 250 ++++++++++++++++++
drivers/regulator/Kconfig | 9 +
drivers/regulator/Makefile | 1 +
drivers/regulator/max77541-regulator.c | 160 +++++++++++
include/linux/mfd/max77541.h | 108 ++++++++
13 files changed, 896 insertions(+)
create mode 100644 Documentation/devicetree/bindings/mfd/adi,max77541.yaml
create mode 100644 Documentation/devicetree/bindings/regulator/adi,max77541-regulator.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
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 | 250 +++++++++++++++++++++++++++++++++++
include/linux/mfd/max77541.h | 108 +++++++++++++++
5 files changed, 379 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..e6bf621cbc8e 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..0ccb96df5db0
--- /dev/null
+++ b/drivers/mfd/max77541.c
@@ -0,0 +1,250 @@
+// 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 const struct chip_info chip[] = {
+ [MAX77540] = {
+ .n_devs = ARRAY_SIZE(max77540_devs),
+ .devs = max77540_devs,
+ },
+ [MAX77541] = {
+ .n_devs = ARRAY_SIZE(max77541_devs),
+ .devs = max77541_devs,
+ },
+};
+
+static int max77541_pmic_irq_init(struct device *dev)
+{
+ struct max77541 *max77541 = dev_get_drvdata(dev);
+ int irq = max77541->i2c->irq;
+ int ret;
+
+ ret = devm_regmap_add_irq_chip(dev, max77541->regmap, irq,
+ IRQF_ONESHOT | IRQF_SHARED, 0,
+ &max77541_src_irq_chip,
+ &max77541->irq_data);
+ if (ret)
+ return ret;
+
+ ret = devm_regmap_add_irq_chip(dev, max77541->regmap, irq,
+ IRQF_ONESHOT | IRQF_SHARED, 0,
+ &max77541_topsys_irq_chip,
+ &max77541->irq_topsys);
+ if (ret)
+ return ret;
+
+ ret = devm_regmap_add_irq_chip(dev, max77541->regmap, irq,
+ IRQF_ONESHOT | IRQF_SHARED, 0,
+ &max77541_buck_irq_chip,
+ &max77541->irq_buck);
+ if (ret)
+ return ret;
+
+ if (max77541->id == MAX77541) {
+ ret = devm_regmap_add_irq_chip(dev, max77541->regmap, irq,
+ IRQF_ONESHOT | IRQF_SHARED, 0,
+ &max77541_adc_irq_chip,
+ &max77541->irq_adc);
+ if (ret)
+ return ret;
+ }
+
+ return ret;
+}
+
+static int max77541_pmic_setup(struct device *dev)
+{
+ struct max77541 *max77541 = dev_get_drvdata(dev);
+ unsigned int val;
+ int ret;
+
+ ret = max77541_pmic_irq_init(dev);
+ if (ret)
+ return dev_err_probe(dev, ret, "Failed to initialize IRQ\n");
+
+ ret = regmap_read(max77541->regmap, MAX77541_REG_INT_SRC, &val);
+ if (ret)
+ return ret;
+
+ ret = regmap_read(max77541->regmap, MAX77541_REG_TOPSYS_INT, &val);
+ if (ret)
+ return ret;
+
+ ret = regmap_read(max77541->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 (max77541->id) {
+ 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 *id)
+{
+ struct device *dev = &client->dev;
+ struct max77541 *max77541;
+ const void *match;
+
+ max77541 = devm_kzalloc(&client->dev, sizeof(*max77541), GFP_KERNEL);
+ if (!max77541)
+ return -ENOMEM;
+
+ i2c_set_clientdata(client, max77541);
+ max77541->i2c = client;
+
+ match = device_get_match_data(dev);
+ if (match)
+ max77541->id = (enum max7754x_ids)match;
+ else if (id)
+ max77541->id = id->driver_data;
+ else
+ return -ENODEV;
+
+ max77541->chip = &chip[max77541->id];
+
+ max77541->regmap = devm_regmap_init_i2c(client,
+ &max77541_regmap_config);
+ if (IS_ERR(max77541->regmap))
+ return dev_err_probe(dev, PTR_ERR(max77541->regmap),
+ "Failed to allocate register map\n");
+
+ return max77541_pmic_setup(dev);
+}
+
+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 = 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..44c3c0c2aa97
--- /dev/null
+++ b/include/linux/mfd/max77541.h
@@ -0,0 +1,108 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+
+#ifndef __MAX77541_MFD_H__
+#define __MAX77541_MFD_H__
+
+#include <linux/bits.h>
+#include <linux/types.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_EN_CTRL 0x0B
+
+/* BUCK CONFIG */
+#define MAX77541_REG_BUCK_INT 0x20
+#define MAX77541_REG_BUCK_INT_M 0x21
+
+#define MAX77541_REG_M1_VOUT 0x23
+#define MAX77541_REG_M1_CFG1 0x25
+
+#define MAX77541_REG_M2_VOUT 0x33
+#define MAX77541_REG_M2_CFG1 0x35
+
+/* 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_BIT_M1_EN BIT(0)
+#define MAX77541_BIT_M2_EN BIT(1)
+
+#define MAX77541_BITS_MX_VOUT GENMASK(7, 0)
+#define MAX77541_BITS_MX_CFG1_RNG GENMASK(7, 6)
+
+/* 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_MAX_REGULATORS 2
+
+#define MAX77541_REGMAP_IRQ_REG(_mask) \
+ { .mask = (_mask) }
+
+enum max7754x_ids {
+ MAX77540 = 1,
+ MAX77541 = 2,
+};
+
+enum max77541_regulators {
+ MAX77541_BUCK1 = 1,
+ MAX77541_BUCK2,
+};
+
+struct chip_info {
+ int n_devs;
+ const struct mfd_cell *devs;
+};
+
+struct regmap;
+struct regmap_irq_chip_data;
+struct i2c_client;
+
+struct max77541 {
+ enum max7754x_ids id;
+ const struct chip_info *chip;
+
+ 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;
+};
+
+#endif /* __MAX77541_MFD_H__ */
--
2.30.2
Regulator driver for both MAX77541 and MAX77540.
The MAX77541 is a high-efficiency step-down converter
with two 3A switching phases for single-cell Li+ battery
and 5VDC systems.
The MAX77540 is a high-efficiency step-down converter
with two 3A switching phases.
Signed-off-by: Okan Sahin <[email protected]>
---
MAINTAINERS | 1 +
drivers/regulator/Kconfig | 9 ++
drivers/regulator/Makefile | 1 +
drivers/regulator/max77541-regulator.c | 160 +++++++++++++++++++++++++
4 files changed, 171 insertions(+)
create mode 100644 drivers/regulator/max77541-regulator.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 22f5a9c490e3..5704ed5afce3 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12503,6 +12503,7 @@ L: [email protected]
S: Maintained
F: Documentation/devicetree/bindings/mfd/adi,max77541.yaml
F: drivers/mfd/max77541.c
+F: drivers/regulator/max77541-regulator.c
F: include/linux/mfd/max77541.h
MAXIM MAX77650 PMIC MFD DRIVER
diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index 070e4403c6c2..1e416c195af9 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -556,6 +556,15 @@ config REGULATOR_MAX597X
The MAX5970/5978 is a smart switch with no output regulation, but
fault protection and voltage and current monitoring capabilities.
+config REGULATOR_MAX77541
+ tristate "Analog Devices MAX77541/77540 Regulator"
+ depends on MFD_MAX77541
+ help
+ This driver controls a Analog Devices MAX77541/77540 regulators
+ via I2C bus. Both MAX77540 and MAX77541 are dual-phase
+ high-efficiency buck converter. Say Y here to
+ enable the regulator driver.
+
config REGULATOR_MAX77620
tristate "Maxim 77620/MAX20024 voltage regulator"
depends on MFD_MAX77620 || COMPILE_TEST
diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index 5962307e1130..c19efc7cfbef 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -68,6 +68,7 @@ obj-$(CONFIG_REGULATOR_LTC3676) += ltc3676.o
obj-$(CONFIG_REGULATOR_MAX14577) += max14577-regulator.o
obj-$(CONFIG_REGULATOR_MAX1586) += max1586.o
obj-$(CONFIG_REGULATOR_MAX597X) += max597x-regulator.o
+obj-$(CONFIG_REGULATOR_MAX77541) += max77541-regulator.o
obj-$(CONFIG_REGULATOR_MAX77620) += max77620-regulator.o
obj-$(CONFIG_REGULATOR_MAX77650) += max77650-regulator.o
obj-$(CONFIG_REGULATOR_MAX8649) += max8649.o
diff --git a/drivers/regulator/max77541-regulator.c b/drivers/regulator/max77541-regulator.c
new file mode 100644
index 000000000000..342d4f59405c
--- /dev/null
+++ b/drivers/regulator/max77541-regulator.c
@@ -0,0 +1,160 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2022 Analog Devices, Inc.
+ * ADI Regulator driver for the MAX77540 and MAX77541
+ */
+
+#include <linux/i2c.h>
+#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>
+
+static const struct regulator_ops max77541_buck_ops = {
+ .enable = regulator_enable_regmap,
+ .disable = regulator_disable_regmap,
+ .is_enabled = regulator_is_enabled_regmap,
+ .list_voltage = regulator_list_voltage_pickable_linear_range,
+ .get_voltage_sel = regulator_get_voltage_sel_pickable_regmap,
+ .set_voltage_sel = regulator_set_voltage_sel_pickable_regmap,
+};
+
+static const struct linear_range max77540_buck_ranges[] = {
+ /* Ranges when VOLT_SEL bits are 0x00 */
+ REGULATOR_LINEAR_RANGE(500000, 0x00, 0x8B, 5000),
+ REGULATOR_LINEAR_RANGE(1200000, 0x8C, 0xFF, 0),
+ /* Ranges when VOLT_SEL bits are 0x40 */
+ REGULATOR_LINEAR_RANGE(1200000, 0x00, 0x8B, 10000),
+ REGULATOR_LINEAR_RANGE(2400000, 0x8C, 0xFF, 0),
+ /* Ranges when VOLT_SEL bits are 0x80 */
+ REGULATOR_LINEAR_RANGE(2000000, 0x00, 0x9F, 20000),
+ REGULATOR_LINEAR_RANGE(5200000, 0xA0, 0xFF, 0),
+};
+
+static const struct linear_range max77541_buck_ranges[] = {
+ /* Ranges when VOLT_SEL bits are 0x00 */
+ REGULATOR_LINEAR_RANGE(300000, 0x00, 0xB3, 5000),
+ REGULATOR_LINEAR_RANGE(1200000, 0xB4, 0xFF, 0),
+ /* Ranges when VOLT_SEL bits are 0x40 */
+ REGULATOR_LINEAR_RANGE(1200000, 0x00, 0x8B, 10000),
+ REGULATOR_LINEAR_RANGE(2400000, 0x8C, 0xFF, 0),
+ /* Ranges when VOLT_SEL bits are 0x80 */
+ REGULATOR_LINEAR_RANGE(2000000, 0x00, 0x9F, 20000),
+ REGULATOR_LINEAR_RANGE(5200000, 0xA0, 0xFF, 0),
+};
+
+static const unsigned int max77541_buck_volt_range_sel[] = {
+ 0x00, 0x00, 0x40, 0x40, 0x80, 0x80,
+};
+
+#define MAX77540_BUCK(_id, _ops) \
+ { .id = MAX77541_BUCK ## _id, \
+ .name = "buck"#_id, \
+ .of_match = "buck"#_id, \
+ .regulators_node = "regulators", \
+ .enable_reg = MAX77541_REG_EN_CTRL, \
+ .enable_mask = MAX77541_BIT_M ## _id ## _EN, \
+ .ops = &(_ops), \
+ .type = REGULATOR_VOLTAGE, \
+ .linear_ranges = max77540_buck_ranges, \
+ .n_linear_ranges = ARRAY_SIZE(max77540_buck_ranges), \
+ .vsel_reg = MAX77541_REG_M ## _id ## _VOUT, \
+ .vsel_mask = MAX77541_BITS_MX_VOUT, \
+ .vsel_range_reg = MAX77541_REG_M ## _id ## _CFG1, \
+ .vsel_range_mask = MAX77541_BITS_MX_CFG1_RNG, \
+ .linear_range_selectors = max77541_buck_volt_range_sel, \
+ .owner = THIS_MODULE, \
+ }
+
+#define MAX77541_BUCK(_id, _ops) \
+ { .id = MAX77541_BUCK ## _id, \
+ .name = "buck"#_id, \
+ .of_match = "buck"#_id, \
+ .regulators_node = "regulators", \
+ .enable_reg = MAX77541_REG_EN_CTRL, \
+ .enable_mask = MAX77541_BIT_M ## _id ## _EN, \
+ .ops = &(_ops), \
+ .type = REGULATOR_VOLTAGE, \
+ .linear_ranges = max77541_buck_ranges, \
+ .n_linear_ranges = ARRAY_SIZE(max77541_buck_ranges), \
+ .vsel_reg = MAX77541_REG_M ## _id ## _VOUT, \
+ .vsel_mask = MAX77541_BITS_MX_VOUT, \
+ .vsel_range_reg = MAX77541_REG_M ## _id ## _CFG1, \
+ .vsel_range_mask = MAX77541_BITS_MX_CFG1_RNG, \
+ .linear_range_selectors = max77541_buck_volt_range_sel, \
+ .owner = THIS_MODULE, \
+ }
+
+static const struct regulator_desc max77540_regulators_desc[] = {
+ MAX77540_BUCK(1, max77541_buck_ops),
+ MAX77540_BUCK(2, max77541_buck_ops),
+};
+
+static const struct regulator_desc max77541_regulators_desc[] = {
+ MAX77541_BUCK(1, max77541_buck_ops),
+ MAX77541_BUCK(2, max77541_buck_ops),
+};
+
+static int max77541_regulator_probe(struct platform_device *pdev)
+{
+ struct max77541 *max77541 = dev_get_drvdata(pdev->dev.parent);
+ struct regulator_config config = {};
+ const struct regulator_desc *desc;
+ struct device *dev = &pdev->dev;
+ struct regulator_dev *rdev;
+ int i;
+
+ config.dev = pdev->dev.parent;
+
+ if (max77541->id == MAX77540)
+ desc = max77540_regulators_desc;
+ else if (max77541->id == MAX77541)
+ desc = max77541_regulators_desc;
+ else
+ return -EINVAL;
+
+ for (i = 0; i < MAX77541_MAX_REGULATORS; i++) {
+ rdev = devm_regulator_register(dev,
+ &desc[i], &config);
+ if (IS_ERR(rdev))
+ return dev_err_probe(dev, PTR_ERR(rdev),
+ "Failed to register regulator\n");
+ }
+
+ return 0;
+}
+
+static const struct platform_device_id max77541_regulator_platform_id[] = {
+ { "max77540-regulator", MAX77540 },
+ { "max77541-regulator", MAX77541 },
+ { /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(platform, max77541_regulator_platform_id);
+
+static const struct of_device_id max77541_regulator_of_id[] = {
+ {
+ .compatible = "adi,max77540-regulator",
+ .data = (void *)MAX77540,
+ },
+ {
+ .compatible = "adi,max77541-regulator",
+ .data = (void *)MAX77541,
+ },
+ { /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, max77541_regulator_of_id);
+
+static struct platform_driver max77541_regulator_driver = {
+ .driver = {
+ .name = "max77541-regulator",
+ .of_match_table = max77541_regulator_of_id,
+ },
+ .probe = max77541_regulator_probe,
+ .id_table = max77541_regulator_platform_id,
+};
+module_platform_driver(max77541_regulator_driver);
+
+MODULE_AUTHOR("Okan Sahin <[email protected]>");
+MODULE_DESCRIPTION("MAX77540/MAX77541 regulator driver");
+MODULE_LICENSE("GPL");
--
2.30.2
On Wed, Jan 18, 2023 at 09:38:08AM +0300, Okan Sahin wrote:
> 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.
...
> +/*
> + * Copyright (c) 2022 Analog Devices, Inc.
Happy New Year!
> + * Mfd core driver for the MAX77540 and MAX77541
MFD
> + */
...
> + ret = devm_regmap_add_irq_chip(dev, max77541->regmap, irq,
> + IRQF_ONESHOT | IRQF_SHARED, 0,
> + &max77541_buck_irq_chip,
> + &max77541->irq_buck);
> + if (ret)
> + return ret;
> +
> + if (max77541->id == MAX77541) {
> + ret = devm_regmap_add_irq_chip(dev, max77541->regmap, irq,
> + IRQF_ONESHOT | IRQF_SHARED, 0,
> + &max77541_adc_irq_chip,
> + &max77541->irq_adc);
> + if (ret)
> + return ret;
> + }
> + return ret;
return 0;
...
> + return devm_mfd_add_devices(dev, -1, max77540_devs,
PLATFORM_DEVID_NONE ?
> + ARRAY_SIZE(max77540_devs),
> + NULL, 0, NULL);
...
> + return devm_mfd_add_devices(dev, -1, max77541_devs,
Ditto.
> + ARRAY_SIZE(max77541_devs),
> + NULL, 0, NULL);
...
> +static int max77541_i2c_probe(struct i2c_client *client,
> + const struct i2c_device_id *id)
No id, please. I.o.w. you should use ->probe_new().
> +{
> + struct device *dev = &client->dev;
> + struct max77541 *max77541;
> + const void *match;
> +
> + max77541 = devm_kzalloc(&client->dev, sizeof(*max77541), GFP_KERNEL);
> + if (!max77541)
> + return -ENOMEM;
> +
> + i2c_set_clientdata(client, max77541);
> + max77541->i2c = client;
> +
> + match = device_get_match_data(dev);
> + if (match)
> + max77541->id = (enum max7754x_ids)match;
This is dangerous if your enum has 0 as a valid value.
Instead, use pointers in the driver_data, like
&chip_info chip[MAX77540]
> + else if (id)
> + max77541->id = id->driver_data;
> + else
It's better to check the ID range here.
Or since the change recommended above, check for NULL.
> + return -ENODEV;
> +
> + max77541->chip = &chip[max77541->id];
> +
> + max77541->regmap = devm_regmap_init_i2c(client,
> + &max77541_regmap_config);
> + if (IS_ERR(max77541->regmap))
> + return dev_err_probe(dev, PTR_ERR(max77541->regmap),
> + "Failed to allocate register map\n");
> +
> + return max77541_pmic_setup(dev);
> +}
...
> +/*BITS OF REGISTERS*/
Missing spaces.
--
With Best Regards,
Andy Shevchenko
On Wed, Jan 18, 2023 at 09:38:10AM +0300, Okan Sahin wrote:
> Regulator driver for both MAX77541 and MAX77540.
> The MAX77541 is a high-efficiency step-down converter
> with two 3A switching phases for single-cell Li+ battery
> and 5VDC systems.
>
> The MAX77540 is a high-efficiency step-down converter
> with two 3A switching phases.
...
> + * Copyright (c) 2022 Analog Devices, Inc.
Happy New Year!
...
> +static int max77541_regulator_probe(struct platform_device *pdev)
> +{
> + struct max77541 *max77541 = dev_get_drvdata(pdev->dev.parent);
> + struct regulator_config config = {};
> + const struct regulator_desc *desc;
> + struct device *dev = &pdev->dev;
You may rearrange this a bit
struct max77541 *max77541 = dev_get_drvdata(dev->parent);
> + struct regulator_dev *rdev;
> + int i;
> + config.dev = pdev->dev.parent;
dev->parent
> +
> + if (max77541->id == MAX77540)
> + desc = max77540_regulators_desc;
> + else if (max77541->id == MAX77541)
> + desc = max77541_regulators_desc;
> + else
> + return -EINVAL;
> +
> + for (i = 0; i < MAX77541_MAX_REGULATORS; i++) {
> + rdev = devm_regulator_register(dev,
> + &desc[i], &config);
This is perfectly one line.
> + if (IS_ERR(rdev))
> + return dev_err_probe(dev, PTR_ERR(rdev),
> + "Failed to register regulator\n");
> + }
> +
> + return 0;
> +}
...
> +static const struct of_device_id max77541_regulator_of_id[] = {
> + {
> + .compatible = "adi,max77540-regulator",
> + .data = (void *)MAX77540,
> + },
> + {
> + .compatible = "adi,max77541-regulator",
> + .data = (void *)MAX77541,
> + },
> + { /* sentinel */ }
As pointed out, better to use pointers directly.
> +};
--
With Best Regards,
Andy Shevchenko
On Wed, 18 Jan 2023, Andy Shevchenko wrote:
> On Wed, Jan 18, 2023 at 09:38:08AM +0300, Okan Sahin wrote:
> > 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.
>
> ...
>
> > +/*
> > + * Copyright (c) 2022 Analog Devices, Inc.
>
> Happy New Year!
If the code hasn't changed greatly since the Copyright, there is no
requirement to update the date.
> > + * Mfd core driver for the MAX77540 and MAX77541
>
> MFD
Please remove all mention of MFD - it's not a thing - we made it up!
--
Lee Jones [李琼斯]
On Thu, Jan 19, 2023 at 02:27:29PM +0000, Lee Jones wrote:
> On Wed, 18 Jan 2023, Andy Shevchenko wrote:
> > On Wed, Jan 18, 2023 at 09:38:08AM +0300, Okan Sahin wrote:
...
> > > +/*
> > > + * Copyright (c) 2022 Analog Devices, Inc.
> >
> > Happy New Year!
>
> If the code hasn't changed greatly since the Copyright, there is no
> requirement to update the date.
It would have made sense if we have/had seen that code in the past.
And since it's a v3 I think this is the case.
--
With Best Regards,
Andy Shevchenko
On Thu, Jan 19, 2023 at 02:27:29PM +0000, Lee Jones wrote:
> On Wed, 18 Jan 2023, Andy Shevchenko wrote:
>
> > On Wed, Jan 18, 2023 at 09:38:08AM +0300, Okan Sahin wrote:
> > > 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.
> >
> > ...
> >
> > > +/*
> > > + * Copyright (c) 2022 Analog Devices, Inc.
> >
> > Happy New Year!
>
> If the code hasn't changed greatly since the Copyright, there is no
> requirement to update the date.
Actually a date isn't necessary at all. In fact, by the Berne Convention
copyright protection exists the moment a work is completed, so you could
even argue a copyright notice is unnecessary and the SPDX line alone is
sufficient. However, I would say there is a practical benefit in a
copyright notice identifying the primary authors to make ownership
clear.
For the curious, this LF blog post gives some reasons about why
copyright notices may be better left simple:
https://www.linuxfoundation.org/blog/blog/copyright-notices-in-open-source-software-projects
William Breathitt Gray
Hi Andy,
Thank you for your feedback and efforts. I have a question below.
On Wed, 18 Jan 2022 11:20 AM
Andy Shevchenko <[email protected]> wrote:
>On Wed, Jan 18, 2023 at 09:38:10AM +0300, Okan Sahin wrote:
>> Regulator driver for both MAX77541 and MAX77540.
>> The MAX77541 is a high-efficiency step-down converter with two 3A
>> switching phases for single-cell Li+ battery and 5VDC systems.
>>
>> The MAX77540 is a high-efficiency step-down converter with two 3A
>> switching phases.
>
>...
>
>> + * Copyright (c) 2022 Analog Devices, Inc.
>
>Happy New Year!
>
>...
>
>> +static int max77541_regulator_probe(struct platform_device *pdev) {
>> + struct max77541 *max77541 = dev_get_drvdata(pdev->dev.parent);
>> + struct regulator_config config = {};
>> + const struct regulator_desc *desc;
>> + struct device *dev = &pdev->dev;
>
>You may rearrange this a bit
>
> struct max77541 *max77541 = dev_get_drvdata(dev->parent);
>
>> + struct regulator_dev *rdev;
>> + int i;
>
>> + config.dev = pdev->dev.parent;
>
>dev->parent
>
>> +
>> + if (max77541->id == MAX77540)
>> + desc = max77540_regulators_desc;
>> + else if (max77541->id == MAX77541)
>> + desc = max77541_regulators_desc;
>> + else
>> + return -EINVAL;
>> +
>> + for (i = 0; i < MAX77541_MAX_REGULATORS; i++) {
>
>> + rdev = devm_regulator_register(dev,
>> + &desc[i], &config);
>
>This is perfectly one line.
Thank you, I will arrange it.
>
>> + if (IS_ERR(rdev))
>> + return dev_err_probe(dev, PTR_ERR(rdev),
>> + "Failed to register regulator\n");
>> + }
>> +
>> + return 0;
>> +}
>
>...
However, this one is not fit when I set max-line-length argument as 80 in checkpatch script. What do you suggest? This line has 99 characters.
>
>> +static const struct of_device_id max77541_regulator_of_id[] = {
>> + {
>> + .compatible = "adi,max77540-regulator",
>> + .data = (void *)MAX77540,
>> + },
>> + {
>> + .compatible = "adi,max77541-regulator",
>> + .data = (void *)MAX77541,
>> + },
>> + { /* sentinel */ }
>
>As pointed out, better to use pointers directly.
>
>> +};
>
>--
>With Best Regards,
>Andy Shevchenko
>
Regards,
Okan Sahin
Hi Andy,
Sorry for second question. I do not want to bother you, but I realized that I need to be sure about driver_data before sending new patch. You said that you need to use pointers directly for driver_data then I fixed that part in mfd, but I do not need or use driver_data in regulator since chip_id comes from mfd device so I think using like below should be enough for my implementation.
static const struct platform_device_id max77541_regulator_platform_id[] = {
{ "max77540-regulator", },
{ "max77541-regulator", },
{ /* sentinel */ }
};
MODULE_DEVICE_TABLE(platform, max77541_regulator_platform_id);
static const struct of_device_id max77541_regulator_of_id[] = {
{ .compatible = "adi,max77540-regulator", },
{ .compatible = "adi,max77541-regulator", },
{ /* sentinel */ }
};
MODULE_DEVICE_TABLE(of, max77541_regulator_of_id);
What do you think?
On Tue, Jan 31, 2023 at 10:21 AM +0300, Okan Sahin wrote:
>Hi Andy,
>
>Thank you for your feedback and efforts. I have a question below.
>
>On Wed, 18 Jan 2022 11:20 AM
>Andy Shevchenko <[email protected]> wrote:
>
>>On Wed, Jan 18, 2023 at 09:38:10AM +0300, Okan Sahin wrote:
>>> Regulator driver for both MAX77541 and MAX77540.
>>> The MAX77541 is a high-efficiency step-down converter with two 3A
>>> switching phases for single-cell Li+ battery and 5VDC systems.
>>>
>>> The MAX77540 is a high-efficiency step-down converter with two 3A
>>> switching phases.
>>
>>...
>>
>>> + * Copyright (c) 2022 Analog Devices, Inc.
>>
>>Happy New Year!
>>
>>...
>>
>>> +static int max77541_regulator_probe(struct platform_device *pdev) {
>>> + struct max77541 *max77541 = dev_get_drvdata(pdev->dev.parent);
>>> + struct regulator_config config = {};
>>> + const struct regulator_desc *desc;
>>> + struct device *dev = &pdev->dev;
>>
>>You may rearrange this a bit
>>
>> struct max77541 *max77541 = dev_get_drvdata(dev->parent);
>>
>>> + struct regulator_dev *rdev;
>>> + int i;
>>
>>> + config.dev = pdev->dev.parent;
>>
>>dev->parent
>>
>>> +
>>> + if (max77541->id == MAX77540)
>>> + desc = max77540_regulators_desc;
>>> + else if (max77541->id == MAX77541)
>>> + desc = max77541_regulators_desc;
>>> + else
>>> + return -EINVAL;
>>> +
>>> + for (i = 0; i < MAX77541_MAX_REGULATORS; i++) {
>>
>>> + rdev = devm_regulator_register(dev,
>>> + &desc[i], &config);
>>
>>This is perfectly one line.
>Thank you, I will arrange it.
>>
>>> + if (IS_ERR(rdev))
>>> + return dev_err_probe(dev, PTR_ERR(rdev),
>>> + "Failed to register regulator\n");
>>> + }
>>> +
>>> + return 0;
>>> +}
>>
>>...
>However, this one is not fit when I set max-line-length argument as 80 in
>checkpatch script. What do you suggest? This line has 99 characters.
>>
>>> +static const struct of_device_id max77541_regulator_of_id[] = {
>>> + {
>>> + .compatible = "adi,max77540-regulator",
>>> + .data = (void *)MAX77540,
>>> + },
>>> + {
>>> + .compatible = "adi,max77541-regulator",
>>> + .data = (void *)MAX77541,
>>> + },
>>> + { /* sentinel */ }
>>
>>As pointed out, better to use pointers directly.
>>
>>> +};
>>
>>--
>>With Best Regards,
>>Andy Shevchenko
>>
>
>Regards,
>Okan Sahin
Regards,
Okan
On Tue, Jan 31, 2023 at 07:20:52AM +0000, Sahin, Okan wrote:
> On Wed, 18 Jan 2022 11:20 AM
> Andy Shevchenko <[email protected]> wrote:
> >On Wed, Jan 18, 2023 at 09:38:10AM +0300, Okan Sahin wrote:
...
> >> + rdev = devm_regulator_register(dev,
> >> + &desc[i], &config);
> >
> >This is perfectly one line.
> Thank you, I will arrange it.
> >
> >> + if (IS_ERR(rdev))
> >> + return dev_err_probe(dev, PTR_ERR(rdev),
> >> + "Failed to register regulator\n");
> >> + }
> >> +
> >> + return 0;
> >> +}
> However, this one is not fit when I set max-line-length argument as 80 in
> checkpatch script. What do you suggest? This line has 99 characters.
Which line do you refer to?
--
With Best Regards,
Andy Shevchenko
On Tue, Jan 31, 2023 at 09:27:48AM +0000, Sahin, Okan wrote:
First of all, please do avoid top-posting.
> Sorry for second question. I do not want to bother you, but I realized that I
> need to be sure about driver_data before sending new patch. You said that you
> need to use pointers directly for driver_data then I fixed that part in mfd,
> but I do not need or use driver_data in regulator since chip_id comes from
> mfd device so I think using like below should be enough for my
> implementation.
>
> static const struct platform_device_id max77541_regulator_platform_id[] = {
> { "max77540-regulator", },
> { "max77541-regulator", },
> { /* sentinel */ }
> };
> MODULE_DEVICE_TABLE(platform, max77541_regulator_platform_id);
>
> static const struct of_device_id max77541_regulator_of_id[] = {
> { .compatible = "adi,max77540-regulator", },
> { .compatible = "adi,max77541-regulator", },
> { /* sentinel */ }
> };
> MODULE_DEVICE_TABLE(of, max77541_regulator_of_id);
>
> What do you think?
If you have got all necessary data from the upper layer, why do you need to
have an ID table here? I'm not sure I understand how this OF ID table works
in this case.
--
With Best Regards,
Andy Shevchenko
On Tue, 31 Jan 2022 3:24 PM
Andy Shevchenko <[email protected]> wrote:
>On Tue, Jan 31, 2023 at 07:20:52AM +0000, Sahin, Okan wrote:
>> On Wed, 18 Jan 2022 11:20 AM
>> Andy Shevchenko <[email protected]> wrote:
>> >On Wed, Jan 18, 2023 at 09:38:10AM +0300, Okan Sahin wrote:
>
>...
>
>> >> + rdev = devm_regulator_register(dev,
>> >> + &desc[i], &config);
>> >
>> >This is perfectly one line.
>> Thank you, I will arrange it.
>> >
>> >> + if (IS_ERR(rdev))
>> >> + return dev_err_probe(dev, PTR_ERR(rdev),
>> >> + "Failed to register regulator\n");
>> >> + }
>> >> +
>> >> + return 0;
>> >> +}
>
>> However, this one is not fit when I set max-line-length argument as 80
>> in checkpatch script. What do you suggest? This line has 99 characters.
>
>Which line do you refer to?
I am referring "return dev_err_probe(dev, PTR_ERR(rdev), "Failed to register regulator\n");"
>
>--
>With Best Regards,
>Andy Shevchenko
>
Best Regards,
Okan Sahin
On Tue, 31 Jan 2022 3:27 PM
Andy Shevchenko <[email protected]> wrote:
>On Tue, Jan 31, 2023 at 09:27:48AM +0000, Sahin, Okan wrote:
>
>First of all, please do avoid top-posting.
>
>> Sorry for second question. I do not want to bother you, but I realized
>> that I need to be sure about driver_data before sending new patch. You
>> said that you need to use pointers directly for driver_data then I
>> fixed that part in mfd, but I do not need or use driver_data in
>> regulator since chip_id comes from mfd device so I think using like
>> below should be enough for my implementation.
>>
>> static const struct platform_device_id max77541_regulator_platform_id[] = {
>> { "max77540-regulator", },
>> { "max77541-regulator", },
>> { /* sentinel */ }
>> };
>> MODULE_DEVICE_TABLE(platform, max77541_regulator_platform_id);
>>
>> static const struct of_device_id max77541_regulator_of_id[] = {
>> { .compatible = "adi,max77540-regulator", },
>> { .compatible = "adi,max77541-regulator", },
>> { /* sentinel */ }
>> };
>> MODULE_DEVICE_TABLE(of, max77541_regulator_of_id);
>>
>> What do you think?
>
>If you have got all necessary data from the upper layer, why do you need to have
>an ID table here? I'm not sure I understand how this OF ID table works in this
>case.
I added it since there is regulator node in device tree. With the help of devm_regulator_register(..), driver takes parameters of regulator node. I also used id to select and to initialize regulator descriptors which are chip specific. So far there is no comment about OF ID table so I kept it. I thought I need to add both of id table and platform id table as name matching is required to initialize platform device from mfd.
>
>--
>With Best Regards,
>Andy Shevchenko
>
Best Regards,
Okan Sahin
On Tue, Jan 31, 2023 at 01:12:26PM +0000, Sahin, Okan wrote:
> On Tue, 31 Jan 2022 3:24 PM
> Andy Shevchenko <[email protected]> wrote:
> >On Tue, Jan 31, 2023 at 07:20:52AM +0000, Sahin, Okan wrote:
> >> On Wed, 18 Jan 2022 11:20 AM
> >> Andy Shevchenko <[email protected]> wrote:
> >> >On Wed, Jan 18, 2023 at 09:38:10AM +0300, Okan Sahin wrote:
...
> >> >> + rdev = devm_regulator_register(dev,
> >> >> + &desc[i], &config);
> >> >
> >> >This is perfectly one line.
> >> Thank you, I will arrange it.
> >> >
> >> >> + if (IS_ERR(rdev))
> >> >> + return dev_err_probe(dev, PTR_ERR(rdev),
> >> >> + "Failed to register regulator\n");
> >> >> + }
> >> >> +
> >> >> + return 0;
> >> >> +}
> >
> >> However, this one is not fit when I set max-line-length argument as 80
> >> in checkpatch script. What do you suggest? This line has 99 characters.
> >
> >Which line do you refer to?
> I am referring "return dev_err_probe(dev, PTR_ERR(rdev), "Failed to register
> regulator\n");"
I have had no comments on that line.
--
With Best Regards,
Andy Shevchenko
On Tue, Jan 31, 2023 at 01:23:33PM +0000, Sahin, Okan wrote:
> On Tue, 31 Jan 2022 3:27 PM
> Andy Shevchenko <[email protected]> wrote:
> >On Tue, Jan 31, 2023 at 09:27:48AM +0000, Sahin, Okan wrote:
...
> >> Sorry for second question. I do not want to bother you, but I realized
> >> that I need to be sure about driver_data before sending new patch. You
> >> said that you need to use pointers directly for driver_data then I
> >> fixed that part in mfd, but I do not need or use driver_data in
> >> regulator since chip_id comes from mfd device so I think using like
> >> below should be enough for my implementation.
> >>
> >> static const struct platform_device_id max77541_regulator_platform_id[] = {
> >> { "max77540-regulator", },
> >> { "max77541-regulator", },
> >> { /* sentinel */ }
> >> };
> >> MODULE_DEVICE_TABLE(platform, max77541_regulator_platform_id);
> >>
> >> static const struct of_device_id max77541_regulator_of_id[] = {
> >> { .compatible = "adi,max77540-regulator", },
> >> { .compatible = "adi,max77541-regulator", },
> >> { /* sentinel */ }
> >> };
> >> MODULE_DEVICE_TABLE(of, max77541_regulator_of_id);
> >>
> >> What do you think?
> >
> >If you have got all necessary data from the upper layer, why do you need to have
> >an ID table here? I'm not sure I understand how this OF ID table works in this
> >case.
> I added it since there is regulator node in device tree. With the help of
> devm_regulator_register(..), driver takes parameters of regulator node. I
> also used id to select and to initialize regulator descriptors which are chip
> specific. So far there is no comment about OF ID table so I kept it. I
> thought I need to add both of id table and platform id table as name matching
> is required to initialize platform device from mfd.
For platform device is one mechanism how to enumerate device, and bind it to
the driver. The OF ID table needs to be present in case you are using it for
direct DT enumeration (there is also something related to MFD child nodes, but
you need to check and explain how your device is enumerated by this driver).
I.o.w. please clarify how the OF ID table is being used.
--
With Best Regards,
Andy Shevchenko
On Tue, Jan 31, 2023 at 01:23:33PM +0000, Sahin, Okan wrote:
> Andy Shevchenko <[email protected]> wrote:
> >> static const struct of_device_id max77541_regulator_of_id[] = {
> >> { .compatible = "adi,max77540-regulator", },
> >> { .compatible = "adi,max77541-regulator", },
> >> { /* sentinel */ }
> >> };
> >> MODULE_DEVICE_TABLE(of, max77541_regulator_of_id);
> >> What do you think?
> >If you have got all necessary data from the upper layer, why do you need to have
> >an ID table here? I'm not sure I understand how this OF ID table works in this
> >case.
> I added it since there is regulator node in device tree. With the help
> of devm_regulator_register(..), driver takes parameters of regulator
> node. I also used id to select and to initialize regulator descriptors
> which are chip specific. So far there is no comment about OF ID table
> so I kept it. I thought I need to add both of id table and platform id
> table as name matching is required to initialize platform device from
> mfd.
You probably shouldn't have compatibles for subdevices of a MFD unless
it's some reusable IP that'll appear elsewhere, especially for something
like the regulators where grouping them all into a single device is very
Linux specific.
On Tue, 31 Jan 2022 4:30 PM
Andy Shevchenko <[email protected]> wrote:
>On Tue, Jan 31, 2023 at 01:23:33PM +0000, Sahin, Okan wrote:
>> On Tue, 31 Jan 2022 3:27 PM
>> Andy Shevchenko <[email protected]> wrote:
>> >On Tue, Jan 31, 2023 at 09:27:48AM +0000, Sahin, Okan wrote:
>
>...
>
>> >> Sorry for second question. I do not want to bother you, but I
>> >> realized that I need to be sure about driver_data before sending
>> >> new patch. You said that you need to use pointers directly for
>> >> driver_data then I fixed that part in mfd, but I do not need or
>> >> use driver_data in regulator since chip_id comes from mfd device so
>> >> I think using like below should be enough for my implementation.
>> >>
>> >> static const struct platform_device_id max77541_regulator_platform_id[] =
>{
>> >> { "max77540-regulator", },
>> >> { "max77541-regulator", },
>> >> { /* sentinel */ }
>> >> };
>> >> MODULE_DEVICE_TABLE(platform, max77541_regulator_platform_id);
>> >>
>> >> static const struct of_device_id max77541_regulator_of_id[] = {
>> >> { .compatible = "adi,max77540-regulator", },
>> >> { .compatible = "adi,max77541-regulator", },
>> >> { /* sentinel */ }
>> >> };
>> >> MODULE_DEVICE_TABLE(of, max77541_regulator_of_id);
>> >>
>> >> What do you think?
>> >
>> >If you have got all necessary data from the upper layer, why do you
>> >need to have an ID table here? I'm not sure I understand how this OF
>> >ID table works in this case.
>
>> I added it since there is regulator node in device tree. With the help
>> of devm_regulator_register(..), driver takes parameters of regulator
>> node. I also used id to select and to initialize regulator descriptors
>> which are chip specific. So far there is no comment about OF ID table
>> so I kept it. I thought I need to add both of id table and platform id
>> table as name matching is required to initialize platform device from mfd.
>
>For platform device is one mechanism how to enumerate device, and bind it to
>the driver. The OF ID table needs to be present in case you are using it for direct
>DT enumeration (there is also something related to MFD child nodes, but you
>need to check and explain how your device is enumerated by this driver).
>
>I.o.w. please clarify how the OF ID table is being used.
>
>--
>With Best Regards,
>Andy Shevchenko
>
Hi Andy,
I do not use "of id table" directly in max77541-regulator.c so do I need to exclude it?
However, devm_regulator_register(..) method initialize each regulator with the nodes under "regulators node". If of_match in desc and name of node matches, then regulator will be initialized with parameters in the node under the regulators node in the device tree. Since I am using device tree to initialize regulators, I added of id table. I hope I explained the situation clearly.
Regards,
Okan Sahin
On Tue, Jan 31, 2023 at 01:59:45PM +0000, Sahin, Okan wrote:
> On Tue, 31 Jan 2022 4:30 PM
> Andy Shevchenko <[email protected]> wrote:
> >On Tue, Jan 31, 2023 at 01:23:33PM +0000, Sahin, Okan wrote:
> >> On Tue, 31 Jan 2022 3:27 PM
> >> Andy Shevchenko <[email protected]> wrote:
> >> >On Tue, Jan 31, 2023 at 09:27:48AM +0000, Sahin, Okan wrote:
...
> >> >> Sorry for second question. I do not want to bother you, but I
> >> >> realized that I need to be sure about driver_data before sending
> >> >> new patch. You said that you need to use pointers directly for
> >> >> driver_data then I fixed that part in mfd, but I do not need or
> >> >> use driver_data in regulator since chip_id comes from mfd device so
> >> >> I think using like below should be enough for my implementation.
> >> >>
> >> >> static const struct platform_device_id max77541_regulator_platform_id[] =
> >{
> >> >> { "max77540-regulator", },
> >> >> { "max77541-regulator", },
> >> >> { /* sentinel */ }
> >> >> };
> >> >> MODULE_DEVICE_TABLE(platform, max77541_regulator_platform_id);
> >> >>
> >> >> static const struct of_device_id max77541_regulator_of_id[] = {
> >> >> { .compatible = "adi,max77540-regulator", },
> >> >> { .compatible = "adi,max77541-regulator", },
> >> >> { /* sentinel */ }
> >> >> };
> >> >> MODULE_DEVICE_TABLE(of, max77541_regulator_of_id);
> >> >>
> >> >> What do you think?
> >> >
> >> >If you have got all necessary data from the upper layer, why do you
> >> >need to have an ID table here? I'm not sure I understand how this OF
> >> >ID table works in this case.
> >
> >> I added it since there is regulator node in device tree. With the help
> >> of devm_regulator_register(..), driver takes parameters of regulator
> >> node. I also used id to select and to initialize regulator descriptors
> >> which are chip specific. So far there is no comment about OF ID table
> >> so I kept it. I thought I need to add both of id table and platform id
> >> table as name matching is required to initialize platform device from mfd.
> >
> >For platform device is one mechanism how to enumerate device, and bind it to
> >the driver. The OF ID table needs to be present in case you are using it for direct
> >DT enumeration (there is also something related to MFD child nodes, but you
> >need to check and explain how your device is enumerated by this driver).
> >
> >I.o.w. please clarify how the OF ID table is being used.
>
> I do not use "of id table" directly in max77541-regulator.c so do I need to exclude it?
Exactly my point. How does this OF ID table affect the device enumeration?
> However, devm_regulator_register(..) method initialize each regulator with
> the nodes under "regulators node". If of_match in desc and name of node
> matches, then regulator will be initialized with parameters in the node under
> the regulators node in the device tree. Since I am using device tree to
> initialize regulators, I added of id table. I hope I explained the situation
> clearly.
This is confusing. If your regulator is enumerated via DT, why do you need MFD?
--
With Best Regards,
Andy Shevchenko
On Tue, 31 Jan 2022 4:30 PM
Andy Shevchenko <[email protected]> wrote:
>On Tue, Jan 31, 2023 at 01:59:45PM +0000, Sahin, Okan wrote:
>> On Tue, 31 Jan 2022 4:30 PM
>> Andy Shevchenko <[email protected]> wrote:
>> >On Tue, Jan 31, 2023 at 01:23:33PM +0000, Sahin, Okan wrote:
>> >> On Tue, 31 Jan 2022 3:27 PM
>> >> Andy Shevchenko <[email protected]> wrote:
>> >> >On Tue, Jan 31, 2023 at 09:27:48AM +0000, Sahin, Okan wrote:
>
>...
>
>> >> >> Sorry for second question. I do not want to bother you, but I
>> >> >> realized that I need to be sure about driver_data before sending
>> >> >> new patch. You said that you need to use pointers directly for
>> >> >> driver_data then I fixed that part in mfd, but I do not need or
>> >> >> use driver_data in regulator since chip_id comes from mfd device
>> >> >> so I think using like below should be enough for my implementation.
>> >> >>
>> >> >> static const struct platform_device_id
>> >> >> max77541_regulator_platform_id[] =
>> >{
>> >> >> { "max77540-regulator", },
>> >> >> { "max77541-regulator", },
>> >> >> { /* sentinel */ }
>> >> >> };
>> >> >> MODULE_DEVICE_TABLE(platform, max77541_regulator_platform_id);
>> >> >>
>> >> >> static const struct of_device_id max77541_regulator_of_id[] = {
>> >> >> { .compatible = "adi,max77540-regulator", },
>> >> >> { .compatible = "adi,max77541-regulator", },
>> >> >> { /* sentinel */ }
>> >> >> };
>> >> >> MODULE_DEVICE_TABLE(of, max77541_regulator_of_id);
>> >> >>
>> >> >> What do you think?
>> >> >
>> >> >If you have got all necessary data from the upper layer, why do
>> >> >you need to have an ID table here? I'm not sure I understand how
>> >> >this OF ID table works in this case.
>> >
>> >> I added it since there is regulator node in device tree. With the
>> >> help of devm_regulator_register(..), driver takes parameters of
>> >> regulator node. I also used id to select and to initialize
>> >> regulator descriptors which are chip specific. So far there is no
>> >> comment about OF ID table so I kept it. I thought I need to add
>> >> both of id table and platform id table as name matching is required to
>initialize platform device from mfd.
>> >
>> >For platform device is one mechanism how to enumerate device, and
>> >bind it to the driver. The OF ID table needs to be present in case
>> >you are using it for direct DT enumeration (there is also something
>> >related to MFD child nodes, but you need to check and explain how your
>device is enumerated by this driver).
>> >
>> >I.o.w. please clarify how the OF ID table is being used.
>>
>> I do not use "of id table" directly in max77541-regulator.c so do I need to
>exclude it?
>
>Exactly my point. How does this OF ID table affect the device enumeration?
>
Since this is sub-device, it seems OF ID table does not affect the device enumarion, but as I stated before, I thought I need to add OF ID table because regulator's parameters are initialized via DT with the help of devm_regulator_register(..). It scans all the nodes under regulators node.
>> However, devm_regulator_register(..) method initialize each regulator
>> with the nodes under "regulators node". If of_match in desc and name
>> of node matches, then regulator will be initialized with parameters in
>> the node under the regulators node in the device tree. Since I am
>> using device tree to initialize regulators, I added of id table. I
>> hope I explained the situation clearly.
>
>This is confusing. If your regulator is enumerated via DT, why do you need MFD?
MAX77541 has also adc that is why I added MAX77541 as mfd device
>
>--
>With Best Regards,
>Andy Shevchenko
>
Hi Andy,
Thank you for your support and your time.
Regards,
Okan Sahin