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

[Guenter Roeck <linux@roeck-us.net>]

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@roeck-us.net>
>> ---
>> 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.

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