On 10/21/2016 4:03 PM, Arnd Bergmann wrote:
> On Thursday, October 20, 2016 7:36:22 PM CEST Imran Khan wrote:
>> +#include <linux/soc/qcom/socinfo.h>
>> +#include <linux/soc/qcom/smem.h>
>> +
>> +#include <asm/system_misc.h>
>
> I don't see anything here that needs asm/system_misc.h
>
Okay. I will not use this header file here.
>> +const char *hw_platform[] = {
>> + [HW_PLATFORM_UNKNOWN] = "Unknown",
>> + [HW_PLATFORM_SURF] = "Surf",
>> + [HW_PLATFORM_FFA] = "FFA",
>> + [HW_PLATFORM_FLUID] = "Fluid",
>> +};
>> +
>> +const char *qrd_hw_platform_subtype[] = {
>> + [PLATFORM_SUBTYPE_QRD] = "QRD",
>> + [PLATFORM_SUBTYPE_SKUAA] = "SKUAA",
>> +};
>> +
>> +const char *hw_platform_subtype[] = {
>> + [PLATFORM_SUBTYPE_UNKNOWN] = "Unknown",
>> +};
>
>
> Make these all static
>
Okay. Will do this.
>> +
>> +/* Used to parse shared memory. Must match the modem. */
>> +struct socinfo_v0_1 {
>> + uint32_t format;
>> + uint32_t id;
>> + uint32_t version;
>
> s/uint32_t/u32/
>
Okay. Will do this.
>> +
>> +uint32_t socinfo_get_id(void)
>> +{
>> + return (socinfo) ? socinfo->v0_1.id : 0;
>> +}
>> +EXPORT_SYMBOL_GPL(socinfo_get_id);
>> +
>> +uint32_t socinfo_get_version(void)
>> +{
>> + return (socinfo) ? socinfo->v0_1.version : 0;
>> +}
>> +EXPORT_SYMBOL_GPL(socinfo_get_version);
>> +
>> +char *socinfo_get_build_id(void)
>> +{
>> + return (socinfo) ? socinfo->v0_1.build_id : NULL;
>> +}
>> +EXPORT_SYMBOL_GPL(socinfo_get_build_id);
>
> Please remove all the exports and mark the functions static,
> we don't want any drivers poking vendor-specific interfaces
> for this.
>
Yes. I will avoid exporting vendor specific interface.
>> +/* socinfo: sysfs functions */
>
> This seems overly verbose, having both raw and human-readable
> IDs is generally not necessary, pick one of the two. If you
> need any fields that we don't already support in soc_device,
> let's talk about adding them to the generic structure.
>
>
Okay. I will go for human readable IDs. Can we add 2 more fields
in the generic structure.
These 2 fields would be:
vendor: A string for vendor name
serial_number: A string containing serial number for the platform
>> +static ssize_t
>> +qcom_get_image_version(struct device *dev,
>> + struct device_attribute *attr,
>> + char *buf)
>> +{
>> + char *string_address;
>> +
>> + string_address = socinfo_get_image_version_base_address(dev);
>> + if (IS_ERR_OR_NULL(string_address)) {
>
> IS_ERR_OR_NULL() indicates that your interface is not well-designed.
> Please return either NULL on all failures, or return an error pointer
> on all failures, not both.
>
>> +/* Platform Subtype String is being deprecated. Use Platform
>> + * Subtype ID instead.
>> + */
>
> If it's deprecated, don't add it to the kernel!
>
>
Okay.
>> diff --git a/include/linux/soc/qcom/socinfo.h b/include/linux/soc/qcom/socinfo.h
>> new file mode 100644
>> index 0000000..17ca50a
>> --- /dev/null
>> +++ b/include/linux/soc/qcom/socinfo.h
>
> Merge the header file into the driver.
>
okay.
> Arnd
>
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a\nmember of the Code Aurora Forum, hosted by The Linux Foundation
--
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
