Hi Bartosz,

On Fri, Jan 18, 2019 at 02:42:43PM +0100, Bartosz Golaszewski wrote:

It is weird that report function returns irqreturn_t. I'd simply moved
input_report_key()/input_sync() into real IRQ handlers.

+}
+
+static irqreturn_t max77650_onkey_falling(int irq, void *data)
+{
+	struct max77650_onkey *onkey = data;
+
+	return max77650_onkey_report(onkey, 0);
+}
+
+static irqreturn_t max77650_onkey_rising(int irq, void *data)
+{
+	struct max77650_onkey *onkey = data;
+
+	return max77650_onkey_report(onkey, 1);
+}
+
+static int max77650_onkey_probe(struct platform_device *pdev)
+{
+	struct regmap_irq_chip_data *irq_data;
+	struct max77650_onkey *onkey;
+	struct device *dev, *parent;
+	int irq_r, irq_f, rv, mode;

Please call "rv" "error".

+	struct i2c_client *i2c;
+	const char *mode_prop;
+	struct regmap *map;
+
+	dev = &pdev->dev;
+	parent = dev->parent;
+	i2c = to_i2c_client(parent);
+	irq_data = i2c_get_clientdata(i2c);
+
+	map = dev_get_regmap(parent, NULL);
+	if (!map)
+		return -ENODEV;
+
+	onkey = devm_kzalloc(dev, sizeof(*onkey), GFP_KERNEL);
+	if (!onkey)
+		return -ENOMEM;
+
+	rv = device_property_read_u32(dev, "linux,code", &onkey->code);
+	if (rv)
+		onkey->code = KEY_POWER;
+
+	rv = device_property_read_string(dev, "maxim,onkey-mode", &mode_prop);
+	if (rv)
+		mode_prop = "push";
+
+	if (strcmp(mode_prop, "push") == 0)
+		mode = MAX77650_ONKEY_MODE_PUSH;
+	else if (strcmp(mode_prop, "slide") == 0)
+		mode = MAX77650_ONKEY_MODE_SLIDE;
+	else
+		return -EINVAL;
+
+	rv = regmap_update_bits(map, MAX77650_REG_CNFG_GLBL,
+				MAX77650_ONKEY_MODE_MASK, mode);
+	if (rv)
+		return rv;
+
+	irq_f = regmap_irq_get_virq(irq_data, MAX77650_INT_nEN_F);
+	if (irq_f <= 0)
+		return -EINVAL;
+
+	irq_r = regmap_irq_get_virq(irq_data, MAX77650_INT_nEN_R);
+	if (irq_r <= 0)
+		return -EINVAL;

Ugh, it would be better if you handled IRQ mapping in the MFD piece and
passed it as resources of platform device. Then you'd simply call
platform_get_irq() here and did not have to reach into parent device for
"irq_dara".

+
+	onkey->input = devm_input_allocate_device(dev);
+	if (!onkey->input)
+		return -ENOMEM;
+
+	onkey->input->name = "max77650_onkey";
+	onkey->input->phys = "max77650_onkey/input0";
+	onkey->input->id.bustype = BUS_I2C;
+	onkey->input->dev.parent = dev;

Not needed since devm_input_allocate_device sets parent for you.

+	input_set_capability(onkey->input, EV_KEY, onkey->code);
+
+	rv = devm_request_threaded_irq(dev, irq_f, NULL,

Why threaded interrupt with only hard interrupt handler? If parent
interrupt is threaded use "any_context_irq" here.

+				       max77650_onkey_falling,
+				       IRQF_ONESHOT, "onkey-down", onkey);

Why do you need oneshot with interrupt that is essentially not threaded?

+	if (rv)
+		return rv;
+
+	rv = devm_request_threaded_irq(dev, irq_r, NULL,
+				       max77650_onkey_rising,
+				       IRQF_ONESHOT, "onkey-up", onkey);
+	if (rv)
+		return rv;
+
+	return input_register_device(onkey->input);
+}
+
+static struct platform_driver max77650_onkey_driver = {
+	.driver = {
+		.name = "max77650-onkey",
+	},
+	.probe = max77650_onkey_probe,
+};
+module_platform_driver(max77650_onkey_driver);
+
+MODULE_DESCRIPTION("MAXIM 77650/77651 ONKEY driver");
+MODULE_AUTHOR("Bartosz Golaszewski [off-list ref]");
+MODULE_LICENSE("GPL");

SPDX header say GPL-2.0 so please "GPL v2" here as "GPL" means v2 and
later.

Thanks.

-- 
Dmitry