[PATCH v2 0/3] mtk-socinfo driver implementation

[PATCH v2 0/3] mtk-socinfo driver implementation

[William-tw Lin]

This purpose of the patches is for soc-related information retrival.
Such information includes manufacturer information, SoC name, SoC 
segment name, and SoC marketing name.

Based on tag: next-20230915, linux-next/master

Changes in v2:
- Remove mtk-socinfo.h
- Consolidate different compatibles into mediatek,socinfo
- Move socinfo node out of MMIO bus
- Move mtk-socinfo.yaml to hwinfo
- Fix reviewer's comments

William-tw Lin (3):
  arm64: dts: Add node for chip info driver
  soc: mediatek: mtk-socinfo: Add driver for getting chip information
  dt-bindings: hwinfo: Add mtk-socinfo driver

 .../bindings/hwinfo/mtk-socinfo.yaml          |  48 +++++
 arch/arm64/boot/dts/mediatek/mt8173.dtsi      |  15 ++
 arch/arm64/boot/dts/mediatek/mt8183.dtsi      |  15 ++
 arch/arm64/boot/dts/mediatek/mt8186.dtsi      |  10 ++
 arch/arm64/boot/dts/mediatek/mt8192.dtsi      |  14 ++
 arch/arm64/boot/dts/mediatek/mt8195.dtsi      |   9 +
 drivers/soc/mediatek/Kconfig                  |   9 +
 drivers/soc/mediatek/Makefile                 |   1 +
 drivers/soc/mediatek/mtk-socinfo.c            | 166 ++++++++++++++++++
 9 files changed, 287 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwinfo/mtk-socinfo.yaml
 create mode 100644 drivers/soc/mediatek/mtk-socinfo.c

-- 
2.18.0

[PATCH v2 1/3] arm64: dts: Add node for chip info driver

[William-tw Lin]

Add dts node for socinfo retrieval for the following projects:
MT8173, MT8183, MT8186, MT8192, MT8195

Signed-off-by: William-tw Lin <william-tw.lin@mediatek.com>
---
 arch/arm64/boot/dts/mediatek/mt8173.dtsi | 15 +++++++++++++++
 arch/arm64/boot/dts/mediatek/mt8183.dtsi | 15 +++++++++++++++
 arch/arm64/boot/dts/mediatek/mt8186.dtsi | 10 ++++++++++
 arch/arm64/boot/dts/mediatek/mt8192.dtsi | 14 ++++++++++++++
 arch/arm64/boot/dts/mediatek/mt8195.dtsi |  9 +++++++++
 5 files changed, 63 insertions(+)

diff --git a/arch/arm64/boot/dts/mediatek/mt8173.dtsi b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
index c47d7d900f28..8cac18ed7833 100644
--- a/arch/arm64/boot/dts/mediatek/mt8173.dtsi
+++ b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
@@ -590,6 +590,15 @@
 			reg = <0 0x10206000 0 0x1000>;
 			#address-cells = <1>;
 			#size-cells = <1>;
+
+			socinfo_data1: socinfo-data1 {
+				reg = <0x040 0x4>;
+			};
+
+			socinfo_data2: socinfo-data2 {
+				reg = <0x044 0x4>;
+			};
+
 			thermal_calibration: calib@528 {
 				reg = <0x528 0xc>;
 			};
@@ -1520,4 +1529,10 @@
 			power-domains = <&spm MT8173_POWER_DOMAIN_VENC_LT>;
 		};
 	};
+
+	socinfo {
+		compatible = "mediatek,socinfo";
+		nvmem-cells = <&socinfo_data1 &socinfo_data2>;
+		nvmem-cell-names = "socinfo-data1", "socinfo-data2";
+	};
 };
diff --git a/arch/arm64/boot/dts/mediatek/mt8183.dtsi b/arch/arm64/boot/dts/mediatek/mt8183.dtsi
index 5169779d01df..b17af0edb198 100644
--- a/arch/arm64/boot/dts/mediatek/mt8183.dtsi
+++ b/arch/arm64/boot/dts/mediatek/mt8183.dtsi
@@ -1706,6 +1706,15 @@
 			reg = <0 0x11f10000 0 0x1000>;
 			#address-cells = <1>;
 			#size-cells = <1>;
+
+			socinfo_data1: socinfo-data1 {
+				reg = <0x04C 0x4>;
+			};
+
+			socinfo_data2: socinfo-data2 {
+				reg = <0x060 0x4>;
+			};
+
 			thermal_calibration: calib@180 {
 				reg = <0x180 0xc>;
 			};
@@ -2105,4 +2114,10 @@
 			power-domains = <&spm MT8183_POWER_DOMAIN_CAM>;
 		};
 	};
+
+	socinfo {
+		compatible = "mediatek,socinfo";
+		nvmem-cells = <&socinfo_data1 &socinfo_data2>;
+		nvmem-cell-names = "socinfo-data1", "socinfo-data2";
+	};
 };
diff --git a/arch/arm64/boot/dts/mediatek/mt8186.dtsi b/arch/arm64/boot/dts/mediatek/mt8186.dtsi
index f04ae70c470a..860559d239a0 100644
--- a/arch/arm64/boot/dts/mediatek/mt8186.dtsi
+++ b/arch/arm64/boot/dts/mediatek/mt8186.dtsi
@@ -1660,6 +1660,10 @@
 				reg = <0x59c 0x4>;
 				bits = <0 3>;
 			};
+
+			socinfo_data1: socinfo-data1 {
+				reg = <0x7a0 0x4>;
+			};
 		};
 
 		mipi_tx0: dsi-phy@11cc0000 {
@@ -2083,4 +2087,10 @@
 			power-domains = <&spm MT8186_POWER_DOMAIN_IPE>;
 		};
 	};
+
+	socinfo {
+		compatible = "mediatek,socinfo";
+		nvmem-cells = <&socinfo_data1>;
+		nvmem-cell-names = "socinfo-data1";
+	};
 };
diff --git a/arch/arm64/boot/dts/mediatek/mt8192.dtsi b/arch/arm64/boot/dts/mediatek/mt8192.dtsi
index 5e94cb4aeb44..3e1315da3b56 100644
--- a/arch/arm64/boot/dts/mediatek/mt8192.dtsi
+++ b/arch/arm64/boot/dts/mediatek/mt8192.dtsi
@@ -1122,6 +1122,14 @@
 			#address-cells = <1>;
 			#size-cells = <1>;
 
+			socinfo_data1: socinfo-data1 {
+				reg = <0x044 0x4>;
+			};
+
+			socinfo_data2: socinfo-data2 {
+				reg = <0x050 0x4>;
+			};
+
 			lvts_e_data1: data1@1c0 {
 				reg = <0x1c0 0x58>;
 			};
@@ -1901,4 +1909,10 @@
 			power-domains = <&spm MT8192_POWER_DOMAIN_MDP>;
 		};
 	};
+
+	socinfo {
+		compatible = "mediatek,socinfo";
+		nvmem-cells = <&socinfo_data1 &socinfo_data2>;
+		nvmem-cell-names = "socinfo-data1", "socinfo-data2";
+	};
 };
diff --git a/arch/arm64/boot/dts/mediatek/mt8195.dtsi b/arch/arm64/boot/dts/mediatek/mt8195.dtsi
index 48b72b3645e1..17c4805d7963 100644
--- a/arch/arm64/boot/dts/mediatek/mt8195.dtsi
+++ b/arch/arm64/boot/dts/mediatek/mt8195.dtsi
@@ -1683,6 +1683,9 @@
 			lvts_efuse_data2: lvts2-calib@1d0 {
 				reg = <0x1d0 0x38>;
 			};
+			socinfo_data1: socinfo-data1 {
+				reg = <0x7a0 0x4>;
+			};
 		};
 
 		u3phy2: t-phy@11c40000 {
@@ -3519,4 +3522,10 @@
 			};
 		};
 	};
+
+	socinfo {
+		compatible = "mediatek,socinfo";
+		nvmem-cells = <&socinfo_data1>;
+		nvmem-cell-names = "socinfo-data1";
+	};
 };
-- 
2.18.0

[PATCH v2 2/3] soc: mediatek: mtk-socinfo: Add driver for getting chip information

[William-tw Lin]

Add driver for socinfo retrieval. This patch includes the following:
1. mtk-socinfo driver for chip info retrieval
2. Related changes to Makefile and Kconfig

Signed-off-by: William-tw Lin <william-tw.lin@mediatek.com>
---
 drivers/soc/mediatek/Kconfig       |   9 ++
 drivers/soc/mediatek/Makefile      |   1 +
 drivers/soc/mediatek/mtk-socinfo.c | 166 +++++++++++++++++++++++++++++
 3 files changed, 176 insertions(+)
 create mode 100644 drivers/soc/mediatek/mtk-socinfo.c

diff --git a/drivers/soc/mediatek/Kconfig b/drivers/soc/mediatek/Kconfig
index a88cf04fc803..5746d3b4c67d 100644
--- a/drivers/soc/mediatek/Kconfig
+++ b/drivers/soc/mediatek/Kconfig
@@ -91,4 +91,13 @@ config MTK_SVS
 	  chip process corner, temperatures and other factors. Then DVFS
 	  driver could apply SVS bank voltage to PMIC/Buck.
 
+config MTK_SOCINFO
+	tristate "MediaTek SOCINFO"
+	depends on MTK_EFUSE && NVMEM
+	help
+	  Say y here to enable mtk socinfo information.
+	  This enables a sysfs node which shows attributes of MTK soc info.
+	  Information of soc info includes the manufacturer, the marketing
+	  name, and the soc used.
+
 endmenu
diff --git a/drivers/soc/mediatek/Makefile b/drivers/soc/mediatek/Makefile
index 9d3ce7878c5c..6830512848fd 100644
--- a/drivers/soc/mediatek/Makefile
+++ b/drivers/soc/mediatek/Makefile
@@ -7,3 +7,4 @@ obj-$(CONFIG_MTK_REGULATOR_COUPLER) += mtk-regulator-coupler.o
 obj-$(CONFIG_MTK_MMSYS) += mtk-mmsys.o
 obj-$(CONFIG_MTK_MMSYS) += mtk-mutex.o
 obj-$(CONFIG_MTK_SVS) += mtk-svs.o
+obj-$(CONFIG_MTK_SOCINFO) += mtk-socinfo.o
diff --git a/drivers/soc/mediatek/mtk-socinfo.c b/drivers/soc/mediatek/mtk-socinfo.c
new file mode 100644
index 000000000000..fe5a68925f58
--- /dev/null
+++ b/drivers/soc/mediatek/mtk-socinfo.c
@@ -0,0 +1,166 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2023 MediaTek Inc.
+ */
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
+#include <linux/pm_runtime.h>
+#include <linux/nvmem-consumer.h>
+#include <linux/device.h>
+#include <linux/debugfs.h>
+#include <linux/seq_file.h>
+#include <linux/string.h>
+#include <linux/sys_soc.h>
+#include <linux/slab.h>
+
+#define MTK_SOCINFO_ENTRY(_soc_name, _segment_name, _marketing_name, _cell_data1, _cell_data2) {\
+	.soc_name = _soc_name,	\
+	.segment_name = _segment_name,	\
+	.marketing_name = _marketing_name,	\
+	.cell_data = {_cell_data1, _cell_data2}	\
+}
+#define CELL_NOT_USED (0xFFFFFFFF)
+#define MAX_CELLS (2)
+
+struct mtk_socinfo {
+	struct device *dev;
+	struct name_data *name_data;
+	struct socinfo_data *socinfo_data;
+};
+
+struct socinfo_data {
+	char *soc_name;
+	char *segment_name;
+	char *marketing_name;
+	u32 cell_data[MAX_CELLS];
+};
+
+const char *soc_manufacturer = "MediaTek";
+struct soc_device *soc_dev;
+char *cell_names[MAX_CELLS] = {"socinfo-data1", "socinfo-data2"};
+
+static struct socinfo_data socinfo_data_table[] = {
+	MTK_SOCINFO_ENTRY("MT8173", "MT8173V/AC", "MT8173", 0x6CA20004, 0x10000000),
+	MTK_SOCINFO_ENTRY("MT8183", "MT8183V/AZA", "Kompanio 500", 0x00010043, 0x00000840),
+	MTK_SOCINFO_ENTRY("MT8186", "MT8186GV/AZA", "Kompanio 520", 0x81861001, CELL_NOT_USED),
+	MTK_SOCINFO_ENTRY("MT8186T", "MT8186TV/AZA", "Kompanio 528", 0x81862001, CELL_NOT_USED),
+	MTK_SOCINFO_ENTRY("MT8188", "MT8188GV/AZA", "Kompanio 830", 0x81880000, 0x00000010),
+	MTK_SOCINFO_ENTRY("MT8188", "MT8188GV/HZA", "Kompanio 830", 0x81880000, 0x00000011),
+	MTK_SOCINFO_ENTRY("MT8192", "MT8192V/AZA", "Kompanio 820", 0x00001100, 0x00040080),
+	MTK_SOCINFO_ENTRY("MT8192T", "MT8192V/ATZA", "Kompanio 828", 0x00000100, 0x000400C0),
+	MTK_SOCINFO_ENTRY("MT8195", "MT8195GV/EZA", "Kompanio 1200", 0x81950300, CELL_NOT_USED),
+	MTK_SOCINFO_ENTRY("MT8195", "MT8195GV/EHZA", "Kompanio 1200", 0x81950304, CELL_NOT_USED),
+	MTK_SOCINFO_ENTRY("MT8195", "MT8195TV/EZA", "Kompanio 1380", 0x81950400, CELL_NOT_USED),
+	MTK_SOCINFO_ENTRY("MT8195", "MT8195TV/EHZA", "Kompanio 1380", 0x81950404, CELL_NOT_USED),
+};
+
+static int mtk_socinfo_create_socinfo_node(struct mtk_socinfo *mtk_socinfop)
+{
+	struct soc_device_attribute *attrs;
+	static char machine[30] = {0};
+
+	attrs = devm_kzalloc(mtk_socinfop->dev, sizeof(*attrs), GFP_KERNEL);
+	if (!attrs)
+		return -ENOMEM;
+
+	snprintf(machine, 30, "%s (%s)", mtk_socinfop->socinfo_data->marketing_name,
+		mtk_socinfop->socinfo_data->soc_name);
+	attrs->family = soc_manufacturer;
+	attrs->machine = machine;
+
+	soc_dev = soc_device_register(attrs);
+	if (IS_ERR(soc_dev))
+		return PTR_ERR(soc_dev);
+
+	dev_info(mtk_socinfop->dev, "%s SoC detected.\n", attrs->machine);
+	return 0;
+}
+
+static int mtk_socinfo_get_socinfo_data(struct mtk_socinfo *mtk_socinfop)
+{
+	unsigned int i = 0, j = 0;
+	unsigned int num_cell_data = 0;
+	u32 *cell_datap[MAX_CELLS] = { NULL };
+	size_t efuse_bytes;
+	struct nvmem_cell *cell;
+	bool match_socinfo = true;
+	int match_socinfo_index = -1;
+
+	for (i = 0; i < MAX_CELLS; i++) {
+		cell = nvmem_cell_get(mtk_socinfop->dev, cell_names[i]);
+		if (IS_ERR_OR_NULL(cell))
+			break;
+		cell_datap[i] = (u32 *)nvmem_cell_read(cell, &efuse_bytes);
+		nvmem_cell_put(cell);
+		num_cell_data++;
+	}
+
+	if (!num_cell_data)
+		return -ENOENT;
+
+	for (i = 0; i < ARRAY_SIZE(socinfo_data_table); i++) {
+		match_socinfo = true;
+		for (j = 0; j < num_cell_data; j++) {
+			if (*(cell_datap[j]) != socinfo_data_table[i].cell_data[j])
+				match_socinfo = false;
+		}
+		if (num_cell_data > 0 && match_socinfo) {
+			mtk_socinfop->socinfo_data = &(socinfo_data_table[i]);
+			match_socinfo_index = i;
+			break;
+		}
+	}
+
+	return match_socinfo_index >= 0 ? match_socinfo_index : -ENOENT;
+}
+
+static const struct of_device_id mtk_socinfo_id_table[] = {
+	{ .compatible = "mediatek,socinfo" },
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, mtk_socinfo_id_table);
+
+static int mtk_socinfo_probe(struct platform_device *pdev)
+{
+	struct mtk_socinfo *mtk_socinfop;
+	int ret = 0;
+
+	mtk_socinfop = devm_kzalloc(&pdev->dev, sizeof(*mtk_socinfop), GFP_KERNEL);
+	if (!mtk_socinfop)
+		return -ENOMEM;
+
+	mtk_socinfop->dev = &pdev->dev;
+
+	ret = mtk_socinfo_get_socinfo_data(mtk_socinfop);
+	if (ret < 0)
+		return dev_err_probe(mtk_socinfop->dev, ret, "Failed to get socinfo data\n");
+
+	ret = mtk_socinfo_create_socinfo_node(mtk_socinfop);
+	if (ret != 0)
+		return dev_err_probe(mtk_socinfop->dev, -EINVAL, "Failed to create socinfo node\n");
+
+	return 0;
+}
+
+static int mtk_socinfo_remove(struct platform_device *pdev)
+{
+	if (soc_dev)
+		soc_device_unregister(soc_dev);
+	return 0;
+}
+
+static struct platform_driver mtk_socinfo = {
+	.probe          = mtk_socinfo_probe,
+	.remove         = mtk_socinfo_remove,
+	.driver         = {
+		.name   = "mtk_socinfo",
+		.owner  = THIS_MODULE,
+		.of_match_table = mtk_socinfo_id_table,
+	},
+};
+module_platform_driver(mtk_socinfo);
+MODULE_AUTHOR("William-TW LIN <william-tw.lin@mediatek.com>");
+MODULE_DESCRIPTION("Mediatek socinfo driver");
+MODULE_LICENSE("GPL");
-- 
2.18.0

[PATCH v2 3/3] dt-bindings: hwinfo: Add mtk-socinfo driver

[William-tw Lin]

dt-binding documentation for mtk-socinfo driver.
mtk-socinfo driver provides SoC-related information.
Such information includes manufacturer information, SoC name,
SoC segment name, and SoC marketing name.

Signed-off-by: William-tw Lin <william-tw.lin@mediatek.com>
---
 .../bindings/hwinfo/mtk-socinfo.yaml          | 48 +++++++++++++++++++
 1 file changed, 48 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwinfo/mtk-socinfo.yaml

diff --git a/Documentation/devicetree/bindings/hwinfo/mtk-socinfo.yaml b/Documentation/devicetree/bindings/hwinfo/mtk-socinfo.yaml
new file mode 100644
index 000000000000..74f03f1dc404
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwinfo/mtk-socinfo.yaml
@@ -0,0 +1,48 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/hwinfo/mtk-socinfo.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: MediaTek SoC ChipID
+
+maintainers:
+  - William Lin <william-tw.lin@mediatek.com>
+  - Matthias Brugger <matthias.bgg@gmail.com>
+  - Kevin Hilman <khilman@kernel.org>
+  - AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
+
+description:
+  MediaTek SoCs store various product information in eFuses, including
+  Chip ID and Revision fields, usable to identify the manufacturer,
+  SoC version, plus segment and marketing names.
+
+properties:
+  compatible:
+    const: mediatek,socinfo
+
+  nvmem-cells:
+    maxItems: 2
+    description: Phandle to nvmem cells containing SoC identification data
+
+  nvmem-cell-names:
+    minItems: 1
+    items:
+      - const: socinfo-data1
+      - const: socinfo-data2
+
+required:
+  - compatible
+  - nvmem-cells
+  - nvmem-cell-names
+
+additionalProperties: false
+
+examples:
+  - |
+        socinfo {
+            compatible = "mediatek,socinfo";
+            nvmem-cells = <&socinfo_data1>, <&socinfo_data2>;
+            nvmem-cell-names = "socinfo-data1", "socinfo-data2";
+        };
+
-- 
2.18.0

Re: [PATCH v2 3/3] dt-bindings: hwinfo: Add mtk-socinfo driver

[Krzysztof Kozlowski]

On 15/09/2023 17:26, William-tw Lin wrote:
> dt-binding documentation for mtk-socinfo driver.

Here and in subject, drop driver and instead descrbe hardware.

> mtk-socinfo driver provides SoC-related information.
> Such information includes manufacturer information, SoC name,
> SoC segment name, and SoC marketing name.
> 
> Signed-off-by: William-tw Lin <william-tw.lin@mediatek.com>
> ---
>  .../bindings/hwinfo/mtk-socinfo.yaml          | 48 +++++++++++++++++++
>  1 file changed, 48 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/hwinfo/mtk-socinfo.yaml
> 
> diff --git a/Documentation/devicetree/bindings/hwinfo/mtk-socinfo.yaml b/Documentation/devicetree/bindings/hwinfo/mtk-socinfo.yaml
> new file mode 100644
> index 000000000000..74f03f1dc404
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwinfo/mtk-socinfo.yaml

Nothing improved.

This is a friendly reminder during the review process.

It seems my previous comments were not fully addressed. Maybe my
feedback got lost between the quotes, maybe you just forgot to apply it.
Please go back to the previous discussion and either implement all
requested changes or keep discussing them.

Thank you.

> @@ -0,0 +1,48 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/hwinfo/mtk-socinfo.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: MediaTek SoC ChipID
> +
> +maintainers:
> +  - William Lin <william-tw.lin@mediatek.com>
> +  - Matthias Brugger <matthias.bgg@gmail.com>
> +  - Kevin Hilman <khilman@kernel.org>
> +  - AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> +
> +description:
> +  MediaTek SoCs store various product information in eFuses, including
> +  Chip ID and Revision fields, usable to identify the manufacturer,
> +  SoC version, plus segment and marketing names.
> +
> +properties:
> +  compatible:
> +    const: mediatek,socinfo

What happened to compatibles? No, this is just wrong and no explained.
You ignored other comments as well. Really, that's not the way to go.

NAK

Best regards,
Krzysztof

Re: [PATCH v2 2/3] soc: mediatek: mtk-socinfo: Add driver for getting chip information

[Krzysztof Kozlowski]

On 15/09/2023 17:26, William-tw Lin wrote:
> Add driver for socinfo retrieval. This patch includes the following:
> 1. mtk-socinfo driver for chip info retrieval
> 2. Related changes to Makefile and Kconfig
> 
> Signed-off-by: William-tw Lin <william-tw.lin@mediatek.com>
> ---
>  drivers/soc/mediatek/Kconfig       |   9 ++
>  drivers/soc/mediatek/Makefile      |   1 +
>  drivers/soc/mediatek/mtk-socinfo.c | 166 +++++++++++++++++++++++++++++
>  3 files changed, 176 insertions(+)
>  create mode 100644 drivers/soc/mediatek/mtk-socinfo.c
> 
> diff --git a/drivers/soc/mediatek/Kconfig b/drivers/soc/mediatek/Kconfig
> index a88cf04fc803..5746d3b4c67d 100644
> --- a/drivers/soc/mediatek/Kconfig
> +++ b/drivers/soc/mediatek/Kconfig
> @@ -91,4 +91,13 @@ config MTK_SVS
>  	  chip process corner, temperatures and other factors. Then DVFS
>  	  driver could apply SVS bank voltage to PMIC/Buck.
>  
> +config MTK_SOCINFO
> +	tristate "MediaTek SOCINFO"
> +	depends on MTK_EFUSE && NVMEM
> +	help
> +	  Say y here to enable mtk socinfo information.
> +	  This enables a sysfs node which shows attributes of MTK soc info.
> +	  Information of soc info includes the manufacturer, the marketing
> +	  name, and the soc used.
> +
>  endmenu
> diff --git a/drivers/soc/mediatek/Makefile b/drivers/soc/mediatek/Makefile
> index 9d3ce7878c5c..6830512848fd 100644
> --- a/drivers/soc/mediatek/Makefile
> +++ b/drivers/soc/mediatek/Makefile
> @@ -7,3 +7,4 @@ obj-$(CONFIG_MTK_REGULATOR_COUPLER) += mtk-regulator-coupler.o
>  obj-$(CONFIG_MTK_MMSYS) += mtk-mmsys.o
>  obj-$(CONFIG_MTK_MMSYS) += mtk-mutex.o
>  obj-$(CONFIG_MTK_SVS) += mtk-svs.o
> +obj-$(CONFIG_MTK_SOCINFO) += mtk-socinfo.o
> diff --git a/drivers/soc/mediatek/mtk-socinfo.c b/drivers/soc/mediatek/mtk-socinfo.c
> new file mode 100644
> index 000000000000..fe5a68925f58
> --- /dev/null
> +++ b/drivers/soc/mediatek/mtk-socinfo.c
> @@ -0,0 +1,166 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2023 MediaTek Inc.
> + */
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_platform.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/nvmem-consumer.h>
> +#include <linux/device.h>
> +#include <linux/debugfs.h>
> +#include <linux/seq_file.h>
> +#include <linux/string.h>
> +#include <linux/sys_soc.h>
> +#include <linux/slab.h>
> +
> +#define MTK_SOCINFO_ENTRY(_soc_name, _segment_name, _marketing_name, _cell_data1, _cell_data2) {\
> +	.soc_name = _soc_name,	\
> +	.segment_name = _segment_name,	\
> +	.marketing_name = _marketing_name,	\
> +	.cell_data = {_cell_data1, _cell_data2}	\
> +}
> +#define CELL_NOT_USED (0xFFFFFFFF)
> +#define MAX_CELLS (2)
> +
> +struct mtk_socinfo {
> +	struct device *dev;
> +	struct name_data *name_data;
> +	struct socinfo_data *socinfo_data;
> +};
> +
> +struct socinfo_data {
> +	char *soc_name;
> +	char *segment_name;
> +	char *marketing_name;
> +	u32 cell_data[MAX_CELLS];
> +};
> +
> +const char *soc_manufacturer = "MediaTek";

Drop, not needed, not even static/

> +struct soc_device *soc_dev;

Drop. First, it's not even static. You cannot have global variables.

Second, it's not even used... This is some poor code.

> +char *cell_names[MAX_CELLS] = {"socinfo-data1", "socinfo-data2"};

static

> +
> +static struct socinfo_data socinfo_data_table[] = {
> +	MTK_SOCINFO_ENTRY("MT8173", "MT8173V/AC", "MT8173", 0x6CA20004, 0x10000000),
> +	MTK_SOCINFO_ENTRY("MT8183", "MT8183V/AZA", "Kompanio 500", 0x00010043, 0x00000840),
> +	MTK_SOCINFO_ENTRY("MT8186", "MT8186GV/AZA", "Kompanio 520", 0x81861001, CELL_NOT_USED),
> +	MTK_SOCINFO_ENTRY("MT8186T", "MT8186TV/AZA", "Kompanio 528", 0x81862001, CELL_NOT_USED),
> +	MTK_SOCINFO_ENTRY("MT8188", "MT8188GV/AZA", "Kompanio 830", 0x81880000, 0x00000010),
> +	MTK_SOCINFO_ENTRY("MT8188", "MT8188GV/HZA", "Kompanio 830", 0x81880000, 0x00000011),
> +	MTK_SOCINFO_ENTRY("MT8192", "MT8192V/AZA", "Kompanio 820", 0x00001100, 0x00040080),
> +	MTK_SOCINFO_ENTRY("MT8192T", "MT8192V/ATZA", "Kompanio 828", 0x00000100, 0x000400C0),
> +	MTK_SOCINFO_ENTRY("MT8195", "MT8195GV/EZA", "Kompanio 1200", 0x81950300, CELL_NOT_USED),
> +	MTK_SOCINFO_ENTRY("MT8195", "MT8195GV/EHZA", "Kompanio 1200", 0x81950304, CELL_NOT_USED),
> +	MTK_SOCINFO_ENTRY("MT8195", "MT8195TV/EZA", "Kompanio 1380", 0x81950400, CELL_NOT_USED),
> +	MTK_SOCINFO_ENTRY("MT8195", "MT8195TV/EHZA", "Kompanio 1380", 0x81950404, CELL_NOT_USED),
> +};
> +
> +static int mtk_socinfo_create_socinfo_node(struct mtk_socinfo *mtk_socinfop)
> +{
> +	struct soc_device_attribute *attrs;
> +	static char machine[30] = {0};
> +
> +	attrs = devm_kzalloc(mtk_socinfop->dev, sizeof(*attrs), GFP_KERNEL);
> +	if (!attrs)
> +		return -ENOMEM;
> +
> +	snprintf(machine, 30, "%s (%s)", mtk_socinfop->socinfo_data->marketing_name,
> +		mtk_socinfop->socinfo_data->soc_name);
> +	attrs->family = soc_manufacturer;
> +	attrs->machine = machine;
> +
> +	soc_dev = soc_device_register(attrs);
> +	if (IS_ERR(soc_dev))
> +		return PTR_ERR(soc_dev);
> +
> +	dev_info(mtk_socinfop->dev, "%s SoC detected.\n", attrs->machine);
> +	return 0;
> +}
> +
> +static int mtk_socinfo_get_socinfo_data(struct mtk_socinfo *mtk_socinfop)
> +{
> +	unsigned int i = 0, j = 0;
> +	unsigned int num_cell_data = 0;
> +	u32 *cell_datap[MAX_CELLS] = { NULL };
> +	size_t efuse_bytes;
> +	struct nvmem_cell *cell;
> +	bool match_socinfo = true;
> +	int match_socinfo_index = -1;
> +
> +	for (i = 0; i < MAX_CELLS; i++) {
> +		cell = nvmem_cell_get(mtk_socinfop->dev, cell_names[i]);
> +		if (IS_ERR_OR_NULL(cell))
> +			break;
> +		cell_datap[i] = (u32 *)nvmem_cell_read(cell, &efuse_bytes);
> +		nvmem_cell_put(cell);
> +		num_cell_data++;
> +	}
> +
> +	if (!num_cell_data)
> +		return -ENOENT;
> +
> +	for (i = 0; i < ARRAY_SIZE(socinfo_data_table); i++) {
> +		match_socinfo = true;
> +		for (j = 0; j < num_cell_data; j++) {
> +			if (*(cell_datap[j]) != socinfo_data_table[i].cell_data[j])
> +				match_socinfo = false;
> +		}
> +		if (num_cell_data > 0 && match_socinfo) {
> +			mtk_socinfop->socinfo_data = &(socinfo_data_table[i]);
> +			match_socinfo_index = i;
> +			break;
> +		}
> +	}
> +
> +	return match_socinfo_index >= 0 ? match_socinfo_index : -ENOENT;
> +}
> +
> +static const struct of_device_id mtk_socinfo_id_table[] = {
> +	{ .compatible = "mediatek,socinfo" },
> +	{ /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(of, mtk_socinfo_id_table);
> +
> +static int mtk_socinfo_probe(struct platform_device *pdev)
> +{
> +	struct mtk_socinfo *mtk_socinfop;
> +	int ret = 0;
> +
> +	mtk_socinfop = devm_kzalloc(&pdev->dev, sizeof(*mtk_socinfop), GFP_KERNEL);
> +	if (!mtk_socinfop)
> +		return -ENOMEM;
> +
> +	mtk_socinfop->dev = &pdev->dev;
> +
> +	ret = mtk_socinfo_get_socinfo_data(mtk_socinfop);
> +	if (ret < 0)
> +		return dev_err_probe(mtk_socinfop->dev, ret, "Failed to get socinfo data\n");
> +
> +	ret = mtk_socinfo_create_socinfo_node(mtk_socinfop);
> +	if (ret != 0)
> +		return dev_err_probe(mtk_socinfop->dev, -EINVAL, "Failed to create socinfo node\n");

Wrap your code according to Linux coding style (see Coding style
document, not checkpatch).

> +
> +	return 0;
> +}
> +
> +static int mtk_socinfo_remove(struct platform_device *pdev)
> +{
> +	if (soc_dev)

And how it could be NULL here? This does not make any sense.

> +		soc_device_unregister(soc_dev);
> +	return 0;
> +}
> +
> +static struct platform_driver mtk_socinfo = {
> +	.probe          = mtk_socinfo_probe,
> +	.remove         = mtk_socinfo_remove,
> +	.driver         = {
> +		.name   = "mtk_socinfo",
> +		.owner  = THIS_MODULE,

Really? We dropped it years ago.

Before submitting new drivers, always, *ALWAYS*, run coccinelle, smatch
and sparse.

Best regards,
Krzysztof

Re: [PATCH v2 0/3] mtk-socinfo driver implementation

[Krzysztof Kozlowski]

On 15/09/2023 17:26, William-tw Lin wrote:
> This purpose of the patches is for soc-related information retrival.
> Such information includes manufacturer information, SoC name, SoC 
> segment name, and SoC marketing name.
> 
> Based on tag: next-20230915, linux-next/master
> 
> Changes in v2:
> - Remove mtk-socinfo.h
> - Consolidate different compatibles into mediatek,socinfo
> - Move socinfo node out of MMIO bus
> - Move mtk-socinfo.yaml to hwinfo
> - Fix reviewer's comments

This is way to vague, especially that you did not fix them. You need to
list all of them. Then we will nicely see what you ignored.


Best regards,
Krzysztof

Re: [PATCH v2 3/3] dt-bindings: hwinfo: Add mtk-socinfo driver

[AngeloGioacchino Del Regno]

Il 17/09/23 10:26, Krzysztof Kozlowski ha scritto:
> On 15/09/2023 17:26, William-tw Lin wrote:
>> dt-binding documentation for mtk-socinfo driver.
> 
> Here and in subject, drop driver and instead descrbe hardware.
> 
>> mtk-socinfo driver provides SoC-related information.
>> Such information includes manufacturer information, SoC name,
>> SoC segment name, and SoC marketing name.
>>
>> Signed-off-by: William-tw Lin <william-tw.lin@mediatek.com>
>> ---
>>   .../bindings/hwinfo/mtk-socinfo.yaml          | 48 +++++++++++++++++++
>>   1 file changed, 48 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/hwinfo/mtk-socinfo.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/hwinfo/mtk-socinfo.yaml b/Documentation/devicetree/bindings/hwinfo/mtk-socinfo.yaml
>> new file mode 100644
>> index 000000000000..74f03f1dc404
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/hwinfo/mtk-socinfo.yaml
> 
> Nothing improved.
> 
> This is a friendly reminder during the review process.
> 
> It seems my previous comments were not fully addressed. Maybe my
> feedback got lost between the quotes, maybe you just forgot to apply it.
> Please go back to the previous discussion and either implement all
> requested changes or keep discussing them.
> 
> Thank you.
> 
>> @@ -0,0 +1,48 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/hwinfo/mtk-socinfo.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: MediaTek SoC ChipID
>> +
>> +maintainers:
>> +  - William Lin <william-tw.lin@mediatek.com>
>> +  - Matthias Brugger <matthias.bgg@gmail.com>
>> +  - Kevin Hilman <khilman@kernel.org>
>> +  - AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
>> +
>> +description:
>> +  MediaTek SoCs store various product information in eFuses, including
>> +  Chip ID and Revision fields, usable to identify the manufacturer,
>> +  SoC version, plus segment and marketing names.
>> +
>> +properties:
>> +  compatible:
>> +    const: mediatek,socinfo
> 
> What happened to compatibles? No, this is just wrong and no explained.
> You ignored other comments as well. Really, that's not the way to go.
> 

Practically, having different compatibles for each SoC is not needed, as
the only thing that changes between SoCs is the eFuse(s) that you read to
get the information - and that's all.

So ... we either use this driver with devicetree, giving it the right eFuses
to read from, or we duplicate the mtk-efuse driver, or we statically assign
the eFuses in the driver itself and we set compatibles like
"mediatek,mt8195-socinfo" to select that... ideas?

Cheers,
Angelo

> NAK
> 
> Best regards,
> Krzysztof
> 

Re: [PATCH v2 3/3] dt-bindings: hwinfo: Add mtk-socinfo driver

[Krzysztof Kozlowski]

On 18/09/2023 10:47, AngeloGioacchino Del Regno wrote:
> Il 17/09/23 10:26, Krzysztof Kozlowski ha scritto:
>> On 15/09/2023 17:26, William-tw Lin wrote:
>>> dt-binding documentation for mtk-socinfo driver.
>>
>> Here and in subject, drop driver and instead descrbe hardware.
>>
>>> mtk-socinfo driver provides SoC-related information.
>>> Such information includes manufacturer information, SoC name,
>>> SoC segment name, and SoC marketing name.
>>>
>>> Signed-off-by: William-tw Lin <william-tw.lin@mediatek.com>
>>> ---
>>>   .../bindings/hwinfo/mtk-socinfo.yaml          | 48 +++++++++++++++++++
>>>   1 file changed, 48 insertions(+)
>>>   create mode 100644 Documentation/devicetree/bindings/hwinfo/mtk-socinfo.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/hwinfo/mtk-socinfo.yaml b/Documentation/devicetree/bindings/hwinfo/mtk-socinfo.yaml
>>> new file mode 100644
>>> index 000000000000..74f03f1dc404
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/hwinfo/mtk-socinfo.yaml
>>
>> Nothing improved.
>>
>> This is a friendly reminder during the review process.
>>
>> It seems my previous comments were not fully addressed. Maybe my
>> feedback got lost between the quotes, maybe you just forgot to apply it.
>> Please go back to the previous discussion and either implement all
>> requested changes or keep discussing them.
>>
>> Thank you.
>>
>>> @@ -0,0 +1,48 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/hwinfo/mtk-socinfo.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: MediaTek SoC ChipID
>>> +
>>> +maintainers:
>>> +  - William Lin <william-tw.lin@mediatek.com>
>>> +  - Matthias Brugger <matthias.bgg@gmail.com>
>>> +  - Kevin Hilman <khilman@kernel.org>
>>> +  - AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
>>> +
>>> +description:
>>> +  MediaTek SoCs store various product information in eFuses, including
>>> +  Chip ID and Revision fields, usable to identify the manufacturer,
>>> +  SoC version, plus segment and marketing names.
>>> +
>>> +properties:
>>> +  compatible:
>>> +    const: mediatek,socinfo
>>
>> What happened to compatibles? No, this is just wrong and no explained.
>> You ignored other comments as well. Really, that's not the way to go.
>>
> 
> Practically, having different compatibles for each SoC is not needed, as
> the only thing that changes between SoCs is the eFuse(s) that you read to
> get the information - and that's all.


And how do you guarantee that no future SoC will have any difference?
How can you even predict it?

> 
> So ... we either use this driver with devicetree, giving it the right eFuses

I am talking about bindings, no driver.

> to read from, or we duplicate the mtk-efuse driver, or we statically assign
> the eFuses in the driver itself and we set compatibles like
> "mediatek,mt8195-socinfo" to select that... ideas?

Device specific compatibles followed by fallback, just like for every
other review coming from me.

Best regards,
Krzysztof

Re: [PATCH v2 3/3] dt-bindings: hwinfo: Add mtk-socinfo driver

[Rob Herring]

On Mon, Sep 18, 2023 at 10:47:53AM +0200, AngeloGioacchino Del Regno wrote:
> Il 17/09/23 10:26, Krzysztof Kozlowski ha scritto:
> > On 15/09/2023 17:26, William-tw Lin wrote:
> > > dt-binding documentation for mtk-socinfo driver.
> > 
> > Here and in subject, drop driver and instead descrbe hardware.
> > 
> > > mtk-socinfo driver provides SoC-related information.
> > > Such information includes manufacturer information, SoC name,
> > > SoC segment name, and SoC marketing name.
> > > 
> > > Signed-off-by: William-tw Lin <william-tw.lin@mediatek.com>
> > > ---
> > >   .../bindings/hwinfo/mtk-socinfo.yaml          | 48 +++++++++++++++++++
> > >   1 file changed, 48 insertions(+)
> > >   create mode 100644 Documentation/devicetree/bindings/hwinfo/mtk-socinfo.yaml
> > > 
> > > diff --git a/Documentation/devicetree/bindings/hwinfo/mtk-socinfo.yaml b/Documentation/devicetree/bindings/hwinfo/mtk-socinfo.yaml
> > > new file mode 100644
> > > index 000000000000..74f03f1dc404
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/hwinfo/mtk-socinfo.yaml
> > 
> > Nothing improved.
> > 
> > This is a friendly reminder during the review process.
> > 
> > It seems my previous comments were not fully addressed. Maybe my
> > feedback got lost between the quotes, maybe you just forgot to apply it.
> > Please go back to the previous discussion and either implement all
> > requested changes or keep discussing them.
> > 
> > Thank you.
> > 
> > > @@ -0,0 +1,48 @@
> > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/hwinfo/mtk-socinfo.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: MediaTek SoC ChipID
> > > +
> > > +maintainers:
> > > +  - William Lin <william-tw.lin@mediatek.com>
> > > +  - Matthias Brugger <matthias.bgg@gmail.com>
> > > +  - Kevin Hilman <khilman@kernel.org>
> > > +  - AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> > > +
> > > +description:
> > > +  MediaTek SoCs store various product information in eFuses, including
> > > +  Chip ID and Revision fields, usable to identify the manufacturer,
> > > +  SoC version, plus segment and marketing names.
> > > +
> > > +properties:
> > > +  compatible:
> > > +    const: mediatek,socinfo
> > 
> > What happened to compatibles? No, this is just wrong and no explained.
> > You ignored other comments as well. Really, that's not the way to go.
> > 
> 
> Practically, having different compatibles for each SoC is not needed, as
> the only thing that changes between SoCs is the eFuse(s) that you read to
> get the information - and that's all.
> 
> So ... we either use this driver with devicetree, giving it the right eFuses
> to read from, or we duplicate the mtk-efuse driver, or we statically assign
> the eFuses in the driver itself and we set compatibles like
> "mediatek,mt8195-socinfo" to select that... ideas?

So this one is just a virtual device to instantiate a driver and there 
is no socinfo hw block? If so, that's definitely a NAK. Either 
instantiate your socinfo driver from the efuse driver or register as a 
socinfo provider in it.

Rob

Re: [PATCH v2 2/3] soc: mediatek: mtk-socinfo: Add driver for getting chip information

[Christophe JAILLET]

Le 15/09/2023 à 17:26, William-tw Lin a écrit :
> Add driver for socinfo retrieval. This patch includes the following:
> 1. mtk-socinfo driver for chip info retrieval
> 2. Related changes to Makefile and Kconfig
> 
> Signed-off-by: William-tw Lin <william-tw.lin-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
> ---

...

> +static int mtk_socinfo_create_socinfo_node(struct mtk_socinfo *mtk_socinfop)
> +{
> +	struct soc_device_attribute *attrs;
> +	static char machine[30] = {0};
> +
> +	attrs = devm_kzalloc(mtk_socinfop->dev, sizeof(*attrs), GFP_KERNEL);
> +	if (!attrs)
> +		return -ENOMEM;
> +
> +	snprintf(machine, 30, "%s (%s)", mtk_socinfop->socinfo_data->marketing_name,

sizeof(machine) instead of 30?

> +		mtk_socinfop->socinfo_data->soc_name);
> +	attrs->family = soc_manufacturer;
> +	attrs->machine = machine;
> +
> +	soc_dev = soc_device_register(attrs);
> +	if (IS_ERR(soc_dev))
> +		return PTR_ERR(soc_dev);
> +
> +	dev_info(mtk_socinfop->dev, "%s SoC detected.\n", attrs->machine);
> +	return 0;
> +}
> +
> +static int mtk_socinfo_get_socinfo_data(struct mtk_socinfo *mtk_socinfop)
> +{
> +	unsigned int i = 0, j = 0;

No need to init.

> +	unsigned int num_cell_data = 0;
> +	u32 *cell_datap[MAX_CELLS] = { NULL };
> +	size_t efuse_bytes;
> +	struct nvmem_cell *cell;
> +	bool match_socinfo = true;

No need to init.

> +	int match_socinfo_index = -1;
> +
> +	for (i = 0; i < MAX_CELLS; i++) {
> +		cell = nvmem_cell_get(mtk_socinfop->dev, cell_names[i]);
> +		if (IS_ERR_OR_NULL(cell))

I don't think that testing for NULL is needed.

> +			break;
> +		cell_datap[i] = (u32 *)nvmem_cell_read(cell, &efuse_bytes);
> +		nvmem_cell_put(cell);
> +		num_cell_data++;
> +	}
> +
> +	if (!num_cell_data)
> +		return -ENOENT;
> +
> +	for (i = 0; i < ARRAY_SIZE(socinfo_data_table); i++) {
> +		match_socinfo = true;
> +		for (j = 0; j < num_cell_data; j++) {
> +			if (*(cell_datap[j]) != socinfo_data_table[i].cell_data[j])
> +				match_socinfo = false;

I think that a "break" could be added.

> +		}
> +		if (num_cell_data > 0 && match_socinfo) {

This test can be simplified, becasue 'num_cell_data' can't be <= 0. It 
is an unsigned int, and we return -ENOENT if it is zero.

> +			mtk_socinfop->socinfo_data = &(socinfo_data_table[i]);
> +			match_socinfo_index = i;
> +			break;
> +		}
> +	}
> +
> +	return match_socinfo_index >= 0 ? match_socinfo_index : -ENOENT;
> +}
> +
> +static const struct of_device_id mtk_socinfo_id_table[] = {
> +	{ .compatible = "mediatek,socinfo" },
> +	{ /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(of, mtk_socinfo_id_table);
> +
> +static int mtk_socinfo_probe(struct platform_device *pdev)
> +{
> +	struct mtk_socinfo *mtk_socinfop;
> +	int ret = 0;

No need to init.

> +
> +	mtk_socinfop = devm_kzalloc(&pdev->dev, sizeof(*mtk_socinfop), GFP_KERNEL);
> +	if (!mtk_socinfop)
> +		return -ENOMEM;
> +
> +	mtk_socinfop->dev = &pdev->dev;
> +
> +	ret = mtk_socinfo_get_socinfo_data(mtk_socinfop);
> +	if (ret < 0)
> +		return dev_err_probe(mtk_socinfop->dev, ret, "Failed to get socinfo data\n");
> +
> +	ret = mtk_socinfo_create_socinfo_node(mtk_socinfop);
> +	if (ret != 0)
> +		return dev_err_probe(mtk_socinfop->dev, -EINVAL, "Failed to create socinfo node\n");

Why not propagating 'ret'?

CJ

> +
> +	return 0;
> +}

...

Re: [PATCH v2 3/3] dt-bindings: hwinfo: Add mtk-socinfo driver

[AngeloGioacchino Del Regno]

Il 18/09/23 13:59, Krzysztof Kozlowski ha scritto:
> On 18/09/2023 10:47, AngeloGioacchino Del Regno wrote:
>> Il 17/09/23 10:26, Krzysztof Kozlowski ha scritto:
>>> On 15/09/2023 17:26, William-tw Lin wrote:
>>>> dt-binding documentation for mtk-socinfo driver.
>>>
>>> Here and in subject, drop driver and instead descrbe hardware.
>>>
>>>> mtk-socinfo driver provides SoC-related information.
>>>> Such information includes manufacturer information, SoC name,
>>>> SoC segment name, and SoC marketing name.
>>>>
>>>> Signed-off-by: William-tw Lin <william-tw.lin@mediatek.com>
>>>> ---
>>>>    .../bindings/hwinfo/mtk-socinfo.yaml          | 48 +++++++++++++++++++
>>>>    1 file changed, 48 insertions(+)
>>>>    create mode 100644 Documentation/devicetree/bindings/hwinfo/mtk-socinfo.yaml
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/hwinfo/mtk-socinfo.yaml b/Documentation/devicetree/bindings/hwinfo/mtk-socinfo.yaml
>>>> new file mode 100644
>>>> index 000000000000..74f03f1dc404
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/hwinfo/mtk-socinfo.yaml
>>>
>>> Nothing improved.
>>>
>>> This is a friendly reminder during the review process.
>>>
>>> It seems my previous comments were not fully addressed. Maybe my
>>> feedback got lost between the quotes, maybe you just forgot to apply it.
>>> Please go back to the previous discussion and either implement all
>>> requested changes or keep discussing them.
>>>
>>> Thank you.
>>>
>>>> @@ -0,0 +1,48 @@
>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>> +%YAML 1.2
>>>> +---
>>>> +$id: http://devicetree.org/schemas/hwinfo/mtk-socinfo.yaml#
>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>> +
>>>> +title: MediaTek SoC ChipID
>>>> +
>>>> +maintainers:
>>>> +  - William Lin <william-tw.lin@mediatek.com>
>>>> +  - Matthias Brugger <matthias.bgg@gmail.com>
>>>> +  - Kevin Hilman <khilman@kernel.org>
>>>> +  - AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
>>>> +
>>>> +description:
>>>> +  MediaTek SoCs store various product information in eFuses, including
>>>> +  Chip ID and Revision fields, usable to identify the manufacturer,
>>>> +  SoC version, plus segment and marketing names.
>>>> +
>>>> +properties:
>>>> +  compatible:
>>>> +    const: mediatek,socinfo
>>>
>>> What happened to compatibles? No, this is just wrong and no explained.
>>> You ignored other comments as well. Really, that's not the way to go.
>>>
>>
>> Practically, having different compatibles for each SoC is not needed, as
>> the only thing that changes between SoCs is the eFuse(s) that you read to
>> get the information - and that's all.
> 
> 
> And how do you guarantee that no future SoC will have any difference?
> How can you even predict it?
> 

You're right, it's too much long sighted and at some point in the future this
assumption will be *inevitably* wrong.

Thanks,
Angelo

>>
>> So ... we either use this driver with devicetree, giving it the right eFuses
> 
> I am talking about bindings, no driver.
> 
>> to read from, or we duplicate the mtk-efuse driver, or we statically assign
>> the eFuses in the driver itself and we set compatibles like
>> "mediatek,mt8195-socinfo" to select that... ideas?
> 
> Device specific compatibles followed by fallback, just like for every
> other review coming from me.
>  > Best regards,
> Krzysztof
>