Hi, On 5/7/24 4:13 AM, 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, Ok. > especially if in > cases where firmware flashing is not essential for device to work (i.e. > it the controller has a flash memory). Right the existing older goodix fw-upload is different because some controllers are flash-less so they need a fw upload every boot. > 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? Right when I was talking about generic helpers that was meant for code re-use purposes. Actually exposing the regmap r/w functionality to userspace is something which should be decided on a case by case basis by the driver (IMHO). Regards, Hans > >> >>> [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. >
