MFD, regulator and ADC driver and related bindings for MAX77540/MAX77541.
The patches are required to be applied in sequence.
Changes in v6:
* Patch 1: "dt-bindings: regulator: max77541: Add ADI MAX77541/MAX77540 Regulator"
* NO CHANGE
* Patch 2: "regulator: max77541: Add ADI MAX77541/MAX77540 Regulator Support"
* Drop unnecessary headers
* Patch 3: "iio: adc: : max77541 Add ADI MAX77541 ADC Support"
* Drop unnecessary headers
* Patch 4: "dt-bindings: mfd: max77541: adi,max77541.yaml Add MAX77541 bindings"
* NO CHANGE
* Patch 5: "mfd: max77541: Add MAX77541/MAX77540 PMIC Support"
* Add more explanation to Kconfig
* Drop unnecessary headers
* Change differentiate method for different IC's
* Modify order of registers in header file
Changes in v5:
* Patch 1: "dt-bindings: regulator: max77541: Add ADI MAX77541/MAX77540 Regulator"
* Drop compatible properties.
* Patch 2: "regulator: max77541: Add ADI MAX77541/MAX77540 Regulator Support"
* Change if-else ladder to switch case for chip->id
* Drop driver_data in platform_device_id
* Patch 3: "iio: adc: : max77541 Add ADI MAX77541 ADC Support"
* Drop max77541_adc_iio struct
* Patch 4: "dt-bindings: mfd: max77541: adi,max77541.yaml Add MAX77541 bindings"
* Drop allOf
* Patch 5: "mfd: max77541: Add MAX77541/MAX77540 PMIC Support"
* Dont use compatible when using MFD_CELL_OF MACRO
Changes in v4:
* Patch 1: "dt-bindings: regulator: Add ADI MAX77541/MAX77540 Regulator"
* NO CHANGE
* Patch 2: "drivers: regulator: Add ADI MAX77541/MAX77540 Regulator Support"
* Drop OF ID Table
* Drop driver_data in platform_device_id
* Patch 3: "drivers: iio: adc: Add ADI MAX77541 ADC Support"
* Add missing blank line
* Patch 4: "dt-bindings: mfd: adi,max77541.yaml Add MAX77541 bindings"
* NO CHANGE(Order of patchset changed, and [4/5] has dependency to [1/5])
* Patch 5: "drivers: mfd: Add MAX77541/MAX77540 PMIC Support"
* Use pointers in the driver_data
* Use probe_new instead of probe
* Use PLATFORM_DEVID_NONE macro instead of "-1"
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):
dt-bindings: regulator: max77541: Add ADI MAX77541/MAX77540 Regulator
regulator: max77541: Add ADI MAX77541/MAX77540 Regulator Support
iio: adc: max77541: Add ADI MAX77541 ADC Support
dt-bindings: mfd: max77541: Add ADI MAX77541/MAX77540
mfd: max77541: Add ADI MAX77541/MAX77540 PMIC Support
.../devicetree/bindings/mfd/adi,max77541.yaml | 68 ++++++
.../regulator/adi,max77541-regulator.yaml | 38 +++
drivers/iio/adc/Kconfig | 11 +
drivers/iio/adc/Makefile | 1 +
drivers/iio/adc/max77541-adc.c | 194 +++++++++++++++
drivers/mfd/Kconfig | 13 +
drivers/mfd/Makefile | 1 +
drivers/mfd/max77541.c | 224 ++++++++++++++++++
drivers/regulator/Kconfig | 9 +
drivers/regulator/Makefile | 1 +
drivers/regulator/max77541-regulator.c | 153 ++++++++++++
include/linux/mfd/max77541.h | 97 ++++++++
12 files changed, 810 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
Add ADI MAX77541/MAX77540 Regulator devicetree document.
Signed-off-by: Okan Sahin <[email protected]>
Reviewed-by: Krzysztof Kozlowski <[email protected]>
---
.../regulator/adi,max77541-regulator.yaml | 38 +++++++++++++++++++
1 file changed, 38 insertions(+)
create mode 100644 Documentation/devicetree/bindings/regulator/adi,max77541-regulator.yaml
diff --git a/Documentation/devicetree/bindings/regulator/adi,max77541-regulator.yaml b/Documentation/devicetree/bindings/regulator/adi,max77541-regulator.yaml
new file mode 100644
index 000000000000..9e36d5467b56
--- /dev/null
+++ b/Documentation/devicetree/bindings/regulator/adi,max77541-regulator.yaml
@@ -0,0 +1,38 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/regulator/adi,max77541-regulator.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Buck Converter for MAX77540/MAX77541
+
+maintainers:
+ - Okan Sahin <[email protected]>
+
+description: |
+ This is a part of device tree bindings for ADI MAX77540/MAX77541
+
+ The buck converter is represented as a sub-node of the PMIC node on the device tree.
+
+ The device has two buck regulators.
+ See also Documentation/devicetree/bindings/mfd/adi,max77541.yaml for
+ additional information and example.
+
+patternProperties:
+ "^buck[12]$":
+ type: object
+ $ref: regulator.yaml#
+ additionalProperties: false
+ description: |
+ Buck regulator.
+
+ properties:
+ regulator-name: true
+ regulator-always-on: true
+ regulator-boot-on: true
+ regulator-min-microvolt:
+ minimum: 300000
+ regulator-max-microvolt:
+ maximum: 5200000
+
+additionalProperties: false
--
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]>
---
drivers/regulator/Kconfig | 9 ++
drivers/regulator/Makefile | 1 +
drivers/regulator/max77541-regulator.c | 153 +++++++++++++++++++++++++
3 files changed, 163 insertions(+)
create mode 100644 drivers/regulator/max77541-regulator.c
diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index aae28d0a489c..f0418274c083 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 ee383d8fc835..1c852f3140a3 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..f99caf3f3990
--- /dev/null
+++ b/drivers/regulator/max77541-regulator.c
@@ -0,0 +1,153 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2022 Analog Devices, Inc.
+ * ADI Regulator driver for the MAX77540 and MAX77541
+ */
+
+#include <linux/mfd/max77541.h>
+#include <linux/mod_devicetable.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/regulator/driver.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,
+};
+
+enum max77541_regulators {
+ MAX77541_BUCK1 = 1,
+ MAX77541_BUCK2,
+};
+
+#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 regulator_config config = {};
+ const struct regulator_desc *desc;
+ struct device *dev = &pdev->dev;
+ struct regulator_dev *rdev;
+ struct max77541 *max77541 = dev_get_drvdata(dev->parent);
+ unsigned int i;
+
+ config.dev = dev->parent;
+
+ switch (max77541->chip->id) {
+ case MAX77540:
+ desc = max77540_regulators_desc;
+ break;
+ case MAX77541:
+ desc = max77541_regulators_desc;
+ break;
+ default:
+ 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" },
+ { "max77541-regulator" },
+ { }
+};
+MODULE_DEVICE_TABLE(platform, max77541_regulator_platform_id);
+
+static struct platform_driver max77541_regulator_driver = {
+ .driver = {
+ .name = "max77541-regulator",
+ },
+ .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
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]>
Reviewed-by: Jonathan Cameron <[email protected]>
---
drivers/iio/adc/Kconfig | 11 ++
drivers/iio/adc/Makefile | 1 +
drivers/iio/adc/max77541-adc.c | 194 +++++++++++++++++++++++++++++++++
3 files changed, 206 insertions(+)
create mode 100644 drivers/iio/adc/max77541-adc.c
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 45af2302be53..518e7bd453aa 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -735,6 +735,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 36c18177322a..f8433b560c3b 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -67,6 +67,7 @@ obj-$(CONFIG_MAX11205) += max11205.o
obj-$(CONFIG_MAX11410) += max11410.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..21d024bde16b
--- /dev/null
+++ b/drivers/iio/adc/max77541-adc.c
@@ -0,0 +1,194 @@
+// 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 <linux/mod_devicetable.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/units.h>
+
+#include <linux/mfd/max77541.h>
+
+enum max77541_adc_range {
+ LOW_RANGE,
+ MID_RANGE,
+ HIGH_RANGE,
+};
+
+enum max77541_adc_channel {
+ MAX77541_ADC_VSYS_V,
+ 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_TEMP:
+ *val = DIV_ROUND_CLOSEST(ABSOLUTE_ZERO_MILLICELSIUS, 1725);
+ return IIO_VAL_INT;
+ default:
+ return -EINVAL;
+ }
+}
+
+static int max77541_adc_scale(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ int *val, int *val2)
+{
+ struct regmap **regmap = iio_priv(indio_dev);
+ unsigned int reg_val;
+ int ret;
+
+ switch (chan->channel) {
+ case MAX77541_ADC_VSYS_V:
+ *val = 25;
+ return IIO_VAL_INT;
+ case MAX77541_ADC_VOUT1_V:
+ case MAX77541_ADC_VOUT2_V:
+ ret = regmap_read(*regmap, MAX77541_REG_M2_CFG1, ®_val);
+ if (ret)
+ return ret;
+
+ reg_val = FIELD_GET(MAX77541_BITS_MX_CFG1_RNG, reg_val);
+ switch (reg_val) {
+ case LOW_RANGE:
+ *val = 6;
+ *val2 = 250000;
+ break;
+ case MID_RANGE:
+ *val = 12;
+ *val2 = 500000;
+ break;
+ case HIGH_RANGE:
+ *val = 25;
+ return IIO_VAL_INT;
+ default:
+ return -EINVAL;
+ }
+
+ return IIO_VAL_INT_PLUS_MICRO;
+ case MAX77541_ADC_TEMP:
+ *val = 1725;
+ return IIO_VAL_INT;
+ default:
+ return -EINVAL;
+ }
+}
+
+static int max77541_adc_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ int *val)
+{
+ struct regmap **regmap = iio_priv(indio_dev);
+ int ret;
+
+ ret = regmap_read(*regmap, chan->address, val);
+ if (ret)
+ return ret;
+
+ return IIO_VAL_INT;
+}
+
+#define MAX77541_ADC_CHANNEL_V(_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), \
+ .datasheet_name = _name, \
+ }
+
+#define MAX77541_ADC_CHANNEL_TEMP(_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, \
+ }
+
+static const struct iio_chan_spec max77541_adc_channels[] = {
+ MAX77541_ADC_CHANNEL_V(MAX77541_ADC_VSYS_V, "vsys_v", IIO_VOLTAGE,
+ MAX77541_REG_ADC_DATA_CH1),
+ MAX77541_ADC_CHANNEL_V(MAX77541_ADC_VOUT1_V, "vout1_v", IIO_VOLTAGE,
+ MAX77541_REG_ADC_DATA_CH2),
+ MAX77541_ADC_CHANNEL_V(MAX77541_ADC_VOUT2_V, "vout2_v", IIO_VOLTAGE,
+ MAX77541_REG_ADC_DATA_CH3),
+ MAX77541_ADC_CHANNEL_TEMP(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 device *dev = &pdev->dev;
+ struct iio_dev *indio_dev;
+ struct regmap **regmap;
+
+ indio_dev = devm_iio_device_alloc(dev, sizeof(*regmap));
+ if (!indio_dev)
+ return -ENOMEM;
+
+ regmap = iio_priv(indio_dev);
+
+ *regmap = dev_get_regmap(dev->parent, NULL);
+ indio_dev->modes = INDIO_DIRECT_MODE;
+
+ indio_dev->name = "max77541";
+ 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(dev, indio_dev);
+}
+
+static const struct platform_device_id max77541_adc_platform_id[] = {
+ { "max77541-adc" },
+ { /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(platform, max77541_adc_platform_id);
+
+static struct platform_driver max77541_adc_driver = {
+ .driver = {
+ .name = "max77541-adc",
+ },
+ .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
Add ADI MAX77541/MAX77540 devicetree document.
Signed-off-by: Okan Sahin <[email protected]>
Reviewed-by: Krzysztof Kozlowski <[email protected]>
---
.../devicetree/bindings/mfd/adi,max77541.yaml | 68 +++++++++++++++++++
1 file changed, 68 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..c7895b2c38c9
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/adi,max77541.yaml
@@ -0,0 +1,68 @@
+# 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-regulator.yaml#
+
+required:
+ - compatible
+ - reg
+ - interrupts
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/interrupt-controller/irq.h>
+
+ i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ pmic@69 {
+ compatible = "adi,max77541";
+ 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;
+ };
+ };
+ };
+ };
--
2.30.2
On Tue, Mar 07, 2023 at 02:28:12PM +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.
Reviewed-by: Andy Shevchenko <[email protected]>
But see below.
> Signed-off-by: Okan Sahin <[email protected]>
> ---
> drivers/regulator/Kconfig | 9 ++
> drivers/regulator/Makefile | 1 +
> drivers/regulator/max77541-regulator.c | 153 +++++++++++++++++++++++++
> 3 files changed, 163 insertions(+)
> create mode 100644 drivers/regulator/max77541-regulator.c
>
> diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
> index aae28d0a489c..f0418274c083 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.
Maybe adding what would be the module name if M is chosen?
> 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 ee383d8fc835..1c852f3140a3 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..f99caf3f3990
> --- /dev/null
> +++ b/drivers/regulator/max77541-regulator.c
> @@ -0,0 +1,153 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) 2022 Analog Devices, Inc.
> + * ADI Regulator driver for the MAX77540 and MAX77541
> + */
I believe Mark asked to have this C++ comment style as well.
// Copyright (c) 2022 Analog Devices, Inc.
// ADI Regulator driver for the MAX77540 and MAX77541
> +#include <linux/mfd/max77541.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/driver.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,
> +};
> +
> +enum max77541_regulators {
> + MAX77541_BUCK1 = 1,
> + MAX77541_BUCK2,
> +};
> +
> +#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 regulator_config config = {};
> + const struct regulator_desc *desc;
> + struct device *dev = &pdev->dev;
> + struct regulator_dev *rdev;
> + struct max77541 *max77541 = dev_get_drvdata(dev->parent);
> + unsigned int i;
> +
> + config.dev = dev->parent;
> +
> + switch (max77541->chip->id) {
> + case MAX77540:
> + desc = max77540_regulators_desc;
> + break;
> + case MAX77541:
> + desc = max77541_regulators_desc;
> + break;
> + default:
> + 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" },
> + { "max77541-regulator" },
> + { }
> +};
> +MODULE_DEVICE_TABLE(platform, max77541_regulator_platform_id);
> +
> +static struct platform_driver max77541_regulator_driver = {
> + .driver = {
> + .name = "max77541-regulator",
> + },
> + .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
>
--
With Best Regards,
Andy Shevchenko
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]>
---
drivers/mfd/Kconfig | 13 ++
drivers/mfd/Makefile | 1 +
drivers/mfd/max77541.c | 224 +++++++++++++++++++++++++++++++++++
include/linux/mfd/max77541.h | 97 +++++++++++++++
4 files changed, 335 insertions(+)
create mode 100644 drivers/mfd/max77541.c
create mode 100644 include/linux/mfd/max77541.h
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index fcc141e067b9..de89245ce1cb 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -777,6 +777,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.
+ There are regulators and adc.
+
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 2f6c89d1e277..1c8540ddead6 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -151,6 +151,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..18a08d64fb6f
--- /dev/null
+++ b/drivers/mfd/max77541.c
@@ -0,0 +1,224 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2022 Analog Devices, Inc.
+ * 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/property.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[] = {
+ { .mask = MAX77541_BIT_INT_SRC_TOPSYS },
+ { .mask = 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_M,
+ .num_regs = 1,
+ .irqs = max77541_src_irqs,
+ .num_irqs = ARRAY_SIZE(max77541_src_irqs),
+};
+
+static const struct regmap_irq max77541_topsys_irqs[] = {
+ { .mask = MAX77541_BIT_TOPSYS_INT_TJ_120C },
+ { .mask = MAX77541_BIT_TOPSYS_INT_TJ_140C },
+ { .mask = MAX77541_BIT_TOPSYS_INT_TSHDN },
+ { .mask = MAX77541_BIT_TOPSYS_INT_UVLO },
+ { .mask = MAX77541_BIT_TOPSYS_INT_ALT_SWO },
+ { .mask = 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[] = {
+ { .mask = MAX77541_BIT_BUCK_INT_M1_POK_FLT },
+ { .mask = MAX77541_BIT_BUCK_INT_M2_POK_FLT },
+ { .mask = MAX77541_BIT_BUCK_INT_M1_SCFLT },
+ { .mask = 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[] = {
+ { .mask = MAX77541_BIT_ADC_INT_CH1_I },
+ { .mask = MAX77541_BIT_ADC_INT_CH2_I },
+ { .mask = MAX77541_BIT_ADC_INT_CH3_I },
+ { .mask = 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_INT_M,
+ .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, NULL),
+};
+
+static const struct mfd_cell max77541_devs[] = {
+ MFD_CELL_OF("max77541-regulator", NULL, NULL, 0, 0, NULL),
+ MFD_CELL_OF("max77541-adc", NULL, NULL, 0, 0, NULL),
+};
+
+static const struct chip_info chip[] = {
+ [MAX77540] = {
+ .id = MAX77540,
+ .n_devs = ARRAY_SIZE(max77540_devs),
+ .devs = max77540_devs,
+ },
+ [MAX77541] = {
+ .id = 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->chip->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 0;
+}
+
+static int max77541_pmic_setup(struct device *dev)
+{
+ struct max77541 *max77541 = dev_get_drvdata(dev);
+ int ret;
+
+ ret = max77541_pmic_irq_init(dev);
+ if (ret)
+ return dev_err_probe(dev, ret, "Failed to initialize IRQ\n");
+
+ ret = device_init_wakeup(dev, true);
+ if (ret)
+ return dev_err_probe(dev, ret, "Unable to init wakeup\n");
+
+ return devm_mfd_add_devices(dev, PLATFORM_DEVID_NONE,
+ max77541->chip->devs,
+ max77541->chip->n_devs,
+ NULL, 0, NULL);
+}
+
+static int max77541_probe(struct i2c_client *client)
+{
+ const struct i2c_device_id *id = i2c_client_get_device_id(client);
+ struct device *dev = &client->dev;
+ struct max77541 *max77541;
+
+ max77541 = devm_kzalloc(dev, sizeof(*max77541), GFP_KERNEL);
+ if (!max77541)
+ return -ENOMEM;
+
+ i2c_set_clientdata(client, max77541);
+ max77541->i2c = client;
+
+ max77541->chip = device_get_match_data(dev);
+ if (!max77541->chip)
+ max77541->chip = (struct chip_info *)id->driver_data;
+
+ if (!max77541->chip)
+ return -EINVAL;
+
+ 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 = &chip[MAX77540],
+ },
+ {
+ .compatible = "adi,max77541",
+ .data = &chip[MAX77541],
+ },
+ { /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, max77541_of_id);
+
+static const struct i2c_device_id max77541_id[] = {
+ { "max77540", (kernel_ulong_t)&chip[MAX77540] },
+ { "max77541", (kernel_ulong_t)&chip[MAX77541] },
+ { }
+};
+MODULE_DEVICE_TABLE(i2c, max77541_id);
+
+static struct i2c_driver max77541_driver = {
+ .driver = {
+ .name = "max77541",
+ .of_match_table = max77541_of_id,
+ },
+ .probe_new = max77541_probe,
+ .id_table = max77541_id,
+};
+module_i2c_driver(max77541_driver);
+
+MODULE_DESCRIPTION("MAX7740/MAX7741 Driver");
+MODULE_AUTHOR("Okan Sahin <[email protected]>");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/mfd/max77541.h b/include/linux/mfd/max77541.h
new file mode 100644
index 000000000000..f3b489207a7f
--- /dev/null
+++ b/include/linux/mfd/max77541.h
@@ -0,0 +1,97 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+
+#ifndef __MFD_MAX77541_H
+#define __MFD_MAX77541_H
+
+#include <linux/bits.h>
+#include <linux/types.h>
+
+/* REGISTERS */
+#define MAX77541_REG_INT_SRC 0x00
+#define MAX77541_REG_INT_SRC_M 0x01
+
+#define MAX77541_BIT_INT_SRC_TOPSYS BIT(0)
+#define MAX77541_BIT_INT_SRC_BUCK BIT(1)
+
+#define MAX77541_REG_TOPSYS_INT 0x02
+#define MAX77541_REG_TOPSYS_INT_M 0x03
+
+#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)
+
+/* REGULATORS */
+#define MAX77541_REG_BUCK_INT 0x20
+#define MAX77541_REG_BUCK_INT_M 0x21
+
+#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_REG_EN_CTRL 0x0B
+
+#define MAX77541_BIT_M1_EN BIT(0)
+#define MAX77541_BIT_M2_EN BIT(1)
+
+#define MAX77541_REG_M1_VOUT 0x23
+#define MAX77541_REG_M2_VOUT 0x33
+
+#define MAX77541_BITS_MX_VOUT GENMASK(7, 0)
+
+#define MAX77541_REG_M1_CFG1 0x25
+#define MAX77541_REG_M2_CFG1 0x35
+
+#define MAX77541_BITS_MX_CFG1_RNG GENMASK(7, 6)
+
+/* ADC */
+#define MAX77541_REG_ADC_INT 0x70
+#define MAX77541_REG_ADC_INT_M 0x71
+
+#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_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
+
+/* INTERRUPT MASKS*/
+#define MAX77541_REG_INT_SRC_MASK 0x00
+#define MAX77541_REG_TOPSYS_INT_MASK 0x00
+#define MAX77541_REG_BUCK_INT_MASK 0x00
+
+#define MAX77541_MAX_REGULATORS 2
+
+enum max7754x_ids {
+ MAX77540,
+ MAX77541,
+};
+
+struct chip_info {
+ enum max7754x_ids id;
+ int n_devs;
+ const struct mfd_cell *devs;
+};
+
+struct regmap;
+struct regmap_irq_chip_data;
+struct i2c_client;
+
+struct max77541 {
+ struct i2c_client *i2c;
+ struct regmap *regmap;
+ 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;
+};
+
+#endif /* __MFD_MAX77541_H */
--
2.30.2
On Tue, 07 Mar 2023, 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.
>
> Signed-off-by: Okan Sahin <[email protected]>
> ---
> drivers/mfd/Kconfig | 13 ++
> drivers/mfd/Makefile | 1 +
> drivers/mfd/max77541.c | 224 +++++++++++++++++++++++++++++++++++
> include/linux/mfd/max77541.h | 97 +++++++++++++++
> 4 files changed, 335 insertions(+)
> create mode 100644 drivers/mfd/max77541.c
> create mode 100644 include/linux/mfd/max77541.h
FYI: I'm not re-reviewing this since you've chosen to ignore some of my
previous review comments. Issues highlighted by review comments don't
just go away on resubmission.
--
Lee Jones [李琼斯]
On Wed, 15 Mar 2023, Lee Jones wrote:
> On Tue, 07 Mar 2023, 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.
> >
> > Signed-off-by: Okan Sahin <[email protected]>
> > ---
> > drivers/mfd/Kconfig | 13 ++
> > drivers/mfd/Makefile | 1 +
> > drivers/mfd/max77541.c | 224 +++++++++++++++++++++++++++++++++++
> > include/linux/mfd/max77541.h | 97 +++++++++++++++
> > 4 files changed, 335 insertions(+)
> > create mode 100644 drivers/mfd/max77541.c
> > create mode 100644 include/linux/mfd/max77541.h
>
> FYI: I'm not re-reviewing this since you've chosen to ignore some of my
> previous review comments. Issues highlighted by review comments don't
> just go away on resubmission.
... and the subject is malformed.
--
Lee Jones [李琼斯]
>On Wed, 15 Mar 2023, Lee Jones wrote:
>
>> On Tue, 07 Mar 2023, 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.
>> >
>> > Signed-off-by: Okan Sahin <[email protected]>
>> > ---
>> > drivers/mfd/Kconfig | 13 ++
>> > drivers/mfd/Makefile | 1 +
>> > drivers/mfd/max77541.c | 224
>+++++++++++++++++++++++++++++++++++
>> > include/linux/mfd/max77541.h | 97 +++++++++++++++
>> > 4 files changed, 335 insertions(+)
>> > create mode 100644 drivers/mfd/max77541.c create mode 100644
>> > include/linux/mfd/max77541.h
>>
>> FYI: I'm not re-reviewing this since you've chosen to ignore some of
>> my previous review comments. Issues highlighted by review comments
>> don't just go away on resubmission.
>
>... and the subject is malformed.
>
>--
>Lee Jones [李琼斯]
Hi Lee,
I am sorry if I missed your review comments, this was not my intention. I want to thank you for your contribution. Your feedbacks are very valuable, and I am trying to understand and fix each one before sending the patch. Indeed, I sorted your feedback on previous patches. As far as I know, I have fixed all of them, is there a problem with any of them that I fixed, or is there any missing review? From you, there were some comments like "why did you use this?", I suppose I need to respond them before sending following patches. I thought I should not bother the maintainers unnecessarily. I am sorry for them.
For previous patch(v5), There was feedback from Andy. I did not fix them.
1)
> They have same regmap except for ADC part of MAX77541.
Extra space in the Subject.
...
> +#include <linux/of_device.h
This is my fault, I missed this comment. I will fix in following patch(v7).
2)
...
> +static const struct regmap_config max77541_regmap_config = {
> + .reg_bits = 8,
> + .val_bits = 8,
Do you need lock of regmap?
> +};
...
Since I do not need lock of regmap, I did not change anything in regmap_config (v6). Do I need to answer this question even if I don't need lock of regmap?
For the other reviews, I fixed them as you said. Thank you for your time, and effort. Sorry for the misunderstanding and confusion.
Regards,
Okan
On Tue, Mar 28, 2023 at 08:26:41AM +0000, Sahin, Okan wrote:
> >On Wed, 15 Mar 2023, Lee Jones wrote:
> >> On Tue, 07 Mar 2023, Okan Sahin wrote:
...
> For previous patch(v5), There was feedback from Andy. I did not fix them.
Why not? :-)
> 1)
> > They have same regmap except for ADC part of MAX77541.
>
> Extra space in the Subject.
>
> ...
>
> > +#include <linux/of_device.h
>
> This is my fault, I missed this comment. I will fix in following patch(v7).
>
> 2)
> ...
>
> > +static const struct regmap_config max77541_regmap_config = {
> > + .reg_bits = 8,
> > + .val_bits = 8,
>
> Do you need lock of regmap?
>
> > +};
>
> ...
>
> Since I do not need lock of regmap, I did not change anything in
> regmap_config (v6). Do I need to answer this question even if I don't need
> lock of regmap?
IIRC the lock is opt-out. You need to explicitly disable it if not needed.
--
With Best Regards,
Andy Shevchenko
On Tue, 2023-03-28 at 15:51 +0300, Andy Shevchenko wrote:
> On Tue, Mar 28, 2023 at 08:26:41AM +0000, Sahin, Okan wrote:
> > > On Wed, 15 Mar 2023, Lee Jones wrote:
> > > > On Tue, 07 Mar 2023, Okan Sahin wrote:
>
> ...
>
> > For previous patch(v5), There was feedback from Andy. I did not fix
> > them.
>
> Why not? :-)
>
> > 1)
> > > They have same regmap except for ADC part of MAX77541.
> >
> > Extra space in the Subject.
> >
> > ...
> >
> > > +#include <linux/of_device.h
> >
> > This is my fault, I missed this comment. I will fix in following
> > patch(v7).
> >
> > 2)
> > ...
> >
> > > +static const struct regmap_config max77541_regmap_config = {
> > > + .reg_bits = 8,
> > > + .val_bits = 8,
> >
> > Do you need lock of regmap?
> >
> > > +};
> >
> > ...
> >
> > Since I do not need lock of regmap, I did not change anything in
> > regmap_config (v6). Do I need to answer this question even if I
> > don't need
> > lock of regmap?
>
> IIRC the lock is opt-out. You need to explicitly disable it if not
> needed.
>
IIRC, regmap_read() is not really reentrant and it is used in the IIO
driver on the sysfs interface. So, yeah, I think you need the regmap
lock and better just leave the config as is. Yes, the lock is opt-out
so let's not disable it :)
- Nuno Sá
On Tue, Mar 28, 2023 at 03:26:44PM +0200, Nuno S? wrote:
> IIRC, regmap_read() is not really reentrant and it is used in the IIO
> driver on the sysfs interface. So, yeah, I think you need the regmap
> lock and better just leave the config as is. Yes, the lock is opt-out
> so let's not disable it :)
All the regmap operations are fully thread safe.
On Tue, 2023-03-28 at 14:46 +0100, Mark Brown wrote:
> On Tue, Mar 28, 2023 at 03:26:44PM +0200, Nuno Sá wrote:
>
> > IIRC, regmap_read() is not really reentrant and it is used in the
> > IIO
> > driver on the sysfs interface. So, yeah, I think you need the
> > regmap
> > lock and better just leave the config as is. Yes, the lock is opt-
> > out
> > so let's not disable it :)
>
> All the regmap operations are fully thread safe.
Even if 'config->disable_locking' is set? I think that is what's being
discussed in here...
- Nuno Sá
On Tue, Mar 28, 2023 at 04:18:30PM +0200, Nuno S? wrote:
> On Tue, 2023-03-28 at 14:46 +0100, Mark Brown wrote:
> > On Tue, Mar 28, 2023 at 03:26:44PM +0200, Nuno S? wrote:
> >
> > > IIRC, regmap_read() is not really reentrant and it is used in the
> > > IIO
> > > driver on the sysfs interface. So, yeah, I think you need the
> > > regmap
> > > lock and better just leave the config as is. Yes, the lock is opt-
> > > out
> > > so let's not disable it :)
> >
> > All the regmap operations are fully thread safe.
>
> Even if 'config->disable_locking' is set? I think that is what's being
> discussed in here...
In case the driver has its own lock to serialize IO how on earth the regmap
lock is needed. That's what I asked the author of the driver. He told the code
doesn't require the regmap lock, and I tend to believe the author. So, why to
keep it?
--
With Best Regards,
Andy Shevchenko
On 3/28/23 07:35, Andy Shevchenko wrote:
> On Tue, Mar 28, 2023 at 04:18:30PM +0200, Nuno Sá wrote:
>> On Tue, 2023-03-28 at 14:46 +0100, Mark Brown wrote:
>>> On Tue, Mar 28, 2023 at 03:26:44PM +0200, Nuno Sá wrote:
>>>
>>>> IIRC, regmap_read() is not really reentrant and it is used in the
>>>> IIO
>>>> driver on the sysfs interface. So, yeah, I think you need the
>>>> regmap
>>>> lock and better just leave the config as is. Yes, the lock is opt-
>>>> out
>>>> so let's not disable it :)
>>> All the regmap operations are fully thread safe.
>> Even if 'config->disable_locking' is set? I think that is what's being
>> discussed in here...
> In case the driver has its own lock to serialize IO how on earth the regmap
> lock is needed.
But the driver does not have its own lock.
On Tue, 2023-03-28 at 17:35 +0300, Andy Shevchenko wrote:
> On Tue, Mar 28, 2023 at 04:18:30PM +0200, Nuno Sá wrote:
> > On Tue, 2023-03-28 at 14:46 +0100, Mark Brown wrote:
> > > On Tue, Mar 28, 2023 at 03:26:44PM +0200, Nuno Sá wrote:
> > >
> > > > IIRC, regmap_read() is not really reentrant and it is used in
> > > > the
> > > > IIO
> > > > driver on the sysfs interface. So, yeah, I think you need the
> > > > regmap
> > > > lock and better just leave the config as is. Yes, the lock is
> > > > opt-
> > > > out
> > > > so let's not disable it :)
> > >
> > > All the regmap operations are fully thread safe.
> >
> > Even if 'config->disable_locking' is set? I think that is what's
> > being
> > discussed in here...
>
> In case the driver has its own lock to serialize IO how on earth the
> regmap
> lock is needed. That's what I asked the author of the driver. He told
> the code
Well, if the driver has it's own locking, then sure we do not need
regmap's lock...
> doesn't require the regmap lock, and I tend to believe the author.
> So, why to
> keep it?
>
However, if you look at the adc driver, I can see plain regmap_read()
calls without any "outside" locking.
- Nuno Sá
On Tue, Mar 28, 2023 at 04:18:30PM +0200, Nuno S? wrote:
> On Tue, 2023-03-28 at 14:46 +0100, Mark Brown wrote:
> > On Tue, Mar 28, 2023 at 03:26:44PM +0200, Nuno S? wrote:
> > > IIRC, regmap_read() is not really reentrant and it is used in the
> > > IIO
> > > driver on the sysfs interface. So, yeah, I think you need the
> > > regmap
> > > lock and better just leave the config as is. Yes, the lock is opt-
> > > out
> > > so let's not disable it :)
> > All the regmap operations are fully thread safe.
> Even if 'config->disable_locking' is set? I think that is what's being
> discussed in here...
In that case the caller has to ensure that the regmap is only used in a
thread safe fashion.
On Tue, Mar 28, 2023 at 04:51:18PM +0200, Nuno S? wrote:
> On Tue, 2023-03-28 at 17:35 +0300, Andy Shevchenko wrote:
> > On Tue, Mar 28, 2023 at 04:18:30PM +0200, Nuno S? wrote:
> > > On Tue, 2023-03-28 at 14:46 +0100, Mark Brown wrote:
> > > > On Tue, Mar 28, 2023 at 03:26:44PM +0200, Nuno S? wrote:
> > > >
> > > > > IIRC, regmap_read() is not really reentrant and it is used in
> > > > > the
> > > > > IIO
> > > > > driver on the sysfs interface. So, yeah, I think you need the
> > > > > regmap
> > > > > lock and better just leave the config as is. Yes, the lock is
> > > > > opt-
> > > > > out
> > > > > so let's not disable it :)
> > > >
> > > > All the regmap operations are fully thread safe.
> > >
> > > Even if 'config->disable_locking' is set? I think that is what's
> > > being
> > > discussed in here...
> >
> > In case the driver has its own lock to serialize IO how on earth the
> > regmap
> > lock is needed. That's what I asked the author of the driver. He told
> > the code
>
> Well, if the driver has it's own locking, then sure we do not need
> regmap's lock...
>
> > doesn't require the regmap lock, and I tend to believe the author.
> > So, why to
> > keep it?
>
> However, if you look at the adc driver, I can see plain regmap_read()
> calls without any "outside" locking.
Then author of the code should know what they are doing. Right?
--
With Best Regards,
Andy Shevchenko
>On Tue, Mar 28, 2023 at 04:51:18PM +0200, Nuno S? wrote:
>> On Tue, 2023-03-28 at 17:35 +0300, Andy Shevchenko wrote:
>> > On Tue, Mar 28, 2023 at 04:18:30PM +0200, Nuno S? wrote:
>> > > On Tue, 2023-03-28 at 14:46 +0100, Mark Brown wrote:
>> > > > On Tue, Mar 28, 2023 at 03:26:44PM +0200, Nuno S? wrote:
>> > > >
>> > > > > IIRC, regmap_read() is not really reentrant and it is used in
>> > > > > the IIO driver on the sysfs interface. So, yeah, I think you
>> > > > > need the regmap lock and better just leave the config as is.
>> > > > > Yes, the lock is
>> > > > > opt-
>> > > > > out
>> > > > > so let's not disable it :)
>> > > >
>> > > > All the regmap operations are fully thread safe.
>> > >
>> > > Even if 'config->disable_locking' is set? I think that is what's
>> > > being discussed in here...
>> >
>> > In case the driver has its own lock to serialize IO how on earth the
>> > regmap lock is needed. That's what I asked the author of the driver.
>> > He told the code
>>
>> Well, if the driver has it's own locking, then sure we do not need
>> regmap's lock...
>>
>> > doesn't require the regmap lock, and I tend to believe the author.
>> > So, why to
>> > keep it?
>>
>> However, if you look at the adc driver, I can see plain regmap_read()
>> calls without any "outside" locking.
>
>Then author of the code should know what they are doing. Right?
>
>--
>With Best Regards,
>Andy Shevchenko
>
Hi Andy,
Actually, I do not want to disable regmap lock that's why I did not update it.
Regards,
Okan
On Tue, 2023-03-28 at 18:47 +0300, Andy Shevchenko wrote:
> On Tue, Mar 28, 2023 at 04:51:18PM +0200, Nuno Sá wrote:
> > On Tue, 2023-03-28 at 17:35 +0300, Andy Shevchenko wrote:
> > > On Tue, Mar 28, 2023 at 04:18:30PM +0200, Nuno Sá wrote:
> > > > On Tue, 2023-03-28 at 14:46 +0100, Mark Brown wrote:
> > > > > On Tue, Mar 28, 2023 at 03:26:44PM +0200, Nuno Sá wrote:
> > > > >
> > > > > > IIRC, regmap_read() is not really reentrant and it is used
> > > > > > in
> > > > > > the
> > > > > > IIO
> > > > > > driver on the sysfs interface. So, yeah, I think you need
> > > > > > the
> > > > > > regmap
> > > > > > lock and better just leave the config as is. Yes, the lock
> > > > > > is
> > > > > > opt-
> > > > > > out
> > > > > > so let's not disable it :)
> > > > >
> > > > > All the regmap operations are fully thread safe.
> > > >
> > > > Even if 'config->disable_locking' is set? I think that is
> > > > what's
> > > > being
> > > > discussed in here...
> > >
> > > In case the driver has its own lock to serialize IO how on earth
> > > the
> > > regmap
> > > lock is needed. That's what I asked the author of the driver. He
> > > told
> > > the code
> >
> > Well, if the driver has it's own locking, then sure we do not need
> > regmap's lock...
> >
> > > doesn't require the regmap lock, and I tend to believe the
> > > author.
> > > So, why to
> > > keep it?
> >
> > However, if you look at the adc driver, I can see plain
> > regmap_read()
> > calls without any "outside" locking.
>
> Then author of the code should know what they are doing. Right?
>
In theory yes, but you know that's not always the case :)
- Nuno Sá
On Wed, Mar 29, 2023 at 09:01:30AM +0200, Nuno S? wrote:
> On Tue, 2023-03-28 at 18:47 +0300, Andy Shevchenko wrote:
> > On Tue, Mar 28, 2023 at 04:51:18PM +0200, Nuno S? wrote:
> > > On Tue, 2023-03-28 at 17:35 +0300, Andy Shevchenko wrote:
> > > > On Tue, Mar 28, 2023 at 04:18:30PM +0200, Nuno S? wrote:
> > > > > On Tue, 2023-03-28 at 14:46 +0100, Mark Brown wrote:
> > > > > > On Tue, Mar 28, 2023 at 03:26:44PM +0200, Nuno S? wrote:
> > > > > >
> > > > > > > IIRC, regmap_read() is not really reentrant and it is used
> > > > > > > in
> > > > > > > the
> > > > > > > IIO
> > > > > > > driver on the sysfs interface. So, yeah, I think you need
> > > > > > > the
> > > > > > > regmap
> > > > > > > lock and better just leave the config as is. Yes, the lock
> > > > > > > is
> > > > > > > opt-
> > > > > > > out
> > > > > > > so let's not disable it :)
> > > > > >
> > > > > > All the regmap operations are fully thread safe.
> > > > >
> > > > > Even if 'config->disable_locking' is set? I think that is
> > > > > what's
> > > > > being
> > > > > discussed in here...
> > > >
> > > > In case the driver has its own lock to serialize IO how on earth
> > > > the
> > > > regmap
> > > > lock is needed. That's what I asked the author of the driver. He
> > > > told
> > > > the code
> > >
> > > Well, if the driver has it's own locking, then sure we do not need
> > > regmap's lock...
> > >
> > > > doesn't require the regmap lock, and I tend to believe the
> > > > author.
> > > > So, why to
> > > > keep it?
> > >
> > > However, if you look at the adc driver, I can see plain
> > > regmap_read()
> > > calls without any "outside" locking.
> >
> > Then author of the code should know what they are doing. Right?
>
> In theory yes, but you know that's not always the case :)
Exactly. That's why I want to hear from the author of the code to make sure
they know _their_ code.
--
With Best Regards,
Andy Shevchenko
On Tue, Mar 28, 2023 at 04:01:21PM +0000, Sahin, Okan wrote:
> >On Tue, Mar 28, 2023 at 04:51:18PM +0200, Nuno S? wrote:
> >> On Tue, 2023-03-28 at 17:35 +0300, Andy Shevchenko wrote:
> >> > On Tue, Mar 28, 2023 at 04:18:30PM +0200, Nuno S? wrote:
> >> > > On Tue, 2023-03-28 at 14:46 +0100, Mark Brown wrote:
> >> > > > On Tue, Mar 28, 2023 at 03:26:44PM +0200, Nuno S? wrote:
> >> > > >
> >> > > > > IIRC, regmap_read() is not really reentrant and it is used in
> >> > > > > the IIO driver on the sysfs interface. So, yeah, I think you
> >> > > > > need the regmap lock and better just leave the config as is.
> >> > > > > Yes, the lock is
> >> > > > > opt-
> >> > > > > out
> >> > > > > so let's not disable it :)
> >> > > >
> >> > > > All the regmap operations are fully thread safe.
> >> > >
> >> > > Even if 'config->disable_locking' is set? I think that is what's
> >> > > being discussed in here...
> >> >
> >> > In case the driver has its own lock to serialize IO how on earth the
> >> > regmap lock is needed. That's what I asked the author of the driver.
> >> > He told the code
> >>
> >> Well, if the driver has it's own locking, then sure we do not need
> >> regmap's lock...
> >>
> >> > doesn't require the regmap lock, and I tend to believe the author.
> >> > So, why to
> >> > keep it?
> >>
> >> However, if you look at the adc driver, I can see plain regmap_read()
> >> calls without any "outside" locking.
> >
> >Then author of the code should know what they are doing. Right?
> Actually, I do not want to disable regmap lock that's why I did not update it.
If you have something like
func1()
regmap_read(reg1)
regmap_read/write(reg2)
func2()
regmap_read/write(regX) // X may or may not be 1 or 2
and func1() and func2() can be run in parallel then the code is racy.
Do you have such in your code?
--
With Best Regards,
Andy Shevchenko
On Wed, Mar 29, 2023 at 05:08:44PM +0300, Andy Shevchenko wrote:
> On Tue, Mar 28, 2023 at 04:01:21PM +0000, Sahin, Okan wrote:
> > >On Tue, Mar 28, 2023 at 04:51:18PM +0200, Nuno S? wrote:
> > >> On Tue, 2023-03-28 at 17:35 +0300, Andy Shevchenko wrote:
> > >> > On Tue, Mar 28, 2023 at 04:18:30PM +0200, Nuno S? wrote:
> > >> > > On Tue, 2023-03-28 at 14:46 +0100, Mark Brown wrote:
> > >> > > > On Tue, Mar 28, 2023 at 03:26:44PM +0200, Nuno S? wrote:
> > >> > > >
> > >> > > > > IIRC, regmap_read() is not really reentrant and it is used in
> > >> > > > > the IIO driver on the sysfs interface. So, yeah, I think you
> > >> > > > > need the regmap lock and better just leave the config as is.
> > >> > > > > Yes, the lock is
> > >> > > > > opt-
> > >> > > > > out
> > >> > > > > so let's not disable it :)
> > >> > > >
> > >> > > > All the regmap operations are fully thread safe.
> > >> > >
> > >> > > Even if 'config->disable_locking' is set? I think that is what's
> > >> > > being discussed in here...
> > >> >
> > >> > In case the driver has its own lock to serialize IO how on earth the
> > >> > regmap lock is needed. That's what I asked the author of the driver.
> > >> > He told the code
> > >>
> > >> Well, if the driver has it's own locking, then sure we do not need
> > >> regmap's lock...
> > >>
> > >> > doesn't require the regmap lock, and I tend to believe the author.
> > >> > So, why to
> > >> > keep it?
> > >>
> > >> However, if you look at the adc driver, I can see plain regmap_read()
> > >> calls without any "outside" locking.
> > >
> > >Then author of the code should know what they are doing. Right?
>
> > Actually, I do not want to disable regmap lock that's why I did not update it.
>
> If you have something like
>
> func1()
> regmap_read(reg1)
> regmap_read/write(reg2)
>
> func2()
> regmap_read/write(regX) // X may or may not be 1 or 2
>
> and func1() and func2() can be run in parallel then the code is racy.
I have to add that it's racy depending on the hardware of course.
In some cases it may be not a problem, in some it can. _Strictly_
speaking it's racy.
> Do you have such in your code?
Please, double check that. It's recommended to explain your locking schema
somewhere in the code top comment so anybody who reads it later and tries
to modify will know what to expect.
--
With Best Regards,
Andy Shevchenko
On Tue, 28 Mar 2023, Sahin, Okan wrote:
> >On Wed, 15 Mar 2023, Lee Jones wrote:
> >
> >> On Tue, 07 Mar 2023, 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.
> >> >
> >> > Signed-off-by: Okan Sahin <[email protected]>
> >> > ---
> >> > drivers/mfd/Kconfig | 13 ++
> >> > drivers/mfd/Makefile | 1 +
> >> > drivers/mfd/max77541.c | 224
> >+++++++++++++++++++++++++++++++++++
> >> > include/linux/mfd/max77541.h | 97 +++++++++++++++
> >> > 4 files changed, 335 insertions(+)
> >> > create mode 100644 drivers/mfd/max77541.c create mode 100644
> >> > include/linux/mfd/max77541.h
> >>
> >> FYI: I'm not re-reviewing this since you've chosen to ignore some of
> >> my previous review comments. Issues highlighted by review comments
> >> don't just go away on resubmission.
> >
> >... and the subject is malformed.
> >
> >--
> >Lee Jones [李琼斯]
>
> Hi Lee,
>
> I am sorry if I missed your review comments, this was not my intention. I want to thank you for your contribution. Your feedbacks are very valuable, and I am trying to understand and fix each one before sending the patch. Indeed, I sorted your feedback on previous patches. As far as I know, I have fixed all of them, is there a problem with any of them that I fixed, or is there any missing review? From you, there were some comments like "why did you use this?", I suppose I need to respond them before sending following patches. I thought I should not bother the maintainers unnecessarily. I am sorry for them.
Please ask your email client to line-wrap.
Here is the part of the review you ignored:
[...]
> +static const struct chip_info chip[] = {
Why do you need this require sub-structure?
> + [MAX77540] = {
> + .id = MAX77540,
> + .n_devs = ARRAY_SIZE(max77540_devs),
> + .devs = max77540_devs,
> + },
> + [MAX77541] = {
> + .id = MAX77541,
> + .n_devs = ARRAY_SIZE(max77541_devs),
> + .devs = max77541_devs,
> + },
> +};
[...]
> +static const struct of_device_id max77541_of_id[] = {
> + {
> + .compatible = "adi,max77540",
> + .data = &chip[MAX77540],
> + },
> + {
> + .compatible = "adi,max77541",
> + .data = &chip[MAX77541],
> + },
> + { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, max77541_of_id);
> +
> +static const struct i2c_device_id max77541_i2c_id[] = {
> + { "max77540", (kernel_ulong_t)&chip[MAX77540] },
> + { "max77541", (kernel_ulong_t)&chip[MAX77541] },
Just 'MAX77540' is fine.
> + { /* sentinel */ }
Remove the comment, we know how terminators work.
Same comments for max77541_of_id.
--
Lee Jones [李琼斯]
On Wed, Mar 29, 2023 at 03:36:15PM +0100, Lee Jones wrote:
> On Tue, 28 Mar 2023, Sahin, Okan wrote:
> > >On Wed, 15 Mar 2023, Lee Jones wrote:
> > >> On Tue, 07 Mar 2023, Okan Sahin wrote:
...
> > +static const struct i2c_device_id max77541_i2c_id[] = {
> > + { "max77540", (kernel_ulong_t)&chip[MAX77540] },
> > + { "max77541", (kernel_ulong_t)&chip[MAX77541] },
>
> Just 'MAX77540' is fine.
I tend to disagree.
There is an error prone approach esp. when we talk with some functions
that unifies OF/ACPI driver data retrieval with legacy ID tables.
In such a case the 0 from enum is hard to distinguish from NULL when
the driver data is not set or not found. On top of that the simple integer
in the legacy driver data will require additional code to be added in
the ->probe().
--
With Best Regards,
Andy Shevchenko
On Wed, 29 Mar 2023, Andy Shevchenko wrote:
> On Wed, Mar 29, 2023 at 03:36:15PM +0100, Lee Jones wrote:
> > On Tue, 28 Mar 2023, Sahin, Okan wrote:
> > > >On Wed, 15 Mar 2023, Lee Jones wrote:
> > > >> On Tue, 07 Mar 2023, Okan Sahin wrote:
>
> ...
>
> > > +static const struct i2c_device_id max77541_i2c_id[] = {
> > > + { "max77540", (kernel_ulong_t)&chip[MAX77540] },
> > > + { "max77541", (kernel_ulong_t)&chip[MAX77541] },
> >
> > Just 'MAX77540' is fine.
>
> I tend to disagree.
>
> There is an error prone approach esp. when we talk with some functions
> that unifies OF/ACPI driver data retrieval with legacy ID tables.
> In such a case the 0 from enum is hard to distinguish from NULL when
> the driver data is not set or not found. On top of that the simple integer
> in the legacy driver data will require additional code to be added in
> the ->probe().
Use a !0 enum?
The extra handling is expected and normal.
--
Lee Jones [李琼斯]
On Wed, 29 Mar 2023, Lee Jones wrote:
> On Wed, 29 Mar 2023, Andy Shevchenko wrote:
>
> > On Wed, Mar 29, 2023 at 03:36:15PM +0100, Lee Jones wrote:
> > > On Tue, 28 Mar 2023, Sahin, Okan wrote:
> > > > >On Wed, 15 Mar 2023, Lee Jones wrote:
> > > > >> On Tue, 07 Mar 2023, Okan Sahin wrote:
> >
> > ...
> >
> > > > +static const struct i2c_device_id max77541_i2c_id[] = {
> > > > + { "max77540", (kernel_ulong_t)&chip[MAX77540] },
> > > > + { "max77541", (kernel_ulong_t)&chip[MAX77541] },
> > >
> > > Just 'MAX77540' is fine.
> >
> > I tend to disagree.
> >
> > There is an error prone approach esp. when we talk with some functions
> > that unifies OF/ACPI driver data retrieval with legacy ID tables.
> > In such a case the 0 from enum is hard to distinguish from NULL when
> > the driver data is not set or not found. On top of that the simple integer
> > in the legacy driver data will require additional code to be added in
> > the ->probe().
>
> Use a !0 enum?
>
> The extra handling is expected and normal.
I've always disliked mixing platform initialisation strategies. Passing
pointers to MFD structs through I2C/Device Tree registration opens the
doors to all sorts of funky interlaced nonsense.
Pass the device ID and then match in C-code please.
--
Lee Jones [李琼斯]
On Wed, 2023-03-29 at 17:11 +0300, Andy Shevchenko wrote:
> On Wed, Mar 29, 2023 at 05:08:44PM +0300, Andy Shevchenko wrote:
> > On Tue, Mar 28, 2023 at 04:01:21PM +0000, Sahin, Okan wrote:
> > > > On Tue, Mar 28, 2023 at 04:51:18PM +0200, Nuno Sá wrote:
> > > > > On Tue, 2023-03-28 at 17:35 +0300, Andy Shevchenko wrote:
> > > > > > On Tue, Mar 28, 2023 at 04:18:30PM +0200, Nuno Sá wrote:
> > > > > > > On Tue, 2023-03-28 at 14:46 +0100, Mark Brown wrote:
> > > > > > > > On Tue, Mar 28, 2023 at 03:26:44PM +0200, Nuno Sá
> > > > > > > > wrote:
> > > > > > > >
> > > > > > > > > IIRC, regmap_read() is not really reentrant and it is
> > > > > > > > > used in
> > > > > > > > > the IIO driver on the sysfs interface. So, yeah, I
> > > > > > > > > think you
> > > > > > > > > need the regmap lock and better just leave the config
> > > > > > > > > as is.
> > > > > > > > > Yes, the lock is
> > > > > > > > > opt-
> > > > > > > > > out
> > > > > > > > > so let's not disable it :)
> > > > > > > >
> > > > > > > > All the regmap operations are fully thread safe.
> > > > > > >
> > > > > > > Even if 'config->disable_locking' is set? I think that is
> > > > > > > what's
> > > > > > > being discussed in here...
> > > > > >
> > > > > > In case the driver has its own lock to serialize IO how on
> > > > > > earth the
> > > > > > regmap lock is needed. That's what I asked the author of
> > > > > > the driver.
> > > > > > He told the code
> > > > >
> > > > > Well, if the driver has it's own locking, then sure we do not
> > > > > need
> > > > > regmap's lock...
> > > > >
> > > > > > doesn't require the regmap lock, and I tend to believe the
> > > > > > author.
> > > > > > So, why to
> > > > > > keep it?
> > > > >
> > > > > However, if you look at the adc driver, I can see plain
> > > > > regmap_read()
> > > > > calls without any "outside" locking.
> > > >
> > > > Then author of the code should know what they are doing. Right?
> >
> > > Actually, I do not want to disable regmap lock that's why I did
> > > not update it.
> >
> > If you have something like
> >
> > func1()
> > regmap_read(reg1)
> > regmap_read/write(reg2)
> >
I would also add that as soon as you have more than one access to
regmap (or spi, or i2c...) in one func and that function is accessible
through sysfs (and likely any other userspace interface), then you
should already take care. So, it might be even the case that func1()
and func2() don't have to run in parallel.
- Nuno Sá
>
On 29/03/2023 17:06, Lee Jones wrote:
> On Wed, 29 Mar 2023, Lee Jones wrote:
>
>> On Wed, 29 Mar 2023, Andy Shevchenko wrote:
>>
>>> On Wed, Mar 29, 2023 at 03:36:15PM +0100, Lee Jones wrote:
>>>> On Tue, 28 Mar 2023, Sahin, Okan wrote:
>>>>>> On Wed, 15 Mar 2023, Lee Jones wrote:
>>>>>>> On Tue, 07 Mar 2023, Okan Sahin wrote:
>>>
>>> ...
>>>
>>>>> +static const struct i2c_device_id max77541_i2c_id[] = {
>>>>> + { "max77540", (kernel_ulong_t)&chip[MAX77540] },
>>>>> + { "max77541", (kernel_ulong_t)&chip[MAX77541] },
>>>>
>>>> Just 'MAX77540' is fine.
>>>
>>> I tend to disagree.
>>>
>>> There is an error prone approach esp. when we talk with some functions
>>> that unifies OF/ACPI driver data retrieval with legacy ID tables.
>>> In such a case the 0 from enum is hard to distinguish from NULL when
>>> the driver data is not set or not found. On top of that the simple integer
>>> in the legacy driver data will require additional code to be added in
>>> the ->probe().
>>
>> Use a !0 enum?
>>
>> The extra handling is expected and normal.
>
> I've always disliked mixing platform initialisation strategies. Passing
> pointers to MFD structs through I2C/Device Tree registration opens the
> doors to all sorts of funky interlaced nonsense.
>
> Pass the device ID and then match in C-code please.
I agree. Especially that casting through ulong_t drops the const, so the
cast back needs const which can be forgotten. The patch already makes
here mistake!
Best regards,
Krzysztof
On 07/03/2023 12:28, Okan Sahin wrote:
> MFD driver for MAX77541/MAX77540 to enable its sub
> devices.
Please wrap commit message according to Linux coding style / submission
process (neither too early nor over the limit):
https://elixir.bootlin.com/linux/v5.18-rc4/source/Documentation/process/submitting-patches.rst#L586
>
> 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.
>
> +static int max77541_probe(struct i2c_client *client)
> +{
> + const struct i2c_device_id *id = i2c_client_get_device_id(client);
> + struct device *dev = &client->dev;
> + struct max77541 *max77541;
> +
> + max77541 = devm_kzalloc(dev, sizeof(*max77541), GFP_KERNEL);
> + if (!max77541)
> + return -ENOMEM;
> +
> + i2c_set_clientdata(client, max77541);
> + max77541->i2c = client;
> +
> + max77541->chip = device_get_match_data(dev);
> + if (!max77541->chip)
> + max77541->chip = (struct chip_info *)id->driver_data;
You have odd indentation/coding style before '='. Use Linux coding style.
Best regards,
Krzysztof
On 30/03/2023 10:04, Krzysztof Kozlowski wrote:
> On 29/03/2023 17:06, Lee Jones wrote:
>> On Wed, 29 Mar 2023, Lee Jones wrote:
>>
>>> On Wed, 29 Mar 2023, Andy Shevchenko wrote:
>>>
>>>> On Wed, Mar 29, 2023 at 03:36:15PM +0100, Lee Jones wrote:
>>>>> On Tue, 28 Mar 2023, Sahin, Okan wrote:
>>>>>>> On Wed, 15 Mar 2023, Lee Jones wrote:
>>>>>>>> On Tue, 07 Mar 2023, Okan Sahin wrote:
>>>>
>>>> ...
>>>>
>>>>>> +static const struct i2c_device_id max77541_i2c_id[] = {
>>>>>> + { "max77540", (kernel_ulong_t)&chip[MAX77540] },
>>>>>> + { "max77541", (kernel_ulong_t)&chip[MAX77541] },
>>>>>
>>>>> Just 'MAX77540' is fine.
>>>>
>>>> I tend to disagree.
>>>>
>>>> There is an error prone approach esp. when we talk with some functions
>>>> that unifies OF/ACPI driver data retrieval with legacy ID tables.
>>>> In such a case the 0 from enum is hard to distinguish from NULL when
>>>> the driver data is not set or not found. On top of that the simple integer
>>>> in the legacy driver data will require additional code to be added in
>>>> the ->probe().
>>>
>>> Use a !0 enum?
>>>
>>> The extra handling is expected and normal.
>>
>> I've always disliked mixing platform initialisation strategies. Passing
>> pointers to MFD structs through I2C/Device Tree registration opens the
>> doors to all sorts of funky interlaced nonsense.
>>
>> Pass the device ID and then match in C-code please.
>
> I agree. Especially that casting through ulong_t drops the const, so the
> cast back needs const which can be forgotten. The patch already makes
> here mistake!
Uh, no, the code is correct - chip_info member is const. Yet it is a
mistake easy to make for the device ID tables using void * or ulong.
Best regards,
Krzysztof
>On Tue, 28 Mar 2023, Sahin, Okan wrote:
>
>> >On Wed, 15 Mar 2023, Lee Jones wrote:
>> >
>> >> On Tue, 07 Mar 2023, 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.
>> >> >
>> >> > Signed-off-by: Okan Sahin <[email protected]>
>> >> > ---
>> >> > drivers/mfd/Kconfig | 13 ++
>> >> > drivers/mfd/Makefile | 1 +
>> >> > drivers/mfd/max77541.c | 224
>> >+++++++++++++++++++++++++++++++++++
>> >> > include/linux/mfd/max77541.h | 97 +++++++++++++++
>> >> > 4 files changed, 335 insertions(+) create mode 100644
>> >> > drivers/mfd/max77541.c create mode 100644
>> >> > include/linux/mfd/max77541.h
>> >>
>> >> FYI: I'm not re-reviewing this since you've chosen to ignore some
>> >> of my previous review comments. Issues highlighted by review
>> >> comments don't just go away on resubmission.
>> >
>> >... and the subject is malformed.
>> >
>> >--
>> >Lee Jones [李琼斯]
>>
>> Hi Lee,
>>
>> I am sorry if I missed your review comments, this was not my intention. I want
>to thank you for your contribution. Your feedbacks are very valuable, and I am
>trying to understand and fix each one before sending the patch. Indeed, I sorted
>your feedback on previous patches. As far as I know, I have fixed all of them, is
>there a problem with any of them that I fixed, or is there any missing review?
>From you, there were some comments like "why did you use this?", I suppose I
>need to respond them before sending following patches. I thought I should not
>bother the maintainers unnecessarily. I am sorry for them.
>
>Please ask your email client to line-wrap.
>
>Here is the part of the review you ignored:
>
>[...]
>
>> +static const struct chip_info chip[] = {
>
>Why do you need this require sub-structure?
>
>> + [MAX77540] = {
>> + .id = MAX77540,
>> + .n_devs = ARRAY_SIZE(max77540_devs),
>> + .devs = max77540_devs,
>> + },
>> + [MAX77541] = {
>> + .id = MAX77541,
>> + .n_devs = ARRAY_SIZE(max77541_devs),
>> + .devs = max77541_devs,
>> + },
>> +};
>
>[...]
>
>> +static const struct of_device_id max77541_of_id[] = {
>> + {
>> + .compatible = "adi,max77540",
>> + .data = &chip[MAX77540],
>> + },
>> + {
>> + .compatible = "adi,max77541",
>> + .data = &chip[MAX77541],
>> + },
>> + { /* sentinel */ }
>> +};
>> +MODULE_DEVICE_TABLE(of, max77541_of_id);
>> +
>> +static const struct i2c_device_id max77541_i2c_id[] = {
>> + { "max77540", (kernel_ulong_t)&chip[MAX77540] },
>> + { "max77541", (kernel_ulong_t)&chip[MAX77541] },
>
>Just 'MAX77540' is fine.
>
>> + { /* sentinel */ }
>
>Remove the comment, we know how terminators work.
>
>Same comments for max77541_of_id.
>
>--
>Lee Jones [李琼斯]
Hi Lee,
In fact, one of the maintainers suggested assigning chip_info to data instead of enumeration. Then I added chip_info and put devices into sub-structure above. I will replace chip_info with id structure in max77541 device structure, right? I will use enumeration for data as I will assign it to id, and distinguish different devices.
Regards
Okan Sahin
On Mon, 03 Apr 2023, Sahin, Okan wrote:
> >On Tue, 28 Mar 2023, Sahin, Okan wrote:
> >
> >> >On Wed, 15 Mar 2023, Lee Jones wrote:
> >> >
> >> >> On Tue, 07 Mar 2023, 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.
> >> >> >
> >> >> > Signed-off-by: Okan Sahin <[email protected]>
> >> >> > ---
> >> >> > drivers/mfd/Kconfig | 13 ++
> >> >> > drivers/mfd/Makefile | 1 +
> >> >> > drivers/mfd/max77541.c | 224
> >> >+++++++++++++++++++++++++++++++++++
> >> >> > include/linux/mfd/max77541.h | 97 +++++++++++++++
> >> >> > 4 files changed, 335 insertions(+) create mode 100644
> >> >> > drivers/mfd/max77541.c create mode 100644
> >> >> > include/linux/mfd/max77541.h
> >> >>
> >> >> FYI: I'm not re-reviewing this since you've chosen to ignore some
> >> >> of my previous review comments. Issues highlighted by review
> >> >> comments don't just go away on resubmission.
> >> >
> >> >... and the subject is malformed.
> >> >
> >> >--
> >> >Lee Jones [李琼斯]
> >>
> >> Hi Lee,
> >>
> >> I am sorry if I missed your review comments, this was not my intention. I want
> >to thank you for your contribution. Your feedbacks are very valuable, and I am
> >trying to understand and fix each one before sending the patch. Indeed, I sorted
> >your feedback on previous patches. As far as I know, I have fixed all of them, is
> >there a problem with any of them that I fixed, or is there any missing review?
> >From you, there were some comments like "why did you use this?", I suppose I
> >need to respond them before sending following patches. I thought I should not
> >bother the maintainers unnecessarily. I am sorry for them.
> >
> >Please ask your email client to line-wrap.
> >
> >Here is the part of the review you ignored:
> >
> >[...]
> >
> >> +static const struct chip_info chip[] = {
> >
> >Why do you need this require sub-structure?
> >
> >> + [MAX77540] = {
> >> + .id = MAX77540,
> >> + .n_devs = ARRAY_SIZE(max77540_devs),
> >> + .devs = max77540_devs,
> >> + },
> >> + [MAX77541] = {
> >> + .id = MAX77541,
> >> + .n_devs = ARRAY_SIZE(max77541_devs),
> >> + .devs = max77541_devs,
> >> + },
> >> +};
> >
> >[...]
> >
> >> +static const struct of_device_id max77541_of_id[] = {
> >> + {
> >> + .compatible = "adi,max77540",
> >> + .data = &chip[MAX77540],
> >> + },
> >> + {
> >> + .compatible = "adi,max77541",
> >> + .data = &chip[MAX77541],
> >> + },
> >> + { /* sentinel */ }
> >> +};
> >> +MODULE_DEVICE_TABLE(of, max77541_of_id);
> >> +
> >> +static const struct i2c_device_id max77541_i2c_id[] = {
> >> + { "max77540", (kernel_ulong_t)&chip[MAX77540] },
> >> + { "max77541", (kernel_ulong_t)&chip[MAX77541] },
> >
> >Just 'MAX77540' is fine.
> >
> >> + { /* sentinel */ }
> >
> >Remove the comment, we know how terminators work.
> >
> >Same comments for max77541_of_id.
Your mailer is still broken. Please line wrap.
> In fact, one of the maintainers suggested assigning chip_info to data instead of enumeration. Then I added chip_info and put devices into sub-structure above. I will replace chip_info with id structure in max77541 device structure, right? I will use enumeration for data as I will assign it to id, and distinguish different devices.
Yes, that's correct. Please remove chip_info altogether.
--
Lee Jones [李琼斯]
>On Tue, Mar 07, 2023 at 02:28:12PM +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.
>
>Reviewed-by: Andy Shevchenko <[email protected]>
>
>But see below.
>
>> Signed-off-by: Okan Sahin <[email protected]>
>> ---
>> drivers/regulator/Kconfig | 9 ++
>> drivers/regulator/Makefile | 1 +
>> drivers/regulator/max77541-regulator.c | 153
>> +++++++++++++++++++++++++
>> 3 files changed, 163 insertions(+)
>> create mode 100644 drivers/regulator/max77541-regulator.c
>>
>> diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
>> index aae28d0a489c..f0418274c083 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.
>
>Maybe adding what would be the module name if M is chosen?
>
>> 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
>> ee383d8fc835..1c852f3140a3 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..f99caf3f3990
>> --- /dev/null
>> +++ b/drivers/regulator/max77541-regulator.c
>> @@ -0,0 +1,153 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +/*
>> + * Copyright (c) 2022 Analog Devices, Inc.
>> + * ADI Regulator driver for the MAX77540 and MAX77541 */
>
>I believe Mark asked to have this C++ comment style as well.
>
>// Copyright (c) 2022 Analog Devices, Inc.
>// ADI Regulator driver for the MAX77540 and MAX77541
>
>> +#include <linux/mfd/max77541.h>
>> +#include <linux/mod_devicetable.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/regmap.h>
>> +#include <linux/regulator/driver.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,
>> +};
>> +
>> +enum max77541_regulators {
>> + MAX77541_BUCK1 = 1,
>> + MAX77541_BUCK2,
>> +};
>> +
>> +#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 regulator_config config = {};
>> + const struct regulator_desc *desc;
>> + struct device *dev = &pdev->dev;
>> + struct regulator_dev *rdev;
>> + struct max77541 *max77541 = dev_get_drvdata(dev->parent);
>> + unsigned int i;
>> +
>> + config.dev = dev->parent;
>> +
>> + switch (max77541->chip->id) {
>> + case MAX77540:
>> + desc = max77540_regulators_desc;
>> + break;
>> + case MAX77541:
>> + desc = max77541_regulators_desc;
>> + break;
>> + default:
>> + 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" },
>> + { "max77541-regulator" },
>> + { }
>> +};
>> +MODULE_DEVICE_TABLE(platform, max77541_regulator_platform_id);
>> +
>> +static struct platform_driver max77541_regulator_driver = {
>> + .driver = {
>> + .name = "max77541-regulator",
>> + },
>> + .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
>>
>
>--
>With Best Regards,
>Andy Shevchenko
>
Hi Andy,
Should I change comment style to C++ comment style for whole patchset?
Regards,
Okan
On Mon, Apr 03, 2023 at 03:09:50PM +0100, Lee Jones wrote:
> On Mon, 03 Apr 2023, Sahin, Okan wrote:
...
> > In fact, one of the maintainers suggested assigning chip_info to data
> > instead of enumeration. Then I added chip_info and put devices into
> > sub-structure above. I will replace chip_info with id structure in max77541
> > device structure, right? I will use enumeration for data as I will assign
> > it to id, and distinguish different devices.
>
> Yes, that's correct. Please remove chip_info altogether.
Then it will provoke casting in the OF ID table which I believe is not what
we want. I would agree on your first suggestion to have a plain number in I?C
ID table, but I'm against it in OF and/or ACPI ID table.
--
With Best Regards,
Andy Shevchenko
On Wed, 05 Apr 2023, Andy Shevchenko wrote:
> On Mon, Apr 03, 2023 at 03:09:50PM +0100, Lee Jones wrote:
> > On Mon, 03 Apr 2023, Sahin, Okan wrote:
>
> ...
>
>
> > > In fact, one of the maintainers suggested assigning chip_info to data
> > > instead of enumeration. Then I added chip_info and put devices into
> > > sub-structure above. I will replace chip_info with id structure in max77541
> > > device structure, right? I will use enumeration for data as I will assign
> > > it to id, and distinguish different devices.
> >
> > Yes, that's correct. Please remove chip_info altogether.
>
> Then it will provoke casting in the OF ID table which I believe is not what
> we want. I would agree on your first suggestion to have a plain number in I²C
> ID table, but I'm against it in OF and/or ACPI ID table.
And I'm against passing MFD information through the OF/ACPI APIs.
You can put through raw platform data or a device descriptor.
Ref: git grep -A5 "struct of_device_id.*{" -- drivers/mfd
--
Lee Jones [李琼斯]
>On Wed, 05 Apr 2023, Andy Shevchenko wrote:
>
>> On Mon, Apr 03, 2023 at 03:09:50PM +0100, Lee Jones wrote:
>> > On Mon, 03 Apr 2023, Sahin, Okan wrote:
>>
>> ...
>>
>>
>> > > In fact, one of the maintainers suggested assigning chip_info to data
>> > > instead of enumeration. Then I added chip_info and put devices into
>> > > sub-structure above. I will replace chip_info with id structure in max77541
>> > > device structure, right? I will use enumeration for data as I will assign
>> > > it to id, and distinguish different devices.
>> >
>> > Yes, that's correct. Please remove chip_info altogether.
>>
>> Then it will provoke casting in the OF ID table which I believe is not what
>> we want. I would agree on your first suggestion to have a plain number in I²C
>> ID table, but I'm against it in OF and/or ACPI ID table.
>
>And I'm against passing MFD information through the OF/ACPI APIs.
>
>You can put through raw platform data or a device descriptor.
>
>Ref: git grep -A5 "struct of_device_id.*{" -- drivers/mfd
>
>--
>Lee Jones [李琼斯]
Hi Lee,
Right now, as you suggested I rewrote code like below
For of_device_id,
. data = (void *)MAX77540,
.data = (void *)MAX77541,
For i2c_device_id,
.data = MAX77540,
.data = MAX77541
I also rewrote other part as chip_info is excluded. I want to be sure before
sending new patch.
Does it seem correct?
Regards,
Okan Sahin
On Sun, 09 Apr 2023, Sahin, Okan wrote:
> >On Wed, 05 Apr 2023, Andy Shevchenko wrote:
> >
> >> On Mon, Apr 03, 2023 at 03:09:50PM +0100, Lee Jones wrote:
> >> > On Mon, 03 Apr 2023, Sahin, Okan wrote:
> >>
> >> ...
> >>
> >>
> >> > > In fact, one of the maintainers suggested assigning chip_info to data
> >> > > instead of enumeration. Then I added chip_info and put devices into
> >> > > sub-structure above. I will replace chip_info with id structure in max77541
> >> > > device structure, right? I will use enumeration for data as I will assign
> >> > > it to id, and distinguish different devices.
> >> >
> >> > Yes, that's correct. Please remove chip_info altogether.
> >>
> >> Then it will provoke casting in the OF ID table which I believe is not what
> >> we want. I would agree on your first suggestion to have a plain number in I²C
> >> ID table, but I'm against it in OF and/or ACPI ID table.
> >
> >And I'm against passing MFD information through the OF/ACPI APIs.
> >
> >You can put through raw platform data or a device descriptor.
> >
> >Ref: git grep -A5 "struct of_device_id.*{" -- drivers/mfd
> >
> >--
> >Lee Jones [李琼斯]
>
> Hi Lee,
>
> Right now, as you suggested I rewrote code like below
> For of_device_id,
> . data = (void *)MAX77540,
> .data = (void *)MAX77541,
> For i2c_device_id,
> .data = MAX77540,
> .data = MAX77541
> I also rewrote other part as chip_info is excluded. I want to be sure before
> sending new patch.
>
> Does it seem correct?
This is one suitable method, yes.
--
Lee Jones [李琼斯]
>On Sun, 09 Apr 2023, Sahin, Okan wrote:
>
>> >On Wed, 05 Apr 2023, Andy Shevchenko wrote:
>> >
>> >> On Mon, Apr 03, 2023 at 03:09:50PM +0100, Lee Jones wrote:
>> >> > On Mon, 03 Apr 2023, Sahin, Okan wrote:
>> >>
>> >> ...
>> >>
>> >>
>> >> > > In fact, one of the maintainers suggested assigning chip_info to data
>> >> > > instead of enumeration. Then I added chip_info and put devices into
>> >> > > sub-structure above. I will replace chip_info with id structure in max77541
>> >> > > device structure, right? I will use enumeration for data as I will assign
>> >> > > it to id, and distinguish different devices.
>> >> >
>> >> > Yes, that's correct. Please remove chip_info altogether.
>> >>
>> >> Then it will provoke casting in the OF ID table which I believe is not what
>> >> we want. I would agree on your first suggestion to have a plain number in I²C
>> >> ID table, but I'm against it in OF and/or ACPI ID table.
>> >
>> >And I'm against passing MFD information through the OF/ACPI APIs.
>> >
>> >You can put through raw platform data or a device descriptor.
>> >
>> >Ref: git grep -A5 "struct of_device_id.*{" -- drivers/mfd
>> >
>> >--
>> >Lee Jones [李琼斯]
>>
>> Hi Lee,
>>
>> Right now, as you suggested I rewrote code like below
>> For of_device_id,
>> . data = (void *)MAX77540,
>> .data = (void *)MAX77541,
>> For i2c_device_id,
>> .data = MAX77540,
>> .data = MAX77541
>> I also rewrote other part as chip_info is excluded. I want to be sure before
>> sending new patch.
>>
>> Does it seem correct?
>
>This is one suitable method, yes.
>
>--
>Lee Jones [李琼斯]
Hi Lee,
Thank you for your support.
Regards,
Okan Sahin