Received-SPF: pass (google.com: domain of linux-kernel+bounces-7635-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Wed, 20 Dec 2023 15:33:46 -0800
From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
To: Karel Balej <karelb@gimli.ms.mff.cuni.cz>
Cc: Karel Balej <balejk@matfyz.cz>, Rob Herring <robh+dt@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Conor Dooley <conor+dt@kernel.org>, Lee Jones <lee@kernel.org>,
	linux-input@vger.kernel.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org,
	Duje =?utf-8?Q?Mihanovi=C4=87?= <duje.mihanovic@skole.hr>,
	~postmarketos/upstreaming@lists.sr.ht, phone-devel@vger.kernel.org
Subject: Re: [RFC PATCH 4/5] input: add onkey driver for Marvell 88PM88X PMICs
Message-ID: <ZYN52mD6-1hj_K7S@google.com>
References: <20231217131838.7569-1-karelb@gimli.ms.mff.cuni.cz>
 <20231217131838.7569-5-karelb@gimli.ms.mff.cuni.cz>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20231217131838.7569-5-karelb@gimli.ms.mff.cuni.cz>

Hi Karel,

On Sun, Dec 17, 2023 at 02:17:02PM +0100, Karel Balej wrote:
> From: Karel Balej <balejk@matfyz.cz>
> 
> The Marvell 88PM88X PMICs provide onkey among other things. Add client
> driver to handle it. The driver currently only provides a basic support
> omitting additional functions found in the vendor version, such as long
> onkey and GPIO integration.
> 
> Signed-off-by: Karel Balej <balejk@matfyz.cz>
> ---
>  drivers/input/misc/88pm88x-onkey.c | 103 +++++++++++++++++++++++++++++
>  drivers/input/misc/Kconfig         |  10 +++
>  drivers/input/misc/Makefile        |   1 +
>  3 files changed, 114 insertions(+)
>  create mode 100644 drivers/input/misc/88pm88x-onkey.c
> 
> diff --git a/drivers/input/misc/88pm88x-onkey.c b/drivers/input/misc/88pm88x-onkey.c
> new file mode 100644
> index 000000000000..0d6056a3cab2
> --- /dev/null
> +++ b/drivers/input/misc/88pm88x-onkey.c
> @@ -0,0 +1,103 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +#include <linux/kernel.h>
> +#include <linux/input.h>
> +#include <linux/regmap.h>
> +#include <linux/irq.h>
> +#include <linux/interrupt.h>
> +#include <linux/platform_device.h>
> +#include <linux/of.h>

Please sort alphabetically.

> +
> +#include <linux/mfd/88pm88x.h>
> +
> +struct pm88x_onkey {
> +	struct input_dev *idev;
> +	struct pm88x_chip *chip;
> +	int irq;

Since you are using devm to request interrupt I do not think you need to
store it here.

> +};
> +
> +static irqreturn_t pm88x_onkey_irq_handler(int irq, void *data)
> +{
> +	struct pm88x_onkey *onkey = data;
> +	unsigned int val;
> +	int ret = 0;

"error". Also no need to initialize it to 0.

> +
> +	ret = regmap_read(onkey->chip->regmaps[PM88X_REGMAP_BASE], PM88X_REG_STATUS1, &val);

I still prefer to keep withing 80 columns, unless longer lines are a
clear win.

> +	if (ret) {
> +		dev_err(onkey->idev->dev.parent, "Failed to read status: %d\n", ret);

Maybe have "dev" in onkey instead of poking through input?

> +		return IRQ_NONE;
> +	}
> +	val &= PM88X_ONKEY_STS1;
> +
> +	input_report_key(onkey->idev, KEY_POWER, val);
> +	input_sync(onkey->idev);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int pm88x_onkey_probe(struct platform_device *pdev)
> +{
> +	struct pm88x_chip *chip = dev_get_drvdata(pdev->dev.parent);
> +	struct pm88x_onkey *onkey;
> +	int err;
> +
> +	onkey = devm_kzalloc(&pdev->dev, sizeof(*onkey), GFP_KERNEL);
> +	if (!onkey)
> +		return -ENOMEM;
> +
> +	onkey->chip = chip;
> +
> +	onkey->irq = platform_get_irq(pdev, 0);
> +	if (onkey->irq < 0) {
> +		dev_err(&pdev->dev, "Failed to get IRQ\n");
> +		return -EINVAL;

Do not clobber the return from platform_get_irq(). So "return
onkey->irq" (or simply irq if you drop it from onkey and use temporary).

> +	}
> +
> +	onkey->idev = devm_input_allocate_device(&pdev->dev);
> +	if (!onkey->idev) {
> +		dev_err(&pdev->dev, "Failed to allocate input device\n");
> +		return -ENOMEM;
> +	}
> +
> +	onkey->idev->name = "88pm88x-onkey";
> +	onkey->idev->phys = "88pm88x-onkey/input0";
> +	onkey->idev->id.bustype = BUS_I2C;
> +	onkey->idev->dev.parent = &pdev->dev;

No need to do this since you are using devm_input_allocate_device().

> +	onkey->idev->evbit[0] = BIT_MASK(EV_KEY);
> +	onkey->idev->keybit[BIT_WORD(KEY_POWER)] = BIT_MASK(KEY_POWER);

Please use

	input_set_capability(onkey->idev, EV_KEY, KEY_POWER);

> +
> +	err = devm_request_threaded_irq(&pdev->dev, onkey->irq, NULL, pm88x_onkey_irq_handler,
> +			IRQF_ONESHOT | IRQF_NO_SUSPEND, "onkey", onkey);

Please align arguments.

> +	if (err) {
> +		dev_err(&pdev->dev, "Failed to request IRQ: %d\n", err);

I was persuaded to stop my crusade against dev_err_probe() so you may
use it here.

> +		return err;
> +	}
> +
> +	err = input_register_device(onkey->idev);
> +	if (err) {
> +		dev_err(&pdev->dev, "Failed to register input device: %d\n", err);
> +		return err;
> +	}
> +
> +	device_init_wakeup(&pdev->dev, 1);

Are there cases where we woudl not want wakeup? Maybe use standard
"wakeup-source" property?

> +
> +	return 0;
> +}
> +
> +static const struct of_device_id pm88x_onkey_of_match[] = {
> +	{ .compatible = "marvell,88pm88x-onkey", },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, pm88x_onkey_of_match);
> +
> +static struct platform_driver pm88x_onkey_driver = {
> +	.driver = {
> +		.name = "88pm88x-onkey",
> +		.of_match_table = of_match_ptr(pm88x_onkey_of_match),

Given that you are not guarding pm88x_onkey_of_match definition with
#ifdef CONFIG_OF you shoudl not use of_match_ptr().

Thanks.

-- 
Dmitry