* Re: [PATCH v2 2/9] x86/extable: switch to using FIELD_GET_SIGNED()
From: Yury Norov @ 2026-04-29 19:00 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen,
H. Peter Anvin, Andy Lutomirski, Jonathan Cameron, David Lechner,
Johannes Berg, David Laight, Nuno Sá, Andy Shevchenko,
Ping-Ke Shih, Richard Cochran, Andrew Lunn, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Alexandre Belloni,
Yury Norov, Rasmus Villemoes, Hans de Goede, Linus Walleij,
Sakari Ailus, Salah Triki, Achim Gratz, Ben Collins, x86,
linux-kernel, linux-iio, linux-wireless, netdev, linux-rtc
In-Reply-To: <20260428093905.GA1026330@noisy.programming.kicks-ass.net>
On Tue, Apr 28, 2026 at 11:39:05AM +0200, Peter Zijlstra wrote:
> On Mon, Apr 27, 2026 at 05:41:19PM -0400, Yury Norov wrote:
> > The EX_DATA register is laid out such that EX_DATA_IMM occupied MSB.
> > It's done to make sure that FIELD_GET() will sign-extend the IMM
> > field during extraction.
> >
> > To enforce that, all EX_DATA masks are made signed integers. This
> > works, but relies on the particular implementation of FIELD_GET(),
> > i.e. masking then shifting, not vice versa; and the particular
> > placement of the fields in the register.
> >
> > Switch to using the dedicated FIELD_GET_SIGNED(), and relax those
> > limitations.
> >
> > Signed-off-by: Yury Norov <ynorov@nvidia.com>
>
> Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Alright, thanks Peter and everyone. Now that all the patches except #9
are reviewed, I'm taking the series into bitmap-for-next.
Thanks,
Yury
^ permalink raw reply
* Re: [PATCH v5 03/11] dt-bindings: mfd: add documentation for S2MU005 PMIC
From: Krzysztof Kozlowski @ 2026-04-30 10:50 UTC (permalink / raw)
To: Kaustabh Chakraborty
Cc: Lee Jones, Pavel Machek, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, MyungJoo Ham, Chanwoo Choi, Sebastian Reichel,
André Draszik, Alexandre Belloni, Jonathan Corbet,
Shuah Khan, Nam Tran, Łukasz Lebiedziński, linux-leds,
devicetree, linux-kernel, linux-pm, linux-samsung-soc, linux-rtc,
linux-doc
In-Reply-To: <DI5NXS3PQ53R.L9JZEBHW5EGI@disroot.org>
On 29/04/2026 15:12, Kaustabh Chakraborty wrote:
> Hi Krzysztof,
>
> This are no review comments here. Did you happen to miss anything?
Thanks for noticing. I mistyped.
>
> On 2026-04-28 08:01 +02:00, Krzysztof Kozlowski wrote:
>> On Fri, Apr 24, 2026 at 01:09:02AM +0530, Kaustabh Chakraborty wrote:
>>> Samsung's S2MU005 PMIC includes subdevices for a charger, an MUIC (Micro
>>> USB Interface Controller), and flash and RGB LED controllers.
>>>
>>> Add the compatible and documentation for the S2MU005 PMIC. Also, add an
>>> example for nodes for supported sub-devices, i.e. MUIC, flash LEDs, and
>>> RGB LEDs. Charger sub-device uses the node of the parent.
>>>
>>> Signed-off-by: Kaustabh Chakraborty <kauschluss@disroot.org>
>>> ---
>>> .../bindings/mfd/samsung,s2mu005-pmic.yaml | 120 +++++++++++++++++++++
>>> 1 file changed, 120 insertions(+)
Patch looks correct:
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>
Best regards,
Krzysztof
^ permalink raw reply
* [PATCH] selftests: rtc: fix flaky date_read_loop test
From: Wake Liu @ 2026-04-30 12:03 UTC (permalink / raw)
To: Alexandre Belloni, Shuah Khan
Cc: linux-rtc, linux-kselftest, linux-kernel, Wake Liu
The test case rtc.date_read_loop in rtctest.c fails intermittently because
it checks that the RTC time does not advance by more than 1 second per loop
iteration. However, the loop sleeps for 11ms via nanosleep(), and if the
test thread is descheduled by the OS scheduler (e.g., under heavy system
load in a VM), more than 1 second can elapse between consecutive RTC reads.
This causes the next RTC time read to be 2 or more seconds ahead of the
previous read, triggering a test assertion failure.
To make the test more resilient against OS scheduling delays, measure the
real elapsed time between iterations using a monotonic clock
(clock_gettime(CLOCK_MONOTONIC)), and compute the actual number of seconds
elapsed (delta_s) between consecutive RTC reads. Then dynamically adjust
the assertion to:
ASSERT_GE(prev_rtc_read + delta_s + 1, rtc_read);
Signed-off-by: Wake Liu <wakel@google.com>
---
tools/testing/selftests/rtc/rtctest.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/tools/testing/selftests/rtc/rtctest.c b/tools/testing/selftests/rtc/rtctest.c
index 8047d9879039..54eb5c255a45 100644
--- a/tools/testing/selftests/rtc/rtctest.c
+++ b/tools/testing/selftests/rtc/rtctest.c
@@ -116,6 +116,7 @@ TEST_F_TIMEOUT(rtc, date_read_loop, READ_LOOP_DURATION_SEC + 2) {
long iter_count = 0;
struct rtc_time rtc_tm;
time_t start_rtc_read, prev_rtc_read;
+ struct timespec prev_mono, cur_mono;
if (self->fd == -1 && errno == ENOENT)
SKIP(return, "Skipping test since %s does not exist", rtc_file);
@@ -126,25 +127,31 @@ TEST_F_TIMEOUT(rtc, date_read_loop, READ_LOOP_DURATION_SEC + 2) {
rc = ioctl(self->fd, RTC_RD_TIME, &rtc_tm);
ASSERT_NE(-1, rc);
+ clock_gettime(CLOCK_MONOTONIC, &prev_mono);
start_rtc_read = rtc_time_to_timestamp(&rtc_tm);
prev_rtc_read = start_rtc_read;
do {
time_t rtc_read;
+ time_t delta_s = 0;
rc = ioctl(self->fd, RTC_RD_TIME, &rtc_tm);
ASSERT_NE(-1, rc);
+ clock_gettime(CLOCK_MONOTONIC, &cur_mono);
rtc_read = rtc_time_to_timestamp(&rtc_tm);
+ delta_s = cur_mono.tv_sec - prev_mono.tv_sec;
+
/* Time should not go backwards */
ASSERT_LE(prev_rtc_read, rtc_read);
- /* Time should not increase more then 1s at a time */
- ASSERT_GE(prev_rtc_read + 1, rtc_read);
+ /* Time should not increase more then elapsed time + 1s */
+ ASSERT_GE(prev_rtc_read + delta_s + 1, rtc_read);
/* Sleep 11ms to avoid killing / overheating the RTC */
nanosleep_with_retries(READ_LOOP_SLEEP_MS * 1000000);
prev_rtc_read = rtc_read;
+ prev_mono = cur_mono;
iter_count++;
} while (prev_rtc_read <= start_rtc_read + READ_LOOP_DURATION_SEC);
--
2.54.0.545.g6539524ca2-goog
^ permalink raw reply related
* [PATCH] rtc: ds1307: handle oscillator stop flag for ds1337/ds1339/ds3231
From: Ronan Dalton @ 2026-05-01 4:46 UTC (permalink / raw)
To: alexandre.belloni
Cc: Ronan Dalton, linux-rtc, linux-kernel, Tyler Hicks, Sasha Levin,
Meagan Lloyd, Rodolfo Giometti, Chris Packham
Prior to commit 6cb0d8587b96 ("rtc: ds1307: remove clear of oscillator
stop flag (OSF) in probe"), the oscillator stop flag (OSF) bit was
checked during device probe for the ds1337, ds1339, ds1341, and ds3231
chips; if it was set, it would be cleared and a warning would be logged
saying "SET TIME!". Since that commit, the OSF bit is no longer cleared,
but the warning is still printed.
Directly following that commit, there was no way to get rid of this
warning because nothing cleared the OSF bit on these chips.
The commit associated with the previous commit, ae03a28e12a7 ("rtc:
ds1307: handle oscillator stop flag (OSF) for ds1341"), made proper use
of the OSF when getting and setting the time in the RTC. However, the
other RTC variants ds1337, ds1339 and ds3231 didn't have a corresponding
change made.
Given that the OSF bit is no longer cleared at probe time when it is
set, the remaining three chips should have the same handling as the
ds1341 chip has for the OSF bit.
Fix the issue on the ds1337, ds1339 and ds3231 chips by applying the
same logic as the ds1341 has to these chips.
Note that any devices brought up between the first referenced commit and
this one may begin mistrusting the time reported by the RTC until it is
set again, if the bit was never explicitly cleared.
Note that only the ds1339 was tested with this change, but the
datasheets for the other chips contain essentially identical
descriptions of the OSF bit so the same change should work.
An alternative to this change could be just to revert the referenced two
commits and not use the OSF bit at all, apart from logging a warning and
clearing it on probe.
Signed-off-by: Ronan Dalton <ronan.dalton@alliedtelesis.co.nz>
Cc: linux-rtc@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: Alexandre Belloni <alexandre.belloni@bootlin.com>
Cc: Tyler Hicks <code@tyhicks.com>
Cc: Sasha Levin <sashal@kernel.org>
Cc: Meagan Lloyd <meaganlloyd@linux.microsoft.com>
Cc: Rodolfo Giometti <giometti@enneenne.com>
Cc: Chris Packham <chris.packham@alliedtelesis.co.nz>
Fixes: 6cb0d8587b96 ("rtc: ds1307: remove clear of oscillator stop flag (OSF) in probe")
---
drivers/rtc/rtc-ds1307.c | 28 +++++++++++++++++-----------
1 file changed, 17 insertions(+), 11 deletions(-)
diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
index 7205c59ff729..edf81b975dec 100644
--- a/drivers/rtc/rtc-ds1307.c
+++ b/drivers/rtc/rtc-ds1307.c
@@ -269,6 +269,16 @@ static int ds1307_get_time(struct device *dev, struct rtc_time *t)
if (tmp & DS1338_BIT_OSF)
return -EINVAL;
break;
+ case ds_1337:
+ case ds_1339:
+ case ds_1341:
+ case ds_3231:
+ ret = regmap_read(ds1307->regmap, DS1337_REG_STATUS, &tmp);
+ if (ret)
+ return ret;
+ if (tmp & DS1337_BIT_OSF)
+ return -EINVAL;
+ break;
case ds_1340:
if (tmp & DS1340_BIT_nEOSC)
return -EINVAL;
@@ -279,13 +289,6 @@ static int ds1307_get_time(struct device *dev, struct rtc_time *t)
if (tmp & DS1340_BIT_OSF)
return -EINVAL;
break;
- case ds_1341:
- ret = regmap_read(ds1307->regmap, DS1337_REG_STATUS, &tmp);
- if (ret)
- return ret;
- if (tmp & DS1337_BIT_OSF)
- return -EINVAL;
- break;
case ds_1388:
ret = regmap_read(ds1307->regmap, DS1388_REG_FLAG, &tmp);
if (ret)
@@ -380,14 +383,17 @@ static int ds1307_set_time(struct device *dev, struct rtc_time *t)
regmap_update_bits(ds1307->regmap, DS1307_REG_CONTROL,
DS1338_BIT_OSF, 0);
break;
+ case ds_1337:
+ case ds_1339:
+ case ds_1341:
+ case ds_3231:
+ regmap_update_bits(ds1307->regmap, DS1337_REG_STATUS,
+ DS1337_BIT_OSF, 0);
+ break;
case ds_1340:
regmap_update_bits(ds1307->regmap, DS1340_REG_FLAG,
DS1340_BIT_OSF, 0);
break;
- case ds_1341:
- regmap_update_bits(ds1307->regmap, DS1337_REG_STATUS,
- DS1337_BIT_OSF, 0);
- break;
case ds_1388:
regmap_update_bits(ds1307->regmap, DS1388_REG_FLAG,
DS1388_BIT_OSF, 0);
--
2.53.0
^ permalink raw reply related
* Re: [PATCH] rtc: ds1307: handle oscillator stop flag for ds1337/ds1339/ds3231
From: Chris Packham @ 2026-05-01 5:14 UTC (permalink / raw)
To: Ronan Dalton, alexandre.belloni@bootlin.com
Cc: linux-rtc@vger.kernel.org, linux-kernel@vger.kernel.org,
Tyler Hicks, Sasha Levin, Meagan Lloyd, Rodolfo Giometti
In-Reply-To: <20260501044657.1003980-2-ronan.dalton@alliedtelesis.co.nz>
Hi Ronan
On 01/05/2026 16:46, Ronan Dalton wrote:
> Prior to commit 6cb0d8587b96 ("rtc: ds1307: remove clear of oscillator
> stop flag (OSF) in probe"), the oscillator stop flag (OSF) bit was
> checked during device probe for the ds1337, ds1339, ds1341, and ds3231
> chips; if it was set, it would be cleared and a warning would be logged
> saying "SET TIME!". Since that commit, the OSF bit is no longer cleared,
> but the warning is still printed.
>
> Directly following that commit, there was no way to get rid of this
> warning because nothing cleared the OSF bit on these chips.
>
> The commit associated with the previous commit, ae03a28e12a7 ("rtc:
> ds1307: handle oscillator stop flag (OSF) for ds1341"), made proper use
> of the OSF when getting and setting the time in the RTC. However, the
> other RTC variants ds1337, ds1339 and ds3231 didn't have a corresponding
> change made.
>
> Given that the OSF bit is no longer cleared at probe time when it is
> set, the remaining three chips should have the same handling as the
> ds1341 chip has for the OSF bit.
>
> Fix the issue on the ds1337, ds1339 and ds3231 chips by applying the
> same logic as the ds1341 has to these chips.
>
> Note that any devices brought up between the first referenced commit and
> this one may begin mistrusting the time reported by the RTC until it is
> set again, if the bit was never explicitly cleared.
>
> Note that only the ds1339 was tested with this change, but the
> datasheets for the other chips contain essentially identical
> descriptions of the OSF bit so the same change should work.
>
> An alternative to this change could be just to revert the referenced two
> commits and not use the OSF bit at all, apart from logging a warning and
> clearing it on probe.
>
> Signed-off-by: Ronan Dalton <ronan.dalton@alliedtelesis.co.nz>
> Cc: linux-rtc@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Cc: Alexandre Belloni <alexandre.belloni@bootlin.com>
> Cc: Tyler Hicks <code@tyhicks.com>
> Cc: Sasha Levin <sashal@kernel.org>
> Cc: Meagan Lloyd <meaganlloyd@linux.microsoft.com>
> Cc: Rodolfo Giometti <giometti@enneenne.com>
> Cc: Chris Packham <chris.packham@alliedtelesis.co.nz>
> Fixes: 6cb0d8587b96 ("rtc: ds1307: remove clear of oscillator stop flag (OSF) in probe")
Reviewed-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> ---
> drivers/rtc/rtc-ds1307.c | 28 +++++++++++++++++-----------
> 1 file changed, 17 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
> index 7205c59ff729..edf81b975dec 100644
> --- a/drivers/rtc/rtc-ds1307.c
> +++ b/drivers/rtc/rtc-ds1307.c
> @@ -269,6 +269,16 @@ static int ds1307_get_time(struct device *dev, struct rtc_time *t)
> if (tmp & DS1338_BIT_OSF)
> return -EINVAL;
> break;
> + case ds_1337:
> + case ds_1339:
> + case ds_1341:
> + case ds_3231:
> + ret = regmap_read(ds1307->regmap, DS1337_REG_STATUS, &tmp);
> + if (ret)
> + return ret;
> + if (tmp & DS1337_BIT_OSF)
> + return -EINVAL;
> + break;
> case ds_1340:
> if (tmp & DS1340_BIT_nEOSC)
> return -EINVAL;
> @@ -279,13 +289,6 @@ static int ds1307_get_time(struct device *dev, struct rtc_time *t)
> if (tmp & DS1340_BIT_OSF)
> return -EINVAL;
> break;
> - case ds_1341:
> - ret = regmap_read(ds1307->regmap, DS1337_REG_STATUS, &tmp);
> - if (ret)
> - return ret;
> - if (tmp & DS1337_BIT_OSF)
> - return -EINVAL;
> - break;
> case ds_1388:
> ret = regmap_read(ds1307->regmap, DS1388_REG_FLAG, &tmp);
> if (ret)
> @@ -380,14 +383,17 @@ static int ds1307_set_time(struct device *dev, struct rtc_time *t)
> regmap_update_bits(ds1307->regmap, DS1307_REG_CONTROL,
> DS1338_BIT_OSF, 0);
> break;
> + case ds_1337:
> + case ds_1339:
> + case ds_1341:
> + case ds_3231:
> + regmap_update_bits(ds1307->regmap, DS1337_REG_STATUS,
> + DS1337_BIT_OSF, 0);
> + break;
> case ds_1340:
> regmap_update_bits(ds1307->regmap, DS1340_REG_FLAG,
> DS1340_BIT_OSF, 0);
> break;
> - case ds_1341:
> - regmap_update_bits(ds1307->regmap, DS1337_REG_STATUS,
> - DS1337_BIT_OSF, 0);
> - break;
> case ds_1388:
> regmap_update_bits(ds1307->regmap, DS1388_REG_FLAG,
> DS1388_BIT_OSF, 0);
^ permalink raw reply
* [PATCH RESEND] rtc: ab8500: replace sprintf() with sysfs_emit()
From: Maxwell Doose @ 2026-05-03 20:12 UTC (permalink / raw)
To: linusw, alexandre.belloni
Cc: moderated list:ARM/NOMADIK/Ux500 ARCHITECTURES,
open list:REAL TIME CLOCK (RTC) SUBSYSTEM, open list
This patch replaces sprintf() with sysfs_emit() to ensure proper
bounds checking. It also simplifies the return logic by directly
returning the error after logging, instead of logging, calling
sprintf(), then returning.
Reviewed-by: Linus Walleij <linusw@kernel.org>
Signed-off-by: Maxwell Doose <m32285159@gmail.com>
---
note 1: original patch can be found here:
https://lore.kernel.org/linux-rtc/CAD++jLkQD_ZSFPGrx4uL+ezrR1Ai2ddUF9_PpesDG+AEwiDrag@mail.gmail.com/T/#t
note 2: I rebased this on to v7.1-rc1 to make sure there wouldn't be
any merge conflicts.
drivers/rtc/rtc-ab8500.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/rtc/rtc-ab8500.c b/drivers/rtc/rtc-ab8500.c
index ed2b6b8bb3bf..c6147837f957 100644
--- a/drivers/rtc/rtc-ab8500.c
+++ b/drivers/rtc/rtc-ab8500.c
@@ -284,11 +284,10 @@ static ssize_t ab8500_sysfs_show_rtc_calibration(struct device *dev,
retval = ab8500_rtc_get_calibration(dev, &calibration);
if (retval < 0) {
dev_err(dev, "Failed to read RTC calibration attribute\n");
- sprintf(buf, "0\n");
return retval;
}
- return sprintf(buf, "%d\n", calibration);
+ return sysfs_emit(buf, "%d\n", calibration);
}
static DEVICE_ATTR(rtc_calibration, S_IRUGO | S_IWUSR,
base-commit: 254f49634ee16a731174d2ae34bc50bd5f45e731
--
2.54.0
^ permalink raw reply related
* Re: [PATCH RESEND] rtc: ab8500: replace sprintf() with sysfs_emit()
From: Alexandre Belloni @ 2026-05-04 16:04 UTC (permalink / raw)
To: linusw, Maxwell Doose; +Cc: linux-arm-kernel, linux-rtc, linux-kernel
In-Reply-To: <20260503201236.29685-1-m32285159@gmail.com>
On Sun, 03 May 2026 15:12:36 -0500, Maxwell Doose wrote:
> This patch replaces sprintf() with sysfs_emit() to ensure proper
> bounds checking. It also simplifies the return logic by directly
> returning the error after logging, instead of logging, calling
> sprintf(), then returning.
Applied, thanks!
[1/1] rtc: ab8500: replace sprintf() with sysfs_emit()
https://git.kernel.org/abelloni/c/b72386864481
Best regards,
--
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply
* [PATCH] dt-bindings: rtc: epson,rx6110: Convert to DT Schema
From: Udaya Kiran Challa @ 2026-05-04 18:37 UTC (permalink / raw)
To: alexandre.belloni, robh, krzk+dt, conor+dt
Cc: skhan, me, linux-rtc, devicetree, linux-kernel,
Udaya Kiran Challa
Convert the Epson RX6110 Real Time Clock devicetree binding
from the legacy text format to DT schema.
Signed-off-by: Udaya Kiran Challa <challauday369@gmail.com>
---
.../devicetree/bindings/rtc/epson,rx6110.txt | 39 -----------
.../devicetree/bindings/rtc/epson,rx6110.yaml | 69 +++++++++++++++++++
2 files changed, 69 insertions(+), 39 deletions(-)
delete mode 100644 Documentation/devicetree/bindings/rtc/epson,rx6110.txt
create mode 100644 Documentation/devicetree/bindings/rtc/epson,rx6110.yaml
diff --git a/Documentation/devicetree/bindings/rtc/epson,rx6110.txt b/Documentation/devicetree/bindings/rtc/epson,rx6110.txt
deleted file mode 100644
index 3dc313e01f77..000000000000
--- a/Documentation/devicetree/bindings/rtc/epson,rx6110.txt
+++ /dev/null
@@ -1,39 +0,0 @@
-Epson RX6110 Real Time Clock
-============================
-
-The Epson RX6110 can be used with SPI or I2C busses. The kind of
-bus depends on the SPISEL pin and can not be configured via software.
-
-I2C mode
---------
-
-Required properties:
- - compatible: should be: "epson,rx6110"
- - reg : the I2C address of the device for I2C
-
-Example:
-
- rtc: rtc@32 {
- compatible = "epson,rx6110"
- reg = <0x32>;
- };
-
-SPI mode
---------
-
-Required properties:
- - compatible: should be: "epson,rx6110"
- - reg: chip select number
- - spi-cs-high: RX6110 needs chipselect high
- - spi-cpha: RX6110 works with SPI shifted clock phase
- - spi-cpol: RX6110 works with SPI inverse clock polarity
-
-Example:
-
- rtc: rtc@3 {
- compatible = "epson,rx6110"
- reg = <3>
- spi-cs-high;
- spi-cpha;
- spi-cpol;
- };
diff --git a/Documentation/devicetree/bindings/rtc/epson,rx6110.yaml b/Documentation/devicetree/bindings/rtc/epson,rx6110.yaml
new file mode 100644
index 000000000000..32d15a014f91
--- /dev/null
+++ b/Documentation/devicetree/bindings/rtc/epson,rx6110.yaml
@@ -0,0 +1,69 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/rtc/epson,rx6110.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Epson RX6110 Real Time Clock
+
+description: |
+ The Epson RX6110 can be used with SPI or I2C busses.
+ The kind of bus depends on the SPISEL pin and can not be
+ configured via software.
+
+maintainers:
+ - Alexandre Belloni <alexandre.belloni@bootlin.com>
+
+properties:
+ compatible:
+ const: epson,rx6110
+
+ reg:
+ maxItems: 1
+
+ # SPI-specific properties
+ spi-cs-high:
+ type: boolean
+ description: RX6110 needs chipselect high
+
+ spi-cpha:
+ type: boolean
+ description: RX6110 works with SPI shifted clock phase
+
+ spi-cpol:
+ type: boolean
+ description: RX6110 works with SPI inverse clock polarity
+
+required:
+ - compatible
+ - reg
+
+additionalProperties: false
+
+examples:
+ # I2C mode
+ - |
+ i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ rtc1: rtc@32 {
+ compatible = "epson,rx6110";
+ reg = <0x32>;
+ };
+ };
+
+ # SPI mode
+ - |
+ spi {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ rtc2: rtc@3 {
+ compatible = "epson,rx6110";
+ reg = <3>;
+ spi-cs-high;
+ spi-cpha;
+ spi-cpol;
+ };
+ };
--
2.43.0
^ permalink raw reply related
* Re: [PATCH] rtc: ds1307: handle oscillator stop flag for ds1337/ds1339/ds3231
From: Meagan Lloyd @ 2026-05-04 22:08 UTC (permalink / raw)
To: Ronan Dalton
Cc: alexandre.belloni, linux-rtc, linux-kernel, Tyler Hicks,
Sasha Levin, Meagan Lloyd, Rodolfo Giometti, Chris Packham,
Thara Gopinath
In-Reply-To: <20260501044657.1003980-2-ronan.dalton@alliedtelesis.co.nz>
Hi Ronan -
On Fri, May 01, 2026 at 04:46:10PM +1200, Ronan Dalton wrote:
> Prior to commit 6cb0d8587b96 ("rtc: ds1307: remove clear of oscillator
> stop flag (OSF) in probe"), the oscillator stop flag (OSF) bit was
> checked during device probe for the ds1337, ds1339, ds1341, and ds3231
> chips; if it was set, it would be cleared and a warning would be logged
> saying "SET TIME!". Since that commit, the OSF bit is no longer cleared,
> but the warning is still printed.
>
> Directly following that commit, there was no way to get rid of this
> warning because nothing cleared the OSF bit on these chips.
>
> The commit associated with the previous commit, ae03a28e12a7 ("rtc:
> ds1307: handle oscillator stop flag (OSF) for ds1341"), made proper use
> of the OSF when getting and setting the time in the RTC. However, the
> other RTC variants ds1337, ds1339 and ds3231 didn't have a corresponding
> change made.
>
> Given that the OSF bit is no longer cleared at probe time when it is
> set, the remaining three chips should have the same handling as the
> ds1341 chip has for the OSF bit.
>
> Fix the issue on the ds1337, ds1339 and ds3231 chips by applying the
> same logic as the ds1341 has to these chips.
>
> Note that any devices brought up between the first referenced commit and
> this one may begin mistrusting the time reported by the RTC until it is
> set again, if the bit was never explicitly cleared.
>
> Note that only the ds1339 was tested with this change, but the
> datasheets for the other chips contain essentially identical
> descriptions of the OSF bit so the same change should work.
>
Thanks for finding and fixing this issue. I agree that the kernel logging
"SET TIME!" on every boot after the OSF gets set is unwanted behavior.
The original series was backported, so we will probably want this fix
backported too.
> An alternative to this change could be just to revert the referenced two
> commits and not use the OSF bit at all, apart from logging a warning and
> clearing it on probe.
The OSF handling fixed a real problem and is in use by a number of other
chip types in the driver, so I think we should keep the commits.
>
> Signed-off-by: Ronan Dalton <ronan.dalton@alliedtelesis.co.nz>
> Cc: linux-rtc@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Cc: Alexandre Belloni <alexandre.belloni@bootlin.com>
> Cc: Tyler Hicks <code@tyhicks.com>
> Cc: Sasha Levin <sashal@kernel.org>
> Cc: Meagan Lloyd <meaganlloyd@linux.microsoft.com>
> Cc: Rodolfo Giometti <giometti@enneenne.com>
> Cc: Chris Packham <chris.packham@alliedtelesis.co.nz>
> Fixes: 6cb0d8587b96 ("rtc: ds1307: remove clear of oscillator stop flag (OSF) in probe")
> ---
> drivers/rtc/rtc-ds1307.c | 28 +++++++++++++++++-----------
> 1 file changed, 17 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
> index 7205c59ff729..edf81b975dec 100644
> --- a/drivers/rtc/rtc-ds1307.c
> +++ b/drivers/rtc/rtc-ds1307.c
> @@ -269,6 +269,16 @@ static int ds1307_get_time(struct device *dev, struct rtc_time *t)
> if (tmp & DS1338_BIT_OSF)
> return -EINVAL;
> break;
> + case ds_1337:
> + case ds_1339:
> + case ds_1341:
> + case ds_3231:
> + ret = regmap_read(ds1307->regmap, DS1337_REG_STATUS, &tmp);
> + if (ret)
> + return ret;
> + if (tmp & DS1337_BIT_OSF)
> + return -EINVAL;
> + break;
If you're going to re-arrange the block to be in somewhat of an order,
perhaps put it above 1338 since 1337 < 1338.
> case ds_1340:
> if (tmp & DS1340_BIT_nEOSC)
> return -EINVAL;
> @@ -279,13 +289,6 @@ static int ds1307_get_time(struct device *dev, struct rtc_time *t)
> if (tmp & DS1340_BIT_OSF)
> return -EINVAL;
> break;
> - case ds_1341:
> - ret = regmap_read(ds1307->regmap, DS1337_REG_STATUS, &tmp);
> - if (ret)
> - return ret;
> - if (tmp & DS1337_BIT_OSF)
> - return -EINVAL;
> - break;
> case ds_1388:
> ret = regmap_read(ds1307->regmap, DS1388_REG_FLAG, &tmp);
> if (ret)
> @@ -380,14 +383,17 @@ static int ds1307_set_time(struct device *dev, struct rtc_time *t)
> regmap_update_bits(ds1307->regmap, DS1307_REG_CONTROL,
> DS1338_BIT_OSF, 0);
> break;
> + case ds_1337:
> + case ds_1339:
> + case ds_1341:
> + case ds_3231:
> + regmap_update_bits(ds1307->regmap, DS1337_REG_STATUS,
> + DS1337_BIT_OSF, 0);
> + break;
Same here
> case ds_1340:
> regmap_update_bits(ds1307->regmap, DS1340_REG_FLAG,
> DS1340_BIT_OSF, 0);
> break;
> - case ds_1341:
> - regmap_update_bits(ds1307->regmap, DS1337_REG_STATUS,
> - DS1337_BIT_OSF, 0);
> - break;
> case ds_1388:
> regmap_update_bits(ds1307->regmap, DS1388_REG_FLAG,
> DS1388_BIT_OSF, 0);
> --
> 2.53.0
^ permalink raw reply
* Re: [PATCH] rtc: ds1307: handle oscillator stop flag for ds1337/ds1339/ds3231
From: Ronan Dalton @ 2026-05-04 23:54 UTC (permalink / raw)
To: meaganlloyd@linux.microsoft.com
Cc: tgopinath@linux.microsoft.com, code@tyhicks.com,
linux-rtc@vger.kernel.org, linux-kernel@vger.kernel.org,
giometti@enneenne.com, Chris Packham, sashal@kernel.org,
alexandre.belloni@bootlin.com
In-Reply-To: <20260504-fd90667b1274c4e3a38a0604@linux.microsoft.com>
Hi Meagan,
On Mon, 2026-05-04 at 15:08 -0700, Meagan Lloyd wrote:
> > + case ds_1337:
> > + case ds_1339:
> > + case ds_1341:
> > + case ds_3231:
> > + ret = regmap_read(ds1307->regmap,
> > DS1337_REG_STATUS, &tmp);
> > + if (ret)
> > + return ret;
> > + if (tmp & DS1337_BIT_OSF)
> > + return -EINVAL;
> > + break;
>
> If you're going to re-arrange the block to be in somewhat of an
> order,
> perhaps put it above 1338 since 1337 < 1338.
I've ordered it this way based on the first case statement in each
block. Since ds_1337 > ds_1308, I've put the block below the block
starting with ds_1308. I could instead order it based on the last case
statement in each block, if you think that's better.
^ permalink raw reply
* Re: [PATCH] dt-bindings: rtc: epson,rx6110: Convert to DT Schema
From: Conor Dooley @ 2026-05-05 16:30 UTC (permalink / raw)
To: Udaya Kiran Challa
Cc: alexandre.belloni, robh, krzk+dt, conor+dt, skhan, me, linux-rtc,
devicetree, linux-kernel
In-Reply-To: <20260504183728.27412-1-challauday369@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 4079 bytes --]
On Tue, May 05, 2026 at 12:07:28AM +0530, Udaya Kiran Challa wrote:
> Convert the Epson RX6110 Real Time Clock devicetree binding
> from the legacy text format to DT schema.
>
> Signed-off-by: Udaya Kiran Challa <challauday369@gmail.com>
> ---
> .../devicetree/bindings/rtc/epson,rx6110.txt | 39 -----------
> .../devicetree/bindings/rtc/epson,rx6110.yaml | 69 +++++++++++++++++++
> 2 files changed, 69 insertions(+), 39 deletions(-)
> delete mode 100644 Documentation/devicetree/bindings/rtc/epson,rx6110.txt
> create mode 100644 Documentation/devicetree/bindings/rtc/epson,rx6110.yaml
>
> diff --git a/Documentation/devicetree/bindings/rtc/epson,rx6110.txt b/Documentation/devicetree/bindings/rtc/epson,rx6110.txt
> deleted file mode 100644
> index 3dc313e01f77..000000000000
> --- a/Documentation/devicetree/bindings/rtc/epson,rx6110.txt
> +++ /dev/null
> @@ -1,39 +0,0 @@
> -Epson RX6110 Real Time Clock
> -============================
> -
> -The Epson RX6110 can be used with SPI or I2C busses. The kind of
> -bus depends on the SPISEL pin and can not be configured via software.
> -
> -I2C mode
> ---------
> -
> -Required properties:
> - - compatible: should be: "epson,rx6110"
> - - reg : the I2C address of the device for I2C
> -
> -Example:
> -
> - rtc: rtc@32 {
> - compatible = "epson,rx6110"
> - reg = <0x32>;
> - };
> -
> -SPI mode
> ---------
> -
> -Required properties:
> - - compatible: should be: "epson,rx6110"
> - - reg: chip select number
> - - spi-cs-high: RX6110 needs chipselect high
> - - spi-cpha: RX6110 works with SPI shifted clock phase
> - - spi-cpol: RX6110 works with SPI inverse clock polarity
> -
> -Example:
> -
> - rtc: rtc@3 {
> - compatible = "epson,rx6110"
> - reg = <3>
> - spi-cs-high;
> - spi-cpha;
> - spi-cpol;
> - };
> diff --git a/Documentation/devicetree/bindings/rtc/epson,rx6110.yaml b/Documentation/devicetree/bindings/rtc/epson,rx6110.yaml
> new file mode 100644
> index 000000000000..32d15a014f91
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/rtc/epson,rx6110.yaml
> @@ -0,0 +1,69 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/rtc/epson,rx6110.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Epson RX6110 Real Time Clock
> +
> +description: |
> + The Epson RX6110 can be used with SPI or I2C busses.
> + The kind of bus depends on the SPISEL pin and can not be
> + configured via software.
> +
> +maintainers:
> + - Alexandre Belloni <alexandre.belloni@bootlin.com>
> +
> +properties:
> + compatible:
> + const: epson,rx6110
> +
> + reg:
> + maxItems: 1
> +
> + # SPI-specific properties
Drop the obvious comment.
> + spi-cs-high:
> + type: boolean
> + description: RX6110 needs chipselect high
> +
> + spi-cpha:
> + type: boolean
> + description: RX6110 works with SPI shifted clock phase
> +
> + spi-cpol:
> + type: boolean
> + description: RX6110 works with SPI inverse clock polarity
These spi properties should be replaced by a ref to /schemas/spi/spi-peripheral-props.yaml
and become "spi-foo: true", unless you want to reword these descriptions
to make it clear that these are all mandatory.
pw-bot: changes-requested
Cheers,
Conor.
> +
> +required:
> + - compatible
> + - reg
> +
> +additionalProperties: false
> +
> +examples:
> + # I2C mode
> + - |
> + i2c {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + rtc1: rtc@32 {
Drop the labels here, since they have no users.
> + compatible = "epson,rx6110";
> + reg = <0x32>;
> + };
> + };
> +
> + # SPI mode
> + - |
> + spi {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + rtc2: rtc@3 {
> + compatible = "epson,rx6110";
> + reg = <3>;
> + spi-cs-high;
> + spi-cpha;
> + spi-cpol;
> + };
> + };
> --
> 2.43.0
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [PATCH] rtc: ds1307: handle oscillator stop flag for ds1337/ds1339/ds3231
From: Meagan Lloyd @ 2026-05-05 19:20 UTC (permalink / raw)
To: Ronan Dalton
Cc: meaganlloyd@linux.microsoft.com, tgopinath@linux.microsoft.com,
code@tyhicks.com, linux-rtc@vger.kernel.org,
linux-kernel@vger.kernel.org, giometti@enneenne.com,
Chris Packham, sashal@kernel.org, alexandre.belloni@bootlin.com
In-Reply-To: <4c097ca4fffed215395ec5979f0f0f43ed85cb97.camel@alliedtelesis.co.nz>
Hi Ronan,
On Mon, May 04, 2026 at 11:54:32PM +0000, Ronan Dalton wrote:
> Hi Meagan,
>
> On Mon, 2026-05-04 at 15:08 -0700, Meagan Lloyd wrote:
> > > + case ds_1337:
> > > + case ds_1339:
> > > + case ds_1341:
> > > + case ds_3231:
> > > + ret = regmap_read(ds1307->regmap,
> > > DS1337_REG_STATUS, &tmp);
> > > + if (ret)
> > > + return ret;
> > > + if (tmp & DS1337_BIT_OSF)
> > > + return -EINVAL;
> > > + break;
> >
> > If you're going to re-arrange the block to be in somewhat of an
> > order,
> > perhaps put it above 1338 since 1337 < 1338.
>
> I've ordered it this way based on the first case statement in each
> block. Since ds_1337 > ds_1308, I've put the block below the block
> starting with ds_1308. I could instead order it based on the last case
> statement in each block, if you think that's better.
I agree with your ordering strategy, but your patch inserts it after the
ds_1338 case statement block (rather than the intended ds_1308).
^ permalink raw reply
* Re: [PATCH] rtc: ds1307: handle oscillator stop flag for ds1337/ds1339/ds3231
From: Ronan Dalton @ 2026-05-05 22:24 UTC (permalink / raw)
To: meaganlloyd@linux.microsoft.com
Cc: tgopinath@linux.microsoft.com, giometti@enneenne.com,
linux-rtc@vger.kernel.org, linux-kernel@vger.kernel.org,
alexandre.belloni@bootlin.com, Chris Packham, sashal@kernel.org,
code@tyhicks.com
In-Reply-To: <20260505-5c718cd011b56364fefc885d@linux.microsoft.com>
Hi Meagan,
On Tue, 2026-05-05 at 12:20 -0700, Meagan Lloyd wrote:
> Hi Ronan,
>
> On Mon, May 04, 2026 at 11:54:32PM +0000, Ronan Dalton wrote:
> > Hi Meagan,
> >
> > On Mon, 2026-05-04 at 15:08 -0700, Meagan Lloyd wrote:
> > > > + case ds_1337:
> > > > + case ds_1339:
> > > > + case ds_1341:
> > > > + case ds_3231:
> > > > + ret = regmap_read(ds1307->regmap,
> > > > DS1337_REG_STATUS, &tmp);
> > > > + if (ret)
> > > > + return ret;
> > > > + if (tmp & DS1337_BIT_OSF)
> > > > + return -EINVAL;
> > > > + break;
> > >
> > > If you're going to re-arrange the block to be in somewhat of an
> > > order,
> > > perhaps put it above 1338 since 1337 < 1338.
> >
> > I've ordered it this way based on the first case statement in each
> > block. Since ds_1337 > ds_1308, I've put the block below the block
> > starting with ds_1308. I could instead order it based on the last
> > case
> > statement in each block, if you think that's better.
>
> I agree with your ordering strategy, but your patch inserts it after
> the
> ds_1338 case statement block (rather than the intended ds_1308).
>
Here's how it currently is in ds1307_get_time:
case ds_1308:
case ds_1338:
[statement block #1]
case ds_1337:
case ds_1339:
case ds_1341:
case ds_3231:
[statement block #2]
To insert statement block #2 after the ds_1308 case label would involve
the following:
case ds_1308:
[statement block #1]
case ds_1337:
case ds_1339:
case ds_1341:
case ds_3231:
[statement block #2]
case ds_1338:
[statement block #1, duplicated]
Or the following with strict order:
case ds_1308:
[statement block #1]
case ds_1337:
[statement block #2]
case ds_1338:
[statement block #1, duplicated]
case ds_1339:
case ds_1341:
case ds_3231:
[statement block #2, duplicated]
The case statements can be put strictly in order, but that will involve
some duplication.
^ permalink raw reply
* Re: [PATCH] rtc: ds1307: handle oscillator stop flag for ds1337/ds1339/ds3231
From: Meagan Lloyd @ 2026-05-06 1:41 UTC (permalink / raw)
To: Ronan Dalton
Cc: meaganlloyd@linux.microsoft.com, tgopinath@linux.microsoft.com,
giometti@enneenne.com, linux-rtc@vger.kernel.org,
linux-kernel@vger.kernel.org, alexandre.belloni@bootlin.com,
Chris Packham, sashal@kernel.org, code@tyhicks.com
In-Reply-To: <39365e134ab175492292317ea14bb16172dd580b.camel@alliedtelesis.co.nz>
Hi Ronan,
On Tue, May 05, 2026 at 10:24:55PM +0000, Ronan Dalton wrote:
> Hi Meagan,
>
> On Tue, 2026-05-05 at 12:20 -0700, Meagan Lloyd wrote:
> > Hi Ronan,
> >
> > On Mon, May 04, 2026 at 11:54:32PM +0000, Ronan Dalton wrote:
> > > Hi Meagan,
> > >
> > > On Mon, 2026-05-04 at 15:08 -0700, Meagan Lloyd wrote:
> > > > > + case ds_1337:
> > > > > + case ds_1339:
> > > > > + case ds_1341:
> > > > > + case ds_3231:
> > > > > + ret = regmap_read(ds1307->regmap,
> > > > > DS1337_REG_STATUS, &tmp);
> > > > > + if (ret)
> > > > > + return ret;
> > > > > + if (tmp & DS1337_BIT_OSF)
> > > > > + return -EINVAL;
> > > > > + break;
> > > >
> > > > If you're going to re-arrange the block to be in somewhat of an
> > > > order,
> > > > perhaps put it above 1338 since 1337 < 1338.
> > >
> > > I've ordered it this way based on the first case statement in each
> > > block. Since ds_1337 > ds_1308, I've put the block below the block
> > > starting with ds_1308. I could instead order it based on the last
> > > case
> > > statement in each block, if you think that's better.
> >
> > I agree with your ordering strategy, but your patch inserts it after
> > the
> > ds_1338 case statement block (rather than the intended ds_1308).
> >
>
> Here's how it currently is in ds1307_get_time:
>
> case ds_1308:
> case ds_1338:
> [statement block #1]
> case ds_1337:
> case ds_1339:
> case ds_1341:
> case ds_3231:
> [statement block #2]
>
> To insert statement block #2 after the ds_1308 case label would involve
> the following:
>
> case ds_1308:
> [statement block #1]
> case ds_1337:
> case ds_1339:
> case ds_1341:
> case ds_3231:
> [statement block #2]
> case ds_1338:
> [statement block #1, duplicated]
>
> Or the following with strict order:
>
> case ds_1308:
> [statement block #1]
> case ds_1337:
> [statement block #2]
> case ds_1338:
> [statement block #1, duplicated]
> case ds_1339:
> case ds_1341:
> case ds_3231:
> [statement block #2, duplicated]
>
> The case statements can be put strictly in order, but that will involve
> some duplication.
You are absolutely right. I missed that ds_1308 and ds_1338 share a
statement block. No need to change anything, thanks for the clear
explanation!
Reviewed-by: Meagan Lloyd <meaganlloyd@linux.microsoft.com>
^ permalink raw reply
* Re: [PATCH] rtc: ds1307: handle oscillator stop flag for ds1337/ds1339/ds3231
From: Ronan Dalton @ 2026-05-06 1:57 UTC (permalink / raw)
To: meaganlloyd@linux.microsoft.com
Cc: tgopinath@linux.microsoft.com, giometti@enneenne.com,
linux-rtc@vger.kernel.org, linux-kernel@vger.kernel.org,
code@tyhicks.com, Chris Packham, sashal@kernel.org,
alexandre.belloni@bootlin.com
In-Reply-To: <20260505-ec9618931e890b4f0f1b62ab@linux.microsoft.com>
On Tue, 2026-05-05 at 18:41 -0700, Meagan Lloyd wrote:
> Reviewed-by: Meagan Lloyd <meaganlloyd@linux.microsoft.com>
Thanks!
^ permalink raw reply
* [PATCH 0/5] rtc: renesas-rtca3: Various fixes and improvements
From: Prabhakar @ 2026-05-06 16:49 UTC (permalink / raw)
To: Alexandre Belloni, Claudiu Beznea, Geert Uytterhoeven
Cc: linux-rtc, linux-renesas-soc, linux-kernel, Prabhakar, Biju Das,
Fabrizio Castro, Lad Prabhakar
From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Hi all,
This patch series includes various fixes and improvements for the
Renesas RTCA-3 RTC driver, including:
- Fixing the polling condition when clearing the PIE bit during alarm
setup error handling.
- Checking the result of the RADJ polling during initial setup and
propagating errors.
- Correcting an error message related to reset control.
- Fixing a typo in the documentation for the rtca3_ppb_per_cycle struct.
- Refactoring year decoding logic into a helper function for better
readability.
Cheers,
Prabhakar
Lad Prabhakar (5):
rtc: renesas-rtca3: Fix PIE clear polling condition in alarm setup
error path
rtc: renesas-rtca3: Check RADJ poll result during initial setup
rtc: renesas-rtca3: Fix incorrect error message for reset assert
rtc: renesas-rtca3: Fix typo in rtca3_ppb_per_cycle documentation
rtc: renesas-rtca3: Factor out year decoding helper
drivers/rtc/rtc-renesas-rtca3.c | 26 +++++++++++++++-----------
1 file changed, 15 insertions(+), 11 deletions(-)
--
2.54.0
^ permalink raw reply
* [PATCH 1/5] rtc: renesas-rtca3: Fix PIE clear polling condition in alarm setup error path
From: Prabhakar @ 2026-05-06 16:49 UTC (permalink / raw)
To: Alexandre Belloni, Claudiu Beznea, Geert Uytterhoeven
Cc: linux-rtc, linux-renesas-soc, linux-kernel, Prabhakar, Biju Das,
Fabrizio Castro, Lad Prabhakar
In-Reply-To: <20260506164914.3987293-1-prabhakar.mahadev-lad.rj@bp.renesas.com>
From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
In rtca3_set_alarm(), the setup_failed path attempts to disable the
Periodic Interrupt Enable (PIE) bit and wait until it is cleared.
However, the polling condition passed to readb_poll_timeout_atomic()
uses an incorrect expression:
!(tmp & ~RTCA3_RCR1_PIE)
As ~RTCA3_RCR1_PIE evaluates to a mask of all bits except PIE, the
condition effectively waits for all non-PIE bits to become zero, which
is unrelated to the intended operation and is unlikely to ever be true.
This causes the poll to time out unnecessarily.
Fix the condition to check for the PIE bit itself being cleared:
!(tmp & RTCA3_RCR1_PIE)
This correctly waits until PIE is deasserted after being cleared.
Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
drivers/rtc/rtc-renesas-rtca3.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/rtc/rtc-renesas-rtca3.c b/drivers/rtc/rtc-renesas-rtca3.c
index cbabaa4dc96a..2dc080d0eb6c 100644
--- a/drivers/rtc/rtc-renesas-rtca3.c
+++ b/drivers/rtc/rtc-renesas-rtca3.c
@@ -455,7 +455,7 @@ static int rtca3_set_alarm(struct device *dev, struct rtc_wkalrm *wkalrm)
* specified timeout for setup.
*/
writeb(rcr1 & ~RTCA3_RCR1_PIE, priv->base + RTCA3_RCR1);
- readb_poll_timeout_atomic(priv->base + RTCA3_RCR1, tmp, !(tmp & ~RTCA3_RCR1_PIE),
+ readb_poll_timeout_atomic(priv->base + RTCA3_RCR1, tmp, !(tmp & RTCA3_RCR1_PIE),
10, RTCA3_DEFAULT_TIMEOUT_US);
atomic_set(&priv->alrm_sstep, RTCA3_ALRM_SSTEP_DONE);
}
--
2.54.0
^ permalink raw reply related
* [PATCH 2/5] rtc: renesas-rtca3: Check RADJ poll result during initial setup
From: Prabhakar @ 2026-05-06 16:49 UTC (permalink / raw)
To: Alexandre Belloni, Claudiu Beznea, Geert Uytterhoeven
Cc: linux-rtc, linux-renesas-soc, linux-kernel, Prabhakar, Biju Das,
Fabrizio Castro, Lad Prabhakar
In-Reply-To: <20260506164914.3987293-1-prabhakar.mahadev-lad.rj@bp.renesas.com>
From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
In rtca3_initial_setup(), the driver clears the RTCA3_RADJ register and
waits for it to reach zero using readb_poll_timeout(). Check the return
value of readb_poll_timeout() and propagate the error if the poll fails.
Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
drivers/rtc/rtc-renesas-rtca3.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/rtc/rtc-renesas-rtca3.c b/drivers/rtc/rtc-renesas-rtca3.c
index 2dc080d0eb6c..af2a3878289e 100644
--- a/drivers/rtc/rtc-renesas-rtca3.c
+++ b/drivers/rtc/rtc-renesas-rtca3.c
@@ -634,6 +634,8 @@ static int rtca3_initial_setup(struct clk *clk, struct rtca3_priv *priv)
writeb(0, priv->base + RTCA3_RADJ);
ret = readb_poll_timeout(priv->base + RTCA3_RADJ, tmp, !tmp, 10,
RTCA3_DEFAULT_TIMEOUT_US);
+ if (ret)
+ return ret;
/* Start the RTC and enable automatic time error adjustment. */
mask = RTCA3_RCR2_START | RTCA3_RCR2_AADJE;
--
2.54.0
^ permalink raw reply related
* [PATCH 3/5] rtc: renesas-rtca3: Fix incorrect error message for reset assert
From: Prabhakar @ 2026-05-06 16:49 UTC (permalink / raw)
To: Alexandre Belloni, Claudiu Beznea, Geert Uytterhoeven
Cc: linux-rtc, linux-renesas-soc, linux-kernel, Prabhakar, Biju Das,
Fabrizio Castro, Lad Prabhakar
In-Reply-To: <20260506164914.3987293-1-prabhakar.mahadev-lad.rj@bp.renesas.com>
From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Update the message to "assert reset" to accurately reflect the
operation being performed.
Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
drivers/rtc/rtc-renesas-rtca3.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/rtc/rtc-renesas-rtca3.c b/drivers/rtc/rtc-renesas-rtca3.c
index af2a3878289e..8763745b9172 100644
--- a/drivers/rtc/rtc-renesas-rtca3.c
+++ b/drivers/rtc/rtc-renesas-rtca3.c
@@ -702,7 +702,7 @@ static void rtca3_action(void *data)
ret = reset_control_assert(priv->rstc);
if (ret)
- dev_err(dev, "Failed to de-assert reset!");
+ dev_err(dev, "Failed to assert reset!");
ret = pm_runtime_put_sync(dev);
if (ret < 0)
--
2.54.0
^ permalink raw reply related
* [PATCH 4/5] rtc: renesas-rtca3: Fix typo in rtca3_ppb_per_cycle documentation
From: Prabhakar @ 2026-05-06 16:49 UTC (permalink / raw)
To: Alexandre Belloni, Claudiu Beznea, Geert Uytterhoeven
Cc: linux-rtc, linux-renesas-soc, linux-kernel, Prabhakar, Biju Das,
Fabrizio Castro, Lad Prabhakar
In-Reply-To: <20260506164914.3987293-1-prabhakar.mahadev-lad.rj@bp.renesas.com>
From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Correct a typo in the kernel-doc comment for struct
rtca3_ppb_per_cycle by fixing "adjutment" to "adjustment".
Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
drivers/rtc/rtc-renesas-rtca3.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/rtc/rtc-renesas-rtca3.c b/drivers/rtc/rtc-renesas-rtca3.c
index 8763745b9172..97e7e65f59a5 100644
--- a/drivers/rtc/rtc-renesas-rtca3.c
+++ b/drivers/rtc/rtc-renesas-rtca3.c
@@ -103,7 +103,7 @@ enum rtca3_alrm_set_step {
/**
* struct rtca3_ppb_per_cycle - PPB per cycle
- * @ten_sec: PPB per cycle in 10 seconds adjutment mode
+ * @ten_sec: PPB per cycle in 10 seconds adjustment mode
* @sixty_sec: PPB per cycle in 60 seconds adjustment mode
*/
struct rtca3_ppb_per_cycle {
--
2.54.0
^ permalink raw reply related
* [PATCH 5/5] rtc: renesas-rtca3: Factor out year decoding helper
From: Prabhakar @ 2026-05-06 16:49 UTC (permalink / raw)
To: Alexandre Belloni, Claudiu Beznea, Geert Uytterhoeven
Cc: linux-rtc, linux-renesas-soc, linux-kernel, Prabhakar, Biju Das,
Fabrizio Castro, Lad Prabhakar
In-Reply-To: <20260506164914.3987293-1-prabhakar.mahadev-lad.rj@bp.renesas.com>
From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
The logic to decode the year value from the hardware registers is
duplicated in both rtca3_read_time() and rtca3_read_alarm().
Introduce a helper rtca3_decode_year() to centralize this conversion.
Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
drivers/rtc/rtc-renesas-rtca3.c | 18 ++++++++++--------
1 file changed, 10 insertions(+), 8 deletions(-)
diff --git a/drivers/rtc/rtc-renesas-rtca3.c b/drivers/rtc/rtc-renesas-rtca3.c
index 97e7e65f59a5..b3875d041de5 100644
--- a/drivers/rtc/rtc-renesas-rtca3.c
+++ b/drivers/rtc/rtc-renesas-rtca3.c
@@ -228,12 +228,19 @@ static void rtca3_prepare_cntalrm_regs_for_read(struct rtca3_priv *priv, bool cn
}
}
+static u32 rtca3_decode_year(u8 mask, u16 year)
+{
+ u8 y = FIELD_GET(mask, year);
+ u32 century = bcd2bin((y == 0x99) ? 0x19 : 0x20);
+
+ return (century * 100 + bcd2bin(y)) - 1900;
+}
+
static int rtca3_read_time(struct device *dev, struct rtc_time *tm)
{
struct rtca3_priv *priv = dev_get_drvdata(dev);
u8 sec, min, hour, wday, mday, month, tmp;
u8 trials = 0;
- u32 year100;
u16 year;
guard(spinlock_irqsave)(&priv->lock);
@@ -274,9 +281,7 @@ static int rtca3_read_time(struct device *dev, struct rtc_time *tm)
tm->tm_wday = bcd2bin(FIELD_GET(RTCA3_RWKCNT_WK, wday));
tm->tm_mday = bcd2bin(FIELD_GET(RTCA3_RDAYCNT_DAY, mday));
tm->tm_mon = bcd2bin(FIELD_GET(RTCA3_RMONCNT_MONTH, month)) - 1;
- year = FIELD_GET(RTCA3_RYRCNT_YEAR, year);
- year100 = bcd2bin((year == 0x99) ? 0x19 : 0x20);
- tm->tm_year = (year100 * 100 + bcd2bin(year)) - 1900;
+ tm->tm_year = rtca3_decode_year(RTCA3_RYRCNT_YEAR, year);
return 0;
}
@@ -354,7 +359,6 @@ static int rtca3_read_alarm(struct device *dev, struct rtc_wkalrm *wkalrm)
struct rtca3_priv *priv = dev_get_drvdata(dev);
u8 sec, min, hour, wday, mday, month;
struct rtc_time *tm = &wkalrm->time;
- u32 year100;
u16 year;
guard(spinlock_irqsave)(&priv->lock);
@@ -373,9 +377,7 @@ static int rtca3_read_alarm(struct device *dev, struct rtc_wkalrm *wkalrm)
tm->tm_wday = bcd2bin(FIELD_GET(RTCA3_RWKAR_DAYW, wday));
tm->tm_mday = bcd2bin(FIELD_GET(RTCA3_RDAYAR_DATE, mday));
tm->tm_mon = bcd2bin(FIELD_GET(RTCA3_RMONAR_MON, month)) - 1;
- year = FIELD_GET(RTCA3_RYRAR_YR, year);
- year100 = bcd2bin((year == 0x99) ? 0x19 : 0x20);
- tm->tm_year = (year100 * 100 + bcd2bin(year)) - 1900;
+ tm->tm_year = rtca3_decode_year(RTCA3_RYRAR_YR, year);
wkalrm->enabled = !!(readb(priv->base + RTCA3_RCR1) & RTCA3_RCR1_AIE);
--
2.54.0
^ permalink raw reply related
* Re: [PATCH] rtc: ds1307: handle oscillator stop flag for ds1337/ds1339/ds3231
From: Tyler Hicks @ 2026-05-07 15:19 UTC (permalink / raw)
To: Ronan Dalton
Cc: alexandre.belloni, linux-rtc, linux-kernel, Sasha Levin,
Meagan Lloyd, Rodolfo Giometti, Chris Packham
In-Reply-To: <20260501044657.1003980-2-ronan.dalton@alliedtelesis.co.nz>
On 2026-05-01 16:46:10, Ronan Dalton wrote:
> Prior to commit 6cb0d8587b96 ("rtc: ds1307: remove clear of oscillator
This commit hash is from the linux-6.12.y stable branch but we should
use hashes from Linus' tree in this commit message:
48458654659c ("rtc: ds1307: remove clear of oscillator stop flag (OSF) in probe")
> stop flag (OSF) in probe"), the oscillator stop flag (OSF) bit was
> checked during device probe for the ds1337, ds1339, ds1341, and ds3231
> chips; if it was set, it would be cleared and a warning would be logged
> saying "SET TIME!". Since that commit, the OSF bit is no longer cleared,
> but the warning is still printed.
>
> Directly following that commit, there was no way to get rid of this
> warning because nothing cleared the OSF bit on these chips.
>
> The commit associated with the previous commit, ae03a28e12a7 ("rtc:
The commit hash referenced here should be 523923cfd5d6.
> ds1307: handle oscillator stop flag (OSF) for ds1341"), made proper use
> of the OSF when getting and setting the time in the RTC. However, the
> other RTC variants ds1337, ds1339 and ds3231 didn't have a corresponding
> change made.
>
> Given that the OSF bit is no longer cleared at probe time when it is
> set, the remaining three chips should have the same handling as the
> ds1341 chip has for the OSF bit.
>
> Fix the issue on the ds1337, ds1339 and ds3231 chips by applying the
> same logic as the ds1341 has to these chips.
>
> Note that any devices brought up between the first referenced commit and
> this one may begin mistrusting the time reported by the RTC until it is
> set again, if the bit was never explicitly cleared.
>
> Note that only the ds1339 was tested with this change, but the
> datasheets for the other chips contain essentially identical
> descriptions of the OSF bit so the same change should work.
>
> An alternative to this change could be just to revert the referenced two
> commits and not use the OSF bit at all, apart from logging a warning and
> clearing it on probe.
>
> Signed-off-by: Ronan Dalton <ronan.dalton@alliedtelesis.co.nz>
> Cc: linux-rtc@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Cc: Alexandre Belloni <alexandre.belloni@bootlin.com>
> Cc: Tyler Hicks <code@tyhicks.com>
> Cc: Sasha Levin <sashal@kernel.org>
> Cc: Meagan Lloyd <meaganlloyd@linux.microsoft.com>
> Cc: Rodolfo Giometti <giometti@enneenne.com>
> Cc: Chris Packham <chris.packham@alliedtelesis.co.nz>
> Fixes: 6cb0d8587b96 ("rtc: ds1307: remove clear of oscillator stop flag (OSF) in probe")
Please adjust the commit hash here, as well. Everything else looks good.
Thanks!
Reviewed-by: Tyler Hicks <code@thicks.com>
Tyler
> ---
> drivers/rtc/rtc-ds1307.c | 28 +++++++++++++++++-----------
> 1 file changed, 17 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
> index 7205c59ff729..edf81b975dec 100644
> --- a/drivers/rtc/rtc-ds1307.c
> +++ b/drivers/rtc/rtc-ds1307.c
> @@ -269,6 +269,16 @@ static int ds1307_get_time(struct device *dev, struct rtc_time *t)
> if (tmp & DS1338_BIT_OSF)
> return -EINVAL;
> break;
> + case ds_1337:
> + case ds_1339:
> + case ds_1341:
> + case ds_3231:
> + ret = regmap_read(ds1307->regmap, DS1337_REG_STATUS, &tmp);
> + if (ret)
> + return ret;
> + if (tmp & DS1337_BIT_OSF)
> + return -EINVAL;
> + break;
> case ds_1340:
> if (tmp & DS1340_BIT_nEOSC)
> return -EINVAL;
> @@ -279,13 +289,6 @@ static int ds1307_get_time(struct device *dev, struct rtc_time *t)
> if (tmp & DS1340_BIT_OSF)
> return -EINVAL;
> break;
> - case ds_1341:
> - ret = regmap_read(ds1307->regmap, DS1337_REG_STATUS, &tmp);
> - if (ret)
> - return ret;
> - if (tmp & DS1337_BIT_OSF)
> - return -EINVAL;
> - break;
> case ds_1388:
> ret = regmap_read(ds1307->regmap, DS1388_REG_FLAG, &tmp);
> if (ret)
> @@ -380,14 +383,17 @@ static int ds1307_set_time(struct device *dev, struct rtc_time *t)
> regmap_update_bits(ds1307->regmap, DS1307_REG_CONTROL,
> DS1338_BIT_OSF, 0);
> break;
> + case ds_1337:
> + case ds_1339:
> + case ds_1341:
> + case ds_3231:
> + regmap_update_bits(ds1307->regmap, DS1337_REG_STATUS,
> + DS1337_BIT_OSF, 0);
> + break;
> case ds_1340:
> regmap_update_bits(ds1307->regmap, DS1340_REG_FLAG,
> DS1340_BIT_OSF, 0);
> break;
> - case ds_1341:
> - regmap_update_bits(ds1307->regmap, DS1337_REG_STATUS,
> - DS1337_BIT_OSF, 0);
> - break;
> case ds_1388:
> regmap_update_bits(ds1307->regmap, DS1388_REG_FLAG,
> DS1388_BIT_OSF, 0);
> --
> 2.53.0
>
^ permalink raw reply
* Re: [PATCH v5 07/11] leds: flash: add support for Samsung S2M series PMIC flash LED device
From: Lee Jones @ 2026-05-07 16:46 UTC (permalink / raw)
To: Kaustabh Chakraborty
Cc: Pavel Machek, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
MyungJoo Ham, Chanwoo Choi, Sebastian Reichel,
Krzysztof Kozlowski, André Draszik, Alexandre Belloni,
Jonathan Corbet, Shuah Khan, Nam Tran,
Łukasz Lebiedziński, linux-leds, devicetree,
linux-kernel, linux-pm, linux-samsung-soc, linux-rtc, linux-doc
In-Reply-To: <20260424-s2mu005-pmic-v5-7-fcbc9da5a004@disroot.org>
On Fri, 24 Apr 2026, Kaustabh Chakraborty wrote:
> Add support for flash LEDs in certain Samsung S2M series PMICs.
> The device has two channels for LEDs, typically for the back and front
> cameras in mobile devices. Both channels can be independently
> controlled, and can be operated in torch or flash modes.
>
> The driver includes initial support for the S2MU005 PMIC flash LEDs.
>
> Signed-off-by: Kaustabh Chakraborty <kauschluss@disroot.org>
> ---
> drivers/leds/flash/Kconfig | 12 ++
> drivers/leds/flash/Makefile | 1 +
> drivers/leds/flash/leds-s2m-flash.c | 358 ++++++++++++++++++++++++++++++++++++
> 3 files changed, 371 insertions(+)
>
> diff --git a/drivers/leds/flash/Kconfig b/drivers/leds/flash/Kconfig
> index 5e08102a67841..be62e05277429 100644
> --- a/drivers/leds/flash/Kconfig
> +++ b/drivers/leds/flash/Kconfig
> @@ -114,6 +114,18 @@ config LEDS_RT8515
> To compile this driver as a module, choose M here: the module
> will be called leds-rt8515.
>
> +config LEDS_S2M_FLASH
> + tristate "Samsung S2M series PMICs flash/torch LED support"
> + depends on LEDS_CLASS
> + depends on MFD_SEC_CORE
> + depends on V4L2_FLASH_LED_CLASS || !V4L2_FLASH_LED_CLASS
The `|| !V4L2_FLASH_LED_CLASS` part of this dependency makes it
unconditionally true. Was this intended? Perhaps this dependency can be
removed entirely.
> + select REGMAP_IRQ
> + help
> + This option enables support for the flash/torch LEDs found in
> + certain Samsung S2M series PMICs, such as the S2MU005. It has
> + a LED channel dedicated for every physical LED. The LEDs can
> + be controlled in flash and torch modes.
> +
> config LEDS_SGM3140
> tristate "LED support for the SGM3140"
> depends on V4L2_FLASH_LED_CLASS || !V4L2_FLASH_LED_CLASS
> diff --git a/drivers/leds/flash/Makefile b/drivers/leds/flash/Makefile
> index 712fb737a428e..44e6c1b4beb37 100644
> --- a/drivers/leds/flash/Makefile
> +++ b/drivers/leds/flash/Makefile
> @@ -10,6 +10,7 @@ obj-$(CONFIG_LEDS_MAX77693) += leds-max77693.o
> obj-$(CONFIG_LEDS_QCOM_FLASH) += leds-qcom-flash.o
> obj-$(CONFIG_LEDS_RT4505) += leds-rt4505.o
> obj-$(CONFIG_LEDS_RT8515) += leds-rt8515.o
> +obj-$(CONFIG_LEDS_S2M_FLASH) += leds-s2m-flash.o
> obj-$(CONFIG_LEDS_SGM3140) += leds-sgm3140.o
> obj-$(CONFIG_LEDS_SY7802) += leds-sy7802.o
> obj-$(CONFIG_LEDS_TPS6131X) += leds-tps6131x.o
> diff --git a/drivers/leds/flash/leds-s2m-flash.c b/drivers/leds/flash/leds-s2m-flash.c
> new file mode 100644
> index 0000000000000..177d23b432ce6
> --- /dev/null
> +++ b/drivers/leds/flash/leds-s2m-flash.c
> @@ -0,0 +1,358 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Flash and Torch LED Driver for Samsung S2M series PMICs.
> + *
> + * Copyright (c) 2015 Samsung Electronics Co., Ltd
> + * Copyright (c) 2026 Kaustabh Chakraborty <kauschluss@disroot.org>
> + */
> +
> +#include <linux/container_of.h>
> +#include <linux/led-class-flash.h>
> +#include <linux/mfd/samsung/core.h>
> +#include <linux/mfd/samsung/s2mu005.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <media/v4l2-flash-led-class.h>
> +
> +#define MAX_CHANNELS 2
> +
> +struct s2m_led {
> + struct regmap *regmap;
> + struct led_classdev_flash fled;
> + struct v4l2_flash *v4l2_flash;
> + /*
> + * The mutex object prevents the concurrent access of flash control
> + * registers by the LED and V4L2 subsystems.
> + */
> + struct mutex lock;
> + unsigned int reg_enable;
> + u8 channel;
> + u8 flash_brightness;
> + u8 flash_timeout;
> +};
> +
> +static struct s2m_led *to_s2m_led(struct led_classdev_flash *fled)
> +{
> + return container_of(fled, struct s2m_led, fled);
> +}
> +
> +static struct led_classdev_flash *to_s2m_fled(struct led_classdev *cdev)
> +{
> + return container_of(cdev, struct led_classdev_flash, led_cdev);
> +}
> +
> +static int s2m_fled_flash_brightness_set(struct led_classdev_flash *fled, u32 brightness)
> +{
> + struct s2m_led *led = to_s2m_led(fled);
> + struct led_flash_setting *setting = &fled->brightness;
> +
> + led->flash_brightness = (brightness - setting->min) / setting->step;
> +
> + return 0;
> +}
> +
> +static int s2m_fled_flash_timeout_set(struct led_classdev_flash *fled, u32 timeout)
> +{
> + struct s2m_led *led = to_s2m_led(fled);
> + struct led_flash_setting *setting = &fled->timeout;
> +
> + led->flash_timeout = (timeout - setting->min) / setting->step;
> +
> + return 0;
> +}
> +
> +#if IS_ENABLED(CONFIG_V4L2_FLASH_LED_CLASS)
> +static int s2m_fled_flash_external_strobe_set(struct v4l2_flash *v4l2_flash, bool enable)
> +{
> + struct s2m_led *led = to_led(v4l2_flash->fled_cdev);
What is to_led()?
Was this tested?
> + mutex_lock(&led->lock);
> +
> + led->fled.ops->strobe_set(&led->fled, enable);
> +
> + mutex_unlock(&led->lock);
> +
> + return 0;
> +}
> +
> +static const struct v4l2_flash_ops s2m_fled_v4l2_flash_ops = {
> + .external_strobe_set = s2m_fled_flash_external_strobe_set,
> +};
> +#else
> +static const struct v4l2_flash_ops s2m_fled_v4l2_flash_ops;
> +#endif
> +
> +static void s2m_fled_v4l2_flash_release(void *v4l2_flash)
> +{
> + v4l2_flash_release(v4l2_flash);
> +}
> +
> +static int s2mu005_fled_torch_brightness_set(struct led_classdev *cdev, enum led_brightness value)
> +{
> + struct s2m_led *led = to_s2m_led(to_s2m_fled(cdev));
> + int ret;
> +
> + mutex_lock(&led->lock);
> +
> + if (!value) {
> + ret = regmap_clear_bits(led->regmap, led->reg_enable,
> + S2MU005_FLED_TORCH_EN(led->channel));
> + if (ret < 0)
> + dev_err(cdev->dev, "failed to disable torch LED\n");
> + goto unlock;
> + }
> +
> + ret = regmap_update_bits(led->regmap, S2MU005_REG_FLED_CH_CTRL1(led->channel),
> + S2MU005_FLED_TORCH_IOUT,
> + FIELD_PREP(S2MU005_FLED_TORCH_IOUT, value - 1));
> + if (ret < 0) {
Is a positive number even possible?
> + dev_err(cdev->dev, "failed to set torch current\n");
> + goto unlock;
> + }
> +
> + ret = regmap_set_bits(led->regmap, led->reg_enable, S2MU005_FLED_TORCH_EN(led->channel));
> + if (ret < 0) {
> + dev_err(cdev->dev, "failed to enable torch LED\n");
> + goto unlock;
> + }
> +
> +unlock:
> + mutex_unlock(&led->lock);
> +
> + return ret;
> +}
> +
> +static int s2mu005_fled_flash_strobe_set(struct led_classdev_flash *fled, bool state)
> +{
> + struct s2m_led *led = to_s2m_led(fled);
> + int ret;
> +
> + mutex_lock(&led->lock);
> +
> + ret = regmap_clear_bits(led->regmap, led->reg_enable, S2MU005_FLED_FLASH_EN(led->channel));
> + if (ret < 0) {
> + dev_err(fled->led_cdev.dev, "failed to disable flash LED\n");
> + goto unlock;
> + }
> +
> + if (!state)
> + goto unlock;
> +
> + ret = regmap_update_bits(led->regmap, S2MU005_REG_FLED_CH_CTRL0(led->channel),
> + S2MU005_FLED_FLASH_IOUT,
> + FIELD_PREP(S2MU005_FLED_FLASH_IOUT, led->flash_brightness));
> + if (ret < 0) {
> + dev_err(fled->led_cdev.dev, "failed to set flash brightness\n");
> + goto unlock;
> + }
> +
> + ret = regmap_update_bits(led->regmap, S2MU005_REG_FLED_CH_CTRL3(led->channel),
> + S2MU005_FLED_FLASH_TIMEOUT,
> + FIELD_PREP(S2MU005_FLED_FLASH_TIMEOUT, led->flash_timeout));
> + if (ret < 0) {
> + dev_err(fled->led_cdev.dev, "failed to set flash timeout\n");
> + goto unlock;
> + }
> +
> + ret = regmap_set_bits(led->regmap, led->reg_enable, S2MU005_FLED_FLASH_EN(led->channel));
> + if (ret < 0) {
> + dev_err(fled->led_cdev.dev, "failed to enable flash LED\n");
> + goto unlock;
> + }
> +
> +unlock:
> + mutex_unlock(&led->lock);
> +
> + return 0;
It seems like this function swallows error codes.
Better if they're propagated properly.
> +}
> +
> +static int s2mu005_fled_flash_strobe_get(struct led_classdev_flash *fled, bool *state)
> +{
> + struct s2m_led *led = to_s2m_led(fled);
> + u32 val;
> + int ret;
> +
> + mutex_lock(&led->lock);
> +
> + ret = regmap_read(led->regmap, S2MU005_REG_FLED_STATUS, &val);
> + if (ret < 0) {
> + dev_err(fled->led_cdev.dev, "failed to fetch LED status");
Missed '/n'.
> + goto unlock;
> + }
> +
> + *state = !!(val & S2MU005_FLED_FLASH_STATUS(led->channel));
> +
> +unlock:
> + mutex_unlock(&led->lock);
> +
> + return ret;
> +}
> +
> +static const struct led_flash_ops s2mu005_fled_flash_ops = {
> + .flash_brightness_set = s2m_fled_flash_brightness_set,
> + .timeout_set = s2m_fled_flash_timeout_set,
> + .strobe_set = s2mu005_fled_flash_strobe_set,
> + .strobe_get = s2mu005_fled_flash_strobe_get,
> +};
> +
> +static int s2mu005_fled_init(struct s2m_led *led, struct device *dev, struct regmap *regmap,
> + unsigned int nr_channels)
> +{
> + unsigned int val;
> + int i, ret;
> +
> + /* Enable the LED channels. */
> + ret = regmap_set_bits(regmap, S2MU005_REG_FLED_CTRL1, S2MU005_FLED_CH_EN);
> + if (ret < 0)
> + return dev_err_probe(dev, ret, "failed to enable LED channels\n");
> +
> + ret = regmap_read(regmap, S2MU005_REG_ID, &val);
> + if (ret < 0)
> + return dev_err_probe(dev, ret, "failed to read revision\n");
> +
> + for (i = 0; i < nr_channels; i++) {
for (int i = 0; i < nr_channels; i++)
> + /*
> + * Read the revision register. Revision EVT0 has the register
> + * at CTRL4, while EVT1 and higher have it at CTRL6.
> + */
> + if (FIELD_GET(S2MU005_ID_MASK, val) == 0)
Why not remove the " == 0" and reverse the branches?
> + led[i].reg_enable = S2MU005_REG_FLED_CTRL4;
> + else
> + led[i].reg_enable = S2MU005_REG_FLED_CTRL6;
> + }
> +
> + return 0;
> +}
> +
> +static int s2mu005_fled_init_channel(struct s2m_led *led, struct device *dev,
> + struct fwnode_handle *fwnp)
> +{
> + struct led_classdev *cdev = &led->fled.led_cdev;
> + struct led_init_data init_data = {};
> + struct v4l2_flash_config v4l2_cfg = {};
> + int ret;
> +
> + cdev->max_brightness = 16;
> + cdev->brightness_set_blocking = s2mu005_fled_torch_brightness_set;
> + cdev->flags |= LED_DEV_CAP_FLASH;
> +
> + led->fled.timeout.min = 62000;
> + led->fled.timeout.step = 62000;
> + led->fled.timeout.max = 992000;
> + led->fled.timeout.val = 992000;
> +
> + led->fled.brightness.min = 25000;
> + led->fled.brightness.step = 25000;
> + led->fled.brightness.max = 375000; /* 400000 causes flickering */
> + led->fled.brightness.val = 375000;
> +
> + s2m_fled_flash_timeout_set(&led->fled, led->fled.timeout.val);
> + s2m_fled_flash_brightness_set(&led->fled, led->fled.brightness.val);
> +
> + led->fled.ops = &s2mu005_fled_flash_ops;
> +
> + init_data.fwnode = fwnp;
> + ret = devm_led_classdev_flash_register_ext(dev, &led->fled, &init_data);
> + if (ret < 0)
> + return dev_err_probe(dev, ret, "failed to create LED flash device\n");
> +
> + v4l2_cfg.intensity.min = led->fled.timeout.min;
> + v4l2_cfg.intensity.step = led->fled.timeout.step;
> + v4l2_cfg.intensity.max = led->fled.timeout.max;
> + v4l2_cfg.intensity.val = led->fled.timeout.val;
Is it correct to configure the V4L2 intensity settings from the timeout
values? I would expect these to be based on the brightness settings.
> +
> + v4l2_cfg.has_external_strobe = true;
> +
> + led->v4l2_flash = v4l2_flash_init(dev, fwnp, &led->fled, &s2m_fled_v4l2_flash_ops,
> + &v4l2_cfg);
> + if (IS_ERR(led->v4l2_flash)) {
> + v4l2_flash_release(led->v4l2_flash);
So you're going to try and release an error?
> + return dev_err_probe(dev, PTR_ERR(led->v4l2_flash),
> + "failed to create V4L2 flash device\n");
> + }
> +
> + ret = devm_add_action_or_reset(dev, (void *)s2m_fled_v4l2_flash_release, led->v4l2_flash);
> + if (ret < 0)
> + return dev_err_probe(dev, ret, "failed to add cleanup action\n");
> +
> + return 0;
> +}
> +
> +static int s2m_fled_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct sec_pmic_dev *ddata = dev_get_drvdata(dev->parent);
> + struct s2m_led *led;
> + int ret;
> +
> + led = devm_kzalloc(dev, sizeof(*led) * MAX_CHANNELS, GFP_KERNEL);
> + if (!led)
> + return -ENOMEM;
> +
> + switch (platform_get_device_id(pdev)->driver_data) {
> + case S2MU005:
> + ret = s2mu005_fled_init(led, dev, ddata->regmap_pmic, MAX_CHANNELS);
> + if (ret)
> + return ret;
> + break;
> + default:
> + return dev_err_probe(dev, -ENODEV, "device type %d is not supported by driver\n",
> + ddata->device_type);
> + }
Will this be expanded in the very near future?
If not, having a switch () with only one entry seems odd.
if (platform_get_device_id(pdev)->driver_data != S2MU005)
dev_err_probe()
> + device_for_each_child_node_scoped(dev, child) {
> + u32 reg;
> +
> + if (fwnode_property_read_u32(child, "reg", ®))
> + continue;
> +
> + if (led[reg].regmap) {
> + dev_warn(dev, "duplicate node for channel %d\n", reg);
> + continue;
> + }
If reg > MAX_CHANNELS, you just created an OOB condition.
> +
> + led[reg].regmap = ddata->regmap_pmic;
> + led[reg].channel = (u8)reg;
> +
> + ret = devm_mutex_init(dev, &led[reg].lock);
> + if (ret)
> + return dev_err_probe(dev, ret, "failed to create mutex\n");
> +
> + switch (platform_get_device_id(pdev)->driver_data) {
> + case S2MU005:
> + ret = s2mu005_fled_init_channel(led + reg, dev, child);
> + if (ret < 0)
> + return ret;
> + break;
> + }
This is even more odd!
> + }
> +
> + return 0;
> +}
> +
> +static const struct platform_device_id s2m_fled_id_table[] = {
> + { "s2mu005-flash", S2MU005 },
> + { /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(platform, s2m_fled_id_table);
> +
> +static const struct of_device_id s2m_fled_of_match_table[] = {
> + { .compatible = "samsung,s2mu005-flash", .data = (void *)S2MU005 },
> + { /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(of, s2m_fled_of_match_table);
> +
> +static struct platform_driver s2m_fled_driver = {
> + .driver = {
> + .name = "s2m-flash",
> + },
> + .probe = s2m_fled_probe,
> + .id_table = s2m_fled_id_table,
> +};
> +module_platform_driver(s2m_fled_driver);
> +
> +MODULE_DESCRIPTION("Flash/Torch LED Driver For Samsung S2M Series PMICs");
> +MODULE_AUTHOR("Kaustabh Chakraborty <kauschluss@disroot.org>");
> +MODULE_LICENSE("GPL");
--
Lee Jones
^ permalink raw reply
* Re: [PATCH v5 07/11] leds: flash: add support for Samsung S2M series PMIC flash LED device
From: Kaustabh Chakraborty @ 2026-05-07 18:33 UTC (permalink / raw)
To: Lee Jones, Kaustabh Chakraborty
Cc: Pavel Machek, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
MyungJoo Ham, Chanwoo Choi, Sebastian Reichel,
Krzysztof Kozlowski, André Draszik, Alexandre Belloni,
Jonathan Corbet, Shuah Khan, Nam Tran,
Łukasz Lebiedziński, linux-leds, devicetree,
linux-kernel, linux-pm, linux-samsung-soc, linux-rtc, linux-doc
In-Reply-To: <20260507164654.GS305027@google.com>
On 2026-05-07 17:46 +01:00, Lee Jones wrote:
> On Fri, 24 Apr 2026, Kaustabh Chakraborty wrote:
>
>> Add support for flash LEDs in certain Samsung S2M series PMICs.
>> The device has two channels for LEDs, typically for the back and front
>> cameras in mobile devices. Both channels can be independently
>> controlled, and can be operated in torch or flash modes.
>>
>> The driver includes initial support for the S2MU005 PMIC flash LEDs.
>>
>> Signed-off-by: Kaustabh Chakraborty <kauschluss@disroot.org>
>> ---
>> drivers/leds/flash/Kconfig | 12 ++
>> drivers/leds/flash/Makefile | 1 +
>> drivers/leds/flash/leds-s2m-flash.c | 358 ++++++++++++++++++++++++++++++++++++
>> 3 files changed, 371 insertions(+)
>>
>> diff --git a/drivers/leds/flash/Kconfig b/drivers/leds/flash/Kconfig
>> index 5e08102a67841..be62e05277429 100644
>> --- a/drivers/leds/flash/Kconfig
>> +++ b/drivers/leds/flash/Kconfig
>> @@ -114,6 +114,18 @@ config LEDS_RT8515
>> To compile this driver as a module, choose M here: the module
>> will be called leds-rt8515.
>>
>> +config LEDS_S2M_FLASH
>> + tristate "Samsung S2M series PMICs flash/torch LED support"
>> + depends on LEDS_CLASS
>> + depends on MFD_SEC_CORE
>> + depends on V4L2_FLASH_LED_CLASS || !V4L2_FLASH_LED_CLASS
>
> The `|| !V4L2_FLASH_LED_CLASS` part of this dependency makes it
> unconditionally true. Was this intended? Perhaps this dependency can be
> removed entirely.
Right? Similar lines are also present in entries of other drivers too.
It is indeed weird, but I disregarded my doubts and added it anyway.
>> + select REGMAP_IRQ
>> + help
>> + This option enables support for the flash/torch LEDs found in
>> + certain Samsung S2M series PMICs, such as the S2MU005. It has
>> + a LED channel dedicated for every physical LED. The LEDs can
>> + be controlled in flash and torch modes.
>> +
>> config LEDS_SGM3140
>> tristate "LED support for the SGM3140"
>> depends on V4L2_FLASH_LED_CLASS || !V4L2_FLASH_LED_CLASS
>> diff --git a/drivers/leds/flash/Makefile b/drivers/leds/flash/Makefile
>> index 712fb737a428e..44e6c1b4beb37 100644
>> --- a/drivers/leds/flash/Makefile
>> +++ b/drivers/leds/flash/Makefile
>> @@ -10,6 +10,7 @@ obj-$(CONFIG_LEDS_MAX77693) += leds-max77693.o
>> obj-$(CONFIG_LEDS_QCOM_FLASH) += leds-qcom-flash.o
>> obj-$(CONFIG_LEDS_RT4505) += leds-rt4505.o
>> obj-$(CONFIG_LEDS_RT8515) += leds-rt8515.o
>> +obj-$(CONFIG_LEDS_S2M_FLASH) += leds-s2m-flash.o
>> obj-$(CONFIG_LEDS_SGM3140) += leds-sgm3140.o
>> obj-$(CONFIG_LEDS_SY7802) += leds-sy7802.o
>> obj-$(CONFIG_LEDS_TPS6131X) += leds-tps6131x.o
>> diff --git a/drivers/leds/flash/leds-s2m-flash.c b/drivers/leds/flash/leds-s2m-flash.c
>> new file mode 100644
>> index 0000000000000..177d23b432ce6
>> --- /dev/null
>> +++ b/drivers/leds/flash/leds-s2m-flash.c
>> @@ -0,0 +1,358 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Flash and Torch LED Driver for Samsung S2M series PMICs.
>> + *
>> + * Copyright (c) 2015 Samsung Electronics Co., Ltd
>> + * Copyright (c) 2026 Kaustabh Chakraborty <kauschluss@disroot.org>
>> + */
>> +
>> +#include <linux/container_of.h>
>> +#include <linux/led-class-flash.h>
>> +#include <linux/mfd/samsung/core.h>
>> +#include <linux/mfd/samsung/s2mu005.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/regmap.h>
>> +#include <media/v4l2-flash-led-class.h>
>> +
>> +#define MAX_CHANNELS 2
>> +
>> +struct s2m_led {
>> + struct regmap *regmap;
>> + struct led_classdev_flash fled;
>> + struct v4l2_flash *v4l2_flash;
>> + /*
>> + * The mutex object prevents the concurrent access of flash control
>> + * registers by the LED and V4L2 subsystems.
>> + */
>> + struct mutex lock;
>> + unsigned int reg_enable;
>> + u8 channel;
>> + u8 flash_brightness;
>> + u8 flash_timeout;
>> +};
>> +
>> +static struct s2m_led *to_s2m_led(struct led_classdev_flash *fled)
>> +{
>> + return container_of(fled, struct s2m_led, fled);
>> +}
>> +
>> +static struct led_classdev_flash *to_s2m_fled(struct led_classdev *cdev)
>> +{
>> + return container_of(cdev, struct led_classdev_flash, led_cdev);
>> +}
>> +
>> +static int s2m_fled_flash_brightness_set(struct led_classdev_flash *fled, u32 brightness)
>> +{
>> + struct s2m_led *led = to_s2m_led(fled);
>> + struct led_flash_setting *setting = &fled->brightness;
>> +
>> + led->flash_brightness = (brightness - setting->min) / setting->step;
>> +
>> + return 0;
>> +}
>> +
>> +static int s2m_fled_flash_timeout_set(struct led_classdev_flash *fled, u32 timeout)
>> +{
>> + struct s2m_led *led = to_s2m_led(fled);
>> + struct led_flash_setting *setting = &fled->timeout;
>> +
>> + led->flash_timeout = (timeout - setting->min) / setting->step;
>> +
>> + return 0;
>> +}
>> +
>> +#if IS_ENABLED(CONFIG_V4L2_FLASH_LED_CLASS)
>> +static int s2m_fled_flash_external_strobe_set(struct v4l2_flash *v4l2_flash, bool enable)
>> +{
>> + struct s2m_led *led = to_led(v4l2_flash->fled_cdev);
>
> What is to_led()?
>
> Was this tested?
I honestly don't know what happened here, and yes this (well, not this
precisely) was tested. I had changed something later? Don't know, its
odd. Will fix.
>> + mutex_lock(&led->lock);
>> +
>> + led->fled.ops->strobe_set(&led->fled, enable);
>> +
>> + mutex_unlock(&led->lock);
>> +
>> + return 0;
>> +}
>> +
>> +static const struct v4l2_flash_ops s2m_fled_v4l2_flash_ops = {
>> + .external_strobe_set = s2m_fled_flash_external_strobe_set,
>> +};
>> +#else
>> +static const struct v4l2_flash_ops s2m_fled_v4l2_flash_ops;
>> +#endif
>> +
>> +static void s2m_fled_v4l2_flash_release(void *v4l2_flash)
>> +{
>> + v4l2_flash_release(v4l2_flash);
>> +}
>> +
>> +static int s2mu005_fled_torch_brightness_set(struct led_classdev *cdev, enum led_brightness value)
>> +{
>> + struct s2m_led *led = to_s2m_led(to_s2m_fled(cdev));
>> + int ret;
>> +
>> + mutex_lock(&led->lock);
>> +
>> + if (!value) {
>> + ret = regmap_clear_bits(led->regmap, led->reg_enable,
>> + S2MU005_FLED_TORCH_EN(led->channel));
>> + if (ret < 0)
>> + dev_err(cdev->dev, "failed to disable torch LED\n");
>> + goto unlock;
>> + }
>> +
>> + ret = regmap_update_bits(led->regmap, S2MU005_REG_FLED_CH_CTRL1(led->channel),
>> + S2MU005_FLED_TORCH_IOUT,
>> + FIELD_PREP(S2MU005_FLED_TORCH_IOUT, value - 1));
>> + if (ret < 0) {
>
> Is a positive number even possible?
As per the docs, no. Will fix here and in other instances as well.
>> + dev_err(cdev->dev, "failed to set torch current\n");
>> + goto unlock;
>> + }
>> +
>> + ret = regmap_set_bits(led->regmap, led->reg_enable, S2MU005_FLED_TORCH_EN(led->channel));
>> + if (ret < 0) {
>> + dev_err(cdev->dev, "failed to enable torch LED\n");
>> + goto unlock;
>> + }
>> +
>> +unlock:
>> + mutex_unlock(&led->lock);
>> +
>> + return ret;
>> +}
>> +
>> +static int s2mu005_fled_flash_strobe_set(struct led_classdev_flash *fled, bool state)
>> +{
>> + struct s2m_led *led = to_s2m_led(fled);
>> + int ret;
>> +
>> + mutex_lock(&led->lock);
>> +
>> + ret = regmap_clear_bits(led->regmap, led->reg_enable, S2MU005_FLED_FLASH_EN(led->channel));
>> + if (ret < 0) {
>> + dev_err(fled->led_cdev.dev, "failed to disable flash LED\n");
>> + goto unlock;
>> + }
>> +
>> + if (!state)
>> + goto unlock;
>> +
>> + ret = regmap_update_bits(led->regmap, S2MU005_REG_FLED_CH_CTRL0(led->channel),
>> + S2MU005_FLED_FLASH_IOUT,
>> + FIELD_PREP(S2MU005_FLED_FLASH_IOUT, led->flash_brightness));
>> + if (ret < 0) {
>> + dev_err(fled->led_cdev.dev, "failed to set flash brightness\n");
>> + goto unlock;
>> + }
>> +
>> + ret = regmap_update_bits(led->regmap, S2MU005_REG_FLED_CH_CTRL3(led->channel),
>> + S2MU005_FLED_FLASH_TIMEOUT,
>> + FIELD_PREP(S2MU005_FLED_FLASH_TIMEOUT, led->flash_timeout));
>> + if (ret < 0) {
>> + dev_err(fled->led_cdev.dev, "failed to set flash timeout\n");
>> + goto unlock;
>> + }
>> +
>> + ret = regmap_set_bits(led->regmap, led->reg_enable, S2MU005_FLED_FLASH_EN(led->channel));
>> + if (ret < 0) {
>> + dev_err(fled->led_cdev.dev, "failed to enable flash LED\n");
>> + goto unlock;
>> + }
>> +
>> +unlock:
>> + mutex_unlock(&led->lock);
>> +
>> + return 0;
>
> It seems like this function swallows error codes.
>
> Better if they're propagated properly.
>
>> +}
>> +
>> +static int s2mu005_fled_flash_strobe_get(struct led_classdev_flash *fled, bool *state)
>> +{
>> + struct s2m_led *led = to_s2m_led(fled);
>> + u32 val;
>> + int ret;
>> +
>> + mutex_lock(&led->lock);
>> +
>> + ret = regmap_read(led->regmap, S2MU005_REG_FLED_STATUS, &val);
>> + if (ret < 0) {
>> + dev_err(fled->led_cdev.dev, "failed to fetch LED status");
>
> Missed '/n'.
>
>> + goto unlock;
>> + }
>> +
>> + *state = !!(val & S2MU005_FLED_FLASH_STATUS(led->channel));
>> +
>> +unlock:
>> + mutex_unlock(&led->lock);
>> +
>> + return ret;
>> +}
>> +
>> +static const struct led_flash_ops s2mu005_fled_flash_ops = {
>> + .flash_brightness_set = s2m_fled_flash_brightness_set,
>> + .timeout_set = s2m_fled_flash_timeout_set,
>> + .strobe_set = s2mu005_fled_flash_strobe_set,
>> + .strobe_get = s2mu005_fled_flash_strobe_get,
>> +};
>> +
>> +static int s2mu005_fled_init(struct s2m_led *led, struct device *dev, struct regmap *regmap,
>> + unsigned int nr_channels)
>> +{
>> + unsigned int val;
>> + int i, ret;
>> +
>> + /* Enable the LED channels. */
>> + ret = regmap_set_bits(regmap, S2MU005_REG_FLED_CTRL1, S2MU005_FLED_CH_EN);
>> + if (ret < 0)
>> + return dev_err_probe(dev, ret, "failed to enable LED channels\n");
>> +
>> + ret = regmap_read(regmap, S2MU005_REG_ID, &val);
>> + if (ret < 0)
>> + return dev_err_probe(dev, ret, "failed to read revision\n");
>> +
>> + for (i = 0; i < nr_channels; i++) {
>
> for (int i = 0; i < nr_channels; i++)
Is that allowed in the kernel source? I have never seen variable
declaration in a for loop.
>> + /*
>> + * Read the revision register. Revision EVT0 has the register
>> + * at CTRL4, while EVT1 and higher have it at CTRL6.
>> + */
>> + if (FIELD_GET(S2MU005_ID_MASK, val) == 0)
>
> Why not remove the " == 0" and reverse the branches?
My intention was to make it explicit that the value used is an integer
value, as opposed to a boolean.
>> + led[i].reg_enable = S2MU005_REG_FLED_CTRL4;
>> + else
>> + led[i].reg_enable = S2MU005_REG_FLED_CTRL6;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int s2mu005_fled_init_channel(struct s2m_led *led, struct device *dev,
>> + struct fwnode_handle *fwnp)
>> +{
>> + struct led_classdev *cdev = &led->fled.led_cdev;
>> + struct led_init_data init_data = {};
>> + struct v4l2_flash_config v4l2_cfg = {};
>> + int ret;
>> +
>> + cdev->max_brightness = 16;
>> + cdev->brightness_set_blocking = s2mu005_fled_torch_brightness_set;
>> + cdev->flags |= LED_DEV_CAP_FLASH;
>> +
>> + led->fled.timeout.min = 62000;
>> + led->fled.timeout.step = 62000;
>> + led->fled.timeout.max = 992000;
>> + led->fled.timeout.val = 992000;
>> +
>> + led->fled.brightness.min = 25000;
>> + led->fled.brightness.step = 25000;
>> + led->fled.brightness.max = 375000; /* 400000 causes flickering */
>> + led->fled.brightness.val = 375000;
>> +
>> + s2m_fled_flash_timeout_set(&led->fled, led->fled.timeout.val);
>> + s2m_fled_flash_brightness_set(&led->fled, led->fled.brightness.val);
>> +
>> + led->fled.ops = &s2mu005_fled_flash_ops;
>> +
>> + init_data.fwnode = fwnp;
>> + ret = devm_led_classdev_flash_register_ext(dev, &led->fled, &init_data);
>> + if (ret < 0)
>> + return dev_err_probe(dev, ret, "failed to create LED flash device\n");
>> +
>> + v4l2_cfg.intensity.min = led->fled.timeout.min;
>> + v4l2_cfg.intensity.step = led->fled.timeout.step;
>> + v4l2_cfg.intensity.max = led->fled.timeout.max;
>> + v4l2_cfg.intensity.val = led->fled.timeout.val;
>
> Is it correct to configure the V4L2 intensity settings from the timeout
> values? I would expect these to be based on the brightness settings.
Stupid me. Admittedly, I am unable to test the v4l2 functionality
properly for now. I will remove all related code in the next revision.
Will add them back when they're needed and I'm able to test.
>> +
>> + v4l2_cfg.has_external_strobe = true;
>> +
>> + led->v4l2_flash = v4l2_flash_init(dev, fwnp, &led->fled, &s2m_fled_v4l2_flash_ops,
>> + &v4l2_cfg);
>> + if (IS_ERR(led->v4l2_flash)) {
>> + v4l2_flash_release(led->v4l2_flash);
>
> So you're going to try and release an error?
>
>> + return dev_err_probe(dev, PTR_ERR(led->v4l2_flash),
>> + "failed to create V4L2 flash device\n");
>> + }
>> +
>> + ret = devm_add_action_or_reset(dev, (void *)s2m_fled_v4l2_flash_release, led->v4l2_flash);
>> + if (ret < 0)
>> + return dev_err_probe(dev, ret, "failed to add cleanup action\n");
>> +
>> + return 0;
>> +}
>> +
>> +static int s2m_fled_probe(struct platform_device *pdev)
>> +{
>> + struct device *dev = &pdev->dev;
>> + struct sec_pmic_dev *ddata = dev_get_drvdata(dev->parent);
>> + struct s2m_led *led;
>> + int ret;
>> +
>> + led = devm_kzalloc(dev, sizeof(*led) * MAX_CHANNELS, GFP_KERNEL);
>> + if (!led)
>> + return -ENOMEM;
>> +
>> + switch (platform_get_device_id(pdev)->driver_data) {
>> + case S2MU005:
>> + ret = s2mu005_fled_init(led, dev, ddata->regmap_pmic, MAX_CHANNELS);
>> + if (ret)
>> + return ret;
>> + break;
>> + default:
>> + return dev_err_probe(dev, -ENODEV, "device type %d is not supported by driver\n",
>> + ddata->device_type);
>> + }
>
> Will this be expanded in the very near future?
>
> If not, having a switch () with only one entry seems odd.
I have plans to introduce support for S2MU004 at one point. I'll change
it now, and re-introduce the switch block later.
> if (platform_get_device_id(pdev)->driver_data != S2MU005)
> dev_err_probe()
>
>> + device_for_each_child_node_scoped(dev, child) {
>> + u32 reg;
>> +
>> + if (fwnode_property_read_u32(child, "reg", ®))
>> + continue;
>> +
>> + if (led[reg].regmap) {
>> + dev_warn(dev, "duplicate node for channel %d\n", reg);
>> + continue;
>> + }
>
> If reg > MAX_CHANNELS, you just created an OOB condition.
>
>> +
>> + led[reg].regmap = ddata->regmap_pmic;
>> + led[reg].channel = (u8)reg;
>> +
>> + ret = devm_mutex_init(dev, &led[reg].lock);
>> + if (ret)
>> + return dev_err_probe(dev, ret, "failed to create mutex\n");
>> +
>> + switch (platform_get_device_id(pdev)->driver_data) {
>> + case S2MU005:
>> + ret = s2mu005_fled_init_channel(led + reg, dev, child);
>> + if (ret < 0)
>> + return ret;
>> + break;
>> + }
>
> This is even more odd!
What's exactly odd about it?
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static const struct platform_device_id s2m_fled_id_table[] = {
>> + { "s2mu005-flash", S2MU005 },
>> + { /* sentinel */ },
>> +};
>> +MODULE_DEVICE_TABLE(platform, s2m_fled_id_table);
>> +
>> +static const struct of_device_id s2m_fled_of_match_table[] = {
>> + { .compatible = "samsung,s2mu005-flash", .data = (void *)S2MU005 },
>> + { /* sentinel */ },
>> +};
>> +MODULE_DEVICE_TABLE(of, s2m_fled_of_match_table);
>> +
>> +static struct platform_driver s2m_fled_driver = {
>> + .driver = {
>> + .name = "s2m-flash",
>> + },
>> + .probe = s2m_fled_probe,
>> + .id_table = s2m_fled_id_table,
>> +};
>> +module_platform_driver(s2m_fled_driver);
>> +
>> +MODULE_DESCRIPTION("Flash/Torch LED Driver For Samsung S2M Series PMICs");
>> +MODULE_AUTHOR("Kaustabh Chakraborty <kauschluss@disroot.org>");
>> +MODULE_LICENSE("GPL");
^ permalink raw reply
* Re: [PATCH v5 08/11] leds: rgb: add support for Samsung S2M series PMIC RGB LED device
From: Lee Jones @ 2026-05-07 19:00 UTC (permalink / raw)
To: Kaustabh Chakraborty
Cc: Pavel Machek, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
MyungJoo Ham, Chanwoo Choi, Sebastian Reichel,
Krzysztof Kozlowski, André Draszik, Alexandre Belloni,
Jonathan Corbet, Shuah Khan, Nam Tran,
Łukasz Lebiedziński, linux-leds, devicetree,
linux-kernel, linux-pm, linux-samsung-soc, linux-rtc, linux-doc
In-Reply-To: <20260424-s2mu005-pmic-v5-8-fcbc9da5a004@disroot.org>
On Fri, 24 Apr 2026, Kaustabh Chakraborty wrote:
> Add support for the RGB LEDs found in certain Samsung S2M series PMICs.
> The device has three LED channels, controlled as a single device. These
> LEDs are typically used as status indicators in mobile phones.
>
> The driver includes initial support for the S2MU005 PMIC RGB LEDs.
>
> Signed-off-by: Kaustabh Chakraborty <kauschluss@disroot.org>
> ---
> drivers/leds/rgb/Kconfig | 11 +
> drivers/leds/rgb/Makefile | 1 +
> drivers/leds/rgb/leds-s2m-rgb.c | 446 ++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 458 insertions(+)
>
> diff --git a/drivers/leds/rgb/Kconfig b/drivers/leds/rgb/Kconfig
> index 28ef4c487367c..30051342f4e4d 100644
> --- a/drivers/leds/rgb/Kconfig
> +++ b/drivers/leds/rgb/Kconfig
> @@ -75,6 +75,17 @@ config LEDS_QCOM_LPG
>
> If compiled as a module, the module will be named leds-qcom-lpg.
>
> +config LEDS_S2M_RGB
> + tristate "Samsung S2M series PMICs RGB LED support"
> + depends on LEDS_CLASS
> + depends on MFD_SEC_CORE
> + select REGMAP_IRQ
> + help
> + This option enables support for the S2MU005 RGB LEDs. These
> + devices have three LED channels, with 8-bit brightness control
> + for each channel. It's usually found in mobile phones as
"The S2MU005 is ..."
> + status indicators.
> +
> config LEDS_MT6370_RGB
> tristate "LED Support for MediaTek MT6370 PMIC"
> depends on MFD_MT6370
> diff --git a/drivers/leds/rgb/Makefile b/drivers/leds/rgb/Makefile
> index be45991f63f50..98050e1aa4255 100644
> --- a/drivers/leds/rgb/Makefile
> +++ b/drivers/leds/rgb/Makefile
> @@ -6,4 +6,5 @@ obj-$(CONFIG_LEDS_LP5812) += leds-lp5812.o
> obj-$(CONFIG_LEDS_NCP5623) += leds-ncp5623.o
> obj-$(CONFIG_LEDS_PWM_MULTICOLOR) += leds-pwm-multicolor.o
> obj-$(CONFIG_LEDS_QCOM_LPG) += leds-qcom-lpg.o
> +obj-$(CONFIG_LEDS_S2M_RGB) += leds-s2m-rgb.o
> obj-$(CONFIG_LEDS_MT6370_RGB) += leds-mt6370-rgb.o
> diff --git a/drivers/leds/rgb/leds-s2m-rgb.c b/drivers/leds/rgb/leds-s2m-rgb.c
> new file mode 100644
> index 0000000000000..51d12f2ef762a
> --- /dev/null
> +++ b/drivers/leds/rgb/leds-s2m-rgb.c
> @@ -0,0 +1,446 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * RGB LED Driver for Samsung S2M series PMICs.
> + *
> + * Copyright (c) 2015 Samsung Electronics Co., Ltd
> + * Copyright (c) 2026 Kaustabh Chakraborty <kauschluss@disroot.org>
> + */
> +
> +#include <linux/container_of.h>
> +#include <linux/led-class-multicolor.h>
> +#include <linux/mfd/samsung/core.h>
> +#include <linux/mfd/samsung/s2mu005.h>
> +#include <linux/minmax.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +
> +struct s2m_rgb {
> + struct device *dev;
> + struct regmap *regmap;
> + struct led_classdev_mc mc;
> + enum sec_device_type device_type;
> + /*
> + * The mutex object prevents race conditions when evaluation and
> + * application of LED pattern state.
> + */
> + struct mutex lock;
> + /*
> + * State variables representing the current LED pattern, these only to
> + * be accessed when lock is held.
> + */
> + u8 ramp_up;
> + u8 ramp_dn;
> + u8 stay_hi;
> + u8 stay_lo;
> +};
> +
> +static struct led_classdev_mc *to_s2m_mc(struct led_classdev *cdev)
> +{
> + return container_of(cdev, struct led_classdev_mc, led_cdev);
> +}
> +
> +static struct s2m_rgb *to_s2m_rgb(struct led_classdev_mc *mc)
> +{
> + return container_of(mc, struct s2m_rgb, mc);
> +}
> +
> +static const u32 s2mu005_rgb_lut_ramp[] = {
> + 0, 100, 200, 300, 400, 500, 600, 700,
> + 800, 1000, 1200, 1400, 1600, 1800, 2000, 2200,
> +};
> +
> +static const u32 s2mu005_rgb_lut_stay_hi[] = {
> + 100, 200, 300, 400, 500, 750, 1000, 1250,
> + 1500, 1750, 2000, 2250, 2500, 2750, 3000, 3250,
> +};
> +
> +static const u32 s2mu005_rgb_lut_stay_lo[] = {
> + 0, 500, 1000, 1500, 2000, 2500, 3000, 3500,
> + 4000, 4500, 5000, 6000, 7000, 8000, 10000, 12000,
> +};
> +
> +static int s2mu005_rgb_apply_params(struct s2m_rgb *rgb)
> +{
> + struct regmap *regmap = rgb->regmap;
> + unsigned int ramp_val = 0;
> + unsigned int stay_val = 0;
> + int ret;
> + int i;
> +
> + ramp_val |= FIELD_PREP(S2MU005_RGB_CH_RAMP_UP, rgb->ramp_up);
> + ramp_val |= FIELD_PREP(S2MU005_RGB_CH_RAMP_DN, rgb->ramp_dn);
> +
> + stay_val |= FIELD_PREP(S2MU005_RGB_CH_STAY_HI, rgb->stay_hi);
> + stay_val |= FIELD_PREP(S2MU005_RGB_CH_STAY_LO, rgb->stay_lo);
> +
> + ret = regmap_write(regmap, S2MU005_REG_RGB_EN, S2MU005_RGB_RESET);
> + if (ret < 0) {
> + dev_err(rgb->dev, "failed to reset RGB LEDs\n");
> + return ret;
> + }
> +
> + for (i = 0; i < rgb->mc.num_colors; i++) {
for (int i = 0; ...)
> + ret = regmap_write(regmap, S2MU005_REG_RGB_CH_CTRL(i),
> + rgb->mc.subled_info[i].brightness);
> + if (ret < 0) {
> + dev_err(rgb->dev, "failed to set LED brightness\n");
> + return ret;
> + }
> +
> + ret = regmap_write(regmap, S2MU005_REG_RGB_CH_RAMP(i), ramp_val);
> + if (ret < 0) {
> + dev_err(rgb->dev, "failed to set ramp timings\n");
> + return ret;
> + }
> +
> + ret = regmap_write(regmap, S2MU005_REG_RGB_CH_STAY(i), stay_val);
> + if (ret < 0) {
> + dev_err(rgb->dev, "failed to set stay timings\n");
> + return ret;
> + }
> + }
> +
> + ret = regmap_update_bits(regmap, S2MU005_REG_RGB_EN, S2MU005_RGB_SLOPE,
> + S2MU005_RGB_SLOPE_SMOOTH);
> + if (ret < 0) {
> + dev_err(rgb->dev, "failed to set ramp slope\n");
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static int s2mu005_rgb_reset_params(struct s2m_rgb *rgb)
> +{
> + struct regmap *regmap = rgb->regmap;
> + int ret;
> +
> + ret = regmap_write(regmap, S2MU005_REG_RGB_EN, S2MU005_RGB_RESET);
> + if (ret < 0) {
> + dev_err(rgb->dev, "failed to reset RGB LEDs\n");
> + return ret;
> + }
> +
> + rgb->ramp_up = 0;
> + rgb->ramp_dn = 0;
> + rgb->stay_hi = 0;
> + rgb->stay_lo = 0;
> +
> + return 0;
> +}
> +
> +static int s2m_rgb_lut_calc_timing(const u32 *lut, const size_t len,
> + const u32 req_time, u8 *idx)
> +{
> + int lo = 0;
> + int hi = len - 2;
> +
> + /* Bounds checking */
> + if (req_time < lut[0] || req_time > lut[len - 1])
> + return -EINVAL;
> +
> + /*
> + * Perform a binary search to pick the best timing from the LUT.
> + *
> + * The search algorithm picks two consecutive elements of the
> + * LUT and tries to search the pair between which the requested
> + * time lies.
> + */
> + while (lo <= hi) {
> + *idx = (lo + hi) / 2;
> +
> + if ((lut[*idx] <= req_time) && (req_time <= lut[*idx + 1]))
> + break;
> +
> + if ((req_time < lut[*idx]) && (req_time < lut[*idx + 1]))
> + hi = *idx - 1;
> + else
> + lo = *idx + 1;
> + }
> +
> + /*
> + * The searched timing is always less than the requested time. At
> + * times, the succeeding timing in the LUT is closer thus more
> + * accurate. Adjust the resulting value if that's the case.
> + */
> + if (abs(req_time - lut[*idx]) > abs(lut[*idx + 1] - req_time))
> + (*idx)++;
As much as I appreciate the comments, most of the function is pretty
unreadable. Are you able to use better variable nomenclature and layout
to better describe your aims?
> + return 0;
> +}
> +
> +static int s2m_rgb_pattern_set(struct led_classdev *cdev, struct led_pattern *pattern,
> + u32 len, int repeat)
> +{
> + struct s2m_rgb *rgb = to_s2m_rgb(to_s2m_mc(cdev));
> + const u32 *lut_ramp_up, *lut_ramp_dn, *lut_stay_hi, *lut_stay_lo;
> + size_t lut_ramp_up_len, lut_ramp_dn_len, lut_stay_hi_len, lut_stay_lo_len;
> + int brightness_peak = 0;
> + u32 time_hi = 0, time_lo = 0;
> + bool ramp_up_en, ramp_dn_en;
> + int ret;
> + int i;
> +
> + /*
> + * The typical pattern supported by this device can be
> + * represented with the following graph:
> + *
> + * 255 T ''''''-. .-'''''''-.
> + * | '. .' '.
> + * | \ / \
> + * | '. .' '.
> + * | '-...........-' '-
> + * 0 +----------------------------------------------------> time (s)
> + *
> + * <---- HIGH ----><-- LOW --><-------- HIGH --------->
> + * <-----><-------><---------><-------><-----><------->
> + * stay_hi ramp_dn stay_lo ramp_up stay_hi ramp_dn
> + *
> + * There are two states, named HIGH and LOW. HIGH has a non-zero
> + * brightness level, while LOW is of zero brightness. The
> + * pattern provided should mention only one zero and non-zero
> + * brightness level. The hardware always starts the pattern from
> + * the HIGH state, as shown in the graph.
> + *
> + * The HIGH state can be divided in three somewhat equal timings:
> + * ramp_up, stay_hi, and ramp_dn. The LOW state has only one
> + * timing: stay_lo.
> + */
> +
> + /* Only indefinitely looping patterns are supported. */
> + if (repeat != -1)
> + return -EINVAL;
> +
> + /* Pattern should consist of at least two tuples. */
> + if (len < 2)
> + return -EINVAL;
> +
> + for (i = 0; i < len; i++) {
for (int i = 0; ...) would be preferable.
> + int brightness = pattern[i].brightness;
> + u32 delta_t = pattern[i].delta_t;
> +
> + if (brightness) {
> + /*
> + * The pattern shold define only one non-zero
> + * brightness in the HIGH state. The device
> + * doesn't have any provisions to handle
> + * multiple peak brightness levels.
> + */
> + if (brightness_peak && brightness_peak != brightness)
> + return -EINVAL;
> +
> + brightness_peak = brightness;
> + time_hi += delta_t;
> + ramp_dn_en = !!delta_t;
> + } else {
> + time_lo += delta_t;
> + ramp_up_en = !!delta_t;
> + }
> + }
> +
> + switch (rgb->device_type) {
> + case S2MU005:
> + lut_ramp_up = s2mu005_rgb_lut_ramp;
> + lut_ramp_up_len = ARRAY_SIZE(s2mu005_rgb_lut_ramp);
> + lut_ramp_dn = s2mu005_rgb_lut_ramp;
> + lut_ramp_dn_len = ARRAY_SIZE(s2mu005_rgb_lut_ramp);
> + lut_stay_hi = s2mu005_rgb_lut_stay_hi;
> + lut_stay_hi_len = ARRAY_SIZE(s2mu005_rgb_lut_stay_hi);
> + lut_stay_lo = s2mu005_rgb_lut_stay_lo;
> + lut_stay_lo_len = ARRAY_SIZE(s2mu005_rgb_lut_stay_lo);
> + break;
> + default:
> + /* execution shouldn't reach here */
Instead of a comment, perhaps a WARN_ON_ONCE(1); or similar would be
more robust here to catch unexpected device types?
> + break;
> + }
> +
> + mutex_lock(&rgb->lock);
> +
> + /*
> + * The timings ramp_up, stay_hi, and ramp_dn of the HIGH state
> + * are roughly equal. Firstly, calculate and set timings for
> + * ramp_up and ramp_dn (making sure they're exactly equal).
> + */
> + rgb->ramp_up = 0;
> + rgb->ramp_dn = 0;
> +
> + if (ramp_up_en) {
> + ret = s2m_rgb_lut_calc_timing(lut_ramp_up, lut_ramp_up_len, time_hi / 3,
> + &rgb->ramp_up);
> + if (ret < 0)
> + goto param_fail;
> + }
> +
> + if (ramp_dn_en) {
> + ret = s2m_rgb_lut_calc_timing(lut_ramp_dn, lut_ramp_dn_len, time_hi / 3,
> + &rgb->ramp_dn);
> + if (ret < 0)
> + goto param_fail;
> + }
> +
> + /*
> + * Subtract the allocated ramp timings from time_hi (and also
> + * making sure it doesn't underflow!). The remaining time is
> + * allocated to stay_hi.
> + */
> + time_hi -= min(time_hi, lut_ramp_up[rgb->ramp_up]);
> + time_hi -= min(time_hi, lut_ramp_dn[rgb->ramp_dn]);
> +
> + ret = s2m_rgb_lut_calc_timing(lut_stay_hi, lut_stay_hi_len, time_hi, &rgb->stay_hi);
> + if (ret < 0)
> + goto param_fail;
> +
> + ret = s2m_rgb_lut_calc_timing(lut_stay_lo, lut_stay_lo_len, time_lo, &rgb->stay_lo);
> + if (ret < 0)
> + goto param_fail;
> +
> + led_mc_calc_color_components(&rgb->mc, brightness_peak);
> + switch (rgb->device_type) {
> + case S2MU005:
> + ret = s2mu005_rgb_apply_params(rgb);
> + break;
> + default:
> + /* execution shouldn't reach here */
> + break;
> + }
> + if (ret < 0)
> + goto param_fail;
Are we expecting positive values in these 'ret's?
If not if (!ret) will do.
> +
> + mutex_unlock(&rgb->lock);
> +
> + return 0;
> +
> +param_fail:
> + rgb->ramp_up = 0;
> + rgb->ramp_dn = 0;
> + rgb->stay_hi = 0;
> + rgb->stay_lo = 0;
> +
> + mutex_unlock(&rgb->lock);
> +
> + return ret;
> +}
> +
> +static int s2m_rgb_pattern_clear(struct led_classdev *cdev)
> +{
> + struct s2m_rgb *rgb = to_s2m_rgb(to_s2m_mc(cdev));
> + int ret = 0;
> +
> + mutex_lock(&rgb->lock);
> +
> + switch (rgb->device_type) {
> + case S2MU005:
> + ret = s2mu005_rgb_reset_params(rgb);
> + break;
> + default:
> + /* execution shouldn't reach here */
> + break;
As above.
And a single branch switch () makes little sense.
> + }
> +
> + mutex_unlock(&rgb->lock);
> +
> + return ret;
> +}
> +
> +static int s2m_rgb_brightness_set(struct led_classdev *cdev, enum led_brightness value)
> +{
> + struct s2m_rgb *rgb = to_s2m_rgb(to_s2m_mc(cdev));
> + int ret = 0;
> +
> + if (!value)
> + return s2m_rgb_pattern_clear(cdev);
> +
> + mutex_lock(&rgb->lock);
> +
> + led_mc_calc_color_components(&rgb->mc, value);
> + switch (rgb->device_type) {
> + case S2MU005:
> + ret = s2mu005_rgb_apply_params(rgb);
> + break;
> + default:
> + /* execution shouldn't reach here */
> + break;
> + }
> +
> + mutex_unlock(&rgb->lock);
> +
> + return ret;
> +}
> +
> +static struct mc_subled s2mu005_rgb_subled_info[] = {
const?
> + { .channel = 0, .color_index = LED_COLOR_ID_BLUE },
> + { .channel = 1, .color_index = LED_COLOR_ID_GREEN },
> + { .channel = 2, .color_index = LED_COLOR_ID_RED },
> +};
> +
> +static int s2m_rgb_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct sec_pmic_dev *pmic_drvdata = dev_get_drvdata(dev->parent);
> + struct s2m_rgb *rgb;
> + struct led_init_data init_data = {};
> + int ret;
> +
> + rgb = devm_kzalloc(dev, sizeof(*rgb), GFP_KERNEL);
> + if (!rgb)
> + return -ENOMEM;
> +
> + platform_set_drvdata(pdev, rgb);
> + rgb->dev = dev;
> + rgb->regmap = pmic_drvdata->regmap_pmic;
> + rgb->device_type = platform_get_device_id(pdev)->driver_data;
We don't tend to use these object oriented-type constructs in the
kernel. Also, we have helper functions of extracting driver_data.
Please use them.
> +
> + switch (rgb->device_type) {
> + case S2MU005:
> + rgb->mc.subled_info = s2mu005_rgb_subled_info;
> + rgb->mc.num_colors = ARRAY_SIZE(s2mu005_rgb_subled_info);
> + break;
> + default:
> + return dev_err_probe(dev, -ENODEV, "device type %d is not supported by driver\n",
> + pmic_drvdata->device_type);
Small point, but for consistency, would it be better to print
`rgb->device_type` here, since that is the value being checked in the
switch statement?
Also, same single branch comment as before.
> + }
> +
> + rgb->mc.led_cdev.max_brightness = 255;
> + rgb->mc.led_cdev.brightness_set_blocking = s2m_rgb_brightness_set;
> + rgb->mc.led_cdev.pattern_set = s2m_rgb_pattern_set;
> + rgb->mc.led_cdev.pattern_clear = s2m_rgb_pattern_clear;
> +
> + ret = devm_mutex_init(dev, &rgb->lock);
> + if (ret)
> + return dev_err_probe(dev, ret, "failed to create mutex lock\n");
> +
> + init_data.fwnode = of_fwnode_handle(dev->of_node);
> + ret = devm_led_classdev_multicolor_register_ext(dev, &rgb->mc, &init_data);
> + if (ret < 0)
> + return dev_err_probe(dev, ret, "failed to create LED device\n");
> +
> + return 0;
> +}
> +
> +static const struct platform_device_id s2m_rgb_id_table[] = {
> + { "s2mu005-rgb", S2MU005 },
> + { /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(platform, s2m_rgb_id_table);
> +
> +static const struct of_device_id s2m_rgb_of_match_table[] = {
> + { .compatible = "samsung,s2mu005-rgb", .data = (void *)S2MU005 },
> + { /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(of, s2m_rgb_of_match_table);
> +
> +static struct platform_driver s2m_rgb_driver = {
> + .driver = {
> + .name = "s2m-rgb",
> + },
> + .probe = s2m_rgb_probe,
> + .id_table = s2m_rgb_id_table,
> +};
> +module_platform_driver(s2m_rgb_driver);
> +
> +MODULE_DESCRIPTION("RGB LED Driver For Samsung S2M Series PMICs");
"for"
> +MODULE_AUTHOR("Kaustabh Chakraborty <kauschluss@disroot.org>");
> +MODULE_LICENSE("GPL");
--
Lee Jones
^ permalink raw reply
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