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:

quoted

On Mon, Jul 12, 2021 at 07:46:05PM +0200, Greg Kroah-Hartman wrote:

quoted

On Mon, Jul 12, 2021 at 05:02:42PM +0200, Sebastian Reichel wrote:

quoted

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.

quoted

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.

quoted

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

quoted

quoted

quoted

Signed-off-by: Sebastian Reichel <redacted>
---
 .../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 :)

quoted

quoted

quoted

+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