From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S932477AbcKNOaP (ORCPT <rfc822;w@1wt.eu>);
        Mon, 14 Nov 2016 09:30:15 -0500
Received: from smtp.codeaurora.org ([198.145.29.96]:33054 "EHLO
        smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S932164AbcKNOaJ (ORCPT
        <rfc822;linux-kernel@vger.kernel.org>);
        Mon, 14 Nov 2016 09:30:09 -0500
DMARC-Filter: OpenDMARC Filter v1.3.1 smtp.codeaurora.org 3EFF0615F0
Authentication-Results: pdx-caf-mail.web.codeaurora.org; dmarc=none header.from=codeaurora.org
Authentication-Results: pdx-caf-mail.web.codeaurora.org; spf=pass smtp.mailfrom=kimran@codeaurora.org
Subject: Re: [PATCH v3] soc: qcom: Add SoC info driver
To: Bjorn Andersson <bjorn.andersson@linaro.org>
References: <1478081201-31998-1-git-send-email-kimran@codeaurora.org>
 <20161102183039.GO25787@tuxbot>
 <9ef1354b-d27c-e71f-b795-1e2f2dbcf9ab@codeaurora.org>
 <20161107193514.GC25787@tuxbot>
Cc: andy.gross@linaro.org, lee.jones@linaro.org,
        David Brown <david.brown@linaro.org>,
        open list <linux-kernel@vger.kernel.org>,
        "open list:ARM/QUALCOMM SUPPORT" <linux-arm-msm@vger.kernel.org>,
        "open list:ARM/QUALCOMM SUPPORT" <linux-soc@vger.kernel.org>
From: Imran Khan <kimran@codeaurora.org>
Message-ID: <c0c36a58-c826-84f5-99ed-bb7ee6c5c786@codeaurora.org>
Date: Mon, 14 Nov 2016 20:00:02 +0530
User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:45.0) Gecko/20100101
 Thunderbird/45.4.0
MIME-Version: 1.0
In-Reply-To: <20161107193514.GC25787@tuxbot>
Content-Type: text/plain; charset=windows-1252
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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