Re: [PATCH 1/7] mfd: qcom-spmi-pmic: expose the PMIC revid information to clients

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Sat, 11 Dec 2021 02:22:18 +0000
Caleb Connolly <caleb.connolly@xxxxxxxxxx> wrote:

> From: Caleb Connolly <caleb.connolly@xxxxxxxxxx>
> 
> 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@xxxxxxxxxx>

Hi Calib,

Comments inline.

Jonathan

> ---
>  drivers/mfd/qcom-spmi-pmic.c | 103 +++++++++++++++++------------------
>  include/soc/qcom/qcom-pmic.h |  58 ++++++++++++++++++++
>  2 files changed, 108 insertions(+), 53 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..78530c1aca4b 100644
> --- a/drivers/mfd/qcom-spmi-pmic.c
> +++ b/drivers/mfd/qcom-spmi-pmic.c
> @@ -3,51 +3,25 @@
>   * 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/string.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,57 +55,64 @@ static const struct of_device_id pmic_spmi_id_table[] = {
>  	{ }
>  };
>  
> -static void pmic_spmi_show_revid(struct regmap *map, struct device *dev)
> +static void 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);

Given this isn't just for a debug print any more, I'd suggest you should
do proper error handling.  If these values can't be read fail the probe.

>  	if (ret < 0)
>  		return;
>  
> -	if (type != PMIC_TYPE_VALUE)
> +	if (pmic->type != PMIC_TYPE_VALUE)
>  		return;
>  
> -	ret = regmap_read(map, PMIC_SUBTYPE, &subtype);
> +	ret = regmap_read(map, PMIC_SUBTYPE, &pmic->subtype);
>  	if (ret < 0)
>  		return;
>  
>  	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;
> +	if (i != ARRAY_SIZE(pmic_spmi_id_table)) {
> +		pmic->name = kmalloc(strlen(pmic_spmi_id_table[i].compatible),
> +			GFP_KERNEL);
> +		strncpy(pmic->name, pmic_spmi_id_table[i].compatible,
> +			strlen(pmic_spmi_id_table[i].compatible));

Under what circumstances could you not just use a pointer to the string in the
table for this?  Is there a life time issue?  Given you free the string in the
remove it seems unlikely.

If so why not use use kstrdup()


> +	}
>  
> -	ret = regmap_read(map, PMIC_REV2, &rev2);
> +	ret = regmap_read(map, PMIC_REV2, &pmic->rev2);
>  	if (ret < 0)
>  		return;
>  
> -	ret = regmap_read(map, PMIC_REV3, &minor);
> +	ret = regmap_read(map, PMIC_REV3, &pmic->minor);
>  	if (ret < 0)
>  		return;
>  
> -	ret = regmap_read(map, PMIC_REV4, &major);
> +	ret = regmap_read(map, PMIC_REV4, &pmic->major);
>  	if (ret < 0)
>  		return;
>  
> +	if (pmic->subtype == PMI8998_SUBTYPE || pmic->subtype == PM660_SUBTYPE) {
> +		ret = regmap_read(map, PMIC_FAB_ID, &pmic->fab_id);
> +		if (ret < 0)
> +			return;
> +	}
> +
>  	/*
>  	 * In early versions of PM8941 and PM8226, the major revision number
>  	 * started incrementing from 0 (eg 0 = v1.0, 1 = v2.0).
>  	 * 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 (subtype == PM8110_SUBTYPE)
> -		minor = rev2;
> +	if ((pmic->subtype == PM8941_SUBTYPE || pmic->subtype == PM8226_SUBTYPE) &&
> +	    pmic->major < 0x02)
> +		pmic->major++;
>  
> -	dev_dbg(dev, "%x: %s v%d.%d\n", subtype, name, major, minor);
> +	if (pmic->subtype == PM8110_SUBTYPE)
> +		pmic->minor = pmic->rev2;
>  }
>  
>  static const struct regmap_config spmi_regmap_config = {
> @@ -144,22 +125,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;
> +
>  	/* 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);
> +}
> +
>  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..bf87a4f2de6a
> --- /dev/null
> +++ b/include/soc/qcom/qcom-pmic.h
> @@ -0,0 +1,58 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/* Copyright (c) 2021 Linaro. All rights reserved.
> + * Copyright (c) 2021 Caleb Connolly <caleb.connolly@xxxxxxxxxx>
> + */
> +
> +#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;
> +	char *name;
> +};
> +
> +inline void qcom_pmic_print_info(struct device *dev, struct qcom_spmi_pmic *pmic)
> +{
> +	dev_info(dev, "%x: %s v%d.%d\n",
> +		pmic->subtype, pmic->name, pmic->major, pmic->minor);
> +}




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [Linux for Sparc]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux