Hi Greg,

On Fri, May 28, 2021 at 02:19:08PM +0200, Greg Kroah-Hartman wrote:

On Fri, May 28, 2021 at 01:33:47PM +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 exposed to userspace (as before).

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.

Signed-off-by: Sebastian Reichel <redacted>
---
 drivers/misc/Kconfig                     |  15 +
 drivers/misc/Makefile                    |   2 +
 drivers/misc/gehc-achc.c                 | 160 ++++++++
 drivers/misc/nxp-ezport.c                | 476 +++++++++++++++++++++++
 drivers/spi/spidev.c                     |   7 +-
 include/linux/platform_data/nxp-ezport.h |   9 +
 include/linux/spi/spi.h                  |   5 +
 7 files changed, 671 insertions(+), 3 deletions(-)
 create mode 100644 drivers/misc/gehc-achc.c
 create mode 100644 drivers/misc/nxp-ezport.c
 create mode 100644 include/linux/platform_data/nxp-ezport.h

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

@@ -208,6 +208,21 @@ 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 NXP_EZPORT
+	tristate
+
+config GEHC_ACHC
+	tristate "GEHC ACHC support"
+	depends on SPI && SYSFS && SPI_SPIDEV
+	select FW_LOADER
+	select NXP_EZPORT
+	help
+	  Support for GE ACHC microcontroller, that is part of the GE
+	  CS One 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..b7bad5a16c8f 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_NXP_EZPORT)	+= nxp-ezport.o
+obj-$(CONFIG_GEHC_ACHC)		+= gehc-achc.o
 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..7856dd70a80c
--- /dev/null
+++ b/drivers/misc/gehc-achc.c

@@ -0,0 +1,160 @@
+// SPDX-License-Identifier: GPL-2.0+

Are you _sure_ you mean "+"?  I have to ask :)

Yes, but I'm also fine with GPL-2.0-only.

quoted

+/*
+ * 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 <linux/platform_data/nxp-ezport.h>
+
+#define ACHC_MAX_FREQ 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)
+		return -EINVAL;
+
+	mutex_lock(&achc->device_lock);
+	ret = ezport_flash(achc->ezport, achc->reset, "achc.bin");
+	mutex_unlock(&achc->device_lock);
+
+	if (ret < 0)
+		return ret;
+
+	return count;
+}
+static DEVICE_ATTR_WO(update_firmware);

You add new sysfs files, yet forget to document them in
Documentation/ABI/  Please fix up for your next submission.

Ack.

quoted

+
+static ssize_t reset_store(struct device *dev, struct device_attribute *attr,
+			   const char *buf, size_t count)
+{
+	struct achc_data *achc = dev_get_drvdata(dev);
+
+	if (!count)
+		return -EINVAL;
+
+	mutex_lock(&achc->device_lock);
+	ezport_reset(achc->reset);
+	mutex_unlock(&achc->device_lock);
+
+	return count;
+}
+static DEVICE_ATTR_WO(reset);
+
+static struct attribute *gehc_achc_attrs[] = {
+	&dev_attr_update_firmware.attr,
+	&dev_attr_reset.attr,
+	NULL,
+};
+
+static const struct attribute_group gehc_achc_attr_group = {
+	.attrs = gehc_achc_attrs,
+};
+
+static void unregister_ezport(void *data)
+{
+	struct spi_device *ezport = data;
+
+	spi_unregister_device(ezport);
+}
+
+static int gehc_achc_probe(struct spi_device *spi)
+{
+	struct achc_data *achc;
+	int ezport_reg, ret;
+
+	spi->max_speed_hz = ACHC_MAX_FREQ;
+	spi->bits_per_word = 8;
+	spi->mode = SPI_MODE_0;
+
+	achc = devm_kzalloc(&spi->dev, sizeof(*achc), GFP_KERNEL);
+	if (!achc)
+		return -ENOMEM;
+	achc->main = spi;
+
+	mutex_init(&achc->device_lock);
+
+	ret = of_property_read_u32_index(spi->dev.of_node, "reg", 1, &ezport_reg);
+	if (ret)
+		return dev_err_probe(&spi->dev, ret, "missing second reg entry!\n");
+
+	achc->ezport = spi_new_ancillary_device(spi, ezport_reg);
+	if (IS_ERR(achc->ezport))
+		return PTR_ERR(achc->ezport);
+
+	ret = devm_add_action_or_reset(&spi->dev, unregister_ezport, achc->ezport);
+	if (ret)
+		return ret;
+
+	achc->reset = devm_gpiod_get(&spi->dev, "reset", GPIOD_OUT_LOW);
+	if (IS_ERR(achc->reset))
+		return dev_err_probe(&spi->dev, PTR_ERR(achc->reset), "Could not get reset gpio\n");
+
+	/*
+	 * The sysfs properties are bound to the dummy device, since the main device already
+	 * uses drvdata assigned by the spidev driver.
+	 */
+	spi_set_drvdata(achc->ezport, achc);
+	ret = devm_device_add_group(&achc->ezport->dev, &gehc_achc_attr_group);

You just raced and lost. Please use the default groups attribute for
your driver instead of this. Or properly attach it to the device some
other way, but what you have done here does not work, sorry.

I've been told the race got fixed from kernel POV?

https://lore.kernel.org/linux-input/20200521055400.GX89269@dtor-ws/ (local)

If this is still an issue, I think most (all?) existing instances of
devm_device_add_group() are a problem and it makes sense to have a
checkpatch warning for it.

And you should probably break this up into at least 2 patches, one for
each "driver" you are adding.

I expect, that Mark wants this to be restructured anyways. If
it stays in its current form I will split this into three patches

1) export spidev probe/remove
2) add ezport support module
3) add gehc-achc driver gluing both together

Thanks for the review.

-- Sebastian