Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 3 Jan 2023 11:27:56 +0300
From:   Dan Carpenter <error27@gmail.com>
To:     Joshua Goins <josh@redstrate.com>
Cc:     linux-input@vger.kernel.org, jose.exposito89@gmail.com,
        kernel test robot <lkp@intel.com>,
        Jiri Kosina <jikos@kernel.org>,
        Benjamin Tissoires <benjamin.tissoires@redhat.com>,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] HID: uclogic: Add support for XP-PEN Artist 22R Pro
Message-ID: <Y7PnDMx1ztWgfA8W@kadam>
References: <2068502.VLH7GnMWUR@adrastea>
 <20230102194911.56083-1-josh@redstrate.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230102194911.56083-1-josh@redstrate.com>
Precedence: bulk

On Mon, Jan 02, 2023 at 02:49:10PM -0500, Joshua Goins wrote:
> @@ -91,9 +115,27 @@ static int uclogic_input_mapping(struct hid_device *hdev,
>  	struct uclogic_drvdata *drvdata = hid_get_drvdata(hdev);
>  	struct uclogic_params *params = &drvdata->params;
>  
> -	/* Discard invalid pen usages */
> -	if (params->pen.usage_invalid && (field->application == HID_DG_PEN))
> -		return -1;
> +	if (field->application == HID_GD_KEYPAD) {
> +		/*
> +		 * Remap input buttons to sensible ones that are not invalid.
> +		 * This only affects previous behavior for devices with more than ten or so buttons.
> +		 */
> +		const int key = (usage->hid & HID_USAGE) - 1;
> +
> +		if (key > 0 && key < ARRAY_SIZE(uclogic_extra_input_mapping)) {

It's an unusual that zero is not valid but I don't know the code at all.

> +			hid_map_usage(hi,
> +				      usage,
> +				      bit,
> +				      max,
> +				      EV_KEY,
> +				      uclogic_extra_input_mapping[key]);
> +			return 1;
> +		}
> +	} else if (field->application == HID_DG_PEN) {
> +		/* Discard invalid pen usages */
> +		if (params->pen.usage_invalid)
> +			return -1;
> +	}
>  
>  	/* Let hid-core decide what to do */
>  	return 0;

[ snip ]

> +/*
> + * uclogic_params_init_ugee_xppen_pro() - Initializes a UGEE XP-Pen Pro tablet device.
> + *
> + * @hdev:	The HID device of the tablet interface to initialize and get
> + *		parameters from. Cannot be NULL.
> + * @params:	Parameters to fill in (to be cleaned with
> + *		uclogic_params_cleanup()). Not modified in case of error.
> + *		Cannot be NULL.
> + *
> + * Returns:
> + *	Zero, if successful. A negative errno code on error.
> + */
> +static int uclogic_params_init_ugee_xppen_pro(struct uclogic_params *params,
> +					      struct hid_device *hdev,
> +					      const u8 rdesc_frame_arr[],
> +					      const size_t rdesc_frame_size)
> +{
> +	int rc = 0;
> +	struct usb_interface *iface;
> +	__u8 bInterfaceNumber;
> +	const int str_desc_len = 12;
> +	u8 *str_desc = NULL;
> +	__u8 *rdesc_pen = NULL;
> +	s32 desc_params[UCLOGIC_RDESC_PH_ID_NUM];
> +	/* The resulting parameters (noop) */
> +	struct uclogic_params p = {0, };
> +
> +	if (!hdev || !params) {
> +		rc = -EINVAL;
> +		goto cleanup;
> +	}
> +
> +	iface = to_usb_interface(hdev->dev.parent);
> +	bInterfaceNumber = iface->cur_altsetting->desc.bInterfaceNumber;
> +
> +	/* Ignore non-pen interfaces */
> +	if (bInterfaceNumber != 2) {
> +		uclogic_params_init_invalid(&p);
> +		goto output;

So this is a success path?  "ret = 0?"  The comments kind of suggest
that but I want to be sure.

> +	}
> +
> +	/*
> +	 * Initialize the interface by sending magic data.
> +	 * This magic data is the same as other UGEE v2 tablets.
> +	 */
> +	rc = uclogic_probe_interface(hdev,
> +				     uclogic_ugee_v2_probe_arr,
> +				     uclogic_ugee_v2_probe_size,
> +				     0x03);
> +	if (rc) {
> +		uclogic_params_init_invalid(&p);
> +		goto output;
> +	}
> +
> +	/**
> +	 * Read the string descriptor containing pen and frame parameters.
> +	 * These are slightly different than typical UGEE v2 devices.
> +	 */
> +	rc = uclogic_params_get_str_desc(&str_desc, hdev, 100, str_desc_len);
> +	if (rc != str_desc_len) {
> +		hid_err(hdev, "failed retrieving pen and frame parameters: %d\n", rc);
> +		uclogic_params_init_invalid(&p);
> +		goto output;

This isn't correct.  You need to do something like:

		rc = (rc < 0) ? rc : -EINVAL;

regards,
dan carpenter

> +	}
> +
> +	rc = uclogic_params_parse_ugee_xppen_pro_desc(str_desc, str_desc_len,
> +						      desc_params,
> +						      ARRAY_SIZE(desc_params));
> +	if (rc)
> +		goto cleanup;
> +
> +	kfree(str_desc);
> +	str_desc = NULL;
> +
> +	/* Initialize the pen interface */
> +	rdesc_pen = uclogic_rdesc_template_apply(
> +				uclogic_rdesc_ugee_v2_pen_template_arr,
> +				uclogic_rdesc_ugee_v2_pen_template_size,
> +				desc_params, ARRAY_SIZE(desc_params));
> +	if (!rdesc_pen) {
> +		rc = -ENOMEM;
> +		goto cleanup;
> +	}
> +
> +	p.pen.desc_ptr = rdesc_pen;
> +	p.pen.desc_size = uclogic_rdesc_ugee_v2_pen_template_size;
> +	p.pen.id = 0x02;
> +	p.pen.subreport_list[0].value = 0xf0;
> +	p.pen.subreport_list[0].id = UCLOGIC_RDESC_V1_FRAME_ID;
> +
> +	/* Initialize the frame interface */
> +	rc = uclogic_params_frame_init_with_desc(
> +		&p.frame_list[0],
> +		rdesc_frame_arr,
> +		rdesc_frame_size,
> +		UCLOGIC_RDESC_V1_FRAME_ID);
> +	if (rc < 0) {
> +		hid_err(hdev, "initializing frame params failed: %d\n", rc);
> +		goto output;
> +	}
> +
> +	p.frame_list[0].bitmap_dial_byte = 7;
> +	p.frame_list[0].bitmap_second_dial_destination_byte = 8;
> +
> +output:
> +	/* Output parameters */
> +	memcpy(params, &p, sizeof(*params));
> +	memset(&p, 0, sizeof(p));
> +	rc = 0;
> +cleanup:
> +	kfree(str_desc);
> +	uclogic_params_cleanup(&p);
> +	return rc;
> +}