Message-ID: <9ef1354b-d27c-e71f-b795-1e2f2dbcf9ab@codeaurora.org>
Date:   Mon, 7 Nov 2016 20:05:47 +0530
From:   Imran Khan <kimran@...eaurora.org>
To:     Bjorn Andersson <bjorn.andersson@...aro.org>
Cc:     andy.gross@...aro.org, lee.jones@...aro.org,
        David Brown <david.brown@...aro.org>,
        open list <linux-kernel@...r.kernel.org>,
        "open list:ARM/QUALCOMM SUPPORT" <linux-arm-msm@...r.kernel.org>,
        "open list:ARM/QUALCOMM SUPPORT" <linux-soc@...r.kernel.org>
Subject: Re: [PATCH v3] soc: qcom: Add SoC info driver

On 11/3/2016 12:00 AM, Bjorn Andersson wrote:
Thanks Bjorn for your review comments. I have taken into account
most of these in the subsequent patch set. For a few of the comments
I want to discuss further, so please see the same inline and 
let me know your feedback about the same.

> 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@...eaurora.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.
> 

Okay Done.

>> 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
> 

Okay done.

>> @@ -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.
> 

Okay done.

>>  	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.
>

Okay. Have removed this redundant comment.
 
>> + *
>> + */
>> +
>> +#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.
> 

Okay done.

<snip>

>> + * 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.
>

Okay. As major is always 0 here (for valid cases) using
only the minor to decide the socinfo format version.
 
>> +
>> +#define PLATFORM_SUBTYPE_MDM	1
> 
> Unused.
> 
>> +#define PLATFORM_SUBTYPE_INTERPOSERV3 2
> 
> Unused.
> 
>> +#define PLATFORM_SUBTYPE_SGLTE	6
> 
> Unused.
> 

Okay. Have removed these macros.

>> +
>> +#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...
>

Okay done.
 
>> +#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.
>

Okay done. Although I have kept the macros to describe
length of various members of image_version object.
 
>> +#define SMEM_IMAGE_VERSION_PARTITION_APPS 10
>> +#define SMEM_ITEM_SIZE_ALIGN 8
> 
> 4096 is already aligned, so you don't need this.
> 

Okay done.

<snip>

>> +	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.
> 

Okay. Have removed the enum. The purpose of using them in first place was to avoid 
magic numbers but you are right, here we can do without this extra indirection.

>> +};
>> +
>> +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.
> 

Okay. Have removed this as this is not used.

<snip>

>> +	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.
>

Okay done.
 
>> +};
>> +
>> +enum accessory_chip_type {
>> +	ACCESSORY_CHIP_UNKNOWN = 0,
>> +	ACCESSORY_CHIP_CHARM = 58
> 
> Unused
> 

Okay. Have removed this.

>> +};
>> +
>> +enum qrd_platform_subtype {
> 
> Please inline values into qrd_hw_platform_subtype.
> 

Okay done.

<snip>

>> +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.
> 

Okay done.

>> +};
>> +
>> +#ifdef CONFIG_SOC_BUS
> 
> Just add "select SOC_BUS" to the SMEM Kconfig 
> 

Okay done.

>> +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 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".
> 

Okay done.

>> +
>> +#ifdef CONFIG_SOC_BUS
> 
> Select SOC_BUS
> 

Okay done.

<snip>

>> +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.
> 

Okay done.

> 	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
>

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

Okay done.

>> +}
>> +
>> +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
> 

Okay done.

>> +}
>> +
>> +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
> 

Okay done

>> +}
>> +
>> +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) {
> 
> 

Okay. Have removed this redundant check.

>> +		dev_err(dev, "Wrong size for smem item\n");
>> +		return ERR_PTR(-EINVAL);
>> +	}
>> +
>> +	return ptr;
>> +}
>> +
>> +
<snip>
>> +
>> +
>> +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.
> 

Okay done. 

>> +}
>> +
>> +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.
> 

Okay done.
>> +}
>> +
>> +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());
>> +}
>> +
<snip>
>> +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.
> 

Okay done.

>> +			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.
> 

Okay done.

>> +}
>> +
>> +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());
>> +}
>> +
<snip>
>> +
>> +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...
> 

Okay done.

>> +	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.
> 

Okay done.

>> +	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.
> 

Okay done.

>> +	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
> 

Okay done.

>> +	}
>> +	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);
> 

Okay done.

>> +	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()
> 

Okay done.
>> +}
>> +
>> +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;
>> +}
>> +
<snip>

>> +
>> +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...
> 
Okay. Have removed this attribute.

>> +}
>> +
>> +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.
> 

Okay done.

>> +}
>> +
>> +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.
> 

Yes. Indeed it was a combined version of above image version attributes.
Have removed this and now we have an attribute froup for each image version
object(boot, video, apps etc.) and that group contains image version 
attributes of respective image.

>> +	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
>

Okay done.

 
>> +
>> +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);
>> +

<snip>

>> +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.
>

Actually along with vendor, I also intend to keep serial number 
and foundry_id in the generic soc_device_attribute structure 
but as this part is still under discussion, so far I have kept
these fields separate. Once these fileds gets accepted as 
part of generic soc_device_attribute, I can modify this driver.
I hope this approach is okay, if you have some alternative suggestion
please let me know.
 
>> +	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);

<snip>

>> +	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".
> 

I am not sure about this. I see other vendors also exposing soc_id as numeric value
and machine is perhaps used for a human readable name. Please let me if I 
am getting something wrong here.

>> +	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".
> 

Yes. We can read the machine string from DT also, but here we want to use
the relevant information latched into SMEM as munch as possible so I went
for reading this from socinfo. Please let me know if you see see some problem
with this approach.

>> +	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.
> 

Yes. It is indeed stating the revision of SoC. I guess the name of the
macro is misleading here. Have corrected that in subsequent patch.

> 
> Also, skip the indentation of the assignments in this section (it's not
> consistent anyways).
> 
>> +	return;
> 
> No need to "return" here.
>

Okay done.
 
>> +
>> +}
>> +
>> +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.
> 

Okay done.

>> +		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.
> 

Okay done.

>> +		return PTR_ERR(soc_dev);
>> +	}
>> +
>> +	qcom_soc_device = soc_device_to_device(soc_dev);
>> +	socinfo_populate_sysfs_files(qcom_soc_device);
>> +	return 0;
>> +}
>> +#else
<snip>
>> +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.
> 

As such this funtion does not serve any overly important purpose. It just prints
socinfo related stuff in one line in boot logs which sometimes helps in identification
of the platform while doing a debugging from dumps. 
So I intend to keep it but if you see some issues please let me know, I can try to
modify it.

>> +	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;

<snip>

>> +	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.
> 

Okay done.

>> +
>> +	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.
> 
Okay done.

>>  #endif
> 
> Regards,
> Bjorn
> 

Thanks and Regards,
Imran


-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a\nmember of the Code Aurora Forum, hosted by The Linux Foundation

Powered by blists - more mailing lists