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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
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
53 changes: 53 additions & 0 deletions Documentation/devicetree/bindings/input/adi,max16150.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
# 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 a low-power pushbutton on/off controller with a switch
debouncer and built-in latch. It accepts a noisy input from a mechanical switch
and produces a clean latched output, as well as a one-shot interrupt output.

properties:
compatible:
description: |
Specifies the supported device variants. The MAX16150 and MAX16169 are supported.
enum:
- adi,max16150
- adi,max16169

interrupts:
description: Interrupt pin connected to the device.
maxItems: 1

linux,code:
description: |
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
Copy link
Collaborator

Choose a reason for hiding this comment

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

there is a standard input property for key events:

https://elixir.bootlin.com/linux/v6.13.1/source/Documentation/devicetree/bindings/input/input.yaml#L26

Check for examples of drivers using it. You'll need to reference the input.yaml from this...

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

maximum: 0x2ff

required:
- compatible
- linux,code
Copy link
Collaborator

Choose a reason for hiding this comment

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

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


additionalProperties: false

examples:
- |
#include <dt-bindings/interrupt-controller/irq.h>
#include <dt-bindings/input/linux-event-codes.h>

power-button {
compatible = "adi,max16150";
interrupt-parent = <&gpio>;
interrupts = <17 IRQ_TYPE_EDGE_BOTH>;
linux,code = <KEY_POWER>;
};
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
tristate "MAX16150/MAX16169 Pushbutton driver"
help
Say Y here if you want to enable power key reporting via
MAX16150/MAX16169 nanoPower Pushbutton On/Off Controller.

To compile this driver as a module, choose M here. The module will
be called max16150.

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
149 changes: 149 additions & 0 deletions drivers/input/misc/max16150.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,149 @@
// SPDX-License-Identifier: GPL-2.0-or-later
/*
* Analog Devices MAX16150/MAX16169 Pushbutton Driver
*
* Copyright 2025 Analog Devices Inc.
*/

#include <linux/errno.h>
#include <linux/init.h>
#include <linux/input.h>
#include <linux/interrupt.h>
#include <linux/kernel.h>
#include <linux/module.h>
#include <linux/platform_device.h>
#include <linux/ktime.h>
#include <linux/of.h>
Copy link
Collaborator

Choose a reason for hiding this comment

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

hmm, is the above header really needed?

Copy link
Collaborator

Choose a reason for hiding this comment

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

you need mod_devicetable.h for the of_device_id table


struct max16150_data {
struct input_dev *bttn;
int custom_key_event;
Copy link
Collaborator

Choose a reason for hiding this comment

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

do we need the above?

ktime_t irq_rising_time;
ktime_t irq_falling_time;
};

static irqreturn_t max16150_rise_irq(int irq, void *_data)
{
struct max16150_data *data = _data;
struct input_dev *bttn = data->bttn;
struct device *dev = bttn->dev.parent;

data->irq_rising_time = ktime_get();

ktime_t irq_low_duration = ktime_sub(data->irq_rising_time,
data->irq_falling_time);
unsigned long duration_ms = ktime_to_ms(irq_low_duration);
Copy link
Collaborator

Choose a reason for hiding this comment

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

please no mixed code declaration...


dev_info(dev, "Interrupt low duration: %lu ms\n", duration_ms);
Copy link
Collaborator

Choose a reason for hiding this comment

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

drop the print... if you really want use dev_dbg_ratelimited()


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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

return IRQ_HANDLED;
}

static irqreturn_t max16150_fall_irq(int irq, void *_data)
{
struct max16150_data *data = _data;
struct input_dev *bttn = data->bttn;

data->irq_falling_time = ktime_get();

input_report_key(bttn, KEY_POWER, 0);
Copy link
Collaborator

Choose a reason for hiding this comment

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

not KEY_POWER.... data->key_event

input_sync(bttn);

return IRQ_HANDLED;
}
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

Copy link
Collaborator

@nunojsa nunojsa Feb 4, 2025

Choose a reason for hiding this comment

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

While this might be a good idea there's one thing that is still not perfect... If for some reason, the HW designers remember to invert the IRQ signal (because... why not :)), this whole logic will fail... Did you tried my suggestion with

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

??


static int max16150_probe(struct platform_device *pdev)
{
struct max16150_data *data;
struct input_dev *bttn;
int fall_irq, rise_irq;
int err;
struct device_node *np = pdev->dev.of_node;

data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
if (!data)
return -ENOMEM;

bttn = devm_input_allocate_device(&pdev->dev);
if (!bttn) {
dev_err(&pdev->dev, "Can't allocate power button\n");
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.


data->bttn = bttn;
data->custom_key_event = KEY_POWER;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would drip "custom" and just call it key_event or key_code


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

fall_irq = platform_get_irq(pdev, 0);
if (fall_irq < 0)
return fall_irq;

rise_irq = platform_get_irq(pdev, 1);
if (rise_irq < 0)
return rise_irq;
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 odd having two irqs for the same pin. But if it works, it is a good way to distinguish between rising and falling edge...


err = devm_request_threaded_irq(&pdev->dev, rise_irq, NULL,
max16150_rise_irq,
IRQF_TRIGGER_RISING | IRQF_ONESHOT,
"max16150_rising", data);
if (err < 0) {
dev_err(&pdev->dev, "Can't register rise irq: %d\n", err);
return err;
}

err = devm_request_threaded_irq(&pdev->dev, fall_irq, NULL,
max16150_fall_irq,
IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
"max16150_falling", data);
if (err < 0) {
dev_err(&pdev->dev, "Can't register fall irq: %d\n", err);
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(). ditto for all the other places. You also need to state in the DT binbings that you need two interrupts

return err;
}

err = input_register_device(bttn);
if (err) {
dev_err(&pdev->dev, "Can't register power button: %d\n", err);
return err;
}

platform_set_drvdata(pdev, data);
Copy link
Collaborator

Choose a reason for hiding this comment

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

once you remove the .remove() callback, you do not need the above

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.


if (np) {
u32 key_event;

if (!of_property_read_u32(np, "linux,code", &key_event))
data->custom_key_event = key_event;
else
dev_warn(&pdev->dev, "Failed to read custom key event, using default\n");
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just use an irq... No need for it to be a gpio

Copy link
Collaborator

Choose a reason for hiding this comment

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

I already mentioned this... do not use OF but device properties. Please ask to me or someone else if you do not understand what i'm suggesting


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,
.driver = {
.name = "max16150",
.of_match_table = max16150_of_match,
},
};
module_platform_driver(max16150_driver);

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