Add support for MAX16150/MAX16169 #2672

mrcsosa · 2024-12-10T08:46:13Z

PR Description

Please replace this comment with a summary of your changes, and add any context
necessary to understand them. List any dependencies required for this change.
To check the checkboxes below, insert a 'x' between square brackets (without
any space), or simply check them after publishing the PR.
If you changes include a breaking change, please specify dependent PRs in the
description and try to push all related PRs simultaneously.

PR Type

Bug fix (a change that fixes an issue)
New feature (a change that adds new functionality)
Breaking change (a change that affects other repos or cause CIs to fail)

PR Checklist

I have conducted a self-review of my own code changes
I have tested the changes on the relevant hardware
I have updated the documentation outside this repo accordingly (if there is the case)

nunojsa

I'm stopping the review for now. Note that you pretty much copied the driver from one subsystem to another without any integration with the input subsystem. Not what was meant.

Please do not rush into opening PRs or having things done. Really try to understand what was asked and seek help if needed 😉

nunojsa · 2024-12-12T07:23:13Z

Documentation/devicetree/bindings/input/adi,max16150.yaml

+            brcm,function = <0>;
+            brcm,pull = <0>;
+        };
+    };


This looks to be very rpi dependent... Typically not needed in bindings AFAIK. But you're definetly missing input properties. Where do you define the key code?

I think the key code is typically used to detect button presses. However, with MAX16150, it sends interrupt signals when certain amount of time the button is pressed.

Same comment. The above is not needed in the example....

I think the key code is typically used to detect button presses. However, with MAX16150, it sends interrupt signals when certain amount of time the button is pressed.

And button presses are also signaled (most of the times) via interrupts. Typically one IRQ for press and one for release. Is this not the same with this button?

Documentation/devicetree/bindings/input/adi,max16150.yaml

nunojsa · 2024-12-12T07:24:23Z

Documentation/devicetree/bindings/input/adi,max16150.yaml

+        adi,variant = "A";
+
+        gpio-pb_in = <&gpio 17 1>;
+        gpio-out = <&gpio 18 0>;


I'm also not convinced we need the above...

Documentation/devicetree/bindings/input/adi,max16150.yaml

nunojsa · 2024-12-12T07:26:03Z

drivers/input/misc/max16150.c

+
+	hrtimer_cancel(&data->debounce_timer);
+	hrtimer_cancel(&data->shutdown_timer);
+	return 0;


This can be done in a devm_action... Also not sure if we really need hrtimers for this

drivers/input/misc/max16150.c

nunojsa · 2024-12-12T07:27:19Z

drivers/input/misc/max16150.c

+	} else {
+		dev_err(&pdev->dev, "Unknown device type: %s\n", type);
+		return -EINVAL;
+	}


use proper chip info structure for subtleties in the chips... The above does not scale at al.

nunojsa · 2024-12-12T07:28:17Z

drivers/input/misc/max16150.c

+		return -EINVAL;
+	}
+
+	ret = device_property_read_string(&pdev->dev, "adi,variant", &variant);


Not in the bindings and I dunno you need a property for this... You already have compatibles and you can different info structure per compatible.

kister-jimenez

You might want to check out existing input device drivers for reference.

kister-jimenez · 2024-12-13T14:56:12Z

drivers/input/misc/max16150.c

+
+	if (data->variant == MAX161X_A && data->out_asserted) {
+		dev_info(data->dev, "Shutdown time exceeded. Deasserting OUT.");
+		gpiod_set_value(data->gpio_clr, 0);


You should use the input subsystem like input_report_key and input_sync to report the key and the value to the input subsystem and to sync input events.

kister-jimenez · 2024-12-13T15:02:02Z

drivers/input/misc/max16150.c

+	case 'A':
+		data->variant = MAX161X_A;
+		data->debounce_time = ktime_set(0, 50 * NSEC_PER_MSEC);
+		data->shutdown_time = (data->type == MAX16150) ? ktime_set(8,
+				      0) : ktime_set(16, 0);
+		break;
+	case 'B':
+		data->variant = MAX161X_B;
+		data->debounce_time = (data->type == MAX16150) ? ktime_set(2,
+				      0) : ktime_set(50, 0);
+		data->shutdown_time = ktime_set(16, 0);
+		break;
+	case 'C':
+		data->variant = MAX161X_C;
+		data->debounce_time = ktime_set(0, 50 * NSEC_PER_MSEC);
+		data->shutdown_time = ktime_set(16, 0);
+		break;


Not sure why you need the debounce time and shutdown time.
Debounce time is for the push button input which should be connected to a push button not the processor system.

Shutdown period is how long the button is pressed before the chip deasserts (in some variant) and pull down the interrupt pin. This is also related to the button side.

kister-jimenez · 2024-12-13T15:03:32Z

drivers/input/misc/max16150.c

+	dev_info(&pdev->dev, "Detected %s variant %s\n",
+		 (data->type == MAX16150) ? "MAX16150" : "MAX16169", variant);
+
+	data->gpio_pb_in = devm_gpiod_get(&pdev->dev, "pb_in", GPIOD_IN);


pb_in is not an input to the MPU and is not connected to the MPU. This is connected to the push button.

kister-jimenez · 2024-12-13T15:05:45Z

drivers/input/misc/max16150.c

+	if (IS_ERR(data->gpio_pb_in))
+		return PTR_ERR(data->gpio_pb_in);
+
+	data->gpio_out = devm_gpiod_get(&pdev->dev, "out", GPIOD_OUT_LOW);


Out pin of the device is typically used to control a switch or to gate the power to the device. You don't need this.

kister-jimenez · 2024-12-13T15:14:18Z

drivers/input/misc/max16150.c

+	if (IS_ERR(data->gpio_clr))
+		return PTR_ERR(data->gpio_clr);
+
+	data->gpio_int = devm_gpiod_get(&pdev->dev, "int", GPIOD_IN);


Just use an irq.

kister-jimenez · 2024-12-13T15:15:38Z

drivers/input/misc/max16150.c

+enum max161x_variant {
+	MAX161X_A,
+	MAX161X_B,
+	MAX161X_C,
+};
+
+enum max161x_type {
+	MAX16150,
+	MAX16169,
+};


Why don't you just combine these? I think the variant is part of the ordering options for customers to select different timing options?

kister-jimenez · 2024-12-13T15:23:29Z

drivers/input/misc/max16150.c

+	if (gpiod_get_value(data->gpio_pb_in)) {
+		/* Button pressed */
+		hrtimer_start(&data->debounce_timer, data->debounce_time,
+			      HRTIMER_MODE_REL);
+	} else {
+		/* Button released */
+		if (data->variant == MAX161X_A && data->out_asserted) {
+			dev_info(data->dev, "Cancelling shutdown timer.");
+			hrtimer_cancel(&data->shutdown_timer);
+		}
+	}


You should use the input subsystem like input_report_key and input_sync to report the key and the value to the input subsystem and to sync input events.

I think what you want is to know the interval between the last irq if you want to distinguish the long press and the short momentary press.

kister-jimenez · 2024-12-13T15:37:23Z

drivers/input/misc/max16150.c

+	hrtimer_init(&data->shutdown_timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
+	data->shutdown_timer.function = max161x_shutdown_timer_cb;
+
+	ret = devm_request_irq(&pdev->dev, gpiod_to_irq(data->gpio_pb_in),


The irq pin is the int pin not the pb_in pin.

mrcsosa · 2025-01-13T02:41:02Z

Changelogs V2:

Converted few functions to use input subsystem
Removed unused gpio pins, leaving int pin (to detect 32ms and 128ms interrupts) and clr pin to reset the output for Variant A devices.
Removed variants on the driver code and dts

kister-jimenez · 2025-01-13T07:02:18Z

drivers/input/misc/max16150.c

+#define GPIO_INT 17
+#define GPIO_CLR 27


Get this from the device tree overlay.

drivers/input/misc/max16150.c

mrcsosa · 2025-02-06T06:35:43Z

Changelogs:

max16150.c

Replaced jiffies to ktime for the duration checking.
Removed the extra key event function along with the 128ms interrupt duration checking.

adi,max16150.yaml

Fixed the description for the linux,code and interrupts.
Added include bindings irq.h and linux-event-codes.h

Kconfig

Changed the configuration and now uses tristate and changed the help text using other references in the input subsystem documentation.

nunojsa

Please do not rush into sending new version. Try to address all the comments and ask to me or a colleague if you fail to understand something. It will make both our lives easier as you won't need to spin this many revisions

nunojsa · 2025-02-07T07:18:19Z

Documentation/devicetree/bindings/input/adi,max16150.yaml

+      Specifies a single numeric keycode value to be used for reporting
+      button/switch events. Specify KEY_RESERVED (0) to opt out of event
+      reporting.
+    $ref: /schemas/types.yaml#/definitions/uint32


no... please look at other examples using this. Hint: git grep "linux,code" Documentation/devicetree.

The property is already defined so no need to do it again. You do need $ref the input bindings

nunojsa · 2025-02-07T07:19:17Z

Documentation/devicetree/bindings/input/adi,max16150.yaml

+
+required:
+  - compatible
+  - linux,code


The code is not required as we the default is KEY_POWER. OTOH the interrupt is indeed a required property

nunojsa · 2025-02-07T07:20:28Z

drivers/input/misc/max16150.c

+	cancel_delayed_work_sync(&button->debounce_work);
+	input_unregister_device(button->input);
+	return 0;
+}


Please do not just ignore comments... this .remove() callback is still here

nunojsa · 2025-02-07T07:21:03Z

drivers/input/misc/max16150.c

+
+struct max16150_data {
+	struct input_dev *bttn;
+	int custom_key_event;


do we need the above?

drivers/input/misc/max16150.c

nunojsa · 2025-02-07T07:29:31Z

drivers/input/misc/max16150.c

+
+	input_report_key(bttn, data->custom_key_event, 1);
+	input_sync(bttn);
+


Where is the support for the required gpio on the chip that do not automatically de-asserts the output?

Can the CLR GPIO just use the gpio-poweroff? So that whenever a power off command or event is triggered, the CLR pin is pulled low.

for an example, I tried using a separate script where a gpio is set to normally high, the same script captures the events send by the driver code, when long press is detected, it will toggle the gpio pin to low and deassert the clr pin

drivers/input/misc/max16150.c

nunojsa · 2025-02-07T07:33:01Z

drivers/input/misc/max16150.c

+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/jiffies.h>
+#include <linux/of.h>


you need mod_devicetable.h for the of_device_id table

mrcsosa · 2025-02-19T03:05:54Z

Changelogs V4
Enhancements & Feature Additions

Short and Long Press Detection

Added detection logic for short and long button presses in max16150_fall_irq.
Defined press duration thresholds:
- SHORT_PRESS_MIN_MS = 32ms
- SHORT_PRESS_MAX_MS = 127ms
- LONG_PRESS_MIN_MS = 128ms
Short press logs "Short press detected" and sends MSC_SCAN, 1.
Long press logs "Long press detected" and sends MSC_SCAN, 2.

Input Subsystem Enhancements

Added support for EV_MSC event with MSC_SCAN in max16150_probe.
Now capable of distinguishing between short and long press events at the input level.

Bug Fixes & Optimizations

IRQ Duration Calculation Moved

Duration calculation for IRQ moved from max16150_rise_irq to max16150_fall_irq, ensuring accurate measurement of button press time.

Removed Redundant Logging

Removed dev_info(dev, "Button press detected") from max16150_rise_irq, as press event logging is now handled in max16150_fall_irq.

General Code Cleanup

Removed unnecessary dev_dbg_ratelimited logging for rising IRQ.
Removed the remove function.

I still think that the gpio clr can be declared outside the driver.

nunojsa · 2025-02-19T11:29:08Z

Documentation/devicetree/bindings/input/adi,max16150.yaml

+properties:
+  compatible:
+    description: |
+      Specifies the supported device variants. The MAX16150 and MAX16169 are supported.


Don't think the text needs to be formatted... This means you can drop '|'

nunojsa · 2025-02-19T11:29:58Z

Documentation/devicetree/bindings/input/adi,max16150.yaml

+  interrupts:
+    maxItems: 1
+
+  linux,code: true


you need to $ref the proper input.yaml file to get this property

nunojsa · 2025-02-19T11:31:39Z

drivers/input/misc/max16150.c

+#include <linux/platform_device.h>
+#include <linux/ktime.h>
+#include <linux/of.h>
+#include <linux/mod_devicetable.h>


not in alphabetical order and you still have of.h

nunojsa · 2025-02-19T11:31:54Z

drivers/input/misc/max16150.c

+
+#define SHORT_PRESS_MIN_MS 32
+#define SHORT_PRESS_MAX_MS 127
+#define LONG_PRESS_MIN_MS 128


typically one uses the driver name as prefix... Example: MAX16150_SHORT_PRESS_MIN_MS

nunojsa · 2025-02-19T11:32:28Z

drivers/input/misc/max16150.c

+		return err;
+	}
+
+	platform_set_drvdata(pdev, data);


not needed...

nunojsa · 2025-02-19T11:33:54Z

drivers/input/misc/max16150.c

+
+	data->bttn = bttn;
+	if (of_property_read_u32(np, "key-event", &data->key_event))
+		data->key_event = KEY_POWER;


Please use device property API. Fairly sure I already asked for it...

nunojsa · 2025-02-19T11:37:30Z

Documentation/devicetree/bindings/input/adi,max16150.yaml

+    power-button {
+        compatible = "adi,max16150";
+        interrupt-parent = <&gpio>;
+        interrupts = <17 IRQ_TYPE_EDGE_BOTH>;


I doubt the driver works if DT is like this... Did you tested it?

nunojsa · 2025-02-19T11:41:20Z

drivers/input/misc/max16150.c

+	err = devm_request_threaded_irq(&pdev->dev, fall_irq, NULL,
+					max16150_fall_irq,
+					IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
+					"max16150_falling", data);


Thinking about this? Did you actually tested this? How did you described this in your DT? You would need to connect the interrupt signal into two different inputs in the processor. So this hack could be useful to know if you are in a rising or falling edge but it's not coherent with the bindings.

nunojsa · 2025-02-19T11:43:43Z

drivers/input/misc/max16150.c

+	bttn->phys = "max16150/input0";
+	bttn->id.bustype = BUS_HOST;
+	input_set_capability(bttn, EV_KEY, data->key_event);
+	input_set_capability(bttn, EV_MSC, MSC_SCAN);


Why do you need the above?

nunojsa · 2025-02-19T11:52:53Z

drivers/input/misc/max16150.c

+
+	data->irq_falling_time = ktime_get();
+	irq_duration = ktime_sub(data->irq_falling_time, data->irq_rising_time);
+	duration_ms = ktime_to_ms(irq_duration);


Well, can you actually ensure that the rising edge already happened? I think you can still have a race during probe. So you could have requested the interrupt while your signal is already high and at that point, I do not think your max16150_rise_irq will trigger. Hence data->irq_rising_time will be 0 and you could wrongly detect a long press.

I think you could simple check data->irq_rising_time for 0 and ignore this interrupt in that case as there's no way to properly get the press duration.

But this hack requesting two interrupts is very questionable... I'll ask this again: Did you tried my suggestion with this API?

https://elixir.bootlin.com/linux/v6.6.65/source/include/linux/interrupt.h#L504

If it works, you could properly do:

err = devm_request_threaded_irq(&pdev->dev, irq, NULL, max16150_irq, IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING | IRQF_ONESHOT, "max16150_irq", data);

mrcsosa · 2025-02-27T13:24:33Z

V5:

Used dev_err_probe();
Added chip_info
removed interrupt and used gpio to track rising and falling edge better
added gpio for clr pin
added compatible variants A, B, and C
removed time prints

Documentation/devicetree/bindings/input/adi,max16150.yaml

drivers/input/misc/max16150.c

Add documentation for device tree bindings for MAX16150/MAX16169 Signed-off-by: Marc Paolo Sosa <[email protected]>

MAX16150/MAX16169 nanoPower Pushbutton On/Off Controller Signed-off-by: Marc Paolo Sosa <[email protected]>

Add entry for the MAX16150/MAX16169 driver. Signed-off-by: Marc Paolo Sosa <[email protected]>

mrcsosa · 2025-02-28T09:05:34Z

Changelogs V6:

Inverted the check value function for better coding indentation
Removed variant c as it is the same with variant b
Removed unnecessary prints and debugs
Removed of.h
Changed interrupt -> gpio-interrupt in yaml
Added definiton of gpio-interrupt
Fixed minor formatting in yaml

nunojsa

Alright, I would say to take my last inputs and send this upstream

nunojsa · 2025-02-28T16:10:39Z

Documentation/devicetree/bindings/input/adi,max16150.yaml

+  interrupt-gpio:
+    maxItems: 1
+
+  clr-gpio:


you should make this mandatory for the compatible it makes sense. See examples here on how to do conditionals on bindings:

https://elixir.bootlin.com/linux/v6.13.4/source/Documentation/devicetree/bindings/iio/imu/adi,adis16475.yaml#L94

nunojsa · 2025-02-28T16:11:14Z

Documentation/devicetree/bindings/input/adi,max16150.yaml

+      - adi,max16169a
+      - adi,max16169b
+
+  interrupt-gpio:


should be interrupt-gpios

nunojsa · 2025-02-28T16:11:32Z

Documentation/devicetree/bindings/input/adi,max16150.yaml

+  interrupt-gpio:
+    maxItems: 1
+
+  clr-gpio:


Also it should be clr-gpios

nunojsa · 2025-02-28T16:24:56Z

drivers/input/misc/max16150.c

+				 "clr GPIO undefined, clr pin will not be asserted\n");
+		} else {
+			gpiod_set_value(max16150->clr_gpiod, 0);
+		}


We are using the clr gpio to reset the device, this is what we have implemented. I think the gpio should be high on boot since it will only go low if we want to reset or deassert the output.

Ahh, I get it know... But now that I thin about this, we should do this in another way because as we have it know, we will never use this pin for chips where has_clr_gpio = false. Sure, for those devices, it's not a mandatory pin but we should still be able to use it if we want too right? So, I would suggest something like:

max16150->clr_gpiod = devm_gpiod_get_optional(dev, "clr", GPIOD_OUT_HIGH); if (IS_ERR(max16150->clr_gpiod)) return dev_err_probe(dev, PTR_ERR(max16150->clr_gpiod), "Failed to get clr GPIO\n"); if (!max16150->clr_gpiod && chip_info->needs_clr_gpio) return dev_err_probe(dev, ...); //clr gpio is mandatory for latching out the output if (max16150->clr_gpiod) { ... }

Makes sense?

nunojsa · 2025-02-28T16:26:53Z

drivers/input/misc/max16150.c

+		if (max16150->duration > MAX16150_LONG_INTERRUPT) {
+			if (max16150->chip_info->has_clr_gpio &&
+			    max16150->clr_gpiod)
+				gpiod_set_value(max16150->clr_gpiod, 1);


remove the if() and just do gpiod_set_value(max16150->clr_gpiod, 1);. The gpio subsystem is expected to handle the case where the gpiod is NULL (i.e do nothing)

mrcsosa requested review from nunojsa, mhennerich, dbogdan, btogorean, chegbeli, buha, amiclaus, rbolboac, danmois, CiprianRegus and machschmitt as code owners December 10, 2024 08:46

mrcsosa force-pushed the dev/max16150 branch 3 times, most recently from fc933bb to 55c3ff9 Compare December 10, 2024 12:44

nunojsa reviewed Dec 12, 2024

View reviewed changes

mrcsosa marked this pull request as draft December 12, 2024 08:06

kister-jimenez reviewed Dec 13, 2024

View reviewed changes

mrcsosa marked this pull request as ready for review December 17, 2024 00:00

mrcsosa marked this pull request as draft January 13, 2025 00:35

mrcsosa force-pushed the dev/max16150 branch 4 times, most recently from 7aebbf1 to e96094d Compare January 13, 2025 01:26

mrcsosa marked this pull request as ready for review January 13, 2025 02:49

mrcsosa marked this pull request as draft January 13, 2025 06:45

mrcsosa force-pushed the dev/max16150 branch from e96094d to 8734318 Compare January 13, 2025 06:46

mrcsosa marked this pull request as ready for review January 13, 2025 08:27

kister-jimenez reviewed Jan 13, 2025

View reviewed changes

mrcsosa marked this pull request as draft January 14, 2025 10:09

mrcsosa force-pushed the dev/max16150 branch 4 times, most recently from 2befc7c to a763cfc Compare February 6, 2025 05:42

mrcsosa marked this pull request as ready for review February 6, 2025 08:44

nunojsa reviewed Feb 7, 2025

View reviewed changes

mrcsosa marked this pull request as draft February 11, 2025 06:08

mrcsosa force-pushed the dev/max16150 branch 7 times, most recently from 024c7d7 to adcc142 Compare February 19, 2025 02:29

mrcsosa marked this pull request as ready for review February 19, 2025 03:05

nunojsa reviewed Feb 19, 2025

View reviewed changes

mrcsosa marked this pull request as draft February 27, 2025 11:23

mrcsosa force-pushed the dev/max16150 branch from adcc142 to 2e8b4a3 Compare February 27, 2025 13:13

mrcsosa marked this pull request as ready for review February 27, 2025 13:14

mrcsosa force-pushed the dev/max16150 branch from 2e8b4a3 to e0a2997 Compare February 27, 2025 13:21

nunojsa reviewed Feb 27, 2025

View reviewed changes

mrcsosa added 3 commits February 28, 2025 16:31

dt-bindings: input: add adi,max16150.yaml

9803aa4

Add documentation for device tree bindings for MAX16150/MAX16169 Signed-off-by: Marc Paolo Sosa <[email protected]>

input: misc: add driver for max16150

fda3963

MAX16150/MAX16169 nanoPower Pushbutton On/Off Controller Signed-off-by: Marc Paolo Sosa <[email protected]>

Kconfig.adi imply MAX16150

4959b0b

Add entry for the MAX16150/MAX16169 driver. Signed-off-by: Marc Paolo Sosa <[email protected]>

mrcsosa force-pushed the dev/max16150 branch from e0a2997 to 4959b0b Compare February 28, 2025 08:32

nunojsa reviewed Feb 28, 2025

View reviewed changes


		input_report_key(bttn, data->custom_key_event, 1);
		input_sync(bttn);

		#define GPIO_INT 17
		#define GPIO_CLR 27

Add support for MAX16150/MAX16169 #2672

Are you sure you want to change the base?

Add support for MAX16150/MAX16169 #2672

Conversation

mrcsosa commented Dec 10, 2024

PR Description

PR Type

PR Checklist

nunojsa 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

kister-jimenez 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

mrcsosa commented Jan 13, 2025 • edited Loading

Choose a reason for hiding this comment

mrcsosa commented Feb 6, 2025

Changelogs:

max16150.c

adi,max16150.yaml

Kconfig

nunojsa 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

mrcsosa commented Feb 19, 2025 • edited Loading

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

mrcsosa commented Feb 27, 2025

mrcsosa commented Feb 28, 2025

nunojsa 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

mrcsosa commented Jan 13, 2025 •

edited

Loading

mrcsosa commented Feb 19, 2025 •

edited

Loading