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? > > > [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. -- Dmitry
