Re: [PATCH v2 2/2] input: misc: Add PixArt PAJ7620 gesture sensor driver

[Krzysztof Kozlowski <krzk@kernel.org>]

On Fri, Apr 17, 2026 at 01:25:27AM -0400, Harpreet Saini wrote:
> +
> +static int paj7620_init(struct paj7620_data *data)
> +{
> +	int state = 0, ret, i;
> +
> +	/* 1. Wake-up sequence: Read register 0x00 until it returns 0x20 */
> +	for (i = 0; i < 10; i++) {
> +		ret = regmap_read(data->regmap, 0x00, &state);
> +		if (ret >= 0 && state == 0x20)
> +			break;
> +		usleep_range(1000, 2000);
> +	}
> +
> +	if (state != 0x20) {
> +		dev_err(&data->client->dev, "Sensor wake-up failed (0x%02x)\n", state);
> +		return -ENODEV;
> +	}
> +
> +	/* 2. Blast full register array into PAJ7620 instantly */
> +	ret = regmap_multi_reg_write(data->regmap, Init_Register,
> +				     ARRAY_SIZE(Init_Register));
> +	if (ret < 0) {
> +		dev_err(&data->client->dev, "Multi-reg write failed (%d)\n", ret);
> +		return ret;
> +	}
> +
> +	ret = regmap_write(data->regmap, PAJ7620_REG_BANK_SEL, 0x00);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = regmap_multi_reg_write(data->regmap, Init_Gesture_Array,
> +				     ARRAY_SIZE(Init_Gesture_Array));
> +	if (ret < 0) {
> +		dev_err(&data->client->dev, "Multi-reg write failed (%d)\n", ret);
> +		return ret;
> +	}
> +
> +	dev_info(&data->client->dev, "Gesture Sensor Registers Initialized\n");

Drop, driver should be silent.

...

> +	data->client = client;
> +	i2c_set_clientdata(client, data);
> +
> +	data->supplies[0].supply = "vdd";
> +	data->supplies[1].supply = "vbus";
> +	data->supplies[2].supply = "vled";
> +
> +	ret = devm_regulator_bulk_get(&client->dev, ARRAY_SIZE(data->supplies), data->supplies);
> +	if (ret)
> +		return dev_err_probe(&client->dev, ret, "Failed to get regulators\n");
> +
> +	ret = regulator_bulk_enable(ARRAY_SIZE(data->supplies), data->supplies);
> +	if (ret)
> +		return ret;
> +
> +	data->regmap = devm_regmap_init_i2c(client, &paj7620_reg_config);
> +	if (IS_ERR(data->regmap))
> +		return PTR_ERR(data->regmap);

Leaking regulator enable.

> +
> +	ret = paj7620_init(data);
> +	if (ret)
> +		goto err_reg;
> +
> +	data->idev = devm_input_allocate_device(&client->dev);
> +	if (!data->idev) {
> +		ret = -ENOMEM; goto err_reg;

Messed syntax/wrapped lines.

And you must not print error msg on ENOMEM error.

> +	}
> +
> +	data->idev->name = "PAJ7620 Gesture Sensor";
> +	data->idev->id.bustype = BUS_I2C;
> +
> +	input_set_capability(data->idev, EV_KEY, KEY_UP);
> +	input_set_capability(data->idev, EV_KEY, KEY_DOWN);
> +	input_set_capability(data->idev, EV_KEY, KEY_LEFT);
> +	input_set_capability(data->idev, EV_KEY, KEY_RIGHT);
> +	input_set_capability(data->idev, EV_KEY, KEY_ENTER);
> +	input_set_capability(data->idev, EV_KEY, KEY_BACK);
> +	input_set_capability(data->idev, EV_KEY, KEY_NEXT);
> +	input_set_capability(data->idev, EV_KEY, KEY_PREVIOUS);
> +	input_set_capability(data->idev, EV_KEY, KEY_MENU);
> +
> +	ret = input_register_device(data->idev);
> +	if (ret)
> +		goto err_reg;
> +
> +	pm_runtime_set_active(&client->dev);
> +	pm_runtime_enable(&client->dev);
> +	pm_runtime_set_autosuspend_delay(&client->dev, 2000);
> +	pm_runtime_use_autosuspend(&client->dev);
> +
> +	ret = devm_request_threaded_irq(&client->dev, client->irq, NULL,
> +					paj7620_irq_thread, IRQF_ONESHOT,
> +					"paj7620", data);
> +	if (ret)
> +		goto err_reg;
> +
> +	dev_info(&client->dev, "Gesture Sensor Initialized\n");

Pointless message, drop. Driver should be silent on success.


> +	return 0;
> +
> +err_reg:
> +	dev_err_probe(&client->dev, ret, "%s: failed with error %d\n", __func__, ret);

No, move it to individual errors, but only where applicable. For example
devm_request_threaded_irq() must not have it.

Neither devm_input_allocate_device.


> +	if (pm_runtime_enabled(&client->dev)) {
> +		pm_runtime_disable(&client->dev);
> +		pm_runtime_dont_use_autosuspend(&client->dev);
> +	}
> +	regulator_bulk_disable(ARRAY_SIZE(data->supplies), data->supplies);
> +	return ret;
> +}
> +
> +static void paj7620_remove(struct i2c_client *client)
> +{
> +	int ret;
> +	struct paj7620_data *data = i2c_get_clientdata(client);
> +
> +	pm_runtime_get_sync(&client->dev);
> +	pm_runtime_disable(&client->dev);
> +	pm_runtime_dont_use_autosuspend(&client->dev);
> +	pm_runtime_put_noidle(&client->dev);
> +
> +	ret = paj7620_power_down(data);
> +	if (ret)
> +		dev_err(&data->client->dev, "Sensor power down failed\n");
> +
> +	ret = regulator_bulk_disable(ARRAY_SIZE(data->supplies), data->supplies);
> +	if (ret)
> +		dev_err(&data->client->dev, "Sensor regulator disable failed\n");
> +}
> +
> +static const struct of_device_id paj7620_of_match[] = {
> +	{ .compatible = "pixart,paj7620" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, paj7620_of_match);
> +
> +static struct i2c_driver paj7620_driver = {
> +	.driver = {
> +		.name = "paj7620",
> +		.of_match_table = paj7620_of_match,
> +		.pm = &paj7620_pm_ops,
> +	},
> +	.probe = paj7620_probe,
> +	.remove = paj7620_remove,
> +};
> +module_i2c_driver(paj7620_driver);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Harpreet Saini");
> +MODULE_DESCRIPTION("PAJ7620 Gesture Input Driver");
> -- 
> 2.43.0
>