From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933966AbcLMTSC (ORCPT ); Tue, 13 Dec 2016 14:18:02 -0500 Received: from mail-pg0-f53.google.com ([74.125.83.53]:35486 "EHLO mail-pg0-f53.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752826AbcLMTRp (ORCPT ); Tue, 13 Dec 2016 14:17:45 -0500 Date: Tue, 13 Dec 2016 11:17:39 -0800 From: Bjorn Andersson To: Imran Khan Cc: andy.gross@linaro.org, lee.jones@linaro.org, David Brown , open list , "open list:ARM/QUALCOMM SUPPORT" , "open list:ARM/QUALCOMM SUPPORT" Subject: Re: [PATCH v6] soc: qcom: Add SoC info driver Message-ID: <20161213191739.GI3439@tuxbot> References: <1481555889-29380-1-git-send-email-kimran@codeaurora.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1481555889-29380-1-git-send-email-kimran@codeaurora.org> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 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