Re: [PATCH 9/9] soc/qcom: Add REVID driver

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

On Sun, Jul 26, 2020 at 01:12:06PM +0200, Konrad Dybcio wrote:
> From: Xiaozhe Shi <xiaozhes@codeaurora.org>
> 
> Add the REVID device driver. The REVID driver will print out the PMIC
> revision at probe time.

Why do we need this noise in the kernel log?

> --- /dev/null
> +++ b/drivers/soc/qcom/qpnp-revid.c
> @@ -0,0 +1,288 @@
> +/* Copyright (c) 2013-2019, The Linux Foundation. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */

You can drop the GPL boilerplate text and add a proper SPDX line at the
top.

Didn't checkpatch ask for that?

> +
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/spmi.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/err.h>
> +#include <linux/qpnp/qpnp-revid.h>
> +#include <linux/of.h>
> +
> +#define REVID_REVISION1	0x0
> +#define REVID_REVISION2	0x1
> +#define REVID_REVISION3	0x2
> +#define REVID_REVISION4	0x3
> +#define REVID_TYPE	0x4
> +#define REVID_SUBTYPE	0x5
> +#define REVID_STATUS1	0x8
> +#define REVID_SPARE_0	0x60
> +#define REVID_TP_REV	0xf1
> +#define REVID_FAB_ID	0xf2
> +
> +#define QPNP_REVID_DEV_NAME "qcom,qpnp-revid"
> +
> +static const char *const pmic_names[] = {
> +	[0] =	"Unknown PMIC",
> +	[PM8941_SUBTYPE] = "PM8941",
> +	[PM8841_SUBTYPE] = "PM8841",
> +	[PM8019_SUBTYPE] = "PM8019",
> +	[PM8226_SUBTYPE] = "PM8226",
> +	[PM8110_SUBTYPE] = "PM8110",
> +	[PMA8084_SUBTYPE] = "PMA8084",
> +	[PMI8962_SUBTYPE] = "PMI8962",
> +	[PMD9635_SUBTYPE] = "PMD9635",
> +	[PM8994_SUBTYPE] = "PM8994",
> +	[PMI8994_SUBTYPE] = "PMI8994",
> +	[PM8916_SUBTYPE] = "PM8916",
> +	[PM8004_SUBTYPE] = "PM8004",
> +	[PM8909_SUBTYPE] = "PM8909",
> +	[PM2433_SUBTYPE] = "PM2433",
> +	[PMD9655_SUBTYPE] = "PMD9655",
> +	[PM8950_SUBTYPE] = "PM8950",
> +	[PMI8950_SUBTYPE] = "PMI8950",
> +	[PMK8001_SUBTYPE] = "PMK8001",
> +	[PMI8996_SUBTYPE] = "PMI8996",
> +	[PM8998_SUBTYPE] = "PM8998",
> +	[PMI8998_SUBTYPE] = "PMI8998",
> +	[PM8005_SUBTYPE] = "PM8005",
> +	[PM8937_SUBTYPE] = "PM8937",
> +	[PM660L_SUBTYPE] = "PM660L",
> +	[PM660_SUBTYPE] = "PM660",
> +	[PMI632_SUBTYPE] = "PMI632",
> +	[PMI8937_SUBTYPE] = "PMI8937",
> +	[PM8150_SUBTYPE] = "PM8150",
> +	[PM8150B_SUBTYPE] = "PM8150B",
> +	[PM8150L_SUBTYPE] = "PM8150L",
> +	[PM6150_SUBTYPE] = "PM6150",
> +	[PM8150A_SUBTYPE] = "PM8150A",
> +	[PME9205_SUBTYPE] = "PME9205",
> +	[PM6125_SUBTYPE] = "PM6125",
> +	[PM8008_SUBTYPE] = "PM8008",
> +	[SMB1355_SUBTYPE] = "SMB1355",
> +	[SMB1390_SUBTYPE] = "SMB1390",
> +};
> +
> +struct revid_chip {
> +	struct list_head	link;
> +	struct device_node	*dev_node;
> +	struct pmic_revid_data	data;
> +};
> +
> +static LIST_HEAD(revid_chips);
> +static DEFINE_MUTEX(revid_chips_lock);
> +
> +static const struct of_device_id qpnp_revid_match_table[] = {
> +	{ .compatible = QPNP_REVID_DEV_NAME },
> +	{}
> +};
> +
> +static u8 qpnp_read_byte(struct regmap *regmap, u16 addr)
> +{
> +	int rc;
> +	int val;
> +
> +	rc = regmap_read(regmap, addr, &val);
> +	if (rc) {
> +		pr_err("read failed rc=%d\n", rc);

Drivers should always use dev_err() and friends, as you have access to a
struct device * always.  Please fix up the driver here to use that api
instead, no pr_* should be needed at all.

> +		return 0;
> +	}
> +	return (u8)val;
> +}
> +
> +/**
> + * get_revid_data - Return the revision information of PMIC
> + * @dev_node: Pointer to the revid peripheral of the PMIC for which
> + *		revision information is seeked
> + *
> + * CONTEXT: Should be called in non atomic context
> + *
> + * RETURNS: pointer to struct pmic_revid_data filled with the information
> + *		about the PMIC revision
> + */
> +struct pmic_revid_data *get_revid_data(struct device_node *dev_node)
> +{
> +	struct revid_chip *revid_chip;
> +
> +	if (!dev_node)
> +		return ERR_PTR(-EINVAL);
> +
> +	mutex_lock(&revid_chips_lock);
> +	list_for_each_entry(revid_chip, &revid_chips, link) {
> +		if (dev_node == revid_chip->dev_node) {
> +			mutex_unlock(&revid_chips_lock);
> +			return &revid_chip->data;
> +		}
> +	}
> +	mutex_unlock(&revid_chips_lock);
> +	return ERR_PTR(-EINVAL);
> +}
> +EXPORT_SYMBOL(get_revid_data);

Horrible global symbol name.  Who calls this?  This is the last patch in
the series, so if there is no user for this, please don't export it.

> +
> +#define PM8941_PERIPHERAL_SUBTYPE	0x01
> +#define PM8226_PERIPHERAL_SUBTYPE	0x04
> +#define PMD9655_PERIPHERAL_SUBTYPE	0x0F
> +#define PMI8950_PERIPHERAL_SUBTYPE	0x11
> +#define PMI8937_PERIPHERAL_SUBTYPE	0x37
> +static size_t build_pmic_string(char *buf, size_t n, int sid,
> +		u8 subtype, u8 rev1, u8 rev2, u8 rev3, u8 rev4)
> +{
> +	size_t pos = 0;
> +	/*
> +	 * 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 (((int)subtype == PM8941_PERIPHERAL_SUBTYPE
> +			|| (int)subtype == PM8226_PERIPHERAL_SUBTYPE)
> +			&& rev4 < 0x02)
> +		rev4++;
> +
> +	pos += snprintf(buf + pos, n - pos, "PMIC@SID%d", sid);
> +	if (subtype >= ARRAY_SIZE(pmic_names) || subtype == 0)
> +		pos += snprintf(buf + pos, n - pos, ": %s (subtype: 0x%02X)",
> +				pmic_names[0], subtype);
> +	else
> +		pos += snprintf(buf + pos, n - pos, ": %s",
> +				pmic_names[subtype]);
> +	pos += snprintf(buf + pos, n - pos, " v%d.%d", rev4, rev3);
> +	if (rev2 || rev1)
> +		pos += snprintf(buf + pos, n - pos, ".%d", rev2);
> +	if (rev1)
> +		pos += snprintf(buf + pos, n - pos, ".%d", rev1);
> +	return pos;
> +}
> +
> +#define PMIC_PERIPHERAL_TYPE		0x51
> +#define PMIC_STRING_MAXLENGTH		80
> +static int qpnp_revid_probe(struct platform_device *pdev)
> +{
> +	u8 rev1, rev2, rev3, rev4, pmic_type, pmic_subtype, pmic_status;
> +	u8 option1, option2, option3, option4, spare0;
> +	unsigned int base;
> +	int rc, fab_id, tp_rev;
> +	char pmic_string[PMIC_STRING_MAXLENGTH] = {'\0'};
> +	struct revid_chip *revid_chip;
> +	struct regmap *regmap;
> +
> +	regmap = dev_get_regmap(pdev->dev.parent, NULL);
> +	if (!regmap) {
> +		dev_err(&pdev->dev, "Couldn't get parent's regmap\n");
> +		return -EINVAL;
> +	}
> +
> +	rc = of_property_read_u32(pdev->dev.of_node, "reg", &base);
> +	if (rc < 0) {
> +		dev_err(&pdev->dev,
> +			"Couldn't find reg in node = %s rc = %d\n",
> +			pdev->dev.of_node->full_name, rc);
> +		return rc;
> +	}
> +	pmic_type = qpnp_read_byte(regmap, base + REVID_TYPE);
> +	if (pmic_type != PMIC_PERIPHERAL_TYPE) {
> +		pr_err("Invalid REVID peripheral type: %02X\n", pmic_type);
> +		return -EINVAL;
> +	}
> +
> +	rev1 = qpnp_read_byte(regmap, base + REVID_REVISION1);
> +	rev2 = qpnp_read_byte(regmap, base + REVID_REVISION2);
> +	rev3 = qpnp_read_byte(regmap, base + REVID_REVISION3);
> +	rev4 = qpnp_read_byte(regmap, base + REVID_REVISION4);
> +
> +	pmic_subtype = qpnp_read_byte(regmap, base + REVID_SUBTYPE);
> +	if (pmic_subtype != PMD9655_PERIPHERAL_SUBTYPE)
> +		pmic_status = qpnp_read_byte(regmap, base + REVID_STATUS1);
> +	else
> +		pmic_status = 0;
> +
> +	/* special case for PMI8937 */
> +	if (pmic_subtype == PMI8950_PERIPHERAL_SUBTYPE) {
> +		/* read spare register */
> +		spare0 = qpnp_read_byte(regmap, base + REVID_SPARE_0);
> +		if (spare0)
> +			pmic_subtype = PMI8937_PERIPHERAL_SUBTYPE;
> +	}
> +
> +	if (of_property_read_bool(pdev->dev.of_node, "qcom,fab-id-valid"))
> +		fab_id = qpnp_read_byte(regmap, base + REVID_FAB_ID);
> +	else
> +		fab_id = -EINVAL;
> +
> +	if (of_property_read_bool(pdev->dev.of_node, "qcom,tp-rev-valid"))
> +		tp_rev = qpnp_read_byte(regmap, base + REVID_TP_REV);
> +	else
> +		tp_rev = -EINVAL;
> +
> +	revid_chip = devm_kzalloc(&pdev->dev, sizeof(struct revid_chip),
> +						GFP_KERNEL);
> +	if (!revid_chip)
> +		return -ENOMEM;
> +
> +	revid_chip->dev_node = pdev->dev.of_node;
> +	revid_chip->data.rev1 = rev1;
> +	revid_chip->data.rev2 = rev2;
> +	revid_chip->data.rev3 = rev3;
> +	revid_chip->data.rev4 = rev4;
> +	revid_chip->data.pmic_subtype = pmic_subtype;
> +	revid_chip->data.pmic_type = pmic_type;
> +	revid_chip->data.fab_id = fab_id;
> +	revid_chip->data.tp_rev = tp_rev;
> +
> +	if (pmic_subtype < ARRAY_SIZE(pmic_names))
> +		revid_chip->data.pmic_name = pmic_names[pmic_subtype];
> +	else
> +		revid_chip->data.pmic_name = pmic_names[0];
> +
> +	mutex_lock(&revid_chips_lock);
> +	list_add(&revid_chip->link, &revid_chips);
> +	mutex_unlock(&revid_chips_lock);
> +
> +	option1 = pmic_status & 0x3;
> +	option2 = (pmic_status >> 2) & 0x3;
> +	option3 = (pmic_status >> 4) & 0x3;
> +	option4 = (pmic_status >> 6) & 0x3;
> +
> +	build_pmic_string(pmic_string, PMIC_STRING_MAXLENGTH,
> +			  to_spmi_device(pdev->dev.parent)->usid,
> +			pmic_subtype, rev1, rev2, rev3, rev4);
> +	pr_info("%s options: %d, %d, %d, %d\n",
> +			pmic_string, option1, option2, option3, option4);

Again, dev_info().

But really, why?  Who uses this information?  Drivers should be quiet if
all is working properly, no need to log anything.

> +	return 0;
> +}
> +
> +static struct platform_driver qpnp_revid_driver = {
> +	.probe	= qpnp_revid_probe,
> +	.driver	= {
> +		.name		= QPNP_REVID_DEV_NAME,
> +		.owner		= THIS_MODULE,
> +		.of_match_table	= qpnp_revid_match_table,
> +	},
> +};
> +
> +static int __init qpnp_revid_init(void)
> +{
> +	return platform_driver_register(&qpnp_revid_driver);
> +}
> +
> +static void __exit qpnp_revid_exit(void)
> +{
> +	return platform_driver_unregister(&qpnp_revid_driver);
> +}
> +
> +subsys_initcall(qpnp_revid_init);

Why subsys_initcall() if no one uses the function in this module?

Can't you just use the "normal" platform module init macros?

> +module_exit(qpnp_revid_exit);
> +
> +MODULE_DESCRIPTION("QPNP REVID DRIVER");
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("platform:" QPNP_REVID_DEV_NAME);
> diff --git a/include/linux/qpnp/qpnp-revid.h b/include/linux/qpnp/qpnp-revid.h
> new file mode 100644
> index 000000000000..0fbdd528d204
> --- /dev/null
> +++ b/include/linux/qpnp/qpnp-revid.h

Why do you need a .h file in the include directory if only a single .c
file needs it?  Just put that info in the .c file itself.


> @@ -0,0 +1,369 @@
> +/* Copyright (c) 2013-2019, The Linux Foundation. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#ifndef __QPNP_REVID
> +#define __QPNP_REVID
> +
> +/* Common TYPE for all PMICs */
> +#define PMIC_TYPE		0x51
> +
> +/* PM8994 */
> +#define PM8941_SUBTYPE		0x01
> +
> +#define PM8941_V1P0_REV1	0x00
> +#define PM8941_V1P0_REV2	0x00
> +#define PM8941_V1P0_REV3	0x00
> +#define PM8941_V1P0_REV4	0x01
> +
> +#define PM8941_V2P0_REV1	0x00
> +#define PM8941_V2P0_REV2	0x00
> +#define PM8941_V2P0_REV3	0x00
> +#define PM8941_V2P0_REV4	0x01
> +
> +#define PM8941_V3P0_REV1	0x00
> +#define PM8941_V3P0_REV2	0x00
> +#define PM8941_V3P0_REV3	0x00
> +#define PM8941_V3P0_REV4	0x03
> +
> +#define PM8941_V3P1_REV1	0x00
> +#define PM8941_V3P1_REV2	0x00
> +#define PM8941_V3P1_REV3	0x01
> +#define PM8941_V3P1_REV4	0x03
> +
> +/* PM8841 */
> +#define PM8841_SUBTYPE		0x02
> +
> +/* PM8019 */
> +#define PM8019_SUBTYPE		0x03
> +
> +/* PM8226 */
> +#define PM8226_SUBTYPE		0x04
> +
> +#define PM8226_V2P2_REV1	0x00
> +#define PM8226_V2P2_REV2	0x00
> +#define PM8226_V2P2_REV3	0x02
> +#define PM8226_V2P2_REV4	0x02
> +
> +#define PM8226_V2P1_REV1	0x00
> +#define PM8226_V2P1_REV2	0x00
> +#define PM8226_V2P1_REV3	0x01
> +#define PM8226_V2P1_REV4	0x02
> +
> +#define PM8226_V2P0_REV1	0x00
> +#define PM8226_V2P0_REV2	0x00
> +#define PM8226_V2P0_REV3	0x00
> +#define PM8226_V2P0_REV4	0x02
> +
> +#define PM8226_V1P0_REV1	0x00
> +#define PM8226_V1P0_REV2	0x00
> +#define PM8226_V1P0_REV3	0x00
> +#define PM8226_V1P0_REV4	0x00
> +
> +/* PM8110 */
> +#define PM8110_SUBTYPE		0x05
> +
> +#define PM8110_V1P0_REV1	0x00
> +#define PM8110_V1P0_REV2	0x00
> +#define PM8110_V1P0_REV3	0x00
> +#define PM8110_V1P0_REV4	0x01
> +
> +#define PM8110_V1P1_REV1	0x00
> +#define PM8110_V1P1_REV2	0x01
> +#define PM8110_V1P1_REV3	0x00
> +#define PM8110_V1P1_REV4	0x01
> +
> +#define PM8110_V1P3_REV1	0x00
> +#define PM8110_V1P3_REV2	0x03
> +#define PM8110_V1P3_REV3	0x00
> +#define PM8110_V1P3_REV4	0x01
> +
> +#define PM8110_V2P0_REV1	0x00
> +#define PM8110_V2P0_REV2	0x00
> +#define PM8110_V2P0_REV3	0x00
> +#define PM8110_V2P0_REV4	0x02
> +
> +/* PMA8084 */
> +#define PMA8084_SUBTYPE		0x06
> +
> +/* PMI8962 */
> +#define PMI8962_SUBTYPE		0x07
> +
> +/* PMD9635 */
> +#define PMD9635_SUBTYPE		0x08
> +/* PM8994 */
> +#define PM8994_SUBTYPE		0x09
> +
> +/* PMI8994 */
> +#define PMI8994_TYPE		0x51
> +#define PMI8994_SUBTYPE		0x0A
> +
> +#define PMI8994_V1P0_REV1	0x00
> +#define PMI8994_V1P0_REV2	0x00
> +#define PMI8994_V1P0_REV3	0x00
> +#define PMI8994_V1P0_REV4	0x01
> +
> +#define PMI8994_V2P0_REV1	0x00
> +#define PMI8994_V2P0_REV2	0x00
> +#define PMI8994_V2P0_REV3	0x00
> +#define PMI8994_V2P0_REV4	0x02
> +
> +/* PM8916 */
> +#define PM8916_SUBTYPE		0x0B
> +
> +#define PM8916_V1P0_REV1	0x00
> +#define PM8916_V1P0_REV2	0x00
> +#define PM8916_V1P0_REV3	0x00
> +#define PM8916_V1P0_REV4	0x01
> +
> +#define PM8916_V1P1_REV1	0x00
> +#define PM8916_V1P1_REV2	0x00
> +#define PM8916_V1P1_REV3	0x01
> +#define PM8916_V1P1_REV4	0x01
> +
> +#define PM8916_V2P0_REV1	0x00
> +#define PM8916_V2P0_REV2	0x00
> +#define PM8916_V2P0_REV3	0x00
> +#define PM8916_V2P0_REV4	0x02
> +
> +/* PM8004 */
> +#define PM8004_SUBTYPE		0x0C
> +
> +/* PM8909 */
> +#define PM8909_SUBTYPE		0x0D
> +
> +#define PM8909_V1P0_REV1	0x00
> +#define PM8909_V1P0_REV2	0x00
> +#define PM8909_V1P0_REV3	0x00
> +#define PM8909_V1P0_REV4	0x01
> +
> +#define PM8909_V1P1_REV1	0x00
> +#define PM8909_V1P1_REV2	0x00
> +#define PM8909_V1P1_REV3	0x01
> +#define PM8909_V1P1_REV4	0x01
> +
> +/* PM2433 */
> +#define PM2433_SUBTYPE		0x0E
> +
> +/* PMD9655 */
> +#define PMD9655_SUBTYPE		0x0F
> +
> +/* PM8950 */
> +#define PM8950_SUBTYPE		0x10
> +#define PM8950_V1P0_REV4	0x01
> +
> +#define PM8950_V2P0_REV4	0x02
> +
> +/* PMI8950 */
> +#define PMI8950_SUBTYPE		0x11
> +
> +/* PMK8001 */
> +#define PMK8001_SUBTYPE		0x12
> +
> +/* PMI8996 */
> +#define PMI8996_SUBTYPE		0x13
> +
> +/* PM8998 */
> +#define PM8998_SUBTYPE	0x14
> +
> +/* PMI8998 */
> +#define PMI8998_SUBTYPE	0x15
> +
> +/* PM660 */
> +#define PM660L_SUBTYPE	0x1A
> +#define PM660_SUBTYPE	0x1B
> +
> +/* PM8150 */
> +#define PM8150_SUBTYPE		0x1E
> +#define PM8150L_SUBTYPE		0x1F
> +#define PM8150B_SUBTYPE		0x20
> +#define PM8150A_SUBTYPE		0x27
> +
> +/* PM6150 SUBTYPE */
> +#define PM6150_SUBTYPE		0x28
> +#define PM6150L_SUBTYPE		0x1F
> +
> +/* PME9205 SUBTYPE */
> +#define PME9205_SUBTYPE		0x24
> +
> +/* PM6125 SUBTYPE */
> +#define PM6125_SUBTYPE		0x2D
> +
> +/* PMI632 */
> +#define PMI632_SUBTYPE	0x25
> +
> +/* PM8008 SUBTYPE */
> +#define PM8008_SUBTYPE	0x2C
> +
> +/* PMI8998 REV_ID */
> +#define PMI8998_V1P0_REV1	0x00
> +#define PMI8998_V1P0_REV2	0x00
> +#define PMI8998_V1P0_REV3	0x00
> +#define PMI8998_V1P0_REV4	0x01
> +
> +#define PMI8998_V1P1_REV1	0x00
> +#define PMI8998_V1P1_REV2	0x00
> +#define PMI8998_V1P1_REV3	0x01
> +#define PMI8998_V1P1_REV4	0x01
> +
> +#define PMI8998_V2P0_REV1	0x00
> +#define PMI8998_V2P0_REV2	0x00
> +#define PMI8998_V2P0_REV3	0x00
> +#define PMI8998_V2P0_REV4	0x02
> +
> +/* PM660 REV_ID */
> +#define PM660_V1P0_REV1		0x00
> +#define PM660_V1P0_REV2		0x00
> +#define PM660_V1P0_REV3		0x00
> +#define PM660_V1P0_REV4		0x01
> +
> +#define PM660_V1P1_REV1		0x00
> +#define PM660_V1P1_REV2		0x00
> +#define PM660_V1P1_REV3		0x01
> +#define PM660_V1P1_REV4		0x01
> +
> +/* PM660L REV_ID */
> +#define PM660L_V1P1_REV1	0x00
> +#define PM660L_V1P1_REV2	0x00
> +#define PM660L_V1P1_REV3	0x01
> +#define PM660L_V1P1_REV4	0x01
> +
> +#define PM660L_V2P0_REV1	0x00
> +#define PM660L_V2P0_REV2	0x00
> +#define PM660L_V2P0_REV3	0x00
> +#define PM660L_V2P0_REV4	0x02
> +
> +/* PMI632 REV_ID */
> +#define PMI632_V1P0_REV1	0x00
> +#define PMI632_V1P0_REV2	0x00
> +#define PMI632_V1P0_REV3	0x00
> +#define PMI632_V1P0_REV4	0x01
> +
> +/* PM8150B_REV_ID */
> +#define PM8150B_V1P0_REV1	0x00
> +#define PM8150B_V1P0_REV2	0x00
> +#define PM8150B_V1P0_REV3	0x00
> +#define PM8150B_V1P0_REV4	0x01
> +
> +#define PM8150B_V2P0_REV1	0x00
> +#define PM8150B_V2P0_REV2	0x00
> +#define PM8150B_V2P0_REV3	0x00
> +#define PM8150B_V2P0_REV4	0x02
> +
> +/* PM8150L_REV_ID */
> +#define PM8150L_V1P0_REV1	0x00
> +#define PM8150L_V1P0_REV2	0x00
> +#define PM8150L_V1P0_REV3	0x00
> +#define PM8150L_V1P0_REV4	0x01
> +
> +#define PM8150L_V2P0_REV1	0x00
> +#define PM8150L_V2P0_REV2	0x00
> +#define PM8150L_V2P0_REV3	0x00
> +#define PM8150L_V2P0_REV4	0x02
> +
> +#define PM8150L_V3P0_REV1	0x00
> +#define PM8150L_V3P0_REV2	0x00
> +#define PM8150L_V3P0_REV3	0x00
> +#define PM8150L_V3P0_REV4	0x03
> +
> +/* PM8150A_REV_ID */
> +#define PM8150A_V1P0_REV1	0x00
> +#define PM8150A_V1P0_REV2	0x00
> +#define PM8150A_V1P0_REV3	0x00
> +#define PM8150A_V1P0_REV4	0x01
> +
> +#define PM8150A_V2P0_REV1	0x00
> +#define PM8150A_V2P0_REV2	0x00
> +#define PM8150A_V2P0_REV3	0x00
> +#define PM8150A_V2P0_REV4	0x02
> +
> +/* PME9205_REV_ID */
> +#define PME9205_V1P0_REV1	0x00
> +#define PME9205_V1P0_REV2	0x00
> +#define PME9205_V1P0_REV3	0x00
> +#define PME9205_V1P0_REV4	0x01
> +
> +#define PME9205_V2P0_REV1	0x00
> +#define PME9205_V2P0_REV2	0x00
> +#define PME9205_V2P0_REV3	0x00
> +#define PME9205_V2P0_REV4	0x02
> +
> +/* PM6150_REV_ID */
> +#define PM6150_V1P0_REV1	0x00
> +#define PM6150_V1P0_REV2	0x00
> +#define PM6150_V1P0_REV3	0x00
> +#define PM6150_V1P0_REV4	0x01
> +
> +#define PM6150_V1P1_REV1	0x00
> +#define PM6150_V1P1_REV2	0x00
> +#define PM6150_V1P1_REV3	0x01
> +#define PM6150_V1P1_REV4	0x01
> +
> +#define PM6150_V2P0_REV1	0x00
> +#define PM6150_V2P0_REV2	0x00
> +#define PM6150_V2P0_REV3	0x00
> +#define PM6150_V2P0_REV4	0x02
> +
> +/* PM6125_REV_ID */
> +#define PM6125_V1P0_REV1	0x00
> +#define PM6125_V1P0_REV2	0x00
> +#define PM6125_V1P0_REV3	0x00
> +#define PM6125_V1P0_REV4	0x01
> +
> +/* PMI8998 FAB_ID */
> +#define PMI8998_FAB_ID_SMIC	0x11
> +#define PMI8998_FAB_ID_GF	0x30
> +
> +/* PM660 FAB_ID */
> +#define PM660_FAB_ID_GF		0x0
> +#define PM660_FAB_ID_TSMC	0x2
> +#define PM660_FAB_ID_MX		0x3
> +
> +/* PM8005 */
> +#define PM8005_SUBTYPE		0x18
> +
> +/* PM8937 */
> +#define PM8937_SUBTYPE		0x19
> +
> +/* PMI8937 */
> +#define PMI8937_SUBTYPE		0x37
> +
> +/* SMB1390 */
> +#define SMB1390_SUBTYPE		0x23
> +
> +/* SMB1381 */
> +#define SMB1381_SUBTYPE		0x17
> +
> +/* SMB1355 */
> +#define SMB1355_SUBTYPE		0x1C
> +
> +struct pmic_revid_data {
> +	u8		rev1;
> +	u8		rev2;
> +	u8		rev3;
> +	u8		rev4;
> +	u8		pmic_type;
> +	u8		pmic_subtype;
> +	const char	*pmic_name;
> +	int		fab_id;
> +	int		tp_rev;

int and not u32 or u64?

But again, who uses this module?  If it's only good for a single line in
the kernel log, that feels like a huge waste to me.

thanks,

greg k-h