On ven., mars 04, 2022 at 14:38, Andy Shevchenko [off-list ref] wrote:

On Fri, Mar 04, 2022 at 11:31:38AM +0100, AngeloGioacchino Del Regno wrote:

quoted

Il 03/03/22 16:43, Mattijs Korpershoek ha scritto:

quoted

From: "fengping.yu" <redacted>

This patch adds matrix keypad support for Mediatek SoCs.

quoted

quoted

+struct mt6779_keypad {
+	struct regmap *regmap;
+	struct input_dev *input_dev;
+	struct clk *clk;

quoted

quoted

+	void __iomem *base;

Not sure why you need this here.

You are right. There is no point of keeping this __iomem region as part
of the structure, since it's only used in the probe() function to create
the regmap.

Will send an improvement part of a later series.

quoted

quoted

+	u32 n_rows;
+	u32 n_cols;
+	DECLARE_BITMAP(keymap_state, MTK_KPD_NUM_BITS);
+};
+
+static const struct regmap_config mt6779_keypad_regmap_cfg = {
+	.reg_bits = 32,
+	.val_bits = 32,
+	.reg_stride = sizeof(u32),
+	.max_register = 36,

Are you sure that you can't use .fast_io = true?

Another version for the same question:
Are you sure that you need to lock with a mutex here, and not with a spinlock?

Since you're performing reads over a MMIO, I think that there's a very good
chance that you can use fast_io.

quoted

+};

...

quoted

Please use dev_err_probe() to simplify error handling in probe functions: you've
done a great job with adding a devm action for the error cases, avoiding gotos to
get out cleanly.. it would be a pity to not finish this to perfection.

I'll give you two examples for this, so that you'll be all set.

	if (IS_ERR(keypad->regmap))
		return dev_err_probe(&pdev->dev, PTR_ERR(keypad->regmap),
				     "regmap init failed\n");

P.S.: No need for %pe here, as dev_err_probe prints the error number for you!

Maintainer of the input subsystem is strongly against dev_err_probe() API. See
other files there. Ditto for other cases you mentioned below.

-- 
With Best Regards,
Andy Shevchenko