Hi Andrew,
thanks for your patch.
A few words prior doing a very rough review. I don't like the idea of
posting two complete different patchsets, furthermore if you could have
added the missing SPI support on-top of my patchset very easily since I
used the regmap-API.
Most of my patches already got the r-b tags and I received a very good
review from Dmitry recently which I currently integrating to send a new
version.
On 26-01-22, AThomas63 wrote:
> A Summary of the added features:
> - Add input driver support for TouchNetix aXiom touchscreen controller
> using either I2C or SPI.
> - Support ABS_MT touch reports in axiom_process_u41_report().
> - Support both polling and interrupt mode (trigger low).
> - Add basic documentation and provide example device tree bindings.
> - Provide the basic structure to add firmware and config download in
> the future via both I2C and SPI.
>
> I understand the process is already under way to add a driver to support
> aXiom by Marco at Pengutronix.
> I think it would be most useful for us to attempt to combine this patch
> in order to support both his changes for config/firmware download and
> the patch here for SPI support along with I2C. I would be happy to help
> with this process.
> This would allow TouchNetix to support a broader range of users of the
> aXiom device family.
>
> I also have a device tree binding however I have not added it sincec
> from my understanding this should go in a different patch:
> Documentation/devicetree/bindings/input/touchscreen/tnx,axiom.yaml
>
> My apologies if there are any obvious mistakes in this patch, this is the
> first patch that I have submitted.
That's not a problem at least not for me, but there are contriubtion
guidelines which you could have read first.
Also I'm a bit surprised that you have sent a driver for the same device
family which is less capable than mine. Your driver is missing the
complete fw-update mechanism and therefore needs still your downstream
python tooling. The fw-update logic is a huge part, since this was hard
to get it right.
There are a few other parts, you're doing wrong (please see the patch
review).
Regards,
Marco
>
> Many thanks for your time,
> Andrew Thomas
>
> AThomas63 (1):
> Adding support for aXiom touchscreen controller
>
> drivers/input/touchscreen/Kconfig | 30 ++
> drivers/input/touchscreen/Makefile | 3 +
> drivers/input/touchscreen/axiom_core.c | 482 +++++++++++++++++++++++++
> drivers/input/touchscreen/axiom_core.h | 128 +++++++
> drivers/input/touchscreen/axiom_i2c.c | 152 ++++++++
> drivers/input/touchscreen/axiom_spi.c | 159 ++++++++
> 6 files changed, 954 insertions(+)
> create mode 100644 drivers/input/touchscreen/axiom_core.c
> create mode 100644 drivers/input/touchscreen/axiom_core.h
> create mode 100644 drivers/input/touchscreen/axiom_i2c.c
> create mode 100644 drivers/input/touchscreen/axiom_spi.c
>
> --
> 2.43.0
>
>
--
#gernperDu
#CallMeByMyFirstName
Pengutronix e.K. | |
Steuerwalder Str. 21 | https://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-9 |