Message-ID: <20251028-funky-rose-rook-3ccab5@kuoka>
Date: Tue, 28 Oct 2025 08:44:29 +0100
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Hendrik Noack <hendrik-noack@....de>
Cc: Dmitry Torokhov <dmitry.torokhov@...il.com>, 
	Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, 
	Conor Dooley <conor+dt@...nel.org>, linux-input@...r.kernel.org, devicetree@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] Input: Add support for Wacom W9000-series penabled
 touchscreens

On Mon, Oct 27, 2025 at 10:25:35PM +0100, Hendrik Noack wrote:
> +
> +static const struct of_device_id wacom_w9000_of_match[] = {
> +	{ .compatible = "wacom,w9007a_lt03", .data = &wacom_w9007a_lt03, },
> +	{ .compatible = "wacom,w9007a_v1", .data = &wacom_w9007a_v1, },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, wacom_w9000_of_match);

This goes next to other ID table, around the probe.

> +
> +static int wacom_w9000_read(struct i2c_client *client, u8 command, int len, char *data)
> +{
> +	struct i2c_msg xfer[2];
> +	bool retried = false;
> +	int ret;
> +

...

> +
> +static int wacom_w9000_probe(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	struct wacom_w9000_data *wacom_data;
> +	struct input_dev *input_dev;
> +	int ret;
> +	u32 val;
> +
> +	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
> +		dev_err(dev, "i2c_check_functionality error\n");
> +		return -EIO;
> +	}
> +
> +	wacom_data = devm_kzalloc(dev, sizeof(struct wacom_w9000_data), GFP_KERNEL);

sizeof(*)

Please use existing kernel coding style, not some downstream version.

> +	if (!wacom_data)
> +		return -ENOMEM;
> +
> +	wacom_data->variant = i2c_get_match_data(client);
> +
> +	wacom_data->client = client;
> +
> +	input_dev = devm_input_allocate_device(dev);
> +	if (!input_dev)
> +		return -ENOMEM;
> +	wacom_data->input_dev = input_dev;
> +
> +	wacom_data->irq = client->irq;
> +	i2c_set_clientdata(client, wacom_data);
> +
> +	wacom_data->regulator = devm_regulator_get(dev, "vdd");
> +	if (IS_ERR(wacom_data->regulator)) {
> +		ret = PTR_ERR(wacom_data->regulator);
> +		dev_err(dev, "Failed to get regulators %d\n", ret);
> +		return ret;

Nope. Look at all other drivers. Syntax is since some years return
dev_err_probe.

> +	}
> +
> +	/* Request flash-mode line and don't go into flash mode */
> +	wacom_data->flash_mode_gpio = devm_gpiod_get_optional(dev, "flash-mode", GPIOD_OUT_LOW);
> +	if (IS_ERR(wacom_data->flash_mode_gpio)) {
> +		ret = PTR_ERR(wacom_data->flash_mode_gpio);
> +		dev_err(dev, "Failed to get flash-mode gpio: %d\n", ret);
> +		return ret;

You must handle deferred probe. Please look at all other drivers how
they do it.

> +	}
> +
> +	/* Request pdct line  */
> +	wacom_data->pen_detect_gpio = devm_gpiod_get_optional(dev, "pdct", GPIOD_IN);
> +	if (IS_ERR(wacom_data->pen_detect_gpio)) {
> +		ret = PTR_ERR(wacom_data->pen_detect_gpio);
> +		dev_err(dev, "Failed to get pdct gpio: %d\n", ret);
> +		return ret;
> +	}
> +
> +	/* Request pen-insert line  */
> +	wacom_data->pen_inserted_gpio = devm_gpiod_get_optional(dev, "pen-inserted", GPIOD_IN);
> +	if (IS_ERR(wacom_data->pen_inserted_gpio)) {
> +		ret = PTR_ERR(wacom_data->pen_inserted_gpio);
> +		dev_err(dev, "Failed to get pen-insert gpio: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = regulator_enable(wacom_data->regulator);
> +	if (ret) {
> +		dev_err(dev, "Failed to enable regulators: %d\n", ret);
> +		return ret;
> +	}
> +
> +	msleep(200);
> +
> +	ret = wacom_w9000_query(wacom_data);
> +	if (ret)
> +		goto err_disable_regulators;
> +
> +	input_dev->name = wacom_data->variant->name;
> +	input_dev->id.bustype = BUS_I2C;
> +	input_dev->dev.parent = dev;
> +	input_dev->id.vendor = 0x56a;
> +	input_dev->id.version = wacom_data->fw_version;
> +	input_dev->open = wacom_w9000_open;
> +	input_dev->close = wacom_w9000_close;
> +
> +	__set_bit(EV_KEY, input_dev->evbit);
> +	__set_bit(EV_ABS, input_dev->evbit);
> +	__set_bit(BTN_TOUCH, input_dev->keybit);
> +	__set_bit(BTN_TOOL_PEN, input_dev->keybit);
> +	__set_bit(BTN_TOOL_RUBBER, input_dev->keybit);
> +	__set_bit(BTN_STYLUS, input_dev->keybit);
> +
> +	// Calculate x and y resolution from size in devicetree
> +	ret = device_property_read_u32(dev, "touchscreen-x-mm", &val);
> +	if (ret)
> +		input_abs_set_res(input_dev, ABS_X, 100);
> +	else
> +		input_abs_set_res(input_dev, ABS_X, wacom_data->prop.max_x / val);
> +	ret = device_property_read_u32(dev, "touchscreen-y-mm", &val);
> +	if (ret)
> +		input_abs_set_res(input_dev, ABS_Y, 100);
> +	else
> +		input_abs_set_res(input_dev, ABS_Y, wacom_data->prop.max_y / val);
> +
> +	input_set_abs_params(input_dev, ABS_X, 0, wacom_data->prop.max_x, 4, 0);
> +	input_set_abs_params(input_dev, ABS_Y, 0, wacom_data->prop.max_y, 4, 0);
> +	input_set_abs_params(input_dev, ABS_PRESSURE, 0, wacom_data->max_pressure, 0, 0);
> +	input_set_abs_params(input_dev, ABS_DISTANCE, 0, 255, 0, 0);
> +
> +	touchscreen_parse_properties(input_dev, false, &wacom_data->prop);
> +
> +	ret = devm_request_threaded_irq(dev, wacom_data->irq, NULL, wacom_w9000_interrupt,
> +					IRQF_ONESHOT | IRQF_NO_AUTOEN, client->name, wacom_data);
> +	if (ret) {
> +		dev_err(dev, "Failed to register interrupt\n");
> +		goto err_disable_regulators;
> +	}
> +
> +	if (wacom_data->pen_detect_gpio) {
> +		wacom_data->pen_detect_irq = gpiod_to_irq(wacom_data->pen_detect_gpio);

Why is this a GPIO? Your binding said this is GPIO, your code says this
is an interrupt.

> +		ret = devm_request_threaded_irq(dev, wacom_data->pen_detect_irq, NULL,
> +						wacom_w9000_interrupt_pen_detect, IRQF_ONESHOT |
> +						IRQF_NO_AUTOEN | IRQF_TRIGGER_RISING |
> +						IRQF_TRIGGER_FALLING, "wacom_pdct", wacom_data);
> +		if (ret) {
> +			dev_err(dev, "Failed to register pdct interrupt\n");
> +			goto err_disable_regulators;
> +		}
> +	}
> +
> +	if (wacom_data->pen_inserted_gpio) {
> +		input_dev->evbit[0] |= BIT_MASK(EV_SW);
> +		input_set_capability(input_dev, EV_SW, SW_PEN_INSERTED);
> +		wacom_data->pen_insert_irq = gpiod_to_irq(wacom_data->pen_inserted_gpio);

Same question here.

> +		ret = devm_request_threaded_irq(dev, wacom_data->pen_insert_irq, NULL,
> +						wacom_w9000_interrupt_pen_insert, IRQF_ONESHOT |
> +						IRQF_NO_AUTOEN | IRQF_TRIGGER_RISING |
> +						IRQF_TRIGGER_FALLING, "wacom_pen_insert",
> +						wacom_data);
> +		if (ret) {
> +			dev_err(dev, "Failed to register pen-insert interrupt\n");
> +			goto err_disable_regulators;
> +		}
> +	}
> +
> +	input_set_drvdata(input_dev, wacom_data);
> +
> +	wacom_data->pen_inserted = gpiod_get_value(wacom_data->pen_inserted_gpio);
> +	if (wacom_data->pen_inserted)
> +		regulator_disable(wacom_data->regulator);
> +	else
> +		enable_irq(wacom_data->irq);
> +
> +	input_report_switch(wacom_data->input_dev, SW_PEN_INSERTED, wacom_data->pen_inserted);
> +	input_sync(wacom_data->input_dev);
> +
> +	if (wacom_data->pen_inserted_gpio)
> +		enable_irq(wacom_data->pen_insert_irq);
> +
> +	if (wacom_data->pen_detect_gpio)
> +		enable_irq(wacom_data->pen_detect_irq);
> +
> +	ret = input_register_device(wacom_data->input_dev);
> +	if (ret) {
> +		dev_err(dev, "Failed to register input device: %d\n", ret);
> +		goto err_disable_regulators;
> +	}
> +
> +	return 0;
> +
> +err_disable_regulators:
> +	regulator_disable(wacom_data->regulator);
> +	return ret;
> +}
> +
> +static void wacom_w9000_remove(struct i2c_client *client)
> +{
> +	struct wacom_w9000_data *wacom_data = i2c_get_clientdata(client);
> +
> +	if (regulator_is_enabled(wacom_data->regulator))
> +		regulator_disable(wacom_data->regulator);
> +}
> +
> +static int wacom_w9000_suspend(struct device *dev)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct wacom_w9000_data *wacom_data = i2c_get_clientdata(client);
> +	struct input_dev *input_dev = wacom_data->input_dev;
> +
> +	mutex_lock(&input_dev->mutex);
> +
> +	if (regulator_is_enabled(wacom_data->regulator)) {
> +		disable_irq(wacom_data->irq);
> +		regulator_disable(wacom_data->regulator);
> +	}
> +
> +	mutex_unlock(&input_dev->mutex);
> +
> +	return 0;
> +}
> +
> +static int wacom_w9000_resume(struct device *dev)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct wacom_w9000_data *wacom_data = i2c_get_clientdata(client);
> +	struct input_dev *input_dev = wacom_data->input_dev;
> +	int ret = 0;
> +
> +	mutex_lock(&input_dev->mutex);
> +
> +	if (!wacom_data->pen_inserted && !regulator_is_enabled(wacom_data->regulator)) {
> +		ret = regulator_enable(wacom_data->regulator);
> +		if (ret) {
> +			dev_err(&wacom_data->client->dev, "Failed to enable regulators: %d\n",
> +				ret);
> +		} else {
> +			msleep(200);
> +			enable_irq(wacom_data->irq);
> +		}
> +	}
> +
> +	mutex_unlock(&input_dev->mutex);
> +
> +	return ret;
> +}
> +
> +static DEFINE_SIMPLE_DEV_PM_OPS(wacom_w9000_pm, wacom_w9000_suspend, wacom_w9000_resume);
> +
> +static const struct i2c_device_id wacom_w9000_id[] = {
> +	{ "w9007a_lt03" },
> +	{ "w9007a_v1" },

This does not match your OF table.

> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, wacom_w9000_id);
> +
> +static struct i2c_driver wacom_w9000_driver = {
> +	.driver = {
> +		.name	= "wacom_w9000",
> +		.of_match_table = wacom_w9000_of_match,
> +		.pm	= pm_sleep_ptr(&wacom_w9000_pm),
> +	},
> +	.probe		= wacom_w9000_probe,
> +	.remove		= wacom_w9000_remove,
> +	.id_table	= wacom_w9000_id,
> +};
> +module_i2c_driver(wacom_w9000_driver);
> +
> +/* Module information */
> +MODULE_AUTHOR("Hendrik Noack <hendrik-noack@....de>");
> +MODULE_DESCRIPTION("Wacom W9000-series penabled touchscreen driver");
> +MODULE_LICENSE("GPL");
> -- 
> 2.43.0
> 

Powered by blists - more mailing lists