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

[Bjorn Andersson <bjorn.andersson@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