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;
Message-ID: <205a4e62-fd87-629c-ea34-d863ff1549d8@baylibre.com>
Date:   Wed, 22 Mar 2023 10:10:23 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.8.0
Subject: Re: [PATCH INTERNAL v1 3/3] regulator: tps6594-regulator: Add driver
 for TI TPS6594 regulators
Content-Language: en-US
To:     Esteban Blanc <eblanc@baylibre.com>, linus.walleij@linaro.org,
        lgirdwood@gmail.com, broonie@kernel.org, a.zummo@towertech.it,
        alexandre.belloni@bootlin.com
Cc:     linux-kernel@vger.kernel.org, linux-gpio@vger.kernel.org,
        linux-rtc@vger.kernel.org, jneanne@baylibre.com
References: <20230224133129.887203-1-eblanc@baylibre.com>
 <20230224133129.887203-4-eblanc@baylibre.com>
From:   Julien Panis <jpanis@baylibre.com>
In-Reply-To: <20230224133129.887203-4-eblanc@baylibre.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 2/24/23 14:31, Esteban Blanc wrote:
> From: Jerome Neanne <jneanne@baylibre.com>
>
> This patch adds support for TPS6594 regulators (bucks and LDOs).
> The output voltages are configurable and are meant to supply power
> to the main processor and other components.
> Bucks can be used in single or multiphase mode, depending on PMIC
> part number.
>
> Signed-off-by: Jerome Neanne <jneanne@baylibre.com>
> ---

(...)

> +static int tps6594_regulator_probe(struct platform_device *pdev)
> +{
> +	struct tps6594 *tps = dev_get_drvdata(pdev->dev.parent);
> +	struct regulator_dev *rdev;
> +	struct regulator_config config = {};
> +	u8 buck_configured[BUCK_NB] = { 0 };
> +	u8 buck_multi[MULTI_PHASE_NB] = { 0 };
> +	int i;
> +	int error;
> +	int irq;
> +	int ext_reg_irq_nb = 2;
> +	struct tps6594_regulator_irq_data *irq_data;
> +	struct tps6594_ext_regulator_irq_data *irq_ext_reg_data;
> +	struct tps6594_regulator_irq_type *irq_type;
> +	struct regulator_dev *rdevbucktbl[BUCK_NB];
> +	struct regulator_dev *rdevmultitbl[MULTI_PHASE_NB];
> +	struct regulator_dev *rdevldotbl[LDO_NB];
> +
> +	int multi_phase_id;
> +	int multi_phase_case = 0xFFFF;
> +
> +	config.dev = tps->dev;
> +	config.driver_data = tps;
> +	config.regmap = tps->regmap;
> +
> +	/*
> +	 * Switch case defines different possible multi phase config
> +	 * This is based on dts custom property: multi-phase-id
> +	 * Using compatible or device rev is a too complex alternative
> +	 * Default case is no Multiphase buck.
> +	 * In case of Multiphase configuration, value should be defined for
> +	 * buck_configured to avoid creating bucks for every buck in multiphase
> +	 */
> +
> +	if (device_property_present(tps->dev, "ti,multi-phase-id")) {

Question @ Mark/Liam:
Shouldn't we use the generic 'regulator-coupled-with' property
instead of 'ti,multi-phase-id' ?
I am in charge of upstreaming dt-bindings and maintainers
pointed out the similarity between 'multi-phase' and 'coupled'
regulator concepts. Does 'regulator-coupled-with' mean that
outputs of buck converters are combined ? If so, this generic
property should replace our specific 'ti,multi-phase-id' prop,
I guess.

> +		device_property_read_u32(tps->dev, "ti,multi-phase-id", &multi_phase_id);
> +		switch (multi_phase_id) {
> +		case 12:
> +			buck_multi[0] = 1;
> +			buck_configured[0] = 1;
> +			buck_configured[1] = 1;
> +			multi_phase_case = TPS6594_BUCK_12;
> +			break;
> +		case 34:
> +			buck_multi[1] = 1;
> +			buck_configured[2] = 1;
> +			buck_configured[3] = 1;
> +			multi_phase_case = TPS6594_BUCK_34;
> +			break;
> +		case 123:
> +			buck_multi[2] = 1;
> +			buck_configured[0] = 1;
> +			buck_configured[1] = 1;
> +			buck_configured[2] = 1;
> +			multi_phase_case = TPS6594_BUCK_123;
> +			break;
> +		case 1234:
> +			buck_multi[3] = 1;
> +			buck_configured[0] = 1;
> +			buck_configured[1] = 1;
> +			buck_configured[2] = 1;
> +			buck_configured[3] = 1;
> +			multi_phase_case = TPS6594_BUCK_1234;
> +			break;
> +		}
> +	}
> +
> +	for (i = 0; i < MULTI_PHASE_NB; i++) {
> +		if (buck_multi[i] == 0)
> +			continue;
> +
> +		rdev = devm_regulator_register(&pdev->dev, &multi_regs[i], &config);
> +		if (IS_ERR(rdev)) {
> +			dev_err(tps->dev, "failed to register %s regulator\n",
> +				pdev->name);
> +			return PTR_ERR(rdev);
> +		}
> +		rdevmultitbl[i] = rdev;
> +	}
> +

(...)