* [PATCH v3 1/4] i2c: spacemit: configure ILCR for accurate SCL frequency
From: Troy Mitchell @ 2025-11-18 6:08 UTC (permalink / raw)
To: Lee Jones, Yixun Lan, Alex Elder, Andi Shyti, Alexandre Belloni,
Liam Girdwood, Mark Brown
Cc: linux-kernel, linux-riscv, spacemit, linux-i2c, linux-rtc,
Troy Mitchell
In-Reply-To: <20251118-p1-kconfig-fix-v3-0-8839c5ac5db3@linux.spacemit.com>
The SpacemiT I2C controller's SCL (Serial Clock Line) frequency for
master mode operations is determined by the ILCR (I2C Load Count Register).
Previously, the driver relied on the hardware's reset default
values for this register.
The hardware's default ILCR values (SLV=0x156, FLV=0x5d) yield SCL
frequencies lower than intended. For example, with the default
31.5 MHz input clock, these default settings result in an SCL
frequency of approximately 93 kHz (standard mode) when targeting 100 kHz,
and approximately 338 kHz (fast mode) when targeting 400 kHz.
These frequencies are below the 100 kHz/400 kHz nominal speeds.
This patch integrates the SCL frequency management into
the Common Clock Framework (CCF). Specifically, the ILCR register,
which acts as a frequency divider for the SCL clock, is now registered
as a managed clock (scl_clk) within the CCF.
This patch also cleans up unnecessary whitespace
in the included header files.
Reviewed-by: Yixun Lan <dlan@gentoo.org>
Link: https://lore.kernel.org/all/176244506110.1925720.10807118665958896958.b4-ty@kernel.org/ [1]
Signed-off-by: Troy Mitchell <troy.mitchell@linux.spacemit.com>
---
This patch was affected by the P1 Kconfig, which caused the maintainer
to revert it.
The current commit is a direct cherry-pick and reserves the original changelog.
This note is to clarify for anyone who sees the cover letter marked as v2
while the changelog entries reach v4.
---
Changelog in v4:
- initialize clk_init_data with {} so that init.flags is implicitly set to 0
- minor cleanup and style fixes for better readability
- remove unused spacemit_i2c_scl_clk_exclusive_put() cleanup callback
- replace clk_set_rate_exclusive()/clk_rate_exclusive_put() pair with clk_set_rate()
- simplify LCR LV field macros by using FIELD_GET/FIELD_MAX helpers
- Link to v3: https://lore.kernel.org/all/20250814-k1-i2c-ilcr-v3-1-317723e74bcd@linux.spacemit.com/
Changelog in v3:
- use MASK macro in `recalc_rate` function
- rename clock name
- Link to v2: https://lore.kernel.org/r/20250718-k1-i2c-ilcr-v2-1-b4c68f13dcb1@linux.spacemit.com
Changelog in v2:
- Align line breaks.
- Check `lv` in `clk_set_rate` function.
- Force fast mode when SCL frequency is illegal or unavailable.
- Change "linux/bits.h" to <linux/bits.h>
- Kconfig: Add dependency on CCF.
- Link to v1: https://lore.kernel.org/all/20250710-k1-i2c-ilcr-v1-1-188d1f460c7d@linux.spacemit.com/
---
drivers/i2c/busses/Kconfig | 2 +-
drivers/i2c/busses/i2c-k1.c | 159 ++++++++++++++++++++++++++++++++++++++++----
2 files changed, 146 insertions(+), 15 deletions(-)
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index fd81e49638aaa161ae264a722e9e06adc7914cda..fedf5d31f9035b73a27a7f8a764bf5c26975d0e1 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -798,7 +798,7 @@ config I2C_JZ4780
config I2C_K1
tristate "SpacemiT K1 I2C adapter"
depends on ARCH_SPACEMIT || COMPILE_TEST
- depends on OF
+ depends on OF && COMMON_CLK
help
This option enables support for the I2C interface on the SpacemiT K1
platform.
diff --git a/drivers/i2c/busses/i2c-k1.c b/drivers/i2c/busses/i2c-k1.c
index 6b918770e612e098b8ad17418f420d87c94df166..e38a0ba71734ca602854c85672dcb61423453515 100644
--- a/drivers/i2c/busses/i2c-k1.c
+++ b/drivers/i2c/busses/i2c-k1.c
@@ -4,18 +4,21 @@
*/
#include <linux/bitfield.h>
- #include <linux/clk.h>
- #include <linux/i2c.h>
- #include <linux/iopoll.h>
- #include <linux/module.h>
- #include <linux/of_address.h>
- #include <linux/platform_device.h>
+#include <linux/bits.h>
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <linux/i2c.h>
+#include <linux/iopoll.h>
+#include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/platform_device.h>
/* spacemit i2c registers */
#define SPACEMIT_ICR 0x0 /* Control register */
#define SPACEMIT_ISR 0x4 /* Status register */
#define SPACEMIT_IDBR 0xc /* Data buffer register */
#define SPACEMIT_IRCR 0x18 /* Reset cycle counter */
+#define SPACEMIT_ILCR 0x10 /* Load Count Register */
#define SPACEMIT_IBMR 0x1c /* Bus monitor register */
/* SPACEMIT_ICR register fields */
@@ -87,6 +90,13 @@
#define SPACEMIT_BMR_SDA BIT(0) /* SDA line level */
#define SPACEMIT_BMR_SCL BIT(1) /* SCL line level */
+#define SPACEMIT_LCR_LV_STANDARD_SHIFT 0
+#define SPACEMIT_LCR_LV_FAST_SHIFT 9
+#define SPACEMIT_LCR_LV_STANDARD_MASK GENMASK(8, 0)
+#define SPACEMIT_LCR_LV_FAST_MASK GENMASK(17, 9)
+#define SPACEMIT_LCR_LV_STANDARD_MAX_VALUE FIELD_MAX(SPACEMIT_LCR_LV_STANDARD_MASK)
+#define SPACEMIT_LCR_LV_FAST_MAX_VALUE FIELD_MAX(SPACEMIT_LCR_LV_FAST_MASK)
+
/* i2c bus recover timeout: us */
#define SPACEMIT_I2C_BUS_BUSY_TIMEOUT 100000
@@ -104,11 +114,20 @@ enum spacemit_i2c_state {
SPACEMIT_STATE_WRITE,
};
+enum spacemit_i2c_mode {
+ SPACEMIT_MODE_STANDARD,
+ SPACEMIT_MODE_FAST
+};
+
/* i2c-spacemit driver's main struct */
struct spacemit_i2c_dev {
struct device *dev;
struct i2c_adapter adapt;
+ struct clk_hw scl_clk_hw;
+ struct clk *scl_clk;
+ enum spacemit_i2c_mode mode;
+
/* hardware resources */
void __iomem *base;
int irq;
@@ -129,6 +148,79 @@ struct spacemit_i2c_dev {
u32 status;
};
+static void spacemit_i2c_scl_clk_disable_unprepare(void *data)
+{
+ struct spacemit_i2c_dev *i2c = data;
+
+ clk_disable_unprepare(i2c->scl_clk);
+}
+
+static int spacemit_i2c_clk_set_rate(struct clk_hw *hw, unsigned long rate,
+ unsigned long parent_rate)
+{
+ struct spacemit_i2c_dev *i2c = container_of(hw, struct spacemit_i2c_dev, scl_clk_hw);
+ u32 lv, lcr, mask, shift, max_lv;
+
+ lv = DIV_ROUND_UP(parent_rate, rate);
+
+ if (i2c->mode == SPACEMIT_MODE_STANDARD) {
+ mask = SPACEMIT_LCR_LV_STANDARD_MASK;
+ shift = SPACEMIT_LCR_LV_STANDARD_SHIFT;
+ max_lv = SPACEMIT_LCR_LV_STANDARD_MAX_VALUE;
+ } else if (i2c->mode == SPACEMIT_MODE_FAST) {
+ mask = SPACEMIT_LCR_LV_FAST_MASK;
+ shift = SPACEMIT_LCR_LV_FAST_SHIFT;
+ max_lv = SPACEMIT_LCR_LV_FAST_MAX_VALUE;
+ }
+
+ if (!lv || lv > max_lv) {
+ dev_err(i2c->dev, "set scl clock failed: lv 0x%x", lv);
+ return -EINVAL;
+ }
+
+ lcr = readl(i2c->base + SPACEMIT_ILCR);
+ lcr &= ~mask;
+ lcr |= lv << shift;
+ writel(lcr, i2c->base + SPACEMIT_ILCR);
+
+ return 0;
+}
+
+static long spacemit_i2c_clk_round_rate(struct clk_hw *hw, unsigned long rate,
+ unsigned long *parent_rate)
+{
+ u32 lv, freq;
+
+ lv = DIV_ROUND_UP(*parent_rate, rate);
+ freq = DIV_ROUND_UP(*parent_rate, lv);
+
+ return freq;
+}
+
+static unsigned long spacemit_i2c_clk_recalc_rate(struct clk_hw *hw,
+ unsigned long parent_rate)
+{
+ struct spacemit_i2c_dev *i2c = container_of(hw, struct spacemit_i2c_dev, scl_clk_hw);
+ u32 lcr, lv = 0;
+
+ lcr = readl(i2c->base + SPACEMIT_ILCR);
+
+ if (i2c->mode == SPACEMIT_MODE_STANDARD)
+ lv = FIELD_GET(SPACEMIT_LCR_LV_STANDARD_MASK, lcr);
+ else if (i2c->mode == SPACEMIT_MODE_FAST)
+ lv = FIELD_GET(SPACEMIT_LCR_LV_FAST_MASK, lcr);
+ else
+ return 0;
+
+ return DIV_ROUND_UP(parent_rate, lv);
+}
+
+static const struct clk_ops spacemit_i2c_clk_ops = {
+ .set_rate = spacemit_i2c_clk_set_rate,
+ .round_rate = spacemit_i2c_clk_round_rate,
+ .recalc_rate = spacemit_i2c_clk_recalc_rate,
+};
+
static void spacemit_i2c_enable(struct spacemit_i2c_dev *i2c)
{
u32 val;
@@ -147,6 +239,26 @@ static void spacemit_i2c_disable(struct spacemit_i2c_dev *i2c)
writel(val, i2c->base + SPACEMIT_ICR);
}
+static struct clk *spacemit_i2c_register_scl_clk(struct spacemit_i2c_dev *i2c,
+ struct clk *parent)
+{
+ struct clk_init_data init = {};
+ char name[32];
+
+ snprintf(name, sizeof(name), "%s_scl_clk", dev_name(i2c->dev));
+
+ init.name = name;
+ init.ops = &spacemit_i2c_clk_ops;
+ init.parent_data = (struct clk_parent_data[]) {
+ { .fw_name = "func" },
+ };
+ init.num_parents = 1;
+
+ i2c->scl_clk_hw.init = &init;
+
+ return devm_clk_register(i2c->dev, &i2c->scl_clk_hw);
+}
+
static void spacemit_i2c_reset(struct spacemit_i2c_dev *i2c)
{
writel(SPACEMIT_CR_UR, i2c->base + SPACEMIT_ICR);
@@ -246,7 +358,7 @@ static void spacemit_i2c_init(struct spacemit_i2c_dev *i2c)
*/
val |= SPACEMIT_CR_DRFIE;
- if (i2c->clock_freq == SPACEMIT_I2C_MAX_FAST_MODE_FREQ)
+ if (i2c->mode == SPACEMIT_MODE_FAST)
val |= SPACEMIT_CR_MODE_FAST;
/* disable response to general call */
@@ -538,14 +650,15 @@ static int spacemit_i2c_probe(struct platform_device *pdev)
dev_warn(dev, "failed to read clock-frequency property: %d\n", ret);
/* For now, this driver doesn't support high-speed. */
- if (!i2c->clock_freq || i2c->clock_freq > SPACEMIT_I2C_MAX_FAST_MODE_FREQ) {
- dev_warn(dev, "unsupported clock frequency %u; using %u\n",
- i2c->clock_freq, SPACEMIT_I2C_MAX_FAST_MODE_FREQ);
+ if (i2c->clock_freq > SPACEMIT_I2C_MAX_STANDARD_MODE_FREQ &&
+ i2c->clock_freq <= SPACEMIT_I2C_MAX_FAST_MODE_FREQ) {
+ i2c->mode = SPACEMIT_MODE_FAST;
+ } else if (i2c->clock_freq && i2c->clock_freq <= SPACEMIT_I2C_MAX_STANDARD_MODE_FREQ) {
+ i2c->mode = SPACEMIT_MODE_STANDARD;
+ } else {
+ dev_warn(i2c->dev, "invalid clock-frequency, fallback to fast mode");
+ i2c->mode = SPACEMIT_MODE_FAST;
i2c->clock_freq = SPACEMIT_I2C_MAX_FAST_MODE_FREQ;
- } else if (i2c->clock_freq < SPACEMIT_I2C_MAX_STANDARD_MODE_FREQ) {
- dev_warn(dev, "unsupported clock frequency %u; using %u\n",
- i2c->clock_freq, SPACEMIT_I2C_MAX_STANDARD_MODE_FREQ);
- i2c->clock_freq = SPACEMIT_I2C_MAX_STANDARD_MODE_FREQ;
}
i2c->dev = &pdev->dev;
@@ -567,10 +680,28 @@ static int spacemit_i2c_probe(struct platform_device *pdev)
if (IS_ERR(clk))
return dev_err_probe(dev, PTR_ERR(clk), "failed to enable func clock");
+ i2c->scl_clk = spacemit_i2c_register_scl_clk(i2c, clk);
+ if (IS_ERR(i2c->scl_clk))
+ return dev_err_probe(&pdev->dev, PTR_ERR(i2c->scl_clk),
+ "failed to register scl clock\n");
+
clk = devm_clk_get_enabled(dev, "bus");
if (IS_ERR(clk))
return dev_err_probe(dev, PTR_ERR(clk), "failed to enable bus clock");
+ ret = clk_set_rate(i2c->scl_clk, i2c->clock_freq);
+ if (ret)
+ return dev_err_probe(&pdev->dev, ret, "failed to set rate for SCL clock");
+
+ ret = clk_prepare_enable(i2c->scl_clk);
+ if (ret)
+ return dev_err_probe(&pdev->dev, ret, "failed to prepare and enable clock");
+
+ ret = devm_add_action_or_reset(dev, spacemit_i2c_scl_clk_disable_unprepare, i2c);
+ if (ret)
+ return dev_err_probe(&pdev->dev, ret,
+ "failed to register cleanup action for clk disable and unprepare");
+
spacemit_i2c_reset(i2c);
i2c_set_adapdata(&i2c->adapt, i2c);
--
2.51.2
^ permalink raw reply related
* [PATCH v3 0/4] fix the SpacemiT P1 Kconfig and resend the K1 I2C ILCR patch.
From: Troy Mitchell @ 2025-11-18 6:08 UTC (permalink / raw)
To: Lee Jones, Yixun Lan, Alex Elder, Andi Shyti, Alexandre Belloni,
Liam Girdwood, Mark Brown
Cc: linux-kernel, linux-riscv, spacemit, linux-i2c, linux-rtc,
Troy Mitchell
Since P1 Kconfig directly selects K1_I2C, after the I2C ILCR patch was
merged, the driver would fail [1] when COMMON_CLK was not selected.
This series fixes the P1 Kconfig and resends the I2C ILCR patch(This
patch has reverted by maintainer [2]).
Now, P1 Kconfig patch has been merged[3], so I2C ILCR patch can be
merged as well.
In addition, the Kconfig for P1's two subdevices, regulator and RTC,
has been updated to use 'depends on MFD_SPACEMIT_P1' instead of 'select' and
change default value from `ARCH_SPACEMIT` to `MFD_SPACEMIT_P1`.
Link: https://lore.kernel.org/oe-kbuild-all/202510202150.2qXd8e7Y-lkp@intel.com/ [1]
Link: https://lore.kernel.org/all/sdhkjmi5l2m4ua4zqkwkecbihul5bc2dbmitudwfd57y66mdht@6ipjfyz7dtmx/ [2]
Link: https://lore.kernel.org/all/176244506110.1925720.10807118665958896958.b4-ty@kernel.org/ [3]
Signed-off-by: Troy Mitchell <troy.mitchell@linux.spacemit.com>
---
Troy Mitchell (4):
i2c: spacemit: configure ILCR for accurate SCL frequency
rtc: spacemit: MFD_SPACEMIT_P1 as dependencies
regulator: spacemit: MFD_SPACEMIT_P1 as dependencies
mfd: simple-mfd-i2c: add default value
drivers/i2c/busses/Kconfig | 2 +-
drivers/i2c/busses/i2c-k1.c | 159 ++++++++++++++++++++++++++++++++++++++++----
drivers/mfd/Kconfig | 1 +
drivers/regulator/Kconfig | 5 +-
drivers/rtc/Kconfig | 4 +-
5 files changed, 151 insertions(+), 20 deletions(-)
---
base-commit: 3a8660878839faadb4f1a6dd72c3179c1df56787
change-id: 20251021-p1-kconfig-fix-6d2b59d03b8f
Best regards,
--
Troy Mitchell <troy.mitchell@linux.spacemit.com>
^ permalink raw reply
* Re: [PATCH v5 06/11] hwmon: Add Apple Silicon SMC hwmon driver
From: Guenter Roeck @ 2025-11-17 19:00 UTC (permalink / raw)
To: James Calligeros
Cc: Sven Peter, Janne Grunau, Alyssa Rosenzweig, Neal Gompa,
Lee Jones, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Alexandre Belloni, Jean Delvare, Dmitry Torokhov, Jonathan Corbet,
asahi, linux-arm-kernel, devicetree, linux-kernel, linux-rtc,
linux-hwmon, linux-input, linux-doc
In-Reply-To: <20251112-macsmc-subdevs-v5-6-728e4b91fe81@gmail.com>
On Wed, Nov 12, 2025 at 09:16:52PM +1000, James Calligeros wrote:
> The System Management Controller on Apple Silicon devices is responsible
> for integrating and exposing the data reported by the vast array of
> hardware monitoring sensors present on these devices. It is also
> responsible for fan control, and allows users to manually set fan
> speeds if they so desire. Add a hwmon driver to expose current,
> power, temperature, and voltage monitoring sensors, as well as
> fan speed monitoring and control via the SMC on Apple Silicon devices.
>
> The SMC firmware has no consistency between devices, even when they
> share an SoC. The FourCC keys used to access sensors are almost
> random. An M1 Mac mini will have different FourCCs for its CPU core
> temperature sensors to an M1 MacBook Pro, for example. For this
> reason, the valid sensors for a given device are specified in a
> child of the SMC Devicetree node. The driver uses this information
> to determine which sensors to make available at runtime.
>
> Reviewed-by: Neal Gompa <neal@gompa.dev>
> Acked-by: Guenter Roeck <linux@roeck-us.net>
> Co-developed-by: Janne Grunau <j@jannau.net>
> Signed-off-by: Janne Grunau <j@jannau.net>
> Signed-off-by: James Calligeros <jcalligeros99@gmail.com>
Applied to hwmon-next.
Note that I can not apply the devicetree patch (2/11), presumably since it depends
on the first patch of the series.
Guenter
^ permalink raw reply
* Re: [PATCH v4 04/16] dt-bindings: power: supply: BD72720 managed battery
From: Matti Vaittinen @ 2025-11-17 15:48 UTC (permalink / raw)
To: Rob Herring
Cc: Matti Vaittinen, Krzysztof Kozlowski, Mark Brown, Linus Walleij,
linux-kernel, Sebastian Reichel, Bartosz Golaszewski,
Alexandre Belloni, linux-clk, Michael Turquette, Matti Vaittinen,
linux-leds, Pavel Machek, Liam Girdwood, linux-gpio, linux-pm,
Andreas Kemnade, Conor Dooley, devicetree, linux-rtc, Lee Jones,
Stephen Boyd
In-Reply-To: <20251117152341.GA1944698-robh@kernel.org>
On 17/11/2025 17:23, Rob Herring wrote:
> On Mon, Nov 17, 2025 at 10:12:01AM +0200, Matti Vaittinen wrote:
>> On 14/11/2025 18:39, Rob Herring wrote:
>>> On Fri, Nov 14, 2025 at 11:04:27AM +0200, Matti Vaittinen wrote:
>>>> On 13/11/2025 12:53, Rob Herring (Arm) wrote:
>>>>>
>>>>> On Thu, 13 Nov 2025 10:52:19 +0200, Matti Vaittinen wrote:
>>>>>> From: Matti Vaittinen <mazziesaccount@gmail.com>
>>
>> //snip
>>
>>>>
>> For VDR there are only:
>>
>> rohm,voltage-vdr-thresh-microvolt,
>
> So "voltage voltage drop rate"? And '-microvolt' says this is voltage
> too. :)
Hm. Yes. This is a threshold voltage for applying the "zero-correction"
algorithm, which uses these "VDR" (a.k.a voltage drop rate) tables. Eg,
the algorithm should only used for the correction when battery voltage
drops below this threshold. AFAICS, this is usually designed to be
slightly higher than the voltage where the system stays still operable.
I suppose this could also be "zero-correction-threshold", but this would
introduce another "buzzword".
>> rohm,volt-drop-soc-bp,
>> rohm,volt-drop-temperatures-millicelsius
>>
>> and
>>
>> patternProperties:
>> '^rohm,volt-drop-[0-9]-microvolt':
>>
>> So, from the binding point of view (.yaml), it's not _that_ lot. In the .dts
>> there will be quite some noise as the tables have several values.
>>
>>
>>> If that
>>> happens, either we are doing a poor job of generically describing
>>> battery parameters or chargers and batteries are tightly coupled and
>>> can't be described independently.
>>
>> I am under impression that chargers tend to be pretty flexible, and they can
>> be configured to work with many different batteries by altering the charging
>> profiles. Most of the battery properties (like and charging phases [like
>> pre, CC, CV], their limits, currents and voltages etc) are very generally
>> usable. So, large subset of charging functionality can be handled with
>> standard properties. I believe it is only the fuel-gauging where things get
>> more hairy.
>>
>> I did prepare a series which does the split and adds new compatible for the
>> 'rohm,vdr-battery'. (The power-supply class is not yet modified in the
>> series, but we would probably want to modify the battery-info getters to
>> also accept the 'rohm,vdr-battery' -compatible.)
>
> I don't think that's the right direction. It's not a Rohm battery.
>
>> I wonder if I should actually prepare also a series where these properties
>> are just placed in the existing static battery node without adding new
>> compatible. That way it would be easier to see which way is better.
>
> That seems like the right thing to do here.
>
> The main question for me is whether these should even be Rohm specific?
> That would probably require a 2nd user to answer for sure.
>
This is a question Linus W asked as well :)
I believe this technique could be applied to other batteries. I,
however, am not aware of any other than ROHM charger drivers which
implement the algorithm. Furthermore, I was told that the mechanism to
measure these "VDR-tables" for batteries is one of those things which
should be "kept under your hat". I think ROHM has also patented some
stuff related to that. Hence I prefixed these tables by "rohm,".
I have no strong objections to dropping the "rohm," though - but I doubt
these tables will be heavily used by any other but ROHM chargers.
>> If I do that, should I only spin these bindings as RFC to avoid the
>> unnecessary noise?
>
> Only if you think something is not complete and/or the patches should
> not be applied.
Oh, Ok. Then I will send only one of the approaches - probably the one
where properties are added to the simple-battery.
Thanks for all the support!
Yours,
-- Matti
---
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
^ permalink raw reply
* Re: [PATCH v4 04/16] dt-bindings: power: supply: BD72720 managed battery
From: Rob Herring @ 2025-11-17 15:23 UTC (permalink / raw)
To: Matti Vaittinen
Cc: Matti Vaittinen, Krzysztof Kozlowski, Mark Brown, Linus Walleij,
linux-kernel, Sebastian Reichel, Bartosz Golaszewski,
Alexandre Belloni, linux-clk, Michael Turquette, Matti Vaittinen,
linux-leds, Pavel Machek, Liam Girdwood, linux-gpio, linux-pm,
Andreas Kemnade, Conor Dooley, devicetree, linux-rtc, Lee Jones,
Stephen Boyd
In-Reply-To: <32303b95-3fd5-44c4-bb7d-e2957a6064fc@gmail.com>
On Mon, Nov 17, 2025 at 10:12:01AM +0200, Matti Vaittinen wrote:
> On 14/11/2025 18:39, Rob Herring wrote:
> > On Fri, Nov 14, 2025 at 11:04:27AM +0200, Matti Vaittinen wrote:
> > > On 13/11/2025 12:53, Rob Herring (Arm) wrote:
> > > >
> > > > On Thu, 13 Nov 2025 10:52:19 +0200, Matti Vaittinen wrote:
> > > > > From: Matti Vaittinen <mazziesaccount@gmail.com>
>
> //snip
>
> > >
> > > So, as far as I understand, the only viable options are expanding the
> > > existing battery.yaml with these properties (which I hoped to avoid, see
> > > below)
> > >
> > > > > The right place for them is the battery node, which is described by the
> > > > > generic "battery.yaml". I was not comfortable with adding these
> > > > > properties to the generic battery.yaml because they are:
> > > > > - Meaningful only for those charger drivers which have the VDR
> > > > > algorithm implemented. (And even though the algorithm is not charger
> > > > > specific, AFAICS, it is currently only used by some ROHM PMIC
> > > > > drivers).
> > > > > - Technique of measuring the VDR tables for a battery is not widely
> > > > > known. AFAICS, only folks at ROHM are measuring those for some
> > > > > customer products. We do have those tables available for some of the
> > > > > products though (Kobo?).
> > >
> > > or, to add new compatible for the "vdr-battery".
> > > AFAICS, adding new compatible would require us to wither duplicate the used
> > > properties from battery.yaml here (as battery.yaml mandates the
> > > "simple-battery" - compatible) - or to split the battery.yaml in two files,
> > > one containing the generic properties, other containing the "simple-battery"
> > > -compatible and referencing the generic one. Then the "vdr-battery" could
> > > also reference the generic one.
> > >
> > > Any suggestions for the next path to follow?
> >
> > Probably the latter option. You could do the former and make the new
> > properties conditional on the "vdr-battery" compatible. That's fine with
> > small differences, but gets messy as there are more properties and
> > variations.
> >
> > But is "VDR" a type of battery though? Is there a certain type/chemistry
> > of battery we should be describing where VDR is applicable?
>
> No. Not that I know. My understanding is that the "VDR (voltage drop rate)"
> refers to measured voltage drop-rates under certain conditions - which can
> be used to (more accurately) estimate the remaining capacity when battery is
> nearly depleted. As far as I know, this is only used with Lithium-ion
> batteries (I am not at all sure of this) - but I _assume_ the technique
> could be applied to other type of batteries as well.
>
> > I don't
> > think it scales well if we define battery compatibles for every
> > variation of charger algorithm. Honestly I don't mind just adding 1
> > property. I care more if we allow undocumented properties than
> > allowing documented but invalid for the platform properties.
>
> I see. The "VDR" stuff is really tightly bound to the fuel-gauging
> algorithm. It is measured characteristics of the battery - but those values
> are only usable by the "VDR" algorithm. I don't really have a good insight
> in the amount of fuel-gauging algorithm related properties suggested to be
> added during the years - but don't think there have been that many of them.
> So, I am not that worried about adding the compatible. On the other hand,
> there is no technical reason (other than adding properties which are unused
> on many platforms) why not to add the vdr tables in the static-battey node
> without adding own compatible. And, reading reply from Andreas (I'll copy it
> here to answer it in same mail)
>
> /// Below text is form Andreas:
> > just keep in mind, that several kobo devices have one pmic in one board
> > revision and another one in the other (e.g. Kobo Nia rev A vs rev C).
> > But probably the same battery. So if the "vdr-battery" is a compatible
> > just to allow a more properties,
> > then "simple-battery" should be allowed as fallback.
>
> I didn't know Kobos use multiple chargers. Thanks Andreas! So, in that
> sense, adding the "vdr" tables in static-battery node, without new
> compatible, would maybe be simplest solution. Then the charger(s)
> (fuel-gauge(s)) which implement VDR algorithm, can pick the tables while
> those chargers which don't implement the VDR will just ignore these tables.
>
> > When it
> > becomes 10, 20, 30 properties, then I might start to care.
>
> For VDR there are only:
>
> rohm,voltage-vdr-thresh-microvolt,
So "voltage voltage drop rate"? And '-microvolt' says this is voltage
too. :)
> rohm,volt-drop-soc-bp,
> rohm,volt-drop-temperatures-millicelsius
>
> and
>
> patternProperties:
> '^rohm,volt-drop-[0-9]-microvolt':
>
> So, from the binding point of view (.yaml), it's not _that_ lot. In the .dts
> there will be quite some noise as the tables have several values.
>
>
> > If that
> > happens, either we are doing a poor job of generically describing
> > battery parameters or chargers and batteries are tightly coupled and
> > can't be described independently.
>
> I am under impression that chargers tend to be pretty flexible, and they can
> be configured to work with many different batteries by altering the charging
> profiles. Most of the battery properties (like and charging phases [like
> pre, CC, CV], their limits, currents and voltages etc) are very generally
> usable. So, large subset of charging functionality can be handled with
> standard properties. I believe it is only the fuel-gauging where things get
> more hairy.
>
> I did prepare a series which does the split and adds new compatible for the
> 'rohm,vdr-battery'. (The power-supply class is not yet modified in the
> series, but we would probably want to modify the battery-info getters to
> also accept the 'rohm,vdr-battery' -compatible.)
I don't think that's the right direction. It's not a Rohm battery.
> I wonder if I should actually prepare also a series where these properties
> are just placed in the existing static battery node without adding new
> compatible. That way it would be easier to see which way is better.
That seems like the right thing to do here.
The main question for me is whether these should even be Rohm specific?
That would probably require a 2nd user to answer for sure.
> If I do that, should I only spin these bindings as RFC to avoid the
> unnecessary noise?
Only if you think something is not complete and/or the patches should
not be applied.
Rob
^ permalink raw reply
* Re: [PATCH v4 04/16] dt-bindings: power: supply: BD72720 managed battery
From: Matti Vaittinen @ 2025-11-17 8:12 UTC (permalink / raw)
To: Rob Herring
Cc: Matti Vaittinen, Krzysztof Kozlowski, Mark Brown, Linus Walleij,
linux-kernel, Sebastian Reichel, Bartosz Golaszewski,
Alexandre Belloni, linux-clk, Michael Turquette, Matti Vaittinen,
linux-leds, Pavel Machek, Liam Girdwood, linux-gpio, linux-pm,
Andreas Kemnade, Conor Dooley, devicetree, linux-rtc, Lee Jones,
Stephen Boyd
In-Reply-To: <20251114163954.GA3399895-robh@kernel.org>
On 14/11/2025 18:39, Rob Herring wrote:
> On Fri, Nov 14, 2025 at 11:04:27AM +0200, Matti Vaittinen wrote:
>> On 13/11/2025 12:53, Rob Herring (Arm) wrote:
>>>
>>> On Thu, 13 Nov 2025 10:52:19 +0200, Matti Vaittinen wrote:
>>>> From: Matti Vaittinen <mazziesaccount@gmail.com>
//snip
>>
>> So, as far as I understand, the only viable options are expanding the
>> existing battery.yaml with these properties (which I hoped to avoid, see
>> below)
>>
>>>> The right place for them is the battery node, which is described by the
>>>> generic "battery.yaml". I was not comfortable with adding these
>>>> properties to the generic battery.yaml because they are:
>>>> - Meaningful only for those charger drivers which have the VDR
>>>> algorithm implemented. (And even though the algorithm is not charger
>>>> specific, AFAICS, it is currently only used by some ROHM PMIC
>>>> drivers).
>>>> - Technique of measuring the VDR tables for a battery is not widely
>>>> known. AFAICS, only folks at ROHM are measuring those for some
>>>> customer products. We do have those tables available for some of the
>>>> products though (Kobo?).
>>
>> or, to add new compatible for the "vdr-battery".
>> AFAICS, adding new compatible would require us to wither duplicate the used
>> properties from battery.yaml here (as battery.yaml mandates the
>> "simple-battery" - compatible) - or to split the battery.yaml in two files,
>> one containing the generic properties, other containing the "simple-battery"
>> -compatible and referencing the generic one. Then the "vdr-battery" could
>> also reference the generic one.
>>
>> Any suggestions for the next path to follow?
>
> Probably the latter option. You could do the former and make the new
> properties conditional on the "vdr-battery" compatible. That's fine with
> small differences, but gets messy as there are more properties and
> variations.
>
> But is "VDR" a type of battery though? Is there a certain type/chemistry
> of battery we should be describing where VDR is applicable?
No. Not that I know. My understanding is that the "VDR (voltage drop
rate)" refers to measured voltage drop-rates under certain conditions -
which can be used to (more accurately) estimate the remaining capacity
when battery is nearly depleted. As far as I know, this is only used
with Lithium-ion batteries (I am not at all sure of this) - but I
_assume_ the technique could be applied to other type of batteries as well.
> I don't
> think it scales well if we define battery compatibles for every
> variation of charger algorithm. Honestly I don't mind just adding 1
> property. I care more if we allow undocumented properties than
> allowing documented but invalid for the platform properties.
I see. The "VDR" stuff is really tightly bound to the fuel-gauging
algorithm. It is measured characteristics of the battery - but those
values are only usable by the "VDR" algorithm. I don't really have a
good insight in the amount of fuel-gauging algorithm related properties
suggested to be added during the years - but don't think there have been
that many of them. So, I am not that worried about adding the
compatible. On the other hand, there is no technical reason (other than
adding properties which are unused on many platforms) why not to add the
vdr tables in the static-battey node without adding own compatible. And,
reading reply from Andreas (I'll copy it here to answer it in same mail)
/// Below text is form Andreas:
> just keep in mind, that several kobo devices have one pmic in one board
> revision and another one in the other (e.g. Kobo Nia rev A vs rev C).
> But probably the same battery. So if the "vdr-battery" is a compatible
> just to allow a more properties,
> then "simple-battery" should be allowed as fallback.
I didn't know Kobos use multiple chargers. Thanks Andreas! So, in that
sense, adding the "vdr" tables in static-battery node, without new
compatible, would maybe be simplest solution. Then the charger(s)
(fuel-gauge(s)) which implement VDR algorithm, can pick the tables while
those chargers which don't implement the VDR will just ignore these tables.
> When it
> becomes 10, 20, 30 properties, then I might start to care.
For VDR there are only:
rohm,voltage-vdr-thresh-microvolt,
rohm,volt-drop-soc-bp,
rohm,volt-drop-temperatures-millicelsius
and
patternProperties:
'^rohm,volt-drop-[0-9]-microvolt':
So, from the binding point of view (.yaml), it's not _that_ lot. In the
.dts there will be quite some noise as the tables have several values.
> If that
> happens, either we are doing a poor job of generically describing
> battery parameters or chargers and batteries are tightly coupled and
> can't be described independently.
I am under impression that chargers tend to be pretty flexible, and they
can be configured to work with many different batteries by altering the
charging profiles. Most of the battery properties (like and charging
phases [like pre, CC, CV], their limits, currents and voltages etc) are
very generally usable. So, large subset of charging functionality can be
handled with standard properties. I believe it is only the fuel-gauging
where things get more hairy.
I did prepare a series which does the split and adds new compatible for
the 'rohm,vdr-battery'. (The power-supply class is not yet modified in
the series, but we would probably want to modify the battery-info
getters to also accept the 'rohm,vdr-battery' -compatible.)
I wonder if I should actually prepare also a series where these
properties are just placed in the existing static battery node without
adding new compatible. That way it would be easier to see which way is
better.
If I do that, should I only spin these bindings as RFC to avoid the
unnecessary noise?
Oh, and a big thanks to both of you Rob and Andreas! I feel this gained
more clarity after your feedback :)
Yours,
-- Matti
---
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
^ permalink raw reply
* Re: [PATCH] rtc: isl12026: Implement callbacks for alarm feature
From: Alexandre Belloni @ 2025-11-16 22:08 UTC (permalink / raw)
To: Akhilesh Patil
Cc: andriy.shevchenko, david.daney, ddaney, david.hunter.linux, skhan,
linux-rtc, linux-kernel, akhileshpatilvnit
In-Reply-To: <20251116-51715-3266073@bhairav-test.ee.iitb.ac.in>
On 16/11/2025 10:47:15+0530, Akhilesh Patil wrote:
> > > static const struct rtc_class_ops isl12026_rtc_ops = {
> > > .read_time = isl12026_rtc_read_time,
> > > .set_time = isl12026_rtc_set_time,
> > > + .set_alarm = isl12026_rtc_set_alarm,
> > > + .read_alarm = isl12026_rtc_read_alarm,
> > > + .alarm_irq_enable = isl12026_rtc_alarm_irq_en,
> > > };
> > >
> >
> > This is missing an interrupt handler and proper handling in probe for
> > the wakeup-source property as this seems to be how you use it.
>
> Agree. However, I thought of first implementing alarm callbacks only and
> test them independedntly using ioctls for alarm settings in this patch.
> I will add interrupt handler and wakeup-source in v2 to complete this
> functionality.
Sure, the issue is that without interrupt handling and wakeup-source,
the driver will behave in a way that will break DT backward
compatibility later on.
--
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply
* Re: [PATCH] rtc: isl12026: Implement callbacks for alarm feature
From: Akhilesh Patil @ 2025-11-16 5:17 UTC (permalink / raw)
To: Alexandre Belloni
Cc: andriy.shevchenko, david.daney, ddaney, david.hunter.linux, skhan,
linux-rtc, linux-kernel, akhileshpatilvnit
In-Reply-To: <20251115112421bdd368f5@mail.local>
On Sat, Nov 15, 2025 at 12:24:21PM +0100, Alexandre Belloni wrote:
> On 15/11/2025 16:35:06+0530, Akhilesh Patil wrote:
> > Add alarm support for isl12026 RTC. Implement alarm function rtc
> > class callbacks - set_alarm, read_alarm and alarm_irq_enable.
> > isl12026 rtc has 2 alarms, this patch adds support to configure alarm0.
> > Note: isl12026 rtc chip share same pin(4) for alarm interrupt and square
> > wave frequency generator, hence forcefully disable SQW functionality
> > while writing to device registers in alarm functions.
> >
> > Tested on TI am62x sk board on i2c-2 port using selftests/rtc/rtctest
> >
> > Signed-off-by: Akhilesh Patil <akhilesh@ee.iitb.ac.in>
> > ---
> > Datasheet of RTC chip.
> > https://www.renesas.com/en/document/dst/isl12026-isl12026a-datasheet?srsltid=AfmBOopgN4vtn8XoN-8sOCfTW6yiLH-T7eeH_IWakqZ2VmENmWFqqh7w
> >
> > drivers/rtc/rtc-isl12026.c | 127 +++++++++++++++++++++++++++++++++++++
> > 1 file changed, 127 insertions(+)
> >
> > diff --git a/drivers/rtc/rtc-isl12026.c b/drivers/rtc/rtc-isl12026.c
> > index 2aabb9151d4c..7fa9ec7e4929 100644
> > --- a/drivers/rtc/rtc-isl12026.c
> > +++ b/drivers/rtc/rtc-isl12026.c
> > @@ -34,6 +34,11 @@
> > #define ISL12026_PAGESIZE 16
> > #define ISL12026_NVMEM_WRITE_TIME 20
Hi Alexandre, Thanks for the review.
Please find my comments below.
[...]
> >
> > +static int isl12026_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
> > +{
> > + struct i2c_client *client = to_i2c_client(dev);
> > + int ret;
> > + u8 buf_alrm_vals[7];
> > + struct i2c_msg msg;
> > + int ir;
> > +
> > + msg.addr = client->addr;
> > + msg.flags = 0x0; /* Write operation */
> > + msg.buf = buf_alrm_vals;
> > + msg.len = sizeof(buf_alrm_vals);
> > +
> > + if (!alrm->enabled) {
> > + /* Disable alarm and return */
>
> You still need to set the alarm time and date properly, the application
> is allowed to first set the alarm and then enable it.
Sure. Understood.
Will fix this in v2.
>
> > + ir = isl12026_read_reg(client, ISL12026_REG_INT);
> > + if (ir < 0)
> > + return ir;
> > + ir &= ~ISL12026_AL0E;
[...]
> > +
> > + /* Non-volatile Page write to AL0 registers and enable INT */
> > + ret = isl12026_arm_write(client);
> > + if (ret < 0)
> > + return ret;
> > + ret = i2c_transfer(client->adapter, &msg, 1);
> > + msleep(ISL12026_NVMEM_WRITE_TIME);
> > + if (ret != 1) {
> > + dev_err(&client->dev, "Error writing to alarm registers\n");
>
> No need for an error message.
ACK.
Will remove error message and just return error.
>
> > + return ret < 0 ? ret : -EIO;
> > + }
> > +
> > + /* Enable AL0 interrupt */
> > + ret = isl12026_write_reg(client, ISL12026_REG_INT, ISL12026_AL0E);
> > +
> > + return ret;
> > +}
> > +
> > +static int isl12026_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
> > +{
> > + struct i2c_client *client = to_i2c_client(dev);
> > + int ret;
> > + int sr, ir;
> > + u8 buf_alrm_vals[5];
> > + u8 addr[2] = {0x0, ISL12026_AL0_REG_SC};
> > + struct i2c_msg msgs[2] = { };
> > +
[...]
> > + alrm->pending = !!(sr & ISL12026_SR_AL0) && alrm->enabled;
> > +
> > + /* Page read for alarm registers */
> > + ret = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
> > + if (ret != ARRAY_SIZE(msgs)) {
> > + dev_err(&client->dev, "Error reading alarm registers\n");
>
> Ditto
ACK. Will remove err message.
>
> > + return ret < 0 ? ret : -EIO;
> > + }
> > +
> > + /* Populate values read */
> > + alrm->time.tm_sec = bcd2bin(buf_alrm_vals[0] & 0x7f);
> > + alrm->time.tm_min = bcd2bin(buf_alrm_vals[1] & 0x7f);
> > + alrm->time.tm_hour = bcd2bin(buf_alrm_vals[2] & 0x3f);
> > + alrm->time.tm_mday = bcd2bin(buf_alrm_vals[3] & 0x3f);
> > + alrm->time.tm_mon = bcd2bin(buf_alrm_vals[4] & 0x1f) - 1;
> > +
> > + return 0;
> > +}
> > +
> > +static int isl12026_rtc_alarm_irq_en(struct device *dev, unsigned int enabled)
> > +{
> > + struct i2c_client *client = to_i2c_client(dev);
> > + int ret;
> > + int ir;
> > +
> > + if (enabled) {
> > + ret = isl12026_write_reg(client, ISL12026_REG_INT, ISL12026_AL0E);
> > + return ret;
> > + }
> > +
> > + /* Disable alarm */
> > + ir = isl12026_read_reg(client, ISL12026_REG_INT);
> > + if (ir < 0)
> > + return ir;
> > + ir &= ~ISL12026_AL0E;
> > + ret = isl12026_write_reg(client, ISL12026_REG_INT, ir);
> > +
> > + return ret;
> > +}
> > +
> > static const struct rtc_class_ops isl12026_rtc_ops = {
> > .read_time = isl12026_rtc_read_time,
> > .set_time = isl12026_rtc_set_time,
> > + .set_alarm = isl12026_rtc_set_alarm,
> > + .read_alarm = isl12026_rtc_read_alarm,
> > + .alarm_irq_enable = isl12026_rtc_alarm_irq_en,
> > };
> >
>
> This is missing an interrupt handler and proper handling in probe for
> the wakeup-source property as this seems to be how you use it.
Agree. However, I thought of first implementing alarm callbacks only and
test them independedntly using ioctls for alarm settings in this patch.
I will add interrupt handler and wakeup-source in v2 to complete this
functionality.
Regards,
Akhilesh
>
>
> --
> Alexandre Belloni, co-owner and COO, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
^ permalink raw reply
* Re: [PATCH 06/13] mfd: sec-irq: add support for creating multiple IRQ chips
From: Kaustabh Chakraborty @ 2025-11-15 15:45 UTC (permalink / raw)
To: André Draszik
Cc: Alexandre Belloni, Lee Jones, Pavel Machek, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, MyungJoo Ham, Chanwoo Choi,
Sebastian Reichel, Krzysztof Kozlowski, Jonathan Corbet,
linux-leds, devicetree, linux-kernel, linux-pm, linux-samsung-soc,
linux-rtc, linux-doc
In-Reply-To: <6479a8d84052b326ffeb5609959aaf3a1aac9ff8.camel@linaro.org>
On 2025-11-14 11:55, André Draszik wrote:
> On Fri, 2025-11-14 at 08:50 +0100, Alexandre Belloni wrote:
>> On 14/11/2025 00:35:07+0530, Kaustabh Chakraborty wrote:
>> > The current state of the driver only allows creating only one IRQ chip
>> > per PMIC. On some PMICs, such as Samsung's S2MU005, there are multiple
>> > interrupt blocks, for which the current implementation stands insufficient.
>> >
>> > Add support for creating multiple IRQ chips for a PMIC. Every IRQ chip is
>> > given it's own index, which is used by sub-device drivers to request IRQs.
>> >
>> > A macro is defined which states the maximum number of chips supported.
>> > It's set to 1 as currently, no PMIC requires more than one IRQ chip. The
>> > value must be changed accordingly on adding new PMICs requiring multiple
>> > IRQ chips.
>> >
>> > Moreover, adjust the s5m RTC driver to initialize IRQs with the
>> > appropriate IRQ chip index.
>> >
>> > Signed-off-by: Kaustabh Chakraborty <kauschluss@disroot.org>
>> Acked-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
>>
>> > ---
>> > drivers/mfd/sec-irq.c | 163 +++++++++++++++++++++++----------------
>> > drivers/rtc/rtc-s5m.c | 15 +++-
>> > include/linux/mfd/samsung/core.h | 5 +-
>> > include/linux/mfd/samsung/irq.h | 14 ++++
>> > 4 files changed, 127 insertions(+), 70 deletions(-)
>
> Your patch reminded me to finally send
> https://lore.kernel.org/all/20251114-s5m-alarm-v1-0-c9b3bebae65f@linaro.org/
>
> If applied first, you wouldn't need to touch rtc-s5m.c I believe.
Oo, this cleans things up greatly!
>
> Equally, I can rebase mine on top of yours - no strong feelings.
I will wait for your series to be applied. Your series is much shorter,
so wouldn't hold that back for this. :)
>
> Cheers,
> Andre'
^ permalink raw reply
* Re: [PATCH v4 05/16] dt-bindings: mfd: ROHM BD72720
From: Krzysztof Kozlowski @ 2025-11-15 11:31 UTC (permalink / raw)
To: Matti Vaittinen
Cc: Matti Vaittinen, Lee Jones, Pavel Machek, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Sebastian Reichel,
Liam Girdwood, Mark Brown, Michael Turquette, Stephen Boyd,
Linus Walleij, Bartosz Golaszewski, Alexandre Belloni, linux-leds,
devicetree, linux-kernel, linux-pm, linux-clk, linux-gpio,
linux-rtc, Andreas Kemnade
In-Reply-To: <ec2cb44d9d00f5edaed2fbe17fd9ddbed914ff37.1763022807.git.mazziesaccount@gmail.com>
On Thu, Nov 13, 2025 at 10:52:35AM +0200, Matti Vaittinen wrote:
> From: Matti Vaittinen <mazziesaccount@gmail.com>
>
> The ROHM BD72720 is a power management IC integrating regulators, GPIOs,
> charger, LEDs, RTC and a clock gate.
>
> Add dt-binding doc for ROHM BD72720.
>
> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
>
> ---
> Revision history:
Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH] rtc: isl12026: Implement callbacks for alarm feature
From: Alexandre Belloni @ 2025-11-15 11:24 UTC (permalink / raw)
To: Akhilesh Patil
Cc: andriy.shevchenko, david.daney, ddaney, david.hunter.linux, skhan,
linux-rtc, linux-kernel, akhileshpatilvnit
In-Reply-To: <20251115-1156-3147571@bhairav-test.ee.iitb.ac.in>
On 15/11/2025 16:35:06+0530, Akhilesh Patil wrote:
> Add alarm support for isl12026 RTC. Implement alarm function rtc
> class callbacks - set_alarm, read_alarm and alarm_irq_enable.
> isl12026 rtc has 2 alarms, this patch adds support to configure alarm0.
> Note: isl12026 rtc chip share same pin(4) for alarm interrupt and square
> wave frequency generator, hence forcefully disable SQW functionality
> while writing to device registers in alarm functions.
>
> Tested on TI am62x sk board on i2c-2 port using selftests/rtc/rtctest
>
> Signed-off-by: Akhilesh Patil <akhilesh@ee.iitb.ac.in>
> ---
> Datasheet of RTC chip.
> https://www.renesas.com/en/document/dst/isl12026-isl12026a-datasheet?srsltid=AfmBOopgN4vtn8XoN-8sOCfTW6yiLH-T7eeH_IWakqZ2VmENmWFqqh7w
>
> drivers/rtc/rtc-isl12026.c | 127 +++++++++++++++++++++++++++++++++++++
> 1 file changed, 127 insertions(+)
>
> diff --git a/drivers/rtc/rtc-isl12026.c b/drivers/rtc/rtc-isl12026.c
> index 2aabb9151d4c..7fa9ec7e4929 100644
> --- a/drivers/rtc/rtc-isl12026.c
> +++ b/drivers/rtc/rtc-isl12026.c
> @@ -34,6 +34,11 @@
> #define ISL12026_PAGESIZE 16
> #define ISL12026_NVMEM_WRITE_TIME 20
>
> +#define ISL12026_AL0_REG_SC 0x0
> +#define ISL12026_REG_INT 0x11
> +#define ISL12026_AL0E BIT(5)
> +#define ISL12026_SR_AL0 BIT(5)
> +
> struct isl12026 {
> struct rtc_device *rtc;
> struct i2c_client *nvm_client;
> @@ -269,9 +274,131 @@ static int isl12026_rtc_read_time(struct device *dev, struct rtc_time *tm)
> return ret;
> }
>
> +static int isl12026_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
> +{
> + struct i2c_client *client = to_i2c_client(dev);
> + int ret;
> + u8 buf_alrm_vals[7];
> + struct i2c_msg msg;
> + int ir;
> +
> + msg.addr = client->addr;
> + msg.flags = 0x0; /* Write operation */
> + msg.buf = buf_alrm_vals;
> + msg.len = sizeof(buf_alrm_vals);
> +
> + if (!alrm->enabled) {
> + /* Disable alarm and return */
You still need to set the alarm time and date properly, the application
is allowed to first set the alarm and then enable it.
> + ir = isl12026_read_reg(client, ISL12026_REG_INT);
> + if (ir < 0)
> + return ir;
> + ir &= ~ISL12026_AL0E;
> + ret = isl12026_write_reg(client, ISL12026_REG_INT, ir);
> +
> + return ret;
> + }
> +
> + /* Prepare 5 bytes alarm data SC, MN, HR, DT, MO */
> + buf_alrm_vals[0] = 0x0;
> + buf_alrm_vals[1] = ISL12026_AL0_REG_SC;
> + buf_alrm_vals[2] = (bin2bcd(alrm->time.tm_sec) & 0x7f) | 0x80;
> + buf_alrm_vals[3] = (bin2bcd(alrm->time.tm_min) & 0x7f) | 0x80;
> + buf_alrm_vals[4] = (bin2bcd(alrm->time.tm_hour) & 0x3f) | 0x80;
> + buf_alrm_vals[5] = (bin2bcd(alrm->time.tm_mday) & 0x3f) | 0x80;
> + buf_alrm_vals[6] = (bin2bcd(alrm->time.tm_mon + 1) & 0x1f) | 0x80;
> +
> + /* Non-volatile Page write to AL0 registers and enable INT */
> + ret = isl12026_arm_write(client);
> + if (ret < 0)
> + return ret;
> + ret = i2c_transfer(client->adapter, &msg, 1);
> + msleep(ISL12026_NVMEM_WRITE_TIME);
> + if (ret != 1) {
> + dev_err(&client->dev, "Error writing to alarm registers\n");
No need for an error message.
> + return ret < 0 ? ret : -EIO;
> + }
> +
> + /* Enable AL0 interrupt */
> + ret = isl12026_write_reg(client, ISL12026_REG_INT, ISL12026_AL0E);
> +
> + return ret;
> +}
> +
> +static int isl12026_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
> +{
> + struct i2c_client *client = to_i2c_client(dev);
> + int ret;
> + int sr, ir;
> + u8 buf_alrm_vals[5];
> + u8 addr[2] = {0x0, ISL12026_AL0_REG_SC};
> + struct i2c_msg msgs[2] = { };
> +
> + msgs[0].addr = client->addr;
> + msgs[0].flags = 0x0; /* Write register address */
> + msgs[0].buf = addr;
> + msgs[0].len = sizeof(addr);
> +
> + msgs[1].addr = client->addr;
> + msgs[1].flags = I2C_M_RD; /* Alarm read operation */
> + msgs[1].buf = buf_alrm_vals;
> + msgs[1].len = sizeof(buf_alrm_vals);
> +
> + /* Read alarm enable status */
> + ir = isl12026_read_reg(client, ISL12026_REG_INT);
> + if (ir < 0)
> + return ir;
> + alrm->enabled = !!(ir & ISL12026_AL0E);
> +
> + /* Read alarm pending status */
> + sr = isl12026_read_reg(client, ISL12026_REG_SR);
> + if (sr < 0)
> + return sr;
> + alrm->pending = !!(sr & ISL12026_SR_AL0) && alrm->enabled;
> +
> + /* Page read for alarm registers */
> + ret = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
> + if (ret != ARRAY_SIZE(msgs)) {
> + dev_err(&client->dev, "Error reading alarm registers\n");
Ditto
> + return ret < 0 ? ret : -EIO;
> + }
> +
> + /* Populate values read */
> + alrm->time.tm_sec = bcd2bin(buf_alrm_vals[0] & 0x7f);
> + alrm->time.tm_min = bcd2bin(buf_alrm_vals[1] & 0x7f);
> + alrm->time.tm_hour = bcd2bin(buf_alrm_vals[2] & 0x3f);
> + alrm->time.tm_mday = bcd2bin(buf_alrm_vals[3] & 0x3f);
> + alrm->time.tm_mon = bcd2bin(buf_alrm_vals[4] & 0x1f) - 1;
> +
> + return 0;
> +}
> +
> +static int isl12026_rtc_alarm_irq_en(struct device *dev, unsigned int enabled)
> +{
> + struct i2c_client *client = to_i2c_client(dev);
> + int ret;
> + int ir;
> +
> + if (enabled) {
> + ret = isl12026_write_reg(client, ISL12026_REG_INT, ISL12026_AL0E);
> + return ret;
> + }
> +
> + /* Disable alarm */
> + ir = isl12026_read_reg(client, ISL12026_REG_INT);
> + if (ir < 0)
> + return ir;
> + ir &= ~ISL12026_AL0E;
> + ret = isl12026_write_reg(client, ISL12026_REG_INT, ir);
> +
> + return ret;
> +}
> +
> static const struct rtc_class_ops isl12026_rtc_ops = {
> .read_time = isl12026_rtc_read_time,
> .set_time = isl12026_rtc_set_time,
> + .set_alarm = isl12026_rtc_set_alarm,
> + .read_alarm = isl12026_rtc_read_alarm,
> + .alarm_irq_enable = isl12026_rtc_alarm_irq_en,
> };
>
This is missing an interrupt handler and proper handling in probe for
the wakeup-source property as this seems to be how you use it.
--
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply
* [PATCH] rtc: isl12026: Implement callbacks for alarm feature
From: Akhilesh Patil @ 2025-11-15 11:05 UTC (permalink / raw)
To: alexandre.belloni, andriy.shevchenko, david.daney, ddaney,
david.hunter.linux, skhan
Cc: linux-rtc, linux-kernel, akhileshpatilvnit
Add alarm support for isl12026 RTC. Implement alarm function rtc
class callbacks - set_alarm, read_alarm and alarm_irq_enable.
isl12026 rtc has 2 alarms, this patch adds support to configure alarm0.
Note: isl12026 rtc chip share same pin(4) for alarm interrupt and square
wave frequency generator, hence forcefully disable SQW functionality
while writing to device registers in alarm functions.
Tested on TI am62x sk board on i2c-2 port using selftests/rtc/rtctest
Signed-off-by: Akhilesh Patil <akhilesh@ee.iitb.ac.in>
---
Datasheet of RTC chip.
https://www.renesas.com/en/document/dst/isl12026-isl12026a-datasheet?srsltid=AfmBOopgN4vtn8XoN-8sOCfTW6yiLH-T7eeH_IWakqZ2VmENmWFqqh7w
drivers/rtc/rtc-isl12026.c | 127 +++++++++++++++++++++++++++++++++++++
1 file changed, 127 insertions(+)
diff --git a/drivers/rtc/rtc-isl12026.c b/drivers/rtc/rtc-isl12026.c
index 2aabb9151d4c..7fa9ec7e4929 100644
--- a/drivers/rtc/rtc-isl12026.c
+++ b/drivers/rtc/rtc-isl12026.c
@@ -34,6 +34,11 @@
#define ISL12026_PAGESIZE 16
#define ISL12026_NVMEM_WRITE_TIME 20
+#define ISL12026_AL0_REG_SC 0x0
+#define ISL12026_REG_INT 0x11
+#define ISL12026_AL0E BIT(5)
+#define ISL12026_SR_AL0 BIT(5)
+
struct isl12026 {
struct rtc_device *rtc;
struct i2c_client *nvm_client;
@@ -269,9 +274,131 @@ static int isl12026_rtc_read_time(struct device *dev, struct rtc_time *tm)
return ret;
}
+static int isl12026_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
+{
+ struct i2c_client *client = to_i2c_client(dev);
+ int ret;
+ u8 buf_alrm_vals[7];
+ struct i2c_msg msg;
+ int ir;
+
+ msg.addr = client->addr;
+ msg.flags = 0x0; /* Write operation */
+ msg.buf = buf_alrm_vals;
+ msg.len = sizeof(buf_alrm_vals);
+
+ if (!alrm->enabled) {
+ /* Disable alarm and return */
+ ir = isl12026_read_reg(client, ISL12026_REG_INT);
+ if (ir < 0)
+ return ir;
+ ir &= ~ISL12026_AL0E;
+ ret = isl12026_write_reg(client, ISL12026_REG_INT, ir);
+
+ return ret;
+ }
+
+ /* Prepare 5 bytes alarm data SC, MN, HR, DT, MO */
+ buf_alrm_vals[0] = 0x0;
+ buf_alrm_vals[1] = ISL12026_AL0_REG_SC;
+ buf_alrm_vals[2] = (bin2bcd(alrm->time.tm_sec) & 0x7f) | 0x80;
+ buf_alrm_vals[3] = (bin2bcd(alrm->time.tm_min) & 0x7f) | 0x80;
+ buf_alrm_vals[4] = (bin2bcd(alrm->time.tm_hour) & 0x3f) | 0x80;
+ buf_alrm_vals[5] = (bin2bcd(alrm->time.tm_mday) & 0x3f) | 0x80;
+ buf_alrm_vals[6] = (bin2bcd(alrm->time.tm_mon + 1) & 0x1f) | 0x80;
+
+ /* Non-volatile Page write to AL0 registers and enable INT */
+ ret = isl12026_arm_write(client);
+ if (ret < 0)
+ return ret;
+ ret = i2c_transfer(client->adapter, &msg, 1);
+ msleep(ISL12026_NVMEM_WRITE_TIME);
+ if (ret != 1) {
+ dev_err(&client->dev, "Error writing to alarm registers\n");
+ return ret < 0 ? ret : -EIO;
+ }
+
+ /* Enable AL0 interrupt */
+ ret = isl12026_write_reg(client, ISL12026_REG_INT, ISL12026_AL0E);
+
+ return ret;
+}
+
+static int isl12026_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
+{
+ struct i2c_client *client = to_i2c_client(dev);
+ int ret;
+ int sr, ir;
+ u8 buf_alrm_vals[5];
+ u8 addr[2] = {0x0, ISL12026_AL0_REG_SC};
+ struct i2c_msg msgs[2] = { };
+
+ msgs[0].addr = client->addr;
+ msgs[0].flags = 0x0; /* Write register address */
+ msgs[0].buf = addr;
+ msgs[0].len = sizeof(addr);
+
+ msgs[1].addr = client->addr;
+ msgs[1].flags = I2C_M_RD; /* Alarm read operation */
+ msgs[1].buf = buf_alrm_vals;
+ msgs[1].len = sizeof(buf_alrm_vals);
+
+ /* Read alarm enable status */
+ ir = isl12026_read_reg(client, ISL12026_REG_INT);
+ if (ir < 0)
+ return ir;
+ alrm->enabled = !!(ir & ISL12026_AL0E);
+
+ /* Read alarm pending status */
+ sr = isl12026_read_reg(client, ISL12026_REG_SR);
+ if (sr < 0)
+ return sr;
+ alrm->pending = !!(sr & ISL12026_SR_AL0) && alrm->enabled;
+
+ /* Page read for alarm registers */
+ ret = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
+ if (ret != ARRAY_SIZE(msgs)) {
+ dev_err(&client->dev, "Error reading alarm registers\n");
+ return ret < 0 ? ret : -EIO;
+ }
+
+ /* Populate values read */
+ alrm->time.tm_sec = bcd2bin(buf_alrm_vals[0] & 0x7f);
+ alrm->time.tm_min = bcd2bin(buf_alrm_vals[1] & 0x7f);
+ alrm->time.tm_hour = bcd2bin(buf_alrm_vals[2] & 0x3f);
+ alrm->time.tm_mday = bcd2bin(buf_alrm_vals[3] & 0x3f);
+ alrm->time.tm_mon = bcd2bin(buf_alrm_vals[4] & 0x1f) - 1;
+
+ return 0;
+}
+
+static int isl12026_rtc_alarm_irq_en(struct device *dev, unsigned int enabled)
+{
+ struct i2c_client *client = to_i2c_client(dev);
+ int ret;
+ int ir;
+
+ if (enabled) {
+ ret = isl12026_write_reg(client, ISL12026_REG_INT, ISL12026_AL0E);
+ return ret;
+ }
+
+ /* Disable alarm */
+ ir = isl12026_read_reg(client, ISL12026_REG_INT);
+ if (ir < 0)
+ return ir;
+ ir &= ~ISL12026_AL0E;
+ ret = isl12026_write_reg(client, ISL12026_REG_INT, ir);
+
+ return ret;
+}
+
static const struct rtc_class_ops isl12026_rtc_ops = {
.read_time = isl12026_rtc_read_time,
.set_time = isl12026_rtc_set_time,
+ .set_alarm = isl12026_rtc_set_alarm,
+ .read_alarm = isl12026_rtc_read_alarm,
+ .alarm_irq_enable = isl12026_rtc_alarm_irq_en,
};
static int isl12026_nvm_read(void *p, unsigned int offset,
--
2.34.1
^ permalink raw reply related
* Re: [PATCH v6 2/2] rtc: Add NXP PCF85053 driver support
From: Alexandre Belloni @ 2025-11-15 11:02 UTC (permalink / raw)
To: Lakshay Piplani
Cc: linux-rtc, linux-kernel, robh, krzk+dt, conor+dt, devicetree,
pankit.garg, vikash.bansal, priyanka.jain, shashank.rebbapragada,
Daniel Aguirre
In-Reply-To: <20251113054243.4045820-2-lakshay.piplani@nxp.com>
On 13/11/2025 11:12:43+0530, Lakshay Piplani wrote:
> +#define PCF85053_REG_BAT_MASK 0x07 /* Battery mask */
> +#define PCF85053A_BVL_MASK 0x07
> +#define PCF85053A_BVL_LOW_THRESHOLD 0x02
> +#define PCF85053_REG_CLKO_F_MASK 0x03 /* Frequenc mask */
> +#define PCF85053_REG_CLKO_CKE 0x80 /* clock out enabled */
> +#define PCF85053_BIT_OF BIT(6)
> +
> +#define PCF85053_HR_PM BIT(7)
> +#define PCF85053_HR_24H_MASK GENMASK(5, 0)
> +
> +struct pcf85053_config {
> + const struct regmap_config regmap;
> + unsigned has_alarms:1;
> +};
> +
> +struct pcf85053 {
> + struct rtc_device *rtc;
> + struct i2c_client *client;
This member is probably not necessary
> + struct regmap *regmap;
> +#ifdef CONFIG_COMMON_CLK
> + struct clk_hw clkout_hw;
> +#endif
> + bool is_primary;
> +};
> +
> +static inline int pcf85053_read_two_bit(struct pcf85053 *pcf85053, bool *two)
> +{
> + unsigned int ctrl;
> + int err;
> +
> + err = regmap_read(pcf85053->regmap, PCF85053_REG_CTRL, &ctrl);
> + if (err)
> + return err;
> +
> + *two = !!(ctrl & PCF85053_BIT_TWO);
> +
> + return 0;
> +}
> +
> +static inline bool pcf85053_time_write_access(struct pcf85053 *pcf85053)
> +{
> + bool two;
> +
> + if (pcf85053_read_two_bit(pcf85053, &two))
> + return false;
> +
> + /* Primary writes iff TWO=1; secondary writes iff TWO=0 */
> + return pcf85053->is_primary ? two : !two;
> +}
> +
> +static int pcf85053_set_alarm_mode(struct device *dev, bool on)
This should take the regmap as a parameter, you have multiple extra
level of indirection when calling this function.
> +{
> + struct pcf85053 *pcf85053 = dev_get_drvdata(dev);
> + unsigned int val;
> + int err;
> +
> + val = on ? PCF85053_BIT_AIE : 0;
> + val &= ~(PCF85053_BIT_CIE | PCF85053_BIT_OFIE);
This doesn't do anything
> +
> + err = regmap_update_bits(pcf85053->regmap, PCF85053_REG_CTRL,
> + PCF85053_BIT_AIE | PCF85053_BIT_CIE | PCF85053_BIT_OFIE,
> + val);
> + if (err)
> + return err;
> +
> + return regmap_update_bits(pcf85053->regmap, PCF85053_REG_ST,
> + PCF85053_BIT_AF, 0);
> +}
> +
> +static int pcf85053_get_alarm_mode(struct device *dev,
> + unsigned char *alarm_enable, unsigned char *alarm_flag)
> +{
> + struct pcf85053 *pcf85053 = dev_get_drvdata(dev);
> + unsigned int val;
> + int err;
> +
> + if (alarm_enable) {
> + err = regmap_read(pcf85053->regmap, PCF85053_REG_CTRL, &val);
> + if (err)
> + return err;
> +
> + *alarm_enable = val & PCF85053_BIT_AIE;
> + }
> +
> + if (alarm_flag) {
> + err = regmap_read(pcf85053->regmap, PCF85053_REG_ST, &val);
> + if (err)
> + return err;
> +
> + *alarm_flag = val & PCF85053_BIT_AF;
> + }
> +
> + return 0;
> +}
> +
> +static irqreturn_t pcf85053_irq(int irq, void *dev_id)
> +{
> + struct pcf85053 *pcf85053 = i2c_get_clientdata(dev_id);
> + unsigned char alarm_flag;
> + unsigned char alarm_enable;
> + int err;
> +
> + err = pcf85053_get_alarm_mode(&pcf85053->client->dev, &alarm_enable, &alarm_flag);
> + if (err)
> + return IRQ_NONE;
> +
> + if (!alarm_flag)
> + return IRQ_NONE;
> +
> + rtc_update_irq(pcf85053->rtc, 1, RTC_IRQF | RTC_AF);
> + pcf85053_set_alarm_mode(&pcf85053->client->dev, false);
I feel like this is reading and writing way too much, you could probably
only test and clear PCF85053_REG_ST, with a single
regmap_update_bits_check call
> +
> + return IRQ_HANDLED;
> +}
> +
> +/*
> + * In the routines that deal directly with the PCF85053 hardware, we use
> + * rtc_time -- month 0-11, hour 0-23, yr = calendar year-epoch.
> + */
> +static int pcf85053_rtc_read_time(struct device *dev, struct rtc_time *tm)
> +{
> + struct pcf85053 *pcf85053 = dev_get_drvdata(dev);
> + unsigned int ctrl, st, h12;
> + bool is_24h, is_bin;
> + u8 regs[10], hr;
> + int err;
> +
> + err = regmap_read(pcf85053->regmap, PCF85053_REG_CTRL, &ctrl);
> + if (err)
> + return err;
> +
> + err = regmap_read(pcf85053->regmap, PCF85053_REG_ST, &st);
> + if (err)
> + return err;
> +
> + if (ctrl & PCF85053_BIT_ST)
> + dev_warn(dev, "RTC is stopped; time may be invalid\n");
no message needed but return -EINVAL
> +
> + err = regmap_bulk_read(pcf85053->regmap, PCF85053_REG_SC, regs, sizeof(regs));
> + if (err)
> + return err;
> +
> + if (ctrl & PCF85053_BIT_DM) {
> + tm->tm_sec = regs[PCF85053_REG_SC] & 0x7F;
> + tm->tm_min = regs[PCF85053_REG_MN] & 0x7F;
> + tm->tm_mday = regs[PCF85053_REG_DM] & 0x3F;
> + tm->tm_mon = (regs[PCF85053_REG_MO] & 0x1F) - 1;
> + tm->tm_year = regs[PCF85053_REG_YR] + 100;
> + } else {
> + tm->tm_sec = bcd2bin(regs[PCF85053_REG_SC] & 0x7F);
> + tm->tm_min = bcd2bin(regs[PCF85053_REG_MN] & 0x7F);
> + tm->tm_mday = bcd2bin(regs[PCF85053_REG_DM] & 0x3F);
> + tm->tm_mon = bcd2bin(regs[PCF85053_REG_MO] & 0x1F) - 1;
> + tm->tm_year = bcd2bin(regs[PCF85053_REG_YR]) + 100;
> + }
> + tm->tm_wday = regs[PCF85053_REG_DW] & 0x07;
> +
> + hr = regs[PCF85053_REG_HR];
> + is_24h = ctrl & PCF85053_BIT_HF;
> + is_bin = ctrl & PCF85053_BIT_DM;
> +
> + if (is_24h) {
> + tm->tm_hour = is_bin
> + ? (hr & PCF85053_HR_24H_MASK)
> + : bcd2bin(hr & PCF85053_HR_24H_MASK);
> + } else {
> + if (is_bin) {
> + h12 = hr & PCF85053_HR_24H_MASK;
This doesn't do anything
> + } else {
> + h12 = is_bin ? (hr & PCF85053_HR_24H_MASK) :
> + bcd2bin(hr & PCF85053_HR_24H_MASK);
> +
> + tm->tm_hour = (h12 == 12) ? ((hr & PCF85053_HR_PM) ? 12 : 0) :
> + ((hr & PCF85053_HR_PM) ? h12 + 12 : h12);
> + }
> + }
> +
> + return 0;
> +}
> +
> +static int pcf85053_rtc_set_time(struct device *dev, struct rtc_time *tm)
> +
> +{
> + struct pcf85053 *pcf85053 = dev_get_drvdata(dev);
> + unsigned int ctrl, h12;
> + int err, ret;
> + u8 buf[10];
> + bool pm;
> +
> + /*
> + * By default, secondary have write access to time registers as TWO
> + * bit is 0 by default, if we set nxp,interface = "primary" and the
> + * nxp,write-access in device tree, then TWO bits gets set and primary
> + * gets write access to time registers.
> + */
> + if (!pcf85053_time_write_access(pcf85053))
> + return -EACCES;
> +
> + err = regmap_read(pcf85053->regmap, PCF85053_REG_CTRL, &ctrl);
> + if (err)
> + return err;
> +
> + buf[0] = tm->tm_sec & 0x7F;
> + buf[1] = 0;
> + buf[2] = tm->tm_min & 0x7F;
> + buf[3] = 0;
> + buf[5] = 0;
> + buf[6] = tm->tm_wday & 0x07;
> + buf[7] = tm->tm_mday & 0x3F;
> + buf[8] = (tm->tm_mon + 1) & 0x1F;
> + buf[9] = (tm->tm_year - 100) & 0xFF;
> +
> + if (ctrl & PCF85053_BIT_HF) {
> + buf[4] = tm->tm_hour & PCF85053_HR_24H_MASK;
> + } else {
> + pm = tm->tm_hour >= 12;
> + h12 = (tm->tm_hour % 12) ? (tm->tm_hour % 12) : 12;
> + buf[4] = (h12 & PCF85053_HR_24H_MASK) | (pm << 7);
> + }
> +
> + if (!(ctrl & PCF85053_BIT_DM)) {
> + buf[0] = bin2bcd(buf[0]);
> + buf[2] = bin2bcd(buf[2]);
> + buf[4] = bin2bcd(buf[4] & PCF85053_HR_24H_MASK) | (buf[4] & PCF85053_HR_PM);
> + buf[7] = bin2bcd(buf[7]);
> + buf[8] = bin2bcd(buf[8]);
> + buf[9] = bin2bcd(buf[9]);
> + }
> +
You have write access, simply set the date to the simplest format
instead of supporting all of them.
> + if (pcf85053->is_primary) {
> + err = regmap_update_bits(pcf85053->regmap, PCF85053_REG_CTRL,
> + PCF85053_BIT_ST, PCF85053_BIT_ST);
> + if (err)
> + return err;
> +
> + ret = regmap_bulk_write(pcf85053->regmap, PCF85053_REG_SC, buf, sizeof(buf));
> + err = regmap_update_bits(pcf85053->regmap, PCF85053_REG_CTRL,
> + PCF85053_BIT_ST, 0);
> + return ret ? ret : err;
> + }
> +
> + return regmap_bulk_write(pcf85053->regmap, PCF85053_REG_SC, buf, sizeof(buf));
> +}
> +
> +static int pcf85053_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *tm)
> +{
> + struct pcf85053 *pcf85053 = dev_get_drvdata(dev);
> + unsigned int ctrl, h12;
> + bool is_24h, is_bin, pm;
> + u8 buf[5];
> + u8 hr;
> + int err;
> +
> + err = regmap_read(pcf85053->regmap, PCF85053_REG_CTRL, &ctrl);
> + if (err)
> + return err;
> +
> + err = regmap_bulk_read(pcf85053->regmap, PCF85053_REG_SCA, buf, sizeof(buf));
> + if (err)
> + return err;
> +
> + if (ctrl & PCF85053_BIT_DM) {
> + tm->time.tm_sec = buf[0] & 0x7F; /* SCA */
> + tm->time.tm_min = buf[2] & 0x7F; /* MNA */
> + } else {
> + tm->time.tm_sec = bcd2bin(buf[0] & 0x7F);
> + tm->time.tm_min = bcd2bin(buf[2] & 0x7F);
> + }
> +
> + hr = buf[4];
> + is_24h = !!(ctrl & PCF85053_BIT_HF);
> + is_bin = !!(ctrl & PCF85053_BIT_DM);
> +
> + if (is_24h) {
> + tm->time.tm_hour = is_bin
> + ? (hr & PCF85053_HR_24H_MASK)
> + : bcd2bin(hr & PCF85053_HR_24H_MASK);
> + } else {
> + pm = !!(hr & PCF85053_HR_PM);
> +
> + if (is_bin)
> + h12 = (hr & PCF85053_HR_24H_MASK);
> + else
> + h12 = (bcd2bin(hr & PCF85053_HR_24H_MASK));
> +
> + if (h12 == 12)
> + h12 = 0;
> + tm->time.tm_hour = pm ? (h12 + 12) : h12;
> + }
> +
> + return pcf85053_get_alarm_mode(dev, &tm->enabled, &tm->pending);
> +}
> +
> +static int pcf85053_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *tm)
> +{
> + struct pcf85053 *pcf85053 = dev_get_drvdata(dev);
> + bool is_24h, is_bin, pm;
> + unsigned int ctrl, h12;
> + u8 sec, min, hra;
> + int err;
> +
> + /*
> + * Only primary can set alarm, as secondary have read only access
> + * to alarm, control and status registers
> + */
> + if (!pcf85053->is_primary)
> + return -EACCES;
> +
> + err = regmap_read(pcf85053->regmap, PCF85053_REG_CTRL, &ctrl);
> + if (err)
> + return err;
> +
> + err = regmap_update_bits(pcf85053->regmap, PCF85053_REG_ST,
> + PCF85053_BIT_AF, 0);
> + if (err)
> + return err;
> +
> + is_24h = !!(ctrl & PCF85053_BIT_HF);
> + is_bin = !!(ctrl & PCF85053_BIT_DM);
> +
> + sec = tm->time.tm_sec & 0x7F;
> + min = tm->time.tm_min & 0x7F;
> +
> + if (is_24h) {
> + hra = tm->time.tm_hour & PCF85053_HR_24H_MASK;
> + if (!is_bin)
> + hra = bin2bcd(hra) & PCF85053_HR_24H_MASK;
> + } else {
> + h12 = tm->time.tm_hour % 12;
> + pm = tm->time.tm_hour >= 12;
> + if (h12 == 0)
> + h12 = 12;
> +
> + if (is_bin)
> + hra = (h12 & PCF85053_HR_24H_MASK) | (pm << 7);
> + else
> + hra = (bin2bcd(h12) & PCF85053_HR_24H_MASK) | (pm << 7);
> + }
> +
> + if (!is_bin) {
> + sec = bin2bcd(sec);
> + min = bin2bcd(min);
> + }
> +
Same here.
> + err = regmap_write(pcf85053->regmap, PCF85053_REG_SCA, sec);
> + if (err)
> + return err;
> +
> + err = regmap_write(pcf85053->regmap, PCF85053_REG_MNA, min);
> + if (err)
> + return err;
> +
> + err = regmap_write(pcf85053->regmap, PCF85053_REG_HRA, hra);
> + if (err)
> + return err;
> +
> + return pcf85053_set_alarm_mode(dev, tm->enabled);
> +}
> +
> +static int pcf85053_irq_enable(struct device *dev, unsigned int enabled)
> +{
> + dev_dbg(dev, "%s: alarm enable=%d\n", __func__, enabled);
> +
> + return pcf85053_set_alarm_mode(dev, enabled);
> +}
> +
> +static int pcf85053_ioctl(struct device *dev, unsigned int cmd, unsigned long arg)
> +{
> + struct pcf85053 *pcf85053 = dev_get_drvdata(dev);
> + unsigned int val = 0, vl_status = 0;
> + unsigned int bvl;
> + int status;
> +
> + switch (cmd) {
> + case RTC_VL_READ:
> + status = regmap_read(pcf85053->regmap, PCF85053_REG_ST, &val);
> + if (status)
> + return status;
> +
> + if (val & PCF85053_BIT_OF)
> + vl_status |= RTC_VL_DATA_INVALID;
> +
> + bvl = val & PCF85053A_BVL_MASK;
> +
> + if (bvl == 0x00)
> + vl_status |= RTC_VL_BACKUP_EMPTY;
> + else if (bvl <= PCF85053A_BVL_LOW_THRESHOLD)
> + vl_status |= RTC_VL_BACKUP_LOW;
> +
> + return put_user(vl_status, (unsigned int __user *)arg);
> +
> + default:
> + return -ENOIOCTLCMD;
> + }
> +}
> +
> +#ifdef CONFIG_COMMON_CLK
> +/*
> + * Handling of the clkout
> + */
> +
> +#define clkout_hw_to_pcf85053(_hw) container_of(_hw, struct pcf85053, clkout_hw)
> +
> +static const int clkout_rates[] = {
> + 32768,
> + 1024,
> + 32,
> + 1,
> +};
> +
> +static unsigned long pcf85053_clkout_recalc_rate(struct clk_hw *hw,
> + unsigned long parent_rate)
> +{
> + struct pcf85053 *pcf85053 = clkout_hw_to_pcf85053(hw);
> + unsigned int val = 0;
> + int err;
> +
> + err = regmap_read(pcf85053->regmap, PCF85053_REG_CLKO, &val);
> + if (err)
> + return 0;
> +
> + val &= PCF85053_REG_CLKO_F_MASK;
> + return clkout_rates[val];
> +}
> +
> +static int pcf85053_clkout_determine_rate(struct clk_hw *hw,
> + struct clk_rate_request *req)
> +{
> + int i;
> + unsigned long best = 0;
> +
> + for (i = 0; i < ARRAY_SIZE(clkout_rates); i++) {
> + if (clkout_rates[i] <= req->rate) {
> + best = clkout_rates[i];
> + break;
> + }
> + }
> + if (!best)
> + best = clkout_rates[ARRAY_SIZE(clkout_rates) - 1];
> +
> + req->rate = best;
> + return 0;
> +}
> +
> +static int pcf85053_clkout_set_rate(struct clk_hw *hw, unsigned long rate,
> + unsigned long parent_rate)
> +{
> + struct pcf85053 *pcf85053 = clkout_hw_to_pcf85053(hw);
> + unsigned int val = 0;
> + int err, i;
> +
> + err = regmap_read(pcf85053->regmap, PCF85053_REG_CLKO, &val);
> + if (err)
> + return err;
> +
> + for (i = 0; i < ARRAY_SIZE(clkout_rates); i++)
> + if (clkout_rates[i] == rate) {
> + val &= ~PCF85053_REG_CLKO_F_MASK;
> + val |= i;
> + return regmap_write(pcf85053->regmap, PCF85053_REG_CLKO, val);
> + }
> +
> + return -EINVAL;
> +}
> +
> +static int pcf85053_clkout_control(struct clk_hw *hw, bool enable)
> +{
> + struct pcf85053 *pcf85053 = clkout_hw_to_pcf85053(hw);
> + unsigned int val = 0;
> + int err;
> +
> + if (!pcf85053->is_primary)
> + return -EACCES;
> +
> + val = PCF85053_BIT_XCLK;
> + err = regmap_write(pcf85053->regmap, PCF85053_REG_ACC, val);
> + if (err)
> + return err;
> +
> + err = regmap_read(pcf85053->regmap, PCF85053_REG_CLKO, &val);
> + if (err)
> + return err;
> +
> + if (enable)
> + val |= PCF85053_REG_CLKO_CKE;
> + else
> + val &= ~PCF85053_REG_CLKO_CKE;
> +
> + return regmap_write(pcf85053->regmap, PCF85053_REG_CLKO, val);
> +}
> +
> +static int pcf85053_clkout_prepare(struct clk_hw *hw)
> +{
> + return pcf85053_clkout_control(hw, 1);
> +}
> +
> +static void pcf85053_clkout_unprepare(struct clk_hw *hw)
> +{
> + pcf85053_clkout_control(hw, 0);
> +}
> +
> +static int pcf85053_clkout_is_prepared(struct clk_hw *hw)
> +{
> + struct pcf85053 *pcf85053 = clkout_hw_to_pcf85053(hw);
> + unsigned int val = 0;
> + int err;
> +
> + err = regmap_read(pcf85053->regmap, PCF85053_REG_CLKO, &val);
> + if (err)
> + return err;
> +
> + return val & PCF85053_REG_CLKO_CKE;
> +}
> +
> +static const struct clk_ops pcf85053_clkout_ops = {
> + .prepare = pcf85053_clkout_prepare,
> + .unprepare = pcf85053_clkout_unprepare,
> + .is_prepared = pcf85053_clkout_is_prepared,
> + .recalc_rate = pcf85053_clkout_recalc_rate,
> + .determine_rate = pcf85053_clkout_determine_rate,
> + .set_rate = pcf85053_clkout_set_rate,
> +};
> +
> +static struct clk *pcf85053_clkout_register_clk(struct pcf85053 *pcf85053)
> +{
> + struct i2c_client *client = pcf85053->client;
> + struct device_node *node = client->dev.of_node;
> + struct clk *clk;
> + struct clk_init_data init;
> +
> + init.name = "pcf85053-clkout";
> + init.ops = &pcf85053_clkout_ops;
> + init.flags = 0;
> + init.parent_names = NULL;
> + init.num_parents = 0;
> + pcf85053->clkout_hw.init = &init;
> +
> + /* optional override of the clockname */
> + of_property_read_string(node, "clock-output-names", &init.name);
> +
> + /* register the clock */
> + clk = devm_clk_register(&client->dev, &pcf85053->clkout_hw);
> +
> + if (!IS_ERR(clk))
> + of_clk_add_provider(node, of_clk_src_simple_get, clk);
> +
> + return clk;
> +}
> +#endif
> +
> +static const struct rtc_class_ops pcf85053_rtc_ops = {
> + .read_time = pcf85053_rtc_read_time,
> + .set_time = pcf85053_rtc_set_time,
> + .read_alarm = pcf85053_rtc_read_alarm,
> + .set_alarm = pcf85053_rtc_set_alarm,
> + .alarm_irq_enable = pcf85053_irq_enable,
> + .ioctl = pcf85053_ioctl,
> +};
> +
> +static const struct pcf85053_config config_pcf85053 = {
> + .regmap = {
> + .reg_bits = 8,
> + .val_bits = 8,
> + .max_register = 0x1D,
> + },
> + .has_alarms = 1,
> +};
> +
> +static int pcf85053_probe(struct i2c_client *client)
> +{
> + struct pcf85053 *pcf85053;
> + const struct pcf85053_config *config;
> + const char *iface = NULL;
> + int err;
> +
> + if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C |
> + I2C_FUNC_SMBUS_BYTE |
> + I2C_FUNC_SMBUS_BLOCK_DATA))
> + return -ENODEV;
> +
> + pcf85053 = devm_kzalloc(&client->dev, sizeof(struct pcf85053),
> + GFP_KERNEL);
> + if (!pcf85053)
> + return -ENOMEM;
> +
> + config = i2c_get_match_data(client);
> + if (!config)
> + return -ENODEV;
> +
> + pcf85053->regmap = devm_regmap_init_i2c(client, &config->regmap);
> + if (IS_ERR(pcf85053->regmap))
> + return PTR_ERR(pcf85053->regmap);
> +
> + i2c_set_clientdata(client, pcf85053);
> +
> + pcf85053->client = client;
> + device_set_wakeup_capable(&client->dev, 1);
> +
> + pcf85053->is_primary = true;
> +
> + if (of_property_read_string(client->dev.of_node, "nxp,interface", &iface))
> + return dev_err_probe(&client->dev, -EINVAL,
> + "Missing mandatory property: nxp,interface\n");
> + if (!strcmp(iface, "primary"))
> + pcf85053->is_primary = true;
> + else if (!strcmp(iface, "secondary"))
> + pcf85053->is_primary = false;
> + else
> + return dev_err_probe(&client->dev, -EINVAL,
> + "Invalid value for nxp,interface: %s\n", iface);
> +
> + if (pcf85053->is_primary) {
> + unsigned int ctrl;
> + int err;
> +
> + err = regmap_read(pcf85053->regmap, PCF85053_REG_CTRL, &ctrl);
> + if (err)
> + return err;
> +
> + if (of_property_read_bool(client->dev.of_node, "nxp,write-access")) {
> + if (!(ctrl & PCF85053_BIT_TWO)) {
> + err = regmap_update_bits(pcf85053->regmap, PCF85053_REG_CTRL,
> + PCF85053_BIT_TWO, PCF85053_BIT_TWO);
> + if (err)
> + return err;
> + }
> + dev_dbg(&client->dev, "Ownership set: TWO=1 (primary writes)\n");
> + } else {
> + /* TWO (Time Write Ownership) bit defaults to 0 (Secondary) */
> + dev_dbg(&client->dev, "Default ownership set: TWO=0 (secondary writes)\n");
> + }
> + }
> +
> + pcf85053->rtc = devm_rtc_allocate_device(&client->dev);
> + if (IS_ERR(pcf85053->rtc))
> + return PTR_ERR(pcf85053->rtc);
> +
> + pcf85053->rtc->ops = &pcf85053_rtc_ops;
> + pcf85053->rtc->range_min = RTC_TIMESTAMP_BEGIN_2000;
Was 2000 a leap year for this RTC? The datasheet would say it is.
> + pcf85053->rtc->range_max = RTC_TIMESTAMP_END_2099;
> + clear_bit(RTC_FEATURE_UPDATE_INTERRUPT, pcf85053->rtc->features);
> + clear_bit(RTC_FEATURE_ALARM, pcf85053->rtc->features);
> +
> + if (config->has_alarms && client->irq > 0) {
> + err = devm_request_threaded_irq(&client->dev, client->irq,
> + NULL, pcf85053_irq,
> + IRQF_ONESHOT | IRQF_TRIGGER_FALLING,
> + "pcf85053", client);
> + if (err) {
> + dev_err(&client->dev, "unable to request IRQ %d\n", client->irq);
> + } else {
> + set_bit(RTC_FEATURE_ALARM, pcf85053->rtc->features);
> + device_init_wakeup(&client->dev, true);
Use devm_device_init_wakeup
> + err = dev_pm_set_wake_irq(&client->dev, client->irq);
> + if (err)
> + dev_err(&client->dev, "failed to enable irq wake\n");
> + }
> + }
> +
> +#ifdef CONFIG_COMMON_CLK
> + /* register clk in common clk framework */
> + pcf85053_clkout_register_clk(pcf85053);
> +#endif
> +
> + return devm_rtc_register_device(pcf85053->rtc);
> +}
> +
> +static const struct i2c_device_id pcf85053_id[] = {
> + { "pcf85053", .driver_data = (kernel_ulong_t)&config_pcf85053 },
> + { }
> +};
> +MODULE_DEVICE_TABLE(i2c, pcf85053_id);
> +
> +static const struct of_device_id pcf85053_of_match[] = {
> + { .compatible = "nxp,pcf85053", .data = &config_pcf85053 },
> + {}
> +};
> +MODULE_DEVICE_TABLE(of, pcf85053_of_match);
> +
> +static struct i2c_driver pcf85053_driver = {
> + .driver = {
> + .name = "rtc-pcf85053",
> + .of_match_table = of_match_ptr(pcf85053_of_match),
> + },
> + .probe = pcf85053_probe,
> + .id_table = pcf85053_id,
> +};
> +
> +module_i2c_driver(pcf85053_driver);
> +
> +MODULE_AUTHOR("Pankit Garg <pankit.garg@nxp.com>");
> +MODULE_AUTHOR("Lakshay Piplani <lakshay.piplani@nxp.com>");
> +MODULE_DESCRIPTION("NXP pcf85053 RTC driver");
> +MODULE_LICENSE("GPL");
> --
> 2.25.1
>
--
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply
* 少子化は追い風。次世代の教育ビジネス
From: 石島紗希 @ 2025-11-14 23:38 UTC (permalink / raw)
To: linux-rtc
新規事業をお考えの経営者様へ
いつもお世話になります。
『少子化の時代に、教育事業！？』と、
思うかもしれませんが
実は、塾代補助金や教育無償化、贈与非課税といった
国策もあり、子ども1人にかける教育費は増加し、
市場は堅調に成長しています。
そんな中、 『少子化だから教育はダメ』
という誤った認識の方が多いので、
学習塾市場にはまだ、『手堅く利益がでるホワイトスペース』が残っています。
いったい、教育市場にどのようなビジネスチャンスがあるのか？
をオンラインセミナーでお伝えしますので、ぜひご視聴ください。
11月18日（火）16：00〜17：00 オンライン開催
----------------------------------------------------------
◆フランチャイズ事業 WEB説明会◆
“プログラミング教育×個別指導”
不況に強く、少子化でも成長する
ハイブリッド型の教育事業
▼ 視聴予約 ▼
https://wam-edu-fc.jp/wam2/
◆ ご案内事業 ◆
個別指導 WAM
◇ 提供 ◇
エイチ・エム・グループ
----------------------------------------------------------
少子化が進む中で、多くの人は 「教育市場は縮小する」 と考えています。
しかし実際は、子ども一人にかける教育費の増加に伴い、
市場は成長し続けています。
また、教育費は不況時でも削減されにくいため
コロナ下でも大きく落ち込むことなく底堅さを見せました。
幼児教育無償化などの国策もあり、今後も教育投資の増加が予想されます。
そこでご紹介するのが、「プログラミング×個別指導」の
ハイブリッド教育事業です。
プログラミングは小学校で必修化されたため、保護者の関心も高まっています。
本事業のスタートは「業界未経験」「社員１名」で可能です。
新たな事業収益づくりをお考えの方は、是非ご参加ください。
11月18日（火）16：00〜17：00 オンライン開催
▼ 視聴予約はこちら
https://wam-edu-fc.jp/wam2/
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
教育事業FCセミナー事務局
電話：0120-891-893
住所：東京都中央区銀座7-13-6
―――――――――――――――――――――――――――――――
本メールのご不要な方には大変ご迷惑をおかけいたしました。
今後ご案内が不要な方は下記URLよりお手続きをお願いいたします。
┗ https://wam-edu-fc.jp/mail/
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
^ permalink raw reply
* Re: [PATCH v4 04/16] dt-bindings: power: supply: BD72720 managed battery
From: Andreas Kemnade @ 2025-11-14 17:40 UTC (permalink / raw)
To: Rob Herring
Cc: Matti Vaittinen, Matti Vaittinen, Krzysztof Kozlowski, Mark Brown,
Linus Walleij, linux-kernel, Sebastian Reichel,
Bartosz Golaszewski, Alexandre Belloni, linux-clk,
Michael Turquette, Matti Vaittinen, linux-leds, Pavel Machek,
Liam Girdwood, linux-gpio, linux-pm, Conor Dooley, devicetree,
linux-rtc, Lee Jones, Stephen Boyd
In-Reply-To: <20251114163954.GA3399895-robh@kernel.org>
On Fri, 14 Nov 2025 10:39:54 -0600
Rob Herring <robh@kernel.org> wrote:
> On Fri, Nov 14, 2025 at 11:04:27AM +0200, Matti Vaittinen wrote:
> > On 13/11/2025 12:53, Rob Herring (Arm) wrote:
> > >
> > > On Thu, 13 Nov 2025 10:52:19 +0200, Matti Vaittinen wrote:
> > > > From: Matti Vaittinen <mazziesaccount@gmail.com>
> > > >
> > > > The BD72720 PMIC has a battery charger + coulomb counter block. These
> > > > can be used to manage charging of a lithium-ion battery and to do fuel
> > > > gauging.
> > > >
> > > > ROHM has developed a so called "zero-correction" -algorithm to improve
> > > > the fuel-gauging accuracy close to the point where battery is depleted.
> > > > This relies on battery specific "VDR" tables, which are measured from
> > > > the battery, and which describe the voltage drop rate. More thorough
> > > > explanation about the "zero correction" and "VDR" parameters is here:
> > > > https://lore.kernel.org/all/676253b9-ff69-7891-1f26-a8b5bb5a421b@fi.rohmeurope.com/
> > > >
> > > > Document the VDR zero-correction specific battery properties used by the
> > > > BD72720 and some other ROHM chargers.
> > > >
> > > > Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
> > > > Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> > > >
> > > > ---
> > > > NOTE:
> > > > Linus' rb-tag holds only if there's no further comments from Rob.
> > > >
> > > > Revision history:
> > > > v3 =>:
> > > > - No changes
> > > >
> > > > v2 => v3:
> > > > - Constrain VDR threshold voltage to 48V
> > > > - Use standard '-bp' -suffix for the rohm,volt-drop-soc
> > > >
> > > > RFCv1 => v2:
> > > > - Add units to rohm,volt-drop-soc (tenths of %)
> > > > - Give real temperatures matching the VDR tables, instead of vague
> > > > 'high', 'normal', 'low', 'very low'. (Add table of temperatures and
> > > > use number matching the right temperature index in the VDR table name).
> > > > - Fix typoed 'algorithm' in commit message.
> > > >
> > > > The parameters are describing the battery voltage drop rates - so they
> > > > are properties of the battery, not the charger. Thus they do not belong
> > > > in the charger node.
> > > >
> >
> > // snip
> >
> > > My bot found errors running 'make dt_binding_check' on your patch:
> > >
> > > yamllint warnings/errors:
> > >
> > > dtschema/dtc warnings/errors:
> > > /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/power/supply/rohm,vdr-battery.example.dtb: battery (simple-battery): 'degrade-cycle-microamp-hours', 'rohm,volt-drop-0-microvolt', 'rohm,volt-drop-1-microvolt', 'rohm,volt-drop-2-microvolt', 'rohm,volt-drop-3-temp-microvolt', 'rohm,volt-drop-soc-bp', 'rohm,volt-drop-temperatures-millicelsius', 'rohm,voltage-vdr-thresh-microvolt' do not match any of the regexes: '^ocv-capacity-table-[0-9]+$', '^pinctrl-[0-9]+$'
> > > from schema $id: http://devicetree.org/schemas/power/supply/battery.yaml
> > >
> >
> > Odd. I am pretty sure I didn't see this when I ran the make
> > dt_binding_check. Not 100% sure what happened there. I get this error now
> > though when including all the bindings to the check.
> >
> > Do I get this right - these errors result from the properties used in
> > example not being included in the battery.yaml? So, this means that the
> > check is done based on the binding (battery.yaml) where the compatible
> > (simple-battery) is defined - not based on the properties which are present
> > in this file where the example resides, (and which references the
> > battery.yaml)?
> >
> > ...
> >
> > Oh... Now that I wrote it I feel like an idiot.
> >
> > This approach couldn't work for the validation, right? Let's assume I had a
> > VDR battery, and I added a static-battery -node for it. Running the
> > validation would pick the battery.yaml based on the compatible (just as it
> > does here), and be completely unaware of this vdr-battery.yaml. I have no
> > idea why I thought this would work. Probably because I only thought this
> > from the documentation POV.
> >
> > So, as far as I understand, the only viable options are expanding the
> > existing battery.yaml with these properties (which I hoped to avoid, see
> > below)
> >
> > >> The right place for them is the battery node, which is described by the
> > >> generic "battery.yaml". I was not comfortable with adding these
> > >> properties to the generic battery.yaml because they are:
> > >> - Meaningful only for those charger drivers which have the VDR
> > >> algorithm implemented. (And even though the algorithm is not charger
> > >> specific, AFAICS, it is currently only used by some ROHM PMIC
> > >> drivers).
> > >> - Technique of measuring the VDR tables for a battery is not widely
> > >> known. AFAICS, only folks at ROHM are measuring those for some
> > >> customer products. We do have those tables available for some of the
> > >> products though (Kobo?).
> >
> > or, to add new compatible for the "vdr-battery".
> > AFAICS, adding new compatible would require us to wither duplicate the used
> > properties from battery.yaml here (as battery.yaml mandates the
> > "simple-battery" - compatible) - or to split the battery.yaml in two files,
> > one containing the generic properties, other containing the "simple-battery"
> > -compatible and referencing the generic one. Then the "vdr-battery" could
> > also reference the generic one.
> >
> > Any suggestions for the next path to follow?
>
> Probably the latter option. You could do the former and make the new
> properties conditional on the "vdr-battery" compatible. That's fine with
> small differences, but gets messy as there are more properties and
> variations.
>
just keep in mind, that several kobo devices have one pmic in one board
revision and another one in the other (e.g. Kobo Nia rev A vs rev C).
But probably the same battery. So if the "vdr-battery" is a compatible
just to allow a more properties,
then "simple-battery" should be allowed as fallback.
Regards,
Andreas
^ permalink raw reply
* Re: [PATCH v4 04/16] dt-bindings: power: supply: BD72720 managed battery
From: Rob Herring @ 2025-11-14 16:39 UTC (permalink / raw)
To: Matti Vaittinen
Cc: Matti Vaittinen, Krzysztof Kozlowski, Mark Brown, Linus Walleij,
linux-kernel, Sebastian Reichel, Bartosz Golaszewski,
Alexandre Belloni, linux-clk, Michael Turquette, Matti Vaittinen,
linux-leds, Pavel Machek, Liam Girdwood, linux-gpio, linux-pm,
Andreas Kemnade, Conor Dooley, devicetree, linux-rtc, Lee Jones,
Stephen Boyd
In-Reply-To: <ee36d7d1-ef47-4a35-9aff-baa6ed32105a@gmail.com>
On Fri, Nov 14, 2025 at 11:04:27AM +0200, Matti Vaittinen wrote:
> On 13/11/2025 12:53, Rob Herring (Arm) wrote:
> >
> > On Thu, 13 Nov 2025 10:52:19 +0200, Matti Vaittinen wrote:
> > > From: Matti Vaittinen <mazziesaccount@gmail.com>
> > >
> > > The BD72720 PMIC has a battery charger + coulomb counter block. These
> > > can be used to manage charging of a lithium-ion battery and to do fuel
> > > gauging.
> > >
> > > ROHM has developed a so called "zero-correction" -algorithm to improve
> > > the fuel-gauging accuracy close to the point where battery is depleted.
> > > This relies on battery specific "VDR" tables, which are measured from
> > > the battery, and which describe the voltage drop rate. More thorough
> > > explanation about the "zero correction" and "VDR" parameters is here:
> > > https://lore.kernel.org/all/676253b9-ff69-7891-1f26-a8b5bb5a421b@fi.rohmeurope.com/
> > >
> > > Document the VDR zero-correction specific battery properties used by the
> > > BD72720 and some other ROHM chargers.
> > >
> > > Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
> > > Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> > >
> > > ---
> > > NOTE:
> > > Linus' rb-tag holds only if there's no further comments from Rob.
> > >
> > > Revision history:
> > > v3 =>:
> > > - No changes
> > >
> > > v2 => v3:
> > > - Constrain VDR threshold voltage to 48V
> > > - Use standard '-bp' -suffix for the rohm,volt-drop-soc
> > >
> > > RFCv1 => v2:
> > > - Add units to rohm,volt-drop-soc (tenths of %)
> > > - Give real temperatures matching the VDR tables, instead of vague
> > > 'high', 'normal', 'low', 'very low'. (Add table of temperatures and
> > > use number matching the right temperature index in the VDR table name).
> > > - Fix typoed 'algorithm' in commit message.
> > >
> > > The parameters are describing the battery voltage drop rates - so they
> > > are properties of the battery, not the charger. Thus they do not belong
> > > in the charger node.
> > >
>
> // snip
>
> > My bot found errors running 'make dt_binding_check' on your patch:
> >
> > yamllint warnings/errors:
> >
> > dtschema/dtc warnings/errors:
> > /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/power/supply/rohm,vdr-battery.example.dtb: battery (simple-battery): 'degrade-cycle-microamp-hours', 'rohm,volt-drop-0-microvolt', 'rohm,volt-drop-1-microvolt', 'rohm,volt-drop-2-microvolt', 'rohm,volt-drop-3-temp-microvolt', 'rohm,volt-drop-soc-bp', 'rohm,volt-drop-temperatures-millicelsius', 'rohm,voltage-vdr-thresh-microvolt' do not match any of the regexes: '^ocv-capacity-table-[0-9]+$', '^pinctrl-[0-9]+$'
> > from schema $id: http://devicetree.org/schemas/power/supply/battery.yaml
> >
>
> Odd. I am pretty sure I didn't see this when I ran the make
> dt_binding_check. Not 100% sure what happened there. I get this error now
> though when including all the bindings to the check.
>
> Do I get this right - these errors result from the properties used in
> example not being included in the battery.yaml? So, this means that the
> check is done based on the binding (battery.yaml) where the compatible
> (simple-battery) is defined - not based on the properties which are present
> in this file where the example resides, (and which references the
> battery.yaml)?
>
> ...
>
> Oh... Now that I wrote it I feel like an idiot.
>
> This approach couldn't work for the validation, right? Let's assume I had a
> VDR battery, and I added a static-battery -node for it. Running the
> validation would pick the battery.yaml based on the compatible (just as it
> does here), and be completely unaware of this vdr-battery.yaml. I have no
> idea why I thought this would work. Probably because I only thought this
> from the documentation POV.
>
> So, as far as I understand, the only viable options are expanding the
> existing battery.yaml with these properties (which I hoped to avoid, see
> below)
>
> >> The right place for them is the battery node, which is described by the
> >> generic "battery.yaml". I was not comfortable with adding these
> >> properties to the generic battery.yaml because they are:
> >> - Meaningful only for those charger drivers which have the VDR
> >> algorithm implemented. (And even though the algorithm is not charger
> >> specific, AFAICS, it is currently only used by some ROHM PMIC
> >> drivers).
> >> - Technique of measuring the VDR tables for a battery is not widely
> >> known. AFAICS, only folks at ROHM are measuring those for some
> >> customer products. We do have those tables available for some of the
> >> products though (Kobo?).
>
> or, to add new compatible for the "vdr-battery".
> AFAICS, adding new compatible would require us to wither duplicate the used
> properties from battery.yaml here (as battery.yaml mandates the
> "simple-battery" - compatible) - or to split the battery.yaml in two files,
> one containing the generic properties, other containing the "simple-battery"
> -compatible and referencing the generic one. Then the "vdr-battery" could
> also reference the generic one.
>
> Any suggestions for the next path to follow?
Probably the latter option. You could do the former and make the new
properties conditional on the "vdr-battery" compatible. That's fine with
small differences, but gets messy as there are more properties and
variations.
But is "VDR" a type of battery though? Is there a certain type/chemistry
of battery we should be describing where VDR is applicable? I don't
think it scales well if we define battery compatibles for every
variation of charger algorithm. Honestly I don't mind just adding 1
property. I care more if we allow undocumented properties than
allowing documented but invalid for the platform properties. When it
becomes 10, 20, 30 properties, then I might start to care. If that
happens, either we are doing a poor job of generically describing
battery parameters or chargers and batteries are tightly coupled and
can't be described independently.
Rob
^ permalink raw reply
* Re: [PATCH v4 01/16] dt-bindings: regulator: ROHM BD72720
From: Rob Herring (Arm) @ 2025-11-14 16:22 UTC (permalink / raw)
To: Matti Vaittinen
Cc: linux-kernel, Michael Turquette, devicetree, Krzysztof Kozlowski,
Lee Jones, Sebastian Reichel, Mark Brown, linux-clk,
Matti Vaittinen, linux-rtc, Conor Dooley, Andreas Kemnade,
Matti Vaittinen, Bartosz Golaszewski, Pavel Machek, Stephen Boyd,
Alexandre Belloni, linux-leds, linux-gpio, Liam Girdwood,
Linus Walleij, linux-pm
In-Reply-To: <21e83fccf2d2422f4bea1c482dcd3cb1aeda4085.1763022807.git.mazziesaccount@gmail.com>
On Thu, 13 Nov 2025 10:51:32 +0200, Matti Vaittinen wrote:
> From: Matti Vaittinen <mazziesaccount@gmail.com>
>
> The ROHM BD72720 is a new PMIC with 10 BUCk and 11 LDO regulators.
>
> The BD72720 is designed to support using the BUCK10 as a supply for
> the LDOs 1 to 4. When the BUCK10 is used for this, it can be set to a
> LDON_HEAD mode. In this mode, the BUCK10 voltage can't be controlled by
> software, but the voltage is adjusted by PMIC to match the LDO1 .. LDO4
> voltages with a given offset. Offset can be 50mV .. 300mV and is
> changeable at 50mV steps.
>
> Add 'ldon-head-microvolt' property to denote a board which is designed
> to utilize the LDON_HEAD mode.
>
> All other properties are already existing.
>
> Add dt-binding doc for ROHM BD72720 regulators to make it usable.
>
> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
>
> ---
> Revision history:
> v3 => v4:
> - Drop type from ldon-head
> - Fix the name patterns for regulator nodes and names
>
> v2 => v3:
> - drop unnecessary descriptions
> - use microvolts for the 'ldon-head' dt-property
>
> RFCv1 => v2:
> - No changes
> ---
> .../regulator/rohm,bd72720-regulator.yaml | 148 ++++++++++++++++++
> 1 file changed, 148 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/regulator/rohm,bd72720-regulator.yaml
>
Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
^ permalink raw reply
* Re: [PATCH v2 2/2] rtc: pcf85363: add support for additional features
From: Alexandre Belloni @ 2025-11-14 16:20 UTC (permalink / raw)
To: Lakshay Piplani
Cc: linux-rtc, linux-kernel, robh, krzk+dt, conor+dt, devicetree,
vikash.bansal, priyanka.jain, shashank.rebbapragada
In-Reply-To: <20250811082123.1099880-2-lakshay.piplani@nxp.com>
Hello,
On 11/08/2025 13:51:23+0530, Lakshay Piplani wrote:
> Add support for additional features to the NXP PCF8263/PCF85363 RTC driver:
> - Alarm2 (minute,hour,weekday)
> - Timestamps recording for TS pin and Battery switch-over events
> - Battery switch over detection
> - Offset calibration
> - Watchdog timer
This needs to be split per added function. The watchdog part has to be
reviewed by the watchdog maintainers
Alarm2 support definitively won't go in unless you provide an actual use
case.
>
> Signed-off-by: Lakshay Piplani <lakshay.piplani@nxp.com>
> ---
> Changes in v2:
> - Watchdog related changes due to removal of vendor specific properties
> from device tree
> * remove vendor DT knobs (enable/timeout/stepsize/repeat)
> * use watchdog_init_timeout (with 10s default)
> * derive clock_sel from final timeout
> * default, repeat=true (repeat mode)
> - Fixed uninitalised warning on 'ret' (reported by kernel test robot)
> - Use dev_dbg instead of dev_info for debug related print messages
> - Minor cleanup and comments
>
> drivers/rtc/rtc-pcf85363.c | 557 ++++++++++++++++++++++++++++++++++---
> 1 file changed, 522 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/rtc/rtc-pcf85363.c b/drivers/rtc/rtc-pcf85363.c
> index 540042b9eec8..c5c59876bad5 100644
> --- a/drivers/rtc/rtc-pcf85363.c
> +++ b/drivers/rtc/rtc-pcf85363.c
> @@ -5,6 +5,10 @@
> * Driver for NXP PCF85363 real-time clock.
> *
> * Copyright (C) 2017 Eric Nelson
> + *
> + * Copyright 2025 NXP
> + * Added support for alarm2, timestamps, battery switch-over,
> + * watchdog, offset calibration.
> */
> #include <linux/module.h>
> #include <linux/i2c.h>
> @@ -15,7 +19,11 @@
> #include <linux/errno.h>
> #include <linux/bcd.h>
> #include <linux/of.h>
> +#include <linux/of_irq.h>
> +#include <linux/of_device.h>
> #include <linux/regmap.h>
> +#include <linux/watchdog.h>
> +#include <linux/uaccess.h>
>
> /*
> * Date/Time registers
> @@ -100,19 +108,48 @@
> #define PIN_IO_INTA_OUT 2
> #define PIN_IO_INTA_HIZ 3
>
> +#define PIN_IO_TSPM GENMASK(3, 2)
> +#define PIN_IO_TSIM BIT(4)
> +
> #define OSC_CAP_SEL GENMASK(1, 0)
> #define OSC_CAP_6000 0x01
> #define OSC_CAP_12500 0x02
>
> #define STOP_EN_STOP BIT(0)
> +#define RTCM_BIT BIT(4)
>
> #define RESET_CPR 0xa4
>
> #define NVRAM_SIZE 0x40
>
> +#define TSR1_MASK 0x03
> +#define TSR2_MASK 0x07
> +#define TSR3_MASK 0x03
> +#define TSR1_SHIFT 0
> +#define TSR2_SHIFT 2
> +#define TSR3_SHIFT 6
> +
> +#define WD_MODE_REPEAT BIT(7)
> +#define WD_TIMEOUT_MASK GENMASK(6, 2)
> +#define WD_TIMEOUT_SHIFT 2
> +#define WD_CLKSEL_MASK GENMASK(1, 0)
> +#define WD_CLKSEL_0_25HZ 0x00
> +#define WD_CLKSEL_1HZ 0x01
> +#define WD_CLKSEL_4HZ 0x02
> +#define WD_CLKSEL_16HZ 0x03
> +
> +#define WD_TIMEOUT_MIN 1
> +#define WD_TIMEOUT_MAX 0x1F
> +
> +#define OFFSET_SIGN_BIT 7
> +#define OFFSET_MINIMUM -128
> +#define OFFSET_MAXIMUM 127
> +#define OFFSET_MASK 0xFF
> +
> struct pcf85363 {
> struct rtc_device *rtc;
> struct regmap *regmap;
> + u8 ts_valid_flags;
> };
>
> struct pcf85x63_config {
> @@ -120,6 +157,15 @@ struct pcf85x63_config {
> unsigned int num_nvram;
> };
>
> +struct pcf85363_watchdog {
> + struct watchdog_device wdd;
> + struct regmap *regmap;
> + struct device *dev;
> + u8 timeout_val;
> + u8 clock_sel;
> + bool repeat;
> +};
> +
> static int pcf85363_load_capacitance(struct pcf85363 *pcf85363, struct device_node *node)
> {
> u32 load = 7000;
> @@ -295,28 +341,147 @@ static int pcf85363_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
> static irqreturn_t pcf85363_rtc_handle_irq(int irq, void *dev_id)
> {
> struct pcf85363 *pcf85363 = i2c_get_clientdata(dev_id);
> + bool handled = false;
> unsigned int flags;
> int err;
>
> err = regmap_read(pcf85363->regmap, CTRL_FLAGS, &flags);
> +
> if (err)
> return IRQ_NONE;
>
> + if (flags) {
> + dev_dbg(&pcf85363->rtc->dev, "IRQ flags: 0x%02x%s%s%s%s%s%s%s\n",
> + flags, (flags & FLAGS_A1F) ? " [A1F]" : "",
> + (flags & FLAGS_A2F) ? " [A2F]" : "",
> + (flags & FLAGS_BSF) ? " [BSF]" : "",
> + (flags & FLAGS_TSR1F) ? " [TSR1F]" : "",
> + (flags & FLAGS_TSR2F) ? " [TSR2F]" : "",
> + (flags & FLAGS_TSR3F) ? " [TSR3F]" : "",
> + (flags & FLAGS_WDF) ? " [WDF]" : "");
> + }
> +
> if (flags & FLAGS_A1F) {
> rtc_update_irq(pcf85363->rtc, 1, RTC_IRQF | RTC_AF);
> regmap_update_bits(pcf85363->regmap, CTRL_FLAGS, FLAGS_A1F, 0);
> - return IRQ_HANDLED;
> + handled = true;
> + }
> +
> + if (flags & FLAGS_A2F) {
> + rtc_update_irq(pcf85363->rtc, 1, RTC_IRQF | RTC_AF);
> + regmap_update_bits(pcf85363->regmap, CTRL_FLAGS, FLAGS_A2F, 0);
> + handled = true;
> + }
> +
> + if (flags & FLAGS_BSF) {
> + regmap_update_bits(pcf85363->regmap, CTRL_FLAGS, FLAGS_BSF, 0);
> + handled = true;
> + }
> +
> + if (flags & FLAGS_TSR1F) {
> + regmap_update_bits(pcf85363->regmap, CTRL_FLAGS, FLAGS_TSR1F, 0);
> + pcf85363->ts_valid_flags |= FLAGS_TSR1F;
> + handled = true;
> + }
> +
> + if (flags & FLAGS_TSR2F) {
> + regmap_update_bits(pcf85363->regmap, CTRL_FLAGS, FLAGS_TSR2F, 0);
> + pcf85363->ts_valid_flags |= FLAGS_TSR2F;
> + handled = true;
> + }
> +
> + if (flags & FLAGS_TSR3F) {
> + regmap_update_bits(pcf85363->regmap, CTRL_FLAGS, FLAGS_TSR3F, 0);
> + pcf85363->ts_valid_flags |= FLAGS_TSR3F;
> + handled = true;
> + }
> +
> + if (flags & FLAGS_WDF) {
> + regmap_update_bits(pcf85363->regmap, CTRL_FLAGS, FLAGS_WDF, 0);
> + handled = true;
> + }
> +
> + return handled ? IRQ_HANDLED : IRQ_NONE;
> +}
> +
> +/*
> + * Read the current RTC offset from the CTRL_OFFSET
> + * register. This value is an 8-bit signed 2's complement
> + * value that corrects osciallator drift.
> + */
> +static int pcf85363_read_offset(struct device *dev, long *offset)
> +{
> + struct pcf85363 *pcf85363 = dev_get_drvdata(dev);
> + unsigned int val;
> + int ret;
> +
> + ret = regmap_read(pcf85363->regmap, CTRL_OFFSET, &val);
> +
> + if (ret)
> + return ret;
> +
> + *offset = sign_extend32(val & OFFSET_MASK, OFFSET_SIGN_BIT);
> +
> + return 0;
> +}
> +
> +/*
> + * Write an oscillator offset correction value to
> + * the CTRL_OFFSET register. The valid range is
> + * -128 to 127 (8-bit signed), typically used to fine
> + * tune accuracy.
> + */
> +static int pcf85363_set_offset(struct device *dev, long offset)
> +{
> + struct pcf85363 *pcf85363 = dev_get_drvdata(dev);
> +
> + if (offset < OFFSET_MINIMUM || offset > OFFSET_MAXIMUM) {
> + dev_warn(dev, "Offset out of range: %ld\n", offset);
> + return -ERANGE;
> + }
> +
> + return regmap_write(pcf85363->regmap, CTRL_OFFSET, offset & OFFSET_MASK);
> +}
> +
> +static int pcf85363_rtc_ioctl(struct device *dev,
> + unsigned int cmd, unsigned long arg)
> +{
> + struct pcf85363 *pcf85363 = dev_get_drvdata(dev);
> + unsigned int val;
> + int ret;
> +
> + switch (cmd) {
> + case RTC_VL_READ: {
> + u32 status = 0;
> +
> + ret = regmap_read(pcf85363->regmap, CTRL_FLAGS, &val);
> +
> + if (ret)
> + return ret;
> +
> + if (val & FLAGS_BSF)
> + status |= RTC_VL_BACKUP_SWITCH;
> +
> + return put_user(status, (u32 __user *)arg);
> }
>
> - return IRQ_NONE;
> + case RTC_VL_CLR:
> + return regmap_update_bits(pcf85363->regmap, CTRL_FLAGS, FLAGS_BSF, 0);
> +
> + default:
> + return -ENOIOCTLCMD;
> + }
> }
>
> static const struct rtc_class_ops rtc_ops = {
> + .ioctl = pcf85363_rtc_ioctl,
> .read_time = pcf85363_rtc_read_time,
> .set_time = pcf85363_rtc_set_time,
> .read_alarm = pcf85363_rtc_read_alarm,
> .set_alarm = pcf85363_rtc_set_alarm,
> .alarm_irq_enable = pcf85363_rtc_alarm_irq_enable,
> + .read_offset = pcf85363_read_offset,
> + .set_offset = pcf85363_set_offset,
> };
>
> static int pcf85363_nvram_read(void *priv, unsigned int offset, void *val,
> @@ -379,11 +544,297 @@ static const struct pcf85x63_config pcf_85363_config = {
> .num_nvram = 2
> };
>
> +/*
> + * This function sets the watchdog control register based on the timeout,
> + * clock selection and repeat mode settings. It prepares the value to
> + * write into the watchdog control register (CTRL_WDOG).
> + */
> +static int pcf85363_wdt_reload(struct pcf85363_watchdog *wd)
> +{
> + u8 val;
> +
> + val = (wd->repeat ? WD_MODE_REPEAT : 0) |
> + ((wd->timeout_val & WD_TIMEOUT_MAX) << WD_TIMEOUT_SHIFT) |
> + (wd->clock_sel & WD_CLKSEL_MASK);
> +
> + return regmap_write(wd->regmap, CTRL_WDOG, val);
> +}
> +
> +static int pcf85363_wdt_start(struct watchdog_device *wdd)
> +{
> + struct pcf85363_watchdog *wd = watchdog_get_drvdata(wdd);
> +
> + return pcf85363_wdt_reload(wd);
> +}
> +
> +static int pcf85363_wdt_stop(struct watchdog_device *wdd)
> +{
> + struct pcf85363_watchdog *wd = watchdog_get_drvdata(wdd);
> +
> + return regmap_write(wd->regmap, CTRL_WDOG, 0);
> +}
> +
> +static int pcf85363_wdt_ping(struct watchdog_device *wdd)
> +{
> + struct pcf85363_watchdog *wd = watchdog_get_drvdata(wdd);
> +
> + regmap_update_bits(wd->regmap, CTRL_FLAGS, FLAGS_WDF, 0);
> +
> + return pcf85363_wdt_reload(wd);
> +}
> +
> +static int pcf85363_wdt_set_timeout(struct watchdog_device *wdd,
> + unsigned int timeout)
> +{
> + struct pcf85363_watchdog *wd = watchdog_get_drvdata(wdd);
> +
> + wd->timeout_val = clamp(timeout, WD_TIMEOUT_MIN, WD_TIMEOUT_MAX);
> + wdd->timeout = wd->timeout_val;
> +
> + return pcf85363_wdt_reload(wd);
> +}
> +
> +static const struct watchdog_info pcf85363_wdt_info = {
> + .identity = "PCF85363 Watchdog",
> + .options = WDIOF_KEEPALIVEPING | WDIOF_SETTIMEOUT,
> +};
> +
> +static const struct watchdog_ops pcf85363_wdt_ops = {
> + .owner = THIS_MODULE,
> + .start = pcf85363_wdt_start,
> + .stop = pcf85363_wdt_stop,
> + .ping = pcf85363_wdt_ping,
> + .set_timeout = pcf85363_wdt_set_timeout,
> +};
> +
> +static int pcf85363_watchdog_init(struct device *dev, struct regmap *regmap)
> +{
> + struct pcf85363_watchdog *wd;
> + unsigned int timeout_sec;
> + int ret;
> +
> + if (!IS_ENABLED(CONFIG_WATCHDOG))
> + return 0;
> +
> + wd = devm_kzalloc(dev, sizeof(*wd), GFP_KERNEL);
> + if (!wd)
> + return -ENOMEM;
> +
> + wd->regmap = regmap;
> + wd->dev = dev;
> +
> + wd->wdd.info = &pcf85363_wdt_info;
> + wd->wdd.ops = &pcf85363_wdt_ops;
> + wd->wdd.min_timeout = WD_TIMEOUT_MIN;
> + wd->wdd.max_timeout = WD_TIMEOUT_MAX;
> + wd->wdd.parent = dev;
> + wd->wdd.status = WATCHDOG_NOWAYOUT_INIT_STATUS;
> +
> + ret = watchdog_init_timeout(&wd->wdd, 10, dev);
> + if (ret)
> + wd->wdd.timeout = clamp(10U, WD_TIMEOUT_MIN, WD_TIMEOUT_MAX);
> +
> + timeout_sec = wd->wdd.timeout;
> +
> + if (timeout_sec <= 2)
> + wd->clock_sel = WD_CLKSEL_16HZ;
> + else if (timeout_sec <= 8)
> + wd->clock_sel = WD_CLKSEL_4HZ;
> + else if (timeout_sec <= 16)
> + wd->clock_sel = WD_CLKSEL_1HZ;
> + else
> + wd->clock_sel = WD_CLKSEL_0_25HZ;
> +
> + wd->repeat = true;
> +
> + ret = regmap_update_bits(regmap, CTRL_FLAGS, FLAGS_WDF, 0);
> + if (ret) {
> + dev_err(dev, "failed to clear WDF:%d\n", ret);
> + return ret;
> + }
> +
> + watchdog_set_drvdata(&wd->wdd, wd);
> +
> + dev_dbg(dev, "pcf85363 watchdog registered (timeout=%us, clk_sel=%u)\n",
> + timeout_sec, wd->clock_sel);
> +
> + return devm_watchdog_register_device(dev, &wd->wdd);
> +}
> +
> +/*
> + * Parses a string in the format "min hour weekday", validates the values,
> + * converts them to BCD, writes them to the Alarm2 registers, and enables
> + * the Alarm2 time match bits (minute, hour, weekday).
> + */
> +static ssize_t alarm2_time_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct pcf85363 *pcf85363 = dev_get_drvdata(dev);
> + int min, hour, weekday;
> + u8 regbuf[3];
> + int ret;
> +
> + if (sscanf(buf, "%d %d %d", &min, &hour, &weekday) != 3)
> + return -EINVAL;
> +
> + if (min < 0 || min > 59 || hour < 0 || hour > 23 || weekday < 0 || weekday > 6)
> + return -EINVAL;
> +
> + regbuf[0] = bin2bcd(min);
> + regbuf[1] = bin2bcd(hour);
> + regbuf[2] = weekday & 0x07;
> +
> + ret = regmap_bulk_write(pcf85363->regmap, DT_MINUTE_ALM2, regbuf, sizeof(regbuf));
> + if (ret)
> + return ret;
> +
> + ret = regmap_update_bits(pcf85363->regmap, DT_ALARM_EN,
> + ALRM_MIN_A2E | ALRM_HR_A2E | ALRM_DAY_A2E,
> + ALRM_MIN_A2E | ALRM_HR_A2E | ALRM_DAY_A2E);
> + if (ret)
> + return ret;
> +
> + return count;
> +}
> +
> +/*
> + * Parses a string ("0" or "1") to control Alarm2 interrupt generation.
> + * Also clears the Alarm2 flag if the alarm is being disabled.
> + */
> +static ssize_t alarm2_enable_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct pcf85363 *pcf85363 = dev_get_drvdata(dev);
> + unsigned long enable;
> + int ret;
> +
> + ret = kstrtoul(buf, 10, &enable);
> + if (ret)
> + return ret;
> +
> + if (enable) {
> + ret = regmap_update_bits(pcf85363->regmap, CTRL_INTA_EN,
> + INT_A2IE, INT_A2IE);
> + } else {
> + ret = regmap_update_bits(pcf85363->regmap, CTRL_INTA_EN,
> + INT_A2IE, 0);
> + if (ret)
> + return ret;
> +
> + ret = regmap_update_bits(pcf85363->regmap, CTRL_FLAGS,
> + FLAGS_A2F, 0);
> + }
> +
> + if (ret)
> + return ret;
> +
> + return count;
> +}
> +
> +static DEVICE_ATTR_WO(alarm2_time);
> +static DEVICE_ATTR_WO(alarm2_enable);
> +
> +static struct attribute *alarm2_attrs[] = {
> + &dev_attr_alarm2_time.attr,
> + &dev_attr_alarm2_enable.attr,
> + NULL
> +};
> +
> +static struct attribute_group alarm2_group = {
> + .name = "alarm2",
> + .attrs = alarm2_attrs,
> +};
> +
> +/*
> + * Reads 6 bytes of timestamp data starting at the given base register,
> + * converts them from BCD to binary, and formats the result into a
> + * human-readable string in "YYYY-MM-DD HH:MM:SS" format.
> + */
> +static int pcf85363_read_timestamp(struct pcf85363 *pcf85363, u8 base_reg, char *buf)
> +{
> + struct rtc_time tm;
> + u8 regs[6];
> + int ret;
> +
> + ret = regmap_bulk_read(pcf85363->regmap, base_reg, regs, sizeof(regs));
> +
> + if (ret)
> + return ret;
> +
> + tm.tm_sec = bcd2bin(regs[0]);
> + tm.tm_min = bcd2bin(regs[1]);
> + tm.tm_hour = bcd2bin(regs[2]);
> + tm.tm_mday = bcd2bin(regs[3]);
> + tm.tm_mon = bcd2bin(regs[4]) - 1;
> + tm.tm_year = bcd2bin(regs[5]) + 100;
> +
> + return sysfs_emit(buf, "%04d-%02d-%02d %02d:%02d:%02d\n",
> + tm.tm_year + 1900, tm.tm_mon + 1, tm.tm_mday,
> + tm.tm_hour, tm.tm_min, tm.tm_sec);
> +}
> +
> +/*
> + * Checks whether a specific timestamp flag is set. If so, reads and
> + * returns the formatted timestamp. Otherwise, returns "00-00-00 00:00:00".
> + */
> +
> +static ssize_t pcf85363_timestamp_show(struct device *dev, char *buf,
> + u8 timestamp_flag, u8 base_reg)
> +{
> + struct pcf85363 *pcf85363 = dev_get_drvdata(dev);
> +
> + if (!(pcf85363->ts_valid_flags & timestamp_flag))
> + return sysfs_emit(buf, "00-00-00 00:00:00\n");
> +
> + return pcf85363_read_timestamp(pcf85363, base_reg, buf);
> +}
> +
> +static ssize_t timestamp1_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + return pcf85363_timestamp_show(dev, buf, FLAGS_TSR1F, DT_TIMESTAMP1);
> +}
> +static DEVICE_ATTR_RO(timestamp1);
> +
> +static ssize_t timestamp2_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + return pcf85363_timestamp_show(dev, buf, FLAGS_TSR2F, DT_TIMESTAMP2);
> +}
> +static DEVICE_ATTR_RO(timestamp2);
> +
> +static ssize_t timestamp3_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + return pcf85363_timestamp_show(dev, buf, FLAGS_TSR3F, DT_TIMESTAMP3);
> +}
> +static DEVICE_ATTR_RO(timestamp3);
> +
> +static struct attribute *pcf85363_attrs[] = {
> + &dev_attr_timestamp1.attr,
> + &dev_attr_timestamp2.attr,
> + &dev_attr_timestamp3.attr,
> + NULL,
> +};
> +
> +static const struct attribute_group pcf85363_attr_group = {
> + .attrs = pcf85363_attrs,
> +};
> +
> static int pcf85363_probe(struct i2c_client *client)
> {
> - struct pcf85363 *pcf85363;
> const struct pcf85x63_config *config = &pcf_85363_config;
> const void *data = of_device_get_match_data(&client->dev);
> + struct device *dev = &client->dev;
> + struct pcf85363 *pcf85363;
> + int irq_a = client->irq;
> + bool wakeup_source;
> + int ret, i, err;
> + u32 tsr_mode[3];
> + u8 val;
> +
> static struct nvmem_config nvmem_cfg[] = {
> {
> .name = "pcf85x63-",
> @@ -401,25 +852,43 @@ static int pcf85363_probe(struct i2c_client *client)
> .reg_write = pcf85363_nvram_write,
> },
> };
> - int ret, i, err;
> - bool wakeup_source;
>
> if (data)
> config = data;
>
> - pcf85363 = devm_kzalloc(&client->dev, sizeof(struct pcf85363),
> - GFP_KERNEL);
> + pcf85363 = devm_kzalloc(&client->dev, sizeof(*pcf85363), GFP_KERNEL);
> if (!pcf85363)
> return -ENOMEM;
>
> + pcf85363->ts_valid_flags = 0;
> +
> pcf85363->regmap = devm_regmap_init_i2c(client, &config->regmap);
> - if (IS_ERR(pcf85363->regmap)) {
> - dev_err(&client->dev, "regmap allocation failed\n");
> - return PTR_ERR(pcf85363->regmap);
> - }
> + if (IS_ERR(pcf85363->regmap))
> + return dev_err_probe(dev, PTR_ERR(pcf85363->regmap), "regmap init failed\n");
>
> i2c_set_clientdata(client, pcf85363);
>
> + ret = regmap_update_bits(pcf85363->regmap, CTRL_FUNCTION, RTCM_BIT, 0);
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to enable RTC mode\n");
> +
> + if (!device_property_read_u32_array(dev, "nxp,timestamp-mode", tsr_mode, 3)) {
> + tsr_mode[0] &= TSR1_MASK;
> + tsr_mode[1] &= TSR2_MASK;
> + tsr_mode[2] &= TSR3_MASK;
> +
> + val = (tsr_mode[2] << TSR3_SHIFT) |
> + (tsr_mode[1] << TSR2_SHIFT) |
> + (tsr_mode[0] << TSR1_SHIFT);
> +
> + ret = regmap_write(pcf85363->regmap, DT_TS_MODE, val);
> + if (ret)
> + dev_warn(dev, "Failed to write timestamp mode register\n");
> +
> + dev_dbg(dev, "Timestamp mode set: TSR1=0x%x TSR2=0x%x TSR3=0x%x\n",
> + tsr_mode[0], tsr_mode[1], tsr_mode[2]);
> + }
> +
> pcf85363->rtc = devm_rtc_allocate_device(&client->dev);
> if (IS_ERR(pcf85363->rtc))
> return PTR_ERR(pcf85363->rtc);
> @@ -433,39 +902,57 @@ static int pcf85363_probe(struct i2c_client *client)
> pcf85363->rtc->range_min = RTC_TIMESTAMP_BEGIN_2000;
> pcf85363->rtc->range_max = RTC_TIMESTAMP_END_2099;
>
> - wakeup_source = device_property_read_bool(&client->dev,
> - "wakeup-source");
> - if (client->irq > 0 || wakeup_source) {
> - regmap_write(pcf85363->regmap, CTRL_FLAGS, 0);
> - regmap_update_bits(pcf85363->regmap, CTRL_PIN_IO,
> - PIN_IO_INTAPM, PIN_IO_INTA_OUT);
> - }
> + wakeup_source = device_property_read_bool(dev, "wakeup-source");
>
> - if (client->irq > 0) {
> - unsigned long irqflags = IRQF_TRIGGER_LOW;
> + ret = regmap_write(pcf85363->regmap, CTRL_FLAGS, 0x00);
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to clear CTRL_FLAGS\n");
> +
> + if (irq_a > 0) {
> + regmap_update_bits(pcf85363->regmap, CTRL_PIN_IO, PIN_IO_INTAPM, PIN_IO_INTA_OUT);
> + ret = devm_request_threaded_irq(dev, irq_a, NULL,
> + pcf85363_rtc_handle_irq,
> + IRQF_TRIGGER_LOW | IRQF_ONESHOT,
> + "pcf85363-inta", client);
>
> - if (dev_fwnode(&client->dev))
> - irqflags = 0;
> - ret = devm_request_threaded_irq(&client->dev, client->irq,
> - NULL, pcf85363_rtc_handle_irq,
> - irqflags | IRQF_ONESHOT,
> - "pcf85363", client);
> if (ret) {
> - dev_warn(&client->dev,
> - "unable to request IRQ, alarms disabled\n");
> - client->irq = 0;
> + dev_err_probe(dev, ret, "INTA IRQ request failed\n");
> + irq_a = 0;
> + } else {
> + regmap_write(pcf85363->regmap, CTRL_INTA_EN, INT_BSIE
> + | INT_TSRIE | INT_WDIE);
> }
> }
>
> - if (client->irq > 0 || wakeup_source) {
> - device_init_wakeup(&client->dev, true);
> - set_bit(RTC_FEATURE_ALARM, pcf85363->rtc->features);
> - } else {
> - clear_bit(RTC_FEATURE_ALARM, pcf85363->rtc->features);
> - }
> + regmap_update_bits(pcf85363->regmap, CTRL_PIN_IO,
> + PIN_IO_TSPM | PIN_IO_TSIM,
> + PIN_IO_TSPM | PIN_IO_TSIM);
> +
> + ret = pcf85363_watchdog_init(dev, pcf85363->regmap);
> +
> + if (ret)
> + dev_err_probe(dev, ret, "Watchdog init failed\n");
> +
> + if (irq_a > 0 || wakeup_source)
> + device_init_wakeup(dev, true);
> +
> + dev_set_drvdata(&pcf85363->rtc->dev, pcf85363);
>
> ret = devm_rtc_register_device(pcf85363->rtc);
>
> + if (ret)
> + return dev_err_probe(dev, ret, "RTC registration failed\n");
> +
> + ret = sysfs_create_group(&pcf85363->rtc->dev.kobj, &alarm2_group);
> +
> + if (ret)
> + dev_err_probe(dev, ret, "Alarm2 sysfs creation failed\n");
> +
> + ret = sysfs_create_group(&pcf85363->rtc->dev.kobj, &pcf85363_attr_group);
Use rtc_add_group(), also, you are not allowed to fail after calling
devm_rtc_register_device)()
> +
> + if (ret)
> + dev_err_probe(dev, ret, "Timestamp sysfs creation failed\n");
> +
> for (i = 0; i < config->num_nvram; i++) {
> nvmem_cfg[i].priv = pcf85363;
> devm_rtc_nvmem_register(pcf85363->rtc, &nvmem_cfg[i]);
> --
> 2.25.1
>
--
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply
* Re: [PATCH 2/3] rtc: s5m: query platform device IRQ resource for alarm IRQ
From: Alexandre Belloni @ 2025-11-14 15:58 UTC (permalink / raw)
To: André Draszik
Cc: Krzysztof Kozlowski, Lee Jones, Peter Griffin, Tudor Ambarus,
Will McVicker, Juan Yescas, Douglas Anderson, kernel-team,
Kaustabh Chakraborty, linux-kernel, linux-samsung-soc, linux-rtc
In-Reply-To: <20251114-s5m-alarm-v1-2-c9b3bebae65f@linaro.org>
On 14/11/2025 11:47:22+0000, André Draszik wrote:
> The core driver now exposes the alarm IRQ as a resource, so we can drop
> the lookup from here to simplify the code and make adding support for
> additional variants easier in this driver.
>
> Signed-off-by: André Draszik <andre.draszik@linaro.org>
Acked-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
> ---
> drivers/rtc/rtc-s5m.c | 21 ++++++++-------------
> 1 file changed, 8 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/rtc/rtc-s5m.c b/drivers/rtc/rtc-s5m.c
> index a7220b4d0e8dd35786b060e2a4106e2a39fe743f..c6ed5a4ca8a0e4554b1c88c879b01fc384735007 100644
> --- a/drivers/rtc/rtc-s5m.c
> +++ b/drivers/rtc/rtc-s5m.c
> @@ -15,7 +15,6 @@
> #include <linux/rtc.h>
> #include <linux/platform_device.h>
> #include <linux/mfd/samsung/core.h>
> -#include <linux/mfd/samsung/irq.h>
> #include <linux/mfd/samsung/rtc.h>
> #include <linux/mfd/samsung/s2mps14.h>
>
> @@ -683,22 +682,18 @@ static int s5m_rtc_probe(struct platform_device *pdev)
> case S2MPS15X:
> regmap_cfg = &s2mps14_rtc_regmap_config;
> info->regs = &s2mps15_rtc_regs;
> - alarm_irq = S2MPS14_IRQ_RTCA0;
> break;
> case S2MPS14X:
> regmap_cfg = &s2mps14_rtc_regmap_config;
> info->regs = &s2mps14_rtc_regs;
> - alarm_irq = S2MPS14_IRQ_RTCA0;
> break;
> case S2MPS13X:
> regmap_cfg = &s2mps14_rtc_regmap_config;
> info->regs = &s2mps13_rtc_regs;
> - alarm_irq = S2MPS14_IRQ_RTCA0;
> break;
> case S5M8767X:
> regmap_cfg = &s5m_rtc_regmap_config;
> info->regs = &s5m_rtc_regs;
> - alarm_irq = S5M8767_IRQ_RTCA1;
> break;
> default:
> return dev_err_probe(&pdev->dev, -ENODEV,
> @@ -719,7 +714,6 @@ static int s5m_rtc_probe(struct platform_device *pdev)
> "Failed to allocate regmap\n");
> } else if (device_type == S2MPG10) {
> info->regs = &s2mpg10_rtc_regs;
> - alarm_irq = S2MPG10_IRQ_RTCA0;
> } else {
> return dev_err_probe(&pdev->dev, -ENODEV,
> "Unsupported device type %d\n",
> @@ -730,13 +724,14 @@ static int s5m_rtc_probe(struct platform_device *pdev)
> info->s5m87xx = s5m87xx;
> info->device_type = device_type;
>
> - if (s5m87xx->irq_data) {
> - info->irq = regmap_irq_get_virq(s5m87xx->irq_data, alarm_irq);
> - if (info->irq <= 0)
> - return dev_err_probe(&pdev->dev, -EINVAL,
> - "Failed to get virtual IRQ %d\n",
> - alarm_irq);
> - }
> + alarm_irq = platform_get_irq_byname_optional(pdev, "alarm");
> + if (alarm_irq > 0)
> + info->irq = alarm_irq;
> + else if (alarm_irq == -ENXIO)
> + info->irq = 0;
> + else
> + return dev_err_probe(&pdev->dev, alarm_irq ? : -EINVAL,
> + "IRQ 'alarm' not found\n");
>
> platform_set_drvdata(pdev, info);
>
>
> --
> 2.52.0.rc1.455.g30608eb744-goog
>
--
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply
* Re: [PATCH v4 14/16] power: supply: bd71828: Support wider register addresses
From: Matti Vaittinen @ 2025-11-14 13:22 UTC (permalink / raw)
To: Andreas Kemnade, Matti Vaittinen
Cc: Matti Vaittinen, Lee Jones, Pavel Machek, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Sebastian Reichel,
Liam Girdwood, Mark Brown, Michael Turquette, Stephen Boyd,
Linus Walleij, Bartosz Golaszewski, Alexandre Belloni, linux-leds,
devicetree, linux-kernel, linux-pm, linux-clk, linux-gpio,
linux-rtc
In-Reply-To: <20251114121509.629d171b@kemnade.info>
Thanks Andreas,
On 14/11/2025 13:15, Andreas Kemnade wrote:
> On Thu, 13 Nov 2025 10:55:39 +0200
> Matti Vaittinen <matti.vaittinen@linux.dev> wrote:
>
>> As a side note, we can reduce the "wasted space / member / instance" from
>> 3 bytes to 1 byte, by using u16 instead of the unsigned int if needed. I
>> rather use unsigned int to be initially prepared for devices with 32 bit
>> registers if there is no need to count bytes.
>
> Well, this is totally internal to the module, so no ABI/API changes, so
> there is no advantage of using 32bit now I think. We can switch any time.
The only advantage is to avoid the churn if 32bit ICs are to be added.
> But we have 32bit stuff in the regmap cache anyways, so that is not above
> the general level of wasting space.
Exactly. And, I am not sure if sparing ~hundred bytes is worth the
hassle - even if it is hassle internal to the driver. But yeah, we can
squeeze a few bytes if it is seen beneficial. That's why I mentioned it
here :)
Yours,
-- Matti
---
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
^ permalink raw reply
* Re: [PATCH 08/13] mfd: sec: store hardware revision in sec_pmic_dev and add S2MU005 support
From: André Draszik @ 2025-11-14 13:14 UTC (permalink / raw)
To: Kaustabh Chakraborty, Lee Jones, Pavel Machek, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, MyungJoo Ham, Chanwoo Choi,
Sebastian Reichel, Krzysztof Kozlowski, Alexandre Belloni,
Jonathan Corbet
Cc: linux-leds, devicetree, linux-kernel, linux-pm, linux-samsung-soc,
linux-rtc, linux-doc
In-Reply-To: <20251114-s2mu005-pmic-v1-8-9e3184d3a0c9@disroot.org>
On Fri, 2025-11-14 at 00:35 +0530, Kaustabh Chakraborty wrote:
> The device revision matters in cases when in some PMICs, the correct
> register offsets very in different revisions. Instead of just debug
> printing the value, store it in the driver data struct.
>
> Unlike other devices, S2MU005 has its hardware revision ID in register
> offset 0x73. Allow handling different devices and add support for S2MU005.
>
> Signed-off-by: Kaustabh Chakraborty <kauschluss@disroot.org>
> ---
> drivers/mfd/sec-common.c | 30 ++++++++++++++++++++++++------
> include/linux/mfd/samsung/core.h | 3 +++
> 2 files changed, 27 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/mfd/sec-common.c b/drivers/mfd/sec-common.c
> index 4c5f4dc2905b..f51c53e7a164 100644
> --- a/drivers/mfd/sec-common.c
> +++ b/drivers/mfd/sec-common.c
> @@ -16,6 +16,7 @@
> #include <linux/mfd/samsung/irq.h>
> #include <linux/mfd/samsung/s2mps11.h>
> #include <linux/mfd/samsung/s2mps13.h>
> +#include <linux/mfd/samsung/s2mu005.h>
> #include <linux/module.h>
> #include <linux/of.h>
> #include <linux/pm.h>
> @@ -86,17 +87,34 @@ static const struct mfd_cell s2mu005_devs[] = {
> MFD_CELL_OF("s2mu005-rgb", NULL, NULL, 0, 0, "samsung,s2mu005-rgb"),
> };
>
> -static void sec_pmic_dump_rev(struct sec_pmic_dev *sec_pmic)
> +static void sec_pmic_store_rev(struct sec_pmic_dev *sec_pmic)
> {
> - unsigned int val;
> + unsigned int reg, mask, shift;
>
> /* For s2mpg1x, the revision is in a different regmap */
> if (sec_pmic->device_type == S2MPG10)
> return;
>
> - /* For each device type, the REG_ID is always the first register */
> - if (!regmap_read(sec_pmic->regmap_pmic, S2MPS11_REG_ID, &val))
> - dev_dbg(sec_pmic->dev, "Revision: 0x%x\n", val);
> + switch (sec_pmic->device_type) {
> + case S2MU005:
> + reg = S2MU005_REG_ID;
> + mask = S2MU005_ID_MASK;
> + shift = S2MU005_ID_SHIFT;
> + break;
> + default:
> + /* For other device types, the REG_ID is always the first register. */
> + reg = S2MPS11_REG_ID;
> + mask = ~0;
> + shift = 0;
> + }
> +
> + if (!regmap_read(sec_pmic->regmap_pmic, reg, &sec_pmic->revision))
> + return;
You should probably propagate the error up to the caller here.
Cheers,
Andre'
> +
> + sec_pmic->revision &= mask;
> + sec_pmic->revision >>= shift;
> +
> + dev_dbg(sec_pmic->dev, "Revision: 0x%x\n", sec_pmic->revision);
> }
>
> static void sec_pmic_configure(struct sec_pmic_dev *sec_pmic)
> @@ -236,7 +254,7 @@ int sec_pmic_probe(struct device *dev, int device_type, unsigned int irq,
> return ret;
>
> sec_pmic_configure(sec_pmic);
> - sec_pmic_dump_rev(sec_pmic);
> + sec_pmic_store_rev(sec_pmic);
>
> return ret;
> }
> diff --git a/include/linux/mfd/samsung/core.h b/include/linux/mfd/samsung/core.h
> index fc07f7944dcd..ccd1bfa15b85 100644
> --- a/include/linux/mfd/samsung/core.h
> +++ b/include/linux/mfd/samsung/core.h
> @@ -63,6 +63,7 @@ enum sec_device_type {
> * @irq_base: Base IRQ number for device, required for IRQs
> * @irq: Generic IRQ number for device
> * @irq_data: Runtime data structure for IRQ controller
> + * @revision: Revision number of the device
> * @wakeup: Whether or not this is a wakeup device
> */
> struct sec_pmic_dev {
> @@ -74,6 +75,8 @@ struct sec_pmic_dev {
> int device_type;
> int irq;
> struct regmap_irq_chip_data *irq_data[MAX_IRQ_CHIPS];
> +
> + unsigned int revision;
> };
>
> struct sec_platform_data {
^ permalink raw reply
* Re: [PATCH 06/13] mfd: sec-irq: add support for creating multiple IRQ chips
From: André Draszik @ 2025-11-14 11:55 UTC (permalink / raw)
To: Alexandre Belloni, Kaustabh Chakraborty
Cc: Lee Jones, Pavel Machek, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, MyungJoo Ham, Chanwoo Choi, Sebastian Reichel,
Krzysztof Kozlowski, Jonathan Corbet, linux-leds, devicetree,
linux-kernel, linux-pm, linux-samsung-soc, linux-rtc, linux-doc
In-Reply-To: <20251114075004a444bff0@mail.local>
On Fri, 2025-11-14 at 08:50 +0100, Alexandre Belloni wrote:
> On 14/11/2025 00:35:07+0530, Kaustabh Chakraborty wrote:
> > The current state of the driver only allows creating only one IRQ chip
> > per PMIC. On some PMICs, such as Samsung's S2MU005, there are multiple
> > interrupt blocks, for which the current implementation stands insufficient.
> >
> > Add support for creating multiple IRQ chips for a PMIC. Every IRQ chip is
> > given it's own index, which is used by sub-device drivers to request IRQs.
> >
> > A macro is defined which states the maximum number of chips supported.
> > It's set to 1 as currently, no PMIC requires more than one IRQ chip. The
> > value must be changed accordingly on adding new PMICs requiring multiple
> > IRQ chips.
> >
> > Moreover, adjust the s5m RTC driver to initialize IRQs with the
> > appropriate IRQ chip index.
> >
> > Signed-off-by: Kaustabh Chakraborty <kauschluss@disroot.org>
> Acked-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
>
> > ---
> > drivers/mfd/sec-irq.c | 163 +++++++++++++++++++++++----------------
> > drivers/rtc/rtc-s5m.c | 15 +++-
> > include/linux/mfd/samsung/core.h | 5 +-
> > include/linux/mfd/samsung/irq.h | 14 ++++
> > 4 files changed, 127 insertions(+), 70 deletions(-)
Your patch reminded me to finally send
https://lore.kernel.org/all/20251114-s5m-alarm-v1-0-c9b3bebae65f@linaro.org/
If applied first, you wouldn't need to touch rtc-s5m.c I believe.
Equally, I can rebase mine on top of yours - no strong feelings.
Cheers,
Andre'
^ permalink raw reply
* [PATCH 3/3] mfd: sec: drop now unused struct sec_pmic_dev::irq_data
From: André Draszik @ 2025-11-14 11:47 UTC (permalink / raw)
To: Krzysztof Kozlowski, Lee Jones, Alexandre Belloni
Cc: Peter Griffin, Tudor Ambarus, Will McVicker, Juan Yescas,
Douglas Anderson, kernel-team, Kaustabh Chakraborty, linux-kernel,
linux-samsung-soc, linux-rtc, André Draszik
In-Reply-To: <20251114-s5m-alarm-v1-0-c9b3bebae65f@linaro.org>
This was used only to allow the s5m RTC driver to deal with the alarm
IRQ. That driver now uses a different approach to acquire that IRQ, and
::irq_data doesn't need to be kept around anymore.
Signed-off-by: André Draszik <andre.draszik@linaro.org>
---
drivers/mfd/sec-common.c | 5 +++--
drivers/mfd/sec-core.h | 2 +-
drivers/mfd/sec-irq.c | 10 ++--------
include/linux/mfd/samsung/core.h | 1 -
4 files changed, 6 insertions(+), 12 deletions(-)
diff --git a/drivers/mfd/sec-common.c b/drivers/mfd/sec-common.c
index 77370db52a7ba81234136b29f85892f4b197f429..794c4e5398e7dd1a816aff9a6559a6c19fec75a5 100644
--- a/drivers/mfd/sec-common.c
+++ b/drivers/mfd/sec-common.c
@@ -163,6 +163,7 @@ sec_pmic_parse_dt_pdata(struct device *dev)
int sec_pmic_probe(struct device *dev, int device_type, unsigned int irq,
struct regmap *regmap, struct i2c_client *client)
{
+ struct regmap_irq_chip_data *irq_data;
struct sec_platform_data *pdata;
const struct mfd_cell *sec_devs;
struct sec_pmic_dev *sec_pmic;
@@ -187,7 +188,7 @@ int sec_pmic_probe(struct device *dev, int device_type, unsigned int irq,
sec_pmic->pdata = pdata;
- ret = sec_irq_init(sec_pmic);
+ ret = sec_irq_init(sec_pmic, &irq_data);
if (ret)
return ret;
@@ -240,7 +241,7 @@ int sec_pmic_probe(struct device *dev, int device_type, unsigned int irq,
sec_pmic->device_type);
}
ret = devm_mfd_add_devices(sec_pmic->dev, -1, sec_devs, num_sec_devs,
- NULL, 0, regmap_irq_get_domain(sec_pmic->irq_data));
+ NULL, 0, regmap_irq_get_domain(irq_data));
if (ret)
return ret;
diff --git a/drivers/mfd/sec-core.h b/drivers/mfd/sec-core.h
index 92c7558ab8b0de44a52e028eeb7998e38358cb4c..c639180ea686f4308af3f872cb1d2209d201b2e7 100644
--- a/drivers/mfd/sec-core.h
+++ b/drivers/mfd/sec-core.h
@@ -18,6 +18,6 @@ int sec_pmic_probe(struct device *dev, int device_type, unsigned int irq,
struct regmap *regmap, struct i2c_client *client);
void sec_pmic_shutdown(struct device *dev);
-int sec_irq_init(struct sec_pmic_dev *sec_pmic);
+int sec_irq_init(struct sec_pmic_dev *sec_pmic, struct regmap_irq_chip_data **irq_data);
#endif /* __SEC_CORE_INT_H */
diff --git a/drivers/mfd/sec-irq.c b/drivers/mfd/sec-irq.c
index c5c80b1ba104e6c5a55b442d2f10a8554201a961..05d4cc350a351d994e00ba08f5ce966d0d5c6a0b 100644
--- a/drivers/mfd/sec-irq.c
+++ b/drivers/mfd/sec-irq.c
@@ -253,7 +253,7 @@ static const struct regmap_irq_chip s5m8767_irq_chip = {
.ack_base = S5M8767_REG_INT1,
};
-int sec_irq_init(struct sec_pmic_dev *sec_pmic)
+int sec_irq_init(struct sec_pmic_dev *sec_pmic, struct regmap_irq_chip_data **irq_data)
{
const struct regmap_irq_chip *sec_irq_chip;
int ret;
@@ -302,17 +302,11 @@ int sec_irq_init(struct sec_pmic_dev *sec_pmic)
ret = devm_regmap_add_irq_chip(sec_pmic->dev, sec_pmic->regmap_pmic,
sec_pmic->irq, IRQF_ONESHOT,
- 0, sec_irq_chip, &sec_pmic->irq_data);
+ 0, sec_irq_chip, irq_data);
if (ret)
return dev_err_probe(sec_pmic->dev, ret,
"Failed to add %s IRQ chip\n",
sec_irq_chip->name);
- /*
- * The rtc-s5m driver requests S2MPS14_IRQ_RTCA0 also for S2MPS11
- * so the interrupt number must be consistent.
- */
- BUILD_BUG_ON(((enum s2mps14_irq)S2MPS11_IRQ_RTCA0) != S2MPS14_IRQ_RTCA0);
-
return 0;
}
diff --git a/include/linux/mfd/samsung/core.h b/include/linux/mfd/samsung/core.h
index d785e101fe795a5d8f9cccf4ccc4232437e89416..c7c3c8cd8d5f99ef0cc3188e1c3b49031f4750f2 100644
--- a/include/linux/mfd/samsung/core.h
+++ b/include/linux/mfd/samsung/core.h
@@ -69,7 +69,6 @@ struct sec_pmic_dev {
int device_type;
int irq;
- struct regmap_irq_chip_data *irq_data;
};
struct sec_platform_data {
--
2.52.0.rc1.455.g30608eb744-goog
^ permalink raw reply related
* [PATCH 2/3] rtc: s5m: query platform device IRQ resource for alarm IRQ
From: André Draszik @ 2025-11-14 11:47 UTC (permalink / raw)
To: Krzysztof Kozlowski, Lee Jones, Alexandre Belloni
Cc: Peter Griffin, Tudor Ambarus, Will McVicker, Juan Yescas,
Douglas Anderson, kernel-team, Kaustabh Chakraborty, linux-kernel,
linux-samsung-soc, linux-rtc, André Draszik
In-Reply-To: <20251114-s5m-alarm-v1-0-c9b3bebae65f@linaro.org>
The core driver now exposes the alarm IRQ as a resource, so we can drop
the lookup from here to simplify the code and make adding support for
additional variants easier in this driver.
Signed-off-by: André Draszik <andre.draszik@linaro.org>
---
drivers/rtc/rtc-s5m.c | 21 ++++++++-------------
1 file changed, 8 insertions(+), 13 deletions(-)
diff --git a/drivers/rtc/rtc-s5m.c b/drivers/rtc/rtc-s5m.c
index a7220b4d0e8dd35786b060e2a4106e2a39fe743f..c6ed5a4ca8a0e4554b1c88c879b01fc384735007 100644
--- a/drivers/rtc/rtc-s5m.c
+++ b/drivers/rtc/rtc-s5m.c
@@ -15,7 +15,6 @@
#include <linux/rtc.h>
#include <linux/platform_device.h>
#include <linux/mfd/samsung/core.h>
-#include <linux/mfd/samsung/irq.h>
#include <linux/mfd/samsung/rtc.h>
#include <linux/mfd/samsung/s2mps14.h>
@@ -683,22 +682,18 @@ static int s5m_rtc_probe(struct platform_device *pdev)
case S2MPS15X:
regmap_cfg = &s2mps14_rtc_regmap_config;
info->regs = &s2mps15_rtc_regs;
- alarm_irq = S2MPS14_IRQ_RTCA0;
break;
case S2MPS14X:
regmap_cfg = &s2mps14_rtc_regmap_config;
info->regs = &s2mps14_rtc_regs;
- alarm_irq = S2MPS14_IRQ_RTCA0;
break;
case S2MPS13X:
regmap_cfg = &s2mps14_rtc_regmap_config;
info->regs = &s2mps13_rtc_regs;
- alarm_irq = S2MPS14_IRQ_RTCA0;
break;
case S5M8767X:
regmap_cfg = &s5m_rtc_regmap_config;
info->regs = &s5m_rtc_regs;
- alarm_irq = S5M8767_IRQ_RTCA1;
break;
default:
return dev_err_probe(&pdev->dev, -ENODEV,
@@ -719,7 +714,6 @@ static int s5m_rtc_probe(struct platform_device *pdev)
"Failed to allocate regmap\n");
} else if (device_type == S2MPG10) {
info->regs = &s2mpg10_rtc_regs;
- alarm_irq = S2MPG10_IRQ_RTCA0;
} else {
return dev_err_probe(&pdev->dev, -ENODEV,
"Unsupported device type %d\n",
@@ -730,13 +724,14 @@ static int s5m_rtc_probe(struct platform_device *pdev)
info->s5m87xx = s5m87xx;
info->device_type = device_type;
- if (s5m87xx->irq_data) {
- info->irq = regmap_irq_get_virq(s5m87xx->irq_data, alarm_irq);
- if (info->irq <= 0)
- return dev_err_probe(&pdev->dev, -EINVAL,
- "Failed to get virtual IRQ %d\n",
- alarm_irq);
- }
+ alarm_irq = platform_get_irq_byname_optional(pdev, "alarm");
+ if (alarm_irq > 0)
+ info->irq = alarm_irq;
+ else if (alarm_irq == -ENXIO)
+ info->irq = 0;
+ else
+ return dev_err_probe(&pdev->dev, alarm_irq ? : -EINVAL,
+ "IRQ 'alarm' not found\n");
platform_set_drvdata(pdev, info);
--
2.52.0.rc1.455.g30608eb744-goog
^ permalink raw reply related
* [PATCH 1/3] mfd: sec: add rtc alarm IRQ as platform device resource
From: André Draszik @ 2025-11-14 11:47 UTC (permalink / raw)
To: Krzysztof Kozlowski, Lee Jones, Alexandre Belloni
Cc: Peter Griffin, Tudor Ambarus, Will McVicker, Juan Yescas,
Douglas Anderson, kernel-team, Kaustabh Chakraborty, linux-kernel,
linux-samsung-soc, linux-rtc, André Draszik
In-Reply-To: <20251114-s5m-alarm-v1-0-c9b3bebae65f@linaro.org>
By adding the RTC alarm IRQ to the MFD cell as a resource, the child
driver (rtc) can simply query that IRQ, instead of having a lookup
table itself.
This change therefore allows the child driver to be simplified with
regards to determining the alarm IRQ.
Signed-off-by: André Draszik <andre.draszik@linaro.org>
---
drivers/mfd/sec-common.c | 38 +++++++++++++++++++++++++++++---------
1 file changed, 29 insertions(+), 9 deletions(-)
diff --git a/drivers/mfd/sec-common.c b/drivers/mfd/sec-common.c
index 42d55e70e34c8d7cd68cddaecc88017e259365b4..77370db52a7ba81234136b29f85892f4b197f429 100644
--- a/drivers/mfd/sec-common.c
+++ b/drivers/mfd/sec-common.c
@@ -23,9 +23,13 @@
#include <linux/regmap.h>
#include "sec-core.h"
+static const struct resource s5m8767_rtc_resources[] = {
+ DEFINE_RES_IRQ_NAMED(S5M8767_IRQ_RTCA1, "alarm"),
+};
+
static const struct mfd_cell s5m8767_devs[] = {
MFD_CELL_NAME("s5m8767-pmic"),
- MFD_CELL_NAME("s5m-rtc"),
+ MFD_CELL_RES("s5m-rtc", s5m8767_rtc_resources),
MFD_CELL_OF("s5m8767-clk", NULL, NULL, 0, 0, "samsung,s5m8767-clk"),
};
@@ -33,50 +37,66 @@ static const struct mfd_cell s2dos05_devs[] = {
MFD_CELL_NAME("s2dos05-regulator"),
};
+static const struct resource s2mpg10_rtc_resources[] = {
+ DEFINE_RES_IRQ_NAMED(S2MPG10_IRQ_RTCA0, "alarm"),
+};
+
static const struct mfd_cell s2mpg10_devs[] = {
MFD_CELL_NAME("s2mpg10-meter"),
MFD_CELL_NAME("s2mpg10-regulator"),
- MFD_CELL_NAME("s2mpg10-rtc"),
+ MFD_CELL_RES("s2mpg10-rtc", s2mpg10_rtc_resources),
MFD_CELL_OF("s2mpg10-clk", NULL, NULL, 0, 0, "samsung,s2mpg10-clk"),
MFD_CELL_OF("s2mpg10-gpio", NULL, NULL, 0, 0, "samsung,s2mpg10-gpio"),
};
+static const struct resource s2mps11_rtc_resources[] = {
+ DEFINE_RES_IRQ_NAMED(S2MPS11_IRQ_RTCA0, "alarm"),
+};
+
static const struct mfd_cell s2mps11_devs[] = {
MFD_CELL_NAME("s2mps11-regulator"),
- MFD_CELL_NAME("s2mps14-rtc"),
+ MFD_CELL_RES("s2mps14-rtc", s2mps11_rtc_resources),
MFD_CELL_OF("s2mps11-clk", NULL, NULL, 0, 0, "samsung,s2mps11-clk"),
};
+static const struct resource s2mps14_rtc_resources[] = {
+ DEFINE_RES_IRQ_NAMED(S2MPS14_IRQ_RTCA0, "alarm"),
+};
+
static const struct mfd_cell s2mps13_devs[] = {
MFD_CELL_NAME("s2mps13-regulator"),
- MFD_CELL_NAME("s2mps13-rtc"),
+ MFD_CELL_RES("s2mps13-rtc", s2mps14_rtc_resources),
MFD_CELL_OF("s2mps13-clk", NULL, NULL, 0, 0, "samsung,s2mps13-clk"),
};
static const struct mfd_cell s2mps14_devs[] = {
MFD_CELL_NAME("s2mps14-regulator"),
- MFD_CELL_NAME("s2mps14-rtc"),
+ MFD_CELL_RES("s2mps14-rtc", s2mps14_rtc_resources),
MFD_CELL_OF("s2mps14-clk", NULL, NULL, 0, 0, "samsung,s2mps14-clk"),
};
static const struct mfd_cell s2mps15_devs[] = {
MFD_CELL_NAME("s2mps15-regulator"),
- MFD_CELL_NAME("s2mps15-rtc"),
+ MFD_CELL_RES("s2mps15-rtc", s2mps14_rtc_resources),
MFD_CELL_OF("s2mps13-clk", NULL, NULL, 0, 0, "samsung,s2mps13-clk"),
};
static const struct mfd_cell s2mpa01_devs[] = {
MFD_CELL_NAME("s2mpa01-pmic"),
- MFD_CELL_NAME("s2mps14-rtc"),
+ MFD_CELL_RES("s2mps14-rtc", s2mps14_rtc_resources),
};
static const struct mfd_cell s2mpu02_devs[] = {
MFD_CELL_NAME("s2mpu02-regulator"),
};
+static const struct resource s2mpu05_rtc_resources[] = {
+ DEFINE_RES_IRQ_NAMED(S2MPU05_IRQ_RTCA0, "alarm"),
+};
+
static const struct mfd_cell s2mpu05_devs[] = {
MFD_CELL_NAME("s2mpu05-regulator"),
- MFD_CELL_NAME("s2mps15-rtc"),
+ MFD_CELL_RES("s2mps15-rtc", s2mpu05_rtc_resources),
};
static void sec_pmic_dump_rev(struct sec_pmic_dev *sec_pmic)
@@ -220,7 +240,7 @@ int sec_pmic_probe(struct device *dev, int device_type, unsigned int irq,
sec_pmic->device_type);
}
ret = devm_mfd_add_devices(sec_pmic->dev, -1, sec_devs, num_sec_devs,
- NULL, 0, NULL);
+ NULL, 0, regmap_irq_get_domain(sec_pmic->irq_data));
if (ret)
return ret;
--
2.52.0.rc1.455.g30608eb744-goog
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox