Re: [PATCHv4 6/6] misc: gehc-achc: new driver
From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Date: 2021-06-11 09:03:19
Also in:
linux-spi, lkml
On Wed, Jun 09, 2021 at 05:12:35PM +0200, Sebastian Reichel wrote:
quoted hunk ↗ jump to hunk
General Electric Healthcare's PPD has a secondary processor from NXP's Kinetis K20 series. That device has two SPI chip selects: The main interface's behaviour depends on the loaded firmware and is currently unused. The secondary interface can be used to update the firmware using EzPort protocol. This is implemented by this driver using the kernel's firmware API. It's not done during probe time, since the device has non-volatile memory and flashing lasts almost 3 minutes. Signed-off-by: Sebastian Reichel <redacted> --- .../ABI/testing/sysfs-driver-ge-achc | 9 ++ drivers/misc/Kconfig | 12 ++ drivers/misc/Makefile | 1 + drivers/misc/gehc-achc.c | 136 ++++++++++++++++++ drivers/spi/spidev.c | 1 - 5 files changed, 158 insertions(+), 1 deletion(-) create mode 100644 Documentation/ABI/testing/sysfs-driver-ge-achc create mode 100644 drivers/misc/gehc-achc.cdiff --git a/Documentation/ABI/testing/sysfs-driver-ge-achc b/Documentation/ABI/testing/sysfs-driver-ge-achc new file mode 100644 index 000000000000..3ca71a1b4836 --- /dev/null +++ b/Documentation/ABI/testing/sysfs-driver-ge-achc@@ -0,0 +1,9 @@ +What: /sys/bus/spi/drivers/gehc-achc/<dev>/update_firmware
Shouldn't the path be through the bus and not through the driver portion of sysfs?
+Date: June 2021 +Contact: linux-kernel@vger.kernel.org
If you are not willing to put your name on this, I'm not willing to take this :(
+Description: Write 1 to this file to trigger a firmware update.
What exactly does this mean? What will be "triggered" and what will happen? And it is not what the kernel code does, it will take any write to this file, not just "1" so something is wrong here.
+ +What: /sys/bus/spi/drivers/gehc-achc/<dev>/reset +Date: June 2021 +Contact: linux-kernel@vger.kernel.org
Again, real name for a contact for obvious reasons.
+Description: Write 1 to this file to trigger a device reset.
Also does not match the code :(
quoted hunk ↗ jump to hunk
diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig index 392c091891a1..a286f0775f2e 100644 --- a/drivers/misc/Kconfig +++ b/drivers/misc/Kconfig@@ -211,6 +211,18 @@ config CS5535_CLOCK_EVENT_SRC 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 + PPD device. + + To compile this driver as a module, choose M here: the + module will be called gehc-achc. + config HP_ILO tristate "Channel interface driver for the HP iLO processor" depends on PCIdiff --git a/drivers/misc/Makefile b/drivers/misc/Makefile index a85e763b9b33..b7bad5a16c8f 100644 --- a/drivers/misc/Makefile +++ b/drivers/misc/Makefile@@ -25,6 +25,7 @@ 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.odiff --git a/drivers/misc/gehc-achc.c b/drivers/misc/gehc-achc.c new file mode 100644 index 000000000000..5868ac99a3f6 --- /dev/null +++ b/drivers/misc/gehc-achc.c@@ -0,0 +1,136 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * datasheet: https://www.nxp.com/docs/en/data-sheet/K20P144M120SF3.pdf + * + * Copyright (C) 2018-2021 Collabora + * Copyright (C) 2018-2021 GE Healthcare + */ + +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/spi/spi.h> +#include <linux/of.h> +#include <linux/platform_data/nxp-ezport.h> + +#define ACHC_MAX_FREQ 300000
What is the units here?
+
+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;So just a non-empty string works? As said above, that's not what you documented :(
+
+ 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;Same here as above. thanks, greg k-h