Messages in this thread | | | Date | Mon, 10 Jan 2022 11:46:39 +0000 | Subject | Re: [PATCH v3 1/7] mfd: qcom-spmi-pmic: expose the PMIC revid information to clients | From | Caleb Connolly <> |
| |
On 09/01/2022 16:57, Jonathan Cameron wrote: > On Thu, 6 Jan 2022 17:31:25 +0000 > Caleb Connolly <caleb.connolly@linaro.org> wrote: > >> Some PMIC functions such as the RRADC need to be aware of the PMIC >> chip revision information to implement errata or otherwise adjust >> behaviour, export the PMIC information to enable this. >> >> This is specifically required to enable the RRADC to adjust >> coefficients based on which chip fab the PMIC was produced in, >> this can vary per unique device and therefore has to be read at >> runtime. >> >> Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org> > Hi Caleb, > > Some comments inline. Hi Jonathan,
Thanks for the feedback, I had a question about one of your points below. > > Thanks, > > Jonathan > >> --- >> drivers/mfd/qcom-spmi-pmic.c | 108 +++++++++++++++++------------------ >> include/soc/qcom/qcom-pmic.h | 63 ++++++++++++++++++++ >> 2 files changed, 114 insertions(+), 57 deletions(-) >> create mode 100644 include/soc/qcom/qcom-pmic.h >> >> diff --git a/drivers/mfd/qcom-spmi-pmic.c b/drivers/mfd/qcom-spmi-pmic.c >> index 1cacc00aa6c9..6b75c2f52b74 100644 >> --- a/drivers/mfd/qcom-spmi-pmic.c >> +++ b/drivers/mfd/qcom-spmi-pmic.c >> @@ -3,51 +3,24 @@ >> * Copyright (c) 2014, The Linux Foundation. All rights reserved. >> */ >> >> +#include <linux/device.h> >> +#include <linux/gfp.h> >> #include <linux/kernel.h> >> #include <linux/module.h> >> +#include <linux/slab.h> >> #include <linux/spmi.h> >> #include <linux/regmap.h> >> #include <linux/of_platform.h> >> +#include <soc/qcom/qcom-pmic.h> >> >> #define PMIC_REV2 0x101 >> #define PMIC_REV3 0x102 >> #define PMIC_REV4 0x103 >> #define PMIC_TYPE 0x104 >> #define PMIC_SUBTYPE 0x105 >> - >> +#define PMIC_FAB_ID 0x1f2 >> #define PMIC_TYPE_VALUE 0x51 >> >> -#define COMMON_SUBTYPE 0x00 >> -#define PM8941_SUBTYPE 0x01 >> -#define PM8841_SUBTYPE 0x02 >> -#define PM8019_SUBTYPE 0x03 >> -#define PM8226_SUBTYPE 0x04 >> -#define PM8110_SUBTYPE 0x05 >> -#define PMA8084_SUBTYPE 0x06 >> -#define PMI8962_SUBTYPE 0x07 >> -#define PMD9635_SUBTYPE 0x08 >> -#define PM8994_SUBTYPE 0x09 >> -#define PMI8994_SUBTYPE 0x0a >> -#define PM8916_SUBTYPE 0x0b >> -#define PM8004_SUBTYPE 0x0c >> -#define PM8909_SUBTYPE 0x0d >> -#define PM8028_SUBTYPE 0x0e >> -#define PM8901_SUBTYPE 0x0f >> -#define PM8950_SUBTYPE 0x10 >> -#define PMI8950_SUBTYPE 0x11 >> -#define PM8998_SUBTYPE 0x14 >> -#define PMI8998_SUBTYPE 0x15 >> -#define PM8005_SUBTYPE 0x18 >> -#define PM660L_SUBTYPE 0x1A >> -#define PM660_SUBTYPE 0x1B >> -#define PM8150_SUBTYPE 0x1E >> -#define PM8150L_SUBTYPE 0x1f >> -#define PM8150B_SUBTYPE 0x20 >> -#define PMK8002_SUBTYPE 0x21 >> -#define PM8009_SUBTYPE 0x24 >> -#define PM8150C_SUBTYPE 0x26 >> -#define SMB2351_SUBTYPE 0x29 >> - >> static const struct of_device_id pmic_spmi_id_table[] = { >> { .compatible = "qcom,pm660", .data = (void *)PM660_SUBTYPE }, >> { .compatible = "qcom,pm660l", .data = (void *)PM660L_SUBTYPE }, >> @@ -81,42 +54,47 @@ static const struct of_device_id pmic_spmi_id_table[] = { >> { } >> }; >> >> -static void pmic_spmi_show_revid(struct regmap *map, struct device *dev) >> +static int pmic_spmi_load_revid(struct regmap *map, struct device *dev, >> + struct qcom_spmi_pmic *pmic) >> { >> - unsigned int rev2, minor, major, type, subtype; >> - const char *name = "unknown"; >> int ret, i; >> >> - ret = regmap_read(map, PMIC_TYPE, &type); >> + ret = regmap_read(map, PMIC_TYPE, &pmic->type); >> if (ret < 0) >> - return; >> + return ret; >> >> - if (type != PMIC_TYPE_VALUE) >> - return; >> + if (pmic->type != PMIC_TYPE_VALUE) >> + return ret; >> >> - ret = regmap_read(map, PMIC_SUBTYPE, &subtype); >> + ret = regmap_read(map, PMIC_SUBTYPE, &pmic->subtype); >> if (ret < 0) >> - return; >> + return ret; >> >> for (i = 0; i < ARRAY_SIZE(pmic_spmi_id_table); i++) { >> - if (subtype == (unsigned long)pmic_spmi_id_table[i].data) >> + if (pmic->subtype == (unsigned long)pmic_spmi_id_table[i].data) >> break; >> } >> >> if (i != ARRAY_SIZE(pmic_spmi_id_table)) >> - name = pmic_spmi_id_table[i].compatible; >> + pmic->name = devm_kstrdup_const(dev, pmic_spmi_id_table[i].compatible, GFP_KERNEL); > > Managed allocation that you call kfree on in remove(). > >> >> - ret = regmap_read(map, PMIC_REV2, &rev2); >> + ret = regmap_read(map, PMIC_REV2, &pmic->rev2); >> if (ret < 0) >> - return; >> + return ret; >> >> - ret = regmap_read(map, PMIC_REV3, &minor); >> + ret = regmap_read(map, PMIC_REV3, &pmic->minor); >> if (ret < 0) >> - return; >> + return ret; >> >> - ret = regmap_read(map, PMIC_REV4, &major); >> + ret = regmap_read(map, PMIC_REV4, &pmic->major); >> if (ret < 0) >> - return; >> + return ret; >> + >> + if (pmic->subtype == PMI8998_SUBTYPE || pmic->subtype == PM660_SUBTYPE) { >> + ret = regmap_read(map, PMIC_FAB_ID, &pmic->fab_id); >> + if (ret < 0) >> + return ret; >> + } >> >> /* >> * In early versions of PM8941 and PM8226, the major revision number >> @@ -124,14 +102,14 @@ static void pmic_spmi_show_revid(struct regmap *map, struct device *dev) >> * Increment the major revision number here if the chip is an early >> * version of PM8941 or PM8226. >> */ >> - if ((subtype == PM8941_SUBTYPE || subtype == PM8226_SUBTYPE) && >> - major < 0x02) >> - major++; >> + if ((pmic->subtype == PM8941_SUBTYPE || pmic->subtype == PM8226_SUBTYPE) && >> + pmic->major < 0x02) >> + pmic->major++; >> >> - if (subtype == PM8110_SUBTYPE) >> - minor = rev2; >> + if (pmic->subtype == PM8110_SUBTYPE) >> + pmic->minor = pmic->rev2; >> >> - dev_dbg(dev, "%x: %s v%d.%d\n", subtype, name, major, minor); >> + return 0; >> } >> >> static const struct regmap_config spmi_regmap_config = { >> @@ -144,22 +122,38 @@ static const struct regmap_config spmi_regmap_config = { >> static int pmic_spmi_probe(struct spmi_device *sdev) >> { >> struct regmap *regmap; >> + struct qcom_spmi_pmic *pmic; >> >> regmap = devm_regmap_init_spmi_ext(sdev, &spmi_regmap_config); >> if (IS_ERR(regmap)) >> return PTR_ERR(regmap); >> >> + pmic = devm_kzalloc(&sdev->dev, sizeof(*pmic), GFP_KERNEL); >> + if (!pmic) >> + return -ENOMEM; > > Within the code visible here, why can't this just be on the stack? I allocated on the heap beacuse the data has to be read by other drivers (it's handed over in spmi_device_set_drvdata() below). I don't have a whole lot of C experience so please forgive the potentially ignorant questions - is it ok to allocate on the stack if the object needs to have a lifetime longer than the function? > >> + >> /* Only the first slave id for a PMIC contains this information */ >> - if (sdev->usid % 2 == 0) >> - pmic_spmi_show_revid(regmap, &sdev->dev); >> + if (sdev->usid % 2 == 0) { >> + pmic_spmi_load_revid(regmap, &sdev->dev, pmic); >> + spmi_device_set_drvdata(sdev, pmic); >> + qcom_pmic_print_info(&sdev->dev, pmic); >> + } >> >> return devm_of_platform_populate(&sdev->dev); >> } >> >> +static void pmic_spmi_remove(struct spmi_device *sdev) >> +{ >> + struct qcom_spmi_pmic *pmic = spmi_device_get_drvdata(sdev); >> + >> + kfree(pmic->name); > > Looks like this is cleaning up a managed allocation unless I'm missing something > which means you'll get a double free on remove. > >> +} >> + >> MODULE_DEVICE_TABLE(of, pmic_spmi_id_table); >> >> static struct spmi_driver pmic_spmi_driver = { >> .probe = pmic_spmi_probe, >> + .remove = pmic_spmi_remove, >> .driver = { >> .name = "pmic-spmi", >> .of_match_table = pmic_spmi_id_table, >> diff --git a/include/soc/qcom/qcom-pmic.h b/include/soc/qcom/qcom-pmic.h >> new file mode 100644 >> index 000000000000..59114988582d >> --- /dev/null >> +++ b/include/soc/qcom/qcom-pmic.h >> @@ -0,0 +1,63 @@ >> +/* SPDX-License-Identifier: GPL-2.0-only */ >> +/* Copyright (c) 2021 Linaro. All rights reserved. >> + * Copyright (c) 2021 Caleb Connolly <caleb.connolly@linaro.org> >> + */ >> + >> +#ifndef __QCOM_PMIC_H__ >> +#define __QCOM_PMIC_H__ >> + >> +#define COMMON_SUBTYPE 0x00 >> +#define PM8941_SUBTYPE 0x01 >> +#define PM8841_SUBTYPE 0x02 >> +#define PM8019_SUBTYPE 0x03 >> +#define PM8226_SUBTYPE 0x04 >> +#define PM8110_SUBTYPE 0x05 >> +#define PMA8084_SUBTYPE 0x06 >> +#define PMI8962_SUBTYPE 0x07 >> +#define PMD9635_SUBTYPE 0x08 >> +#define PM8994_SUBTYPE 0x09 >> +#define PMI8994_SUBTYPE 0x0a >> +#define PM8916_SUBTYPE 0x0b >> +#define PM8004_SUBTYPE 0x0c >> +#define PM8909_SUBTYPE 0x0d >> +#define PM8028_SUBTYPE 0x0e >> +#define PM8901_SUBTYPE 0x0f >> +#define PM8950_SUBTYPE 0x10 >> +#define PMI8950_SUBTYPE 0x11 >> +#define PM8998_SUBTYPE 0x14 >> +#define PMI8998_SUBTYPE 0x15 >> +#define PM8005_SUBTYPE 0x18 >> +#define PM660L_SUBTYPE 0x1A >> +#define PM660_SUBTYPE 0x1B >> +#define PM8150_SUBTYPE 0x1E >> +#define PM8150L_SUBTYPE 0x1f >> +#define PM8150B_SUBTYPE 0x20 >> +#define PMK8002_SUBTYPE 0x21 >> +#define PM8009_SUBTYPE 0x24 >> +#define PM8150C_SUBTYPE 0x26 >> +#define SMB2351_SUBTYPE 0x29 >> + >> +#define PMI8998_FAB_ID_SMIC 0x11 >> +#define PMI8998_FAB_ID_GF 0x30 >> + >> +#define PM660_FAB_ID_GF 0x0 >> +#define PM660_FAB_ID_TSMC 0x2 >> +#define PM660_FAB_ID_MX 0x3 >> + >> +struct qcom_spmi_pmic { >> + unsigned int type; >> + unsigned int subtype; >> + unsigned int major; >> + unsigned int minor; >> + unsigned int rev2; >> + unsigned int fab_id; >> + const char *name; >> +}; >> + > struct device; > >> +static inline void qcom_pmic_print_info(struct device *dev, struct qcom_spmi_pmic *pmic) >> +{ > > Need an include to get you access to dev_info() and possibly so > > Headers should stand and build on their own including any inline functions they > contain. > > >> + dev_info(dev, "%x: %s v%d.%d\n", >> + pmic->subtype, pmic->name, pmic->major, pmic->minor); >> +} >> + >> +#endif /* __QCOM_PMIC_H__ */ >
-- Kind Regards, Caleb (they/them)
| |