diff mbox series

[v4,4/4] HID: i2c-hid: Introduce goodix-i2c-hid as a subclass of i2c-hid

Message ID 20201103172824.v4.4.If41b7d621633b94d56653c6d53f5f89c5274de7b@changeid (mailing list archive)
State Superseded
Delegated to: Jiri Kosina
Headers show
Series HID: i2c-hid: Reorganize to allow supporting goodix,gt7375p | expand

Commit Message

Douglas Anderson Nov. 4, 2020, 1:29 a.m. UTC 
Goodix i2c-hid touchscreens are mostly i2c-hid compliant but have some
special power sequencing requirements, including the need to drive a
reset line during the sequencing.

Let's use the new ability of i2c-hid to have subclasses for power
sequencing to support the first Goodix i2c-hid touchscreen: GT7375P

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

Changes in v4:
- Totally redid based on the new subclass system.

Changes in v3:
- Rework to use subclassing.

 drivers/hid/i2c-hid/Kconfig             |  19 +++-
 drivers/hid/i2c-hid/Makefile            |   1 +
 drivers/hid/i2c-hid/i2c-hid-of-goodix.c | 120 ++++++++++++++++++++++++
 3 files changed, 138 insertions(+), 2 deletions(-)
 create mode 100644 drivers/hid/i2c-hid/i2c-hid-of-goodix.c

Comments

Hans de Goede Nov. 4, 2020, 12:09 p.m. UTC | #1 
Hi,

On 11/4/20 2:29 AM, Douglas Anderson wrote:
> Goodix i2c-hid touchscreens are mostly i2c-hid compliant but have some
> special power sequencing requirements, including the need to drive a
> reset line during the sequencing.
> 
> Let's use the new ability of i2c-hid to have subclasses for power
> sequencing to support the first Goodix i2c-hid touchscreen: GT7375P
> 
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> 
> Changes in v4:
> - Totally redid based on the new subclass system.

Again just my 2 cents, but I'm not sure if this should be an
entirely separate driver, or just something added to the
generic drivers/hid/i2c-hid/i2c-hid-of.c code.

I have no real preference either way, just asking the
question to make sure both options are considered.

Regards,

Hans




> Changes in v3:
> - Rework to use subclassing.
> 
>  drivers/hid/i2c-hid/Kconfig             |  19 +++-
>  drivers/hid/i2c-hid/Makefile            |   1 +
>  drivers/hid/i2c-hid/i2c-hid-of-goodix.c | 120 ++++++++++++++++++++++++
>  3 files changed, 138 insertions(+), 2 deletions(-)
>  create mode 100644 drivers/hid/i2c-hid/i2c-hid-of-goodix.c
> 
> diff --git a/drivers/hid/i2c-hid/Kconfig b/drivers/hid/i2c-hid/Kconfig
> index 819b7521c182..a16c6a69680b 100644
> --- a/drivers/hid/i2c-hid/Kconfig
> +++ b/drivers/hid/i2c-hid/Kconfig
> @@ -32,10 +32,25 @@ config I2C_HID_OF
>  	  will be called i2c-hid-of.  It will also build/depend on the
>  	  module i2c-hid.
>  
> +config I2C_HID_OF_GOODIX
> +	tristate "Driver for Goodix hid-i2c based devices on OF systems"
> +	default n
> +	depends on I2C && INPUT && OF
> +	help
> +	  Say Y here if you want support for Goodix i2c devices that use
> +	  the i2c-hid protocol on Open Firmware (Device Tree)-based
> +	  systems.
> +
> +	  If unsure, say N.
> +
> +	  This support is also available as a module.  If so, the module
> +	  will be called i2c-hid-of-goodix.  It will also build/depend on
> +	  the module i2c-hid.
> +
>  endmenu
>  
>  config I2C_HID_CORE
>  	tristate
> -	default y if I2C_HID_ACPI=y || I2C_HID_OF=y
> -	default m if I2C_HID_ACPI=m || I2C_HID_OF=m
> +	default y if I2C_HID_ACPI=y || I2C_HID_OF=y || I2C_HID_OF_GOODIX=y
> +	default m if I2C_HID_ACPI=m || I2C_HID_OF=m || I2C_HID_OF_GOODIX=m
>  	select HID
> diff --git a/drivers/hid/i2c-hid/Makefile b/drivers/hid/i2c-hid/Makefile
> index 9b4a73446841..302545a771f3 100644
> --- a/drivers/hid/i2c-hid/Makefile
> +++ b/drivers/hid/i2c-hid/Makefile
> @@ -10,3 +10,4 @@ i2c-hid-$(CONFIG_DMI)				+= i2c-hid-dmi-quirks.o
>  
>  obj-$(CONFIG_I2C_HID_ACPI)			+= i2c-hid-acpi.o
>  obj-$(CONFIG_I2C_HID_OF)			+= i2c-hid-of.o
> +obj-$(CONFIG_I2C_HID_OF_GOODIX)			+= i2c-hid-of-goodix.o
> diff --git a/drivers/hid/i2c-hid/i2c-hid-of-goodix.c b/drivers/hid/i2c-hid/i2c-hid-of-goodix.c
> new file mode 100644
> index 000000000000..7b27fd8b7401
> --- /dev/null
> +++ b/drivers/hid/i2c-hid/i2c-hid-of-goodix.c
> @@ -0,0 +1,120 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Driver for Goodix touchscreens that use the i2c-hid protocol.
> + *
> + * Copyright 2020 Google LLC
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/i2c.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/pm.h>
> +#include <linux/regulator/consumer.h>
> +
> +#include "i2c-hid.h"
> +
> +struct goodix_i2c_hid_timing_data {
> +	unsigned int post_gpio_reset_delay_ms;
> +	unsigned int post_power_delay_ms;
> +};
> +
> +struct i2c_hid_of_goodix {
> +	struct i2chid_subclass_data subclass;
> +
> +	struct regulator *vdd;
> +	struct gpio_desc *reset_gpio;
> +	const struct goodix_i2c_hid_timing_data *timings;
> +};
> +
> +static int goodix_i2c_hid_power_up_device(struct i2chid_subclass_data *subclass)
> +{
> +	struct i2c_hid_of_goodix *ihid_goodix =
> +		container_of(subclass, struct i2c_hid_of_goodix, subclass);
> +	int ret;
> +
> +	ret = regulator_enable(ihid_goodix->vdd);
> +	if (ret)
> +		return ret;
> +
> +	if (ihid_goodix->timings->post_power_delay_ms)
> +		msleep(ihid_goodix->timings->post_power_delay_ms);
> +
> +	gpiod_set_value_cansleep(ihid_goodix->reset_gpio, 0);
> +	if (ihid_goodix->timings->post_gpio_reset_delay_ms)
> +		msleep(ihid_goodix->timings->post_gpio_reset_delay_ms);
> +
> +	return 0;
> +}
> +
> +static void goodix_i2c_hid_power_down_device(struct i2chid_subclass_data *subclass)
> +{
> +	struct i2c_hid_of_goodix *ihid_goodix =
> +		container_of(subclass, struct i2c_hid_of_goodix, subclass);
> +
> +	gpiod_set_value_cansleep(ihid_goodix->reset_gpio, 1);
> +	regulator_disable(ihid_goodix->vdd);
> +}
> +
> +static int i2c_hid_of_goodix_probe(struct i2c_client *client,
> +				   const struct i2c_device_id *id)
> +{
> +	struct i2c_hid_of_goodix *ihid_goodix;
> +
> +	ihid_goodix = devm_kzalloc(&client->dev, sizeof(*ihid_goodix),
> +				   GFP_KERNEL);
> +	if (!ihid_goodix)
> +		return -ENOMEM;
> +
> +	ihid_goodix->subclass.power_up_device = goodix_i2c_hid_power_up_device;
> +	ihid_goodix->subclass.power_down_device = goodix_i2c_hid_power_down_device;
> +
> +	/* Start out with reset asserted */
> +	ihid_goodix->reset_gpio =
> +		devm_gpiod_get_optional(&client->dev, "reset", GPIOD_OUT_HIGH);
> +	if (IS_ERR(ihid_goodix->reset_gpio))
> +		return PTR_ERR(ihid_goodix->reset_gpio);
> +
> +	ihid_goodix->vdd = devm_regulator_get(&client->dev, "vdd");
> +	if (IS_ERR(ihid_goodix->vdd))
> +		return PTR_ERR(ihid_goodix->vdd);
> +
> +	ihid_goodix->timings = device_get_match_data(&client->dev);
> +
> +	return i2c_hid_core_probe(client, &ihid_goodix->subclass, 0x0001);
> +}
> +
> +static const struct goodix_i2c_hid_timing_data goodix_gt7375p_timing_data = {
> +	.post_power_delay_ms = 10,
> +	.post_gpio_reset_delay_ms = 120,
> +};
> +
> +static const struct of_device_id goodix_i2c_hid_of_match[] = {
> +	{ .compatible = "goodix,gt7375p", .data = &goodix_gt7375p_timing_data },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, goodix_i2c_hid_of_match);
> +
> +static const struct dev_pm_ops goodix_i2c_hid_pm = {
> +	SET_SYSTEM_SLEEP_PM_OPS(i2c_hid_core_suspend, i2c_hid_core_resume)
> +};
> +
> +static struct i2c_driver goodix_i2c_hid_ts_driver = {
> +	.driver = {
> +		.name	= "i2c_hid_of_goodix",
> +		.pm	= &goodix_i2c_hid_pm,
> +		.probe_type = PROBE_PREFER_ASYNCHRONOUS,
> +		.of_match_table = of_match_ptr(goodix_i2c_hid_of_match),
> +	},
> +	.probe		= i2c_hid_of_goodix_probe,
> +	.remove		= i2c_hid_core_remove,
> +	.shutdown	= i2c_hid_core_shutdown,
> +};
> +module_i2c_driver(goodix_i2c_hid_ts_driver);
> +
> +MODULE_AUTHOR("Douglas Anderson <dianders@chromium.org>");
> +MODULE_DESCRIPTION("Goodix i2c-hid touchscreen driver");
> +MODULE_LICENSE("GPL v2");
>
Douglas Anderson Nov. 4, 2020, 4:10 p.m. UTC | #2 
Hi,

On Wed, Nov 4, 2020 at 4:09 AM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi,
>
> On 11/4/20 2:29 AM, Douglas Anderson wrote:
> > Goodix i2c-hid touchscreens are mostly i2c-hid compliant but have some
> > special power sequencing requirements, including the need to drive a
> > reset line during the sequencing.
> >
> > Let's use the new ability of i2c-hid to have subclasses for power
> > sequencing to support the first Goodix i2c-hid touchscreen: GT7375P
> >
> > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > ---
> >
> > Changes in v4:
> > - Totally redid based on the new subclass system.
>
> Again just my 2 cents, but I'm not sure if this should be an
> entirely separate driver, or just something added to the
> generic drivers/hid/i2c-hid/i2c-hid-of.c code.
>
> I have no real preference either way, just asking the
> question to make sure both options are considered.

Yeah, I thought about it.  ...but at the moment I'm not convinced it's
really any cleaner and I think there's very little shared code.  In
the goodix case, I don't want to specify the extra regulator or the
timings or even the hid descriptor address.  In the non-goodix case I
don't want the goodix properties.  It also sounded as if Benjamin's
preferred solutions involved having two separate files.  I'll wait for
Benjamin's feedback here, though, and if he wants me to combine them
then I'll give it a shot for v5.

-Doug

-Doug
diff mbox series

Patch

diff --git a/drivers/hid/i2c-hid/Kconfig b/drivers/hid/i2c-hid/Kconfig
index 819b7521c182..a16c6a69680b 100644
--- a/drivers/hid/i2c-hid/Kconfig
+++ b/drivers/hid/i2c-hid/Kconfig
@@ -32,10 +32,25 @@  config I2C_HID_OF
 	  will be called i2c-hid-of.  It will also build/depend on the
 	  module i2c-hid.
 
+config I2C_HID_OF_GOODIX
+	tristate "Driver for Goodix hid-i2c based devices on OF systems"
+	default n
+	depends on I2C && INPUT && OF
+	help
+	  Say Y here if you want support for Goodix i2c devices that use
+	  the i2c-hid protocol on Open Firmware (Device Tree)-based
+	  systems.
+
+	  If unsure, say N.
+
+	  This support is also available as a module.  If so, the module
+	  will be called i2c-hid-of-goodix.  It will also build/depend on
+	  the module i2c-hid.
+
 endmenu
 
 config I2C_HID_CORE
 	tristate
-	default y if I2C_HID_ACPI=y || I2C_HID_OF=y
-	default m if I2C_HID_ACPI=m || I2C_HID_OF=m
+	default y if I2C_HID_ACPI=y || I2C_HID_OF=y || I2C_HID_OF_GOODIX=y
+	default m if I2C_HID_ACPI=m || I2C_HID_OF=m || I2C_HID_OF_GOODIX=m
 	select HID
diff --git a/drivers/hid/i2c-hid/Makefile b/drivers/hid/i2c-hid/Makefile
index 9b4a73446841..302545a771f3 100644
--- a/drivers/hid/i2c-hid/Makefile
+++ b/drivers/hid/i2c-hid/Makefile
@@ -10,3 +10,4 @@  i2c-hid-$(CONFIG_DMI)				+= i2c-hid-dmi-quirks.o
 
 obj-$(CONFIG_I2C_HID_ACPI)			+= i2c-hid-acpi.o
 obj-$(CONFIG_I2C_HID_OF)			+= i2c-hid-of.o
+obj-$(CONFIG_I2C_HID_OF_GOODIX)			+= i2c-hid-of-goodix.o
diff --git a/drivers/hid/i2c-hid/i2c-hid-of-goodix.c b/drivers/hid/i2c-hid/i2c-hid-of-goodix.c
new file mode 100644
index 000000000000..7b27fd8b7401
--- /dev/null
+++ b/drivers/hid/i2c-hid/i2c-hid-of-goodix.c
@@ -0,0 +1,120 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Driver for Goodix touchscreens that use the i2c-hid protocol.
+ *
+ * Copyright 2020 Google LLC
+ */
+
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/gpio/consumer.h>
+#include <linux/i2c.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/pm.h>
+#include <linux/regulator/consumer.h>
+
+#include "i2c-hid.h"
+
+struct goodix_i2c_hid_timing_data {
+	unsigned int post_gpio_reset_delay_ms;
+	unsigned int post_power_delay_ms;
+};
+
+struct i2c_hid_of_goodix {
+	struct i2chid_subclass_data subclass;
+
+	struct regulator *vdd;
+	struct gpio_desc *reset_gpio;
+	const struct goodix_i2c_hid_timing_data *timings;
+};
+
+static int goodix_i2c_hid_power_up_device(struct i2chid_subclass_data *subclass)
+{
+	struct i2c_hid_of_goodix *ihid_goodix =
+		container_of(subclass, struct i2c_hid_of_goodix, subclass);
+	int ret;
+
+	ret = regulator_enable(ihid_goodix->vdd);
+	if (ret)
+		return ret;
+
+	if (ihid_goodix->timings->post_power_delay_ms)
+		msleep(ihid_goodix->timings->post_power_delay_ms);
+
+	gpiod_set_value_cansleep(ihid_goodix->reset_gpio, 0);
+	if (ihid_goodix->timings->post_gpio_reset_delay_ms)
+		msleep(ihid_goodix->timings->post_gpio_reset_delay_ms);
+
+	return 0;
+}
+
+static void goodix_i2c_hid_power_down_device(struct i2chid_subclass_data *subclass)
+{
+	struct i2c_hid_of_goodix *ihid_goodix =
+		container_of(subclass, struct i2c_hid_of_goodix, subclass);
+
+	gpiod_set_value_cansleep(ihid_goodix->reset_gpio, 1);
+	regulator_disable(ihid_goodix->vdd);
+}
+
+static int i2c_hid_of_goodix_probe(struct i2c_client *client,
+				   const struct i2c_device_id *id)
+{
+	struct i2c_hid_of_goodix *ihid_goodix;
+
+	ihid_goodix = devm_kzalloc(&client->dev, sizeof(*ihid_goodix),
+				   GFP_KERNEL);
+	if (!ihid_goodix)
+		return -ENOMEM;
+
+	ihid_goodix->subclass.power_up_device = goodix_i2c_hid_power_up_device;
+	ihid_goodix->subclass.power_down_device = goodix_i2c_hid_power_down_device;
+
+	/* Start out with reset asserted */
+	ihid_goodix->reset_gpio =
+		devm_gpiod_get_optional(&client->dev, "reset", GPIOD_OUT_HIGH);
+	if (IS_ERR(ihid_goodix->reset_gpio))
+		return PTR_ERR(ihid_goodix->reset_gpio);
+
+	ihid_goodix->vdd = devm_regulator_get(&client->dev, "vdd");
+	if (IS_ERR(ihid_goodix->vdd))
+		return PTR_ERR(ihid_goodix->vdd);
+
+	ihid_goodix->timings = device_get_match_data(&client->dev);
+
+	return i2c_hid_core_probe(client, &ihid_goodix->subclass, 0x0001);
+}
+
+static const struct goodix_i2c_hid_timing_data goodix_gt7375p_timing_data = {
+	.post_power_delay_ms = 10,
+	.post_gpio_reset_delay_ms = 120,
+};
+
+static const struct of_device_id goodix_i2c_hid_of_match[] = {
+	{ .compatible = "goodix,gt7375p", .data = &goodix_gt7375p_timing_data },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, goodix_i2c_hid_of_match);
+
+static const struct dev_pm_ops goodix_i2c_hid_pm = {
+	SET_SYSTEM_SLEEP_PM_OPS(i2c_hid_core_suspend, i2c_hid_core_resume)
+};
+
+static struct i2c_driver goodix_i2c_hid_ts_driver = {
+	.driver = {
+		.name	= "i2c_hid_of_goodix",
+		.pm	= &goodix_i2c_hid_pm,
+		.probe_type = PROBE_PREFER_ASYNCHRONOUS,
+		.of_match_table = of_match_ptr(goodix_i2c_hid_of_match),
+	},
+	.probe		= i2c_hid_of_goodix_probe,
+	.remove		= i2c_hid_core_remove,
+	.shutdown	= i2c_hid_core_shutdown,
+};
+module_i2c_driver(goodix_i2c_hid_ts_driver);
+
+MODULE_AUTHOR("Douglas Anderson <dianders@chromium.org>");
+MODULE_DESCRIPTION("Goodix i2c-hid touchscreen driver");
+MODULE_LICENSE("GPL v2");