Re: [PATCH] Input: goodix-berlin - Add sysfs interface for reading and writing touch IC registers

From: Hans de Goede
Date: Mon May 06 2024 - 08:03:28 EST


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@xxxxxxxxx>
> ---
> 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@xxxxxxxxxx>");
> 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);