* [PATCH v4 0/3] TI TPS6594 PMIC support (RTC, pinctrl, regulators)
@ 2023-05-12 14:17 Esteban Blanc
2023-05-12 14:17 ` [PATCH v4 1/3] rtc: tps6594: Add driver for TPS6594 RTC Esteban Blanc
` (2 more replies)
0 siblings, 3 replies; 20+ messages in thread
From: Esteban Blanc @ 2023-05-12 14:17 UTC (permalink / raw)
To: linus.walleij, lgirdwood, broonie, a.zummo, alexandre.belloni
Cc: linux-kernel, linux-gpio, linux-rtc, jpanis, jneanne, aseketeli,
eblanc, sterzik, u-kumar1
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/20230406075622.8990-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
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 | 31 ++
drivers/pinctrl/Makefile | 2 +
drivers/pinctrl/pinctrl-tps6594.c | 301 +++++++++++++
drivers/regulator/Kconfig | 13 +
drivers/regulator/Makefile | 1 +
drivers/regulator/tps6594-regulator.c | 620 ++++++++++++++++++++++++++
drivers/rtc/Kconfig | 9 +
drivers/rtc/Makefile | 1 +
drivers/rtc/rtc-tps6594.c | 479 ++++++++++++++++++++
include/linux/mfd/tps6594.h | 3 +-
10 files changed, 1459 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
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v4 1/3] rtc: tps6594: Add driver for TPS6594 RTC
2023-05-12 14:17 [PATCH v4 0/3] TI TPS6594 PMIC support (RTC, pinctrl, regulators) Esteban Blanc
@ 2023-05-12 14:17 ` Esteban Blanc
2023-05-12 17:22 ` andy.shevchenko
2023-05-12 14:17 ` [PATCH v4 2/3] pinctrl: tps6594: Add driver for TPS6594 pinctrl and GPIOs Esteban Blanc
2023-05-12 14:17 ` [PATCH v4 3/3] regulator: tps6594-regulator: Add driver for TI TPS6594 regulators Esteban Blanc
2 siblings, 1 reply; 20+ messages in thread
From: Esteban Blanc @ 2023-05-12 14:17 UTC (permalink / raw)
To: linus.walleij, lgirdwood, broonie, a.zummo, alexandre.belloni
Cc: linux-kernel, linux-gpio, linux-rtc, jpanis, jneanne, aseketeli,
eblanc, sterzik, u-kumar1
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 | 9 +
drivers/rtc/Makefile | 1 +
drivers/rtc/rtc-tps6594.c | 479 ++++++++++++++++++++++++++++++++++++++
3 files changed, 489 insertions(+)
create mode 100644 drivers/rtc/rtc-tps6594.c
diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
index 753872408615..b12784e63d61 100644
--- a/drivers/rtc/Kconfig
+++ b/drivers/rtc/Kconfig
@@ -578,6 +578,15 @@ 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.
+
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..db0401cbf9d0
--- /dev/null
+++ b/drivers/rtc/rtc-tps6594.c
@@ -0,0 +1,479 @@
+// 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>
+
+#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)
+ 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);
+}
+
+/*
+ * 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;
+
+ /* 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;
+}
+
+/*
+ * 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)
+ 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);
+}
+
+/*
+ * 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)
+ 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 ? 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)
+ 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);
+ 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)
+ 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;
+ u16 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 = (u16)comp_data[0] | ((u16)comp_data[1] << 8);
+
+ *calibration = (s16)value;
+
+ return ret;
+}
+
+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 ret;
+}
+
+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 * (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;
+
+ 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;
+ struct rtc_device *rtc;
+ int irq;
+ int ret;
+
+ tps = dev_get_drvdata(pdev->dev.parent);
+
+ 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) {
+ /* Start rtc */
+ ret = regmap_set_bits(tps->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(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) {
+ 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;
+ }
+
+ ret = device_init_wakeup(&pdev->dev, true);
+ if (ret < 0) {
+ dev_err(&pdev->dev, "Failed to init rtc as wakeup source\n");
+ return ret;
+ }
+
+ 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:tps6594-rtc");
+MODULE_AUTHOR("Esteban Blanc <eblanc@baylibre.com>");
+MODULE_DESCRIPTION("TPS6594 RTC driver");
+MODULE_LICENSE("GPL");
--
2.39.2
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v4 2/3] pinctrl: tps6594: Add driver for TPS6594 pinctrl and GPIOs
2023-05-12 14:17 [PATCH v4 0/3] TI TPS6594 PMIC support (RTC, pinctrl, regulators) Esteban Blanc
2023-05-12 14:17 ` [PATCH v4 1/3] rtc: tps6594: Add driver for TPS6594 RTC Esteban Blanc
@ 2023-05-12 14:17 ` Esteban Blanc
2023-05-12 17:07 ` andy.shevchenko
2023-05-12 14:17 ` [PATCH v4 3/3] regulator: tps6594-regulator: Add driver for TI TPS6594 regulators Esteban Blanc
2 siblings, 1 reply; 20+ messages in thread
From: Esteban Blanc @ 2023-05-12 14:17 UTC (permalink / raw)
To: linus.walleij, lgirdwood, broonie, a.zummo, alexandre.belloni
Cc: linux-kernel, linux-gpio, linux-rtc, jpanis, jneanne, aseketeli,
eblanc, sterzik, u-kumar1
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 | 31 +++
drivers/pinctrl/Makefile | 2 +
drivers/pinctrl/pinctrl-tps6594.c | 301 ++++++++++++++++++++++++++++++
include/linux/mfd/tps6594.h | 3 +-
4 files changed, 336 insertions(+), 1 deletion(-)
create mode 100644 drivers/pinctrl/pinctrl-tps6594.c
diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
index 5787c579dcf6..dd73df978d5c 100644
--- a/drivers/pinctrl/Kconfig
+++ b/drivers/pinctrl/Kconfig
@@ -480,6 +480,37 @@ config PINCTRL_TB10X
depends on OF && ARC_PLAT_TB10X
select GPIOLIB
+config PINCTRL_THUNDERBAY
+ tristate "Generic pinctrl and GPIO driver for Intel Thunder Bay SoC"
+ depends on ARCH_THUNDERBAY || (ARM64 && COMPILE_TEST)
+ depends on HAS_IOMEM
+ select PINMUX
+ select PINCONF
+ select GENERIC_PINCONF
+ select GENERIC_PINCTRL_GROUPS
+ select GENERIC_PINMUX_FUNCTIONS
+ select GPIOLIB
+ select GPIOLIB_IRQCHIP
+ select GPIO_GENERIC
+ help
+ This selects pin control driver for the Intel Thunder Bay SoC.
+ It provides pin config functions such as pull-up, pull-down,
+ interrupt, drive strength, sec lock, Schmitt trigger, slew
+ 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
+ default MFD_TPS6594
+ select PINMUX
+ select GPIOLIB
+ select REGMAP
+ select GPIO_REGMAP
+ help
+ This driver supports GPIOs and pinmuxing 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 e196c6e324ad..a48cbf4e6126 100644
--- a/drivers/pinctrl/Makefile
+++ b/drivers/pinctrl/Makefile
@@ -49,6 +49,8 @@ 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_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..322e83ce8f6e
--- /dev/null
+++ b/drivers/pinctrl/pinctrl-tps6594.c
@@ -0,0 +1,301 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Pinmux and GPIO driver for tps6594 PMIC
+ *
+ * Copyright (C) 2022 BayLibre Incorporated - https://www.baylibre.com/
+ */
+
+#include <linux/gpio/regmap.h>
+#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_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_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].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;
+ 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)) {
+ dev_err(&pdev->dev, "Couldn't register pinctrl driver\n");
+ return PTR_ERR(pinctrl->pctl_dev);
+ }
+
+ 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)) {
+ dev_err(&pdev->dev, "Couldn't register gpio_regmap driver\n");
+ return PTR_ERR(pinctrl->pctl_dev);
+ }
+
+ 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..93da782ed2b7 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))
+#define TPS6594_REG_GPIO1_CONF 0x31
+#define TPS6594_REG_GPIOX_CONF(gpio_inst) (TPS6594_REG_GPIO1_CONF + (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
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v4 3/3] regulator: tps6594-regulator: Add driver for TI TPS6594 regulators
2023-05-12 14:17 [PATCH v4 0/3] TI TPS6594 PMIC support (RTC, pinctrl, regulators) Esteban Blanc
2023-05-12 14:17 ` [PATCH v4 1/3] rtc: tps6594: Add driver for TPS6594 RTC Esteban Blanc
2023-05-12 14:17 ` [PATCH v4 2/3] pinctrl: tps6594: Add driver for TPS6594 pinctrl and GPIOs Esteban Blanc
@ 2023-05-12 14:17 ` Esteban Blanc
2023-05-12 17:34 ` andy.shevchenko
2 siblings, 1 reply; 20+ messages in thread
From: Esteban Blanc @ 2023-05-12 14:17 UTC (permalink / raw)
To: linus.walleij, lgirdwood, broonie, a.zummo, alexandre.belloni
Cc: linux-kernel, linux-gpio, linux-rtc, jpanis, jneanne, aseketeli,
eblanc, sterzik, u-kumar1
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 | 620 ++++++++++++++++++++++++++
3 files changed, 634 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..fa113d475593
--- /dev/null
+++ b/drivers/regulator/tps6594-regulator.c
@@ -0,0 +1,620 @@
+// 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 (strcmp((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_configured[0] = 1;
+ buck_configured[1] = 1;
+ buck_multi[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(tps->dev,
+ ARRAY_SIZE(tps6594_bucks_irq_types) *
+ REGS_INT_NB *
+ sizeof(struct tps6594_regulator_irq_data) +
+ ARRAY_SIZE(tps6594_ldos_irq_types) *
+ REGS_INT_NB *
+ sizeof(struct tps6594_regulator_irq_data),
+ 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)) {
+ dev_err(tps->dev, "failed to register %s regulator\n",
+ pdev->name);
+ return PTR_ERR(rdev);
+ }
+ /* 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)) {
+ dev_err(tps->dev, "failed to register %s regulator\n",
+ pdev->name);
+ return PTR_ERR(rdev);
+ }
+ 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)) {
+ dev_err(tps->dev,
+ "failed to register %s regulator\n",
+ pdev->name);
+ return PTR_ERR(rdev);
+ }
+ 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(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.39.2
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH v4 2/3] pinctrl: tps6594: Add driver for TPS6594 pinctrl and GPIOs
2023-05-12 14:17 ` [PATCH v4 2/3] pinctrl: tps6594: Add driver for TPS6594 pinctrl and GPIOs Esteban Blanc
@ 2023-05-12 17:07 ` andy.shevchenko
2023-05-16 13:05 ` Esteban Blanc
0 siblings, 1 reply; 20+ messages in thread
From: andy.shevchenko @ 2023-05-12 17:07 UTC (permalink / raw)
To: Esteban Blanc
Cc: linus.walleij, lgirdwood, broonie, a.zummo, alexandre.belloni,
linux-kernel, linux-gpio, linux-rtc, jpanis, jneanne, aseketeli,
sterzik, u-kumar1
Fri, May 12, 2023 at 04:17:54PM +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.
...
> +config PINCTRL_THUNDERBAY
Is it correct name? To me sounds not. The problem is that you use platform name
for the non-platform-wide pin control, i.e. for PMIC exclusively.
Did I miss anything?
> + tristate "Generic pinctrl and GPIO driver for Intel Thunder Bay SoC"
> + depends on ARCH_THUNDERBAY || (ARM64 && COMPILE_TEST)
This doesn't look correct, but I remember some Kconfig options that are using
this way of dependency.
> + depends on HAS_IOMEM
> + select PINMUX
> + select PINCONF
> + select GENERIC_PINCONF
> + select GENERIC_PINCTRL_GROUPS
> + select GENERIC_PINMUX_FUNCTIONS
> + select GPIOLIB
> + select GPIOLIB_IRQCHIP
> + select GPIO_GENERIC
> + help
> + This selects pin control driver for the Intel Thunder Bay SoC.
> + It provides pin config functions such as pull-up, pull-down,
> + interrupt, drive strength, sec lock, Schmitt trigger, slew
> + rate control and direction control. This module will be
> + called as pinctrl-thunderbay.
Ah, the above simply a mistake. right?
Why is it in this patch?
> +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
> + This driver supports GPIOs and pinmuxing for the TPS6594
> + PMICs chip family.
Module name?
...
> +obj-$(CONFIG_PINCTRL_THUNDERBAY) += pinctrl-thunderbay.o
Huh?!
> +obj-$(CONFIG_PINCTRL_TPS6594) += pinctrl-tps6594.o
...
> +#include <linux/gpio/regmap.h>
> +#include <linux/gpio/driver.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/pinctrl/pinmux.h>
Ordered?
...
> +static const char *groups_name[TPS6594_PINCTRL_PINS_NB] = {
> + "GPIO0", "GPIO1", "GPIO2", "GPIO3", "GPIO4", "GPIO5",
> + "GPIO6", "GPIO7", "GPIO8", "GPIO9", "GPIO10"
Leave trailing comma even for known size.
> +};
...
> +struct tps6594_pinctrl_function {
> + const char *name;
> + u8 muxval;
> + const char **groups;
> + unsigned long ngroups;
We have struct pinfunction. Use it here (as embedded).
> +};
...
> +static const struct tps6594_pinctrl_function pinctrl_functions[] = {
> + { "gpio", TPS6594_PINCTRL_GPIO_FUNCTION, groups_name,
> + TPS6594_PINCTRL_PINS_NB },
Here and further use PINCTRL_PINFUNCTION() macro.
> +};
...
> +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];
Why casting?
> + *num_pins = 1;
> +
> + return 0;
> +}
...
> + pinctrl->pctl_dev =
> + devm_pinctrl_register(&pdev->dev, pctrl_desc, pinctrl);
One line?
> + if (IS_ERR(pinctrl->pctl_dev)) {
> + dev_err(&pdev->dev, "Couldn't register pinctrl driver\n");
> + return PTR_ERR(pinctrl->pctl_dev);
return dev_err_probe(...);
> + }
...
> + pinctrl->gpio_regmap = devm_gpio_regmap_register(&pdev->dev, &config);
> + if (IS_ERR(pinctrl->gpio_regmap)) {
> + dev_err(&pdev->dev, "Couldn't register gpio_regmap driver\n");
> + return PTR_ERR(pinctrl->pctl_dev);
Ditto.
> + }
> +
> + return 0;
> +}
...
> -#define TPS6594_REG_GPIOX_CONF(gpio_inst) (0x31 + (gpio_inst))
> +#define TPS6594_REG_GPIO1_CONF 0x31
> +#define TPS6594_REG_GPIOX_CONF(gpio_inst) (TPS6594_REG_GPIO1_CONF + (gpio_inst))
Why? The original code with parameter 0 will issue the same.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v4 1/3] rtc: tps6594: Add driver for TPS6594 RTC
2023-05-12 14:17 ` [PATCH v4 1/3] rtc: tps6594: Add driver for TPS6594 RTC Esteban Blanc
@ 2023-05-12 17:22 ` andy.shevchenko
2023-05-17 16:47 ` Esteban Blanc
0 siblings, 1 reply; 20+ messages in thread
From: andy.shevchenko @ 2023-05-12 17:22 UTC (permalink / raw)
To: Esteban Blanc
Cc: linus.walleij, lgirdwood, broonie, a.zummo, alexandre.belloni,
linux-kernel, linux-gpio, linux-rtc, jpanis, jneanne, aseketeli,
sterzik, u-kumar1
Fri, May 12, 2023 at 04:17:53PM +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.
...
> +/*
> + * 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
Please, format it better and do not forget to use proper punctuation (commas,
periods, etc.).
> + */
> +#define MIN_OFFSET (-277774)
> +#define MAX_OFFSET (277774)
...
> +/* Multiplier for ppb conversions */
> +#define PPB_MULT (1000000000LL)
We have something in units.h. Can you use generic macro?
...
> + ret = regmap_update_bits(tps->regmap, TPS6594_REG_RTC_INTERRUPTS,
> + TPS6594_BIT_IT_ALARM, val);
> +
> + return ret;
return regmap_update_bits(...);
...
> + /*
> + * 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
> + */
Please, check all your multi-line comments for proper punctuation.
...
> + /* Check if RTC is running. */
Please, keep a single style for the one-line comments (with or without period
at the end).
> + alm->enabled = int_val & TPS6594_BIT_IT_ALARM ? 1 : 0;
Ternary is reduntand.
> + return ret;
Why not return 0 explicitly? Or do you return positive value?
...
> + comp_data[0] = (u16)value & 0xFF;
> + comp_data[1] = ((u16)value >> 8) & 0xFF;
Use proper bitwise type, i.e. __le16.
...
> + value = (u16)comp_data[0] | ((u16)comp_data[1] << 8);
Ditto.
...
> + /* Convert from RTC calibration register format to ppb format */
> + tmp = calibration * (s64)PPB_MULT;
Is casting really needed?
> + if (tmp < 0)
> + tmp -= TICKS_PER_HOUR / 2LL;
> + else
> + tmp += TICKS_PER_HOUR / 2LL;
Is it guaranteed to have no overflow here?
> + 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 ret;
ret?!
> +}
> +
> +static int tps6594_rtc_set_offset(struct device *dev, long offset)
> +{
> + int calibration;
> + s64 tmp;
Similar questions here as per above routine.
> + /* 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;
> +
> + return tps6594_rtc_set_calibration(dev, calibration);
> +}
...
> +static int tps6594_rtc_probe(struct platform_device *pdev)
> +{
> + struct tps6594 *tps;
> + struct rtc_device *rtc;
> + int irq;
> + int ret;
> + tps = dev_get_drvdata(pdev->dev.parent);
Can be united with definition of tps above.
...
> + /* RTC not running */
> + if (ret == 0) {
> + /* Start rtc */
RTC for the sake of consistency.
But I think one of the comment is redundant.
...
> + mdelay(100);
Such long delays have to be explicitly elaborated (in the comment on top).
> + }
...
> + irq = platform_get_irq_byname(pdev, TPS6594_IRQ_NAME_ALARM);
> + if (irq < 0) {
> + dev_err(&pdev->dev, "Failed to get irq\n");
> + return irq;
return dev_err_probe(...);
> + }
...
> + dev_err(&pdev->dev, "Failed to request_threaded_irq\n");
> + return ret;
Ditto.
...
> + dev_err(&pdev->dev, "Failed to init rtc as wakeup source\n");
> + return ret;
Ditto.
...
> +
Blank line is not needed.
> +module_platform_driver(tps6594_rtc_driver);
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v4 3/3] regulator: tps6594-regulator: Add driver for TI TPS6594 regulators
2023-05-12 14:17 ` [PATCH v4 3/3] regulator: tps6594-regulator: Add driver for TI TPS6594 regulators Esteban Blanc
@ 2023-05-12 17:34 ` andy.shevchenko
0 siblings, 0 replies; 20+ messages in thread
From: andy.shevchenko @ 2023-05-12 17:34 UTC (permalink / raw)
To: Esteban Blanc
Cc: linus.walleij, lgirdwood, broonie, a.zummo, alexandre.belloni,
linux-kernel, linux-gpio, linux-rtc, jpanis, jneanne, aseketeli,
sterzik, u-kumar1
Fri, May 12, 2023 at 04:17:55PM +0200, Esteban Blanc kirjoitti:
> 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.
...
> + 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
Missing TABs?
> + };
...
> + for (multi = MULTI_FIRST ; multi < MULTI_NUM ; multi++) {
Just a remark: spaces before ; are not standard.
> + 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 (strcmp((of_node_full_name(np_pmic_parent)), tps->dev->of_node->full_name))
> + continue;
Isn't there an API to compare OF node name with a given parameter?
> + 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_configured[0] = 1;
> + buck_configured[1] = 1;
> + buck_multi[1] = 1;
Might be easier to read if this is grouped with [0] assignment above.
> + 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(tps->dev,
> + ARRAY_SIZE(tps6594_bucks_irq_types) *
> + REGS_INT_NB *
> + sizeof(struct tps6594_regulator_irq_data) +
> + ARRAY_SIZE(tps6594_ldos_irq_types) *
> + REGS_INT_NB *
> + sizeof(struct tps6594_regulator_irq_data),
We have respective macros in overflow.h that can be used here.
> + GFP_KERNEL);
> + if (!irq_data)
> + return -ENOMEM;
...
> + 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);
return dev_err_probe(...);
?
> + }
...
> + 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);
Same question.
> + }
...
> + 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);
Same question.
> + }
> + irq_ext_reg_data = devm_kmalloc(tps->dev,
> + ext_reg_irq_nb *
> + sizeof(struct tps6594_ext_regulator_irq_data),
> + GFP_KERNEL);
devm_kmalloc_array()
> + if (!irq_ext_reg_data)
> + return -ENOMEM;
...
> + 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 dev_err_probe(...);
?
> + }
> + }
> + return 0;
> +}
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v4 2/3] pinctrl: tps6594: Add driver for TPS6594 pinctrl and GPIOs
2023-05-12 17:07 ` andy.shevchenko
@ 2023-05-16 13:05 ` Esteban Blanc
2023-05-16 16:48 ` Andy Shevchenko
0 siblings, 1 reply; 20+ messages in thread
From: Esteban Blanc @ 2023-05-16 13:05 UTC (permalink / raw)
To: andy.shevchenko
Cc: linus.walleij, lgirdwood, broonie, a.zummo, alexandre.belloni,
linux-kernel, linux-gpio, linux-rtc, jpanis, jneanne, aseketeli,
sterzik, u-kumar1
On Fri May 12, 2023 at 7:07 PM CEST, wrote:
> Fri, May 12, 2023 at 04:17:54PM +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.
>
> ...
>
> > +config PINCTRL_THUNDERBAY
>
> Is it correct name? To me sounds not. The problem is that you use platform name
> for the non-platform-wide pin control, i.e. for PMIC exclusively.
> Did I miss anything?
>
> > + tristate "Generic pinctrl and GPIO driver for Intel Thunder Bay SoC"
> > + depends on ARCH_THUNDERBAY || (ARM64 && COMPILE_TEST)
>
> This doesn't look correct, but I remember some Kconfig options that are using
> this way of dependency.
>
> > + depends on HAS_IOMEM
> > + select PINMUX
> > + select PINCONF
> > + select GENERIC_PINCONF
> > + select GENERIC_PINCTRL_GROUPS
> > + select GENERIC_PINMUX_FUNCTIONS
> > + select GPIOLIB
> > + select GPIOLIB_IRQCHIP
> > + select GPIO_GENERIC
> > + help
> > + This selects pin control driver for the Intel Thunder Bay SoC.
> > + It provides pin config functions such as pull-up, pull-down,
> > + interrupt, drive strength, sec lock, Schmitt trigger, slew
> > + rate control and direction control. This module will be
> > + called as pinctrl-thunderbay.
>
> Ah, the above simply a mistake. right?
> Why is it in this patch?
I respond to all comments about PINCTRL_THUNDERBAY.
It is indeed a mistake on my side. I failed my rebase on 6.4-rc1...
I will fix it for the next version.
Sorry about this...
> > +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
> > + This driver supports GPIOs and pinmuxing for the TPS6594
> > + PMICs chip family.
>
> Module name?
I'm not sure to understand what you are looking for. Something like this
?:
To compile this driver as a module, choose M here: the
module will be called pinctrl-tps6594.
> > +obj-$(CONFIG_PINCTRL_THUNDERBAY) += pinctrl-thunderbay.o
>
> Huh?!
Same as for Kconfig file, it's a mistake.
> > +#include <linux/gpio/regmap.h>
> > +#include <linux/gpio/driver.h>
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pinctrl/pinmux.h>
>
> Ordered?
>
It's not, I fix this.
> > +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];
>
> Why casting?
It's an error, thanks.
> > + pinctrl->pctl_dev =
> > + devm_pinctrl_register(&pdev->dev, pctrl_desc, pinctrl);
>
> One line?
I use clang-format quite extensively so I would rather keep it like
that to still be able to just run it over the whole file without having
to fix this line every time.
If you feel like I should not respect the 80 columns recommendation, I
will fix it.
> > + if (IS_ERR(pinctrl->pctl_dev)) {
> > + dev_err(&pdev->dev, "Couldn't register pinctrl driver\n");
> > + return PTR_ERR(pinctrl->pctl_dev);
>
> return dev_err_probe(...);
Did not know this one, I will use it. Thanks.
> > -#define TPS6594_REG_GPIOX_CONF(gpio_inst) (0x31 + (gpio_inst))
> > +#define TPS6594_REG_GPIO1_CONF 0x31
> > +#define TPS6594_REG_GPIOX_CONF(gpio_inst) (TPS6594_REG_GPIO1_CONF + (gpio_inst))
>
> Why? The original code with parameter 0 will issue the same.
I felt that replacing 0x31 with a constant would make the computation
in TPS6594_REG_GPIOX_CONFIG more understandable. What do you think?
Thanks for your time. Sorry again about this "thunderbay" confusion...
Note for the future, don't send patches on Fridays :)
Best regards,
--
Esteban Blanc
BayLibre
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v4 2/3] pinctrl: tps6594: Add driver for TPS6594 pinctrl and GPIOs
2023-05-16 13:05 ` Esteban Blanc
@ 2023-05-16 16:48 ` Andy Shevchenko
2023-05-17 9:58 ` Esteban Blanc
0 siblings, 1 reply; 20+ messages in thread
From: Andy Shevchenko @ 2023-05-16 16:48 UTC (permalink / raw)
To: Esteban Blanc
Cc: linus.walleij, lgirdwood, broonie, a.zummo, alexandre.belloni,
linux-kernel, linux-gpio, linux-rtc, jpanis, jneanne, aseketeli,
sterzik, u-kumar1
On Tue, May 16, 2023 at 4:05 PM Esteban Blanc <eblanc@baylibre.com> wrote:
> On Fri May 12, 2023 at 7:07 PM CEST, wrote:
> > Fri, May 12, 2023 at 04:17:54PM +0200, Esteban Blanc kirjoitti:
...
> > > +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
> > > + This driver supports GPIOs and pinmuxing for the TPS6594
> > > + PMICs chip family.
> >
> > Module name?
>
> I'm not sure to understand what you are looking for. Something like this
> ?:
>
> To compile this driver as a module, choose M here: the
> module will be called pinctrl-tps6594.
Yes.
...
> > > + pinctrl->pctl_dev =
> > > + devm_pinctrl_register(&pdev->dev, pctrl_desc, pinctrl);
> >
> > One line?
>
> I use clang-format quite extensively so I would rather keep it like
> that to still be able to just run it over the whole file without having
> to fix this line every time.
> If you feel like I should not respect the 80 columns recommendation, I
> will fix it.
As you wish.
...
> > > -#define TPS6594_REG_GPIOX_CONF(gpio_inst) (0x31 + (gpio_inst))
> > > +#define TPS6594_REG_GPIO1_CONF 0x31
> > > +#define TPS6594_REG_GPIOX_CONF(gpio_inst) (TPS6594_REG_GPIO1_CONF + (gpio_inst))
> >
> > Why? The original code with parameter 0 will issue the same.
>
> I felt that replacing 0x31 with a constant would make the computation
> in TPS6594_REG_GPIOX_CONFIG more understandable. What do you think?
The question is why that register is so special that you need to have
it as a constant explicitly?
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v4 2/3] pinctrl: tps6594: Add driver for TPS6594 pinctrl and GPIOs
2023-05-16 16:48 ` Andy Shevchenko
@ 2023-05-17 9:58 ` Esteban Blanc
2023-05-17 13:51 ` Andy Shevchenko
0 siblings, 1 reply; 20+ messages in thread
From: Esteban Blanc @ 2023-05-17 9:58 UTC (permalink / raw)
To: Andy Shevchenko
Cc: linus.walleij, lgirdwood, broonie, a.zummo, alexandre.belloni,
linux-kernel, linux-gpio, linux-rtc, jpanis, jneanne, aseketeli,
sterzik, u-kumar1
On Tue May 16, 2023 at 6:48 PM CEST, Andy Shevchenko wrote:
> On Tue, May 16, 2023 at 4:05 PM Esteban Blanc <eblanc@baylibre.com> wrote:
> > On Fri May 12, 2023 at 7:07 PM CEST, wrote:
> > > Fri, May 12, 2023 at 04:17:54PM +0200, Esteban Blanc kirjoitti:
...
> > > > -#define TPS6594_REG_GPIOX_CONF(gpio_inst) (0x31 + (gpio_inst))
> > > > +#define TPS6594_REG_GPIO1_CONF 0x31
> > > > +#define TPS6594_REG_GPIOX_CONF(gpio_inst) (TPS6594_REG_GPIO1_CONF + (gpio_inst))
> > >
> > > Why? The original code with parameter 0 will issue the same.
> >
> > I felt that replacing 0x31 with a constant would make the computation
> > in TPS6594_REG_GPIOX_CONFIG more understandable. What do you think?
>
> The question is why that register is so special that you need to have
> it as a constant explicitly?
It is not special, it's just the first one of the serie of config
registers. I felt like just having 0x31 without context was a bit weird
Best regards,
--
Esteban Blanc
BayLibre
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v4 2/3] pinctrl: tps6594: Add driver for TPS6594 pinctrl and GPIOs
2023-05-17 9:58 ` Esteban Blanc
@ 2023-05-17 13:51 ` Andy Shevchenko
2023-05-17 14:43 ` Esteban Blanc
0 siblings, 1 reply; 20+ messages in thread
From: Andy Shevchenko @ 2023-05-17 13:51 UTC (permalink / raw)
To: Esteban Blanc
Cc: linus.walleij, lgirdwood, broonie, a.zummo, alexandre.belloni,
linux-kernel, linux-gpio, linux-rtc, jpanis, jneanne, aseketeli,
sterzik, u-kumar1
On Wed, May 17, 2023 at 12:58 PM Esteban Blanc <eblanc@baylibre.com> wrote:
> On Tue May 16, 2023 at 6:48 PM CEST, Andy Shevchenko wrote:
> > On Tue, May 16, 2023 at 4:05 PM Esteban Blanc <eblanc@baylibre.com> wrote:
> > > On Fri May 12, 2023 at 7:07 PM CEST, wrote:
> > > > Fri, May 12, 2023 at 04:17:54PM +0200, Esteban Blanc kirjoitti:
...
> > > > > -#define TPS6594_REG_GPIOX_CONF(gpio_inst) (0x31 + (gpio_inst))
> > > > > +#define TPS6594_REG_GPIO1_CONF 0x31
> > > > > +#define TPS6594_REG_GPIOX_CONF(gpio_inst) (TPS6594_REG_GPIO1_CONF + (gpio_inst))
> > > >
> > > > Why? The original code with parameter 0 will issue the same.
> > >
> > > I felt that replacing 0x31 with a constant would make the computation
> > > in TPS6594_REG_GPIOX_CONFIG more understandable. What do you think?
> >
> > The question is why that register is so special that you need to have
> > it as a constant explicitly?
>
> It is not special, it's just the first one of the serie of config
> registers. I felt like just having 0x31 without context was a bit weird
I'm not sure I understand what 'context' you are talking about.
This is pretty normal to have two kind of definitions (depending on the case):
1/
#define FOO_1 ...
#define FOO_2 ...
and so on
2/
#define FOO(x) (... (x) ...)
Having a mix of them seems quite unusual.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v4 2/3] pinctrl: tps6594: Add driver for TPS6594 pinctrl and GPIOs
2023-05-17 13:51 ` Andy Shevchenko
@ 2023-05-17 14:43 ` Esteban Blanc
2023-05-17 15:04 ` Andy Shevchenko
0 siblings, 1 reply; 20+ messages in thread
From: Esteban Blanc @ 2023-05-17 14:43 UTC (permalink / raw)
To: Andy Shevchenko
Cc: linus.walleij, lgirdwood, broonie, a.zummo, alexandre.belloni,
linux-kernel, linux-gpio, linux-rtc, jpanis, jneanne, aseketeli,
sterzik, u-kumar1
On Wed May 17, 2023 at 3:51 PM CEST, Andy Shevchenko wrote:
> On Wed, May 17, 2023 at 12:58 PM Esteban Blanc <eblanc@baylibre.com> wrote:
> > On Tue May 16, 2023 at 6:48 PM CEST, Andy Shevchenko wrote:
> > > On Tue, May 16, 2023 at 4:05 PM Esteban Blanc <eblanc@baylibre.com> wrote:
> > > > On Fri May 12, 2023 at 7:07 PM CEST, wrote:
> > > > > Fri, May 12, 2023 at 04:17:54PM +0200, Esteban Blanc kirjoitti:
>
> ...
>
> > > > > > -#define TPS6594_REG_GPIOX_CONF(gpio_inst) (0x31 + (gpio_inst))
> > > > > > +#define TPS6594_REG_GPIO1_CONF 0x31
> > > > > > +#define TPS6594_REG_GPIOX_CONF(gpio_inst) (TPS6594_REG_GPIO1_CONF + (gpio_inst))
> > > > >
> > > > > Why? The original code with parameter 0 will issue the same.
> > > >
> > > > I felt that replacing 0x31 with a constant would make the computation
> > > > in TPS6594_REG_GPIOX_CONFIG more understandable. What do you think?
> > >
> > > The question is why that register is so special that you need to have
> > > it as a constant explicitly?
> >
> > It is not special, it's just the first one of the serie of config
> > registers. I felt like just having 0x31 without context was a bit weird
>
> I'm not sure I understand what 'context' you are talking about.
I was trying to convey the fact that 0x31 was representing
TPS6594_REG_GPIO1_CONF address. This way when looking at
TPS6594_REG_GPIOX_CONF(...), one will better understand that this macro
is just about offsetting from the first GPIO_CONF register.
> This is pretty normal to have two kind of definitions (depending on the case):
> 1/
>
> #define FOO_1 ...
> #define FOO_2 ...
>
> and so on
>
> 2/
>
> #define FOO(x) (... (x) ...)
>
>
> Having a mix of them seems quite unusual.
I did not know that. I will revert this change for next version then.
Thanks again for your time. Best regards,
--
Esteban Blanc
BayLibre
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v4 2/3] pinctrl: tps6594: Add driver for TPS6594 pinctrl and GPIOs
2023-05-17 14:43 ` Esteban Blanc
@ 2023-05-17 15:04 ` Andy Shevchenko
2023-05-22 8:45 ` Esteban Blanc
2023-05-23 9:26 ` Esteban Blanc
0 siblings, 2 replies; 20+ messages in thread
From: Andy Shevchenko @ 2023-05-17 15:04 UTC (permalink / raw)
To: Esteban Blanc
Cc: linus.walleij, lgirdwood, broonie, a.zummo, alexandre.belloni,
linux-kernel, linux-gpio, linux-rtc, jpanis, jneanne, aseketeli,
sterzik, u-kumar1
On Wed, May 17, 2023 at 5:43 PM Esteban Blanc <eblanc@baylibre.com> wrote:
> On Wed May 17, 2023 at 3:51 PM CEST, Andy Shevchenko wrote:
> > On Wed, May 17, 2023 at 12:58 PM Esteban Blanc <eblanc@baylibre.com> wrote:
> > > On Tue May 16, 2023 at 6:48 PM CEST, Andy Shevchenko wrote:
> > > > On Tue, May 16, 2023 at 4:05 PM Esteban Blanc <eblanc@baylibre.com> wrote:
> > > > > On Fri May 12, 2023 at 7:07 PM CEST, wrote:
> > > > > > Fri, May 12, 2023 at 04:17:54PM +0200, Esteban Blanc kirjoitti:
...
> > > > > > > -#define TPS6594_REG_GPIOX_CONF(gpio_inst) (0x31 + (gpio_inst))
> > > > > > > +#define TPS6594_REG_GPIO1_CONF 0x31
> > > > > > > +#define TPS6594_REG_GPIOX_CONF(gpio_inst) (TPS6594_REG_GPIO1_CONF + (gpio_inst))
> > > > > >
> > > > > > Why? The original code with parameter 0 will issue the same.
> > > > >
> > > > > I felt that replacing 0x31 with a constant would make the computation
> > > > > in TPS6594_REG_GPIOX_CONFIG more understandable. What do you think?
> > > >
> > > > The question is why that register is so special that you need to have
> > > > it as a constant explicitly?
> > >
> > > It is not special, it's just the first one of the serie of config
> > > registers. I felt like just having 0x31 without context was a bit weird
> >
> > I'm not sure I understand what 'context' you are talking about.
> I was trying to convey the fact that 0x31 was representing
> TPS6594_REG_GPIO1_CONF address. This way when looking at
> TPS6594_REG_GPIOX_CONF(...), one will better understand that this macro
> is just about offsetting from the first GPIO_CONF register.
You can add a comment on top of the macro, so anybody can read and see
what this macro is doing.
> > This is pretty normal to have two kind of definitions (depending on the case):
> > 1/
> >
> > #define FOO_1 ...
> > #define FOO_2 ...
> >
> > and so on
> >
> > 2/
> >
> > #define FOO(x) (... (x) ...)
> >
> > Having a mix of them seems quite unusual.
> I did not know that. I will revert this change for next version then.
Don't get me wrong, it's possible to have, but since it's unusual it
needs to be well justified. In the change you proposed you have
changed that, but I haven't seen where the new definition is used (in
*.c files).
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v4 1/3] rtc: tps6594: Add driver for TPS6594 RTC
2023-05-12 17:22 ` andy.shevchenko
@ 2023-05-17 16:47 ` Esteban Blanc
2023-05-17 16:52 ` Andy Shevchenko
0 siblings, 1 reply; 20+ messages in thread
From: Esteban Blanc @ 2023-05-17 16:47 UTC (permalink / raw)
To: andy.shevchenko
Cc: linus.walleij, lgirdwood, broonie, a.zummo, alexandre.belloni,
linux-kernel, linux-gpio, linux-rtc, jpanis, jneanne, aseketeli,
sterzik, u-kumar1
On Fri May 12, 2023 at 7:22 PM CEST, wrote:
> Fri, May 12, 2023 at 04:17:53PM +0200, Esteban Blanc kirjoitti:
> > +/* Multiplier for ppb conversions */
> > +#define PPB_MULT (1000000000LL)
>
> We have something in units.h. Can you use generic macro?
I found GIGA, NANO and NANOHZ_PER_HZ that have the same value in
units.h. However I'm not sure any of them have the correct meaning in
this situation.
> > + /*
> > + * 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
> > + */
>
> Please, check all your multi-line comments for proper punctuation.
Ok. I will do my best.
> > + /* Check if RTC is running. */
>
> Please, keep a single style for the one-line comments (with or without period
> at the end).
Sure.
> > + return ret;
>
> Why not return 0 explicitly? Or do you return positive value?
I should return 0 here, you are right.
> > + comp_data[0] = (u16)value & 0xFF;
> > + comp_data[1] = ((u16)value >> 8) & 0xFF;
>
> Use proper bitwise type, i.e. __le16.
I was not aware of theses kind of types. Thanks!
> > + /* Convert from RTC calibration register format to ppb format */
> > + tmp = calibration * (s64)PPB_MULT;
>
> Is casting really needed?
No it's not. Thanks!
> > + if (tmp < 0)
> > + tmp -= TICKS_PER_HOUR / 2LL;
> > + else
> > + tmp += TICKS_PER_HOUR / 2LL;
>
> Is it guaranteed to have no overflow here?
We know from `tps6594_rtc_set_offset` that the loaded value can't be
more than 277774 (register default value is 0), So `tmp` can't exceed
277774000000000 which is lower than 2^63-1. No overflow here.
TICK_PER_HOUR / 2LL = 117964800, so at the end of this computation,
`tmp` can have a maximum value of 277774117964800 which is still
inferior to 2^63-1.
> > + 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 ret;
>
> ret?!
That's a mistake. Thanks!
> > + /* RTC not running */
> > + if (ret == 0) {
> > + /* Start rtc */
>
> RTC for the sake of consistency.
>
> But I think one of the comment is redundant.
I will remove the second one
> > + mdelay(100);
>
> Such long delays have to be explicitly elaborated (in the comment on top).
This should just not be there.
Best regards,
--
Esteban Blanc
BayLibre
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v4 1/3] rtc: tps6594: Add driver for TPS6594 RTC
2023-05-17 16:47 ` Esteban Blanc
@ 2023-05-17 16:52 ` Andy Shevchenko
2023-05-22 11:49 ` Esteban Blanc
0 siblings, 1 reply; 20+ messages in thread
From: Andy Shevchenko @ 2023-05-17 16:52 UTC (permalink / raw)
To: Esteban Blanc
Cc: linus.walleij, lgirdwood, broonie, a.zummo, alexandre.belloni,
linux-kernel, linux-gpio, linux-rtc, jpanis, jneanne, aseketeli,
sterzik, u-kumar1
On Wed, May 17, 2023 at 7:47 PM Esteban Blanc <eblanc@baylibre.com> wrote:
> On Fri May 12, 2023 at 7:22 PM CEST, wrote:
> > Fri, May 12, 2023 at 04:17:53PM +0200, Esteban Blanc kirjoitti:
...
> > > +/* Multiplier for ppb conversions */
> > > +#define PPB_MULT (1000000000LL)
> >
> > We have something in units.h. Can you use generic macro?
>
> I found GIGA, NANO and NANOHZ_PER_HZ that have the same value in
> units.h. However I'm not sure any of them have the correct meaning in
> this situation.
MULT[IPLIER] has no units AFAIU, so SI macro can be used, no? NANO or
GIGA depends on what the actual sign of the exponent of the multiplier
is. Write it on paper and check the exponent in the equation(s) and
hence decide which one to use.
...
> > > + if (tmp < 0)
> > > + tmp -= TICKS_PER_HOUR / 2LL;
> > > + else
> > > + tmp += TICKS_PER_HOUR / 2LL;
> >
> > Is it guaranteed to have no overflow here?
>
> We know from `tps6594_rtc_set_offset` that the loaded value can't be
> more than 277774 (register default value is 0), So `tmp` can't exceed
> 277774000000000 which is lower than 2^63-1. No overflow here.
>
> TICK_PER_HOUR / 2LL = 117964800, so at the end of this computation,
> `tmp` can have a maximum value of 277774117964800 which is still
> inferior to 2^63-1.
Please add a respective comment.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v4 2/3] pinctrl: tps6594: Add driver for TPS6594 pinctrl and GPIOs
2023-05-17 15:04 ` Andy Shevchenko
@ 2023-05-22 8:45 ` Esteban Blanc
2023-05-23 9:26 ` Esteban Blanc
1 sibling, 0 replies; 20+ messages in thread
From: Esteban Blanc @ 2023-05-22 8:45 UTC (permalink / raw)
To: Andy Shevchenko
Cc: linus.walleij, lgirdwood, broonie, a.zummo, alexandre.belloni,
linux-kernel, linux-gpio, linux-rtc, jpanis, jneanne, aseketeli,
sterzik, u-kumar1
On Wed May 17, 2023 at 5:04 PM CEST, Andy Shevchenko wrote:
> On Wed, May 17, 2023 at 5:43 PM Esteban Blanc <eblanc@baylibre.com> wrote:
> > On Wed May 17, 2023 at 3:51 PM CEST, Andy Shevchenko wrote:
> > > On Wed, May 17, 2023 at 12:58 PM Esteban Blanc <eblanc@baylibre.com> wrote:
> > > > On Tue May 16, 2023 at 6:48 PM CEST, Andy Shevchenko wrote:
> > > > > On Tue, May 16, 2023 at 4:05 PM Esteban Blanc <eblanc@baylibre.com> wrote:
> > > > > > On Fri May 12, 2023 at 7:07 PM CEST, wrote:
> > > > > > > Fri, May 12, 2023 at 04:17:54PM +0200, Esteban Blanc kirjoitti:
>
> ...
>
> > > > > > > > -#define TPS6594_REG_GPIOX_CONF(gpio_inst) (0x31 + (gpio_inst))
> > > > > > > > +#define TPS6594_REG_GPIO1_CONF 0x31
> > > > > > > > +#define TPS6594_REG_GPIOX_CONF(gpio_inst) (TPS6594_REG_GPIO1_CONF + (gpio_inst))
> > > > > > >
> > > > > > > Why? The original code with parameter 0 will issue the same.
> > > > > >
> > > > > > I felt that replacing 0x31 with a constant would make the computation
> > > > > > in TPS6594_REG_GPIOX_CONFIG more understandable. What do you think?
> > > > >
> > > > > The question is why that register is so special that you need to have
> > > > > it as a constant explicitly?
> > > >
> > > > It is not special, it's just the first one of the serie of config
> > > > registers. I felt like just having 0x31 without context was a bit weird
> > >
> > > I'm not sure I understand what 'context' you are talking about.
> > I was trying to convey the fact that 0x31 was representing
> > TPS6594_REG_GPIO1_CONF address. This way when looking at
> > TPS6594_REG_GPIOX_CONF(...), one will better understand that this macro
> > is just about offsetting from the first GPIO_CONF register.
>
> You can add a comment on top of the macro, so anybody can read and see
> what this macro is doing.
Ok I will do that then. Thanks :)
> > > This is pretty normal to have two kind of definitions (depending on the case):
> > > 1/
> > >
> > > #define FOO_1 ...
> > > #define FOO_2 ...
> > >
> > > and so on
> > >
> > > 2/
> > >
> > > #define FOO(x) (... (x) ...)
> > >
> > > Having a mix of them seems quite unusual.
> > I did not know that. I will revert this change for next version then.
>
> Don't get me wrong, it's possible to have, but since it's unusual it
> needs to be well justified. In the change you proposed you have
> changed that, but I haven't seen where the new definition is used (in
> *.c files).
GPIO1_CONF is only used by the GPIOX_CONF macro in the header.
Best regards,
--
Esteban Blanc
BayLibre
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v4 1/3] rtc: tps6594: Add driver for TPS6594 RTC
2023-05-17 16:52 ` Andy Shevchenko
@ 2023-05-22 11:49 ` Esteban Blanc
0 siblings, 0 replies; 20+ messages in thread
From: Esteban Blanc @ 2023-05-22 11:49 UTC (permalink / raw)
To: Andy Shevchenko
Cc: linus.walleij, lgirdwood, broonie, a.zummo, alexandre.belloni,
linux-kernel, linux-gpio, linux-rtc, jpanis, jneanne, aseketeli,
sterzik, u-kumar1
On Wed May 17, 2023 at 6:52 PM CEST, Andy Shevchenko wrote:
> On Wed, May 17, 2023 at 7:47 PM Esteban Blanc <eblanc@baylibre.com> wrote:
> > On Fri May 12, 2023 at 7:22 PM CEST, wrote:
> > > Fri, May 12, 2023 at 04:17:53PM +0200, Esteban Blanc kirjoitti:
>
> ...
>
> > > > +/* Multiplier for ppb conversions */
> > > > +#define PPB_MULT (1000000000LL)
> > >
> > > We have something in units.h. Can you use generic macro?
> >
> > I found GIGA, NANO and NANOHZ_PER_HZ that have the same value in
> > units.h. However I'm not sure any of them have the correct meaning in
> > this situation.
>
> MULT[IPLIER] has no units AFAIU, so SI macro can be used, no? NANO or
> GIGA depends on what the actual sign of the exponent of the multiplier
> is. Write it on paper and check the exponent in the equation(s) and
> hence decide which one to use.
Thanks. I've checked and it should be NANO.
> > > > + if (tmp < 0)
> > > > + tmp -= TICKS_PER_HOUR / 2LL;
> > > > + else
> > > > + tmp += TICKS_PER_HOUR / 2LL;
> > >
> > > Is it guaranteed to have no overflow here?
> >
> > We know from `tps6594_rtc_set_offset` that the loaded value can't be
> > more than 277774 (register default value is 0), So `tmp` can't exceed
> > 277774000000000 which is lower than 2^63-1. No overflow here.
> >
> > TICK_PER_HOUR / 2LL = 117964800, so at the end of this computation,
> > `tmp` can have a maximum value of 277774117964800 which is still
> > inferior to 2^63-1.
>
> Please add a respective comment.
I've reformatted this and put it in a SAFETY comment.
Thanks for your help,
--
Esteban Blanc
BayLibre
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v4 2/3] pinctrl: tps6594: Add driver for TPS6594 pinctrl and GPIOs
2023-05-17 15:04 ` Andy Shevchenko
2023-05-22 8:45 ` Esteban Blanc
@ 2023-05-23 9:26 ` Esteban Blanc
2023-05-23 11:03 ` Andy Shevchenko
1 sibling, 1 reply; 20+ messages in thread
From: Esteban Blanc @ 2023-05-23 9:26 UTC (permalink / raw)
To: Andy Shevchenko
Cc: linus.walleij, lgirdwood, broonie, a.zummo, alexandre.belloni,
linux-kernel, linux-gpio, linux-rtc, jpanis, jneanne, aseketeli,
sterzik, u-kumar1
On Wed May 17, 2023 at 5:04 PM CEST, Andy Shevchenko wrote:
> On Wed, May 17, 2023 at 5:43 PM Esteban Blanc <eblanc@baylibre.com> wrote:
> > On Wed May 17, 2023 at 3:51 PM CEST, Andy Shevchenko wrote:
> > > On Wed, May 17, 2023 at 12:58 PM Esteban Blanc <eblanc@baylibre.com> wrote:
> > > > On Tue May 16, 2023 at 6:48 PM CEST, Andy Shevchenko wrote:
> > > > > On Tue, May 16, 2023 at 4:05 PM Esteban Blanc <eblanc@baylibre.com> wrote:
> > > > > > On Fri May 12, 2023 at 7:07 PM CEST, wrote:
> > > > > > > Fri, May 12, 2023 at 04:17:54PM +0200, Esteban Blanc kirjoitti:
>
> ...
>
> > > > > > > > -#define TPS6594_REG_GPIOX_CONF(gpio_inst) (0x31 + (gpio_inst))
> > > > > > > > +#define TPS6594_REG_GPIO1_CONF 0x31
> > > > > > > > +#define TPS6594_REG_GPIOX_CONF(gpio_inst) (TPS6594_REG_GPIO1_CONF + (gpio_inst))
> > > > > > >
> > > > > > > Why? The original code with parameter 0 will issue the same.
> > > > > >
> > > > > > I felt that replacing 0x31 with a constant would make the computation
> > > > > > in TPS6594_REG_GPIOX_CONFIG more understandable. What do you think?
> > > > >
> > > > > The question is why that register is so special that you need to have
> > > > > it as a constant explicitly?
> > > >
> > > > It is not special, it's just the first one of the serie of config
> > > > registers. I felt like just having 0x31 without context was a bit weird
> > >
> > > I'm not sure I understand what 'context' you are talking about.
> > I was trying to convey the fact that 0x31 was representing
> > TPS6594_REG_GPIO1_CONF address. This way when looking at
> > TPS6594_REG_GPIOX_CONF(...), one will better understand that this macro
> > is just about offsetting from the first GPIO_CONF register.
>
> You can add a comment on top of the macro, so anybody can read and see
> what this macro is doing.
> > > This is pretty normal to have two kind of definitions (depending on the case):
> > > 1/
> > >
> > > #define FOO_1 ...
> > > #define FOO_2 ...
> > >
> > > and so on
> > >
> > > 2/
> > >
> > > #define FOO(x) (... (x) ...)
> > >
> > > Having a mix of them seems quite unusual.
> > I did not know that. I will revert this change for next version then.
>
> Don't get me wrong, it's possible to have, but since it's unusual it
> needs to be well justified. In the change you proposed you have
> changed that, but I haven't seen where the new definition is used (in
> *.c files).
Actualy it used in 2 places:
- In the switch case of `tps6594_gpio_regmap_xlate`
- In `tps6594_pinctrl_probe` when setting `reg_dir_out_base`
I already sent a v5 with this change but I managed to fail my .config
and this driver was not compiled... and it is not compiling... I feel so
stupid.
I need to send a v6 now anyway. Should I convert all
TPS6594_REG_GPIO1_CONF to TPS6594_REG_GPIOX_CONF(0)?
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v4 2/3] pinctrl: tps6594: Add driver for TPS6594 pinctrl and GPIOs
2023-05-23 9:26 ` Esteban Blanc
@ 2023-05-23 11:03 ` Andy Shevchenko
2023-05-23 11:32 ` Esteban Blanc
0 siblings, 1 reply; 20+ messages in thread
From: Andy Shevchenko @ 2023-05-23 11:03 UTC (permalink / raw)
To: Esteban Blanc
Cc: linus.walleij, lgirdwood, broonie, a.zummo, alexandre.belloni,
linux-kernel, linux-gpio, linux-rtc, jpanis, jneanne, aseketeli,
sterzik, u-kumar1
On Tue, May 23, 2023 at 12:26 PM Esteban Blanc <eblanc@baylibre.com> wrote:
> On Wed May 17, 2023 at 5:04 PM CEST, Andy Shevchenko wrote:
> > On Wed, May 17, 2023 at 5:43 PM Esteban Blanc <eblanc@baylibre.com> wrote:
...
> > Don't get me wrong, it's possible to have, but since it's unusual it
> > needs to be well justified. In the change you proposed you have
> > changed that, but I haven't seen where the new definition is used (in
> > *.c files).
>
> Actualy it used in 2 places:
> - In the switch case of `tps6594_gpio_regmap_xlate`
> - In `tps6594_pinctrl_probe` when setting `reg_dir_out_base`
>
> I already sent a v5 with this change but I managed to fail my .config
> and this driver was not compiled... and it is not compiling... I feel so
> stupid.
People are prone to making mistakes. :-)
> I need to send a v6 now anyway. Should I convert all
> TPS6594_REG_GPIO1_CONF to TPS6594_REG_GPIOX_CONF(0)?
Again, if you want to leave that definition you need to well justify
why it's so special that code needs it. Easiest way is to use the
macro with 0 as an argument.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v4 2/3] pinctrl: tps6594: Add driver for TPS6594 pinctrl and GPIOs
2023-05-23 11:03 ` Andy Shevchenko
@ 2023-05-23 11:32 ` Esteban Blanc
0 siblings, 0 replies; 20+ messages in thread
From: Esteban Blanc @ 2023-05-23 11:32 UTC (permalink / raw)
To: Andy Shevchenko
Cc: linus.walleij, lgirdwood, broonie, a.zummo, alexandre.belloni,
linux-kernel, linux-gpio, linux-rtc, jpanis, jneanne, aseketeli,
sterzik, u-kumar1
On Tue May 23, 2023 at 1:03 PM CEST, Andy Shevchenko wrote:
> On Tue, May 23, 2023 at 12:26 PM Esteban Blanc <eblanc@baylibre.com> wrote:
> > On Wed May 17, 2023 at 5:04 PM CEST, Andy Shevchenko wrote:
> > > On Wed, May 17, 2023 at 5:43 PM Esteban Blanc <eblanc@baylibre.com> wrote:
...
> > I need to send a v6 now anyway. Should I convert all
> > TPS6594_REG_GPIO1_CONF to TPS6594_REG_GPIOX_CONF(0)?
>
> Again, if you want to leave that definition you need to well justify
> why it's so special that code needs it. Easiest way is to use the
> macro with 0 as an argument.
Ok. Thanks for your input and your patience :)
Best regards,
--
Esteban Blanc
BayLibre
^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2023-05-23 11:33 UTC | newest]
Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-05-12 14:17 [PATCH v4 0/3] TI TPS6594 PMIC support (RTC, pinctrl, regulators) Esteban Blanc
2023-05-12 14:17 ` [PATCH v4 1/3] rtc: tps6594: Add driver for TPS6594 RTC Esteban Blanc
2023-05-12 17:22 ` andy.shevchenko
2023-05-17 16:47 ` Esteban Blanc
2023-05-17 16:52 ` Andy Shevchenko
2023-05-22 11:49 ` Esteban Blanc
2023-05-12 14:17 ` [PATCH v4 2/3] pinctrl: tps6594: Add driver for TPS6594 pinctrl and GPIOs Esteban Blanc
2023-05-12 17:07 ` andy.shevchenko
2023-05-16 13:05 ` Esteban Blanc
2023-05-16 16:48 ` Andy Shevchenko
2023-05-17 9:58 ` Esteban Blanc
2023-05-17 13:51 ` Andy Shevchenko
2023-05-17 14:43 ` Esteban Blanc
2023-05-17 15:04 ` Andy Shevchenko
2023-05-22 8:45 ` Esteban Blanc
2023-05-23 9:26 ` Esteban Blanc
2023-05-23 11:03 ` Andy Shevchenko
2023-05-23 11:32 ` Esteban Blanc
2023-05-12 14:17 ` [PATCH v4 3/3] regulator: tps6594-regulator: Add driver for TI TPS6594 regulators Esteban Blanc
2023-05-12 17:34 ` andy.shevchenko
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).