| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <155361c0-5068-44df-9fca-d775fc518b7d@redhat.com>
Date: Tue, 7 May 2024 10:25:29 +0200
From: Hans de Goede <hdegoede@...hat.com>
To: Dmitry Torokhov <dmitry.torokhov@...il.com>
Cc: Charles Wang <charles.goodix@...il.com>, hadess@...ess.net,
Richard Hughes <hughsient@...il.com>, linux-input@...r.kernel.org,
linux-kernel@...r.kernel.org, neil.armstrong@...aro.org,
Mark Brown <broonie@...nel.org>
Subject: Re: [PATCH] Input: goodix-berlin - Add sysfs interface for reading
and writing touch IC registers
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@...il.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.
>
Powered by blists - more mailing lists