RE: [PATCH 2/2] input: misc: add driver for max16150

[Sosa, Marc Paolo]

> -----Original Message-----
> From: Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx>
> Sent: Wednesday, February 25, 2026 6:42 AM
> To: Sosa, Marc Paolo <MarcPaolo.Sosa@xxxxxxxxxx>
> Cc: Rob Herring <robh@xxxxxxxxxx>; Krzysztof Kozlowski
> <krzk+dt@xxxxxxxxxx>; Conor Dooley <conor+dt@xxxxxxxxxx>; linux-
> input@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH 2/2] input: misc: add driver for max16150
> 
> [External]
> 
> Hi Marc,
> 
> On Mon, Feb 23, 2026 at 07:03:40PM +0800, Marc Paolo Sosa via B4 Relay
> wrote:
> > From: Marc Paolo Sosa <marcpaolo.sosa@xxxxxxxxxx>
> >
> > MAX16150/MAX16169 nanoPower Pushbutton On/Off Controller
> >
> > Signed-off-by: Marc Paolo Sosa <marcpaolo.sosa@xxxxxxxxxx>
> > ---
> >  drivers/input/misc/Kconfig    |   9 +++
> >  drivers/input/misc/Makefile   |   1 +
> >  drivers/input/misc/max16150.c | 161
> > ++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 171 insertions(+)
> >
> > diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
> > index 94a753fcb64f..a31d3d2a7fd6 100644
> > --- a/drivers/input/misc/Kconfig
> > +++ b/drivers/input/misc/Kconfig
> > @@ -178,6 +178,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
> > diff --git a/drivers/input/misc/Makefile b/drivers/input/misc/Makefile
> > index 415fc4e2918b..c2c1c45f2df6 100644
> > --- a/drivers/input/misc/Makefile
> > +++ b/drivers/input/misc/Makefile
> > @@ -52,6 +52,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_MAX7360_ROTARY)	+= max7360-rotary.o
> >  obj-$(CONFIG_INPUT_MAX77650_ONKEY)	+= max77650-onkey.o
> >  obj-$(CONFIG_INPUT_MAX77693_HAPTIC)	+= max77693-haptic.o
> > diff --git a/drivers/input/misc/max16150.c
> > b/drivers/input/misc/max16150.c new file mode 100644 index
> > 000000000000..ae353b926afc
> > --- /dev/null
> > +++ b/drivers/input/misc/max16150.c
> > @@ -0,0 +1,161 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * Analog Devices MAX16150/MAX16169 Pushbutton Driver
> > + *
> > + * Copyright 2025 Analog Devices Inc.
> > + */
> > +
> > +#include <linux/delay.h>
> > +#include <linux/init.h>
> > +#include <linux/input.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/gpio/consumer.h>
> > +#include <linux/kernel.h>
> > +#include <linux/mod_devicetable.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/property.h>
> > +
> > +#define MAX16150_LONG_INTERRUPT 120000000
> > +
> > +struct max16150_chip_info {
> > +	bool has_clr_gpio;
> > +};
> > +
> > +struct max16150_device {
> > +	struct input_dev *input;
> > +	struct gpio_desc *gpiod;
> > +	struct gpio_desc *clr_gpiod;
> > +	const struct max16150_chip_info *chip_info;
> > +	u64 low, high, duration;
> 
> I do not think you need to store "high" and "duration", just "press"
> time. I also do not think you need nanosecond resilution, jiffies will do.
> 
> > +	unsigned int keycode;
> > +};
> > +
> > +static irqreturn_t max16150_irq_handler(int irq, void *_max16150) {
> > +	struct max16150_device *max16150 = _max16150;
> > +	int value;
> > +
> > +	value = gpiod_get_value(max16150->gpiod);
> > +
> > +	if (!value) {
> > +		max16150->low = ktime_get_ns();
> > +		return IRQ_HANDLED;
> > +	}
> > +
> > +	max16150->high = ktime_get_ns();
> > +	if (max16150->low) {
> > +		max16150->duration = max16150->high - max16150->low;
> > +
> > +		if (max16150->duration > MAX16150_LONG_INTERRUPT) {
> 
> time_after() is probably what you need here.
> 
> > +			gpiod_set_value(max16150->clr_gpiod, 1);
> > +			input_report_key(max16150->input, max16150-
> >keycode, 1);
> > +			input_sync(max16150->input);
> 
> Why is press not reported right away?
> 
> > +			input_report_key(max16150->input, max16150-
> >keycode, 0);
> > +			input_sync(max16150->input);
> > +		}
> > +
> > +		max16150->low = 0;
> > +	}
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +static const struct max16150_chip_info max16150_variant_a = {
> > +	.has_clr_gpio = true,
> > +};
> > +
> > +static const struct max16150_chip_info max16150_variant_b = {
> > +	.has_clr_gpio = false,
> > +};
> > +
> > +static int max16150_probe(struct platform_device *pdev) {
> > +	const struct max16150_chip_info *chip_info;
> > +	struct max16150_device *max16150;
> > +	struct device *dev = &pdev->dev;
> > +	int err, irq, ret;
> 
> Why do you need both err and ret?
> 
> > +	u32 keycode;
> > +
> > +	chip_info = device_get_match_data(dev);
> > +	if (!chip_info)
> > +		return -EINVAL;
> > +
> > +	max16150 = devm_kzalloc(dev, sizeof(*max16150), GFP_KERNEL);
> > +	if (!max16150)
> > +		return -ENOMEM;
> > +
> > +	max16150->chip_info = chip_info;
> > +
> > +	max16150->input = devm_input_allocate_device(dev);
> > +	if (!max16150->input)
> > +		return -ENOMEM;
> > +
> > +	max16150->input->name = "MAX16150 Pushbutton";
> > +	max16150->input->phys = "max16150/input0";
> > +	max16150->input->id.bustype = BUS_HOST;
> > +
> > +	keycode = KEY_POWER;
> > +	ret = device_property_read_u32(dev, "linux,code", &keycode);
> 
> 	err = ...
> 
> > +	if (ret)
> > +		return dev_err_probe(dev, ret, "Failed to get keycode\n");
> > +
> > +	max16150->keycode = keycode;
> > +
> > +	input_set_capability(max16150->input, EV_KEY, max16150->keycode);
> > +
> > +	max16150->gpiod = devm_gpiod_get(dev, "interrupt", GPIOD_IN);
> > +	if (IS_ERR(max16150->gpiod))
> > +		return dev_err_probe(dev, PTR_ERR(max16150->gpiod),
> > +				     "Failed to get interrupt GPIO\n");
> > +
> > +	if (chip_info->has_clr_gpio) {
> > +		max16150->clr_gpiod = devm_gpiod_get(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)
> 
> 
> How would we end up here? You are using devm_gpiod_get() which will never
> return NULL GPIO descriptor.
> 
> > +			return dev_err_probe(dev, -ENODEV,
> > +						 "clr GPIO is mandatory\n");
> > +
> > +		if (max16150->clr_gpiod) {
> > +			fsleep(1000);
> > +			gpiod_set_value(max16150->clr_gpiod, 0);
> > +		}
> > +	}
> > +
> > +	irq = gpiod_to_irq(max16150->gpiod);
> > +	if (irq < 0)
> > +		return dev_err_probe(dev, irq,
> > +				     "MAX16150: Failed to map GPIO to IRQ");
> 
> As Rob said in DT binding review use interrupts property and separate IRQ and
> GPIO handling.
> 
> > +
> > +	err = devm_request_irq(dev, irq, max16150_irq_handler,
> > +			       IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
> > +			       "max16150_irq", max16150);
> > +	if (err)
> > +		return err;
> > +
> > +	return input_register_device(max16150->input);
> 
> 	err = input_register_device(...);
> 	if (err)
> 		return err;
> 
> 	return 0;
> 
> > +}
> > +
> > +static const struct of_device_id max16150_of_match[] = {
> > +	{ .compatible = "adi,max16150a", .data = &max16150_variant_a },
> > +	{ .compatible = "adi,max16150b", .data = &max16150_variant_b },
> > +	{ .compatible = "adi,max16169a", .data = &max16150_variant_a },
> > +	{ .compatible = "adi,max16169b", .data = &max16150_variant_b },
> > +	{ }
> > +};
> > +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 <marcpaolo.sosa@xxxxxxxxxx>");
> > +MODULE_DESCRIPTION("MAX16150/MAX16169 Pushbutton Driver");
> > +MODULE_LICENSE("GPL");
> 


Thank you for the initial review, I'll test the ff suggestions above and apply it to V2, for summary,

  1. Switch from nanoseconds to jiffies with single press_time variable and
  time_after() for comparison
  2. Remove redundant err/ret variables and use consistent naming
  3. Remove impossible NULL checks after devm_gpiod_get() since it never returns NULL
  4. Fix device_property_read_u32() error handling since linux,code is optional with
  default
  5. Use explicit error checking pattern instead of implicit return
  6. The MAX16150's OUT pin serves dual purposes (interrupt + GPIO state reading) -
  should I use separate 'interrupts' and 'out-gpios' properties for the same physical pin, or is there a
  preferred approach for this hardware design?
  7. The duration measurement is needed since the device ignores short presses, but I could
   report press immediately and validate on release instead

  I'll send v2 with these improvements once I clarify the preferred IRQ/GPIO handling approach.

Best regards,
Pao