Quoting Vaishali Thakkar (2019-02-24 22:50:42)
> diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c
> index f80d040601fd..efe0b053ef82 100644
> --- a/drivers/soc/qcom/smem.c
> +++ b/drivers/soc/qcom/smem.c
> @@ -971,11 +972,18 @@ static int qcom_smem_probe(struct platform_device *pdev)
>
> __smem = smem;
>
> + smem->socinfo = platform_device_register_data(&pdev->dev, "qcom-socinfo",
> + PLATFORM_DEVID_NONE, NULL,
> + 0);
> + if (IS_ERR(smem->socinfo))
> + dev_err(&pdev->dev, "failed to register socinfo device\n");
But we don't fail the probe? Maybe a comment should be added indicating
why it's ok to not fail here, and dev_err should be changed to dev_dbg()
then.
> +
> return 0;
> }
>
> static int qcom_smem_remove(struct platform_device *pdev)
> {
> +
Nitpick: Drop this change? Or add the code to remove the socinfo device
that is created in probe?
> hwspin_lock_free(__smem->hwlock);
> __smem = NULL;
>
> diff --git a/drivers/soc/qcom/socinfo.c b/drivers/soc/qcom/socinfo.c
> new file mode 100644
> index 000000000000..02078049fac7
> --- /dev/null
> +++ b/drivers/soc/qcom/socinfo.c
> @@ -0,0 +1,197 @@
[...]
> +
> +struct soc_of_id {
> + unsigned int id;
> + const char *name;
> +};
> +
> +static const struct soc_of_id soc_of_id[] = {
> + {87, "MSM8960"},
Nitpick: Please space out the numbers and strings:
{ 87, "MSM8960" },
> +};
> +
> +static const char *socinfo_machine(struct device *dev, unsigned 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;
Why is it called soc_of_id? Is that supposed to be "SoC of ID" or is it
DT based and is "SoC OF ID"? If it's the latter, I'd prefer some non-DT
type of name to provide more clarity that this has nothing to do with
DeviceTree.
> + }
> +
> + if (IS_ERR(soc_of_id[idx].name))
> + dev_err(dev, "Unknown soc id\n");
> +
> + return NULL;
> +}
> +
> +static int qcom_socinfo_probe(struct platform_device *pdev)
> +{
> + struct qcom_socinfo *qs;
> + struct socinfo *info;
> + size_t item_size;
> +
> + info = qcom_smem_get(QCOM_SMEM_HOST_ANY, SMEM_HW_SW_BUILD_ID,
> + &item_size);
> + if (IS_ERR(info)) {
> + dev_err(&pdev->dev, "Couldn't find socinfo\n");
> + return -EINVAL;
Why not return PTR_ERR(info)?
> + }
> +
> + qs = devm_kzalloc(&pdev->dev, sizeof(*qs), GFP_KERNEL);
> + if (!qs)
> + return -ENOMEM;
> +
> + qs->attr.family = "Snapdragon";
> + qs->attr.machine = socinfo_machine(&pdev->dev,
> + le32_to_cpu(info->id));
> + qs->attr.revision = devm_kasprintf(&pdev->dev, GFP_KERNEL, "%u.%u",
> + SOCINFO_MAJOR(le32_to_cpu(info->ver)),
> + SOCINFO_MINOR(le32_to_cpu(info->ver)));
> + if (le32_to_cpu(info->fmt) >= 10)
Maybe this would make more sense if it was written with item_size and
offset of info->fmt? Something like
if (offsetof(struct qcom_socinfo, serial_num) <= item_size)
> + qs->attr.serial_number = devm_kasprintf(&pdev->dev, GFP_KERNEL,
> + "%u",
> + le32_to_cpu(info->serial_num));
> +
> + qs->soc_dev = soc_device_register(&qs->attr);
> + if (IS_ERR(qs->soc_dev))
> + return PTR_ERR(qs->soc_dev);
> +
> + /* Feed the soc specific unique data into entropy pool */
> + add_device_randomness(info, item_size);
> +
> + platform_set_drvdata(pdev, qs->soc_dev);
Weird, this is setting it to qs->soc_dev....
> +
> + return 0;
> +}
> +
> +static int qcom_socinfo_remove(struct platform_device *pdev)
> +{
> + struct qcom_socinfo *qs = platform_get_drvdata(pdev);
And then extracting this as qs only...
> +
> + soc_device_unregister(qs->soc_dev);
And so it looks wrong? Probably already have qs->soc_dev from the
platform_get_drvdata() call?
> +
> + return 0;
> +}
> +
> +static struct platform_driver qcom_socinfo_driver = {
> + .probe = qcom_socinfo_probe,
> + .remove = qcom_socinfo_remove,
> + .driver = {
> + .name = "qcom-socinfo",
> + },
> +};
> +
> +module_platform_driver(qcom_socinfo_driver);
> +
> +MODULE_DESCRIPTION("Qualcomm socinfo driver");
Maybe write socinfo as SoCinfo here?
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("platform:qcom-socinfo");
