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@xxxxxxxxxxxx>
---
(...)
+
+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
