Received-SPF: pass (google.com: domain of linux-kernel+bounces-170577-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Mon, 6 May 2024 19:13:38 -0700
From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
To: Hans de Goede <hdegoede@redhat.com>
Cc: Charles Wang <charles.goodix@gmail.com>, hadess@hadess.net,
	Richard Hughes <hughsient@gmail.com>, linux-input@vger.kernel.org,
	linux-kernel@vger.kernel.org, neil.armstrong@linaro.org,
	Mark Brown <broonie@kernel.org>
Subject: Re: [PATCH] Input: goodix-berlin - Add sysfs interface for reading
 and writing touch IC registers
Message-ID: <ZjmOUp725QTHrfcT@google.com>
References: <20240506114752.47204-1-charles.goodix@gmail.com>
 <6362e889-7df2-4c61-8ad5-bfe199e451ec@redhat.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <6362e889-7df2-4c61-8ad5-bfe199e451ec@redhat.com>

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.

-- 
Dmitry