[PATCHv3 0/5] GE Healthcare PPD firmware upgrade driver for ACHC

[PATCHv3 0/5] GE Healthcare PPD firmware upgrade driver for ACHC

[Sebastian Reichel]

GE Healthcare PPD firmware upgrade driver for ACHC

Hi,

The PPD has a secondary processor (NXP Kinetis K20) , which can be
programmed from the main system. It is connected to the main processor
by having it's EzPort interface connected to the SPI bus. Currently
both (normal and EzPort) interfaces are simply exposed to userspace.
This does not work for the EzPort, since EzPort usage requires a device
reset. The proper solution is to do the flashing from kernel space
with properly timed toggling of EzPort chip-select and reset line. In
PATCHv2 it was suggested, that this should happen via an SPI ancillary
device, so this has now been implemented in PATCHv3.

I'm a bit unhappy with the spidev_probe() call in gehc-achc driver, but
it also seems like a bad idea to add all that code to generic spidev
driver. If somebody has a better idea I'm all ears.

Other than that patch 2, which modifies the spi-controller.yaml to
allow multiple reg properties seems to be incorrect, but I did not
find the problem. I hope Rob can point out what I'm missing.

Changes since PATCHv2:
 * https://lore.kernel.org/lkml/20180327135259.30890-1-sebastian.reichel@collabora.co.uk/
 * add SPI core support for ancillary devices
 * modify ACHC binding to make use of ancillary device
 * rewrite driver to use ancillary device
 * rebased to 5.13-rc1

Changes since PATCHv1:
 * https://lore.kernel.org/lkml/20180320172201.2065-1-sebastian.reichel@collabora.co.uk/
 * split DT binding update into its own patch
 * add sysfs attribute documentation
 * fix problem reported by kbuild test robot

Sorry for the very long delay since PATCHv2 and thanks for looking into this,

-- Sebastian

Sebastian Reichel (5):
  spi: add ancillary device support
  spi: dt-bindings: support devices with multiple chipselects
  dt-bindings: misc: ge-achc: Convert to DT schema format
  ARM: dts: imx53-ppd: Fix ACHC entry
  misc: gehc-achc: new driver

 .../devicetree/bindings/misc/ge-achc.txt      |  26 -
 .../devicetree/bindings/misc/ge-achc.yaml     |  67 +++
 .../bindings/spi/spi-controller.yaml          |   7 +-
 arch/arm/boot/dts/imx53-ppd.dts               |  23 +-
 drivers/misc/Kconfig                          |  15 +
 drivers/misc/Makefile                         |   2 +
 drivers/misc/gehc-achc.c                      | 160 ++++++
 drivers/misc/nxp-ezport.c                     | 476 ++++++++++++++++++
 drivers/spi/spi.c                             | 139 +++--
 drivers/spi/spidev.c                          |   7 +-
 include/linux/platform_data/nxp-ezport.h      |   9 +
 include/linux/spi/spi.h                       |   7 +
 12 files changed, 865 insertions(+), 73 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/misc/ge-achc.txt
 create mode 100644 Documentation/devicetree/bindings/misc/ge-achc.yaml
 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

-- 
2.30.2

[PATCHv3 1/5] spi: add ancillary device support

[Sebastian Reichel]

Introduce support for ancillary devices, similar to existing
implementation for I2C. This is useful for devices having
multiple chip-selects, for example some microcontrollers
provide a normal SPI interface and a flashing SPI interface.

Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
---
 drivers/spi/spi.c       | 139 +++++++++++++++++++++++++++++++---------
 include/linux/spi/spi.h |   2 +
 2 files changed, 109 insertions(+), 32 deletions(-)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index ba425b9c7700..7fdf224262b1 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -558,49 +558,23 @@ static int spi_dev_check(struct device *dev, void *data)
 	return 0;
 }
 
-/**
- * spi_add_device - Add spi_device allocated with spi_alloc_device
- * @spi: spi_device to register
- *
- * Companion function to spi_alloc_device.  Devices allocated with
- * spi_alloc_device can be added onto the spi bus with this function.
- *
- * Return: 0 on success; negative errno on failure
- */
-int spi_add_device(struct spi_device *spi)
+static int __spi_add_device(struct spi_device *spi)
 {
 	struct spi_controller *ctlr = spi->controller;
 	struct device *dev = ctlr->dev.parent;
 	int status;
 
-	/* Chipselects are numbered 0..max; validate. */
-	if (spi->chip_select >= ctlr->num_chipselect) {
-		dev_err(dev, "cs%d >= max %d\n", spi->chip_select,
-			ctlr->num_chipselect);
-		return -EINVAL;
-	}
-
-	/* Set the bus ID string */
-	spi_dev_set_name(spi);
-
-	/* We need to make sure there's no other device with this
-	 * chipselect **BEFORE** we call setup(), else we'll trash
-	 * its configuration.  Lock against concurrent add() calls.
-	 */
-	mutex_lock(&spi_add_lock);
-
 	status = bus_for_each_dev(&spi_bus_type, NULL, spi, spi_dev_check);
 	if (status) {
 		dev_err(dev, "chipselect %d already in use\n",
 				spi->chip_select);
-		goto done;
+		return status;
 	}
 
 	/* Controller may unregister concurrently */
 	if (IS_ENABLED(CONFIG_SPI_DYNAMIC) &&
 	    !device_is_registered(&ctlr->dev)) {
-		status = -ENODEV;
-		goto done;
+		return -ENODEV;
 	}
 
 	/* Descriptors take precedence */
@@ -617,7 +591,7 @@ int spi_add_device(struct spi_device *spi)
 	if (status < 0) {
 		dev_err(dev, "can't setup %s, status %d\n",
 				dev_name(&spi->dev), status);
-		goto done;
+		return status;
 	}
 
 	/* Device may be bound to an active driver when this returns */
@@ -628,12 +602,64 @@ int spi_add_device(struct spi_device *spi)
 	else
 		dev_dbg(dev, "registered child %s\n", dev_name(&spi->dev));
 
-done:
+	return status;
+}
+
+/**
+ * spi_add_device - Add spi_device allocated with spi_alloc_device
+ * @spi: spi_device to register
+ *
+ * Companion function to spi_alloc_device.  Devices allocated with
+ * spi_alloc_device can be added onto the spi bus with this function.
+ *
+ * Return: 0 on success; negative errno on failure
+ */
+int spi_add_device(struct spi_device *spi)
+{
+	struct spi_controller *ctlr = spi->controller;
+	struct device *dev = ctlr->dev.parent;
+	int status;
+
+	/* Chipselects are numbered 0..max; validate. */
+	if (spi->chip_select >= ctlr->num_chipselect) {
+		dev_err(dev, "cs%d >= max %d\n", spi->chip_select,
+			ctlr->num_chipselect);
+		return -EINVAL;
+	}
+
+	/* Set the bus ID string */
+	spi_dev_set_name(spi);
+
+	/* We need to make sure there's no other device with this
+	 * chipselect **BEFORE** we call setup(), else we'll trash
+	 * its configuration.  Lock against concurrent add() calls.
+	 */
+	mutex_lock(&spi_add_lock);
+	status = __spi_add_device(spi);
 	mutex_unlock(&spi_add_lock);
 	return status;
 }
 EXPORT_SYMBOL_GPL(spi_add_device);
 
+static int spi_add_device_locked(struct spi_device *spi)
+{
+	struct spi_controller *ctlr = spi->controller;
+	struct device *dev = ctlr->dev.parent;
+
+	/* Chipselects are numbered 0..max; validate. */
+	if (spi->chip_select >= ctlr->num_chipselect) {
+		dev_err(dev, "cs%d >= max %d\n", spi->chip_select,
+			ctlr->num_chipselect);
+		return -EINVAL;
+	}
+
+	/* Set the bus ID string */
+	spi_dev_set_name(spi);
+
+	WARN_ON(!mutex_is_locked(&spi_add_lock));
+	return __spi_add_device(spi);
+}
+
 /**
  * spi_new_device - instantiate one new SPI device
  * @ctlr: Controller to which device is connected
@@ -1993,7 +2019,7 @@ static int of_spi_parse_dt(struct spi_controller *ctlr, struct spi_device *spi,
 	}
 
 	/* Device address */
-	rc = of_property_read_u32(nc, "reg", &value);
+	rc = of_property_read_u32_index(nc, "reg", 0, &value);
 	if (rc) {
 		dev_err(&ctlr->dev, "%pOF has no valid 'reg' property (%d)\n",
 			nc, rc);
@@ -2084,6 +2110,55 @@ static void of_register_spi_devices(struct spi_controller *ctlr)
 static void of_register_spi_devices(struct spi_controller *ctlr) { }
 #endif
 
+/**
+ * spi_new_ancillary_device() - Register ancillary SPI device
+ * @spi:         Pointer to the main SPI device registering the ancillary device
+ * @chip_select: Chip Select of the ancillary device
+ *
+ * Register an ancillary SPI device; for example some chips have a chip-select
+ * for normal device usage and another one for setup/firmware upload.
+ *
+ * This may only be called from main SPI device's probe routine.
+ *
+ * Return: 0 on success; negative errno on failure
+ */
+struct spi_device *spi_new_ancillary_device(struct spi_device *spi,
+					     u8 chip_select)
+{
+	struct spi_device *ancillary;
+	int rc = 0;
+
+	/* Alloc an spi_device */
+	ancillary = spi_alloc_device(spi->controller);
+	if (!ancillary) {
+		rc = -ENOMEM;
+		goto err_out;
+	}
+
+	strlcpy(ancillary->modalias, "dummy", sizeof(ancillary->modalias));
+
+	/* Use provided chip-select for ancillary device */
+	ancillary->chip_select = chip_select;
+
+	/* Take over SPI mode/speed from SPI main device */
+	ancillary->max_speed_hz = spi->max_speed_hz;
+	ancillary->mode = ancillary->mode;
+
+	/* Register the new device */
+	rc = spi_add_device_locked(ancillary);
+	if (rc) {
+		dev_err(&spi->dev, "failed to register ancillary device\n");
+		goto err_out;
+	}
+
+	return ancillary;
+
+err_out:
+	spi_dev_put(ancillary);
+	return ERR_PTR(rc);
+}
+EXPORT_SYMBOL_GPL(spi_new_ancillary_device);
+
 #ifdef CONFIG_ACPI
 struct acpi_spi_lookup {
 	struct spi_controller 	*ctlr;
diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
index 360a3bc767ca..1949230ccd3a 100644
--- a/include/linux/spi/spi.h
+++ b/include/linux/spi/spi.h
@@ -299,6 +299,8 @@ static inline void spi_unregister_driver(struct spi_driver *sdrv)
 		driver_unregister(&sdrv->driver);
 }
 
+extern struct spi_device *spi_new_ancillary_device(struct spi_device *spi, u8 chip_select);
+
 /* use a define to avoid include chaining to get THIS_MODULE */
 #define spi_register_driver(driver) \
 	__spi_register_driver(THIS_MODULE, driver)
-- 
2.30.2

[PATCHv3 2/5] spi: dt-bindings: support devices with multiple chipselects

[Sebastian Reichel]

Add binding support for devices, that have more than one
chip select. A typical example are SPI connected microcontroller,
that can also be programmed over SPI like NXP Kinetis or
chips with a configuration and a data chip select, such as
Microchip's MRF89XA transceiver.

Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
---
 Documentation/devicetree/bindings/spi/spi-controller.yaml | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/spi/spi-controller.yaml b/Documentation/devicetree/bindings/spi/spi-controller.yaml
index 0477396e4945..faef4f6f55b8 100644
--- a/Documentation/devicetree/bindings/spi/spi-controller.yaml
+++ b/Documentation/devicetree/bindings/spi/spi-controller.yaml
@@ -114,8 +114,11 @@ patternProperties:
           Compatible of the SPI device.
 
       reg:
-        minimum: 0
-        maximum: 256
+        minItems: 1
+        maxItems: 256
+        items:
+          minimum: 0
+          maximum: 256
         description:
           Chip select used by the device.
 
-- 
2.30.2

[PATCHv3 3/5] dt-bindings: misc: ge-achc: Convert to DT schema format

[Sebastian Reichel]

Convert the binding to DT schema format. Also update the binding
to fix shortcomings

 * Add "nxp,kinetis-k20" fallback compatible
 * add programming SPI interface and reset GPIO
 * add main clock
 * add voltage supplies
 * drop spi-max-frequency from required properties,
   driver will setup max. frequency

Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
---
 .../devicetree/bindings/misc/ge-achc.txt      | 26 -------
 .../devicetree/bindings/misc/ge-achc.yaml     | 67 +++++++++++++++++++
 2 files changed, 67 insertions(+), 26 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/misc/ge-achc.txt
 create mode 100644 Documentation/devicetree/bindings/misc/ge-achc.yaml

diff --git a/Documentation/devicetree/bindings/misc/ge-achc.txt b/Documentation/devicetree/bindings/misc/ge-achc.txt
deleted file mode 100644
index 77df94d7a32f..000000000000
--- a/Documentation/devicetree/bindings/misc/ge-achc.txt
+++ /dev/null
@@ -1,26 +0,0 @@
-* GE Healthcare USB Management Controller
-
-A device which handles data aquisition from compatible USB based peripherals.
-SPI is used for device management.
-
-Note: This device does not expose the peripherals as USB devices.
-
-Required properties:
-
-- compatible : Should be "ge,achc"
-
-Required SPI properties:
-
-- reg : Should be address of the device chip select within
-  the controller.
-
-- spi-max-frequency : Maximum SPI clocking speed of device in Hz, should be
-  1MHz for the GE ACHC.
-
-Example:
-
-spidev0: spi@0 {
-	compatible = "ge,achc";
-	reg = <0>;
-	spi-max-frequency = <1000000>;
-};
diff --git a/Documentation/devicetree/bindings/misc/ge-achc.yaml b/Documentation/devicetree/bindings/misc/ge-achc.yaml
new file mode 100644
index 000000000000..a9c36fa7cf20
--- /dev/null
+++ b/Documentation/devicetree/bindings/misc/ge-achc.yaml
@@ -0,0 +1,67 @@
+# SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause
+# Copyright (C) 2021 GE Inc.
+# Copyright (C) 2021 Collabora Ltd.
+%YAML 1.2
+---
+$id: "http://devicetree.org/schemas/misc/ge-achc.yaml#"
+$schema: "http://devicetree.org/meta-schemas/core.yaml#"
+
+title: GE Healthcare USB Management Controller
+
+description: |
+  A device which handles data acquisition from compatible USB based peripherals.
+  SPI is used for device management.
+
+  Note: This device does not expose the peripherals as USB devices.
+
+maintainers:
+  - Sebastian Reichel <sre@kernel.org>
+
+properties:
+  compatible:
+    items:
+      - const: ge,achc
+      - const: nxp,kinetis-k20
+
+  clocks:
+    maxItems: 1
+
+  vdd-supply:
+    description: Digital power supply regulator on VDD pin
+
+  vdda-supply:
+    description: Analog power supply regulator on VDDA pin
+
+  reg:
+    minItems: 2
+    maxItems: 2
+    items:
+      - description: Control interface
+      - description: Firmware programming interface
+
+  reset-gpios:
+    description: GPIO used for hardware reset.
+    maxItems: 1
+
+required:
+  - compatible
+  - clocks
+  - reg
+  - reset-gpios
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+    spi {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        spi@1 {
+            compatible = "ge,achc", "nxp,kinetis-k20";
+            reg = <1>, <0>;
+            clocks = <&achc_24M>;
+            reset-gpios = <&gpio3 6 GPIO_ACTIVE_LOW>;
+        };
+    };
-- 
2.30.2

[PATCHv3 4/5] ARM: dts: imx53-ppd: Fix ACHC entry

[Sebastian Reichel]

PPD has only one ACHC device, which effectively is a Kinetis
microcontroller. It has one SPI interface used for normal
communication. Additionally it's possible to flash the device
firmware using NXP's EzPort protocol by correctly driving a
second chip select pin and the device reset pin.

Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
---
 arch/arm/boot/dts/imx53-ppd.dts | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/arch/arm/boot/dts/imx53-ppd.dts b/arch/arm/boot/dts/imx53-ppd.dts
index be040b6a02fa..1f3ee60fb102 100644
--- a/arch/arm/boot/dts/imx53-ppd.dts
+++ b/arch/arm/boot/dts/imx53-ppd.dts
@@ -70,6 +70,12 @@ cko2_11M: sgtl-clock-cko2 {
 		clock-frequency = <11289600>;
 	};
 
+	achc_24M: achc-clock {
+		compatible = "fixed-clock";
+		#clock-cells = <0>;
+		clock-frequency = <24000000>;
+	};
+
 	sgtlsound: sound {
 		compatible = "fsl,imx53-cpuvo-sgtl5000",
 			     "fsl,imx-audio-sgtl5000";
@@ -314,16 +320,13 @@ &gpio4 11 GPIO_ACTIVE_LOW
 		    &gpio4 12 GPIO_ACTIVE_LOW>;
 	status = "okay";
 
-	spidev0: spi@0 {
-		compatible = "ge,achc";
-		reg = <0>;
-		spi-max-frequency = <1000000>;
-	};
-
-	spidev1: spi@1 {
-		compatible = "ge,achc";
-		reg = <1>;
-		spi-max-frequency = <1000000>;
+	spidev0: spi@1 {
+		compatible = "ge,achc", "nxp,kinetis-k20";
+		reg = <1>, <0>;
+		vdd-supply = <&reg_3v3>;
+		vdda-supply = <&reg_3v3>;
+		clocks = <&achc_24M>;
+		reset-gpios = <&gpio3 6 GPIO_ACTIVE_LOW>;
 	};
 
 	gpioxra0: gpio@2 {
-- 
2.30.2

[PATCHv3 5/5] misc: gehc-achc: new driver

[Sebastian Reichel]

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 <sebastian.reichel@collabora.com>
---
 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+
+/*
+ * 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);
+
+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);
+	if (ret)
+		return ret;
+
+	/*
+	 * Anything before this must use device managed resources to ensure resources being
+	 * free'd in reverse allocation order.
+	 */
+	ret = spidev_probe(spi);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int gehc_achc_remove(struct spi_device *spi)
+{
+	return spidev_remove(spi);
+}
+
+static const struct spi_device_id gehc_achc_id[] = {
+	{ "ge,achc", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(spi, gehc_achc_id);
+
+static const struct of_device_id gehc_achc_of_match[] = {
+	{ .compatible = "ge,achc" },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, gehc_achc_of_match);
+
+static struct spi_driver gehc_achc_spi_driver = {
+	.driver = {
+		.name	= "gehc-achc",
+		.of_match_table = gehc_achc_of_match,
+	},
+	.probe		= gehc_achc_probe,
+	.remove		= gehc_achc_remove,
+	.id_table	= gehc_achc_id,
+};
+module_spi_driver(gehc_achc_spi_driver);
+
+MODULE_DESCRIPTION("GEHC ACHC driver");
+MODULE_AUTHOR("Sebastian Reichel <sebastian.reichel@collabora.com>");
+MODULE_LICENSE("GPL");
diff --git a/drivers/misc/nxp-ezport.c b/drivers/misc/nxp-ezport.c
new file mode 100644
index 000000000000..b905c0132bff
--- /dev/null
+++ b/drivers/misc/nxp-ezport.c
@@ -0,0 +1,476 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * This library implements NXP's EzPort protocol used to
+ * flash the firmware of Kinetis microcontrollers, such
+ * as https://www.nxp.com/part/MK20FN1M0VMD12 and is
+ * intended to be used as ancillary SPI device.
+ *
+ * Copyright (C) 2018-2021 Collabora
+ * Copyright (C) 2018-2021 GE Healthcare
+ */
+
+#include <linux/delay.h>
+#include <linux/firmware.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/gpio/consumer.h>
+#include <linux/spi/spi.h>
+#include <linux/platform_data/nxp-ezport.h>
+
+#define EZPORT_RESET_DELAY_MS	100
+#define EZPORT_STARTUP_DELAY_MS	200
+#define EZPORT_WRITE_WAIT_MS	10
+#define EZPORT_TRANSFER_SIZE	2048
+
+#define EZPORT_CMD_SP		0x02 /* flash section program */
+#define EZPORT_CMD_RDSR		0x05 /* read status register */
+#define EZPORT_CMD_WREN		0x06 /* write enable */
+#define EZPORT_CMD_FAST_READ	0x0b /* flash read data at high speed */
+#define EZPORT_CMD_RESET	0xb9 /* reset chip */
+#define EZPORT_CMD_BE		0xc7 /* bulk erase */
+#define EZPORT_CMD_SE		0xd8 /* sector erase */
+
+#define EZPORT_SECTOR_SIZE	4096
+#define EZPORT_SECTOR_MASK	(EZPORT_SECTOR_SIZE - 1)
+
+#define EZPORT_STATUS_WIP	BIT(0) /* write in progress */
+#define EZPORT_STATUS_WEN	BIT(1) /* write enable */
+#define EZPORT_STATUS_BEDIS	BIT(2) /* bulk erase disable */
+#define EZPORT_STATUS_FLEXRAM	BIT(3) /* FlexRAM mode */
+#define EZPORT_STATUS_WEF	BIT(6) /* write error flag */
+#define EZPORT_STATUS_FS	BIT(7) /* flash security */
+
+/**
+ * ezport_reset - reset ezport device
+ * @reset: the gpio connected to the device reset pin
+ *
+ * Context: can sleep
+ */
+void ezport_reset(struct gpio_desc *reset)
+{
+	gpiod_set_value(reset, 1);
+	msleep(EZPORT_RESET_DELAY_MS);
+	gpiod_set_value(reset, 0);
+	msleep(EZPORT_STARTUP_DELAY_MS);
+}
+EXPORT_SYMBOL_GPL(ezport_reset);
+
+static int ezport_start_programming(struct spi_device *spi, struct gpio_desc *reset)
+{
+	struct spi_message msg;
+	struct spi_transfer assert_cs = {
+		.cs_change   = 1,
+	};
+	struct spi_transfer release_cs = { };
+	int ret;
+
+	spi_bus_lock(spi->master);
+
+	/* assert chip select */
+	spi_message_init(&msg);
+	spi_message_add_tail(&assert_cs, &msg);
+	ret = spi_sync_locked(spi, &msg);
+	if (ret)
+		goto fail;
+
+	msleep(EZPORT_STARTUP_DELAY_MS);
+
+	/* reset with asserted chip select to switch into programming mode */
+	ezport_reset(reset);
+
+	/* release chip select */
+	spi_message_init(&msg);
+	spi_message_add_tail(&release_cs, &msg);
+	ret = spi_sync_locked(spi, &msg);
+
+fail:
+	spi_bus_unlock(spi->master);
+	return ret;
+}
+
+static void ezport_stop_programming(struct spi_device *spi, struct gpio_desc *reset)
+{
+	/* reset without asserted chip select to return into normal mode */
+	spi_bus_lock(spi->master);
+	ezport_reset(reset);
+	spi_bus_unlock(spi->master);
+}
+
+static int ezport_get_status_register(struct spi_device *spi)
+{
+	int ret;
+
+	ret = spi_w8r8(spi, EZPORT_CMD_RDSR);
+	if (ret < 0)
+		return ret;
+	if (ret == 0xff) {
+		dev_err(&spi->dev, "Invalid EzPort status, EzPort is not functional!\n");
+		return -EINVAL;
+	}
+
+	return ret;
+}
+
+static int ezport_soft_reset(struct spi_device *spi)
+{
+	u8 cmd = EZPORT_CMD_RESET;
+	int ret;
+
+	ret = spi_write(spi, &cmd, 1);
+	if (ret < 0)
+		return ret;
+
+	msleep(EZPORT_STARTUP_DELAY_MS);
+
+	return 0;
+}
+
+static int ezport_send_simple(struct spi_device *spi, u8 cmd)
+{
+	int ret;
+
+	ret = spi_write(spi, &cmd, 1);
+	if (ret < 0)
+		return ret;
+
+	return ezport_get_status_register(spi);
+}
+
+static int ezport_wait_write(struct spi_device *spi, u32 retries)
+{
+	int ret;
+	u32 i;
+
+	for (i = 0; i < retries; i++) {
+		ret = ezport_get_status_register(spi);
+		if (ret > 0 && !(ret & EZPORT_STATUS_WIP))
+			break;
+		msleep(EZPORT_WRITE_WAIT_MS);
+	}
+
+	return ret;
+}
+
+static int ezport_write_enable(struct spi_device *spi)
+{
+	int ret = 0, retries = 3;
+
+	for (retries = 0; retries < 3; retries++) {
+		ret = ezport_send_simple(spi, EZPORT_CMD_WREN);
+		if (ret > 0 && ret & EZPORT_STATUS_WEN)
+			break;
+	}
+
+	if (!(ret & EZPORT_STATUS_WEN)) {
+		dev_err(&spi->dev, "EzPort write enable timed out\n");
+		return -ETIMEDOUT;
+	}
+	return 0;
+}
+
+static int ezport_bulk_erase(struct spi_device *spi)
+{
+	int ret;
+	static const u8 cmd = EZPORT_CMD_BE;
+
+	dev_dbg(&spi->dev, "EzPort bulk erase...\n");
+
+	ret = ezport_write_enable(spi);
+	if (ret < 0)
+		return ret;
+
+	ret = spi_write(spi, &cmd, 1);
+	if (ret < 0)
+		return ret;
+
+	ret = ezport_wait_write(spi, 1000);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
+static int ezport_section_erase(struct spi_device *spi, u32 address)
+{
+	u8 query[] = {EZPORT_CMD_SE, (address >> 16) & 0xff, (address >> 8) & 0xff, address & 0xff};
+	int ret;
+
+	dev_dbg(&spi->dev, "Ezport section erase @ 0x%06x...\n", address);
+
+	if (address & EZPORT_SECTOR_MASK)
+		return -EINVAL;
+
+	ret = ezport_write_enable(spi);
+	if (ret < 0)
+		return ret;
+
+	ret = spi_write(spi, query, sizeof(query));
+	if (ret < 0)
+		return ret;
+
+	return ezport_wait_write(spi, 200);
+}
+
+static int ezport_flash_transfer(struct spi_device *spi, u32 address,
+				 const u8 *payload, size_t payload_size)
+{
+	struct spi_transfer xfers[2] = {};
+	u8 *command;
+	int ret;
+
+	dev_dbg(&spi->dev, "EzPort write %u bytes @ 0x%06x...\n", payload_size, address);
+
+	command = kmalloc(4, GFP_KERNEL | GFP_DMA);
+	if (!command)
+		return -ENOMEM;
+
+	command[0] = EZPORT_CMD_SP;
+	command[1] = address >> 16;
+	command[2] = address >> 8;
+	command[3] = address >> 0;
+
+	xfers[0].tx_buf = command;
+	xfers[0].len = 4;
+
+	xfers[1].tx_buf = payload;
+	xfers[1].len = payload_size;
+
+	ret = spi_sync_transfer(spi, xfers, 2);
+	kfree(command);
+	if (ret < 0)
+		return ret;
+
+	return ezport_wait_write(spi, 40);
+}
+
+static int ezport_flash_compare(struct spi_device *spi, u32 address,
+				const u8 *payload, size_t payload_size)
+{
+	struct spi_transfer xfers[2] = {};
+	u8 *buffer;
+	int ret;
+
+	buffer = kmalloc(payload_size + 4, GFP_KERNEL | GFP_DMA);
+	if (!buffer)
+		return -ENOMEM;
+
+	buffer[0] = EZPORT_CMD_FAST_READ;
+	buffer[1] = address >> 16;
+	buffer[2] = address >> 8;
+	buffer[3] = address >> 0;
+
+	xfers[0].tx_buf = buffer;
+	xfers[0].len = 4;
+
+	xfers[1].rx_buf = buffer + 4;
+	xfers[1].len = payload_size;
+
+	ret = spi_sync_transfer(spi, xfers, 2);
+	if (ret)
+		goto err;
+
+	ret = memcmp(payload, buffer + 4, payload_size);
+	if (ret) {
+		ret = -EBADMSG;
+		dev_dbg(&spi->dev, "Verfication failure @ %06x", address);
+		print_hex_dump_bytes("fw:  ", DUMP_PREFIX_OFFSET, payload, payload_size);
+		print_hex_dump_bytes("dev: ", DUMP_PREFIX_OFFSET, buffer + 4, payload_size);
+	}
+
+err:
+	kfree(buffer);
+	return ret;
+}
+
+static int ezport_firmware_compare_data(struct spi_device *spi,
+					const u8 *data, size_t size)
+{
+	int ret;
+	u32 address = 0;
+	u32 transfer_size;
+
+	dev_dbg(&spi->dev, "EzPort compare data with %u bytes...\n", size);
+
+	ret = ezport_get_status_register(spi);
+	if (ret < 0)
+		return ret;
+
+	if (ret & EZPORT_STATUS_FS) {
+		dev_info(&spi->dev, "Device is in secure mode (status=0x%02x)!\n", ret);
+		dev_info(&spi->dev, "FW verification is not possible\n");
+		return -EACCES;
+	}
+
+	while (size - address > 0) {
+		transfer_size = min((u32) EZPORT_TRANSFER_SIZE, size - address);
+
+		ret = ezport_flash_compare(spi, address, data+address, transfer_size);
+		if (ret)
+			return ret;
+
+		address += transfer_size;
+	}
+
+	return 0;
+}
+
+static int ezport_firmware_flash_data(struct spi_device *spi,
+				      const u8 *data, size_t size)
+{
+	int ret;
+	u32 address = 0;
+	u32 transfer_size;
+
+	dev_dbg(&spi->dev, "EzPort flash data with %u bytes...\n", size);
+
+	ret = ezport_get_status_register(spi);
+	if (ret < 0)
+		return ret;
+
+	if (ret & EZPORT_STATUS_FS) {
+		ret = ezport_bulk_erase(spi);
+		if (ret < 0)
+			return ret;
+		if (ret & EZPORT_STATUS_FS)
+			return -EINVAL;
+	}
+
+	while (size - address > 0) {
+		if (!(address & EZPORT_SECTOR_MASK)) {
+			ret = ezport_section_erase(spi, address);
+			if (ret < 0)
+				return ret;
+			if (ret & EZPORT_STATUS_WIP || ret & EZPORT_STATUS_WEF)
+				return -EIO;
+		}
+
+		transfer_size = min((u32) EZPORT_TRANSFER_SIZE, size - address);
+
+		ret = ezport_flash_transfer(spi, address,
+					    data+address, transfer_size);
+		if (ret < 0)
+			return ret;
+		else if (ret & EZPORT_STATUS_WIP)
+			return -ETIMEDOUT;
+		else if (ret & EZPORT_STATUS_WEF)
+			return -EIO;
+
+		address += transfer_size;
+	}
+
+	ret = ezport_soft_reset(spi);
+	if (ret < 0)
+		dev_warn(&spi->dev, "EzPort reset failed!\n");
+
+	dev_dbg(&spi->dev, "EzPort verify flashed data...\n");
+	ret = ezport_firmware_compare_data(spi, data, size);
+
+	/* allow missing FW verfication in secure mode */
+	if (ret == -EACCES)
+		ret = 0;
+
+	if (ret < 0)
+		dev_err(&spi->dev, "Failed to verify flashed data: %d\n", ret);
+
+	return ret;
+}
+
+static int ezport_firmware_load(struct spi_device *spi, const char *fwname)
+{
+	const struct firmware *fw;
+	int ret;
+
+	ret = request_firmware(&fw, fwname, &spi->dev);
+	if (ret) {
+		dev_err(&spi->dev, "Could not get firmware: %d\n", ret);
+		return ret;
+	}
+
+	ret = ezport_firmware_flash_data(spi, fw->data, fw->size);
+
+	release_firmware(fw);
+
+	return ret;
+}
+
+static int ezport_firmware_verify(struct spi_device *spi, const char *fwname)
+{
+	const struct firmware *fw;
+	int ret;
+
+	ret = request_firmware(&fw, fwname, &spi->dev);
+	if (ret) {
+		dev_err(&spi->dev, "Could not get firmware: %d\n", ret);
+		return ret;
+	}
+
+	ret = ezport_firmware_compare_data(spi, fw->data, fw->size);
+
+	release_firmware(fw);
+
+	return ret;
+}
+
+/**
+ * ezport_flash - flash device firmware
+ * @spi: SPI device for NXP EzPort interface
+ * @reset: the gpio connected to the device reset pin
+ * @fwname: filename of the firmware that should be flashed
+ *
+ * Context: can sleep
+ *
+ * Return: 0 on success; negative errno on failure
+ */
+int ezport_flash(struct spi_device *spi, struct gpio_desc *reset, const char *fwname)
+{
+	int ret;
+
+	ret = ezport_start_programming(spi, reset);
+	if (ret)
+		return ret;
+
+	ret = ezport_firmware_load(spi, fwname);
+
+	ezport_stop_programming(spi, reset);
+
+	if (ret)
+		dev_err(&spi->dev, "Failed to flash firmware: %d\n", ret);
+	else
+		dev_dbg(&spi->dev, "Finished FW flashing!\n");
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(ezport_flash);
+
+/**
+ * ezport_verify - verify device firmware
+ * @spi: SPI device for NXP EzPort interface
+ * @reset: the gpio connected to the device reset pin
+ * @fwname: filename of the firmware that should be compared
+ *
+ * Context: can sleep
+ *
+ * Return: 0 on success; negative errno on failure
+ */
+int ezport_verify(struct spi_device *spi, struct gpio_desc *reset, const char *fwname)
+{
+	int ret;
+
+	ret = ezport_start_programming(spi, reset);
+	if (ret)
+		return ret;
+
+	ret = ezport_firmware_verify(spi, fwname);
+
+	ezport_stop_programming(spi, reset);
+
+	if (ret)
+		dev_err(&spi->dev, "Failed to verify firmware: %d\n", ret);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(ezport_verify);
+
+MODULE_DESCRIPTION("NXP EzPort protocol support");
+MODULE_AUTHOR("Sebastian Reichel <sebastian.reichel@collabora.com>");
+MODULE_LICENSE("GPL");
diff --git a/drivers/spi/spidev.c b/drivers/spi/spidev.c
index f56e0e975a46..e481ff8d1fc4 100644
--- a/drivers/spi/spidev.c
+++ b/drivers/spi/spidev.c
@@ -677,7 +677,6 @@ static struct class *spidev_class;
 static const struct of_device_id spidev_dt_ids[] = {
 	{ .compatible = "rohm,dh2228fv" },
 	{ .compatible = "lineartechnology,ltc2488" },
-	{ .compatible = "ge,achc" },
 	{ .compatible = "semtech,sx1301" },
 	{ .compatible = "lwn,bk4" },
 	{ .compatible = "dh,dhcom-board" },
@@ -728,7 +727,7 @@ static inline void spidev_probe_acpi(struct spi_device *spi) {}
 
 /*-------------------------------------------------------------------------*/
 
-static int spidev_probe(struct spi_device *spi)
+int spidev_probe(struct spi_device *spi)
 {
 	struct spidev_data	*spidev;
 	int			status;
@@ -789,8 +788,9 @@ static int spidev_probe(struct spi_device *spi)
 
 	return status;
 }
+EXPORT_SYMBOL_GPL(spidev_probe);
 
-static int spidev_remove(struct spi_device *spi)
+int spidev_remove(struct spi_device *spi)
 {
 	struct spidev_data	*spidev = spi_get_drvdata(spi);
 
@@ -810,6 +810,7 @@ static int spidev_remove(struct spi_device *spi)
 
 	return 0;
 }
+EXPORT_SYMBOL_GPL(spidev_remove);
 
 static struct spi_driver spidev_spi_driver = {
 	.driver = {
diff --git a/include/linux/platform_data/nxp-ezport.h b/include/linux/platform_data/nxp-ezport.h
new file mode 100644
index 000000000000..b0a2af9c1285
--- /dev/null
+++ b/include/linux/platform_data/nxp-ezport.h
@@ -0,0 +1,9 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+#ifndef __NXP_EZPORT_H
+#define __NXP_EZPORT_H
+
+extern void ezport_reset(struct gpio_desc *reset);
+extern int ezport_flash(struct spi_device *spi, struct gpio_desc *reset, const char *fwname);
+extern int ezport_verify(struct spi_device *spi, struct gpio_desc *reset, const char *fwname);
+
+#endif
diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
index 1949230ccd3a..7b753f9da56d 100644
--- a/include/linux/spi/spi.h
+++ b/include/linux/spi/spi.h
@@ -1525,6 +1525,11 @@ of_find_spi_device_by_node(struct device_node *node)
 
 #endif /* IS_ENABLED(CONFIG_OF) */
 
+#ifdef CONFIG_SPI_SPIDEV
+extern int spidev_probe(struct spi_device *spi);
+extern int spidev_remove(struct spi_device *spi);
+#endif
+
 /* Compatibility layer */
 #define spi_master			spi_controller
 
-- 
2.30.2

Re: [PATCHv3 5/5] misc: gehc-achc: new driver

[Greg Kroah-Hartman]

On Fri, May 28, 2021 at 01:33:47PM +0200, Sebastian Reichel wrote:
> 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 <sebastian.reichel@collabora.com>
> ---
>  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 :)

> +/*
> + * 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.

> +
> +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.


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

thanks,

greg k-h

Re: [PATCHv3 5/5] misc: gehc-achc: new driver

[-- Attachment #1: Type: text/plain, Size: 5379 bytes --]

Hi Sebastian,

I love your patch! Perhaps something to improve:

[auto build test WARNING on robh/for-next]
[also build test WARNING on char-misc/char-misc-testing v5.13-rc3]
[cannot apply to spi/for-next next-20210528]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Sebastian-Reichel/GE-Healthcare-PPD-firmware-upgrade-driver-for-ACHC/20210528-193816
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
config: mips-randconfig-r031-20210528 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project 6505c630407c5feec818f0bb1c284f9eeebf2071)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install mips cross compiling tool for clang build
        # apt-get install binutils-mips-linux-gnu
        # https://github.com/0day-ci/linux/commit/dac014da90e2715a80e4f7139ac40333cd3d4bec
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Sebastian-Reichel/GE-Healthcare-PPD-firmware-upgrade-driver-for-ACHC/20210528-193816
        git checkout dac014da90e2715a80e4f7139ac40333cd3d4bec
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=mips 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/spi/spidev.c:730:5: warning: no previous prototype for function 'spidev_probe' [-Wmissing-prototypes]
   int spidev_probe(struct spi_device *spi)
       ^
   drivers/spi/spidev.c:730:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   int spidev_probe(struct spi_device *spi)
   ^
   static 
>> drivers/spi/spidev.c:793:5: warning: no previous prototype for function 'spidev_remove' [-Wmissing-prototypes]
   int spidev_remove(struct spi_device *spi)
       ^
   drivers/spi/spidev.c:793:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   int spidev_remove(struct spi_device *spi)
   ^
   static 
   2 warnings generated.


vim +/spidev_probe +730 drivers/spi/spidev.c

   729	
 > 730	int spidev_probe(struct spi_device *spi)
   731	{
   732		struct spidev_data	*spidev;
   733		int			status;
   734		unsigned long		minor;
   735	
   736		/*
   737		 * spidev should never be referenced in DT without a specific
   738		 * compatible string, it is a Linux implementation thing
   739		 * rather than a description of the hardware.
   740		 */
   741		WARN(spi->dev.of_node &&
   742		     of_device_is_compatible(spi->dev.of_node, "spidev"),
   743		     "%pOF: buggy DT: spidev listed directly in DT\n", spi->dev.of_node);
   744	
   745		spidev_probe_acpi(spi);
   746	
   747		/* Allocate driver data */
   748		spidev = kzalloc(sizeof(*spidev), GFP_KERNEL);
   749		if (!spidev)
   750			return -ENOMEM;
   751	
   752		/* Initialize the driver data */
   753		spidev->spi = spi;
   754		spin_lock_init(&spidev->spi_lock);
   755		mutex_init(&spidev->buf_lock);
   756	
   757		INIT_LIST_HEAD(&spidev->device_entry);
   758	
   759		/* If we can allocate a minor number, hook up this device.
   760		 * Reusing minors is fine so long as udev or mdev is working.
   761		 */
   762		mutex_lock(&device_list_lock);
   763		minor = find_first_zero_bit(minors, N_SPI_MINORS);
   764		if (minor < N_SPI_MINORS) {
   765			struct device *dev;
   766	
   767			spidev->devt = MKDEV(SPIDEV_MAJOR, minor);
   768			dev = device_create(spidev_class, &spi->dev, spidev->devt,
   769					    spidev, "spidev%d.%d",
   770					    spi->master->bus_num, spi->chip_select);
   771			status = PTR_ERR_OR_ZERO(dev);
   772		} else {
   773			dev_dbg(&spi->dev, "no minor number available!\n");
   774			status = -ENODEV;
   775		}
   776		if (status == 0) {
   777			set_bit(minor, minors);
   778			list_add(&spidev->device_entry, &device_list);
   779		}
   780		mutex_unlock(&device_list_lock);
   781	
   782		spidev->speed_hz = spi->max_speed_hz;
   783	
   784		if (status == 0)
   785			spi_set_drvdata(spi, spidev);
   786		else
   787			kfree(spidev);
   788	
   789		return status;
   790	}
   791	EXPORT_SYMBOL_GPL(spidev_probe);
   792	
 > 793	int spidev_remove(struct spi_device *spi)
   794	{
   795		struct spidev_data	*spidev = spi_get_drvdata(spi);
   796	
   797		/* prevent new opens */
   798		mutex_lock(&device_list_lock);
   799		/* make sure ops on existing fds can abort cleanly */
   800		spin_lock_irq(&spidev->spi_lock);
   801		spidev->spi = NULL;
   802		spin_unlock_irq(&spidev->spi_lock);
   803	
   804		list_del(&spidev->device_entry);
   805		device_destroy(spidev_class, spidev->devt);
   806		clear_bit(MINOR(spidev->devt), minors);
   807		if (spidev->users == 0)
   808			kfree(spidev);
   809		mutex_unlock(&device_list_lock);
   810	
   811		return 0;
   812	}
   813	EXPORT_SYMBOL_GPL(spidev_remove);
   814	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 33899 bytes --]

Re: [PATCHv3 5/5] misc: gehc-achc: new driver

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 5522 bytes --]

Hi Sebastian,

I love your patch! Perhaps something to improve:

[auto build test WARNING on robh/for-next]
[also build test WARNING on char-misc/char-misc-testing v5.13-rc3]
[cannot apply to spi/for-next next-20210528]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Sebastian-Reichel/GE-Healthcare-PPD-firmware-upgrade-driver-for-ACHC/20210528-193816
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
config: mips-randconfig-r031-20210528 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project 6505c630407c5feec818f0bb1c284f9eeebf2071)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install mips cross compiling tool for clang build
        # apt-get install binutils-mips-linux-gnu
        # https://github.com/0day-ci/linux/commit/dac014da90e2715a80e4f7139ac40333cd3d4bec
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Sebastian-Reichel/GE-Healthcare-PPD-firmware-upgrade-driver-for-ACHC/20210528-193816
        git checkout dac014da90e2715a80e4f7139ac40333cd3d4bec
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=mips 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/spi/spidev.c:730:5: warning: no previous prototype for function 'spidev_probe' [-Wmissing-prototypes]
   int spidev_probe(struct spi_device *spi)
       ^
   drivers/spi/spidev.c:730:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   int spidev_probe(struct spi_device *spi)
   ^
   static 
>> drivers/spi/spidev.c:793:5: warning: no previous prototype for function 'spidev_remove' [-Wmissing-prototypes]
   int spidev_remove(struct spi_device *spi)
       ^
   drivers/spi/spidev.c:793:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   int spidev_remove(struct spi_device *spi)
   ^
   static 
   2 warnings generated.


vim +/spidev_probe +730 drivers/spi/spidev.c

   729	
 > 730	int spidev_probe(struct spi_device *spi)
   731	{
   732		struct spidev_data	*spidev;
   733		int			status;
   734		unsigned long		minor;
   735	
   736		/*
   737		 * spidev should never be referenced in DT without a specific
   738		 * compatible string, it is a Linux implementation thing
   739		 * rather than a description of the hardware.
   740		 */
   741		WARN(spi->dev.of_node &&
   742		     of_device_is_compatible(spi->dev.of_node, "spidev"),
   743		     "%pOF: buggy DT: spidev listed directly in DT\n", spi->dev.of_node);
   744	
   745		spidev_probe_acpi(spi);
   746	
   747		/* Allocate driver data */
   748		spidev = kzalloc(sizeof(*spidev), GFP_KERNEL);
   749		if (!spidev)
   750			return -ENOMEM;
   751	
   752		/* Initialize the driver data */
   753		spidev->spi = spi;
   754		spin_lock_init(&spidev->spi_lock);
   755		mutex_init(&spidev->buf_lock);
   756	
   757		INIT_LIST_HEAD(&spidev->device_entry);
   758	
   759		/* If we can allocate a minor number, hook up this device.
   760		 * Reusing minors is fine so long as udev or mdev is working.
   761		 */
   762		mutex_lock(&device_list_lock);
   763		minor = find_first_zero_bit(minors, N_SPI_MINORS);
   764		if (minor < N_SPI_MINORS) {
   765			struct device *dev;
   766	
   767			spidev->devt = MKDEV(SPIDEV_MAJOR, minor);
   768			dev = device_create(spidev_class, &spi->dev, spidev->devt,
   769					    spidev, "spidev%d.%d",
   770					    spi->master->bus_num, spi->chip_select);
   771			status = PTR_ERR_OR_ZERO(dev);
   772		} else {
   773			dev_dbg(&spi->dev, "no minor number available!\n");
   774			status = -ENODEV;
   775		}
   776		if (status == 0) {
   777			set_bit(minor, minors);
   778			list_add(&spidev->device_entry, &device_list);
   779		}
   780		mutex_unlock(&device_list_lock);
   781	
   782		spidev->speed_hz = spi->max_speed_hz;
   783	
   784		if (status == 0)
   785			spi_set_drvdata(spi, spidev);
   786		else
   787			kfree(spidev);
   788	
   789		return status;
   790	}
   791	EXPORT_SYMBOL_GPL(spidev_probe);
   792	
 > 793	int spidev_remove(struct spi_device *spi)
   794	{
   795		struct spidev_data	*spidev = spi_get_drvdata(spi);
   796	
   797		/* prevent new opens */
   798		mutex_lock(&device_list_lock);
   799		/* make sure ops on existing fds can abort cleanly */
   800		spin_lock_irq(&spidev->spi_lock);
   801		spidev->spi = NULL;
   802		spin_unlock_irq(&spidev->spi_lock);
   803	
   804		list_del(&spidev->device_entry);
   805		device_destroy(spidev_class, spidev->devt);
   806		clear_bit(MINOR(spidev->devt), minors);
   807		if (spidev->users == 0)
   808			kfree(spidev);
   809		mutex_unlock(&device_list_lock);
   810	
   811		return 0;
   812	}
   813	EXPORT_SYMBOL_GPL(spidev_remove);
   814	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 33899 bytes --]

Re: [PATCHv3 5/5] misc: gehc-achc: new driver

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 7752 bytes --]

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:
> > 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 <sebastian.reichel@collabora.com>
> > ---
> >  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.

> > +/*
> > + * 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.

> > +
> > +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/

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCHv3 5/5] misc: gehc-achc: new driver

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 4835 bytes --]

Hi Sebastian,

I love your patch! Perhaps something to improve:

[auto build test WARNING on robh/for-next]
[also build test WARNING on char-misc/char-misc-testing v5.13-rc3]
[cannot apply to spi/for-next next-20210528]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Sebastian-Reichel/GE-Healthcare-PPD-firmware-upgrade-driver-for-ACHC/20210528-193816
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
config: nds32-randconfig-p002-20210528 (attached as .config)
compiler: nds32le-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/dac014da90e2715a80e4f7139ac40333cd3d4bec
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Sebastian-Reichel/GE-Healthcare-PPD-firmware-upgrade-driver-for-ACHC/20210528-193816
        git checkout dac014da90e2715a80e4f7139ac40333cd3d4bec
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=nds32 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/spi/spidev.c:730:5: warning: no previous prototype for 'spidev_probe' [-Wmissing-prototypes]
     730 | int spidev_probe(struct spi_device *spi)
         |     ^~~~~~~~~~~~
>> drivers/spi/spidev.c:793:5: warning: no previous prototype for 'spidev_remove' [-Wmissing-prototypes]
     793 | int spidev_remove(struct spi_device *spi)
         |     ^~~~~~~~~~~~~


vim +/spidev_probe +730 drivers/spi/spidev.c

   729	
 > 730	int spidev_probe(struct spi_device *spi)
   731	{
   732		struct spidev_data	*spidev;
   733		int			status;
   734		unsigned long		minor;
   735	
   736		/*
   737		 * spidev should never be referenced in DT without a specific
   738		 * compatible string, it is a Linux implementation thing
   739		 * rather than a description of the hardware.
   740		 */
   741		WARN(spi->dev.of_node &&
   742		     of_device_is_compatible(spi->dev.of_node, "spidev"),
   743		     "%pOF: buggy DT: spidev listed directly in DT\n", spi->dev.of_node);
   744	
   745		spidev_probe_acpi(spi);
   746	
   747		/* Allocate driver data */
   748		spidev = kzalloc(sizeof(*spidev), GFP_KERNEL);
   749		if (!spidev)
   750			return -ENOMEM;
   751	
   752		/* Initialize the driver data */
   753		spidev->spi = spi;
   754		spin_lock_init(&spidev->spi_lock);
   755		mutex_init(&spidev->buf_lock);
   756	
   757		INIT_LIST_HEAD(&spidev->device_entry);
   758	
   759		/* If we can allocate a minor number, hook up this device.
   760		 * Reusing minors is fine so long as udev or mdev is working.
   761		 */
   762		mutex_lock(&device_list_lock);
   763		minor = find_first_zero_bit(minors, N_SPI_MINORS);
   764		if (minor < N_SPI_MINORS) {
   765			struct device *dev;
   766	
   767			spidev->devt = MKDEV(SPIDEV_MAJOR, minor);
   768			dev = device_create(spidev_class, &spi->dev, spidev->devt,
   769					    spidev, "spidev%d.%d",
   770					    spi->master->bus_num, spi->chip_select);
   771			status = PTR_ERR_OR_ZERO(dev);
   772		} else {
   773			dev_dbg(&spi->dev, "no minor number available!\n");
   774			status = -ENODEV;
   775		}
   776		if (status == 0) {
   777			set_bit(minor, minors);
   778			list_add(&spidev->device_entry, &device_list);
   779		}
   780		mutex_unlock(&device_list_lock);
   781	
   782		spidev->speed_hz = spi->max_speed_hz;
   783	
   784		if (status == 0)
   785			spi_set_drvdata(spi, spidev);
   786		else
   787			kfree(spidev);
   788	
   789		return status;
   790	}
   791	EXPORT_SYMBOL_GPL(spidev_probe);
   792	
 > 793	int spidev_remove(struct spi_device *spi)
   794	{
   795		struct spidev_data	*spidev = spi_get_drvdata(spi);
   796	
   797		/* prevent new opens */
   798		mutex_lock(&device_list_lock);
   799		/* make sure ops on existing fds can abort cleanly */
   800		spin_lock_irq(&spidev->spi_lock);
   801		spidev->spi = NULL;
   802		spin_unlock_irq(&spidev->spi_lock);
   803	
   804		list_del(&spidev->device_entry);
   805		device_destroy(spidev_class, spidev->devt);
   806		clear_bit(MINOR(spidev->devt), minors);
   807		if (spidev->users == 0)
   808			kfree(spidev);
   809		mutex_unlock(&device_list_lock);
   810	
   811		return 0;
   812	}
   813	EXPORT_SYMBOL_GPL(spidev_remove);
   814	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 24046 bytes --]

Re: [PATCHv3 5/5] misc: gehc-achc: new driver

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 4967 bytes --]

Hi Sebastian,

I love your patch! Perhaps something to improve:

[auto build test WARNING on robh/for-next]
[also build test WARNING on char-misc/char-misc-testing v5.13-rc3]
[cannot apply to spi/for-next next-20210528]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Sebastian-Reichel/GE-Healthcare-PPD-firmware-upgrade-driver-for-ACHC/20210528-193816
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
config: nds32-randconfig-p002-20210528 (attached as .config)
compiler: nds32le-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/dac014da90e2715a80e4f7139ac40333cd3d4bec
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Sebastian-Reichel/GE-Healthcare-PPD-firmware-upgrade-driver-for-ACHC/20210528-193816
        git checkout dac014da90e2715a80e4f7139ac40333cd3d4bec
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=nds32 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/spi/spidev.c:730:5: warning: no previous prototype for 'spidev_probe' [-Wmissing-prototypes]
     730 | int spidev_probe(struct spi_device *spi)
         |     ^~~~~~~~~~~~
>> drivers/spi/spidev.c:793:5: warning: no previous prototype for 'spidev_remove' [-Wmissing-prototypes]
     793 | int spidev_remove(struct spi_device *spi)
         |     ^~~~~~~~~~~~~


vim +/spidev_probe +730 drivers/spi/spidev.c

   729	
 > 730	int spidev_probe(struct spi_device *spi)
   731	{
   732		struct spidev_data	*spidev;
   733		int			status;
   734		unsigned long		minor;
   735	
   736		/*
   737		 * spidev should never be referenced in DT without a specific
   738		 * compatible string, it is a Linux implementation thing
   739		 * rather than a description of the hardware.
   740		 */
   741		WARN(spi->dev.of_node &&
   742		     of_device_is_compatible(spi->dev.of_node, "spidev"),
   743		     "%pOF: buggy DT: spidev listed directly in DT\n", spi->dev.of_node);
   744	
   745		spidev_probe_acpi(spi);
   746	
   747		/* Allocate driver data */
   748		spidev = kzalloc(sizeof(*spidev), GFP_KERNEL);
   749		if (!spidev)
   750			return -ENOMEM;
   751	
   752		/* Initialize the driver data */
   753		spidev->spi = spi;
   754		spin_lock_init(&spidev->spi_lock);
   755		mutex_init(&spidev->buf_lock);
   756	
   757		INIT_LIST_HEAD(&spidev->device_entry);
   758	
   759		/* If we can allocate a minor number, hook up this device.
   760		 * Reusing minors is fine so long as udev or mdev is working.
   761		 */
   762		mutex_lock(&device_list_lock);
   763		minor = find_first_zero_bit(minors, N_SPI_MINORS);
   764		if (minor < N_SPI_MINORS) {
   765			struct device *dev;
   766	
   767			spidev->devt = MKDEV(SPIDEV_MAJOR, minor);
   768			dev = device_create(spidev_class, &spi->dev, spidev->devt,
   769					    spidev, "spidev%d.%d",
   770					    spi->master->bus_num, spi->chip_select);
   771			status = PTR_ERR_OR_ZERO(dev);
   772		} else {
   773			dev_dbg(&spi->dev, "no minor number available!\n");
   774			status = -ENODEV;
   775		}
   776		if (status == 0) {
   777			set_bit(minor, minors);
   778			list_add(&spidev->device_entry, &device_list);
   779		}
   780		mutex_unlock(&device_list_lock);
   781	
   782		spidev->speed_hz = spi->max_speed_hz;
   783	
   784		if (status == 0)
   785			spi_set_drvdata(spi, spidev);
   786		else
   787			kfree(spidev);
   788	
   789		return status;
   790	}
   791	EXPORT_SYMBOL_GPL(spidev_probe);
   792	
 > 793	int spidev_remove(struct spi_device *spi)
   794	{
   795		struct spidev_data	*spidev = spi_get_drvdata(spi);
   796	
   797		/* prevent new opens */
   798		mutex_lock(&device_list_lock);
   799		/* make sure ops on existing fds can abort cleanly */
   800		spin_lock_irq(&spidev->spi_lock);
   801		spidev->spi = NULL;
   802		spin_unlock_irq(&spidev->spi_lock);
   803	
   804		list_del(&spidev->device_entry);
   805		device_destroy(spidev_class, spidev->devt);
   806		clear_bit(MINOR(spidev->devt), minors);
   807		if (spidev->users == 0)
   808			kfree(spidev);
   809		mutex_unlock(&device_list_lock);
   810	
   811		return 0;
   812	}
   813	EXPORT_SYMBOL_GPL(spidev_remove);
   814	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 24046 bytes --]

Re: [PATCHv3 5/5] misc: gehc-achc: new driver

[Greg Kroah-Hartman]

On Fri, May 28, 2021 at 04:06:54PM +0200, Sebastian Reichel wrote:
> > > +	/*
> > > +	 * 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/

Kind of, it's still not good to do this "by hand", the driver core
should do it for you.

> 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.

Yes, I need to just get rid of that call entirely.  I'll sick one of my
interns on it...

thanks,

greg k-h

Re: [PATCHv3 5/5] misc: gehc-achc: new driver

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 10887 bytes --]

Hi Sebastian,

I love your patch! Yet something to improve:

[auto build test ERROR on robh/for-next]
[also build test ERROR on char-misc/char-misc-testing v5.13-rc3]
[cannot apply to spi/for-next next-20210528]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Sebastian-Reichel/GE-Healthcare-PPD-firmware-upgrade-driver-for-ACHC/20210528-193816
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
config: ia64-allmodconfig (attached as .config)
compiler: ia64-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/dac014da90e2715a80e4f7139ac40333cd3d4bec
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Sebastian-Reichel/GE-Healthcare-PPD-firmware-upgrade-driver-for-ACHC/20210528-193816
        git checkout dac014da90e2715a80e4f7139ac40333cd3d4bec
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=ia64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All error/warnings (new ones prefixed by >>):

   In file included from include/linux/printk.h:409,
                    from include/linux/kernel.h:17,
                    from include/linux/delay.h:22,
                    from drivers/misc/nxp-ezport.c:12:
   drivers/misc/nxp-ezport.c: In function 'ezport_flash_transfer':
>> drivers/misc/nxp-ezport.c:221:21: warning: format '%u' expects argument of type 'unsigned int', but argument 4 has type 'size_t' {aka 'long unsigned int'} [-Wformat=]
     221 |  dev_dbg(&spi->dev, "EzPort write %u bytes @ 0x%06x...\n", payload_size, address);
         |                     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/dynamic_debug.h:129:15: note: in definition of macro '__dynamic_func_call'
     129 |   func(&id, ##__VA_ARGS__);  \
         |               ^~~~~~~~~~~
   include/linux/dynamic_debug.h:161:2: note: in expansion of macro '_dynamic_func_call'
     161 |  _dynamic_func_call(fmt,__dynamic_dev_dbg,   \
         |  ^~~~~~~~~~~~~~~~~~
   include/linux/dev_printk.h:123:2: note: in expansion of macro 'dynamic_dev_dbg'
     123 |  dynamic_dev_dbg(dev, dev_fmt(fmt), ##__VA_ARGS__)
         |  ^~~~~~~~~~~~~~~
   include/linux/dev_printk.h:123:23: note: in expansion of macro 'dev_fmt'
     123 |  dynamic_dev_dbg(dev, dev_fmt(fmt), ##__VA_ARGS__)
         |                       ^~~~~~~
   drivers/misc/nxp-ezport.c:221:2: note: in expansion of macro 'dev_dbg'
     221 |  dev_dbg(&spi->dev, "EzPort write %u bytes @ 0x%06x...\n", payload_size, address);
         |  ^~~~~~~
   drivers/misc/nxp-ezport.c:221:36: note: format string is defined here
     221 |  dev_dbg(&spi->dev, "EzPort write %u bytes @ 0x%06x...\n", payload_size, address);
         |                                   ~^
         |                                    |
         |                                    unsigned int
         |                                   %lu
   In file included from include/linux/printk.h:409,
                    from include/linux/kernel.h:17,
                    from include/linux/delay.h:22,
                    from drivers/misc/nxp-ezport.c:12:
   drivers/misc/nxp-ezport.c: In function 'ezport_firmware_compare_data':
   drivers/misc/nxp-ezport.c:292:21: warning: format '%u' expects argument of type 'unsigned int', but argument 4 has type 'size_t' {aka 'long unsigned int'} [-Wformat=]
     292 |  dev_dbg(&spi->dev, "EzPort compare data with %u bytes...\n", size);
         |                     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/dynamic_debug.h:129:15: note: in definition of macro '__dynamic_func_call'
     129 |   func(&id, ##__VA_ARGS__);  \
         |               ^~~~~~~~~~~
   include/linux/dynamic_debug.h:161:2: note: in expansion of macro '_dynamic_func_call'
     161 |  _dynamic_func_call(fmt,__dynamic_dev_dbg,   \
         |  ^~~~~~~~~~~~~~~~~~
   include/linux/dev_printk.h:123:2: note: in expansion of macro 'dynamic_dev_dbg'
     123 |  dynamic_dev_dbg(dev, dev_fmt(fmt), ##__VA_ARGS__)
         |  ^~~~~~~~~~~~~~~
   include/linux/dev_printk.h:123:23: note: in expansion of macro 'dev_fmt'
     123 |  dynamic_dev_dbg(dev, dev_fmt(fmt), ##__VA_ARGS__)
         |                       ^~~~~~~
   drivers/misc/nxp-ezport.c:292:2: note: in expansion of macro 'dev_dbg'
     292 |  dev_dbg(&spi->dev, "EzPort compare data with %u bytes...\n", size);
         |  ^~~~~~~
   drivers/misc/nxp-ezport.c:292:48: note: format string is defined here
     292 |  dev_dbg(&spi->dev, "EzPort compare data with %u bytes...\n", size);
         |                                               ~^
         |                                                |
         |                                                unsigned int
         |                                               %lu
   In file included from include/linux/kernel.h:15,
                    from include/linux/delay.h:22,
                    from drivers/misc/nxp-ezport.c:12:
   include/linux/minmax.h:18:28: warning: comparison of distinct pointer types lacks a cast
      18 |  (!!(sizeof((typeof(x) *)1 == (typeof(y) *)1)))
         |                            ^~
   include/linux/minmax.h:32:4: note: in expansion of macro '__typecheck'
      32 |   (__typecheck(x, y) && __no_side_effects(x, y))
         |    ^~~~~~~~~~~
   include/linux/minmax.h:42:24: note: in expansion of macro '__safe_cmp'
      42 |  __builtin_choose_expr(__safe_cmp(x, y), \
         |                        ^~~~~~~~~~
   include/linux/minmax.h:51:19: note: in expansion of macro '__careful_cmp'
      51 | #define min(x, y) __careful_cmp(x, y, <)
         |                   ^~~~~~~~~~~~~
   drivers/misc/nxp-ezport.c:305:19: note: in expansion of macro 'min'
     305 |   transfer_size = min((u32) EZPORT_TRANSFER_SIZE, size - address);
         |                   ^~~
   In file included from include/linux/printk.h:409,
                    from include/linux/kernel.h:17,
                    from include/linux/delay.h:22,
                    from drivers/misc/nxp-ezport.c:12:
   drivers/misc/nxp-ezport.c: In function 'ezport_firmware_flash_data':
   drivers/misc/nxp-ezport.c:324:21: warning: format '%u' expects argument of type 'unsigned int', but argument 4 has type 'size_t' {aka 'long unsigned int'} [-Wformat=]
     324 |  dev_dbg(&spi->dev, "EzPort flash data with %u bytes...\n", size);
         |                     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/dynamic_debug.h:129:15: note: in definition of macro '__dynamic_func_call'
     129 |   func(&id, ##__VA_ARGS__);  \
         |               ^~~~~~~~~~~
   include/linux/dynamic_debug.h:161:2: note: in expansion of macro '_dynamic_func_call'
     161 |  _dynamic_func_call(fmt,__dynamic_dev_dbg,   \
         |  ^~~~~~~~~~~~~~~~~~
   include/linux/dev_printk.h:123:2: note: in expansion of macro 'dynamic_dev_dbg'
     123 |  dynamic_dev_dbg(dev, dev_fmt(fmt), ##__VA_ARGS__)
         |  ^~~~~~~~~~~~~~~
   include/linux/dev_printk.h:123:23: note: in expansion of macro 'dev_fmt'
     123 |  dynamic_dev_dbg(dev, dev_fmt(fmt), ##__VA_ARGS__)
         |                       ^~~~~~~
   drivers/misc/nxp-ezport.c:324:2: note: in expansion of macro 'dev_dbg'
     324 |  dev_dbg(&spi->dev, "EzPort flash data with %u bytes...\n", size);
         |  ^~~~~~~
   drivers/misc/nxp-ezport.c:324:46: note: format string is defined here
     324 |  dev_dbg(&spi->dev, "EzPort flash data with %u bytes...\n", size);
         |                                             ~^
         |                                              |
         |                                              unsigned int
         |                                             %lu
   In file included from include/linux/kernel.h:15,
--
   drivers/misc/gehc-achc.c: In function 'gehc_achc_probe':
>> drivers/misc/gehc-achc.c:123:8: error: implicit declaration of function 'spidev_probe' [-Werror=implicit-function-declaration]
     123 |  ret = spidev_probe(spi);
         |        ^~~~~~~~~~~~
   drivers/misc/gehc-achc.c: In function 'gehc_achc_remove':
>> drivers/misc/gehc-achc.c:132:9: error: implicit declaration of function 'spidev_remove'; did you mean 'idr_remove'? [-Werror=implicit-function-declaration]
     132 |  return spidev_remove(spi);
         |         ^~~~~~~~~~~~~
         |         idr_remove
   cc1: some warnings being treated as errors


vim +/spidev_probe +123 drivers/misc/gehc-achc.c

    77	
    78	static int gehc_achc_probe(struct spi_device *spi)
    79	{
    80		struct achc_data *achc;
    81		int ezport_reg, ret;
    82	
    83		spi->max_speed_hz = ACHC_MAX_FREQ;
    84		spi->bits_per_word = 8;
    85		spi->mode = SPI_MODE_0;
    86	
    87		achc = devm_kzalloc(&spi->dev, sizeof(*achc), GFP_KERNEL);
    88		if (!achc)
    89			return -ENOMEM;
    90		achc->main = spi;
    91	
    92		mutex_init(&achc->device_lock);
    93	
    94		ret = of_property_read_u32_index(spi->dev.of_node, "reg", 1, &ezport_reg);
    95		if (ret)
    96			return dev_err_probe(&spi->dev, ret, "missing second reg entry!\n");
    97	
    98		achc->ezport = spi_new_ancillary_device(spi, ezport_reg);
    99		if (IS_ERR(achc->ezport))
   100			return PTR_ERR(achc->ezport);
   101	
   102		ret = devm_add_action_or_reset(&spi->dev, unregister_ezport, achc->ezport);
   103		if (ret)
   104			return ret;
   105	
   106		achc->reset = devm_gpiod_get(&spi->dev, "reset", GPIOD_OUT_LOW);
   107		if (IS_ERR(achc->reset))
   108			return dev_err_probe(&spi->dev, PTR_ERR(achc->reset), "Could not get reset gpio\n");
   109	
   110		/*
   111		 * The sysfs properties are bound to the dummy device, since the main device already
   112		 * uses drvdata assigned by the spidev driver.
   113		 */
   114		spi_set_drvdata(achc->ezport, achc);
   115		ret = devm_device_add_group(&achc->ezport->dev, &gehc_achc_attr_group);
   116		if (ret)
   117			return ret;
   118	
   119		/*
   120		 * Anything before this must use device managed resources to ensure resources being
   121		 * free'd in reverse allocation order.
   122		 */
 > 123		ret = spidev_probe(spi);
   124		if (ret)
   125			return ret;
   126	
   127		return 0;
   128	}
   129	
   130	static int gehc_achc_remove(struct spi_device *spi)
   131	{
 > 132		return spidev_remove(spi);
   133	}
   134	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 64449 bytes --]

Re: [PATCHv3 5/5] misc: gehc-achc: new driver

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 11102 bytes --]

Hi Sebastian,

I love your patch! Yet something to improve:

[auto build test ERROR on robh/for-next]
[also build test ERROR on char-misc/char-misc-testing v5.13-rc3]
[cannot apply to spi/for-next next-20210528]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Sebastian-Reichel/GE-Healthcare-PPD-firmware-upgrade-driver-for-ACHC/20210528-193816
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
config: ia64-allmodconfig (attached as .config)
compiler: ia64-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/dac014da90e2715a80e4f7139ac40333cd3d4bec
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Sebastian-Reichel/GE-Healthcare-PPD-firmware-upgrade-driver-for-ACHC/20210528-193816
        git checkout dac014da90e2715a80e4f7139ac40333cd3d4bec
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=ia64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All error/warnings (new ones prefixed by >>):

   In file included from include/linux/printk.h:409,
                    from include/linux/kernel.h:17,
                    from include/linux/delay.h:22,
                    from drivers/misc/nxp-ezport.c:12:
   drivers/misc/nxp-ezport.c: In function 'ezport_flash_transfer':
>> drivers/misc/nxp-ezport.c:221:21: warning: format '%u' expects argument of type 'unsigned int', but argument 4 has type 'size_t' {aka 'long unsigned int'} [-Wformat=]
     221 |  dev_dbg(&spi->dev, "EzPort write %u bytes @ 0x%06x...\n", payload_size, address);
         |                     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/dynamic_debug.h:129:15: note: in definition of macro '__dynamic_func_call'
     129 |   func(&id, ##__VA_ARGS__);  \
         |               ^~~~~~~~~~~
   include/linux/dynamic_debug.h:161:2: note: in expansion of macro '_dynamic_func_call'
     161 |  _dynamic_func_call(fmt,__dynamic_dev_dbg,   \
         |  ^~~~~~~~~~~~~~~~~~
   include/linux/dev_printk.h:123:2: note: in expansion of macro 'dynamic_dev_dbg'
     123 |  dynamic_dev_dbg(dev, dev_fmt(fmt), ##__VA_ARGS__)
         |  ^~~~~~~~~~~~~~~
   include/linux/dev_printk.h:123:23: note: in expansion of macro 'dev_fmt'
     123 |  dynamic_dev_dbg(dev, dev_fmt(fmt), ##__VA_ARGS__)
         |                       ^~~~~~~
   drivers/misc/nxp-ezport.c:221:2: note: in expansion of macro 'dev_dbg'
     221 |  dev_dbg(&spi->dev, "EzPort write %u bytes @ 0x%06x...\n", payload_size, address);
         |  ^~~~~~~
   drivers/misc/nxp-ezport.c:221:36: note: format string is defined here
     221 |  dev_dbg(&spi->dev, "EzPort write %u bytes @ 0x%06x...\n", payload_size, address);
         |                                   ~^
         |                                    |
         |                                    unsigned int
         |                                   %lu
   In file included from include/linux/printk.h:409,
                    from include/linux/kernel.h:17,
                    from include/linux/delay.h:22,
                    from drivers/misc/nxp-ezport.c:12:
   drivers/misc/nxp-ezport.c: In function 'ezport_firmware_compare_data':
   drivers/misc/nxp-ezport.c:292:21: warning: format '%u' expects argument of type 'unsigned int', but argument 4 has type 'size_t' {aka 'long unsigned int'} [-Wformat=]
     292 |  dev_dbg(&spi->dev, "EzPort compare data with %u bytes...\n", size);
         |                     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/dynamic_debug.h:129:15: note: in definition of macro '__dynamic_func_call'
     129 |   func(&id, ##__VA_ARGS__);  \
         |               ^~~~~~~~~~~
   include/linux/dynamic_debug.h:161:2: note: in expansion of macro '_dynamic_func_call'
     161 |  _dynamic_func_call(fmt,__dynamic_dev_dbg,   \
         |  ^~~~~~~~~~~~~~~~~~
   include/linux/dev_printk.h:123:2: note: in expansion of macro 'dynamic_dev_dbg'
     123 |  dynamic_dev_dbg(dev, dev_fmt(fmt), ##__VA_ARGS__)
         |  ^~~~~~~~~~~~~~~
   include/linux/dev_printk.h:123:23: note: in expansion of macro 'dev_fmt'
     123 |  dynamic_dev_dbg(dev, dev_fmt(fmt), ##__VA_ARGS__)
         |                       ^~~~~~~
   drivers/misc/nxp-ezport.c:292:2: note: in expansion of macro 'dev_dbg'
     292 |  dev_dbg(&spi->dev, "EzPort compare data with %u bytes...\n", size);
         |  ^~~~~~~
   drivers/misc/nxp-ezport.c:292:48: note: format string is defined here
     292 |  dev_dbg(&spi->dev, "EzPort compare data with %u bytes...\n", size);
         |                                               ~^
         |                                                |
         |                                                unsigned int
         |                                               %lu
   In file included from include/linux/kernel.h:15,
                    from include/linux/delay.h:22,
                    from drivers/misc/nxp-ezport.c:12:
   include/linux/minmax.h:18:28: warning: comparison of distinct pointer types lacks a cast
      18 |  (!!(sizeof((typeof(x) *)1 == (typeof(y) *)1)))
         |                            ^~
   include/linux/minmax.h:32:4: note: in expansion of macro '__typecheck'
      32 |   (__typecheck(x, y) && __no_side_effects(x, y))
         |    ^~~~~~~~~~~
   include/linux/minmax.h:42:24: note: in expansion of macro '__safe_cmp'
      42 |  __builtin_choose_expr(__safe_cmp(x, y), \
         |                        ^~~~~~~~~~
   include/linux/minmax.h:51:19: note: in expansion of macro '__careful_cmp'
      51 | #define min(x, y) __careful_cmp(x, y, <)
         |                   ^~~~~~~~~~~~~
   drivers/misc/nxp-ezport.c:305:19: note: in expansion of macro 'min'
     305 |   transfer_size = min((u32) EZPORT_TRANSFER_SIZE, size - address);
         |                   ^~~
   In file included from include/linux/printk.h:409,
                    from include/linux/kernel.h:17,
                    from include/linux/delay.h:22,
                    from drivers/misc/nxp-ezport.c:12:
   drivers/misc/nxp-ezport.c: In function 'ezport_firmware_flash_data':
   drivers/misc/nxp-ezport.c:324:21: warning: format '%u' expects argument of type 'unsigned int', but argument 4 has type 'size_t' {aka 'long unsigned int'} [-Wformat=]
     324 |  dev_dbg(&spi->dev, "EzPort flash data with %u bytes...\n", size);
         |                     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/dynamic_debug.h:129:15: note: in definition of macro '__dynamic_func_call'
     129 |   func(&id, ##__VA_ARGS__);  \
         |               ^~~~~~~~~~~
   include/linux/dynamic_debug.h:161:2: note: in expansion of macro '_dynamic_func_call'
     161 |  _dynamic_func_call(fmt,__dynamic_dev_dbg,   \
         |  ^~~~~~~~~~~~~~~~~~
   include/linux/dev_printk.h:123:2: note: in expansion of macro 'dynamic_dev_dbg'
     123 |  dynamic_dev_dbg(dev, dev_fmt(fmt), ##__VA_ARGS__)
         |  ^~~~~~~~~~~~~~~
   include/linux/dev_printk.h:123:23: note: in expansion of macro 'dev_fmt'
     123 |  dynamic_dev_dbg(dev, dev_fmt(fmt), ##__VA_ARGS__)
         |                       ^~~~~~~
   drivers/misc/nxp-ezport.c:324:2: note: in expansion of macro 'dev_dbg'
     324 |  dev_dbg(&spi->dev, "EzPort flash data with %u bytes...\n", size);
         |  ^~~~~~~
   drivers/misc/nxp-ezport.c:324:46: note: format string is defined here
     324 |  dev_dbg(&spi->dev, "EzPort flash data with %u bytes...\n", size);
         |                                             ~^
         |                                              |
         |                                              unsigned int
         |                                             %lu
   In file included from include/linux/kernel.h:15,
--
   drivers/misc/gehc-achc.c: In function 'gehc_achc_probe':
>> drivers/misc/gehc-achc.c:123:8: error: implicit declaration of function 'spidev_probe' [-Werror=implicit-function-declaration]
     123 |  ret = spidev_probe(spi);
         |        ^~~~~~~~~~~~
   drivers/misc/gehc-achc.c: In function 'gehc_achc_remove':
>> drivers/misc/gehc-achc.c:132:9: error: implicit declaration of function 'spidev_remove'; did you mean 'idr_remove'? [-Werror=implicit-function-declaration]
     132 |  return spidev_remove(spi);
         |         ^~~~~~~~~~~~~
         |         idr_remove
   cc1: some warnings being treated as errors


vim +/spidev_probe +123 drivers/misc/gehc-achc.c

    77	
    78	static int gehc_achc_probe(struct spi_device *spi)
    79	{
    80		struct achc_data *achc;
    81		int ezport_reg, ret;
    82	
    83		spi->max_speed_hz = ACHC_MAX_FREQ;
    84		spi->bits_per_word = 8;
    85		spi->mode = SPI_MODE_0;
    86	
    87		achc = devm_kzalloc(&spi->dev, sizeof(*achc), GFP_KERNEL);
    88		if (!achc)
    89			return -ENOMEM;
    90		achc->main = spi;
    91	
    92		mutex_init(&achc->device_lock);
    93	
    94		ret = of_property_read_u32_index(spi->dev.of_node, "reg", 1, &ezport_reg);
    95		if (ret)
    96			return dev_err_probe(&spi->dev, ret, "missing second reg entry!\n");
    97	
    98		achc->ezport = spi_new_ancillary_device(spi, ezport_reg);
    99		if (IS_ERR(achc->ezport))
   100			return PTR_ERR(achc->ezport);
   101	
   102		ret = devm_add_action_or_reset(&spi->dev, unregister_ezport, achc->ezport);
   103		if (ret)
   104			return ret;
   105	
   106		achc->reset = devm_gpiod_get(&spi->dev, "reset", GPIOD_OUT_LOW);
   107		if (IS_ERR(achc->reset))
   108			return dev_err_probe(&spi->dev, PTR_ERR(achc->reset), "Could not get reset gpio\n");
   109	
   110		/*
   111		 * The sysfs properties are bound to the dummy device, since the main device already
   112		 * uses drvdata assigned by the spidev driver.
   113		 */
   114		spi_set_drvdata(achc->ezport, achc);
   115		ret = devm_device_add_group(&achc->ezport->dev, &gehc_achc_attr_group);
   116		if (ret)
   117			return ret;
   118	
   119		/*
   120		 * Anything before this must use device managed resources to ensure resources being
   121		 * free'd in reverse allocation order.
   122		 */
 > 123		ret = spidev_probe(spi);
   124		if (ret)
   125			return ret;
   126	
   127		return 0;
   128	}
   129	
   130	static int gehc_achc_remove(struct spi_device *spi)
   131	{
 > 132		return spidev_remove(spi);
   133	}
   134	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 64449 bytes --]

Re: [PATCHv3 3/5] dt-bindings: misc: ge-achc: Convert to DT schema format

[Rob Herring]

On Fri, 28 May 2021 13:33:45 +0200, Sebastian Reichel wrote:
> Convert the binding to DT schema format. Also update the binding
> to fix shortcomings
> 
>  * Add "nxp,kinetis-k20" fallback compatible
>  * add programming SPI interface and reset GPIO
>  * add main clock
>  * add voltage supplies
>  * drop spi-max-frequency from required properties,
>    driver will setup max. frequency
> 
> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> ---
>  .../devicetree/bindings/misc/ge-achc.txt      | 26 -------
>  .../devicetree/bindings/misc/ge-achc.yaml     | 67 +++++++++++++++++++
>  2 files changed, 67 insertions(+), 26 deletions(-)
>  delete mode 100644 Documentation/devicetree/bindings/misc/ge-achc.txt
>  create mode 100644 Documentation/devicetree/bindings/misc/ge-achc.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/misc/ge-achc.yaml: properties:reg:minItems: False schema does not allow 2
	hint: "minItems/maxItems" equal to the "items" list length are not necessary
	from schema $id: http://devicetree.org/meta-schemas/items.yaml#
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/misc/ge-achc.yaml: properties:reg:maxItems: False schema does not allow 2
	hint: "minItems/maxItems" equal to the "items" list length are not necessary
	from schema $id: http://devicetree.org/meta-schemas/items.yaml#
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/misc/ge-achc.yaml: ignoring, error in schema: properties: reg: minItems
warning: no schema found in file: ./Documentation/devicetree/bindings/misc/ge-achc.yaml
Documentation/devicetree/bindings/misc/ge-achc.example.dt.yaml:0:0: /example-0/spi/spi@1: failed to match any schema with compatible: ['ge,achc', 'nxp,kinetis-k20']
Documentation/devicetree/bindings/misc/ge-achc.example.dt.yaml:0:0: /example-0/spi/spi@1: failed to match any schema with compatible: ['ge,achc', 'nxp,kinetis-k20']

See https://patchwork.ozlabs.org/patch/1485219

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.

Re: [PATCHv3 2/5] spi: dt-bindings: support devices with multiple chipselects

[Rob Herring]

On Fri, 28 May 2021 13:33:44 +0200, Sebastian Reichel wrote:
> Add binding support for devices, that have more than one
> chip select. A typical example are SPI connected microcontroller,
> that can also be programmed over SPI like NXP Kinetis or
> chips with a configuration and a data chip select, such as
> Microchip's MRF89XA transceiver.
> 
> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> ---
>  Documentation/devicetree/bindings/spi/spi-controller.yaml | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCHv3 3/5] dt-bindings: misc: ge-achc: Convert to DT schema format

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 422 bytes --]

On Fri, May 28, 2021 at 01:33:45PM +0200, Sebastian Reichel wrote:

> -Required SPI properties:
> -
> -- reg : Should be address of the device chip select within
> -  the controller.

There is an existing binding...

> +  reg:
> +    minItems: 2
> +    maxItems: 2
> +    items:
> +      - description: Control interface
> +      - description: Firmware programming interface

...but this new one is incompatible with it.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCHv3 5/5] misc: gehc-achc: new driver

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1969 bytes --]

On Fri, May 28, 2021 at 01:33:47PM +0200, Sebastian Reichel wrote:

>  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 +

This clearly seems to be something that should be split into separate
patches, even without getting as far as the code it's hard to see how
changes to spidev.c wouldn't be standalone changes that should be split
out from what looks like adding two new drivers.

> +static int gehc_achc_probe(struct spi_device *spi)
> +{

...

> +       ret = spidev_probe(spi);
> +       if (ret)
> +               return ret;

This isn't integrated at all with the flashing code, userspace might be
trying to do something with the device while flashing happens.  I would
expect at a very minimum that we'd be hot unplugging the spidev while
the flashing is in progress.  

> -static int spidev_probe(struct spi_device *spi)
> +int spidev_probe(struct spi_device *spi)
>  {
>  	struct spidev_data	*spidev;
>  	int			status;
> @@ -789,8 +788,9 @@ static int spidev_probe(struct spi_device *spi)
>  
>  	return status;
>  }
> +EXPORT_SYMBOL_GPL(spidev_probe);

This really looks like a very, very bad idea in general - attempting to
combine the unmediated userspace access that spidev offers with an in
kernel driver seems like it's not going to go well, as with the flashing
issue.  Really this looks like two marginally related devices that are
getting glued together which is abot the best case for this being
supportable and even then it'd need more work.  I'm not keen about
providing the interface at all as it's the sort of thing people are
going to get wrong a bunch.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCHv3 3/5] dt-bindings: misc: ge-achc: Convert to DT schema format

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 1256 bytes --]

Hi,

On Wed, Jun 09, 2021 at 12:47:47PM +0100, Mark Brown wrote:
> On Fri, May 28, 2021 at 01:33:45PM +0200, Sebastian Reichel wrote:
> 
> > -Required SPI properties:
> > -
> > -- reg : Should be address of the device chip select within
> > -  the controller.
> 
> There is an existing binding...
> 
> > +  reg:
> > +    minItems: 2
> > +    maxItems: 2
> > +    items:
> > +      - description: Control interface
> > +      - description: Firmware programming interface
> 
> ...but this new one is incompatible with it.

The current binding is broken, since it uses the same compatible
value for two different SPI "devices" (main SPI interface and
EzPort). It only "works" because compatible string is only used
to export the device to userspace via spidev and otherwise ignored.

But the main spidev is not used by current FW and FW update cannot
be done from userspace because it requires reset pin handling during
spi transactions. So basically upstream support for this binding is
completley broken anyways (downstream has additional patches to work
around the limitations).

In PATCHv2 it was pointed out, that there should be one device for
both chip selects, since the reset/clocks/... are shared.

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]