-
Notifications
You must be signed in to change notification settings - Fork 854
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?
Conversation
fc933bb
to
55c3ff9
Compare
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.
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 😉
brcm,function = <0>; | ||
brcm,pull = <0>; | ||
}; | ||
}; |
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.
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?
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.
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.
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.
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?
compatible = "adi,max16150"; | ||
adi,variant = "A"; | ||
|
||
gpio-pb_in = <&gpio 17 1>; |
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.
Don't think the above needs to be a gpio. It's a button that you should register with the input subsystem
adi,variant = "A"; | ||
|
||
gpio-pb_in = <&gpio 17 1>; | ||
gpio-out = <&gpio 18 0>; |
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.
I'm also not convinced we need the above...
gpio-pb_in = <&gpio 17 1>; | ||
gpio-out = <&gpio 18 0>; | ||
gpio-clr = <&gpio 22 1>; | ||
gpio-int = <&gpio 23 0>; |
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.
This is an interrupts
property and not a gpio
|
||
hrtimer_cancel(&data->debounce_timer); | ||
hrtimer_cancel(&data->shutdown_timer); | ||
return 0; |
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.
This can be done in a devm_action... Also not sure if we really need hrtimers for this
drivers/input/misc/max16150.c
Outdated
if (ret) { | ||
dev_err(&pdev->dev, "Failed to request IRQ for PB_IN\n"); | ||
return ret; | ||
} |
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.
Just use an irq... No need for it to be a gpio
} else { | ||
dev_err(&pdev->dev, "Unknown device type: %s\n", type); | ||
return -EINVAL; | ||
} |
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.
use proper chip info structure for subtleties in the chips... The above does not scale at al.
drivers/input/misc/max16150.c
Outdated
return -EINVAL; | ||
} | ||
|
||
ret = device_property_read_string(&pdev->dev, "adi,variant", &variant); |
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.
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.
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.
You might want to check out existing input device drivers for reference.
drivers/input/misc/max16150.c
Outdated
|
||
if (data->variant == MAX161X_A && data->out_asserted) { | ||
dev_info(data->dev, "Shutdown time exceeded. Deasserting OUT."); | ||
gpiod_set_value(data->gpio_clr, 0); |
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.
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.
drivers/input/misc/max16150.c
Outdated
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; |
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.
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.
drivers/input/misc/max16150.c
Outdated
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); |
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.
pb_in is not an input to the MPU and is not connected to the MPU. This is connected to the push button.
drivers/input/misc/max16150.c
Outdated
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); |
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.
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.
drivers/input/misc/max16150.c
Outdated
if (IS_ERR(data->gpio_clr)) | ||
return PTR_ERR(data->gpio_clr); | ||
|
||
data->gpio_int = devm_gpiod_get(&pdev->dev, "int", GPIOD_IN); |
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.
Just use an irq.
drivers/input/misc/max16150.c
Outdated
enum max161x_variant { | ||
MAX161X_A, | ||
MAX161X_B, | ||
MAX161X_C, | ||
}; | ||
|
||
enum max161x_type { | ||
MAX16150, | ||
MAX16169, | ||
}; |
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.
Why don't you just combine these? I think the variant is part of the ordering options for customers to select different timing options?
drivers/input/misc/max16150.c
Outdated
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); | ||
} | ||
} |
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.
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.
drivers/input/misc/max16150.c
Outdated
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), |
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.
The irq pin is the int pin not the pb_in pin.
7aebbf1
to
e96094d
Compare
Changelogs V2:
|
drivers/input/misc/max16150.c
Outdated
#define GPIO_INT 17 | ||
#define GPIO_CLR 27 |
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.
Get this from the device tree overlay.
drivers/input/misc/max16150.c
Outdated
static irqreturn_t max16150_isr(int irq, void *dev_id) | ||
{ | ||
ktime_t now = ktime_get(); | ||
ktime_t duration = ktime_sub(now, last_time); | ||
|
||
if (duration >= long_pulse) | ||
gpio_set_value(GPIO_CLR, 0); | ||
|
||
last_time = now; | ||
return IRQ_HANDLED; | ||
} |
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.
How will the input event propagate to sysfs?
How can users define different events for the long button press and short button press?
c49c74f
to
0b28d2c
Compare
Add documentation for device tree bindings for MAX16150/MAX16169 Signed-off-by: Marc Paolo Sosa <[email protected]>
98b76e0
to
f413730
Compare
brcm,function = <0>; | ||
brcm,pull = <0>; | ||
}; | ||
}; |
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.
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?
- compatible | ||
- gpios | ||
- interrupt-parent | ||
- interrupts |
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.
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)
@@ -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 |
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.
This is a very odd dependency. Most likely this is not what you want
drivers/input/misc/max16150.c
Outdated
int ret; | ||
|
||
gpio_int = of_get_named_gpio(node, "gpios", 0); | ||
gpio_clr = of_get_named_gpio(node, "gpios", 1); |
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.
nope.. you need to use gpio_desc APIs. Example:
https://elixir.bootlin.com/linux/v6.12.6/source/drivers/gpio/gpiolib-devres.c#L50
drivers/input/misc/max16150.c
Outdated
if (!gpio_is_valid(gpio_int) || !gpio_is_valid(gpio_clr)) { | ||
dev_err(&pdev->dev, "Invalid GPIOs\n"); | ||
return -EINVAL; | ||
} |
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.
the above can be dropped with the the API I pointed
input_report_key(button, KEY_WAKEUP, 1); | ||
input_sync(button); | ||
falling_edge = false; | ||
} |
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.
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
static int get_irq(struct platform_device *pdev) | ||
{ | ||
return platform_get_irq(pdev, 0); | ||
} |
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.
No need for a function for this. Call platform_get_irq() directly
|
||
prev_time = ktime_get(); | ||
platform_set_drvdata(pdev, button); | ||
device_init_wakeup(&pdev->dev, true); |
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.
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
|
||
static struct platform_driver max16150_driver = { | ||
.probe = max16150_probe, | ||
.remove = max16150_remove, |
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.
remove is not needed since you're using devm_input_allocate_device()
.of_match_table = max16150_of_match, | ||
}, | ||
}; | ||
|
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.
nit: no need for new line in these cases
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]>
PR Description
necessary to understand them. List any dependencies required for this change.
any space), or simply check them after publishing the PR.
description and try to push all related PRs simultaneously.
PR Type
PR Checklist