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

[Hans de Goede]

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.
>>>
>>
>