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

[Sebastian Reichel]

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@xxxxxxxxxxxxx>
> > ---
> >  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: signature.asc
Description: PGP signature