Received-SPF: pass (google.com: domain of linux-kernel+bounces-169764-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Message-ID: <6362e889-7df2-4c61-8ad5-bfe199e451ec@redhat.com>
Date: Mon, 6 May 2024 14:03:13 +0200
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] Input: goodix-berlin - Add sysfs interface for reading
 and writing touch IC registers
To: Charles Wang <charles.goodix@gmail.com>, hadess@hadess.net,
 dmitry.torokhov@gmail.com, Richard Hughes <hughsient@gmail.com>
Cc: linux-input@vger.kernel.org, linux-kernel@vger.kernel.org,
 neil.armstrong@linaro.org
References: <20240506114752.47204-1-charles.goodix@gmail.com>
Content-Language: en-US
From: Hans de Goede <hdegoede@redhat.com>
In-Reply-To: <20240506114752.47204-1-charles.goodix@gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit

Hi,

On 5/6/24 1:47 PM, Charles Wang wrote:
> Export a sysfs interface that would allow reading and writing touchscreen
> IC registers. With this interface many things can be done in usersapce
> such as firmware updates. An example tool that utilizes this interface
> for performing firmware updates can be found at [1].

I'm not sure if I'm a fan of adding an interface to export raw register
access for fwupdate.

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/input/touchscreen/goodix_fwupload.c

Contains update support for older goodix touchscreens and it is not
that big. I think it might be better to just have an in kernel fwupdate
driver for this and have a sysfs file to which to write the new firmware.

Adding Richard Hughes, fwupd maintainer to the Cc. Richard, do you have
any input on the suggested method for firmware updating ?

If raw register access is seen as a good solution, then I think this
should use regmap + some generic helpers (to be written) to export
regmap r/w access to userspace.

> [1] https://github.com/goodix/fwupdate_for_berlin_linux

Hmm, that tool seems to have some licensing issues there is an Apache License v2.0
LICENSE file, but the header of fwupdate.c claims it is confidential ...

Regards,

Hans


> Signed-off-by: Charles Wang <charles.goodix@gmail.com>
> ---
>  drivers/input/touchscreen/goodix_berlin.h     |  2 +
>  .../input/touchscreen/goodix_berlin_core.c    | 52 +++++++++++++++++++
>  drivers/input/touchscreen/goodix_berlin_i2c.c |  6 +++
>  drivers/input/touchscreen/goodix_berlin_spi.c |  6 +++
>  4 files changed, 66 insertions(+)
> 
> diff --git a/drivers/input/touchscreen/goodix_berlin.h b/drivers/input/touchscreen/goodix_berlin.h
> index 1fd77eb69..1741f2d15 100644
> --- a/drivers/input/touchscreen/goodix_berlin.h
> +++ b/drivers/input/touchscreen/goodix_berlin.h
> @@ -19,6 +19,8 @@ struct regmap;
>  int goodix_berlin_probe(struct device *dev, int irq, const struct input_id *id,
>  			struct regmap *regmap);
>  
> +void goodix_berlin_remove(struct device *dev);
> +
>  extern const struct dev_pm_ops goodix_berlin_pm_ops;
>  
>  #endif
> diff --git a/drivers/input/touchscreen/goodix_berlin_core.c b/drivers/input/touchscreen/goodix_berlin_core.c
> index e7b41a926..e02160841 100644
> --- a/drivers/input/touchscreen/goodix_berlin_core.c
> +++ b/drivers/input/touchscreen/goodix_berlin_core.c
> @@ -672,6 +672,44 @@ static void goodix_berlin_power_off_act(void *data)
>  	goodix_berlin_power_off(cd);
>  }
>  
> +static ssize_t goodix_berlin_registers_read(struct file *filp, struct kobject *kobj,
> +	struct bin_attribute *bin_attr, char *buf, loff_t off, size_t count)
> +{
> +	struct goodix_berlin_core *cd;
> +	struct device *dev;
> +	int error;
> +
> +	dev = kobj_to_dev(kobj);
> +	cd = dev_get_drvdata(dev);
> +
> +	error = regmap_raw_read(cd->regmap, (unsigned int)off,
> +				buf, count);
> +
> +	return error ? error : count;
> +}
> +
> +static ssize_t goodix_berlin_registers_write(struct file *filp, struct kobject *kobj,
> +	struct bin_attribute *bin_attr, char *buf, loff_t off, size_t count)
> +{
> +	struct goodix_berlin_core *cd;
> +	struct device *dev;
> +	int error;
> +
> +	dev = kobj_to_dev(kobj);
> +	cd = dev_get_drvdata(dev);
> +
> +	error = regmap_raw_write(cd->regmap, (unsigned int)off,
> +				 buf, count);
> +
> +	return error ? error : count;
> +}
> +
> +static struct bin_attribute goodix_berlin_registers_attr = {
> +	.attr = {.name = "registers", .mode = 0600},
> +	.read = goodix_berlin_registers_read,
> +	.write = goodix_berlin_registers_write,
> +};
> +
>  int goodix_berlin_probe(struct device *dev, int irq, const struct input_id *id,
>  			struct regmap *regmap)
>  {
> @@ -743,6 +781,14 @@ int goodix_berlin_probe(struct device *dev, int irq, const struct input_id *id,
>  
>  	dev_set_drvdata(dev, cd);
>  
> +	error = sysfs_create_bin_file(&cd->dev->kobj,
> +				      &goodix_berlin_registers_attr);
> +
> +	if (error) {
> +		dev_err(dev, "unable to create sysfs file, err=%d\n", error);
> +		return error;
> +	}
> +
>  	dev_dbg(dev, "Goodix Berlin %s Touchscreen Controller",
>  		cd->fw_version.patch_pid);
>  
> @@ -750,6 +796,12 @@ int goodix_berlin_probe(struct device *dev, int irq, const struct input_id *id,
>  }
>  EXPORT_SYMBOL_GPL(goodix_berlin_probe);
>  
> +void goodix_berlin_remove(struct device *dev)
> +{
> +	sysfs_remove_bin_file(&dev->kobj, &goodix_berlin_registers_attr);
> +}
> +EXPORT_SYMBOL_GPL(goodix_berlin_remove);
> +
>  MODULE_LICENSE("GPL");
>  MODULE_DESCRIPTION("Goodix Berlin Core Touchscreen driver");
>  MODULE_AUTHOR("Neil Armstrong <neil.armstrong@linaro.org>");
> diff --git a/drivers/input/touchscreen/goodix_berlin_i2c.c b/drivers/input/touchscreen/goodix_berlin_i2c.c
> index 6ed9aa808..35ef21cc8 100644
> --- a/drivers/input/touchscreen/goodix_berlin_i2c.c
> +++ b/drivers/input/touchscreen/goodix_berlin_i2c.c
> @@ -46,6 +46,11 @@ static int goodix_berlin_i2c_probe(struct i2c_client *client)
>  	return 0;
>  }
>  
> +static void goodix_berlin_i2c_remove(struct i2c_client *client)
> +{
> +	goodix_berlin_remove(&client->dev);
> +}
> +
>  static const struct i2c_device_id goodix_berlin_i2c_id[] = {
>  	{ "gt9916", 0 },
>  	{ }
> @@ -66,6 +71,7 @@ static struct i2c_driver goodix_berlin_i2c_driver = {
>  		.pm = pm_sleep_ptr(&goodix_berlin_pm_ops),
>  	},
>  	.probe = goodix_berlin_i2c_probe,
> +	.remove = goodix_berlin_i2c_remove,
>  	.id_table = goodix_berlin_i2c_id,
>  };
>  module_i2c_driver(goodix_berlin_i2c_driver);
> diff --git a/drivers/input/touchscreen/goodix_berlin_spi.c b/drivers/input/touchscreen/goodix_berlin_spi.c
> index 4cc557da0..8ffbe1289 100644
> --- a/drivers/input/touchscreen/goodix_berlin_spi.c
> +++ b/drivers/input/touchscreen/goodix_berlin_spi.c
> @@ -150,6 +150,11 @@ static int goodix_berlin_spi_probe(struct spi_device *spi)
>  	return 0;
>  }
>  
> +static void goodix_berlin_spi_remove(struct spi_device *spi)
> +{
> +	goodix_berlin_remove(&spi->dev);
> +}
> +
>  static const struct spi_device_id goodix_berlin_spi_ids[] = {
>  	{ "gt9916" },
>  	{ },
> @@ -169,6 +174,7 @@ static struct spi_driver goodix_berlin_spi_driver = {
>  		.pm = pm_sleep_ptr(&goodix_berlin_pm_ops),
>  	},
>  	.probe = goodix_berlin_spi_probe,
> +	.remove = goodix_berlin_spi_remove,
>  	.id_table = goodix_berlin_spi_ids,
>  };
>  module_spi_driver(goodix_berlin_spi_driver);