Re: [PATCH v3 2/4] HID: touchscreen: Add initial support for Himax HID-over-SPI

From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
To: Tylor Yang <tylor_yang@himax.corp-partner.google.com>,
	dmitry.torokhov@gmail.com, robh+dt@kernel.org,
	krzysztof.kozlowski+dt@linaro.org, conor+dt@kernel.org,
	jikos@kernel.org, benjamin.tissoires@redhat.com,
	linux-input@vger.kernel.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org
Cc: poyuan_chang@himax.corp-partner.google.com,
	jingyliang@chromium.org, hbarnor@chromium.org, wuxy23@lenovo.com,
	luolm1@lenovo.com, poyu_hung@himax.corp-partner.google.com
Subject: Re: [PATCH v3 2/4] HID: touchscreen: Add initial support for Himax HID-over-SPI
Date: Tue, 17 Oct 2023 19:01:22 +0200	[thread overview]
Message-ID: <dd47ff4f-bea8-4091-b572-5bef4aa187d3@linaro.org> (raw)
In-Reply-To: <20231017091900.801989-3-tylor_yang@himax.corp-partner.google.com>

On 17/10/2023 11:18, Tylor Yang wrote:
> The hx83102j is a TDDI IC (Touch with Display Driver). The
> IC using SPI to transferring HID packet to host CPU. The IC also
> report HID report descriptor for driver to register HID device.
> The driver is designed as a framework for future expansion and
> hx83102j is the first case. Each hx_spi_hid_hx8xxxxx modules are
> mutual exclusive, it should be initiate one at a time.
> 
> This driver takes a position similar to i2c-hid, it initialize
> and control the touch IC below and register HID to upper hid-core.
> When touch ic report an interrupt, it receive the data from IC
> and report as HID input to hid-core. Let hid-core dispatch input
> to registered hid-protocol and report to related input sub-system.
> 
> This driver also provide advanced functions by hidraw interface:
> - runtime firmware update
> - debug functions, such as reg r/w
> - self test for touch panel
> 
> Due to patch size is too big, separate into 3 part. This is part 1.
> 
> Signed-off-by: Tylor Yang <tylor_yang@himax.corp-partner.google.com>
> ---
>  MAINTAINERS                       |    1 +
>  drivers/hid/hx-hid/hx_acpi.c      |   81 ++
>  drivers/hid/hx-hid/hx_core.c      | 1605 +++++++++++++++++++++++++++++
>  drivers/hid/hx-hid/hx_core.h      |  489 +++++++++
>  drivers/hid/hx-hid/hx_hid.c       |  753 ++++++++++++++
>  drivers/hid/hx-hid/hx_hid.h       |   96 ++
>  drivers/hid/hx-hid/hx_ic_83102j.c |  340 ++++++
>  drivers/hid/hx-hid/hx_ic_83102j.h |   42 +
>  8 files changed, 3407 insertions(+)
>  create mode 100644 drivers/hid/hx-hid/hx_acpi.c
>  create mode 100644 drivers/hid/hx-hid/hx_core.c
>  create mode 100644 drivers/hid/hx-hid/hx_core.h
>  create mode 100644 drivers/hid/hx-hid/hx_hid.c
>  create mode 100644 drivers/hid/hx-hid/hx_hid.h
>  create mode 100644 drivers/hid/hx-hid/hx_ic_83102j.c
>  create mode 100644 drivers/hid/hx-hid/hx_ic_83102j.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 883870ab316f..95ea8159eced 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -9345,6 +9345,7 @@ M:	Tylor Yang <tylor_yang@himax.corp-partner.google.com>
>  L:	linux-input@vger.kernel.org
>  S:	Supported
>  F:	Documentation/devicetree/bindings/input/himax,hid.yaml
> +F:	drivers/hid/hx-hid/
>  
>  HIMAX HX83112B TOUCHSCREEN SUPPORT
>  M:	Job Noorman <job@noorman.info>
> diff --git a/drivers/hid/hx-hid/hx_acpi.c b/drivers/hid/hx-hid/hx_acpi.c
> new file mode 100644
> index 000000000000..2dc7c611a61a
> --- /dev/null
> +++ b/drivers/hid/hx-hid/hx_acpi.c
> @@ -0,0 +1,81 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*  Himax Driver Code for Common IC to simulate HID
> + *
> + *  Copyright (C) 2023 Himax Corporation.
> + *
> + *  This software is licensed under the terms of the GNU General Public
> + *  License version 2,  as published by the Free Software Foundation,  and
> + *  may be copied,  distributed,  and modified under those terms.
> + *
> + *  This program is distributed in the hope that it will be useful,
> + *   but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *  GNU General Public License for more details.

Drop boiler plate. It's gone since some years. Having it here suggests
you just push downstream crappy code. Please don't. Start from scratch
taking existing driver.

> + */
> +
> +#include "hx_core.h"
> +
> +int himax_parse_acpi(struct device *dev,
> +		     struct himax_platform_data *pdata)
> +{
> +	int ret = 0;
> +	struct gpio_desc *desc;
> +	const u32 interrupt_pin_idx = 0;
> +	// const u32 reset_pin_idx = 1;
> +	const char *interrupt_pin_dsd_name = "irq"; // to name "irq-gpios"
> +	const char *reset_pin_dsd_name = "reset"; // to name "reset-gpios"

This style, dead code, comments is not a Linux coding style.

> +
> +	D("Entered");

OK, I'll finish review. A lot further looks even worse. This is not code
suitable for inclusion in mainline. Please start from scratch from
existing code and customize it per your needs. This way you will keep
Linux coding style instead introducing some totally different coding
style from downstream, terrible quality driver.

Best regards,
Krzysztof