Re: [PATCH v4 3/5] soc: qcom: Add socinfo driver

[Stephen Boyd]

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");