Re: [PATCH v8 1/2] soc: qcom: Add SoC info driver

[Bjorn Andersson]

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.

[..]
> +
> +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.

[..]
> +
> +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.

[..]
> +
> +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.

> +
> +	/* 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