| Message ID | 20240506114752.47204-1-charles.goodix@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Input: goodix-berlin - Add sysfs interface for reading and writing touch IC registers | expand |
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);
On Mon, 6 May 2024 at 13:03, Hans de Goede <hdegoede@redhat.com> wrote: > Adding Richard Hughes, fwupd maintainer to the Cc. Richard, do you have > any input on the suggested method for firmware updating ? I'm okay with either; it's obviously easier for fwupd to just squirt a file into a sysfs file to update the firmware, but some devices also need something a bit more nuanced -- e.g. putting the device into an update mode, setting the power domains, fallback recovery and stuff like that. Richard
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@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 we want to instantiate attributes from the driver please utilize dev_groups in respective driver structures to create and remove the attributes automatically. Thanks.
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@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 we want to instantiate attributes from the driver please utilize > dev_groups in respective driver structures to create and remove the > attributes automatically. > > Thanks. >
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@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 we want to instantiate attributes from the driver please utilize > dev_groups in respective driver structures to create and remove the > attributes automatically. > > Thanks. >
Hi, On Mon, May 06, 2024 at 02:03:13PM +0200, Hans de Goede wrote: > > [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 ... Thanks for pointing that out. The confidential claims has been removed from fwupdate.c in the latest commits. Thanks, Charles
Hi, On Mon, May 06, 2024 at 07:13:38PM -0700, Dmitry Torokhov wrote: > If we want to instantiate attributes from the driver please utilize > dev_groups in respective driver structures to create and remove the > attributes automatically. Ack Thanks, Charles
Hi, On 5/7/24 2:28 PM, Charles Wang wrote: > Hi, > > On Tue, May 07, 2024 at 10:25:29AM +0200, Hans de Goede wrote: >> 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). > > So what's the final conclusion, does the interface need to be modified? I believe that the final conclusion is that the interface is fine. Personally I think if the syfs store/show functions used for this could be made into generic regmap helpers and then use those helpers in the driver, so that if other drivers want similar functionality they can re-use the show / store functions. You should be able to use dev_get_regmap() to make the show/store functions generic. Regards, Hans >>>>> --- >>>>> 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. >>> >> >
On Mon, May 06, 2024 at 07:13:38PM -0700, Dmitry Torokhov wrote: > On Mon, May 06, 2024 at 02:03:13PM +0200, Hans de Goede wrote: > > 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? I think unmediated raw register access is a terrible idea, you can't safely write a driver if userspace can just go in and randomly write to registers with no coordination with the running driver and for some devices the kernel needs to ensure that any writes don't damage or destabalise the system. If a driver provides an interface that looks like raw register accesses that's of course fine (I mean, a lot of firmware formats basically boil down to register write sequences which is clearly fine) but it should be the driver doing that and it should be looking at what's going on and ensure that it's joined up with the needs of the rest of the system.
Hi all, On Tue, May 07, 2024 at 11:36:41PM +0900, Mark Brown wrote: > On Mon, May 06, 2024 at 07:13:38PM -0700, Dmitry Torokhov wrote: > > On Mon, May 06, 2024 at 02:03:13PM +0200, Hans de Goede wrote: > > > > 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? > > I think unmediated raw register access is a terrible idea, you can't > safely write a driver if userspace can just go in and randomly write to > registers with no coordination with the running driver and for some > devices the kernel needs to ensure that any writes don't damage or > destabalise the system. If a driver provides an interface that looks > like raw register accesses that's of course fine (I mean, a lot of > firmware formats basically boil down to register write sequences which > is clearly fine) but it should be the driver doing that and it should be > looking at what's going on and ensure that it's joined up with the needs > of the rest of the system. I happen to agree here; especially in the case of writing new FW to a flash; this is a very hardware-centric and device-specific function, which by definition belongs in a kernel driver. For example, many devices must be placed in a bootloader mode during the FW update, and may clamp or toggle an interrupt pin during this mode switch. If user space initiates this sequence while the driver is unaware of this process, it may attempt to read status registers from an I2C address that is temporarily offline. A much more common design pattern is for the driver to expose one W/O sysfs attribute for accepting the FW file name, and one R/O attribute for displaying the current FW version in flash. A small script runs at start-up to check the version against what is stored on "disk", and if what is stored in flash is deemed out of date, the script writes to the W/O attribute. This is the extent of user space's involvement. Kind regards, Jeff LaBundy
On Tue, May 07, 2024 at 10:14:28AM -0500, Jeff LaBundy wrote: > Hi all, > > On Tue, May 07, 2024 at 11:36:41PM +0900, Mark Brown wrote: > > On Mon, May 06, 2024 at 07:13:38PM -0700, Dmitry Torokhov wrote: > > > On Mon, May 06, 2024 at 02:03:13PM +0200, Hans de Goede wrote: > > > > > > 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? > > > > I think unmediated raw register access is a terrible idea, you can't > > safely write a driver if userspace can just go in and randomly write to > > registers with no coordination with the running driver and for some > > devices the kernel needs to ensure that any writes don't damage or > > destabalise the system. If a driver provides an interface that looks > > like raw register accesses that's of course fine (I mean, a lot of > > firmware formats basically boil down to register write sequences which > > is clearly fine) but it should be the driver doing that and it should be > > looking at what's going on and ensure that it's joined up with the needs > > of the rest of the system. > > I happen to agree here; especially in the case of writing new FW to a > flash; this is a very hardware-centric and device-specific function, > which by definition belongs in a kernel driver. > > For example, many devices must be placed in a bootloader mode during > the FW update, and may clamp or toggle an interrupt pin during this > mode switch. If user space initiates this sequence while the driver is > unaware of this process, it may attempt to read status registers from > an I2C address that is temporarily offline. And yet we have i2c-dev and hidraw that are often successfully used to flash the firmware, do diagnostics, etc. without encumbering the kernel. They are more likely to work on ACPI systems because such systems have separation between power management and function pieces (whereas on non-ACPI systems power management is crammed into the same driver and it is not possible to properly power up device without wiring up the rest of it). This is something that I feel we will have to fix in the long term. > > A much more common design pattern is for the driver to expose one W/O > sysfs attribute for accepting the FW file name, and one R/O attribute > for displaying the current FW version in flash. A small script runs at > start-up to check the version against what is stored on "disk", and if > what is stored in flash is deemed out of date, the script writes to the > W/O attribute. This is the extent of user space's involvement. This however means that code that is not used 99.9999 percent of the time has to stay in the kernel, occupying precious memory. I agree that if firmware update is very involved and needs precise control of interrupts coming from the device and has certain timing restriction, then in-kernel implementation is preferable, but in many instance userspace updaters work just fine. Thanks.
On Tue, May 07, 2024 at 02:09:59PM -0700, Dmitry Torokhov wrote: > On Tue, May 07, 2024 at 10:14:28AM -0500, Jeff LaBundy wrote: > > For example, many devices must be placed in a bootloader mode during > > the FW update, and may clamp or toggle an interrupt pin during this > > mode switch. If user space initiates this sequence while the driver is > > unaware of this process, it may attempt to read status registers from > > an I2C address that is temporarily offline. > And yet we have i2c-dev and hidraw that are often successfully used to > flash the firmware, do diagnostics, etc. without encumbering the kernel. Yeah, those seem like a reasonable enough model for safer devices - they do the exclusion thing so you don't have a real driver running at the same time. For things like PMICs there's some concerns of course. The other model I've seen used BTW is to expose a MTD device, if the device actually looks like a MTD device (perhaps even is just a flash that's fairly directly exposed) that minimises the kernel code quite well.
On Wed, 8 May 2024 at 03:37, Mark Brown <broonie@kernel.org> wrote: > The other model I've seen used BTW is to expose a MTD device, if the > device actually looks like a MTD device (perhaps even is just a flash > that's fairly directly exposed) that minimises the kernel code quite > well. If it helps, fwupd already uses mtd for other devices too, although at the moment we're using it only for system firmware -- e.g. intel-spi style. The MTD subsystem doesn't give fwupd much info about the {removable} device itself, and that can pose a problem unless you start using heuristics about the parent device to match firmware to the mtd device. Richard.
On Wed, May 08, 2024 at 09:39:41AM +0100, Richard Hughes wrote: > If it helps, fwupd already uses mtd for other devices too, although at > the moment we're using it only for system firmware -- e.g. intel-spi > style. The MTD subsystem doesn't give fwupd much info about the > {removable} device itself, and that can pose a problem unless you > start using heuristics about the parent device to match firmware to > the mtd device. FWIW I know SolareFlare network cards used to do this too, though I don't know about current products.
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);
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]. [1] https://github.com/goodix/fwupdate_for_berlin_linux 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(+)