* [PATCH v4 0/6] Maxim Integrated MAX77759 PMIC MFD-based drivers
@ 2025-03-12 9:25 André Draszik
2025-03-12 9:25 ` [PATCH v4 1/6] dt-bindings: gpio: add max77759 binding André Draszik
` (5 more replies)
0 siblings, 6 replies; 12+ messages in thread
From: André Draszik @ 2025-03-12 9:25 UTC (permalink / raw)
To: Lee Jones, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Linus Walleij, Bartosz Golaszewski, Srinivas Kandagatla,
Kees Cook, Gustavo A. R. Silva
Cc: Peter Griffin, Tudor Ambarus, Will McVicker, kernel-team,
linux-kernel, devicetree, linux-gpio, linux-hardening,
André Draszik, Bartosz Golaszewski
Hi,
This series improves support for the Maxim Integrated MAX77759
companion PMIC for USB Type-C applications using the MFD framework.
This series must be applied in-order, due to interdependencies of some
of the patches:
* to avoid use of undocumented compatibles by the newly added drivers,
the bindings are added first in this series
* patch 1 ("dt-bindings: gpio: add max77759 binding") also creates a
new MAINTAINERS entry, including a wildcard match for the other
bindings in this series
* patch 3 ("dt-bindings: mfd: add max77759 binding") references the
bindings added in patch 1 and 2 and can not work if those aren't
available
* patch 4 ("mfd: max77759: add Maxim MAX77759 core mfd driver") adds
the core MFD driver, which also exposes an API to its leaf drivers
and is used by patches 5 and 6
* patches 5 and 6 won't compile without patch 4
The MAX77759 PMIC includes Battery Charger, Fuel Gauge, temperature
sensors, USB Type-C Port Controller (TCPC), NVMEM, and a GPIO expander.
This PMIC is used on the Google Pixel 6 and 6 Pro (oriole / raven).
This series adds support for the top-level MFD device, the gpio, and
nvmem cells. Other components are excluded for the following reasons:
While in the same package, Fuel Gauge and TCPC have separate and
independent I2C addresses, register maps, interrupt lines, and
aren't part of the top-level package interrupt hierarchy.
Furthermore, a driver for the TCPC part exists already (in
drivers/usb/typec/tcpm/tcpci_maxim_core.c).
I'm leaving out temperature sensors and charger in this submission,
because the former are not in use on Pixel 6 and I therefore can
not test them, and the latter can be added later, once we look at
the whole charging topic in more detail.
To make maintainers' work easier, I am planning to send the relevant
DTS and defconfig changes via a different series, unless everything
is expected to go via Lee's MFD tree in one series?
Cheers,
Andre'
Signed-off-by: André Draszik <andre.draszik@linaro.org>
---
Changes in v4:
- collect tags
- mfd: add missing build_bug.h include
- mfd: update an irq chip comment
- mfd: fix a whitespace in register definitions
- Link to v3: https://lore.kernel.org/r/20250228-max77759-mfd-v3-0-0c3627d42526@linaro.org
Changes in v3:
- collect tags
- mfd: drop gpio-controller and gpio-cells, GPIO is provided by the
child (Rob)
- gpio: drop duplicate init of 'handled' variable in irq handler
- gpio: use boolean with IRQ_RETVAL() (Linus)
- gpio: drop 'virq' variable inside irq handler to avoid confusion
(Linus)
- gpio: drop assignment of struct gpio_chip::owner (Linus)
- Link to v2: https://lore.kernel.org/r/20250226-max77759-mfd-v2-0-a65ebe2bc0a9@linaro.org
Changes in v2:
- reorder bindings patches to avoid validation failures
- add dependency information to cover letter (Krzysztof)
- fix max77759_gpio_direction_from_control() in gpio driver
- gpio: drop 'interrupts' property from binding and sort properties
alphabetically (Rob)
- nvmem: drop example from nvmem binding as the MFD binding has a
complete one (Rob)
- nvmem: rename expected nvmem subdev nodename to 'nvmem-0' (Rob)
- mfd: add kernel doc
- mfd: fix an msec / usec typo
- mfd: error handling of devm_mutex_init (Christophe)
- whitespace fixes & tidy-ups (Christophe)
- Link to v1: https://lore.kernel.org/r/20250224-max77759-mfd-v1-0-2bff36f9d055@linaro.org
---
André Draszik (6):
dt-bindings: gpio: add max77759 binding
dt-bindings: nvmem: add max77759 binding
dt-bindings: mfd: add max77759 binding
mfd: max77759: add Maxim MAX77759 core mfd driver
gpio: max77759: add Maxim MAX77759 gpio driver
nvmem: max77759: add Maxim MAX77759 NVMEM driver
.../bindings/gpio/maxim,max77759-gpio.yaml | 44 ++
.../devicetree/bindings/mfd/maxim,max77759.yaml | 99 +++
.../bindings/nvmem/maxim,max77759-nvmem.yaml | 32 +
MAINTAINERS | 10 +
drivers/gpio/Kconfig | 13 +
drivers/gpio/Makefile | 1 +
drivers/gpio/gpio-max77759.c | 524 +++++++++++++++
drivers/mfd/Kconfig | 20 +
drivers/mfd/Makefile | 1 +
drivers/mfd/max77759.c | 738 +++++++++++++++++++++
drivers/nvmem/Kconfig | 12 +
drivers/nvmem/Makefile | 2 +
drivers/nvmem/max77759-nvmem.c | 156 +++++
include/linux/mfd/max77759.h | 98 +++
14 files changed, 1750 insertions(+)
---
base-commit: 0226d0ce98a477937ed295fb7df4cc30b46fc304
change-id: 20250224-max77759-mfd-aaa7a3121b62
Best regards,
--
André Draszik <andre.draszik@linaro.org>
^ permalink raw reply [flat|nested] 12+ messages in thread* [PATCH v4 1/6] dt-bindings: gpio: add max77759 binding 2025-03-12 9:25 [PATCH v4 0/6] Maxim Integrated MAX77759 PMIC MFD-based drivers André Draszik @ 2025-03-12 9:25 ` André Draszik 2025-03-12 10:01 ` Bartosz Golaszewski 2025-03-12 9:26 ` [PATCH v4 2/6] dt-bindings: nvmem: " André Draszik ` (4 subsequent siblings) 5 siblings, 1 reply; 12+ messages in thread From: André Draszik @ 2025-03-12 9:25 UTC (permalink / raw) To: Lee Jones, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Linus Walleij, Bartosz Golaszewski, Srinivas Kandagatla, Kees Cook, Gustavo A. R. Silva Cc: Peter Griffin, Tudor Ambarus, Will McVicker, kernel-team, linux-kernel, devicetree, linux-gpio, linux-hardening, André Draszik The Maxim MAX77759 is a companion PMIC for USB Type-C applications and includes Battery Charger, Fuel Gauge, temperature sensors, USB Type-C Port Controller (TCPC), NVMEM, and a GPIO expander. This describes its GPIO module. Reviewed-by: Rob Herring (Arm) <robh@kernel.org> Signed-off-by: André Draszik <andre.draszik@linaro.org> --- v2: * drop 'interrupts' property and sort properties alphabetically --- .../bindings/gpio/maxim,max77759-gpio.yaml | 44 ++++++++++++++++++++++ MAINTAINERS | 6 +++ 2 files changed, 50 insertions(+) diff --git a/Documentation/devicetree/bindings/gpio/maxim,max77759-gpio.yaml b/Documentation/devicetree/bindings/gpio/maxim,max77759-gpio.yaml new file mode 100644 index 0000000000000000000000000000000000000000..55734190d5ebdbc351e1f91675dddd8a9db80cd7 --- /dev/null +++ b/Documentation/devicetree/bindings/gpio/maxim,max77759-gpio.yaml @@ -0,0 +1,44 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/gpio/maxim,max77759-gpio.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Maxim Integrated MAX77759 GPIO + +maintainers: + - André Draszik <andre.draszik@linaro.org> + +description: | + This module is part of the MAX77759 PMIC. For additional information, see + Documentation/devicetree/bindings/mfd/maxim,max77759.yaml. + + The MAX77759 is a PMIC integrating, amongst others, a GPIO controller + including interrupt support for 2 GPIO lines. + +properties: + compatible: + const: maxim,max77759-gpio + + "#gpio-cells": + const: 2 + + gpio-controller: true + + gpio-line-names: + minItems: 1 + maxItems: 2 + + "#interrupt-cells": + const: 2 + + interrupt-controller: true + +required: + - compatible + - "#gpio-cells" + - gpio-controller + - "#interrupt-cells" + - interrupt-controller + +additionalProperties: false diff --git a/MAINTAINERS b/MAINTAINERS index 86fa62154d3b23bf84ce2d53cbbdc33f3d43006e..a45d1bd64d116d14bc05a64fa3da852a41e1de7f 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -14347,6 +14347,12 @@ F: Documentation/devicetree/bindings/mfd/maxim,max77714.yaml F: drivers/mfd/max77714.c F: include/linux/mfd/max77714.h +MAXIM MAX77759 PMIC MFD DRIVER +M: André Draszik <andre.draszik@linaro.org> +L: linux-kernel@vger.kernel.org +S: Maintained +F: Documentation/devicetree/bindings/*/maxim,max77759*.yaml + MAXIM MAX77802 PMIC REGULATOR DEVICE DRIVER M: Javier Martinez Canillas <javier@dowhile0.org> L: linux-kernel@vger.kernel.org -- 2.49.0.rc0.332.g42c0ae87b1-goog ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v4 1/6] dt-bindings: gpio: add max77759 binding 2025-03-12 9:25 ` [PATCH v4 1/6] dt-bindings: gpio: add max77759 binding André Draszik @ 2025-03-12 10:01 ` Bartosz Golaszewski 0 siblings, 0 replies; 12+ messages in thread From: Bartosz Golaszewski @ 2025-03-12 10:01 UTC (permalink / raw) To: André Draszik Cc: Lee Jones, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Linus Walleij, Srinivas Kandagatla, Kees Cook, Gustavo A. R. Silva, Peter Griffin, Tudor Ambarus, Will McVicker, kernel-team, linux-kernel, devicetree, linux-gpio, linux-hardening On Wed, Mar 12, 2025 at 10:26 AM André Draszik <andre.draszik@linaro.org> wrote: > > The Maxim MAX77759 is a companion PMIC for USB Type-C applications and > includes Battery Charger, Fuel Gauge, temperature sensors, USB Type-C > Port Controller (TCPC), NVMEM, and a GPIO expander. > > This describes its GPIO module. > > Reviewed-by: Rob Herring (Arm) <robh@kernel.org> > Signed-off-by: André Draszik <andre.draszik@linaro.org> > --- Acked-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v4 2/6] dt-bindings: nvmem: add max77759 binding 2025-03-12 9:25 [PATCH v4 0/6] Maxim Integrated MAX77759 PMIC MFD-based drivers André Draszik 2025-03-12 9:25 ` [PATCH v4 1/6] dt-bindings: gpio: add max77759 binding André Draszik @ 2025-03-12 9:26 ` André Draszik 2025-03-12 9:26 ` [PATCH v4 3/6] dt-bindings: mfd: " André Draszik ` (3 subsequent siblings) 5 siblings, 0 replies; 12+ messages in thread From: André Draszik @ 2025-03-12 9:26 UTC (permalink / raw) To: Lee Jones, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Linus Walleij, Bartosz Golaszewski, Srinivas Kandagatla, Kees Cook, Gustavo A. R. Silva Cc: Peter Griffin, Tudor Ambarus, Will McVicker, kernel-team, linux-kernel, devicetree, linux-gpio, linux-hardening, André Draszik The Maxim MAX77759 is a companion PMIC for USB Type-C applications and includes Battery Charger, Fuel Gauge, temperature sensors, USB Type-C Port Controller (TCPC), NVMEM, and a GPIO expander. This describes its storage module (NVMEM). Reviewed-by: Rob Herring (Arm) <robh@kernel.org> Signed-off-by: André Draszik <andre.draszik@linaro.org> --- v2: * drop example as the MFD binding has a complete one (Rob) Note: MAINTAINERS doesn't need updating, the binding update for the first leaf device (gpio) adds a wildcard matching all max77759 bindings --- .../bindings/nvmem/maxim,max77759-nvmem.yaml | 32 ++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/Documentation/devicetree/bindings/nvmem/maxim,max77759-nvmem.yaml b/Documentation/devicetree/bindings/nvmem/maxim,max77759-nvmem.yaml new file mode 100644 index 0000000000000000000000000000000000000000..1e3bd4433007341a11040f513bf444866b9e38a8 --- /dev/null +++ b/Documentation/devicetree/bindings/nvmem/maxim,max77759-nvmem.yaml @@ -0,0 +1,32 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/nvmem/maxim,max77759-nvmem.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Maxim Integrated MAX77759 Non Volatile Memory + +maintainers: + - André Draszik <andre.draszik@linaro.org> + +description: | + This module is part of the MAX77759 PMIC. For additional information, see + Documentation/devicetree/bindings/mfd/maxim,max77759.yaml. + + The MAX77759 is a PMIC integrating, amongst others, Non Volatile Memory + (NVMEM) with 30 bytes of storage which can be used by software to store + information or communicate with a boot loader. + +properties: + compatible: + const: maxim,max77759-nvmem + + wp-gpios: false + +required: + - compatible + +allOf: + - $ref: nvmem.yaml# + +unevaluatedProperties: false -- 2.49.0.rc0.332.g42c0ae87b1-goog ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v4 3/6] dt-bindings: mfd: add max77759 binding 2025-03-12 9:25 [PATCH v4 0/6] Maxim Integrated MAX77759 PMIC MFD-based drivers André Draszik 2025-03-12 9:25 ` [PATCH v4 1/6] dt-bindings: gpio: add max77759 binding André Draszik 2025-03-12 9:26 ` [PATCH v4 2/6] dt-bindings: nvmem: " André Draszik @ 2025-03-12 9:26 ` André Draszik 2025-03-12 9:26 ` [PATCH v4 4/6] mfd: max77759: add Maxim MAX77759 core mfd driver André Draszik ` (2 subsequent siblings) 5 siblings, 0 replies; 12+ messages in thread From: André Draszik @ 2025-03-12 9:26 UTC (permalink / raw) To: Lee Jones, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Linus Walleij, Bartosz Golaszewski, Srinivas Kandagatla, Kees Cook, Gustavo A. R. Silva Cc: Peter Griffin, Tudor Ambarus, Will McVicker, kernel-team, linux-kernel, devicetree, linux-gpio, linux-hardening, André Draszik The Maxim MAX77759 is a companion PMIC for USB Type-C applications and includes Battery Charger, Fuel Gauge, temperature sensors, USB Type-C Port Controller (TCPC), NVMEM, and a GPIO expander. This describes the top-level device. Reviewed-by: Rob Herring (Arm) <robh@kernel.org> Signed-off-by: André Draszik <andre.draszik@linaro.org> --- v3: * drop gpio-controller and gpio-cells, GPIO is provided by the child (Rob) v2: * rename expected nvmem subdev nodename to 'nvmem-0' I'd have preferred just 'nvmem', but that matches nvmem-consumer.yaml and fails validation. Note: MAINTAINERS doesn't need updating, the binding update for the first leaf device (gpio) adds a wildcard matching all max77759 bindings --- .../devicetree/bindings/mfd/maxim,max77759.yaml | 99 ++++++++++++++++++++++ 1 file changed, 99 insertions(+) diff --git a/Documentation/devicetree/bindings/mfd/maxim,max77759.yaml b/Documentation/devicetree/bindings/mfd/maxim,max77759.yaml new file mode 100644 index 0000000000000000000000000000000000000000..525de9ab3c2b7b431e48497330640857540625b1 --- /dev/null +++ b/Documentation/devicetree/bindings/mfd/maxim,max77759.yaml @@ -0,0 +1,99 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/mfd/maxim,max77759.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Maxim Integrated MAX77759 PMIC for USB Type-C applications + +maintainers: + - André Draszik <andre.draszik@linaro.org> + +description: | + This is a part of device tree bindings for the MAX77759 companion Power + Management IC for USB Type-C applications. + + The MAX77759 includes Battery Charger, Fuel Gauge, temperature sensors, USB + Type-C Port Controller (TCPC), NVMEM, and a GPIO expander. + +properties: + compatible: + const: maxim,max77759 + + interrupts: + maxItems: 1 + + interrupt-controller: true + + "#interrupt-cells": + const: 2 + + reg: + maxItems: 1 + + gpio: + $ref: /schemas/gpio/maxim,max77759-gpio.yaml + + nvmem-0: + $ref: /schemas/nvmem/maxim,max77759-nvmem.yaml + +required: + - compatible + - interrupts + - reg + +additionalProperties: false + +examples: + - | + #include <dt-bindings/interrupt-controller/irq.h> + + i2c { + #address-cells = <1>; + #size-cells = <0>; + + pmic@66 { + compatible = "maxim,max77759"; + reg = <0x66>; + interrupts-extended = <&gpa8 3 IRQ_TYPE_LEVEL_LOW>; + + interrupt-controller; + #interrupt-cells = <2>; + + gpio { + compatible = "maxim,max77759-gpio"; + + gpio-controller; + #gpio-cells = <2>; + + interrupt-controller; + #interrupt-cells = <2>; + }; + + nvmem-0 { + compatible = "maxim,max77759-nvmem"; + + nvmem-layout { + compatible = "fixed-layout"; + #address-cells = <1>; + #size-cells = <1>; + + reboot-mode@0 { + reg = <0x0 0x4>; + }; + + boot-reason@4 { + reg = <0x4 0x4>; + }; + + shutdown-user-flag@8 { + reg = <0x8 0x1>; + }; + + rsoc@10 { + reg = <0xa 0x2>; + }; + }; + }; + }; + }; -- 2.49.0.rc0.332.g42c0ae87b1-goog ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v4 4/6] mfd: max77759: add Maxim MAX77759 core mfd driver 2025-03-12 9:25 [PATCH v4 0/6] Maxim Integrated MAX77759 PMIC MFD-based drivers André Draszik ` (2 preceding siblings ...) 2025-03-12 9:26 ` [PATCH v4 3/6] dt-bindings: mfd: " André Draszik @ 2025-03-12 9:26 ` André Draszik 2025-03-14 12:31 ` Lee Jones 2025-03-12 9:26 ` [PATCH v4 5/6] gpio: max77759: add Maxim MAX77759 gpio driver André Draszik 2025-03-12 9:26 ` [PATCH v4 6/6] nvmem: max77759: add Maxim MAX77759 NVMEM driver André Draszik 5 siblings, 1 reply; 12+ messages in thread From: André Draszik @ 2025-03-12 9:26 UTC (permalink / raw) To: Lee Jones, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Linus Walleij, Bartosz Golaszewski, Srinivas Kandagatla, Kees Cook, Gustavo A. R. Silva Cc: Peter Griffin, Tudor Ambarus, Will McVicker, kernel-team, linux-kernel, devicetree, linux-gpio, linux-hardening, André Draszik The Maxim MAX77759 is a companion PMIC for USB Type-C applications and includes Battery Charger, Fuel Gauge, temperature sensors, USB Type-C Port Controller (TCPC), NVMEM, and a GPIO expander. Fuel Gauge and TCPC have separate and independent I2C addresses, register maps, and interrupt lines and are therefore excluded from the MFD core device driver here. The GPIO and NVMEM interfaces are accessed via specific commands to the built-in microprocessor. This driver implements an API that client drivers can use for accessing those. Signed-off-by: André Draszik <andre.draszik@linaro.org> --- v4: * add missing build_bug.h include * update an irq chip comment * fix a whitespace in register definitions v2: * add kernel doc for max77759_maxq_command() and related structs * fix an msec / usec typo * add missing error handling of devm_mutex_init() (Christophe) * align sentinel in max77759_of_id[] with max77759_i2c_id[] (Christophe) * some tidy-ups in max77759_maxq_command() (Christophe) --- MAINTAINERS | 2 + drivers/mfd/Kconfig | 20 ++ drivers/mfd/Makefile | 1 + drivers/mfd/max77759.c | 738 +++++++++++++++++++++++++++++++++++++++++++ include/linux/mfd/max77759.h | 98 ++++++ 5 files changed, 859 insertions(+) diff --git a/MAINTAINERS b/MAINTAINERS index a45d1bd64d116d14bc05a64fa3da852a41e1de7f..38844ea24e464d0b58f8852b79e2b94f18d48998 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -14352,6 +14352,8 @@ M: André Draszik <andre.draszik@linaro.org> L: linux-kernel@vger.kernel.org S: Maintained F: Documentation/devicetree/bindings/*/maxim,max77759*.yaml +F: drivers/mfd/max77759.c +F: include/linux/mfd/max77759.h MAXIM MAX77802 PMIC REGULATOR DEVICE DRIVER M: Javier Martinez Canillas <javier@dowhile0.org> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig index d44c69bb3dfd52d53fe26aa2d5e5ace346448f57..1d72bf086401064608cc41e9f9a0af044c6df68e 100644 --- a/drivers/mfd/Kconfig +++ b/drivers/mfd/Kconfig @@ -930,6 +930,26 @@ config MFD_MAX77714 drivers must be enabled in order to use each functionality of the device. +config MFD_MAX77759 + tristate "Maxim Integrated MAX77759 PMIC" + depends on I2C + depends on OF + select IRQ_DOMAIN + select MFD_CORE + select REGMAP_I2C + select REGMAP_IRQ + help + Say yes here to add support for Maxim Integrated MAX77759. + This is a companion Power Management IC for USB Type-C applications + with Battery Charger, Fuel Gauge, temperature sensors, USB Type-C + Port Controller (TCPC), NVMEM, and additional GPIO interfaces. + This driver provides common support for accessing the device; + additional drivers must be enabled in order to use the functionality + of the device. + + To compile this driver as a module, choose M here: the module will be + called max77759. + config MFD_MAX77843 bool "Maxim Semiconductor MAX77843 PMIC Support" depends on I2C=y diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile index 9220eaf7cf1255a8922430fe3e50e41771bbaa60..cc9362afd8f060d64ca0f0f028a0d7cfe9cfe512 100644 --- a/drivers/mfd/Makefile +++ b/drivers/mfd/Makefile @@ -169,6 +169,7 @@ 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_MAX77759) += max77759.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/max77759.c b/drivers/mfd/max77759.c new file mode 100644 index 0000000000000000000000000000000000000000..a4f1616d600bde250a73b69464ada1fbf6d7e67e --- /dev/null +++ b/drivers/mfd/max77759.c @@ -0,0 +1,738 @@ +// SPDX-License-Identifier: GPL-2.0-only +// +// Copyright 2020 Google Inc +// Copyright 2025 Linaro Ltd. +// +// Core MFD driver for Maxim MAX77759 companion PMIC for USB Type-C + +#include <linux/array_size.h> +#include <linux/bitfield.h> +#include <linux/bits.h> +#include <linux/build_bug.h> +#include <linux/cleanup.h> +#include <linux/completion.h> +#include <linux/dev_printk.h> +#include <linux/device.h> +#include <linux/err.h> +#include <linux/i2c.h> +#include <linux/init.h> +#include <linux/interrupt.h> +#include <linux/irq.h> +#include <linux/jiffies.h> +#include <linux/mfd/core.h> +#include <linux/mfd/max77759.h> +#include <linux/mod_devicetable.h> +#include <linux/module.h> +#include <linux/mutex.h> +#include <linux/of.h> +#include <linux/overflow.h> +#include <linux/regmap.h> + +/* registers - registers useful to drivers are declared in the public header */ +/* PMIC / TOP */ +#define MAX77759_PMIC_REG_PMIC_ID 0x00 +#define MAX77759_PMIC_REG_PMIC_ID_MAX77759 59 + +#define MAX77759_PMIC_REG_PMIC_REVISION 0x01 +#define MAX77759_PMIC_REG_OTP_REVISION 0x02 + +#define MAX77759_PMIC_REG_INTSRC 0x22 +#define MAX77759_PMIC_REG_INTSRCMASK 0x23 +#define MAX77759_PMIC_REG_INTSRC_MAXQ BIT(3) +#define MAX77759_PMIC_REG_INTSRC_TOPSYS BIT(1) +#define MAX77759_PMIC_REG_INTSRC_CHGR BIT(0) + +#define MAX77759_PMIC_REG_TOPSYS_INT 0x24 +#define MAX77759_PMIC_REG_TOPSYS_INT_MASK 0x26 +#define MAX77759_PMIC_REG_TOPSYS_INT_TSHDN BIT(6) +#define MAX77759_PMIC_REG_TOPSYS_INT_SYSOVLO BIT(5) +#define MAX77759_PMIC_REG_TOPSYS_INT_SYSUVLO BIT(4) +#define MAX77759_PMIC_REG_TOPSYS_INT_FSHIP BIT(0) + +#define MAX77759_PMIC_REG_I2C_CNFG 0x40 +#define MAX77759_PMIC_REG_SWRESET 0x50 +#define MAX77759_PMIC_REG_CONTROL_FG 0x51 +#define MAX77759_PMIC_REG_LAST_REGISTER MAX77759_PMIC_REG_CONTROL_FG + +/* MaxQ */ +#define MAX77759_MAXQ_REG_AP_DATAOUT0 0x81 +#define MAX77759_MAXQ_REG_AP_DATAOUT32 0xa1 +#define MAX77759_MAXQ_REG_AP_MESSAGESZ_MAX (MAX77759_MAXQ_REG_AP_DATAOUT32 \ + - MAX77759_MAXQ_REG_AP_DATAOUT0 \ + + 1) +#define MAX77759_MAXQ_REG_AP_DATAIN0 0xb1 +#define MAX77759_MAXQ_REG_LAST_REGISTER 0xe0 + +/* charger */ +#define MAX77759_CHGR_REG_LAST_REGISTER 0xcc + +enum max77759_i2c_subdev_id { + MAX77759_I2C_SUBDEV_ID_MAXQ, + MAX77759_I2C_SUBDEV_ID_CHARGER, +}; + +struct max77759_mfd { + /* protecting MaxQ commands - only one can be active */ + struct mutex maxq_lock; + struct regmap *regmap_maxq; + struct completion cmd_done; + + struct regmap *regmap_top; + struct regmap *regmap_charger; +}; + +struct max77759_i2c_subdev { + enum max77759_i2c_subdev_id id; + const struct regmap_config *cfg; + u16 i2c_address; +}; + +/* TOP registers */ +static const struct regmap_range max77759_top_registers[] = { + regmap_reg_range(0x00, 0x02), + regmap_reg_range(0x22, 0x24), + regmap_reg_range(0x26, 0x26), + regmap_reg_range(0x40, 0x40), + regmap_reg_range(0x50, 0x51), +}; + +static const struct regmap_range max77759_top_ro_registers[] = { + regmap_reg_range(0x00, 0x02), + regmap_reg_range(0x22, 0x22), +}; + +static const struct regmap_range max77759_top_volatile_registers[] = { + regmap_reg_range(0x22, 0x22), + regmap_reg_range(0x24, 0x24), +}; + +static const struct regmap_access_table max77759_top_wr_table = { + .yes_ranges = max77759_top_registers, + .n_yes_ranges = ARRAY_SIZE(max77759_top_registers), + .no_ranges = max77759_top_ro_registers, + .n_no_ranges = ARRAY_SIZE(max77759_top_ro_registers), +}; + +static const struct regmap_access_table max77759_top_rd_table = { + .yes_ranges = max77759_top_registers, + .n_yes_ranges = ARRAY_SIZE(max77759_top_registers), +}; + +static const struct regmap_access_table max77759_top_volatile_table = { + .yes_ranges = max77759_top_volatile_registers, + .n_yes_ranges = ARRAY_SIZE(max77759_top_volatile_registers), +}; + +static const struct regmap_config max77759_regmap_config_top = { + .name = "top", + .reg_bits = 8, + .val_bits = 8, + .max_register = MAX77759_PMIC_REG_LAST_REGISTER, + .wr_table = &max77759_top_wr_table, + .rd_table = &max77759_top_rd_table, + .volatile_table = &max77759_top_volatile_table, + .num_reg_defaults_raw = MAX77759_PMIC_REG_LAST_REGISTER + 1, + .cache_type = REGCACHE_MAPLE, +}; + +/* MaxQ registers */ +static const struct regmap_range max77759_maxq_registers[] = { + regmap_reg_range(0x60, 0x73), + regmap_reg_range(0x81, 0xa1), + regmap_reg_range(0xb1, 0xd1), + regmap_reg_range(0xe0, 0xe0), +}; + +static const struct regmap_range max77759_maxq_ro_registers[] = { + regmap_reg_range(0x60, 0x63), + regmap_reg_range(0x68, 0x6f), + regmap_reg_range(0xb1, 0xd1), +}; + +static const struct regmap_range max77759_maxq_volatile_registers[] = { + regmap_reg_range(0x64, 0x6f), + regmap_reg_range(0xb1, 0xd1), + regmap_reg_range(0xe0, 0xe0), +}; + +static const struct regmap_access_table max77759_maxq_wr_table = { + .yes_ranges = max77759_maxq_registers, + .n_yes_ranges = ARRAY_SIZE(max77759_maxq_registers), + .no_ranges = max77759_maxq_ro_registers, + .n_no_ranges = ARRAY_SIZE(max77759_maxq_ro_registers), +}; + +static const struct regmap_access_table max77759_maxq_rd_table = { + .yes_ranges = max77759_maxq_registers, + .n_yes_ranges = ARRAY_SIZE(max77759_maxq_registers), +}; + +static const struct regmap_access_table max77759_maxq_volatile_table = { + .yes_ranges = max77759_maxq_volatile_registers, + .n_yes_ranges = ARRAY_SIZE(max77759_maxq_volatile_registers), +}; + +static const struct regmap_config max77759_regmap_config_maxq = { + .name = "maxq", + .reg_bits = 8, + .val_bits = 8, + .max_register = MAX77759_MAXQ_REG_LAST_REGISTER, + .wr_table = &max77759_maxq_wr_table, + .rd_table = &max77759_maxq_rd_table, + .volatile_table = &max77759_maxq_volatile_table, + .num_reg_defaults_raw = MAX77759_MAXQ_REG_LAST_REGISTER + 1, + .cache_type = REGCACHE_MAPLE, +}; + +/* charger registers */ +static const struct regmap_range max77759_charger_registers[] = { + regmap_reg_range(0xb0, 0xcc), +}; + +static const struct regmap_range max77759_charger_ro_registers[] = { + regmap_reg_range(0xb4, 0xb8), +}; + +static const struct regmap_range max77759_charger_volatile_registers[] = { + regmap_reg_range(0xb0, 0xb1), + regmap_reg_range(0xb4, 0xb8), +}; + +static const struct regmap_access_table max77759_charger_wr_table = { + .yes_ranges = max77759_charger_registers, + .n_yes_ranges = ARRAY_SIZE(max77759_charger_registers), + .no_ranges = max77759_charger_ro_registers, + .n_no_ranges = ARRAY_SIZE(max77759_charger_ro_registers), +}; + +static const struct regmap_access_table max77759_charger_rd_table = { + .yes_ranges = max77759_charger_registers, + .n_yes_ranges = ARRAY_SIZE(max77759_charger_registers), +}; + +static const struct regmap_access_table max77759_charger_volatile_table = { + .yes_ranges = max77759_charger_volatile_registers, + .n_yes_ranges = ARRAY_SIZE(max77759_charger_volatile_registers), +}; + +static const struct regmap_config max77759_regmap_config_charger = { + .name = "charger", + .reg_bits = 8, + .val_bits = 8, + .max_register = MAX77759_CHGR_REG_LAST_REGISTER, + .wr_table = &max77759_charger_wr_table, + .rd_table = &max77759_charger_rd_table, + .volatile_table = &max77759_charger_volatile_table, + .num_reg_defaults_raw = MAX77759_CHGR_REG_LAST_REGISTER + 1, + .cache_type = REGCACHE_MAPLE, +}; + +/* + * Interrupts - with the following interrupt hierarchy: + * pmic IRQs (INTSRC) + * - MAXQ_INT: MaxQ IRQs + * - UIC_INT1 + * - APCmdResI + * - SysMsgI + * - GPIOxI + * - TOPSYS_INT: topsys + * - TOPSYS_INT + * - TSHDN_INT + * - SYSOVLO_INT + * - SYSUVLO_INT + * - FSHIP_NOT_RD + * - CHGR_INT: charger + * - CHG_INT + * - CHG_INT2 + */ +enum { + MAX77759_INT_MAXQ, + MAX77759_INT_TOPSYS, + MAX77759_INT_CHGR, +}; + +enum { + MAX77759_TOPSYS_INT_TSHDN, + MAX77759_TOPSYS_INT_SYSOVLO, + MAX77759_TOPSYS_INT_SYSUVLO, + MAX77759_TOPSYS_INT_FSHIP_NOT_RD, +}; + +enum { + MAX77759_MAXQ_INT_APCMDRESI, + MAX77759_MAXQ_INT_SYSMSGI, + MAX77759_MAXQ_INT_GPIO, + MAX77759_MAXQ_INT_UIC1, + MAX77759_MAXQ_INT_UIC2, + MAX77759_MAXQ_INT_UIC3, + MAX77759_MAXQ_INT_UIC4, +}; + +enum { + MAX77759_CHARGER_INT_1, + MAX77759_CHARGER_INT_2, +}; + +static const struct regmap_irq max77759_pmic_irqs[] = { + REGMAP_IRQ_REG(MAX77759_INT_MAXQ, 0, MAX77759_PMIC_REG_INTSRC_MAXQ), + REGMAP_IRQ_REG(MAX77759_INT_TOPSYS, 0, MAX77759_PMIC_REG_INTSRC_TOPSYS), + REGMAP_IRQ_REG(MAX77759_INT_CHGR, 0, MAX77759_PMIC_REG_INTSRC_CHGR), +}; + +static const struct regmap_irq max77759_maxq_irqs[] = { + REGMAP_IRQ_REG(MAX77759_MAXQ_INT_APCMDRESI, + 0, MAX77759_MAXQ_REG_UIC_INT1_APCMDRESI), + REGMAP_IRQ_REG(MAX77759_MAXQ_INT_SYSMSGI, + 0, MAX77759_MAXQ_REG_UIC_INT1_SYSMSGI), + REGMAP_IRQ_REG(MAX77759_MAXQ_INT_GPIO, 0, GENMASK(1, 0)), + REGMAP_IRQ_REG(MAX77759_MAXQ_INT_UIC1, 0, GENMASK(5, 2)), + REGMAP_IRQ_REG(MAX77759_MAXQ_INT_UIC2, 1, GENMASK(7, 0)), + REGMAP_IRQ_REG(MAX77759_MAXQ_INT_UIC3, 2, GENMASK(7, 0)), + REGMAP_IRQ_REG(MAX77759_MAXQ_INT_UIC4, 3, GENMASK(7, 0)), +}; + +static const struct regmap_irq max77759_topsys_irqs[] = { + REGMAP_IRQ_REG(MAX77759_TOPSYS_INT_TSHDN, + 0, MAX77759_PMIC_REG_TOPSYS_INT_TSHDN), + REGMAP_IRQ_REG(MAX77759_TOPSYS_INT_SYSOVLO, + 0, MAX77759_PMIC_REG_TOPSYS_INT_SYSOVLO), + REGMAP_IRQ_REG(MAX77759_TOPSYS_INT_SYSUVLO, + 0, MAX77759_PMIC_REG_TOPSYS_INT_SYSUVLO), + REGMAP_IRQ_REG(MAX77759_TOPSYS_INT_FSHIP_NOT_RD, + 0, MAX77759_PMIC_REG_TOPSYS_INT_FSHIP), +}; + +static const struct regmap_irq max77759_chgr_irqs[] = { + REGMAP_IRQ_REG(MAX77759_CHARGER_INT_1, 0, GENMASK(7, 0)), + REGMAP_IRQ_REG(MAX77759_CHARGER_INT_2, 1, GENMASK(7, 0)), +}; + +static const struct regmap_irq_chip max77759_pmic_irq_chip = { + .name = "max77759-pmic", + /* INTSRC is read-only and doesn't require clearing */ + .status_base = MAX77759_PMIC_REG_INTSRC, + .mask_base = MAX77759_PMIC_REG_INTSRCMASK, + .num_regs = 1, + .irqs = max77759_pmic_irqs, + .num_irqs = ARRAY_SIZE(max77759_pmic_irqs), +}; + +/* + * We can let regmap-irq auto-ack the topsys interrupt bits as required, but + * for all others the individual drivers need to know which interrupt bit + * exactly is set inside their interrupt handlers, and therefore we can not set + * .ack_base for those. + */ +static const struct regmap_irq_chip max77759_maxq_irq_chip = { + .name = "max77759-maxq", + .domain_suffix = "MAXQ", + .status_base = MAX77759_MAXQ_REG_UIC_INT1, + .mask_base = MAX77759_MAXQ_REG_UIC_INT1_M, + .num_regs = 4, + .irqs = max77759_maxq_irqs, + .num_irqs = ARRAY_SIZE(max77759_maxq_irqs), +}; + +static const struct regmap_irq_chip max77759_topsys_irq_chip = { + .name = "max77759-topsys", + .domain_suffix = "TOPSYS", + .status_base = MAX77759_PMIC_REG_TOPSYS_INT, + .mask_base = MAX77759_PMIC_REG_TOPSYS_INT_MASK, + .ack_base = MAX77759_PMIC_REG_TOPSYS_INT, + .num_regs = 1, + .irqs = max77759_topsys_irqs, + .num_irqs = ARRAY_SIZE(max77759_topsys_irqs), +}; + +static const struct regmap_irq_chip max77759_chrg_irq_chip = { + .name = "max77759-chgr", + .domain_suffix = "CHGR", + .status_base = MAX77759_CHGR_REG_CHG_INT, + .mask_base = MAX77759_CHGR_REG_CHG_INT_MASK, + .num_regs = 2, + .irqs = max77759_chgr_irqs, + .num_irqs = ARRAY_SIZE(max77759_chgr_irqs), +}; + +static const struct max77759_i2c_subdev max77759_i2c_subdevs[] = { + { + .id = MAX77759_I2C_SUBDEV_ID_MAXQ, + /* I2C address is same as top's */ + .cfg = &max77759_regmap_config_maxq, + }, + { + .id = MAX77759_I2C_SUBDEV_ID_CHARGER, + .cfg = &max77759_regmap_config_charger, + .i2c_address = 0x69, + }, +}; + +static const struct resource max77759_gpio_resources[] = { + DEFINE_RES_IRQ_NAMED(MAX77759_MAXQ_INT_GPIO, "GPI"), +}; + +static const struct resource max77759_charger_resources[] = { + DEFINE_RES_IRQ_NAMED(MAX77759_CHARGER_INT_1, "INT1"), + DEFINE_RES_IRQ_NAMED(MAX77759_CHARGER_INT_2, "INT2"), +}; + +static const struct mfd_cell max77759_cells[] = { + MFD_CELL_OF("max77759-nvmem", NULL, NULL, 0, 0, + "maxim,max77759-nvmem"), +}; + +static const struct mfd_cell max77759_maxq_cells[] = { + MFD_CELL_OF("max77759-gpio", max77759_gpio_resources, NULL, 0, 0, + "maxim,max77759-gpio"), +}; + +static const struct mfd_cell max77759_charger_cells[] = { + MFD_CELL_RES("max77759-charger", max77759_charger_resources), +}; + +int max77759_maxq_command(struct max77759_mfd *max77759_mfd, + const struct max77759_maxq_command *cmd, + struct max77759_maxq_response *rsp) +{ + DEFINE_FLEX(struct max77759_maxq_response, _rsp, rsp, length, 1); + int ret; + struct device *dev = regmap_get_device(max77759_mfd->regmap_maxq); + static const unsigned int timeout_ms = 200; + + if (cmd->length > MAX77759_MAXQ_REG_AP_MESSAGESZ_MAX) + return -EINVAL; + + /* rsp is allowed to be NULL. In that case we do need a temporary. */ + if (!rsp) + rsp = _rsp; + + BUILD_BUG_ON(MAX77759_MAXQ_OPCODE_MAXLENGTH + != MAX77759_MAXQ_REG_AP_MESSAGESZ_MAX); + if (!rsp->length || rsp->length > MAX77759_MAXQ_REG_AP_MESSAGESZ_MAX) + return -EINVAL; + + guard(mutex)(&max77759_mfd->maxq_lock); + + reinit_completion(&max77759_mfd->cmd_done); + + /* write the opcode and data */ + ret = regmap_bulk_write(max77759_mfd->regmap_maxq, + MAX77759_MAXQ_REG_AP_DATAOUT0, cmd->cmd, + cmd->length); + if (!ret && cmd->length < MAX77759_MAXQ_REG_AP_MESSAGESZ_MAX) + /* writing the last byte triggers MaxQ */ + ret = regmap_write(max77759_mfd->regmap_maxq, + MAX77759_MAXQ_REG_AP_DATAOUT32, 0); + if (ret) { + dev_warn(dev, "write data failed: %d\n", ret); + return ret; + } + + /* wait for response from MaxQ */ + if (!wait_for_completion_timeout(&max77759_mfd->cmd_done, + msecs_to_jiffies(timeout_ms))) { + dev_err(dev, "timed out waiting for data\n"); + return -ETIMEDOUT; + } + + ret = regmap_bulk_read(max77759_mfd->regmap_maxq, + MAX77759_MAXQ_REG_AP_DATAIN0, + rsp->rsp, rsp->length); + if (ret) { + dev_warn(dev, "read data failed: %d\n", ret); + return ret; + } + + /* + * As per the protocol, the first byte of the reply will match the + * request. + */ + if (cmd->cmd[0] != rsp->rsp[0]) { + dev_warn(dev, "unexpected opcode response for %#.2x: %*ph\n", + cmd->cmd[0], (int)rsp->length, rsp->rsp); + return -EIO; + } + + return 0; +} +EXPORT_SYMBOL_GPL(max77759_maxq_command); + +static irqreturn_t apcmdres_irq_handler(int irq, void *irq_data) +{ + struct max77759_mfd *max77759_mfd = irq_data; + + regmap_write(max77759_mfd->regmap_maxq, MAX77759_MAXQ_REG_UIC_INT1, + MAX77759_MAXQ_REG_UIC_INT1_APCMDRESI); + + complete(&max77759_mfd->cmd_done); + + return IRQ_HANDLED; +} + +static int max77759_create_i2c_subdev(struct i2c_client *client, + struct max77759_mfd *max77759_mfd, + const struct max77759_i2c_subdev *sd) +{ + struct i2c_client *sub; + struct regmap *regmap; + int ret; + + if (sd->i2c_address) { + sub = devm_i2c_new_dummy_device(&client->dev, + client->adapter, + sd->i2c_address); + + if (IS_ERR(sub)) + return dev_err_probe(&client->dev, PTR_ERR(sub), + "failed to claim i2c device %s\n", + sd->cfg->name); + } else { + sub = client; + } + + regmap = devm_regmap_init_i2c(sub, sd->cfg); + if (IS_ERR(regmap)) + return dev_err_probe(&sub->dev, PTR_ERR(regmap), + "regmap init failed\n"); + + ret = regmap_attach_dev(&client->dev, regmap, sd->cfg); + if (ret) + return dev_err_probe(&client->dev, ret, + "regmap attach failed\n"); + + if (sd->id == MAX77759_I2C_SUBDEV_ID_MAXQ) + max77759_mfd->regmap_maxq = regmap; + else if (sd->id == MAX77759_I2C_SUBDEV_ID_CHARGER) + max77759_mfd->regmap_charger = regmap; + + return 0; +} + +static int max77759_add_chained_irq_chip(struct device *dev, + struct regmap *regmap, + int pirq, + struct regmap_irq_chip_data *parent, + const struct regmap_irq_chip *chip, + struct regmap_irq_chip_data **data) +{ + int irq, ret; + + irq = regmap_irq_get_virq(parent, pirq); + if (irq < 0) + return dev_err_probe(dev, irq, + "Failed to get parent vIRQ(%d) for chip %s\n", + pirq, chip->name); + + ret = devm_regmap_add_irq_chip(dev, regmap, irq, + IRQF_ONESHOT | IRQF_SHARED, 0, chip, + data); + if (ret) + return dev_err_probe(dev, ret, "Failed to add %s IRQ chip\n", + chip->name); + + return 0; +} + +static int max77759_add_chained_maxq(struct i2c_client *client, + struct max77759_mfd *max77759_mfd, + struct regmap_irq_chip_data *parent) +{ + struct regmap_irq_chip_data *irq_chip_data; + int ret; + int apcmdres_irq; + + ret = max77759_add_chained_irq_chip(&client->dev, + max77759_mfd->regmap_maxq, + MAX77759_INT_MAXQ, + parent, + &max77759_maxq_irq_chip, + &irq_chip_data); + if (ret) + return ret; + + /* + * We need to register our own IRQ handler before any MFD cells, to + * ensure client drivers can use our MaxQ interface APIs without + * any race conditions. + */ + init_completion(&max77759_mfd->cmd_done); + apcmdres_irq = regmap_irq_get_virq(irq_chip_data, + MAX77759_MAXQ_INT_APCMDRESI); + + ret = devm_request_threaded_irq(&client->dev, apcmdres_irq, + NULL, apcmdres_irq_handler, + IRQF_ONESHOT | IRQF_SHARED, + dev_name(&client->dev), max77759_mfd); + if (ret) + return dev_err_probe(&client->dev, ret, + "MAX77759_MAXQ_INT_APCMDRESI failed\n"); + + ret = devm_mfd_add_devices(&client->dev, PLATFORM_DEVID_AUTO, + max77759_maxq_cells, + ARRAY_SIZE(max77759_maxq_cells), + NULL, 0, + regmap_irq_get_domain(irq_chip_data)); + if (ret) + return dev_err_probe(&client->dev, ret, + "failed to add MFD devices (MaxQ)\n"); + + return 0; +} + +static int max77759_add_chained_topsys(struct i2c_client *client, + struct max77759_mfd *max77759_mfd, + struct regmap_irq_chip_data *parent) +{ + struct regmap_irq_chip_data *irq_chip_data; + int ret; + + ret = max77759_add_chained_irq_chip(&client->dev, + max77759_mfd->regmap_top, + MAX77759_INT_TOPSYS, + parent, + &max77759_topsys_irq_chip, + &irq_chip_data); + if (ret) + return ret; + + return 0; +} + +static int max77759_add_chained_charger(struct i2c_client *client, + struct max77759_mfd *max77759_mfd, + struct regmap_irq_chip_data *parent) +{ + struct regmap_irq_chip_data *irq_chip_data; + int ret; + + ret = max77759_add_chained_irq_chip(&client->dev, + max77759_mfd->regmap_charger, + MAX77759_INT_CHGR, + parent, + &max77759_chrg_irq_chip, + &irq_chip_data); + if (ret) + return ret; + + ret = devm_mfd_add_devices(&client->dev, PLATFORM_DEVID_AUTO, + max77759_charger_cells, + ARRAY_SIZE(max77759_charger_cells), + NULL, 0, + regmap_irq_get_domain(irq_chip_data)); + if (ret) + return dev_err_probe(&client->dev, ret, + "failed to add MFD devices (charger)\n"); + + return 0; +} + +static int max77759_probe(struct i2c_client *client) +{ + struct regmap *regmap_top; + unsigned int pmic_id; + int ret; + struct irq_data *irq_data; + struct max77759_mfd *max77759_mfd; + unsigned long irq_flags; + struct regmap_irq_chip_data *irq_chip_data_pmic; + + regmap_top = devm_regmap_init_i2c(client, &max77759_regmap_config_top); + if (IS_ERR(regmap_top)) + return dev_err_probe(&client->dev, PTR_ERR(regmap_top), + "regmap init failed\n"); + + ret = regmap_read(regmap_top, MAX77759_PMIC_REG_PMIC_ID, &pmic_id); + if (ret) + return dev_err_probe(&client->dev, ret, + "Unable to read Device ID\n"); + + if (pmic_id != MAX77759_PMIC_REG_PMIC_ID_MAX77759) + return dev_err_probe(&client->dev, -ENODEV, + "Unsupported Device ID %#.2x (%d)\n", + pmic_id, pmic_id); + + irq_data = irq_get_irq_data(client->irq); + if (!irq_data) + return dev_err_probe(&client->dev, -EINVAL, + "Invalid IRQ: %d\n", client->irq); + + max77759_mfd = devm_kzalloc(&client->dev, sizeof(*max77759_mfd), + GFP_KERNEL); + if (!max77759_mfd) + return -ENOMEM; + + max77759_mfd->regmap_top = regmap_top; + ret = devm_mutex_init(&client->dev, &max77759_mfd->maxq_lock); + if (ret) + return ret; + + i2c_set_clientdata(client, max77759_mfd); + + for (int i = 0; i < ARRAY_SIZE(max77759_i2c_subdevs); ++i) { + ret = max77759_create_i2c_subdev(client, + max77759_mfd, + &max77759_i2c_subdevs[i]); + if (ret) + return ret; + } + + irq_flags = IRQF_ONESHOT | IRQF_SHARED; + irq_flags |= irqd_get_trigger_type(irq_data); + + ret = devm_regmap_add_irq_chip(&client->dev, max77759_mfd->regmap_top, + client->irq, irq_flags, 0, + &max77759_pmic_irq_chip, + &irq_chip_data_pmic); + if (ret) + return dev_err_probe(&client->dev, ret, + "Failed to add IRQ chip\n"); + + /* INTSRC - MaxQ & children */ + ret = max77759_add_chained_maxq(client, max77759_mfd, + irq_chip_data_pmic); + if (ret) + return ret; + + /* INTSRC - topsys & children */ + ret = max77759_add_chained_topsys(client, max77759_mfd, + irq_chip_data_pmic); + if (ret) + return ret; + + /* INTSRC - charger & children */ + ret = max77759_add_chained_charger(client, max77759_mfd, + irq_chip_data_pmic); + if (ret) + return ret; + + return devm_mfd_add_devices(&client->dev, PLATFORM_DEVID_AUTO, + max77759_cells, ARRAY_SIZE(max77759_cells), + NULL, 0, + regmap_irq_get_domain(irq_chip_data_pmic)); +} + +static const struct i2c_device_id max77759_i2c_id[] = { + { "max77759", 0 }, + { } +}; +MODULE_DEVICE_TABLE(i2c, max77759_i2c_id); + +static const struct of_device_id max77759_of_id[] = { + { .compatible = "maxim,max77759", }, + { } +}; +MODULE_DEVICE_TABLE(of, max77759_of_id); + +static struct i2c_driver max77759_i2c_driver = { + .driver = { + .name = "max77759", + .of_match_table = max77759_of_id, + }, + .probe = max77759_probe, + .id_table = max77759_i2c_id, +}; +module_i2c_driver(max77759_i2c_driver); + +MODULE_AUTHOR("André Draszik <andre.draszik@linaro.org>"); +MODULE_DESCRIPTION("Maxim MAX77759 multi-function core driver"); +MODULE_LICENSE("GPL"); diff --git a/include/linux/mfd/max77759.h b/include/linux/mfd/max77759.h new file mode 100644 index 0000000000000000000000000000000000000000..b038b4e9b748287e23e3a7030496f09dc8bdc816 --- /dev/null +++ b/include/linux/mfd/max77759.h @@ -0,0 +1,98 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* + * Copyright 2020 Google Inc. + * Copyright 2025 Linaro Ltd. + * + * Client interface for Maxim MAX77759 MFD driver + */ + +#ifndef __LINUX_MFD_MAX77759_H +#define __LINUX_MFD_MAX77759_H + +/* MaxQ opcodes */ +#define MAX77759_MAXQ_OPCODE_MAXLENGTH 33 + +#define MAX77759_MAXQ_OPCODE_GPIO_TRIGGER_READ 0x21 +#define MAX77759_MAXQ_OPCODE_GPIO_TRIGGER_WRITE 0x22 +#define MAX77759_MAXQ_OPCODE_GPIO_CONTROL_READ 0x23 +#define MAX77759_MAXQ_OPCODE_GPIO_CONTROL_WRITE 0x24 +#define MAX77759_MAXQ_OPCODE_USER_SPACE_READ 0x81 +#define MAX77759_MAXQ_OPCODE_USER_SPACE_WRITE 0x82 + +/* + * register map (incomplete) - registers not useful for drivers are not + * declared here + */ +/* MaxQ */ +#define MAX77759_MAXQ_REG_UIC_INT1 0x64 +#define MAX77759_MAXQ_REG_UIC_INT1_APCMDRESI BIT(7) +#define MAX77759_MAXQ_REG_UIC_INT1_SYSMSGI BIT(6) +#define MAX77759_MAXQ_REG_UIC_INT1_GPIO6I BIT(1) +#define MAX77759_MAXQ_REG_UIC_INT1_GPIO5I BIT(0) +#define MAX77759_MAXQ_REG_UIC_INT1_GPIOxI(offs, en) (((en) & 1) << (offs)) +#define MAX77759_MAXQ_REG_UIC_INT1_GPIOxI_MASK(offs) \ + MAX77759_MAXQ_REG_UIC_INT1_GPIOxI(offs, ~0) + +#define MAX77759_MAXQ_REG_UIC_INT2 0x65 +#define MAX77759_MAXQ_REG_UIC_INT3 0x66 +#define MAX77759_MAXQ_REG_UIC_INT4 0x67 +#define MAX77759_MAXQ_REG_UIC_UIC_STATUS1 0x68 +#define MAX77759_MAXQ_REG_UIC_UIC_STATUS2 0x69 +#define MAX77759_MAXQ_REG_UIC_UIC_STATUS3 0x6a +#define MAX77759_MAXQ_REG_UIC_UIC_STATUS4 0x6b +#define MAX77759_MAXQ_REG_UIC_UIC_STATUS5 0x6c +#define MAX77759_MAXQ_REG_UIC_UIC_STATUS6 0x6d +#define MAX77759_MAXQ_REG_UIC_UIC_STATUS7 0x6f +#define MAX77759_MAXQ_REG_UIC_UIC_STATUS8 0x6f +#define MAX77759_MAXQ_REG_UIC_INT1_M 0x70 +#define MAX77759_MAXQ_REG_UIC_INT2_M 0x71 +#define MAX77759_MAXQ_REG_UIC_INT3_M 0x72 +#define MAX77759_MAXQ_REG_UIC_INT4_M 0x73 + +/* charger */ +#define MAX77759_CHGR_REG_CHG_INT 0xb0 +#define MAX77759_CHGR_REG_CHG_INT2 0xb1 +#define MAX77759_CHGR_REG_CHG_INT_MASK 0xb2 +#define MAX77759_CHGR_REG_CHG_INT2_MASK 0xb3 + +struct max77759_mfd; + +/** + * struct max77759_maxq_command - structure containing the MaxQ command to + * send + * + * @length: The number of bytes to send. + * @cmd: The data to send. + */ +struct max77759_maxq_command { + u8 length; + u8 cmd[] __counted_by(length); +}; + +/** + * struct max77759_maxq_response - structure containing the MaxQ response + * + * @length: The number of bytes to receive. + * @rsp: The data received. Must have at least @length bytes space. + */ +struct max77759_maxq_response { + u8 length; + u8 rsp[] __counted_by(length); +}; + +/** + * max77759_maxq_command() - issue a MaxQ command and wait for the response + * and associated data + * + * @max77759_mfd: The core max77759 mfd device handle. + * @cmd: The command to be sent. + * @rsp: Any response data associated with the command will be copied here; + * can be %NULL if the command has no response (other than ACK). + * + * Return: 0 on success, a negative error number otherwise. + */ +int max77759_maxq_command(struct max77759_mfd *max77759_mfd, + const struct max77759_maxq_command *cmd, + struct max77759_maxq_response *rsp); + +#endif /* __LINUX_MFD_MAX77759_H */ -- 2.49.0.rc0.332.g42c0ae87b1-goog ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v4 4/6] mfd: max77759: add Maxim MAX77759 core mfd driver 2025-03-12 9:26 ` [PATCH v4 4/6] mfd: max77759: add Maxim MAX77759 core mfd driver André Draszik @ 2025-03-14 12:31 ` Lee Jones 2025-03-14 15:30 ` André Draszik 0 siblings, 1 reply; 12+ messages in thread From: Lee Jones @ 2025-03-14 12:31 UTC (permalink / raw) To: André Draszik Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Linus Walleij, Bartosz Golaszewski, Srinivas Kandagatla, Kees Cook, Gustavo A. R. Silva, Peter Griffin, Tudor Ambarus, Will McVicker, kernel-team, linux-kernel, devicetree, linux-gpio, linux-hardening On Wed, 12 Mar 2025, André Draszik wrote: > The Maxim MAX77759 is a companion PMIC for USB Type-C applications and > includes Battery Charger, Fuel Gauge, temperature sensors, USB Type-C > Port Controller (TCPC), NVMEM, and a GPIO expander. > > Fuel Gauge and TCPC have separate and independent I2C addresses, > register maps, and interrupt lines and are therefore excluded from the > MFD core device driver here. > > The GPIO and NVMEM interfaces are accessed via specific commands to the > built-in microprocessor. This driver implements an API that client > drivers can use for accessing those. > > Signed-off-by: André Draszik <andre.draszik@linaro.org> > > --- > v4: > * add missing build_bug.h include > * update an irq chip comment > * fix a whitespace in register definitions > > v2: > * add kernel doc for max77759_maxq_command() and related structs > * fix an msec / usec typo > * add missing error handling of devm_mutex_init() (Christophe) > * align sentinel in max77759_of_id[] with max77759_i2c_id[] > (Christophe) > * some tidy-ups in max77759_maxq_command() (Christophe) > --- > MAINTAINERS | 2 + > drivers/mfd/Kconfig | 20 ++ > drivers/mfd/Makefile | 1 + > drivers/mfd/max77759.c | 738 +++++++++++++++++++++++++++++++++++++++++++ > include/linux/mfd/max77759.h | 98 ++++++ > 5 files changed, 859 insertions(+) > > diff --git a/MAINTAINERS b/MAINTAINERS > index a45d1bd64d116d14bc05a64fa3da852a41e1de7f..38844ea24e464d0b58f8852b79e2b94f18d48998 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -14352,6 +14352,8 @@ M: André Draszik <andre.draszik@linaro.org> > L: linux-kernel@vger.kernel.org > S: Maintained > F: Documentation/devicetree/bindings/*/maxim,max77759*.yaml > +F: drivers/mfd/max77759.c > +F: include/linux/mfd/max77759.h > > MAXIM MAX77802 PMIC REGULATOR DEVICE DRIVER > M: Javier Martinez Canillas <javier@dowhile0.org> > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig > index d44c69bb3dfd52d53fe26aa2d5e5ace346448f57..1d72bf086401064608cc41e9f9a0af044c6df68e 100644 > --- a/drivers/mfd/Kconfig > +++ b/drivers/mfd/Kconfig > @@ -930,6 +930,26 @@ config MFD_MAX77714 > drivers must be enabled in order to use each functionality of the > device. > > +config MFD_MAX77759 > + tristate "Maxim Integrated MAX77759 PMIC" > + depends on I2C > + depends on OF > + select IRQ_DOMAIN > + select MFD_CORE > + select REGMAP_I2C > + select REGMAP_IRQ > + help > + Say yes here to add support for Maxim Integrated MAX77759. > + This is a companion Power Management IC for USB Type-C applications > + with Battery Charger, Fuel Gauge, temperature sensors, USB Type-C > + Port Controller (TCPC), NVMEM, and additional GPIO interfaces. > + This driver provides common support for accessing the device; > + additional drivers must be enabled in order to use the functionality > + of the device. > + > + To compile this driver as a module, choose M here: the module will be > + called max77759. > + > config MFD_MAX77843 > bool "Maxim Semiconductor MAX77843 PMIC Support" > depends on I2C=y > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile > index 9220eaf7cf1255a8922430fe3e50e41771bbaa60..cc9362afd8f060d64ca0f0f028a0d7cfe9cfe512 100644 > --- a/drivers/mfd/Makefile > +++ b/drivers/mfd/Makefile > @@ -169,6 +169,7 @@ 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_MAX77759) += max77759.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/max77759.c b/drivers/mfd/max77759.c > new file mode 100644 > index 0000000000000000000000000000000000000000..a4f1616d600bde250a73b69464ada1fbf6d7e67e > --- /dev/null > +++ b/drivers/mfd/max77759.c > @@ -0,0 +1,738 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +// > +// Copyright 2020 Google Inc > +// Copyright 2025 Linaro Ltd. > +// > +// Core MFD driver for Maxim MAX77759 companion PMIC for USB Type-C Only the SPDX should be in C++ style comments. Please C block-comment the rest. Drop all references to MFD - it's not a real thing. > +#include <linux/array_size.h> > +#include <linux/bitfield.h> > +#include <linux/bits.h> > +#include <linux/build_bug.h> > +#include <linux/cleanup.h> > +#include <linux/completion.h> > +#include <linux/dev_printk.h> > +#include <linux/device.h> > +#include <linux/err.h> > +#include <linux/i2c.h> > +#include <linux/init.h> > +#include <linux/interrupt.h> > +#include <linux/irq.h> > +#include <linux/jiffies.h> > +#include <linux/mfd/core.h> > +#include <linux/mfd/max77759.h> > +#include <linux/mod_devicetable.h> > +#include <linux/module.h> > +#include <linux/mutex.h> > +#include <linux/of.h> > +#include <linux/overflow.h> > +#include <linux/regmap.h> This is a huge list of includes. Please comb them to see if you really do need all of them. > +/* registers - registers useful to drivers are declared in the public header */ This sort of goes without saying. Suggest you remove this comment. > +/* PMIC / TOP */ > +#define MAX77759_PMIC_REG_PMIC_ID 0x00 > +#define MAX77759_PMIC_REG_PMIC_ID_MAX77759 59 Is this a register or a value? > +#define MAX77759_PMIC_REG_PMIC_REVISION 0x01 > +#define MAX77759_PMIC_REG_OTP_REVISION 0x02 > + > +#define MAX77759_PMIC_REG_INTSRC 0x22 > +#define MAX77759_PMIC_REG_INTSRCMASK 0x23 > +#define MAX77759_PMIC_REG_INTSRC_MAXQ BIT(3) > +#define MAX77759_PMIC_REG_INTSRC_TOPSYS BIT(1) > +#define MAX77759_PMIC_REG_INTSRC_CHGR BIT(0) These look like bit offsets rather than reg addresses? > +#define MAX77759_PMIC_REG_TOPSYS_INT 0x24 > +#define MAX77759_PMIC_REG_TOPSYS_INT_MASK 0x26 > +#define MAX77759_PMIC_REG_TOPSYS_INT_TSHDN BIT(6) > +#define MAX77759_PMIC_REG_TOPSYS_INT_SYSOVLO BIT(5) > +#define MAX77759_PMIC_REG_TOPSYS_INT_SYSUVLO BIT(4) > +#define MAX77759_PMIC_REG_TOPSYS_INT_FSHIP BIT(0) > + > +#define MAX77759_PMIC_REG_I2C_CNFG 0x40 > +#define MAX77759_PMIC_REG_SWRESET 0x50 > +#define MAX77759_PMIC_REG_CONTROL_FG 0x51 > +#define MAX77759_PMIC_REG_LAST_REGISTER MAX77759_PMIC_REG_CONTROL_FG You could just use MAX77759_PMIC_REG_CONTROL_FG in-place? > +/* MaxQ */ How does this improve readability? The name is already in the define. If you want to provide a comment, which isn't a bad idea, extend the meaning and describe the purpose instead. > +#define MAX77759_MAXQ_REG_AP_DATAOUT0 0x81 > +#define MAX77759_MAXQ_REG_AP_DATAOUT32 0xa1 > +#define MAX77759_MAXQ_REG_AP_MESSAGESZ_MAX (MAX77759_MAXQ_REG_AP_DATAOUT32 \ > + - MAX77759_MAXQ_REG_AP_DATAOUT0 \ > + + 1) This is a strange way to line things up. > +#define MAX77759_MAXQ_REG_AP_DATAIN0 0xb1 > +#define MAX77759_MAXQ_REG_LAST_REGISTER 0xe0 What is the last register's purpose, besides being the last one? > +/* charger */ Not sure this provides much, but if you insist on keeping it, all comments should start with an upper-case char here. > +#define MAX77759_CHGR_REG_LAST_REGISTER 0xcc Where are the other registers? > +enum max77759_i2c_subdev_id { > + MAX77759_I2C_SUBDEV_ID_MAXQ, > + MAX77759_I2C_SUBDEV_ID_CHARGER, Are these truly arbitrary or are you relying on the fact that the compiler usually starts from 0 and incs by 1? > +}; > + > +struct max77759_mfd { Drop all "mfd"ness. > + /* protecting MaxQ commands - only one can be active */ "This protects" > + struct mutex maxq_lock; > + struct regmap *regmap_maxq; > + struct completion cmd_done; > + > + struct regmap *regmap_top; What is top? > + struct regmap *regmap_charger; > +}; > + > +struct max77759_i2c_subdev { > + enum max77759_i2c_subdev_id id; > + const struct regmap_config *cfg; > + u16 i2c_address; > +}; > + > +/* TOP registers */ > +static const struct regmap_range max77759_top_registers[] = { > + regmap_reg_range(0x00, 0x02), > + regmap_reg_range(0x22, 0x24), > + regmap_reg_range(0x26, 0x26), > + regmap_reg_range(0x40, 0x40), > + regmap_reg_range(0x50, 0x51), What are these magic numbers? Can you define them? > +}; > + > +static const struct regmap_range max77759_top_ro_registers[] = { > + regmap_reg_range(0x00, 0x02), > + regmap_reg_range(0x22, 0x22), > +}; > + > +static const struct regmap_range max77759_top_volatile_registers[] = { > + regmap_reg_range(0x22, 0x22), > + regmap_reg_range(0x24, 0x24), > +}; > + > +static const struct regmap_access_table max77759_top_wr_table = { > + .yes_ranges = max77759_top_registers, > + .n_yes_ranges = ARRAY_SIZE(max77759_top_registers), > + .no_ranges = max77759_top_ro_registers, > + .n_no_ranges = ARRAY_SIZE(max77759_top_ro_registers), > +}; > + > +static const struct regmap_access_table max77759_top_rd_table = { > + .yes_ranges = max77759_top_registers, > + .n_yes_ranges = ARRAY_SIZE(max77759_top_registers), > +}; > + > +static const struct regmap_access_table max77759_top_volatile_table = { > + .yes_ranges = max77759_top_volatile_registers, > + .n_yes_ranges = ARRAY_SIZE(max77759_top_volatile_registers), > +}; > + > +static const struct regmap_config max77759_regmap_config_top = { > + .name = "top", > + .reg_bits = 8, > + .val_bits = 8, > + .max_register = MAX77759_PMIC_REG_LAST_REGISTER, > + .wr_table = &max77759_top_wr_table, > + .rd_table = &max77759_top_rd_table, > + .volatile_table = &max77759_top_volatile_table, > + .num_reg_defaults_raw = MAX77759_PMIC_REG_LAST_REGISTER + 1, > + .cache_type = REGCACHE_MAPLE, > +}; > + > +/* MaxQ registers */ Again, this is obvious due to the nomenclature. > +static const struct regmap_range max77759_maxq_registers[] = { > + regmap_reg_range(0x60, 0x73), > + regmap_reg_range(0x81, 0xa1), > + regmap_reg_range(0xb1, 0xd1), > + regmap_reg_range(0xe0, 0xe0), > +}; > + > +static const struct regmap_range max77759_maxq_ro_registers[] = { > + regmap_reg_range(0x60, 0x63), > + regmap_reg_range(0x68, 0x6f), > + regmap_reg_range(0xb1, 0xd1), > +}; > + > +static const struct regmap_range max77759_maxq_volatile_registers[] = { > + regmap_reg_range(0x64, 0x6f), > + regmap_reg_range(0xb1, 0xd1), > + regmap_reg_range(0xe0, 0xe0), > +}; > + > +static const struct regmap_access_table max77759_maxq_wr_table = { > + .yes_ranges = max77759_maxq_registers, > + .n_yes_ranges = ARRAY_SIZE(max77759_maxq_registers), > + .no_ranges = max77759_maxq_ro_registers, > + .n_no_ranges = ARRAY_SIZE(max77759_maxq_ro_registers), > +}; > + > +static const struct regmap_access_table max77759_maxq_rd_table = { > + .yes_ranges = max77759_maxq_registers, > + .n_yes_ranges = ARRAY_SIZE(max77759_maxq_registers), > +}; > + > +static const struct regmap_access_table max77759_maxq_volatile_table = { > + .yes_ranges = max77759_maxq_volatile_registers, > + .n_yes_ranges = ARRAY_SIZE(max77759_maxq_volatile_registers), > +}; > + > +static const struct regmap_config max77759_regmap_config_maxq = { > + .name = "maxq", > + .reg_bits = 8, > + .val_bits = 8, > + .max_register = MAX77759_MAXQ_REG_LAST_REGISTER, > + .wr_table = &max77759_maxq_wr_table, > + .rd_table = &max77759_maxq_rd_table, > + .volatile_table = &max77759_maxq_volatile_table, > + .num_reg_defaults_raw = MAX77759_MAXQ_REG_LAST_REGISTER + 1, > + .cache_type = REGCACHE_MAPLE, > +}; > + > +/* charger registers */ > +static const struct regmap_range max77759_charger_registers[] = { > + regmap_reg_range(0xb0, 0xcc), > +}; > + > +static const struct regmap_range max77759_charger_ro_registers[] = { > + regmap_reg_range(0xb4, 0xb8), > +}; > + > +static const struct regmap_range max77759_charger_volatile_registers[] = { > + regmap_reg_range(0xb0, 0xb1), > + regmap_reg_range(0xb4, 0xb8), > +}; > + > +static const struct regmap_access_table max77759_charger_wr_table = { > + .yes_ranges = max77759_charger_registers, > + .n_yes_ranges = ARRAY_SIZE(max77759_charger_registers), > + .no_ranges = max77759_charger_ro_registers, > + .n_no_ranges = ARRAY_SIZE(max77759_charger_ro_registers), > +}; > + > +static const struct regmap_access_table max77759_charger_rd_table = { > + .yes_ranges = max77759_charger_registers, > + .n_yes_ranges = ARRAY_SIZE(max77759_charger_registers), > +}; > + > +static const struct regmap_access_table max77759_charger_volatile_table = { > + .yes_ranges = max77759_charger_volatile_registers, > + .n_yes_ranges = ARRAY_SIZE(max77759_charger_volatile_registers), > +}; > + > +static const struct regmap_config max77759_regmap_config_charger = { > + .name = "charger", > + .reg_bits = 8, > + .val_bits = 8, > + .max_register = MAX77759_CHGR_REG_LAST_REGISTER, > + .wr_table = &max77759_charger_wr_table, > + .rd_table = &max77759_charger_rd_table, > + .volatile_table = &max77759_charger_volatile_table, > + .num_reg_defaults_raw = MAX77759_CHGR_REG_LAST_REGISTER + 1, > + .cache_type = REGCACHE_MAPLE, > +}; > + > +/* > + * Interrupts - with the following interrupt hierarchy: > + * pmic IRQs (INTSRC) > + * - MAXQ_INT: MaxQ IRQs > + * - UIC_INT1 > + * - APCmdResI > + * - SysMsgI > + * - GPIOxI > + * - TOPSYS_INT: topsys > + * - TOPSYS_INT > + * - TSHDN_INT > + * - SYSOVLO_INT > + * - SYSUVLO_INT > + * - FSHIP_NOT_RD > + * - CHGR_INT: charger > + * - CHG_INT > + * - CHG_INT2 > + */ > +enum { > + MAX77759_INT_MAXQ, > + MAX77759_INT_TOPSYS, > + MAX77759_INT_CHGR, > +}; > + > +enum { > + MAX77759_TOPSYS_INT_TSHDN, > + MAX77759_TOPSYS_INT_SYSOVLO, > + MAX77759_TOPSYS_INT_SYSUVLO, > + MAX77759_TOPSYS_INT_FSHIP_NOT_RD, > +}; > + > +enum { > + MAX77759_MAXQ_INT_APCMDRESI, > + MAX77759_MAXQ_INT_SYSMSGI, > + MAX77759_MAXQ_INT_GPIO, > + MAX77759_MAXQ_INT_UIC1, > + MAX77759_MAXQ_INT_UIC2, > + MAX77759_MAXQ_INT_UIC3, > + MAX77759_MAXQ_INT_UIC4, > +}; > + > +enum { > + MAX77759_CHARGER_INT_1, > + MAX77759_CHARGER_INT_2, > +}; > + > +static const struct regmap_irq max77759_pmic_irqs[] = { > + REGMAP_IRQ_REG(MAX77759_INT_MAXQ, 0, MAX77759_PMIC_REG_INTSRC_MAXQ), > + REGMAP_IRQ_REG(MAX77759_INT_TOPSYS, 0, MAX77759_PMIC_REG_INTSRC_TOPSYS), > + REGMAP_IRQ_REG(MAX77759_INT_CHGR, 0, MAX77759_PMIC_REG_INTSRC_CHGR), > +}; > + > +static const struct regmap_irq max77759_maxq_irqs[] = { > + REGMAP_IRQ_REG(MAX77759_MAXQ_INT_APCMDRESI, > + 0, MAX77759_MAXQ_REG_UIC_INT1_APCMDRESI), Use up to 100-chars here to avoid the line-break. This isn't 1999 anymore - we have wide-screen 4k monitors. =:-) > + REGMAP_IRQ_REG(MAX77759_MAXQ_INT_SYSMSGI, > + 0, MAX77759_MAXQ_REG_UIC_INT1_SYSMSGI), > + REGMAP_IRQ_REG(MAX77759_MAXQ_INT_GPIO, 0, GENMASK(1, 0)), > + REGMAP_IRQ_REG(MAX77759_MAXQ_INT_UIC1, 0, GENMASK(5, 2)), > + REGMAP_IRQ_REG(MAX77759_MAXQ_INT_UIC2, 1, GENMASK(7, 0)), > + REGMAP_IRQ_REG(MAX77759_MAXQ_INT_UIC3, 2, GENMASK(7, 0)), > + REGMAP_IRQ_REG(MAX77759_MAXQ_INT_UIC4, 3, GENMASK(7, 0)), > +}; > + > +static const struct regmap_irq max77759_topsys_irqs[] = { > + REGMAP_IRQ_REG(MAX77759_TOPSYS_INT_TSHDN, > + 0, MAX77759_PMIC_REG_TOPSYS_INT_TSHDN), > + REGMAP_IRQ_REG(MAX77759_TOPSYS_INT_SYSOVLO, > + 0, MAX77759_PMIC_REG_TOPSYS_INT_SYSOVLO), > + REGMAP_IRQ_REG(MAX77759_TOPSYS_INT_SYSUVLO, > + 0, MAX77759_PMIC_REG_TOPSYS_INT_SYSUVLO), > + REGMAP_IRQ_REG(MAX77759_TOPSYS_INT_FSHIP_NOT_RD, > + 0, MAX77759_PMIC_REG_TOPSYS_INT_FSHIP), > +}; > + > +static const struct regmap_irq max77759_chgr_irqs[] = { > + REGMAP_IRQ_REG(MAX77759_CHARGER_INT_1, 0, GENMASK(7, 0)), > + REGMAP_IRQ_REG(MAX77759_CHARGER_INT_2, 1, GENMASK(7, 0)), > +}; > + > +static const struct regmap_irq_chip max77759_pmic_irq_chip = { > + .name = "max77759-pmic", > + /* INTSRC is read-only and doesn't require clearing */ > + .status_base = MAX77759_PMIC_REG_INTSRC, > + .mask_base = MAX77759_PMIC_REG_INTSRCMASK, > + .num_regs = 1, > + .irqs = max77759_pmic_irqs, > + .num_irqs = ARRAY_SIZE(max77759_pmic_irqs), > +}; > + > +/* > + * We can let regmap-irq auto-ack the topsys interrupt bits as required, but > + * for all others the individual drivers need to know which interrupt bit > + * exactly is set inside their interrupt handlers, and therefore we can not set > + * .ack_base for those. > + */ Nice - thanks for this! > +static const struct regmap_irq_chip max77759_maxq_irq_chip = { > + .name = "max77759-maxq", > + .domain_suffix = "MAXQ", > + .status_base = MAX77759_MAXQ_REG_UIC_INT1, > + .mask_base = MAX77759_MAXQ_REG_UIC_INT1_M, > + .num_regs = 4, > + .irqs = max77759_maxq_irqs, > + .num_irqs = ARRAY_SIZE(max77759_maxq_irqs), > +}; > + > +static const struct regmap_irq_chip max77759_topsys_irq_chip = { > + .name = "max77759-topsys", > + .domain_suffix = "TOPSYS", > + .status_base = MAX77759_PMIC_REG_TOPSYS_INT, > + .mask_base = MAX77759_PMIC_REG_TOPSYS_INT_MASK, > + .ack_base = MAX77759_PMIC_REG_TOPSYS_INT, > + .num_regs = 1, > + .irqs = max77759_topsys_irqs, > + .num_irqs = ARRAY_SIZE(max77759_topsys_irqs), > +}; > + > +static const struct regmap_irq_chip max77759_chrg_irq_chip = { > + .name = "max77759-chgr", > + .domain_suffix = "CHGR", > + .status_base = MAX77759_CHGR_REG_CHG_INT, > + .mask_base = MAX77759_CHGR_REG_CHG_INT_MASK, > + .num_regs = 2, > + .irqs = max77759_chgr_irqs, > + .num_irqs = ARRAY_SIZE(max77759_chgr_irqs), > +}; > + > +static const struct max77759_i2c_subdev max77759_i2c_subdevs[] = { > + { > + .id = MAX77759_I2C_SUBDEV_ID_MAXQ, > + /* I2C address is same as top's */ It took me a moment to figure out why this is relevant. Maybe shift this down, so it lines-up with the next element. > + .cfg = &max77759_regmap_config_maxq, > + }, > + { > + .id = MAX77759_I2C_SUBDEV_ID_CHARGER, > + .cfg = &max77759_regmap_config_charger, > + .i2c_address = 0x69, > + }, > +}; > + > +static const struct resource max77759_gpio_resources[] = { > + DEFINE_RES_IRQ_NAMED(MAX77759_MAXQ_INT_GPIO, "GPI"), > +}; > + > +static const struct resource max77759_charger_resources[] = { > + DEFINE_RES_IRQ_NAMED(MAX77759_CHARGER_INT_1, "INT1"), > + DEFINE_RES_IRQ_NAMED(MAX77759_CHARGER_INT_2, "INT2"), > +}; > + > +static const struct mfd_cell max77759_cells[] = { > + MFD_CELL_OF("max77759-nvmem", NULL, NULL, 0, 0, > + "maxim,max77759-nvmem"), > +}; > + > +static const struct mfd_cell max77759_maxq_cells[] = { > + MFD_CELL_OF("max77759-gpio", max77759_gpio_resources, NULL, 0, 0, > + "maxim,max77759-gpio"), > +}; > + > +static const struct mfd_cell max77759_charger_cells[] = { > + MFD_CELL_RES("max77759-charger", max77759_charger_resources), > +}; Are there situations where only 1 or 2 of these will be used? If they will all be used all of the time, save yourself a bunch of lines and complexity and add them all to a single struct. > +int max77759_maxq_command(struct max77759_mfd *max77759_mfd, > + const struct max77759_maxq_command *cmd, > + struct max77759_maxq_response *rsp) > +{ > + DEFINE_FLEX(struct max77759_maxq_response, _rsp, rsp, length, 1); > + int ret; Nit: Please place this at the bottom of the block. *twitch* > + struct device *dev = regmap_get_device(max77759_mfd->regmap_maxq); > + static const unsigned int timeout_ms = 200; Why 200? > + > + if (cmd->length > MAX77759_MAXQ_REG_AP_MESSAGESZ_MAX) > + return -EINVAL; > + > + /* rsp is allowed to be NULL. In that case we do need a temporary. */ More details please. Why is NULL valid? Why does it need a placeholder? What are you using as the placeholder? > + if (!rsp) > + rsp = _rsp; > + > + BUILD_BUG_ON(MAX77759_MAXQ_OPCODE_MAXLENGTH > + != MAX77759_MAXQ_REG_AP_MESSAGESZ_MAX); Probably useful during development. Let useful in production code. > + if (!rsp->length || rsp->length > MAX77759_MAXQ_REG_AP_MESSAGESZ_MAX) > + return -EINVAL; > + > + guard(mutex)(&max77759_mfd->maxq_lock); > + > + reinit_completion(&max77759_mfd->cmd_done); > + > + /* write the opcode and data */ Upper case throughout please. > + ret = regmap_bulk_write(max77759_mfd->regmap_maxq, > + MAX77759_MAXQ_REG_AP_DATAOUT0, cmd->cmd, > + cmd->length); > + if (!ret && cmd->length < MAX77759_MAXQ_REG_AP_MESSAGESZ_MAX) > + /* writing the last byte triggers MaxQ */ > + ret = regmap_write(max77759_mfd->regmap_maxq, > + MAX77759_MAXQ_REG_AP_DATAOUT32, 0); > + if (ret) { Do this first. Then you only need to check: cmd->length < MAX77759_MAXQ_REG_AP_MESSAGESZ_MAX ... in the subsequent statement. But what happens if: !ret && cmd->length >= MAX77759_MAXQ_REG_AP_MESSAGESZ_MAX ^ Is that okay? Nothing to be done? > + dev_warn(dev, "write data failed: %d\n", ret); This should be dev_err(). > + return ret; > + } > + > + /* wait for response from MaxQ */ > + if (!wait_for_completion_timeout(&max77759_mfd->cmd_done, > + msecs_to_jiffies(timeout_ms))) { > + dev_err(dev, "timed out waiting for data\n"); What data? > + return -ETIMEDOUT; > + } > + > + ret = regmap_bulk_read(max77759_mfd->regmap_maxq, > + MAX77759_MAXQ_REG_AP_DATAIN0, > + rsp->rsp, rsp->length); > + if (ret) { > + dev_warn(dev, "read data failed: %d\n", ret); As above. > + return ret; > + } > + > + /* > + * As per the protocol, the first byte of the reply will match the > + * request. > + */ > + if (cmd->cmd[0] != rsp->rsp[0]) { > + dev_warn(dev, "unexpected opcode response for %#.2x: %*ph\n", > + cmd->cmd[0], (int)rsp->length, rsp->rsp); > + return -EIO; > + } > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(max77759_maxq_command); > + > +static irqreturn_t apcmdres_irq_handler(int irq, void *irq_data) > +{ > + struct max77759_mfd *max77759_mfd = irq_data; > + > + regmap_write(max77759_mfd->regmap_maxq, MAX77759_MAXQ_REG_UIC_INT1, > + MAX77759_MAXQ_REG_UIC_INT1_APCMDRESI); > + > + complete(&max77759_mfd->cmd_done); > + > + return IRQ_HANDLED; > +} > + > +static int max77759_create_i2c_subdev(struct i2c_client *client, > + struct max77759_mfd *max77759_mfd, > + const struct max77759_i2c_subdev *sd) > +{ > + struct i2c_client *sub; > + struct regmap *regmap; > + int ret; > + > + if (sd->i2c_address) { > + sub = devm_i2c_new_dummy_device(&client->dev, > + client->adapter, > + sd->i2c_address); > + > + if (IS_ERR(sub)) > + return dev_err_probe(&client->dev, PTR_ERR(sub), > + "failed to claim i2c device %s\n", "I2C" > + sd->cfg->name); > + } else { > + sub = client; Assign during declaration, with a comment, then dispose of this leg. > + } > + > + regmap = devm_regmap_init_i2c(sub, sd->cfg); > + if (IS_ERR(regmap)) > + return dev_err_probe(&sub->dev, PTR_ERR(regmap), > + "regmap init failed\n"); > + > + ret = regmap_attach_dev(&client->dev, regmap, sd->cfg); > + if (ret) > + return dev_err_probe(&client->dev, ret, > + "regmap attach failed\n"); > + > + if (sd->id == MAX77759_I2C_SUBDEV_ID_MAXQ) > + max77759_mfd->regmap_maxq = regmap; > + else if (sd->id == MAX77759_I2C_SUBDEV_ID_CHARGER) > + max77759_mfd->regmap_charger = regmap; > + > + return 0; > +} > + > +static int max77759_add_chained_irq_chip(struct device *dev, > + struct regmap *regmap, > + int pirq, > + struct regmap_irq_chip_data *parent, > + const struct regmap_irq_chip *chip, > + struct regmap_irq_chip_data **data) > +{ > + int irq, ret; > + > + irq = regmap_irq_get_virq(parent, pirq); > + if (irq < 0) > + return dev_err_probe(dev, irq, > + "Failed to get parent vIRQ(%d) for chip %s\n", > + pirq, chip->name); > + > + ret = devm_regmap_add_irq_chip(dev, regmap, irq, > + IRQF_ONESHOT | IRQF_SHARED, 0, chip, > + data); > + if (ret) > + return dev_err_probe(dev, ret, "Failed to add %s IRQ chip\n", Some of your messages start will uppercase whilst others do not. *twitch* Please pick one and use throughout. > + chip->name); > + > + return 0; > +} > + > +static int max77759_add_chained_maxq(struct i2c_client *client, > + struct max77759_mfd *max77759_mfd, > + struct regmap_irq_chip_data *parent) > +{ > + struct regmap_irq_chip_data *irq_chip_data; > + int ret; Bottom please. Some people like reverse Christmas tree. Structure helps me sleep at night! =;-) > + int apcmdres_irq; > + > + ret = max77759_add_chained_irq_chip(&client->dev, > + max77759_mfd->regmap_maxq, > + MAX77759_INT_MAXQ, > + parent, > + &max77759_maxq_irq_chip, > + &irq_chip_data); > + if (ret) > + return ret; > + > + /* > + * We need to register our own IRQ handler before any MFD cells, to > + * ensure client drivers can use our MaxQ interface APIs without > + * any race conditions. > + */ This is normal. > + init_completion(&max77759_mfd->cmd_done); > + apcmdres_irq = regmap_irq_get_virq(irq_chip_data, > + MAX77759_MAXQ_INT_APCMDRESI); > + > + ret = devm_request_threaded_irq(&client->dev, apcmdres_irq, > + NULL, apcmdres_irq_handler, > + IRQF_ONESHOT | IRQF_SHARED, > + dev_name(&client->dev), max77759_mfd); > + if (ret) > + return dev_err_probe(&client->dev, ret, > + "MAX77759_MAXQ_INT_APCMDRESI failed\n"); > + > + ret = devm_mfd_add_devices(&client->dev, PLATFORM_DEVID_AUTO, > + max77759_maxq_cells, > + ARRAY_SIZE(max77759_maxq_cells), > + NULL, 0, > + regmap_irq_get_domain(irq_chip_data)); > + if (ret) > + return dev_err_probe(&client->dev, ret, > + "failed to add MFD devices (MaxQ)\n"); > + > + return 0; > +} > + > +static int max77759_add_chained_topsys(struct i2c_client *client, > + struct max77759_mfd *max77759_mfd, > + struct regmap_irq_chip_data *parent) > +{ > + struct regmap_irq_chip_data *irq_chip_data; > + int ret; > + > + ret = max77759_add_chained_irq_chip(&client->dev, > + max77759_mfd->regmap_top, > + MAX77759_INT_TOPSYS, > + parent, > + &max77759_topsys_irq_chip, > + &irq_chip_data); > + if (ret) > + return ret; > + > + return 0; > +} > + > +static int max77759_add_chained_charger(struct i2c_client *client, > + struct max77759_mfd *max77759_mfd, > + struct regmap_irq_chip_data *parent) > +{ > + struct regmap_irq_chip_data *irq_chip_data; > + int ret; > + > + ret = max77759_add_chained_irq_chip(&client->dev, > + max77759_mfd->regmap_charger, > + MAX77759_INT_CHGR, > + parent, > + &max77759_chrg_irq_chip, > + &irq_chip_data); > + if (ret) > + return ret; > + > + ret = devm_mfd_add_devices(&client->dev, PLATFORM_DEVID_AUTO, > + max77759_charger_cells, > + ARRAY_SIZE(max77759_charger_cells), > + NULL, 0, > + regmap_irq_get_domain(irq_chip_data)); > + if (ret) > + return dev_err_probe(&client->dev, ret, > + "failed to add MFD devices (charger)\n"); "sub" or "child" devices. > + > + return 0; > +} > + > +static int max77759_probe(struct i2c_client *client) > +{ > + struct regmap *regmap_top; > + unsigned int pmic_id; > + int ret; > + struct irq_data *irq_data; > + struct max77759_mfd *max77759_mfd; > + unsigned long irq_flags; > + struct regmap_irq_chip_data *irq_chip_data_pmic; Structure ! > + > + regmap_top = devm_regmap_init_i2c(client, &max77759_regmap_config_top); > + if (IS_ERR(regmap_top)) > + return dev_err_probe(&client->dev, PTR_ERR(regmap_top), > + "regmap init failed\n"); Which regmap failed? > + ret = regmap_read(regmap_top, MAX77759_PMIC_REG_PMIC_ID, &pmic_id); > + if (ret) > + return dev_err_probe(&client->dev, ret, > + "Unable to read Device ID\n"); > + > + if (pmic_id != MAX77759_PMIC_REG_PMIC_ID_MAX77759) > + return dev_err_probe(&client->dev, -ENODEV, > + "Unsupported Device ID %#.2x (%d)\n", > + pmic_id, pmic_id); > + > + irq_data = irq_get_irq_data(client->irq); > + if (!irq_data) > + return dev_err_probe(&client->dev, -EINVAL, > + "Invalid IRQ: %d\n", client->irq); > + > + max77759_mfd = devm_kzalloc(&client->dev, sizeof(*max77759_mfd), > + GFP_KERNEL); > + if (!max77759_mfd) > + return -ENOMEM; Device data is almost universally allocated at the top of .probe(). > + max77759_mfd->regmap_top = regmap_top; These two blocks are unrelated - please separate them with a line break here. > + ret = devm_mutex_init(&client->dev, &max77759_mfd->maxq_lock); > + if (ret) > + return ret; > + > + i2c_set_clientdata(client, max77759_mfd); > + > + for (int i = 0; i < ARRAY_SIZE(max77759_i2c_subdevs); ++i) { Any reason for the pre-increment? If not, it's more standards to post-inc. > + ret = max77759_create_i2c_subdev(client, > + max77759_mfd, > + &max77759_i2c_subdevs[i]); > + if (ret) > + return ret; > + } > + > + irq_flags = IRQF_ONESHOT | IRQF_SHARED; > + irq_flags |= irqd_get_trigger_type(irq_data); > + > + ret = devm_regmap_add_irq_chip(&client->dev, max77759_mfd->regmap_top, > + client->irq, irq_flags, 0, > + &max77759_pmic_irq_chip, > + &irq_chip_data_pmic); > + if (ret) > + return dev_err_probe(&client->dev, ret, > + "Failed to add IRQ chip\n"); > + > + /* INTSRC - MaxQ & children */ What does these comments add beyond what is already obvious by the nomenclature? > + ret = max77759_add_chained_maxq(client, max77759_mfd, > + irq_chip_data_pmic); > + if (ret) > + return ret; > + > + /* INTSRC - topsys & children */ > + ret = max77759_add_chained_topsys(client, max77759_mfd, > + irq_chip_data_pmic); > + if (ret) > + return ret; > + > + /* INTSRC - charger & children */ > + ret = max77759_add_chained_charger(client, max77759_mfd, > + irq_chip_data_pmic); > + if (ret) > + return ret; > + > + return devm_mfd_add_devices(&client->dev, PLATFORM_DEVID_AUTO, > + max77759_cells, ARRAY_SIZE(max77759_cells), > + NULL, 0, > + regmap_irq_get_domain(irq_chip_data_pmic)); > +} > + > +static const struct i2c_device_id max77759_i2c_id[] = { > + { "max77759", 0 }, The driver data attribute can be omitted if it's 0. > + { } > +}; > +MODULE_DEVICE_TABLE(i2c, max77759_i2c_id); > + > +static const struct of_device_id max77759_of_id[] = { > + { .compatible = "maxim,max77759", }, > + { } > +}; > +MODULE_DEVICE_TABLE(of, max77759_of_id); > + > +static struct i2c_driver max77759_i2c_driver = { > + .driver = { > + .name = "max77759", > + .of_match_table = max77759_of_id, > + }, > + .probe = max77759_probe, > + .id_table = max77759_i2c_id, > +}; > +module_i2c_driver(max77759_i2c_driver); > + > +MODULE_AUTHOR("André Draszik <andre.draszik@linaro.org>"); > +MODULE_DESCRIPTION("Maxim MAX77759 multi-function core driver"); No it isn't. :) > +MODULE_LICENSE("GPL"); > diff --git a/include/linux/mfd/max77759.h b/include/linux/mfd/max77759.h > new file mode 100644 > index 0000000000000000000000000000000000000000..b038b4e9b748287e23e3a7030496f09dc8bdc816 > --- /dev/null > +++ b/include/linux/mfd/max77759.h > @@ -0,0 +1,98 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > +/* > + * Copyright 2020 Google Inc. > + * Copyright 2025 Linaro Ltd. > + * > + * Client interface for Maxim MAX77759 MFD driver > + */ > + > +#ifndef __LINUX_MFD_MAX77759_H > +#define __LINUX_MFD_MAX77759_H > + > +/* MaxQ opcodes */ > +#define MAX77759_MAXQ_OPCODE_MAXLENGTH 33 > + > +#define MAX77759_MAXQ_OPCODE_GPIO_TRIGGER_READ 0x21 > +#define MAX77759_MAXQ_OPCODE_GPIO_TRIGGER_WRITE 0x22 > +#define MAX77759_MAXQ_OPCODE_GPIO_CONTROL_READ 0x23 > +#define MAX77759_MAXQ_OPCODE_GPIO_CONTROL_WRITE 0x24 > +#define MAX77759_MAXQ_OPCODE_USER_SPACE_READ 0x81 > +#define MAX77759_MAXQ_OPCODE_USER_SPACE_WRITE 0x82 > + > +/* > + * register map (incomplete) - registers not useful for drivers are not > + * declared here > + */ > +/* MaxQ */ > +#define MAX77759_MAXQ_REG_UIC_INT1 0x64 > +#define MAX77759_MAXQ_REG_UIC_INT1_APCMDRESI BIT(7) > +#define MAX77759_MAXQ_REG_UIC_INT1_SYSMSGI BIT(6) > +#define MAX77759_MAXQ_REG_UIC_INT1_GPIO6I BIT(1) > +#define MAX77759_MAXQ_REG_UIC_INT1_GPIO5I BIT(0) > +#define MAX77759_MAXQ_REG_UIC_INT1_GPIOxI(offs, en) (((en) & 1) << (offs)) > +#define MAX77759_MAXQ_REG_UIC_INT1_GPIOxI_MASK(offs) \ > + MAX77759_MAXQ_REG_UIC_INT1_GPIOxI(offs, ~0) > + > +#define MAX77759_MAXQ_REG_UIC_INT2 0x65 > +#define MAX77759_MAXQ_REG_UIC_INT3 0x66 > +#define MAX77759_MAXQ_REG_UIC_INT4 0x67 > +#define MAX77759_MAXQ_REG_UIC_UIC_STATUS1 0x68 > +#define MAX77759_MAXQ_REG_UIC_UIC_STATUS2 0x69 > +#define MAX77759_MAXQ_REG_UIC_UIC_STATUS3 0x6a > +#define MAX77759_MAXQ_REG_UIC_UIC_STATUS4 0x6b > +#define MAX77759_MAXQ_REG_UIC_UIC_STATUS5 0x6c > +#define MAX77759_MAXQ_REG_UIC_UIC_STATUS6 0x6d > +#define MAX77759_MAXQ_REG_UIC_UIC_STATUS7 0x6f > +#define MAX77759_MAXQ_REG_UIC_UIC_STATUS8 0x6f > +#define MAX77759_MAXQ_REG_UIC_INT1_M 0x70 > +#define MAX77759_MAXQ_REG_UIC_INT2_M 0x71 > +#define MAX77759_MAXQ_REG_UIC_INT3_M 0x72 > +#define MAX77759_MAXQ_REG_UIC_INT4_M 0x73 > + > +/* charger */ > +#define MAX77759_CHGR_REG_CHG_INT 0xb0 > +#define MAX77759_CHGR_REG_CHG_INT2 0xb1 > +#define MAX77759_CHGR_REG_CHG_INT_MASK 0xb2 > +#define MAX77759_CHGR_REG_CHG_INT2_MASK 0xb3 > + > +struct max77759_mfd; Place the definition in here instead. > +/** > + * struct max77759_maxq_command - structure containing the MaxQ command to > + * send > + * > + * @length: The number of bytes to send. > + * @cmd: The data to send. > + */ > +struct max77759_maxq_command { > + u8 length; > + u8 cmd[] __counted_by(length); > +}; > + > +/** > + * struct max77759_maxq_response - structure containing the MaxQ response > + * > + * @length: The number of bytes to receive. > + * @rsp: The data received. Must have at least @length bytes space. > + */ > +struct max77759_maxq_response { > + u8 length; > + u8 rsp[] __counted_by(length); > +}; > + > +/** > + * max77759_maxq_command() - issue a MaxQ command and wait for the response > + * and associated data > + * > + * @max77759_mfd: The core max77759 mfd device handle. > + * @cmd: The command to be sent. > + * @rsp: Any response data associated with the command will be copied here; > + * can be %NULL if the command has no response (other than ACK). > + * > + * Return: 0 on success, a negative error number otherwise. > + */ > +int max77759_maxq_command(struct max77759_mfd *max77759_mfd, > + const struct max77759_maxq_command *cmd, > + struct max77759_maxq_response *rsp); > + > +#endif /* __LINUX_MFD_MAX77759_H */ > > -- > 2.49.0.rc0.332.g42c0ae87b1-goog > -- Lee Jones [李琼斯] ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v4 4/6] mfd: max77759: add Maxim MAX77759 core mfd driver 2025-03-14 12:31 ` Lee Jones @ 2025-03-14 15:30 ` André Draszik 2025-03-21 10:54 ` Lee Jones 0 siblings, 1 reply; 12+ messages in thread From: André Draszik @ 2025-03-14 15:30 UTC (permalink / raw) To: Lee Jones Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Linus Walleij, Bartosz Golaszewski, Srinivas Kandagatla, Kees Cook, Gustavo A. R. Silva, Peter Griffin, Tudor Ambarus, Will McVicker, kernel-team, linux-kernel, devicetree, linux-gpio, linux-hardening Hi Lee, Thanks for your review! On Fri, 2025-03-14 at 12:31 +0000, Lee Jones wrote: > On Wed, 12 Mar 2025, André Draszik wrote: > > > The Maxim MAX77759 is a companion PMIC for USB Type-C applications and > > includes Battery Charger, Fuel Gauge, temperature sensors, USB Type-C > > Port Controller (TCPC), NVMEM, and a GPIO expander. > > > > Fuel Gauge and TCPC have separate and independent I2C addresses, > > register maps, and interrupt lines and are therefore excluded from the > > MFD core device driver here. > > > > The GPIO and NVMEM interfaces are accessed via specific commands to the > > built-in microprocessor. This driver implements an API that client > > drivers can use for accessing those. > > > > Signed-off-by: André Draszik <andre.draszik@linaro.org> > > > > --- > > v4: > > * add missing build_bug.h include > > * update an irq chip comment > > * fix a whitespace in register definitions > > > > v2: > > * add kernel doc for max77759_maxq_command() and related structs > > * fix an msec / usec typo > > * add missing error handling of devm_mutex_init() (Christophe) > > * align sentinel in max77759_of_id[] with max77759_i2c_id[] > > (Christophe) > > * some tidy-ups in max77759_maxq_command() (Christophe) > > --- > > MAINTAINERS | 2 + > > drivers/mfd/Kconfig | 20 ++ > > drivers/mfd/Makefile | 1 + > > drivers/mfd/max77759.c | 738 +++++++++++++++++++++++++++++++++++++++++++ > > include/linux/mfd/max77759.h | 98 ++++++ > > 5 files changed, 859 insertions(+) > > > > diff --git a/MAINTAINERS b/MAINTAINERS > > index a45d1bd64d116d14bc05a64fa3da852a41e1de7f..38844ea24e464d0b58f8852b79e2b94f18d48998 100644 > > --- a/MAINTAINERS > > +++ b/MAINTAINERS > > @@ -14352,6 +14352,8 @@ M: André Draszik <andre.draszik@linaro.org> > > L: linux-kernel@vger.kernel.org > > S: Maintained > > F: Documentation/devicetree/bindings/*/maxim,max77759*.yaml > > +F: drivers/mfd/max77759.c > > +F: include/linux/mfd/max77759.h > > > > MAXIM MAX77802 PMIC REGULATOR DEVICE DRIVER > > M: Javier Martinez Canillas <javier@dowhile0.org> > > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig > > index d44c69bb3dfd52d53fe26aa2d5e5ace346448f57..1d72bf086401064608cc41e9f9a0af044c6df68e 100644 > > --- a/drivers/mfd/Kconfig > > +++ b/drivers/mfd/Kconfig > > @@ -930,6 +930,26 @@ config MFD_MAX77714 > > drivers must be enabled in order to use each functionality of the > > device. > > > > +config MFD_MAX77759 > > + tristate "Maxim Integrated MAX77759 PMIC" > > + depends on I2C > > + depends on OF > > + select IRQ_DOMAIN > > + select MFD_CORE > > + select REGMAP_I2C > > + select REGMAP_IRQ > > + help > > + Say yes here to add support for Maxim Integrated MAX77759. > > + This is a companion Power Management IC for USB Type-C applications > > + with Battery Charger, Fuel Gauge, temperature sensors, USB Type-C > > + Port Controller (TCPC), NVMEM, and additional GPIO interfaces. > > + This driver provides common support for accessing the device; > > + additional drivers must be enabled in order to use the functionality > > + of the device. > > + > > + To compile this driver as a module, choose M here: the module will be > > + called max77759. > > + > > config MFD_MAX77843 > > bool "Maxim Semiconductor MAX77843 PMIC Support" > > depends on I2C=y > > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile > > index 9220eaf7cf1255a8922430fe3e50e41771bbaa60..cc9362afd8f060d64ca0f0f028a0d7cfe9cfe512 100644 > > --- a/drivers/mfd/Makefile > > +++ b/drivers/mfd/Makefile > > @@ -169,6 +169,7 @@ 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_MAX77759) += max77759.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/max77759.c b/drivers/mfd/max77759.c > > new file mode 100644 > > index 0000000000000000000000000000000000000000..a4f1616d600bde250a73b69464ada1fbf6d7e67e > > --- /dev/null > > +++ b/drivers/mfd/max77759.c > > @@ -0,0 +1,738 @@ > > +// SPDX-License-Identifier: GPL-2.0-only > > +// > > +// Copyright 2020 Google Inc > > +// Copyright 2025 Linaro Ltd. > > +// > > +// Core MFD driver for Maxim MAX77759 companion PMIC for USB Type-C > > Only the SPDX should be in C++ style comments. > > Please C block-comment the rest. It's a pity that there is different guidance on how to add the comment, e.g. for regulators the preference is C++ style, see e.g. https://lore.kernel.org/all/ZulJuCu-QcMYrphP@finisterre.sirena.org.uk/ I'd rather have one style for max77759, the nvmem and gpio parts also use C++ for this, and some drivers in mfd/ also use C++ style. It'd be nice if there was one style :-) Do you want me to change all drivers in this series? > Drop all references to MFD - it's not a real thing. Sure. > > +#include <linux/array_size.h> > > +#include <linux/bitfield.h> > > +#include <linux/bits.h> > > +#include <linux/build_bug.h> > > +#include <linux/cleanup.h> > > +#include <linux/completion.h> > > +#include <linux/dev_printk.h> > > +#include <linux/device.h> > > +#include <linux/err.h> > > +#include <linux/i2c.h> > > +#include <linux/init.h> > > +#include <linux/interrupt.h> > > +#include <linux/irq.h> > > +#include <linux/jiffies.h> > > +#include <linux/mfd/core.h> > > +#include <linux/mfd/max77759.h> > > +#include <linux/mod_devicetable.h> > > +#include <linux/module.h> > > +#include <linux/mutex.h> > > +#include <linux/of.h> > > +#include <linux/overflow.h> > > +#include <linux/regmap.h> > > This is a huge list of includes. > > Please comb them to see if you really do need all of them. This is the minimal list as it's usually preferred to not rely on implicit includes. It is good practice to directly include the headers used, it avoids implicit dependencies and spurious breakage if someone rearranges headers and causes the implicit include to vanish. > > > +/* registers - registers useful to drivers are declared in the public header */ > > This sort of goes without saying. Suggest you remove this comment. > > > +/* PMIC / TOP */ > > +#define MAX77759_PMIC_REG_PMIC_ID 0x00 > > +#define MAX77759_PMIC_REG_PMIC_ID_MAX77759 59 > > Is this a register or a value? It's a value, which the suffix in the macro name was meant to convey :-) 59 is the value (in decimal) for max77759 that the ID register is expected to contain. > > > +#define MAX77759_PMIC_REG_PMIC_REVISION 0x01 > > +#define MAX77759_PMIC_REG_OTP_REVISION 0x02 > > + > > +#define MAX77759_PMIC_REG_INTSRC 0x22 > > +#define MAX77759_PMIC_REG_INTSRCMASK 0x23 > > +#define MAX77759_PMIC_REG_INTSRC_MAXQ BIT(3) > > +#define MAX77759_PMIC_REG_INTSRC_TOPSYS BIT(1) > > +#define MAX77759_PMIC_REG_INTSRC_CHGR BIT(0) > > These look like bit offsets rather than reg addresses? Of course, could you please clarify what you're hinting at here? > > +#define MAX77759_PMIC_REG_TOPSYS_INT 0x24 > > +#define MAX77759_PMIC_REG_TOPSYS_INT_MASK 0x26 > > +#define MAX77759_PMIC_REG_TOPSYS_INT_TSHDN BIT(6) > > +#define MAX77759_PMIC_REG_TOPSYS_INT_SYSOVLO BIT(5) > > +#define MAX77759_PMIC_REG_TOPSYS_INT_SYSUVLO BIT(4) > > +#define MAX77759_PMIC_REG_TOPSYS_INT_FSHIP BIT(0) > > + > > +#define MAX77759_PMIC_REG_I2C_CNFG 0x40 > > +#define MAX77759_PMIC_REG_SWRESET 0x50 > > +#define MAX77759_PMIC_REG_CONTROL_FG 0x51 > > +#define MAX77759_PMIC_REG_LAST_REGISTER MAX77759_PMIC_REG_CONTROL_FG > > You could just use MAX77759_PMIC_REG_CONTROL_FG in-place? I think this makes it more obvious in the regmap definition below - struct regmap_config::max_register vs struct regmap_config::num_reg_defaults_raw > > > +/* MaxQ */ > > How does this improve readability? The name is already in the define. Due to Max77759 having different functional blocks, it appeared worthwhile to visually separate the registers as well. > If you want to provide a comment, which isn't a bad idea, extend the > meaning and describe the purpose instead. Will do. > > +#define MAX77759_MAXQ_REG_AP_DATAOUT0 0x81 > > +#define MAX77759_MAXQ_REG_AP_DATAOUT32 0xa1 > > +#define MAX77759_MAXQ_REG_AP_MESSAGESZ_MAX (MAX77759_MAXQ_REG_AP_DATAOUT32 \ > > + - MAX77759_MAXQ_REG_AP_DATAOUT0 \ > > + + 1) > > This is a strange way to line things up. > > > +#define MAX77759_MAXQ_REG_AP_DATAIN0 0xb1 > > +#define MAX77759_MAXQ_REG_LAST_REGISTER 0xe0 > > What is the last register's purpose, besides being the last one? As above, I think this makes it more obvious in the regmap definition below - struct regmap_config::max_register vs struct regmap_config::num_reg_defaults_raw > > > +/* charger */ > > Not sure this provides much, but if you insist on keeping it, all > comments should start with an upper-case char here. Again, different functional block's register set starts here :-) > > > +#define MAX77759_CHGR_REG_LAST_REGISTER 0xcc > > Where are the other registers? The intention is to add them once we start working on charger. > > > +enum max77759_i2c_subdev_id { > > + MAX77759_I2C_SUBDEV_ID_MAXQ, > > + MAX77759_I2C_SUBDEV_ID_CHARGER, > > Are these truly arbitrary or are you relying on the fact that the > compiler usually starts from 0 and incs by 1? It's not arbitrary. A conforming compiler is required to start with 0, this is part of the C standard, e.g. https://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf 6.7.2.2.3 > > > +}; > > + > > +struct max77759_mfd { > > Drop all "mfd"ness. sure > > > + /* protecting MaxQ commands - only one can be active */ > > "This protects" OK > > > + struct mutex maxq_lock; > > + struct regmap *regmap_maxq; > > + struct completion cmd_done; > > + > > + struct regmap *regmap_top; > > What is top? One of its functional blocks as per the data sheet. I think it makes sense to use nomenclature from it. > > > + struct regmap *regmap_charger; > > +}; > > + > > +struct max77759_i2c_subdev { > > + enum max77759_i2c_subdev_id id; > > + const struct regmap_config *cfg; > > + u16 i2c_address; > > +}; > > + > > +/* TOP registers */ > > +static const struct regmap_range max77759_top_registers[] = { > > + regmap_reg_range(0x00, 0x02), > > + regmap_reg_range(0x22, 0x24), > > + regmap_reg_range(0x26, 0x26), > > + regmap_reg_range(0x40, 0x40), > > + regmap_reg_range(0x50, 0x51), > > What are these magic numbers? Can you define them? This is on purpose, as I think it makes it much easier to have the ranges as numbers, otherwise you always have to look them up in the header when trying to verify correctness or simply looking at this. Additionally, things wouldn't be nicely aligned anymore, Without data sheet, whether these are numbers or macro names makes no difference, and it's impossible to reason about them. With data sheet, it's easier this way to compare. But I'll change them. > > > +}; > > + > > +static const struct regmap_range max77759_top_ro_registers[] = { > > + regmap_reg_range(0x00, 0x02), > > + regmap_reg_range(0x22, 0x22), > > +}; > > + > > +static const struct regmap_range max77759_top_volatile_registers[] = { > > + regmap_reg_range(0x22, 0x22), > > + regmap_reg_range(0x24, 0x24), > > +}; > > + > > +static const struct regmap_access_table max77759_top_wr_table = { > > + .yes_ranges = max77759_top_registers, > > + .n_yes_ranges = ARRAY_SIZE(max77759_top_registers), > > + .no_ranges = max77759_top_ro_registers, > > + .n_no_ranges = ARRAY_SIZE(max77759_top_ro_registers), > > +}; > > + > > +static const struct regmap_access_table max77759_top_rd_table = { > > + .yes_ranges = max77759_top_registers, > > + .n_yes_ranges = ARRAY_SIZE(max77759_top_registers), > > +}; > > + > > +static const struct regmap_access_table max77759_top_volatile_table = { > > + .yes_ranges = max77759_top_volatile_registers, > > + .n_yes_ranges = ARRAY_SIZE(max77759_top_volatile_registers), > > +}; > > + > > +static const struct regmap_config max77759_regmap_config_top = { > > + .name = "top", > > + .reg_bits = 8, > > + .val_bits = 8, > > + .max_register = MAX77759_PMIC_REG_LAST_REGISTER, > > + .wr_table = &max77759_top_wr_table, > > + .rd_table = &max77759_top_rd_table, > > + .volatile_table = &max77759_top_volatile_table, > > + .num_reg_defaults_raw = MAX77759_PMIC_REG_LAST_REGISTER + 1, > > + .cache_type = REGCACHE_MAPLE, > > +}; > > + > > +/* MaxQ registers */ > > Again, this is obvious due to the nomenclature. Yes, but it gives a visual separation. I'll drop it. > > > +static const struct regmap_range max77759_maxq_registers[] = { > > + regmap_reg_range(0x60, 0x73), > > + regmap_reg_range(0x81, 0xa1), > > + regmap_reg_range(0xb1, 0xd1), > > + regmap_reg_range(0xe0, 0xe0), > > +}; > > + > > +static const struct regmap_range max77759_maxq_ro_registers[] = { > > + regmap_reg_range(0x60, 0x63), > > + regmap_reg_range(0x68, 0x6f), > > + regmap_reg_range(0xb1, 0xd1), > > +}; > > + > > +static const struct regmap_range max77759_maxq_volatile_registers[] = { > > + regmap_reg_range(0x64, 0x6f), > > + regmap_reg_range(0xb1, 0xd1), > > + regmap_reg_range(0xe0, 0xe0), > > +}; > > + > > +static const struct regmap_access_table max77759_maxq_wr_table = { > > + .yes_ranges = max77759_maxq_registers, > > + .n_yes_ranges = ARRAY_SIZE(max77759_maxq_registers), > > + .no_ranges = max77759_maxq_ro_registers, > > + .n_no_ranges = ARRAY_SIZE(max77759_maxq_ro_registers), > > +}; > > + > > +static const struct regmap_access_table max77759_maxq_rd_table = { > > + .yes_ranges = max77759_maxq_registers, > > + .n_yes_ranges = ARRAY_SIZE(max77759_maxq_registers), > > +}; > > + > > +static const struct regmap_access_table max77759_maxq_volatile_table = { > > + .yes_ranges = max77759_maxq_volatile_registers, > > + .n_yes_ranges = ARRAY_SIZE(max77759_maxq_volatile_registers), > > +}; > > + > > +static const struct regmap_config max77759_regmap_config_maxq = { > > + .name = "maxq", > > + .reg_bits = 8, > > + .val_bits = 8, > > + .max_register = MAX77759_MAXQ_REG_LAST_REGISTER, > > + .wr_table = &max77759_maxq_wr_table, > > + .rd_table = &max77759_maxq_rd_table, > > + .volatile_table = &max77759_maxq_volatile_table, > > + .num_reg_defaults_raw = MAX77759_MAXQ_REG_LAST_REGISTER + 1, > > + .cache_type = REGCACHE_MAPLE, > > +}; > > + > > +/* charger registers */ > > +static const struct regmap_range max77759_charger_registers[] = { > > + regmap_reg_range(0xb0, 0xcc), > > +}; > > + > > +static const struct regmap_range max77759_charger_ro_registers[] = { > > + regmap_reg_range(0xb4, 0xb8), > > +}; > > + > > +static const struct regmap_range max77759_charger_volatile_registers[] = { > > + regmap_reg_range(0xb0, 0xb1), > > + regmap_reg_range(0xb4, 0xb8), > > +}; > > + > > +static const struct regmap_access_table max77759_charger_wr_table = { > > + .yes_ranges = max77759_charger_registers, > > + .n_yes_ranges = ARRAY_SIZE(max77759_charger_registers), > > + .no_ranges = max77759_charger_ro_registers, > > + .n_no_ranges = ARRAY_SIZE(max77759_charger_ro_registers), > > +}; > > + > > +static const struct regmap_access_table max77759_charger_rd_table = { > > + .yes_ranges = max77759_charger_registers, > > + .n_yes_ranges = ARRAY_SIZE(max77759_charger_registers), > > +}; > > + > > +static const struct regmap_access_table max77759_charger_volatile_table = { > > + .yes_ranges = max77759_charger_volatile_registers, > > + .n_yes_ranges = ARRAY_SIZE(max77759_charger_volatile_registers), > > +}; > > + > > +static const struct regmap_config max77759_regmap_config_charger = { > > + .name = "charger", > > + .reg_bits = 8, > > + .val_bits = 8, > > + .max_register = MAX77759_CHGR_REG_LAST_REGISTER, > > + .wr_table = &max77759_charger_wr_table, > > + .rd_table = &max77759_charger_rd_table, > > + .volatile_table = &max77759_charger_volatile_table, > > + .num_reg_defaults_raw = MAX77759_CHGR_REG_LAST_REGISTER + 1, > > + .cache_type = REGCACHE_MAPLE, > > +}; > > + > > +/* > > + * Interrupts - with the following interrupt hierarchy: > > + * pmic IRQs (INTSRC) > > + * - MAXQ_INT: MaxQ IRQs > > + * - UIC_INT1 > > + * - APCmdResI > > + * - SysMsgI > > + * - GPIOxI > > + * - TOPSYS_INT: topsys > > + * - TOPSYS_INT > > + * - TSHDN_INT > > + * - SYSOVLO_INT > > + * - SYSUVLO_INT > > + * - FSHIP_NOT_RD > > + * - CHGR_INT: charger > > + * - CHG_INT > > + * - CHG_INT2 > > + */ > > +enum { > > + MAX77759_INT_MAXQ, > > + MAX77759_INT_TOPSYS, > > + MAX77759_INT_CHGR, > > +}; > > + > > +enum { > > + MAX77759_TOPSYS_INT_TSHDN, > > + MAX77759_TOPSYS_INT_SYSOVLO, > > + MAX77759_TOPSYS_INT_SYSUVLO, > > + MAX77759_TOPSYS_INT_FSHIP_NOT_RD, > > +}; > > + > > +enum { > > + MAX77759_MAXQ_INT_APCMDRESI, > > + MAX77759_MAXQ_INT_SYSMSGI, > > + MAX77759_MAXQ_INT_GPIO, > > + MAX77759_MAXQ_INT_UIC1, > > + MAX77759_MAXQ_INT_UIC2, > > + MAX77759_MAXQ_INT_UIC3, > > + MAX77759_MAXQ_INT_UIC4, > > +}; > > + > > +enum { > > + MAX77759_CHARGER_INT_1, > > + MAX77759_CHARGER_INT_2, > > +}; > > + > > +static const struct regmap_irq max77759_pmic_irqs[] = { > > + REGMAP_IRQ_REG(MAX77759_INT_MAXQ, 0, MAX77759_PMIC_REG_INTSRC_MAXQ), > > + REGMAP_IRQ_REG(MAX77759_INT_TOPSYS, 0, MAX77759_PMIC_REG_INTSRC_TOPSYS), > > + REGMAP_IRQ_REG(MAX77759_INT_CHGR, 0, MAX77759_PMIC_REG_INTSRC_CHGR), > > +}; > > + > > +static const struct regmap_irq max77759_maxq_irqs[] = { > > + REGMAP_IRQ_REG(MAX77759_MAXQ_INT_APCMDRESI, > > + 0, MAX77759_MAXQ_REG_UIC_INT1_APCMDRESI), > > Use up to 100-chars here to avoid the line-break. Will do. > This isn't 1999 anymore - we have wide-screen 4k monitors. =:-) > > > + REGMAP_IRQ_REG(MAX77759_MAXQ_INT_SYSMSGI, > > + 0, MAX77759_MAXQ_REG_UIC_INT1_SYSMSGI), > > + REGMAP_IRQ_REG(MAX77759_MAXQ_INT_GPIO, 0, GENMASK(1, 0)), > > + REGMAP_IRQ_REG(MAX77759_MAXQ_INT_UIC1, 0, GENMASK(5, 2)), > > + REGMAP_IRQ_REG(MAX77759_MAXQ_INT_UIC2, 1, GENMASK(7, 0)), > > + REGMAP_IRQ_REG(MAX77759_MAXQ_INT_UIC3, 2, GENMASK(7, 0)), > > + REGMAP_IRQ_REG(MAX77759_MAXQ_INT_UIC4, 3, GENMASK(7, 0)), > > +}; > > + > > +static const struct regmap_irq max77759_topsys_irqs[] = { > > + REGMAP_IRQ_REG(MAX77759_TOPSYS_INT_TSHDN, > > + 0, MAX77759_PMIC_REG_TOPSYS_INT_TSHDN), > > + REGMAP_IRQ_REG(MAX77759_TOPSYS_INT_SYSOVLO, > > + 0, MAX77759_PMIC_REG_TOPSYS_INT_SYSOVLO), > > + REGMAP_IRQ_REG(MAX77759_TOPSYS_INT_SYSUVLO, > > + 0, MAX77759_PMIC_REG_TOPSYS_INT_SYSUVLO), > > + REGMAP_IRQ_REG(MAX77759_TOPSYS_INT_FSHIP_NOT_RD, > > + 0, MAX77759_PMIC_REG_TOPSYS_INT_FSHIP), > > +}; > > + > > +static const struct regmap_irq max77759_chgr_irqs[] = { > > + REGMAP_IRQ_REG(MAX77759_CHARGER_INT_1, 0, GENMASK(7, 0)), > > + REGMAP_IRQ_REG(MAX77759_CHARGER_INT_2, 1, GENMASK(7, 0)), > > +}; > > + > > +static const struct regmap_irq_chip max77759_pmic_irq_chip = { > > + .name = "max77759-pmic", > > + /* INTSRC is read-only and doesn't require clearing */ > > + .status_base = MAX77759_PMIC_REG_INTSRC, > > + .mask_base = MAX77759_PMIC_REG_INTSRCMASK, > > + .num_regs = 1, > > + .irqs = max77759_pmic_irqs, > > + .num_irqs = ARRAY_SIZE(max77759_pmic_irqs), > > +}; > > + > > +/* > > + * We can let regmap-irq auto-ack the topsys interrupt bits as required, but > > + * for all others the individual drivers need to know which interrupt bit > > + * exactly is set inside their interrupt handlers, and therefore we can not set > > + * .ack_base for those. > > + */ > > Nice - thanks for this! > > > +static const struct regmap_irq_chip max77759_maxq_irq_chip = { > > + .name = "max77759-maxq", > > + .domain_suffix = "MAXQ", > > + .status_base = MAX77759_MAXQ_REG_UIC_INT1, > > + .mask_base = MAX77759_MAXQ_REG_UIC_INT1_M, > > + .num_regs = 4, > > + .irqs = max77759_maxq_irqs, > > + .num_irqs = ARRAY_SIZE(max77759_maxq_irqs), > > +}; > > + > > +static const struct regmap_irq_chip max77759_topsys_irq_chip = { > > + .name = "max77759-topsys", > > + .domain_suffix = "TOPSYS", > > + .status_base = MAX77759_PMIC_REG_TOPSYS_INT, > > + .mask_base = MAX77759_PMIC_REG_TOPSYS_INT_MASK, > > + .ack_base = MAX77759_PMIC_REG_TOPSYS_INT, > > + .num_regs = 1, > > + .irqs = max77759_topsys_irqs, > > + .num_irqs = ARRAY_SIZE(max77759_topsys_irqs), > > +}; > > + > > +static const struct regmap_irq_chip max77759_chrg_irq_chip = { > > + .name = "max77759-chgr", > > + .domain_suffix = "CHGR", > > + .status_base = MAX77759_CHGR_REG_CHG_INT, > > + .mask_base = MAX77759_CHGR_REG_CHG_INT_MASK, > > + .num_regs = 2, > > + .irqs = max77759_chgr_irqs, > > + .num_irqs = ARRAY_SIZE(max77759_chgr_irqs), > > +}; > > + > > +static const struct max77759_i2c_subdev max77759_i2c_subdevs[] = { > > + { > > + .id = MAX77759_I2C_SUBDEV_ID_MAXQ, > > + /* I2C address is same as top's */ > > It took me a moment to figure out why this is relevant. > > Maybe shift this down, so it lines-up with the next element. OK. > > > + .cfg = &max77759_regmap_config_maxq, > > + }, > > + { > > + .id = MAX77759_I2C_SUBDEV_ID_CHARGER, > > + .cfg = &max77759_regmap_config_charger, > > + .i2c_address = 0x69, > > + }, > > +}; > > + > > +static const struct resource max77759_gpio_resources[] = { > > + DEFINE_RES_IRQ_NAMED(MAX77759_MAXQ_INT_GPIO, "GPI"), > > +}; > > + > > +static const struct resource max77759_charger_resources[] = { > > + DEFINE_RES_IRQ_NAMED(MAX77759_CHARGER_INT_1, "INT1"), > > + DEFINE_RES_IRQ_NAMED(MAX77759_CHARGER_INT_2, "INT2"), > > +}; > > + > > +static const struct mfd_cell max77759_cells[] = { > > + MFD_CELL_OF("max77759-nvmem", NULL, NULL, 0, 0, > > + "maxim,max77759-nvmem"), > > +}; > > + > > +static const struct mfd_cell max77759_maxq_cells[] = { > > + MFD_CELL_OF("max77759-gpio", max77759_gpio_resources, NULL, 0, 0, > > + "maxim,max77759-gpio"), > > +}; > > + > > +static const struct mfd_cell max77759_charger_cells[] = { > > + MFD_CELL_RES("max77759-charger", max77759_charger_resources), > > +}; > > Are there situations where only 1 or 2 of these will be used? > > If they will all be used all of the time, save yourself a bunch of lines > and complexity and add them all to a single struct. Hm, they all have different interrupt domains. This was inspired by intel_soc_pmic_bxtwc.c I'll see what I can do. > > > +int max77759_maxq_command(struct max77759_mfd *max77759_mfd, > > + const struct max77759_maxq_command *cmd, > > + struct max77759_maxq_response *rsp) > > +{ > > + DEFINE_FLEX(struct max77759_maxq_response, _rsp, rsp, length, 1); > > + int ret; > > Nit: Please place this at the bottom of the block. OK. > > *twitch* > > > + struct device *dev = regmap_get_device(max77759_mfd->regmap_maxq); > > > + static const unsigned int timeout_ms = 200; > > Why 200? This is what downstream uses, I can add a comment to say so. > > > + > > + if (cmd->length > MAX77759_MAXQ_REG_AP_MESSAGESZ_MAX) > > + return -EINVAL; > > + > > + /* rsp is allowed to be NULL. In that case we do need a temporary. */ > > More details please. > > Why is NULL valid? The kernel doc explains it: * @rsp: Any response data associated with the command will be copied here; * can be %NULL if the command has no response (other than ACK). Do you want me to duplicate this information? > > Why does it need a placeholder? What are you using as the placeholder? Here, we still need a location to write the response to, as we need to verify that the command was indeed completed correctly. > > > + if (!rsp) > > + rsp = _rsp; > > + > > + BUILD_BUG_ON(MAX77759_MAXQ_OPCODE_MAXLENGTH > > + != MAX77759_MAXQ_REG_AP_MESSAGESZ_MAX); > > Probably useful during development. Let useful in production code. This build bug ensures that if anybody ever changed one of the macros in an incorrect way, things fall apart. One of the macros is part of the API, the other one is derived internally in this driver. Better to fail loudly, than to silently ignore such errors. I'd rather leave this check in. > > > + if (!rsp->length || rsp->length > MAX77759_MAXQ_REG_AP_MESSAGESZ_MAX) > > + return -EINVAL; > > + > > + guard(mutex)(&max77759_mfd->maxq_lock); > > + > > + reinit_completion(&max77759_mfd->cmd_done); > > + > > + /* write the opcode and data */ > > Upper case throughout please. OK > > + ret = regmap_bulk_write(max77759_mfd->regmap_maxq, > > + MAX77759_MAXQ_REG_AP_DATAOUT0, cmd->cmd, > > + cmd->length); > > + if (!ret && cmd->length < MAX77759_MAXQ_REG_AP_MESSAGESZ_MAX) > > + /* writing the last byte triggers MaxQ */ > > + ret = regmap_write(max77759_mfd->regmap_maxq, > > + MAX77759_MAXQ_REG_AP_DATAOUT32, 0); > > + if (ret) { > > Do this first. > Then you only need to check: > > cmd->length < MAX77759_MAXQ_REG_AP_MESSAGESZ_MAX > > ... in the subsequent statement. > > But what happens if: > > !ret && cmd->length >= MAX77759_MAXQ_REG_AP_MESSAGESZ_MAX > ^ > Is that okay? Nothing to be done? Yes, that is OK for length==MAX77759_MAXQ_REG_AP_MESSAGESZ_MAX The command buffer is MAX77759_MAXQ_REG_AP_MESSAGESZ_MAX byte long. If the message to be sent is smaller than that, then the last byte still needs to be written to MaxQ (DATAOUT32 register above) to submit the command ("/* writing the last byte triggers MaxQ */" comment above). So there are three cases to cater for: message > MESSAGESZ_MAX -> return -EINVAL at start of function message == MESSAGESZ_MAX -> just write message using regmap_bulk_write(), DATAOUT32 will be written due to cmd->length message < MESSAGESZ_MAX -> write available bytes, then do dummy write to DATAOUT32 As is, the code ensures that, it does the extra DATAOUT32 write only if the bulk write succeeded and if necessary. It avoids duplicated tests of the regmap_bulk_write() / regmap_write() return value. With your suggestion, the code would become more complicated I believe: /* write the opcode and data */ ret = regmap_bulk_write(max77759_mfd->regmap_maxq, MAX77759_MAXQ_REG_AP_DATAOUT0, cmd->cmd, cmd->length); if (ret) { dev_err(dev, "write data failed: %d\n", ret); return ret; } if (cmd->length < MAX77759_MAXQ_REG_AP_MESSAGESZ_MAX) { /* writing the last byte triggers MaxQ */ ret = regmap_write(max77759_mfd->regmap_maxq, MAX77759_MAXQ_REG_AP_DATAOUT32, 0); if (ret) { dev_err(dev, "write data failed: %d\n", ret); return ret; } } vs at the moment: /* write the opcode and data */ ret = regmap_bulk_write(max77759_mfd->regmap_maxq, MAX77759_MAXQ_REG_AP_DATAOUT0, cmd->cmd, cmd->length); if (!ret && cmd->length < MAX77759_MAXQ_REG_AP_MESSAGESZ_MAX) /* writing the last byte triggers MaxQ */ ret = regmap_write(max77759_mfd->regmap_maxq, MAX77759_MAXQ_REG_AP_DATAOUT32, 0); if (ret) { dev_err(dev, "write data failed: %d\n", ret); return ret; } > > > + dev_warn(dev, "write data failed: %d\n", ret); > > This should be dev_err(). OK. > > > + return ret; > > + } > > + > > + /* wait for response from MaxQ */ > > + if (!wait_for_completion_timeout(&max77759_mfd->cmd_done, > > + msecs_to_jiffies(timeout_ms))) { > > + dev_err(dev, "timed out waiting for data\n"); > > What data? I can change to 'reply' > > > + return -ETIMEDOUT; > > + } > > + > > + ret = regmap_bulk_read(max77759_mfd->regmap_maxq, > > + MAX77759_MAXQ_REG_AP_DATAIN0, > > + rsp->rsp, rsp->length); > > + if (ret) { > > + dev_warn(dev, "read data failed: %d\n", ret); > > As above. OK > > > + return ret; > > + } > > + > > + /* > > + * As per the protocol, the first byte of the reply will match the > > + * request. > > + */ > > + if (cmd->cmd[0] != rsp->rsp[0]) { > > + dev_warn(dev, "unexpected opcode response for %#.2x: %*ph\n", > > + cmd->cmd[0], (int)rsp->length, rsp->rsp); > > + return -EIO; > > + } > > + > > + return 0; > > +} > > +EXPORT_SYMBOL_GPL(max77759_maxq_command); > > + > > +static irqreturn_t apcmdres_irq_handler(int irq, void *irq_data) > > +{ > > + struct max77759_mfd *max77759_mfd = irq_data; > > + > > + regmap_write(max77759_mfd->regmap_maxq, MAX77759_MAXQ_REG_UIC_INT1, > > + MAX77759_MAXQ_REG_UIC_INT1_APCMDRESI); > > + > > + complete(&max77759_mfd->cmd_done); > > + > > + return IRQ_HANDLED; > > +} > > + > > +static int max77759_create_i2c_subdev(struct i2c_client *client, > > + struct max77759_mfd *max77759_mfd, > > + const struct max77759_i2c_subdev *sd) > > +{ > > + struct i2c_client *sub; > > + struct regmap *regmap; > > + int ret; > > + > > + if (sd->i2c_address) { > > + sub = devm_i2c_new_dummy_device(&client->dev, > > + client->adapter, > > + sd->i2c_address); > > + > > + if (IS_ERR(sub)) > > + return dev_err_probe(&client->dev, PTR_ERR(sub), > > + "failed to claim i2c device %s\n", > > "I2C" OK > > > + sd->cfg->name); > > + } else { > > + sub = client; > > Assign during declaration, with a comment, then dispose of this leg. OK > > > + } > > + > > + regmap = devm_regmap_init_i2c(sub, sd->cfg); > > + if (IS_ERR(regmap)) > > + return dev_err_probe(&sub->dev, PTR_ERR(regmap), > > + "regmap init failed\n"); > > + > > + ret = regmap_attach_dev(&client->dev, regmap, sd->cfg); > > + if (ret) > > + return dev_err_probe(&client->dev, ret, > > + "regmap attach failed\n"); > > + > > + if (sd->id == MAX77759_I2C_SUBDEV_ID_MAXQ) > > + max77759_mfd->regmap_maxq = regmap; > > + else if (sd->id == MAX77759_I2C_SUBDEV_ID_CHARGER) > > + max77759_mfd->regmap_charger = regmap; > > + > > + return 0; > > +} > > + > > +static int max77759_add_chained_irq_chip(struct device *dev, > > + struct regmap *regmap, > > + int pirq, > > + struct regmap_irq_chip_data *parent, > > + const struct regmap_irq_chip *chip, > > + struct regmap_irq_chip_data **data) > > +{ > > + int irq, ret; > > + > > + irq = regmap_irq_get_virq(parent, pirq); > > + if (irq < 0) > > + return dev_err_probe(dev, irq, > > + "Failed to get parent vIRQ(%d) for chip %s\n", > > + pirq, chip->name); > > + > > + ret = devm_regmap_add_irq_chip(dev, regmap, irq, > > + IRQF_ONESHOT | IRQF_SHARED, 0, chip, > > + data); > > + if (ret) > > + return dev_err_probe(dev, ret, "Failed to add %s IRQ chip\n", > > Some of your messages start will uppercase whilst others do not. > > *twitch* > > Please pick one and use throughout. OK :-) > > > + chip->name); > > + > > + return 0; > > +} > > + > > +static int max77759_add_chained_maxq(struct i2c_client *client, > > + struct max77759_mfd *max77759_mfd, > > + struct regmap_irq_chip_data *parent) > > +{ > > + struct regmap_irq_chip_data *irq_chip_data; > > + int ret; > > Bottom please. > > Some people like reverse Christmas tree. > > Structure helps me sleep at night! =;-) I usually declare in order of use - I'll change ordering; don't want to rob you of your sleep :-)) > > > + int apcmdres_irq; > > + > > + ret = max77759_add_chained_irq_chip(&client->dev, > > + max77759_mfd->regmap_maxq, > > + MAX77759_INT_MAXQ, > > + parent, > > + &max77759_maxq_irq_chip, > > + &irq_chip_data); > > + if (ret) > > + return ret; > > + > > + /* > > + * We need to register our own IRQ handler before any MFD cells, to > > + * ensure client drivers can use our MaxQ interface APIs without > > + * any race conditions. > > + */ > > This is normal. Will drop. > > + init_completion(&max77759_mfd->cmd_done); > > + apcmdres_irq = regmap_irq_get_virq(irq_chip_data, > > + MAX77759_MAXQ_INT_APCMDRESI); > > + > > + ret = devm_request_threaded_irq(&client->dev, apcmdres_irq, > > + NULL, apcmdres_irq_handler, > > + IRQF_ONESHOT | IRQF_SHARED, > > + dev_name(&client->dev), max77759_mfd); > > + if (ret) > > + return dev_err_probe(&client->dev, ret, > > + "MAX77759_MAXQ_INT_APCMDRESI failed\n"); > > + > > + ret = devm_mfd_add_devices(&client->dev, PLATFORM_DEVID_AUTO, > > + max77759_maxq_cells, > > + ARRAY_SIZE(max77759_maxq_cells), > > + NULL, 0, > > + regmap_irq_get_domain(irq_chip_data)); > > + if (ret) > > + return dev_err_probe(&client->dev, ret, > > + "failed to add MFD devices (MaxQ)\n"); > > + > > + return 0; > > +} > > + > > +static int max77759_add_chained_topsys(struct i2c_client *client, > > + struct max77759_mfd *max77759_mfd, > > + struct regmap_irq_chip_data *parent) > > +{ > > + struct regmap_irq_chip_data *irq_chip_data; > > + int ret; > > + > > + ret = max77759_add_chained_irq_chip(&client->dev, > > + max77759_mfd->regmap_top, > > + MAX77759_INT_TOPSYS, > > + parent, > > + &max77759_topsys_irq_chip, > > + &irq_chip_data); > > + if (ret) > > + return ret; > > + > > + return 0; > > +} > > + > > +static int max77759_add_chained_charger(struct i2c_client *client, > > + struct max77759_mfd *max77759_mfd, > > + struct regmap_irq_chip_data *parent) > > +{ > > + struct regmap_irq_chip_data *irq_chip_data; > > + int ret; > > + > > + ret = max77759_add_chained_irq_chip(&client->dev, > > + max77759_mfd->regmap_charger, > > + MAX77759_INT_CHGR, > > + parent, > > + &max77759_chrg_irq_chip, > > + &irq_chip_data); > > + if (ret) > > + return ret; > > + > > + ret = devm_mfd_add_devices(&client->dev, PLATFORM_DEVID_AUTO, > > + max77759_charger_cells, > > + ARRAY_SIZE(max77759_charger_cells), > > + NULL, 0, > > + regmap_irq_get_domain(irq_chip_data)); > > + if (ret) > > + return dev_err_probe(&client->dev, ret, > > + "failed to add MFD devices (charger)\n"); > > "sub" or "child" devices. OK > > > + > > + return 0; > > +} > > + > > +static int max77759_probe(struct i2c_client *client) > > +{ > > + struct regmap *regmap_top; > > + unsigned int pmic_id; > > + int ret; > > + struct irq_data *irq_data; > > + struct max77759_mfd *max77759_mfd; > > + unsigned long irq_flags; > > + struct regmap_irq_chip_data *irq_chip_data_pmic; > > Structure ! As above, and I'll change it. > > > + > > + regmap_top = devm_regmap_init_i2c(client, &max77759_regmap_config_top); > > + if (IS_ERR(regmap_top)) > > + return dev_err_probe(&client->dev, PTR_ERR(regmap_top), > > + "regmap init failed\n"); > > Which regmap failed? There's only one regmap for 'client'. The other regmaps are for the different i2c dummy devices created, and will cause the error message to have a different device name prefix. I'll update the messages. > > > + ret = regmap_read(regmap_top, MAX77759_PMIC_REG_PMIC_ID, &pmic_id); > > + if (ret) > > + return dev_err_probe(&client->dev, ret, > > + "Unable to read Device ID\n"); > > + > > + if (pmic_id != MAX77759_PMIC_REG_PMIC_ID_MAX77759) > > + return dev_err_probe(&client->dev, -ENODEV, > > + "Unsupported Device ID %#.2x (%d)\n", > > + pmic_id, pmic_id); > > + > > + irq_data = irq_get_irq_data(client->irq); > > + if (!irq_data) > > + return dev_err_probe(&client->dev, -EINVAL, > > + "Invalid IRQ: %d\n", client->irq); > > + > > + max77759_mfd = devm_kzalloc(&client->dev, sizeof(*max77759_mfd), > > + GFP_KERNEL); > > + if (!max77759_mfd) > > + return -ENOMEM; > > Device data is almost universally allocated at the top of .probe(). Will do. > > > + max77759_mfd->regmap_top = regmap_top; > > These two blocks are unrelated - please separate them with a line break here. Will do. > > > + ret = devm_mutex_init(&client->dev, &max77759_mfd->maxq_lock); > > + if (ret) > > + return ret; > > + > > + i2c_set_clientdata(client, max77759_mfd); > > + > > + for (int i = 0; i < ARRAY_SIZE(max77759_i2c_subdevs); ++i) { > > Any reason for the pre-increment? > > If not, it's more standards to post-inc. My reason is that historically compilers have created better code with preinc, and I generally still prefer it, to make it obvious. There are thousands of users doing preinc in for loops in the kernel tree, so unless you really insist, I'd like to keep it that way for that reason if you're OK with reason :-) > > + ret = max77759_create_i2c_subdev(client, > > + max77759_mfd, > > + &max77759_i2c_subdevs[i]); > > + if (ret) > > + return ret; > > + } > > + > > + irq_flags = IRQF_ONESHOT | IRQF_SHARED; > > + irq_flags |= irqd_get_trigger_type(irq_data); > > + > > + ret = devm_regmap_add_irq_chip(&client->dev, max77759_mfd->regmap_top, > > + client->irq, irq_flags, 0, > > + &max77759_pmic_irq_chip, > > + &irq_chip_data_pmic); > > + if (ret) > > + return dev_err_probe(&client->dev, ret, > > + "Failed to add IRQ chip\n"); > > + > > + /* INTSRC - MaxQ & children */ > > What does these comments add beyond what is already obvious by the nomenclature? Visual separation. > > > + ret = max77759_add_chained_maxq(client, max77759_mfd, > > + irq_chip_data_pmic); > > + if (ret) > > + return ret; > > + > > + /* INTSRC - topsys & children */ > > + ret = max77759_add_chained_topsys(client, max77759_mfd, > > + irq_chip_data_pmic); > > + if (ret) > > + return ret; > > + > > + /* INTSRC - charger & children */ > > + ret = max77759_add_chained_charger(client, max77759_mfd, > > + irq_chip_data_pmic); > > + if (ret) > > + return ret; > > + > > + return devm_mfd_add_devices(&client->dev, PLATFORM_DEVID_AUTO, > > + max77759_cells, ARRAY_SIZE(max77759_cells), > > + NULL, 0, > > + regmap_irq_get_domain(irq_chip_data_pmic)); > > +} > > + > > +static const struct i2c_device_id max77759_i2c_id[] = { > > + { "max77759", 0 }, > > The driver data attribute can be omitted if it's 0. > > > + { } > > +}; > > +MODULE_DEVICE_TABLE(i2c, max77759_i2c_id); > > + > > +static const struct of_device_id max77759_of_id[] = { > > + { .compatible = "maxim,max77759", }, > > + { } > > +}; > > +MODULE_DEVICE_TABLE(of, max77759_of_id); > > + > > +static struct i2c_driver max77759_i2c_driver = { > > + .driver = { > > + .name = "max77759", > > + .of_match_table = max77759_of_id, > > + }, > > + .probe = max77759_probe, > > + .id_table = max77759_i2c_id, > > +}; > > +module_i2c_driver(max77759_i2c_driver); > > + > > +MODULE_AUTHOR("André Draszik <andre.draszik@linaro.org>"); > > +MODULE_DESCRIPTION("Maxim MAX77759 multi-function core driver"); > > No it isn't. :) OK > > > +MODULE_LICENSE("GPL"); > > diff --git a/include/linux/mfd/max77759.h b/include/linux/mfd/max77759.h > > new file mode 100644 > > index 0000000000000000000000000000000000000000..b038b4e9b748287e23e3a7030496f09dc8bdc816 > > --- /dev/null > > +++ b/include/linux/mfd/max77759.h > > @@ -0,0 +1,98 @@ > > +/* SPDX-License-Identifier: GPL-2.0-only */ > > +/* > > + * Copyright 2020 Google Inc. > > + * Copyright 2025 Linaro Ltd. > > + * > > + * Client interface for Maxim MAX77759 MFD driver > > + */ > > + > > +#ifndef __LINUX_MFD_MAX77759_H > > +#define __LINUX_MFD_MAX77759_H > > + > > +/* MaxQ opcodes */ > > +#define MAX77759_MAXQ_OPCODE_MAXLENGTH 33 > > + > > +#define MAX77759_MAXQ_OPCODE_GPIO_TRIGGER_READ 0x21 > > +#define MAX77759_MAXQ_OPCODE_GPIO_TRIGGER_WRITE 0x22 > > +#define MAX77759_MAXQ_OPCODE_GPIO_CONTROL_READ 0x23 > > +#define MAX77759_MAXQ_OPCODE_GPIO_CONTROL_WRITE 0x24 > > +#define MAX77759_MAXQ_OPCODE_USER_SPACE_READ 0x81 > > +#define MAX77759_MAXQ_OPCODE_USER_SPACE_WRITE 0x82 > > + > > +/* > > + * register map (incomplete) - registers not useful for drivers are not > > + * declared here > > + */ > > +/* MaxQ */ > > +#define MAX77759_MAXQ_REG_UIC_INT1 0x64 > > +#define MAX77759_MAXQ_REG_UIC_INT1_APCMDRESI BIT(7) > > +#define MAX77759_MAXQ_REG_UIC_INT1_SYSMSGI BIT(6) > > +#define MAX77759_MAXQ_REG_UIC_INT1_GPIO6I BIT(1) > > +#define MAX77759_MAXQ_REG_UIC_INT1_GPIO5I BIT(0) > > +#define MAX77759_MAXQ_REG_UIC_INT1_GPIOxI(offs, en) (((en) & 1) << (offs)) > > +#define MAX77759_MAXQ_REG_UIC_INT1_GPIOxI_MASK(offs) \ > > + MAX77759_MAXQ_REG_UIC_INT1_GPIOxI(offs, ~0) > > + > > +#define MAX77759_MAXQ_REG_UIC_INT2 0x65 > > +#define MAX77759_MAXQ_REG_UIC_INT3 0x66 > > +#define MAX77759_MAXQ_REG_UIC_INT4 0x67 > > +#define MAX77759_MAXQ_REG_UIC_UIC_STATUS1 0x68 > > +#define MAX77759_MAXQ_REG_UIC_UIC_STATUS2 0x69 > > +#define MAX77759_MAXQ_REG_UIC_UIC_STATUS3 0x6a > > +#define MAX77759_MAXQ_REG_UIC_UIC_STATUS4 0x6b > > +#define MAX77759_MAXQ_REG_UIC_UIC_STATUS5 0x6c > > +#define MAX77759_MAXQ_REG_UIC_UIC_STATUS6 0x6d > > +#define MAX77759_MAXQ_REG_UIC_UIC_STATUS7 0x6f > > +#define MAX77759_MAXQ_REG_UIC_UIC_STATUS8 0x6f > > +#define MAX77759_MAXQ_REG_UIC_INT1_M 0x70 > > +#define MAX77759_MAXQ_REG_UIC_INT2_M 0x71 > > +#define MAX77759_MAXQ_REG_UIC_INT3_M 0x72 > > +#define MAX77759_MAXQ_REG_UIC_INT4_M 0x73 > > + > > +/* charger */ > > +#define MAX77759_CHGR_REG_CHG_INT 0xb0 > > +#define MAX77759_CHGR_REG_CHG_INT2 0xb1 > > +#define MAX77759_CHGR_REG_CHG_INT_MASK 0xb2 > > +#define MAX77759_CHGR_REG_CHG_INT2_MASK 0xb3 > > + > > +struct max77759_mfd; > > Place the definition in here instead. I would like to keep it private. There is no need for it to become public, it's meant to be an opaque handle. Thanks again Lee for you detailed review! Cheers, Andre' > > > +/** > > + * struct max77759_maxq_command - structure containing the MaxQ command to > > + * send > > + * > > + * @length: The number of bytes to send. > > + * @cmd: The data to send. > > + */ > > +struct max77759_maxq_command { > > + u8 length; > > + u8 cmd[] __counted_by(length); > > +}; > > + > > +/** > > + * struct max77759_maxq_response - structure containing the MaxQ response > > + * > > + * @length: The number of bytes to receive. > > + * @rsp: The data received. Must have at least @length bytes space. > > + */ > > +struct max77759_maxq_response { > > + u8 length; > > + u8 rsp[] __counted_by(length); > > +}; > > + > > +/** > > + * max77759_maxq_command() - issue a MaxQ command and wait for the response > > + * and associated data > > + * > > + * @max77759_mfd: The core max77759 mfd device handle. > > + * @cmd: The command to be sent. > > + * @rsp: Any response data associated with the command will be copied here; > > + * can be %NULL if the command has no response (other than ACK). > > + * > > + * Return: 0 on success, a negative error number otherwise. > > + */ > > +int max77759_maxq_command(struct max77759_mfd *max77759_mfd, > > + const struct max77759_maxq_command *cmd, > > + struct max77759_maxq_response *rsp); > > + > > +#endif /* __LINUX_MFD_MAX77759_H */ > > > > -- > > 2.49.0.rc0.332.g42c0ae87b1-goog > > > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v4 4/6] mfd: max77759: add Maxim MAX77759 core mfd driver 2025-03-14 15:30 ` André Draszik @ 2025-03-21 10:54 ` Lee Jones 2025-03-25 7:44 ` André Draszik 0 siblings, 1 reply; 12+ messages in thread From: Lee Jones @ 2025-03-21 10:54 UTC (permalink / raw) To: André Draszik Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Linus Walleij, Bartosz Golaszewski, Srinivas Kandagatla, Kees Cook, Gustavo A. R. Silva, Peter Griffin, Tudor Ambarus, Will McVicker, kernel-team, linux-kernel, devicetree, linux-gpio, linux-hardening On Fri, 14 Mar 2025, André Draszik wrote: > Hi Lee, > > Thanks for your review! > > On Fri, 2025-03-14 at 12:31 +0000, Lee Jones wrote: > > On Wed, 12 Mar 2025, André Draszik wrote: > > > > > The Maxim MAX77759 is a companion PMIC for USB Type-C applications and > > > includes Battery Charger, Fuel Gauge, temperature sensors, USB Type-C > > > Port Controller (TCPC), NVMEM, and a GPIO expander. > > > > > > Fuel Gauge and TCPC have separate and independent I2C addresses, > > > register maps, and interrupt lines and are therefore excluded from the > > > MFD core device driver here. > > > > > > The GPIO and NVMEM interfaces are accessed via specific commands to the > > > built-in microprocessor. This driver implements an API that client > > > drivers can use for accessing those. > > > > > > Signed-off-by: André Draszik <andre.draszik@linaro.org> > > > > > > --- > > > v4: > > > * add missing build_bug.h include > > > * update an irq chip comment > > > * fix a whitespace in register definitions > > > > > > v2: > > > * add kernel doc for max77759_maxq_command() and related structs > > > * fix an msec / usec typo > > > * add missing error handling of devm_mutex_init() (Christophe) > > > * align sentinel in max77759_of_id[] with max77759_i2c_id[] > > > (Christophe) > > > * some tidy-ups in max77759_maxq_command() (Christophe) > > > --- > > > MAINTAINERS | 2 + > > > drivers/mfd/Kconfig | 20 ++ > > > drivers/mfd/Makefile | 1 + > > > drivers/mfd/max77759.c | 738 +++++++++++++++++++++++++++++++++++++++++++ > > > include/linux/mfd/max77759.h | 98 ++++++ > > > 5 files changed, 859 insertions(+) > > > > > > diff --git a/MAINTAINERS b/MAINTAINERS > > > index a45d1bd64d116d14bc05a64fa3da852a41e1de7f..38844ea24e464d0b58f8852b79e2b94f18d48998 100644 > > > --- a/MAINTAINERS > > > +++ b/MAINTAINERS > > > @@ -14352,6 +14352,8 @@ M: André Draszik <andre.draszik@linaro.org> > > > L: linux-kernel@vger.kernel.org > > > S: Maintained > > > F: Documentation/devicetree/bindings/*/maxim,max77759*.yaml > > > +F: drivers/mfd/max77759.c > > > +F: include/linux/mfd/max77759.h > > > > > > MAXIM MAX77802 PMIC REGULATOR DEVICE DRIVER > > > M: Javier Martinez Canillas <javier@dowhile0.org> > > > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig > > > index d44c69bb3dfd52d53fe26aa2d5e5ace346448f57..1d72bf086401064608cc41e9f9a0af044c6df68e 100644 > > > --- a/drivers/mfd/Kconfig > > > +++ b/drivers/mfd/Kconfig > > > @@ -930,6 +930,26 @@ config MFD_MAX77714 > > > drivers must be enabled in order to use each functionality of the > > > device. > > > > > > +config MFD_MAX77759 > > > + tristate "Maxim Integrated MAX77759 PMIC" > > > + depends on I2C > > > + depends on OF > > > + select IRQ_DOMAIN > > > + select MFD_CORE > > > + select REGMAP_I2C > > > + select REGMAP_IRQ > > > + help > > > + Say yes here to add support for Maxim Integrated MAX77759. > > > + This is a companion Power Management IC for USB Type-C applications > > > + with Battery Charger, Fuel Gauge, temperature sensors, USB Type-C > > > + Port Controller (TCPC), NVMEM, and additional GPIO interfaces. > > > + This driver provides common support for accessing the device; > > > + additional drivers must be enabled in order to use the functionality > > > + of the device. > > > + > > > + To compile this driver as a module, choose M here: the module will be > > > + called max77759. > > > + > > > config MFD_MAX77843 > > > bool "Maxim Semiconductor MAX77843 PMIC Support" > > > depends on I2C=y > > > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile > > > index 9220eaf7cf1255a8922430fe3e50e41771bbaa60..cc9362afd8f060d64ca0f0f028a0d7cfe9cfe512 100644 > > > --- a/drivers/mfd/Makefile > > > +++ b/drivers/mfd/Makefile > > > @@ -169,6 +169,7 @@ 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_MAX77759) += max77759.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/max77759.c b/drivers/mfd/max77759.c > > > new file mode 100644 > > > index 0000000000000000000000000000000000000000..a4f1616d600bde250a73b69464ada1fbf6d7e67e > > > --- /dev/null > > > +++ b/drivers/mfd/max77759.c > > > @@ -0,0 +1,738 @@ > > > +// SPDX-License-Identifier: GPL-2.0-only > > > +// > > > +// Copyright 2020 Google Inc > > > +// Copyright 2025 Linaro Ltd. > > > +// > > > +// Core MFD driver for Maxim MAX77759 companion PMIC for USB Type-C > > > > Only the SPDX should be in C++ style comments. > > > > Please C block-comment the rest. > > It's a pity that there is different guidance on how to add the > comment, e.g. for regulators the preference is C++ style, see e.g. > https://lore.kernel.org/all/ZulJuCu-QcMYrphP@finisterre.sirena.org.uk/ I know that Mark likes the file header to be unified. > I'd rather have one style for max77759, the nvmem and gpio parts > also use C++ for this, and some drivers in mfd/ also use C++ style. > > It'd be nice if there was one style :-) > > Do you want me to change all drivers in this series? Sorry, styles that are not universally common and documented are subsystem based, not by driver family. > > Drop all references to MFD - it's not a real thing. > > Sure. > > > > +#include <linux/array_size.h> > > > +#include <linux/bitfield.h> > > > +#include <linux/bits.h> > > > +#include <linux/build_bug.h> > > > +#include <linux/cleanup.h> > > > +#include <linux/completion.h> > > > +#include <linux/dev_printk.h> > > > +#include <linux/device.h> > > > +#include <linux/err.h> > > > +#include <linux/i2c.h> > > > +#include <linux/init.h> > > > +#include <linux/interrupt.h> > > > +#include <linux/irq.h> > > > +#include <linux/jiffies.h> > > > +#include <linux/mfd/core.h> > > > +#include <linux/mfd/max77759.h> > > > +#include <linux/mod_devicetable.h> > > > +#include <linux/module.h> > > > +#include <linux/mutex.h> > > > +#include <linux/of.h> > > > +#include <linux/overflow.h> > > > +#include <linux/regmap.h> > > > > This is a huge list of includes. > > > > Please comb them to see if you really do need all of them. > > This is the minimal list as it's usually preferred to not > rely on implicit includes. It is good practice to directly > include the headers used, it avoids implicit dependencies > and spurious breakage if someone rearranges headers and > causes the implicit include to vanish. You're preaching to the converted with respect to implicit includes. So long as this driver uses all of the headers here, you're golden. > > > +/* registers - registers useful to drivers are declared in the public header */ > > > > This sort of goes without saying. Suggest you remove this comment. > > > > > +/* PMIC / TOP */ > > > +#define MAX77759_PMIC_REG_PMIC_ID 0x00 > > > +#define MAX77759_PMIC_REG_PMIC_ID_MAX77759 59 > > > > Is this a register or a value? > > It's a value, which the suffix in the macro name was meant to > convey :-) 59 is the value (in decimal) for max77759 that > the ID register is expected to contain. You mean the REG suffix, that is in both the register address and value? And why do you have MAX77759 in there twice? That's just odd. Suggest that expected values, like IDs are placed in an enum with the other supported platforms enum { MAX77759_CHIP_ID = 59, MAX77760_CHIP_ID = 60, MAX77761_CHIP_ID = 61, }; > > > > > > +#define MAX77759_PMIC_REG_PMIC_REVISION 0x01 > > > +#define MAX77759_PMIC_REG_OTP_REVISION 0x02 > > > + > > > +#define MAX77759_PMIC_REG_INTSRC 0x22 > > > +#define MAX77759_PMIC_REG_INTSRCMASK 0x23 > > > +#define MAX77759_PMIC_REG_INTSRC_MAXQ BIT(3) > > > +#define MAX77759_PMIC_REG_INTSRC_TOPSYS BIT(1) > > > +#define MAX77759_PMIC_REG_INTSRC_CHGR BIT(0) > > > > These look like bit offsets rather than reg addresses? > > Of course, could you please clarify what you're hinting at > here? Register bits/masks {c,sh}ould be indented below the register names: #define MAX77759_PMIC_REG_INTSRCMASK 0x23 #define MAX77759_PMIC_REG_INTSRC_MAXQ BIT(3) > > > +#define MAX77759_PMIC_REG_TOPSYS_INT 0x24 > > > +#define MAX77759_PMIC_REG_TOPSYS_INT_MASK 0x26 > > > +#define MAX77759_PMIC_REG_TOPSYS_INT_TSHDN BIT(6) > > > +#define MAX77759_PMIC_REG_TOPSYS_INT_SYSOVLO BIT(5) > > > +#define MAX77759_PMIC_REG_TOPSYS_INT_SYSUVLO BIT(4) > > > +#define MAX77759_PMIC_REG_TOPSYS_INT_FSHIP BIT(0) > > > + > > > +#define MAX77759_PMIC_REG_I2C_CNFG 0x40 > > > +#define MAX77759_PMIC_REG_SWRESET 0x50 > > > +#define MAX77759_PMIC_REG_CONTROL_FG 0x51 > > > +#define MAX77759_PMIC_REG_LAST_REGISTER MAX77759_PMIC_REG_CONTROL_FG > > > > You could just use MAX77759_PMIC_REG_CONTROL_FG in-place? > > I think this makes it more obvious in the regmap definition > below - struct regmap_config::max_register vs > struct regmap_config::num_reg_defaults_raw The attributes are already suitably named. Creating a new define to make this doubly obvious is not required. [...] > > Where are the other registers? > > The intention is to add them once we start working on charger. Add it when you do instead. > > > +enum max77759_i2c_subdev_id { > > > + MAX77759_I2C_SUBDEV_ID_MAXQ, > > > + MAX77759_I2C_SUBDEV_ID_CHARGER, > > > > Are these truly arbitrary or are you relying on the fact that the > > compiler usually starts from 0 and incs by 1? > > It's not arbitrary. A conforming compiler is required to start with > 0, this is part of the C standard, e.g. > https://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf 6.7.2.2.3 I'm aware of the standard. However from a readability perspective they look arbitrary, so either add a comment to say that you're relying on this behaviour and that the presence and order of each entry is fixed and should not be amended or do future contributors a favour and hard code them from the beginning. [...] > > > + struct mutex maxq_lock; > > > + struct regmap *regmap_maxq; > > > + struct completion cmd_done; > > > + > > > + struct regmap *regmap_top; > > > > What is top? > > One of its functional blocks as per the data sheet. I think > it makes sense to use nomenclature from it. Maybe a comment should be added to say what top actually represents. Not all readers of this code will have the datasheet open. > > > + struct regmap *regmap_charger; > > > +}; > > > + > > > +struct max77759_i2c_subdev { > > > + enum max77759_i2c_subdev_id id; > > > + const struct regmap_config *cfg; > > > + u16 i2c_address; > > > +}; > > > + > > > +/* TOP registers */ > > > +static const struct regmap_range max77759_top_registers[] = { > > > + regmap_reg_range(0x00, 0x02), > > > + regmap_reg_range(0x22, 0x24), > > > + regmap_reg_range(0x26, 0x26), > > > + regmap_reg_range(0x40, 0x40), > > > + regmap_reg_range(0x50, 0x51), > > > > What are these magic numbers? Can you define them? > > This is on purpose, as I think it makes it much easier to have > the ranges as numbers, otherwise you always have to look them up > in the header when trying to verify correctness or simply looking > at this. Additionally, things wouldn't be nicely aligned anymore, > > Without data sheet, whether these are numbers or macro names > makes no difference, and it's impossible to reason about them. > > With data sheet, it's easier this way to compare. > > But I'll change them. Or provide come helpful comments. Magic numbers are generally decried. This is made worse here, since it's not obvious what this function block does. > > > +}; > > > + > > > +static const struct regmap_range max77759_top_ro_registers[] = { > > > + regmap_reg_range(0x00, 0x02), > > > + regmap_reg_range(0x22, 0x22), > > > +}; > > > + > > > +static const struct regmap_range max77759_top_volatile_registers[] = { > > > + regmap_reg_range(0x22, 0x22), > > > + regmap_reg_range(0x24, 0x24), > > > +}; > > > + > > > +static const struct regmap_access_table max77759_top_wr_table = { > > > + .yes_ranges = max77759_top_registers, > > > + .n_yes_ranges = ARRAY_SIZE(max77759_top_registers), > > > + .no_ranges = max77759_top_ro_registers, > > > + .n_no_ranges = ARRAY_SIZE(max77759_top_ro_registers), > > > +}; > > > + > > > +static const struct regmap_access_table max77759_top_rd_table = { > > > + .yes_ranges = max77759_top_registers, > > > + .n_yes_ranges = ARRAY_SIZE(max77759_top_registers), > > > +}; > > > + > > > +static const struct regmap_access_table max77759_top_volatile_table = { > > > + .yes_ranges = max77759_top_volatile_registers, > > > + .n_yes_ranges = ARRAY_SIZE(max77759_top_volatile_registers), > > > +}; > > > + > > > +static const struct regmap_config max77759_regmap_config_top = { > > > + .name = "top", > > > + .reg_bits = 8, > > > + .val_bits = 8, > > > + .max_register = MAX77759_PMIC_REG_LAST_REGISTER, > > > + .wr_table = &max77759_top_wr_table, > > > + .rd_table = &max77759_top_rd_table, > > > + .volatile_table = &max77759_top_volatile_table, > > > + .num_reg_defaults_raw = MAX77759_PMIC_REG_LAST_REGISTER + 1, > > > + .cache_type = REGCACHE_MAPLE, > > > +}; > > > + > > > +/* MaxQ registers */ > > > > Again, this is obvious due to the nomenclature. > > Yes, but it gives a visual separation. I'll drop it. Or expand it to provide information that is actually useful. [...] > > > +int max77759_maxq_command(struct max77759_mfd *max77759_mfd, > > > + const struct max77759_maxq_command *cmd, > > > + struct max77759_maxq_response *rsp) > > > +{ > > > + DEFINE_FLEX(struct max77759_maxq_response, _rsp, rsp, length, 1); > > > + int ret; > > > > Nit: Please place this at the bottom of the block. > > OK. > > > > > *twitch* > > > > > + struct device *dev = regmap_get_device(max77759_mfd->regmap_maxq); > > > > > + static const unsigned int timeout_ms = 200; > > > > Why 200? > > This is what downstream uses, I can add a comment to say so. We're not generally interested in downstream here. Why do downstream use it? Is there a value in the datasheet? > > > + > > > + if (cmd->length > MAX77759_MAXQ_REG_AP_MESSAGESZ_MAX) > > > + return -EINVAL; > > > + > > > + /* rsp is allowed to be NULL. In that case we do need a temporary. */ > > > > More details please. > > > > Why is NULL valid? > > The kernel doc explains it: > > * @rsp: Any response data associated with the command will be copied here; > * can be %NULL if the command has no response (other than ACK). > > Do you want me to duplicate this information? You already did, just in a less useful way. It's arguably more critical to document it here than tucked away in the header. > > Why does it need a placeholder? What are you using as the placeholder? > > Here, we still need a location to write the response to, as we > need to verify that the command was indeed completed correctly. Then please tell all of the future readers who may have the same queries. > > > + if (!rsp) > > > + rsp = _rsp; > > > + > > > + BUILD_BUG_ON(MAX77759_MAXQ_OPCODE_MAXLENGTH > > > + != MAX77759_MAXQ_REG_AP_MESSAGESZ_MAX); > > > > Probably useful during development. Let useful in production code. > > This build bug ensures that if anybody ever changed one of the > macros in an incorrect way, things fall apart. One of the macros is > part of the API, the other one is derived internally in this driver. > Better to fail loudly, than to silently ignore such errors. > > I'd rather leave this check in. I think this is a bad idea and could set a ridiculous precedent of validating all sorts of related defines. Author the defines and test them thoroughly. If anyone were to come along and incorrectly tinker with your hard work, that's on them. If they have to be identical, derive one from the other. > > > + if (!rsp->length || rsp->length > MAX77759_MAXQ_REG_AP_MESSAGESZ_MAX) > > > + return -EINVAL; > > > + > > > + guard(mutex)(&max77759_mfd->maxq_lock); > > > + > > > + reinit_completion(&max77759_mfd->cmd_done); > > > + > > > + /* write the opcode and data */ > > > > Upper case throughout please. > > OK > > > > + ret = regmap_bulk_write(max77759_mfd->regmap_maxq, > > > + MAX77759_MAXQ_REG_AP_DATAOUT0, cmd->cmd, > > > + cmd->length); > > > + if (!ret && cmd->length < MAX77759_MAXQ_REG_AP_MESSAGESZ_MAX) > > > + /* writing the last byte triggers MaxQ */ > > > + ret = regmap_write(max77759_mfd->regmap_maxq, > > > + MAX77759_MAXQ_REG_AP_DATAOUT32, 0); > > > + if (ret) { > > > > Do this first. > > > Then you only need to check: > > > > cmd->length < MAX77759_MAXQ_REG_AP_MESSAGESZ_MAX > > > > ... in the subsequent statement. > > > > But what happens if: > > > > !ret && cmd->length >= MAX77759_MAXQ_REG_AP_MESSAGESZ_MAX > > ^ > > Is that okay? Nothing to be done? > > Yes, that is OK for length==MAX77759_MAXQ_REG_AP_MESSAGESZ_MAX > > The command buffer is MAX77759_MAXQ_REG_AP_MESSAGESZ_MAX byte long. > > If the message to be sent is smaller than that, then the last byte > still needs to be written to MaxQ (DATAOUT32 register above) to > submit the command ("/* writing the last byte triggers MaxQ */" > comment above). > > So there are three cases to cater for: > > message > MESSAGESZ_MAX -> return -EINVAL at start of function > message == MESSAGESZ_MAX -> just write message using regmap_bulk_write(), > DATAOUT32 will be written due to cmd->length > message < MESSAGESZ_MAX -> write available bytes, then do dummy write > to DATAOUT32 > > As is, the code ensures that, it does the extra DATAOUT32 write only > if the bulk write succeeded and if necessary. It avoids duplicated > tests of the regmap_bulk_write() / regmap_write() return value. > > With your suggestion, the code would become more complicated I believe: > > /* write the opcode and data */ > ret = regmap_bulk_write(max77759_mfd->regmap_maxq, > MAX77759_MAXQ_REG_AP_DATAOUT0, cmd->cmd, > cmd->length); > if (ret) { > dev_err(dev, "write data failed: %d\n", ret); > return ret; > } > if (cmd->length < MAX77759_MAXQ_REG_AP_MESSAGESZ_MAX) { > /* writing the last byte triggers MaxQ */ > ret = regmap_write(max77759_mfd->regmap_maxq, > MAX77759_MAXQ_REG_AP_DATAOUT32, 0); > if (ret) { > dev_err(dev, "write data failed: %d\n", ret); > return ret; > } > } > > vs at the moment: > > /* write the opcode and data */ > ret = regmap_bulk_write(max77759_mfd->regmap_maxq, > MAX77759_MAXQ_REG_AP_DATAOUT0, cmd->cmd, > cmd->length); > if (!ret && cmd->length < MAX77759_MAXQ_REG_AP_MESSAGESZ_MAX) > /* writing the last byte triggers MaxQ */ > ret = regmap_write(max77759_mfd->regmap_maxq, > MAX77759_MAXQ_REG_AP_DATAOUT32, 0); > if (ret) { > dev_err(dev, "write data failed: %d\n", ret); > return ret; > } Ah, I missed the re-assignment of 'ret'. It's better as it is, thanks. [...] > > > +static int max77759_add_chained_maxq(struct i2c_client *client, > > > + struct max77759_mfd *max77759_mfd, > > > + struct regmap_irq_chip_data *parent) > > > +{ > > > + struct regmap_irq_chip_data *irq_chip_data; > > > + int ret; > > > > Bottom please. > > > > Some people like reverse Christmas tree. > > > > Structure helps me sleep at night! =;-) > > I usually declare in order of use - I'll change ordering; don't want > to rob you of your sleep :-)) Yeah, that's common during development. But it's usually adapted / neatened-up before submission. Readable code == happy contributors [...] > > > + ret = regmap_read(regmap_top, MAX77759_PMIC_REG_PMIC_ID, &pmic_id); > > > + if (ret) > > > + return dev_err_probe(&client->dev, ret, > > > + "Unable to read Device ID\n"); > > > + > > > + if (pmic_id != MAX77759_PMIC_REG_PMIC_ID_MAX77759) > > > + return dev_err_probe(&client->dev, -ENODEV, > > > + "Unsupported Device ID %#.2x (%d)\n", > > > + pmic_id, pmic_id); > > > + > > > + irq_data = irq_get_irq_data(client->irq); > > > + if (!irq_data) > > > + return dev_err_probe(&client->dev, -EINVAL, > > > + "Invalid IRQ: %d\n", client->irq); > > > + > > > + max77759_mfd = devm_kzalloc(&client->dev, sizeof(*max77759_mfd), > > > + GFP_KERNEL); > > > + if (!max77759_mfd) > > > + return -ENOMEM; > > > > Device data is almost universally allocated at the top of .probe(). > > Will do. > > > > > > + max77759_mfd->regmap_top = regmap_top; > > > > These two blocks are unrelated - please separate them with a line break here. > > Will do. > > > > > > + ret = devm_mutex_init(&client->dev, &max77759_mfd->maxq_lock); > > > + if (ret) > > > + return ret; > > > + > > > + i2c_set_clientdata(client, max77759_mfd); > > > + > > > + for (int i = 0; i < ARRAY_SIZE(max77759_i2c_subdevs); ++i) { > > > > Any reason for the pre-increment? > > > > If not, it's more standards to post-inc. > > My reason is that historically compilers have created better code > with preinc, and I generally still prefer it, to make it obvious. Is this still true today? Historically, we used program computers with punched cards as well. =:-) > There are thousands of users doing preinc in for loops in the kernel > tree, so unless you really insist, I'd like to keep it that way for > that reason if you're OK with reason :-) Let's play top-trumps: % git grep "++i" | wc -l 7062 % git grep "i++" | wc -l 75158 There is more than an order of magnitude of difference between the styles and I bet quite a few of the aforementioned cases were authored because they _required_ pre-inc. Anyway, it's not a hard and fast rule, but it is the kernel's preference. > > > + ret = max77759_create_i2c_subdev(client, > > > + max77759_mfd, > > > + &max77759_i2c_subdevs[i]); > > > + if (ret) > > > + return ret; > > > + } > > > + > > > + irq_flags = IRQF_ONESHOT | IRQF_SHARED; > > > + irq_flags |= irqd_get_trigger_type(irq_data); > > > + > > > + ret = devm_regmap_add_irq_chip(&client->dev, max77759_mfd->regmap_top, > > > + client->irq, irq_flags, 0, > > > + &max77759_pmic_irq_chip, > > > + &irq_chip_data_pmic); > > > + if (ret) > > > + return dev_err_probe(&client->dev, ret, > > > + "Failed to add IRQ chip\n"); > > > + > > > + /* INTSRC - MaxQ & children */ > > > > What does these comments add beyond what is already obvious by the nomenclature? > > Visual separation. Operating on different blocks is kinda MFD's 'thing' and is pretty obvious by the function names already. All you're doing here is repeating information that is already present. > > > + ret = max77759_add_chained_maxq(client, max77759_mfd, > > > + irq_chip_data_pmic); > > > + if (ret) > > > + return ret; > > > + > > > + /* INTSRC - topsys & children */ > > > + ret = max77759_add_chained_topsys(client, max77759_mfd, > > > + irq_chip_data_pmic); > > > + if (ret) > > > + return ret; > > > + > > > + /* INTSRC - charger & children */ > > > + ret = max77759_add_chained_charger(client, max77759_mfd, > > > + irq_chip_data_pmic); > > > + if (ret) > > > + return ret; > > > + > > > + return devm_mfd_add_devices(&client->dev, PLATFORM_DEVID_AUTO, > > > + max77759_cells, ARRAY_SIZE(max77759_cells), > > > + NULL, 0, > > > + regmap_irq_get_domain(irq_chip_data_pmic)); > > > +} [...] > > > diff --git a/include/linux/mfd/max77759.h b/include/linux/mfd/max77759.h > > > new file mode 100644 > > > index 0000000000000000000000000000000000000000..b038b4e9b748287e23e3a7030496f09dc8bdc816 > > > --- /dev/null > > > +++ b/include/linux/mfd/max77759.h > > > @@ -0,0 +1,98 @@ > > > +/* SPDX-License-Identifier: GPL-2.0-only */ > > > +/* > > > + * Copyright 2020 Google Inc. > > > + * Copyright 2025 Linaro Ltd. > > > + * > > > + * Client interface for Maxim MAX77759 MFD driver > > > + */ > > > + > > > +#ifndef __LINUX_MFD_MAX77759_H > > > +#define __LINUX_MFD_MAX77759_H > > > + > > > +/* MaxQ opcodes */ > > > +#define MAX77759_MAXQ_OPCODE_MAXLENGTH 33 > > > + > > > +#define MAX77759_MAXQ_OPCODE_GPIO_TRIGGER_READ 0x21 > > > +#define MAX77759_MAXQ_OPCODE_GPIO_TRIGGER_WRITE 0x22 > > > +#define MAX77759_MAXQ_OPCODE_GPIO_CONTROL_READ 0x23 > > > +#define MAX77759_MAXQ_OPCODE_GPIO_CONTROL_WRITE 0x24 > > > +#define MAX77759_MAXQ_OPCODE_USER_SPACE_READ 0x81 > > > +#define MAX77759_MAXQ_OPCODE_USER_SPACE_WRITE 0x82 > > > + > > > +/* > > > + * register map (incomplete) - registers not useful for drivers are not > > > + * declared here > > > + */ > > > +/* MaxQ */ > > > +#define MAX77759_MAXQ_REG_UIC_INT1 0x64 > > > +#define MAX77759_MAXQ_REG_UIC_INT1_APCMDRESI BIT(7) > > > +#define MAX77759_MAXQ_REG_UIC_INT1_SYSMSGI BIT(6) > > > +#define MAX77759_MAXQ_REG_UIC_INT1_GPIO6I BIT(1) > > > +#define MAX77759_MAXQ_REG_UIC_INT1_GPIO5I BIT(0) > > > +#define MAX77759_MAXQ_REG_UIC_INT1_GPIOxI(offs, en) (((en) & 1) << (offs)) > > > +#define MAX77759_MAXQ_REG_UIC_INT1_GPIOxI_MASK(offs) \ > > > + MAX77759_MAXQ_REG_UIC_INT1_GPIOxI(offs, ~0) > > > + > > > +#define MAX77759_MAXQ_REG_UIC_INT2 0x65 > > > +#define MAX77759_MAXQ_REG_UIC_INT3 0x66 > > > +#define MAX77759_MAXQ_REG_UIC_INT4 0x67 > > > +#define MAX77759_MAXQ_REG_UIC_UIC_STATUS1 0x68 > > > +#define MAX77759_MAXQ_REG_UIC_UIC_STATUS2 0x69 > > > +#define MAX77759_MAXQ_REG_UIC_UIC_STATUS3 0x6a > > > +#define MAX77759_MAXQ_REG_UIC_UIC_STATUS4 0x6b > > > +#define MAX77759_MAXQ_REG_UIC_UIC_STATUS5 0x6c > > > +#define MAX77759_MAXQ_REG_UIC_UIC_STATUS6 0x6d > > > +#define MAX77759_MAXQ_REG_UIC_UIC_STATUS7 0x6f > > > +#define MAX77759_MAXQ_REG_UIC_UIC_STATUS8 0x6f > > > +#define MAX77759_MAXQ_REG_UIC_INT1_M 0x70 > > > +#define MAX77759_MAXQ_REG_UIC_INT2_M 0x71 > > > +#define MAX77759_MAXQ_REG_UIC_INT3_M 0x72 > > > +#define MAX77759_MAXQ_REG_UIC_INT4_M 0x73 > > > + > > > +/* charger */ > > > +#define MAX77759_CHGR_REG_CHG_INT 0xb0 > > > +#define MAX77759_CHGR_REG_CHG_INT2 0xb1 > > > +#define MAX77759_CHGR_REG_CHG_INT_MASK 0xb2 > > > +#define MAX77759_CHGR_REG_CHG_INT2_MASK 0xb3 > > > + > > > +struct max77759_mfd; > > > > Place the definition in here instead. > > I would like to keep it private. There is no need for it to > become public, it's meant to be an opaque handle. No, it's not. It's device data that you already shared with all of the children. If you don't want them to have it, don't give it to them. -- Lee Jones [李琼斯] ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v4 4/6] mfd: max77759: add Maxim MAX77759 core mfd driver 2025-03-21 10:54 ` Lee Jones @ 2025-03-25 7:44 ` André Draszik 0 siblings, 0 replies; 12+ messages in thread From: André Draszik @ 2025-03-25 7:44 UTC (permalink / raw) To: Lee Jones Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Linus Walleij, Bartosz Golaszewski, Srinivas Kandagatla, Kees Cook, Gustavo A. R. Silva, Peter Griffin, Tudor Ambarus, Will McVicker, kernel-team, linux-kernel, devicetree, linux-gpio, linux-hardening Thanks Lee for your patience, On Fri, 2025-03-21 at 10:54 +0000, Lee Jones wrote: > On Fri, 14 Mar 2025, André Draszik wrote: > > > Hi Lee, > > > > Thanks for your review! > > > > On Fri, 2025-03-14 at 12:31 +0000, Lee Jones wrote: > > > On Wed, 12 Mar 2025, André Draszik wrote: [...] > > > > > This sort of goes without saying. Suggest you remove this comment. > > > > > > > +/* PMIC / TOP */ > > > > +#define MAX77759_PMIC_REG_PMIC_ID 0x00 > > > > +#define MAX77759_PMIC_REG_PMIC_ID_MAX77759 59 > > > > > > Is this a register or a value? > > > > It's a value, which the suffix in the macro name was meant to > > convey :-) 59 is the value (in decimal) for max77759 that > > the ID register is expected to contain. > > You mean the REG suffix, that is in both the register address and value? With suffix, I meant the last part '_MAX77759$' above. > > And why do you have MAX77759 in there twice? That's just odd. Everything starts with max77759 in this driver, for namespace reasons. The suffix could be different if support for a different ICs was added in the future, but I'll use your suggested enum below instead. > > Suggest that expected values, like IDs are placed in an enum with the > other supported platforms > > enum { > MAX77759_CHIP_ID = 59, > MAX77760_CHIP_ID = 60, > MAX77761_CHIP_ID = 61, > }; OK, will do, although this introduces a new prefix for a symbol name in this driver in case support for a different ICs was added in the future. > > > > > > > > > +#define MAX77759_PMIC_REG_PMIC_REVISION 0x01 > > > > +#define MAX77759_PMIC_REG_OTP_REVISION 0x02 > > > > + > > > > +#define MAX77759_PMIC_REG_INTSRC 0x22 > > > > +#define MAX77759_PMIC_REG_INTSRCMASK 0x23 > > > > +#define MAX77759_PMIC_REG_INTSRC_MAXQ BIT(3) > > > > +#define MAX77759_PMIC_REG_INTSRC_TOPSYS BIT(1) > > > > +#define MAX77759_PMIC_REG_INTSRC_CHGR BIT(0) > > > > > > These look like bit offsets rather than reg addresses? > > > > Of course, could you please clarify what you're hinting at > > here? > > Register bits/masks {c,sh}ould be indented below the register names: > > #define MAX77759_PMIC_REG_INTSRCMASK 0x23 > #define MAX77759_PMIC_REG_INTSRC_MAXQ BIT(3) Thanks for your clarification. > > > > +#define MAX77759_PMIC_REG_TOPSYS_INT 0x24 > > > > +#define MAX77759_PMIC_REG_TOPSYS_INT_MASK 0x26 > > > > +#define MAX77759_PMIC_REG_TOPSYS_INT_TSHDN BIT(6) > > > > +#define MAX77759_PMIC_REG_TOPSYS_INT_SYSOVLO BIT(5) > > > > +#define MAX77759_PMIC_REG_TOPSYS_INT_SYSUVLO BIT(4) > > > > +#define MAX77759_PMIC_REG_TOPSYS_INT_FSHIP BIT(0) > > > > + > > > > +#define MAX77759_PMIC_REG_I2C_CNFG 0x40 > > > > +#define MAX77759_PMIC_REG_SWRESET 0x50 > > > > +#define MAX77759_PMIC_REG_CONTROL_FG 0x51 > > > > +#define MAX77759_PMIC_REG_LAST_REGISTER MAX77759_PMIC_REG_CONTROL_FG > > > > > > You could just use MAX77759_PMIC_REG_CONTROL_FG in-place? > > > > I think this makes it more obvious in the regmap definition > > below - struct regmap_config::max_register vs > > struct regmap_config::num_reg_defaults_raw > > The attributes are already suitably named. > > Creating a new define to make this doubly obvious is not required. Ok. > > [...] > > > > Where are the other registers? > > > > The intention is to add them once we start working on charger. > > Add it when you do instead. I've added them here now. > > > > > +enum max77759_i2c_subdev_id { > > > > + MAX77759_I2C_SUBDEV_ID_MAXQ, > > > > + MAX77759_I2C_SUBDEV_ID_CHARGER, > > > > > > Are these truly arbitrary or are you relying on the fact that the > > > compiler usually starts from 0 and incs by 1? > > > > It's not arbitrary. A conforming compiler is required to start with > > 0, this is part of the C standard, e.g. > > https://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf 6.7.2.2.3 > > I'm aware of the standard. > > However from a readability perspective they look arbitrary, so either > add a comment to say that you're relying on this behaviour and that the > presence and order of each entry is fixed and should not be amended or > do future contributors a favour and hard code them from the beginning. I've added a comment to clarify. > [...] > > > > > + struct mutex maxq_lock; > > > > + struct regmap *regmap_maxq; > > > > + struct completion cmd_done; > > > > + > > > > + struct regmap *regmap_top; > > > > > > What is top? > > > > One of its functional blocks as per the data sheet. I think > > it makes sense to use nomenclature from it. > > Maybe a comment should be added to say what top actually represents. > > Not all readers of this code will have the datasheet open. I've added a kerneldoc to the struct, hopefully that's good enough :-) > > > > > + struct regmap *regmap_charger; > > > > +}; > > > > + > > > > +struct max77759_i2c_subdev { > > > > + enum max77759_i2c_subdev_id id; > > > > + const struct regmap_config *cfg; > > > > + u16 i2c_address; > > > > +}; > > > > + > > > > +/* TOP registers */ > > > > +static const struct regmap_range max77759_top_registers[] = { > > > > + regmap_reg_range(0x00, 0x02), > > > > + regmap_reg_range(0x22, 0x24), > > > > + regmap_reg_range(0x26, 0x26), > > > > + regmap_reg_range(0x40, 0x40), > > > > + regmap_reg_range(0x50, 0x51), > > > > > > What are these magic numbers? Can you define them? > > > > This is on purpose, as I think it makes it much easier to have > > the ranges as numbers, otherwise you always have to look them up > > in the header when trying to verify correctness or simply looking > > at this. Additionally, things wouldn't be nicely aligned anymore, > > > > Without data sheet, whether these are numbers or macro names > > makes no difference, and it's impossible to reason about them. > > > > With data sheet, it's easier this way to compare. > > > > But I'll change them. > > Or provide come helpful comments. Magic numbers are generally decried. > > This is made worse here, since it's not obvious what this function block does. I've added some comments, hopefully better :-) [... > > > > + struct device *dev = regmap_get_device(max77759_mfd->regmap_maxq); > > > > > > > + static const unsigned int timeout_ms = 200; > > > > > > Why 200? > > > > This is what downstream uses, I can add a comment to say so. > > We're not generally interested in downstream here. > > Why do downstream use it? Is there a value in the datasheet? No, there isn't. We know that 200ms must work reliably since downstream uses that value, hence I re-used it. I've dropped it to 20ms now, since in most cases in very simple tests it takes less than 5ms, but I don't know if there are cases when e.g. the uC is busy (e.g doing USB-C related handling) where it might take longer. I guess we'll see. > > > > > + > > > > + if (cmd->length > MAX77759_MAXQ_REG_AP_MESSAGESZ_MAX) > > > > + return -EINVAL; > > > > + > > > > + /* rsp is allowed to be NULL. In that case we do need a temporary. */ > > > > > > More details please. > > > > > > Why is NULL valid? > > > > The kernel doc explains it: > > > > * @rsp: Any response data associated with the command will be copied here; > > * can be %NULL if the command has no response (other than ACK). > > > > Do you want me to duplicate this information? > > You already did, just in a less useful way. > > It's arguably more critical to document it here than tucked away in the header. > > > > Why does it need a placeholder? What are you using as the placeholder? > > > > Here, we still need a location to write the response to, as we > > need to verify that the command was indeed completed correctly. > > Then please tell all of the future readers who may have the same queries. The header / kdoc is for API users, so it makes sense to have it there. I've updated the comment here as well. > > > > [...] > > > > + ret = devm_mutex_init(&client->dev, &max77759_mfd->maxq_lock); > > > > + if (ret) > > > > + return ret; > > > > + > > > > + i2c_set_clientdata(client, max77759_mfd); > > > > + > > > > + for (int i = 0; i < ARRAY_SIZE(max77759_i2c_subdevs); ++i) { > > > > > > Any reason for the pre-increment? > > > > > > If not, it's more standards to post-inc. > > > > My reason is that historically compilers have created better code > > with preinc, and I generally still prefer it, to make it obvious. > > Is this still true today? > > Historically, we used program computers with punched cards as well. =:-) > > > There are thousands of users doing preinc in for loops in the kernel > > tree, so unless you really insist, I'd like to keep it that way for > > that reason if you're OK with reason :-) > > Let's play top-trumps: > > % git grep "++i" | wc -l > 7062 > % git grep "i++" | wc -l > 75158 > > There is more than an order of magnitude of difference between the > styles and I bet quite a few of the aforementioned cases were authored > because they _required_ pre-inc. It's unlikely that $ git grep 'for (i.*++i' | wc -l 5861 _require_ pre-inc. But sure, since that's the preference, I've changed it > > [...] > > > > > diff --git a/include/linux/mfd/max77759.h b/include/linux/mfd/max77759.h > > > > new file mode 100644 > > > > index 0000000000000000000000000000000000000000..b038b4e9b748287e23e3a7030496f09dc8bdc816 > > > > --- /dev/null > > > > +++ b/include/linux/mfd/max77759.h > > > > @@ -0,0 +1,98 @@ > > > > +/* SPDX-License-Identifier: GPL-2.0-only */ > > > > +/* > > > > + * Copyright 2020 Google Inc. > > > > + * Copyright 2025 Linaro Ltd. > > > > + * > > > > + * Client interface for Maxim MAX77759 MFD driver > > > > + */ > > > > + > > > > +#ifndef __LINUX_MFD_MAX77759_H > > > > +#define __LINUX_MFD_MAX77759_H > > > > + > > > > +/* MaxQ opcodes */ > > > > +#define MAX77759_MAXQ_OPCODE_MAXLENGTH 33 > > > > + > > > > +#define MAX77759_MAXQ_OPCODE_GPIO_TRIGGER_READ 0x21 > > > > +#define MAX77759_MAXQ_OPCODE_GPIO_TRIGGER_WRITE 0x22 > > > > +#define MAX77759_MAXQ_OPCODE_GPIO_CONTROL_READ 0x23 > > > > +#define MAX77759_MAXQ_OPCODE_GPIO_CONTROL_WRITE 0x24 > > > > +#define MAX77759_MAXQ_OPCODE_USER_SPACE_READ 0x81 > > > > +#define MAX77759_MAXQ_OPCODE_USER_SPACE_WRITE 0x82 > > > > + > > > > +/* > > > > + * register map (incomplete) - registers not useful for drivers are not > > > > + * declared here > > > > + */ > > > > +/* MaxQ */ > > > > +#define MAX77759_MAXQ_REG_UIC_INT1 0x64 > > > > +#define MAX77759_MAXQ_REG_UIC_INT1_APCMDRESI BIT(7) > > > > +#define MAX77759_MAXQ_REG_UIC_INT1_SYSMSGI BIT(6) > > > > +#define MAX77759_MAXQ_REG_UIC_INT1_GPIO6I BIT(1) > > > > +#define MAX77759_MAXQ_REG_UIC_INT1_GPIO5I BIT(0) > > > > +#define MAX77759_MAXQ_REG_UIC_INT1_GPIOxI(offs, en) (((en) & 1) << (offs)) > > > > +#define MAX77759_MAXQ_REG_UIC_INT1_GPIOxI_MASK(offs) \ > > > > + MAX77759_MAXQ_REG_UIC_INT1_GPIOxI(offs, ~0) > > > > + > > > > +#define MAX77759_MAXQ_REG_UIC_INT2 0x65 > > > > +#define MAX77759_MAXQ_REG_UIC_INT3 0x66 > > > > +#define MAX77759_MAXQ_REG_UIC_INT4 0x67 > > > > +#define MAX77759_MAXQ_REG_UIC_UIC_STATUS1 0x68 > > > > +#define MAX77759_MAXQ_REG_UIC_UIC_STATUS2 0x69 > > > > +#define MAX77759_MAXQ_REG_UIC_UIC_STATUS3 0x6a > > > > +#define MAX77759_MAXQ_REG_UIC_UIC_STATUS4 0x6b > > > > +#define MAX77759_MAXQ_REG_UIC_UIC_STATUS5 0x6c > > > > +#define MAX77759_MAXQ_REG_UIC_UIC_STATUS6 0x6d > > > > +#define MAX77759_MAXQ_REG_UIC_UIC_STATUS7 0x6f > > > > +#define MAX77759_MAXQ_REG_UIC_UIC_STATUS8 0x6f > > > > +#define MAX77759_MAXQ_REG_UIC_INT1_M 0x70 > > > > +#define MAX77759_MAXQ_REG_UIC_INT2_M 0x71 > > > > +#define MAX77759_MAXQ_REG_UIC_INT3_M 0x72 > > > > +#define MAX77759_MAXQ_REG_UIC_INT4_M 0x73 > > > > + > > > > +/* charger */ > > > > +#define MAX77759_CHGR_REG_CHG_INT 0xb0 > > > > +#define MAX77759_CHGR_REG_CHG_INT2 0xb1 > > > > +#define MAX77759_CHGR_REG_CHG_INT_MASK 0xb2 > > > > +#define MAX77759_CHGR_REG_CHG_INT2_MASK 0xb3 > > > > + > > > > +struct max77759_mfd; > > > > > > Place the definition in here instead. > > > > I would like to keep it private. There is no need for it to > > become public, it's meant to be an opaque handle. > > No, it's not. > > It's device data that you already shared with all of the children. > > If you don't want them to have it, don't give it to them. It's normal to have opaque handles for API users if those API users don't need to know implementation details, like in this case. Anyway, I've made it public. Thanks Lee! Cheers, A. ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v4 5/6] gpio: max77759: add Maxim MAX77759 gpio driver 2025-03-12 9:25 [PATCH v4 0/6] Maxim Integrated MAX77759 PMIC MFD-based drivers André Draszik ` (3 preceding siblings ...) 2025-03-12 9:26 ` [PATCH v4 4/6] mfd: max77759: add Maxim MAX77759 core mfd driver André Draszik @ 2025-03-12 9:26 ` André Draszik 2025-03-12 9:26 ` [PATCH v4 6/6] nvmem: max77759: add Maxim MAX77759 NVMEM driver André Draszik 5 siblings, 0 replies; 12+ messages in thread From: André Draszik @ 2025-03-12 9:26 UTC (permalink / raw) To: Lee Jones, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Linus Walleij, Bartosz Golaszewski, Srinivas Kandagatla, Kees Cook, Gustavo A. R. Silva Cc: Peter Griffin, Tudor Ambarus, Will McVicker, kernel-team, linux-kernel, devicetree, linux-gpio, linux-hardening, André Draszik, Bartosz Golaszewski The Maxim MAX77759 is a companion PMIC for USB Type-C applications and includes Battery Charger, Fuel Gauge, temperature sensors, USB Type-C Port Controller (TCPC), NVMEM, and a GPIO expander. This driver supports the GPIO functions using the platform device registered by the core MFD driver. Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Acked-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> Signed-off-by: André Draszik <andre.draszik@linaro.org> --- v3: * drop duplicate init of 'handled' variable in irq handler * use boolean with IRQ_RETVAL() (Linus) * drop 'virq' variable inside irq handler to avoid confusion (Linus) * drop assignment of struct gpio_chip::owner (Linus) v2: * fix max77759_gpio_direction_from_control() * add missing error handling of devm_mutex_init() (Christophe) * align sentinel in max77759_gpio_of_id[] with other max77759 drivers (Christophe) --- MAINTAINERS | 1 + drivers/gpio/Kconfig | 13 ++ drivers/gpio/Makefile | 1 + drivers/gpio/gpio-max77759.c | 524 +++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 539 insertions(+) diff --git a/MAINTAINERS b/MAINTAINERS index 38844ea24e464d0b58f8852b79e2b94f18d48998..ef3aadcf86ce35d8807733c94f790cde0f7255af 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -14352,6 +14352,7 @@ M: André Draszik <andre.draszik@linaro.org> L: linux-kernel@vger.kernel.org S: Maintained F: Documentation/devicetree/bindings/*/maxim,max77759*.yaml +F: drivers/gpio/gpio-max77759.c F: drivers/mfd/max77759.c F: include/linux/mfd/max77759.h diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig index 3e9b174fee84142a8b50cdca5b967671eeda6dd3..216e80580fa443160c20da3e0916979fd08df99a 100644 --- a/drivers/gpio/Kconfig +++ b/drivers/gpio/Kconfig @@ -1463,6 +1463,19 @@ config GPIO_MAX77650 GPIO driver for MAX77650/77651 PMIC from Maxim Semiconductor. These chips have a single pin that can be configured as GPIO. +config GPIO_MAX77759 + tristate "Maxim Integrated MAX77759 GPIO support" + depends on MFD_MAX77759 + default MFD_MAX77759 + select GPIOLIB_IRQCHIP + help + GPIO driver for MAX77759 PMIC from Maxim Integrated. + There are two GPIOs available on these chips in total, both of + which can also generate interrupts. + + This driver can also be built as a module. If so, the module will be + called gpio-max77759. + config GPIO_PALMAS bool "TI PALMAS series PMICs GPIO" depends on MFD_PALMAS diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile index af130882ffeeef8b1d518867bfe1493ec4f21b5f..3b9a55f9f79eda33c135cfedb230ef7775b51bee 100644 --- a/drivers/gpio/Makefile +++ b/drivers/gpio/Makefile @@ -105,6 +105,7 @@ obj-$(CONFIG_GPIO_MAX730X) += gpio-max730x.o obj-$(CONFIG_GPIO_MAX732X) += gpio-max732x.o obj-$(CONFIG_GPIO_MAX77620) += gpio-max77620.o obj-$(CONFIG_GPIO_MAX77650) += gpio-max77650.o +obj-$(CONFIG_GPIO_MAX77759) += gpio-max77759.o obj-$(CONFIG_GPIO_MB86S7X) += gpio-mb86s7x.o obj-$(CONFIG_GPIO_MC33880) += gpio-mc33880.o obj-$(CONFIG_GPIO_MENZ127) += gpio-menz127.o diff --git a/drivers/gpio/gpio-max77759.c b/drivers/gpio/gpio-max77759.c new file mode 100644 index 0000000000000000000000000000000000000000..b0f5d839a347e472cbe0b8624b534ecf8f21f64c --- /dev/null +++ b/drivers/gpio/gpio-max77759.c @@ -0,0 +1,524 @@ +// SPDX-License-Identifier: GPL-2.0-only +// +// Copyright 2020 Google Inc +// Copyright 2025 Linaro Ltd. +// +// GPIO driver for Maxim MAX77759 + +#include <linux/dev_printk.h> +#include <linux/device.h> +#include <linux/device/driver.h> +#include <linux/gpio/driver.h> +#include <linux/interrupt.h> +#include <linux/irq.h> +#include <linux/irqreturn.h> +#include <linux/lockdep.h> +#include <linux/mfd/max77759.h> +#include <linux/mod_devicetable.h> +#include <linux/module.h> +#include <linux/overflow.h> +#include <linux/platform_device.h> +#include <linux/regmap.h> +#include <linux/seq_file.h> + +#define MAX77759_N_GPIOS ARRAY_SIZE(max77759_gpio_line_names) +static const char * const max77759_gpio_line_names[] = { "GPIO5", "GPIO6" }; + +struct max77759_gpio_chip { + struct regmap *map; + struct max77759_mfd *max77759_mfd; + struct gpio_chip gc; + struct mutex maxq_lock; /* protect MaxQ r/m/w operations */ + + struct mutex irq_lock; /* protect irq bus */ + int irq_mask; + int irq_mask_changed; + int irq_trig; + int irq_trig_changed; +}; + +#define MAX77759_GPIOx_TRIGGER(offs, val) (((val) & 1) << (offs)) +#define MAX77759_GPIOx_TRIGGER_MASK(offs) MAX77759_GPIOx_TRIGGER(offs, ~0) +enum max77759_trigger_gpio_type { + MAX77759_GPIO_TRIGGER_RISING = 0, + MAX77759_GPIO_TRIGGER_FALLING = 1 +}; + +#define MAX77759_GPIOx_DIR(offs, dir) (((dir) & 1) << (2 + (3 * (offs)))) +#define MAX77759_GPIOx_DIR_MASK(offs) MAX77759_GPIOx_DIR(offs, ~0) +enum max77759_control_gpio_dir { + MAX77759_GPIO_DIR_IN = 0, + MAX77759_GPIO_DIR_OUT = 1 +}; + +#define MAX77759_GPIOx_OUTVAL(offs, val) (((val) & 1) << (3 + (3 * (offs)))) +#define MAX77759_GPIOx_OUTVAL_MASK(offs) MAX77759_GPIOx_OUTVAL(offs, ~0) + +#define MAX77759_GPIOx_INVAL_MASK(offs) (BIT(4) << (3 * (offs))) + +static int max77759_gpio_maxq_gpio_trigger_read(struct max77759_gpio_chip *chip) +{ + DEFINE_FLEX(struct max77759_maxq_command, cmd, cmd, length, 1); + DEFINE_FLEX(struct max77759_maxq_response, rsp, rsp, length, 2); + int ret; + + cmd->cmd[0] = MAX77759_MAXQ_OPCODE_GPIO_TRIGGER_READ; + + ret = max77759_maxq_command(chip->max77759_mfd, cmd, rsp); + if (ret < 0) + return ret; + + return rsp->rsp[1]; +} + +static int max77759_gpio_maxq_gpio_trigger_write(struct max77759_gpio_chip *chip, + u8 trigger) +{ + DEFINE_FLEX(struct max77759_maxq_command, cmd, cmd, length, 2); + + cmd->cmd[0] = MAX77759_MAXQ_OPCODE_GPIO_TRIGGER_WRITE; + cmd->cmd[1] = trigger; + + return max77759_maxq_command(chip->max77759_mfd, cmd, NULL); +} + +static int max77759_gpio_maxq_gpio_control_read(struct max77759_gpio_chip *chip) +{ + DEFINE_FLEX(struct max77759_maxq_command, cmd, cmd, length, 1); + DEFINE_FLEX(struct max77759_maxq_response, rsp, rsp, length, 2); + int ret; + + cmd->cmd[0] = MAX77759_MAXQ_OPCODE_GPIO_CONTROL_READ; + + ret = max77759_maxq_command(chip->max77759_mfd, cmd, rsp); + if (ret < 0) + return ret; + + return rsp->rsp[1]; +} + +static int max77759_gpio_maxq_gpio_control_write(struct max77759_gpio_chip *chip, + u8 ctrl) +{ + DEFINE_FLEX(struct max77759_maxq_command, cmd, cmd, length, 2); + + cmd->cmd[0] = MAX77759_MAXQ_OPCODE_GPIO_CONTROL_WRITE; + cmd->cmd[1] = ctrl; + + return max77759_maxq_command(chip->max77759_mfd, cmd, NULL); +} + +static int +max77759_gpio_direction_from_control(int ctrl, unsigned int offset) +{ + enum max77759_control_gpio_dir dir; + + dir = !!(ctrl & MAX77759_GPIOx_DIR_MASK(offset)); + return ((dir == MAX77759_GPIO_DIR_OUT) + ? GPIO_LINE_DIRECTION_OUT + : GPIO_LINE_DIRECTION_IN); +} + +static int max77759_gpio_get_direction(struct gpio_chip *gc, + unsigned int offset) +{ + struct max77759_gpio_chip *chip = gpiochip_get_data(gc); + int ctrl; + + ctrl = max77759_gpio_maxq_gpio_control_read(chip); + if (ctrl < 0) + return ctrl; + + return max77759_gpio_direction_from_control(ctrl, offset); +} + +static int max77759_gpio_direction_helper(struct gpio_chip *gc, + unsigned int offset, + enum max77759_control_gpio_dir dir, + int value) +{ + struct max77759_gpio_chip *chip = gpiochip_get_data(gc); + int ctrl, new_ctrl; + + guard(mutex)(&chip->maxq_lock); + + ctrl = max77759_gpio_maxq_gpio_control_read(chip); + if (ctrl < 0) + return ctrl; + + new_ctrl = ctrl & ~MAX77759_GPIOx_DIR_MASK(offset); + new_ctrl |= MAX77759_GPIOx_DIR(offset, dir); + + if (dir == MAX77759_GPIO_DIR_OUT) { + new_ctrl &= ~MAX77759_GPIOx_OUTVAL_MASK(offset); + new_ctrl |= MAX77759_GPIOx_OUTVAL(offset, value); + } + + if (new_ctrl == ctrl) + return 0; + + return max77759_gpio_maxq_gpio_control_write(chip, new_ctrl); +} + +static int max77759_gpio_direction_input(struct gpio_chip *gc, + unsigned int offset) +{ + return max77759_gpio_direction_helper(gc, offset, + MAX77759_GPIO_DIR_IN, -1); +} + +static int max77759_gpio_direction_output(struct gpio_chip *gc, + unsigned int offset, int value) +{ + return max77759_gpio_direction_helper(gc, offset, + MAX77759_GPIO_DIR_OUT, value); +} + +static int max77759_gpio_get_value(struct gpio_chip *gc, unsigned int offset) +{ + struct max77759_gpio_chip *chip = gpiochip_get_data(gc); + int ctrl, mask; + + ctrl = max77759_gpio_maxq_gpio_control_read(chip); + if (ctrl < 0) + return ctrl; + + /* + * The input status bit doesn't reflect the pin state when the GPIO is + * configured as an output. Check the direction, and inspect the input + * or output bit accordingly. + */ + mask = ((max77759_gpio_direction_from_control(ctrl, offset) + == GPIO_LINE_DIRECTION_IN) + ? MAX77759_GPIOx_INVAL_MASK(offset) + : MAX77759_GPIOx_OUTVAL_MASK(offset)); + + return !!(ctrl & mask); +} + +static void max77759_gpio_set_value(struct gpio_chip *gc, + unsigned int offset, int value) +{ + struct max77759_gpio_chip *chip = gpiochip_get_data(gc); + int ctrl, new_ctrl; + + guard(mutex)(&chip->maxq_lock); + + ctrl = max77759_gpio_maxq_gpio_control_read(chip); + if (ctrl < 0) + return; + + new_ctrl = ctrl & ~MAX77759_GPIOx_OUTVAL_MASK(offset); + new_ctrl |= MAX77759_GPIOx_OUTVAL(offset, value); + + if (new_ctrl == ctrl) + return; + + max77759_gpio_maxq_gpio_control_write(chip, new_ctrl); +} + +static void max77759_gpio_irq_mask(struct irq_data *d) +{ + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); + struct max77759_gpio_chip *chip = gpiochip_get_data(gc); + irq_hw_number_t hwirq = irqd_to_hwirq(d); + + chip->irq_mask &= ~MAX77759_MAXQ_REG_UIC_INT1_GPIOxI_MASK(hwirq); + chip->irq_mask |= MAX77759_MAXQ_REG_UIC_INT1_GPIOxI(hwirq, 1); + chip->irq_mask_changed |= MAX77759_MAXQ_REG_UIC_INT1_GPIOxI(hwirq, 1); + + gpiochip_disable_irq(gc, hwirq); +} + +static void max77759_gpio_irq_unmask(struct irq_data *d) +{ + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); + struct max77759_gpio_chip *chip = gpiochip_get_data(gc); + irq_hw_number_t hwirq = irqd_to_hwirq(d); + + gpiochip_enable_irq(gc, hwirq); + + chip->irq_mask &= ~MAX77759_MAXQ_REG_UIC_INT1_GPIOxI_MASK(hwirq); + chip->irq_mask |= MAX77759_MAXQ_REG_UIC_INT1_GPIOxI(hwirq, 0); + chip->irq_mask_changed |= MAX77759_MAXQ_REG_UIC_INT1_GPIOxI(hwirq, 1); +} + +static int max77759_gpio_set_irq_type(struct irq_data *d, unsigned int type) +{ + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); + struct max77759_gpio_chip *chip = gpiochip_get_data(gc); + irq_hw_number_t hwirq = irqd_to_hwirq(d); + + chip->irq_trig &= ~MAX77759_GPIOx_TRIGGER_MASK(hwirq); + switch (type) { + case IRQ_TYPE_EDGE_RISING: + chip->irq_trig |= MAX77759_GPIOx_TRIGGER(hwirq, + MAX77759_GPIO_TRIGGER_RISING); + break; + + case IRQ_TYPE_EDGE_FALLING: + chip->irq_trig |= MAX77759_GPIOx_TRIGGER(hwirq, + MAX77759_GPIO_TRIGGER_FALLING); + break; + + default: + return -EINVAL; + } + + chip->irq_trig_changed |= MAX77759_GPIOx_TRIGGER(hwirq, 1); + + return 0; +} + +static void max77759_gpio_bus_lock(struct irq_data *d) +{ + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); + struct max77759_gpio_chip *chip = gpiochip_get_data(gc); + + mutex_lock(&chip->irq_lock); +} + +static int max77759_gpio_bus_sync_unlock_helper(struct gpio_chip *gc, + struct max77759_gpio_chip *chip) + __must_hold(&chip->maxq_lock) +{ + int ctrl, trigger, new_trigger, new_ctrl; + unsigned long irq_trig_changed; + int offset; + int ret; + + lockdep_assert_held(&chip->maxq_lock); + + ctrl = max77759_gpio_maxq_gpio_control_read(chip); + trigger = max77759_gpio_maxq_gpio_trigger_read(chip); + if (ctrl < 0 || trigger < 0) { + dev_err(gc->parent, "failed to read current state: %d / %d\n", + ctrl, trigger); + return (ctrl < 0) ? ctrl : trigger; + } + + new_trigger = trigger & ~chip->irq_trig_changed; + new_trigger |= (chip->irq_trig & chip->irq_trig_changed); + + /* change GPIO direction if required */ + new_ctrl = ctrl; + irq_trig_changed = chip->irq_trig_changed; + for_each_set_bit(offset, &irq_trig_changed, MAX77759_N_GPIOS) { + new_ctrl &= ~MAX77759_GPIOx_DIR_MASK(offset); + new_ctrl |= MAX77759_GPIOx_DIR(offset, MAX77759_GPIO_DIR_IN); + } + + if (new_trigger != trigger) { + ret = max77759_gpio_maxq_gpio_trigger_write(chip, new_trigger); + if (ret) { + dev_err(gc->parent, + "failed to write new trigger: %d\n", ret); + return ret; + } + } + + if (new_ctrl != ctrl) { + ret = max77759_gpio_maxq_gpio_control_write(chip, new_ctrl); + if (ret) { + dev_err(gc->parent, + "failed to write new control: %d\n", ret); + return ret; + } + } + + chip->irq_trig_changed = 0; + + return 0; +} + +static void max77759_gpio_bus_sync_unlock(struct irq_data *d) +{ + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); + struct max77759_gpio_chip *chip = gpiochip_get_data(gc); + int ret; + + scoped_guard(mutex, &chip->maxq_lock) { + ret = max77759_gpio_bus_sync_unlock_helper(gc, chip); + if (ret) + goto out_unlock; + } + + ret = regmap_update_bits(chip->map, + MAX77759_MAXQ_REG_UIC_INT1_M, + chip->irq_mask_changed, chip->irq_mask); + if (ret) { + dev_err(gc->parent, + "failed to update UIC_INT1 irq mask: %d\n", ret); + goto out_unlock; + } + + chip->irq_mask_changed = 0; + +out_unlock: + mutex_unlock(&chip->irq_lock); +} + +static void max77759_gpio_irq_print_chip(struct irq_data *d, struct seq_file *p) +{ + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); + + seq_puts(p, dev_name(gc->parent)); +} + +static const struct irq_chip max77759_gpio_irq_chip = { + .irq_mask = max77759_gpio_irq_mask, + .irq_unmask = max77759_gpio_irq_unmask, + .irq_set_type = max77759_gpio_set_irq_type, + .irq_bus_lock = max77759_gpio_bus_lock, + .irq_bus_sync_unlock = max77759_gpio_bus_sync_unlock, + .irq_print_chip = max77759_gpio_irq_print_chip, + .flags = IRQCHIP_IMMUTABLE, + GPIOCHIP_IRQ_RESOURCE_HELPERS, +}; + +static irqreturn_t max77759_gpio_irqhandler(int irq, void *data) +{ + struct max77759_gpio_chip *chip = data; + struct gpio_chip *gc = &chip->gc; + bool handled = false; + + /* iterate until no interrupt is pending */ + while (true) { + unsigned int uic_int1; + int ret; + unsigned long pending; + int offset; + + ret = regmap_read(chip->map, MAX77759_MAXQ_REG_UIC_INT1, + &uic_int1); + if (ret < 0) { + dev_err_ratelimited(gc->parent, + "failed to read IRQ status: %d\n", + ret); + /* + * If !handled, we have looped not even once, which + * means we should return IRQ_NONE in that case (and + * of course IRQ_HANDLED otherwise). + */ + return IRQ_RETVAL(handled); + } + + pending = uic_int1; + pending &= (MAX77759_MAXQ_REG_UIC_INT1_GPIO6I + | MAX77759_MAXQ_REG_UIC_INT1_GPIO5I); + if (!pending) + break; + + for_each_set_bit(offset, &pending, MAX77759_N_GPIOS) { + /* + * ACK interrupt by writing 1 to bit 'offset', all + * others need to be written as 0. This needs to be + * done unconditionally hence regmap_set_bits() is + * inappropriate here. + */ + regmap_write(chip->map, MAX77759_MAXQ_REG_UIC_INT1, + BIT(offset)); + + handle_nested_irq(irq_find_mapping(gc->irq.domain, + offset)); + + handled = true; + } + } + + return IRQ_RETVAL(handled); +} + +static int max77759_gpio_probe(struct platform_device *pdev) +{ + struct max77759_gpio_chip *chip; + int irq; + struct gpio_irq_chip *girq; + int ret; + unsigned long irq_flags; + struct irq_data *irqd; + + chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL); + if (!chip) + return -ENOMEM; + + chip->map = dev_get_regmap(pdev->dev.parent, "maxq"); + if (!chip->map) + return dev_err_probe(&pdev->dev, -ENODEV, "Missing regmap\n"); + + irq = platform_get_irq_byname(pdev, "GPI"); + if (irq < 0) + return dev_err_probe(&pdev->dev, irq, "Failed to get IRQ\n"); + + chip->max77759_mfd = dev_get_drvdata(pdev->dev.parent); + ret = devm_mutex_init(&pdev->dev, &chip->maxq_lock); + if (ret) + return ret; + ret = devm_mutex_init(&pdev->dev, &chip->irq_lock); + if (ret) + return ret; + + chip->gc.base = -1; + chip->gc.label = dev_name(&pdev->dev); + chip->gc.parent = &pdev->dev; + chip->gc.can_sleep = true; + + chip->gc.names = max77759_gpio_line_names; + chip->gc.ngpio = MAX77759_N_GPIOS; + chip->gc.get_direction = max77759_gpio_get_direction; + chip->gc.direction_input = max77759_gpio_direction_input; + chip->gc.direction_output = max77759_gpio_direction_output; + chip->gc.get = max77759_gpio_get_value; + chip->gc.set = max77759_gpio_set_value; + + girq = &chip->gc.irq; + gpio_irq_chip_set_chip(girq, &max77759_gpio_irq_chip); + /* This will let us handle the parent IRQ in the driver */ + girq->parent_handler = NULL; + girq->num_parents = 0; + girq->parents = NULL; + girq->default_type = IRQ_TYPE_NONE; + girq->handler = handle_simple_irq; + girq->threaded = true; + + ret = devm_gpiochip_add_data(&pdev->dev, &chip->gc, chip); + if (ret < 0) + return dev_err_probe(&pdev->dev, ret, + "Failed to add GPIO chip\n"); + + irq_flags = IRQF_ONESHOT | IRQF_SHARED; + irqd = irq_get_irq_data(irq); + if (irqd) + irq_flags |= irqd_get_trigger_type(irqd); + + ret = devm_request_threaded_irq(&pdev->dev, irq, NULL, + max77759_gpio_irqhandler, irq_flags, + dev_name(&pdev->dev), chip); + if (ret < 0) + return dev_err_probe(&pdev->dev, ret, + "Failed to request IRQ\n"); + + return ret; +} + +static const struct of_device_id max77759_gpio_of_id[] = { + { .compatible = "maxim,max77759-gpio", }, + { } +}; +MODULE_DEVICE_TABLE(of, max77759_gpio_of_id); + +static struct platform_driver max77759_gpio_driver = { + .driver = { + .name = "max77759-gpio", + .probe_type = PROBE_PREFER_ASYNCHRONOUS, + .of_match_table = max77759_gpio_of_id, + }, + .probe = max77759_gpio_probe, +}; + +module_platform_driver(max77759_gpio_driver); + +MODULE_AUTHOR("André Draszik <andre.draszik@linaro.org>"); +MODULE_DESCRIPTION("GPIO driver for Maxim MAX77759"); +MODULE_LICENSE("GPL"); +MODULE_ALIAS("platform:max77759-gpio"); -- 2.49.0.rc0.332.g42c0ae87b1-goog ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v4 6/6] nvmem: max77759: add Maxim MAX77759 NVMEM driver 2025-03-12 9:25 [PATCH v4 0/6] Maxim Integrated MAX77759 PMIC MFD-based drivers André Draszik ` (4 preceding siblings ...) 2025-03-12 9:26 ` [PATCH v4 5/6] gpio: max77759: add Maxim MAX77759 gpio driver André Draszik @ 2025-03-12 9:26 ` André Draszik 5 siblings, 0 replies; 12+ messages in thread From: André Draszik @ 2025-03-12 9:26 UTC (permalink / raw) To: Lee Jones, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Linus Walleij, Bartosz Golaszewski, Srinivas Kandagatla, Kees Cook, Gustavo A. R. Silva Cc: Peter Griffin, Tudor Ambarus, Will McVicker, kernel-team, linux-kernel, devicetree, linux-gpio, linux-hardening, André Draszik The Maxim MAX77759 is a companion PMIC for USB Type-C applications and includes Battery Charger, Fuel Gauge, temperature sensors, USB Type-C Port Controller (TCPC), NVMEM, and a GPIO expander. This driver exposes the non volatile memory using the platform device registered by the core MFD driver. Signed-off-by: André Draszik <andre.draszik@linaro.org> --- v2: * align sentinel in max77759_nvmem_of_id[] with other max77759 drivers (Christophe) --- MAINTAINERS | 1 + drivers/nvmem/Kconfig | 12 ++++ drivers/nvmem/Makefile | 2 + drivers/nvmem/max77759-nvmem.c | 156 +++++++++++++++++++++++++++++++++++++++++ 4 files changed, 171 insertions(+) diff --git a/MAINTAINERS b/MAINTAINERS index ef3aadcf86ce35d8807733c94f790cde0f7255af..88c53e3fabe1760abf7914290c8729330739b0b9 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -14354,6 +14354,7 @@ S: Maintained F: Documentation/devicetree/bindings/*/maxim,max77759*.yaml F: drivers/gpio/gpio-max77759.c F: drivers/mfd/max77759.c +F: drivers/nvmem/max77759-nvmem.c F: include/linux/mfd/max77759.h MAXIM MAX77802 PMIC REGULATOR DEVICE DRIVER diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig index 8671b7c974b933e147154bb40b5d41b5730518d2..3de07ef524906ad24a89e58abdfe93529a83c80f 100644 --- a/drivers/nvmem/Kconfig +++ b/drivers/nvmem/Kconfig @@ -154,6 +154,18 @@ config NVMEM_LPC18XX_OTP To compile this driver as a module, choose M here: the module will be called nvmem_lpc18xx_otp. +config NVMEM_MAX77759 + tristate "Maxim Integrated MAX77759 NVMEM Support" + depends on MFD_MAX77759 + default MFD_MAX77759 + help + Say Y here to include support for the user-accessible storage found + in Maxim Integrated MAX77759 PMICs. This IC provides space for 30 + bytes of storage. + + This driver can also be built as a module. If so, the module + will be called nvmem-max77759. + config NVMEM_MESON_EFUSE tristate "Amlogic Meson GX eFuse Support" depends on (ARCH_MESON || COMPILE_TEST) && MESON_SM diff --git a/drivers/nvmem/Makefile b/drivers/nvmem/Makefile index 5b77bbb6488bf89bfb305750a1cbf4a6731a0a58..a9d03cfbbd27e68d40f8c330e72e20378b12a481 100644 --- a/drivers/nvmem/Makefile +++ b/drivers/nvmem/Makefile @@ -34,6 +34,8 @@ obj-$(CONFIG_NVMEM_LPC18XX_EEPROM) += nvmem_lpc18xx_eeprom.o nvmem_lpc18xx_eeprom-y := lpc18xx_eeprom.o obj-$(CONFIG_NVMEM_LPC18XX_OTP) += nvmem_lpc18xx_otp.o nvmem_lpc18xx_otp-y := lpc18xx_otp.o +obj-$(CONFIG_NVMEM_MAX77759) += nvmem-max77759.o +nvmem-max77759-y := max77759-nvmem.o obj-$(CONFIG_NVMEM_MESON_EFUSE) += nvmem_meson_efuse.o nvmem_meson_efuse-y := meson-efuse.o obj-$(CONFIG_NVMEM_MESON_MX_EFUSE) += nvmem_meson_mx_efuse.o diff --git a/drivers/nvmem/max77759-nvmem.c b/drivers/nvmem/max77759-nvmem.c new file mode 100644 index 0000000000000000000000000000000000000000..bc535a73daaaf2caeb772cd17da61f8a030b6a6f --- /dev/null +++ b/drivers/nvmem/max77759-nvmem.c @@ -0,0 +1,156 @@ +// SPDX-License-Identifier: GPL-2.0-only +// +// Copyright 2020 Google Inc +// Copyright 2025 Linaro Ltd. +// +// NVMEM driver for Maxim MAX77759 + +#include <linux/dev_printk.h> +#include <linux/device.h> +#include <linux/device/driver.h> +#include <linux/err.h> +#include <linux/mfd/max77759.h> +#include <linux/mod_devicetable.h> +#include <linux/module.h> +#include <linux/nvmem-provider.h> +#include <linux/overflow.h> +#include <linux/platform_device.h> +#include <linux/string.h> + +#define MAX77759_NVMEM_OPCODE_HEADER_LEN 3 +/* + * NVMEM commands have a three byte header (which becomes part of the command), + * so we need to subtract that. + */ +#define MAX77759_NVMEM_SIZE (MAX77759_MAXQ_OPCODE_MAXLENGTH \ + - MAX77759_NVMEM_OPCODE_HEADER_LEN) + +struct max77759_nvmem { + struct device *dev; + struct max77759_mfd *max77759_mfd; +}; + +static bool max77759_nvmem_is_valid(unsigned int offset, size_t bytes) +{ + return (offset + bytes - 1 <= MAX77759_NVMEM_SIZE); +} + +static int max77759_nvmem_reg_read(void *priv, unsigned int offset, + void *val, size_t bytes) +{ + struct max77759_nvmem *nvmem = priv; + DEFINE_FLEX(struct max77759_maxq_command, cmd, cmd, length, + MAX77759_NVMEM_OPCODE_HEADER_LEN); + DEFINE_FLEX(struct max77759_maxq_response, rsp, rsp, length, + MAX77759_MAXQ_OPCODE_MAXLENGTH); + int ret; + + if (!max77759_nvmem_is_valid(offset, bytes)) { + dev_err(nvmem->dev, "outside NVMEM area: %u / %zu\n", + offset, bytes); + return -EINVAL; + } + + cmd->cmd[0] = MAX77759_MAXQ_OPCODE_USER_SPACE_READ; + cmd->cmd[1] = offset; + cmd->cmd[2] = bytes; + rsp->length = bytes + MAX77759_NVMEM_OPCODE_HEADER_LEN; + + ret = max77759_maxq_command(nvmem->max77759_mfd, cmd, rsp); + if (ret < 0) + return ret; + + if (memcmp(cmd->cmd, rsp->rsp, MAX77759_NVMEM_OPCODE_HEADER_LEN)) { + dev_warn(nvmem->dev, "protocol error (read)\n"); + return -EIO; + } + + memcpy(val, &rsp->rsp[MAX77759_NVMEM_OPCODE_HEADER_LEN], bytes); + + return 0; +} + +static int max77759_nvmem_reg_write(void *priv, unsigned int offset, + void *val, size_t bytes) +{ + struct max77759_nvmem *nvmem = priv; + DEFINE_FLEX(struct max77759_maxq_command, cmd, cmd, length, + MAX77759_MAXQ_OPCODE_MAXLENGTH); + DEFINE_FLEX(struct max77759_maxq_response, rsp, rsp, length, + MAX77759_MAXQ_OPCODE_MAXLENGTH); + int ret; + + if (!max77759_nvmem_is_valid(offset, bytes)) { + dev_err(nvmem->dev, "outside NVMEM area: %u / %zu\n", + offset, bytes); + return -EINVAL; + } + + cmd->cmd[0] = MAX77759_MAXQ_OPCODE_USER_SPACE_WRITE; + cmd->cmd[1] = offset; + cmd->cmd[2] = bytes; + memcpy(&cmd->cmd[MAX77759_NVMEM_OPCODE_HEADER_LEN], val, bytes); + cmd->length = bytes + MAX77759_NVMEM_OPCODE_HEADER_LEN; + rsp->length = cmd->length; + + ret = max77759_maxq_command(nvmem->max77759_mfd, cmd, rsp); + if (ret < 0) + return ret; + + if (memcmp(cmd->cmd, rsp->rsp, cmd->length)) { + dev_warn(nvmem->dev, "protocol error (write)\n"); + return -EIO; + } + + return 0; +} + +static int max77759_nvmem_probe(struct platform_device *pdev) +{ + struct nvmem_config config = { + .dev = &pdev->dev, + .name = dev_name(&pdev->dev), + .id = NVMEM_DEVID_NONE, + .type = NVMEM_TYPE_EEPROM, + .ignore_wp = true, + .size = MAX77759_NVMEM_SIZE, + .word_size = sizeof(u8), + .stride = sizeof(u8), + .reg_read = max77759_nvmem_reg_read, + .reg_write = max77759_nvmem_reg_write, + }; + struct max77759_nvmem *nvmem; + + nvmem = devm_kzalloc(&pdev->dev, sizeof(*nvmem), GFP_KERNEL); + if (!nvmem) + return -ENOMEM; + + nvmem->dev = &pdev->dev; + nvmem->max77759_mfd = dev_get_drvdata(pdev->dev.parent); + + config.priv = nvmem; + + return PTR_ERR_OR_ZERO(devm_nvmem_register(config.dev, &config)); +} + +static const struct of_device_id max77759_nvmem_of_id[] = { + { .compatible = "maxim,max77759-nvmem", }, + { } +}; +MODULE_DEVICE_TABLE(of, max77759_nvmem_of_id); + +static struct platform_driver max77759_nvmem_driver = { + .driver = { + .name = "max77759-nvmem", + .probe_type = PROBE_PREFER_ASYNCHRONOUS, + .of_match_table = max77759_nvmem_of_id, + }, + .probe = max77759_nvmem_probe, +}; + +module_platform_driver(max77759_nvmem_driver); + +MODULE_AUTHOR("André Draszik <andre.draszik@linaro.org>"); +MODULE_DESCRIPTION("NVMEM driver for Maxim MAX77759"); +MODULE_LICENSE("GPL"); +MODULE_ALIAS("platform:max77759-nvmem"); -- 2.49.0.rc0.332.g42c0ae87b1-goog ^ permalink raw reply related [flat|nested] 12+ messages in thread
end of thread, other threads:[~2025-03-25 7:44 UTC | newest] Thread overview: 12+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-03-12 9:25 [PATCH v4 0/6] Maxim Integrated MAX77759 PMIC MFD-based drivers André Draszik 2025-03-12 9:25 ` [PATCH v4 1/6] dt-bindings: gpio: add max77759 binding André Draszik 2025-03-12 10:01 ` Bartosz Golaszewski 2025-03-12 9:26 ` [PATCH v4 2/6] dt-bindings: nvmem: " André Draszik 2025-03-12 9:26 ` [PATCH v4 3/6] dt-bindings: mfd: " André Draszik 2025-03-12 9:26 ` [PATCH v4 4/6] mfd: max77759: add Maxim MAX77759 core mfd driver André Draszik 2025-03-14 12:31 ` Lee Jones 2025-03-14 15:30 ` André Draszik 2025-03-21 10:54 ` Lee Jones 2025-03-25 7:44 ` André Draszik 2025-03-12 9:26 ` [PATCH v4 5/6] gpio: max77759: add Maxim MAX77759 gpio driver André Draszik 2025-03-12 9:26 ` [PATCH v4 6/6] nvmem: max77759: add Maxim MAX77759 NVMEM driver André Draszik
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.