Date:   Wed, 15 Feb 2017 11:01:48 +0530
From:   Imran Khan <kimran@...eaurora.org>
To:     Bjorn Andersson <bjorn.andersson@...aro.org>
Cc:     andy.gross@...aro.org, david.brown@...aro.org,
        linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org,
        linux-soc@...r.kernel.org
Subject: Re: [PATCH v8 1/2] soc: qcom: Add SoC info driver

On 2/15/2017 5:54 AM, Bjorn Andersson wrote:
> On Tue 10 Jan 07:48 PST 2017, Imran Khan wrote:
> 
>> diff --git a/drivers/soc/qcom/socinfo.c b/drivers/soc/qcom/socinfo.c
>> new file mode 100644
>> index 0000000..40c180d
>> --- /dev/null
>> +++ b/drivers/soc/qcom/socinfo.c
>> @@ -0,0 +1,516 @@
>> +/*
>> + * Copyright (c) 2009-2016, The Linux Foundation. All rights reserved.
> 
> Sorry for the slow progress, please bump the year now.
>

Sure, Will update the year.
 
> [..]
>> +
>> +static const char *const pmic_model[] = {
>> +	[0]  = "Unknown PMIC model",
>> +	[13] = "PM8058",
>> +	[14] = "PM8028",
>> +	[15] = "PM8901",
>> +	[16] = "PM8027",
>> +	[17] = "ISL9519",
>> +	[18] = "PM8921",
>> +	[19] = "PM8018",
>> +	[20] = "PM8015",
>> +	[21] = "PM8014",
>> +	[22] = "PM8821",
>> +	[23] = "PM8038",
>> +	[24] = "PM8922",
>> +	[25] = "PM8917",
>> +};
> 
> I thought the conclusion was to drop the "hw_platform",
> "qrd_hw_platform_subtype" and hw_platform_subtype" lists, but to keep
> the cpu_of_id (although named soc_of_id).
> 
> The hw_platform ids are re-used by ODMs and might have different
> meaning, but the SoC name is quite useful. So please put the soc_of_id
> back in here.
> 

cpu_of_id was mapping soc-id read from SMEM into SoC name, which in turn
was being used as machine name in soc_device_attribute. But we can also
read machine name from DT. Reading the machine name from DT would make the 
solution more flexible as we don't have to make an entry in soc_of_id every
time we get a new SoC.
Also as soc_of_id uses soc-id as index, there is theoretically no limit on how
many elements it may end up having.
Because of the above reasons, I wanted to get rid off soc_of_id (or cpu_of_id)
array and use DT to get machine name as shown in the following snippet:

+	attr->family = "Snapdragon";
+	prop = fdt_get_property(initial_boot_params, 0, "model", NULL);
+	if (prop)
+		attr->machine = kasprintf(GFP_KERNEL, "%s", prop->data);

Could you please let me know if this approach looks fine to you?

> [..]
>> +
>> +static ssize_t
>> +qcom_odm_show(struct device *dev, struct device_attribute *attr, char *buf)
>> +{
>> +	return scnprintf(buf, PAGE_SIZE, "%s\n", odm_name);
>> +}
>> +DEVICE_ATTR_RO(qcom_odm);
> 
> In the case that ODMs will start using this implementation for
> communicating information to user-space I believe a name will not be
> enough - most likely there would be some product name and some
> revision/build information.
> 
> My suggestion is that you just skip the "odm" attribute in your patch,
> this gives a good opportunity for the ODMs to make a simple contribution
> of what they actually want here.
> 
> [..]

Okay, will skip the odm attribute.
>> +
>> +void qcom_socinfo_init(struct device *device)
>> +{
>> +	struct soc_device_attribute *attr;
>> +	const struct fdt_property *prop;
>> +	struct soc_device *soc_dev;
>> +	struct device *dev;
>> +	size_t item_size;
>> +	size_t size;
>> +	int i;
>> +
>> +	socinfo = qcom_smem_get(QCOM_SMEM_HOST_ANY, SMEM_HW_SW_BUILD_ID,
>> +			&item_size);
>> +	if (IS_ERR(socinfo)) {
>> +		dev_err(device, "Coudn't find socinfo\n");
>> +		return;
>> +	}
>> +
>> +	if ((SOCINFO_VERSION_MAJOR(le32_to_cpu(socinfo->v0_1.fmt)) != 0) ||
>> +	(SOCINFO_VERSION_MINOR(le32_to_cpu(socinfo->v0_1.fmt)) < 0) ||
>> +	(le32_to_cpu(socinfo->v0_1.fmt) > MAX_SOCINFO_FORMAT)) {
> 
> Indent wrapped lines by the start parenthesis and skip the extra
> parenthesis please.
> 
> I.e.
> 
> if (... ||
>     ... ||
>     ...) {
> 
>> +		dev_err(device, "Wrong socinfo format\n");
>> +		return;
>> +	}
>> +
> 
> [..]
> 
>> +
>> +	odm_name = of_get_property(device->of_node, "qcom,odm", NULL);
>> +	if (odm_name)
>> +		device_create_file(dev, &dev_attr_qcom_odm);
> 
> Skip this for now.
> 

Okay.
>> +
>> +	/* Feed the soc specific unique data into entropy pool */
>> +	add_device_randomness(socinfo, item_size);
>> +}
>> +EXPORT_SYMBOL(qcom_socinfo_init);
> 
> Please add me as To: on the next spin of your patch so I don't miss it
> on the mailing list. I do expect to be able to recommend Andy to merge
> that version.
> 
> Regards,
> Bjorn

Sure. Will send the next patch set once I have confirmation of the above
query. 

Thanks and Regards,
Imran

> --
> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 


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