Re: [PATCH v3] soc: qcom: Add SoC info driver

[Bjorn Andersson <bjorn.andersson@linaro.org>]

On Wed 02 Nov 03:06 PDT 2016, Imran Khan wrote:

> The SoC info driver provides information such as Chip ID,
> Chip family, serial number and other such details about
> Qualcomm SoCs.
> 
> Signed-off-by: Imran Khan <kimran@codeaurora.org>
[..]
> diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
> index fdd664e..b4fb8f8 100644
> --- a/drivers/soc/qcom/Makefile
> +++ b/drivers/soc/qcom/Makefile
> @@ -2,7 +2,7 @@ obj-$(CONFIG_QCOM_GSBI)	+=	qcom_gsbi.o
>  obj-$(CONFIG_QCOM_PM)	+=	spm.o
>  obj-$(CONFIG_QCOM_SMD) +=	smd.o
>  obj-$(CONFIG_QCOM_SMD_RPM)	+= smd-rpm.o
> -obj-$(CONFIG_QCOM_SMEM) +=	smem.o
> +obj-$(CONFIG_QCOM_SMEM) +=	smem.o socinfo.o

This should be something like:

obj-$(CONFIG_QCOM_SMEM) += qcom_smem.o
qcom_smem-y = smem.o
qcom_smem-y = socinfo.o

Otherwise this is treated as two different modules.

>  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 18ec52f..05ff935 100644
> --- a/drivers/soc/qcom/smem.c
> +++ b/drivers/soc/qcom/smem.c
> @@ -85,6 +85,12 @@
>  /* Max number of processors/hosts in a system */
>  #define SMEM_HOST_COUNT		9
>  
> +/*
> + * Shared memory identifiers, used to acquire handles to respective memory
> + * region.
> + */
> +#define SMEM_HW_SW_BUILD_ID		137

Move to socinfo.c

> +
>  /**
>    * struct smem_proc_comm - proc_comm communication struct (legacy)
>    * @command:	current command to be executed
> @@ -695,7 +701,8 @@ static int qcom_smem_probe(struct platform_device *pdev)
>  {
>  	struct smem_header *header;
>  	struct qcom_smem *smem;
> -	size_t array_size;
> +	void *socinfo;
> +	size_t array_size, size;
>  	int num_regions;
>  	int hwlock_id;
>  	u32 version;
> @@ -751,6 +758,15 @@ static int qcom_smem_probe(struct platform_device *pdev)
>  
>  	__smem = smem;
>  
> +	socinfo = qcom_smem_get(QCOM_SMEM_HOST_ANY, SMEM_HW_SW_BUILD_ID,
> +			&size);
> +	if (IS_ERR(socinfo)) {
> +		dev_warn(&pdev->dev,
> +			"Can't find SMEM_HW_SW_BUILD_ID; falling back on dummy values.\n");
> +		socinfo = setup_dummy_socinfo();
> +	}
> +	qcom_socinfo_init(socinfo, size);
> +

Please just replace this with an unconditional qcom_socinfo_init() call
and make it acquire the smem item and initialize itself.


And rather than setting up a dummy socinfo I think you should either not
register the soc_device or make it up of the information we know. I see
no point in exposing the value "Unknown" in various forms - better to
just hide the attributes.

>  	return 0;
>  }
>  
> diff --git a/drivers/soc/qcom/socinfo.c b/drivers/soc/qcom/socinfo.c
[..]
> +/*
> + * SOC Info Routines

This comment doesn't add any value.

> + *
> + */
> +
> +#define pr_fmt(fmt) "%s: " fmt, __func__

Now that this lives inside the smem driver you can pass the smem device
* and replace all pr_ with dev_ equivalents.

> +
> +#include <linux/export.h>
> +#include <linux/module.h>
> +#include <linux/err.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/sys_soc.h>
> +#include <linux/slab.h>
> +#include <linux/stat.h>
> +#include <linux/string.h>
> +#include <linux/types.h>
> +#include <linux/random.h>
> +#include <linux/soc/qcom/smem.h>
> +
> +/*
> + * SOC version type with major number in the upper 16 bits and minor
> + * number in the lower 16 bits.  For example:
> + *   1.0 -> 0x00010000
> + *   2.3 -> 0x00020003
> + */
> +#define SOCINFO_VERSION_MAJOR(ver) (((ver) & 0xffff0000) >> 16)
> +#define SOCINFO_VERSION_MINOR(ver) ((ver) & 0x0000ffff)

These actually represents the socinfo->version, so this is fine.

> +#define SOCINFO_VERSION(maj, min)  ((((maj) & 0xffff) << 16)|((min) & 0xffff))

But this is socinfo->format, which always has major == 0.  So there's no
need for this complexity until you actually bump the major. Just use the
(minor) "format" directly and drop this.

> +
> +#define PLATFORM_SUBTYPE_MDM	1

Unused.

> +#define PLATFORM_SUBTYPE_INTERPOSERV3 2

Unused.

> +#define PLATFORM_SUBTYPE_SGLTE	6

Unused.

> +
> +#define SMEM_SOCINFO_BUILD_ID_LENGTH 32
> +#define SMEM_IMAGE_VERSION_BLOCKS_COUNT 32
> +#define SMEM_IMAGE_VERSION_SINGLE_BLOCK_SIZE 128
> +#define SMEM_IMAGE_VERSION_SIZE 4096

You only need two of the count, block size and total size...

> +#define SMEM_IMAGE_VERSION_NAME_SIZE 75
> +#define SMEM_IMAGE_VERSION_VARIANT_SIZE 20
> +#define SMEM_IMAGE_VERSION_VARIANT_OFFSET 75
> +#define SMEM_IMAGE_VERSION_OEM_SIZE 32
> +#define SMEM_IMAGE_VERSION_OEM_OFFSET 96

Please throw these into a:

struct image_version {
	char name[75];
	char variant[20];
	char pad;
	char oem[32];
};

This allows you to describe the version SMEM item just as an array of
SMEM_IMAGE_VERSION_BLOCKS_COUNT image_version objects - i.e. the
compiler will take care of doing the math for you.

> +#define SMEM_IMAGE_VERSION_PARTITION_APPS 10
> +#define SMEM_ITEM_SIZE_ALIGN 8

4096 is already aligned, so you don't need this.

> +
> +/*
> + * Shared memory identifiers, used to acquire handles to respective memory
> + * region.
> + */
> +#define SMEM_IMAGE_VERSION_TABLE	469
> +
> +/*
> + * Qcom SoC types
> + */
> +enum qcom_cpu {
> +	MSM_CPU_UNKNOWN = 0,
> +	MSM_CPU_8960,
> +	MSM_CPU_8960AB,
> +	MSM_CPU_8064,
> +	MSM_CPU_8974,
> +	MSM_CPU_8974PRO_AA,
> +	MSM_CPU_8974PRO_AB,
> +	MSM_CPU_8974PRO_AC,
> +	MSM_CPU_8916,
> +	MSM_CPU_8084,
> +	MSM_CPU_8996
> +};
> +
> +/*
> + * Qcom SoC IDs
> + */
> +enum qcom_cpu_id {
> +	MSM_UNKNOWN_ID,
> +	MSM_8960_ID = 87,
> +	APQ_8064_ID = 109,
> +	MSM_8660A_ID = 122,
> +	MSM_8260A_ID,
> +	APQ_8060A_ID,
> +	MSM_8974_ID = 126,
> +	MPQ_8064_ID = 130,
> +	MSM_8960AB_ID = 138,
> +	APQ_8060AB_ID,
> +	MSM_8260AB_ID,
> +	MSM_8660AB_ID,
> +	APQ_8084_ID = 178,
> +	APQ_8074_ID = 184,
> +	MSM_8274_ID,
> +	MSM_8674_ID,
> +	MSM_8974PRO_ID = 194,
> +	MSM_8916_ID = 206,
> +	APQ_8074_AA_ID = 208,
> +	APQ_8074_AB_ID,
> +	APQ_8074PRO_ID,
> +	MSM_8274_AA_ID,
> +	MSM_8274_AB_ID,
> +	MSM_8274PRO_ID,
> +	MSM_8674_AA_ID,
> +	MSM_8674_AB_ID,
> +	MSM_8674PRO_ID,
> +	MSM_8974_AA_ID,
> +	MSM_8974_AB_ID,
> +	MSM_8996_ID = 246,
> +	APQ_8016_ID,
> +	MSM_8216_ID,
> +	MSM_8116_ID,
> +	MSM_8616_ID,
> +	APQ8096_ID = 291,
> +	MSM_8996SG_ID = 305,
> +	MSM_8996AU_ID = 310,
> +	APQ_8096AU_ID,
> +	APQ_8096SG_ID

These values are only ever used one time, so I think it would be more
readable if you just insert their numeric values into the cpu_of_id
directly.

Compare

	[APQ_8096SG_ID] = {MSM_CPU_8996, "APQ8096SG"},

with:

	[312] = { MSM_CPU_8996, "APQ8096SG" },

They mean the same to the compiler, but saves us all from having to
add and follow the extra indirection when maintaining this.

> +};
> +
> +struct qcom_soc_info {
> +	enum qcom_cpu generic_soc_type;
> +	char *soc_id_string;
> +};
> +
> +enum qcom_pmic_model {

None of the values in this enum are used.

> +	PMIC_MODEL_PM8058 = 13,
> +	PMIC_MODEL_PM8028,
> +	PMIC_MODEL_PM8901,
> +	PMIC_MODEL_PM8027,
> +	PMIC_MODEL_ISL_9519,
> +	PMIC_MODEL_PM8921,
> +	PMIC_MODEL_PM8018,
> +	PMIC_MODEL_PM8015,
> +	PMIC_MODEL_PM8014,
> +	PMIC_MODEL_PM8821,
> +	PMIC_MODEL_PM8038,
> +	PMIC_MODEL_PM8922,
> +	PMIC_MODEL_PM8917,
> +	PMIC_MODEL_UNKNOWN = 0xFFFFFFFF
> +};
> +
> +enum hw_platform_type {
> +	HW_PLATFORM_UNKNOWN,
> +	HW_PLATFORM_SURF,
> +	HW_PLATFORM_FFA,
> +	HW_PLATFORM_FLUID,
> +	HW_PLATFORM_SVLTE_FFA,
> +	HW_PLATFORM_SVLTE_SURF,
> +	HW_PLATFORM_MTP_MDM = 7,
> +	HW_PLATFORM_MTP,
> +	HW_PLATFORM_LIQUID,
> +	/* Dragonboard platform id is assigned as 10 in CDT */
> +	HW_PLATFORM_DRAGON,
> +	HW_PLATFORM_QRD,
> +	HW_PLATFORM_HRD	= 13,
> +	HW_PLATFORM_DTV,
> +	HW_PLATFORM_RCM	= 21,
> +	HW_PLATFORM_STP = 23,
> +	HW_PLATFORM_SBC,
> +	HW_PLATFORM_INVALID

Just inline these values into hw_platform, makes it easier to read.

> +};
> +
> +enum accessory_chip_type {
> +	ACCESSORY_CHIP_UNKNOWN = 0,
> +	ACCESSORY_CHIP_CHARM = 58

Unused

> +};
> +
> +enum qrd_platform_subtype {

Please inline values into qrd_hw_platform_subtype.

> +	PLATFORM_SUBTYPE_QRD,
> +	PLATFORM_SUBTYPE_SKUAA,
> +	PLATFORM_SUBTYPE_SKUF,
> +	PLATFORM_SUBTYPE_SKUAB,
> +	PLATFORM_SUBTYPE_SKUG = 0x5,
> +	PLATFORM_SUBTYPE_QRD_INVALID
> +};
> +
> +enum platform_subtype {
> +	PLATFORM_SUBTYPE_UNKNOWN,
> +	PLATFORM_SUBTYPE_CHARM,
> +	PLATFORM_SUBTYPE_STRANGE,
> +	PLATFORM_SUBTYPE_STRANGE_2A,
> +	PLATFORM_SUBTYPE_INVALID

Please inline these into hw_platform_subtype.

> +};
> +
> +#ifdef CONFIG_SOC_BUS

Just add "select SOC_BUS" to the SMEM Kconfig 

> +static const char *hw_platform[] = {
> +	[HW_PLATFORM_UNKNOWN] = "Unknown",
> +	[HW_PLATFORM_SURF] = "Surf",
> +	[HW_PLATFORM_FFA] = "FFA",
> +	[HW_PLATFORM_FLUID] = "Fluid",
> +	[HW_PLATFORM_SVLTE_FFA] = "SVLTE_FFA",
> +	[HW_PLATFORM_SVLTE_SURF] = "SLVTE_SURF",
> +	[HW_PLATFORM_MTP_MDM] = "MDM_MTP_NO_DISPLAY",
> +	[HW_PLATFORM_MTP] = "MTP",
> +	[HW_PLATFORM_RCM] = "RCM",
> +	[HW_PLATFORM_LIQUID] = "Liquid",
> +	[HW_PLATFORM_DRAGON] = "Dragon",
> +	[HW_PLATFORM_QRD] = "QRD",
> +	[HW_PLATFORM_HRD] = "HRD",
> +	[HW_PLATFORM_DTV] = "DTV",
> +	[HW_PLATFORM_STP] = "STP",
> +	[HW_PLATFORM_SBC] = "SBC",
> +};
> +
> +static const char *qrd_hw_platform_subtype[] = {
> +	[PLATFORM_SUBTYPE_QRD] = "QRD",
> +	[PLATFORM_SUBTYPE_SKUAA] = "SKUAA",
> +	[PLATFORM_SUBTYPE_SKUF] = "SKUF",
> +	[PLATFORM_SUBTYPE_SKUAB] = "SKUAB",
> +	[PLATFORM_SUBTYPE_SKUG] = "SKUG",
> +	[PLATFORM_SUBTYPE_QRD_INVALID] = "INVALID",
> +};
> +
> +static const char *hw_platform_subtype[] = {
> +	[PLATFORM_SUBTYPE_UNKNOWN] = "Unknown",
> +	[PLATFORM_SUBTYPE_CHARM] = "charm",
> +	[PLATFORM_SUBTYPE_STRANGE] = "strange",
> +	[PLATFORM_SUBTYPE_STRANGE_2A] = "strange_2a",
> +	[PLATFORM_SUBTYPE_INVALID] = "Invalid",
> +};
> +#endif
> +
[..]
> +
> +static struct qcom_soc_info cpu_of_id[] = {
> +
> +	[MSM_UNKNOWN_ID] = {MSM_CPU_UNKNOWN, "Unknown CPU"},
> +
> +	/* 8x60 IDs */
> +	[MSM_8960_ID] = {MSM_CPU_8960, "MSM8960"},
> +
> +	/* 8x64 IDs */
> +	[APQ_8064_ID] = {MSM_CPU_8064, "APQ8064"},
> +	[MPQ_8064_ID] = {MSM_CPU_8064, "MPQ8064"},
> +
> +	/* 8x60A IDs */
> +	[MSM_8660A_ID] = {MSM_CPU_8960, "MSM8660A"},
> +	[MSM_8260A_ID] = {MSM_CPU_8960, "MSM8260A"},
> +	[APQ_8060A_ID] = {MSM_CPU_8960, "APQ8060A"},
> +
> +	/* 8x74 IDs */
> +	[MSM_8974_ID] = {MSM_CPU_8974, "MSM8974"},
> +	[APQ_8074_ID] = {MSM_CPU_8974, "APQ8074"},
> +	[MSM_8274_ID] = {MSM_CPU_8974, "MSM8274"},
> +	[MSM_8674_ID] = {MSM_CPU_8974, "MSM8674"},
> +
> +	/* 8x74AA IDs */
> +	[APQ_8074_AA_ID] = {MSM_CPU_8974PRO_AA, "APQ8074-AA"},
> +	[MSM_8274_AA_ID] = {MSM_CPU_8974PRO_AA, "MSM8274-AA"},
> +	[MSM_8674_AA_ID] = {MSM_CPU_8974PRO_AA, "MSM8674-AA"},
> +	[MSM_8974_AA_ID] = {MSM_CPU_8974PRO_AA, "MSM8974-AA"},
> +
> +	/* 8x74AB IDs */
> +	[APQ_8074_AB_ID] = {MSM_CPU_8974PRO_AB, "APQ8074-AB"},
> +	[MSM_8274_AB_ID] = {MSM_CPU_8974PRO_AB, "MSM8274-AB"},
> +	[MSM_8674_AB_ID] = {MSM_CPU_8974PRO_AB, "MSM8674-AB"},
> +	[MSM_8974_AB_ID] = {MSM_CPU_8974PRO_AB, "MSM8974-AB"},
> +
> +	/* 8x74AC IDs */
> +	[MSM_8974PRO_ID] = {MSM_CPU_8974PRO_AC, "MSM8974PRO"},
> +	[APQ_8074PRO_ID] = {MSM_CPU_8974PRO_AC, "APQ8074PRO"},
> +	[MSM_8274PRO_ID] = {MSM_CPU_8974PRO_AC, "MSM8274PRO"},
> +	[MSM_8674PRO_ID] = {MSM_CPU_8974PRO_AC, "MSM8674PRO"},
> +
> +	/* 8x60AB IDs */
> +	[MSM_8960AB_ID] = {MSM_CPU_8960AB, "MSM8960AB"},
> +	[APQ_8060AB_ID] = {MSM_CPU_8960AB, "APQ8060AB"},
> +	[MSM_8260AB_ID] = {MSM_CPU_8960AB, "MSM8260AB"},
> +	[MSM_8660AB_ID] = {MSM_CPU_8960AB, "MSM8660AB"},
> +
> +	/* 8x84 IDs */
> +	[APQ_8084_ID] = {MSM_CPU_8084, "APQ8084"},
> +
> +	/* 8x16 IDs */
> +	[MSM_8916_ID] = {MSM_CPU_8916, "MSM8916"},
> +	[APQ_8016_ID] = {MSM_CPU_8916, "APQ8016"},
> +	[MSM_8216_ID] = {MSM_CPU_8916, "MSM8216"},
> +	[MSM_8116_ID] = {MSM_CPU_8916, "MSM8116"},
> +	[MSM_8616_ID] = {MSM_CPU_8916, "MSM8616"},
> +
> +	/* 8x96 IDs */
> +	[MSM_8996_ID] = {MSM_CPU_8996, "MSM8996"},
> +	[MSM_8996AU_ID] = {MSM_CPU_8996, "MSM8996AU"},
> +	[APQ_8096AU_ID] = {MSM_CPU_8996, "APQ8096AU"},
> +	[APQ8096_ID] = {MSM_CPU_8996, "APQ8096"},
> +	[MSM_8996SG_ID] = {MSM_CPU_8996, "MSM8996SG"},
> +	[APQ_8096SG_ID] = {MSM_CPU_8996, "APQ8096SG"},
> +
> +	/*
> +	 * Uninitialized IDs are not known to run Linux.
> +	 * MSM_CPU_UNKNOWN is set to 0 to ensure these IDs are
> +	 * considered as unknown CPU.
> +	 */
> +};
> +
> +static u32 socinfo_format;
> +
> +static struct socinfo_v0_1 dummy_socinfo = {
> +	.format = SOCINFO_VERSION(0, 1),
> +	.version = 1,
> +};

Drop the dummy, if you don't know the "build_id" then lets just not
expose a build id of "Unknown".

> +
> +#ifdef CONFIG_SOC_BUS

Select SOC_BUS

> +static int current_image;
> +static u32 socinfo_get_id(void);
> +
> +/* socinfo: sysfs functions */
> +
> +static char *socinfo_get_id_string(void)
> +{
> +	return (socinfo) ? cpu_of_id[socinfo->v0_1.id].soc_id_string : NULL;
> +}
> +
> +static u32 socinfo_get_accessory_chip(void)
> +{
> +	return socinfo ?
> +		(socinfo_format >= SOCINFO_VERSION(0, 5) ?

The numerous indirections used makes it non-obvious, but the associated
attribute will only be created if you have a socinfo and it's version >=
0.5.

So with some manual constant propagation this function does nothing more
than and as such serves no purpose being broken out from
qcom_get_accessory_chip() - just inline it.

	return socinfo->v0_5.accessory_chip;

> +			socinfo->v0_5.accessory_chip : 0)
> +		: 0;
> +}
> +
> +static u32 socinfo_get_foundry_id(void)
> +{
> +	return socinfo ?
> +		(socinfo_format >= SOCINFO_VERSION(0, 9) ?
> +			socinfo->v0_9.foundry_id : 0)

Dito

> +		: 0;
> +}
> +
> +static u32 socinfo_get_chip_family(void)
> +{
> +	return socinfo ?
> +		(socinfo_format >= SOCINFO_VERSION(0, 12) ?
> +			socinfo->v0_12.chip_family : 0)
> +		: 0;

Dito

> +}
> +
> +static u32 socinfo_get_raw_device_family(void)
> +{
> +	return socinfo ?
> +		(socinfo_format >= SOCINFO_VERSION(0, 12) ?
> +			socinfo->v0_12.raw_device_family : 0)
> +		: 0;

Dito

> +}
> +
> +static u32 socinfo_get_raw_device_number(void)
> +{
> +	return socinfo ?
> +		(socinfo_format >= SOCINFO_VERSION(0, 12) ?
> +			socinfo->v0_12.raw_device_number : 0)
> +		: 0;

Dito

> +}
> +
> +static char *socinfo_get_image_version_base_address(struct device *dev)
> +{
> +	size_t size, size_in;
> +	void *ptr;
> +
> +	ptr = qcom_smem_get(QCOM_SMEM_HOST_ANY, SMEM_IMAGE_VERSION_TABLE,
> +			&size);
> +	if (IS_ERR(ptr))
> +		return ptr;
> +
> +	size_in = ALIGN(SMEM_IMAGE_VERSION_SIZE, SMEM_ITEM_SIZE_ALIGN);

4096 is an even power of 8, you don't need to align it.

> +	if (size_in != size) {


> +		dev_err(dev, "Wrong size for smem item\n");
> +		return ERR_PTR(-EINVAL);
> +	}
> +
> +	return ptr;
> +}
> +
> +
> +static u32 socinfo_get_version(void)
> +{
> +	return (socinfo) ? socinfo->v0_1.version : 0;
> +}
> +
> +static char *socinfo_get_build_id(void)
> +{
> +	return (socinfo) ? socinfo->v0_1.build_id : NULL;
> +}
> +
> +static u32 socinfo_get_raw_version(void)
> +{
> +	return socinfo ?
> +		(socinfo_format >= SOCINFO_VERSION(0, 2) ?
> +			socinfo->v0_2.raw_version : 0)
> +		: 0;
> +}
> +
> +static u32 socinfo_get_platform_type(void)
> +{
> +	return socinfo ?
> +		(socinfo_format >= SOCINFO_VERSION(0, 3) ?
> +			socinfo->v0_3.hw_platform : 0)
> +		: 0;
> +}
> +
> +static u32 socinfo_get_platform_version(void)
> +{
> +	return socinfo ?
> +		(socinfo_format >= SOCINFO_VERSION(0, 4) ?
> +			socinfo->v0_4.platform_version : 0)
> +		: 0;
> +}
> +
> +static u32 socinfo_get_platform_subtype(void)
> +{
> +	return socinfo ?
> +		(socinfo_format >= SOCINFO_VERSION(0, 6) ?
> +			socinfo->v0_6.hw_platform_subtype : 0)
> +		: 0;
> +}
> +
> +static u32 socinfo_get_serial_number(void)
> +{
> +	return socinfo ?
> +		(socinfo_format >= SOCINFO_VERSION(0, 10) ?
> +			socinfo->v0_10.serial_number : 0)
> +		: 0;
> +}
> +
> +static enum qcom_pmic_model socinfo_get_pmic_model(void)
> +{
> +	return socinfo ?
> +		(socinfo_format >= SOCINFO_VERSION(0, 7) ?
> +			socinfo->v0_7.pmic_model : PMIC_MODEL_UNKNOWN)
> +		: PMIC_MODEL_UNKNOWN;
> +}
> +
> +static u32 socinfo_get_pmic_die_revision(void)
> +{
> +	return socinfo ?
> +		(socinfo_format >= SOCINFO_VERSION(0, 7) ?
> +			socinfo->v0_7.pmic_die_revision : 0)
> +		: 0;
> +}
> +
> +
> +static ssize_t
> +qcom_get_vendor(struct device *dev,
> +		struct device_attribute *attr,
> +		char *buf)
> +{
> +	return snprintf(buf, PAGE_SIZE, "Qualcomm\n");

Per Documentation/filesystem/sysfs.txt, never use snprintf. Use
scnprintf() if you're unsure about the size of your data and sprintf()
otherwise.

> +}
> +
> +static ssize_t
> +qcom_get_raw_version(struct device *dev,
> +		     struct device_attribute *attr,
> +		     char *buf)
> +{
> +	return snprintf(buf, PAGE_SIZE, "%u\n",
> +		socinfo_get_raw_version());

There's no need to line wrap these and that %u won't ever exceed
PAGE_SIZE, so feel free to use sprintf() if you feel the 80 char limit
is too close.

> +}
> +
> +static ssize_t
> +qcom_get_build_id(struct device *dev,
> +		   struct device_attribute *attr,
> +		   char *buf)
> +{
> +	return snprintf(buf, PAGE_SIZE, "%-.32s\n",
> +			socinfo_get_build_id());
> +}
> +
> +static ssize_t
> +qcom_get_hw_platform(struct device *dev,
> +			struct device_attribute *attr,
> +			char *buf)
> +{
> +	u32 hw_type;
> +
> +	hw_type = socinfo_get_platform_type();
> +
> +	return snprintf(buf, PAGE_SIZE, "%-.32s\n",
> +			hw_platform[hw_type]);
> +}
> +
> +static ssize_t
> +qcom_get_platform_version(struct device *dev,
> +				struct device_attribute *attr,
> +				char *buf)
> +{
> +	return snprintf(buf, PAGE_SIZE, "%u\n",
> +		socinfo_get_platform_version());
> +}
> +
> +static ssize_t
> +qcom_get_accessory_chip(struct device *dev,
> +				struct device_attribute *attr,
> +				char *buf)
> +{
> +	return snprintf(buf, PAGE_SIZE, "%u\n",
> +		socinfo_get_accessory_chip());
> +}
> +
> +static ssize_t
> +qcom_get_platform_subtype(struct device *dev,
> +			struct device_attribute *attr,
> +			char *buf)
> +{
> +	u32 hw_subtype;
> +
> +	hw_subtype = socinfo_get_platform_subtype();
> +	if (socinfo_get_platform_type() == HW_PLATFORM_QRD) {
> +		if (hw_subtype >= PLATFORM_SUBTYPE_QRD_INVALID) {
> +			pr_err("Invalid hardware platform sub type for qrd found\n");

This print doesn't help my user space application much, figure this
condition out during initialization and skip creating platform_subtype
if you don't know.

> +			hw_subtype = PLATFORM_SUBTYPE_QRD_INVALID;
> +		}
> +		return snprintf(buf, PAGE_SIZE, "%-.32s\n",
> +					qrd_hw_platform_subtype[hw_subtype]);
> +	} else {
> +		if (hw_subtype >= PLATFORM_SUBTYPE_INVALID) {
> +			pr_err("Invalid hardware platform subtype\n");
> +			hw_subtype = PLATFORM_SUBTYPE_INVALID;
> +		}
> +		return snprintf(buf, PAGE_SIZE, "%-.32s\n",
> +			hw_platform_subtype[hw_subtype]);
> +	}
> +}
> +
> +static ssize_t
> +qcom_get_platform_subtype_id(struct device *dev,
> +			struct device_attribute *attr,
> +			char *buf)
> +{
> +	u32 hw_subtype;
> +
> +	hw_subtype = socinfo_get_platform_subtype();
> +	return snprintf(buf, PAGE_SIZE, "%u\n",
> +		hw_subtype);

Drop the local variable and just put socinfo->v0_6.hw_platform_subtype
here.

> +}
> +
> +static ssize_t
> +qcom_get_foundry_id(struct device *dev,
> +			struct device_attribute *attr,
> +			char *buf)
> +{
> +	return snprintf(buf, PAGE_SIZE, "%u\n",
> +		socinfo_get_foundry_id());
> +}
> +
> +static ssize_t
> +qcom_get_serial_number(struct device *dev,
> +			struct device_attribute *attr,
> +			char *buf)
> +{
> +	return snprintf(buf, PAGE_SIZE, "%u\n",
> +		socinfo_get_serial_number());
> +}
> +
> +static ssize_t
> +qcom_get_chip_family(struct device *dev,
> +			struct device_attribute *attr,
> +			char *buf)
> +{
> +	return snprintf(buf, PAGE_SIZE, "0x%x\n",
> +		socinfo_get_chip_family());
> +}
> +
> +static ssize_t
> +qcom_get_raw_device_family(struct device *dev,
> +			struct device_attribute *attr,
> +			char *buf)
> +{
> +	return snprintf(buf, PAGE_SIZE, "0x%x\n",
> +		socinfo_get_raw_device_family());
> +}
> +
> +static ssize_t
> +qcom_get_raw_device_number(struct device *dev,
> +			struct device_attribute *attr,
> +			char *buf)
> +{
> +	return snprintf(buf, PAGE_SIZE, "0x%x\n",
> +		socinfo_get_raw_device_number());
> +}
> +
> +static ssize_t
> +qcom_get_pmic_model(struct device *dev,
> +			struct device_attribute *attr,
> +			char *buf)
> +{

I would prefer if this printed the human readable name of the PMIC
rather than a number that I can look up in the source code...

> +	return snprintf(buf, PAGE_SIZE, "%u\n",
> +		socinfo_get_pmic_model());
> +}
> +
> +static ssize_t
> +qcom_get_pmic_die_revision(struct device *dev,
> +			       struct device_attribute *attr,
> +			       char *buf)
> +{
> +	return snprintf(buf, PAGE_SIZE, "%u\n",
> +			 socinfo_get_pmic_die_revision());
> +}
> +
> +static ssize_t
> +qcom_get_image_version(struct device *dev,
> +			struct device_attribute *attr,
> +			char *buf)
> +{
> +	char *string_address;
> +
> +	string_address = socinfo_get_image_version_base_address(dev);

Make this call once during initialization and don't expose the version
attributes if you can't find the item.

> +	if (IS_ERR(string_address)) {
> +		pr_err("Failed to get image version base address");
> +		return snprintf(buf, SMEM_IMAGE_VERSION_NAME_SIZE, "Unknown");
> +	}
> +	string_address += current_image * SMEM_IMAGE_VERSION_SINGLE_BLOCK_SIZE;
> +	return snprintf(buf, SMEM_IMAGE_VERSION_NAME_SIZE, "%-.75s\n",
> +			string_address);
> +}
> +
> +static ssize_t
> +qcom_set_image_version(struct device *dev,
> +			struct device_attribute *attr,
> +			const char *buf,
> +			size_t count)
> +{
> +	char *store_address;
> +
> +	if (current_image != SMEM_IMAGE_VERSION_PARTITION_APPS)
> +		return count;

If you just make this 0644 for apps and 0444 then this won't happen, but
in cases like this you shouldn't just lie to user space about the
success.

> +	store_address = socinfo_get_image_version_base_address(dev);
> +	if (IS_ERR(store_address)) {
> +		pr_err("Failed to get image version base address");
> +		return count;

Dito

> +	}
> +	store_address += current_image * SMEM_IMAGE_VERSION_SINGLE_BLOCK_SIZE;
> +	snprintf(store_address, SMEM_IMAGE_VERSION_NAME_SIZE, "%-.75s", buf);

I don't see the point in using a string formatter here, or why to limit
the output to 75 chars by the format string - and the string is limited
to 74 chars and then a \0.

Just replace it with:

	strlcpy(store_address, buf, SMEM_IMAGE_VERSION_NAME_SIZE);

> +	return count;
> +}
> +
> +static ssize_t
> +qcom_get_image_variant(struct device *dev,
> +			struct device_attribute *attr,
> +			char *buf)
> +{
> +	char *string_address;
> +
> +	string_address = socinfo_get_image_version_base_address(dev);
> +	if (IS_ERR(string_address)) {
> +		pr_err("Failed to get image version base address");
> +		return snprintf(buf, SMEM_IMAGE_VERSION_VARIANT_SIZE,
> +		"Unknown");
> +	}
> +	string_address += current_image * SMEM_IMAGE_VERSION_SINGLE_BLOCK_SIZE;
> +	string_address += SMEM_IMAGE_VERSION_VARIANT_OFFSET;
> +	return snprintf(buf, SMEM_IMAGE_VERSION_VARIANT_SIZE, "%-.20s\n",
> +			string_address);

The output of this is the sysfs buffer, so PAGE_SIZE is the size you're
looking for. Is this string not terminated or why the limiting format
string?

And use scnprintf()

> +}
> +
> +static ssize_t
> +qcom_set_image_variant(struct device *dev,
> +			struct device_attribute *attr,
> +			const char *buf,
> +			size_t count)
> +{
> +	char *store_address;
> +
> +	if (current_image != SMEM_IMAGE_VERSION_PARTITION_APPS)
> +		return count;
> +	store_address = socinfo_get_image_version_base_address(dev);
> +	if (IS_ERR(store_address)) {
> +		pr_err("Failed to get image version base address");
> +		return count;
> +	}
> +	store_address += current_image * SMEM_IMAGE_VERSION_SINGLE_BLOCK_SIZE;
> +	store_address += SMEM_IMAGE_VERSION_VARIANT_OFFSET;
> +	snprintf(store_address, SMEM_IMAGE_VERSION_VARIANT_SIZE, "%-.20s", buf);
> +	return count;
> +}
> +
> +static ssize_t
> +qcom_get_image_crm_version(struct device *dev,
> +			struct device_attribute *attr,
> +			char *buf)
> +{
> +	char *string_address;
> +
> +	string_address = socinfo_get_image_version_base_address(dev);
> +	if (IS_ERR(string_address)) {
> +		pr_err("Failed to get image version base address");
> +		return snprintf(buf, SMEM_IMAGE_VERSION_OEM_SIZE, "Unknown");
> +	}
> +	string_address += current_image * SMEM_IMAGE_VERSION_SINGLE_BLOCK_SIZE;
> +	string_address += SMEM_IMAGE_VERSION_OEM_OFFSET;
> +	return snprintf(buf, SMEM_IMAGE_VERSION_OEM_SIZE, "%-.32s\n",
> +			string_address);
> +}
> +
> +static ssize_t
> +qcom_set_image_crm_version(struct device *dev,
> +			struct device_attribute *attr,
> +			const char *buf,
> +			size_t count)
> +{
> +	char *store_address;
> +
> +	if (current_image != SMEM_IMAGE_VERSION_PARTITION_APPS)
> +		return count;
> +	store_address = socinfo_get_image_version_base_address(dev);
> +	if (IS_ERR(store_address)) {
> +		pr_err("Failed to get image version base address");
> +		return count;
> +	}
> +	store_address += current_image * SMEM_IMAGE_VERSION_SINGLE_BLOCK_SIZE;
> +	store_address += SMEM_IMAGE_VERSION_OEM_OFFSET;
> +	snprintf(store_address, SMEM_IMAGE_VERSION_OEM_SIZE, "%-.32s", buf);
> +	return count;
> +}
> +
> +static ssize_t
> +qcom_get_image_number(struct device *dev,
> +			struct device_attribute *attr,
> +			char *buf)
> +{
> +	return snprintf(buf, PAGE_SIZE, "%d\n",
> +			current_image);

This is not a property of your soc...

> +}
> +
> +static ssize_t
> +qcom_select_image(struct device *dev, struct device_attribute *attr,
> +			const char *buf, size_t count)
> +{
> +	int ret, digit;
> +
> +	ret = kstrtoint(buf, 10, &digit);
> +	if (ret)
> +		return ret;
> +	if (digit >= 0 && digit < SMEM_IMAGE_VERSION_BLOCKS_COUNT)
> +		current_image = digit;
> +	else
> +		current_image = 0;
> +	return count;

...it's part of your version paging mechanism, which is not ok to put in
sysfs.


Create individual attribute files for each version entry, make the ones
representing apps read/write and the others read only.

> +}
> +
> +static ssize_t
> +qcom_get_images(struct device *dev,
> +		struct device_attribute *attr, char *buf)
> +{

Isn't this just a combined version of the above versions? sysfs
attributes should contain one entry each, no tables.

> +	int pos = 0;
> +	int image;
> +	char *image_address;
> +
> +	image_address = socinfo_get_image_version_base_address(dev);
> +	if (IS_ERR(image_address))
> +		return snprintf(buf, PAGE_SIZE, "Unavailable\n");
> +
> +	*buf = '\0';
> +	for (image = 0; image < SMEM_IMAGE_VERSION_BLOCKS_COUNT; image++) {
> +		if (*image_address == '\0') {
> +			image_address += SMEM_IMAGE_VERSION_SINGLE_BLOCK_SIZE;
> +			continue;
> +		}
> +
> +		pos += snprintf(buf + pos, PAGE_SIZE - pos, "%d:\n",
> +				image);
> +		pos += snprintf(buf + pos, PAGE_SIZE - pos,
> +				"\tCRM:\t\t%-.75s\n", image_address);
> +		pos += snprintf(buf + pos, PAGE_SIZE - pos,
> +				"\tVariant:\t%-.20s\n", image_address +
> +				SMEM_IMAGE_VERSION_VARIANT_OFFSET);
> +		pos += snprintf(buf + pos, PAGE_SIZE - pos,
> +				"\tVersion:\t%-.32s\n\n",
> +				image_address + SMEM_IMAGE_VERSION_OEM_OFFSET);
> +
> +		image_address += SMEM_IMAGE_VERSION_SINGLE_BLOCK_SIZE;
> +	}
> +
> +	return pos;
> +}
> +
> +static struct device_attribute qcom_soc_attr_raw_version =
> +	__ATTR(raw_version, S_IRUGO, qcom_get_raw_version,  NULL);

As checkpatch tell you, replace S_IRUGO with 0444

> +
> +static struct device_attribute qcom_soc_attr_vendor =
> +	__ATTR(vendor, S_IRUGO, qcom_get_vendor,  NULL);
> +
> +static struct device_attribute qcom_soc_attr_build_id =
> +	__ATTR(build_id, S_IRUGO, qcom_get_build_id, NULL);
> +
> +static struct device_attribute qcom_soc_attr_hw_platform =
> +	__ATTR(hw_platform, S_IRUGO, qcom_get_hw_platform, NULL);
> +
> +
> +static struct device_attribute qcom_soc_attr_platform_version =
> +	__ATTR(platform_version, S_IRUGO,
> +			qcom_get_platform_version, NULL);
> +
> +static struct device_attribute qcom_soc_attr_accessory_chip =
> +	__ATTR(accessory_chip, S_IRUGO,
> +			qcom_get_accessory_chip, NULL);
> +
> +static struct device_attribute qcom_soc_attr_platform_subtype =
> +	__ATTR(platform_subtype, S_IRUGO,
> +			qcom_get_platform_subtype, NULL);
> +
> +static struct device_attribute qcom_soc_attr_platform_subtype_id =
> +	__ATTR(platform_subtype_id, S_IRUGO,
> +			qcom_get_platform_subtype_id, NULL);
> +
> +static struct device_attribute qcom_soc_attr_foundry_id =
> +	__ATTR(foundry_id, S_IRUGO,
> +			qcom_get_foundry_id, NULL);
> +
> +static struct device_attribute qcom_soc_attr_serial_number =
> +	__ATTR(serial_number, S_IRUGO,
> +			qcom_get_serial_number, NULL);
> +
> +static struct device_attribute qcom_soc_attr_chip_family =
> +	__ATTR(chip_family, S_IRUGO,
> +			qcom_get_chip_family, NULL);
> +
> +static struct device_attribute qcom_soc_attr_raw_device_family =
> +	__ATTR(raw_device_family, S_IRUGO,
> +			qcom_get_raw_device_family, NULL);
> +
> +static struct device_attribute qcom_soc_attr_raw_device_number =
> +	__ATTR(raw_device_number, S_IRUGO,
> +			qcom_get_raw_device_number, NULL);
> +
> +static struct device_attribute qcom_soc_attr_pmic_model =
> +	__ATTR(pmic_model, S_IRUGO,
> +			qcom_get_pmic_model, NULL);
> +
> +static struct device_attribute qcom_soc_attr_pmic_die_revision =
> +	__ATTR(pmic_die_revision, S_IRUGO,
> +			qcom_get_pmic_die_revision, NULL);
> +
> +static struct device_attribute image_version =
> +	__ATTR(image_version, S_IRUGO | S_IWUSR,
> +			qcom_get_image_version, qcom_set_image_version);
> +
> +static struct device_attribute image_variant =
> +	__ATTR(image_variant, S_IRUGO | S_IWUSR,
> +			qcom_get_image_variant, qcom_set_image_variant);
> +
> +static struct device_attribute image_crm_version =
> +	__ATTR(image_crm_version, S_IRUGO | S_IWUSR,
> +			qcom_get_image_crm_version, qcom_set_image_crm_version);
> +
> +static struct device_attribute select_image =
> +	__ATTR(select_image, S_IRUGO | S_IWUSR,
> +			qcom_get_image_number, qcom_select_image);
> +
> +static struct device_attribute images =
> +	__ATTR(images, S_IRUGO, qcom_get_images, NULL);
> +
> +
> +static void socinfo_populate_sysfs_files(struct device *qcom_soc_device)
> +{
> +	device_create_file(qcom_soc_device, &qcom_soc_attr_vendor);

If you're keeping vendor it should be added to the generic
soc_device_attribute struct.

> +	device_create_file(qcom_soc_device, &image_version);
> +	device_create_file(qcom_soc_device, &image_variant);
> +	device_create_file(qcom_soc_device, &image_crm_version);
> +	device_create_file(qcom_soc_device, &select_image);
> +	device_create_file(qcom_soc_device, &images);
> +
> +	switch (socinfo_format) {
> +	case SOCINFO_VERSION(0, 12):
> +		device_create_file(qcom_soc_device,
> +					&qcom_soc_attr_chip_family);
> +		device_create_file(qcom_soc_device,
> +					&qcom_soc_attr_raw_device_family);
> +		device_create_file(qcom_soc_device,
> +					&qcom_soc_attr_raw_device_number);
> +	case SOCINFO_VERSION(0, 11):
> +	case SOCINFO_VERSION(0, 10):
> +		 device_create_file(qcom_soc_device,
> +					&qcom_soc_attr_serial_number);
> +	case SOCINFO_VERSION(0, 9):
> +		 device_create_file(qcom_soc_device,
> +					&qcom_soc_attr_foundry_id);
> +	case SOCINFO_VERSION(0, 8):
> +	case SOCINFO_VERSION(0, 7):
> +		device_create_file(qcom_soc_device,
> +					&qcom_soc_attr_pmic_model);
> +		device_create_file(qcom_soc_device,
> +					&qcom_soc_attr_pmic_die_revision);
> +	case SOCINFO_VERSION(0, 6):
> +		device_create_file(qcom_soc_device,
> +					&qcom_soc_attr_platform_subtype);
> +		device_create_file(qcom_soc_device,
> +					&qcom_soc_attr_platform_subtype_id);
> +	case SOCINFO_VERSION(0, 5):
> +		device_create_file(qcom_soc_device,
> +					&qcom_soc_attr_accessory_chip);
> +	case SOCINFO_VERSION(0, 4):
> +		device_create_file(qcom_soc_device,
> +					&qcom_soc_attr_platform_version);
> +	case SOCINFO_VERSION(0, 3):
> +		device_create_file(qcom_soc_device,
> +					&qcom_soc_attr_hw_platform);
> +	case SOCINFO_VERSION(0, 2):
> +		device_create_file(qcom_soc_device,
> +					&qcom_soc_attr_raw_version);
> +	case SOCINFO_VERSION(0, 1):
> +		device_create_file(qcom_soc_device,
> +					&qcom_soc_attr_build_id);
> +		break;
> +	default:
> +		pr_err("Unknown socinfo format: v%u.%u\n",
> +				SOCINFO_VERSION_MAJOR(socinfo_format),
> +				SOCINFO_VERSION_MINOR(socinfo_format));
> +		break;
> +	}
> +}
> +
> +static void socinfo_populate(struct soc_device_attribute *soc_dev_attr)
> +{
> +	u32 soc_version = socinfo_get_version();
> +
> +	soc_dev_attr->soc_id   = kasprintf(GFP_KERNEL, "%d", socinfo_get_id());

I believe soc_id is supposed to be a human readable name; e.g. "MSM8996"
not "246".

> +	soc_dev_attr->family  =  "Snapdragon";
> +	soc_dev_attr->machine  = socinfo_get_id_string();

I think you're supposed to read the /model string from DT and put in
"machine".

> +	soc_dev_attr->revision = kasprintf(GFP_KERNEL, "%u.%u",
> +			SOCINFO_VERSION_MAJOR(soc_version),
> +			SOCINFO_VERSION_MINOR(soc_version));

"revision" is supposed to state the revision of the SoC, not of the
socinfo data.


Also, skip the indentation of the assignments in this section (it's not
consistent anyways).

> +	return;

No need to "return" here.

> +
> +}
> +
> +static int socinfo_init_sysfs(void)
> +{
> +	struct device *qcom_soc_device;
> +	struct soc_device *soc_dev;
> +	struct soc_device_attribute *soc_dev_attr;
> +
> +	soc_dev_attr = kzalloc(sizeof(*soc_dev_attr), GFP_KERNEL);
> +	if (!soc_dev_attr) {
> +		pr_err("Soc Device alloc failed!\n");

No need to print error message on kzalloc failures.

> +		return -ENOMEM;
> +	}
> +
> +	socinfo_populate(soc_dev_attr);

Just inline socinfo_populate() here.

> +	soc_dev = soc_device_register(soc_dev_attr);
> +	if (IS_ERR(soc_dev)) {
> +		kfree(soc_dev_attr);
> +		pr_err("Soc device register failed\n");

I believe soc_device_register() will print something useful in the
various error paths, so you shouldn't need to add another print here.

> +		return PTR_ERR(soc_dev);
> +	}
> +
> +	qcom_soc_device = soc_device_to_device(soc_dev);
> +	socinfo_populate_sysfs_files(qcom_soc_device);
> +	return 0;
> +}
> +#else
> +static int socinfo_init_sysfs(void)
> +{
> +	return 0;
> +}
> +#endif
> +
> +static u32 socinfo_get_id(void)
> +{
> +	return (socinfo) ? socinfo->v0_1.id : 0;
> +}
> +
> +void *setup_dummy_socinfo(void)
> +{
> +	dummy_socinfo.id = MSM_UNKNOWN_ID;
> +	strlcpy(dummy_socinfo.build_id, "Unknown build",
> +		sizeof(dummy_socinfo.build_id));
> +	return (void *) &dummy_socinfo;
> +}
> +
> +static void socinfo_print(void)
> +{

Does this function serve a purpose?

We already have an overwhelming amount of information (noise) being
thrown at us during boot.

> +	u32 f_maj = SOCINFO_VERSION_MAJOR(socinfo_format);
> +	u32 f_min = SOCINFO_VERSION_MINOR(socinfo_format);
> +	u32 v_maj = SOCINFO_VERSION_MAJOR(socinfo->v0_1.version);
> +	u32 v_min = SOCINFO_VERSION_MINOR(socinfo->v0_1.version);
> +
> +	switch (socinfo_format) {
> +	case SOCINFO_VERSION(0, 1):
> +		pr_info("v%u.%u, id=%u, ver=%u.%u\n",
> +			f_maj, f_min, socinfo->v0_1.id, v_maj, v_min);
> +		break;
> +	case SOCINFO_VERSION(0, 2):
> +		pr_info("v%u.%u, id=%u, ver=%u.%u, raw_ver=%u\n",
> +			f_maj, f_min, socinfo->v0_1.id, v_maj, v_min,
> +			socinfo->v0_2.raw_version);
> +		break;
> +	case SOCINFO_VERSION(0, 3):
> +		pr_info("v%u.%u, id=%u, ver=%u.%u, raw_ver=%u, hw_plat=%u\n",
> +			f_maj, f_min, socinfo->v0_1.id,
> +			v_maj, v_min, socinfo->v0_2.raw_version,
> +			socinfo->v0_3.hw_platform);
> +		break;
> +	case SOCINFO_VERSION(0, 4):
> +		pr_info("v%u.%u, id=%u, ver=%u.%u, raw_ver=%u, hw_plat=%u, hw_plat_ver=%u\n",
> +			f_maj, f_min, socinfo->v0_1.id, v_maj, v_min,
> +			socinfo->v0_2.raw_version,
> +			socinfo->v0_3.hw_platform,
> +			socinfo->v0_4.platform_version);
> +		break;
> +	case SOCINFO_VERSION(0, 5):
> +		pr_info("v%u.%u, id=%u, ver=%u.%u, raw_ver=%u, hw_plat=%u, hw_plat_ver=%u accessory_chip=%u\n",
> +			f_maj, f_min, socinfo->v0_1.id, v_maj, v_min,
> +			socinfo->v0_2.raw_version,
> +			socinfo->v0_3.hw_platform,
> +			socinfo->v0_4.platform_version,
> +			socinfo->v0_5.accessory_chip);
> +		break;
> +	case SOCINFO_VERSION(0, 6):
> +		pr_info("v%u.%u, id=%u, ver=%u.%u, raw_ver=%u, hw_plat=%u, hw_plat_ver=%u accessory_chip=%u, hw_plat_subtype=%u\n",
> +			f_maj, f_min, socinfo->v0_1.id,
> +			v_maj, v_min, socinfo->v0_2.raw_version,
> +			socinfo->v0_3.hw_platform,
> +			socinfo->v0_4.platform_version,
> +			socinfo->v0_5.accessory_chip,
> +			socinfo->v0_6.hw_platform_subtype);
> +		break;
> +	case SOCINFO_VERSION(0, 7):
> +	case SOCINFO_VERSION(0, 8):
> +		pr_info("v%u.%u, id=%u, ver=%u.%u, raw_ver=%u, hw_plat=%u, hw_plat_ver=%u accessory_chip=%u, hw_plat_subtype=%u, pmic_model=%u, pmic_die_revision=%u\n",
> +			f_maj, f_min, socinfo->v0_1.id, v_maj, v_min,
> +			socinfo->v0_2.raw_version,
> +			socinfo->v0_3.hw_platform,
> +			socinfo->v0_4.platform_version,
> +			socinfo->v0_5.accessory_chip,
> +			socinfo->v0_6.hw_platform_subtype,
> +			socinfo->v0_7.pmic_model,
> +			socinfo->v0_7.pmic_die_revision);
> +		break;
> +	case SOCINFO_VERSION(0, 9):
> +		pr_info("v%u.%u, id=%u, ver=%u.%u, raw_ver=%u, hw_plat=%u, hw_plat_ver=%u accessory_chip=%u, hw_plat_subtype=%u, pmic_model=%u, pmic_die_revision=%u foundry_id=%u\n",
> +			f_maj, f_min, socinfo->v0_1.id, v_maj, v_min,
> +			socinfo->v0_2.raw_version,
> +			socinfo->v0_3.hw_platform,
> +			socinfo->v0_4.platform_version,
> +			socinfo->v0_5.accessory_chip,
> +			socinfo->v0_6.hw_platform_subtype,
> +			socinfo->v0_7.pmic_model,
> +			socinfo->v0_7.pmic_die_revision,
> +			socinfo->v0_9.foundry_id);
> +		break;
> +	case SOCINFO_VERSION(0, 10):
> +		pr_info("v%u.%u, id=%u, ver=%u.%u, raw_ver=%u, hw_plat=%u, hw_plat_ver=%u accessory_chip=%u, hw_plat_subtype=%u, pmic_model=%u, pmic_die_revision=%u, foundry_id=%u, serial_number=%u\n",
> +			f_maj, f_min, socinfo->v0_1.id, v_maj, v_min,
> +			socinfo->v0_2.raw_version,
> +			socinfo->v0_3.hw_platform,
> +			socinfo->v0_4.platform_version,
> +			socinfo->v0_5.accessory_chip,
> +			socinfo->v0_6.hw_platform_subtype,
> +			socinfo->v0_7.pmic_model,
> +			socinfo->v0_7.pmic_die_revision,
> +			socinfo->v0_9.foundry_id,
> +			socinfo->v0_10.serial_number);
> +		break;
> +	case SOCINFO_VERSION(0, 11):
> +		pr_info("v%u.%u, id=%u, ver=%u.%u, raw_ver=%u, hw_plat=%u, hw_plat_ver=%u, accessory_chip=%u, hw_plat_subtype=%u, pmic_model=%u, pmic_die_revision=%u, foundry_id=%u, serial_number=%u num_pmics=%u\n",
> +			f_maj, f_min, socinfo->v0_1.id, v_maj, v_min,
> +			socinfo->v0_2.raw_version,
> +			socinfo->v0_3.hw_platform,
> +			socinfo->v0_4.platform_version,
> +			socinfo->v0_5.accessory_chip,
> +			socinfo->v0_6.hw_platform_subtype,
> +			socinfo->v0_7.pmic_model,
> +			socinfo->v0_7.pmic_die_revision,
> +			socinfo->v0_9.foundry_id,
> +			socinfo->v0_10.serial_number,
> +			socinfo->v0_11.num_pmics);
> +		break;
> +	case SOCINFO_VERSION(0, 12):
> +		pr_info("v%u.%u, id=%u, ver=%u.%u, raw_ver=%u, hw_plat=%u, hw_plat_ver=%u accessory_chip=%u, hw_plat_subtype=%u, pmic_model=%u, pmic_die_revision=%u, foundry_id=%u, serial_number=%u, num_pmics=%u, chip_family=0x%x, raw_device_family=0x%x, raw_device_number=0x%x\n",
> +			f_maj, f_min, socinfo->v0_1.id, v_maj, v_min,
> +			socinfo->v0_2.raw_version,
> +			socinfo->v0_3.hw_platform,
> +			socinfo->v0_4.platform_version,
> +			socinfo->v0_5.accessory_chip,
> +			socinfo->v0_6.hw_platform_subtype,
> +			socinfo->v0_7.pmic_model,
> +			socinfo->v0_7.pmic_die_revision,
> +			socinfo->v0_9.foundry_id,
> +			socinfo->v0_10.serial_number,
> +			socinfo->v0_11.num_pmics,
> +			socinfo->v0_12.chip_family,
> +			socinfo->v0_12.raw_device_family,
> +			socinfo->v0_12.raw_device_number);
> +		break;
> +
> +	default:
> +		pr_err("Unknown format found: v%u.%u\n", f_maj, f_min);
> +		break;
> +	}
> +}
> +
> +static void socinfo_select_format(void)
> +{
> +	u32 f_maj = SOCINFO_VERSION_MAJOR(socinfo->v0_1.format);
> +	u32 f_min = SOCINFO_VERSION_MINOR(socinfo->v0_1.format);
> +
> +	if (f_maj != 0) {
> +		pr_err("Unsupported format v%u.%u. Falling back to dummy values.\n",
> +			f_maj, f_min);
> +		socinfo = setup_dummy_socinfo();
> +	}
> +
> +	if (socinfo->v0_1.format > MAX_SOCINFO_FORMAT) {
> +		pr_warn("Unsupported format v%u.%u. Falling back to v%u.%u.\n",
> +			f_maj, f_min, SOCINFO_VERSION_MAJOR(MAX_SOCINFO_FORMAT),
> +			SOCINFO_VERSION_MINOR(MAX_SOCINFO_FORMAT));
> +		socinfo_format = MAX_SOCINFO_FORMAT;
> +	} else {
> +		socinfo_format = socinfo->v0_1.format;
> +	}
> +}
> +
> +int qcom_socinfo_init(void *info, size_t size)
> +{
> +	socinfo = info;
> +
> +	socinfo_select_format();
> +
> +	WARN(!socinfo_get_id(), "Unknown SOC ID!\n");

No need to WARN() about this.

I bet this is not really ever happening outside early development at
Qualcomm, so I would go with a print and just skip the entire soc_device
initialization when this happens.

> +
> +	WARN(socinfo_get_id() >= ARRAY_SIZE(cpu_of_id),
> +		"New IDs added! ID => CPU mapping needs an update.\n");
> +
> +	socinfo_print();
> +
> +	socinfo_init_sysfs();
> +
> +	/* Feed the soc specific unique data into entropy pool */
> +	add_device_randomness(info, size);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(qcom_socinfo_init);
> +
> diff --git a/include/linux/soc/qcom/smem.h b/include/linux/soc/qcom/smem.h
> index 785e196..62e9476 100644
> --- a/include/linux/soc/qcom/smem.h
> +++ b/include/linux/soc/qcom/smem.h
> @@ -7,5 +7,6 @@
>  void *qcom_smem_get(unsigned host, unsigned item, size_t *size);
>  
>  int qcom_smem_get_free_space(unsigned host);
> -
> +int qcom_socinfo_init(void *info, size_t size);

soc/qcom/smem.h is the public API for smem, this function is not part of
that API so I don't like having it defined here.

Normally we would create an internal include file in drivers/soc/qcom,
but as it's only a single function I would suggest just declaring it
external inside smem.c

> +void *setup_dummy_socinfo(void);

And drop this.

>  #endif

Regards,
Bjorn