[PATCH v1 0/3] TI TPS6594 PMIC support (RTC, pinctrl, regulators, device trees)

[PATCH v1 0/3] TI TPS6594 PMIC support (RTC, pinctrl, regulators, device trees)

[Esteban Blanc]

TPS6594 is a Power Management IC which provides regulators and others
features like GPIOs, RTC, watchdog, ESMs (Error Signal Monitor), and
PFSM (Pre-configurable Finite State Machine). The SoC and the PMIC can
communicate through the I2C or SPI interfaces.
TPS6594 is the super-set device while TPS6593 and LP8764X are derivatives.

This series adds support to TI TPS6594 PMIC and its derivatives.

This should be applied on top of other patch series:
- https://lore.kernel.org/all/20230216114410.183489-1-jpanis@baylibre.com/
  For core MFD driver

The features implemented in this series are:
- RTC (child device)
- Pinmux/GPIO (child device)
- Regulator (child device)

RTC description:
The TPS6594 family has an RTC built-in, except for LP8764X.
It provides time and an alarm.

Pinmux/GPIO:
TPS6594 family has 11 GPIOs. Those GPIO can also serve different
functions such as I2C or SPI interface, watchdog disable functions.
The driver provides both pinmuxing for the functions and GPIO capability.

Regulator:
TPS6594/TPS6593: 5 BUCKs and 4LDOs
LP8764X: 4 BUCKs and no LDO
Bucks can be used in multipahse mode

Esteban Blanc (2):
  rtc: tps6594: add driver for TPS6594 PMIC RTC
  pinctrl: tps6594: add for TPS6594 PMIC

Jerome Neanne (1):
  regulator: tps6594-regulator: Add driver for TI TPS6594 regulators

 drivers/pinctrl/Kconfig               |   9 +
 drivers/pinctrl/Makefile              |   1 +
 drivers/pinctrl/pinctrl-tps6594.c     | 367 +++++++++++++++++
 drivers/regulator/Kconfig             |  12 +
 drivers/regulator/Makefile            |   1 +
 drivers/regulator/tps6594-regulator.c | 559 ++++++++++++++++++++++++++
 drivers/rtc/Kconfig                   |   8 +
 drivers/rtc/Makefile                  |   1 +
 drivers/rtc/rtc-tps6594.c             | 516 ++++++++++++++++++++++++
 9 files changed, 1474 insertions(+)
 create mode 100644 drivers/pinctrl/pinctrl-tps6594.c
 create mode 100644 drivers/regulator/tps6594-regulator.c
 create mode 100644 drivers/rtc/rtc-tps6594.c

-- 
2.38.1

[PATCH INTERNAL v1 1/3] rtc: tps6594: add driver for TPS6594 PMIC RTC

[Esteban Blanc]

TPS6594 PMIC is a MFD driver. This add support for the RTC found inside
TPS6594 family of PMIC.

Alarm is also supported.

Signed-off-by: Esteban Blanc <eblanc@baylibre.com>
---
 drivers/rtc/Kconfig       |   8 +
 drivers/rtc/Makefile      |   1 +
 drivers/rtc/rtc-tps6594.c | 516 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 525 insertions(+)
 create mode 100644 drivers/rtc/rtc-tps6594.c

diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
index 677d2601d305..34c63555669d 100644
--- a/drivers/rtc/Kconfig
+++ b/drivers/rtc/Kconfig
@@ -578,6 +578,14 @@ config RTC_DRV_TPS6586X
 	  along with alarm. This driver supports the RTC driver for
 	  the TPS6586X RTC module.
 
+config RTC_DRV_TPS6594
+	tristate "TI TPS6594 RTC driver"
+	depends on MFD_TPS6594
+	help
+	  TI Power Management IC TPS6594 supports RTC functionality
+	  along with alarm. This driver supports the RTC driver for
+	  the TPS6594 RTC module.
+
 config RTC_DRV_TPS65910
 	tristate "TI TPS65910 RTC driver"
 	depends on MFD_TPS65910
diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
index d3c042dcbc73..7dbeec711ad5 100644
--- a/drivers/rtc/Makefile
+++ b/drivers/rtc/Makefile
@@ -174,6 +174,7 @@ obj-$(CONFIG_RTC_DRV_TEGRA)	+= rtc-tegra.o
 obj-$(CONFIG_RTC_DRV_TEST)	+= rtc-test.o
 obj-$(CONFIG_RTC_DRV_TI_K3)	+= rtc-ti-k3.o
 obj-$(CONFIG_RTC_DRV_TPS6586X)	+= rtc-tps6586x.o
+obj-$(CONFIG_RTC_DRV_TPS6594)	+= rtc-tps6594.o
 obj-$(CONFIG_RTC_DRV_TPS65910)	+= rtc-tps65910.o
 obj-$(CONFIG_RTC_DRV_TWL4030)	+= rtc-twl.o
 obj-$(CONFIG_RTC_DRV_V3020)	+= rtc-v3020.o
diff --git a/drivers/rtc/rtc-tps6594.c b/drivers/rtc/rtc-tps6594.c
new file mode 100644
index 000000000000..f150ef33f65f
--- /dev/null
+++ b/drivers/rtc/rtc-tps6594.c
@@ -0,0 +1,516 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * RTC driver for tps6594 PMIC
+ *
+ * Copyright (C) 2022 BayLibre Incorporated - https://www.baylibre.com/
+ */
+
+#include <linux/bcd.h>
+#include <linux/errno.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/math64.h>
+#include <linux/module.h>
+#include <linux/rtc.h>
+#include <linux/types.h>
+#include <linux/platform_device.h>
+#include <linux/property.h>
+
+#include <linux/mfd/tps6594.h>
+
+struct tps6594_rtc {
+	struct rtc_device *rtc;
+};
+
+#define TPS6594_GET_TIME_ON TPS6594_BIT_GET_TIME
+#define TPS6594_GET_TIME_OFF 0
+#define TPS6594_IT_ALARM_ON TPS6594_BIT_IT_ALARM
+#define TPS6594_IT_ALARM_OFF 0
+#define TPS6594_AUTO_COMP_ON TPS6594_BIT_IT_ALARM
+
+/* Total number of RTC registers needed to set time*/
+#define NUM_TIME_REGS (TPS6594_REG_RTC_WEEKS - TPS6594_REG_RTC_SECONDS + 1)
+
+/* Total number of RTC alarm register */
+#define NUM_TIME_ALARM_REGS (NUM_TIME_REGS - 1)
+
+/* Total number of RTC registers needed to set compensation registers */
+#define NUM_COMP_REGS (TPS6594_REG_RTC_COMP_MSB - TPS6594_REG_RTC_COMP_LSB + 1)
+
+/* Min and max values supported with 'offset' interface (swapped sign)
+ * After conversion, the values does not exceed the range [-32767, 33767] which COMP_REG must
+ * conform to
+ */
+#define MIN_OFFSET (-277774)
+#define MAX_OFFSET (277774)
+
+/* Number of ticks per hour */
+#define TICKS_PER_HOUR (32768 * 3600)
+
+/* Multiplier for ppb conversions */
+#define PPB_MULT (1000000000LL)
+
+static int tps6594_rtc_alarm_irq_enable(struct device *dev,
+					unsigned int enabled)
+{
+	struct tps6594 *tps = dev_get_drvdata(dev->parent);
+	u8 val = 0;
+	int ret;
+
+	val = enabled ? TPS6594_IT_ALARM_ON : TPS6594_IT_ALARM_OFF;
+
+	ret = regmap_update_bits(tps->regmap, TPS6594_REG_RTC_INTERRUPTS,
+				 TPS6594_BIT_IT_ALARM, val);
+
+	return ret;
+}
+
+/* Pulse GET_TIME field of RTC_CTRL_1 to store a timestamp in shadow registers */
+static int tps6594_rtc_shadow_timestamp(struct device *dev, struct tps6594 *tps)
+{
+	int ret;
+
+	/* Set GET_TIME to 0. This way, next time we set GET_TIME to 1 we are sure to store an
+	 * up-to-date timestamp
+	 */
+	ret = regmap_clear_bits(tps->regmap, TPS6594_REG_RTC_CTRL_1,
+				TPS6594_BIT_GET_TIME);
+	if (ret < 0) {
+		dev_err(dev, "RTC CTRL1 reg update failed with err:%d\n", ret);
+		return ret;
+	}
+
+	/* Copy content of RTC registers to shadow registers or latches to read a coherent
+	 *  timestamp
+	 */
+	ret = regmap_set_bits(tps->regmap, TPS6594_REG_RTC_CTRL_1,
+			      TPS6594_BIT_GET_TIME);
+	if (ret < 0) {
+		dev_err(dev, "RTC CTRL1 reg update failed with err:%d\n", ret);
+		return ret;
+	}
+
+	return ret;
+}
+
+/*
+ * Gets current tps6594 RTC time and date parameters.
+ *
+ * The RTC's time/alarm representation is not what gmtime(3) requires
+ * Linux to use:
+ *
+ *  - Months are 1..12 vs Linux 0-11
+ *  - Years are 0..99 vs Linux 1900..N (we assume 21st century)
+ */
+static int tps6594_rtc_read_time(struct device *dev, struct rtc_time *tm)
+{
+	unsigned char rtc_data[NUM_TIME_REGS];
+	struct tps6594 *tps = dev_get_drvdata(dev->parent);
+	int ret;
+
+	ret = tps6594_rtc_shadow_timestamp(dev, tps);
+	if (ret < 0) {
+		dev_err(dev,
+			"failed to store a timestamp in shadow registers:%d\n",
+			ret);
+		return ret;
+	}
+
+	/* Read shadowed RTC registers */
+	ret = regmap_bulk_read(tps->regmap, TPS6594_REG_RTC_SECONDS, rtc_data,
+			       NUM_TIME_REGS);
+	if (ret < 0) {
+		dev_err(dev, "rtc_read_time failed with error :%d\n", ret);
+		return ret;
+	}
+
+	tm->tm_sec = bcd2bin(rtc_data[0]);
+	tm->tm_min = bcd2bin(rtc_data[1]);
+	tm->tm_hour = bcd2bin(rtc_data[2]);
+	tm->tm_mday = bcd2bin(rtc_data[3]);
+	tm->tm_mon = bcd2bin(rtc_data[4]) - 1;
+	tm->tm_year = bcd2bin(rtc_data[5]) + 100;
+	tm->tm_wday = bcd2bin(rtc_data[6]);
+
+	return ret;
+}
+
+/*
+ * Sets current tps6594 RTC time and date parameters.
+ *
+ * The RTC's time/alarm representation is not what gmtime(3) requires
+ * Linux to use:
+ *
+ *  - Months are 1..12 vs Linux 0-11
+ *  - Years are 0..99 vs Linux 1900..N (we assume 21st century)
+ */
+static int tps6594_rtc_set_time(struct device *dev, struct rtc_time *tm)
+{
+	unsigned char rtc_data[NUM_TIME_REGS];
+	struct tps6594 *tps = dev_get_drvdata(dev->parent);
+	int ret;
+
+	rtc_data[0] = bin2bcd(tm->tm_sec);
+	rtc_data[1] = bin2bcd(tm->tm_min);
+	rtc_data[2] = bin2bcd(tm->tm_hour);
+	rtc_data[3] = bin2bcd(tm->tm_mday);
+	rtc_data[4] = bin2bcd(tm->tm_mon + 1);
+	rtc_data[5] = bin2bcd(tm->tm_year - 100);
+	rtc_data[6] = bin2bcd(tm->tm_wday);
+
+	/* Stop RTC while updating the RTC time registers */
+	ret = regmap_clear_bits(tps->regmap, TPS6594_REG_RTC_CTRL_1,
+				TPS6594_BIT_STOP_RTC);
+	if (ret < 0) {
+		dev_err(dev, "RTC stop failed: %d\n", ret);
+		return ret;
+	}
+
+	/* Update all the time registers in one shot */
+	ret = regmap_bulk_write(tps->regmap, TPS6594_REG_RTC_SECONDS, rtc_data,
+				NUM_TIME_REGS);
+	if (ret < 0) {
+		dev_err(dev, "rtc_set_time failed: %d\n", ret);
+		return ret;
+	}
+
+	/* Start back RTC */
+	ret = regmap_set_bits(tps->regmap, TPS6594_REG_RTC_CTRL_1,
+			      TPS6594_BIT_STOP_RTC);
+	if (ret < 0)
+		dev_err(dev, "RTC start failed: %d\n", ret);
+
+	return ret;
+}
+
+/*
+ * Gets current tps6594 RTC alarm time.
+ */
+static int tps6594_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alm)
+{
+	unsigned char alarm_data[NUM_TIME_ALARM_REGS];
+	u32 int_val;
+	struct tps6594 *tps = dev_get_drvdata(dev->parent);
+	int ret;
+
+	ret = regmap_bulk_read(tps->regmap, TPS6594_REG_ALARM_SECONDS,
+			       alarm_data, NUM_TIME_ALARM_REGS);
+	if (ret < 0) {
+		dev_err(dev, "rtc_read_alarm failed read alarm registers: %d\n",
+			ret);
+		return ret;
+	}
+
+	alm->time.tm_sec = bcd2bin(alarm_data[0]);
+	alm->time.tm_min = bcd2bin(alarm_data[1]);
+	alm->time.tm_hour = bcd2bin(alarm_data[2]);
+	alm->time.tm_mday = bcd2bin(alarm_data[3]);
+	alm->time.tm_mon = bcd2bin(alarm_data[4]) - 1;
+	alm->time.tm_year = bcd2bin(alarm_data[5]) + 100;
+
+	ret = regmap_read(tps->regmap, TPS6594_REG_RTC_INTERRUPTS, &int_val);
+	if (ret < 0) {
+		dev_err(dev, "rtc_read_alarm failed to read alarm status: %d\n",
+			ret);
+		return ret;
+	}
+
+	alm->enabled = int_val & TPS6594_BIT_IT_ALARM ? 1 : 0;
+
+	return ret;
+}
+
+static int tps6594_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alm)
+{
+	unsigned char alarm_data[NUM_TIME_ALARM_REGS];
+	struct tps6594 *tps = dev_get_drvdata(dev->parent);
+	int ret;
+
+	/* Disable alarm irq before changing the alarm timestamp */
+	ret = tps6594_rtc_alarm_irq_enable(dev, 0);
+	if (ret) {
+		dev_err(dev, "rtc_set_alarm failed to disable alarm: %d\n",
+			ret);
+		return ret;
+	}
+
+	alarm_data[0] = bin2bcd(alm->time.tm_sec);
+	alarm_data[1] = bin2bcd(alm->time.tm_min);
+	alarm_data[2] = bin2bcd(alm->time.tm_hour);
+	alarm_data[3] = bin2bcd(alm->time.tm_mday);
+	alarm_data[4] = bin2bcd(alm->time.tm_mon + 1);
+	alarm_data[5] = bin2bcd(alm->time.tm_year - 100);
+
+	/* Update all the alarm registers in one shot */
+	ret = regmap_bulk_write(tps->regmap, TPS6594_REG_ALARM_SECONDS,
+				alarm_data, NUM_TIME_ALARM_REGS);
+	if (ret) {
+		dev_err(dev,
+			"rtc_set_alarm failed to write alarm registers: %d\n",
+			ret);
+		return ret;
+	}
+
+	if (alm->enabled) {
+		ret = tps6594_rtc_alarm_irq_enable(dev, 1);
+		if (ret < 0) {
+			dev_err(dev,
+				"rtc_set_alarm failed to re-enable the alarm: %d\n",
+				ret);
+			return ret;
+		}
+	}
+
+	return ret;
+}
+
+static int tps6594_rtc_set_calibration(struct device *dev, int calibration)
+{
+	unsigned char comp_data[NUM_COMP_REGS];
+	struct tps6594 *tps = dev_get_drvdata(dev->parent);
+	s16 value;
+	int ret;
+
+	/*
+	 * TPS6594 uses two's complement 16 bit value for compensation for RTC
+	 * crystal inaccuracies. One time every hour when seconds counter
+	 * increments from 0 to 1 compensation value will be added to internal
+	 * RTC counter value.
+	 *
+	 *
+	 * Valid range for compensation value: [-32767 .. 32767]
+	 */
+	if (calibration < -32767 || calibration > 32767) {
+		dev_err(dev, "RTC calibration value out of range: %d\n",
+			calibration);
+		return -EINVAL;
+	}
+
+	value = (s16)calibration;
+
+	comp_data[0] = (u16)value & 0xFF;
+	comp_data[1] = ((u16)value >> 8) & 0xFF;
+
+	/* Update all the compensation registers in one shot */
+	ret = regmap_bulk_write(tps->regmap, TPS6594_REG_RTC_COMP_LSB,
+				comp_data, NUM_COMP_REGS);
+	if (ret < 0) {
+		dev_err(dev,
+			"rtc_set_calibration failed to write compensation value: %d\n",
+			ret);
+		return ret;
+	}
+
+	/* Enable automatic compensation */
+	ret = regmap_set_bits(tps->regmap, TPS6594_REG_RTC_CTRL_1,
+			      TPS6594_BIT_AUTO_COMP);
+	if (ret < 0)
+		dev_err(dev,
+			"rtc_set_calibration failed to enable auto_comp: %d\n",
+			ret);
+
+	return ret;
+}
+
+static int tps6594_rtc_get_calibration(struct device *dev, int *calibration)
+{
+	unsigned char comp_data[NUM_COMP_REGS];
+	struct tps6594 *tps = dev_get_drvdata(dev->parent);
+	unsigned int ctrl;
+	u16 value;
+	int ret;
+
+	ret = regmap_read(tps->regmap, TPS6594_REG_RTC_CTRL_1, &ctrl);
+	if (ret < 0) {
+		dev_err(dev,
+			"rtc_get_calibration failed to read control register 1: %d\n",
+			ret);
+		return ret;
+	}
+
+	/* If automatic compensation is not enabled report back zero */
+	if (!(ctrl & TPS6594_BIT_AUTO_COMP)) {
+		*calibration = 0;
+		return 0;
+	}
+
+	ret = regmap_bulk_read(tps->regmap, TPS6594_REG_RTC_COMP_LSB, comp_data,
+			       NUM_COMP_REGS);
+	if (ret < 0) {
+		dev_err(dev,
+			"rtc_get_calibration failed to read compensation registers: %d\n",
+			ret);
+		return ret;
+	}
+
+	value = (u16)comp_data[0] | ((u16)comp_data[1] << 8);
+
+	*calibration = (s16)value;
+
+	return 0;
+}
+
+static int tps6594_rtc_read_offset(struct device *dev, long *offset)
+{
+	int calibration;
+	s64 tmp;
+	int ret;
+
+	ret = tps6594_rtc_get_calibration(dev, &calibration);
+	if (ret < 0)
+		return ret;
+
+	/* Convert from RTC calibration register format to ppb format */
+	tmp = calibration * (s64)PPB_MULT;
+	if (tmp < 0)
+		tmp -= TICKS_PER_HOUR / 2LL;
+	else
+		tmp += TICKS_PER_HOUR / 2LL;
+	tmp = div_s64(tmp, TICKS_PER_HOUR);
+
+	/* Offset value operates in negative way, so swap sign.
+	 * See 8.3.10.5, (32768 - COMP_REG)
+	 */
+	*offset = (long)-tmp;
+
+	return 0;
+}
+
+static int tps6594_rtc_set_offset(struct device *dev, long offset)
+{
+	int calibration;
+	s64 tmp;
+	int ret;
+
+	/* Make sure offset value is within supported range */
+	if (offset < MIN_OFFSET || offset > MAX_OFFSET)
+		return -ERANGE;
+
+	/* Convert from ppb format to RTC calibration register format */
+	tmp = offset * (s64)TICKS_PER_HOUR;
+	if (tmp < 0)
+		tmp -= PPB_MULT / 2LL;
+	else
+		tmp += PPB_MULT / 2LL;
+	tmp = div_s64(tmp, PPB_MULT);
+
+	/* Offset value operates in negative way, so swap sign */
+	calibration = (int)-tmp;
+
+	ret = tps6594_rtc_set_calibration(dev, calibration);
+
+	return ret;
+}
+
+static irqreturn_t tps6594_rtc_interrupt(int irq, void *rtc)
+{
+	struct device *dev = rtc;
+	unsigned long events = 0;
+	struct tps6594 *tps = dev_get_drvdata(dev->parent);
+	struct tps6594_rtc *tps_rtc = dev_get_drvdata(dev);
+	int ret;
+	u32 rtc_reg;
+
+	ret = regmap_read(tps->regmap, TPS6594_REG_RTC_STATUS, &rtc_reg);
+	if (ret)
+		return IRQ_NONE;
+
+	if (rtc_reg & TPS6594_BIT_ALARM)
+		events = RTC_IRQF | RTC_AF;
+
+	/* Notify RTC core on event */
+	rtc_update_irq(tps_rtc->rtc, 1, events);
+
+	return IRQ_HANDLED;
+}
+
+static const struct rtc_class_ops tps6594_rtc_ops = {
+	.read_time = tps6594_rtc_read_time,
+	.set_time = tps6594_rtc_set_time,
+	.read_alarm = tps6594_rtc_read_alarm,
+	.set_alarm = tps6594_rtc_set_alarm,
+	.alarm_irq_enable = tps6594_rtc_alarm_irq_enable,
+	.read_offset = tps6594_rtc_read_offset,
+	.set_offset = tps6594_rtc_set_offset,
+};
+
+static int tps6594_rtc_probe(struct platform_device *pdev)
+{
+	struct tps6594 *tps6594;
+	struct tps6594_rtc *tps_rtc;
+	int irq;
+	int ret;
+
+	tps6594 = dev_get_drvdata(pdev->dev.parent);
+
+	tps_rtc = devm_kzalloc(&pdev->dev, sizeof(struct tps6594_rtc),
+			       GFP_KERNEL);
+	if (!tps_rtc)
+		return -ENOMEM;
+
+	tps_rtc->rtc = devm_rtc_allocate_device(&pdev->dev);
+	if (IS_ERR(tps_rtc->rtc))
+		return PTR_ERR(tps_rtc->rtc);
+
+	/* Enable crystal oscillator */
+	ret = regmap_set_bits(tps6594->regmap, TPS6594_REG_RTC_CTRL_2,
+			      TPS6594_BIT_XTAL_EN);
+	if (ret < 0)
+		return ret;
+
+	/* Start rtc */
+	ret = regmap_set_bits(tps6594->regmap, TPS6594_REG_RTC_CTRL_1,
+			      TPS6594_BIT_STOP_RTC);
+	if (ret < 0)
+		return ret;
+
+	mdelay(100);
+
+	/*
+	 * RTC should be running now. Check if this is the case.
+	 * If not it might be a missing oscillator.
+	 */
+	ret = regmap_test_bits(tps6594->regmap, TPS6594_REG_RTC_STATUS,
+			       TPS6594_BIT_RUN);
+	if (ret < 0)
+		return ret;
+	if (ret == 0)
+		return -ENODEV;
+
+	platform_set_drvdata(pdev, tps_rtc);
+
+	irq = platform_get_irq_byname(pdev, TPS6594_IRQ_NAME_ALARM);
+	if (irq < 0) {
+		dev_err(&pdev->dev, "Failed to get irq\n");
+		return irq;
+	}
+
+	ret = devm_request_threaded_irq(&pdev->dev, irq, NULL,
+					tps6594_rtc_interrupt, IRQF_ONESHOT,
+					TPS6594_IRQ_NAME_ALARM, &pdev->dev);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "Failed to request_threaded_irq\n");
+		return ret;
+	}
+
+	tps_rtc->rtc->ops = &tps6594_rtc_ops;
+	tps_rtc->rtc->range_min = RTC_TIMESTAMP_BEGIN_2000;
+	tps_rtc->rtc->range_max = RTC_TIMESTAMP_END_2099;
+
+	return devm_rtc_register_device(tps_rtc->rtc);
+}
+
+static struct platform_driver tps6594_rtc_driver = {
+	.probe		= tps6594_rtc_probe,
+	.driver		= {
+		.name	= "tps6594-rtc",
+	},
+};
+
+module_platform_driver(tps6594_rtc_driver);
+
+MODULE_ALIAS("platform:tps6594-rtc");
+MODULE_AUTHOR("Esteban Blanc <eblanc@baylibre.com>");
+MODULE_DESCRIPTION("TPS6594 RTC driver");
+MODULE_LICENSE("GPL");
-- 
2.38.1

Re: [PATCH INTERNAL v1 1/3] rtc: tps6594: add driver for TPS6594 PMIC RTC

[Alexandre Belloni]

Hello,

On 24/02/2023 14:31:27+0100, Esteban Blanc wrote:
> +struct tps6594_rtc {
> +	struct rtc_device *rtc;
> +};

Is the struct actually useful?

> +/* Pulse GET_TIME field of RTC_CTRL_1 to store a timestamp in shadow registers */
> +static int tps6594_rtc_shadow_timestamp(struct device *dev, struct tps6594 *tps)
> +{
> +	int ret;
> +
> +	/* Set GET_TIME to 0. This way, next time we set GET_TIME to 1 we are sure to store an
> +	 * up-to-date timestamp
> +	 */
> +	ret = regmap_clear_bits(tps->regmap, TPS6594_REG_RTC_CTRL_1,
> +				TPS6594_BIT_GET_TIME);
> +	if (ret < 0) {
> +		dev_err(dev, "RTC CTRL1 reg update failed with err:%d\n", ret);

This is very verbose and the user doesn't have any meaningful action
once this happens. Either remove the dev_err or convert them to dev_dbg.

> +		return ret;
> +	}
> +
> +	/* Copy content of RTC registers to shadow registers or latches to read a coherent
> +	 *  timestamp
> +	 */
> +	ret = regmap_set_bits(tps->regmap, TPS6594_REG_RTC_CTRL_1,
> +			      TPS6594_BIT_GET_TIME);
> +	if (ret < 0) {
> +		dev_err(dev, "RTC CTRL1 reg update failed with err:%d\n", ret);
> +		return ret;
> +	}
> +
> +	return ret;
> +}
> +
> +/*
> + * Gets current tps6594 RTC time and date parameters.
> + *
> + * The RTC's time/alarm representation is not what gmtime(3) requires
> + * Linux to use:
> + *
> + *  - Months are 1..12 vs Linux 0-11
> + *  - Years are 0..99 vs Linux 1900..N (we assume 21st century)
> + */

I don't find this comment to be particularly useful.

> +static int tps6594_rtc_read_time(struct device *dev, struct rtc_time *tm)
> +{
> +	unsigned char rtc_data[NUM_TIME_REGS];
> +	struct tps6594 *tps = dev_get_drvdata(dev->parent);
> +	int ret;
> +

You could check whether the RTC is actually started here and return
-EINVAL if this is not the case.

> +	ret = tps6594_rtc_shadow_timestamp(dev, tps);
> +	if (ret < 0) {
> +		dev_err(dev,
> +			"failed to store a timestamp in shadow registers:%d\n",
> +			ret);
> +		return ret;
> +	}
> +
> +	/* Read shadowed RTC registers */
> +	ret = regmap_bulk_read(tps->regmap, TPS6594_REG_RTC_SECONDS, rtc_data,
> +			       NUM_TIME_REGS);
> +	if (ret < 0) {
> +		dev_err(dev, "rtc_read_time failed with error :%d\n", ret);
> +		return ret;
> +	}
> +
> +	tm->tm_sec = bcd2bin(rtc_data[0]);
> +	tm->tm_min = bcd2bin(rtc_data[1]);
> +	tm->tm_hour = bcd2bin(rtc_data[2]);
> +	tm->tm_mday = bcd2bin(rtc_data[3]);
> +	tm->tm_mon = bcd2bin(rtc_data[4]) - 1;
> +	tm->tm_year = bcd2bin(rtc_data[5]) + 100;
> +	tm->tm_wday = bcd2bin(rtc_data[6]);
> +
> +	return ret;
> +}
> +
> +/*
> + * Sets current tps6594 RTC time and date parameters.
> + *
> + * The RTC's time/alarm representation is not what gmtime(3) requires
> + * Linux to use:
> + *
> + *  - Months are 1..12 vs Linux 0-11
> + *  - Years are 0..99 vs Linux 1900..N (we assume 21st century)
> + */

Ditto

> +static int tps6594_rtc_set_time(struct device *dev, struct rtc_time *tm)
> +{
> +	unsigned char rtc_data[NUM_TIME_REGS];
> +	struct tps6594 *tps = dev_get_drvdata(dev->parent);
> +	int ret;
> +
> +	rtc_data[0] = bin2bcd(tm->tm_sec);
> +	rtc_data[1] = bin2bcd(tm->tm_min);
> +	rtc_data[2] = bin2bcd(tm->tm_hour);
> +	rtc_data[3] = bin2bcd(tm->tm_mday);
> +	rtc_data[4] = bin2bcd(tm->tm_mon + 1);
> +	rtc_data[5] = bin2bcd(tm->tm_year - 100);
> +	rtc_data[6] = bin2bcd(tm->tm_wday);
> +
> +	/* Stop RTC while updating the RTC time registers */
> +	ret = regmap_clear_bits(tps->regmap, TPS6594_REG_RTC_CTRL_1,
> +				TPS6594_BIT_STOP_RTC);
> +	if (ret < 0) {
> +		dev_err(dev, "RTC stop failed: %d\n", ret);
> +		return ret;
> +	}
> +
> +	/* Update all the time registers in one shot */
> +	ret = regmap_bulk_write(tps->regmap, TPS6594_REG_RTC_SECONDS, rtc_data,
> +				NUM_TIME_REGS);
> +	if (ret < 0) {
> +		dev_err(dev, "rtc_set_time failed: %d\n", ret);
> +		return ret;
> +	}
> +
> +	/* Start back RTC */
> +	ret = regmap_set_bits(tps->regmap, TPS6594_REG_RTC_CTRL_1,
> +			      TPS6594_BIT_STOP_RTC);
> +	if (ret < 0)
> +		dev_err(dev, "RTC start failed: %d\n", ret);
> +
> +	return ret;
> +}
> +


> +static int tps6594_rtc_probe(struct platform_device *pdev)
> +{
> +	struct tps6594 *tps6594;
> +	struct tps6594_rtc *tps_rtc;
> +	int irq;
> +	int ret;
> +
> +	tps6594 = dev_get_drvdata(pdev->dev.parent);
> +
> +	tps_rtc = devm_kzalloc(&pdev->dev, sizeof(struct tps6594_rtc),
> +			       GFP_KERNEL);
> +	if (!tps_rtc)
> +		return -ENOMEM;
> +
> +	tps_rtc->rtc = devm_rtc_allocate_device(&pdev->dev);
> +	if (IS_ERR(tps_rtc->rtc))
> +		return PTR_ERR(tps_rtc->rtc);
> +
> +	/* Enable crystal oscillator */
> +	ret = regmap_set_bits(tps6594->regmap, TPS6594_REG_RTC_CTRL_2,
> +			      TPS6594_BIT_XTAL_EN);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Start rtc */
> +	ret = regmap_set_bits(tps6594->regmap, TPS6594_REG_RTC_CTRL_1,
> +			      TPS6594_BIT_STOP_RTC);
> +	if (ret < 0)
> +		return ret;

Do that (XTAL_EN and clearing STOP) only once the time is known to be
set to a correct value so read_time doesn't have a chance to return a
bogus value.

> +
> +	mdelay(100);
> +
> +	/*
> +	 * RTC should be running now. Check if this is the case.
> +	 * If not it might be a missing oscillator.
> +	 */
> +	ret = regmap_test_bits(tps6594->regmap, TPS6594_REG_RTC_STATUS,
> +			       TPS6594_BIT_RUN);
> +	if (ret < 0)
> +		return ret;
> +	if (ret == 0)
> +		return -ENODEV;
> +
> +	platform_set_drvdata(pdev, tps_rtc);
> +
> +	irq = platform_get_irq_byname(pdev, TPS6594_IRQ_NAME_ALARM);
> +	if (irq < 0) {
> +		dev_err(&pdev->dev, "Failed to get irq\n");
> +		return irq;
> +	}
> +
> +	ret = devm_request_threaded_irq(&pdev->dev, irq, NULL,
> +					tps6594_rtc_interrupt, IRQF_ONESHOT,
> +					TPS6594_IRQ_NAME_ALARM, &pdev->dev);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "Failed to request_threaded_irq\n");
> +		return ret;
> +	}
> +
> +	tps_rtc->rtc->ops = &tps6594_rtc_ops;
> +	tps_rtc->rtc->range_min = RTC_TIMESTAMP_BEGIN_2000;
> +	tps_rtc->rtc->range_max = RTC_TIMESTAMP_END_2099;
> +
> +	return devm_rtc_register_device(tps_rtc->rtc);
> +}
> +
> +static struct platform_driver tps6594_rtc_driver = {
> +	.probe		= tps6594_rtc_probe,
> +	.driver		= {
> +		.name	= "tps6594-rtc",
> +	},
> +};
> +
> +module_platform_driver(tps6594_rtc_driver);
> +
> +MODULE_ALIAS("platform:tps6594-rtc");
> +MODULE_AUTHOR("Esteban Blanc <eblanc@baylibre.com>");
> +MODULE_DESCRIPTION("TPS6594 RTC driver");
> +MODULE_LICENSE("GPL");
> -- 
> 2.38.1
> 

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH INTERNAL v1 1/3] rtc: tps6594: add driver for TPS6594 PMIC RTC

[Esteban Blanc]

On Tue Mar 7, 2023 at 12:08 PM CET, Alexandre Belloni wrote:
> On 24/02/2023 14:31:27+0100, Esteban Blanc wrote:
> > +struct tps6594_rtc {
> > +   struct rtc_device *rtc;
> > +};
>
> Is the struct actually useful?

Good catch, it's not. I will remove it for V2.

(...)

> > +/*
> > + * Gets current tps6594 RTC time and date parameters.
> > + *
> > + * The RTC's time/alarm representation is not what gmtime(3) requires
> > + * Linux to use:
> > + *
> > + *  - Months are 1..12 vs Linux 0-11
> > + *  - Years are 0..99 vs Linux 1900..N (we assume 21st century)
> > + */
>
> I don't find this comment to be particularly useful.

Ok. I propose that I add 2 constants for the -1 and +100 in the month and year
calculation. This way, without the comment the computation would be a
bit more self explanatory.
What do you think?

(...)

> > +static int tps6594_rtc_probe(struct platform_device *pdev)
> > +{
> > +   struct tps6594 *tps6594;
> > +   struct tps6594_rtc *tps_rtc;
> > +   int irq;
> > +   int ret;
> > +
> > +   tps6594 = dev_get_drvdata(pdev->dev.parent);
> > +
> > +   tps_rtc = devm_kzalloc(&pdev->dev, sizeof(struct tps6594_rtc),
> > +                          GFP_KERNEL);
> > +   if (!tps_rtc)
> > +           return -ENOMEM;
> > +
> > +   tps_rtc->rtc = devm_rtc_allocate_device(&pdev->dev);
> > +   if (IS_ERR(tps_rtc->rtc))
> > +           return PTR_ERR(tps_rtc->rtc);
> > +
> > +   /* Enable crystal oscillator */
> > +   ret = regmap_set_bits(tps6594->regmap, TPS6594_REG_RTC_CTRL_2,
> > +                         TPS6594_BIT_XTAL_EN);
> > +   if (ret < 0)
> > +           return ret;
> > +
> > +   /* Start rtc */
> > +   ret = regmap_set_bits(tps6594->regmap, TPS6594_REG_RTC_CTRL_1,
> > +                         TPS6594_BIT_STOP_RTC);
> > +   if (ret < 0)
> > +           return ret;
>
> Do that (XTAL_EN and clearing STOP) only once the time is known to be
> set to a correct value so read_time doesn't have a chance to return a
> bogus value.
>

(...)

I understand your point, however I'm not sure of the canonical way to do
this. Simply calling `tps6594_rtc_set_time` is enough?

> -- 
> Alexandre Belloni, co-owner and COO, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com

Thanks for your time. Best regards,
-- 
Esteban Blanc
BayLibre

Re: [PATCH INTERNAL v1 1/3] rtc: tps6594: add driver for TPS6594 PMIC RTC

[Alexandre Belloni]

On 13/03/2023 10:18:45+0100, Esteban Blanc wrote:
> On Tue Mar 7, 2023 at 12:08 PM CET, Alexandre Belloni wrote:
> > On 24/02/2023 14:31:27+0100, Esteban Blanc wrote:
> > > +struct tps6594_rtc {
> > > +   struct rtc_device *rtc;
> > > +};
> >
> > Is the struct actually useful?
> 
> Good catch, it's not. I will remove it for V2.
> 
> (...)
> 
> > > +/*
> > > + * Gets current tps6594 RTC time and date parameters.
> > > + *
> > > + * The RTC's time/alarm representation is not what gmtime(3) requires
> > > + * Linux to use:
> > > + *
> > > + *  - Months are 1..12 vs Linux 0-11
> > > + *  - Years are 0..99 vs Linux 1900..N (we assume 21st century)
> > > + */
> >
> > I don't find this comment to be particularly useful.
> 
> Ok. I propose that I add 2 constants for the -1 and +100 in the month and year
> calculation. This way, without the comment the computation would be a
> bit more self explanatory.
> What do you think?

I don't think this is necessary, keep -1 for the month and +100 for the
year, those are very common operations in the subsystem and don't really
need any explanation

> 
> (...)
> 
> > > +static int tps6594_rtc_probe(struct platform_device *pdev)
> > > +{
> > > +   struct tps6594 *tps6594;
> > > +   struct tps6594_rtc *tps_rtc;
> > > +   int irq;
> > > +   int ret;
> > > +
> > > +   tps6594 = dev_get_drvdata(pdev->dev.parent);
> > > +
> > > +   tps_rtc = devm_kzalloc(&pdev->dev, sizeof(struct tps6594_rtc),
> > > +                          GFP_KERNEL);
> > > +   if (!tps_rtc)
> > > +           return -ENOMEM;
> > > +
> > > +   tps_rtc->rtc = devm_rtc_allocate_device(&pdev->dev);
> > > +   if (IS_ERR(tps_rtc->rtc))
> > > +           return PTR_ERR(tps_rtc->rtc);
> > > +
> > > +   /* Enable crystal oscillator */
> > > +   ret = regmap_set_bits(tps6594->regmap, TPS6594_REG_RTC_CTRL_2,
> > > +                         TPS6594_BIT_XTAL_EN);
> > > +   if (ret < 0)
> > > +           return ret;
> > > +
> > > +   /* Start rtc */
> > > +   ret = regmap_set_bits(tps6594->regmap, TPS6594_REG_RTC_CTRL_1,
> > > +                         TPS6594_BIT_STOP_RTC);
> > > +   if (ret < 0)
> > > +           return ret;
> >
> > Do that (XTAL_EN and clearing STOP) only once the time is known to be
> > set to a correct value so read_time doesn't have a chance to return a
> > bogus value.
> >
> 
> (...)
> 
> I understand your point, however I'm not sure of the canonical way to do
> this. Simply calling `tps6594_rtc_set_time` is enough?

Yeah, let userspace set the time and start the rtc at that point.


-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH INTERNAL v1 1/3] rtc: tps6594: add driver for TPS6594 PMIC RTC

[Esteban Blanc]

On Mon Mar 13, 2023 at 12:01 PM CET, Alexandre Belloni wrote:
> On 13/03/2023 10:18:45+0100, Esteban Blanc wrote:
> > On Tue Mar 7, 2023 at 12:08 PM CET, Alexandre Belloni wrote:
> > > On 24/02/2023 14:31:27+0100, Esteban Blanc wrote:
> > > > +/*
> > > > + * Gets current tps6594 RTC time and date parameters.
> > > > + *
> > > > + * The RTC's time/alarm representation is not what gmtime(3) requires
> > > > + * Linux to use:
> > > > + *
> > > > + *  - Months are 1..12 vs Linux 0-11
> > > > + *  - Years are 0..99 vs Linux 1900..N (we assume 21st century)
> > > > + */
> > >
> > > I don't find this comment to be particularly useful.
> > 
> > Ok. I propose that I add 2 constants for the -1 and +100 in the month and year
> > calculation. This way, without the comment the computation would be a
> > bit more self explanatory.
> > What do you think?
>
> I don't think this is necessary, keep -1 for the month and +100 for the
> year, those are very common operations in the subsystem and don't really
> need any explanation

Ok. I will just remove the comment then.

> > > > +static int tps6594_rtc_probe(struct platform_device *pdev)
> > > > +{
> > > > +   struct tps6594 *tps6594;
> > > > +   struct tps6594_rtc *tps_rtc;
> > > > +   int irq;
> > > > +   int ret;
> > > > +
> > > > +   tps6594 = dev_get_drvdata(pdev->dev.parent);
> > > > +
> > > > +   tps_rtc = devm_kzalloc(&pdev->dev, sizeof(struct tps6594_rtc),
> > > > +                          GFP_KERNEL);
> > > > +   if (!tps_rtc)
> > > > +           return -ENOMEM;
> > > > +
> > > > +   tps_rtc->rtc = devm_rtc_allocate_device(&pdev->dev);
> > > > +   if (IS_ERR(tps_rtc->rtc))
> > > > +           return PTR_ERR(tps_rtc->rtc);
> > > > +
> > > > +   /* Enable crystal oscillator */
> > > > +   ret = regmap_set_bits(tps6594->regmap, TPS6594_REG_RTC_CTRL_2,
> > > > +                         TPS6594_BIT_XTAL_EN);
> > > > +   if (ret < 0)
> > > > +           return ret;
> > > > +
> > > > +   /* Start rtc */
> > > > +   ret = regmap_set_bits(tps6594->regmap, TPS6594_REG_RTC_CTRL_1,
> > > > +                         TPS6594_BIT_STOP_RTC);
> > > > +   if (ret < 0)
> > > > +           return ret;
> > >
> > > Do that (XTAL_EN and clearing STOP) only once the time is known to be
> > > set to a correct value so read_time doesn't have a chance to return a
> > > bogus value.
> > >
> > 
> > (...)
> > 
> > I understand your point, however I'm not sure of the canonical way to do
> > this. Simply calling `tps6594_rtc_set_time` is enough?
>
> Yeah, let userspace set the time and start the rtc at that point.

The problem with that is we might have some RTCs that will just not be
usable. We have boards with multiple TP6594 PMICs where only one of them
has a crystal oscillator. The way to detect this is to start the RTC
then checked if the STOP_RTC bit is still 0. By doing this in the probe,
I'm able to not register an RTC device that doesn't work.

If I just start the RTC on the first call to `tps6594_rtc_set_time`, it
will work for the RTC with the crystal and fails for all the others 

I can stop the RTC at the end of the probe, after the check to rule out
unusable devices. If I add the check you proposed in
`tps6594_rtc_read_time` it will fail until a successful call to
`tps6594_rtc_set_time`. Would that be a suitable solution?

Best regards,
-- 
Esteban Blanc
BayLibre

Re: [PATCH INTERNAL v1 1/3] rtc: tps6594: add driver for TPS6594 PMIC RTC

[Alexandre Belloni]

On 13/03/2023 13:10:37+0100, Esteban Blanc wrote:
> On Mon Mar 13, 2023 at 12:01 PM CET, Alexandre Belloni wrote:
> > On 13/03/2023 10:18:45+0100, Esteban Blanc wrote:
> > > On Tue Mar 7, 2023 at 12:08 PM CET, Alexandre Belloni wrote:
> > > > On 24/02/2023 14:31:27+0100, Esteban Blanc wrote:
> > > > > +/*
> > > > > + * Gets current tps6594 RTC time and date parameters.
> > > > > + *
> > > > > + * The RTC's time/alarm representation is not what gmtime(3) requires
> > > > > + * Linux to use:
> > > > > + *
> > > > > + *  - Months are 1..12 vs Linux 0-11
> > > > > + *  - Years are 0..99 vs Linux 1900..N (we assume 21st century)
> > > > > + */
> > > >
> > > > I don't find this comment to be particularly useful.
> > > 
> > > Ok. I propose that I add 2 constants for the -1 and +100 in the month and year
> > > calculation. This way, without the comment the computation would be a
> > > bit more self explanatory.
> > > What do you think?
> >
> > I don't think this is necessary, keep -1 for the month and +100 for the
> > year, those are very common operations in the subsystem and don't really
> > need any explanation
> 
> Ok. I will just remove the comment then.
> 
> > > > > +static int tps6594_rtc_probe(struct platform_device *pdev)
> > > > > +{
> > > > > +   struct tps6594 *tps6594;
> > > > > +   struct tps6594_rtc *tps_rtc;
> > > > > +   int irq;
> > > > > +   int ret;
> > > > > +
> > > > > +   tps6594 = dev_get_drvdata(pdev->dev.parent);
> > > > > +
> > > > > +   tps_rtc = devm_kzalloc(&pdev->dev, sizeof(struct tps6594_rtc),
> > > > > +                          GFP_KERNEL);
> > > > > +   if (!tps_rtc)
> > > > > +           return -ENOMEM;
> > > > > +
> > > > > +   tps_rtc->rtc = devm_rtc_allocate_device(&pdev->dev);
> > > > > +   if (IS_ERR(tps_rtc->rtc))
> > > > > +           return PTR_ERR(tps_rtc->rtc);
> > > > > +
> > > > > +   /* Enable crystal oscillator */
> > > > > +   ret = regmap_set_bits(tps6594->regmap, TPS6594_REG_RTC_CTRL_2,
> > > > > +                         TPS6594_BIT_XTAL_EN);
> > > > > +   if (ret < 0)
> > > > > +           return ret;
> > > > > +
> > > > > +   /* Start rtc */
> > > > > +   ret = regmap_set_bits(tps6594->regmap, TPS6594_REG_RTC_CTRL_1,
> > > > > +                         TPS6594_BIT_STOP_RTC);
> > > > > +   if (ret < 0)
> > > > > +           return ret;
> > > >
> > > > Do that (XTAL_EN and clearing STOP) only once the time is known to be
> > > > set to a correct value so read_time doesn't have a chance to return a
> > > > bogus value.
> > > >
> > > 
> > > (...)
> > > 
> > > I understand your point, however I'm not sure of the canonical way to do
> > > this. Simply calling `tps6594_rtc_set_time` is enough?
> >
> > Yeah, let userspace set the time and start the rtc at that point.
> 
> The problem with that is we might have some RTCs that will just not be
> usable. We have boards with multiple TP6594 PMICs where only one of them
> has a crystal oscillator. The way to detect this is to start the RTC
> then checked if the STOP_RTC bit is still 0. By doing this in the probe,
> I'm able to not register an RTC device that doesn't work.
> 
> If I just start the RTC on the first call to `tps6594_rtc_set_time`, it
> will work for the RTC with the crystal and fails for all the others 
> 
> I can stop the RTC at the end of the probe, after the check to rule out
> unusable devices. If I add the check you proposed in
> `tps6594_rtc_read_time` it will fail until a successful call to
> `tps6594_rtc_set_time`. Would that be a suitable solution?
> 

That would work, yes


-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[PATCH INTERNAL v1 2/3] pinctrl: tps6594: add for TPS6594 PMIC

[Esteban Blanc]

TI TPS6594 PMIC has 11 GPIOs which can be used for different
functions

This add a pinctrl and pinmux drivers in order to use those functions

Signed-off-by: Esteban Blanc <eblanc@baylibre.com>
---
 drivers/pinctrl/Kconfig           |   9 +
 drivers/pinctrl/Makefile          |   1 +
 drivers/pinctrl/pinctrl-tps6594.c | 367 ++++++++++++++++++++++++++++++
 3 files changed, 377 insertions(+)
 create mode 100644 drivers/pinctrl/pinctrl-tps6594.c

diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
index 7d5f5458c72e..323c5b0d63bd 100644
--- a/drivers/pinctrl/Kconfig
+++ b/drivers/pinctrl/Kconfig
@@ -499,6 +499,15 @@ config PINCTRL_THUNDERBAY
 	  rate control and direction control. This module will be
 	  called as pinctrl-thunderbay.
 
+config PINCTRL_TPS6594
+	tristate "Pinctrl and GPIO driver for TI TPS6594 PMIC"
+	depends on MFD_TPS6594
+	select PINMUX
+	select GPIOLIB
+	help
+	  This driver supports the GPIO for the TPS6594 PMICs.
+	  chip family.
+
 config PINCTRL_ZYNQ
 	bool "Pinctrl driver for Xilinx Zynq"
 	depends on ARCH_ZYNQ
diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile
index d5939840bb2a..ba7149883a06 100644
--- a/drivers/pinctrl/Makefile
+++ b/drivers/pinctrl/Makefile
@@ -49,6 +49,7 @@ obj-$(CONFIG_PINCTRL_STMFX) 	+= pinctrl-stmfx.o
 obj-$(CONFIG_PINCTRL_SX150X)	+= pinctrl-sx150x.o
 obj-$(CONFIG_PINCTRL_TB10X)	+= pinctrl-tb10x.o
 obj-$(CONFIG_PINCTRL_THUNDERBAY) += pinctrl-thunderbay.o
+obj-$(CONFIG_PINCTRL_TPS6594) += pinctrl-tps6594.o
 obj-$(CONFIG_PINCTRL_ZYNQMP)	+= pinctrl-zynqmp.o
 obj-$(CONFIG_PINCTRL_ZYNQ)	+= pinctrl-zynq.o
 
diff --git a/drivers/pinctrl/pinctrl-tps6594.c b/drivers/pinctrl/pinctrl-tps6594.c
new file mode 100644
index 000000000000..8decf758ff54
--- /dev/null
+++ b/drivers/pinctrl/pinctrl-tps6594.c
@@ -0,0 +1,367 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Pinmux and GPIO driver for tps6594 PMIC
+ *
+ * Copyright (C) 2022 BayLibre Incorporated - https://www.baylibre.com/
+ */
+
+#define DEBUG
+
+#include <linux/gpio/driver.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/pinctrl/pinmux.h>
+
+#include <linux/mfd/tps6594.h>
+
+#define TPS6594_GPIO_DIR_IN 0
+#define TPS6594_GPIO_DIR_OUT TPS6594_BIT_GPIO_DIR
+#define TPS6594_PINCTRL_PINS_NB 11
+
+#define TPS6594_PINCTRL_GPIO_FUNCTION 0
+#define TPS6594_PINCTRL_SCL_I2C2_CS_SPI_FUNCTION 1
+#define TPS6594_PINCTRL_TRIG_WDOG_FUNCTION 1
+#define TPS6594_PINCTRL_CLK32KOUT_FUNCTION 1
+#define TPS6594_PINCTRL_SCLK_SPMI_FUNCTION 1
+#define TPS6594_PINCTRL_SDATA_SPMI_FUNCTION 1
+#define TPS6594_PINCTRL_NERR_MCU_FUNCTION 1
+#define TPS6594_PINCTRL_PDOG_FUNCTION 1
+#define TPS6594_PINCTRL_SYNCCLKIN_FUNCTION 1
+#define TPS6594_PINCTRL_NRSTOUT_SOC_FUNCTION 2
+#define TPS6594_PINCTRL_SYNCCLKOUT_FUNCTION 2
+#define TPS6594_PINCTRL_SDA_I2C2_SDO_SPI_FUNCTION 2
+#define TPS6594_PINCTRL_NERR_SOC_FUNCTION 2
+#define TPS6594_PINCTRL_DISABLE_WDOG_FUNCTION 3
+#define TPS6594_PINCTRL_NSLEEP1_FUNCTION 4
+#define TPS6594_PINCTRL_NSLEEP2_FUNCTION 5
+#define TPS6594_PINCTRL_WKUP1_FUNCTION 6
+#define TPS6594_PINCTRL_WKUP2_FUNCTION 7
+
+/* Special muxval for recalcitrant pins */
+#define TPS6594_PINCTRL_DISABLE_WDOG_FUNCTION_GPIO8 2
+#define TPS6594_PINCTRL_SYNCCLKOUT_FUNCTION_GPIO8 3
+#define TPS6594_PINCTRL_CLK32KOUT_FUNCTION_GPIO9 3
+
+#define TPS6594_OFFSET_GPIO_SEL 5
+
+static const struct pinctrl_pin_desc tps6594_pins[TPS6594_PINCTRL_PINS_NB] = {
+	PINCTRL_PIN(0, "GPIO0"),   PINCTRL_PIN(1, "GPIO1"),
+	PINCTRL_PIN(2, "GPIO2"),   PINCTRL_PIN(3, "GPIO3"),
+	PINCTRL_PIN(4, "GPIO4"),   PINCTRL_PIN(5, "GPIO5"),
+	PINCTRL_PIN(6, "GPIO6"),   PINCTRL_PIN(7, "GPIO7"),
+	PINCTRL_PIN(8, "GPIO8"),   PINCTRL_PIN(9, "GPIO9"),
+	PINCTRL_PIN(10, "GPIO10"),
+};
+
+static const char *groups_name[TPS6594_PINCTRL_PINS_NB] = {
+	"GPIO0", "GPIO1", "GPIO2", "GPIO3", "GPIO4", "GPIO5",
+	"GPIO6", "GPIO7", "GPIO8", "GPIO9", "GPIO10"
+};
+
+struct tps6594_pinctrl_function {
+	const char *name;
+	u8 muxval;
+	const char **groups;
+	unsigned long ngroups;
+};
+
+static const struct tps6594_pinctrl_function pinctrl_functions[] = {
+	{ "gpio", TPS6594_PINCTRL_GPIO_FUNCTION, groups_name,
+	  TPS6594_PINCTRL_PINS_NB },
+	{ "nsleep1", TPS6594_PINCTRL_NSLEEP1_FUNCTION, groups_name,
+	  TPS6594_PINCTRL_PINS_NB },
+	{ "nsleep2", TPS6594_PINCTRL_NSLEEP2_FUNCTION, groups_name,
+	  TPS6594_PINCTRL_PINS_NB },
+	{ "wkup1", TPS6594_PINCTRL_WKUP1_FUNCTION, groups_name,
+	  TPS6594_PINCTRL_PINS_NB },
+	{ "wkup2", TPS6594_PINCTRL_WKUP2_FUNCTION, groups_name,
+	  TPS6594_PINCTRL_PINS_NB },
+	{ "scl_i2c2-cs_spi", TPS6594_PINCTRL_SCL_I2C2_CS_SPI_FUNCTION,
+	  (const char *[]){ "GPIO0", "GPIO1" }, 2 },
+	{ "nrstout_soc", TPS6594_PINCTRL_NRSTOUT_SOC_FUNCTION,
+	  (const char *[]){ "GPIO0", "GPIO10" }, 2 },
+	{ "trig_wdog", TPS6594_PINCTRL_TRIG_WDOG_FUNCTION,
+	  (const char *[]){ "GPIO1", "GPIO10" }, 2 },
+	{ "sda_i2c2-sdo_spi", TPS6594_PINCTRL_SDA_I2C2_SDO_SPI_FUNCTION,
+	  (const char *[]){ "GPIO1" }, 1 },
+	{ "clk32kout", TPS6594_PINCTRL_CLK32KOUT_FUNCTION,
+	  (const char *[]){ "GPIO2", "GPIO3", "GPIO7" }, 3 },
+	{ "nerr_soc", TPS6594_PINCTRL_NERR_SOC_FUNCTION,
+	  (const char *[]){ "GPIO2" }, 1 },
+	{ "sclk_spmi", TPS6594_PINCTRL_SCLK_SPMI_FUNCTION,
+	  (const char *[]){ "GPIO4" }, 1 },
+	{ "sdata_spmi", TPS6594_PINCTRL_SDATA_SPMI_FUNCTION,
+	  (const char *[]){ "GPIO5" }, 1 },
+	{ "nerr_mcu", TPS6594_PINCTRL_NERR_MCU_FUNCTION,
+	  (const char *[]){ "GPIO6" }, 1 },
+	{ "syncclkout", TPS6594_PINCTRL_SYNCCLKOUT_FUNCTION,
+	  (const char *[]){ "GPIO7", "GPIO9" }, 2 },
+	{ "disable_wdog", TPS6594_PINCTRL_DISABLE_WDOG_FUNCTION,
+	  (const char *[]){ "GPIO7", "GPIO8" }, 2 },
+	{ "pdog", TPS6594_PINCTRL_PDOG_FUNCTION, (const char *[]){ "GPIO8" },
+	  1 },
+	{ "syncclkin", TPS6594_PINCTRL_SYNCCLKIN_FUNCTION,
+	  (const char *[]){ "GPIO9" }, 1 },
+};
+
+struct tps6594_pinctrl {
+	struct tps6594 *tps;
+	struct gpio_chip gpio_chip;
+	struct pinctrl_dev *pctl_dev;
+	const struct tps6594_pinctrl_function *funcs;
+	const struct pinctrl_pin_desc *pins;
+};
+
+static int tps6594_gpio_get(struct gpio_chip *gc, unsigned int offset)
+{
+	struct tps6594_pinctrl *pinctrl = gpiochip_get_data(gc);
+	int ret, val;
+
+	ret = regmap_read(pinctrl->tps->regmap, TPS6594_REG_GPIOX_IN(offset),
+			  &val);
+	if (ret < 0)
+		return ret;
+
+	val = (val & TPS6594_BIT_GPIOX_IN(offset)) > 0;
+
+	return val;
+}
+
+static void tps6594_gpio_set(struct gpio_chip *gc, unsigned int offset,
+			     int value)
+{
+	struct tps6594_pinctrl *pinctrl = gpiochip_get_data(gc);
+	unsigned int set_register = TPS6594_REG_GPIOX_OUT(offset);
+	int ret;
+
+	ret = regmap_update_bits(pinctrl->tps->regmap, set_register,
+				 TPS6594_BIT_GPIOX_OUT(offset),
+				 value ? TPS6594_BIT_GPIOX_OUT(offset) : 0);
+	if (ret < 0)
+		dev_err(pinctrl->tps->dev,
+			"gpio_set failed to set GPIO%d to %d: %d\n", offset,
+			value, ret);
+}
+
+static int tps6594_gpio_get_direction(struct gpio_chip *gc, unsigned int offset)
+{
+	struct tps6594_pinctrl *pinctrl = gpiochip_get_data(gc);
+	int ret;
+
+	ret = regmap_test_bits(pinctrl->tps->regmap,
+			       TPS6594_REG_GPIOX_CONF(offset),
+			       TPS6594_BIT_GPIO_DIR);
+	if (ret < 0) {
+		dev_err(pinctrl->tps->dev,
+			"gpio_get_direction for GPIO%d failed: %d\n", offset,
+			ret);
+		return ret;
+	}
+
+	/*
+	 * TPS6594 direction is 0 = input and 1 = output but Linux direction is 0 = output and
+	 * 1 = input
+	 * Let's invert our value
+	 */
+	return !ret;
+}
+
+static int tps6594_gpio_change_direction(struct gpio_chip *gc,
+					 unsigned int offset,
+					 unsigned int direction)
+{
+	struct tps6594_pinctrl *pinctrl = gpiochip_get_data(gc);
+	int ret;
+
+	ret = regmap_update_bits(pinctrl->tps->regmap,
+				 TPS6594_REG_GPIOX_CONF(offset),
+				 TPS6594_BIT_GPIO_DIR, direction);
+	if (ret < 0)
+		dev_err(pinctrl->tps->dev,
+			"gpio_change_direction for GPIO%d to %u direction failed: %d\n",
+			offset, direction, ret);
+
+	return ret;
+}
+
+static int tps6594_gpio_direction_input(struct gpio_chip *gc,
+					unsigned int offset)
+{
+	return tps6594_gpio_change_direction(gc, offset, TPS6594_GPIO_DIR_IN);
+}
+
+static int tps6594_gpio_direction_output(struct gpio_chip *gc,
+					 unsigned int offset, int value)
+{
+	tps6594_gpio_set(gc, offset, value);
+
+	return tps6594_gpio_change_direction(gc, offset, TPS6594_GPIO_DIR_OUT);
+}
+
+static const struct gpio_chip template_gpio_chip = {
+	.label = "tps6594-gpio",
+	.owner = THIS_MODULE,
+	.get_direction = tps6594_gpio_get_direction,
+	.direction_input = tps6594_gpio_direction_input,
+	.direction_output = tps6594_gpio_direction_output,
+	.get = tps6594_gpio_get,
+	.set = tps6594_gpio_set,
+	.base = -1,
+	.ngpio = TPS6594_PINCTRL_PINS_NB,
+	.can_sleep = true,
+};
+
+static int tps6594_pmx_func_cnt(struct pinctrl_dev *pctldev)
+{
+	return ARRAY_SIZE(pinctrl_functions);
+}
+
+static const char *tps6594_pmx_func_name(struct pinctrl_dev *pctldev,
+					 unsigned int selector)
+{
+	struct tps6594_pinctrl *pinctrl = pinctrl_dev_get_drvdata(pctldev);
+
+	return pinctrl->funcs[selector].name;
+}
+
+static int tps6594_pmx_func_groups(struct pinctrl_dev *pctldev,
+				   unsigned int selector,
+				   const char *const **groups,
+				   unsigned int *num_groups)
+{
+	struct tps6594_pinctrl *pinctrl = pinctrl_dev_get_drvdata(pctldev);
+
+	*groups = pinctrl->funcs[selector].groups;
+	*num_groups = pinctrl->funcs[selector].ngroups;
+
+	return 0;
+}
+
+static int tps6594_pmx_set(struct tps6594_pinctrl *pinctrl, unsigned int pin,
+			   u8 muxval)
+{
+	u8 mux_sel_val = muxval << TPS6594_OFFSET_GPIO_SEL;
+
+	return regmap_update_bits(pinctrl->tps->regmap,
+				  TPS6594_REG_GPIOX_CONF(pin),
+				  TPS6594_MASK_GPIO_SEL, mux_sel_val);
+}
+
+static int tps6594_pmx_set_mux(struct pinctrl_dev *pctldev,
+			       unsigned int function, unsigned int group)
+{
+	struct tps6594_pinctrl *pinctrl = pinctrl_dev_get_drvdata(pctldev);
+	u8 muxval = pinctrl->funcs[function].muxval;
+
+	/* Some pins don't have the same muxval for the same function... */
+	if (group == 8) {
+		if (muxval == TPS6594_PINCTRL_DISABLE_WDOG_FUNCTION)
+			muxval = TPS6594_PINCTRL_DISABLE_WDOG_FUNCTION_GPIO8;
+		else if (muxval == TPS6594_PINCTRL_SYNCCLKOUT_FUNCTION)
+			muxval = TPS6594_PINCTRL_SYNCCLKOUT_FUNCTION_GPIO8;
+	} else if (group == 9) {
+		if (muxval == TPS6594_PINCTRL_CLK32KOUT_FUNCTION)
+			muxval = TPS6594_PINCTRL_CLK32KOUT_FUNCTION_GPIO9;
+	}
+
+	return tps6594_pmx_set(pinctrl, group, muxval);
+}
+
+static int tps6594_pmx_gpio_set_direction(struct pinctrl_dev *pctldev,
+					  struct pinctrl_gpio_range *range,
+					  unsigned int offset, bool input)
+{
+	struct tps6594_pinctrl *pinctrl = pinctrl_dev_get_drvdata(pctldev);
+	u8 muxval = pinctrl->funcs[TPS6594_PINCTRL_GPIO_FUNCTION].muxval;
+
+	return tps6594_pmx_set(pinctrl, offset, muxval);
+}
+
+static const struct pinmux_ops tps6594_pmx_ops = {
+	.get_functions_count = tps6594_pmx_func_cnt,
+	.get_function_name = tps6594_pmx_func_name,
+	.get_function_groups = tps6594_pmx_func_groups,
+	.set_mux = tps6594_pmx_set_mux,
+	.gpio_set_direction = tps6594_pmx_gpio_set_direction,
+	.strict = true,
+};
+
+static int tps6594_groups_cnt(struct pinctrl_dev *pctldev)
+{
+	return ARRAY_SIZE(tps6594_pins);
+}
+
+static int tps6594_group_pins(struct pinctrl_dev *pctldev,
+			      unsigned int selector, const unsigned int **pins,
+			      unsigned int *num_pins)
+{
+	struct tps6594_pinctrl *pinctrl = pinctrl_dev_get_drvdata(pctldev);
+
+	*pins = (unsigned int *)&pinctrl->pins[selector];
+	*num_pins = 1;
+
+	return 0;
+}
+
+static const char *tps6594_group_name(struct pinctrl_dev *pctldev,
+				      unsigned int selector)
+{
+	struct tps6594_pinctrl *pinctrl = pinctrl_dev_get_drvdata(pctldev);
+
+	return pinctrl->pins[selector].name;
+}
+
+static const struct pinctrl_ops tps6594_pctrl_ops = {
+	.dt_node_to_map = pinconf_generic_dt_node_to_map_group,
+	.dt_free_map = pinconf_generic_dt_free_map,
+	.get_groups_count = tps6594_groups_cnt,
+	.get_group_name = tps6594_group_name,
+	.get_group_pins = tps6594_group_pins,
+};
+
+static int tps6594_pinctrl_probe(struct platform_device *pdev)
+{
+	struct tps6594 *tps = dev_get_drvdata(pdev->dev.parent);
+	struct tps6594_pinctrl *pinctrl;
+	struct pinctrl_desc *pctrl_desc;
+
+	pinctrl = devm_kzalloc(&pdev->dev, sizeof(*pinctrl), GFP_KERNEL);
+	if (!pinctrl)
+		return -ENOMEM;
+
+	pinctrl->tps = dev_get_drvdata(pdev->dev.parent);
+	pinctrl->gpio_chip = template_gpio_chip;
+	pinctrl->gpio_chip.parent = tps->dev;
+
+	pctrl_desc = devm_kzalloc(&pdev->dev, sizeof(*pctrl_desc), GFP_KERNEL);
+	if (!pctrl_desc)
+		return -ENOMEM;
+
+	pctrl_desc->name = dev_name(&pdev->dev);
+	pctrl_desc->owner = THIS_MODULE;
+	pctrl_desc->pins = tps6594_pins;
+	pctrl_desc->npins = ARRAY_SIZE(tps6594_pins);
+	pctrl_desc->pctlops = &tps6594_pctrl_ops;
+	pctrl_desc->pmxops = &tps6594_pmx_ops;
+	pinctrl->funcs = pinctrl_functions;
+	pinctrl->pins = tps6594_pins;
+	pinctrl->pctl_dev =
+		devm_pinctrl_register(&pdev->dev, pctrl_desc, pinctrl);
+	if (IS_ERR(pinctrl->pctl_dev)) {
+		dev_err(&pdev->dev, "Couldn't register pinctrl driver\n");
+		return PTR_ERR(pinctrl->pctl_dev);
+	}
+
+	return devm_gpiochip_add_data(&pdev->dev, &pinctrl->gpio_chip, pinctrl);
+}
+
+static struct platform_driver tps6594_pinctrl_driver = {
+	.driver = { .name = "tps6594-pinctrl" },
+	.probe = tps6594_pinctrl_probe,
+};
+module_platform_driver(tps6594_pinctrl_driver);
+
+MODULE_ALIAS("platform:tps6594-pinctrl");
+MODULE_AUTHOR("Esteban Blanc <eblanc@baylibre.com>");
+MODULE_DESCRIPTION("TPS6594 pinctrl and GPIO driver");
+MODULE_LICENSE("GPL");
-- 
2.38.1

Re: [PATCH INTERNAL v1 2/3] pinctrl: tps6594: add for TPS6594 PMIC

[kernel test robot]

Hi Esteban,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on linusw-pinctrl/devel]
[also build test WARNING on linusw-pinctrl/for-next broonie-regulator/for-next abelloni/rtc-next linus/master v6.2 next-20230224]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Esteban-Blanc/rtc-tps6594-add-driver-for-TPS6594-PMIC-RTC/20230224-213323
base:   https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl.git devel
patch link:    https://lore.kernel.org/r/20230224133129.887203-3-eblanc%40baylibre.com
patch subject: [PATCH INTERNAL v1 2/3] pinctrl: tps6594: add for TPS6594 PMIC
config: sparc-allyesconfig (https://download.01.org/0day-ci/archive/20230225/202302250208.XGrlUK3B-lkp@intel.com/config)
compiler: sparc64-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/5abddc736234d1cd3e28ef7e205bc0bfef263c15
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Esteban-Blanc/rtc-tps6594-add-driver-for-TPS6594-PMIC-RTC/20230224-213323
        git checkout 5abddc736234d1cd3e28ef7e205bc0bfef263c15
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=sparc olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=sparc SHELL=/bin/bash drivers/pinctrl/ drivers/regulator/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202302250208.XGrlUK3B-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/pinctrl/pinctrl-tps6594.c:8: warning: "DEBUG" redefined
       8 | #define DEBUG
         | 
   <command-line>: note: this is the location of the previous definition


vim +/DEBUG +8 drivers/pinctrl/pinctrl-tps6594.c

   > 8	#define DEBUG
     9	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

Re: [PATCH INTERNAL v1 2/3] pinctrl: tps6594: add for TPS6594 PMIC

[kernel test robot]

Hi Esteban,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on linusw-pinctrl/devel]
[also build test WARNING on linusw-pinctrl/for-next broonie-regulator/for-next abelloni/rtc-next linus/master v6.2 next-20230225]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Esteban-Blanc/rtc-tps6594-add-driver-for-TPS6594-PMIC-RTC/20230224-213323
base:   https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl.git devel
patch link:    https://lore.kernel.org/r/20230224133129.887203-3-eblanc%40baylibre.com
patch subject: [PATCH INTERNAL v1 2/3] pinctrl: tps6594: add for TPS6594 PMIC
config: x86_64-allmodconfig (https://download.01.org/0day-ci/archive/20230226/202302260423.CycoSTtT-lkp@intel.com/config)
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/5abddc736234d1cd3e28ef7e205bc0bfef263c15
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Esteban-Blanc/rtc-tps6594-add-driver-for-TPS6594-PMIC-RTC/20230224-213323
        git checkout 5abddc736234d1cd3e28ef7e205bc0bfef263c15
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/pinctrl/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202302260423.CycoSTtT-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/pinctrl/pinctrl-tps6594.c:8:9: warning: 'DEBUG' macro redefined [-Wmacro-redefined]
   #define DEBUG
           ^
   <command line>:6:9: note: previous definition is here
   #define DEBUG 1
           ^
   1 warning generated.


vim +/DEBUG +8 drivers/pinctrl/pinctrl-tps6594.c

   > 8	#define DEBUG
     9	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

Re: [PATCH INTERNAL v1 2/3] pinctrl: tps6594: add for TPS6594 PMIC

[Linus Walleij]

Hi Esteban,

thanks for your patch!

On Fri, Feb 24, 2023 at 2:31 PM Esteban Blanc <eblanc@baylibre.com> wrote:

> TI TPS6594 PMIC has 11 GPIOs which can be used for different
> functions
>
> This add a pinctrl and pinmux drivers in order to use those functions
>
> Signed-off-by: Esteban Blanc <eblanc@baylibre.com>

> +config PINCTRL_TPS6594
> +       tristate "Pinctrl and GPIO driver for TI TPS6594 PMIC"
> +       depends on MFD_TPS6594

I would add:

default MFD_TPS6594

so you always get this as module or built in along with the MFD.
Otherwise Kconfig gets complicated and tedious for users.

> +       select PINMUX
> +       select GPIOLIB
> +       help
> +         This driver supports the GPIO for the TPS6594 PMICs.
> +         chip family.

(...)
> +#define DEBUG

Don't put this in production code.

Look in drivers/pinctrl/Kconfig.

config DEBUG_PINCTRL
        bool "Debug PINCTRL calls"
        depends on DEBUG_KERNEL
        help
          Say Y here to add some extra checks and diagnostics to PINCTRL calls.

Look in drivers/pinctrl/Makefile:

subdir-ccflags-$(CONFIG_DEBUG_PINCTRL)  += -DDEBUG

Nifty eh? :D

> +static const struct tps6594_pinctrl_function pinctrl_functions[] = {
(...)
> +       { "scl_i2c2-cs_spi", TPS6594_PINCTRL_SCL_I2C2_CS_SPI_FUNCTION,
> +         (const char *[]){ "GPIO0", "GPIO1" }, 2 },

Ow this is starting to look hairy.

Is there some better way to get here?

Other than this the code looks very nice.

Yours,
Linus Walleij

Re: [PATCH INTERNAL v1 2/3] pinctrl: tps6594: add for TPS6594 PMIC

[Esteban Blanc]

Hi Linus,

On Mon Mar 6, 2023 at 3:10 PM CET, Linus Walleij wrote:
> On Fri, Feb 24, 2023 at 2:31 PM Esteban Blanc <eblanc@baylibre.com> wrote:

> > TI TPS6594 PMIC has 11 GPIOs which can be used for different
> > functions
> >
> > This add a pinctrl and pinmux drivers in order to use those functions
> >
> > Signed-off-by: Esteban Blanc <eblanc@baylibre.com>
>
> > +config PINCTRL_TPS6594
> > +       tristate "Pinctrl and GPIO driver for TI TPS6594 PMIC"
> > +       depends on MFD_TPS6594
>
> I would add:
>
> default MFD_TPS6594
>
> so you always get this as module or built in along with the MFD.
> Otherwise Kconfig gets complicated and tedious for users.

I did not know this, thanks. I will add this to V2.

(...)

> > +#define DEBUG
>
> Don't put this in production code.
>
> Look in drivers/pinctrl/Kconfig.
>
> config DEBUG_PINCTRL
>         bool "Debug PINCTRL calls"
>         depends on DEBUG_KERNEL
>         help
>           Say Y here to add some extra checks and diagnostics to PINCTRL calls.
>
> Look in drivers/pinctrl/Makefile:
>
> subdir-ccflags-$(CONFIG_DEBUG_PINCTRL)  += -DDEBUG
>
> Nifty eh? :D

Nifty indeed :D. #define DEBUG will be removed for V2, I should have
noticed it for V1...

> > +static const struct tps6594_pinctrl_function pinctrl_functions[] = {
> (...)
> > +       { "scl_i2c2-cs_spi", TPS6594_PINCTRL_SCL_I2C2_CS_SPI_FUNCTION,
> > +         (const char *[]){ "GPIO0", "GPIO1" }, 2 },
>
> Ow this is starting to look hairy.
>
> Is there some better way to get here?

I tried with a macro but I was not able to pass an array directly to
avoid both the cast and the hard coded length. I saw on other drivers
that I could generate the table dynamically but I'm not a huge fan of that
and I feel like it would look even more clunky...

>
> Other than this the code looks very nice.
>
> Yours,
> Linus Walleij

[PATCH INTERNAL v1 3/3] regulator: tps6594-regulator: Add driver for TI TPS6594 regulators

[Esteban Blanc]

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>
---
 drivers/regulator/Kconfig             |  12 +
 drivers/regulator/Makefile            |   1 +
 drivers/regulator/tps6594-regulator.c | 559 ++++++++++++++++++++++++++
 3 files changed, 572 insertions(+)
 create mode 100644 drivers/regulator/tps6594-regulator.c

diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index 820c9a0788e5..921540af6958 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -1432,6 +1432,18 @@ config REGULATOR_TPS65219
 	  voltage regulators. It supports software based voltage control
 	  for different voltage domains.
 
+config REGULATOR_TPS6594
+	tristate "TI TPS6594 Power regulators"
+	depends on MFD_TPS6594 && OF
+	help
+	  This driver supports TPS6594 voltage regulator chips.
+	  TPS6594 series of PMICs have 5 BUCKs and 4 LDOs
+	  voltage regulators.
+	  BUCKs 1,2,3,4 can be used in single phase or multiphase mode.
+	  Part number defines which single or multiphase mode is i used.
+	  It supports software based voltage control
+	  for different voltage domains.
+
 config REGULATOR_TPS6524X
 	tristate "TI TPS6524X Power regulators"
 	depends on SPI
diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index b9f5eb35bf5f..948b53f6156b 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -171,6 +171,7 @@ obj-$(CONFIG_REGULATOR_TPS6524X) += tps6524x-regulator.o
 obj-$(CONFIG_REGULATOR_TPS6586X) += tps6586x-regulator.o
 obj-$(CONFIG_REGULATOR_TPS65910) += tps65910-regulator.o
 obj-$(CONFIG_REGULATOR_TPS65912) += tps65912-regulator.o
+obj-$(CONFIG_REGULATOR_TPS6594) += tps6594-regulator.o
 obj-$(CONFIG_REGULATOR_TPS65132) += tps65132-regulator.o
 obj-$(CONFIG_REGULATOR_TPS68470) += tps68470-regulator.o
 obj-$(CONFIG_REGULATOR_TWL4030) += twl-regulator.o twl6030-regulator.o
diff --git a/drivers/regulator/tps6594-regulator.c b/drivers/regulator/tps6594-regulator.c
new file mode 100644
index 000000000000..c099711fd460
--- /dev/null
+++ b/drivers/regulator/tps6594-regulator.c
@@ -0,0 +1,559 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Regulator driver for tps6594 PMIC
+ *
+ * Copyright (C) 2022 BayLibre Incorporated - https://www.baylibre.com/
+ *
+ * This implementation derived from tps65218 authored by "J Keerthy <j-keerthy@ti.com>"
+ */
+
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/regulator/driver.h>
+#include <linux/regulator/machine.h>
+#include <linux/regulator/of_regulator.h>
+
+#include <linux/mfd/tps6594.h>
+
+#define BUCK_NB		5
+#define LDO_NB		4
+#define MULTI_PHASE_NB	4
+
+enum tps6594_regulator_id {
+	/* DCDC's */
+	TPS6594_BUCK_1,
+	TPS6594_BUCK_2,
+	TPS6594_BUCK_3,
+	TPS6594_BUCK_4,
+	TPS6594_BUCK_5,
+
+	/* LDOs */
+	TPS6594_LDO_1,
+	TPS6594_LDO_2,
+	TPS6594_LDO_3,
+	TPS6594_LDO_4,
+};
+
+enum tps6594_multi_regulator_id {
+	/* Multi-phase DCDC's */
+	TPS6594_BUCK_12,
+	TPS6594_BUCK_34,
+	TPS6594_BUCK_123,
+	TPS6594_BUCK_1234,
+};
+
+struct tps6594_regulator_irq_type {
+	const char *irq_name;
+	const char *regulator_name;
+	const char *event_name;
+	unsigned long event;
+};
+
+static struct tps6594_regulator_irq_type tps6594_regulator_irq_types[] = {
+	{ TPS6594_IRQ_NAME_BUCK1_OV, "BUCK1", "overvoltage", REGULATOR_EVENT_OVER_VOLTAGE_WARN },
+	{ TPS6594_IRQ_NAME_BUCK1_UV, "BUCK1", "undervoltage", REGULATOR_EVENT_UNDER_VOLTAGE },
+	{ TPS6594_IRQ_NAME_BUCK1_SC, "BUCK1", "short circuit", REGULATOR_EVENT_REGULATION_OUT },
+	{ TPS6594_IRQ_NAME_BUCK1_ILIM, "BUCK1", "reach ilim, overcurrent",
+	  REGULATOR_EVENT_OVER_CURRENT },
+	{ TPS6594_IRQ_NAME_BUCK2_OV, "BUCK2", "overvoltage", REGULATOR_EVENT_OVER_VOLTAGE_WARN },
+	{ TPS6594_IRQ_NAME_BUCK2_UV, "BUCK2", "undervoltage", REGULATOR_EVENT_UNDER_VOLTAGE },
+	{ TPS6594_IRQ_NAME_BUCK2_SC, "BUCK2", "short circuit", REGULATOR_EVENT_REGULATION_OUT },
+	{ TPS6594_IRQ_NAME_BUCK2_ILIM, "BUCK2", "reach ilim, overcurrent",
+	  REGULATOR_EVENT_OVER_CURRENT },
+	{ TPS6594_IRQ_NAME_BUCK3_OV, "BUCK3", "overvoltage", REGULATOR_EVENT_OVER_VOLTAGE_WARN },
+	{ TPS6594_IRQ_NAME_BUCK3_UV, "BUCK3", "undervoltage", REGULATOR_EVENT_UNDER_VOLTAGE },
+	{ TPS6594_IRQ_NAME_BUCK3_SC, "BUCK3", "short circuit", REGULATOR_EVENT_REGULATION_OUT },
+	{ TPS6594_IRQ_NAME_BUCK3_ILIM, "BUCK3", "reach ilim, overcurrent",
+	  REGULATOR_EVENT_OVER_CURRENT },
+	{ TPS6594_IRQ_NAME_BUCK4_OV, "BUCK4", "overvoltage", REGULATOR_EVENT_OVER_VOLTAGE_WARN },
+	{ TPS6594_IRQ_NAME_BUCK4_UV, "BUCK4", "undervoltage", REGULATOR_EVENT_UNDER_VOLTAGE },
+	{ TPS6594_IRQ_NAME_BUCK4_SC, "BUCK4", "short circuit", REGULATOR_EVENT_REGULATION_OUT },
+	{ TPS6594_IRQ_NAME_BUCK4_ILIM, "BUCK4", "reach ilim, overcurrent",
+	  REGULATOR_EVENT_OVER_CURRENT },
+	{ TPS6594_IRQ_NAME_BUCK5_OV, "BUCK5", "overvoltage", REGULATOR_EVENT_OVER_VOLTAGE_WARN },
+	{ TPS6594_IRQ_NAME_BUCK5_UV, "BUCK5", "undervoltage", REGULATOR_EVENT_UNDER_VOLTAGE },
+	{ TPS6594_IRQ_NAME_BUCK5_SC, "BUCK5", "short circuit", REGULATOR_EVENT_REGULATION_OUT },
+	{ TPS6594_IRQ_NAME_BUCK5_ILIM, "BUCK5", "reach ilim, overcurrent",
+	  REGULATOR_EVENT_OVER_CURRENT },
+	{ TPS6594_IRQ_NAME_LDO1_OV, "LDO1", "overvoltage", REGULATOR_EVENT_OVER_VOLTAGE_WARN },
+	{ TPS6594_IRQ_NAME_LDO1_UV, "LDO1", "undervoltage", REGULATOR_EVENT_UNDER_VOLTAGE },
+	{ TPS6594_IRQ_NAME_LDO1_SC, "LDO1", "short circuit", REGULATOR_EVENT_REGULATION_OUT },
+	{ TPS6594_IRQ_NAME_LDO1_ILIM, "LDO1", "reach ilim, overcurrent",
+	  REGULATOR_EVENT_OVER_CURRENT },
+	{ TPS6594_IRQ_NAME_LDO2_OV, "LDO2", "overvoltage", REGULATOR_EVENT_OVER_VOLTAGE_WARN },
+	{ TPS6594_IRQ_NAME_LDO2_UV, "LDO2", "undervoltage", REGULATOR_EVENT_UNDER_VOLTAGE },
+	{ TPS6594_IRQ_NAME_LDO2_SC, "LDO2", "short circuit", REGULATOR_EVENT_REGULATION_OUT },
+	{ TPS6594_IRQ_NAME_LDO2_ILIM, "LDO2", "reach ilim, overcurrent",
+	  REGULATOR_EVENT_OVER_CURRENT },
+	{ TPS6594_IRQ_NAME_LDO3_OV, "LDO3", "overvoltage", REGULATOR_EVENT_OVER_VOLTAGE_WARN },
+	{ TPS6594_IRQ_NAME_LDO3_UV, "LDO3", "undervoltage", REGULATOR_EVENT_UNDER_VOLTAGE },
+	{ TPS6594_IRQ_NAME_LDO3_SC, "LDO3", "short circuit", REGULATOR_EVENT_REGULATION_OUT },
+	{ TPS6594_IRQ_NAME_LDO3_ILIM, "LDO3", "reach ilim, overcurrent",
+	  REGULATOR_EVENT_OVER_CURRENT },
+	{ TPS6594_IRQ_NAME_LDO4_OV, "LDO4", "overvoltage", REGULATOR_EVENT_OVER_VOLTAGE_WARN },
+	{ TPS6594_IRQ_NAME_LDO4_UV, "LDO4", "undervoltage", REGULATOR_EVENT_UNDER_VOLTAGE },
+	{ TPS6594_IRQ_NAME_LDO4_SC, "LDO4", "short circuit", REGULATOR_EVENT_REGULATION_OUT },
+	{ TPS6594_IRQ_NAME_LDO4_ILIM, "LDO4", "reach ilim, overcurrent",
+	  REGULATOR_EVENT_OVER_CURRENT },
+};
+
+static struct tps6594_regulator_irq_type tps6594_ext_regulator_irq_types[] = {
+	{ TPS6594_IRQ_NAME_VCCA_OV, "VCCA", "overvoltage", REGULATOR_EVENT_OVER_VOLTAGE_WARN },
+	{ TPS6594_IRQ_NAME_VCCA_UV, "VCCA", "undervoltage", REGULATOR_EVENT_UNDER_VOLTAGE },
+	{ TPS6594_IRQ_NAME_VMON1_OV, "VMON1", "overvoltage", REGULATOR_EVENT_OVER_VOLTAGE_WARN },
+	{ TPS6594_IRQ_NAME_VMON1_UV, "VMON1", "undervoltage", REGULATOR_EVENT_UNDER_VOLTAGE },
+	{ TPS6594_IRQ_NAME_VMON1_RV, "VMON1", "residual voltage",
+	  REGULATOR_EVENT_OVER_VOLTAGE_WARN },
+	{ TPS6594_IRQ_NAME_VMON2_OV, "VMON2", "overvoltage", REGULATOR_EVENT_OVER_VOLTAGE_WARN },
+	{ TPS6594_IRQ_NAME_VMON2_UV, "VMON2", "undervoltage", REGULATOR_EVENT_UNDER_VOLTAGE },
+	{ TPS6594_IRQ_NAME_VMON2_RV, "VMON2", "residual voltage",
+	  REGULATOR_EVENT_OVER_VOLTAGE_WARN },
+};
+
+struct tps6594_regulator_irq_data {
+	struct device *dev;
+	struct tps6594_regulator_irq_type *type;
+	struct regulator_dev *rdev;
+};
+
+struct tps6594_ext_regulator_irq_data {
+	struct device *dev;
+	struct tps6594_regulator_irq_type *type;
+};
+
+#define TPS6594_REGULATOR(_name, _of, _id, _type, _ops, _n, _vr, _vm, _er, \
+			   _em, _cr, _cm, _lr, _nlr, _delay, _fuv, \
+			   _ct, _ncl, _bpm) \
+	{								\
+		.name			= _name,			\
+		.of_match		= _of,				\
+		.regulators_node	= of_match_ptr("regulators"),	\
+		.supply_name		= _of,				\
+		.id			= _id,				\
+		.ops			= &(_ops),			\
+		.n_voltages		= _n,				\
+		.type			= _type,			\
+		.owner			= THIS_MODULE,			\
+		.vsel_reg		= _vr,				\
+		.vsel_mask		= _vm,				\
+		.csel_reg		= _cr,				\
+		.csel_mask		= _cm,				\
+		.curr_table		= _ct,				\
+		.n_current_limits	= _ncl,				\
+		.enable_reg		= _er,				\
+		.enable_mask		= _em,				\
+		.volt_table		= NULL,				\
+		.linear_ranges		= _lr,				\
+		.n_linear_ranges	= _nlr,				\
+		.ramp_delay		= _delay,			\
+		.fixed_uV		= _fuv,				\
+		.bypass_reg		= _vr,				\
+		.bypass_mask		= _bpm,				\
+	}								\
+
+static const struct linear_range bucks_ranges[] = {
+	REGULATOR_LINEAR_RANGE(300000, 0x0, 0xe, 20000),
+	REGULATOR_LINEAR_RANGE(600000, 0xf, 0x72, 5000),
+	REGULATOR_LINEAR_RANGE(1100000, 0x73, 0xaa, 10000),
+	REGULATOR_LINEAR_RANGE(1660000, 0xab, 0xff, 20000),
+};
+
+static const struct linear_range ldos_1_2_3_ranges[] = {
+	REGULATOR_LINEAR_RANGE(600000, 0x4, 0x3a, 50000),
+};
+
+static const struct linear_range ldos_4_ranges[] = {
+	REGULATOR_LINEAR_RANGE(1200000, 0x20, 0x74, 25000),
+};
+
+static unsigned int tps6594_get_mode(struct regulator_dev *dev)
+{
+	return REGULATOR_MODE_NORMAL;
+}
+
+/* Operations permitted on BUCK1/2/3/4/5 */
+static const struct regulator_ops tps6594_bucks_ops = {
+	.is_enabled		= regulator_is_enabled_regmap,
+	.enable			= regulator_enable_regmap,
+	.disable		= regulator_disable_regmap,
+	.get_mode		= tps6594_get_mode,
+	.get_voltage_sel	= regulator_get_voltage_sel_regmap,
+	.set_voltage_sel	= regulator_set_voltage_sel_regmap,
+	.list_voltage		= regulator_list_voltage_linear_range,
+	.map_voltage		= regulator_map_voltage_linear_range,
+	.set_voltage_time_sel	= regulator_set_voltage_time_sel,
+
+};
+
+/* Operations permitted on LDO1/2/3 */
+static const struct regulator_ops tps6594_ldos_1_2_3_ops = {
+	.is_enabled		= regulator_is_enabled_regmap,
+	.enable			= regulator_enable_regmap,
+	.disable		= regulator_disable_regmap,
+	.get_mode		= tps6594_get_mode,
+	.get_voltage_sel	= regulator_get_voltage_sel_regmap,
+	.set_voltage_sel	= regulator_set_voltage_sel_regmap,
+	.list_voltage		= regulator_list_voltage_linear_range,
+	.map_voltage		= regulator_map_voltage_linear_range,
+	.set_bypass		= regulator_set_bypass_regmap,
+	.get_bypass		= regulator_get_bypass_regmap,
+};
+
+/* Operations permitted on LDO4 */
+static const struct regulator_ops tps6594_ldos_4_ops = {
+	.is_enabled		= regulator_is_enabled_regmap,
+	.enable			= regulator_enable_regmap,
+	.disable		= regulator_disable_regmap,
+	.get_mode		= tps6594_get_mode,
+	.get_voltage_sel	= regulator_get_voltage_sel_regmap,
+	.set_voltage_sel	= regulator_set_voltage_sel_regmap,
+	.list_voltage		= regulator_list_voltage_linear_range,
+	.map_voltage		= regulator_map_voltage_linear_range,
+};
+
+static const struct regulator_desc buck_regs[] = {
+	TPS6594_REGULATOR("BUCK1", "buck1", TPS6594_BUCK_1,
+			  REGULATOR_VOLTAGE, tps6594_bucks_ops, TPS6594_MASK_BUCKS_VSET,
+			  TPS6594_REG_BUCKX_VOUT_1(0),
+			  TPS6594_MASK_BUCKS_VSET,
+			  TPS6594_REG_BUCKX_CTRL(0),
+			  TPS6594_BIT_BUCK_EN, 0, 0, bucks_ranges,
+			  4, 0, 0, NULL, 0, 0),
+	TPS6594_REGULATOR("BUCK2", "buck2", TPS6594_BUCK_2,
+			  REGULATOR_VOLTAGE, tps6594_bucks_ops, TPS6594_MASK_BUCKS_VSET,
+			  TPS6594_REG_BUCKX_VOUT_1(1),
+			  TPS6594_MASK_BUCKS_VSET,
+			  TPS6594_REG_BUCKX_CTRL(1),
+			  TPS6594_BIT_BUCK_EN, 0, 0, bucks_ranges,
+			  4, 0, 0, NULL, 0, 0),
+	TPS6594_REGULATOR("BUCK3", "buck3", TPS6594_BUCK_3,
+			  REGULATOR_VOLTAGE, tps6594_bucks_ops, TPS6594_MASK_BUCKS_VSET,
+			  TPS6594_REG_BUCKX_VOUT_1(2),
+			  TPS6594_MASK_BUCKS_VSET,
+			  TPS6594_REG_BUCKX_CTRL(2),
+			  TPS6594_BIT_BUCK_EN, 0, 0, bucks_ranges,
+			  4, 0, 0, NULL, 0, 0),
+	TPS6594_REGULATOR("BUCK4", "buck4", TPS6594_BUCK_4,
+			  REGULATOR_VOLTAGE, tps6594_bucks_ops, TPS6594_MASK_BUCKS_VSET,
+			  TPS6594_REG_BUCKX_VOUT_1(3),
+			  TPS6594_MASK_BUCKS_VSET,
+			  TPS6594_REG_BUCKX_CTRL(3),
+			  TPS6594_BIT_BUCK_EN, 0, 0, bucks_ranges,
+			  4, 0, 0, NULL, 0, 0),
+	TPS6594_REGULATOR("BUCK5", "buck5", TPS6594_BUCK_5,
+			  REGULATOR_VOLTAGE, tps6594_bucks_ops, TPS6594_MASK_BUCKS_VSET,
+			  TPS6594_REG_BUCKX_VOUT_1(4),
+			  TPS6594_MASK_BUCKS_VSET,
+			  TPS6594_REG_BUCKX_CTRL(4),
+			  TPS6594_BIT_BUCK_EN, 0, 0, bucks_ranges,
+			  4, 0, 0, NULL, 0, 0),
+};
+
+static const struct regulator_desc multi_regs[] = {
+	TPS6594_REGULATOR("BUCK12", "buck12", TPS6594_BUCK_1,
+			  REGULATOR_VOLTAGE, tps6594_bucks_ops, TPS6594_MASK_BUCKS_VSET,
+			  TPS6594_REG_BUCKX_VOUT_1(1),
+			  TPS6594_MASK_BUCKS_VSET,
+			  TPS6594_REG_BUCKX_CTRL(1),
+			  TPS6594_BIT_BUCK_EN, 0, 0, bucks_ranges,
+			  4, 4000, 0, NULL, 0, 0),
+	TPS6594_REGULATOR("BUCK34", "buck34", TPS6594_BUCK_3,
+			  REGULATOR_VOLTAGE, tps6594_bucks_ops, TPS6594_MASK_BUCKS_VSET,
+			  TPS6594_REG_BUCKX_VOUT_1(3),
+			  TPS6594_MASK_BUCKS_VSET,
+			  TPS6594_REG_BUCKX_CTRL(3),
+			  TPS6594_BIT_BUCK_EN, 0, 0, bucks_ranges,
+			  4, 0, 0, NULL, 0, 0),
+	TPS6594_REGULATOR("BUCK123", "buck123", TPS6594_BUCK_1,
+			  REGULATOR_VOLTAGE, tps6594_bucks_ops, TPS6594_MASK_BUCKS_VSET,
+			  TPS6594_REG_BUCKX_VOUT_1(1),
+			  TPS6594_MASK_BUCKS_VSET,
+			  TPS6594_REG_BUCKX_CTRL(1),
+			  TPS6594_BIT_BUCK_EN, 0, 0, bucks_ranges,
+			  4, 4000, 0, NULL, 0, 0),
+	TPS6594_REGULATOR("BUCK1234", "buck1234", TPS6594_BUCK_1,
+			  REGULATOR_VOLTAGE, tps6594_bucks_ops, TPS6594_MASK_BUCKS_VSET,
+			  TPS6594_REG_BUCKX_VOUT_1(1),
+			  TPS6594_MASK_BUCKS_VSET,
+			  TPS6594_REG_BUCKX_CTRL(1),
+			  TPS6594_BIT_BUCK_EN, 0, 0, bucks_ranges,
+			  4, 4000, 0, NULL, 0, 0),
+};
+
+static const struct regulator_desc ldo_regs[] = {
+	TPS6594_REGULATOR("LDO1", "ldo1", TPS6594_LDO_1,
+			  REGULATOR_VOLTAGE, tps6594_ldos_1_2_3_ops, TPS6594_MASK_LDO123_VSET,
+			  TPS6594_REG_LDOX_VOUT(0),
+			  TPS6594_MASK_LDO123_VSET,
+			  TPS6594_REG_LDOX_CTRL(0),
+			  TPS6594_BIT_LDO_EN, 0, 0, ldos_1_2_3_ranges,
+			  1, 0, 0, NULL, 0, TPS6594_BIT_LDO_BYPASS),
+	TPS6594_REGULATOR("LDO2", "ldo2", TPS6594_LDO_2,
+			  REGULATOR_VOLTAGE, tps6594_ldos_1_2_3_ops, TPS6594_MASK_LDO123_VSET,
+			  TPS6594_REG_LDOX_VOUT(1),
+			  TPS6594_MASK_LDO123_VSET,
+			  TPS6594_REG_LDOX_CTRL(1),
+			  TPS6594_BIT_LDO_EN, 0, 0, ldos_1_2_3_ranges,
+			  1, 0, 0, NULL, 0, TPS6594_BIT_LDO_BYPASS),
+	TPS6594_REGULATOR("LDO3", "ldo3", TPS6594_LDO_3,
+			  REGULATOR_VOLTAGE, tps6594_ldos_1_2_3_ops, TPS6594_MASK_LDO123_VSET,
+			  TPS6594_REG_LDOX_VOUT(2),
+			  TPS6594_MASK_LDO123_VSET,
+			  TPS6594_REG_LDOX_CTRL(2),
+			  TPS6594_BIT_LDO_EN, 0, 0, ldos_1_2_3_ranges,
+			  1, 0, 0, NULL, 0, TPS6594_BIT_LDO_BYPASS),
+	TPS6594_REGULATOR("LDO4", "ldo4", TPS6594_LDO_4,
+			  REGULATOR_VOLTAGE, tps6594_ldos_4_ops, TPS6594_MASK_LDO4_VSET >> 1,
+			  TPS6594_REG_LDOX_VOUT(3),
+			  TPS6594_MASK_LDO4_VSET,
+			  TPS6594_REG_LDOX_CTRL(3),
+			  TPS6594_BIT_LDO_EN, 0, 0, ldos_4_ranges,
+			  1, 0, 0, NULL, 0, 0),
+};
+
+static irqreturn_t tps6594_regulator_irq_handler(int irq, void *data)
+{
+	struct tps6594_regulator_irq_data *irq_data = data;
+
+	if (irq_data->type->event_name[0] == '\0') {
+		/* This is the timeout interrupt no specific regulator */
+		dev_err(irq_data->dev,
+			"System was put in shutdown due to timeout during an active or standby transition.\n");
+		return IRQ_HANDLED;
+	}
+
+	regulator_notifier_call_chain(irq_data->rdev,
+				      irq_data->type->event, NULL);
+
+	dev_err(irq_data->dev, "Error IRQ trap %s for %s\n",
+		irq_data->type->event_name, irq_data->type->regulator_name);
+	return IRQ_HANDLED;
+}
+
+static int tps6594_get_rdev_by_name(const char *regulator_name,
+				    struct regulator_dev *rdevbucktbl[BUCK_NB],
+				    struct regulator_dev *rdevldotbl[LDO_NB],
+				    struct regulator_dev *dev)
+{
+	int i;
+
+	for (i = 0; i <= BUCK_NB; i++) {
+		if (strcmp(regulator_name, buck_regs[i].name) == 0) {
+			dev = rdevbucktbl[i];
+			return 0;
+		}
+	}
+
+	for (i = 0; i < ARRAY_SIZE(ldo_regs); i++) {
+		if (strcmp(regulator_name, ldo_regs[i].name) == 0) {
+			dev = rdevldotbl[i];
+			return 0;
+		}
+	}
+	return -EINVAL;
+}
+
+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")) {
+		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;
+	}
+
+	if (tps->chip_id == LP8764X)
+		/* There is only 4 buck on LP8764X */
+		buck_configured[4] = 1;
+
+	for (i = 0; i < BUCK_NB; i++) {
+		if (buck_configured[i] == 1)
+			continue;
+
+		rdev = devm_regulator_register(&pdev->dev, &buck_regs[i], &config);
+		if (IS_ERR(rdev)) {
+			dev_err(tps->dev, "failed to register %s regulator\n",
+				pdev->name);
+			return PTR_ERR(rdev);
+		}
+		rdevbucktbl[i] = rdev;
+	}
+
+	/* LP8764X dosen't have LDO */
+	if (tps->chip_id != LP8764X) {
+		for (i = 0; i < ARRAY_SIZE(ldo_regs); i++) {
+			rdev = devm_regulator_register(&pdev->dev, &ldo_regs[i], &config);
+			if (IS_ERR(rdev)) {
+				dev_err(tps->dev,
+					"failed to register %s regulator\n",
+					pdev->name);
+				return PTR_ERR(rdev);
+			}
+			rdevldotbl[i] = rdev;
+		}
+	}
+
+	irq_data = devm_kmalloc(tps->dev,
+				ARRAY_SIZE(tps6594_regulator_irq_types) *
+				sizeof(struct tps6594_regulator_irq_data),
+				GFP_KERNEL);
+	if (!irq_data)
+		return -ENOMEM;
+
+	for (i = 0; i < ARRAY_SIZE(tps6594_regulator_irq_types); ++i) {
+		irq_type = &tps6594_regulator_irq_types[i];
+
+		irq = platform_get_irq_byname(pdev, irq_type->irq_name);
+		if (irq < 0)
+			return -EINVAL;
+
+		irq_data[i].dev = tps->dev;
+		irq_data[i].type = irq_type;
+
+		tps6594_get_rdev_by_name(irq_type->regulator_name, rdevbucktbl,
+					 rdevldotbl, rdev);
+
+		if (rdev < 0) {
+			dev_err(tps->dev, "Failed to get rdev for %s\n",
+				irq_type->regulator_name);
+			return -EINVAL;
+		}
+		irq_data[i].rdev = rdev;
+
+		error = devm_request_threaded_irq(tps->dev, irq, NULL,
+						  tps6594_regulator_irq_handler,
+						  IRQF_ONESHOT,
+						  irq_type->irq_name,
+						  &irq_data[i]);
+		if (error) {
+			dev_err(tps->dev, "failed to request %s IRQ %d: %d\n",
+				irq_type->irq_name, irq, error);
+			return error;
+		}
+	}
+
+	if (tps->chip_id == LP8764X)
+		ext_reg_irq_nb = ARRAY_SIZE(tps6594_ext_regulator_irq_types);
+
+	irq_ext_reg_data = devm_kmalloc(tps->dev,
+					ext_reg_irq_nb *
+					sizeof(struct tps6594_ext_regulator_irq_data),
+					GFP_KERNEL);
+	if (!irq_ext_reg_data)
+		return -ENOMEM;
+
+	for (i = 0; i < ext_reg_irq_nb; ++i) {
+		irq_type = &tps6594_ext_regulator_irq_types[i];
+
+		irq = platform_get_irq_byname(pdev, irq_type->irq_name);
+		if (irq < 0)
+			return -EINVAL;
+
+		irq_ext_reg_data[i].dev = tps->dev;
+		irq_ext_reg_data[i].type = irq_type;
+
+		error = devm_request_threaded_irq(tps->dev, irq, NULL,
+						  tps6594_regulator_irq_handler,
+						  IRQF_ONESHOT,
+						  irq_type->irq_name,
+						  &irq_ext_reg_data[i]);
+		if (error) {
+			dev_err(tps->dev, "failed to request %s IRQ %d: %d\n",
+				irq_type->irq_name, irq, error);
+			return error;
+		}
+	}
+
+	return 0;
+}
+
+static struct platform_driver tps6594_regulator_driver = {
+	.driver = {
+		.name = "tps6594-regulator",
+	},
+	.probe = tps6594_regulator_probe,
+};
+
+module_platform_driver(tps6594_regulator_driver);
+
+MODULE_ALIAS("platform:tps6594-regulator");
+MODULE_AUTHOR("Jerome Neanne <jneanne@baylibre.com>");
+MODULE_DESCRIPTION("TPS6594 voltage regulator driver");
+MODULE_LICENSE("GPL");
-- 
2.38.1

Re: [PATCH INTERNAL v1 3/3] regulator: tps6594-regulator: Add driver for TI TPS6594 regulators

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 3117 bytes --]

On Fri, Feb 24, 2023 at 02:31:29PM +0100, 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>
> ---

You've not provided a Signed-off-by for this so I can't do anything with
it, please see Documentation/process/submitting-patches.rst for details
on what this is and why it's important.

> @@ -0,0 +1,559 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Regulator driver for tps6594 PMIC
> + *
> + * Copyright (C) 2022 BayLibre Incorporated - https://www.baylibre.com/

Please make the entire comment block a C++ one so things look more
intentional.

> +static unsigned int tps6594_get_mode(struct regulator_dev *dev)
> +{
> +	return REGULATOR_MODE_NORMAL;
> +}

If configuring modes isn't supported just omit all mode operations.

> +	}
> +
> +	regulator_notifier_call_chain(irq_data->rdev,
> +				      irq_data->type->event, NULL);
> +
> +	dev_err(irq_data->dev, "Error IRQ trap %s for %s\n",
> +		irq_data->type->event_name, irq_data->type->regulator_name);

I suspect it might avoid future confusion to log the error before
notifying so that any consequences of the error more clearly happen in
response to the error.

> +static int tps6594_get_rdev_by_name(const char *regulator_name,
> +				    struct regulator_dev *rdevbucktbl[BUCK_NB],
> +				    struct regulator_dev *rdevldotbl[LDO_NB],
> +				    struct regulator_dev *dev)
> +{
> +	int i;
> +
> +	for (i = 0; i <= BUCK_NB; i++) {
> +		if (strcmp(regulator_name, buck_regs[i].name) == 0) {
> +			dev = rdevbucktbl[i];
> +			return 0;
> +		}
> +	}
> +
> +	for (i = 0; i < ARRAY_SIZE(ldo_regs); i++) {
> +		if (strcmp(regulator_name, ldo_regs[i].name) == 0) {
> +			dev = rdevldotbl[i];
> +			return 0;
> +		}
> +	}
> +	return -EINVAL;
> +}

> +	for (i = 0; i < ARRAY_SIZE(tps6594_regulator_irq_types); ++i) {
> +		irq_type = &tps6594_regulator_irq_types[i];
> +
> +		irq = platform_get_irq_byname(pdev, irq_type->irq_name);
> +		if (irq < 0)
> +			return -EINVAL;
> +
> +		irq_data[i].dev = tps->dev;
> +		irq_data[i].type = irq_type;
> +
> +		tps6594_get_rdev_by_name(irq_type->regulator_name, rdevbucktbl,
> +					 rdevldotbl, rdev);

This would be simpler and you wouldn't need this lookup function if the
regulator descriptions included their IRQ names, then you could just
request the interrupts while registering the regulators.

> +		error = devm_request_threaded_irq(tps->dev, irq, NULL,
> +						  tps6594_regulator_irq_handler,
> +						  IRQF_ONESHOT,
> +						  irq_type->irq_name,
> +						  &irq_data[i]);
> +		if (error) {
> +			dev_err(tps->dev, "failed to request %s IRQ %d: %d\n",
> +				irq_type->irq_name, irq, error);
> +			return error;
> +		}

This leaks all previously requested interrupts.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH INTERNAL v1 3/3] regulator: tps6594-regulator: Add driver for TI TPS6594 regulators

[jerome Neanne]

On 24/02/2023 14:42, Mark Brown wrote:
> On Fri, Feb 24, 2023 at 02:31:29PM +0100, 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>
>> ---
> 
> You've not provided a Signed-off-by for this so I can't do anything with
> it, please see Documentation/process/submitting-patches.rst for details
> on what this is and why it's important.
> 
I did this patch but Esteban sent the whole patch-list. The sign-off has 
not been updated accordingly. Sorry for disturbance. We'll fix that.
>> @@ -0,0 +1,559 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Regulator driver for tps6594 PMIC
>> + *
>> + * Copyright (C) 2022 BayLibre Incorporated - https://www.baylibre.com/
> 
> Please make the entire comment block a C++ one so things look more
> intentional.
> 
>> +static unsigned int tps6594_get_mode(struct regulator_dev *dev)
>> +{
>> +	return REGULATOR_MODE_NORMAL;
>> +}
> 
> If configuring modes isn't supported just omit all mode operations.
> 
>> +	}
>> +
>> +	regulator_notifier_call_chain(irq_data->rdev,
>> +				      irq_data->type->event, NULL);
>> +
>> +	dev_err(irq_data->dev, "Error IRQ trap %s for %s\n",
>> +		irq_data->type->event_name, irq_data->type->regulator_name);
> 
> I suspect it might avoid future confusion to log the error before
> notifying so that any consequences of the error more clearly happen in
> response to the error.
> 
I'll rework all that section for v2 following your recommendations
>> +static int tps6594_get_rdev_by_name(const char *regulator_name,
>> +				    struct regulator_dev *rdevbucktbl[BUCK_NB],
>> +				    struct regulator_dev *rdevldotbl[LDO_NB],
>> +				    struct regulator_dev *dev)
>> +{
>> +	int i;
>> +
>> +	for (i = 0; i <= BUCK_NB; i++) {
>> +		if (strcmp(regulator_name, buck_regs[i].name) == 0) {
>> +			dev = rdevbucktbl[i];
>> +			return 0;
>> +		}
>> +	}
>> +
>> +	for (i = 0; i < ARRAY_SIZE(ldo_regs); i++) {
>> +		if (strcmp(regulator_name, ldo_regs[i].name) == 0) {
>> +			dev = rdevldotbl[i];
>> +			return 0;
>> +		}
>> +	}
>> +	return -EINVAL;
>> +}
> 
>> +	for (i = 0; i < ARRAY_SIZE(tps6594_regulator_irq_types); ++i) {
>> +		irq_type = &tps6594_regulator_irq_types[i];
>> +
>> +		irq = platform_get_irq_byname(pdev, irq_type->irq_name);
>> +		if (irq < 0)
>> +			return -EINVAL;
>> +
>> +		irq_data[i].dev = tps->dev;
>> +		irq_data[i].type = irq_type;
>> +
>> +		tps6594_get_rdev_by_name(irq_type->regulator_name, rdevbucktbl,
>> +					 rdevldotbl, rdev);
> 
> This would be simpler and you wouldn't need this lookup function if the
> regulator descriptions included their IRQ names, then you could just
> request the interrupts while registering the regulators.
> 
>> +		error = devm_request_threaded_irq(tps->dev, irq, NULL,
>> +						  tps6594_regulator_irq_handler,
>> +						  IRQF_ONESHOT,
>> +						  irq_type->irq_name,
>> +						  &irq_data[i]);
>> +		if (error) {
>> +			dev_err(tps->dev, "failed to request %s IRQ %d: %d\n",
>> +				irq_type->irq_name, irq, error);
>> +			return error;
>> +		}
> 
> This leaks all previously requested interrupts.
Thanks for your time and precious feedback.

Re: [PATCH INTERNAL v1 3/3] regulator: tps6594-regulator: Add driver for TI TPS6594 regulators

[jerome Neanne]

>> +static int tps6594_get_rdev_by_name(const char *regulator_name,
>> +				    struct regulator_dev *rdevbucktbl[BUCK_NB],
>> +				    struct regulator_dev *rdevldotbl[LDO_NB],
>> +				    struct regulator_dev *dev)
>> +{
>> +	int i;
>> +
>> +	for (i = 0; i <= BUCK_NB; i++) {
>> +		if (strcmp(regulator_name, buck_regs[i].name) == 0) {
>> +			dev = rdevbucktbl[i];
>> +			return 0;
>> +		}
>> +	}
>> +
>> +	for (i = 0; i < ARRAY_SIZE(ldo_regs); i++) {
>> +		if (strcmp(regulator_name, ldo_regs[i].name) == 0) {
>> +			dev = rdevldotbl[i];
>> +			return 0;
>> +		}
>> +	}
>> +	return -EINVAL;
>> +}
> 
>> +	for (i = 0; i < ARRAY_SIZE(tps6594_regulator_irq_types); ++i) {
>> +		irq_type = &tps6594_regulator_irq_types[i];
>> +
>> +		irq = platform_get_irq_byname(pdev, irq_type->irq_name);
>> +		if (irq < 0)
>> +			return -EINVAL;
>> +
>> +		irq_data[i].dev = tps->dev;
>> +		irq_data[i].type = irq_type;
>> +
>> +		tps6594_get_rdev_by_name(irq_type->regulator_name, rdevbucktbl,
>> +					 rdevldotbl, rdev);
> 
> This would be simpler and you wouldn't need this lookup function if the
> regulator descriptions included their IRQ names, then you could just
> request the interrupts while registering the regulators.
I changed the code to follow your recommendations then now in case of a 
multiphase buck, only one set of interrupt is requested.
ex: multiphase buck1234 single buck5
cat /proc/interrupts:
563:          0          0  tps6594-0-0x4c   0 Edge      buck1_ov
564:          0          0  tps6594-0-0x4c   1 Edge      buck1_uv
565:          0          0  tps6594-0-0x4c   2 Edge      buck1_sc
566:          0          0  tps6594-0-0x4c   3 Edge      buck1_ilim
579:          0          0  tps6594-0-0x4c  16 Edge      buck5_ov
580:          0          0  tps6594-0-0x4c  17 Edge      buck5_uv
581:          0          0  tps6594-0-0x4c  18 Edge      buck5_sc
582:          0          0  tps6594-0-0x4c  19 Edge      buck5_ilim

buck2, buck3, buck4 are not associated to a regulator device because 
buck1 registers control all the multiphase bucks (only one logic 
regulator). Consequently the mapping for the associated interrupts does 
not occur.
I'm not sure it's the right option.
Do you suggest to keep it like that for multiphase?
Is it better to request all the interrupts anyway and map it to the same 
rdev?

> 
>> +		error = devm_request_threaded_irq(tps->dev, irq, NULL,
>> +						  tps6594_regulator_irq_handler,
>> +						  IRQF_ONESHOT,
>> +						  irq_type->irq_name,
>> +						  &irq_data[i]);
>> +		if (error) {
>> +			dev_err(tps->dev, "failed to request %s IRQ %d: %d\n",
>> +				irq_type->irq_name, irq, error);
>> +			return error;
>> +		}
> 
> This leaks all previously requested interrupts.
I'm not sure to understand this sentence correctly. You mean all the 
interrupts already requested are still allocated after the error occurs?

Re: [PATCH INTERNAL v1 3/3] regulator: tps6594-regulator: Add driver for TI TPS6594 regulators

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1646 bytes --]

On Thu, Mar 23, 2023 at 10:12:21AM +0100, jerome Neanne wrote:

> > This would be simpler and you wouldn't need this lookup function if the
> > regulator descriptions included their IRQ names, then you could just
> > request the interrupts while registering the regulators.

> I changed the code to follow your recommendations then now in case of a
> multiphase buck, only one set of interrupt is requested.

> buck2, buck3, buck4 are not associated to a regulator device because buck1
> registers control all the multiphase bucks (only one logic regulator).
> Consequently the mapping for the associated interrupts does not occur.
> I'm not sure it's the right option.
> Do you suggest to keep it like that for multiphase?
> Is it better to request all the interrupts anyway and map it to the same
> rdev?

Do the other interrupts do anything useful for this configuration?  With
a lot of hardware the whole control interface gets merged into one which
includes the interrupts.

> > > +		error = devm_request_threaded_irq(tps->dev, irq, NULL,
> > > +						  tps6594_regulator_irq_handler,
> > > +						  IRQF_ONESHOT,
> > > +						  irq_type->irq_name,
> > > +						  &irq_data[i]);
> > > +		if (error) {
> > > +			dev_err(tps->dev, "failed to request %s IRQ %d: %d\n",
> > > +				irq_type->irq_name, irq, error);
> > > +			return error;
> > > +		}

> > This leaks all previously requested interrupts.

> I'm not sure to understand this sentence correctly. You mean all the
> interrupts already requested are still allocated after the error occurs?

Yes, I'd either not registered the devm or thought there was some other
interrupt wasn't devm.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH INTERNAL v1 3/3] regulator: tps6594-regulator: Add driver for TI TPS6594 regulators

[jerome Neanne]

On 23/03/2023 12:38, Mark Brown wrote:
> On Thu, Mar 23, 2023 at 10:12:21AM +0100, jerome Neanne wrote:
> 
>>> This would be simpler and you wouldn't need this lookup function if the
>>> regulator descriptions included their IRQ names, then you could just
>>> request the interrupts while registering the regulators.
> 
>> I changed the code to follow your recommendations then now in case of a
>> multiphase buck, only one set of interrupt is requested.
> 
>> buck2, buck3, buck4 are not associated to a regulator device because buck1
>> registers control all the multiphase bucks (only one logic regulator).
>> Consequently the mapping for the associated interrupts does not occur.
>> I'm not sure it's the right option.
>> Do you suggest to keep it like that for multiphase?
>> Is it better to request all the interrupts anyway and map it to the same
>> rdev?
> 
> Do the other interrupts do anything useful for this configuration?  With
> a lot of hardware the whole control interface gets merged into one which
> includes the interrupts.
> 
Discussed the point with TI in //. In case of multiphase buck ex: buck12
All the control is delegated to buck1 registers but there is still a 
possibility that an interrupt triggers on buck2 (overcurrent typically).
I slightly changed the logic so that all the interrupts are registered 
even in multiphase mode. In that case interrupts for buck2 are attached 
to rdev buck12.
>>>> +		error = devm_request_threaded_irq(tps->dev, irq, NULL,
>>>> +						  tps6594_regulator_irq_handler,
>>>> +						  IRQF_ONESHOT,
>>>> +						  irq_type->irq_name,
>>>> +						  &irq_data[i]);
>>>> +		if (error) {
>>>> +			dev_err(tps->dev, "failed to request %s IRQ %d: %d\n",
>>>> +				irq_type->irq_name, irq, error);
>>>> +			return error;
>>>> +		}
> 
>>> This leaks all previously requested interrupts.
> 
>> I'm not sure to understand this sentence correctly. You mean all the
>> interrupts already requested are still allocated after the error occurs?
> 
> Yes, I'd either not registered the devm or thought there was some other
> interrupt wasn't devm.
All the interrupts are requested with devm, then should be fine.

Re: [PATCH INTERNAL v1 3/3] regulator: tps6594-regulator: Add driver for TI TPS6594 regulators

[Matti Vaittinen]

Hi Esteban,

On 2/24/23 15: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>
> ---
>   drivers/regulator/Kconfig             |  12 +
>   drivers/regulator/Makefile            |   1 +
>   drivers/regulator/tps6594-regulator.c | 559 ++++++++++++++++++++++++++
>   3 files changed, 572 insertions(+)
>   create mode 100644 drivers/regulator/tps6594-regulator.c
> 
> diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
> index 820c9a0788e5..921540af6958 100644
> --- a/drivers/regulator/Kconfig
> +++ b/drivers/regulator/Kconfig
> @@ -1432,6 +1432,18 @@ config REGULATOR_TPS65219
>   	  voltage regulators. It supports software based voltage control
>   	  for different voltage domains.
>   
> +config REGULATOR_TPS6594
> +	tristate "TI TPS6594 Power regulators"
> +	depends on MFD_TPS6594 && OF
> +	help
> +	  This driver supports TPS6594 voltage regulator chips.
> +	  TPS6594 series of PMICs have 5 BUCKs and 4 LDOs
> +	  voltage regulators.
> +	  BUCKs 1,2,3,4 can be used in single phase or multiphase mode.
> +	  Part number defines which single or multiphase mode is i used.
> +	  It supports software based voltage control
> +	  for different voltage domains.
> +
>   config REGULATOR_TPS6524X
>   	tristate "TI TPS6524X Power regulators"
>   	depends on SPI
> diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
> index b9f5eb35bf5f..948b53f6156b 100644
> --- a/drivers/regulator/Makefile
> +++ b/drivers/regulator/Makefile
> @@ -171,6 +171,7 @@ obj-$(CONFIG_REGULATOR_TPS6524X) += tps6524x-regulator.o
>   obj-$(CONFIG_REGULATOR_TPS6586X) += tps6586x-regulator.o
>   obj-$(CONFIG_REGULATOR_TPS65910) += tps65910-regulator.o
>   obj-$(CONFIG_REGULATOR_TPS65912) += tps65912-regulator.o
> +obj-$(CONFIG_REGULATOR_TPS6594) += tps6594-regulator.o
>   obj-$(CONFIG_REGULATOR_TPS65132) += tps65132-regulator.o
>   obj-$(CONFIG_REGULATOR_TPS68470) += tps68470-regulator.o
>   obj-$(CONFIG_REGULATOR_TWL4030) += twl-regulator.o twl6030-regulator.o
> diff --git a/drivers/regulator/tps6594-regulator.c b/drivers/regulator/tps6594-regulator.c
> new file mode 100644
> index 000000000000..c099711fd460
> --- /dev/null
> +++ b/drivers/regulator/tps6594-regulator.c
> @@ -0,0 +1,559 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Regulator driver for tps6594 PMIC
> + *
> + * Copyright (C) 2022 BayLibre Incorporated - https://www.baylibre.com/
> + *
> + * This implementation derived from tps65218 authored by "J Keerthy <j-keerthy@ti.com>"
> + */
> +
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/driver.h>
> +#include <linux/regulator/machine.h>
> +#include <linux/regulator/of_regulator.h>
> +
> +#include <linux/mfd/tps6594.h>
> +
> +#define BUCK_NB		5
> +#define LDO_NB		4
> +#define MULTI_PHASE_NB	4
> +
> +enum tps6594_regulator_id {
> +	/* DCDC's */
> +	TPS6594_BUCK_1,
> +	TPS6594_BUCK_2,
> +	TPS6594_BUCK_3,
> +	TPS6594_BUCK_4,
> +	TPS6594_BUCK_5,
> +
> +	/* LDOs */
> +	TPS6594_LDO_1,
> +	TPS6594_LDO_2,
> +	TPS6594_LDO_3,
> +	TPS6594_LDO_4,
> +};
> +
> +enum tps6594_multi_regulator_id {
> +	/* Multi-phase DCDC's */
> +	TPS6594_BUCK_12,
> +	TPS6594_BUCK_34,
> +	TPS6594_BUCK_123,
> +	TPS6594_BUCK_1234,
> +};
> +
> +struct tps6594_regulator_irq_type {
> +	const char *irq_name;
> +	const char *regulator_name;
> +	const char *event_name;
> +	unsigned long event;
> +};
> +
> +static struct tps6594_regulator_irq_type tps6594_regulator_irq_types[] = {
> +	{ TPS6594_IRQ_NAME_BUCK1_OV, "BUCK1", "overvoltage", REGULATOR_EVENT_OVER_VOLTAGE_WARN },
> +	{ TPS6594_IRQ_NAME_BUCK1_UV, "BUCK1", "undervoltage", REGULATOR_EVENT_UNDER_VOLTAGE },

You have warning level IRQs - which is cool :)

As warning level IRQs are used for non fatal errors, you probably would 
like to also implement a mechanism for consumers to know when the 
"warning is over" (assuming the HW provides the status information). 
Maybe regulator_get_error_flags() would serve you?

I'd be _really_ interested in hearing if you already have a use-case for 
the warnings.

> +	{ TPS6594_IRQ_NAME_BUCK1_SC, "BUCK1", "short circuit", REGULATOR_EVENT_REGULATION_OUT },
> +	{ TPS6594_IRQ_NAME_BUCK1_ILIM, "BUCK1", "reach ilim, overcurrent",
> +	  REGULATOR_EVENT_OVER_CURRENT },
> +	{ TPS6594_IRQ_NAME_BUCK2_OV, "BUCK2", "overvoltage", REGULATOR_EVENT_OVER_VOLTAGE_WARN },
> +	{ TPS6594_IRQ_NAME_BUCK2_UV, "BUCK2", "undervoltage", REGULATOR_EVENT_UNDER_VOLTAGE },
> +	{ TPS6594_IRQ_NAME_BUCK2_SC, "BUCK2", "short circuit", REGULATOR_EVENT_REGULATION_OUT },
> +	{ TPS6594_IRQ_NAME_BUCK2_ILIM, "BUCK2", "reach ilim, overcurrent",
> +	  REGULATOR_EVENT_OVER_CURRENT },
> +	{ TPS6594_IRQ_NAME_BUCK3_OV, "BUCK3", "overvoltage", REGULATOR_EVENT_OVER_VOLTAGE_WARN },
> +	{ TPS6594_IRQ_NAME_BUCK3_UV, "BUCK3", "undervoltage", REGULATOR_EVENT_UNDER_VOLTAGE },
> +	{ TPS6594_IRQ_NAME_BUCK3_SC, "BUCK3", "short circuit", REGULATOR_EVENT_REGULATION_OUT },
> +	{ TPS6594_IRQ_NAME_BUCK3_ILIM, "BUCK3", "reach ilim, overcurrent",
> +	  REGULATOR_EVENT_OVER_CURRENT },
> +	{ TPS6594_IRQ_NAME_BUCK4_OV, "BUCK4", "overvoltage", REGULATOR_EVENT_OVER_VOLTAGE_WARN },
> +	{ TPS6594_IRQ_NAME_BUCK4_UV, "BUCK4", "undervoltage", REGULATOR_EVENT_UNDER_VOLTAGE },
> +	{ TPS6594_IRQ_NAME_BUCK4_SC, "BUCK4", "short circuit", REGULATOR_EVENT_REGULATION_OUT },
> +	{ TPS6594_IRQ_NAME_BUCK4_ILIM, "BUCK4", "reach ilim, overcurrent",
> +	  REGULATOR_EVENT_OVER_CURRENT },
> +	{ TPS6594_IRQ_NAME_BUCK5_OV, "BUCK5", "overvoltage", REGULATOR_EVENT_OVER_VOLTAGE_WARN },
> +	{ TPS6594_IRQ_NAME_BUCK5_UV, "BUCK5", "undervoltage", REGULATOR_EVENT_UNDER_VOLTAGE },
> +	{ TPS6594_IRQ_NAME_BUCK5_SC, "BUCK5", "short circuit", REGULATOR_EVENT_REGULATION_OUT },
> +	{ TPS6594_IRQ_NAME_BUCK5_ILIM, "BUCK5", "reach ilim, overcurrent",
> +	  REGULATOR_EVENT_OVER_CURRENT },
> +	{ TPS6594_IRQ_NAME_LDO1_OV, "LDO1", "overvoltage", REGULATOR_EVENT_OVER_VOLTAGE_WARN },
> +	{ TPS6594_IRQ_NAME_LDO1_UV, "LDO1", "undervoltage", REGULATOR_EVENT_UNDER_VOLTAGE },
> +	{ TPS6594_IRQ_NAME_LDO1_SC, "LDO1", "short circuit", REGULATOR_EVENT_REGULATION_OUT },
> +	{ TPS6594_IRQ_NAME_LDO1_ILIM, "LDO1", "reach ilim, overcurrent",
> +	  REGULATOR_EVENT_OVER_CURRENT },
> +	{ TPS6594_IRQ_NAME_LDO2_OV, "LDO2", "overvoltage", REGULATOR_EVENT_OVER_VOLTAGE_WARN },
> +	{ TPS6594_IRQ_NAME_LDO2_UV, "LDO2", "undervoltage", REGULATOR_EVENT_UNDER_VOLTAGE },
> +	{ TPS6594_IRQ_NAME_LDO2_SC, "LDO2", "short circuit", REGULATOR_EVENT_REGULATION_OUT },
> +	{ TPS6594_IRQ_NAME_LDO2_ILIM, "LDO2", "reach ilim, overcurrent",
> +	  REGULATOR_EVENT_OVER_CURRENT },
> +	{ TPS6594_IRQ_NAME_LDO3_OV, "LDO3", "overvoltage", REGULATOR_EVENT_OVER_VOLTAGE_WARN },
> +	{ TPS6594_IRQ_NAME_LDO3_UV, "LDO3", "undervoltage", REGULATOR_EVENT_UNDER_VOLTAGE },
> +	{ TPS6594_IRQ_NAME_LDO3_SC, "LDO3", "short circuit", REGULATOR_EVENT_REGULATION_OUT },
> +	{ TPS6594_IRQ_NAME_LDO3_ILIM, "LDO3", "reach ilim, overcurrent",
> +	  REGULATOR_EVENT_OVER_CURRENT },
> +	{ TPS6594_IRQ_NAME_LDO4_OV, "LDO4", "overvoltage", REGULATOR_EVENT_OVER_VOLTAGE_WARN },
> +	{ TPS6594_IRQ_NAME_LDO4_UV, "LDO4", "undervoltage", REGULATOR_EVENT_UNDER_VOLTAGE },
> +	{ TPS6594_IRQ_NAME_LDO4_SC, "LDO4", "short circuit", REGULATOR_EVENT_REGULATION_OUT },
> +	{ TPS6594_IRQ_NAME_LDO4_ILIM, "LDO4", "reach ilim, overcurrent",
> +	  REGULATOR_EVENT_OVER_CURRENT },
> +};
> +
> +static struct tps6594_regulator_irq_type tps6594_ext_regulator_irq_types[] = {
> +	{ TPS6594_IRQ_NAME_VCCA_OV, "VCCA", "overvoltage", REGULATOR_EVENT_OVER_VOLTAGE_WARN },
> +	{ TPS6594_IRQ_NAME_VCCA_UV, "VCCA", "undervoltage", REGULATOR_EVENT_UNDER_VOLTAGE },
> +	{ TPS6594_IRQ_NAME_VMON1_OV, "VMON1", "overvoltage", REGULATOR_EVENT_OVER_VOLTAGE_WARN },
> +	{ TPS6594_IRQ_NAME_VMON1_UV, "VMON1", "undervoltage", REGULATOR_EVENT_UNDER_VOLTAGE },
> +	{ TPS6594_IRQ_NAME_VMON1_RV, "VMON1", "residual voltage",
> +	  REGULATOR_EVENT_OVER_VOLTAGE_WARN },
> +	{ TPS6594_IRQ_NAME_VMON2_OV, "VMON2", "overvoltage", REGULATOR_EVENT_OVER_VOLTAGE_WARN },
> +	{ TPS6594_IRQ_NAME_VMON2_UV, "VMON2", "undervoltage", REGULATOR_EVENT_UNDER_VOLTAGE },
> +	{ TPS6594_IRQ_NAME_VMON2_RV, "VMON2", "residual voltage",
> +	  REGULATOR_EVENT_OVER_VOLTAGE_WARN },
> +};
> +
> +struct tps6594_regulator_irq_data {
> +	struct device *dev;
> +	struct tps6594_regulator_irq_type *type;
> +	struct regulator_dev *rdev;
> +};
> +
> +struct tps6594_ext_regulator_irq_data {
> +	struct device *dev;
> +	struct tps6594_regulator_irq_type *type;
> +};
> +
> +	for (i = 0; i < ARRAY_SIZE(tps6594_regulator_irq_types); ++i) {
> +		irq_type = &tps6594_regulator_irq_types[i];
> +
> +		irq = platform_get_irq_byname(pdev, irq_type->irq_name);
> +		if (irq < 0)
> +			return -EINVAL;
> +
> +		irq_data[i].dev = tps->dev;
> +		irq_data[i].type = irq_type;
> +
> +		tps6594_get_rdev_by_name(irq_type->regulator_name, rdevbucktbl,
> +					 rdevldotbl, rdev);
> +
> +		if (rdev < 0) {
> +			dev_err(tps->dev, "Failed to get rdev for %s\n",
> +				irq_type->regulator_name);
> +			return -EINVAL;
> +		}
> +		irq_data[i].rdev = rdev;
> +
> +		error = devm_request_threaded_irq(tps->dev, irq, NULL,
> +						  tps6594_regulator_irq_handler,
> +						  IRQF_ONESHOT,
> +						  irq_type->irq_name,
> +						  &irq_data[i]);
> +		if (error) {
> +			dev_err(tps->dev, "failed to request %s IRQ %d: %d\n",
> +				irq_type->irq_name, irq, error);
> +			return error;
> +		}
> +	}

If I read this correctly, you have exact, 1 to 1 mapping from an IRQ to 
regulator and event? Maybe you could slightly simplify the driver by 
using the devm_regulator_irq_helper() and 
regulator_irq_map_event_simple() with it's map-event? And if the 
devm_regulator_irq_helper() does not work for you I'll be interested in 
hearing if it could be improved.

> +
> +	if (tps->chip_id == LP8764X)
> +		ext_reg_irq_nb = ARRAY_SIZE(tps6594_ext_regulator_irq_types);
> +
> +	irq_ext_reg_data = devm_kmalloc(tps->dev,
> +					ext_reg_irq_nb *
> +					sizeof(struct tps6594_ext_regulator_irq_data),
> +					GFP_KERNEL);
> +	if (!irq_ext_reg_data)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < ext_reg_irq_nb; ++i) {
> +		irq_type = &tps6594_ext_regulator_irq_types[i];
> +
> +		irq = platform_get_irq_byname(pdev, irq_type->irq_name);
> +		if (irq < 0)
> +			return -EINVAL;
> +
> +		irq_ext_reg_data[i].dev = tps->dev;
> +		irq_ext_reg_data[i].type = irq_type;
> +
> +		error = devm_request_threaded_irq(tps->dev, irq, NULL,
> +						  tps6594_regulator_irq_handler,
> +						  IRQF_ONESHOT,
> +						  irq_type->irq_name,
> +						  &irq_ext_reg_data[i]);
> +		if (error) {
> +			dev_err(tps->dev, "failed to request %s IRQ %d: %d\n",
> +				irq_type->irq_name, irq, error);
> +			return error;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static struct platform_driver tps6594_regulator_driver = {
> +	.driver = {
> +		.name = "tps6594-regulator",
> +	},
> +	.probe = tps6594_regulator_probe,
> +};
> +
> +module_platform_driver(tps6594_regulator_driver);
> +
> +MODULE_ALIAS("platform:tps6594-regulator");
> +MODULE_AUTHOR("Jerome Neanne <jneanne@baylibre.com>");
> +MODULE_DESCRIPTION("TPS6594 voltage regulator driver");
> +MODULE_LICENSE("GPL");

Yours,
	-- Matti

-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~

Re: [PATCH INTERNAL v1 3/3] regulator: tps6594-regulator: Add driver for TI TPS6594 regulators

[jerome Neanne]

On 24/02/2023 15:05, Matti Vaittinen wrote:
> Hi Esteban,
> 
> On 2/24/23 15: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>
>> ---
>>   drivers/regulator/Kconfig             |  12 +
>>   drivers/regulator/Makefile            |   1 +
>>   drivers/regulator/tps6594-regulator.c | 559 ++++++++++++++++++++++++++
>>   3 files changed, 572 insertions(+)
>>   create mode 100644 drivers/regulator/tps6594-regulator.c
>>
>> diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
>> index 820c9a0788e5..921540af6958 100644
>> --- a/drivers/regulator/Kconfig
>> +++ b/drivers/regulator/Kconfig
>> @@ -1432,6 +1432,18 @@ config REGULATOR_TPS65219
>>         voltage regulators. It supports software based voltage control
>>         for different voltage domains.
>> +config REGULATOR_TPS6594
>> +    tristate "TI TPS6594 Power regulators"
>> +    depends on MFD_TPS6594 && OF
>> +    help
>> +      This driver supports TPS6594 voltage regulator chips.
>> +      TPS6594 series of PMICs have 5 BUCKs and 4 LDOs
>> +      voltage regulators.
>> +      BUCKs 1,2,3,4 can be used in single phase or multiphase mode.
>> +      Part number defines which single or multiphase mode is i used.
>> +      It supports software based voltage control
>> +      for different voltage domains.
>> +
>>   config REGULATOR_TPS6524X
>>       tristate "TI TPS6524X Power regulators"
>>       depends on SPI
>> diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
>> index b9f5eb35bf5f..948b53f6156b 100644
>> --- a/drivers/regulator/Makefile
>> +++ b/drivers/regulator/Makefile
>> @@ -171,6 +171,7 @@ obj-$(CONFIG_REGULATOR_TPS6524X) += 
>> tps6524x-regulator.o
>>   obj-$(CONFIG_REGULATOR_TPS6586X) += tps6586x-regulator.o
>>   obj-$(CONFIG_REGULATOR_TPS65910) += tps65910-regulator.o
>>   obj-$(CONFIG_REGULATOR_TPS65912) += tps65912-regulator.o
>> +obj-$(CONFIG_REGULATOR_TPS6594) += tps6594-regulator.o
>>   obj-$(CONFIG_REGULATOR_TPS65132) += tps65132-regulator.o
>>   obj-$(CONFIG_REGULATOR_TPS68470) += tps68470-regulator.o
>>   obj-$(CONFIG_REGULATOR_TWL4030) += twl-regulator.o twl6030-regulator.o
>> diff --git a/drivers/regulator/tps6594-regulator.c 
>> b/drivers/regulator/tps6594-regulator.c
>> new file mode 100644
>> index 000000000000..c099711fd460
>> --- /dev/null
>> +++ b/drivers/regulator/tps6594-regulator.c
>> @@ -0,0 +1,559 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Regulator driver for tps6594 PMIC
>> + *
>> + * Copyright (C) 2022 BayLibre Incorporated - https://www.baylibre.com/
>> + *
>> + * This implementation derived from tps65218 authored by "J Keerthy 
>> <j-keerthy@ti.com>"
>> + */
>> +
>> +#include <linux/device.h>
>> +#include <linux/err.h>
>> +#include <linux/init.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/of_device.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/regmap.h>
>> +#include <linux/regulator/driver.h>
>> +#include <linux/regulator/machine.h>
>> +#include <linux/regulator/of_regulator.h>
>> +
>> +#include <linux/mfd/tps6594.h>
>> +
>> +#define BUCK_NB        5
>> +#define LDO_NB        4
>> +#define MULTI_PHASE_NB    4
>> +
>> +enum tps6594_regulator_id {
>> +    /* DCDC's */
>> +    TPS6594_BUCK_1,
>> +    TPS6594_BUCK_2,
>> +    TPS6594_BUCK_3,
>> +    TPS6594_BUCK_4,
>> +    TPS6594_BUCK_5,
>> +
>> +    /* LDOs */
>> +    TPS6594_LDO_1,
>> +    TPS6594_LDO_2,
>> +    TPS6594_LDO_3,
>> +    TPS6594_LDO_4,
>> +};
>> +
>> +enum tps6594_multi_regulator_id {
>> +    /* Multi-phase DCDC's */
>> +    TPS6594_BUCK_12,
>> +    TPS6594_BUCK_34,
>> +    TPS6594_BUCK_123,
>> +    TPS6594_BUCK_1234,
>> +};
>> +
>> +struct tps6594_regulator_irq_type {
>> +    const char *irq_name;
>> +    const char *regulator_name;
>> +    const char *event_name;
>> +    unsigned long event;
>> +};
>> +
>> +static struct tps6594_regulator_irq_type 
>> tps6594_regulator_irq_types[] = {
>> +    { TPS6594_IRQ_NAME_BUCK1_OV, "BUCK1", "overvoltage", 
>> REGULATOR_EVENT_OVER_VOLTAGE_WARN },
>> +    { TPS6594_IRQ_NAME_BUCK1_UV, "BUCK1", "undervoltage", 
>> REGULATOR_EVENT_UNDER_VOLTAGE },
> 
> You have warning level IRQs - which is cool :)
> 
> As warning level IRQs are used for non fatal errors, you probably would 
> like to also implement a mechanism for consumers to know when the 
> "warning is over" (assuming the HW provides the status information). 
> Maybe regulator_get_error_flags() would serve you?
> 
> I'd be _really_ interested in hearing if you already have a use-case for 
> the warnings.
I double checked with TI PMIC team and so far we don't have any routine.
The requirement for upstream driver is to raise the warning to the 
processor nothing else. Up to the final customer to customize further.
Note that it can be dangerous to handle in sw it in a generic way since 
those warnings might affect some regulators that is supplying some 
resources needed by the processor for correct behavior...
> 
>> +    { TPS6594_IRQ_NAME_BUCK1_SC, "BUCK1", "short circuit", 
>> REGULATOR_EVENT_REGULATION_OUT },
>> +    { TPS6594_IRQ_NAME_BUCK1_ILIM, "BUCK1", "reach ilim, overcurrent",
>> +      REGULATOR_EVENT_OVER_CURRENT },
>> +    { TPS6594_IRQ_NAME_BUCK2_OV, "BUCK2", "overvoltage", 
>> REGULATOR_EVENT_OVER_VOLTAGE_WARN },
>> +    { TPS6594_IRQ_NAME_BUCK2_UV, "BUCK2", "undervoltage", 
>> REGULATOR_EVENT_UNDER_VOLTAGE },
>> +    { TPS6594_IRQ_NAME_BUCK2_SC, "BUCK2", "short circuit", 
>> REGULATOR_EVENT_REGULATION_OUT },
>> +    { TPS6594_IRQ_NAME_BUCK2_ILIM, "BUCK2", "reach ilim, overcurrent",
>> +      REGULATOR_EVENT_OVER_CURRENT },
>> +    { TPS6594_IRQ_NAME_BUCK3_OV, "BUCK3", "overvoltage", 
>> REGULATOR_EVENT_OVER_VOLTAGE_WARN },
>> +    { TPS6594_IRQ_NAME_BUCK3_UV, "BUCK3", "undervoltage", 
>> REGULATOR_EVENT_UNDER_VOLTAGE },
>> +    { TPS6594_IRQ_NAME_BUCK3_SC, "BUCK3", "short circuit", 
>> REGULATOR_EVENT_REGULATION_OUT },
>> +    { TPS6594_IRQ_NAME_BUCK3_ILIM, "BUCK3", "reach ilim, overcurrent",
>> +      REGULATOR_EVENT_OVER_CURRENT },
>> +    { TPS6594_IRQ_NAME_BUCK4_OV, "BUCK4", "overvoltage", 
>> REGULATOR_EVENT_OVER_VOLTAGE_WARN },
>> +    { TPS6594_IRQ_NAME_BUCK4_UV, "BUCK4", "undervoltage", 
>> REGULATOR_EVENT_UNDER_VOLTAGE },
>> +    { TPS6594_IRQ_NAME_BUCK4_SC, "BUCK4", "short circuit", 
>> REGULATOR_EVENT_REGULATION_OUT },
>> +    { TPS6594_IRQ_NAME_BUCK4_ILIM, "BUCK4", "reach ilim, overcurrent",
>> +      REGULATOR_EVENT_OVER_CURRENT },
>> +    { TPS6594_IRQ_NAME_BUCK5_OV, "BUCK5", "overvoltage", 
>> REGULATOR_EVENT_OVER_VOLTAGE_WARN },
>> +    { TPS6594_IRQ_NAME_BUCK5_UV, "BUCK5", "undervoltage", 
>> REGULATOR_EVENT_UNDER_VOLTAGE },
>> +    { TPS6594_IRQ_NAME_BUCK5_SC, "BUCK5", "short circuit", 
>> REGULATOR_EVENT_REGULATION_OUT },
>> +    { TPS6594_IRQ_NAME_BUCK5_ILIM, "BUCK5", "reach ilim, overcurrent",
>> +      REGULATOR_EVENT_OVER_CURRENT },
>> +    { TPS6594_IRQ_NAME_LDO1_OV, "LDO1", "overvoltage", 
>> REGULATOR_EVENT_OVER_VOLTAGE_WARN },
>> +    { TPS6594_IRQ_NAME_LDO1_UV, "LDO1", "undervoltage", 
>> REGULATOR_EVENT_UNDER_VOLTAGE },
>> +    { TPS6594_IRQ_NAME_LDO1_SC, "LDO1", "short circuit", 
>> REGULATOR_EVENT_REGULATION_OUT },
>> +    { TPS6594_IRQ_NAME_LDO1_ILIM, "LDO1", "reach ilim, overcurrent",
>> +      REGULATOR_EVENT_OVER_CURRENT },
>> +    { TPS6594_IRQ_NAME_LDO2_OV, "LDO2", "overvoltage", 
>> REGULATOR_EVENT_OVER_VOLTAGE_WARN },
>> +    { TPS6594_IRQ_NAME_LDO2_UV, "LDO2", "undervoltage", 
>> REGULATOR_EVENT_UNDER_VOLTAGE },
>> +    { TPS6594_IRQ_NAME_LDO2_SC, "LDO2", "short circuit", 
>> REGULATOR_EVENT_REGULATION_OUT },
>> +    { TPS6594_IRQ_NAME_LDO2_ILIM, "LDO2", "reach ilim, overcurrent",
>> +      REGULATOR_EVENT_OVER_CURRENT },
>> +    { TPS6594_IRQ_NAME_LDO3_OV, "LDO3", "overvoltage", 
>> REGULATOR_EVENT_OVER_VOLTAGE_WARN },
>> +    { TPS6594_IRQ_NAME_LDO3_UV, "LDO3", "undervoltage", 
>> REGULATOR_EVENT_UNDER_VOLTAGE },
>> +    { TPS6594_IRQ_NAME_LDO3_SC, "LDO3", "short circuit", 
>> REGULATOR_EVENT_REGULATION_OUT },
>> +    { TPS6594_IRQ_NAME_LDO3_ILIM, "LDO3", "reach ilim, overcurrent",
>> +      REGULATOR_EVENT_OVER_CURRENT },
>> +    { TPS6594_IRQ_NAME_LDO4_OV, "LDO4", "overvoltage", 
>> REGULATOR_EVENT_OVER_VOLTAGE_WARN },
>> +    { TPS6594_IRQ_NAME_LDO4_UV, "LDO4", "undervoltage", 
>> REGULATOR_EVENT_UNDER_VOLTAGE },
>> +    { TPS6594_IRQ_NAME_LDO4_SC, "LDO4", "short circuit", 
>> REGULATOR_EVENT_REGULATION_OUT },
>> +    { TPS6594_IRQ_NAME_LDO4_ILIM, "LDO4", "reach ilim, overcurrent",
>> +      REGULATOR_EVENT_OVER_CURRENT },
>> +};
>> +
>> +static struct tps6594_regulator_irq_type 
>> tps6594_ext_regulator_irq_types[] = {
>> +    { TPS6594_IRQ_NAME_VCCA_OV, "VCCA", "overvoltage", 
>> REGULATOR_EVENT_OVER_VOLTAGE_WARN },
>> +    { TPS6594_IRQ_NAME_VCCA_UV, "VCCA", "undervoltage", 
>> REGULATOR_EVENT_UNDER_VOLTAGE },
>> +    { TPS6594_IRQ_NAME_VMON1_OV, "VMON1", "overvoltage", 
>> REGULATOR_EVENT_OVER_VOLTAGE_WARN },
>> +    { TPS6594_IRQ_NAME_VMON1_UV, "VMON1", "undervoltage", 
>> REGULATOR_EVENT_UNDER_VOLTAGE },
>> +    { TPS6594_IRQ_NAME_VMON1_RV, "VMON1", "residual voltage",
>> +      REGULATOR_EVENT_OVER_VOLTAGE_WARN },
>> +    { TPS6594_IRQ_NAME_VMON2_OV, "VMON2", "overvoltage", 
>> REGULATOR_EVENT_OVER_VOLTAGE_WARN },
>> +    { TPS6594_IRQ_NAME_VMON2_UV, "VMON2", "undervoltage", 
>> REGULATOR_EVENT_UNDER_VOLTAGE },
>> +    { TPS6594_IRQ_NAME_VMON2_RV, "VMON2", "residual voltage",
>> +      REGULATOR_EVENT_OVER_VOLTAGE_WARN },
>> +};
>> +
>> +struct tps6594_regulator_irq_data {
>> +    struct device *dev;
>> +    struct tps6594_regulator_irq_type *type;
>> +    struct regulator_dev *rdev;
>> +};
>> +
>> +struct tps6594_ext_regulator_irq_data {
>> +    struct device *dev;
>> +    struct tps6594_regulator_irq_type *type;
>> +};
>> +
>> +    for (i = 0; i < ARRAY_SIZE(tps6594_regulator_irq_types); ++i) {
>> +        irq_type = &tps6594_regulator_irq_types[i];
>> +
>> +        irq = platform_get_irq_byname(pdev, irq_type->irq_name);
>> +        if (irq < 0)
>> +            return -EINVAL;
>> +
>> +        irq_data[i].dev = tps->dev;
>> +        irq_data[i].type = irq_type;
>> +
>> +        tps6594_get_rdev_by_name(irq_type->regulator_name, rdevbucktbl,
>> +                     rdevldotbl, rdev);
>> +
>> +        if (rdev < 0) {
>> +            dev_err(tps->dev, "Failed to get rdev for %s\n",
>> +                irq_type->regulator_name);
>> +            return -EINVAL;
>> +        }
>> +        irq_data[i].rdev = rdev;
>> +
>> +        error = devm_request_threaded_irq(tps->dev, irq, NULL,
>> +                          tps6594_regulator_irq_handler,
>> +                          IRQF_ONESHOT,
>> +                          irq_type->irq_name,
>> +                          &irq_data[i]);
>> +        if (error) {
>> +            dev_err(tps->dev, "failed to request %s IRQ %d: %d\n",
>> +                irq_type->irq_name, irq, error);
>> +            return error;
>> +        }
>> +    }
> 
> If I read this correctly, you have exact, 1 to 1 mapping from an IRQ to 
> regulator and event? Maybe you could slightly simplify the driver by 
> using the devm_regulator_irq_helper() and 
> regulator_irq_map_event_simple() with it's map-event? And if the 
> devm_regulator_irq_helper() does not work for you I'll be interested in 
> hearing if it could be improved.
> 
I'll give it a try.

Thanks

Re: [PATCH INTERNAL v1 3/3] regulator: tps6594-regulator: Add driver for TI TPS6594 regulators

[kernel test robot]

Hi Esteban,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on linusw-pinctrl/devel]
[also build test WARNING on linusw-pinctrl/for-next broonie-regulator/for-next abelloni/rtc-next linus/master v6.2 next-20230224]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Esteban-Blanc/rtc-tps6594-add-driver-for-TPS6594-PMIC-RTC/20230224-213323
base:   https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl.git devel
patch link:    https://lore.kernel.org/r/20230224133129.887203-4-eblanc%40baylibre.com
patch subject: [PATCH INTERNAL v1 3/3] regulator: tps6594-regulator: Add driver for TI TPS6594 regulators
config: sparc-allyesconfig (https://download.01.org/0day-ci/archive/20230225/202302250541.p9Kg2Tc6-lkp@intel.com/config)
compiler: sparc64-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/740925ed262d27dda4f7b9af4c0173a845fa0578
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Esteban-Blanc/rtc-tps6594-add-driver-for-TPS6594-PMIC-RTC/20230224-213323
        git checkout 740925ed262d27dda4f7b9af4c0173a845fa0578
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=sparc olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=sparc SHELL=/bin/bash drivers/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202302250541.p9Kg2Tc6-lkp@intel.com/

All warnings (new ones prefixed by >>):

   drivers/regulator/tps6594-regulator.c: In function 'tps6594_get_rdev_by_name':
>> drivers/regulator/tps6594-regulator.c:342:59: warning: parameter 'dev' set but not used [-Wunused-but-set-parameter]
     342 |                                     struct regulator_dev *dev)
         |                                     ~~~~~~~~~~~~~~~~~~~~~~^~~
   drivers/regulator/tps6594-regulator.c: In function 'tps6594_regulator_probe':
>> drivers/regulator/tps6594-regulator.c:493:26: warning: ordered comparison of pointer with integer zero [-Wextra]
     493 |                 if (rdev < 0) {
         |                          ^
>> drivers/regulator/tps6594-regulator.c:381:13: warning: variable 'multi_phase_case' set but not used [-Wunused-but-set-variable]
     381 |         int multi_phase_case = 0xFFFF;
         |             ^~~~~~~~~~~~~~~~
>> drivers/regulator/tps6594-regulator.c:377:31: warning: variable 'rdevmultitbl' set but not used [-Wunused-but-set-variable]
     377 |         struct regulator_dev *rdevmultitbl[MULTI_PHASE_NB];
         |                               ^~~~~~~~~~~~


vim +/dev +342 drivers/regulator/tps6594-regulator.c

   338	
   339	static int tps6594_get_rdev_by_name(const char *regulator_name,
   340					    struct regulator_dev *rdevbucktbl[BUCK_NB],
   341					    struct regulator_dev *rdevldotbl[LDO_NB],
 > 342					    struct regulator_dev *dev)
   343	{
   344		int i;
   345	
   346		for (i = 0; i <= BUCK_NB; i++) {
   347			if (strcmp(regulator_name, buck_regs[i].name) == 0) {
   348				dev = rdevbucktbl[i];
   349				return 0;
   350			}
   351		}
   352	
   353		for (i = 0; i < ARRAY_SIZE(ldo_regs); i++) {
   354			if (strcmp(regulator_name, ldo_regs[i].name) == 0) {
   355				dev = rdevldotbl[i];
   356				return 0;
   357			}
   358		}
   359		return -EINVAL;
   360	}
   361	
   362	static int tps6594_regulator_probe(struct platform_device *pdev)
   363	{
   364		struct tps6594 *tps = dev_get_drvdata(pdev->dev.parent);
   365		struct regulator_dev *rdev;
   366		struct regulator_config config = {};
   367		u8 buck_configured[BUCK_NB] = { 0 };
   368		u8 buck_multi[MULTI_PHASE_NB] = { 0 };
   369		int i;
   370		int error;
   371		int irq;
   372		int ext_reg_irq_nb = 2;
   373		struct tps6594_regulator_irq_data *irq_data;
   374		struct tps6594_ext_regulator_irq_data *irq_ext_reg_data;
   375		struct tps6594_regulator_irq_type *irq_type;
   376		struct regulator_dev *rdevbucktbl[BUCK_NB];
 > 377		struct regulator_dev *rdevmultitbl[MULTI_PHASE_NB];
   378		struct regulator_dev *rdevldotbl[LDO_NB];
   379	
   380		int multi_phase_id;
 > 381		int multi_phase_case = 0xFFFF;
   382	
   383		config.dev = tps->dev;
   384		config.driver_data = tps;
   385		config.regmap = tps->regmap;
   386	
   387		/*
   388		 * Switch case defines different possible multi phase config
   389		 * This is based on dts custom property: multi-phase-id
   390		 * Using compatible or device rev is a too complex alternative
   391		 * Default case is no Multiphase buck.
   392		 * In case of Multiphase configuration, value should be defined for
   393		 * buck_configured to avoid creating bucks for every buck in multiphase
   394		 */
   395	
   396		if (device_property_present(tps->dev, "ti,multi-phase-id")) {
   397			device_property_read_u32(tps->dev, "ti,multi-phase-id", &multi_phase_id);
   398			switch (multi_phase_id) {
   399			case 12:
   400				buck_multi[0] = 1;
   401				buck_configured[0] = 1;
   402				buck_configured[1] = 1;
   403				multi_phase_case = TPS6594_BUCK_12;
   404				break;
   405			case 34:
   406				buck_multi[1] = 1;
   407				buck_configured[2] = 1;
   408				buck_configured[3] = 1;
   409				multi_phase_case = TPS6594_BUCK_34;
   410				break;
   411			case 123:
   412				buck_multi[2] = 1;
   413				buck_configured[0] = 1;
   414				buck_configured[1] = 1;
   415				buck_configured[2] = 1;
   416				multi_phase_case = TPS6594_BUCK_123;
   417				break;
   418			case 1234:
   419				buck_multi[3] = 1;
   420				buck_configured[0] = 1;
   421				buck_configured[1] = 1;
   422				buck_configured[2] = 1;
   423				buck_configured[3] = 1;
   424				multi_phase_case = TPS6594_BUCK_1234;
   425				break;
   426			}
   427		}
   428	
   429		for (i = 0; i < MULTI_PHASE_NB; i++) {
   430			if (buck_multi[i] == 0)
   431				continue;
   432	
   433			rdev = devm_regulator_register(&pdev->dev, &multi_regs[i], &config);
   434			if (IS_ERR(rdev)) {
   435				dev_err(tps->dev, "failed to register %s regulator\n",
   436					pdev->name);
   437				return PTR_ERR(rdev);
   438			}
   439			rdevmultitbl[i] = rdev;
   440		}
   441	
   442		if (tps->chip_id == LP8764X)
   443			/* There is only 4 buck on LP8764X */
   444			buck_configured[4] = 1;
   445	
   446		for (i = 0; i < BUCK_NB; i++) {
   447			if (buck_configured[i] == 1)
   448				continue;
   449	
   450			rdev = devm_regulator_register(&pdev->dev, &buck_regs[i], &config);
   451			if (IS_ERR(rdev)) {
   452				dev_err(tps->dev, "failed to register %s regulator\n",
   453					pdev->name);
   454				return PTR_ERR(rdev);
   455			}
   456			rdevbucktbl[i] = rdev;
   457		}
   458	
   459		/* LP8764X dosen't have LDO */
   460		if (tps->chip_id != LP8764X) {
   461			for (i = 0; i < ARRAY_SIZE(ldo_regs); i++) {
   462				rdev = devm_regulator_register(&pdev->dev, &ldo_regs[i], &config);
   463				if (IS_ERR(rdev)) {
   464					dev_err(tps->dev,
   465						"failed to register %s regulator\n",
   466						pdev->name);
   467					return PTR_ERR(rdev);
   468				}
   469				rdevldotbl[i] = rdev;
   470			}
   471		}
   472	
   473		irq_data = devm_kmalloc(tps->dev,
   474					ARRAY_SIZE(tps6594_regulator_irq_types) *
   475					sizeof(struct tps6594_regulator_irq_data),
   476					GFP_KERNEL);
   477		if (!irq_data)
   478			return -ENOMEM;
   479	
   480		for (i = 0; i < ARRAY_SIZE(tps6594_regulator_irq_types); ++i) {
   481			irq_type = &tps6594_regulator_irq_types[i];
   482	
   483			irq = platform_get_irq_byname(pdev, irq_type->irq_name);
   484			if (irq < 0)
   485				return -EINVAL;
   486	
   487			irq_data[i].dev = tps->dev;
   488			irq_data[i].type = irq_type;
   489	
   490			tps6594_get_rdev_by_name(irq_type->regulator_name, rdevbucktbl,
   491						 rdevldotbl, rdev);
   492	
 > 493			if (rdev < 0) {
   494				dev_err(tps->dev, "Failed to get rdev for %s\n",
   495					irq_type->regulator_name);
   496				return -EINVAL;
   497			}
   498			irq_data[i].rdev = rdev;
   499	
   500			error = devm_request_threaded_irq(tps->dev, irq, NULL,
   501							  tps6594_regulator_irq_handler,
   502							  IRQF_ONESHOT,
   503							  irq_type->irq_name,
   504							  &irq_data[i]);
   505			if (error) {
   506				dev_err(tps->dev, "failed to request %s IRQ %d: %d\n",
   507					irq_type->irq_name, irq, error);
   508				return error;
   509			}
   510		}
   511	
   512		if (tps->chip_id == LP8764X)
   513			ext_reg_irq_nb = ARRAY_SIZE(tps6594_ext_regulator_irq_types);
   514	
   515		irq_ext_reg_data = devm_kmalloc(tps->dev,
   516						ext_reg_irq_nb *
   517						sizeof(struct tps6594_ext_regulator_irq_data),
   518						GFP_KERNEL);
   519		if (!irq_ext_reg_data)
   520			return -ENOMEM;
   521	
   522		for (i = 0; i < ext_reg_irq_nb; ++i) {
   523			irq_type = &tps6594_ext_regulator_irq_types[i];
   524	
   525			irq = platform_get_irq_byname(pdev, irq_type->irq_name);
   526			if (irq < 0)
   527				return -EINVAL;
   528	
   529			irq_ext_reg_data[i].dev = tps->dev;
   530			irq_ext_reg_data[i].type = irq_type;
   531	
   532			error = devm_request_threaded_irq(tps->dev, irq, NULL,
   533							  tps6594_regulator_irq_handler,
   534							  IRQF_ONESHOT,
   535							  irq_type->irq_name,
   536							  &irq_ext_reg_data[i]);
   537			if (error) {
   538				dev_err(tps->dev, "failed to request %s IRQ %d: %d\n",
   539					irq_type->irq_name, irq, error);
   540				return error;
   541			}
   542		}
   543	
   544		return 0;
   545	}
   546	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

Re: [PATCH INTERNAL v1 3/3] regulator: tps6594-regulator: Add driver for TI TPS6594 regulators

[Julien Panis]

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;
> +	}
> +

(...)

Re: [PATCH INTERNAL v1 3/3] regulator: tps6594-regulator: Add driver for TI TPS6594 regulators

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 889 bytes --]

On Wed, Mar 22, 2023 at 10:10:23AM +0100, Julien Panis wrote:

> Question @ Mark/Liam:
> Shouldn't we use the generic 'regulator-coupled-with' property
> instead of 'ti,multi-phase-id' ?

My understanding was that this was a hardware configuration where
two regulators operate as one with only one set of registers used
for configuration.

> 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.

No, coupled regulators are regulators where the voltages can vary
but there's a constraint that their configurations need to be
related somehow, for example they must be within 200mV of each
other or something like that.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH INTERNAL v1 3/3] regulator: tps6594-regulator: Add driver for TI TPS6594 regulators

[Julien Panis]

On 3/22/23 14:13, Mark Brown wrote:
> On Wed, Mar 22, 2023 at 10:10:23AM +0100, Julien Panis wrote:
>
>> Question @ Mark/Liam:
>> Shouldn't we use the generic 'regulator-coupled-with' property
>> instead of 'ti,multi-phase-id' ?
> My understanding was that this was a hardware configuration where
> two regulators operate as one with only one set of registers used
> for configuration.

Your understanding was correct.

>
>> 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.
> No, coupled regulators are regulators where the voltages can vary
> but there's a constraint that their configurations need to be
> related somehow, for example they must be within 200mV of each
> other or something like that.

OK, thank you for this explanation.
So, we keep 'ti,multi-phase-id' property.