Re: [PATCH 2/2] Input: Add support for Wacom W9000-series penabled touchscreens

[Kernel GMX <hendrik-noack@gmx.de>]

28.10.2025 08:44:34 Krzysztof Kozlowski <krzk@kernel.org>:

> 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.
>

Okay, I was orienting myself on the atmel_mxt_ts driver and it had it like this.

>> +   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.

You are right, it's not necessary here.

>> +       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.
>

The driver needs to know the GPIO state to determine if the pen is inserted
so that it can enable or disable the regulator and main interrupt accordingly.
An interrupt for this GPIO is necessary for this evaluation to occur
on every signal change.

>> +       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;
>> +       }
>> +   }
>> +

...

Thank you for you feedback, I will address the feedback in my next iteration.

Best regards,
Hendrik