On 02/16, Imran Khan wrote:
> diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c
> index 18ec52f..4f0ccf8 100644
> --- a/drivers/soc/qcom/smem.c
> +++ b/drivers/soc/qcom/smem.c
> @@ -85,6 +85,9 @@
>  /* Max number of processors/hosts in a system */
>  #define SMEM_HOST_COUNT              9
>  
> +
> +extern void qcom_socinfo_init(struct device *device);
> +

Sparse still complains... o well.

> diff --git a/drivers/soc/qcom/socinfo.c b/drivers/soc/qcom/socinfo.c
> new file mode 100644
> index 0000000..495f937
> --- /dev/null
> +++ b/drivers/soc/qcom/socinfo.c
> @@ -0,0 +1,557 @@
> +/*
> + * Copyright (c) 2009-2017, The Linux Foundation. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + */
> +
> +#include <linux/export.h>
> +#include <linux/err.h>
> +#include <linux/of.h>
> +#include <linux/of_fdt.h>
> +#include <linux/libfdt.h>
> +#include <linux/platform_device.h>
> +#include <linux/sys_soc.h>
> +#include <linux/slab.h>
> +#include <linux/string.h>
> +#include <linux/types.h>
> +#include <linux/random.h>
> +#include <linux/soc/qcom/smem.h>
> +
> +/*
> + * 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_VER_MAJ(ver) (((ver) & 0xffff0000) >> 16)
> +#define SOC_VER_MIN(ver) ((ver) & 0x0000ffff)
> +#define SOCINFO_VERSION_MAJOR        SOC_VER_MAJ
> +#define SOCINFO_VERSION_MINOR        SOC_VER_MIN

Do we really need two defines for the same thing?

> +
> +#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
> +
> +/*
> + * SMEM item ids, used to acquire handles to respective
> + * SMEM region.
> + */
> +#define SMEM_IMAGE_VERSION_TABLE     469
> +#define SMEM_HW_SW_BUILD_ID          137
> +
> +/*
> + * SMEM Image table indices
> + */
> +#define SMEM_IMAGE_TABLE_BOOT_INDEX  0
> +#define SMEM_IMAGE_TABLE_TZ_INDEX    1
> +#define SMEM_IMAGE_TABLE_RPM_INDEX   3
> +#define SMEM_IMAGE_TABLE_APPS_INDEX  10
> +#define SMEM_IMAGE_TABLE_MPSS_INDEX  11
> +#define SMEM_IMAGE_TABLE_ADSP_INDEX  12
> +#define SMEM_IMAGE_TABLE_CNSS_INDEX  13
> +#define SMEM_IMAGE_TABLE_VIDEO_INDEX 14
> +
> +struct qcom_socinfo_attr {
> +     struct device_attribute attr;
> +     int min_ver;
> +};
> +
> +#define QCOM_SOCINFO_ATTR(_name, _show, _min_ver) \
> +     { __ATTR(_name, 0444, _show, NULL), _min_ver }
> +
> +
> +struct smem_image_attribute {
> +     struct device_attribute version;
> +     struct device_attribute variant;
> +     struct device_attribute crm;
> +     int index;
> +};
> +
> +#define QCOM_SMEM_IMG_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 = { \

can be const.

> +             .attrs = (struct attribute*[]) { \
> +                     &_name##_image_attrs.version.attr, \
> +                     &_name##_image_attrs.variant.attr, \
> +                     &_name##_image_attrs.crm.attr, \
> +                     NULL \
> +             } \
> +     }
> +
> +static const char *const pmic_model[] = {
> +     [0]  = "Unknown PMIC model",

Missing pm8916, but it doesn't seem to matter for me. My db410c
says 0 here.

> +     [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",
> +};
> +
> +struct smem_image_version {
> +     char name[SMEM_IMAGE_VERSION_NAME_SIZE];
> +     char variant[SMEM_IMAGE_VERSION_VARIANT_SIZE];
> +     char pad;
> +     char oem[SMEM_IMAGE_VERSION_OEM_SIZE];
> +};
> +
> +struct qcom_soc_info {
> +     int id;
> +     const char *name;
> +};
> +
> +/* Used to parse shared memory. Must match the modem. */
> +struct socinfo_v0_1 {
> +     __le32 fmt;
> +     __le32 id;
> +     __le32 ver;
> +     char build_id[SMEM_SOCINFO_BUILD_ID_LENGTH];

Is this a C style string? Or just a bunch of bytes?

> +};
> +
> +struct socinfo_v0_2 {
> +     struct socinfo_v0_1 v0_1;
> +     __le32 raw_ver;
> +};
> +
> +struct socinfo_v0_3 {
> +     struct socinfo_v0_2 v0_2;
> +     __le32 hw_plat;
> +};
> +
> +struct socinfo_v0_4 {
> +     struct socinfo_v0_3 v0_3;
> +     __le32 plat_ver;
> +};
> +
> +struct socinfo_v0_5 {
> +     struct socinfo_v0_4 v0_4;
> +     __le32 accsry_chip;

accesory_chip?

> +};
> +
> +struct socinfo_v0_6 {
> +     struct socinfo_v0_5 v0_5;
> +     __le32 hw_plat_subtype;
> +};
> +
> +struct socinfo_v0_7 {
> +     struct socinfo_v0_6 v0_6;
> +     __le32 pmic_model;
> +     __le32 pmic_die_rev;
> +};
> +
> +struct socinfo_v0_8 {
> +     struct socinfo_v0_7 v0_7;
> +     __le32 pmic_model_1;
> +     __le32 pmic_die_rev_1;
> +     __le32 pmic_model_2;
> +     __le32 pmic_die_rev_2;
> +};

So we have multiple ways to print out pmic information. Hopefully
we can do the right one at the right time based on version we
support?

> +
> +struct socinfo_v0_9 {
> +     struct socinfo_v0_8 v0_8;
> +     __le32 fndry_id;

Can we write foundry_id?

> +};
> +
> +struct socinfo_v0_10 {
> +     struct socinfo_v0_9 v0_9;
> +     __le32 srl_num;

And serial_number or serial_num? Not sure why we lost so many vowels.

> +};
> +
> +struct socinfo_v0_11 {
> +     struct socinfo_v0_10 v0_10;
> +     __le32 num_pmics;
> +     __le32 pmic_array_offset;
> +};
> +
> +struct socinfo_v0_12 {
> +     struct socinfo_v0_11 v0_11;
> +     __le32 chip_family;
> +     __le32 raw_dev_fam;

raw_device_family?

> +     __le32 raw_dev_num;
> +};
> +
> +static union {
> +     struct socinfo_v0_1 v0_1;
> +     struct socinfo_v0_2 v0_2;
> +     struct socinfo_v0_3 v0_3;
> +     struct socinfo_v0_4 v0_4;
> +     struct socinfo_v0_5 v0_5;
> +     struct socinfo_v0_6 v0_6;
> +     struct socinfo_v0_7 v0_7;
> +     struct socinfo_v0_8 v0_8;
> +     struct socinfo_v0_9 v0_9;
> +     struct socinfo_v0_10 v0_10;
> +     struct socinfo_v0_11 v0_11;
> +     struct socinfo_v0_12 v0_12;

I find this design odd. Do we really care that all these versions
append to each other? Why not just have one structure with
comments delimiting where a new version fields start and then
dropping the whole v0_1, v0_2 thing?

> +} *socinfo;
> +
> +static struct qcom_soc_info soc_of_id[] = {

const?

> +     {87, "MSM8960"},

Also please throw some spaces around brackets here.

> +     {109, "APQ8064"},
> +     {130, "MPQ8064"},
> +     {122, "MSM8660A"},
> +     {123, "MSM8260A"},
> +     {124, "APQ8060A"},
> +     {126, "MSM8974"},
> +     {184, "APQ8074"},
> +     {185, "MSM8274"},
> +     {186, "MSM8674"},
> +     {208, "APQ8074-AA"},
> +     {211, "MSM8274-AA"},
> +     {214, "MSM8674-AA"},
> +     {217, "MSM8974-AA"},
> +     {209, "APQ8074-AB"},
> +     {212, "MSM8274-AB"},
> +     {215, "MSM8674-AB"},
> +     {218, "MSM8974-AB"},
> +     {194, "MSM8974PRO"},
> +     {210, "APQ8074PRO"},
> +     {213, "MSM8274PRO"},
> +     {216, "MSM8674PRO"},
> +     {138, "MSM8960AB"},
> +     {139, "APQ8060AB"},
> +     {140, "MSM8260AB"},
> +     {141, "MSM8660AB"},
> +     {178, "APQ8084"},
> +     {206, "MSM8916"},
> +     {247, "APQ8016"},
> +     {248, "MSM8216"},
> +     {249, "MSM8116"},
> +     {250, "MSM8616"},
> +     {246, "MSM8996"},
> +     {310, "MSM8996AU"},
> +     {311, "APQ8096AU"},
> +     {291, "APQ8096"},
> +     {305, "MSM8996SG"},
> +     {312, "APQ8096SG"},
> +};
> +
> +static struct smem_image_version *smem_image_version;
> +
> +/* max socinfo format version supported */
> +#define MAX_SOCINFO_FORMAT 12
> +
> +/* socinfo: sysfs functions */
> +
> +static ssize_t
> +qcom_show_vendor(struct device *dev, struct device_attribute *attr, char 
> *buf)
> +{
> +     return sprintf(buf, "%s", "Qualcomm\n");

We can't just sprintf(buf, "Qualcomm\n") ?

> +}
> +
> +static ssize_t
> +qcom_show_raw_version(struct device *dev, struct device_attribute *attr,
> +                  char *buf)
> +{
> +     return sprintf(buf, "%u\n", le32_to_cpu(socinfo->v0_2.raw_ver));
> +}
> +
> +static ssize_t
> +qcom_show_build_id(struct device *dev, struct device_attribute *attr, char 
> *buf)
> +{
> +     return scnprintf(buf, PAGE_SIZE, "%s\n", socinfo->v0_1.build_id);

Why does this one get PAGE_SIZE but others don't?

> +}
> +
> +static ssize_t
> +qcom_show_hw_platform(struct device *dev, struct device_attribute *attr,
> +                     char *buf)
> +{
> +     return sprintf(buf, "%u\n", le32_to_cpu(socinfo->v0_3.hw_plat));
> +}
> +
> +static ssize_t
> +qcom_show_platform_version(struct device *dev, struct device_attribute *attr,
> +                             char *buf)
> +{
> +     return sprintf(buf, "%u\n", le32_to_cpu(socinfo->v0_4.plat_ver));
> +}
> +
> +static ssize_t
> +qcom_show_accessory_chip(struct device *dev, struct device_attribute *attr,
> +                     char *buf)
> +{
> +     return sprintf(buf, "%u\n", le32_to_cpu(socinfo->v0_5.accsry_chip));
> +}
> +
> +static ssize_t
> +qcom_show_platform_subtype(struct device *dev, struct device_attribute *attr,
> +                             char *buf)
> +{
> +     u32 subtype = le32_to_cpu(socinfo->v0_6.hw_plat_subtype);
> +
> +     if (subtype < 0)

How can an unsigned type be less than zero?

> +             return -EINVAL;
> +
> +     return sprintf(buf, "%u\n", subtype);
> +}
> +
[...]
> +
> +static const char *socinfo_get_id_string(int id)
> +{
> +     int idx;
> +
> +     for (idx = 0; idx < ARRAY_SIZE(soc_of_id); idx++) {
> +             if (soc_of_id[idx].id == id)
> +                     return soc_of_id[idx].name;
> +     }
> +
> +     return NULL;
> +}
> +
> +void qcom_socinfo_init(struct device *device)
> +{
> +     struct soc_device_attribute *attr;
> +     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");

Couldn't

> +             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) {
> +             dev_err(device, "Wrong socinfo format\n");
> +             return;
> +     }
> +
> +     if (!le32_to_cpu(socinfo->v0_1.id))
> +             dev_err(device, "socinfo: Unknown SoC ID!\n");

Drop socinfo: please.

---8<----
diff --git a/drivers/soc/qcom/socinfo.c b/drivers/soc/qcom/socinfo.c
index 495f937ce77b..6a453be283ca 100644
--- a/drivers/soc/qcom/socinfo.c
+++ b/drivers/soc/qcom/socinfo.c
@@ -88,7 +88,7 @@ struct smem_image_attribute {
                        qcom_show_image_crm, qcom_store_image_crm), \
                _index \
        }; \
-       static struct attribute_group _name##_image_attr_group = { \
+       static const struct attribute_group _name##_image_attr_group = { \
                .attrs = (struct attribute*[]) { \
                        &_name##_image_attrs.version.attr, \
                        &_name##_image_attrs.variant.attr, \
@@ -99,6 +99,7 @@ struct smem_image_attribute {
 
 static const char *const pmic_model[] = {
        [0]  = "Unknown PMIC model",
+       [11] = "PM8916",
        [13] = "PM8058",
        [14] = "PM8028",
        [15] = "PM8901",
@@ -453,8 +454,8 @@ QCOM_SMEM_IMG_ITEM(adsp, 0444, SMEM_IMAGE_TABLE_ADSP_INDEX);
 QCOM_SMEM_IMG_ITEM(cnss, 0444, SMEM_IMAGE_TABLE_CNSS_INDEX);
 QCOM_SMEM_IMG_ITEM(video, 0444, SMEM_IMAGE_TABLE_VIDEO_INDEX);
 
-static const struct attribute_group
-       *smem_img_tbl[SMEM_IMAGE_VERSION_BLOCKS_COUNT] = {
+static const
+struct attribute_group *smem_img_tbl[SMEM_IMAGE_VERSION_BLOCKS_COUNT] = {
                [SMEM_IMAGE_TABLE_BOOT_INDEX] = &boot_image_attr_group,
                [SMEM_IMAGE_TABLE_TZ_INDEX] = &tz_image_attr_group,
                [SMEM_IMAGE_TABLE_RPM_INDEX] = &rpm_image_attr_group,
@@ -489,7 +490,7 @@ void qcom_socinfo_init(struct device *device)
        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");
+               dev_err(device, "Couldn't find socinfo\n");
                return;
        }
 
@@ -501,7 +502,7 @@ void qcom_socinfo_init(struct device *device)
        }
 
        if (!le32_to_cpu(socinfo->v0_1.id))
-               dev_err(device, "socinfo: Unknown SoC ID!\n");
+               dev_err(device, "Unknown SoC ID!\n");
 
        smem_image_version = qcom_smem_get(QCOM_SMEM_HOST_ANY,
                                SMEM_IMAGE_VERSION_TABLE,
@@ -534,7 +535,7 @@ void qcom_socinfo_init(struct device *device)
        /*
         * 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
+        * 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.
         */
-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Reply via email to