[0/5] Input: Add support for TouchNetix aXiom touchscreen

Hi,

This adds the initial support for the TouchNetix aXiom touchcontroller
family.

This series is the successor of [1] (many thanks to Bootlin) but I
started from a fresh v1 since I had to rework the driver completely.

The following features are added:
 - I2C communication
 - Input event handling
 - Touchcontroller firmware (AXFW or ALC) updates
 - Touchcontroller config (TH2CFGBIN) updates

[1] https://lore.kernel.org/all/20240703142520.207066-1-kamel.bouhara@bootlin.com

Regards,
  Marco

---
Kamel Bouhara (2):
      dt-bindings: vendor-prefixes: Add TouchNetix AS
      dt-bindings: input: Add TouchNetix axiom touchscreen

Marco Felsch (3):
      firmware_loader: expand firmware error codes with skip error
      firmware_loader: add support to handle FW_UPLOAD_ERR_SKIP
      Input: Add TouchNetix aXiom I2C Touchscreen support

 .../input/touchscreen/touchnetix,ax54a.yaml        |   62 +
 .../devicetree/bindings/vendor-prefixes.yaml       |    2 +
 drivers/base/firmware_loader/sysfs_upload.c        |    5 +
 drivers/input/touchscreen/Kconfig                  |   15 +
 drivers/input/touchscreen/Makefile                 |    1 +
 drivers/input/touchscreen/touchnetix_axiom.c       | 2764 ++++++++++++++++++++
 include/linux/firmware.h                           |    2 +
 lib/test_firmware.c                                |    1 +
 8 files changed, 2852 insertions(+)
---
base-commit: 9852d85ec9d492ebef56dc5f229416c925758edc
change-id: 20240704-v6-10-topic-touchscreen-axiom-105761e81011

Best regards,

On Tue, Nov 19, 2024 at 11:33:51PM +0100, Marco Felsch wrote:
> It's no error if a driver indicates that the firmware is already
> up-to-date and the update can be skipped.
> 
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> ---
>  drivers/base/firmware_loader/sysfs_upload.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/base/firmware_loader/sysfs_upload.c b/drivers/base/firmware_loader/sysfs_upload.c
> index b3cbe5b156e3..44f3d8fa5e64 100644
> --- a/drivers/base/firmware_loader/sysfs_upload.c
> +++ b/drivers/base/firmware_loader/sysfs_upload.c
> @@ -174,6 +174,10 @@ static void fw_upload_main(struct work_struct *work)
>  	fw_upload_update_progress(fwlp, FW_UPLOAD_PROG_PREPARING);
>  	ret = fwlp->ops->prepare(fwl, fwlp->data, fwlp->remaining_size);
>  	if (ret != FW_UPLOAD_ERR_NONE) {
> +		if (ret == FW_UPLOAD_ERR_SKIP) {
> +			dev_info(fw_dev, "firmware already up-to-date, skip update\n");
> +			ret = FW_UPLOAD_ERR_NONE;
> +		}

If you change the error-code from FW_UPLOAD_ERR_SKIP to
FW_UPLOAD_ERR_NONE, then the "skip" string provided in the previous
patch will never be seen. There are currently no other instances where
an error code requires special-case modifications to the fw_upload
code and I don't think it is necessary to add it here.

The dev_info() message above can be provided by the device driver
that is using this API.

I think you can either:

(1) allow "skip" to be treated as an error. The update didn't happen...

-or-

(2) The prepare function could detect the situation and set
    a flag in the same device driver. Your write function could
    set *written to the full data size and return without writing
    anything. Your poll_complete handler could also return
    FW_UPLOAD_ERR_NONE. Then you don't need to add FW_UPLOAD_ERR_SKIP
    at all. You would get the info message from the device driver
    and fw_upload would exit without an error.

Thanks,
- Russ

>  		fw_upload_set_error(fwlp, ret);
>  		goto putdev_exit;
>  	}
> 
> -- 
> 2.39.5
>

On Wed, Nov 20, 2024 at 06:30:37PM +0100, Marco Felsch wrote:
> Hi,
> 
> On 24-11-20, Russ Weight wrote:
> > On Tue, Nov 19, 2024 at 11:33:51PM +0100, Marco Felsch wrote:
> > > It's no error if a driver indicates that the firmware is already
> > > up-to-date and the update can be skipped.
> > > 
> > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > > ---
> > >  drivers/base/firmware_loader/sysfs_upload.c | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > > 
> > > diff --git a/drivers/base/firmware_loader/sysfs_upload.c b/drivers/base/firmware_loader/sysfs_upload.c
> > > index b3cbe5b156e3..44f3d8fa5e64 100644
> > > --- a/drivers/base/firmware_loader/sysfs_upload.c
> > > +++ b/drivers/base/firmware_loader/sysfs_upload.c
> > > @@ -174,6 +174,10 @@ static void fw_upload_main(struct work_struct *work)
> > >  	fw_upload_update_progress(fwlp, FW_UPLOAD_PROG_PREPARING);
> > >  	ret = fwlp->ops->prepare(fwl, fwlp->data, fwlp->remaining_size);
> > >  	if (ret != FW_UPLOAD_ERR_NONE) {
> > > +		if (ret == FW_UPLOAD_ERR_SKIP) {
> > > +			dev_info(fw_dev, "firmware already up-to-date, skip update\n");
> > > +			ret = FW_UPLOAD_ERR_NONE;
> > > +		}
> > 
> > If you change the error-code from FW_UPLOAD_ERR_SKIP to
> > FW_UPLOAD_ERR_NONE, then the "skip" string provided in the previous
> > patch will never be seen. There are currently no other instances where
> 
> Do we really need to set it? As explained within the commit message,
> it's no error if FW_UPLOAD_ERR_SKIP is returned. The previous patch just
> added all pieces which may be required later on.
> 
> > an error code requires special-case modifications to the fw_upload
> > code and I don't think it is necessary to add it here.
> 
> Because at the moment no one is checking it except for the gb-beagleplay
> driver. This driver prints a dev_warn() string and returns a failure.
> Now the userspace needs some heuristic by parsing dmesg to check the
> reason. This is rather complex and very error prone as the sting can be
> changed in the future.
> 
> Therefore I added the support to have a simple error code which can be
> returned by a driver. I'm open to return "skip" as error instead of
> casting it to none. Both is fine for me since both allow the userspace
> to easily check if the error is a 'real' error or if the fw-update was
> just skipped due to already-up-to-date.

Are you saying that you intend for the user-space code to see "skip"?
Because in the current implementation, I don't think the user-space
code would see "skip". If you ultimately return FW_UPLOAD_ERR_NONE,
then cat'ing the error file should result in an empty file.

> 
> I wouldn't say that this is a special case, it is very common but no one
> is performing a fw-version check. Therefore I added this to the common
> code, to make it easier for driver devs.

By "special case" I meant to say that this is the first time this
core code has had to know about any error codes other than
FW_UPLOAD_ERR_NONE - and the first time that an error type alters
the code flow.

I understand that other drivers may also want to abort if the
firmware being loaded is a duplicate.

> 
> > The dev_info() message above can be provided by the device driver
> > that is using this API.
> > 
> > I think you can either:
> > 
> > (1) allow "skip" to be treated as an error. The update didn't happen...
> 
> Please see above.
> 
> > -or-
> > 
> > (2) The prepare function could detect the situation and set
> >     a flag in the same device driver. Your write function could
> >     set *written to the full data size and return without writing
> >     anything. Your poll_complete handler could also return
> >     FW_UPLOAD_ERR_NONE. Then you don't need to add FW_UPLOAD_ERR_SKIP
> >     at all. You would get the info message from the device driver
> >     and fw_upload would exit without an error.
> 
> Please see above. I don't think that this is special case and why making
> the life hard for driver devs instead of having a well known fw
> behaviour?

If you are not opposed to treating it as an error, then all you need
to add are the error code and the string to go with it.

Instead of FW_UPLOAD_ERR_SKIP -> "skip", how about
FW_UPLOAD_ERR_DUPLICATE -> "duplicate_firmware"?

Thanks,
- Russ

> 
> Regards,
>   Marco
> 
> > 
> > Thanks,
> > - Russ
> > 
> > >  		fw_upload_set_error(fwlp, ret);
> > >  		goto putdev_exit;
> > >  	}
> > > 
> > > -- 
> > > 2.39.5
> > > 
> >