Message-ID: <20161213191739.GI3439@tuxbot>
Date:   Tue, 13 Dec 2016 11:17:39 -0800
From:   Bjorn Andersson <bjorn.andersson@...aro.org>
To:     Imran Khan <kimran@...eaurora.org>
Cc:     andy.gross@...aro.org, lee.jones@...aro.org,
        David Brown <david.brown@...aro.org>,
        open list <linux-kernel@...r.kernel.org>,
        "open list:ARM/QUALCOMM SUPPORT" <linux-arm-msm@...r.kernel.org>,
        "open list:ARM/QUALCOMM SUPPORT" <linux-soc@...r.kernel.org>
Subject: Re: [PATCH v6] soc: qcom: Add SoC info driver

On Mon 12 Dec 07:17 PST 2016, Imran Khan wrote:

> The SoC info driver provides information such as Chip ID,
> Chip family, serial number and other such details about
> Qualcomm SoCs.
> 
> Signed-off-by: Imran Khan <kimran@...eaurora.org>

Looks good, just some minor style things.

[..]
> diff --git a/drivers/soc/qcom/socinfo.c b/drivers/soc/qcom/socinfo.c
[..]
> +
> +#define PMIC_MODEL_UNKNOWN		0

Unused define

> +#define HW_PLATFORM_QRD			11
> +#define PLATFORM_SUBTYPE_QRD_INVALID	6

Better replace this with ARRAY_SIZE(qrd_hw_platform_subtype) in
qcom_show_platform_subtype().

> +#define PLATFORM_SUBTYPE_INVALID	4

Dito

> +/*
> + * SOC version type with major number in the upper 16 bits and minor
> + * number in the lower 16 bits.  For example:
> + *   1.0 -> 0x00010000
> + *   2.3 -> 0x00020003
> + */
> +#define SOC_VERSION_MAJOR(ver) (((ver) & 0xffff0000) >> 16)
> +#define SOC_VERSION_MINOR(ver) ((ver) & 0x0000ffff)
> +#define SOCINFO_VERSION_MAJOR	SOC_VERSION_MAJOR
> +#define SOCINFO_VERSION_MINOR	SOC_VERSION_MINOR
> +
> +#define SMEM_SOCINFO_BUILD_ID_LENGTH		32
> +#define SMEM_IMAGE_VERSION_BLOCKS_COUNT		32
> +#define SMEM_IMAGE_VERSION_SIZE			4096
> +#define SMEM_IMAGE_VERSION_NAME_SIZE		75
> +#define SMEM_IMAGE_VERSION_VARIANT_SIZE		20
> +#define SMEM_IMAGE_VERSION_OEM_SIZE		32
> +#define SMEM_IMAGE_VERSION_PARTITION_APPS	10

SMEM_IMAGE_VERSION_PARTITION_APPS is unused.

> +
> +/*
> + * SMEM item ids, used to acquire handles to respective
> + * SMEM region.
> + */
> +#define SMEM_IMAGE_VERSION_TABLE	469
> +#define SMEM_HW_SW_BUILD_ID		137
> +
> +#define MAX_ATTR_COUNT	15

Replace this with ARRAY_SIZE(qcom_socinfo_attrs).

> +
> +/*
> + * SMEM Image table indices
> + */
> +enum smem_image_table_index {
> +	SMEM_IMAGE_TABLE_BOOT_INDEX = 0,
> +	SMEM_IMAGE_TABLE_TZ_INDEX,
> +	SMEM_IMAGE_TABLE_RPM_INDEX = 3,
> +	SMEM_IMAGE_TABLE_APPS_INDEX = 10,
> +	SMEM_IMAGE_TABLE_MPSS_INDEX,
> +	SMEM_IMAGE_TABLE_ADSP_INDEX,
> +	SMEM_IMAGE_TABLE_CNSS_INDEX,
> +	SMEM_IMAGE_TABLE_VIDEO_INDEX
> +};
> +
> +struct qcom_socinfo_attr {
> +	struct device_attribute attr;
> +	int min_version;
> +};
> +
> +#define QCOM_SOCINFO_ATTR(_name, _show, _min_version) \
> +	{ __ATTR(_name, 0444, _show, NULL), _min_version }
> +
> +#define SMEM_IMG_ATTR_ENTRY(_name, _mode, _show, _store, _index)	\
> +	struct dev_ext_attribute dev_attr_##_name =			\
> +	{ __ATTR(_name, _mode, _show, _store), (void *)_index }
> +
> +#define QCOM_SMEM_IMG_ITEM(_name, _mode, _index)			\
> +	SMEM_IMG_ATTR_ENTRY(_name##_image_version, _mode,		\
> +		qcom_show_image_version, qcom_store_image_version,	\
> +		(unsigned long)_index);					\
> +	SMEM_IMG_ATTR_ENTRY(_name##_image_variant, _mode,		\
> +		qcom_show_image_variant, qcom_store_image_variant,	\
> +		(unsigned long)_index);					\
> +	SMEM_IMG_ATTR_ENTRY(_name##_image_crm, _mode,			\
> +		qcom_show_image_crm, qcom_store_image_crm,		\
> +		(unsigned long)_index);					\
> +static struct attribute *_name##_image_attrs[] = {			\
> +	&dev_attr_##_name##_image_version.attr.attr,			\
> +	&dev_attr_##_name##_image_variant.attr.attr,			\
> +	&dev_attr_##_name##_image_crm.attr.attr,			\
> +	NULL,								\
> +};									\
> +static struct attribute_group _name##_image_attr_group = {		\
> +	.attrs = _name##_image_attrs,					\
> +}

Rather than reusing dev_ext_attribute directly, you can roll your own
struct. The following would save you a bunch of type casts:

struct smem_image_attribute {
	struct device_attribute version;
	struct device_attribute variant;
	struct device_attribute crm;

	int index;
};

#define QCOM_SMEM_IMAGE_ITEM(_name, _mode, _index) \
	static struct smem_image_attribute _name##_image_attrs = { \
		__ATTR(_name##_image_version, _mode, \
		       qcom_show_image_version, qcom_store_image_version), \
		__ATTR(_name##_image_variant, _mode, \
		       qcom_show_image_variant, qcom_store_image_variant), \
		__ATTR(_name##_image_crm, _mode, \
		       qcom_show_image_crm, qcom_store_image_crm), \
		_index \
	}; \
	static struct attribute_group _name##_image_attr_group = { \
		.attrs = (struct attribute*[]) { \
			&_name##_image_attrs.version.attr, \
			&_name##_image_attrs.variant.attr, \
			&_name##_image_attrs.crm.attr, \
			NULL \
		} \
	}

Making the show functions something like:

qcom_show_image_version()
{
	struct smem_image_attribute *smem_attr;

	smem_attr = container_of(attr, struct smem_image_attribute, version);
	return scnprintf(buf, PAGE_SIZE, "%s\n",
			smem_image_version[smem_attr->index].name);
}

[..]
> +
> +static struct smem_image_version *smem_image_version;
> +static u32 socinfo_format;

I still think you should fold socinfo_populate_sysfs_files() into
qcom_socinfo_init(), and then this will turn into a local variable
instead.

[..]
> +
> +static ssize_t
> +qcom_show_platform_subtype(struct device *dev,
> +			struct device_attribute *attr,
> +			char *buf)
> +{
> +	u32 hw_subtype = socinfo->v0_6.hw_platform_subtype;
> +

To simplify the inner conditionals you can move the common check here:

if (hw_subtype < 0)
	return -EINVAL;

> +	if (socinfo->v0_3.hw_platform == HW_PLATFORM_QRD)

Use {} around if statements that are more than a single line.

> +		if (hw_subtype >= 0 &&
> +				hw_subtype < PLATFORM_SUBTYPE_QRD_INVALID)

Just to follow common style, handle the error case first, like:

		if (hw_subtype >= ARRAY_SIZE(qrd_hw_platform_subtype))
			return -EINVAL;

		return sprintf(buf, "%s\n", qrd_hw_platform_subtype[hw_subtype]);

And if you shorten "hw_subtype" to just "subtype" you can get that in
under 80 characters without loosing any clarity.

> +			return sprintf(buf, "%s\n",
> +				qrd_hw_platform_subtype[hw_subtype]);
> +		else
> +			return -EINVAL;
> +	else
> +		if (hw_subtype >= 0 &&
> +				hw_subtype < PLATFORM_SUBTYPE_INVALID)
> +			return sprintf(buf, "%s\n",
> +				hw_platform_subtype[hw_subtype]);
> +		else
> +			return -EINVAL;
> +}
> +
[..]
> +
> +static void socinfo_populate_sysfs_files(struct device *dev)
> +{
> +	int idx;
> +	int err;
> +
> +	/*
> +	 * Expose SMEM_IMAGE_TABLE to sysfs only when we have IMAGE_TABLE
> +	 * available in SMEM. As IMAGE_TABLE and SOCINFO are two separate
> +	 * items within SMEM, we expose the remaining soc information(i.e
> +	 * only the SOCINFO item available in SMEM) to sysfs even in the
> +	 * absence of an IMAGE_TABLE.
> +	 */
> +	if (smem_image_version)

Use {} around blocks larger than a single line.

> +		for (idx = 0; idx < SMEM_IMAGE_VERSION_BLOCKS_COUNT; idx++)
> +			if (smem_image_table[idx])
> +				err = sysfs_create_group(&dev->kobj,
> +					smem_image_table[idx]);

Broken lines should be indented to line up with the parenthesis on the
line above.

And as you don't care about the error here, just drop the "err"
variable.

> +
> +	for (idx = 0; idx < MAX_ATTR_COUNT; idx++)

{}

> +		if (qcom_socinfo_attrs[idx].min_version <= socinfo_format)
> +			device_create_file(dev, &qcom_socinfo_attrs[idx].attr);
> +}
> +
> +void qcom_socinfo_init(struct platform_device *pdev)
> +{
> +	struct soc_device_attribute *attr;
> +	struct device *qcom_soc_device;
> +	struct soc_device *soc_dev;
> +	size_t img_tbl_size;
> +	u32 soc_version;
> +	size_t size;
> +
> +	socinfo = qcom_smem_get(QCOM_SMEM_HOST_ANY, SMEM_HW_SW_BUILD_ID,
> +			&size);

As this value needs to be remembered to the end of the function name it
less generic, e.g. item_size.

> +	if (IS_ERR(socinfo)) {
> +		dev_err(&pdev->dev, "Coudn't find socinfo.\n");
> +		return;
> +	}
> +
> +	if ((SOCINFO_VERSION_MAJOR(socinfo->v0_1.format) != 0) ||
> +			(SOCINFO_VERSION_MINOR(socinfo->v0_1.format) < 0) ||
> +			(socinfo->v0_1.format > MAX_SOCINFO_FORMAT)) {
> +		dev_err(&pdev->dev, "Wrong socinfo format\n");
> +		return;
> +	}
> +	socinfo_format = socinfo->v0_1.format;
> +
> +	if (!socinfo->v0_1.id)
> +		dev_err(&pdev->dev, "socinfo: Unknown SoC ID!\n");
> +
> +	WARN(socinfo->v0_1.id >= ARRAY_SIZE(cpu_of_id),
> +		"New IDs added! ID => CPU mapping needs an update.\n");
> +
> +	smem_image_version = qcom_smem_get(QCOM_SMEM_HOST_ANY,
> +				SMEM_IMAGE_VERSION_TABLE,
> +				&img_tbl_size);
> +	if (IS_ERR(smem_image_version) ||
> +		(img_tbl_size != SMEM_IMAGE_VERSION_SIZE)) {

This size is on the other hand very short lived, so the name doesn't
matter as much - name it "size" and you don't have to line wrap the
conditional.

> +		dev_warn(&pdev->dev, "Image version table absent.\n");
> +		smem_image_version = NULL;
> +	}
> +
> +	attr = kzalloc(sizeof(*attr), GFP_KERNEL);
> +	if (!attr) {
> +		dev_err(&pdev->dev, "Unable to allocate attributes.\n");

kzalloc() will print an error message if it fails, so need for you to
print another one.

> +		return;
> +	}
> +	soc_version = socinfo->v0_1.version;
> +
> +	attr->soc_id = kasprintf(GFP_KERNEL, "%d", socinfo->v0_1.id);
> +	attr->family = "Snapdragon";
> +	attr->machine = cpu_of_id[socinfo->v0_1.id];
> +	attr->revision = kasprintf(GFP_KERNEL, "%u.%u",
> +				SOC_VERSION_MAJOR(soc_version),
> +				SOC_VERSION_MINOR(soc_version));
> +
> +	soc_dev = soc_device_register(attr);
> +	if (IS_ERR(soc_dev)) {
> +		kfree(attr);
> +		return;
> +	}
> +
> +	qcom_soc_device = soc_device_to_device(soc_dev);
> +	socinfo_populate_sysfs_files(qcom_soc_device);
> +
> +	/* Feed the soc specific unique data into entropy pool */
> +	add_device_randomness(socinfo, size);
> +}
> +EXPORT_SYMBOL(qcom_socinfo_init);

Regards,
Bjorn

Powered by blists - more mailing lists