From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To: Sebastian Reichel <sebastian.reichel@collabora.com>
Cc: Mark Brown <broonie@kernel.org>, Rob Herring <robh+dt@kernel.org>,
Arnd Bergmann <arnd@arndb.de>, Shawn Guo <shawnguo@kernel.org>,
Sascha Hauer <kernel@pengutronix.de>,
Fabio Estevam <festevam@gmail.com>, Ian Ray <ian.ray@ge.com>,
linux-kernel@vger.kernel.org, linux-spi@vger.kernel.org,
devicetree@vger.kernel.org, kernel@collabora.com
Subject: Re: [PATCHv6 3/3] misc: gehc-achc: new driver
Date: Mon, 12 Jul 2021 19:46:05 +0200 [thread overview]
Message-ID: <YOx/3YaIg24Tx+OQ@kroah.com> (raw)
In-Reply-To: <20210712150242.146545-4-sebastian.reichel@collabora.com>
On Mon, Jul 12, 2021 at 05:02:42PM +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 currently unused.
>
> The secondary interface can be used to update the firmware using
> EzPort protocol. This is implemented by this driver using the
> kernel's firmware API. It's not done during probe time, since
> the device has non-volatile memory and flashing lasts almost 3
> minutes.
In thinking about this some more, why does it matter? Spin up a
workqueue when probing and do the firmware loading then. That way you
do not end up creating yet another custom user/kernel api just to do
something as trivial as loading the firmware for a device.
And I think the firmware loader even handles async loading, or at least
it used to, maybe not anymore, it's a complex api, I recommend reading
the docs...
>
> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> ---
> .../ABI/testing/sysfs-driver-ge-achc | 14 +
> drivers/misc/Kconfig | 11 +
> drivers/misc/Makefile | 2 +
> drivers/misc/gehc-achc.c | 136 +++++
> drivers/misc/nxp-ezport.c | 468 ++++++++++++++++++
> drivers/misc/nxp-ezport.h | 9 +
Why is there two different modules here for the same piece of hardware?
> drivers/spi/spidev.c | 1 -
> 7 files changed, 640 insertions(+), 1 deletion(-)
> create mode 100644 Documentation/ABI/testing/sysfs-driver-ge-achc
> create mode 100644 drivers/misc/gehc-achc.c
> create mode 100644 drivers/misc/nxp-ezport.c
> create mode 100644 drivers/misc/nxp-ezport.h
>
> diff --git a/Documentation/ABI/testing/sysfs-driver-ge-achc b/Documentation/ABI/testing/sysfs-driver-ge-achc
> new file mode 100644
> index 000000000000..f02812731082
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-driver-ge-achc
> @@ -0,0 +1,14 @@
> +What: /sys/bus/spi/<dev>/update_firmware
> +Date: Jul 2021
> +Contact: sebastian.reichel@collabora.com
> +Description: Write 1 to this file to update the ACHC microcontroller
> + firmware via the EzPort interface. For this the kernel
> + will load "achc.bin" via the firmware API (so usually
> + from /lib/firmware). The write will block until the FW
> + has either been flashed successfully or an error occured.
> +
> +What: /sys/bus/spi/<dev>/reset
> +Date: Jul 2021
> +Contact: sebastian.reichel@collabora.com
> +Description: Write 1 to this file to reset the microcontroller via the
> + reset GPIO. The write will block until the reset completes.
> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> index f4fb5c52b863..1ebf7000671d 100644
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -208,6 +208,17 @@ config CS5535_CLOCK_EVENT_SRC
> MFGPTs have a better resolution and max interval than the
> generic PIT, and are suitable for use as high-res timers.
>
> +config GEHC_ACHC
> + tristate "GEHC ACHC support"
> + depends on SPI && SYSFS
> + select FW_LOADER
> + help
> + Support for GE ACHC microcontroller, that is part of the GE
> + PPD device.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called gehc-achc.
> +
> config HP_ILO
> tristate "Channel interface driver for the HP iLO processor"
> depends on PCI
> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
> index e92a56d4442f..b50180796931 100644
> --- a/drivers/misc/Makefile
> +++ b/drivers/misc/Makefile
> @@ -24,6 +24,8 @@ obj-$(CONFIG_KGDB_TESTS) += kgdbts.o
> obj-$(CONFIG_SGI_XP) += sgi-xp/
> obj-$(CONFIG_SGI_GRU) += sgi-gru/
> obj-$(CONFIG_CS5535_MFGPT) += cs5535-mfgpt.o
> +obj-$(CONFIG_GEHC_ACHC) += gehc-achc-mod.o
> +gehc-achc-mod-objs := gehc-achc.o nxp-ezport.o
Yeah, you are creating 2 modules for one Kconfig option, why not put
these together into 1? Makes the code smaller.
> obj-$(CONFIG_HP_ILO) += hpilo.o
> obj-$(CONFIG_APDS9802ALS) += apds9802als.o
> obj-$(CONFIG_ISL29003) += isl29003.o
> diff --git a/drivers/misc/gehc-achc.c b/drivers/misc/gehc-achc.c
> new file mode 100644
> index 000000000000..893940e7ca4e
> --- /dev/null
> +++ b/drivers/misc/gehc-achc.c
> @@ -0,0 +1,136 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * datasheet: https://www.nxp.com/docs/en/data-sheet/K20P144M120SF3.pdf
> + *
> + * Copyright (C) 2018-2021 Collabora
> + * Copyright (C) 2018-2021 GE Healthcare
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/spi/spi.h>
> +#include <linux/of.h>
> +#include "nxp-ezport.h"
> +
> +#define ACHC_MAX_FREQ_HZ 300000
> +
> +struct achc_data {
> + struct spi_device *main;
> + struct spi_device *ezport;
> + struct gpio_desc *reset;
> +
> + struct mutex device_lock; /* avoid concurrent device access */
> +};
> +
> +static ssize_t update_firmware_store(struct device *dev, struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct achc_data *achc = dev_get_drvdata(dev);
> + int ret;
> +
> + if (count != 1 || buf[0] != '1')
> + return -EINVAL;
There a core kernel call to see if the data written to a sysfs file is
"true/false" I would recommend, if you stick with this, to use that
instead.
thanks,
greg k-h
next prev parent reply other threads:[~2021-07-12 17:46 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-07-12 15:02 [PATCHv6 0/3] GE Healthcare PPD firmware upgrade driver for ACHC Sebastian Reichel
2021-07-12 15:02 ` [PATCHv6 1/3] dt-bindings: misc: ge-achc: Convert to DT schema format Sebastian Reichel
2021-07-13 14:03 ` Rob Herring
2021-07-13 15:07 ` Rob Herring
2021-07-12 15:02 ` [PATCHv6 2/3] ARM: dts: imx53-ppd: Fix ACHC entry Sebastian Reichel
2021-07-12 15:02 ` [PATCHv6 3/3] misc: gehc-achc: new driver Sebastian Reichel
2021-07-12 17:46 ` Greg Kroah-Hartman [this message]
2021-07-12 23:20 ` Sebastian Reichel
2021-07-13 5:41 ` Greg Kroah-Hartman
2021-07-13 10:46 ` Sebastian Reichel
2021-07-13 10:56 ` Greg Kroah-Hartman
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YOx/3YaIg24Tx+OQ@kroah.com \
--to=gregkh@linuxfoundation.org \
--cc=arnd@arndb.de \
--cc=broonie@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=festevam@gmail.com \
--cc=ian.ray@ge.com \
--cc=kernel@collabora.com \
--cc=kernel@pengutronix.de \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-spi@vger.kernel.org \
--cc=robh+dt@kernel.org \
--cc=sebastian.reichel@collabora.com \
--cc=shawnguo@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).