Messages in this thread | | | Date | Mon, 6 Mar 2023 17:41:37 +0100 | Subject | Re: [PATCH v1 2/4] mfd: tps6594: Add driver for TI TPS6594 PMIC | From | Julien Panis <> |
| |
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
| |