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

[Armin Wolf <W_Armin@gmx.de>]

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@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.
>
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@gmx.de>