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
