From: Stephen Boyd <swboyd@chromium.org> To: andy.gross@linaro.org Cc: david.brown@linaro.org, gregkh@linuxfoundation.org, linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org, rafael@kernel.org, bjorn.andersson@linaro.org, vkoul@kernel.org, Imran Khan <kimran@codeaurora.org>, Vaishali Thakkar <vaishali.thakkar@linaro.org> Subject: Re: [PATCH v4 3/5] soc: qcom: Add socinfo driver Date: Thu, 28 Feb 2019 11:34:56 -0800 [thread overview] Message-ID: <155138249693.16805.4376975971704887156@swboyd.mtv.corp.google.com> (raw) In-Reply-To: <20190225065044.11023-4-vaishali.thakkar@linaro.org> 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");
WARNING: multiple messages have this Message-ID (diff)
From: Stephen Boyd <swboyd@chromium.org> To: Vaishali Thakkar <vaishali.thakkar@linaro.org>, andy.gross@linaro.org Cc: david.brown@linaro.org, gregkh@linuxfoundation.org, linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org, rafael@kernel.org, bjorn.andersson@linaro.org, vkoul@kernel.org, Imran Khan <kimran@codeaurora.org>, Vaishali Thakkar <vaishali.thakkar@linaro.org> Subject: Re: [PATCH v4 3/5] soc: qcom: Add socinfo driver Date: Thu, 28 Feb 2019 11:34:56 -0800 [thread overview] Message-ID: <155138249693.16805.4376975971704887156@swboyd.mtv.corp.google.com> (raw) In-Reply-To: <20190225065044.11023-4-vaishali.thakkar@linaro.org> 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");
next prev parent reply other threads:[~2019-02-28 19:34 UTC|newest] Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-02-25 6:50 [PATCH v4 0/5] soc: qcom: Add SoC info driver Vaishali Thakkar 2019-02-25 6:50 ` [PATCH v4 1/5] base: soc: Add serial_number attribute to soc Vaishali Thakkar 2019-02-28 19:23 ` Stephen Boyd 2019-02-28 19:23 ` Stephen Boyd 2019-02-25 6:50 ` [PATCH v4 2/5] base: soc: Export soc_device_register/unregister APIs Vaishali Thakkar 2019-02-28 19:23 ` Stephen Boyd 2019-02-28 19:23 ` Stephen Boyd 2019-03-01 19:08 ` Bjorn Andersson 2019-02-25 6:50 ` [PATCH v4 3/5] soc: qcom: Add socinfo driver Vaishali Thakkar 2019-02-28 19:34 ` Stephen Boyd [this message] 2019-02-28 19:34 ` Stephen Boyd 2019-03-01 19:23 ` Bjorn Andersson 2019-02-25 6:50 ` [PATCH v4 4/5] soc: qcom: socinfo: Expose custom attributes Vaishali Thakkar 2019-02-28 21:32 ` Stephen Boyd 2019-02-28 21:32 ` Stephen Boyd 2019-03-14 11:25 ` Vaishali Thakkar 2019-03-14 15:58 ` Stephen Boyd 2019-03-21 5:51 ` Vaishali Thakkar 2019-03-23 0:01 ` Stephen Boyd 2019-03-24 17:42 ` Vaishali Thakkar 2019-03-25 16:01 ` Stephen Boyd 2019-03-25 20:58 ` Vaishali Thakkar 2019-03-01 19:42 ` Bjorn Andersson 2019-02-25 6:50 ` [PATCH v4 5/5] soc: qcom: socinfo: Expose image information Vaishali Thakkar 2019-02-28 21:34 ` Stephen Boyd 2019-02-28 21:34 ` Stephen Boyd 2019-03-01 19:51 ` Bjorn Andersson
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=155138249693.16805.4376975971704887156@swboyd.mtv.corp.google.com \ --to=swboyd@chromium.org \ --cc=andy.gross@linaro.org \ --cc=bjorn.andersson@linaro.org \ --cc=david.brown@linaro.org \ --cc=gregkh@linuxfoundation.org \ --cc=kimran@codeaurora.org \ --cc=linux-arm-msm@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=rafael@kernel.org \ --cc=vaishali.thakkar@linaro.org \ --cc=vkoul@kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.