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

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

[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 LP8764 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/20230511095126.105104-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 LP8764.
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
LP8764: 4 BUCKs and no LDO
Bucks can be used in multipahse mode.

Changes since v1:
https://lore.kernel.org/all/20230224133129.887203-1-eblanc@baylibre.com/
Rtc:
- Removed struct tps6594_rtc.
- Removed some dev_err messages.
- Removed some comments.
- Remove some whitespaces in comments and error messages.
- Check if RTC is running before reading a timestamp in read_rtc.
- Stop RTC at the end of probe to wait for a timestamp to be set.
- Add default MFD_TPS6594 to Kconfig.

Pinctrl:
- Removed #define DEBUG.
- Add default MFD_TPS6594 to Kconfig.
- Fix typo and reword help message of Kconfig.

Regulators:
Further to Mark Brown review:
- File header whole block C++ style.
- Configuring modes not supported: omit all mode operations
- Log the error before notifying.
- Request the interrupts while registering the regulators (then remove
  the lookup function).
Further to Matti review:
- Postponed: devm_regulator_irq_helper() and
  regulator_irq_map_event_simple() can probably be used but code.
  refactoring is not so trivial. This can be done later as an enhancement
  after this patch list is merged.
Buck Multi phase management:
- Multiphase property can take an array when 2 multi phase buck, buck12
  and buck34.
- Configuration multi phase buck34 without multiphase buck12 is not
  supported (when only one multiphase, must be buck12). Not clear from the
  spec but confirmed by TI.
- Supported multiphase conficurations: buck12, buck123, buck1234,
  buck12 + buck34.
- All interrupts are attached to the multiphase buck (ie: for regulator
  buck12, buck1 & buck2 interrupts are registered).

Changes since v2:
https://lore.kernel.org/all/20230328091448.648452-1-eblanc@baylibre.com/
Rtc:
- Add logic to avoid reinitializing a working clock.
- Fix some multiline comments format.

Regulators:
Further to Mark Brown review:
- Log the error before notifying.
- Request the interrupts while registering the regulators.
Further to Krzysztof Kozlowski:
https://lore.kernel.org/all/75f0a18d-aed9-8610-2925-4e604b4b0241@baylibre.com/
- Remove ti, multi-phase-id property which is redundant with buck dts naming
  rules.

Changes since v3:
https://lore.kernel.org/lkml/20230414101217.1342891-1-eblanc@baylibre.com/
RTC:
- Add wakeup source

Pinctrl:
- Switch to GPIO_REGMAP framework

Change since v4:
https://lore.kernel.org/lkml/20230512141755.1712358-1-eblanc@baylibre.com/
Update Copyright notice date
Reorder includes

RTC:
- Rework some comments, fixing punctuation and style
- Use NANO macro from units.h for PPB_MULT
- Rework to use bitwise types
- Remove unnecessary casts
- Add SAFETY comments
- Use `dev_err_probe(...)` instead of print then return

Pinctrl:
- Reword help message and add module name in Kconfig
- Rework code to use struct pinfunction and PINCTRL_PINFUNCTION() macro
- Remove unnecessary casts
- Use `dev_err_probe(...)` instead of print then return
- Replace TPS6594_REG_GPIO1_CONF with a comment for TPS6594_REG_GPIOX_CONF

Regulators:
- nits: Add missing tabs, standard spaces, group "buck_multi".
- Use OF dedicated of_node_cmp API instead of standard strcmp.
- Use devm_kmalloc_array(...) API instead of devm_kmalloc(...) wherever possible.
- return dev_err_probe(...) wherever possible.

Esteban Blanc (2):
  rtc: tps6594: Add driver for TPS6594 RTC
  pinctrl: tps6594: Add driver for TPS6594 pinctrl and GPIOs

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

 drivers/pinctrl/Kconfig               |  13 +
 drivers/pinctrl/Makefile              |   1 +
 drivers/pinctrl/pinctrl-tps6594.c     | 367 +++++++++++++++
 drivers/regulator/Kconfig             |  13 +
 drivers/regulator/Makefile            |   1 +
 drivers/regulator/tps6594-regulator.c | 615 ++++++++++++++++++++++++++
 drivers/rtc/Kconfig                   |  12 +
 drivers/rtc/Makefile                  |   1 +
 drivers/rtc/rtc-tps6594.c             | 463 +++++++++++++++++++
 include/linux/mfd/tps6594.h           |   3 +-
 10 files changed, 1488 insertions(+), 1 deletion(-)
 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.39.2

[PATCH v5 1/3] rtc: tps6594: Add driver for TPS6594 RTC

[Esteban Blanc]

TPS6594 PMIC is a MFD. This patch adds 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       |  12 +
 drivers/rtc/Makefile      |   1 +
 drivers/rtc/rtc-tps6594.c | 463 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 476 insertions(+)
 create mode 100644 drivers/rtc/rtc-tps6594.c

diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
index 753872408615..0fc10710647a 100644
--- a/drivers/rtc/Kconfig
+++ b/drivers/rtc/Kconfig
@@ -578,6 +578,18 @@ 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
+	default 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.
+
+	  This driver can also be built as a module. If so, the module
+	  will be called tps6594-rtc
+
 config RTC_DRV_TPS65910
 	tristate "TI TPS65910 RTC driver"
 	depends on MFD_TPS65910
diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
index ea445d1ebb17..3d3f8c9d0697 100644
--- a/drivers/rtc/Makefile
+++ b/drivers/rtc/Makefile
@@ -175,6 +175,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_VT8500)	+= rtc-vt8500.o
diff --git a/drivers/rtc/rtc-tps6594.c b/drivers/rtc/rtc-tps6594.c
new file mode 100644
index 000000000000..a91f7b546e07
--- /dev/null
+++ b/drivers/rtc/rtc-tps6594.c
@@ -0,0 +1,463 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * RTC driver for tps6594 PMIC
+ *
+ * Copyright (C) 2023 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/platform_device.h>
+#include <linux/property.h>
+#include <linux/rtc.h>
+#include <linux/types.h>
+#include <linux/units.h>
+
+#include <linux/mfd/tps6594.h>
+
+#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 registers
+#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 by 'offset' interface (swapped sign).
+ * After conversion, the values do 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 NANO
+
+static int tps6594_rtc_alarm_irq_enable(struct device *dev,
+					unsigned int enabled)
+{
+	struct tps6594 *tps = dev_get_drvdata(dev->parent);
+	u8 val = 0;
+
+	val = enabled ? TPS6594_IT_ALARM_ON : TPS6594_IT_ALARM_OFF;
+
+	return regmap_update_bits(tps->regmap, TPS6594_REG_RTC_INTERRUPTS,
+				  TPS6594_BIT_IT_ALARM, val);
+}
+
+/* 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. Next time we set GET_TIME to 1 we will be 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)
+		return ret;
+
+	/*
+	 * Copy content of RTC registers to shadow registers or latches to read
+	 * a coherent timestamp.
+	 */
+	return regmap_set_bits(tps->regmap, TPS6594_REG_RTC_CTRL_1,
+			       TPS6594_BIT_GET_TIME);
+}
+
+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;
+
+	// Check if RTC is running.
+	ret = regmap_test_bits(tps->regmap, TPS6594_REG_RTC_STATUS,
+			       TPS6594_BIT_RUN);
+	if (ret < 0)
+		return ret;
+	if (ret == 0)
+		return -EINVAL;
+
+	ret = tps6594_rtc_shadow_timestamp(dev, tps);
+	if (ret < 0)
+		return ret;
+
+	// Read shadowed RTC registers.
+	ret = regmap_bulk_read(tps->regmap, TPS6594_REG_RTC_SECONDS, rtc_data,
+			       NUM_TIME_REGS);
+	if (ret < 0)
+		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;
+}
+
+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)
+		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)
+		return ret;
+
+	// Start back RTC.
+	return regmap_set_bits(tps->regmap, TPS6594_REG_RTC_CTRL_1,
+			       TPS6594_BIT_STOP_RTC);
+}
+
+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)
+		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)
+		return ret;
+
+	alm->enabled = int_val & TPS6594_BIT_IT_ALARM;
+
+	return 0;
+}
+
+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)
+		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 < 0)
+		return ret;
+
+	if (alm->enabled)
+		ret = tps6594_rtc_alarm_irq_enable(dev, 1);
+
+	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);
+	__le16 value;
+	int ret;
+
+	/*
+	 * TPS6594 uses two's complement 16 bit value for compensation of 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 = (__le16)calibration;
+
+	comp_data[0] = value & 0xFF;
+	comp_data[1] = (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)
+		return ret;
+
+	// Enable automatic compensation.
+	return regmap_set_bits(tps->regmap, TPS6594_REG_RTC_CTRL_1,
+			       TPS6594_BIT_AUTO_COMP);
+}
+
+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;
+	__le16 value;
+	int ret;
+
+	ret = regmap_read(tps->regmap, TPS6594_REG_RTC_CTRL_1, &ctrl);
+	if (ret < 0)
+		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)
+		return ret;
+
+	value = (__le16)comp_data[0] | ((__le16)comp_data[1] << 8);
+
+	*calibration = 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 * PPB_MULT;
+
+	if (tmp < 0)
+		tmp -= TICKS_PER_HOUR / 2LL;
+	else
+		tmp += TICKS_PER_HOUR / 2LL;
+	tmp = div_s64(tmp, TICKS_PER_HOUR);
+
+	/*
+	 * SAFETY:
+	 * Compution is the reverse operation of the one done in
+	 * `tps6594_rtc_set_offset`. The safety remarks applie here too.
+	 */
+
+	/*
+	 * 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;
+
+	// 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 * TICKS_PER_HOUR;
+	if (tmp < 0)
+		tmp -= PPB_MULT / 2LL;
+	else
+		tmp += PPB_MULT / 2LL;
+	tmp = div_s64(tmp, PPB_MULT);
+
+	/*
+	 * SAFETY:
+	 * - tmp = offset * TICK_PER_HOUR :
+	 *	`offset` can't be more than 277774, so `tmp` can't exceed 277774000000000
+	 *	which is lower than the maximum value in an `s64` (2^63-1). No overflow here.
+	 *
+	 * - tmp += TICK_PER_HOUR / 2LL :
+	 *	tmp will have a maximum value of 277774117964800 which is still inferior to 2^63-1.
+	 */
+
+	// Offset value operates in negative way, so swap sign.
+	calibration = (int)-tmp;
+
+	return tps6594_rtc_set_calibration(dev, calibration);
+}
+
+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 rtc_device *rtc_dev = 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(rtc_dev, 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 *tps = dev_get_drvdata(pdev->dev.parent);
+	struct rtc_device *rtc;
+	int irq;
+	int ret;
+
+	rtc = devm_kzalloc(&pdev->dev, sizeof(*rtc), GFP_KERNEL);
+	if (!rtc)
+		return -ENOMEM;
+
+	rtc = devm_rtc_allocate_device(&pdev->dev);
+	if (IS_ERR(rtc))
+		return PTR_ERR(rtc);
+
+	// Enable crystal oscillator.
+	ret = regmap_set_bits(tps->regmap, TPS6594_REG_RTC_CTRL_2,
+			      TPS6594_BIT_XTAL_EN);
+	if (ret < 0)
+		return ret;
+
+	ret = regmap_test_bits(tps->regmap, TPS6594_REG_RTC_STATUS,
+			       TPS6594_BIT_RUN);
+	if (ret < 0)
+		return ret;
+	// RTC not running.
+	if (ret == 0) {
+		ret = regmap_set_bits(tps->regmap, TPS6594_REG_RTC_CTRL_1,
+				      TPS6594_BIT_STOP_RTC);
+		if (ret < 0)
+			return ret;
+
+		/*
+		 * RTC should be running now. Check if this is the case.
+		 * If not it might be a missing oscillator.
+		 */
+		ret = regmap_test_bits(tps->regmap, TPS6594_REG_RTC_STATUS,
+				       TPS6594_BIT_RUN);
+		if (ret < 0)
+			return ret;
+		if (ret == 0)
+			return -ENODEV;
+
+		// Stop RTC until first call to `tps6594_rtc_set_time`.
+		ret = regmap_clear_bits(tps->regmap, TPS6594_REG_RTC_CTRL_1,
+					TPS6594_BIT_STOP_RTC);
+		if (ret < 0)
+			return ret;
+	}
+
+	platform_set_drvdata(pdev, rtc);
+
+	irq = platform_get_irq_byname(pdev, TPS6594_IRQ_NAME_ALARM);
+	if (irq < 0)
+		return dev_err_probe(&pdev->dev, irq, "Failed to get irq\n");
+
+	ret = devm_request_threaded_irq(&pdev->dev, irq, NULL,
+					tps6594_rtc_interrupt, IRQF_ONESHOT,
+					TPS6594_IRQ_NAME_ALARM, &pdev->dev);
+	if (ret < 0)
+		return dev_err_probe(&pdev->dev, ret,
+				     "Failed to request_threaded_irq\n");
+
+	ret = device_init_wakeup(&pdev->dev, true);
+	if (ret < 0)
+		return dev_err_probe(&pdev->dev, ret,
+				     "Failed to init rtc as wakeup source\n");
+
+	rtc->ops = &tps6594_rtc_ops;
+	rtc->range_min = RTC_TIMESTAMP_BEGIN_2000;
+	rtc->range_max = RTC_TIMESTAMP_END_2099;
+
+	return devm_rtc_register_device(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:rtc-tps6594-rtc");
+MODULE_AUTHOR("Esteban Blanc <eblanc@baylibre.com>");
+MODULE_DESCRIPTION("TPS6594 RTC driver");
+MODULE_LICENSE("GPL");
-- 
2.39.2

[PATCH v5 2/3] pinctrl: tps6594: Add driver for TPS6594 pinctrl and GPIOs

[Esteban Blanc]

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

This patch adds a pinctrl and GPIO drivers in
order to use those functions.

Signed-off-by: Esteban Blanc <eblanc@baylibre.com>
---
 drivers/pinctrl/Kconfig           |  13 ++
 drivers/pinctrl/Makefile          |   1 +
 drivers/pinctrl/pinctrl-tps6594.c | 367 ++++++++++++++++++++++++++++++
 include/linux/mfd/tps6594.h       |   3 +-
 4 files changed, 383 insertions(+), 1 deletion(-)
 create mode 100644 drivers/pinctrl/pinctrl-tps6594.c

diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
index 5787c579dcf6..73903fa59776 100644
--- a/drivers/pinctrl/Kconfig
+++ b/drivers/pinctrl/Kconfig
@@ -480,6 +480,19 @@ config PINCTRL_TB10X
 	depends on OF && ARC_PLAT_TB10X
 	select GPIOLIB
 
+config PINCTRL_TPS6594
+	tristate "Pinctrl and GPIO driver for TI TPS6594 PMIC"
+	depends on MFD_TPS6594
+	default MFD_TPS6594
+	select PINMUX
+	select GPIOLIB
+	select REGMAP
+	select GPIO_REGMAP
+	help
+	  Say Y to select the pinmuxing and GPIOs driver for the TPS6594
+	  PMICs chip family. This driver can also be built as a module
+	  called tps6594-pinctrl.
+
 config PINCTRL_ZYNQ
 	bool "Pinctrl driver for Xilinx Zynq"
 	depends on ARCH_ZYNQ
diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile
index e196c6e324ad..28271a8d5275 100644
--- a/drivers/pinctrl/Makefile
+++ b/drivers/pinctrl/Makefile
@@ -49,6 +49,7 @@ obj-$(CONFIG_PINCTRL_ST) 	+= pinctrl-st.o
 obj-$(CONFIG_PINCTRL_STMFX) 	+= pinctrl-stmfx.o
 obj-$(CONFIG_PINCTRL_SX150X)	+= pinctrl-sx150x.o
 obj-$(CONFIG_PINCTRL_TB10X)	+= pinctrl-tb10x.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..e663109249db
--- /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) 2023 BayLibre Incorporated - https://www.baylibre.com/
+ */
+
+#include <linux/gpio/driver.h>
+#include <linux/gpio/regmap.h>
+#include <linux/module.h>
+#include <linux/pinctrl/pinmux.h>
+#include <linux/platform_device.h>
+
+#include <linux/mfd/tps6594.h>
+
+#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
+
+#define FUNCTION(n, g, v)                                                    \
+	{                                                                    \
+		.pinfunction = PINCTRL_PINFUNCTION((n), (g), ARRAY_SIZE(g)), \
+		.muxval = v,                                                 \
+	}
+
+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 *const tps6594_gpio_func_group_names[] = {
+	"GPIO0", "GPIO1", "GPIO2", "GPIO3", "GPIO4",  "GPIO5",
+	"GPIO6", "GPIO7", "GPIO8", "GPIO9", "GPIO10",
+};
+static const char *const tps6594_nsleep1_func_group_names[] = {
+	"GPIO0", "GPIO1", "GPIO2", "GPIO3", "GPIO4",  "GPIO5",
+	"GPIO6", "GPIO7", "GPIO8", "GPIO9", "GPIO10",
+};
+static const char *const tps6594_nsleep2_func_group_names[] = {
+	"GPIO0", "GPIO1", "GPIO2", "GPIO3", "GPIO4",  "GPIO5",
+	"GPIO6", "GPIO7", "GPIO8", "GPIO9", "GPIO10",
+};
+static const char *const tps6594_wkup1_func_group_names[] = {
+	"GPIO0", "GPIO1", "GPIO2", "GPIO3", "GPIO4",  "GPIO5",
+	"GPIO6", "GPIO7", "GPIO8", "GPIO9", "GPIO10",
+};
+static const char *const tps6594_wkup2_func_group_names[] = {
+	"GPIO0", "GPIO1", "GPIO2", "GPIO3", "GPIO4",  "GPIO5",
+	"GPIO6", "GPIO7", "GPIO8", "GPIO9", "GPIO10",
+};
+static const char *const tps6594_scl_i2c2_cs_spi_func_group_names[] = {
+	"GPIO0",
+	"GPIO1",
+};
+static const char *const tps6594_nrstout_soc_func_group_names[] = {
+	"GPIO0",
+	"GPIO10",
+};
+static const char *const tps6594_trig_wdog_func_group_names[] = {
+	"GPIO1",
+	"GPIO10",
+};
+static const char *const tps6594_sda_i2c2_sdo_spi_func_group_names[] = {
+	"GPIO1",
+};
+static const char *const tps6594_clk32kout_func_group_names[] = {
+	"GPIO2",
+	"GPIO3",
+	"GPIO7",
+};
+static const char *const tps6594_nerr_soc_func_group_names[] = {
+	"GPIO2",
+};
+static const char *const tps6594_sclk_spmi_func_group_names[] = {
+	"GPIO4",
+};
+static const char *const tps6594_sdata_spmi_func_group_names[] = {
+	"GPIO5",
+};
+static const char *const tps6594_nerr_mcu_func_group_names[] = {
+	"GPIO6",
+};
+static const char *const tps6594_syncclkout_func_group_names[] = {
+	"GPIO7",
+	"GPIO9",
+};
+static const char *const tps6594_disable_wdog_func_group_names[] = {
+	"GPIO7",
+	"GPIO8",
+};
+static const char *const tps6594_pdog_func_group_names[] = {
+	"GPIO8",
+};
+static const char *const tps6594_syncclkin_func_group_names[] = {
+	"GPIO9",
+};
+
+struct tps6594_pinctrl_function {
+	struct pinfunction pinfunction;
+	u8 muxval;
+};
+
+static const struct tps6594_pinctrl_function pinctrl_functions[] = {
+	FUNCTION("gpio", tps6594_gpio_func_group_names,
+		 TPS6594_PINCTRL_GPIO_FUNCTION),
+	FUNCTION("nsleep1", tps6594_nsleep1_func_group_names,
+		 TPS6594_PINCTRL_NSLEEP1_FUNCTION),
+	FUNCTION("nsleep2", tps6594_nsleep2_func_group_names,
+		 TPS6594_PINCTRL_NSLEEP2_FUNCTION),
+	FUNCTION("wkup1", tps6594_wkup1_func_group_names,
+		 TPS6594_PINCTRL_WKUP1_FUNCTION),
+	FUNCTION("wkup2", tps6594_wkup2_func_group_names,
+		 TPS6594_PINCTRL_WKUP2_FUNCTION),
+	FUNCTION("scl_i2c2-cs_spi", tps6594_scl_i2c2_cs_spi_func_group_names,
+		 TPS6594_PINCTRL_SCL_I2C2_CS_SPI_FUNCTION),
+	FUNCTION("nrstout_soc", tps6594_nrstout_soc_func_group_names,
+		 TPS6594_PINCTRL_NRSTOUT_SOC_FUNCTION),
+	FUNCTION("trig_wdog", tps6594_trig_wdog_func_group_names,
+		 TPS6594_PINCTRL_TRIG_WDOG_FUNCTION),
+	FUNCTION("sda_i2c2-sdo_spi", tps6594_sda_i2c2_sdo_spi_func_group_names,
+		 TPS6594_PINCTRL_SDA_I2C2_SDO_SPI_FUNCTION),
+	FUNCTION("clk32kout", tps6594_clk32kout_func_group_names,
+		 TPS6594_PINCTRL_CLK32KOUT_FUNCTION),
+	FUNCTION("nerr_soc", tps6594_nerr_soc_func_group_names,
+		 TPS6594_PINCTRL_NERR_SOC_FUNCTION),
+	FUNCTION("sclk_spmi", tps6594_sclk_spmi_func_group_names,
+		 TPS6594_PINCTRL_SCLK_SPMI_FUNCTION),
+	FUNCTION("sdata_spmi", tps6594_sdata_spmi_func_group_names,
+		 TPS6594_PINCTRL_SDATA_SPMI_FUNCTION),
+	FUNCTION("nerr_mcu", tps6594_nerr_mcu_func_group_names,
+		 TPS6594_PINCTRL_NERR_MCU_FUNCTION),
+	FUNCTION("syncclkout", tps6594_syncclkout_func_group_names,
+		 TPS6594_PINCTRL_SYNCCLKOUT_FUNCTION),
+	FUNCTION("disable_wdog", tps6594_disable_wdog_func_group_names,
+		 TPS6594_PINCTRL_DISABLE_WDOG_FUNCTION),
+	FUNCTION("pdog", tps6594_pdog_func_group_names,
+		 TPS6594_PINCTRL_PDOG_FUNCTION),
+	FUNCTION("syncclkin", tps6594_syncclkin_func_group_names,
+		 TPS6594_PINCTRL_SYNCCLKIN_FUNCTION),
+};
+
+struct tps6594_pinctrl {
+	struct tps6594 *tps;
+	struct gpio_regmap *gpio_regmap;
+	struct pinctrl_dev *pctl_dev;
+	const struct tps6594_pinctrl_function *funcs;
+	const struct pinctrl_pin_desc *pins;
+};
+
+static int tps6594_gpio_regmap_xlate(struct gpio_regmap *gpio,
+				     unsigned int base, unsigned int offset,
+				     unsigned int *reg, unsigned int *mask)
+{
+	unsigned int line = offset % 8;
+	unsigned int stride = offset / 8;
+
+	switch (base) {
+	case TPS6594_REG_GPIO1_CONF:
+		*reg = TPS6594_REG_GPIOX_CONF(offset);
+		*mask = TPS6594_BIT_GPIO_DIR;
+		break;
+	case TPS6594_REG_GPIO_IN_1:
+	case TPS6594_REG_GPIO_OUT_1:
+		*reg = base + stride;
+		*mask = BIT(line);
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+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].pinfunction.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].pinfunction.groups;
+	*num_groups = pinctrl->funcs[selector].pinfunction.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 = &pinctrl->pins[selector].number;
+	*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;
+	struct gpio_regmap_config config = {};
+
+	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 = devm_kzalloc(&pdev->dev, sizeof(*pinctrl), GFP_KERNEL);
+	if (!pinctrl)
+		return -ENOMEM;
+	pinctrl->tps = dev_get_drvdata(pdev->dev.parent);
+	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)) {
+		return dev_err_probe(&pdev->dev, PTR_ERR(pinctrl->pctl_dev),
+				     "Couldn't register pinctrl driver\n");
+	}
+
+	config.parent = tps->dev;
+	config.regmap = tps->regmap;
+	config.ngpio = TPS6594_PINCTRL_PINS_NB;
+	config.ngpio_per_reg = 8;
+	config.reg_dat_base = TPS6594_REG_GPIO_IN_1;
+	config.reg_set_base = TPS6594_REG_GPIO_OUT_1;
+	config.reg_dir_out_base = TPS6594_REG_GPIO1_CONF;
+	config.reg_mask_xlate = tps6594_gpio_regmap_xlate;
+
+	pinctrl->gpio_regmap = devm_gpio_regmap_register(&pdev->dev, &config);
+	if (IS_ERR(pinctrl->gpio_regmap)) {
+		return dev_err_probe(&pdev->dev, PTR_ERR(pinctrl->gpio_regmap),
+				     "Couldn't register gpio_regmap driver\n");
+	}
+
+	return 0;
+}
+
+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");
diff --git a/include/linux/mfd/tps6594.h b/include/linux/mfd/tps6594.h
index 3f7c5e23cd4c..2792841f555c 100644
--- a/include/linux/mfd/tps6594.h
+++ b/include/linux/mfd/tps6594.h
@@ -47,7 +47,8 @@ enum pmic_id {
 #define TPS6594_REG_VMON2_PG_WINDOW			0x2f
 #define TPS6594_REG_VMON2_PG_LEVEL			0x30
 
-#define TPS6594_REG_GPIOX_CONF(gpio_inst)		(0x31 + (gpio_inst))
+// Used to compute register address of GPIO1_CONF to GPIO11_CONF
+#define TPS6594_REG_GPIOX_CONF(gpio_inst)	(0x31 + (gpio_inst))
 #define TPS6594_REG_NPWRON_CONF				0x3c
 #define TPS6594_REG_GPIO_OUT_1				0x3d
 #define TPS6594_REG_GPIO_OUT_2				0x3e
-- 
2.39.2

[PATCH v5 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>
Signed-off-by: Esteban Blanc <eblanc@baylibre.com>
---
 drivers/regulator/Kconfig             |  13 +
 drivers/regulator/Makefile            |   1 +
 drivers/regulator/tps6594-regulator.c | 615 ++++++++++++++++++++++++++
 3 files changed, 629 insertions(+)
 create mode 100644 drivers/regulator/tps6594-regulator.c

diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index e5f3613c15fa..b832702dcb2c 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -1463,6 +1463,19 @@ 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
+	default MFD_TPS6594
+	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 58dfe0147cd4..8bbead39cceb 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -174,6 +174,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..d5a574ec6d12
--- /dev/null
+++ b/drivers/regulator/tps6594-regulator.c
@@ -0,0 +1,615 @@
+// SPDX-License-Identifier: GPL-2.0
+//
+// Regulator driver for tps6594 PMIC
+//
+// Copyright (C) 2023 BayLibre Incorporated - https://www.baylibre.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
+#define REGS_INT_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_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),
+};
+
+/* 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_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_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_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 struct tps6594_regulator_irq_type tps6594_buck1_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 },
+};
+
+static struct tps6594_regulator_irq_type tps6594_buck2_irq_types[] = {
+	{ 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 },
+};
+
+static struct tps6594_regulator_irq_type tps6594_buck3_irq_types[] = {
+	{ 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 },
+};
+
+static struct tps6594_regulator_irq_type tps6594_buck4_irq_types[] = {
+	{ 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 },
+};
+
+static struct tps6594_regulator_irq_type tps6594_buck5_irq_types[] = {
+	{ 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 },
+};
+
+static struct tps6594_regulator_irq_type tps6594_ldo1_irq_types[] = {
+	{ 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 },
+};
+
+static struct tps6594_regulator_irq_type tps6594_ldo2_irq_types[] = {
+	{ 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 },
+};
+
+static struct tps6594_regulator_irq_type tps6594_ldo3_irq_types[] = {
+	{ 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 },
+};
+
+static struct tps6594_regulator_irq_type tps6594_ldo4_irq_types[] = {
+	{ 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_bucks_irq_types[] = {
+	tps6594_buck1_irq_types,
+	tps6594_buck2_irq_types,
+	tps6594_buck3_irq_types,
+	tps6594_buck4_irq_types,
+	tps6594_buck5_irq_types,
+};
+
+static struct tps6594_regulator_irq_type *tps6594_ldos_irq_types[] = {
+	tps6594_ldo1_irq_types,
+	tps6594_ldo2_irq_types,
+	tps6594_ldo3_irq_types,
+	tps6594_ldo4_irq_types,
+};
+
+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;
+	}
+
+	dev_err(irq_data->dev, "Error IRQ trap %s for %s\n",
+		irq_data->type->event_name, irq_data->type->regulator_name);
+
+	regulator_notifier_call_chain(irq_data->rdev,
+				      irq_data->type->event, NULL);
+
+	return IRQ_HANDLED;
+}
+
+static int tps6594_request_reg_irqs(struct platform_device *pdev,
+				    struct regulator_dev *rdev,
+				    struct tps6594_regulator_irq_data *irq_data,
+				    struct tps6594_regulator_irq_type *tps6594_regs_irq_types,
+				    int *irq_idx)
+{
+	struct tps6594_regulator_irq_type *irq_type;
+	struct tps6594 *tps = dev_get_drvdata(pdev->dev.parent);
+	int j;
+	int irq;
+	int error;
+
+	for (j = 0; j < REGS_INT_NB; j++) {
+		irq_type = &tps6594_regs_irq_types[j];
+		irq = platform_get_irq_byname(pdev, irq_type->irq_name);
+		if (irq < 0)
+			return -EINVAL;
+
+		irq_data[*irq_idx + j].dev = tps->dev;
+		irq_data[*irq_idx + j].type = irq_type;
+		irq_data[*irq_idx + j].rdev = rdev;
+
+		error = devm_request_threaded_irq(tps->dev, irq, NULL,
+						  tps6594_regulator_irq_handler,
+						  IRQF_ONESHOT,
+						  irq_type->irq_name,
+						  &irq_data[*irq_idx]);
+		(*irq_idx)++;
+		if (error) {
+			dev_err(tps->dev, "tps6594 failed to request %s IRQ %d: %d\n",
+				irq_type->irq_name, irq, error);
+			return error;
+		}
+	}
+	return 0;
+}
+
+static int tps6594_regulator_probe(struct platform_device *pdev)
+{
+	struct tps6594 *tps = dev_get_drvdata(pdev->dev.parent);
+	struct regulator_dev *rdev;
+	struct device_node *np = NULL;
+	struct device_node *np_pmic_parent = NULL;
+	struct regulator_config config = {};
+	struct tps6594_regulator_irq_data *irq_data;
+	struct tps6594_ext_regulator_irq_data *irq_ext_reg_data;
+	struct tps6594_regulator_irq_type *irq_type;
+	u8 buck_configured[BUCK_NB] = { 0 };
+	u8 buck_multi[MULTI_PHASE_NB] = { 0 };
+	static const char * const multiphases[] = {"buck12", "buck123", "buck1234", "buck34"};
+	static const char *npname;
+	int error, i, irq, multi, delta;
+	int irq_idx = 0;
+	int buck_idx = 0;
+	int ext_reg_irq_nb = 2;
+
+	enum {
+		MULTI_BUCK12,
+		MULTI_BUCK123,
+		MULTI_BUCK1234,
+		MULTI_BUCK12_34,
+		MULTI_FIRST = MULTI_BUCK12,
+		MULTI_LAST = MULTI_BUCK12_34,
+		MULTI_NUM = MULTI_LAST - MULTI_FIRST + 1
+	};
+
+	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 buck node name.
+	 * Buck node name must be chosen accordingly.
+	 * 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
+	 */
+	for (multi = MULTI_FIRST; multi < MULTI_NUM; multi++) {
+		np = of_find_node_by_name(tps->dev->of_node, multiphases[multi]);
+		npname = of_node_full_name(np);
+		np_pmic_parent = of_get_parent(of_get_parent(np));
+		if (of_node_cmp(of_node_full_name(np_pmic_parent), tps->dev->of_node->full_name))
+			continue;
+		delta = strcmp(npname, multiphases[multi]);
+		if (!delta) {
+			switch (multi) {
+			case MULTI_BUCK12:
+				buck_multi[0] = 1;
+				buck_configured[0] = 1;
+				buck_configured[1] = 1;
+				break;
+			/* multiphase buck34 is supported only with buck12 */
+			case MULTI_BUCK12_34:
+				buck_multi[0] = 1;
+				buck_multi[1] = 1;
+				buck_configured[0] = 1;
+				buck_configured[1] = 1;
+				buck_configured[2] = 1;
+				buck_configured[3] = 1;
+				break;
+			case MULTI_BUCK123:
+				buck_multi[2] = 1;
+				buck_configured[0] = 1;
+				buck_configured[1] = 1;
+				buck_configured[2] = 1;
+				break;
+			case MULTI_BUCK1234:
+				buck_multi[3] = 1;
+				buck_configured[0] = 1;
+				buck_configured[1] = 1;
+				buck_configured[2] = 1;
+				buck_configured[3] = 1;
+				break;
+			}
+		}
+	}
+
+	if (tps->chip_id == LP8764)
+		/* There is only 4 buck on LP8764 */
+		buck_configured[4] = 1;
+
+	irq_data = devm_kmalloc_array(tps->dev,
+				REGS_INT_NB * sizeof(struct tps6594_regulator_irq_data),
+				ARRAY_SIZE(tps6594_bucks_irq_types) +
+				ARRAY_SIZE(tps6594_ldos_irq_types),
+				GFP_KERNEL);
+	if (!irq_data)
+		return -ENOMEM;
+
+	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))
+			return dev_err_probe(tps->dev, PTR_ERR(rdev),
+					     "failed to register %s regulator\n",
+					     pdev->name);
+
+		/* config multiphase buck12+buck34 */
+		if (i == 1)
+			buck_idx = 2;
+		error = tps6594_request_reg_irqs(pdev, rdev, irq_data,
+						 tps6594_bucks_irq_types[buck_idx], &irq_idx);
+		if (error)
+			return error;
+		error = tps6594_request_reg_irqs(pdev, rdev, irq_data,
+						 tps6594_bucks_irq_types[buck_idx + 1], &irq_idx);
+		if (error)
+			return error;
+
+		if (i == 2 || i == 3) {
+			error = tps6594_request_reg_irqs(pdev, rdev, irq_data,
+							 tps6594_bucks_irq_types[buck_idx + 2],
+							 &irq_idx);
+			if (error)
+				return error;
+		}
+		if (i == 3) {
+			error = tps6594_request_reg_irqs(pdev, rdev, irq_data,
+							 tps6594_bucks_irq_types[buck_idx + 3],
+							 &irq_idx);
+			if (error)
+				return error;
+		}
+	}
+
+	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))
+			return dev_err_probe(tps->dev, PTR_ERR(rdev),
+					     "failed to register %s regulator\n",
+					     pdev->name);
+
+		error = tps6594_request_reg_irqs(pdev, rdev, irq_data,
+						 tps6594_bucks_irq_types[i], &irq_idx);
+		if (error)
+			return error;
+	}
+
+	/* LP8764 dosen't have LDO */
+	if (tps->chip_id != LP8764) {
+		for (i = 0; i < ARRAY_SIZE(ldo_regs); i++) {
+			rdev = devm_regulator_register(&pdev->dev, &ldo_regs[i], &config);
+			if (IS_ERR(rdev))
+				return dev_err_probe(tps->dev, PTR_ERR(rdev),
+						     "failed to register %s regulator\n",
+						     pdev->name);
+
+			error = tps6594_request_reg_irqs(pdev, rdev, irq_data,
+							 tps6594_ldos_irq_types[i],
+							 &irq_idx);
+			if (error)
+				return error;
+		}
+	}
+
+	if (tps->chip_id == LP8764)
+		ext_reg_irq_nb = ARRAY_SIZE(tps6594_ext_regulator_irq_types);
+
+	irq_ext_reg_data = devm_kmalloc_array(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)
+			return dev_err_probe(tps->dev, error,
+					     "failed to request %s IRQ %d\n",
+					     irq_type->irq_name, irq);
+	}
+	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.39.2

Re: [PATCH v5 1/3] rtc: tps6594: Add driver for TPS6594 RTC

[andy.shevchenko]

Mon, May 22, 2023 at 06:31:13PM +0200, Esteban Blanc kirjoitti:
> TPS6594 PMIC is a MFD. This patch adds support for
> the RTC found inside TPS6594 family of PMIC.
> 
> Alarm is also supported.

...

> +	help
> +	  TI Power Management IC TPS6594 supports RTC functionality
> +	  along with alarm. This driver supports the RTC driver for
> +	  the TPS6594 RTC module.
> +
> +	  This driver can also be built as a module. If so, the module
> +	  will be called tps6594-rtc

Grammar period at the end?

...

> +#define TPS6594_GET_TIME_ON TPS6594_BIT_GET_TIME
> +#define TPS6594_GET_TIME_OFF 0

Not used.

> +#define TPS6594_IT_ALARM_ON TPS6594_BIT_IT_ALARM
> +#define TPS6594_IT_ALARM_OFF 0

Used only once.

> +#define TPS6594_AUTO_COMP_ON TPS6594_BIT_IT_ALARM

No _OFF counterpart.

That said the _OFF can be dropped completely. And the rest I see no value to
have, just use those bit definitions directly?

...

> +static int tps6594_rtc_alarm_irq_enable(struct device *dev,
> +					unsigned int enabled)
> +{
> +	struct tps6594 *tps = dev_get_drvdata(dev->parent);
> +	u8 val = 0;

Redundant assignment.

> +	val = enabled ? TPS6594_IT_ALARM_ON : TPS6594_IT_ALARM_OFF;
> +
> +	return regmap_update_bits(tps->regmap, TPS6594_REG_RTC_INTERRUPTS,
> +				  TPS6594_BIT_IT_ALARM, val);
> +}

...

> +	// Read shadowed RTC registers.
> +	ret = regmap_bulk_read(tps->regmap, TPS6594_REG_RTC_SECONDS, rtc_data,
> +			       NUM_TIME_REGS);
> +	if (ret < 0)
> +		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;

	return 0;

No?

...

> +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);
> +	__le16 value;
> +	int ret;
> +
> +	/*
> +	 * TPS6594 uses two's complement 16 bit value for compensation of 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].

This is defined naturally by the bits available, correct?

> +	 */
> +	if (calibration < -32767 || calibration > 32767) {

So, this can be S16_MIN / S16_MAX range. The question here is what the
-32768 meaning is and why it can't be used.

> +		dev_err(dev, "RTC calibration value out of range: %d\n",
> +			calibration);
> +		return -EINVAL;

-ERANGE

> +	}

> +	value = (__le16)calibration;
> +
> +	comp_data[0] = value & 0xFF;
> +	comp_data[1] = (value >> 8) & 0xFF;

Of course these three lines is not what expected.

	value = cpu_to_le16();

> +	// Update all the compensation registers in one shot.
> +	ret = regmap_bulk_write(tps->regmap, TPS6594_REG_RTC_COMP_LSB,
> +				comp_data, NUM_COMP_REGS);

				&value, sizeof(value) ?

> +	if (ret < 0)
> +		return ret;
> +
> +	// Enable automatic compensation.
> +	return regmap_set_bits(tps->regmap, TPS6594_REG_RTC_CTRL_1,
> +			       TPS6594_BIT_AUTO_COMP);
> +}

...

> +	ret = regmap_bulk_read(tps->regmap, TPS6594_REG_RTC_COMP_LSB, comp_data,
> +			       NUM_COMP_REGS);
> +	if (ret < 0)
> +		return ret;
> +
> +	value = (__le16)comp_data[0] | ((__le16)comp_data[1] << 8);
> +
> +	*calibration = value;

In the similar (complementary API) way as above.

...

> +	ret = devm_request_threaded_irq(&pdev->dev, irq, NULL,

Having

	struct device *dev = &pdev->dev;

might make this and other lines shorter / neater.

> +					tps6594_rtc_interrupt, IRQF_ONESHOT,
> +					TPS6594_IRQ_NAME_ALARM, &pdev->dev);
> +	if (ret < 0)
> +		return dev_err_probe(&pdev->dev, ret,
> +				     "Failed to request_threaded_irq\n");

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 1/3] rtc: tps6594: Add driver for TPS6594 RTC

[Alexandre Belloni]

On 23/05/2023 16:36:59+0300, andy.shevchenko@gmail.com wrote:
> Mon, May 22, 2023 at 06:31:13PM +0200, Esteban Blanc kirjoitti:
> > TPS6594 PMIC is a MFD. This patch adds support for
> > the RTC found inside TPS6594 family of PMIC.
> > 
> > Alarm is also supported.
> 
> ...
> 
> > +	help
> > +	  TI Power Management IC TPS6594 supports RTC functionality
> > +	  along with alarm. This driver supports the RTC driver for
> > +	  the TPS6594 RTC module.
> > +
> > +	  This driver can also be built as a module. If so, the module
> > +	  will be called tps6594-rtc
> 
> Grammar period at the end?
> 
> ...
> 
> > +#define TPS6594_GET_TIME_ON TPS6594_BIT_GET_TIME
> > +#define TPS6594_GET_TIME_OFF 0
> 
> Not used.
> 
> > +#define TPS6594_IT_ALARM_ON TPS6594_BIT_IT_ALARM
> > +#define TPS6594_IT_ALARM_OFF 0
> 
> Used only once.
> 
> > +#define TPS6594_AUTO_COMP_ON TPS6594_BIT_IT_ALARM
> 
> No _OFF counterpart.
> 
> That said the _OFF can be dropped completely. And the rest I see no value to
> have, just use those bit definitions directly?
> 
> ...
> 
> > +static int tps6594_rtc_alarm_irq_enable(struct device *dev,
> > +					unsigned int enabled)
> > +{
> > +	struct tps6594 *tps = dev_get_drvdata(dev->parent);
> > +	u8 val = 0;
> 
> Redundant assignment.
> 
> > +	val = enabled ? TPS6594_IT_ALARM_ON : TPS6594_IT_ALARM_OFF;
> > +
> > +	return regmap_update_bits(tps->regmap, TPS6594_REG_RTC_INTERRUPTS,
> > +				  TPS6594_BIT_IT_ALARM, val);
> > +}
> 
> ...
> 
> > +	// Read shadowed RTC registers.
> > +	ret = regmap_bulk_read(tps->regmap, TPS6594_REG_RTC_SECONDS, rtc_data,
> > +			       NUM_TIME_REGS);
> > +	if (ret < 0)
> > +		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;
> 
> 	return 0;
> 
> No?
> 
> ...
> 
> > +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);
> > +	__le16 value;
> > +	int ret;
> > +
> > +	/*
> > +	 * TPS6594 uses two's complement 16 bit value for compensation of 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].
> 
> This is defined naturally by the bits available, correct?
> 
> > +	 */
> > +	if (calibration < -32767 || calibration > 32767) {
> 
> So, this can be S16_MIN / S16_MAX range. The question here is what the
> -32768 meaning is and why it can't be used.
> 
> > +		dev_err(dev, "RTC calibration value out of range: %d\n",
> > +			calibration);
> > +		return -EINVAL;
> 
> -ERANGE
> 

And after using -ERANGE, you can drop the dev_err string.

> > +	}
> 
> > +	value = (__le16)calibration;
> > +
> > +	comp_data[0] = value & 0xFF;
> > +	comp_data[1] = (value >> 8) & 0xFF;
> 
> Of course these three lines is not what expected.
> 
> 	value = cpu_to_le16();
> 
> > +	// Update all the compensation registers in one shot.
> > +	ret = regmap_bulk_write(tps->regmap, TPS6594_REG_RTC_COMP_LSB,
> > +				comp_data, NUM_COMP_REGS);
> 
> 				&value, sizeof(value) ?
> 
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	// Enable automatic compensation.
> > +	return regmap_set_bits(tps->regmap, TPS6594_REG_RTC_CTRL_1,
> > +			       TPS6594_BIT_AUTO_COMP);
> > +}
> 
> ...
> 
> > +	ret = regmap_bulk_read(tps->regmap, TPS6594_REG_RTC_COMP_LSB, comp_data,
> > +			       NUM_COMP_REGS);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	value = (__le16)comp_data[0] | ((__le16)comp_data[1] << 8);
> > +
> > +	*calibration = value;
> 
> In the similar (complementary API) way as above.
> 
> ...
> 
> > +	ret = devm_request_threaded_irq(&pdev->dev, irq, NULL,
> 
> Having
> 
> 	struct device *dev = &pdev->dev;
> 
> might make this and other lines shorter / neater.
> 
> > +					tps6594_rtc_interrupt, IRQF_ONESHOT,
> > +					TPS6594_IRQ_NAME_ALARM, &pdev->dev);
> > +	if (ret < 0)
> > +		return dev_err_probe(&pdev->dev, ret,
> > +				     "Failed to request_threaded_irq\n");
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

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

Re: [PATCH v5 2/3] pinctrl: tps6594: Add driver for TPS6594 pinctrl and GPIOs

[andy.shevchenko]

Mon, May 22, 2023 at 06:31:14PM +0200, Esteban Blanc kirjoitti:
> TI TPS6594 PMIC has 11 GPIOs which can be used
> for different functions.
> 
> This patch adds a pinctrl and GPIO drivers in
> order to use those functions.

...

> +#define FUNCTION(n, g, v)                                                    \
> +	{                                                                    \
> +		.pinfunction = PINCTRL_PINFUNCTION((n), (g), ARRAY_SIZE(g)), \
> +		.muxval = v,                                                 \
> +	}

It seems you have used SPACEs before \, can you move to TABs?

...

> +static int tps6594_gpio_regmap_xlate(struct gpio_regmap *gpio,
> +				     unsigned int base, unsigned int offset,
> +				     unsigned int *reg, unsigned int *mask)
> +{
> +	unsigned int line = offset % 8;
> +	unsigned int stride = offset / 8;
> +
> +	switch (base) {
> +	case TPS6594_REG_GPIO1_CONF:
> +		*reg = TPS6594_REG_GPIOX_CONF(offset);
> +		*mask = TPS6594_BIT_GPIO_DIR;
> +		break;
> +	case TPS6594_REG_GPIO_IN_1:
> +	case TPS6594_REG_GPIO_OUT_1:
> +		*reg = base + stride;
> +		*mask = BIT(line);
> +		break;
> +	default:
> +		return -EINVAL;
> +	}

> +	return 0;

You can return directly instead of breaking.

> +}

> +	pinctrl->pins = tps6594_pins;
> +	pinctrl->pctl_dev =
> +		devm_pinctrl_register(&pdev->dev, pctrl_desc, pinctrl);

With

	struct device *dev = &pdev->dev;

the above becomes a single line. This may help other similar uses.

> +	if (IS_ERR(pinctrl->pctl_dev)) {
> +		return dev_err_probe(&pdev->dev, PTR_ERR(pinctrl->pctl_dev),
> +				     "Couldn't register pinctrl driver\n");
> +	}

Also the {} can be dropped.

...

> +	pinctrl->gpio_regmap = devm_gpio_regmap_register(&pdev->dev, &config);
> +	if (IS_ERR(pinctrl->gpio_regmap)) {
> +		return dev_err_probe(&pdev->dev, PTR_ERR(pinctrl->gpio_regmap),
> +				     "Couldn't register gpio_regmap driver\n");
> +	}

Ditto.

...

> +static struct platform_driver tps6594_pinctrl_driver = {
> +	.driver = { .name = "tps6594-pinctrl" },

Can you use more standard way of style here, i.e.

	.driver = {
		.name = "tps6594-pinctrl",
	},

In case someone is going to add something there in the future it will become
just as cleaner as possible.

> +	.probe = tps6594_pinctrl_probe,
> +};
> +
> +module_platform_driver(tps6594_pinctrl_driver);

Move the above blank line here.

> +MODULE_ALIAS("platform:tps6594-pinctrl");
> +MODULE_AUTHOR("Esteban Blanc <eblanc@baylibre.com>");

...

> +// Used to compute register address of GPIO1_CONF to GPIO11_CONF

This is good.

> -#define TPS6594_REG_GPIOX_CONF(gpio_inst)		(0x31 + (gpio_inst))
> +#define TPS6594_REG_GPIOX_CONF(gpio_inst)	(0x31 + (gpio_inst))

But why this?!

-- 
With Best Regards,
Andy Shevchenko

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

[andy.shevchenko]

Mon, May 22, 2023 at 06:31:15PM +0200, Esteban Blanc kirjoitti:
> From: Jerome Neanne <jneanne@baylibre.com>
> 
> This patch adds support for TPS6594 regulators (bucks and LDOs).

BUCKs (otherwise $$$?)

> 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

BUCKs (otherwise $$$?)

> part number.

...

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

i?!

> +	  It supports software based voltage control
> +	  for different voltage domains.

...

> +#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>

Are you sure this one is correct and / or of.h is not missing? of_match_ptr()
IIRC is defined in of.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>

...

> +/* 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_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,

> +

Redundant blank line.

> +};

...

> +	int error;
> +
> +	for (j = 0; j < REGS_INT_NB; j++) {
> +		irq_type = &tps6594_regs_irq_types[j];
> +		irq = platform_get_irq_byname(pdev, irq_type->irq_name);
> +		if (irq < 0)
> +			return -EINVAL;
> +
> +		irq_data[*irq_idx + j].dev = tps->dev;
> +		irq_data[*irq_idx + j].type = irq_type;
> +		irq_data[*irq_idx + j].rdev = rdev;
> +
> +		error = devm_request_threaded_irq(tps->dev, irq, NULL,
> +						  tps6594_regulator_irq_handler,
> +						  IRQF_ONESHOT,
> +						  irq_type->irq_name,
> +						  &irq_data[*irq_idx]);

> +		(*irq_idx)++;

This is interesing. So, even in error case we touch given parameter. Usually
the pattern is not to touch the output if we know there is an error.

> +		if (error) {
> +			dev_err(tps->dev, "tps6594 failed to request %s IRQ %d: %d\n",
> +				irq_type->irq_name, irq, error);
> +			return error;
> +		}
> +	}

...

> +	u8 buck_configured[BUCK_NB] = { 0 };
> +	u8 buck_multi[MULTI_PHASE_NB] = { 0 };

0:s are not needed but I dunno if it's a style in the regulator subsystem.

> +	static const char * const multiphases[] = {"buck12", "buck123", "buck1234", "buck34"};
> +	static const char *npname;
> +	int error, i, irq, multi, delta;
> +	int irq_idx = 0;
> +	int buck_idx = 0;
> +	int ext_reg_irq_nb = 2;

> +

Redundant blank line.

> +	enum {
> +		MULTI_BUCK12,
> +		MULTI_BUCK123,
> +		MULTI_BUCK1234,
> +		MULTI_BUCK12_34,

> +		MULTI_FIRST = MULTI_BUCK12,
> +		MULTI_LAST = MULTI_BUCK12_34,
> +		MULTI_NUM = MULTI_LAST - MULTI_FIRST + 1

		MULT_NUM

will suffice instead all this.

> +	};

But why enum at all? See below.

...

> +	/*
> +	 * Switch case defines different possible multi phase config
> +	 * This is based on dts buck node name.
> +	 * Buck node name must be chosen accordingly.
> +	 * 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
> +	 */
> +	for (multi = MULTI_FIRST; multi < MULTI_NUM; multi++) {
> +		np = of_find_node_by_name(tps->dev->of_node, multiphases[multi]);
> +		npname = of_node_full_name(np);
> +		np_pmic_parent = of_get_parent(of_get_parent(np));
> +		if (of_node_cmp(of_node_full_name(np_pmic_parent), tps->dev->of_node->full_name))

Why not of_node_full_name() in the second case?


> +			continue;
> +		delta = strcmp(npname, multiphases[multi]);
> +		if (!delta) {
> +			switch (multi) {
> +			case MULTI_BUCK12:

This all looks like match_string() reinvention.

> +				buck_multi[0] = 1;
> +				buck_configured[0] = 1;
> +				buck_configured[1] = 1;
> +				break;
> +			/* multiphase buck34 is supported only with buck12 */
> +			case MULTI_BUCK12_34:
> +				buck_multi[0] = 1;
> +				buck_multi[1] = 1;
> +				buck_configured[0] = 1;
> +				buck_configured[1] = 1;
> +				buck_configured[2] = 1;
> +				buck_configured[3] = 1;
> +				break;
> +			case MULTI_BUCK123:
> +				buck_multi[2] = 1;
> +				buck_configured[0] = 1;
> +				buck_configured[1] = 1;
> +				buck_configured[2] = 1;
> +				break;
> +			case MULTI_BUCK1234:
> +				buck_multi[3] = 1;
> +				buck_configured[0] = 1;
> +				buck_configured[1] = 1;
> +				buck_configured[2] = 1;
> +				buck_configured[3] = 1;
> +				break;
> +			}
> +		}
> +	}

...

> +	irq_data = devm_kmalloc_array(tps->dev,
> +				REGS_INT_NB * sizeof(struct tps6594_regulator_irq_data),
> +				ARRAY_SIZE(tps6594_bucks_irq_types) +
> +				ARRAY_SIZE(tps6594_ldos_irq_types),
> +				GFP_KERNEL);

Have you checked overflow.h? There are macros to help with the above calculus.

> +	if (!irq_data)
> +		return -ENOMEM;

...

> +		rdev = devm_regulator_register(&pdev->dev, &multi_regs[i], &config);
> +		if (IS_ERR(rdev))
> +			return dev_err_probe(tps->dev, PTR_ERR(rdev),

Why not &pdev->dev here?

> +					     "failed to register %s regulator\n",
> +					     pdev->name);

...

> +		rdev = devm_regulator_register(&pdev->dev, &buck_regs[i], &config);
> +		if (IS_ERR(rdev))
> +			return dev_err_probe(tps->dev, PTR_ERR(rdev),
> +					     "failed to register %s regulator\n",
> +					     pdev->name);

Hmm... Again, why the error is printed against different device than regulator
registration?

...

> +	/* LP8764 dosen't have LDO */
> +	if (tps->chip_id != LP8764) {
> +		for (i = 0; i < ARRAY_SIZE(ldo_regs); i++) {
> +			rdev = devm_regulator_register(&pdev->dev, &ldo_regs[i], &config);
> +			if (IS_ERR(rdev))
> +				return dev_err_probe(tps->dev, PTR_ERR(rdev),
> +						     "failed to register %s regulator\n",
> +						     pdev->name);
> +
> +			error = tps6594_request_reg_irqs(pdev, rdev, irq_data,
> +							 tps6594_ldos_irq_types[i],
> +							 &irq_idx);
> +			if (error)
> +				return error;
> +		}
> +	}
> +
> +	if (tps->chip_id == LP8764)

'else'?

Or actually

	if (tps->chip_id == LP8764) {
		...
	} else {
		the above part
	}

?


> +		ext_reg_irq_nb = ARRAY_SIZE(tps6594_ext_regulator_irq_types);

...

> +static struct platform_driver tps6594_regulator_driver = {
> +	.driver = {
> +		.name = "tps6594-regulator",
> +	},
> +	.probe = tps6594_regulator_probe,
> +};

> +

This blank line is not needed.

> +module_platform_driver(tps6594_regulator_driver);

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 1/3] rtc: tps6594: Add driver for TPS6594 RTC

[Esteban Blanc]

On Tue May 23, 2023 at 3:36 PM CEST,  wrote:
> Mon, May 22, 2023 at 06:31:13PM +0200, Esteban Blanc kirjoitti:
> > TPS6594 PMIC is a MFD. This patch adds support for
> > the RTC found inside TPS6594 family of PMIC.
> >
> > Alarm is also supported.
>
> ...
>
> > +	help
> > +	  TI Power Management IC TPS6594 supports RTC functionality
> > +	  along with alarm. This driver supports the RTC driver for
> > +	  the TPS6594 RTC module.
> > +
> > +	  This driver can also be built as a module. If so, the module
> > +	  will be called tps6594-rtc
>
> Grammar period at the end?

Thanks!

> > +#define TPS6594_GET_TIME_ON TPS6594_BIT_GET_TIME
> > +#define TPS6594_GET_TIME_OFF 0
>
> Not used.

Thanks.
>
> > +#define TPS6594_IT_ALARM_ON TPS6594_BIT_IT_ALARM
> > +#define TPS6594_IT_ALARM_OFF 0
>
> Used only once.

True. Is that a bad thing?

> > +#define TPS6594_AUTO_COMP_ON TPS6594_BIT_IT_ALARM
>
> No _OFF counterpart.
>
> That said the _OFF can be dropped completely. And the rest I see no value to
> have, just use those bit definitions directly?

I was thinking it would make this more readable. I will remove them, no
problem.

> > +static int tps6594_rtc_alarm_irq_enable(struct device *dev,
> > +					unsigned int enabled)
> > +{
> > +	struct tps6594 *tps = dev_get_drvdata(dev->parent);
> > +	u8 val = 0;
>
> Redundant assignment.
>

Thanks!

> > +	// Read shadowed RTC registers.
> > +	ret = regmap_bulk_read(tps->regmap, TPS6594_REG_RTC_SECONDS, rtc_data,
> > +			       NUM_TIME_REGS);
> > +	if (ret < 0)
> > +		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;
>
> 	return 0;
>
> No?

`regmap_bulk_read` returns 0 on success so ret should be 0 here. I will
apply your suggestion anyway it is more readable. Thanks!

> > +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);
> > +	__le16 value;
> > +	int ret;
> > +
> > +	/*
> > +	 * TPS6594 uses two's complement 16 bit value for compensation of 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].
>
> This is defined naturally by the bits available, correct?

Your right. Maybe `calibration` argument should be an s16 instead of an
int?

> > +	 */
> > +	if (calibration < -32767 || calibration > 32767) {
>
> So, this can be S16_MIN / S16_MAX range. The question here is what the
> -32768 meaning is and why it can't be used.

I will rewrite it using this 2 macros.

This range [-32767,32767] is specified in the datasheet. As for why
-32768 can't be used I have no idea.

> > +		dev_err(dev, "RTC calibration value out of range: %d\n",
> > +			calibration);
> > +		return -EINVAL;
>
> -ERANGE

Ok, thanks.

> > +	}
>
> > +	value = (__le16)calibration;
> > +
> > +	comp_data[0] = value & 0xFF;
> > +	comp_data[1] = (value >> 8) & 0xFF;
>
> Of course these three lines is not what expected.
>
> 	value = cpu_to_le16();

Sorry for the mistake. I've tried to find more information on those type
but I did not realize there was specific functions/macros for them.

I have learn something today :)

> > +	// Update all the compensation registers in one shot.
> > +	ret = regmap_bulk_write(tps->regmap, TPS6594_REG_RTC_COMP_LSB,
> > +				comp_data, NUM_COMP_REGS);
>
> 				&value, sizeof(value) ?

This is way cleaner indeed.

> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	// Enable automatic compensation.
> > +	return regmap_set_bits(tps->regmap, TPS6594_REG_RTC_CTRL_1,
> > +			       TPS6594_BIT_AUTO_COMP);
> > +}

> > +	ret = regmap_bulk_read(tps->regmap, TPS6594_REG_RTC_COMP_LSB, comp_data,
> > +			       NUM_COMP_REGS);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	value = (__le16)comp_data[0] | ((__le16)comp_data[1] << 8);
> > +
> > +	*calibration = value;
>
> In the similar (complementary API) way as above.

Sure.

> > +	ret = devm_request_threaded_irq(&pdev->dev, irq, NULL,
>
> Having
>
> 	struct device *dev = &pdev->dev;
>
> might make this and other lines shorter / neater.

Will do, thanks.

> > +					tps6594_rtc_interrupt, IRQF_ONESHOT,
> > +					TPS6594_IRQ_NAME_ALARM, &pdev->dev);
> > +	if (ret < 0)
> > +		return dev_err_probe(&pdev->dev, ret,
> > +				     "Failed to request_threaded_irq\n");

Thanks for your review.

Best regards,

--
Esteban Blanc
BayLibre

Re: [PATCH v5 2/3] pinctrl: tps6594: Add driver for TPS6594 pinctrl and GPIOs

[Esteban Blanc]

On Tue May 23, 2023 at 9:14 PM CEST,  wrote:
> Mon, May 22, 2023 at 06:31:14PM +0200, Esteban Blanc kirjoitti:
> > TI TPS6594 PMIC has 11 GPIOs which can be used
> > for different functions.
> > 
> > This patch adds a pinctrl and GPIO drivers in
> > order to use those functions.
>
> ...
>
> > +#define FUNCTION(n, g, v)                                                    \
> > +	{                                                                    \
> > +		.pinfunction = PINCTRL_PINFUNCTION((n), (g), ARRAY_SIZE(g)), \
> > +		.muxval = v,                                                 \
> > +	}
>
> It seems you have used SPACEs before \, can you move to TABs?

Once again clang-format is not doing the right thing. Sur I will fix
this.

> > +// Used to compute register address of GPIO1_CONF to GPIO11_CONF
>
> This is good.
>
> > -#define TPS6594_REG_GPIOX_CONF(gpio_inst)		(0x31 + (gpio_inst))
> > +#define TPS6594_REG_GPIOX_CONF(gpio_inst)	(0x31 + (gpio_inst))
>
> But why this?!

Once again, clang-format... I will fix this.

Thanks for your help.
Best regards,

-- 
Esteban Blanc
BayLibre

Re: [PATCH v5 2/3] pinctrl: tps6594: Add driver for TPS6594 pinctrl and GPIOs

[Andy Shevchenko]

On Thu, May 25, 2023 at 5:07 PM Esteban Blanc <eblanc@baylibre.com> wrote:
> On Tue May 23, 2023 at 9:14 PM CEST,  wrote:
> > Mon, May 22, 2023 at 06:31:14PM +0200, Esteban Blanc kirjoitti:

...

> > > +// Used to compute register address of GPIO1_CONF to GPIO11_CONF
> >
> > This is good.

Btw

  the register

and shouldn't it start from GPIO0_CONF (please, double check, my
memory can do tricks on me)?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 2/3] pinctrl: tps6594: Add driver for TPS6594 pinctrl and GPIOs

[Esteban Blanc]

On Fri May 26, 2023 at 8:43 PM CEST, Andy Shevchenko wrote:
> On Thu, May 25, 2023 at 5:07 PM Esteban Blanc <eblanc@baylibre.com> wrote:
> > On Tue May 23, 2023 at 9:14 PM CEST,  wrote:
> > > Mon, May 22, 2023 at 06:31:14PM +0200, Esteban Blanc kirjoitti:
>
> ...
>
> > > > +// Used to compute register address of GPIO1_CONF to GPIO11_CONF
> > >
> > > This is good.
>
> Btw
>
>   the register

Ah yes, thanks.

> and shouldn't it start from GPIO0_CONF (please, double check, my
> memory can do tricks on me)?

On Linux side it is GPIO0 but on datasheet side it is GPIO1_CONF to
GPIO11_CONF. I was referring to the "real" register names (datasheet
ones)

>
> -- 
> With Best Regards,
> Andy Shevchenko

Re: [PATCH v5 0/3] TI TPS6594 PMIC support (RTC, pinctrl, regulators)

[Mark Brown]

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

On Mon, May 22, 2023 at 06:31:12PM +0200, Esteban Blanc wrote:

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

Please if you're doing this in future just post a single series, it
makes things much easier to follow.

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

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

[jerome Neanne]

> 
>> +	enum {
>> +		MULTI_BUCK12,
>> +		MULTI_BUCK123,
>> +		MULTI_BUCK1234,
>> +		MULTI_BUCK12_34,
> 
>> +		MULTI_FIRST = MULTI_BUCK12,
>> +		MULTI_LAST = MULTI_BUCK12_34,
>> +		MULTI_NUM = MULTI_LAST - MULTI_FIRST + 1
> 
> 		MULT_NUM
> 
> will suffice instead all this.
> 
>> +	};
> 
> But why enum at all? See below.
Just for the switch case readability.
I have to iterate across the multiphases array for look up name into 
device tree and evaluate in that order.

This can be reduced to:
	enum {
		MULTI_BUCK12,
		MULTI_BUCK123,
		MULTI_BUCK1234,
		MULTI_BUCK12_34,
		MULTI_NUM = MULTI_BUCK12_34 - MULTI_BUCK12 + 1
	};

> 
> ...
> 
>> +	/*
>> +	 * Switch case defines different possible multi phase config
>> +	 * This is based on dts buck node name.
>> +	 * Buck node name must be chosen accordingly.
>> +	 * 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
>> +	 */
>> +	for (multi = MULTI_FIRST; multi < MULTI_NUM; multi++) {
>> +		np = of_find_node_by_name(tps->dev->of_node, multiphases[multi]);
>> +		npname = of_node_full_name(np);
>> +		np_pmic_parent = of_get_parent(of_get_parent(np));
>> +		if (of_node_cmp(of_node_full_name(np_pmic_parent), tps->dev->of_node->full_name))
> 
> Why not of_node_full_name() in the second case?
Sure.
> 
> 
>> +			continue;
>> +		delta = strcmp(npname, multiphases[multi]);
>> +		if (!delta) {
>> +			switch (multi) {
>> +			case MULTI_BUCK12:
> 
> This all looks like match_string() reinvention.
I can go with match_string but this is not significantly changing the game:

index = match_string(multiphases, ARRAY_SIZE(multiphases), npname);
if (index >= 0) {
	switch (index) {

No question on all your other feedback. Just wondering if I missed 
something with match_string use. Looks like a good idea indeed but this 
is not drastically changing the code as you seem to expect... Let me 
know if you think I'm doing it in a wrong way.

Regards,
Jerome.

Re: (subset) [PATCH v5 0/3] TI TPS6594 PMIC support (RTC, pinctrl, regulators)

[Mark Brown]

On Mon, 22 May 2023 18:31:12 +0200, Esteban Blanc wrote:
> 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 LP8764 are derivatives.
> 
> This series adds support to TI TPS6594 PMIC and its derivatives.
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git for-next

Thanks!

[3/3] regulator: tps6594-regulator: Add driver for TI TPS6594 regulators
      commit: f17ccc5deb4d024bb52fd3433471e77ab7ae9ad8

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

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

[Andy Shevchenko]

On Wed, Jun 7, 2023 at 2:44 PM jerome Neanne <jneanne@baylibre.com> wrote:

...

> >> +    enum {
> >> +            MULTI_BUCK12,
> >> +            MULTI_BUCK123,
> >> +            MULTI_BUCK1234,
> >> +            MULTI_BUCK12_34,
> >
> >> +            MULTI_FIRST = MULTI_BUCK12,
> >> +            MULTI_LAST = MULTI_BUCK12_34,
> >> +            MULTI_NUM = MULTI_LAST - MULTI_FIRST + 1

> >               MULT_NUM
> >
> > will suffice instead of all this.

(1)

> >> +    };
> >
> > But why enum at all? See below.
> Just for the switch case readability.
> I have to iterate across the multiphases array for look up name into
> device tree and evaluate in that order.
>
> This can be reduced to:
>         enum {
>                 MULTI_BUCK12,
>                 MULTI_BUCK123,
>                 MULTI_BUCK1234,
>                 MULTI_BUCK12_34,

>                 MULTI_NUM = MULTI_BUCK12_34 - MULTI_BUCK12 + 1

See (1) above.

>         };

...

> >> +                    continue;
> >> +            delta = strcmp(npname, multiphases[multi]);
> >> +            if (!delta) {
> >> +                    switch (multi) {
> >> +                    case MULTI_BUCK12:
> >
> > This all looks like match_string() reinvention.
> I can go with match_string but this is not significantly changing the game:
>
> index = match_string(multiphases, ARRAY_SIZE(multiphases), npname);
> if (index >= 0) {
>         switch (index) {
>
> No question on all your other feedback. Just wondering if I missed
> something with match_string use. Looks like a good idea indeed but this
> is not drastically changing the code as you seem to expect... Let me
> know if you think I'm doing it in a wrong way.

I guess the entire big for-loop can be optimized, but I haven't looked
at that. At least match_string() would help understanding what you are
trying to do,

In any case it seems Mark applied your version, so the follow ups can be made.

-- 
With Best Regards,
Andy Shevchenko