drivers/mfd: it8801: Implement ITE IT8801 mfd drivers #79416

GTLin08 · 2024-10-04T10:19:12Z

This PR Implements the ITE IT8801 IO expander multi-function device driver. The IO expander provides GPIO, PWM, and keyboard functions via the I2C bus.

keith-zephyr

Also consider running clang-format on all the files.

keith-zephyr · 2024-10-04T15:14:47Z

drivers/mfd/mfd_ite_it8801.c

+#include <zephyr/logging/log.h>
+LOG_MODULE_REGISTER(mfd_ite_it8801, CONFIG_MFD_LOG_LEVEL);
+
+static const struct device *kbd_dev = DEVICE_DT_GET_ANY(ite_it8801_kbd);


This driver supports multiple instances, but this this assumes that there is only one IT8801 defined.

Fixed, use multiple instances.

keith-zephyr · 2024-10-04T15:18:21Z

drivers/mfd/mfd_ite_it8801.c

+	for (i = 0; i < config->sub_mfd_node_num; i++) {
+		if (config->sub_mfd_dev[i] != NULL) {
+			if (config->sub_mfd_dev[i] == kbd_dev) {
+				it8801_input_alert_handler(config->sub_mfd_dev[i]);


This function isn't available in this commit. Each commit should build on it's own. Add this support with the KBD driver commit.

You could have the child drivers register an alert callback. Then MFD driver doesn't need to know the type of each child.

If you wanted the alert callbacks to be configured at build time, you could define an iterable section per it8801 instance.

See INPUT_CALL_DEFINE for an example of creating an iterable section for a specific device instance.

Moved to respective driver.

Because the KBD and GPIO devices of IT8801 share the gpio irq. Consider that if IT8801 is connected to the EC, the GPIO interrupt is enabled in the interrupt configuration using irq_connect_dynamic.
Repeated registration of gpio irq will cause Assert problem, so here we only register it once in mfd_ite_it8801.

keith-zephyr · 2024-10-04T15:43:59Z

dts/riscv/ite/it8801-mfd-gpiocr.dtsi

+
+/ {
+	soc {
+		gpio0: gpiocr@0a {


Nodename gpio0 is likely to conflict the the GPIO controller on the SoC.

keith-zephyr · 2024-10-04T15:46:33Z

drivers/input/input_ite_it8801_kbd.c

+#include <zephyr/logging/log.h>
+LOG_MODULE_REGISTER(input_ite_it8801_kbd, CONFIG_INPUT_LOG_LEVEL);
+
+#define IT8801_MFDCTRL_COUNT IT8801_DT_INST_MFDCTRL_LEN(0)


This assumes only one KBD driver installed. This should be calculated on each KBD instance.

Fixed, use multiple instances.

keith-zephyr · 2024-10-04T15:49:42Z

drivers/input/input_ite_it8801_kbd.c

+
+static struct kbd_it8801_data kbd_it8801_data_0;
+
+PM_DEVICE_DT_INST_DEFINE(0, input_kbd_matrix_pm_action);


Drivers should support multiple instances.

Fixed, use multiple instances.

keith-zephyr · 2024-10-04T15:51:10Z

drivers/pwm/pwm_ite_it8801.c

+	static struct pwm_it8801_data pwm_it8801_data_##inst;                                      \
+	DEVICE_DT_INST_DEFINE(inst, &pwm_it8801_init, NULL, &pwm_it8801_data_##inst,               \
+			      &pwm_it8801_cfg_##inst, POST_KERNEL,                                 \
+			      CONFIG_PWM_IT8801_INIT_PRIORITY, &pwm_it8801_api);


Add a BUILD_ASSERT that the init priority is lower (higher number) than the MFD parent.

I suggested in a separate comment that this could all use the same priority, then the sub priority would take care of sequencing the init correctly, I think it's a better approach, less options kicking around. Build asserts should not be needed anymore since the build priorites are checked at build time btw.

Is there anything I can refer to here? I'm encountering a build error when using CONFIG_MFD_INIT_PRIORITY + 1 in Kconfig or here.

keith-zephyr · 2024-10-04T15:53:15Z

drivers/gpio/Kconfig.it8801

+	  IT8801 GPIO port device driver initialization priority. The priority
+	  must be lower than MFD_INIT_PRIORITY device.


Add a BUILD_ASSERT to check the init priority.

fabiobaltieri

Just an quick initial, pass, noticed some comments of mine overlaps with Keith.

Hey is there a board where you could add this? I'm a bit confused by the dtsi file you have in one of the commits, would be nice to have build testing in CI as well.

fabiobaltieri · 2024-10-04T15:49:19Z

drivers/mfd/mfd_ite_it8801.c

+	struct mfd_it8801_data *data = dev->data;
+	int ret;
+
+	if (!device_is_ready(config->i2c_dev.bus)) {


you can use i2c_is_ready_dt

fabiobaltieri · 2024-10-04T15:51:36Z

drivers/mfd/mfd_ite_it8801.c

+		.i2c_dev = I2C_DT_SPEC_INST_GET(inst),                                             \
+		.irq_gpios = GPIO_DT_SPEC_INST_GET(inst, irq_gpios),                               \
+		.sub_mfd_dev = sub_mfd_dev_##inst,                                                 \
+		.sub_mfd_node_num = DT_INST_CHILD_NUM(inst),                                       \


wondering it would not be safer to use ARRAY_SIZE(sub_mfd_dev_##inst), just in case

fabiobaltieri · 2024-10-04T15:52:21Z

drivers/mfd/mfd_ite_it8801.c

+	int i;
+
+	for (i = 0; i < config->sub_mfd_node_num; i++) {
+		if (config->sub_mfd_dev[i] != NULL) {


can this happen? I think it would fail the build if there's no actual device here

Redundant, removed.

fabiobaltieri · 2024-10-04T15:55:36Z

drivers/mfd/Kconfig.it8801

+config MFD_IT8801_ALTCTRL_INIT_PRIORITY
+	int "IT8801 GPIO port init priority"
+	default 81


hey can you try to initialize all the sub devices at CONFIG_MFD_INIT_PRIORITY? the sub-priority should take care of the sequencing, less priority options kicking around... you can check the output with west build -t initlevels

Is there anything I can refer to here? I'm encountering a build error when using CONFIG_MFD_INIT_PRIORITY + 1 here.

fabiobaltieri · 2024-10-04T16:00:13Z

dts/riscv/ite/it8801-mfd-gpiocr.dtsi

+ */
+
+/ {
+	soc {


This does not look quite right, how is this dtsi going to be used? the child nodes here should be child nodes of the mfd device, right?

Yes, this is the child node that mfd will use. Renamed.

fabiobaltieri · 2024-10-04T16:14:05Z

drivers/pwm/pwm_ite_it8801.c

+
+struct pwm_it8801_data {
+	/* I2C device for the MFD parent */
+	const struct i2c_dt_spec *i2c_dev;


ite,it8801-pwm is a child node of the mfd device right? Then can you get this with DEVICE_DT_GET(DT_PARENT(...)) at build time?

That's right, thanks for the tip, we can use this: I2C_DT_SPEC_GET(DT_INST_PARENT(inst))

fabiobaltieri · 2024-10-04T16:15:10Z

drivers/pwm/pwm_ite_it8801.c

+	static struct pwm_it8801_data pwm_it8801_data_##inst;                                      \
+	DEVICE_DT_INST_DEFINE(inst, &pwm_it8801_init, NULL, &pwm_it8801_data_##inst,               \
+			      &pwm_it8801_cfg_##inst, POST_KERNEL,                                 \
+			      CONFIG_PWM_IT8801_INIT_PRIORITY, &pwm_it8801_api);


I suggested in a separate comment that this could all use the same priority, then the sub priority would take care of sequencing the init correctly, I think it's a better approach, less options kicking around. Build asserts should not be needed anymore since the build priorites are checked at build time btw.

The IT8801 is an I/O expander that provides GPIO, PWM, Keyboard functions via the I2C bus. Signed-off-by: Tim Lin <tim2.lin@ite.corp-partner.google.com>

IT8801 support GPIO alternate function switching. Some GPIO pins can be switched as KSO or PWM function. Signed-off-by: Tim Lin <tim2.lin@ite.corp-partner.google.com>

Add I2C-based keyboard matrix scan device driver. IT8801 support 8 KSI pins and 19 KSO pins [22:11] [6:0]. Signed-off-by: Tim Lin <tim2.lin@ite.corp-partner.google.com>

Add I2C-based PWM device driver. Supports 7 open-drain/push-pull outputs. Signed-off-by: Tim Lin <tim2.lin@ite.corp-partner.google.com>

Add I2C-based GPIO device driver. Supports 16-port GPIO divided into 3 groups. Signed-off-by: Tim Lin <tim2.lin@ite.corp-partner.google.com>

GTLin08 · 2024-10-07T11:23:35Z

Just an quick initial, pass, noticed some comments of mine overlaps with Keith.

Hey is there a board where you could add this? I'm a bit confused by the dtsi file you have in one of the commits, would be nice to have build testing in CI as well.

I have created a test program to connect with it8xxx2_evb as shown in the link

GTLin08 · 2024-10-07T11:28:31Z

Also consider running clang-format on all the files.

This is very strange. When using ./scripts/ci/check_compliance.py locally, there is a prompt such as:

You may want to run clang-format on this change

drivers/mfd/mfd_ite_it8801.c:133 
-		DT_INST_FOREACH_CHILD_STATUS_OKAY_SEP(inst, DEVICE_DT_GET, (,))};                  \
+		DT_INST_FOREACH_CHILD_STATUS_OKAY_SEP(inst, DEVICE_DT_GET, (, ))};                 \

But after I upload the program, I get the following error:

SPACING: space prohibited before that close parenthesis ')'
File:drivers/mfd/mfd_ite_it8801.c
Line:132

zephyrbot added area: Input Input Subsystem and Drivers area: MFD area: PWM Pulse Width Modulation area: GPIO area: RISCV RISCV Architecture (32-bit & 64-bit) platform: ITE ITE labels Oct 4, 2024

zephyrbot requested review from aasinclair, anangl, brockus-zephyr, Dino-Li, fabiobaltieri, fkokosinski, gmarull, henrikbrixandersen, jackrosenthal, keith-zephyr, kgugala, mgielda, mnkp, sjg20 and tgorochowik October 4, 2024 10:20

zephyrbot assigned fabiobaltieri Oct 4, 2024

GTLin08 force-pushed the it8xxx2_mfd_driver branch 2 times, most recently from eaac9c2 to 372ce90 Compare October 4, 2024 13:55

keith-zephyr requested changes Oct 4, 2024

View reviewed changes

fabiobaltieri reviewed Oct 4, 2024

View reviewed changes

GTLin08 force-pushed the it8xxx2_mfd_driver branch from 372ce90 to 2d60741 Compare October 7, 2024 10:59

GTLin08 added 3 commits October 7, 2024 19:05

drivers: mfd: it8801: Initialize IT8801 multi-function device drivers

2617f0a

The IT8801 is an I/O expander that provides GPIO, PWM, Keyboard functions via the I2C bus. Signed-off-by: Tim Lin <tim2.lin@ite.corp-partner.google.com>

drivers: mfd: it8801_altctrl: Add alternate controller for MFD

06cb46e

IT8801 support GPIO alternate function switching. Some GPIO pins can be switched as KSO or PWM function. Signed-off-by: Tim Lin <tim2.lin@ite.corp-partner.google.com>

drivers: input: it8801: Add I2C-based keyboard matrix scan controller

c2af18f

Add I2C-based keyboard matrix scan device driver. IT8801 support 8 KSI pins and 19 KSO pins [22:11] [6:0]. Signed-off-by: Tim Lin <tim2.lin@ite.corp-partner.google.com>

GTLin08 added 2 commits October 7, 2024 19:06

drivers: pwm: it8801: Add I2C-based PWM device driver

b24c7a7

Add I2C-based PWM device driver. Supports 7 open-drain/push-pull outputs. Signed-off-by: Tim Lin <tim2.lin@ite.corp-partner.google.com>

drivers: gpio: it8801: Add I2C-based GPIO device driver

71000b0

Add I2C-based GPIO device driver. Supports 16-port GPIO divided into 3 groups. Signed-off-by: Tim Lin <tim2.lin@ite.corp-partner.google.com>

GTLin08 force-pushed the it8xxx2_mfd_driver branch from 2d60741 to 71000b0 Compare October 7, 2024 11:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

drivers/mfd: it8801: Implement ITE IT8801 mfd drivers #79416

drivers/mfd: it8801: Implement ITE IT8801 mfd drivers #79416

GTLin08 commented Oct 4, 2024

keith-zephyr left a comment

keith-zephyr Oct 4, 2024

GTLin08 Oct 7, 2024

keith-zephyr Oct 4, 2024

GTLin08 Oct 7, 2024

keith-zephyr Oct 4, 2024

GTLin08 Oct 7, 2024

keith-zephyr Oct 4, 2024

GTLin08 Oct 7, 2024

keith-zephyr Oct 4, 2024

GTLin08 Oct 7, 2024

keith-zephyr Oct 4, 2024

fabiobaltieri Oct 4, 2024

GTLin08 Oct 7, 2024

keith-zephyr Oct 4, 2024

fabiobaltieri left a comment

fabiobaltieri Oct 4, 2024

GTLin08 Oct 7, 2024

fabiobaltieri Oct 4, 2024

GTLin08 Oct 7, 2024

fabiobaltieri Oct 4, 2024

GTLin08 Oct 7, 2024

fabiobaltieri Oct 4, 2024

GTLin08 Oct 7, 2024

fabiobaltieri Oct 4, 2024

GTLin08 Oct 7, 2024

fabiobaltieri Oct 4, 2024

GTLin08 Oct 7, 2024

fabiobaltieri Oct 4, 2024

GTLin08 commented Oct 7, 2024

GTLin08 commented Oct 7, 2024


		static struct kbd_it8801_data kbd_it8801_data_0;

		PM_DEVICE_DT_INST_DEFINE(0, input_kbd_matrix_pm_action);

		IT8801 GPIO port device driver initialization priority. The priority
		must be lower than MFD_INIT_PRIORITY device.

+               */
+              / {
+              	soc {

drivers/mfd: it8801: Implement ITE IT8801 mfd drivers #79416

Are you sure you want to change the base?

drivers/mfd: it8801: Implement ITE IT8801 mfd drivers #79416

Conversation

GTLin08 commented Oct 4, 2024

keith-zephyr 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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabiobaltieri 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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GTLin08 commented Oct 7, 2024

GTLin08 commented Oct 7, 2024