Message-ID: <7ade5f76-426c-297c-2809-af5f67279cf9@baylibre.com>
Date:   Mon, 6 Mar 2023 17:41:37 +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 v1 2/4] mfd: tps6594: Add driver for TI TPS6594 PMIC
Content-Language: en-US
To:     Lee Jones <lee@kernel.org>
Cc:     robh+dt@kernel.org, krzysztof.kozlowski+dt@linaro.org,
        corbet@lwn.net, hdegoede@redhat.com, eric.auger@redhat.com,
        jgg@ziepe.ca, razor@blackwall.org, suma.hegde@amd.com,
        stephen@networkplumber.org, arnd@arndb.de,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-doc@vger.kernel.org, eblanc@baylibre.com,
        jneanne@baylibre.com
References: <20230216114410.183489-1-jpanis@baylibre.com>
 <20230216114410.183489-3-jpanis@baylibre.com>
 <20230303150355.GQ2420672@google.com>
From:   Julien Panis <jpanis@baylibre.com>
In-Reply-To: <20230303150355.GQ2420672@google.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 3/3/23 16:03, Lee Jones wrote:
> On Thu, 16 Feb 2023, Julien Panis wrote:
>
>> This patch adds support for TPS6594 PMIC MFD core. It provides
>> communication through the I2C and SPI interfaces, and supports
>> protocols with embedded CRC data fields for safety applications.
>>
>> Signed-off-by: Julien Panis <jpanis@baylibre.com>
>> ---

(...)

>> +
>> +static int tps6594_check_crc_mode(struct tps6594 *tps, bool primary_pmic)
>> +{
>> +	int ret;
>> +
>> +	/*
>> +	 * Ensure that CRC is enabled.
>> +	 * Once CRC is enabled, it can't be disabled until next power cycle.
>> +	 */
>> +	tps->use_crc = true;
>> +	ret = regmap_test_bits(tps->regmap, TPS6594_REG_SERIAL_IF_CONFIG,
>> +			       TPS6594_BIT_I2C1_SPI_CRC_EN);
>> +	if (ret < 0) {
>> +		tps->use_crc = false;
>> +	} else if (ret == 0) {
>> +		tps->use_crc = false;
> Will this value be used again after you return an error?

No, it is not used any more. I will remove this line in v2.

>
>> +		ret = -EIO;
>> +	} else {
>> +		dev_info(tps->dev, "CRC feature enabled on %s PMIC",
>> +			 primary_pmic ? "primary" : "secondary");
>> +		ret = 0;
> I would consider reversing the logic of the if()s, default to 'false'
> then set 'true' in here before the print.

Do you speak about 'tps->use_crc' value ?
'tps->use_crc' is used in regmap read/write callbacks, so it
must be set 'true' before calling 'regmap_test_bits()' function.
In other words, CRC_EN bit must be read with 'tps->use_crc = true'.

>
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>> +static int tps6594_set_crc_feature(struct tps6594 *tps)
>> +{
>> +	int ret;
>> +
>> +	/* Force PFSM I2C_2 trigger to enable CRC on primary PMIC */
>> +	ret = regmap_write_bits(tps->regmap, TPS6594_REG_FSM_I2C_TRIGGERS,
>> +				TPS6594_BIT_TRIGGER_I2C(2), TPS6594_BIT_TRIGGER_I2C(2));
>> +	if (ret)
>> +		return ret;
>> +
>> +	/* Wait for PFSM to process trigger */
>> +	msleep(20);
> Is this the time specified in the datasheet?

I checked with the customer after your review and the datasheet
specifies 2 ms.
The clock specification is +/-5%. The customer recommends
using 4ms, which is a simple number providing sufficient margin.
As a consequence, I will adjust this delay in v2.

>
>> +	return tps6594_check_crc_mode(tps, true);
>> +}
>> +
>> +int tps6594_device_init(struct tps6594 *tps)
>> +{
>> +	struct device *dev = tps->dev;
>> +	unsigned int prop;
> Since this only has a single use, better to rename it to something specific.
>
>> +	unsigned long timeout = msecs_to_jiffies(TPS6594_CRC_SYNC_TIMEOUT_MS);
>> +	int n_dev = ARRAY_SIZE(tps6594_cells);
>> +	int ret;
>> +
>> +	/* Keep PMIC in ACTIVE state */
>> +	ret = regmap_set_bits(tps->regmap, TPS6594_REG_FSM_NSLEEP_TRIGGERS,
>> +			      TPS6594_BIT_NSLEEP1B | TPS6594_BIT_NSLEEP2B);
>> +	if (ret)
>> +		return dev_err_probe(dev, ret, "Failed to set PMIC state\n");
>> +
>> +	/*
>> +	 * CRC mode can be used with I2C or SPI protocols.
>> +	 * If this mode is specified for primary PMIC, it will also be applied to secondary PMICs
>> +	 * through SPMI serial interface.
>> +	 * In this multi-PMIC synchronization scheme, the primary PMIC is the controller device
>> +	 * on the SPMI bus, and the secondary PMICs are the target devices on the SPMI bus.
>> +	 */
>> +	prop = of_property_read_bool(dev->of_node, "ti,use-crc");

As discussed with Krzysztof for dt-bindings, this 'ti,use-crc'
property will be removed from the device tree, in v2.
Instead, a property will be used to identify the primary PMIC.
Moreover, since using CRC applies either to all the PMICs or
to none of them, it is a global feature. That's why a driver
parameter will be added to enable CRC feature at initialization
(something like a 'enable_crc' bool).

(...)

>> diff --git a/include/linux/mfd/tps6594.h b/include/linux/mfd/tps6594.h
>> new file mode 100644
>> index 000000000000..e2ffd4dc034d
>> --- /dev/null
>> +++ b/include/linux/mfd/tps6594.h
>> @@ -0,0 +1,1018 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * Functions to access TPS6594 Power Management IC
>> + *
>> + * Copyright (C) 2022 BayLibre Incorporated - https://www.baylibre.com/
>> + */
>> +
>> +#ifndef __LINUX_MFD_TPS6594_H
>> +#define __LINUX_MFD_TPS6594_H
>> +
>> +#include <linux/device.h>
>> +#include <linux/regmap.h>
>> +
>> +struct regmap_irq_chip_data;
>> +
>> +/* Chip id list */
>> +#define TPS6594		0
>> +#define TPS6593		1
>> +#define LP8764X		2
> enum?

Yes indeed, I will fix that in v2.

(...)

Your others suggestions will also be implemented in v2.

Thank you Lee for your time and feedback.

Julien