hwmon: (pmbus/max34440): Add max34451 config load #3067

actorreno · 2026-01-06T07:00:58Z

Adds a few attributes for MAX34451 version ETNA+6 and later. A few attributes reads CRC of the device and the other feature is being able to load a firmware or config file from a hex file. This hex file is assumed to be added in /lib/firmware folder.

PR Description

This adds features/attributes for the max34451. This is only applicable to later releases with versions ETNA+6 and above.
4 attributes added. 3 are similar that just reads a PMBUS register for CRC, which is used to check if hex files are loaded correctly. 1 attribute loads a hex file that is assumed to be placed in /lib/firmware.
Datasheets MAX34451.

PR Type

Bug fix (a change that fixes an issue)
New feature (a change that adds new functionality)
Breaking change (a change that affects other repos or cause CIs to fail)

PR Checklist

I have conducted a self-review of my own code changes
I have compiled my changes, including the documentation
I have tested the changes on the relevant hardware
I have updated the documentation outside this repo accordingly
I have provided links for the relevant upstream lore

Adds a few attributes for MAX34451 version ETNA+6 and later. A few attributes reads CRC of the device and the other feature is being able to load a firmware or config file from a hex file. This hex file is assumed to be added in /lib/firmware folder. Signed-off-by: Alexis Czezar Torreno <[email protected]>

nunojsa · 2026-01-16T14:24:06Z

drivers/hwmon/pmbus/max34440.c

 	if (!data)
 		return -ENOMEM;
+
+	mutex_init(&data->lock);


devm_mutex_init()

nunojsa · 2026-01-16T14:24:48Z

drivers/hwmon/pmbus/max34440.c

+static DEVICE_ATTR_WO(program_memory);
+static DEVICE_ATTR_RO(main_flash_crc);
+static DEVICE_ATTR_RO(backup_flash_crc);
+static DEVICE_ATTR_RO(ram_crc);


Why do we need the CRC attributes? What's the usecase? The commit message should help in understanding that 😉

will edit the commit msg after.

the usecase is it seems the hex/config file contains a corresponding CRC. when loaded it's one way to double check if all registers where written correctly. it think they also want to be able to use the flash memories to store the config and hence the other CRC attributes.

nunojsa · 2026-01-16T14:27:09Z

drivers/hwmon/pmbus/max34440.c

+	return len;
+}
+
+static DEVICE_ATTR_WO(program_memory);


before jumping in reviewing the function why do we need this? Isn't this something that should be only done once during probe? Having a firmware which is actually an hexfile looks very hacky to me.

If the usecase is for this to be done once at startup, I think it can be a devicetree table.

they said customers are showing interest in this process of loading/configuring.
we do think it's a bit hacky...

anyways yes they do want to "program_memory" beyond startup.
(refer to email thread for more context, I think you were cc'ed in the discussion regarding max34451.
email subject: 'MAX34451 software suppporting')

will ping the request for rereview but will wait for a review on this program memory function before sending v2

So a bit of a long thread 😅 . First, we can always go upstream and ask but I find it very hard for the above process to be accepted upstream. Mainly because it's not really something that belongs to /lib/firmware. It's not binary data AFAICT. So the question to me is, can't those PMBUS commands be done through the typical HWMON interface?

anyways yes they do want to "program_memory" beyond startup.

This hard to understand. Why would they want to write it multiple times? Even one more reason why FW is not for this... Device FW is something you load once during a device initialization.

OTOH, binary attributes are also not meant for this... So I'm really not sure how this should be done. But taking one step back, what does "program_memory" actually means? Are we programming some non volatile memory here?

So, if this is some kind of nvmem, I would recommend to look at:

https://elixir.bootlin.com/linux/v6.18.6/source/drivers/nvmem/core.c#L903

And specially this layout stuff:

https://elixir.bootlin.com/linux/v6.18.6/source/drivers/nvmem/core.c#L1021

It seems one can implemente specific layouts like TLV:

https://elixir.bootlin.com/linux/v6.18.6/source/drivers/nvmem/layouts/onie-tlv.c

Maybe all the above can fit. I just looked at it briefly as I don't have the time to go deep in understanding the code but looked promising at the very least. The idea would be to create an aux device (from the HWMON main device) to support the non volatile memory

as far as I understand.

The config/hex file is just a text containing all writable pmbus addresses and intended data (plus some formatting header and checksum)

The "program_memory" attribute just takes this file, parse it, then writes all the data to the registers (inside a RAM which datasheet says volatile).

(let me ask them again why we need multiple program/config)

to quote one of the apps eng'g on the device

"Yes, we allow the users to re-program the device with config files multiple times, whenever they want"

Why they want this kind of feature it seems.

That's still a very big hack and I'm not seeing this being accepted upstream. Why not a debug interface to directly read/write registers (similar to IIO) and let them handle this from userspace? Alternatively, we could just use regmap and if they enable a regmap config option (for debug) they can write registers using regmap (IIRC).

"Yes, we allow the users to re-program the device with config files multiple times, whenever they want"

Having the possibility to do so in HW does not mean it's useful :). And does not mean there's real usecases to do it multiple times without a reboot. I could understand if this some startup configuration that the device needs depending on the system usecase and hence, I proposed overlays. Some other option is using DT properties with DT overlays every time they want to change the configuration which would mean re-probing the device (not sure if it suits their usecase).

To quote again from them:

"The code used the term "fw/firmware" for the content used in program_memory, but it's actually just a configuration file. We don't allow users to modify/program device firmware by themselves"

Am not sure how to balance satisfying requestors/customers and these hacky features.
Will talk to manager and them first. Thanks for the input!

"The code used the term "fw/firmware" for the content used in program_memory, but it's actually just a configuration file. We don't allow users to modify/program device firmware by themselves"

Yeah, that's just a big hack. I mean, people like to abuse and have all of it 😅. I mean, if they want that kind of full control, then just go and implement an userspace driver using i2c-dev. The kernel driver is meant to use the proper standard interfaces and don't allow userspace this kind of full access (other than debug interfaces).

Am not sure how to balance satisfying requestors/customers and these hacky features.
Will talk to manager and them first. Thanks for the input!

Me neither, I would really like to have some kind of "other" solution in here. I would propose:

Having this configuration table as a DT property and if they want to do it multiple time, consider using DT overlays (which means re-probing the device). But even as a DT property, I'm not sure if it's acceptable upstream. It really needs to be some configuration that might depend on the system (and hence the HW) the device will be used.

But if we really have to go with the current approach, I propose to use binary attributes and have directly binary content in the file so there's no parsing in the kernel and we just pass the configuration down to the device. Maybe, that way it will be more acceptable.

nunojsa · 2026-01-16T14:27:20Z

drivers/hwmon/pmbus/max34440.c

+		}
+	}
+
+	//Send data to the device


not kernel style comment

nunojsa · 2026-01-16T14:27:44Z

drivers/hwmon/pmbus/max34440.c

+
+	struct i2c_client *client = to_i2c_client(dev->parent);
+	const struct pmbus_driver_info *info = pmbus_get_driver_info(client);
+	struct max34440_data *data = to_max34440_data(info);


why the spaces in the declaration? It makes it look a bit weird tbh

this was old code that we are trying to PR/upstream to make it a bit official publicly. no specific reason, I just missed it during code formatting.

actorreno force-pushed the dev_max34451_hex_load branch 5 times, most recently from bd2a7a5 to af2c180 Compare January 6, 2026 08:31

actorreno force-pushed the dev_max34451_hex_load branch from af2c180 to f0eec73 Compare January 6, 2026 08:40

actorreno marked this pull request as ready for review January 6, 2026 10:34

actorreno requested review from CiprianRegus, amiclaus, btogorean, buha, chegbeli, danmois, dbogdan, machschmitt, mhennerich, nunojsa and rbolboac as code owners January 6, 2026 10:34

nunojsa reviewed Jan 16, 2026

View reviewed changes

actorreno requested a review from nunojsa January 19, 2026 02:05

hwmon: (pmbus/max34440): Add max34451 config load #3067

Are you sure you want to change the base?

hwmon: (pmbus/max34440): Add max34451 config load #3067

Uh oh!

Conversation

actorreno commented Jan 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Description

PR Type

PR Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

actorreno commented Jan 6, 2026 •

edited

Loading