Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 17 Aug 2023 18:02:23 +0100
From:   Lee Jones <lee@kernel.org>
To:     werneazc@gmail.com
Cc:     lgirdwood@gmail.com, broonie@kernel.org,
        linux-kernel@vger.kernel.org,
        Andre Werner <andre.werner@systec-electronic.com>
Subject: Re: [PATCH 1/2] mfd: (tps65086): Read DEVICE ID register 1 from
 device
Message-ID: <20230817170223.GI986605@google.com>
References: <20230809101429.7885-1-andre.werner@systec-electronic.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20230809101429.7885-1-andre.werner@systec-electronic.com>
Precedence: bulk

On Wed, 09 Aug 2023, werneazc@gmail.com wrote:

> From: Andre Werner <andre.werner@systec-electronic.com>
> 
> This commit prepares a following commit for the regulator part of the MFD.
> The driver should support different device chips that differ in their
> register definitions, for instance to control LDOA1 and SWB2.
> So it is necessary to use a dedicated regulator description for a
> specific device variant. Thus, the content from DEVICEID Register 1 is
> used to choose a dedicated configuration between the different device
> variants.
> 
> Signed-off-by: Andre Werner <andre.werner@systec-electronic.com>
> ---
>  drivers/mfd/tps65086.c       | 37 ++++++++++++++++++++++++++++++------
>  include/linux/mfd/tps65086.h | 27 ++++++++++++++++++++------
>  2 files changed, 52 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/mfd/tps65086.c b/drivers/mfd/tps65086.c
> index 6a21000aad4a..38f8572c265e 100644
> --- a/drivers/mfd/tps65086.c
> +++ b/drivers/mfd/tps65086.c
> @@ -64,7 +64,7 @@ MODULE_DEVICE_TABLE(of, tps65086_of_match_table);
>  static int tps65086_probe(struct i2c_client *client)
>  {
>  	struct tps65086 *tps;
> -	unsigned int version;
> +	unsigned int version, id;
>  	int ret;
>  
>  	tps = devm_kzalloc(&client->dev, sizeof(*tps), GFP_KERNEL);
> @@ -81,16 +81,41 @@ static int tps65086_probe(struct i2c_client *client)
>  		return PTR_ERR(tps->regmap);
>  	}
>  
> -	ret = regmap_read(tps->regmap, TPS65086_DEVICEID, &version);
> +	ret = regmap_read(tps->regmap, TPS65086_DEVICEID1, &id);
>  	if (ret) {
> -		dev_err(tps->dev, "Failed to read revision register\n");
> +		dev_err(tps->dev, "Failed to read revision register 1\n");
> +		return ret;
> +	}
> +
> +	/* Store device ID to load regulator configuration that fit to IC variant */
> +	switch (id) {
> +	case TPS6508640_ID:
> +		tps->chip_id = TPS6508640;

Why not use the meaningful TPS6508640_ID for the chip_id instead of an
arbitrary enum?

> +		break;
> +	case TPS65086401_ID:
> +		tps->chip_id = TPS65086401;
> +		break;
> +	case TPS6508641_ID:
> +		tps->chip_id = TPS6508641;
> +		break;
> +	case TPS65086470_ID:
> +		tps->chip_id = TPS65086470;
> +		break;
> +	default:
> +		dev_err(tps->dev, "Unknown device ID. Cannot determine regulator config.\n");
> +		return -ENODEV;
> +	}
> +
> +	ret = regmap_read(tps->regmap, TPS65086_DEVICEID2, &version);
> +	if (ret) {
> +		dev_err(tps->dev, "Failed to read revision register 2\n");
>  		return ret;
>  	}
>  
>  	dev_info(tps->dev, "Device: TPS65086%01lX, OTP: %c, Rev: %ld\n",
> -		 (version & TPS65086_DEVICEID_PART_MASK),
> -		 (char)((version & TPS65086_DEVICEID_OTP_MASK) >> 4) + 'A',
> -		 (version & TPS65086_DEVICEID_REV_MASK) >> 6);
> +		 (version & TPS65086_DEVICEID2_PART_MASK),
> +		 (char)((version & TPS65086_DEVICEID2_OTP_MASK) >> 4) + 'A',
> +		 (version & TPS65086_DEVICEID2_REV_MASK) >> 6);
>  
>  	if (tps->irq > 0) {
>  		ret = regmap_add_irq_chip(tps->regmap, tps->irq, IRQF_ONESHOT, 0,
> diff --git a/include/linux/mfd/tps65086.h b/include/linux/mfd/tps65086.h
> index 16f87cccc003..88df344b38df 100644
> --- a/include/linux/mfd/tps65086.h
> +++ b/include/linux/mfd/tps65086.h
> @@ -13,8 +13,9 @@
>  #include <linux/regmap.h>
>  
>  /* List of registers for TPS65086 */
> -#define TPS65086_DEVICEID		0x01
> -#define TPS65086_IRQ			0x02
> +#define TPS65086_DEVICEID1		0x00
> +#define TPS65086_DEVICEID2		0x01
> +#define TPS65086_IRQ		0x02
>  #define TPS65086_IRQ_MASK		0x03
>  #define TPS65086_PMICSTAT		0x04
>  #define TPS65086_SHUTDNSRC		0x05
> @@ -75,16 +76,29 @@
>  #define TPS65086_IRQ_SHUTDN_MASK	BIT(3)
>  #define TPS65086_IRQ_FAULT_MASK		BIT(7)
>  
> -/* DEVICEID Register field definitions */
> -#define TPS65086_DEVICEID_PART_MASK	GENMASK(3, 0)
> -#define TPS65086_DEVICEID_OTP_MASK	GENMASK(5, 4)
> -#define TPS65086_DEVICEID_REV_MASK	GENMASK(7, 6)
> +/* DEVICEID1 Register field definitions */
> +#define TPS6508640_ID			0x00
> +#define TPS65086401_ID			0x01
> +#define TPS6508641_ID			0x10
> +#define TPS65086470_ID			0x70
> +
> +/* DEVICEID2 Register field definitions */
> +#define TPS65086_DEVICEID2_PART_MASK	GENMASK(3, 0)
> +#define TPS65086_DEVICEID2_OTP_MASK	GENMASK(5, 4)
> +#define TPS65086_DEVICEID2_REV_MASK	GENMASK(7, 6)
>  
>  /* VID Masks */
>  #define BUCK_VID_MASK			GENMASK(7, 1)
>  #define VDOA1_VID_MASK			GENMASK(4, 1)
>  #define VDOA23_VID_MASK			GENMASK(3, 0)
>  
> +enum tps65086_ids {
> +	TPS6508640,
> +	TPS65086401,
> +	TPS6508641,
> +	TPS65086470,
> +};
> +
>  /* Define the TPS65086 IRQ numbers */
>  enum tps65086_irqs {
>  	TPS65086_IRQ_DIETEMP,
> @@ -100,6 +114,7 @@ enum tps65086_irqs {
>  struct tps65086 {
>  	struct device *dev;
>  	struct regmap *regmap;
> +	unsigned int chip_id;
>  
>  	/* IRQ Data */
>  	int irq;
> -- 
> 2.41.0
> 

-- 
Lee Jones [李琼斯]