Re: [PATCH v4 4/6] hwmon: (spd5118) Add support for reading SPD data

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Am 04.06.24 um 16:30 schrieb Guenter Roeck:

On 6/4/24 04:58, Armin Wolf wrote:
Am 04.06.24 um 06:02 schrieb Guenter Roeck:

Add support for reading SPD NVMEM data from SPD5118 (Jedec JESD300)
compliant memory modules. NVMEM write operation is not supported.

NVMEM support is optional. If CONFIG_NVMEM is disabled, the driver will
still instantiate but not provide NVMEM attribute files.

Signed-off-by: Guenter Roeck <linux@xxxxxxxxxxxx>
---
v4: Use NVMEM_DEVID_NONE instead of NVMEM_DEVID_AUTO
     Ignore nvmem registration failure if nvmem support is disabled

v3: New patch

  Documentation/hwmon/spd5118.rst |   8 ++
  drivers/hwmon/spd5118.c         | 147
+++++++++++++++++++++++++++++++-


[ ... ]

+static int spd5118_nvmem_init(struct device *dev, struct
spd5118_data *data)
+{
+    struct nvmem_config nvmem_config = {
+        .type = NVMEM_TYPE_EEPROM,
+        .name = dev_name(dev),
+        .id = NVMEM_DEVID_NONE,
+        .dev = dev,
+        .base_dev = dev,
+        .read_only = true,
+        .root_only = false,
+        .owner = THIS_MODULE,
+        .compat = true,

Hi,

do we really need this setting here?


The "eeprom" file is only created if both "base_dev" and "compat" are
set.
decode-dimms depends on it. While decode-dimms has to be updated anyway,
I did not want to make that more complicated than necessary.

I understand.

Another option would be not to use the nvmem subsystem in the first
place,
similar to the ee1004 driver, but my understanding is that the use of the
nvmem subsystem is preferred.

[ ... ]

+
+    err = spd5118_nvmem_init(dev, data);
+    /* Ignore if NVMEM support is disabled */
+    if (err && err != -EOPNOTSUPP) {

Maybe we can use IS_REACHABLE(CONFIG_NVMEM) here?


We could, but I prefer to avoid conditionals in the code if possible,
the dummy devm_nvmem_register() is there specifically to cover that
situation, and no other driver does that. Also, since the underlying
functions are dummy, the compiler should optimize it all away if
CONFIG_NVMEM=n.

Thanks,
Guenter

Ok, then i am ok with with this patch.

Tested-by: Armin Wolf <W_Armin@xxxxxx>






[Index of Archives]     [LM Sensors]     [Linux Sound]     [ALSA Users]     [ALSA Devel]     [Linux Audio Users]     [Linux Media]     [Kernel]     [Gimp]     [Yosemite News]     [Linux Media]

  Powered by Linux