Add a simple driver for the Maxim MAX77714 PMIC, supporting RTC and
watchdog only.
Signed-off-by: Luca Ceresoli <[email protected]>
---
Changes in v2:
- fix "watchdog" word in heading comment (Guenter Roeck)
- move struct max77714 to .c file (Krzysztof Kozlowski)
- change include guard format (Krzysztof Kozlowski)
- allow building as a module (Krzysztof Kozlowski)
- remove of_match_ptr usage (Krzysztof Kozlowski / lkp)
(Reported-by: kernel test robot <[email protected]>)
---
MAINTAINERS | 2 +
drivers/mfd/Kconfig | 14 +++
drivers/mfd/Makefile | 1 +
drivers/mfd/max77714.c | 165 +++++++++++++++++++++++++++++++++++
include/linux/mfd/max77714.h | 60 +++++++++++++
5 files changed, 242 insertions(+)
create mode 100644 drivers/mfd/max77714.c
create mode 100644 include/linux/mfd/max77714.h
diff --git a/MAINTAINERS b/MAINTAINERS
index 514ff4a735e5..abd9de8a9d99 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11390,6 +11390,8 @@ MAXIM MAX77714 PMIC MFD DRIVER
M: Luca Ceresoli <[email protected]>
S: Maintained
F: Documentation/devicetree/bindings/mfd/maxim,max77714.yaml
+F: drivers/mfd/max77714.c
+F: include/linux/mfd/max77714.h
MAXIM MAX77802 PMIC REGULATOR DEVICE DRIVER
M: Javier Martinez Canillas <[email protected]>
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index ca0edab91aeb..295a04b479c6 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -853,6 +853,20 @@ config MFD_MAX77693
additional drivers must be enabled in order to use the functionality
of the device.
+config MFD_MAX77714
+ tristate "Maxim Semiconductor MAX77714 PMIC Support"
+ depends on I2C
+ depends on OF || COMPILE_TEST
+ select MFD_CORE
+ select REGMAP_I2C
+ help
+ Say yes here to add support for Maxim Semiconductor MAX77714.
+ This is a Power Management IC with 4 buck regulators, 9
+ low-dropout regulators, 8 GPIOs, RTC, watchdog etc. This driver
+ provides common support for accessing the device; additional
+ drivers must be enabled in order to use each functionality of the
+ device.
+
config MFD_MAX77843
bool "Maxim Semiconductor MAX77843 PMIC Support"
depends on I2C=y
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index 2ba6646e874c..fe43f2fdd5cb 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -163,6 +163,7 @@ obj-$(CONFIG_MFD_MAX77620) += max77620.o
obj-$(CONFIG_MFD_MAX77650) += max77650.o
obj-$(CONFIG_MFD_MAX77686) += max77686.o
obj-$(CONFIG_MFD_MAX77693) += max77693.o
+obj-$(CONFIG_MFD_MAX77714) += max77714.o
obj-$(CONFIG_MFD_MAX77843) += max77843.o
obj-$(CONFIG_MFD_MAX8907) += max8907.o
max8925-objs := max8925-core.o max8925-i2c.o
diff --git a/drivers/mfd/max77714.c b/drivers/mfd/max77714.c
new file mode 100644
index 000000000000..4b49d16fe199
--- /dev/null
+++ b/drivers/mfd/max77714.c
@@ -0,0 +1,165 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Maxim MAX77714 MFD Driver
+ *
+ * Copyright (C) 2021 Luca Ceresoli
+ * Author: Luca Ceresoli <[email protected]>
+ */
+
+#include <linux/i2c.h>
+#include <linux/interrupt.h>
+#include <linux/mfd/core.h>
+#include <linux/mfd/max77714.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/regmap.h>
+
+struct max77714 {
+ struct device *dev;
+ struct regmap *regmap;
+ struct regmap_irq_chip_data *irq_data;
+
+ int irq;
+};
+
+static const struct regmap_range max77714_readable_ranges[] = {
+ regmap_reg_range(MAX77714_INT_TOP, MAX77714_INT_TOP),
+ regmap_reg_range(MAX77714_INT_TOPM, MAX77714_INT_TOPM),
+ regmap_reg_range(MAX77714_32K_STATUS, MAX77714_32K_CONFIG),
+ regmap_reg_range(MAX77714_CNFG_GLBL2, MAX77714_CNFG2_ONOFF),
+};
+
+static const struct regmap_range max77714_writable_ranges[] = {
+ regmap_reg_range(MAX77714_INT_TOPM, MAX77714_INT_TOPM),
+ regmap_reg_range(MAX77714_32K_CONFIG, MAX77714_32K_CONFIG),
+ regmap_reg_range(MAX77714_CNFG_GLBL2, MAX77714_CNFG2_ONOFF),
+};
+
+static const struct regmap_access_table max77714_readable_table = {
+ .yes_ranges = max77714_readable_ranges,
+ .n_yes_ranges = ARRAY_SIZE(max77714_readable_ranges),
+};
+
+static const struct regmap_access_table max77714_writable_table = {
+ .yes_ranges = max77714_writable_ranges,
+ .n_yes_ranges = ARRAY_SIZE(max77714_writable_ranges),
+};
+
+static const struct regmap_config max77714_regmap_config = {
+ .reg_bits = 8,
+ .val_bits = 8,
+ .max_register = MAX77714_CNFG2_ONOFF,
+ .rd_table = &max77714_readable_table,
+ .wr_table = &max77714_writable_table,
+};
+
+static const struct regmap_irq max77714_top_irqs[] = {
+ REGMAP_IRQ_REG(MAX77714_IRQ_TOP_ONOFF, 0, MAX77714_INT_TOP_ONOFF),
+ REGMAP_IRQ_REG(MAX77714_IRQ_TOP_RTC, 0, MAX77714_INT_TOP_RTC),
+ REGMAP_IRQ_REG(MAX77714_IRQ_TOP_GPIO, 0, MAX77714_INT_TOP_GPIO),
+ REGMAP_IRQ_REG(MAX77714_IRQ_TOP_LDO, 0, MAX77714_INT_TOP_LDO),
+ REGMAP_IRQ_REG(MAX77714_IRQ_TOP_SD, 0, MAX77714_INT_TOP_SD),
+ REGMAP_IRQ_REG(MAX77714_IRQ_TOP_GLBL, 0, MAX77714_INT_TOP_GLBL),
+};
+
+static const struct regmap_irq_chip max77714_irq_chip = {
+ .name = "max77714-pmic",
+ .status_base = MAX77714_INT_TOP,
+ .mask_base = MAX77714_INT_TOPM,
+ .num_regs = 1,
+ .irqs = max77714_top_irqs,
+ .num_irqs = ARRAY_SIZE(max77714_top_irqs),
+};
+
+static const struct mfd_cell max77714_cells[] = {
+ { .name = "max77714-watchdog" },
+ { .name = "max77714-rtc" },
+};
+
+/*
+ * MAX77714 initially uses the internal, low precision oscillator. Enable
+ * the external oscillator by setting the XOSC_RETRY bit. If the external
+ * oscillator is not OK (probably not installed) this has no effect.
+ */
+static int max77714_setup_xosc(struct max77714 *chip)
+{
+ /* Internal Crystal Load Capacitance, indexed by value of 32KLOAD bits */
+ static const unsigned int load_cap[4] = {0, 10, 12, 22};
+ unsigned int load_cap_idx;
+ unsigned int status;
+ int err;
+
+ err = regmap_update_bits(chip->regmap, MAX77714_32K_CONFIG,
+ MAX77714_32K_CONFIG_XOSC_RETRY,
+ MAX77714_32K_CONFIG_XOSC_RETRY);
+ if (err)
+ return dev_err_probe(chip->dev, err, "cannot configure XOSC\n");
+
+ err = regmap_read(chip->regmap, MAX77714_32K_STATUS, &status);
+ if (err)
+ return dev_err_probe(chip->dev, err, "cannot read XOSC status\n");
+
+ load_cap_idx = (status >> MAX77714_32K_STATUS_32KLOAD_SHF)
+ & MAX77714_32K_STATUS_32KLOAD_MSK;
+
+ dev_info(chip->dev, "Using %s oscillator, %d pF load cap\n",
+ status & MAX77714_32K_STATUS_32KSOURCE ? "internal" : "external",
+ load_cap[load_cap_idx]);
+
+ return 0;
+}
+
+static int max77714_probe(struct i2c_client *client)
+{
+ struct max77714 *chip;
+ int err;
+
+ chip = devm_kzalloc(&client->dev, sizeof(*chip), GFP_KERNEL);
+ if (!chip)
+ return -ENOMEM;
+
+ i2c_set_clientdata(client, chip);
+ chip->dev = &client->dev;
+
+ chip->regmap = devm_regmap_init_i2c(client, &max77714_regmap_config);
+ if (IS_ERR(chip->regmap))
+ return dev_err_probe(chip->dev, PTR_ERR(chip->regmap),
+ "failed to initialise regmap\n");
+
+ err = max77714_setup_xosc(chip);
+ if (err)
+ return err;
+
+ err = devm_regmap_add_irq_chip(chip->dev, chip->regmap, client->irq,
+ IRQF_ONESHOT | IRQF_SHARED, 0,
+ &max77714_irq_chip, &chip->irq_data);
+ if (err)
+ return dev_err_probe(chip->dev, err, "failed to add PMIC irq chip\n");
+
+ err = devm_mfd_add_devices(chip->dev, PLATFORM_DEVID_NONE,
+ max77714_cells, ARRAY_SIZE(max77714_cells),
+ NULL, 0, NULL);
+ if (err)
+ return dev_err_probe(chip->dev, err, "failed adding MFD children\n");
+
+ return 0;
+}
+
+static const struct of_device_id max77714_dt_match[] = {
+ { .compatible = "maxim,max77714" },
+ {},
+};
+MODULE_DEVICE_TABLE(of, max77714_dt_match);
+
+static struct i2c_driver max77714_driver = {
+ .driver = {
+ .name = "max77714",
+ .of_match_table = max77714_dt_match,
+ },
+ .probe_new = max77714_probe,
+};
+module_i2c_driver(max77714_driver);
+
+MODULE_DESCRIPTION("Maxim MAX77714 MFD core driver");
+MODULE_AUTHOR("Luca Ceresoli <[email protected]>");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/mfd/max77714.h b/include/linux/mfd/max77714.h
new file mode 100644
index 000000000000..4a274592d4f2
--- /dev/null
+++ b/include/linux/mfd/max77714.h
@@ -0,0 +1,60 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Maxim MAX77714 Register and data structures definition.
+ *
+ * Copyright (C) 2021 Luca Ceresoli
+ * Author: Luca Ceresoli <[email protected]>
+ */
+
+#ifndef __LINUX_MFD_MAX77714_H_
+#define __LINUX_MFD_MAX77714_H_
+
+#include <linux/bits.h>
+
+#define MAX77714_INT_TOP 0x00
+#define MAX77714_INT_TOPM 0x07 /* Datasheet says "read only", but it is RW */
+
+#define MAX77714_INT_TOP_ONOFF BIT(1)
+#define MAX77714_INT_TOP_RTC BIT(3)
+#define MAX77714_INT_TOP_GPIO BIT(4)
+#define MAX77714_INT_TOP_LDO BIT(5)
+#define MAX77714_INT_TOP_SD BIT(6)
+#define MAX77714_INT_TOP_GLBL BIT(7)
+
+#define MAX77714_32K_STATUS 0x30
+#define MAX77714_32K_STATUS_SIOSCOK BIT(5)
+#define MAX77714_32K_STATUS_XOSCOK BIT(4)
+#define MAX77714_32K_STATUS_32KSOURCE BIT(3)
+#define MAX77714_32K_STATUS_32KLOAD_MSK 0x3
+#define MAX77714_32K_STATUS_32KLOAD_SHF 1
+#define MAX77714_32K_STATUS_CRYSTAL_CFG BIT(0)
+
+#define MAX77714_32K_CONFIG 0x31
+#define MAX77714_32K_CONFIG_XOSC_RETRY BIT(4)
+
+#define MAX77714_CNFG_GLBL2 0x91
+#define MAX77714_WDTEN BIT(2)
+#define MAX77714_WDTSLPC BIT(3)
+#define MAX77714_TWD_MASK 0x3
+#define MAX77714_TWD_2s 0x0
+#define MAX77714_TWD_16s 0x1
+#define MAX77714_TWD_64s 0x2
+#define MAX77714_TWD_128s 0x3
+
+#define MAX77714_CNFG_GLBL3 0x92
+#define MAX77714_WDTC BIT(0)
+
+#define MAX77714_CNFG2_ONOFF 0x94
+#define MAX77714_WD_RST_WK BIT(5)
+
+/* Interrupts */
+enum {
+ MAX77714_IRQ_TOP_ONOFF,
+ MAX77714_IRQ_TOP_RTC, /* Real-time clock */
+ MAX77714_IRQ_TOP_GPIO, /* GPIOs */
+ MAX77714_IRQ_TOP_LDO, /* Low-dropout regulators */
+ MAX77714_IRQ_TOP_SD, /* Step-down regulators */
+ MAX77714_IRQ_TOP_GLBL, /* "Global resources": Low-Battery, overtemp... */
+};
+
+#endif /* __LINUX_MFD_MAX77714_H_ */
--
2.25.1
On 19/10/2021 16:59, Luca Ceresoli wrote:
> Add a simple driver for the Maxim MAX77714 PMIC, supporting RTC and
> watchdog only.
>
> Signed-off-by: Luca Ceresoli <[email protected]>
>
> ---
>
> Changes in v2:
> - fix "watchdog" word in heading comment (Guenter Roeck)
> - move struct max77714 to .c file (Krzysztof Kozlowski)
> - change include guard format (Krzysztof Kozlowski)
> - allow building as a module (Krzysztof Kozlowski)
> - remove of_match_ptr usage (Krzysztof Kozlowski / lkp)
> (Reported-by: kernel test robot <[email protected]>)
> ---
> MAINTAINERS | 2 +
> drivers/mfd/Kconfig | 14 +++
> drivers/mfd/Makefile | 1 +
> drivers/mfd/max77714.c | 165 +++++++++++++++++++++++++++++++++++
> include/linux/mfd/max77714.h | 60 +++++++++++++
> 5 files changed, 242 insertions(+)
> create mode 100644 drivers/mfd/max77714.c
> create mode 100644 include/linux/mfd/max77714.h
>
Reviewed-by: Krzysztof Kozlowski <[email protected]>
Best regards,
Krzysztof
On Tue, 19 Oct 2021, Luca Ceresoli wrote:
> Add a simple driver for the Maxim MAX77714 PMIC, supporting RTC and
> watchdog only.
>
> Signed-off-by: Luca Ceresoli <[email protected]>
>
> ---
>
> Changes in v2:
> - fix "watchdog" word in heading comment (Guenter Roeck)
> - move struct max77714 to .c file (Krzysztof Kozlowski)
> - change include guard format (Krzysztof Kozlowski)
> - allow building as a module (Krzysztof Kozlowski)
> - remove of_match_ptr usage (Krzysztof Kozlowski / lkp)
> (Reported-by: kernel test robot <[email protected]>)
> ---
> MAINTAINERS | 2 +
> drivers/mfd/Kconfig | 14 +++
> drivers/mfd/Makefile | 1 +
> drivers/mfd/max77714.c | 165 +++++++++++++++++++++++++++++++++++
> include/linux/mfd/max77714.h | 60 +++++++++++++
> 5 files changed, 242 insertions(+)
> create mode 100644 drivers/mfd/max77714.c
> create mode 100644 include/linux/mfd/max77714.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 514ff4a735e5..abd9de8a9d99 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -11390,6 +11390,8 @@ MAXIM MAX77714 PMIC MFD DRIVER
> M: Luca Ceresoli <[email protected]>
> S: Maintained
> F: Documentation/devicetree/bindings/mfd/maxim,max77714.yaml
> +F: drivers/mfd/max77714.c
> +F: include/linux/mfd/max77714.h
>
> MAXIM MAX77802 PMIC REGULATOR DEVICE DRIVER
> M: Javier Martinez Canillas <[email protected]>
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index ca0edab91aeb..295a04b479c6 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -853,6 +853,20 @@ config MFD_MAX77693
> additional drivers must be enabled in order to use the functionality
> of the device.
>
> +config MFD_MAX77714
> + tristate "Maxim Semiconductor MAX77714 PMIC Support"
> + depends on I2C
> + depends on OF || COMPILE_TEST
> + select MFD_CORE
> + select REGMAP_I2C
> + help
> + Say yes here to add support for Maxim Semiconductor MAX77714.
> + This is a Power Management IC with 4 buck regulators, 9
> + low-dropout regulators, 8 GPIOs, RTC, watchdog etc. This driver
> + provides common support for accessing the device; additional
> + drivers must be enabled in order to use each functionality of the
> + device.
> +
> config MFD_MAX77843
> bool "Maxim Semiconductor MAX77843 PMIC Support"
> depends on I2C=y
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 2ba6646e874c..fe43f2fdd5cb 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -163,6 +163,7 @@ obj-$(CONFIG_MFD_MAX77620) += max77620.o
> obj-$(CONFIG_MFD_MAX77650) += max77650.o
> obj-$(CONFIG_MFD_MAX77686) += max77686.o
> obj-$(CONFIG_MFD_MAX77693) += max77693.o
> +obj-$(CONFIG_MFD_MAX77714) += max77714.o
> obj-$(CONFIG_MFD_MAX77843) += max77843.o
> obj-$(CONFIG_MFD_MAX8907) += max8907.o
> max8925-objs := max8925-core.o max8925-i2c.o
> diff --git a/drivers/mfd/max77714.c b/drivers/mfd/max77714.c
> new file mode 100644
> index 000000000000..4b49d16fe199
> --- /dev/null
> +++ b/drivers/mfd/max77714.c
> @@ -0,0 +1,165 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Maxim MAX77714 MFD Driver
> + *
> + * Copyright (C) 2021 Luca Ceresoli
> + * Author: Luca Ceresoli <[email protected]>
> + */
> +
> +#include <linux/i2c.h>
> +#include <linux/interrupt.h>
> +#include <linux/mfd/core.h>
> +#include <linux/mfd/max77714.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/regmap.h>
> +
> +struct max77714 {
> + struct device *dev;
> + struct regmap *regmap;
> + struct regmap_irq_chip_data *irq_data;
Is this used outside of .probe()?
> + int irq;
> +};
> +
> +static const struct regmap_range max77714_readable_ranges[] = {
> + regmap_reg_range(MAX77714_INT_TOP, MAX77714_INT_TOP),
> + regmap_reg_range(MAX77714_INT_TOPM, MAX77714_INT_TOPM),
> + regmap_reg_range(MAX77714_32K_STATUS, MAX77714_32K_CONFIG),
> + regmap_reg_range(MAX77714_CNFG_GLBL2, MAX77714_CNFG2_ONOFF),
> +};
> +
> +static const struct regmap_range max77714_writable_ranges[] = {
> + regmap_reg_range(MAX77714_INT_TOPM, MAX77714_INT_TOPM),
> + regmap_reg_range(MAX77714_32K_CONFIG, MAX77714_32K_CONFIG),
> + regmap_reg_range(MAX77714_CNFG_GLBL2, MAX77714_CNFG2_ONOFF),
> +};
> +
> +static const struct regmap_access_table max77714_readable_table = {
> + .yes_ranges = max77714_readable_ranges,
> + .n_yes_ranges = ARRAY_SIZE(max77714_readable_ranges),
> +};
> +
> +static const struct regmap_access_table max77714_writable_table = {
> + .yes_ranges = max77714_writable_ranges,
> + .n_yes_ranges = ARRAY_SIZE(max77714_writable_ranges),
> +};
> +
> +static const struct regmap_config max77714_regmap_config = {
> + .reg_bits = 8,
> + .val_bits = 8,
> + .max_register = MAX77714_CNFG2_ONOFF,
> + .rd_table = &max77714_readable_table,
> + .wr_table = &max77714_writable_table,
> +};
> +
> +static const struct regmap_irq max77714_top_irqs[] = {
> + REGMAP_IRQ_REG(MAX77714_IRQ_TOP_ONOFF, 0, MAX77714_INT_TOP_ONOFF),
> + REGMAP_IRQ_REG(MAX77714_IRQ_TOP_RTC, 0, MAX77714_INT_TOP_RTC),
> + REGMAP_IRQ_REG(MAX77714_IRQ_TOP_GPIO, 0, MAX77714_INT_TOP_GPIO),
> + REGMAP_IRQ_REG(MAX77714_IRQ_TOP_LDO, 0, MAX77714_INT_TOP_LDO),
> + REGMAP_IRQ_REG(MAX77714_IRQ_TOP_SD, 0, MAX77714_INT_TOP_SD),
> + REGMAP_IRQ_REG(MAX77714_IRQ_TOP_GLBL, 0, MAX77714_INT_TOP_GLBL),
> +};
> +
> +static const struct regmap_irq_chip max77714_irq_chip = {
> + .name = "max77714-pmic",
> + .status_base = MAX77714_INT_TOP,
> + .mask_base = MAX77714_INT_TOPM,
> + .num_regs = 1,
> + .irqs = max77714_top_irqs,
> + .num_irqs = ARRAY_SIZE(max77714_top_irqs),
> +};
> +
> +static const struct mfd_cell max77714_cells[] = {
> + { .name = "max77714-watchdog" },
> + { .name = "max77714-rtc" },
> +};
Please place this at the top of the file.
> +/*
> + * MAX77714 initially uses the internal, low precision oscillator. Enable
> + * the external oscillator by setting the XOSC_RETRY bit. If the external
> + * oscillator is not OK (probably not installed) this has no effect.
> + */
> +static int max77714_setup_xosc(struct max77714 *chip)
May as well just pass 'dev' and 'regmap' to this function and do away
with the superfluous struct along with all of it's memory management
handling requirements.
> +{
> + /* Internal Crystal Load Capacitance, indexed by value of 32KLOAD bits */
> + static const unsigned int load_cap[4] = {0, 10, 12, 22};
Probably best to define these magic numbers.
> + unsigned int load_cap_idx;
> + unsigned int status;
> + int err;
> +
> + err = regmap_update_bits(chip->regmap, MAX77714_32K_CONFIG,
> + MAX77714_32K_CONFIG_XOSC_RETRY,
> + MAX77714_32K_CONFIG_XOSC_RETRY);
> + if (err)
> + return dev_err_probe(chip->dev, err, "cannot configure XOSC\n");
Error messages should be clear:
"Failed to configure the external oscillator"
Same for the messages below.
> + err = regmap_read(chip->regmap, MAX77714_32K_STATUS, &status);
> + if (err)
> + return dev_err_probe(chip->dev, err, "cannot read XOSC status\n");
> +
> + load_cap_idx = (status >> MAX77714_32K_STATUS_32KLOAD_SHF)
> + & MAX77714_32K_STATUS_32KLOAD_MSK;
> +
> + dev_info(chip->dev, "Using %s oscillator, %d pF load cap\n",
> + status & MAX77714_32K_STATUS_32KSOURCE ? "internal" : "external",
> + load_cap[load_cap_idx]);
> +
> + return 0;
> +}
> +
> +static int max77714_probe(struct i2c_client *client)
> +{
> + struct max77714 *chip;
> + int err;
> +
> + chip = devm_kzalloc(&client->dev, sizeof(*chip), GFP_KERNEL);
> + if (!chip)
> + return -ENOMEM;
> +
> + i2c_set_clientdata(client, chip);
Where else is this used?
The definition appears to be local.
> + chip->dev = &client->dev;
> +
> + chip->regmap = devm_regmap_init_i2c(client, &max77714_regmap_config);
> + if (IS_ERR(chip->regmap))
> + return dev_err_probe(chip->dev, PTR_ERR(chip->regmap),
> + "failed to initialise regmap\n");
> +
> + err = max77714_setup_xosc(chip);
> + if (err)
> + return err;
> +
> + err = devm_regmap_add_irq_chip(chip->dev, chip->regmap, client->irq,
> + IRQF_ONESHOT | IRQF_SHARED, 0,
> + &max77714_irq_chip, &chip->irq_data);
> + if (err)
> + return dev_err_probe(chip->dev, err, "failed to add PMIC irq chip\n");
IRQ
> + err = devm_mfd_add_devices(chip->dev, PLATFORM_DEVID_NONE,
> + max77714_cells, ARRAY_SIZE(max77714_cells),
> + NULL, 0, NULL);
> + if (err)
> + return dev_err_probe(chip->dev, err, "failed adding MFD children\n");
"Failed to register child devices"
> + return 0;
> +}
> +
> +static const struct of_device_id max77714_dt_match[] = {
> + { .compatible = "maxim,max77714" },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, max77714_dt_match);
> +
> +static struct i2c_driver max77714_driver = {
> + .driver = {
> + .name = "max77714",
> + .of_match_table = max77714_dt_match,
> + },
> + .probe_new = max77714_probe,
> +};
> +module_i2c_driver(max77714_driver);
> +
> +MODULE_DESCRIPTION("Maxim MAX77714 MFD core driver");
> +MODULE_AUTHOR("Luca Ceresoli <[email protected]>");
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/mfd/max77714.h b/include/linux/mfd/max77714.h
> new file mode 100644
> index 000000000000..4a274592d4f2
> --- /dev/null
> +++ b/include/linux/mfd/max77714.h
> @@ -0,0 +1,60 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Maxim MAX77714 Register and data structures definition.
> + *
> + * Copyright (C) 2021 Luca Ceresoli
> + * Author: Luca Ceresoli <[email protected]>
> + */
> +
> +#ifndef __LINUX_MFD_MAX77714_H_
> +#define __LINUX_MFD_MAX77714_H_
> +
> +#include <linux/bits.h>
> +
> +#define MAX77714_INT_TOP 0x00
> +#define MAX77714_INT_TOPM 0x07 /* Datasheet says "read only", but it is RW */
> +
> +#define MAX77714_INT_TOP_ONOFF BIT(1)
> +#define MAX77714_INT_TOP_RTC BIT(3)
> +#define MAX77714_INT_TOP_GPIO BIT(4)
> +#define MAX77714_INT_TOP_LDO BIT(5)
> +#define MAX77714_INT_TOP_SD BIT(6)
> +#define MAX77714_INT_TOP_GLBL BIT(7)
> +
> +#define MAX77714_32K_STATUS 0x30
> +#define MAX77714_32K_STATUS_SIOSCOK BIT(5)
> +#define MAX77714_32K_STATUS_XOSCOK BIT(4)
> +#define MAX77714_32K_STATUS_32KSOURCE BIT(3)
> +#define MAX77714_32K_STATUS_32KLOAD_MSK 0x3
> +#define MAX77714_32K_STATUS_32KLOAD_SHF 1
> +#define MAX77714_32K_STATUS_CRYSTAL_CFG BIT(0)
> +
> +#define MAX77714_32K_CONFIG 0x31
> +#define MAX77714_32K_CONFIG_XOSC_RETRY BIT(4)
> +
> +#define MAX77714_CNFG_GLBL2 0x91
> +#define MAX77714_WDTEN BIT(2)
> +#define MAX77714_WDTSLPC BIT(3)
> +#define MAX77714_TWD_MASK 0x3
> +#define MAX77714_TWD_2s 0x0
> +#define MAX77714_TWD_16s 0x1
> +#define MAX77714_TWD_64s 0x2
> +#define MAX77714_TWD_128s 0x3
> +
> +#define MAX77714_CNFG_GLBL3 0x92
> +#define MAX77714_WDTC BIT(0)
> +
> +#define MAX77714_CNFG2_ONOFF 0x94
> +#define MAX77714_WD_RST_WK BIT(5)
> +
> +/* Interrupts */
> +enum {
> + MAX77714_IRQ_TOP_ONOFF,
> + MAX77714_IRQ_TOP_RTC, /* Real-time clock */
> + MAX77714_IRQ_TOP_GPIO, /* GPIOs */
> + MAX77714_IRQ_TOP_LDO, /* Low-dropout regulators */
> + MAX77714_IRQ_TOP_SD, /* Step-down regulators */
> + MAX77714_IRQ_TOP_GLBL, /* "Global resources": Low-Battery, overtemp... */
> +};
> +
> +#endif /* __LINUX_MFD_MAX77714_H_ */
--
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog
Hi Lee,
On 21/10/21 20:43, Lee Jones wrote:
> On Tue, 19 Oct 2021, Luca Ceresoli wrote:
[...]
>> diff --git a/drivers/mfd/max77714.c b/drivers/mfd/max77714.c
>> new file mode 100644
>> index 000000000000..4b49d16fe199
>> --- /dev/null
>> +++ b/drivers/mfd/max77714.c
>> @@ -0,0 +1,165 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Maxim MAX77714 MFD Driver
>> + *
>> + * Copyright (C) 2021 Luca Ceresoli
>> + * Author: Luca Ceresoli <[email protected]>
>> + */
>> +
>> +#include <linux/i2c.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/mfd/core.h>
>> +#include <linux/mfd/max77714.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/regmap.h>
>> +
>> +struct max77714 {
>> + struct device *dev;
>> + struct regmap *regmap;
>> + struct regmap_irq_chip_data *irq_data;
>
> Is this used outside of .probe()?
No.
>> +/*
>> + * MAX77714 initially uses the internal, low precision oscillator. Enable
>> + * the external oscillator by setting the XOSC_RETRY bit. If the external
>> + * oscillator is not OK (probably not installed) this has no effect.
>> + */
>> +static int max77714_setup_xosc(struct max77714 *chip)
>
> May as well just pass 'dev' and 'regmap' to this function and do away
> with the superfluous struct along with all of it's memory management
> handling requirements.
Good idea!
>> +{
>> + /* Internal Crystal Load Capacitance, indexed by value of 32KLOAD bits */
>> + static const unsigned int load_cap[4] = {0, 10, 12, 22};
>
> Probably best to define these magic numbers.
Since these numbers do not appear anywhere else I don't find added value in:
#define MAX77714_LOAD_CAP_0 0
#define MAX77714_LOAD_CAP_10 10
#define MAX77714_LOAD_CAP_12 12
#define MAX77714_LOAD_CAP_22 22
[...]
static const unsigned int load_cap[4] = {
MAX77714_LOAD_CAP_0,
MAX77714_LOAD_CAP_10,
MAX77714_LOAD_CAP_12,
MAX77714_LOAD_CAP_12,
};
besides adding lots of lines and lots of "MAX77714_LOAD_CAP_". Even
worse, there is potential for copy-paste errors -- can you spot it? ;)
Finally, consider this is not even global but local to a small function.
But I'd rather add the unit ("pF") to either the comment line of the
array name (load_cap -> load_cap_pf) for clarity. Would that be OK for you?
Apart from this coding style topic I'm OK with all the other
improvements you proposed to this patch, all of them will be in v3.
Thank you for the detailed review!
--
Luca
On Wed, 27 Oct 2021, Luca Ceresoli wrote:
> Hi Lee,
>
> On 21/10/21 20:43, Lee Jones wrote:
> > On Tue, 19 Oct 2021, Luca Ceresoli wrote:
> [...]
> >> diff --git a/drivers/mfd/max77714.c b/drivers/mfd/max77714.c
> >> new file mode 100644
> >> index 000000000000..4b49d16fe199
> >> --- /dev/null
> >> +++ b/drivers/mfd/max77714.c
> >> @@ -0,0 +1,165 @@
> >> +// SPDX-License-Identifier: GPL-2.0-only
> >> +/*
> >> + * Maxim MAX77714 MFD Driver
> >> + *
> >> + * Copyright (C) 2021 Luca Ceresoli
> >> + * Author: Luca Ceresoli <[email protected]>
> >> + */
> >> +
> >> +#include <linux/i2c.h>
> >> +#include <linux/interrupt.h>
> >> +#include <linux/mfd/core.h>
> >> +#include <linux/mfd/max77714.h>
> >> +#include <linux/module.h>
> >> +#include <linux/of.h>
> >> +#include <linux/regmap.h>
> >> +
> >> +struct max77714 {
> >> + struct device *dev;
> >> + struct regmap *regmap;
> >> + struct regmap_irq_chip_data *irq_data;
> >
> > Is this used outside of .probe()?
>
> No.
Then you don't need to store it in a struct.
[...]
> >> + /* Internal Crystal Load Capacitance, indexed by value of 32KLOAD bits */
> >> + static const unsigned int load_cap[4] = {0, 10, 12, 22};
> >
> > Probably best to define these magic numbers.
>
> Since these numbers do not appear anywhere else I don't find added value in:
>
> #define MAX77714_LOAD_CAP_0 0
> #define MAX77714_LOAD_CAP_10 10
> #define MAX77714_LOAD_CAP_12 12
> #define MAX77714_LOAD_CAP_22 22
> [...]
> static const unsigned int load_cap[4] = {
> MAX77714_LOAD_CAP_0,
> MAX77714_LOAD_CAP_10,
> MAX77714_LOAD_CAP_12,
> MAX77714_LOAD_CAP_12,
> };
I don't find value in that nomenclature either! :)
I was suggesting that you used better, more forthcoming names.
LOAD_CAPACITANCE_00_pF
LOAD_CAPACITANCE_10_pF
LOAD_CAPACITANCE_12_pF
LOAD_CAPACITANCE_22_pF
> besides adding lots of lines and lots of "MAX77714_LOAD_CAP_". Even
> worse, there is potential for copy-paste errors -- can you spot it? ;)
Yes. Straight away.
> Finally, consider this is not even global but local to a small function.
>
> But I'd rather add the unit ("pF") to either the comment line of the
> array name (load_cap -> load_cap_pf) for clarity. Would that be OK for you?
I did have to read the code again to get a handle on things (probably
not a good sign). To keep things simple, just add "/* pF */" onto the
end of the load_cap line for now. That should clear things up at
first glance.
--
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog
Hi Lee,
On 27/10/21 15:44, Lee Jones wrote:
> On Wed, 27 Oct 2021, Luca Ceresoli wrote:
>
>> Hi Lee,
>>
>> On 21/10/21 20:43, Lee Jones wrote:
>>> On Tue, 19 Oct 2021, Luca Ceresoli wrote:
>> [...]
>>>> diff --git a/drivers/mfd/max77714.c b/drivers/mfd/max77714.c
>>>> new file mode 100644
>>>> index 000000000000..4b49d16fe199
>>>> --- /dev/null
>>>> +++ b/drivers/mfd/max77714.c
>>>> @@ -0,0 +1,165 @@
>>>> +// SPDX-License-Identifier: GPL-2.0-only
>>>> +/*
>>>> + * Maxim MAX77714 MFD Driver
>>>> + *
>>>> + * Copyright (C) 2021 Luca Ceresoli
>>>> + * Author: Luca Ceresoli <[email protected]>
>>>> + */
>>>> +
>>>> +#include <linux/i2c.h>
>>>> +#include <linux/interrupt.h>
>>>> +#include <linux/mfd/core.h>
>>>> +#include <linux/mfd/max77714.h>
>>>> +#include <linux/module.h>
>>>> +#include <linux/of.h>
>>>> +#include <linux/regmap.h>
>>>> +
>>>> +struct max77714 {
>>>> + struct device *dev;
>>>> + struct regmap *regmap;
>>>> + struct regmap_irq_chip_data *irq_data;
>>>
>>> Is this used outside of .probe()?
>>
>> No.
>
> Then you don't need to store it in a struct.
>
> [...]
>
>>>> + /* Internal Crystal Load Capacitance, indexed by value of 32KLOAD bits */
>>>> + static const unsigned int load_cap[4] = {0, 10, 12, 22};
>>>
>>> Probably best to define these magic numbers.
>>
>> Since these numbers do not appear anywhere else I don't find added value in:
>>
>> #define MAX77714_LOAD_CAP_0 0
>> #define MAX77714_LOAD_CAP_10 10
>> #define MAX77714_LOAD_CAP_12 12
>> #define MAX77714_LOAD_CAP_22 22
>> [...]
>> static const unsigned int load_cap[4] = {
>> MAX77714_LOAD_CAP_0,
>> MAX77714_LOAD_CAP_10,
>> MAX77714_LOAD_CAP_12,
>> MAX77714_LOAD_CAP_12,
>> };
>
> I don't find value in that nomenclature either! :)
>
> I was suggesting that you used better, more forthcoming names.
>
> LOAD_CAPACITANCE_00_pF
> LOAD_CAPACITANCE_10_pF
> LOAD_CAPACITANCE_12_pF
> LOAD_CAPACITANCE_22_pF
>
>> besides adding lots of lines and lots of "MAX77714_LOAD_CAP_". Even
>> worse, there is potential for copy-paste errors -- can you spot it? ;)
>
> Yes. Straight away.
>
>> Finally, consider this is not even global but local to a small function.
>>
>> But I'd rather add the unit ("pF") to either the comment line of the
>> array name (load_cap -> load_cap_pf) for clarity. Would that be OK for you?
>
> I did have to read the code again to get a handle on things (probably
> not a good sign). To keep things simple, just add "/* pF */" onto the
> end of the load_cap line for now. That should clear things up at
> first glance.
Ok, let's see how it works. I'm sending v3 in the next few days.
--
Luca