Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for MAX16150/MAX16169 #2672

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 56 additions & 0 deletions Documentation/devicetree/bindings/input/adi,max16150.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
%YAML 1.2
---
$id: http://devicetree.org/schemas/input/adi,max16150.yaml#
$schema: http://devicetree.org/meta-schemas/core.yaml#

title: Analog Devices MAX16150/MAX16169 nanoPower Pushbutton On/Off Controller

maintainers:
- Marc Paolo Sosa <[email protected]>

description: |
The MAX16150/MAX16169 is an extremely low-power, pushbutton, on/off controller
with a switch debouncer and built-in latch. This device accepts a noisy input
from a mechanical switch and produces a clean, latched output, as well as a
one-shot interrupt output, in response to a switch closure exceeding the
debounce period at PB_IN. A switch closure longer than the shutdown period at
PB_IN results in a longer one-shot interrupt output. The MAX16150 family has
two sets of devices, one in which a longer switch closure greater than the
shutdown period deasserts the latched output, and another in which the latched
output stays asserted.

Specifications about the controller can be found at:
https://www.analog.com/en/products/max16150.html
https://www.analog.com/en/products/max16169.html

properties:
compatible:
enum:
- adi,max16150
- adi,max16169

required:
- compatible
- gpios
- interrupt-parent
- interrupts
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You have gpios, interrupt-parent and interrupts as required but you don't specify them at all... Please look at other bindings for examples. I'm also not convinced we need gpios (maybe one is needed but two still not sure)


additionalProperties: false

examples:
- |
gpio {
max16150_pins: max16150_pins {
pins = <17>;
function = <0>;
};
};
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?


power-button {
compatible = "adi,max16150";
gpios = <&gpio 17 0>;
interrupt-parent = <&gpio>;
interrupts = <17 2>;
status = "okay";
};
1 change: 1 addition & 0 deletions drivers/input/Kconfig.adi
Original file line number Diff line number Diff line change
Expand Up @@ -15,3 +15,4 @@ config INPUT_ALL_ADI_DRIVERS
imply INPUT_AD714X
imply INPUT_AD714X_I2C
imply INPUT_AD714X_SPI
imply INPUT_MAX16150_PWRBUTTON
9 changes: 9 additions & 0 deletions drivers/input/misc/Kconfig
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,15 @@ config INPUT_E3X0_BUTTON
To compile this driver as a module, choose M here: the
module will be called e3x0_button.

config INPUT_MAX16150_PWRBUTTON
bool "MAX16150/MAX16169 Pushbutton driver"
depends on OF_GPIO
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a very odd dependency. Most likely this is not what you want

help
This driver supports the MAX16150 and MAX16169 pushbutton
controllers, which are low-power devices with a switch
debouncer and built-in latch. Device bindings are specified
in the device tree.

config INPUT_PCSPKR
tristate "PC Speaker support"
depends on PCSPKR_PLATFORM
Expand Down
1 change: 1 addition & 0 deletions drivers/input/misc/Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ obj-$(CONFIG_INPUT_IQS7222) += iqs7222.o
obj-$(CONFIG_INPUT_KEYSPAN_REMOTE) += keyspan_remote.o
obj-$(CONFIG_INPUT_KXTJ9) += kxtj9.o
obj-$(CONFIG_INPUT_M68K_BEEP) += m68kspkr.o
obj-$(CONFIG_INPUT_MAX16150_PWRBUTTON) += max16150.o
obj-$(CONFIG_INPUT_MAX77650_ONKEY) += max77650-onkey.o
obj-$(CONFIG_INPUT_MAX77693_HAPTIC) += max77693-haptic.o
obj-$(CONFIG_INPUT_MAX8925_ONKEY) += max8925_onkey.o
Expand Down
119 changes: 119 additions & 0 deletions drivers/input/misc/max16150.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,119 @@
// SPDX-License-Identifier: GPL-2.0-or-later
/*
* Analog Devices MAX16150/MAX16169 Pushbutton Driver
*
* Copyright 2024 Analog Devices Inc.
*/

#include <linux/module.h>
#include <linux/gpio.h>
#include <linux/interrupt.h>
#include <linux/input.h>
#include <linux/platform_device.h>
#include <linux/of_gpio.h>
#include <linux/of_device.h>

static int irq_number;
static ktime_t prev_time;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove the above lines. You need your own structure per device. If you have multiple max16150 devices, things will fail badly since the above variables will be shared between them

static const ktime_t short_pulse = 32 * NSEC_PER_MSEC;
static const ktime_t long_pulse = 128 * NSEC_PER_MSEC;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The const here does not offer you much


static irqreturn_t max16150_isr(int irq, void *dev_id)
{
ktime_t now = ktime_get();
ktime_t duration;
struct input_dev *button = dev_id;
static bool falling_edge = false;

if (gpio_get_value(irq) == 0) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The above is not guaranteed. You can't know if your IRQ controller is a gpiochip and even if it is there's no guarantee it can retrieve the line state. So, I think we need to assume the initial state of the INT line is deasserted so the first time we get in the handler is when INT is asserting. This opens the door to some subtle races (because we can get the initial state of the INT line wrong) but I'm not sure if there's any other reliable way for doing this. Restricting this to only work with GPIO backed IRQ controllers seems wrong (and again, even in that case the above is not guaranteed to work)

Just saw this now:

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

Maybe give it a try and see what do you get for IRQCHIP_STATE_LINE_LEVEL

prev_time = now;
falling_edge = true;
} else if (falling_edge) {
duration = ktime_sub(now, prev_time);
if (duration >= long_pulse)
input_report_key(button, KEY_POWER, 1);
else
input_report_key(button, KEY_WAKEUP, 1);

input_sync(button);
falling_edge = false;
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Things are better (I was reviewing the older version when you sent this one) but still look wrong to me. We just have one event: KEY_POWER. IIUC, if the INT pulse is short then we have:

/* OUT is asserted so the value for KEY_POWER should 1 */
input_report_key(button, KEY_POWER, 1);`

If we have a long INT pulse, then

/* OUT is deasserted so the value for KEY_POWER should 0 */
input_report_key(button, KEY_POWER, 0);
/* For the chips where OUT is not automatically de-asserted, we should make this gpio mandatory */
gpio_value_set(gpiod_clr, 1);

AFAICT, we only have one even and depending on the duration of the INT pulse we either value 1 or 0. Also, while KEY_POWER is likely the default event, I guess there's nothing mandating that. Consider doing something like:

https://elixir.bootlin.com/linux/v6.12.6/source/drivers/input/misc/pm8941-pwrkey.c#L321

But use device properties and not OF


return IRQ_HANDLED;
}

static int get_irq(struct platform_device *pdev)
{
return platform_get_irq(pdev, 0);
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for a function for this. Call platform_get_irq() directly


static int max16150_probe(struct platform_device *pdev)
{
struct input_dev *button;
int ret;

button = devm_input_allocate_device(&pdev->dev);
if (!button) {
dev_err(&pdev->dev, "Can't allocate input device\n");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dev_err_probe()

return -ENOMEM;
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


button->name = "max16150";
button->phys = "max16150/input0";
button->id.bustype = BUS_HOST;
input_set_capability(button, EV_KEY, KEY_POWER);
input_set_capability(button, EV_KEY, KEY_WAKEUP);

ret = input_register_device(button);
if (ret) {
dev_err(&pdev->dev, "Can't register input device: %d\n", ret);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

return ret;
}

irq_number = get_irq(pdev);
if (irq_number < 0) {
dev_err(&pdev->dev, "Can't get IRQ\n");
return irq_number;
}

ret = devm_request_irq(&pdev->dev, irq_number, max16150_isr,
IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING, "max16150_irq", button);
if (ret)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The line seems long. Typically the flags should be given in DT and we should not overwrite them here. But in this particular case, we do need these specific flags so not sure.

return ret;

prev_time = ktime_get();
platform_set_drvdata(pdev, button);
device_init_wakeup(&pdev->dev, true);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


return 0;
}

static int max16150_remove(struct platform_device *pdev)
{
struct input_dev *button = platform_get_drvdata(pdev);

input_unregister_device(button);
return 0;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

static const struct of_device_id max16150_of_match[] = {
{ .compatible = "adi,max16150" },
{ .compatible = "adi,max16169" },
{ }
};
MODULE_DEVICE_TABLE(of, max16150_of_match);

static struct platform_driver max16150_driver = {
.probe = max16150_probe,
.remove = max16150_remove,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove is not needed since you're using devm_input_allocate_device()

.driver = {
.name = "max16150",
.of_match_table = max16150_of_match,
},
};

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: no need for new line in these cases

module_platform_driver(max16150_driver);

MODULE_AUTHOR("Marc Paolo Sosa <[email protected]>");
MODULE_DESCRIPTION("MAX16150/MAX16169 Pushbutton Driver");
MODULE_LICENSE("GPL");
Loading