drivers: gpio: axp192: GPIO driver implementation #59768

MrMarteng · 2023-06-26T16:20:54Z

AXP192 is a small power management IC, that also features 5 GPIOS.
Besides GPIO driver this commit also includes needed modifications in axp192 regulator and mfd driver as LDOIO0 functioanlity is multiplexed with GPIO0 pin.

Signed-off-by: Martin Kiepfer [email protected]

MrMarteng · 2023-08-08T18:48:40Z

Hey guys, this is a kind reminder to support me on this PR.

gmarull · 2023-08-09T07:38:16Z

drivers/gpio/gpio_axp192.c

+	if (pin >= AXP192_GPIO_NUM_PINS) {
+		LOG_ERR("Invalid gpio pin (%d)", pin);
+		return -EINVAL;
+	}


I think you should be using ngpios property

The number is physically restricted and this information is also needed on mfd layer. I can use the ngpios property in gpio driver but would it really be the better solution to have 2 definitions then?

gmarull · 2023-08-09T07:40:15Z

drivers/gpio/gpio_axp192.c

+	uint8_t port_val;
+	const struct gpio_axp192_config *config = dev->config;
+
+	ret = mfd_axp192_gpio_read_port(config->mfd, &port_val);


this can block, right? If so you need something like https://github.com/zephyrproject-rtos/zephyr/blob/main/drivers/gpio/gpio_npm6001.c#L53 (per API interface specs)

gmarull · 2023-08-09T07:41:49Z

Hey guys, this is a kind reminder to support me on this PR.

lgtm, just some minor comments. Believe it or not, GPIO area does not have a maintainer right now...

gmarull · 2023-08-10T07:05:29Z

drivers/mfd/mfd_axp192.c

@@ -50,12 +153,495 @@ static int mfd_axp192_init(const struct device *dev)
 	return 0;
 }

+int mfd_axp192_gpio_func_get(const struct device *dev, uint8_t gpio, enum axp192_gpio_func *func)


question: why is this part of the mfd layer? who else is using this?

It is used by regulator and gpio drivers as some pins of the pmu are multiplexed. And it also was a recommendation in my regulator PR (#58809 (comment)) .

gmarull · 2023-08-10T07:05:55Z

drivers/mfd/mfd_axp192.c

+	if (k_is_in_isr()) {
+		return -EWOULDBLOCK;
+	}


this is translating GPIO API requirements to the mfd layer... wrong

I See. Thank you.
Fixed it.

gmarull · 2023-08-16T08:28:24Z

drivers/gpio/gpio_axp192.c

+	return gpio_axp192_port_set_masked_raw(dev, pins, 0);
+}
+
+static inline int gpio_axp192_configure(const struct device *dev, gpio_pin_t pin,


Suggested change

static inline int gpio_axp192_configure(const struct device *dev, gpio_pin_t pin,

static int gpio_axp192_configure(const struct device *dev, gpio_pin_t pin,

gmarull · 2023-08-16T08:29:41Z

tests/drivers/build_all/gpio/prj.conf

@@ -4,4 +4,5 @@ CONFIG_TEST_USERSPACE=y
 CONFIG_I2C=y
 CONFIG_GPIO_PCA95XX_INTERRUPT=y
 CONFIG_SPI=y
+CONFIG_MFD=y


redundant, driver must select MFD

gmarull

changes to regulator driver should go into its own commit maybe?

aasinclair · 2023-08-16T10:16:31Z

drivers/mfd/mfd_axp192.c

+#define AXP192_GPIO0_INPUT_VAL      0x10U
+#define AXP192_GPIO1_INPUT_VAL      0x20U
+#define AXP192_GPIO2_INPUT_VAL      0x40U
+#define AXP192_GPIO012_INTPUT_SHIFT 4U


nit: Should this be INPUT?

aasinclair · 2023-08-16T10:44:12Z

drivers/mfd/mfd_axp192.c

+	if ((port_val & (1u << pin)) != 0) {
+		*value = true;
+	} else {
+		*value = false;
+	}


Suggested change

if ((port_val & (1u << pin)) != 0) {

*value = true;

} else {

*value = false;

}

*value = (port_val & BIT(pin)) != 0;

aasinclair · 2023-08-16T10:45:20Z

drivers/mfd/mfd_axp192.c

-	DEVICE_DT_INST_DEFINE(inst, mfd_axp192_init, NULL, NULL, &config##inst, POST_KERNEL,       \
-			      CONFIG_MFD_INIT_PRIORITY, NULL);
+	static struct mfd_axp192_data data##inst = {                                               \
+		.gpio_mask_used = {0},                                                             \


You don't need to initialise static data

aasinclair

This is looking really good

aasinclair · 2023-08-17T08:52:52Z

drivers/gpio/gpio_axp192.c

+			ret = mfd_axp192_gpio_write_pin(config->mfd, pin, false);
+		} else if ((flags & GPIO_OUTPUT_INIT_HIGH) != 0) {
+			ret = mfd_axp192_gpio_write_pin(config->mfd, pin, true);


Is this the only place you are using mfd_axp192_gpio_write_pin?

If so you could replace it with a call to mfd_axp192_gpio_write_port(config->mfd, BIT(pin),...) and remove mfd_axp192_gpio_write_pin entirely.

aasinclair · 2023-08-17T08:58:50Z

drivers/mfd/mfd_axp192.c

+	return 0;
+}
+
+int mfd_axp192_gpio_read_pin(const struct device *dev, uint8_t pin, bool *value)


Is this function needed?

AXP192 is a small power management IC, that also features 5 GPIOS. Besides GPIO driver this commit also includes needed modifications in axp192 regulator and mfd driver as LDOIO0 functioanlity is multiplexed with GPIO0 pin. Signed-off-by: Martin Kiepfer <[email protected]>

MrMarteng · 2023-08-22T10:59:40Z

@gmarull @henrikbrixandersen do you have additional comments or would you like to approve the PR as well?

MrMarteng force-pushed the axp192_gpio branch 6 times, most recently from a8e6ab9 to 17a04d0 Compare June 28, 2023 05:10

MrMarteng mentioned this pull request Jun 28, 2023

driver: regulator: Add support for AXP192 power management IC #58809

Merged

MrMarteng force-pushed the axp192_gpio branch 5 times, most recently from 1660e05 to 3c10038 Compare July 4, 2023 08:46

MrMarteng force-pushed the axp192_gpio branch from 3c10038 to 0d6703a Compare July 9, 2023 20:46

MrMarteng force-pushed the axp192_gpio branch 4 times, most recently from 43be5b7 to 9200109 Compare July 21, 2023 07:06

MrMarteng marked this pull request as ready for review July 21, 2023 08:07

MrMarteng requested review from nashif, carlescufi, galak, MaureenHelm, gmarull and mnkp as code owners July 21, 2023 08:07

zephyrbot added area: Regulators area: GPIO area: Devicetree Binding PR modifies or adds a Device Tree binding labels Jul 21, 2023

zephyrbot requested review from aasinclair, danieldegrasse and henrikbrixandersen July 21, 2023 08:07

gmarull reviewed Aug 9, 2023

View reviewed changes

MrMarteng force-pushed the axp192_gpio branch from b142ff2 to e762e26 Compare August 9, 2023 20:38

gmarull requested changes Aug 10, 2023

View reviewed changes

MrMarteng force-pushed the axp192_gpio branch 2 times, most recently from aa5c50d to 252c73e Compare August 11, 2023 07:22

MrMarteng requested a review from gmarull August 11, 2023 08:17

gmarull reviewed Aug 16, 2023

View reviewed changes

MrMarteng force-pushed the axp192_gpio branch from 252c73e to b374fa7 Compare August 16, 2023 08:51

MrMarteng requested a review from gmarull August 16, 2023 09:59

aasinclair reviewed Aug 16, 2023

View reviewed changes

MrMarteng force-pushed the axp192_gpio branch 2 times, most recently from 42823dc to b9424a1 Compare August 16, 2023 14:45

MrMarteng requested a review from aasinclair August 16, 2023 16:06

aasinclair reviewed Aug 17, 2023

View reviewed changes

MrMarteng force-pushed the axp192_gpio branch from b9424a1 to c36d916 Compare August 17, 2023 11:07

MrMarteng force-pushed the axp192_gpio branch from c36d916 to 148bd05 Compare August 17, 2023 11:13

MrMarteng requested a review from aasinclair August 17, 2023 14:45

aasinclair approved these changes Aug 17, 2023

View reviewed changes

gmarull approved these changes Aug 22, 2023

View reviewed changes

henrikbrixandersen approved these changes Aug 22, 2023

View reviewed changes

carlescufi merged commit 74db02b into zephyrproject-rtos:main Aug 22, 2023
17 checks passed

MrMarteng mentioned this pull request Sep 4, 2023

board: m5stack-core2 module support #61812

Merged

MrMarteng deleted the axp192_gpio branch September 14, 2023 17:58

MrMarteng mentioned this pull request Jan 17, 2024

Nominate mrmarteng as zephyr contributor #67746

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

drivers: gpio: axp192: GPIO driver implementation #59768

drivers: gpio: axp192: GPIO driver implementation #59768

MrMarteng commented Jun 26, 2023 •

edited

Loading

MrMarteng commented Aug 8, 2023

gmarull Aug 9, 2023

MrMarteng Aug 9, 2023

gmarull Aug 9, 2023

gmarull commented Aug 9, 2023

gmarull Aug 10, 2023

MrMarteng Aug 10, 2023

gmarull Aug 10, 2023

MrMarteng Aug 10, 2023 •

edited

Loading

gmarull Aug 16, 2023

gmarull Aug 16, 2023

gmarull left a comment

aasinclair Aug 16, 2023

aasinclair Aug 16, 2023

aasinclair Aug 16, 2023

aasinclair left a comment

aasinclair Aug 17, 2023

aasinclair Aug 17, 2023

MrMarteng commented Aug 22, 2023 •

edited

Loading

	static inline int gpio_axp192_configure(const struct device *dev, gpio_pin_t pin,
	static int gpio_axp192_configure(const struct device *dev, gpio_pin_t pin,

drivers: gpio: axp192: GPIO driver implementation #59768

drivers: gpio: axp192: GPIO driver implementation #59768

Conversation

MrMarteng commented Jun 26, 2023 • edited Loading

MrMarteng commented Aug 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmarull commented Aug 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MrMarteng Aug 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmarull left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aasinclair left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MrMarteng commented Aug 22, 2023 • edited Loading

MrMarteng commented Jun 26, 2023 •

edited

Loading

MrMarteng Aug 10, 2023 •

edited

Loading

MrMarteng commented Aug 22, 2023 •

edited

Loading