[PATCH v4 0/5] soc: qcom: Add SoC info driver

[PATCH v4 0/5] soc: qcom: Add SoC info driver

[Vaishali Thakkar]

This patchset adds SoC info driver which can provide information
such as Chip ID, Chip family and serial number about Qualcomm SoCs
to user space via sysfs. Furthermore, it allows userspace to get
information about custom attributes and various image version
information via debugfs.

The patchset cleanly applies on top of v5.0-rc6.

Changes since v1:
        - Align ifdefs to left, remove unnecessary debugfs dir
          creation check and fix function signatures in patch 3
        - Fix comment for teh case when serial number is not
          available in patch 1
Changes since v2:
        - Reorder patches [patch five -> patch two]

Changes since v3:
	- Add reviewed-bys from Greg
	- Fix build warning when debugfs is disabled
	- Remove extra checks for dir creations in patch 5

Vaishali Thakkar (5):
  base: soc: Add serial_number attribute to soc
  base: soc: Export soc_device_register/unregister APIs
  soc: qcom: Add socinfo driver
  soc: qcom: socinfo: Expose custom attributes
  soc: qcom: socinfo: Expose image information

 Documentation/ABI/testing/sysfs-devices-soc |   7 +
 drivers/base/soc.c                          |   9 +
 drivers/soc/qcom/Kconfig                    |   8 +
 drivers/soc/qcom/Makefile                   |   1 +
 drivers/soc/qcom/smem.c                     |   8 +
 drivers/soc/qcom/socinfo.c                  | 573 ++++++++++++++++++++
 include/linux/sys_soc.h                     |   1 +
 7 files changed, 607 insertions(+)
 create mode 100644 drivers/soc/qcom/socinfo.c

-- 
2.17.1

[PATCH v4 1/5] base: soc: Add serial_number attribute to soc

[Vaishali Thakkar]

From: Bjorn Andersson <bjorn.andersson@linaro.org>

Add new attribute named "serial_number" as a standard interface for
user space to acquire the serial number of the device.

For ST-Ericsson SoCs this is exposed by the cryptically named "soc_id"
attribute, but this provides a human readable standardized name for this
property.

Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Signed-off-by: Vaishali Thakkar <vaishali.thakkar@linaro.org>
Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
Changes since v3:
	- Add Greg's Reviewed-by
Changes since v2:
	- None
Changes since v1:
	- Make comment more clear for the case when serial
	  number is not available
---
 Documentation/ABI/testing/sysfs-devices-soc | 7 +++++++
 drivers/base/soc.c                          | 7 +++++++
 include/linux/sys_soc.h                     | 1 +
 3 files changed, 15 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-devices-soc b/Documentation/ABI/testing/sysfs-devices-soc
index 6d9cc253f2b2..ba3a3fac0ee1 100644
--- a/Documentation/ABI/testing/sysfs-devices-soc
+++ b/Documentation/ABI/testing/sysfs-devices-soc
@@ -26,6 +26,13 @@ Description:
 		Read-only attribute common to all SoCs. Contains SoC family name
 		(e.g. DB8500).
 
+What:		/sys/devices/socX/serial_number
+Date:		January 2019
+contact:	Bjorn Andersson <bjorn.andersson@linaro.org>
+Description:
+		Read-only attribute supported by most SoCs. Contains the SoC's
+		serial number, if available.
+
 What:		/sys/devices/socX/soc_id
 Date:		January 2012
 contact:	Lee Jones <lee.jones@linaro.org>
diff --git a/drivers/base/soc.c b/drivers/base/soc.c
index 10b280f30217..b0933b9fe67f 100644
--- a/drivers/base/soc.c
+++ b/drivers/base/soc.c
@@ -33,6 +33,7 @@ static struct bus_type soc_bus_type = {
 
 static DEVICE_ATTR(machine,  S_IRUGO, soc_info_get,  NULL);
 static DEVICE_ATTR(family,   S_IRUGO, soc_info_get,  NULL);
+static DEVICE_ATTR(serial_number, S_IRUGO, soc_info_get,  NULL);
 static DEVICE_ATTR(soc_id,   S_IRUGO, soc_info_get,  NULL);
 static DEVICE_ATTR(revision, S_IRUGO, soc_info_get,  NULL);
 
@@ -57,6 +58,9 @@ static umode_t soc_attribute_mode(struct kobject *kobj,
 	if ((attr == &dev_attr_revision.attr)
 	    && (soc_dev->attr->revision != NULL))
 		return attr->mode;
+	if ((attr == &dev_attr_serial_number.attr)
+	    && (soc_dev->attr->serial_number != NULL))
+		return attr->mode;
 	if ((attr == &dev_attr_soc_id.attr)
 	    && (soc_dev->attr->soc_id != NULL))
 		return attr->mode;
@@ -77,6 +81,8 @@ static ssize_t soc_info_get(struct device *dev,
 		return sprintf(buf, "%s\n", soc_dev->attr->family);
 	if (attr == &dev_attr_revision)
 		return sprintf(buf, "%s\n", soc_dev->attr->revision);
+	if (attr == &dev_attr_serial_number)
+		return sprintf(buf, "%s\n", soc_dev->attr->serial_number);
 	if (attr == &dev_attr_soc_id)
 		return sprintf(buf, "%s\n", soc_dev->attr->soc_id);
 
@@ -87,6 +93,7 @@ static ssize_t soc_info_get(struct device *dev,
 static struct attribute *soc_attr[] = {
 	&dev_attr_machine.attr,
 	&dev_attr_family.attr,
+	&dev_attr_serial_number.attr,
 	&dev_attr_soc_id.attr,
 	&dev_attr_revision.attr,
 	NULL,
diff --git a/include/linux/sys_soc.h b/include/linux/sys_soc.h
index bed223b70217..2a13bc033bd6 100644
--- a/include/linux/sys_soc.h
+++ b/include/linux/sys_soc.h
@@ -12,6 +12,7 @@ struct soc_device_attribute {
 	const char *machine;
 	const char *family;
 	const char *revision;
+	const char *serial_number;
 	const char *soc_id;
 	const void *data;
 };
-- 
2.17.1

Re: [PATCH v4 1/5] base: soc: Add serial_number attribute to soc

[Stephen Boyd]

Quoting Vaishali Thakkar (2019-02-24 22:50:40)
> From: Bjorn Andersson <bjorn.andersson@linaro.org>
> 
> Add new attribute named "serial_number" as a standard interface for
> user space to acquire the serial number of the device.
> 
> For ST-Ericsson SoCs this is exposed by the cryptically named "soc_id"
> attribute, but this provides a human readable standardized name for this
> property.
> 
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> Signed-off-by: Vaishali Thakkar <vaishali.thakkar@linaro.org>
> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---

Reviewed-by: Stephen Boyd <swboyd@chromium.org>

Re: [PATCH v4 1/5] base: soc: Add serial_number attribute to soc

[Stephen Boyd]

Quoting Vaishali Thakkar (2019-02-24 22:50:40)
> From: Bjorn Andersson <bjorn.andersson@linaro.org>
> 
> Add new attribute named "serial_number" as a standard interface for
> user space to acquire the serial number of the device.
> 
> For ST-Ericsson SoCs this is exposed by the cryptically named "soc_id"
> attribute, but this provides a human readable standardized name for this
> property.
> 
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> Signed-off-by: Vaishali Thakkar <vaishali.thakkar@linaro.org>
> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---

Reviewed-by: Stephen Boyd <swboyd@chromium.org>

[PATCH v4 2/5] base: soc: Export soc_device_register/unregister APIs

[Vaishali Thakkar]

From: Vinod Koul <vkoul@kernel.org>

Qcom Socinfo driver can be built as a module, so
export these two APIs.

Signed-off-by: Vinod Koul <vkoul@kernel.org>
Signed-off-by: Vaishali Thakkar <vaishali.thakkar@linaro.org>
Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
Changes since v3:
	- Add Greg's reviewed-by
Changes since v2:
	- Reorder patches [patch 5->patch 2]
Changes since v1:
	- None
---
 drivers/base/soc.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/base/soc.c b/drivers/base/soc.c
index b0933b9fe67f..7c0c5ca5953d 100644
--- a/drivers/base/soc.c
+++ b/drivers/base/soc.c
@@ -164,6 +164,7 @@ struct soc_device *soc_device_register(struct soc_device_attribute *soc_dev_attr
 out1:
 	return ERR_PTR(ret);
 }
+EXPORT_SYMBOL_GPL(soc_device_register);
 
 /* Ensure soc_dev->attr is freed prior to calling soc_device_unregister. */
 void soc_device_unregister(struct soc_device *soc_dev)
@@ -173,6 +174,7 @@ void soc_device_unregister(struct soc_device *soc_dev)
 	device_unregister(&soc_dev->dev);
 	early_soc_dev_attr = NULL;
 }
+EXPORT_SYMBOL_GPL(soc_device_unregister);
 
 static int __init soc_bus_register(void)
 {
-- 
2.17.1

Re: [PATCH v4 2/5] base: soc: Export soc_device_register/unregister APIs

[Stephen Boyd]

Quoting Vaishali Thakkar (2019-02-24 22:50:41)
> From: Vinod Koul <vkoul@kernel.org>
> 
> Qcom Socinfo driver can be built as a module, so
> export these two APIs.
> 
> Signed-off-by: Vinod Koul <vkoul@kernel.org>
> Signed-off-by: Vaishali Thakkar <vaishali.thakkar@linaro.org>
> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---

Reviewed-by: Stephen Boyd <swboyd@chromium.org>

Re: [PATCH v4 2/5] base: soc: Export soc_device_register/unregister APIs

[Stephen Boyd]

Quoting Vaishali Thakkar (2019-02-24 22:50:41)
> From: Vinod Koul <vkoul@kernel.org>
> 
> Qcom Socinfo driver can be built as a module, so
> export these two APIs.
> 
> Signed-off-by: Vinod Koul <vkoul@kernel.org>
> Signed-off-by: Vaishali Thakkar <vaishali.thakkar@linaro.org>
> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---

Reviewed-by: Stephen Boyd <swboyd@chromium.org>

Re: [PATCH v4 2/5] base: soc: Export soc_device_register/unregister APIs

[Bjorn Andersson]

On Sun 24 Feb 22:50 PST 2019, Vaishali Thakkar wrote:

> From: Vinod Koul <vkoul@kernel.org>
> 
> Qcom Socinfo driver can be built as a module, so
> export these two APIs.
> 
> Signed-off-by: Vinod Koul <vkoul@kernel.org>
> Signed-off-by: Vaishali Thakkar <vaishali.thakkar@linaro.org>
> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Regards,
Bjorn

> ---
> Changes since v3:
> 	- Add Greg's reviewed-by
> Changes since v2:
> 	- Reorder patches [patch 5->patch 2]
> Changes since v1:
> 	- None
> ---
>  drivers/base/soc.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/base/soc.c b/drivers/base/soc.c
> index b0933b9fe67f..7c0c5ca5953d 100644
> --- a/drivers/base/soc.c
> +++ b/drivers/base/soc.c
> @@ -164,6 +164,7 @@ struct soc_device *soc_device_register(struct soc_device_attribute *soc_dev_attr
>  out1:
>  	return ERR_PTR(ret);
>  }
> +EXPORT_SYMBOL_GPL(soc_device_register);
>  
>  /* Ensure soc_dev->attr is freed prior to calling soc_device_unregister. */
>  void soc_device_unregister(struct soc_device *soc_dev)
> @@ -173,6 +174,7 @@ void soc_device_unregister(struct soc_device *soc_dev)
>  	device_unregister(&soc_dev->dev);
>  	early_soc_dev_attr = NULL;
>  }
> +EXPORT_SYMBOL_GPL(soc_device_unregister);
>  
>  static int __init soc_bus_register(void)
>  {
> -- 
> 2.17.1
> 

[PATCH v4 3/5] soc: qcom: Add socinfo driver

[Vaishali Thakkar]

From: Imran Khan <kimran@codeaurora.org>

The Qualcomm socinfo driver exposes information about the SoC, its
version and its serial number to user space.

Signed-off-by: Imran Khan <kimran@codeaurora.org>
[Bjorn: Extract code to platform_driver, split patch in multiple]
Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
[Vaishali: Simplify declarations, introduce qcom_socinfo struc, Fix
           memory leak, Remove extra code and Misc code refactoring]
Signed-off-by: Vaishali Thakkar <vaishali.thakkar@linaro.org>
---
Changes since v3:
	- None
Changes since v2:
        - Fix typo in the work log under signed off by s
Changes since v1:
        - None
---
 drivers/soc/qcom/Kconfig   |   8 ++
 drivers/soc/qcom/Makefile  |   1 +
 drivers/soc/qcom/smem.c    |   8 ++
 drivers/soc/qcom/socinfo.c | 197 +++++++++++++++++++++++++++++++++++++
 4 files changed, 214 insertions(+)
 create mode 100644 drivers/soc/qcom/socinfo.c

diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
index fcbf8a2e4080..1e31eda07934 100644
--- a/drivers/soc/qcom/Kconfig
+++ b/drivers/soc/qcom/Kconfig
@@ -144,6 +144,14 @@ config QCOM_SMSM
 	  Say yes here to support the Qualcomm Shared Memory State Machine.
 	  The state machine is represented by bits in shared memory.
 
+config QCOM_SOCINFO
+	tristate "Qualcomm socinfo driver"
+	depends on QCOM_SMEM
+	select SOC_BUS
+	help
+	 Say yes here to support the Qualcomm socinfo driver, providing
+	 information about the SoC to user space.
+
 config QCOM_WCNSS_CTRL
 	tristate "Qualcomm WCNSS control driver"
 	depends on ARCH_QCOM || COMPILE_TEST
diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
index f25b54cd6cf8..c817da4f4140 100644
--- a/drivers/soc/qcom/Makefile
+++ b/drivers/soc/qcom/Makefile
@@ -14,6 +14,7 @@ qcom_rpmh-y			+= rpmh-rsc.o
 qcom_rpmh-y			+= rpmh.o
 obj-$(CONFIG_QCOM_SMD_RPM)	+= smd-rpm.o
 obj-$(CONFIG_QCOM_SMEM) +=	smem.o
+obj-$(CONFIG_QCOM_SOCINFO)     += socinfo.o
 obj-$(CONFIG_QCOM_SMEM_STATE) += smem_state.o
 obj-$(CONFIG_QCOM_SMP2P)	+= smp2p.o
 obj-$(CONFIG_QCOM_SMSM)	+= smsm.o
diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c
index f80d040601fd..efe0b053ef82 100644
--- a/drivers/soc/qcom/smem.c
+++ b/drivers/soc/qcom/smem.c
@@ -276,6 +276,7 @@ struct qcom_smem {
 	struct smem_partition_header *partitions[SMEM_HOST_COUNT];
 	size_t cacheline[SMEM_HOST_COUNT];
 	u32 item_count;
+	struct platform_device *socinfo;
 
 	unsigned num_regions;
 	struct smem_region regions[];
@@ -971,11 +972,18 @@ static int qcom_smem_probe(struct platform_device *pdev)
 
 	__smem = smem;
 
+	smem->socinfo = platform_device_register_data(&pdev->dev, "qcom-socinfo",
+						      PLATFORM_DEVID_NONE, NULL,
+						      0);
+	if (IS_ERR(smem->socinfo))
+		dev_err(&pdev->dev, "failed to register socinfo device\n");
+
 	return 0;
 }
 
 static int qcom_smem_remove(struct platform_device *pdev)
 {
+
 	hwspin_lock_free(__smem->hwlock);
 	__smem = NULL;
 
diff --git a/drivers/soc/qcom/socinfo.c b/drivers/soc/qcom/socinfo.c
new file mode 100644
index 000000000000..02078049fac7
--- /dev/null
+++ b/drivers/soc/qcom/socinfo.c
@@ -0,0 +1,197 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2009-2017, The Linux Foundation. All rights reserved.
+ * Copyright (c) 2017-2019, Linaro Ltd.
+ */
+
+#include <linux/err.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/random.h>
+#include <linux/slab.h>
+#include <linux/soc/qcom/smem.h>
+#include <linux/string.h>
+#include <linux/sys_soc.h>
+#include <linux/types.h>
+
+/*
+ * SoC version type with major number in the upper 16 bits and minor
+ * number in the lower 16 bits.
+ */
+#define SOCINFO_MAJOR(ver) (((ver) >> 16) & 0xffff)
+#define SOCINFO_MINOR(ver) ((ver) & 0xffff)
+
+#define SMEM_SOCINFO_BUILD_ID_LENGTH           32
+
+/*
+ * SMEM item ids, used to acquire handles to respective
+ * SMEM region.
+ */
+#define SMEM_HW_SW_BUILD_ID            137
+
+/* Socinfo SMEM item structure */
+struct socinfo {
+	__le32 fmt;
+	__le32 id;
+	__le32 ver;
+	char build_id[SMEM_SOCINFO_BUILD_ID_LENGTH];
+	/* Version 2 */
+	__le32 raw_id;
+	__le32 raw_ver;
+	/* Version 3 */
+	__le32 hw_plat;
+	/* Version 4 */
+	__le32 plat_ver;
+	/* Version 5 */
+	__le32 accessory_chip;
+	/* Version 6 */
+	__le32 hw_plat_subtype;
+	/* Version 7 */
+	__le32 pmic_model;
+	__le32 pmic_die_rev;
+	/* Version 8 */
+	__le32 pmic_model_1;
+	__le32 pmic_die_rev_1;
+	__le32 pmic_model_2;
+	__le32 pmic_die_rev_2;
+	/* Version 9 */
+	__le32 foundry_id;
+	/* Version 10 */
+	__le32 serial_num;
+	/* Version 11 */
+	__le32 num_pmics;
+	__le32 pmic_array_offset;
+	/* Version 12 */
+	__le32 chip_family;
+	__le32 raw_device_family;
+	__le32 raw_device_num;
+};
+
+struct qcom_socinfo {
+	struct soc_device *soc_dev;
+	struct soc_device_attribute attr;
+};
+
+struct soc_of_id {
+	unsigned int id;
+	const char *name;
+};
+
+static const struct soc_of_id soc_of_id[] = {
+	{87, "MSM8960"},
+	{109, "APQ8064"},
+	{122, "MSM8660A"},
+	{123, "MSM8260A"},
+	{124, "APQ8060A"},
+	{126, "MSM8974"},
+	{130, "MPQ8064"},
+	{138, "MSM8960AB"},
+	{139, "APQ8060AB"},
+	{140, "MSM8260AB"},
+	{141, "MSM8660AB"},
+	{178, "APQ8084"},
+	{184, "APQ8074"},
+	{185, "MSM8274"},
+	{186, "MSM8674"},
+	{194, "MSM8974PRO"},
+	{206, "MSM8916"},
+	{208, "APQ8074-AA"},
+	{209, "APQ8074-AB"},
+	{210, "APQ8074PRO"},
+	{211, "MSM8274-AA"},
+	{212, "MSM8274-AB"},
+	{213, "MSM8274PRO"},
+	{214, "MSM8674-AA"},
+	{215, "MSM8674-AB"},
+	{216, "MSM8674PRO"},
+	{217, "MSM8974-AA"},
+	{218, "MSM8974-AB"},
+	{246, "MSM8996"},
+	{247, "APQ8016"},
+	{248, "MSM8216"},
+	{249, "MSM8116"},
+	{250, "MSM8616"},
+	{291, "APQ8096"},
+	{305, "MSM8996SG"},
+	{310, "MSM8996AU"},
+	{311, "APQ8096AU"},
+	{312, "APQ8096SG"},
+};
+
+static const char *socinfo_machine(struct device *dev, unsigned int id)
+{
+	int idx;
+
+	for (idx = 0; idx < ARRAY_SIZE(soc_of_id); idx++) {
+		if (soc_of_id[idx].id == id)
+			return soc_of_id[idx].name;
+	}
+
+	if (IS_ERR(soc_of_id[idx].name))
+		dev_err(dev, "Unknown soc id\n");
+
+	return NULL;
+}
+
+static int qcom_socinfo_probe(struct platform_device *pdev)
+{
+	struct qcom_socinfo *qs;
+	struct socinfo *info;
+	size_t item_size;
+
+	info = qcom_smem_get(QCOM_SMEM_HOST_ANY, SMEM_HW_SW_BUILD_ID,
+			      &item_size);
+	if (IS_ERR(info)) {
+		dev_err(&pdev->dev, "Couldn't find socinfo\n");
+		return -EINVAL;
+	}
+
+	qs = devm_kzalloc(&pdev->dev, sizeof(*qs), GFP_KERNEL);
+	if (!qs)
+		return -ENOMEM;
+
+	qs->attr.family = "Snapdragon";
+	qs->attr.machine = socinfo_machine(&pdev->dev,
+					   le32_to_cpu(info->id));
+	qs->attr.revision = devm_kasprintf(&pdev->dev, GFP_KERNEL, "%u.%u",
+					   SOCINFO_MAJOR(le32_to_cpu(info->ver)),
+					   SOCINFO_MINOR(le32_to_cpu(info->ver)));
+	if (le32_to_cpu(info->fmt) >= 10)
+		qs->attr.serial_number = devm_kasprintf(&pdev->dev, GFP_KERNEL,
+							"%u",
+							le32_to_cpu(info->serial_num));
+
+	qs->soc_dev = soc_device_register(&qs->attr);
+	if (IS_ERR(qs->soc_dev))
+		return PTR_ERR(qs->soc_dev);
+
+	/* Feed the soc specific unique data into entropy pool */
+	add_device_randomness(info, item_size);
+
+	platform_set_drvdata(pdev, qs->soc_dev);
+
+	return 0;
+}
+
+static int qcom_socinfo_remove(struct platform_device *pdev)
+{
+	struct qcom_socinfo *qs = platform_get_drvdata(pdev);
+
+	soc_device_unregister(qs->soc_dev);
+
+	return 0;
+}
+
+static struct platform_driver qcom_socinfo_driver = {
+	.probe = qcom_socinfo_probe,
+	.remove = qcom_socinfo_remove,
+	.driver  = {
+		.name = "qcom-socinfo",
+	},
+};
+
+module_platform_driver(qcom_socinfo_driver);
+
+MODULE_DESCRIPTION("Qualcomm socinfo driver");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:qcom-socinfo");
-- 
2.17.1

Re: [PATCH v4 3/5] soc: qcom: Add socinfo driver

[Stephen Boyd]

Quoting Vaishali Thakkar (2019-02-24 22:50:42)
> diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c
> index f80d040601fd..efe0b053ef82 100644
> --- a/drivers/soc/qcom/smem.c
> +++ b/drivers/soc/qcom/smem.c
> @@ -971,11 +972,18 @@ static int qcom_smem_probe(struct platform_device *pdev)
>  
>         __smem = smem;
>  
> +       smem->socinfo = platform_device_register_data(&pdev->dev, "qcom-socinfo",
> +                                                     PLATFORM_DEVID_NONE, NULL,
> +                                                     0);
> +       if (IS_ERR(smem->socinfo))
> +               dev_err(&pdev->dev, "failed to register socinfo device\n");

But we don't fail the probe? Maybe a comment should be added indicating
why it's ok to not fail here, and dev_err should be changed to dev_dbg()
then.

> +
>         return 0;
>  }
>  
>  static int qcom_smem_remove(struct platform_device *pdev)
>  {
> +

Nitpick: Drop this change? Or add the code to remove the socinfo device
that is created in probe?

>         hwspin_lock_free(__smem->hwlock);
>         __smem = NULL;
>  
> diff --git a/drivers/soc/qcom/socinfo.c b/drivers/soc/qcom/socinfo.c
> new file mode 100644
> index 000000000000..02078049fac7
> --- /dev/null
> +++ b/drivers/soc/qcom/socinfo.c
> @@ -0,0 +1,197 @@
[...]
> +
> +struct soc_of_id {
> +       unsigned int id;
> +       const char *name;
> +};
> +
> +static const struct soc_of_id soc_of_id[] = {
> +       {87, "MSM8960"},

Nitpick: Please space out the numbers and strings:

	{ 87, "MSM8960" },

> +};
> +
> +static const char *socinfo_machine(struct device *dev, unsigned int id)
> +{
> +       int idx;
> +
> +       for (idx = 0; idx < ARRAY_SIZE(soc_of_id); idx++) {
> +               if (soc_of_id[idx].id == id)
> +                       return soc_of_id[idx].name;

Why is it called soc_of_id? Is that supposed to be "SoC of ID" or is it
DT based and is "SoC OF ID"? If it's the latter, I'd prefer some non-DT
type of name to provide more clarity that this has nothing to do with
DeviceTree.

> +       }
> +
> +       if (IS_ERR(soc_of_id[idx].name))
> +               dev_err(dev, "Unknown soc id\n");
> +
> +       return NULL;
> +}
> +
> +static int qcom_socinfo_probe(struct platform_device *pdev)
> +{
> +       struct qcom_socinfo *qs;
> +       struct socinfo *info;
> +       size_t item_size;
> +
> +       info = qcom_smem_get(QCOM_SMEM_HOST_ANY, SMEM_HW_SW_BUILD_ID,
> +                             &item_size);
> +       if (IS_ERR(info)) {
> +               dev_err(&pdev->dev, "Couldn't find socinfo\n");
> +               return -EINVAL;

Why not return PTR_ERR(info)?

> +       }
> +
> +       qs = devm_kzalloc(&pdev->dev, sizeof(*qs), GFP_KERNEL);
> +       if (!qs)
> +               return -ENOMEM;
> +
> +       qs->attr.family = "Snapdragon";
> +       qs->attr.machine = socinfo_machine(&pdev->dev,
> +                                          le32_to_cpu(info->id));
> +       qs->attr.revision = devm_kasprintf(&pdev->dev, GFP_KERNEL, "%u.%u",
> +                                          SOCINFO_MAJOR(le32_to_cpu(info->ver)),
> +                                          SOCINFO_MINOR(le32_to_cpu(info->ver)));
> +       if (le32_to_cpu(info->fmt) >= 10)

Maybe this would make more sense if it was written with item_size and
offset of info->fmt? Something like

	if (offsetof(struct qcom_socinfo, serial_num) <= item_size)

> +               qs->attr.serial_number = devm_kasprintf(&pdev->dev, GFP_KERNEL,
> +                                                       "%u",
> +                                                       le32_to_cpu(info->serial_num));
> +
> +       qs->soc_dev = soc_device_register(&qs->attr);
> +       if (IS_ERR(qs->soc_dev))
> +               return PTR_ERR(qs->soc_dev);
> +
> +       /* Feed the soc specific unique data into entropy pool */
> +       add_device_randomness(info, item_size);
> +
> +       platform_set_drvdata(pdev, qs->soc_dev);

Weird, this is setting it to qs->soc_dev....

> +
> +       return 0;
> +}
> +
> +static int qcom_socinfo_remove(struct platform_device *pdev)
> +{
> +       struct qcom_socinfo *qs = platform_get_drvdata(pdev);

And then extracting this as qs only...


> +
> +       soc_device_unregister(qs->soc_dev);

And so it looks wrong? Probably already have qs->soc_dev from the
platform_get_drvdata() call?

> +
> +       return 0;
> +}
> +
> +static struct platform_driver qcom_socinfo_driver = {
> +       .probe = qcom_socinfo_probe,
> +       .remove = qcom_socinfo_remove,
> +       .driver  = {
> +               .name = "qcom-socinfo",
> +       },
> +};
> +
> +module_platform_driver(qcom_socinfo_driver);
> +
> +MODULE_DESCRIPTION("Qualcomm socinfo driver");

Maybe write socinfo as SoCinfo here?

> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("platform:qcom-socinfo");

Re: [PATCH v4 3/5] soc: qcom: Add socinfo driver

[Stephen Boyd]

Quoting Vaishali Thakkar (2019-02-24 22:50:42)
> diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c
> index f80d040601fd..efe0b053ef82 100644
> --- a/drivers/soc/qcom/smem.c
> +++ b/drivers/soc/qcom/smem.c
> @@ -971,11 +972,18 @@ static int qcom_smem_probe(struct platform_device *pdev)
>  
>         __smem = smem;
>  
> +       smem->socinfo = platform_device_register_data(&pdev->dev, "qcom-socinfo",
> +                                                     PLATFORM_DEVID_NONE, NULL,
> +                                                     0);
> +       if (IS_ERR(smem->socinfo))
> +               dev_err(&pdev->dev, "failed to register socinfo device\n");

But we don't fail the probe? Maybe a comment should be added indicating
why it's ok to not fail here, and dev_err should be changed to dev_dbg()
then.

> +
>         return 0;
>  }
>  
>  static int qcom_smem_remove(struct platform_device *pdev)
>  {
> +

Nitpick: Drop this change? Or add the code to remove the socinfo device
that is created in probe?

>         hwspin_lock_free(__smem->hwlock);
>         __smem = NULL;
>  
> diff --git a/drivers/soc/qcom/socinfo.c b/drivers/soc/qcom/socinfo.c
> new file mode 100644
> index 000000000000..02078049fac7
> --- /dev/null
> +++ b/drivers/soc/qcom/socinfo.c
> @@ -0,0 +1,197 @@
[...]
> +
> +struct soc_of_id {
> +       unsigned int id;
> +       const char *name;
> +};
> +
> +static const struct soc_of_id soc_of_id[] = {
> +       {87, "MSM8960"},

Nitpick: Please space out the numbers and strings:

	{ 87, "MSM8960" },

> +};
> +
> +static const char *socinfo_machine(struct device *dev, unsigned int id)
> +{
> +       int idx;
> +
> +       for (idx = 0; idx < ARRAY_SIZE(soc_of_id); idx++) {
> +               if (soc_of_id[idx].id == id)
> +                       return soc_of_id[idx].name;

Why is it called soc_of_id? Is that supposed to be "SoC of ID" or is it
DT based and is "SoC OF ID"? If it's the latter, I'd prefer some non-DT
type of name to provide more clarity that this has nothing to do with
DeviceTree.

> +       }
> +
> +       if (IS_ERR(soc_of_id[idx].name))
> +               dev_err(dev, "Unknown soc id\n");
> +
> +       return NULL;
> +}
> +
> +static int qcom_socinfo_probe(struct platform_device *pdev)
> +{
> +       struct qcom_socinfo *qs;
> +       struct socinfo *info;
> +       size_t item_size;
> +
> +       info = qcom_smem_get(QCOM_SMEM_HOST_ANY, SMEM_HW_SW_BUILD_ID,
> +                             &item_size);
> +       if (IS_ERR(info)) {
> +               dev_err(&pdev->dev, "Couldn't find socinfo\n");
> +               return -EINVAL;

Why not return PTR_ERR(info)?

> +       }
> +
> +       qs = devm_kzalloc(&pdev->dev, sizeof(*qs), GFP_KERNEL);
> +       if (!qs)
> +               return -ENOMEM;
> +
> +       qs->attr.family = "Snapdragon";
> +       qs->attr.machine = socinfo_machine(&pdev->dev,
> +                                          le32_to_cpu(info->id));
> +       qs->attr.revision = devm_kasprintf(&pdev->dev, GFP_KERNEL, "%u.%u",
> +                                          SOCINFO_MAJOR(le32_to_cpu(info->ver)),
> +                                          SOCINFO_MINOR(le32_to_cpu(info->ver)));
> +       if (le32_to_cpu(info->fmt) >= 10)

Maybe this would make more sense if it was written with item_size and
offset of info->fmt? Something like

	if (offsetof(struct qcom_socinfo, serial_num) <= item_size)

> +               qs->attr.serial_number = devm_kasprintf(&pdev->dev, GFP_KERNEL,
> +                                                       "%u",
> +                                                       le32_to_cpu(info->serial_num));
> +
> +       qs->soc_dev = soc_device_register(&qs->attr);
> +       if (IS_ERR(qs->soc_dev))
> +               return PTR_ERR(qs->soc_dev);
> +
> +       /* Feed the soc specific unique data into entropy pool */
> +       add_device_randomness(info, item_size);
> +
> +       platform_set_drvdata(pdev, qs->soc_dev);

Weird, this is setting it to qs->soc_dev....

> +
> +       return 0;
> +}
> +
> +static int qcom_socinfo_remove(struct platform_device *pdev)
> +{
> +       struct qcom_socinfo *qs = platform_get_drvdata(pdev);

And then extracting this as qs only...


> +
> +       soc_device_unregister(qs->soc_dev);

And so it looks wrong? Probably already have qs->soc_dev from the
platform_get_drvdata() call?

> +
> +       return 0;
> +}
> +
> +static struct platform_driver qcom_socinfo_driver = {
> +       .probe = qcom_socinfo_probe,
> +       .remove = qcom_socinfo_remove,
> +       .driver  = {
> +               .name = "qcom-socinfo",
> +       },
> +};
> +
> +module_platform_driver(qcom_socinfo_driver);
> +
> +MODULE_DESCRIPTION("Qualcomm socinfo driver");

Maybe write socinfo as SoCinfo here?

> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("platform:qcom-socinfo");

Re: [PATCH v4 3/5] soc: qcom: Add socinfo driver

[Bjorn Andersson]

On Sun 24 Feb 22:50 PST 2019, Vaishali Thakkar wrote:

[..]
> diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
> index f25b54cd6cf8..c817da4f4140 100644
> --- a/drivers/soc/qcom/Makefile
> +++ b/drivers/soc/qcom/Makefile
> @@ -14,6 +14,7 @@ qcom_rpmh-y			+= rpmh-rsc.o
>  qcom_rpmh-y			+= rpmh.o
>  obj-$(CONFIG_QCOM_SMD_RPM)	+= smd-rpm.o
>  obj-$(CONFIG_QCOM_SMEM) +=	smem.o
> +obj-$(CONFIG_QCOM_SOCINFO)     += socinfo.o

Try to keep these sorted by moving this entry down a few steps.

>  obj-$(CONFIG_QCOM_SMEM_STATE) += smem_state.o
>  obj-$(CONFIG_QCOM_SMP2P)	+= smp2p.o
>  obj-$(CONFIG_QCOM_SMSM)	+= smsm.o
> diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c
> index f80d040601fd..efe0b053ef82 100644
> --- a/drivers/soc/qcom/smem.c
> +++ b/drivers/soc/qcom/smem.c
> @@ -276,6 +276,7 @@ struct qcom_smem {
>  	struct smem_partition_header *partitions[SMEM_HOST_COUNT];
>  	size_t cacheline[SMEM_HOST_COUNT];
>  	u32 item_count;
> +	struct platform_device *socinfo;
>  
>  	unsigned num_regions;
>  	struct smem_region regions[];
> @@ -971,11 +972,18 @@ static int qcom_smem_probe(struct platform_device *pdev)
>  
>  	__smem = smem;
>  
> +	smem->socinfo = platform_device_register_data(&pdev->dev, "qcom-socinfo",
> +						      PLATFORM_DEVID_NONE, NULL,
> +						      0);
> +	if (IS_ERR(smem->socinfo))
> +		dev_err(&pdev->dev, "failed to register socinfo device\n");
> +
>  	return 0;
>  }
>  
>  static int qcom_smem_remove(struct platform_device *pdev)
>  {
> +

Let's platform_device_unregister(smem->socinfo) here.

Note that it will handle being passed a ERR_PTR, so no need to make the
call conditional on socinfo being valid.

>  	hwspin_lock_free(__smem->hwlock);
>  	__smem = NULL;
>  
> diff --git a/drivers/soc/qcom/socinfo.c b/drivers/soc/qcom/socinfo.c
> new file mode 100644
> index 000000000000..02078049fac7
> --- /dev/null
> +++ b/drivers/soc/qcom/socinfo.c
> @@ -0,0 +1,197 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2009-2017, The Linux Foundation. All rights reserved.
> + * Copyright (c) 2017-2019, Linaro Ltd.
> + */
> +
> +#include <linux/err.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/random.h>
> +#include <linux/slab.h>
> +#include <linux/soc/qcom/smem.h>
> +#include <linux/string.h>
> +#include <linux/sys_soc.h>
> +#include <linux/types.h>
> +
> +/*
> + * SoC version type with major number in the upper 16 bits and minor
> + * number in the lower 16 bits.
> + */
> +#define SOCINFO_MAJOR(ver) (((ver) >> 16) & 0xffff)
> +#define SOCINFO_MINOR(ver) ((ver) & 0xffff)
> +
> +#define SMEM_SOCINFO_BUILD_ID_LENGTH           32
> +
> +/*
> + * SMEM item ids, used to acquire handles to respective
> + * SMEM region.

There's only one socinfo SMEM item, so this sentence should be turned
singular.

> + */
> +#define SMEM_HW_SW_BUILD_ID            137
> +
> +/* Socinfo SMEM item structure */
> +struct socinfo {
> +	__le32 fmt;
> +	__le32 id;
> +	__le32 ver;
> +	char build_id[SMEM_SOCINFO_BUILD_ID_LENGTH];
> +	/* Version 2 */
> +	__le32 raw_id;
> +	__le32 raw_ver;
> +	/* Version 3 */
> +	__le32 hw_plat;
> +	/* Version 4 */
> +	__le32 plat_ver;
> +	/* Version 5 */
> +	__le32 accessory_chip;
> +	/* Version 6 */
> +	__le32 hw_plat_subtype;
> +	/* Version 7 */
> +	__le32 pmic_model;
> +	__le32 pmic_die_rev;
> +	/* Version 8 */
> +	__le32 pmic_model_1;
> +	__le32 pmic_die_rev_1;
> +	__le32 pmic_model_2;
> +	__le32 pmic_die_rev_2;
> +	/* Version 9 */
> +	__le32 foundry_id;
> +	/* Version 10 */
> +	__le32 serial_num;
> +	/* Version 11 */
> +	__le32 num_pmics;
> +	__le32 pmic_array_offset;
> +	/* Version 12 */
> +	__le32 chip_family;
> +	__le32 raw_device_family;
> +	__le32 raw_device_num;
> +};
> +
> +struct qcom_socinfo {
> +	struct soc_device *soc_dev;
> +	struct soc_device_attribute attr;
> +};
> +
> +struct soc_of_id {
> +	unsigned int id;
> +	const char *name;
> +};
> +
> +static const struct soc_of_id soc_of_id[] = {
> +	{87, "MSM8960"},
> +	{109, "APQ8064"},
> +	{122, "MSM8660A"},
> +	{123, "MSM8260A"},
> +	{124, "APQ8060A"},
> +	{126, "MSM8974"},
> +	{130, "MPQ8064"},
> +	{138, "MSM8960AB"},
> +	{139, "APQ8060AB"},
> +	{140, "MSM8260AB"},
> +	{141, "MSM8660AB"},
> +	{178, "APQ8084"},
> +	{184, "APQ8074"},
> +	{185, "MSM8274"},
> +	{186, "MSM8674"},
> +	{194, "MSM8974PRO"},
> +	{206, "MSM8916"},
> +	{208, "APQ8074-AA"},
> +	{209, "APQ8074-AB"},
> +	{210, "APQ8074PRO"},
> +	{211, "MSM8274-AA"},
> +	{212, "MSM8274-AB"},
> +	{213, "MSM8274PRO"},
> +	{214, "MSM8674-AA"},
> +	{215, "MSM8674-AB"},
> +	{216, "MSM8674PRO"},
> +	{217, "MSM8974-AA"},
> +	{218, "MSM8974-AB"},
> +	{246, "MSM8996"},
> +	{247, "APQ8016"},
> +	{248, "MSM8216"},
> +	{249, "MSM8116"},
> +	{250, "MSM8616"},
> +	{291, "APQ8096"},
> +	{305, "MSM8996SG"},
> +	{310, "MSM8996AU"},
> +	{311, "APQ8096AU"},
> +	{312, "APQ8096SG"},
> +};
> +
> +static const char *socinfo_machine(struct device *dev, unsigned int id)
> +{
> +	int idx;
> +
> +	for (idx = 0; idx < ARRAY_SIZE(soc_of_id); idx++) {
> +		if (soc_of_id[idx].id == id)
> +			return soc_of_id[idx].name;
> +	}
> +
> +	if (IS_ERR(soc_of_id[idx].name))

idx will be == ARRAY_SIZE(soc_of_id) here, so you're reading outside the
array. You can write your error unconditionally here, because you didn't
find a match - or just skip the error message completely.

If you decide to keep it, that would be to facilitate adding new entries
to the list, so include the value of "id".

> +		dev_err(dev, "Unknown soc id\n");
> +
> +	return NULL;
> +}
> +
> +static int qcom_socinfo_probe(struct platform_device *pdev)
> +{
> +	struct qcom_socinfo *qs;
> +	struct socinfo *info;
> +	size_t item_size;
> +
> +	info = qcom_smem_get(QCOM_SMEM_HOST_ANY, SMEM_HW_SW_BUILD_ID,
> +			      &item_size);
> +	if (IS_ERR(info)) {
> +		dev_err(&pdev->dev, "Couldn't find socinfo\n");
> +		return -EINVAL;
> +	}
> +
> +	qs = devm_kzalloc(&pdev->dev, sizeof(*qs), GFP_KERNEL);
> +	if (!qs)
> +		return -ENOMEM;
> +
> +	qs->attr.family = "Snapdragon";
> +	qs->attr.machine = socinfo_machine(&pdev->dev,
> +					   le32_to_cpu(info->id));
> +	qs->attr.revision = devm_kasprintf(&pdev->dev, GFP_KERNEL, "%u.%u",
> +					   SOCINFO_MAJOR(le32_to_cpu(info->ver)),
> +					   SOCINFO_MINOR(le32_to_cpu(info->ver)));
> +	if (le32_to_cpu(info->fmt) >= 10)
> +		qs->attr.serial_number = devm_kasprintf(&pdev->dev, GFP_KERNEL,
> +							"%u",
> +							le32_to_cpu(info->serial_num));
> +
> +	qs->soc_dev = soc_device_register(&qs->attr);
> +	if (IS_ERR(qs->soc_dev))
> +		return PTR_ERR(qs->soc_dev);
> +
> +	/* Feed the soc specific unique data into entropy pool */
> +	add_device_randomness(info, item_size);
> +
> +	platform_set_drvdata(pdev, qs->soc_dev);
> +
> +	return 0;
> +}

Regards,
Bjorn

[PATCH v4 4/5] soc: qcom: socinfo: Expose custom attributes

[Vaishali Thakkar]

The Qualcomm socinfo provides a number of additional attributes,
add these to the socinfo driver and expose them via debugfs
functionality.

Signed-off-by: Vaishali Thakkar <vaishali.thakkar@linaro.org>
---
Changes since v3:
	- Fix compilation error in function signatures when
	  debugfs is disabled
Changes since v2:
        - None
Changes since v1:
        - Remove unnecessary debugfs dir creation check
        - Align ifdefs to left
        - Fix function signatures for debugfs init/exit
---
 drivers/soc/qcom/socinfo.c | 198 +++++++++++++++++++++++++++++++++++++
 1 file changed, 198 insertions(+)

diff --git a/drivers/soc/qcom/socinfo.c b/drivers/soc/qcom/socinfo.c
index 02078049fac7..ccadeba69a81 100644
--- a/drivers/soc/qcom/socinfo.c
+++ b/drivers/soc/qcom/socinfo.c
@@ -4,6 +4,7 @@
  * Copyright (c) 2017-2019, Linaro Ltd.
  */
 
+#include <linux/debugfs.h>
 #include <linux/err.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
@@ -29,6 +30,28 @@
  */
 #define SMEM_HW_SW_BUILD_ID            137
 
+#ifdef CONFIG_DEBUG_FS
+/* pmic model info */
+static const char *const pmic_model[] = {
+	[0]  = "Unknown PMIC model",
+	[9]  = "PM8994",
+	[11] = "PM8916",
+	[13] = "PM8058",
+	[14] = "PM8028",
+	[15] = "PM8901",
+	[16] = "PM8027",
+	[17] = "ISL9519",
+	[18] = "PM8921",
+	[19] = "PM8018",
+	[20] = "PM8015",
+	[21] = "PM8014",
+	[22] = "PM8821",
+	[23] = "PM8038",
+	[24] = "PM8922",
+	[25] = "PM8917",
+};
+#endif /* CONFIG_DEBUG_FS */
+
 /* Socinfo SMEM item structure */
 struct socinfo {
 	__le32 fmt;
@@ -70,6 +93,10 @@ struct socinfo {
 struct qcom_socinfo {
 	struct soc_device *soc_dev;
 	struct soc_device_attribute attr;
+#ifdef CONFIG_DEBUG_FS
+	struct dentry *dbg_root;
+#endif /* CONFIG_DEBUG_FS */
+	struct socinfo *socinfo;
 };
 
 struct soc_of_id {
@@ -133,6 +160,171 @@ static const char *socinfo_machine(struct device *dev, unsigned int id)
 	return NULL;
 }
 
+#ifdef CONFIG_DEBUG_FS
+
+#define UINT_SHOW(name, attr)						\
+static int qcom_show_##name(struct seq_file *seq, void *p)		\
+{									\
+	struct socinfo *socinfo = seq->private;				\
+	seq_printf(seq, "%u\n", le32_to_cpu(socinfo->attr));		\
+	return 0;							\
+}									\
+static int qcom_open_##name(struct inode *inode, struct file *file)	\
+{									\
+	return single_open(file, qcom_show_##name, inode->i_private);	\
+}									\
+									\
+static const struct file_operations qcom_ ##name## _ops = {		\
+	.open = qcom_open_##name,					\
+	.read = seq_read,						\
+	.llseek = seq_lseek,						\
+	.release = single_release,					\
+}
+
+#define DEBUGFS_UINT_ADD(name)						\
+	debugfs_create_file(__stringify(name), 0400,			\
+			    qcom_socinfo->dbg_root,			\
+			    qcom_socinfo->socinfo, &qcom_ ##name## _ops)
+
+#define HEX_SHOW(name, attr)						\
+static int qcom_show_##name(struct seq_file *seq, void *p)		\
+{									\
+	struct socinfo *socinfo = seq->private;				\
+	seq_printf(seq, "0x%x\n", le32_to_cpu(socinfo->attr));		\
+	return 0;							\
+}									\
+static int qcom_open_##name(struct inode *inode, struct file *file)	\
+{									\
+	return single_open(file, qcom_show_##name, inode->i_private);	\
+}									\
+									\
+static const struct file_operations qcom_ ##name## _ops = {		\
+	.open = qcom_open_##name,					\
+	.read = seq_read,						\
+	.llseek = seq_lseek,						\
+	.release = single_release,					\
+}
+
+#define DEBUGFS_HEX_ADD(name)						\
+	debugfs_create_file(__stringify(name), 0400,			\
+			    qcom_socinfo->dbg_root,			\
+			    qcom_socinfo->socinfo, &qcom_ ##name## _ops)
+
+
+#define QCOM_OPEN(name, _func)						\
+static int qcom_open_##name(struct inode *inode, struct file *file)	\
+{									\
+	return single_open(file, _func, inode->i_private);		\
+}									\
+									\
+static const struct file_operations qcom_ ##name## _ops = {		\
+	.open = qcom_open_##name,					\
+	.read = seq_read,						\
+	.llseek = seq_lseek,						\
+	.release = single_release,					\
+}
+
+#define DEBUGFS_ADD(name)						\
+	debugfs_create_file(__stringify(name), 0400,			\
+			    qcom_socinfo->dbg_root,			\
+			    qcom_socinfo->socinfo, &qcom_ ##name## _ops)
+
+
+static int qcom_show_build_id(struct seq_file *seq, void *p)
+{
+	struct socinfo *socinfo = seq->private;
+
+	seq_printf(seq, "%s\n", socinfo->build_id);
+
+	return 0;
+}
+
+static int qcom_show_accessory_chip(struct seq_file *seq, void *p)
+{
+	struct socinfo *socinfo = seq->private;
+
+	seq_printf(seq, "%d\n", le32_to_cpu(socinfo->accessory_chip));
+
+	return 0;
+}
+
+static int qcom_show_platform_subtype(struct seq_file *seq, void *p)
+{
+	struct socinfo *socinfo = seq->private;
+	int subtype = le32_to_cpu(socinfo->hw_plat_subtype);
+
+	if (subtype < 0)
+		return -EINVAL;
+
+	seq_printf(seq, "%u\n", subtype);
+
+	return 0;
+}
+
+static int qcom_show_pmic_model(struct seq_file *seq, void *p)
+{
+	struct socinfo *socinfo = seq->private;
+	int model = SOCINFO_MINOR(le32_to_cpu(socinfo->pmic_model));
+
+	if (model < 0)
+		return -EINVAL;
+
+	seq_printf(seq, "%s\n", pmic_model[model]);
+
+	return 0;
+}
+
+static int qcom_show_pmic_die_revision(struct seq_file *seq, void *p)
+{
+	struct socinfo *socinfo = seq->private;
+
+	seq_printf(seq, "%u.%u\n",
+		   SOCINFO_MAJOR(le32_to_cpu(socinfo->pmic_die_rev)),
+		   SOCINFO_MINOR(le32_to_cpu(socinfo->pmic_die_rev)));
+
+	return 0;
+}
+
+UINT_SHOW(raw_version, raw_ver);
+UINT_SHOW(hardware_platform, hw_plat);
+UINT_SHOW(platform_version, plat_ver);
+UINT_SHOW(foundry_id, foundry_id);
+HEX_SHOW(chip_family, chip_family);
+HEX_SHOW(raw_device_family, raw_device_family);
+HEX_SHOW(raw_device_number, raw_device_num);
+QCOM_OPEN(build_id, qcom_show_build_id);
+QCOM_OPEN(accessory_chip, qcom_show_accessory_chip);
+QCOM_OPEN(pmic_model, qcom_show_pmic_model);
+QCOM_OPEN(platform_subtype, qcom_show_platform_subtype);
+QCOM_OPEN(pmic_die_revision, qcom_show_pmic_die_revision);
+
+static void socinfo_debugfs_init(struct qcom_socinfo *qcom_socinfo)
+{
+	qcom_socinfo->dbg_root = debugfs_create_dir("qcom_socinfo", NULL);
+
+	DEBUGFS_UINT_ADD(raw_version);
+	DEBUGFS_UINT_ADD(hardware_platform);
+	DEBUGFS_UINT_ADD(platform_version);
+	DEBUGFS_UINT_ADD(foundry_id);
+	DEBUGFS_HEX_ADD(chip_family);
+	DEBUGFS_HEX_ADD(raw_device_family);
+	DEBUGFS_HEX_ADD(raw_device_number);
+	DEBUGFS_ADD(build_id);
+	DEBUGFS_ADD(accessory_chip);
+	DEBUGFS_ADD(pmic_model);
+	DEBUGFS_ADD(platform_subtype);
+	DEBUGFS_ADD(pmic_die_revision);
+}
+
+static void socinfo_debugfs_exit(struct qcom_socinfo *qcom_socinfo)
+{
+	debugfs_remove_recursive(qcom_socinfo->dbg_root);
+}
+#else
+static void socinfo_debugfs_init(struct qcom_socinfo *qcom_socinfo) {  }
+static void socinfo_debugfs_exit(struct qcom_socinfo *qcom_socinfo) {  }
+#endif /* CONFIG_DEBUG_FS */
+
 static int qcom_socinfo_probe(struct platform_device *pdev)
 {
 	struct qcom_socinfo *qs;
@@ -165,6 +357,10 @@ static int qcom_socinfo_probe(struct platform_device *pdev)
 	if (IS_ERR(qs->soc_dev))
 		return PTR_ERR(qs->soc_dev);
 
+	qs->socinfo = info;
+
+	socinfo_debugfs_init(qs);
+
 	/* Feed the soc specific unique data into entropy pool */
 	add_device_randomness(info, item_size);
 
@@ -179,6 +375,8 @@ static int qcom_socinfo_remove(struct platform_device *pdev)
 
 	soc_device_unregister(qs->soc_dev);
 
+	socinfo_debugfs_exit(qs);
+
 	return 0;
 }
 
-- 
2.17.1

Re: [PATCH v4 4/5] soc: qcom: socinfo: Expose custom attributes

[Stephen Boyd]

Quoting Vaishali Thakkar (2019-02-24 22:50:43)
> diff --git a/drivers/soc/qcom/socinfo.c b/drivers/soc/qcom/socinfo.c
> index 02078049fac7..ccadeba69a81 100644
> --- a/drivers/soc/qcom/socinfo.c
> +++ b/drivers/soc/qcom/socinfo.c
> @@ -70,6 +93,10 @@ struct socinfo {
>  struct qcom_socinfo {
>         struct soc_device *soc_dev;
>         struct soc_device_attribute attr;
> +#ifdef CONFIG_DEBUG_FS
> +       struct dentry *dbg_root;
> +#endif /* CONFIG_DEBUG_FS */
> +       struct socinfo *socinfo;

This doesn't look necessary, instead just pass it through to the
functions that need the pointer.

>  };
>  
>  struct soc_of_id {
> @@ -133,6 +160,171 @@ static const char *socinfo_machine(struct device *dev, unsigned int id)
>         return NULL;
>  }
>  
> +#ifdef CONFIG_DEBUG_FS
> +
> +#define UINT_SHOW(name, attr)                                          \
> +static int qcom_show_##name(struct seq_file *seq, void *p)             \
> +{                                                                      \
> +       struct socinfo *socinfo = seq->private;                         \
> +       seq_printf(seq, "%u\n", le32_to_cpu(socinfo->attr));            \
> +       return 0;                                                       \
> +}                                                                      \
> +static int qcom_open_##name(struct inode *inode, struct file *file)    \
> +{                                                                      \
> +       return single_open(file, qcom_show_##name, inode->i_private);   \
> +}                                                                      \
> +                                                                       \
> +static const struct file_operations qcom_ ##name## _ops = {            \
> +       .open = qcom_open_##name,                                       \
> +       .read = seq_read,                                               \
> +       .llseek = seq_lseek,                                            \
> +       .release = single_release,                                      \
> +}

Why can't we use the debugfs_create_u32 function? It would make things
clearer if there was either a debugfs_create_le32() function or if the
socinfo structure stored in smem was unmarshalled from little endian
to the cpu native endian format during probe time and then all the rest
of the code can treat it as a native endian u32 values.

> +
> +#define DEBUGFS_UINT_ADD(name)                                         \
> +       debugfs_create_file(__stringify(name), 0400,                    \
> +                           qcom_socinfo->dbg_root,                     \
> +                           qcom_socinfo->socinfo, &qcom_ ##name## _ops)
> +
> +#define HEX_SHOW(name, attr)                                           \
> +static int qcom_show_##name(struct seq_file *seq, void *p)             \
> +{                                                                      \
> +       struct socinfo *socinfo = seq->private;                         \
> +       seq_printf(seq, "0x%x\n", le32_to_cpu(socinfo->attr));          \

Use "%#x\n" format?

> +       return 0;                                                       \
> +}                                                                      \
> +static int qcom_open_##name(struct inode *inode, struct file *file)    \
> +{                                                                      \
> +       return single_open(file, qcom_show_##name, inode->i_private);   \
> +}                                                                      \
> +                                                                       \
> +static const struct file_operations qcom_ ##name## _ops = {            \
> +       .open = qcom_open_##name,                                       \
> +       .read = seq_read,                                               \
> +       .llseek = seq_lseek,                                            \
> +       .release = single_release,                                      \
> +}
> +
> +#define DEBUGFS_HEX_ADD(name)                                          \
> +       debugfs_create_file(__stringify(name), 0400,                    \
> +                           qcom_socinfo->dbg_root,                     \
> +                           qcom_socinfo->socinfo, &qcom_ ##name## _ops)
> +
> +
> +#define QCOM_OPEN(name, _func)                                         \
> +static int qcom_open_##name(struct inode *inode, struct file *file)    \
> +{                                                                      \
> +       return single_open(file, _func, inode->i_private);              \
> +}                                                                      \
> +                                                                       \
> +static const struct file_operations qcom_ ##name## _ops = {            \
> +       .open = qcom_open_##name,                                       \
> +       .read = seq_read,                                               \
> +       .llseek = seq_lseek,                                            \
> +       .release = single_release,                                      \
> +}
> +
> +#define DEBUGFS_ADD(name)                                              \
> +       debugfs_create_file(__stringify(name), 0400,                    \
> +                           qcom_socinfo->dbg_root,                     \
> +                           qcom_socinfo->socinfo, &qcom_ ##name## _ops)
> +
> +
> +static int qcom_show_build_id(struct seq_file *seq, void *p)
> +{
> +       struct socinfo *socinfo = seq->private;
> +
> +       seq_printf(seq, "%s\n", socinfo->build_id);
> +
> +       return 0;
> +}
> +
> +static int qcom_show_accessory_chip(struct seq_file *seq, void *p)
> +{
> +       struct socinfo *socinfo = seq->private;
> +
> +       seq_printf(seq, "%d\n", le32_to_cpu(socinfo->accessory_chip));
> +
> +       return 0;
> +}
> +
> +static int qcom_show_platform_subtype(struct seq_file *seq, void *p)
> +{
> +       struct socinfo *socinfo = seq->private;
> +       int subtype = le32_to_cpu(socinfo->hw_plat_subtype);
> +
> +       if (subtype < 0)
> +               return -EINVAL;

Can it ever be negative? Why is it assigned to int type at all?

> +
> +       seq_printf(seq, "%u\n", subtype);

And then we print it as an unsigned value? Why not use %d to match the
type?

> +
> +       return 0;
> +}
> +
> +static int qcom_show_pmic_model(struct seq_file *seq, void *p)
> +{
> +       struct socinfo *socinfo = seq->private;
> +       int model = SOCINFO_MINOR(le32_to_cpu(socinfo->pmic_model));
> +
> +       if (model < 0)
> +               return -EINVAL;
> +
> +       seq_printf(seq, "%s\n", pmic_model[model]);

Is there a debugfs_create_file_string()?

> +
> +       return 0;
> +}
> +
> +static int qcom_show_pmic_die_revision(struct seq_file *seq, void *p)
> +{
> +       struct socinfo *socinfo = seq->private;
> +
> +       seq_printf(seq, "%u.%u\n",
> +                  SOCINFO_MAJOR(le32_to_cpu(socinfo->pmic_die_rev)),
> +                  SOCINFO_MINOR(le32_to_cpu(socinfo->pmic_die_rev)));
> +
> +       return 0;
> +}
> +

Re: [PATCH v4 4/5] soc: qcom: socinfo: Expose custom attributes

[Stephen Boyd]

Quoting Vaishali Thakkar (2019-02-24 22:50:43)
> diff --git a/drivers/soc/qcom/socinfo.c b/drivers/soc/qcom/socinfo.c
> index 02078049fac7..ccadeba69a81 100644
> --- a/drivers/soc/qcom/socinfo.c
> +++ b/drivers/soc/qcom/socinfo.c
> @@ -70,6 +93,10 @@ struct socinfo {
>  struct qcom_socinfo {
>         struct soc_device *soc_dev;
>         struct soc_device_attribute attr;
> +#ifdef CONFIG_DEBUG_FS
> +       struct dentry *dbg_root;
> +#endif /* CONFIG_DEBUG_FS */
> +       struct socinfo *socinfo;

This doesn't look necessary, instead just pass it through to the
functions that need the pointer.

>  };
>  
>  struct soc_of_id {
> @@ -133,6 +160,171 @@ static const char *socinfo_machine(struct device *dev, unsigned int id)
>         return NULL;
>  }
>  
> +#ifdef CONFIG_DEBUG_FS
> +
> +#define UINT_SHOW(name, attr)                                          \
> +static int qcom_show_##name(struct seq_file *seq, void *p)             \
> +{                                                                      \
> +       struct socinfo *socinfo = seq->private;                         \
> +       seq_printf(seq, "%u\n", le32_to_cpu(socinfo->attr));            \
> +       return 0;                                                       \
> +}                                                                      \
> +static int qcom_open_##name(struct inode *inode, struct file *file)    \
> +{                                                                      \
> +       return single_open(file, qcom_show_##name, inode->i_private);   \
> +}                                                                      \
> +                                                                       \
> +static const struct file_operations qcom_ ##name## _ops = {            \
> +       .open = qcom_open_##name,                                       \
> +       .read = seq_read,                                               \
> +       .llseek = seq_lseek,                                            \
> +       .release = single_release,                                      \
> +}

Why can't we use the debugfs_create_u32 function? It would make things
clearer if there was either a debugfs_create_le32() function or if the
socinfo structure stored in smem was unmarshalled from little endian
to the cpu native endian format during probe time and then all the rest
of the code can treat it as a native endian u32 values.

> +
> +#define DEBUGFS_UINT_ADD(name)                                         \
> +       debugfs_create_file(__stringify(name), 0400,                    \
> +                           qcom_socinfo->dbg_root,                     \
> +                           qcom_socinfo->socinfo, &qcom_ ##name## _ops)
> +
> +#define HEX_SHOW(name, attr)                                           \
> +static int qcom_show_##name(struct seq_file *seq, void *p)             \
> +{                                                                      \
> +       struct socinfo *socinfo = seq->private;                         \
> +       seq_printf(seq, "0x%x\n", le32_to_cpu(socinfo->attr));          \

Use "%#x\n" format?

> +       return 0;                                                       \
> +}                                                                      \
> +static int qcom_open_##name(struct inode *inode, struct file *file)    \
> +{                                                                      \
> +       return single_open(file, qcom_show_##name, inode->i_private);   \
> +}                                                                      \
> +                                                                       \
> +static const struct file_operations qcom_ ##name## _ops = {            \
> +       .open = qcom_open_##name,                                       \
> +       .read = seq_read,                                               \
> +       .llseek = seq_lseek,                                            \
> +       .release = single_release,                                      \
> +}
> +
> +#define DEBUGFS_HEX_ADD(name)                                          \
> +       debugfs_create_file(__stringify(name), 0400,                    \
> +                           qcom_socinfo->dbg_root,                     \
> +                           qcom_socinfo->socinfo, &qcom_ ##name## _ops)
> +
> +
> +#define QCOM_OPEN(name, _func)                                         \
> +static int qcom_open_##name(struct inode *inode, struct file *file)    \
> +{                                                                      \
> +       return single_open(file, _func, inode->i_private);              \
> +}                                                                      \
> +                                                                       \
> +static const struct file_operations qcom_ ##name## _ops = {            \
> +       .open = qcom_open_##name,                                       \
> +       .read = seq_read,                                               \
> +       .llseek = seq_lseek,                                            \
> +       .release = single_release,                                      \
> +}
> +
> +#define DEBUGFS_ADD(name)                                              \
> +       debugfs_create_file(__stringify(name), 0400,                    \
> +                           qcom_socinfo->dbg_root,                     \
> +                           qcom_socinfo->socinfo, &qcom_ ##name## _ops)
> +
> +
> +static int qcom_show_build_id(struct seq_file *seq, void *p)
> +{
> +       struct socinfo *socinfo = seq->private;
> +
> +       seq_printf(seq, "%s\n", socinfo->build_id);
> +
> +       return 0;
> +}
> +
> +static int qcom_show_accessory_chip(struct seq_file *seq, void *p)
> +{
> +       struct socinfo *socinfo = seq->private;
> +
> +       seq_printf(seq, "%d\n", le32_to_cpu(socinfo->accessory_chip));
> +
> +       return 0;
> +}
> +
> +static int qcom_show_platform_subtype(struct seq_file *seq, void *p)
> +{
> +       struct socinfo *socinfo = seq->private;
> +       int subtype = le32_to_cpu(socinfo->hw_plat_subtype);
> +
> +       if (subtype < 0)
> +               return -EINVAL;

Can it ever be negative? Why is it assigned to int type at all?

> +
> +       seq_printf(seq, "%u\n", subtype);

And then we print it as an unsigned value? Why not use %d to match the
type?

> +
> +       return 0;
> +}
> +
> +static int qcom_show_pmic_model(struct seq_file *seq, void *p)
> +{
> +       struct socinfo *socinfo = seq->private;
> +       int model = SOCINFO_MINOR(le32_to_cpu(socinfo->pmic_model));
> +
> +       if (model < 0)
> +               return -EINVAL;
> +
> +       seq_printf(seq, "%s\n", pmic_model[model]);

Is there a debugfs_create_file_string()?

> +
> +       return 0;
> +}
> +
> +static int qcom_show_pmic_die_revision(struct seq_file *seq, void *p)
> +{
> +       struct socinfo *socinfo = seq->private;
> +
> +       seq_printf(seq, "%u.%u\n",
> +                  SOCINFO_MAJOR(le32_to_cpu(socinfo->pmic_die_rev)),
> +                  SOCINFO_MINOR(le32_to_cpu(socinfo->pmic_die_rev)));
> +
> +       return 0;
> +}
> +

Re: [PATCH v4 4/5] soc: qcom: socinfo: Expose custom attributes

[Vaishali Thakkar]

On Fri, 1 Mar 2019 at 03:02, Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Vaishali Thakkar (2019-02-24 22:50:43)
> > diff --git a/drivers/soc/qcom/socinfo.c b/drivers/soc/qcom/socinfo.c
> > index 02078049fac7..ccadeba69a81 100644
> > --- a/drivers/soc/qcom/socinfo.c
> > +++ b/drivers/soc/qcom/socinfo.c
> > @@ -70,6 +93,10 @@ struct socinfo {
> >  struct qcom_socinfo {
> >         struct soc_device *soc_dev;
> >         struct soc_device_attribute attr;
> > +#ifdef CONFIG_DEBUG_FS
> > +       struct dentry *dbg_root;
> > +#endif /* CONFIG_DEBUG_FS */
> > +       struct socinfo *socinfo;
>
> This doesn't look necessary, instead just pass it through to the
> functions that need the pointer.
>
> >  };
> >
> >  struct soc_of_id {
> > @@ -133,6 +160,171 @@ static const char *socinfo_machine(struct device *dev, unsigned int id)
> >         return NULL;
> >  }
> >
> > +#ifdef CONFIG_DEBUG_FS
> > +
> > +#define UINT_SHOW(name, attr)                                          \
> > +static int qcom_show_##name(struct seq_file *seq, void *p)             \
> > +{                                                                      \
> > +       struct socinfo *socinfo = seq->private;                         \
> > +       seq_printf(seq, "%u\n", le32_to_cpu(socinfo->attr));            \
> > +       return 0;                                                       \
> > +}                                                                      \
> > +static int qcom_open_##name(struct inode *inode, struct file *file)    \
> > +{                                                                      \
> > +       return single_open(file, qcom_show_##name, inode->i_private);   \
> > +}                                                                      \
> > +                                                                       \
> > +static const struct file_operations qcom_ ##name## _ops = {            \
> > +       .open = qcom_open_##name,                                       \
> > +       .read = seq_read,                                               \
> > +       .llseek = seq_lseek,                                            \
> > +       .release = single_release,                                      \
> > +}
>
> Why can't we use the debugfs_create_u32 function? It would make things
> clearer if there was either a debugfs_create_le32() function or if the
> socinfo structure stored in smem was unmarshalled from little endian
> to the cpu native endian format during probe time and then all the rest
> of the code can treat it as a native endian u32 values.

Thanks for the review. I've worked on the next version with all the
changes you suggested in the patchset but I'm kind of not sure
what would be the best solution in this case.

I'm bit skeptical about introducing debugfs_create_le32 as we don't
really have any endian specific functions in the debugfs core at the
moment. And if I do that, should I also introduce debugfs_create_be32
[and to an extent debugfs_create_le{16,64}]? More importantly, would
it be useful to introduce them in core?

In the case of converting it to cpu native during probe, I'll need to
declare an extra struct with u32 being the parsed version for it to be
correct. Wouldn't it add extra overhead?

> > +
> > +#define DEBUGFS_UINT_ADD(name)                                         \
> > +       debugfs_create_file(__stringify(name), 0400,                    \
> > +                           qcom_socinfo->dbg_root,                     \
> > +                           qcom_socinfo->socinfo, &qcom_ ##name## _ops)
> > +
> > +#define HEX_SHOW(name, attr)                                           \
> > +static int qcom_show_##name(struct seq_file *seq, void *p)             \
> > +{                                                                      \
> > +       struct socinfo *socinfo = seq->private;                         \
> > +       seq_printf(seq, "0x%x\n", le32_to_cpu(socinfo->attr));          \
>
> Use "%#x\n" format?
>
> > +       return 0;                                                       \
> > +}                                                                      \
> > +static int qcom_open_##name(struct inode *inode, struct file *file)    \
> > +{                                                                      \
> > +       return single_open(file, qcom_show_##name, inode->i_private);   \
> > +}                                                                      \
> > +                                                                       \
> > +static const struct file_operations qcom_ ##name## _ops = {            \
> > +       .open = qcom_open_##name,                                       \
> > +       .read = seq_read,                                               \
> > +       .llseek = seq_lseek,                                            \
> > +       .release = single_release,                                      \
> > +}
> > +
> > +#define DEBUGFS_HEX_ADD(name)                                          \
> > +       debugfs_create_file(__stringify(name), 0400,                    \
> > +                           qcom_socinfo->dbg_root,                     \
> > +                           qcom_socinfo->socinfo, &qcom_ ##name## _ops)
> > +
> > +
> > +#define QCOM_OPEN(name, _func)                                         \
> > +static int qcom_open_##name(struct inode *inode, struct file *file)    \
> > +{                                                                      \
> > +       return single_open(file, _func, inode->i_private);              \
> > +}                                                                      \
> > +                                                                       \
> > +static const struct file_operations qcom_ ##name## _ops = {            \
> > +       .open = qcom_open_##name,                                       \
> > +       .read = seq_read,                                               \
> > +       .llseek = seq_lseek,                                            \
> > +       .release = single_release,                                      \
> > +}
> > +
> > +#define DEBUGFS_ADD(name)                                              \
> > +       debugfs_create_file(__stringify(name), 0400,                    \
> > +                           qcom_socinfo->dbg_root,                     \
> > +                           qcom_socinfo->socinfo, &qcom_ ##name## _ops)
> > +
> > +
> > +static int qcom_show_build_id(struct seq_file *seq, void *p)
> > +{
> > +       struct socinfo *socinfo = seq->private;
> > +
> > +       seq_printf(seq, "%s\n", socinfo->build_id);
> > +
> > +       return 0;
> > +}
> > +
> > +static int qcom_show_accessory_chip(struct seq_file *seq, void *p)
> > +{
> > +       struct socinfo *socinfo = seq->private;
> > +
> > +       seq_printf(seq, "%d\n", le32_to_cpu(socinfo->accessory_chip));
> > +
> > +       return 0;
> > +}
> > +
> > +static int qcom_show_platform_subtype(struct seq_file *seq, void *p)
> > +{
> > +       struct socinfo *socinfo = seq->private;
> > +       int subtype = le32_to_cpu(socinfo->hw_plat_subtype);
> > +
> > +       if (subtype < 0)
> > +               return -EINVAL;
>
> Can it ever be negative? Why is it assigned to int type at all?
>
> > +
> > +       seq_printf(seq, "%u\n", subtype);
>
> And then we print it as an unsigned value? Why not use %d to match the
> type?
>
> > +
> > +       return 0;
> > +}
> > +
> > +static int qcom_show_pmic_model(struct seq_file *seq, void *p)
> > +{
> > +       struct socinfo *socinfo = seq->private;
> > +       int model = SOCINFO_MINOR(le32_to_cpu(socinfo->pmic_model));
> > +
> > +       if (model < 0)
> > +               return -EINVAL;
> > +
> > +       seq_printf(seq, "%s\n", pmic_model[model]);
>
> Is there a debugfs_create_file_string()?
>
> > +
> > +       return 0;
> > +}
> > +
> > +static int qcom_show_pmic_die_revision(struct seq_file *seq, void *p)
> > +{
> > +       struct socinfo *socinfo = seq->private;
> > +
> > +       seq_printf(seq, "%u.%u\n",
> > +                  SOCINFO_MAJOR(le32_to_cpu(socinfo->pmic_die_rev)),
> > +                  SOCINFO_MINOR(le32_to_cpu(socinfo->pmic_die_rev)));
> > +
> > +       return 0;
> > +}
> > +

Re: [PATCH v4 4/5] soc: qcom: socinfo: Expose custom attributes

[Stephen Boyd]

Quoting Vaishali Thakkar (2019-03-14 04:25:16)
> On Fri, 1 Mar 2019 at 03:02, Stephen Boyd <swboyd@chromium.org> wrote:
> >
> > Why can't we use the debugfs_create_u32 function? It would make things
> > clearer if there was either a debugfs_create_le32() function or if the
> > socinfo structure stored in smem was unmarshalled from little endian
> > to the cpu native endian format during probe time and then all the rest
> > of the code can treat it as a native endian u32 values.
> 
> Thanks for the review. I've worked on the next version with all the
> changes you suggested in the patchset but I'm kind of not sure
> what would be the best solution in this case.

Alright, thanks.

> 
> I'm bit skeptical about introducing debugfs_create_le32 as we don't
> really have any endian specific functions in the debugfs core at the
> moment. And if I do that, should I also introduce debugfs_create_be32
> [and to an extent debugfs_create_le{16,64}]? More importantly, would
> it be useful to introduce them in core?

I suppose it's ambiguous if the endianness should be swapped to be CPU
native, or if it should just export them as little endian or big endian
to userspace. I wouldn't introduce any APIs that aren't used, because
it's just dead code. If the code is documented clearly and indicates
what it does then it should be fine. This patch has pretty much already
written the code, so it's just a matter of moving it into debugfs core
now and getting gregkh to approve.

> 
> In the case of converting it to cpu native during probe, I'll need to
> declare an extra struct with u32 being the parsed version for it to be
> correct. Wouldn't it add extra overhead?

Yes it would be some small extra overhead that could be allocated on the
kernel's heap. What's the maximum size? A hundred bytes or so?

I don't see much of a problem with this approach. It simplifies the
patch series because nothing new is introduced in debugfs core and the
endian conversion is done once in one place instead of being scattered
throughout the code. Sounds like a good improvement to me.

Re: [PATCH v4 4/5] soc: qcom: socinfo: Expose custom attributes

[Vaishali Thakkar]

On Thu, 14 Mar 2019 at 21:28, Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Vaishali Thakkar (2019-03-14 04:25:16)
> > On Fri, 1 Mar 2019 at 03:02, Stephen Boyd <swboyd@chromium.org> wrote:
> > >
> > > Why can't we use the debugfs_create_u32 function? It would make things
> > > clearer if there was either a debugfs_create_le32() function or if the
> > > socinfo structure stored in smem was unmarshalled from little endian
> > > to the cpu native endian format during probe time and then all the rest
> > > of the code can treat it as a native endian u32 values.
> >
> > Thanks for the review. I've worked on the next version with all the
> > changes you suggested in the patchset but I'm kind of not sure
> > what would be the best solution in this case.
>
> Alright, thanks.
>
> >
> > I'm bit skeptical about introducing debugfs_create_le32 as we don't
> > really have any endian specific functions in the debugfs core at the
> > moment. And if I do that, should I also introduce debugfs_create_be32
> > [and to an extent debugfs_create_le{16,64}]? More importantly, would
> > it be useful to introduce them in core?
>
> I suppose it's ambiguous if the endianness should be swapped to be CPU
> native, or if it should just export them as little endian or big endian
> to userspace. I wouldn't introduce any APIs that aren't used, because
> it's just dead code. If the code is documented clearly and indicates
> what it does then it should be fine. This patch has pretty much already
> written the code, so it's just a matter of moving it into debugfs core
> now and getting gregkh to approve.
>
> >
> > In the case of converting it to cpu native during probe, I'll need to
> > declare an extra struct with u32 being the parsed version for it to be
> > correct. Wouldn't it add extra overhead?
>
> Yes it would be some small extra overhead that could be allocated on the
> kernel's heap. What's the maximum size? A hundred bytes or so?
>
> I don't see much of a problem with this approach. It simplifies the
> patch series because nothing new is introduced in debugfs core and the
> endian conversion is done once in one place instead of being scattered
> throughout the code. Sounds like a good improvement to me.
>

Yes, it's true that this approach is better than introducing new endian
functions in debugfs core but we should also keep in mind that this is
applicable only for 4 use cases. For other usecases, we want to print
string and hex values. So, I would either need new debugfs core
functions for the same. I tried introducing debugfs_create_str for string
values but we're ending up with introducing bunch of other helpers in
the core as simple_attr_read expects integer values. Similarly, for hex
values , I can't use debugfs_create_u32 as defined attributes in the
core has unsigned int as a specifier, will need to introduce some extra
helpers over there again.

Also, in case of keeping all other cases as it is, it'll look quite
asymmetric to use debugfs u32 function in init and using local macros
for other cases. I can have DEBUGFS_UINT_ADD like wrapper
macro for debugfs_create_u32 but again not sure if doing
all of this looks better than what we have at the moment as just having
3 local macros covering our all cases without having lot of duplicated
code.

 Let me know if about your opinion on the same. Thanks.

Re: [PATCH v4 4/5] soc: qcom: socinfo: Expose custom attributes

[Stephen Boyd]

Quoting Vaishali Thakkar (2019-03-20 22:51:20)
> On Thu, 14 Mar 2019 at 21:28, Stephen Boyd <swboyd@chromium.org> wrote:
> >
> > Quoting Vaishali Thakkar (2019-03-14 04:25:16)
> > > On Fri, 1 Mar 2019 at 03:02, Stephen Boyd <swboyd@chromium.org> wrote:
> >
> > >
> > > In the case of converting it to cpu native during probe, I'll need to
> > > declare an extra struct with u32 being the parsed version for it to be
> > > correct. Wouldn't it add extra overhead?
> >
> > Yes it would be some small extra overhead that could be allocated on the
> > kernel's heap. What's the maximum size? A hundred bytes or so?
> >
> > I don't see much of a problem with this approach. It simplifies the
> > patch series because nothing new is introduced in debugfs core and the
> > endian conversion is done once in one place instead of being scattered
> > throughout the code. Sounds like a good improvement to me.
> >
> 
> Yes, it's true that this approach is better than introducing new endian
> functions in debugfs core but we should also keep in mind that this is
> applicable only for 4 use cases. For other usecases, we want to print
> string and hex values. So, I would either need new debugfs core
> functions for the same. I tried introducing debugfs_create_str for string
> values but we're ending up with introducing bunch of other helpers in
> the core as simple_attr_read expects integer values. Similarly, for hex
> values , I can't use debugfs_create_u32 as defined attributes in the
> core has unsigned int as a specifier, will need to introduce some extra
> helpers over there again.

I imagine there are other uses of printing a string and hex value in
debugfs. There's debugfs_create_x32() and debugfs_create_x64() for the
hex value printing part (if you want that format). There's also
debugfs_create_devm_seqfile() which may work to print a string from some
struct member. I'm not sure why you're using simple_attr_read(). Where
does that become important?

> 
> Also, in case of keeping all other cases as it is, it'll look quite
> asymmetric to use debugfs u32 function in init and using local macros
> for other cases. I can have DEBUGFS_UINT_ADD like wrapper
> macro for debugfs_create_u32 but again not sure if doing
> all of this looks better than what we have at the moment as just having
> 3 local macros covering our all cases without having lot of duplicated
> code.
> 
>  Let me know if about your opinion on the same. Thanks.

My opinion is still that it would be best to push things that aren't SoC
specific into the debugfs core and try to use as much from the core as
possible. There doesn't seem to be anything very SoC specific here so
I'm lost why this isn't doable.

Re: [PATCH v4 4/5] soc: qcom: socinfo: Expose custom attributes

[Vaishali Thakkar]

On Sat, 23 Mar 2019 at 05:31, Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Vaishali Thakkar (2019-03-20 22:51:20)
> > On Thu, 14 Mar 2019 at 21:28, Stephen Boyd <swboyd@chromium.org> wrote:
> > >
> > > Quoting Vaishali Thakkar (2019-03-14 04:25:16)
> > > > On Fri, 1 Mar 2019 at 03:02, Stephen Boyd <swboyd@chromium.org> wrote:
> > >
> > > >
> > > > In the case of converting it to cpu native during probe, I'll need to
> > > > declare an extra struct with u32 being the parsed version for it to be
> > > > correct. Wouldn't it add extra overhead?
> > >
> > > Yes it would be some small extra overhead that could be allocated on the
> > > kernel's heap. What's the maximum size? A hundred bytes or so?
> > >
> > > I don't see much of a problem with this approach. It simplifies the
> > > patch series because nothing new is introduced in debugfs core and the
> > > endian conversion is done once in one place instead of being scattered
> > > throughout the code. Sounds like a good improvement to me.
> > >
> >
> > Yes, it's true that this approach is better than introducing new endian
> > functions in debugfs core but we should also keep in mind that this is
> > applicable only for 4 use cases. For other usecases, we want to print
> > string and hex values. So, I would either need new debugfs core
> > functions for the same. I tried introducing debugfs_create_str for string
> > values but we're ending up with introducing bunch of other helpers in
> > the core as simple_attr_read expects integer values. Similarly, for hex
> > values , I can't use debugfs_create_u32 as defined attributes in the
> > core has unsigned int as a specifier, will need to introduce some extra
> > helpers over there again.
>
> I imagine there are other uses of printing a string and hex value in
> debugfs. There's debugfs_create_x32() and debugfs_create_x64() for the
> hex value printing part (if you want that format). There's also

Ok.

> debugfs_create_devm_seqfile() which may work to print a string from some
> struct member. I'm not sure why you're using simple_attr_read(). Where
> does that become important?

DEFINE_DEBUGFS_ATTRIBUTE has simple_attr helpers which
expects int value. So, in the case of a string it requires to implement
similar macro and separate helpers for the same.

> >
> > Also, in case of keeping all other cases as it is, it'll look quite
> > asymmetric to use debugfs u32 function in init and using local macros
> > for other cases. I can have DEBUGFS_UINT_ADD like wrapper
> > macro for debugfs_create_u32 but again not sure if doing
> > all of this looks better than what we have at the moment as just having
> > 3 local macros covering our all cases without having lot of duplicated
> > code.
> >
> >  Let me know if about your opinion on the same. Thanks.
>
> My opinion is still that it would be best to push things that aren't SoC
> specific into the debugfs core and try to use as much from the core as
> possible. There doesn't seem to be anything very SoC specific here so
> I'm lost why this isn't doable.

Yes, that's true. We don't have much of SoC specific code here.

>

Re: [PATCH v4 4/5] soc: qcom: socinfo: Expose custom attributes

[Stephen Boyd]

Quoting Vaishali Thakkar (2019-03-24 10:42:36)
> On Sat, 23 Mar 2019 at 05:31, Stephen Boyd <swboyd@chromium.org> wrote:
> 
> > debugfs_create_devm_seqfile() which may work to print a string from some
> > struct member. I'm not sure why you're using simple_attr_read(). Where
> > does that become important?
> 
> DEFINE_DEBUGFS_ATTRIBUTE has simple_attr helpers which
> expects int value. So, in the case of a string it requires to implement
> similar macro and separate helpers for the same.

Why does DEFINE_DEBUGFS_ATTRIBUTE need to be used?

Re: [PATCH v4 4/5] soc: qcom: socinfo: Expose custom attributes

[Vaishali Thakkar]

On Mon, 25 Mar 2019 at 21:31, Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Vaishali Thakkar (2019-03-24 10:42:36)
> > On Sat, 23 Mar 2019 at 05:31, Stephen Boyd <swboyd@chromium.org> wrote:
> >
> > > debugfs_create_devm_seqfile() which may work to print a string from some
> > > struct member. I'm not sure why you're using simple_attr_read(). Where
> > > does that become important?
> >
> > DEFINE_DEBUGFS_ATTRIBUTE has simple_attr helpers which
> > expects int value. So, in the case of a string it requires to implement
> > similar macro and separate helpers for the same.
>
> Why does DEFINE_DEBUGFS_ATTRIBUTE need to be used?

For defining files inside the debugfs code [with get/set functions].
Also, most of the similar functions in the debugfs core seems to be
using it.

>

Re: [PATCH v4 4/5] soc: qcom: socinfo: Expose custom attributes

[Bjorn Andersson]

On Sun 24 Feb 22:50 PST 2019, Vaishali Thakkar wrote:

> +#ifdef CONFIG_DEBUG_FS
> +/* pmic model info */

Please drop this comment and make "pmic_model" plural.

> +static const char *const pmic_model[] = {
> +	[0]  = "Unknown PMIC model",
> +	[9]  = "PM8994",
> +	[11] = "PM8916",
> +	[13] = "PM8058",
> +	[14] = "PM8028",
> +	[15] = "PM8901",
> +	[16] = "PM8027",
> +	[17] = "ISL9519",
> +	[18] = "PM8921",
> +	[19] = "PM8018",
> +	[20] = "PM8015",
> +	[21] = "PM8014",
> +	[22] = "PM8821",
> +	[23] = "PM8038",
> +	[24] = "PM8922",
> +	[25] = "PM8917",
> +};
> +#endif /* CONFIG_DEBUG_FS */
[..]
> +static int qcom_show_pmic_model(struct seq_file *seq, void *p)
> +{
> +	struct socinfo *socinfo = seq->private;
> +	int model = SOCINFO_MINOR(le32_to_cpu(socinfo->pmic_model));
> +
> +	if (model < 0)
> +		return -EINVAL;

You need to deal with the fact that model might be >=
ARRAY_SIZE(pmic_model) and that pmic_mode[model] might be NULL, in the
event that you missed entries in the list or this driver is used on
newer platforms.

> +
> +	seq_printf(seq, "%s\n", pmic_model[model]);
> +
> +	return 0;
> +}
> +
> +static int qcom_show_pmic_die_revision(struct seq_file *seq, void *p)
> +{
> +	struct socinfo *socinfo = seq->private;
> +
> +	seq_printf(seq, "%u.%u\n",
> +		   SOCINFO_MAJOR(le32_to_cpu(socinfo->pmic_die_rev)),
> +		   SOCINFO_MINOR(le32_to_cpu(socinfo->pmic_die_rev)));
> +
> +	return 0;
> +}
> +
> +UINT_SHOW(raw_version, raw_ver);
> +UINT_SHOW(hardware_platform, hw_plat);
> +UINT_SHOW(platform_version, plat_ver);
> +UINT_SHOW(foundry_id, foundry_id);
> +HEX_SHOW(chip_family, chip_family);
> +HEX_SHOW(raw_device_family, raw_device_family);
> +HEX_SHOW(raw_device_number, raw_device_num);
> +QCOM_OPEN(build_id, qcom_show_build_id);
> +QCOM_OPEN(accessory_chip, qcom_show_accessory_chip);
> +QCOM_OPEN(pmic_model, qcom_show_pmic_model);
> +QCOM_OPEN(platform_subtype, qcom_show_platform_subtype);
> +QCOM_OPEN(pmic_die_revision, qcom_show_pmic_die_revision);
> +
> +static void socinfo_debugfs_init(struct qcom_socinfo *qcom_socinfo)
> +{
> +	qcom_socinfo->dbg_root = debugfs_create_dir("qcom_socinfo", NULL);
> +
> +	DEBUGFS_UINT_ADD(raw_version);
> +	DEBUGFS_UINT_ADD(hardware_platform);

Note that the content of struct socinfo has grown over time, so based on
the comments in the struct the size of the struct would not cover
hw_plat if version < 3.

So you should make the addition of these conditional on socinfo->ver.
As each version adds more entries I suggest that you do this with a:

switch (qcom_socinfo->socinfo->ver) {
case 12:
	add v12 entries;
case 11:
	add v11 entries;
case 10:
	add v10 entries;
...
};

> +	DEBUGFS_UINT_ADD(platform_version);
> +	DEBUGFS_UINT_ADD(foundry_id);
> +	DEBUGFS_HEX_ADD(chip_family);
> +	DEBUGFS_HEX_ADD(raw_device_family);
> +	DEBUGFS_HEX_ADD(raw_device_number);
> +	DEBUGFS_ADD(build_id);
> +	DEBUGFS_ADD(accessory_chip);
> +	DEBUGFS_ADD(pmic_model);
> +	DEBUGFS_ADD(platform_subtype);
> +	DEBUGFS_ADD(pmic_die_revision);
> +}
> +

Regards,
Bjorn

[PATCH v4 5/5] soc: qcom: socinfo: Expose image information

[Vaishali Thakkar]

The socinfo driver provides information about version of the various
images loaded in the system. Expose this to user space for debugging
purpose.

Signed-off-by: Vaishali Thakkar <vaishali.thakkar@linaro.org>
---
Changes since v3:
	- Remove extra debugfs directory creation checks
Changes since v2:
        - None
Changes since v1:
        - None
---
 drivers/soc/qcom/socinfo.c | 178 +++++++++++++++++++++++++++++++++++++
 1 file changed, 178 insertions(+)

diff --git a/drivers/soc/qcom/socinfo.c b/drivers/soc/qcom/socinfo.c
index ccadeba69a81..5d77265054a3 100644
--- a/drivers/soc/qcom/socinfo.c
+++ b/drivers/soc/qcom/socinfo.c
@@ -31,6 +31,25 @@
 #define SMEM_HW_SW_BUILD_ID            137
 
 #ifdef CONFIG_DEBUG_FS
+#define SMEM_IMAGE_VERSION_BLOCKS_COUNT        32
+#define SMEM_IMAGE_VERSION_SIZE                4096
+#define SMEM_IMAGE_VERSION_NAME_SIZE           75
+#define SMEM_IMAGE_VERSION_VARIANT_SIZE        20
+#define SMEM_IMAGE_VERSION_OEM_SIZE            32
+
+/*
+ * SMEM Image table indices
+ */
+#define SMEM_IMAGE_TABLE_BOOT_INDEX     0
+#define SMEM_IMAGE_TABLE_TZ_INDEX       1
+#define SMEM_IMAGE_TABLE_RPM_INDEX      3
+#define SMEM_IMAGE_TABLE_APPS_INDEX     10
+#define SMEM_IMAGE_TABLE_MPSS_INDEX     11
+#define SMEM_IMAGE_TABLE_ADSP_INDEX     12
+#define SMEM_IMAGE_TABLE_CNSS_INDEX     13
+#define SMEM_IMAGE_TABLE_VIDEO_INDEX    14
+#define SMEM_IMAGE_VERSION_TABLE       469
+
 /* pmic model info */
 static const char *const pmic_model[] = {
 	[0]  = "Unknown PMIC model",
@@ -90,11 +109,21 @@ struct socinfo {
 	__le32 raw_device_num;
 };
 
+#ifdef CONFIG_DEBUG_FS
+struct smem_image_version {
+	char name[SMEM_IMAGE_VERSION_NAME_SIZE];
+	char variant[SMEM_IMAGE_VERSION_VARIANT_SIZE];
+	char pad;
+	char oem[SMEM_IMAGE_VERSION_OEM_SIZE];
+};
+#endif /* CONFIG_DEBUG_FS */
+
 struct qcom_socinfo {
 	struct soc_device *soc_dev;
 	struct soc_device_attribute attr;
 #ifdef CONFIG_DEBUG_FS
 	struct dentry *dbg_root;
+	struct dentry *boot, *tz, *rpm, *apps, *mpss, *adsp, *cnss, *video;
 #endif /* CONFIG_DEBUG_FS */
 	struct socinfo *socinfo;
 };
@@ -298,8 +327,97 @@ QCOM_OPEN(pmic_model, qcom_show_pmic_model);
 QCOM_OPEN(platform_subtype, qcom_show_platform_subtype);
 QCOM_OPEN(pmic_die_revision, qcom_show_pmic_die_revision);
 
+#define IMAGE_SHOW_NAME(attr)						  \
+static int show_ ##attr## _name(struct seq_file *seq, void *p)		  \
+{									  \
+	struct smem_image_version *image_version = seq->private;	  \
+	seq_puts(seq, image_version->name);				  \
+	seq_puts(seq, "\n");						  \
+	return 0;							  \
+}									  \
+static int open_ ##attr## _name(struct inode *inode, struct file *file)	  \
+{									  \
+	return single_open(file, show_ ##attr## _name, inode->i_private); \
+}									  \
+									  \
+static const struct file_operations qcom_ ##attr## _name_ops = {	  \
+	.open = open_ ##attr## _name,					  \
+	.read = seq_read,						  \
+	.llseek = seq_lseek,						  \
+	.release = single_release,					  \
+}									  \
+
+#define DEBUGFS_IMAGE_NAME(fname, attr, index)				  \
+debugfs_create_file(__stringify(fname), 0400, qcom_socinfo->attr,	  \
+		    &smem_image_version[index], &qcom_ ##attr## _name_ops)
+
+#define IMAGE_SHOW_VARIANT(attr)					     \
+static int show_ ##attr## _variant(struct seq_file *seq, void *p)	     \
+{									     \
+	struct smem_image_version *image_version = seq->private;	     \
+	seq_puts(seq, image_version->variant);				     \
+	seq_puts(seq, "\n");						     \
+	return 0;							     \
+}									     \
+static int open_ ##attr## _variant(struct inode *inode, struct file *file)   \
+{                                                                            \
+	return single_open(file, show_ ##attr## _variant, inode->i_private); \
+}									     \
+									     \
+static const struct file_operations qcom_ ##attr## _variant_ops = {	     \
+	.open = open_ ##attr## _variant,				     \
+	.read = seq_read,						     \
+	.llseek = seq_lseek,						     \
+	.release = single_release,					     \
+}
+
+#define DEBUGFS_IMAGE_VARIANT(fname, attr, index)			    \
+debugfs_create_file(__stringify(fname), 0400, qcom_socinfo->attr,	    \
+		    &smem_image_version[index], &qcom_ ##attr## _variant_ops)
+
+#define IMAGE_SHOW_OEM(attr)						 \
+static int show_ ##attr## _oem(struct seq_file *seq, void *p)		 \
+{									 \
+	struct smem_image_version *image_version = seq->private;	 \
+	seq_puts(seq, image_version->oem);				 \
+	seq_puts(seq, "\n");						 \
+	return 0;							 \
+}									 \
+static int open_ ##attr## _oem(struct inode *inode, struct file *file)	 \
+{									 \
+	return single_open(file, show_ ##attr## _oem, inode->i_private); \
+}									 \
+									 \
+static const struct file_operations qcom_ ##attr## _oem_ops = {		 \
+	.open = open_ ##attr## _oem,					 \
+	.read = seq_read,						 \
+	.llseek = seq_lseek,						 \
+	.release = single_release,					 \
+}
+
+#define DEBUGFS_IMAGE_OEM(fname, attr, index)				 \
+debugfs_create_file(__stringify(fname), 0400, qcom_socinfo->attr,	 \
+		    &smem_image_version[index], &qcom_ ##attr## _oem_ops)
+
+#define IMAGE_SHOW(name)	  \
+	IMAGE_SHOW_NAME(name);    \
+	IMAGE_SHOW_VARIANT(name); \
+	IMAGE_SHOW_OEM(name)	  \
+
+IMAGE_SHOW(boot);
+IMAGE_SHOW(tz);
+IMAGE_SHOW(rpm);
+IMAGE_SHOW(apps);
+IMAGE_SHOW(mpss);
+IMAGE_SHOW(adsp);
+IMAGE_SHOW(cnss);
+IMAGE_SHOW(video);
+
 static void socinfo_debugfs_init(struct qcom_socinfo *qcom_socinfo)
 {
+	struct smem_image_version *smem_image_version;
+	size_t size;
+
 	qcom_socinfo->dbg_root = debugfs_create_dir("qcom_socinfo", NULL);
 
 	DEBUGFS_UINT_ADD(raw_version);
@@ -314,6 +432,66 @@ static void socinfo_debugfs_init(struct qcom_socinfo *qcom_socinfo)
 	DEBUGFS_ADD(pmic_model);
 	DEBUGFS_ADD(platform_subtype);
 	DEBUGFS_ADD(pmic_die_revision);
+
+	smem_image_version = qcom_smem_get(QCOM_SMEM_HOST_ANY,
+					   SMEM_IMAGE_VERSION_TABLE,
+					   &size);
+
+	qcom_socinfo->boot = debugfs_create_dir("boot",
+						qcom_socinfo->dbg_root);
+
+	DEBUGFS_IMAGE_NAME(name, boot, SMEM_IMAGE_TABLE_BOOT_INDEX);
+	DEBUGFS_IMAGE_VARIANT(variant, boot, SMEM_IMAGE_TABLE_BOOT_INDEX);
+	DEBUGFS_IMAGE_OEM(oem, boot, SMEM_IMAGE_TABLE_BOOT_INDEX);
+
+	qcom_socinfo->tz = debugfs_create_dir("tz",
+					      qcom_socinfo->dbg_root);
+
+	DEBUGFS_IMAGE_NAME(name, tz, SMEM_IMAGE_TABLE_TZ_INDEX);
+	DEBUGFS_IMAGE_VARIANT(variant, tz, SMEM_IMAGE_TABLE_TZ_INDEX);
+	DEBUGFS_IMAGE_OEM(oem, tz, SMEM_IMAGE_TABLE_TZ_INDEX);
+
+	qcom_socinfo->rpm = debugfs_create_dir("rpm",
+					       qcom_socinfo->dbg_root);
+
+	DEBUGFS_IMAGE_NAME(name, rpm, SMEM_IMAGE_TABLE_RPM_INDEX);
+	DEBUGFS_IMAGE_VARIANT(variant, rpm, SMEM_IMAGE_TABLE_RPM_INDEX);
+	DEBUGFS_IMAGE_OEM(oem, rpm, SMEM_IMAGE_TABLE_RPM_INDEX);
+
+	qcom_socinfo->apps = debugfs_create_dir("apps",
+						qcom_socinfo->dbg_root);
+
+	DEBUGFS_IMAGE_NAME(name, apps, SMEM_IMAGE_TABLE_APPS_INDEX);
+	DEBUGFS_IMAGE_VARIANT(variant, apps, SMEM_IMAGE_TABLE_APPS_INDEX);
+	DEBUGFS_IMAGE_OEM(oem, apps, SMEM_IMAGE_TABLE_APPS_INDEX);
+
+	qcom_socinfo->mpss = debugfs_create_dir("mpss",
+						qcom_socinfo->dbg_root);
+
+	DEBUGFS_IMAGE_NAME(name, mpss, SMEM_IMAGE_TABLE_MPSS_INDEX);
+	DEBUGFS_IMAGE_VARIANT(variant, mpss, SMEM_IMAGE_TABLE_MPSS_INDEX);
+	DEBUGFS_IMAGE_OEM(oem, mpss, SMEM_IMAGE_TABLE_MPSS_INDEX);
+
+	qcom_socinfo->adsp = debugfs_create_dir("adsp",
+						qcom_socinfo->dbg_root);
+
+	DEBUGFS_IMAGE_NAME(name, adsp, SMEM_IMAGE_TABLE_ADSP_INDEX);
+	DEBUGFS_IMAGE_VARIANT(variant, adsp, SMEM_IMAGE_TABLE_ADSP_INDEX);
+	DEBUGFS_IMAGE_OEM(oem, adsp, SMEM_IMAGE_TABLE_ADSP_INDEX);
+
+	qcom_socinfo->cnss = debugfs_create_dir("cnss",
+						qcom_socinfo->dbg_root);
+
+	DEBUGFS_IMAGE_NAME(name, cnss, SMEM_IMAGE_TABLE_CNSS_INDEX);
+	DEBUGFS_IMAGE_VARIANT(variant, cnss, SMEM_IMAGE_TABLE_CNSS_INDEX);
+	DEBUGFS_IMAGE_OEM(oem, cnss, SMEM_IMAGE_TABLE_CNSS_INDEX);
+
+	qcom_socinfo->video = debugfs_create_dir("video",
+						 qcom_socinfo->dbg_root);
+
+	DEBUGFS_IMAGE_NAME(name, video, SMEM_IMAGE_TABLE_VIDEO_INDEX);
+	DEBUGFS_IMAGE_VARIANT(variant, video, SMEM_IMAGE_TABLE_VIDEO_INDEX);
+	DEBUGFS_IMAGE_OEM(oem, video, SMEM_IMAGE_TABLE_VIDEO_INDEX);
 }
 
 static void socinfo_debugfs_exit(struct qcom_socinfo *qcom_socinfo)
-- 
2.17.1

Re: [PATCH v4 5/5] soc: qcom: socinfo: Expose image information

[Stephen Boyd]

Quoting Vaishali Thakkar (2019-02-24 22:50:44)
> +                                               qcom_socinfo->dbg_root);
> +
> +       DEBUGFS_IMAGE_NAME(name, apps, SMEM_IMAGE_TABLE_APPS_INDEX);
> +       DEBUGFS_IMAGE_VARIANT(variant, apps, SMEM_IMAGE_TABLE_APPS_INDEX);
> +       DEBUGFS_IMAGE_OEM(oem, apps, SMEM_IMAGE_TABLE_APPS_INDEX);
> +
> +       qcom_socinfo->mpss = debugfs_create_dir("mpss",
> +                                               qcom_socinfo->dbg_root);
> +
> +       DEBUGFS_IMAGE_NAME(name, mpss, SMEM_IMAGE_TABLE_MPSS_INDEX);
> +       DEBUGFS_IMAGE_VARIANT(variant, mpss, SMEM_IMAGE_TABLE_MPSS_INDEX);
> +       DEBUGFS_IMAGE_OEM(oem, mpss, SMEM_IMAGE_TABLE_MPSS_INDEX);
> +
> +       qcom_socinfo->adsp = debugfs_create_dir("adsp",
> +                                               qcom_socinfo->dbg_root);
> +
> +       DEBUGFS_IMAGE_NAME(name, adsp, SMEM_IMAGE_TABLE_ADSP_INDEX);
> +       DEBUGFS_IMAGE_VARIANT(variant, adsp, SMEM_IMAGE_TABLE_ADSP_INDEX);
> +       DEBUGFS_IMAGE_OEM(oem, adsp, SMEM_IMAGE_TABLE_ADSP_INDEX);
> +
> +       qcom_socinfo->cnss = debugfs_create_dir("cnss",
> +                                               qcom_socinfo->dbg_root);
> +
> +       DEBUGFS_IMAGE_NAME(name, cnss, SMEM_IMAGE_TABLE_CNSS_INDEX);
> +       DEBUGFS_IMAGE_VARIANT(variant, cnss, SMEM_IMAGE_TABLE_CNSS_INDEX);
> +       DEBUGFS_IMAGE_OEM(oem, cnss, SMEM_IMAGE_TABLE_CNSS_INDEX);
> +
> +       qcom_socinfo->video = debugfs_create_dir("video",
> +                                                qcom_socinfo->dbg_root);
> +
> +       DEBUGFS_IMAGE_NAME(name, video, SMEM_IMAGE_TABLE_VIDEO_INDEX);
> +       DEBUGFS_IMAGE_VARIANT(variant, video, SMEM_IMAGE_TABLE_VIDEO_INDEX);
> +       DEBUGFS_IMAGE_OEM(oem, video, SMEM_IMAGE_TABLE_VIDEO_INDEX);

Couldn't these four lines be a function itself that takes the name,
dbg_root, index, etc and create a directory? Looks like a lot of copy
paste right now.

>  }
>  
>  static void socinfo_debugfs_exit(struct qcom_socinfo *qcom_socinfo)
> -- 
> 2.17.1
> 

Re: [PATCH v4 5/5] soc: qcom: socinfo: Expose image information

[Stephen Boyd]

Quoting Vaishali Thakkar (2019-02-24 22:50:44)
> +                                               qcom_socinfo->dbg_root);
> +
> +       DEBUGFS_IMAGE_NAME(name, apps, SMEM_IMAGE_TABLE_APPS_INDEX);
> +       DEBUGFS_IMAGE_VARIANT(variant, apps, SMEM_IMAGE_TABLE_APPS_INDEX);
> +       DEBUGFS_IMAGE_OEM(oem, apps, SMEM_IMAGE_TABLE_APPS_INDEX);
> +
> +       qcom_socinfo->mpss = debugfs_create_dir("mpss",
> +                                               qcom_socinfo->dbg_root);
> +
> +       DEBUGFS_IMAGE_NAME(name, mpss, SMEM_IMAGE_TABLE_MPSS_INDEX);
> +       DEBUGFS_IMAGE_VARIANT(variant, mpss, SMEM_IMAGE_TABLE_MPSS_INDEX);
> +       DEBUGFS_IMAGE_OEM(oem, mpss, SMEM_IMAGE_TABLE_MPSS_INDEX);
> +
> +       qcom_socinfo->adsp = debugfs_create_dir("adsp",
> +                                               qcom_socinfo->dbg_root);
> +
> +       DEBUGFS_IMAGE_NAME(name, adsp, SMEM_IMAGE_TABLE_ADSP_INDEX);
> +       DEBUGFS_IMAGE_VARIANT(variant, adsp, SMEM_IMAGE_TABLE_ADSP_INDEX);
> +       DEBUGFS_IMAGE_OEM(oem, adsp, SMEM_IMAGE_TABLE_ADSP_INDEX);
> +
> +       qcom_socinfo->cnss = debugfs_create_dir("cnss",
> +                                               qcom_socinfo->dbg_root);
> +
> +       DEBUGFS_IMAGE_NAME(name, cnss, SMEM_IMAGE_TABLE_CNSS_INDEX);
> +       DEBUGFS_IMAGE_VARIANT(variant, cnss, SMEM_IMAGE_TABLE_CNSS_INDEX);
> +       DEBUGFS_IMAGE_OEM(oem, cnss, SMEM_IMAGE_TABLE_CNSS_INDEX);
> +
> +       qcom_socinfo->video = debugfs_create_dir("video",
> +                                                qcom_socinfo->dbg_root);
> +
> +       DEBUGFS_IMAGE_NAME(name, video, SMEM_IMAGE_TABLE_VIDEO_INDEX);
> +       DEBUGFS_IMAGE_VARIANT(variant, video, SMEM_IMAGE_TABLE_VIDEO_INDEX);
> +       DEBUGFS_IMAGE_OEM(oem, video, SMEM_IMAGE_TABLE_VIDEO_INDEX);

Couldn't these four lines be a function itself that takes the name,
dbg_root, index, etc and create a directory? Looks like a lot of copy
paste right now.

>  }
>  
>  static void socinfo_debugfs_exit(struct qcom_socinfo *qcom_socinfo)
> -- 
> 2.17.1
> 

Re: [PATCH v4 5/5] soc: qcom: socinfo: Expose image information

[Bjorn Andersson]

On Sun 24 Feb 22:50 PST 2019, Vaishali Thakkar wrote:
> +#define IMAGE_SHOW_OEM(attr)						 \
> +static int show_ ##attr## _oem(struct seq_file *seq, void *p)		 \
> +{									 \
> +	struct smem_image_version *image_version = seq->private;	 \
> +	seq_puts(seq, image_version->oem);				 \
> +	seq_puts(seq, "\n");						 \
> +	return 0;							 \
> +}									 \
> +static int open_ ##attr## _oem(struct inode *inode, struct file *file)	 \
> +{									 \
> +	return single_open(file, show_ ##attr## _oem, inode->i_private); \
> +}									 \
> +									 \
> +static const struct file_operations qcom_ ##attr## _oem_ops = {		 \
> +	.open = open_ ##attr## _oem,					 \
> +	.read = seq_read,						 \
> +	.llseek = seq_lseek,						 \
> +	.release = single_release,					 \
> +}
> +
> +#define DEBUGFS_IMAGE_OEM(fname, attr, index)				 \
> +debugfs_create_file(__stringify(fname), 0400, qcom_socinfo->attr,	 \
> +		    &smem_image_version[index], &qcom_ ##attr## _oem_ops)
> +
> +#define IMAGE_SHOW(name)	  \
> +	IMAGE_SHOW_NAME(name);    \
> +	IMAGE_SHOW_VARIANT(name); \
> +	IMAGE_SHOW_OEM(name)	  \
> +
> +IMAGE_SHOW(boot);

Given that you pass &smem_image_version[index] as "data" it seems that
these instances will only differ in the function name. So you should be
able to have one set of these functions to handle all the images.

> +IMAGE_SHOW(tz);
> +IMAGE_SHOW(rpm);
> +IMAGE_SHOW(apps);
> +IMAGE_SHOW(mpss);
> +IMAGE_SHOW(adsp);
> +IMAGE_SHOW(cnss);
> +IMAGE_SHOW(video);
> +

Regards,
Bjorn