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

[Imran Khan]

On 11/8/2016 1:05 AM, Bjorn Andersson wrote:
> On Mon 07 Nov 06:35 PST 2016, Imran Khan wrote:
> 
> 
> 
> [..]
> 
>>>> +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.
>>
> 
> I'm slightly confused to what these various properties are supposed to
> contain, according to Documentation/ABI/testing/sysfs-devices-soc soc_id
> should contain the SoC serial number, while most implementations does
> like you and put something telling which SoC it is.
> 
> 246 is however not a useful number, as everyone reading it - be it human
> or computer - will have to carry the translation table to figure out
> what it actually says.
>

Yeah. I agree on this point. I was just following the lead of other SoCs here.
Just worried if having a string here breaks the convention. At least having
a numeric number is more in line with the documentation which expects a 
serial number. May be here by serial number the documentation means numeric
id itself. Can someone please provide some feedback? 
 
>>>> +	soc_dev_attr->family  =  "Snapdragon";
> 
> I think family should be e.g. "MSM8996" and then machine should be e.g.
> "MSM8996AU".
> 

I think here family should be Snapdragon.The following site also mentions
the SoCs as Snapdragon family of processors.

https://www.qualcomm.com/products/snapdragon/processors/comparison

Could you please confirm if it's okay?

<snip>

>>
>> 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.
>>
> 
> I presumed this was used for post mortem purposes. But how useful is
> this? Doesn't the dump file come with enough information to identify the
> platform you collected the dump on?
> 
> And when we enter download mode the SMEM items should be intact, so the
> raw information should be available already - verbatim.
> 
> 
> The kernel log is for human consumption, first and foremost, so if
> you're printing anything it must make sense to humans.
> 
>>>> +	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);
> 
> This is neither nicely formatted (info logs should not be based on
> __func__) nor useful to the average developer.
> 
> The reason why I'm objecting is that booting a CAF kernel gives me pages
> and pages of cryptic debug messages, warning and error messages. But due
> to the amount of logs no-one will actually read through the log and
> correct things - and when you're actually looking for an error the
> scroll-back buffer is too short.
> 
> So I object to this, based on the broken window theory.
> 
> [..]

Okay. I will remove this function in next patch set.

>> Thanks and Regards,
> 
> Thank you Imran, looking forward to the next version.
> 
> Regards,
> Bjorn
> 

Thanks Bjorn, Looking forward for your answers about the queries, so that
I can float the next patch set.

Best Regards,
Imran


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