linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
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: [PATCHv4 6/6] misc: gehc-achc: new driver
Date: Fri, 11 Jun 2021 11:02:59 +0200	[thread overview]
Message-ID: <YMMmw11wOzMZejkS@kroah.com> (raw)
In-Reply-To: <20210609151235.48964-7-sebastian.reichel@collabora.com>

On Wed, Jun 09, 2021 at 05:12:35PM +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.
> 
> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> ---
>  .../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.c
> 
> diff --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 :(


> 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 PCI
> diff --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.o
> diff --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

  reply	other threads:[~2021-06-11  9:03 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-06-09 15:12 [PATCHv4 0/6] GE Healthcare PPD firmware upgrade driver for ACHC Sebastian Reichel
2021-06-09 15:12 ` [PATCHv4 1/6] spi: add ancillary device support Sebastian Reichel
2021-06-11  9:06   ` Greg Kroah-Hartman
2021-06-11 17:22     ` Sebastian Reichel
2021-06-13 14:18   ` Andy Shevchenko
2021-06-17 12:51   ` Mark Brown
2021-06-09 15:12 ` [PATCHv4 2/6] spi: dt-bindings: support devices with multiple chipselects Sebastian Reichel
2021-06-09 15:12 ` [PATCHv4 3/6] dt-bindings: misc: ge-achc: Convert to DT schema format Sebastian Reichel
2021-06-10 16:41   ` Rob Herring
2021-06-09 15:12 ` [PATCHv4 4/6] ARM: dts: imx53-ppd: Fix ACHC entry Sebastian Reichel
2021-06-09 15:12 ` [PATCHv4 5/6] misc: nxp-ezport: introduce EzPort support Sebastian Reichel
2021-06-11  9:05   ` Greg Kroah-Hartman
2021-06-11  9:45     ` Greg Kroah-Hartman
2021-06-09 15:12 ` [PATCHv4 6/6] misc: gehc-achc: new driver Sebastian Reichel
2021-06-11  9:02   ` Greg Kroah-Hartman [this message]
2021-06-22 15:14 ` (subset) [PATCHv4 0/6] GE Healthcare PPD firmware upgrade driver for ACHC Mark Brown

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=YMMmw11wOzMZejkS@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).