From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756853AbcKBSar (ORCPT ); Wed, 2 Nov 2016 14:30:47 -0400 Received: from mail-pf0-f173.google.com ([209.85.192.173]:32826 "EHLO mail-pf0-f173.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756766AbcKBSap (ORCPT ); Wed, 2 Nov 2016 14:30:45 -0400 Date: Wed, 2 Nov 2016 11:30:39 -0700 From: Bjorn Andersson To: Imran Khan Cc: andy.gross@linaro.org, lee.jones@linaro.org, David Brown , open list , "open list:ARM/QUALCOMM SUPPORT" , "open list:ARM/QUALCOMM SUPPORT" Subject: Re: [PATCH v3] soc: qcom: Add SoC info driver Message-ID: <20161102183039.GO25787@tuxbot> References: <1478081201-31998-1-git-send-email-kimran@codeaurora.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1478081201-31998-1-git-send-email-kimran@codeaurora.org> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.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 [..] > 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 > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +/* > + * 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