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@xxxxxxxxxxxxxx>
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
--
To unsubscribe from this list: send the line "unsubscribe linux-soc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
