| Message ID | 20210712150242.146545-1-sebastian.reichel@collabora.com |
|---|---|
| Headers | show |
| Series | GE Healthcare PPD firmware upgrade driver for ACHC | expand |
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
Hi Greg, On Mon, Jul 12, 2021 at 07:46:05PM +0200, Greg Kroah-Hartman wrote: > 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... Flashing the firmware during boot instead of on-demand is not a good idea for two reasons: 1. This will wear the flash memory of the microcontroller for no good reason. 2. Even if the flashing happens in the background, the microcontroller will not be usable while it is being programmed. Thus the system boot takes 3 minutes instead of 30 seconds. Note, that this is not the first device with this kind of problem. I've seen this before for e.g. atmel touchscreen [0] basically does the same via (undocumented) mxt_update_fw_store(). [0] drivers/input/touchscreen/atmel_mxt_ts.c > > 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? It's only one module, but I put all the generic ezport code into its own file for easy code reuse. I can put it all into gehc-achc and thus reduce the chance of code reuse if somebody else needs to do ezport flashing. Since both files are built into the same module the size savings of merging both files are minimal. > > 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. Actually the above builds both C files into objects and then both objects into 1 module. It's a common pattern in some parts of the kernel, e.g. drm. > > 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. I have problems understanding that sentence, it seems to be missing a word. As far as I can tell -EINVAL is usually used when the user supplied data is not formatted correctly for sysfs store routines, e.g. lot's of them are returning -EINVAL like this: rc = kstrtoul(buf, 0, &value); if (rc) return rc; Did I miss something? Thanks, -- Sebastian
On Tue, Jul 13, 2021 at 01:20:16AM +0200, Sebastian Reichel wrote: > Hi Greg, > > On Mon, Jul 12, 2021 at 07:46:05PM +0200, Greg Kroah-Hartman wrote: > > 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... > > Flashing the firmware during boot instead of on-demand is not > a good idea for two reasons: > > 1. This will wear the flash memory of the microcontroller for no > good reason. Why would you boot with this hardware and not want the firmware loaded? > 2. Even if the flashing happens in the background, the microcontroller > will not be usable while it is being programmed. Thus the system > boot takes 3 minutes instead of 30 seconds. Why would the system boot depend on the firmware being loaded? You have to wait sometime, might as well get the first 30 seconds of waiting out of the way while the rest of the system boots :) > Note, that this is not the first device with this kind of problem. > I've seen this before for e.g. atmel touchscreen [0] basically does > the same via (undocumented) mxt_update_fw_store(). > > [0] drivers/input/touchscreen/atmel_mxt_ts.c Yes, but everyone else uses the normal firmware api method of doing this, don't take one odd driver as being the "rule" :) > > > 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? > > It's only one module, but I put all the generic ezport code into its > own file for easy code reuse. I can put it all into gehc-achc and > thus reduce the chance of code reuse if somebody else needs to do > ezport flashing. Since both files are built into the same module > the size savings of merging both files are minimal. Sorry, you are right, I read the code and Makefile wrong. But, from an organizing point of view, having 2 random files in that directory that are not named alike being built together into one module is not obvious at all. For now, keep it to one file. If future users come along that need this method, split the code then, no need to do extra work at this point in time for unknown uses. > > > +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. > > I have problems understanding that sentence, it seems to be missing > a word. > > As far as I can tell -EINVAL is usually used when the user supplied > data is not formatted correctly for sysfs store routines, e.g. lot's > of them are returning -EINVAL like this: > > rc = kstrtoul(buf, 0, &value); > if (rc) > return rc; > > Did I miss something? I was trying to say "use kstrtoul()" instead of trying to hand-roll much the same thing. Sorry for not being specific. thanks, greg k-h
Hi, On Tue, Jul 13, 2021 at 07:41:51AM +0200, Greg Kroah-Hartman wrote: > On Tue, Jul 13, 2021 at 01:20:16AM +0200, Sebastian Reichel wrote: > > On Mon, Jul 12, 2021 at 07:46:05PM +0200, Greg Kroah-Hartman wrote: > > > 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... > > > > Flashing the firmware during boot instead of on-demand is not > > a good idea for two reasons: > > > > 1. This will wear the flash memory of the microcontroller for no > > good reason. > > Why would you boot with this hardware and not want the firmware > loaded? As written in the commit message this code is updateing the firmware in non-volatile memory, so the previously flashed FW will be used until a new one is flashed. The datasheet for the memory being programmed states, that it has a guaranteed programming endurance of 10.000 times. So programming it at every boot instead of manually when a new FW should be flashed (i.e. once every few years) shortens the device life time considerably. > > 2. Even if the flashing happens in the background, the microcontroller > > will not be usable while it is being programmed. Thus the system > > boot takes 3 minutes instead of 30 seconds. > > Why would the system boot depend on the firmware being loaded? > > You have to wait sometime, might as well get the first 30 seconds of > waiting out of the way while the rest of the system boots :) The microcontroller collects sensor data for the system and is essential for the overall functionality of the medical device. While the firmware flashing is on-going the device cannot be used normally, since the microcontroller will not process and forward the data. > > Note, that this is not the first device with this kind of problem. > > I've seen this before for e.g. atmel touchscreen [0] basically does > > the same via (undocumented) mxt_update_fw_store(). > > > > [0] drivers/input/touchscreen/atmel_mxt_ts.c > > Yes, but everyone else uses the normal firmware api method of doing > this, don't take one odd driver as being the "rule" :) Most FW updates happen to volatile memory (= firmware data is lost on power loss) and must be programmed at boot time. So I'm not looking for the average FW update driver, but for the odd driver also updating FW in non-volatile memory. Those often have the same limitations as the Kinetis K20 has: * limited amount of programming cycles available * slow programming speed > > > > 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? > > > > It's only one module, but I put all the generic ezport code into its > > own file for easy code reuse. I can put it all into gehc-achc and > > thus reduce the chance of code reuse if somebody else needs to do > > ezport flashing. Since both files are built into the same module > > the size savings of merging both files are minimal. > > Sorry, you are right, I read the code and Makefile wrong. > > But, from an organizing point of view, having 2 random files in that > directory that are not named alike being built together into one module > is not obvious at all. > > For now, keep it to one file. If future users come along that need this > method, split the code then, no need to do extra work at this point in > time for unknown uses. Ok, I will put it all into gehc-achc. I doubt somebody will find ezport code in there in the future, but that's not my problem :) > > > > +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. > > > > I have problems understanding that sentence, it seems to be missing > > a word. > > > > As far as I can tell -EINVAL is usually used when the user supplied > > data is not formatted correctly for sysfs store routines, e.g. lot's > > of them are returning -EINVAL like this: > > > > rc = kstrtoul(buf, 0, &value); > > if (rc) > > return rc; > > > > Did I miss something? > > I was trying to say "use kstrtoul()" instead of trying to hand-roll much > the same thing. Sorry for not being specific. Ok. Thanks, -- Sebastian
On Tue, Jul 13, 2021 at 12:46:00PM +0200, Sebastian Reichel wrote: > Hi, > > On Tue, Jul 13, 2021 at 07:41:51AM +0200, Greg Kroah-Hartman wrote: > > On Tue, Jul 13, 2021 at 01:20:16AM +0200, Sebastian Reichel wrote: > > > On Mon, Jul 12, 2021 at 07:46:05PM +0200, Greg Kroah-Hartman wrote: > > > > 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... > > > > > > Flashing the firmware during boot instead of on-demand is not > > > a good idea for two reasons: > > > > > > 1. This will wear the flash memory of the microcontroller for no > > > good reason. > > > > Why would you boot with this hardware and not want the firmware > > loaded? > > As written in the commit message this code is updateing the firmware > in non-volatile memory, so the previously flashed FW will be used > until a new one is flashed. > > The datasheet for the memory being programmed states, that it has a > guaranteed programming endurance of 10.000 times. So programming it > at every boot instead of manually when a new FW should be flashed > (i.e. once every few years) shortens the device life time considerably. Ah, ok, that was not obvious at all. This is much more like a "BIOS update" in that it is infrequent. You might want to document the heck out of this so others are not confused like me. thanks, greg k-h
On Mon, 12 Jul 2021 17:02:40 +0200, Sebastian Reichel wrote: > Convert the binding to DT schema format. Also update the binding > to fix shortcomings > > * Add "nxp,kinetis-k20" fallback compatible > * add programming SPI interface and reset GPIO > * add main clock > * add voltage supplies > * drop spi-max-frequency from required properties, > driver will setup max. frequency > > Reviewed-by: Rob Herring <robh@kernel.org> > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com> > --- > .../devicetree/bindings/misc/ge-achc.txt | 26 -------- > .../devicetree/bindings/misc/ge-achc.yaml | 65 +++++++++++++++++++ > 2 files changed, 65 insertions(+), 26 deletions(-) > delete mode 100644 Documentation/devicetree/bindings/misc/ge-achc.txt > create mode 100644 Documentation/devicetree/bindings/misc/ge-achc.yaml > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' on your patch (DT_CHECKER_FLAGS is new in v5.13): yamllint warnings/errors: dtschema/dtc warnings/errors: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/misc/ge-achc.example.dt.yaml: spi: spi@1:reg: [[1], [0]] is too long From schema: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/spi/spi-controller.yaml /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/misc/ge-achc.example.dt.yaml: spi: spi@1:reg: Additional items are not allowed ([0] was unexpected) From schema: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/spi/spi-controller.yaml \ndoc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/patch/1504037 This check can fail if there are any dependencies. The base for a patch series is generally the most recent rc1. If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date: pip3 install dtschema --upgrade Please check and re-submit.
On Tue, Jul 13, 2021 at 8:03 AM Rob Herring <robh@kernel.org> wrote: > > On Mon, 12 Jul 2021 17:02:40 +0200, Sebastian Reichel wrote: > > Convert the binding to DT schema format. Also update the binding > > to fix shortcomings > > > > * Add "nxp,kinetis-k20" fallback compatible > > * add programming SPI interface and reset GPIO > > * add main clock > > * add voltage supplies > > * drop spi-max-frequency from required properties, > > driver will setup max. frequency > > > > Reviewed-by: Rob Herring <robh@kernel.org> > > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com> > > --- > > .../devicetree/bindings/misc/ge-achc.txt | 26 -------- > > .../devicetree/bindings/misc/ge-achc.yaml | 65 +++++++++++++++++++ > > 2 files changed, 65 insertions(+), 26 deletions(-) > > delete mode 100644 Documentation/devicetree/bindings/misc/ge-achc.txt > > create mode 100644 Documentation/devicetree/bindings/misc/ge-achc.yaml > > > > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' > on your patch (DT_CHECKER_FLAGS is new in v5.13): > > yamllint warnings/errors: > > dtschema/dtc warnings/errors: > /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/misc/ge-achc.example.dt.yaml: spi: spi@1:reg: [[1], [0]] is too long > From schema: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/spi/spi-controller.yaml > /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/misc/ge-achc.example.dt.yaml: spi: spi@1:reg: Additional items are not allowed ([0] was unexpected) > From schema: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/spi/spi-controller.yaml Ignore this. The necessary change is in v5.14-rc1. Rob