-
Notifications
You must be signed in to change notification settings - Fork 857
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. no... please look at other examples using this. Hint: 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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>; | ||
}; |
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> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hmm, is the above header really needed? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not KEY_POWER.... data->key_event |
||
input_sync(bttn); | ||
|
||
return IRQ_HANDLED; | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No need for a function for this. Call platform_get_irq() directly There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Look at this to see how wakeup is used: https://elixir.bootlin.com/linux/v6.12.6/source/drivers/input/misc/pm8941-pwrkey.c#L222 |
||
|
||
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"); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just use an irq... No need for it to be a gpio There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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"); |
There was a problem hiding this comment.
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...