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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 07/05/2024 04:13, Dmitry Torokhov wrote:
On Mon, May 06, 2024 at 02:03:13PM +0200, Hans de Goede wrote:
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.

I think the less code we have in kernel the better, especially if in
cases where firmware flashing is not essential for device to work (i.e.
it the controller has a flash memory). That said IIRC Mark felt very
strongly about allowing regmap writes from userspace... but maybe he
softened the stance or we could have this functionality opt-in?

The update code is quite ugly, but we could probably limit the functionnality
and filtering the registers read/write to only the update registers.

Neil



[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 we want to instantiate attributes from the driver please utilize
dev_groups in respective driver structures to create and remove the
attributes automatically.

Thanks.






[Index of Archives]     [Linux Media Devel]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Linux Wireless Networking]     [Linux Omap]

  Powered by Linux