Hi Greg,

On Mon, Jul 12, 2021 at 07:46:05PM +0200, Greg Kroah-Hartman wrote:

On Mon, Jul 12, 2021 at 05:02:42PM +0200, Sebastian Reichel wrote:

quoted

General Electric Healthcare's PPD has a secondary processor from
NXP's Kinetis K20 series. That device has two SPI chip selects:

The main interface's behaviour depends on the loaded firmware
and is currently unused.

The secondary interface can be used to update the firmware using
EzPort protocol. This is implemented by this driver using the
kernel's firmware API. It's not done during probe time, since
the device has non-volatile memory and flashing lasts almost 3
minutes.

In thinking about this some more, why does it matter?  Spin up a
workqueue when probing and do the firmware loading then. That way you
do not end up creating yet another custom user/kernel api just to do
something as trivial as loading the firmware for a device.

And I think the firmware loader even handles async loading, or at least
it used to, maybe not anymore, it's a complex api, I recommend reading
the docs...

Flashing the firmware during boot instead of on-demand is not
a good idea for two reasons:

1. This will wear the flash memory of the microcontroller for no
   good reason.

2. Even if the flashing happens in the background, the microcontroller
   will not be usable while it is being programmed. Thus the system
   boot takes 3 minutes instead of 30 seconds.

Note, that this is not the first device with this kind of problem.
I've seen this before for e.g. atmel touchscreen [0] basically does
the same via (undocumented) mxt_update_fw_store().

[0] drivers/input/touchscreen/atmel_mxt_ts.c

quoted

Signed-off-by: Sebastian Reichel <redacted>
---
 .../ABI/testing/sysfs-driver-ge-achc          |  14 +
 drivers/misc/Kconfig                          |  11 +
 drivers/misc/Makefile                         |   2 +
 drivers/misc/gehc-achc.c                      | 136 +++++
 drivers/misc/nxp-ezport.c                     | 468 ++++++++++++++++++
 drivers/misc/nxp-ezport.h                     |   9 +

Why is there two different modules here for the same piece of hardware?

It's only one module, but I put all the generic ezport code into its
own file for easy code reuse. I can put it all into gehc-achc and
thus reduce the chance of code reuse if somebody else needs to do
ezport flashing. Since both files are built into the same module
the size savings of merging both files are minimal.

quoted

 drivers/spi/spidev.c                          |   1 -
 7 files changed, 640 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/ABI/testing/sysfs-driver-ge-achc
 create mode 100644 drivers/misc/gehc-achc.c
 create mode 100644 drivers/misc/nxp-ezport.c
 create mode 100644 drivers/misc/nxp-ezport.h

diff --git a/Documentation/ABI/testing/sysfs-driver-ge-achc b/Documentation/ABI/testing/sysfs-driver-ge-achc
new file mode 100644
index 000000000000..f02812731082
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-driver-ge-achc

@@ -0,0 +1,14 @@
+What:		/sys/bus/spi/<dev>/update_firmware
+Date:		Jul 2021
+Contact:	sebastian.reichel@collabora.com
+Description:	Write 1 to this file to update the ACHC microcontroller
+		firmware via the EzPort interface. For this the kernel
+		will load "achc.bin" via the firmware API (so usually
+		from /lib/firmware). The write will block until the FW
+		has either been flashed successfully or an error occured.
+
+What:		/sys/bus/spi/<dev>/reset
+Date:		Jul 2021
+Contact:	sebastian.reichel@collabora.com
+Description:	Write 1 to this file to reset the microcontroller via the
+		reset GPIO. The write will block until the reset completes.

diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index f4fb5c52b863..1ebf7000671d 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig

@@ -208,6 +208,17 @@ config CS5535_CLOCK_EVENT_SRC
 	  MFGPTs have a better resolution and max interval than the
 	  generic PIT, and are suitable for use as high-res timers.
 
+config GEHC_ACHC
+	tristate "GEHC ACHC support"
+	depends on SPI && SYSFS
+	select FW_LOADER
+	help
+	  Support for GE ACHC microcontroller, that is part of the GE
+	  PPD device.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called gehc-achc.
+
 config HP_ILO
 	tristate "Channel interface driver for the HP iLO processor"
 	depends on PCI

diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index e92a56d4442f..b50180796931 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile

@@ -24,6 +24,8 @@ obj-$(CONFIG_KGDB_TESTS)	+= kgdbts.o
 obj-$(CONFIG_SGI_XP)		+= sgi-xp/
 obj-$(CONFIG_SGI_GRU)		+= sgi-gru/
 obj-$(CONFIG_CS5535_MFGPT)	+= cs5535-mfgpt.o
+obj-$(CONFIG_GEHC_ACHC)		+= gehc-achc-mod.o
+gehc-achc-mod-objs		:= gehc-achc.o nxp-ezport.o

Yeah, you are creating 2 modules for one Kconfig option, why not put
these together into 1?  Makes the code smaller.

Actually the above builds both C files into objects and then both
objects into 1 module. It's a common pattern in some parts of the
kernel, e.g. drm.

quoted

 obj-$(CONFIG_HP_ILO)		+= hpilo.o
 obj-$(CONFIG_APDS9802ALS)	+= apds9802als.o
 obj-$(CONFIG_ISL29003)		+= isl29003.o

diff --git a/drivers/misc/gehc-achc.c b/drivers/misc/gehc-achc.c
new file mode 100644
index 000000000000..893940e7ca4e
--- /dev/null
+++ b/drivers/misc/gehc-achc.c

@@ -0,0 +1,136 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * datasheet: https://www.nxp.com/docs/en/data-sheet/K20P144M120SF3.pdf
+ *
+ * Copyright (C) 2018-2021 Collabora
+ * Copyright (C) 2018-2021 GE Healthcare
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/spi/spi.h>
+#include <linux/of.h>
+#include "nxp-ezport.h"
+
+#define ACHC_MAX_FREQ_HZ 300000
+
+struct achc_data {
+	struct spi_device *main;
+	struct spi_device *ezport;
+	struct gpio_desc *reset;
+
+	struct mutex device_lock; /* avoid concurrent device access */
+};
+
+static ssize_t update_firmware_store(struct device *dev, struct device_attribute *attr,
+				     const char *buf, size_t count)
+{
+	struct achc_data *achc = dev_get_drvdata(dev);
+	int ret;
+
+	if (count != 1 || buf[0] != '1')
+		return -EINVAL;

There a core kernel call to see if the data written to a sysfs file is
"true/false" I would recommend, if you stick with this, to use that
instead.

I have problems understanding that sentence, it seems to be missing
a word.

As far as I can tell -EINVAL is usually used when the user supplied
data is not formatted correctly for sysfs store routines, e.g. lot's
of them are returning -EINVAL like this:

rc = kstrtoul(buf, 0, &value);
if (rc)
    return rc;

Did I miss something?

Thanks,

-- Sebastian