Re: [PATCH v4 4/5] soc: qcom: socinfo: Expose custom attributes

From: Bjorn Andersson <bjorn.andersson@linaro.org>
To: Vaishali Thakkar <vaishali.thakkar@linaro.org>
Cc: andy.gross@linaro.org, david.brown@linaro.org,
	gregkh@linuxfoundation.org, linux-arm-msm@vger.kernel.org,
	linux-kernel@vger.kernel.org, rafael@kernel.org,
	vkoul@kernel.org
Subject: Re: [PATCH v4 4/5] soc: qcom: socinfo: Expose custom attributes
Date: Fri, 1 Mar 2019 11:42:49 -0800	[thread overview]
Message-ID: <20190301194249.GD27005@builder> (raw)
In-Reply-To: <20190225065044.11023-5-vaishali.thakkar@linaro.org>

On Sun 24 Feb 22:50 PST 2019, Vaishali Thakkar wrote:

> +#ifdef CONFIG_DEBUG_FS
> +/* pmic model info */

Please drop this comment and make "pmic_model" plural.

> +static const char *const pmic_model[] = {
> +	[0]  = "Unknown PMIC model",
> +	[9]  = "PM8994",
> +	[11] = "PM8916",
> +	[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",
> +};
> +#endif /* CONFIG_DEBUG_FS */
[..]
> +static int qcom_show_pmic_model(struct seq_file *seq, void *p)
> +{
> +	struct socinfo *socinfo = seq->private;
> +	int model = SOCINFO_MINOR(le32_to_cpu(socinfo->pmic_model));
> +
> +	if (model < 0)
> +		return -EINVAL;

You need to deal with the fact that model might be >=
ARRAY_SIZE(pmic_model) and that pmic_mode[model] might be NULL, in the
event that you missed entries in the list or this driver is used on
newer platforms.

> +
> +	seq_printf(seq, "%s\n", pmic_model[model]);
> +
> +	return 0;
> +}
> +
> +static int qcom_show_pmic_die_revision(struct seq_file *seq, void *p)
> +{
> +	struct socinfo *socinfo = seq->private;
> +
> +	seq_printf(seq, "%u.%u\n",
> +		   SOCINFO_MAJOR(le32_to_cpu(socinfo->pmic_die_rev)),
> +		   SOCINFO_MINOR(le32_to_cpu(socinfo->pmic_die_rev)));
> +
> +	return 0;
> +}
> +
> +UINT_SHOW(raw_version, raw_ver);
> +UINT_SHOW(hardware_platform, hw_plat);
> +UINT_SHOW(platform_version, plat_ver);
> +UINT_SHOW(foundry_id, foundry_id);
> +HEX_SHOW(chip_family, chip_family);
> +HEX_SHOW(raw_device_family, raw_device_family);
> +HEX_SHOW(raw_device_number, raw_device_num);
> +QCOM_OPEN(build_id, qcom_show_build_id);
> +QCOM_OPEN(accessory_chip, qcom_show_accessory_chip);
> +QCOM_OPEN(pmic_model, qcom_show_pmic_model);
> +QCOM_OPEN(platform_subtype, qcom_show_platform_subtype);
> +QCOM_OPEN(pmic_die_revision, qcom_show_pmic_die_revision);
> +
> +static void socinfo_debugfs_init(struct qcom_socinfo *qcom_socinfo)
> +{
> +	qcom_socinfo->dbg_root = debugfs_create_dir("qcom_socinfo", NULL);
> +
> +	DEBUGFS_UINT_ADD(raw_version);
> +	DEBUGFS_UINT_ADD(hardware_platform);

Note that the content of struct socinfo has grown over time, so based on
the comments in the struct the size of the struct would not cover
hw_plat if version < 3.

So you should make the addition of these conditional on socinfo->ver.
As each version adds more entries I suggest that you do this with a:

switch (qcom_socinfo->socinfo->ver) {
case 12:
	add v12 entries;
case 11:
	add v11 entries;
case 10:
	add v10 entries;
...
};

> +	DEBUGFS_UINT_ADD(platform_version);
> +	DEBUGFS_UINT_ADD(foundry_id);
> +	DEBUGFS_HEX_ADD(chip_family);
> +	DEBUGFS_HEX_ADD(raw_device_family);
> +	DEBUGFS_HEX_ADD(raw_device_number);
> +	DEBUGFS_ADD(build_id);
> +	DEBUGFS_ADD(accessory_chip);
> +	DEBUGFS_ADD(pmic_model);
> +	DEBUGFS_ADD(platform_subtype);
> +	DEBUGFS_ADD(pmic_die_revision);
> +}
> +

Regards,
Bjorn