On 10. 04. 25 9:13 dop., Krzysztof Kozlowski wrote:
On 09/04/2025 16:42, Ivan Vecera wrote:
Add register definitions for components versions and report them
during probe.
Signed-off-by: Ivan Vecera <ivec...@redhat.com>
---
drivers/mfd/zl3073x-core.c | 36 ++++++++++++++++++++++++++++++++++++
1 file changed, 36 insertions(+)
diff --git a/drivers/mfd/zl3073x-core.c b/drivers/mfd/zl3073x-core.c
index f0d85f77a7a76..28f28d00da1cc 100644
--- a/drivers/mfd/zl3073x-core.c
+++ b/drivers/mfd/zl3073x-core.c
@@ -1,7 +1,9 @@
// SPDX-License-Identifier: GPL-2.0-only
#include <linux/array_size.h>
+#include <linux/bitfield.h>
#include <linux/bits.h>
+#include <linux/cleanup.h>
#include <linux/dev_printk.h>
#include <linux/device.h>
#include <linux/export.h>
@@ -13,6 +15,14 @@
#include <net/devlink.h>
#include "zl3073x.h"
+/*
+ * Register Map Page 0, General
+ */
+ZL3073X_REG16_DEF(id, 0x0001);
+ZL3073X_REG16_DEF(revision, 0x0003);
+ZL3073X_REG16_DEF(fw_ver, 0x0005);
+ZL3073X_REG32_DEF(custom_config_ver, 0x0007);
+
/*
* Regmap ranges
*/
@@ -196,7 +206,9 @@ static void zl3073x_devlink_unregister(void *ptr)
*/
int zl3073x_dev_init(struct zl3073x_dev *zldev)
{
+ u16 id, revision, fw_ver;
struct devlink *devlink;
+ u32 cfg_ver;
int rc;
rc = devm_mutex_init(zldev->dev, &zldev->lock);
@@ -205,6 +217,30 @@ int zl3073x_dev_init(struct zl3073x_dev *zldev)
return rc;
}
+ /* Take device lock */
What is a device lock? Why do you need to comment standard guards/mutexes?
Just to inform code reader, this is a section that accesses device
registers that are protected by this zl3073x device lock.
+ scoped_guard(zl3073x, zldev) {
+ rc = zl3073x_read_id(zldev, &id);
+ if (rc)
+ return rc;
+ rc = zl3073x_read_revision(zldev, &revision);
+ if (rc)
+ return rc;
+ rc = zl3073x_read_fw_ver(zldev, &fw_ver);
+ if (rc)
+ return rc;
+ rc = zl3073x_read_custom_config_ver(zldev, &cfg_ver);
+ if (rc)
+ return rc;
+ }
Nothing improved here. Andrew comments are still valid and do not send
v3 before the discussion is resolved.
I'm accessing device registers here and they are protected by the device
lock. I have to take the lock, register access functions expect this by
lockdep_assert.
+
+ dev_info(zldev->dev, "ChipID(%X), ChipRev(%X), FwVer(%u)\n",
+ id, revision, fw_ver);
+ dev_info(zldev->dev, "Custom config version: %lu.%lu.%lu.%lu\n",
+ FIELD_GET(GENMASK(31, 24), cfg_ver),
+ FIELD_GET(GENMASK(23, 16), cfg_ver),
+ FIELD_GET(GENMASK(15, 8), cfg_ver),
+ FIELD_GET(GENMASK(7, 0), cfg_ver));
Both should be dev_dbg. Your driver should be silent on success.
+1
will change.
+
devlink = priv_to_devlink(zldev);
devlink_register(devlink);
Best regards,
Krzysztof
