Received-SPF: pass (google.com: domain of linux-kernel+bounces-206444-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Message-ID: <c6f398f2-a16c-462a-8c79-d2eda3dbd56e@gmx.de>
Date: Fri, 7 Jun 2024 17:59:59 +0200
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v4 4/6] hwmon: (spd5118) Add support for reading SPD data
To: Guenter Roeck <linux@roeck-us.net>, linux-hwmon@vger.kernel.org
Cc: linux-i2c@vger.kernel.org, linux-kernel@vger.kernel.org,
 devicetree@vger.kernel.org, Krzysztof Kozlowski <krzk+dt@kernel.org>,
 Wolfram Sang <wsa+renesas@sang-engineering.com>,
 =?UTF-8?Q?Ren=C3=A9_Rebe?= <rene@exactcode.de>,
 =?UTF-8?Q?Thomas_Wei=C3=9Fschuh?= <linux@weissschuh.net>,
 Stephen Horvath <s.horvath@outlook.com.au>
References: <20240604040237.1064024-1-linux@roeck-us.net>
 <20240604040237.1064024-5-linux@roeck-us.net>
 <4cfe1004-77d4-432b-b07e-557a2e57de58@gmx.de>
 <2c94220a-29e9-4b83-a427-5ad406ff1c48@roeck-us.net>
Content-Language: en-US
From: Armin Wolf <W_Armin@gmx.de>
In-Reply-To: <2c94220a-29e9-4b83-a427-5ad406ff1c48@roeck-us.net>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: quoted-printable
UI-OutboundReport: notjunk:1;M01:P0:wZGDi/oXCIM=;vYmL5x6uzzRQTVhF2LXO3SJZGYa
 +MFZ5gk2HrpjbVxGHGcRMSDsnCBoSCbgmRMPalOWosmL+RxOtNccoo22PjH2dvQcvNntRv8Ip
 qWaAoLgBgZ3RTTeZ1aXXwzGaCBkkFml4VErK+8p+vQ32PN2SEEEaexbHGTvMg1Y6zrSvQ3Ky/
 bUJAnRXzTPVvnt59G3e5YpmygFGmxZP6Ev0IWAhMzOHfa8NjTB4ZhOVZthVH24nDwokr1EYWH
 EKddhxKiwIVpgdGyntHIsKBbo5m4HkzPIK4zSUFLyW8of+GsUMJAjA659uiRCcUHQduEmjk6+
 VnKAkhLNWgLlYal/QTOGYI3W6nez6ezXpdkkLeGQV+Iqhw7HkYsLLP2mgHPOKnYKwR55jTrkn
 9Kz3HBBxlnH64VRhKQ4trYwgDvwFVFdjXTpx9ibOsYIaead1P1i1qV4Uy3ofaapf/5Tes/Nh4
 39VGPwk31EZbol0Q5FAzvnjW5Bedf20Jn9D+D/VVkopW9WhLGvlBEldDGNfR//DxShrycXGsg
 eCHO1ONCScDqBuo9lOPHiAJhbIaRNEFJGYM3s+WXe7v9AU4N7w3oQM5LXHyo+xIRTK3yQrBpn
 bLnoDQCIua3AYnNxrNlqPkEbfeAySCwTFAdy0SopyVc8ft8SfWguKaP0yGPeK1xHXC+QvL3sA
 Mzoa0zWI4AE1tYQ5hUtQTMQ7INI5b1Qoxqc027dhdEsqRIqGUYLVe30FG83Koft5+Z7JzXY3r
 +tR5Vb8i4Qu7oS3K9htnKVYfFLZbRBxpmkabf5p7P1rm8KbK08oENEBp1IKj1220C6xoOccgQ
 1KF8Z/3jzfVRj3w8fxSwBwohCK2bMzKIAgCe2mfscZ0bc=

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 wil=
l
>>> 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
>>> =C2=A0=C2=A0=C2=A0=C2=A0 Ignore nvmem registration failure if nvmem su=
pport is disabled
>>>
>>> v3: New patch
>>>
>>> =C2=A0 Documentation/hwmon/spd5118.rst |=C2=A0=C2=A0 8 ++
>>> =C2=A0 drivers/hwmon/spd5118.c=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0 | 147
>>> +++++++++++++++++++++++++++++++-
>
>
> [ ... ]
>
>>> +static int spd5118_nvmem_init(struct device *dev, struct
>>> spd5118_data *data)
>>> +{
>>> +=C2=A0=C2=A0=C2=A0 struct nvmem_config nvmem_config =3D {
>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 .type =3D NVMEM_TYPE_EEPRO=
M,
>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 .name =3D dev_name(dev),
>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 .id =3D NVMEM_DEVID_NONE,
>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 .dev =3D dev,
>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 .base_dev =3D dev,
>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 .read_only =3D true,
>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 .root_only =3D false,
>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 .owner =3D THIS_MODULE,
>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 .compat =3D 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 th=
e
> nvmem subsystem is preferred.
>
> [ ... ]
>
>>> +
>>> +=C2=A0=C2=A0=C2=A0 err =3D spd5118_nvmem_init(dev, data);
>>> +=C2=A0=C2=A0=C2=A0 /* Ignore if NVMEM support is disabled */
>>> +=C2=A0=C2=A0=C2=A0 if (err && err !=3D -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=3Dn.
>
> Thanks,
> Guenter
>
Ok, then i am ok with with this patch.

Tested-by: Armin Wolf <W_Armin@gmx.de>