* Re: [PATCH v6 2/2] rtc: Add NXP PCF85053 driver support
From: kernel test robot @ 2025-11-18 20:58 UTC (permalink / raw)
To: Lakshay Piplani, alexandre.belloni, linux-rtc, linux-kernel, robh,
krzk+dt, conor+dt, devicetree
Cc: oe-kbuild-all, pankit.garg, vikash.bansal, priyanka.jain,
shashank.rebbapragada, Lakshay Piplani, Daniel Aguirre
In-Reply-To: <20251113054243.4045820-2-lakshay.piplani@nxp.com>
Hi Lakshay,
kernel test robot noticed the following build errors:
[auto build test ERROR on abelloni/rtc-next]
[also build test ERROR on linus/master v6.18-rc6 next-20251118]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Lakshay-Piplani/rtc-Add-NXP-PCF85053-driver-support/20251113-134432
base: https://git.kernel.org/pub/scm/linux/kernel/git/abelloni/linux.git rtc-next
patch link: https://lore.kernel.org/r/20251113054243.4045820-2-lakshay.piplani%40nxp.com
patch subject: [PATCH v6 2/2] rtc: Add NXP PCF85053 driver support
config: sparc64-randconfig-001-20251119 (https://download.01.org/0day-ci/archive/20251119/202511190432.xfs91BVq-lkp@intel.com/config)
compiler: sparc64-linux-gcc (GCC) 15.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20251119/202511190432.xfs91BVq-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202511190432.xfs91BVq-lkp@intel.com/
All error/warnings (new ones prefixed by >>):
drivers/rtc/rtc-pcf85053.c: In function 'pcf85053_probe':
>> drivers/rtc/rtc-pcf85053.c:616:14: error: implicit declaration of function 'i2c_check_functionality' [-Wimplicit-function-declaration]
616 | if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C |
| ^~~~~~~~~~~~~~~~~~~~~~~
drivers/rtc/rtc-pcf85053.c: At top level:
>> drivers/rtc/rtc-pcf85053.c:729:1: warning: data definition has no type or storage class
729 | module_i2c_driver(pcf85053_driver);
| ^~~~~~~~~~~~~~~~~
>> drivers/rtc/rtc-pcf85053.c:729:1: error: type defaults to 'int' in declaration of 'module_i2c_driver' [-Wimplicit-int]
>> drivers/rtc/rtc-pcf85053.c:729:1: error: parameter names (without types) in function declaration [-Wdeclaration-missing-parameter-type]
>> drivers/rtc/rtc-pcf85053.c:720:26: warning: 'pcf85053_driver' defined but not used [-Wunused-variable]
720 | static struct i2c_driver pcf85053_driver = {
| ^~~~~~~~~~~~~~~
vim +/i2c_check_functionality +616 drivers/rtc/rtc-pcf85053.c
608
609 static int pcf85053_probe(struct i2c_client *client)
610 {
611 struct pcf85053 *pcf85053;
612 const struct pcf85053_config *config;
613 const char *iface = NULL;
614 int err;
615
> 616 if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C |
617 I2C_FUNC_SMBUS_BYTE |
618 I2C_FUNC_SMBUS_BLOCK_DATA))
619 return -ENODEV;
620
621 pcf85053 = devm_kzalloc(&client->dev, sizeof(struct pcf85053),
622 GFP_KERNEL);
623 if (!pcf85053)
624 return -ENOMEM;
625
626 config = i2c_get_match_data(client);
627 if (!config)
628 return -ENODEV;
629
630 pcf85053->regmap = devm_regmap_init_i2c(client, &config->regmap);
631 if (IS_ERR(pcf85053->regmap))
632 return PTR_ERR(pcf85053->regmap);
633
634 i2c_set_clientdata(client, pcf85053);
635
636 pcf85053->client = client;
637 device_set_wakeup_capable(&client->dev, 1);
638
639 pcf85053->is_primary = true;
640
641 if (of_property_read_string(client->dev.of_node, "nxp,interface", &iface))
642 return dev_err_probe(&client->dev, -EINVAL,
643 "Missing mandatory property: nxp,interface\n");
644 if (!strcmp(iface, "primary"))
645 pcf85053->is_primary = true;
646 else if (!strcmp(iface, "secondary"))
647 pcf85053->is_primary = false;
648 else
649 return dev_err_probe(&client->dev, -EINVAL,
650 "Invalid value for nxp,interface: %s\n", iface);
651
652 if (pcf85053->is_primary) {
653 unsigned int ctrl;
654 int err;
655
656 err = regmap_read(pcf85053->regmap, PCF85053_REG_CTRL, &ctrl);
657 if (err)
658 return err;
659
660 if (of_property_read_bool(client->dev.of_node, "nxp,write-access")) {
661 if (!(ctrl & PCF85053_BIT_TWO)) {
662 err = regmap_update_bits(pcf85053->regmap, PCF85053_REG_CTRL,
663 PCF85053_BIT_TWO, PCF85053_BIT_TWO);
664 if (err)
665 return err;
666 }
667 dev_dbg(&client->dev, "Ownership set: TWO=1 (primary writes)\n");
668 } else {
669 /* TWO (Time Write Ownership) bit defaults to 0 (Secondary) */
670 dev_dbg(&client->dev, "Default ownership set: TWO=0 (secondary writes)\n");
671 }
672 }
673
674 pcf85053->rtc = devm_rtc_allocate_device(&client->dev);
675 if (IS_ERR(pcf85053->rtc))
676 return PTR_ERR(pcf85053->rtc);
677
678 pcf85053->rtc->ops = &pcf85053_rtc_ops;
679 pcf85053->rtc->range_min = RTC_TIMESTAMP_BEGIN_2000;
680 pcf85053->rtc->range_max = RTC_TIMESTAMP_END_2099;
681 clear_bit(RTC_FEATURE_UPDATE_INTERRUPT, pcf85053->rtc->features);
682 clear_bit(RTC_FEATURE_ALARM, pcf85053->rtc->features);
683
684 if (config->has_alarms && client->irq > 0) {
685 err = devm_request_threaded_irq(&client->dev, client->irq,
686 NULL, pcf85053_irq,
687 IRQF_ONESHOT | IRQF_TRIGGER_FALLING,
688 "pcf85053", client);
689 if (err) {
690 dev_err(&client->dev, "unable to request IRQ %d\n", client->irq);
691 } else {
692 set_bit(RTC_FEATURE_ALARM, pcf85053->rtc->features);
693 device_init_wakeup(&client->dev, true);
694 err = dev_pm_set_wake_irq(&client->dev, client->irq);
695 if (err)
696 dev_err(&client->dev, "failed to enable irq wake\n");
697 }
698 }
699
700 #ifdef CONFIG_COMMON_CLK
701 /* register clk in common clk framework */
702 pcf85053_clkout_register_clk(pcf85053);
703 #endif
704
705 return devm_rtc_register_device(pcf85053->rtc);
706 }
707
708 static const struct i2c_device_id pcf85053_id[] = {
709 { "pcf85053", .driver_data = (kernel_ulong_t)&config_pcf85053 },
710 { }
711 };
712 MODULE_DEVICE_TABLE(i2c, pcf85053_id);
713
714 static const struct of_device_id pcf85053_of_match[] = {
715 { .compatible = "nxp,pcf85053", .data = &config_pcf85053 },
716 {}
717 };
718 MODULE_DEVICE_TABLE(of, pcf85053_of_match);
719
> 720 static struct i2c_driver pcf85053_driver = {
721 .driver = {
722 .name = "rtc-pcf85053",
723 .of_match_table = of_match_ptr(pcf85053_of_match),
724 },
725 .probe = pcf85053_probe,
726 .id_table = pcf85053_id,
727 };
728
> 729 module_i2c_driver(pcf85053_driver);
730
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply
* Re: [PATCH v3 4/4] mfd: simple-mfd-i2c: add default value
From: Alex Elder @ 2025-11-18 17:32 UTC (permalink / raw)
To: Troy Mitchell, Lee Jones, Yixun Lan, Andi Shyti,
Alexandre Belloni, Liam Girdwood, Mark Brown
Cc: linux-kernel, linux-riscv, spacemit, linux-i2c, linux-rtc
In-Reply-To: <20251118-p1-kconfig-fix-v3-4-8839c5ac5db3@linux.spacemit.com>
On 11/18/25 12:08 AM, Troy Mitchell wrote:
> The default value of the P1 sub-device depends on the value
> of P1, so P1 should have a default value here.
>
> Signed-off-by: Troy Mitchell <troy.mitchell@linux.spacemit.com>
> ---
> drivers/mfd/Kconfig | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 6cec1858947bf7ab5ee78beb730c95dabcb43a98..b0f109b3acc40b074e4d0178e123437495853496 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -1260,6 +1260,7 @@ config MFD_SPACEMIT_P1
> depends on I2C
> select I2C_K1
> select MFD_SIMPLE_MFD_I2C
> + default ARCH_SPACEMIT
> help
> This option supports the I2C-based SpacemiT P1 PMIC, which
> contains regulators, a power switch, GPIOs, an RTC, and more.
>
I agree with Emil on making this be default m if possible.
Acked-by: Alex Elder <elder@riscstar.com>
Thank you for these fixes.
^ permalink raw reply
* Re: [PATCH v3 3/4] regulator: spacemit: MFD_SPACEMIT_P1 as dependencies
From: Alex Elder @ 2025-11-18 17:32 UTC (permalink / raw)
To: Troy Mitchell, Lee Jones, Yixun Lan, Andi Shyti,
Alexandre Belloni, Liam Girdwood, Mark Brown
Cc: linux-kernel, linux-riscv, spacemit, linux-i2c, linux-rtc
In-Reply-To: <20251118-p1-kconfig-fix-v3-3-8839c5ac5db3@linux.spacemit.com>
On 11/18/25 12:08 AM, Troy Mitchell wrote:
> REGULATOR_SPACEMIT_P1 is a subdevice of P1 and should depend on
> MFD_SPACEMIT_P1 rather than selecting it directly. Using 'select'
> does not always respect the parent's dependencies, so 'depends on'
> is the safer and more correct choice.
>
> Since MFD_SPACEMIT_P1 already depends on I2C_K1, the dependency
> in REGULATOR_SPACEMIT_P1 is now redundant.
>
> Additionally, the default value depends on MFD_SPACEMIT_P1 rather
> than ARCH_SPACEMIT.
>
> Acked-by: Mark Brown <broonie@kernel.org>
> Signed-off-by: Troy Mitchell <troy.mitchell@linux.spacemit.com>
> ---
> Changelog in v3:
> - modify commit message
> - change default value from ARCH_SPACEMIT to MFD_SPACEMIT_P1
> - Link to v2: https://lore.kernel.org/all/20251027-p1-kconfig-fix-v2-4-49688f30bae8@linux.spacemit.com/
> ---
> drivers/regulator/Kconfig | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
> index d84f3d054c59d86d91d859808aa73a3b609d16d0..e2cbbb90500189a1c4282511b8d7141301cae1f0 100644
> --- a/drivers/regulator/Kconfig
> +++ b/drivers/regulator/Kconfig
> @@ -1455,9 +1455,8 @@ config REGULATOR_SLG51000
> config REGULATOR_SPACEMIT_P1
> tristate "SpacemiT P1 regulators"
> depends on ARCH_SPACEMIT || COMPILE_TEST
> - depends on I2C
> - select MFD_SPACEMIT_P1
> - default ARCH_SPACEMIT
> + depends on MFD_SPACEMIT_P1
> + default MFD_SPACEMIT_P1
If possible:
default m if MFD_SPACEMIT_P1
Acked-by: Alex Elder <elder@riscstar.com>
> help
> Enable support for regulators implemented by the SpacemiT P1
> power controller. The P1 implements 6 high-efficiency buck
>
^ permalink raw reply
* Re: [PATCH v3 2/4] rtc: spacemit: MFD_SPACEMIT_P1 as dependencies
From: Alex Elder @ 2025-11-18 17:32 UTC (permalink / raw)
To: Troy Mitchell, Lee Jones, Yixun Lan, Andi Shyti,
Alexandre Belloni, Liam Girdwood, Mark Brown
Cc: linux-kernel, linux-riscv, spacemit, linux-i2c, linux-rtc
In-Reply-To: <20251118-p1-kconfig-fix-v3-2-8839c5ac5db3@linux.spacemit.com>
On 11/18/25 12:08 AM, Troy Mitchell wrote:
> RTC_DRV_SPACEMIT_P1 is a subdevice of P1 and should depend on
> MFD_SPACEMIT_P1 rather than selecting it directly. Using 'select'
> does not always respect the parent's dependencies, so 'depends on'
> is the safer and more correct choice.
In particular, it looks like it doesn't depend on I2C...
> Additionally, the default value depends on MFD_SPACEMIT_P1 rather
> than ARCH_SPACEMIT.
>
> Signed-off-by: Troy Mitchell <troy.mitchell@linux.spacemit.com>
> ---
> Changelog in v3:
> - modify commit message
> - change default value from ARCH_SPACEMIT to MFD_SPACEMIT_P1
> - Link to v2: https://lore.kernel.org/all/20251027-p1-kconfig-fix-v2-3-49688f30bae8@linux.spacemit.com/
> ---
> drivers/rtc/Kconfig | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
> index 2933c41c77c88e60df721fe65b9c8afb995ae51e..b392e6d096ed077e841a2e68b70d8b80d9ad1cde 100644
> --- a/drivers/rtc/Kconfig
> +++ b/drivers/rtc/Kconfig
> @@ -409,8 +409,8 @@ config RTC_DRV_MAX77686
> config RTC_DRV_SPACEMIT_P1
> tristate "SpacemiT P1 RTC"
> depends on ARCH_SPACEMIT || COMPILE_TEST
> - select MFD_SPACEMIT_P1
> - default ARCH_SPACEMIT
> + depends on MFD_SPACEMIT_P1
> + default MFD_SPACEMIT_P1
If possible, this should maybe be:
default m if MFD_SPACEMIT_P1
In any case, this looks like an improvement.
Acked-by: Alex Elder <elder@riscstar.com>
> help
> Enable support for the RTC function in the SpacemiT P1 PMIC.
> This driver can also be built as a module, which will be called
>
^ permalink raw reply
* Re: [PATCH v3 1/4] i2c: spacemit: configure ILCR for accurate SCL frequency
From: Alex Elder @ 2025-11-18 17:32 UTC (permalink / raw)
To: Troy Mitchell, Lee Jones, Yixun Lan, Andi Shyti,
Alexandre Belloni, Liam Girdwood, Mark Brown
Cc: linux-kernel, linux-riscv, spacemit, linux-i2c, linux-rtc
In-Reply-To: <20251118-p1-kconfig-fix-v3-1-8839c5ac5db3@linux.spacemit.com>
On 11/18/25 12:08 AM, Troy Mitchell wrote:
> The SpacemiT I2C controller's SCL (Serial Clock Line) frequency for
> master mode operations is determined by the ILCR (I2C Load Count Register).
> Previously, the driver relied on the hardware's reset default
> values for this register.
>
> The hardware's default ILCR values (SLV=0x156, FLV=0x5d) yield SCL
> frequencies lower than intended. For example, with the default
> 31.5 MHz input clock, these default settings result in an SCL
> frequency of approximately 93 kHz (standard mode) when targeting 100 kHz,
> and approximately 338 kHz (fast mode) when targeting 400 kHz.
> These frequencies are below the 100 kHz/400 kHz nominal speeds.
>
> This patch integrates the SCL frequency management into
> the Common Clock Framework (CCF). Specifically, the ILCR register,
> which acts as a frequency divider for the SCL clock, is now registered
> as a managed clock (scl_clk) within the CCF.
>
> This patch also cleans up unnecessary whitespace
> in the included header files.
I have a few comments below. Sorry I didn't comment on
earlier versions. > Reviewed-by: Yixun Lan <dlan@gentoo.org>
> Link: https://lore.kernel.org/all/176244506110.1925720.10807118665958896958.b4-ty@kernel.org/ [1]
> Signed-off-by: Troy Mitchell <troy.mitchell@linux.spacemit.com>
> ---
> This patch was affected by the P1 Kconfig, which caused the maintainer
> to revert it.
> The current commit is a direct cherry-pick and reserves the original changelog.
> This note is to clarify for anyone who sees the cover letter marked as v2
> while the changelog entries reach v4.
> ---
> Changelog in v4:
> - initialize clk_init_data with {} so that init.flags is implicitly set to 0
> - minor cleanup and style fixes for better readability
> - remove unused spacemit_i2c_scl_clk_exclusive_put() cleanup callback
> - replace clk_set_rate_exclusive()/clk_rate_exclusive_put() pair with clk_set_rate()
> - simplify LCR LV field macros by using FIELD_GET/FIELD_MAX helpers
> - Link to v3: https://lore.kernel.org/all/20250814-k1-i2c-ilcr-v3-1-317723e74bcd@linux.spacemit.com/
>
> Changelog in v3:
> - use MASK macro in `recalc_rate` function
> - rename clock name
> - Link to v2: https://lore.kernel.org/r/20250718-k1-i2c-ilcr-v2-1-b4c68f13dcb1@linux.spacemit.com
>
> Changelog in v2:
> - Align line breaks.
> - Check `lv` in `clk_set_rate` function.
> - Force fast mode when SCL frequency is illegal or unavailable.
> - Change "linux/bits.h" to <linux/bits.h>
> - Kconfig: Add dependency on CCF.
> - Link to v1: https://lore.kernel.org/all/20250710-k1-i2c-ilcr-v1-1-188d1f460c7d@linux.spacemit.com/
> ---
> drivers/i2c/busses/Kconfig | 2 +-
> drivers/i2c/busses/i2c-k1.c | 159 ++++++++++++++++++++++++++++++++++++++++----
> 2 files changed, 146 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index fd81e49638aaa161ae264a722e9e06adc7914cda..fedf5d31f9035b73a27a7f8a764bf5c26975d0e1 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -798,7 +798,7 @@ config I2C_JZ4780
> config I2C_K1
> tristate "SpacemiT K1 I2C adapter"
> depends on ARCH_SPACEMIT || COMPILE_TEST
> - depends on OF
> + depends on OF && COMMON_CLK
> help
> This option enables support for the I2C interface on the SpacemiT K1
> platform.
> diff --git a/drivers/i2c/busses/i2c-k1.c b/drivers/i2c/busses/i2c-k1.c
> index 6b918770e612e098b8ad17418f420d87c94df166..e38a0ba71734ca602854c85672dcb61423453515 100644
> --- a/drivers/i2c/busses/i2c-k1.c
> +++ b/drivers/i2c/busses/i2c-k1.c
> @@ -4,18 +4,21 @@
> */
>
> #include <linux/bitfield.h>
> - #include <linux/clk.h>
> - #include <linux/i2c.h>
> - #include <linux/iopoll.h>
> - #include <linux/module.h>
> - #include <linux/of_address.h>
> - #include <linux/platform_device.h>
> +#include <linux/bits.h>
> +#include <linux/clk.h>
> +#include <linux/clk-provider.h>
> +#include <linux/i2c.h>
> +#include <linux/iopoll.h>
> +#include <linux/module.h>
> +#include <linux/of_address.h>
> +#include <linux/platform_device.h>
>
> /* spacemit i2c registers */
> #define SPACEMIT_ICR 0x0 /* Control register */
> #define SPACEMIT_ISR 0x4 /* Status register */
> #define SPACEMIT_IDBR 0xc /* Data buffer register */
> #define SPACEMIT_IRCR 0x18 /* Reset cycle counter */
> +#define SPACEMIT_ILCR 0x10 /* Load Count Register */
> #define SPACEMIT_IBMR 0x1c /* Bus monitor register */
>
> /* SPACEMIT_ICR register fields */
> @@ -87,6 +90,13 @@
> #define SPACEMIT_BMR_SDA BIT(0) /* SDA line level */
> #define SPACEMIT_BMR_SCL BIT(1) /* SCL line level */
>
> +#define SPACEMIT_LCR_LV_STANDARD_SHIFT 0
> +#define SPACEMIT_LCR_LV_FAST_SHIFT 9
Why do you need these SHIFT symbols? Just use the masks
and FIELD_GET() related macros. I'll provide examples below.
> +#define SPACEMIT_LCR_LV_STANDARD_MASK GENMASK(8, 0)
> +#define SPACEMIT_LCR_LV_FAST_MASK GENMASK(17, 9)
> +#define SPACEMIT_LCR_LV_STANDARD_MAX_VALUE FIELD_MAX(SPACEMIT_LCR_LV_STANDARD_MASK)
> +#define SPACEMIT_LCR_LV_FAST_MAX_VALUE FIELD_MAX(SPACEMIT_LCR_LV_FAST_MASK)
> +
> /* i2c bus recover timeout: us */
> #define SPACEMIT_I2C_BUS_BUSY_TIMEOUT 100000
>
> @@ -104,11 +114,20 @@ enum spacemit_i2c_state {
> SPACEMIT_STATE_WRITE,
> };
>
> +enum spacemit_i2c_mode {
> + SPACEMIT_MODE_STANDARD,
> + SPACEMIT_MODE_FAST
> +};
Will there ever be more than two i2c modes? If not, this
could simply be a Boolean.
> +
> /* i2c-spacemit driver's main struct */
> struct spacemit_i2c_dev {
> struct device *dev;
> struct i2c_adapter adapt;
>
> + struct clk_hw scl_clk_hw;
> + struct clk *scl_clk;
> + enum spacemit_i2c_mode mode;
Perhaps this could be:
bool fast_mode;
> +
> /* hardware resources */
> void __iomem *base;
> int irq;
> @@ -129,6 +148,79 @@ struct spacemit_i2c_dev {
> u32 status;
> };
>
> +static void spacemit_i2c_scl_clk_disable_unprepare(void *data)
> +{
> + struct spacemit_i2c_dev *i2c = data;
> +
> + clk_disable_unprepare(i2c->scl_clk);
> +}
> +
> +static int spacemit_i2c_clk_set_rate(struct clk_hw *hw, unsigned long rate,
> + unsigned long parent_rate)
> +{
> + struct spacemit_i2c_dev *i2c = container_of(hw, struct spacemit_i2c_dev, scl_clk_hw);
> + u32 lv, lcr, mask, shift, max_lv;
> +
> + lv = DIV_ROUND_UP(parent_rate, rate);
Would DIV_ROUND_CLOSEST() give a more accurate value?
> +
> + if (i2c->mode == SPACEMIT_MODE_STANDARD) {
> + mask = SPACEMIT_LCR_LV_STANDARD_MASK;
> + shift = SPACEMIT_LCR_LV_STANDARD_SHIFT;
> + max_lv = SPACEMIT_LCR_LV_STANDARD_MAX_VALUE;
> + } else if (i2c->mode == SPACEMIT_MODE_FAST) {
> + mask = SPACEMIT_LCR_LV_FAST_MASK;
> + shift = SPACEMIT_LCR_LV_FAST_SHIFT;
> + max_lv = SPACEMIT_LCR_LV_FAST_MAX_VALUE;
> + }
> +
> + if (!lv || lv > max_lv) {
> + dev_err(i2c->dev, "set scl clock failed: lv 0x%x", lv);
> + return -EINVAL;
> + }
> +
> + lcr = readl(i2c->base + SPACEMIT_ILCR);
> + lcr &= ~mask;
> + lcr |= lv << shift;
> + writel(lcr, i2c->base + SPACEMIT_ILCR);
FIELD_MODIFY(mask, &lcr, lv);
I suppose this might give you trouble because the mask isn't
constant at compile time, but anyway I think something like
this is simpler:
lv = DIV_ROUND_CLOSEST(parent_rate, rate);
lcr = readl(i2c->base + SPACEMIT_ILCR);
if (i2c->fast_mode)
FIELD_MODIFY(SPACEMIT_LCR_LV_FAST_MASK, &lcr, lv);
else
FIELD_MODIFY(SPACEMIT_LCR_LV_STANDARD_MASK, &lcr, lv);
writel(lcr, i2c->base + SPACEMIT_ILCR);
Note: I've never used FIELD_MODIFY(), but it looks like this is
how it's supposed to be used.
> + return 0;
> +}
> +
> +static long spacemit_i2c_clk_round_rate(struct clk_hw *hw, unsigned long rate,
> + unsigned long *parent_rate)
> +{
> + u32 lv, freq;
> +
> + lv = DIV_ROUND_UP(*parent_rate, rate);
> + freq = DIV_ROUND_UP(*parent_rate, lv);
Consider whether DIV_ROUND_CLOSEST() (in one or both of
these) provides a rate that is as close as possible to the
requested rate.
> +
> + return freq;
> +}
> +
> +static unsigned long spacemit_i2c_clk_recalc_rate(struct clk_hw *hw,
> + unsigned long parent_rate)
> +{
> + struct spacemit_i2c_dev *i2c = container_of(hw, struct spacemit_i2c_dev, scl_clk_hw);
> + u32 lcr, lv = 0;
> +
> + lcr = readl(i2c->base + SPACEMIT_ILCR);
> +
> + if (i2c->mode == SPACEMIT_MODE_STANDARD)
> + lv = FIELD_GET(SPACEMIT_LCR_LV_STANDARD_MASK, lcr);
> + else if (i2c->mode == SPACEMIT_MODE_FAST)
> + lv = FIELD_GET(SPACEMIT_LCR_LV_FAST_MASK, lcr);
> + else
> + return 0;
You shouldn't need the last else here. You can probably tell
by inspection that it will always be one mode or the other.
And a Boolean reinforces that.
> +
> + return DIV_ROUND_UP(parent_rate, lv);
> +}
> +
> +static const struct clk_ops spacemit_i2c_clk_ops = {
> + .set_rate = spacemit_i2c_clk_set_rate,
> + .round_rate = spacemit_i2c_clk_round_rate,
> + .recalc_rate = spacemit_i2c_clk_recalc_rate,
> +};
> +
> static void spacemit_i2c_enable(struct spacemit_i2c_dev *i2c)
> {
> u32 val;
> @@ -147,6 +239,26 @@ static void spacemit_i2c_disable(struct spacemit_i2c_dev *i2c)
> writel(val, i2c->base + SPACEMIT_ICR);
> }
>
> +static struct clk *spacemit_i2c_register_scl_clk(struct spacemit_i2c_dev *i2c,
> + struct clk *parent)
> +{
> + struct clk_init_data init = {};
> + char name[32];
> +
> + snprintf(name, sizeof(name), "%s_scl_clk", dev_name(i2c->dev));
What if dev_name(i2c->dev) is longer than 24? You should
be checking the return value here.
> +
> + init.name = name;
> + init.ops = &spacemit_i2c_clk_ops;
> + init.parent_data = (struct clk_parent_data[]) {
> + { .fw_name = "func" },
> + };
> + init.num_parents = 1;
> +
> + i2c->scl_clk_hw.init = &init;
> +
> + return devm_clk_register(i2c->dev, &i2c->scl_clk_hw);
> +}
> +
> static void spacemit_i2c_reset(struct spacemit_i2c_dev *i2c)
> {
> writel(SPACEMIT_CR_UR, i2c->base + SPACEMIT_ICR);
> @@ -246,7 +358,7 @@ static void spacemit_i2c_init(struct spacemit_i2c_dev *i2c)
> */
> val |= SPACEMIT_CR_DRFIE;
>
> - if (i2c->clock_freq == SPACEMIT_I2C_MAX_FAST_MODE_FREQ)
> + if (i2c->mode == SPACEMIT_MODE_FAST)
> val |= SPACEMIT_CR_MODE_FAST;
>
> /* disable response to general call */
> @@ -538,14 +650,15 @@ static int spacemit_i2c_probe(struct platform_device *pdev)
> dev_warn(dev, "failed to read clock-frequency property: %d\n", ret);
I don't think there's any need to warn when the "clock-frequency"
property is not found. It's an optional property, and the default
is specified in the binding to be 400 KHz.
> /* For now, this driver doesn't support high-speed. */
> - if (!i2c->clock_freq || i2c->clock_freq > SPACEMIT_I2C_MAX_FAST_MODE_FREQ) {
> - dev_warn(dev, "unsupported clock frequency %u; using %u\n",
> - i2c->clock_freq, SPACEMIT_I2C_MAX_FAST_MODE_FREQ);
> + if (i2c->clock_freq > SPACEMIT_I2C_MAX_STANDARD_MODE_FREQ &&
> + i2c->clock_freq <= SPACEMIT_I2C_MAX_FAST_MODE_FREQ) {
> + i2c->mode = SPACEMIT_MODE_FAST;
> + } else if (i2c->clock_freq && i2c->clock_freq <= SPACEMIT_I2C_MAX_STANDARD_MODE_FREQ) {
> + i2c->mode = SPACEMIT_MODE_STANDARD;
So this I2C driver will literally support *either* the standard
speed *or* the high speed frequency. Is this a necessary
restriction? If it is, perhaps the DT binding should be
clear that the speeds should be one of those supported
(because anything else will result in using a supported
speed, not the one provided).
(Sorry, these last two comments are not about your patch,
but about the driver that's already accepted.)
-Alex
> + } else {
> + dev_warn(i2c->dev, "invalid clock-frequency, fallback to fast mode");
> + i2c->mode = SPACEMIT_MODE_FAST;
> i2c->clock_freq = SPACEMIT_I2C_MAX_FAST_MODE_FREQ;
> - } else if (i2c->clock_freq < SPACEMIT_I2C_MAX_STANDARD_MODE_FREQ) {
> - dev_warn(dev, "unsupported clock frequency %u; using %u\n",
> - i2c->clock_freq, SPACEMIT_I2C_MAX_STANDARD_MODE_FREQ);
> - i2c->clock_freq = SPACEMIT_I2C_MAX_STANDARD_MODE_FREQ;
> }
>
> i2c->dev = &pdev->dev;
> @@ -567,10 +680,28 @@ static int spacemit_i2c_probe(struct platform_device *pdev)
> if (IS_ERR(clk))
> return dev_err_probe(dev, PTR_ERR(clk), "failed to enable func clock");
>
> + i2c->scl_clk = spacemit_i2c_register_scl_clk(i2c, clk);
> + if (IS_ERR(i2c->scl_clk))
> + return dev_err_probe(&pdev->dev, PTR_ERR(i2c->scl_clk),
> + "failed to register scl clock\n");
> +
> clk = devm_clk_get_enabled(dev, "bus");
> if (IS_ERR(clk))
> return dev_err_probe(dev, PTR_ERR(clk), "failed to enable bus clock");
>
> + ret = clk_set_rate(i2c->scl_clk, i2c->clock_freq);
> + if (ret)
> + return dev_err_probe(&pdev->dev, ret, "failed to set rate for SCL clock");
> +
> + ret = clk_prepare_enable(i2c->scl_clk);
> + if (ret)
> + return dev_err_probe(&pdev->dev, ret, "failed to prepare and enable clock");
> +
> + ret = devm_add_action_or_reset(dev, spacemit_i2c_scl_clk_disable_unprepare, i2c);
> + if (ret)
> + return dev_err_probe(&pdev->dev, ret,
> + "failed to register cleanup action for clk disable and unprepare");
> +
> spacemit_i2c_reset(i2c);
>
> i2c_set_adapdata(&i2c->adapt, i2c);
>
^ permalink raw reply
* Re: [PATCH v3 4/4] mfd: simple-mfd-i2c: add default value
From: Emil Renner Berthing @ 2025-11-18 16:16 UTC (permalink / raw)
To: Alex Elder, Alexandre Belloni, Andi Shyti, Lee Jones,
Liam Girdwood, Mark Brown, Troy Mitchell, Yixun Lan
Cc: linux-kernel, linux-riscv, spacemit, linux-i2c, linux-rtc
In-Reply-To: <20251118-p1-kconfig-fix-v3-4-8839c5ac5db3@linux.spacemit.com>
Quoting Troy Mitchell (2025-11-18 07:08:08)
> The default value of the P1 sub-device depends on the value
> of P1, so P1 should have a default value here.
>
> Signed-off-by: Troy Mitchell <troy.mitchell@linux.spacemit.com>
> ---
> drivers/mfd/Kconfig | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 6cec1858947bf7ab5ee78beb730c95dabcb43a98..b0f109b3acc40b074e4d0178e123437495853496 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -1260,6 +1260,7 @@ config MFD_SPACEMIT_P1
> depends on I2C
> select I2C_K1
> select MFD_SIMPLE_MFD_I2C
> + default ARCH_SPACEMIT
Can this not be default m if ARCH_SPACEMIT?
/Emil
^ permalink raw reply
* [PATCH next] rtc: atcrtc100: Fix signedness bug in probe()
From: Dan Carpenter @ 2025-11-18 10:48 UTC (permalink / raw)
To: CL Wang; +Cc: Alexandre Belloni, linux-rtc, linux-kernel, kernel-janitors
The "atcrtc_dev->alarm_irq" variable is an unsigned int but it needs to
be signed for the error handling to work. Use the "ret" variable
instead.
Fixes: 7adca706fe16 ("rtc: atcrtc100: Add ATCRTC100 RTC driver")
Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
---
drivers/rtc/rtc-atcrtc100.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/rtc/rtc-atcrtc100.c b/drivers/rtc/rtc-atcrtc100.c
index 51933ae1a2fa..9808fc2c5a49 100644
--- a/drivers/rtc/rtc-atcrtc100.c
+++ b/drivers/rtc/rtc-atcrtc100.c
@@ -296,10 +296,12 @@ static int atcrtc_probe(struct platform_device *pdev)
"Failed to initialize RTC: unsupported hardware ID 0x%x\n",
rtc_id);
- atcrtc_dev->alarm_irq = platform_get_irq(pdev, 1);
- if (atcrtc_dev->alarm_irq < 0)
- return dev_err_probe(&pdev->dev, atcrtc_dev->alarm_irq,
+ ret = platform_get_irq(pdev, 1);
+ if (ret < 0)
+ return dev_err_probe(&pdev->dev, ret,
"Failed to get IRQ for alarm\n");
+ atcrtc_dev->alarm_irq = ret;
+
ret = devm_request_irq(&pdev->dev,
atcrtc_dev->alarm_irq,
atcrtc_alarm_isr,
--
2.51.0
^ permalink raw reply related
* [PATCH v3 4/4] mfd: simple-mfd-i2c: add default value
From: Troy Mitchell @ 2025-11-18 6:08 UTC (permalink / raw)
To: Lee Jones, Yixun Lan, Alex Elder, Andi Shyti, Alexandre Belloni,
Liam Girdwood, Mark Brown
Cc: linux-kernel, linux-riscv, spacemit, linux-i2c, linux-rtc,
Troy Mitchell
In-Reply-To: <20251118-p1-kconfig-fix-v3-0-8839c5ac5db3@linux.spacemit.com>
The default value of the P1 sub-device depends on the value
of P1, so P1 should have a default value here.
Signed-off-by: Troy Mitchell <troy.mitchell@linux.spacemit.com>
---
drivers/mfd/Kconfig | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 6cec1858947bf7ab5ee78beb730c95dabcb43a98..b0f109b3acc40b074e4d0178e123437495853496 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -1260,6 +1260,7 @@ config MFD_SPACEMIT_P1
depends on I2C
select I2C_K1
select MFD_SIMPLE_MFD_I2C
+ default ARCH_SPACEMIT
help
This option supports the I2C-based SpacemiT P1 PMIC, which
contains regulators, a power switch, GPIOs, an RTC, and more.
--
2.51.2
^ permalink raw reply related
* [PATCH v3 3/4] regulator: spacemit: MFD_SPACEMIT_P1 as dependencies
From: Troy Mitchell @ 2025-11-18 6:08 UTC (permalink / raw)
To: Lee Jones, Yixun Lan, Alex Elder, Andi Shyti, Alexandre Belloni,
Liam Girdwood, Mark Brown
Cc: linux-kernel, linux-riscv, spacemit, linux-i2c, linux-rtc,
Troy Mitchell
In-Reply-To: <20251118-p1-kconfig-fix-v3-0-8839c5ac5db3@linux.spacemit.com>
REGULATOR_SPACEMIT_P1 is a subdevice of P1 and should depend on
MFD_SPACEMIT_P1 rather than selecting it directly. Using 'select'
does not always respect the parent's dependencies, so 'depends on'
is the safer and more correct choice.
Since MFD_SPACEMIT_P1 already depends on I2C_K1, the dependency
in REGULATOR_SPACEMIT_P1 is now redundant.
Additionally, the default value depends on MFD_SPACEMIT_P1 rather
than ARCH_SPACEMIT.
Acked-by: Mark Brown <broonie@kernel.org>
Signed-off-by: Troy Mitchell <troy.mitchell@linux.spacemit.com>
---
Changelog in v3:
- modify commit message
- change default value from ARCH_SPACEMIT to MFD_SPACEMIT_P1
- Link to v2: https://lore.kernel.org/all/20251027-p1-kconfig-fix-v2-4-49688f30bae8@linux.spacemit.com/
---
drivers/regulator/Kconfig | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index d84f3d054c59d86d91d859808aa73a3b609d16d0..e2cbbb90500189a1c4282511b8d7141301cae1f0 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -1455,9 +1455,8 @@ config REGULATOR_SLG51000
config REGULATOR_SPACEMIT_P1
tristate "SpacemiT P1 regulators"
depends on ARCH_SPACEMIT || COMPILE_TEST
- depends on I2C
- select MFD_SPACEMIT_P1
- default ARCH_SPACEMIT
+ depends on MFD_SPACEMIT_P1
+ default MFD_SPACEMIT_P1
help
Enable support for regulators implemented by the SpacemiT P1
power controller. The P1 implements 6 high-efficiency buck
--
2.51.2
^ permalink raw reply related
* [PATCH v3 2/4] rtc: spacemit: MFD_SPACEMIT_P1 as dependencies
From: Troy Mitchell @ 2025-11-18 6:08 UTC (permalink / raw)
To: Lee Jones, Yixun Lan, Alex Elder, Andi Shyti, Alexandre Belloni,
Liam Girdwood, Mark Brown
Cc: linux-kernel, linux-riscv, spacemit, linux-i2c, linux-rtc,
Troy Mitchell
In-Reply-To: <20251118-p1-kconfig-fix-v3-0-8839c5ac5db3@linux.spacemit.com>
RTC_DRV_SPACEMIT_P1 is a subdevice of P1 and should depend on
MFD_SPACEMIT_P1 rather than selecting it directly. Using 'select'
does not always respect the parent's dependencies, so 'depends on'
is the safer and more correct choice.
Additionally, the default value depends on MFD_SPACEMIT_P1 rather
than ARCH_SPACEMIT.
Signed-off-by: Troy Mitchell <troy.mitchell@linux.spacemit.com>
---
Changelog in v3:
- modify commit message
- change default value from ARCH_SPACEMIT to MFD_SPACEMIT_P1
- Link to v2: https://lore.kernel.org/all/20251027-p1-kconfig-fix-v2-3-49688f30bae8@linux.spacemit.com/
---
drivers/rtc/Kconfig | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
index 2933c41c77c88e60df721fe65b9c8afb995ae51e..b392e6d096ed077e841a2e68b70d8b80d9ad1cde 100644
--- a/drivers/rtc/Kconfig
+++ b/drivers/rtc/Kconfig
@@ -409,8 +409,8 @@ config RTC_DRV_MAX77686
config RTC_DRV_SPACEMIT_P1
tristate "SpacemiT P1 RTC"
depends on ARCH_SPACEMIT || COMPILE_TEST
- select MFD_SPACEMIT_P1
- default ARCH_SPACEMIT
+ depends on MFD_SPACEMIT_P1
+ default MFD_SPACEMIT_P1
help
Enable support for the RTC function in the SpacemiT P1 PMIC.
This driver can also be built as a module, which will be called
--
2.51.2
^ permalink raw reply related
* [PATCH v3 1/4] i2c: spacemit: configure ILCR for accurate SCL frequency
From: Troy Mitchell @ 2025-11-18 6:08 UTC (permalink / raw)
To: Lee Jones, Yixun Lan, Alex Elder, Andi Shyti, Alexandre Belloni,
Liam Girdwood, Mark Brown
Cc: linux-kernel, linux-riscv, spacemit, linux-i2c, linux-rtc,
Troy Mitchell
In-Reply-To: <20251118-p1-kconfig-fix-v3-0-8839c5ac5db3@linux.spacemit.com>
The SpacemiT I2C controller's SCL (Serial Clock Line) frequency for
master mode operations is determined by the ILCR (I2C Load Count Register).
Previously, the driver relied on the hardware's reset default
values for this register.
The hardware's default ILCR values (SLV=0x156, FLV=0x5d) yield SCL
frequencies lower than intended. For example, with the default
31.5 MHz input clock, these default settings result in an SCL
frequency of approximately 93 kHz (standard mode) when targeting 100 kHz,
and approximately 338 kHz (fast mode) when targeting 400 kHz.
These frequencies are below the 100 kHz/400 kHz nominal speeds.
This patch integrates the SCL frequency management into
the Common Clock Framework (CCF). Specifically, the ILCR register,
which acts as a frequency divider for the SCL clock, is now registered
as a managed clock (scl_clk) within the CCF.
This patch also cleans up unnecessary whitespace
in the included header files.
Reviewed-by: Yixun Lan <dlan@gentoo.org>
Link: https://lore.kernel.org/all/176244506110.1925720.10807118665958896958.b4-ty@kernel.org/ [1]
Signed-off-by: Troy Mitchell <troy.mitchell@linux.spacemit.com>
---
This patch was affected by the P1 Kconfig, which caused the maintainer
to revert it.
The current commit is a direct cherry-pick and reserves the original changelog.
This note is to clarify for anyone who sees the cover letter marked as v2
while the changelog entries reach v4.
---
Changelog in v4:
- initialize clk_init_data with {} so that init.flags is implicitly set to 0
- minor cleanup and style fixes for better readability
- remove unused spacemit_i2c_scl_clk_exclusive_put() cleanup callback
- replace clk_set_rate_exclusive()/clk_rate_exclusive_put() pair with clk_set_rate()
- simplify LCR LV field macros by using FIELD_GET/FIELD_MAX helpers
- Link to v3: https://lore.kernel.org/all/20250814-k1-i2c-ilcr-v3-1-317723e74bcd@linux.spacemit.com/
Changelog in v3:
- use MASK macro in `recalc_rate` function
- rename clock name
- Link to v2: https://lore.kernel.org/r/20250718-k1-i2c-ilcr-v2-1-b4c68f13dcb1@linux.spacemit.com
Changelog in v2:
- Align line breaks.
- Check `lv` in `clk_set_rate` function.
- Force fast mode when SCL frequency is illegal or unavailable.
- Change "linux/bits.h" to <linux/bits.h>
- Kconfig: Add dependency on CCF.
- Link to v1: https://lore.kernel.org/all/20250710-k1-i2c-ilcr-v1-1-188d1f460c7d@linux.spacemit.com/
---
drivers/i2c/busses/Kconfig | 2 +-
drivers/i2c/busses/i2c-k1.c | 159 ++++++++++++++++++++++++++++++++++++++++----
2 files changed, 146 insertions(+), 15 deletions(-)
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index fd81e49638aaa161ae264a722e9e06adc7914cda..fedf5d31f9035b73a27a7f8a764bf5c26975d0e1 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -798,7 +798,7 @@ config I2C_JZ4780
config I2C_K1
tristate "SpacemiT K1 I2C adapter"
depends on ARCH_SPACEMIT || COMPILE_TEST
- depends on OF
+ depends on OF && COMMON_CLK
help
This option enables support for the I2C interface on the SpacemiT K1
platform.
diff --git a/drivers/i2c/busses/i2c-k1.c b/drivers/i2c/busses/i2c-k1.c
index 6b918770e612e098b8ad17418f420d87c94df166..e38a0ba71734ca602854c85672dcb61423453515 100644
--- a/drivers/i2c/busses/i2c-k1.c
+++ b/drivers/i2c/busses/i2c-k1.c
@@ -4,18 +4,21 @@
*/
#include <linux/bitfield.h>
- #include <linux/clk.h>
- #include <linux/i2c.h>
- #include <linux/iopoll.h>
- #include <linux/module.h>
- #include <linux/of_address.h>
- #include <linux/platform_device.h>
+#include <linux/bits.h>
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <linux/i2c.h>
+#include <linux/iopoll.h>
+#include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/platform_device.h>
/* spacemit i2c registers */
#define SPACEMIT_ICR 0x0 /* Control register */
#define SPACEMIT_ISR 0x4 /* Status register */
#define SPACEMIT_IDBR 0xc /* Data buffer register */
#define SPACEMIT_IRCR 0x18 /* Reset cycle counter */
+#define SPACEMIT_ILCR 0x10 /* Load Count Register */
#define SPACEMIT_IBMR 0x1c /* Bus monitor register */
/* SPACEMIT_ICR register fields */
@@ -87,6 +90,13 @@
#define SPACEMIT_BMR_SDA BIT(0) /* SDA line level */
#define SPACEMIT_BMR_SCL BIT(1) /* SCL line level */
+#define SPACEMIT_LCR_LV_STANDARD_SHIFT 0
+#define SPACEMIT_LCR_LV_FAST_SHIFT 9
+#define SPACEMIT_LCR_LV_STANDARD_MASK GENMASK(8, 0)
+#define SPACEMIT_LCR_LV_FAST_MASK GENMASK(17, 9)
+#define SPACEMIT_LCR_LV_STANDARD_MAX_VALUE FIELD_MAX(SPACEMIT_LCR_LV_STANDARD_MASK)
+#define SPACEMIT_LCR_LV_FAST_MAX_VALUE FIELD_MAX(SPACEMIT_LCR_LV_FAST_MASK)
+
/* i2c bus recover timeout: us */
#define SPACEMIT_I2C_BUS_BUSY_TIMEOUT 100000
@@ -104,11 +114,20 @@ enum spacemit_i2c_state {
SPACEMIT_STATE_WRITE,
};
+enum spacemit_i2c_mode {
+ SPACEMIT_MODE_STANDARD,
+ SPACEMIT_MODE_FAST
+};
+
/* i2c-spacemit driver's main struct */
struct spacemit_i2c_dev {
struct device *dev;
struct i2c_adapter adapt;
+ struct clk_hw scl_clk_hw;
+ struct clk *scl_clk;
+ enum spacemit_i2c_mode mode;
+
/* hardware resources */
void __iomem *base;
int irq;
@@ -129,6 +148,79 @@ struct spacemit_i2c_dev {
u32 status;
};
+static void spacemit_i2c_scl_clk_disable_unprepare(void *data)
+{
+ struct spacemit_i2c_dev *i2c = data;
+
+ clk_disable_unprepare(i2c->scl_clk);
+}
+
+static int spacemit_i2c_clk_set_rate(struct clk_hw *hw, unsigned long rate,
+ unsigned long parent_rate)
+{
+ struct spacemit_i2c_dev *i2c = container_of(hw, struct spacemit_i2c_dev, scl_clk_hw);
+ u32 lv, lcr, mask, shift, max_lv;
+
+ lv = DIV_ROUND_UP(parent_rate, rate);
+
+ if (i2c->mode == SPACEMIT_MODE_STANDARD) {
+ mask = SPACEMIT_LCR_LV_STANDARD_MASK;
+ shift = SPACEMIT_LCR_LV_STANDARD_SHIFT;
+ max_lv = SPACEMIT_LCR_LV_STANDARD_MAX_VALUE;
+ } else if (i2c->mode == SPACEMIT_MODE_FAST) {
+ mask = SPACEMIT_LCR_LV_FAST_MASK;
+ shift = SPACEMIT_LCR_LV_FAST_SHIFT;
+ max_lv = SPACEMIT_LCR_LV_FAST_MAX_VALUE;
+ }
+
+ if (!lv || lv > max_lv) {
+ dev_err(i2c->dev, "set scl clock failed: lv 0x%x", lv);
+ return -EINVAL;
+ }
+
+ lcr = readl(i2c->base + SPACEMIT_ILCR);
+ lcr &= ~mask;
+ lcr |= lv << shift;
+ writel(lcr, i2c->base + SPACEMIT_ILCR);
+
+ return 0;
+}
+
+static long spacemit_i2c_clk_round_rate(struct clk_hw *hw, unsigned long rate,
+ unsigned long *parent_rate)
+{
+ u32 lv, freq;
+
+ lv = DIV_ROUND_UP(*parent_rate, rate);
+ freq = DIV_ROUND_UP(*parent_rate, lv);
+
+ return freq;
+}
+
+static unsigned long spacemit_i2c_clk_recalc_rate(struct clk_hw *hw,
+ unsigned long parent_rate)
+{
+ struct spacemit_i2c_dev *i2c = container_of(hw, struct spacemit_i2c_dev, scl_clk_hw);
+ u32 lcr, lv = 0;
+
+ lcr = readl(i2c->base + SPACEMIT_ILCR);
+
+ if (i2c->mode == SPACEMIT_MODE_STANDARD)
+ lv = FIELD_GET(SPACEMIT_LCR_LV_STANDARD_MASK, lcr);
+ else if (i2c->mode == SPACEMIT_MODE_FAST)
+ lv = FIELD_GET(SPACEMIT_LCR_LV_FAST_MASK, lcr);
+ else
+ return 0;
+
+ return DIV_ROUND_UP(parent_rate, lv);
+}
+
+static const struct clk_ops spacemit_i2c_clk_ops = {
+ .set_rate = spacemit_i2c_clk_set_rate,
+ .round_rate = spacemit_i2c_clk_round_rate,
+ .recalc_rate = spacemit_i2c_clk_recalc_rate,
+};
+
static void spacemit_i2c_enable(struct spacemit_i2c_dev *i2c)
{
u32 val;
@@ -147,6 +239,26 @@ static void spacemit_i2c_disable(struct spacemit_i2c_dev *i2c)
writel(val, i2c->base + SPACEMIT_ICR);
}
+static struct clk *spacemit_i2c_register_scl_clk(struct spacemit_i2c_dev *i2c,
+ struct clk *parent)
+{
+ struct clk_init_data init = {};
+ char name[32];
+
+ snprintf(name, sizeof(name), "%s_scl_clk", dev_name(i2c->dev));
+
+ init.name = name;
+ init.ops = &spacemit_i2c_clk_ops;
+ init.parent_data = (struct clk_parent_data[]) {
+ { .fw_name = "func" },
+ };
+ init.num_parents = 1;
+
+ i2c->scl_clk_hw.init = &init;
+
+ return devm_clk_register(i2c->dev, &i2c->scl_clk_hw);
+}
+
static void spacemit_i2c_reset(struct spacemit_i2c_dev *i2c)
{
writel(SPACEMIT_CR_UR, i2c->base + SPACEMIT_ICR);
@@ -246,7 +358,7 @@ static void spacemit_i2c_init(struct spacemit_i2c_dev *i2c)
*/
val |= SPACEMIT_CR_DRFIE;
- if (i2c->clock_freq == SPACEMIT_I2C_MAX_FAST_MODE_FREQ)
+ if (i2c->mode == SPACEMIT_MODE_FAST)
val |= SPACEMIT_CR_MODE_FAST;
/* disable response to general call */
@@ -538,14 +650,15 @@ static int spacemit_i2c_probe(struct platform_device *pdev)
dev_warn(dev, "failed to read clock-frequency property: %d\n", ret);
/* For now, this driver doesn't support high-speed. */
- if (!i2c->clock_freq || i2c->clock_freq > SPACEMIT_I2C_MAX_FAST_MODE_FREQ) {
- dev_warn(dev, "unsupported clock frequency %u; using %u\n",
- i2c->clock_freq, SPACEMIT_I2C_MAX_FAST_MODE_FREQ);
+ if (i2c->clock_freq > SPACEMIT_I2C_MAX_STANDARD_MODE_FREQ &&
+ i2c->clock_freq <= SPACEMIT_I2C_MAX_FAST_MODE_FREQ) {
+ i2c->mode = SPACEMIT_MODE_FAST;
+ } else if (i2c->clock_freq && i2c->clock_freq <= SPACEMIT_I2C_MAX_STANDARD_MODE_FREQ) {
+ i2c->mode = SPACEMIT_MODE_STANDARD;
+ } else {
+ dev_warn(i2c->dev, "invalid clock-frequency, fallback to fast mode");
+ i2c->mode = SPACEMIT_MODE_FAST;
i2c->clock_freq = SPACEMIT_I2C_MAX_FAST_MODE_FREQ;
- } else if (i2c->clock_freq < SPACEMIT_I2C_MAX_STANDARD_MODE_FREQ) {
- dev_warn(dev, "unsupported clock frequency %u; using %u\n",
- i2c->clock_freq, SPACEMIT_I2C_MAX_STANDARD_MODE_FREQ);
- i2c->clock_freq = SPACEMIT_I2C_MAX_STANDARD_MODE_FREQ;
}
i2c->dev = &pdev->dev;
@@ -567,10 +680,28 @@ static int spacemit_i2c_probe(struct platform_device *pdev)
if (IS_ERR(clk))
return dev_err_probe(dev, PTR_ERR(clk), "failed to enable func clock");
+ i2c->scl_clk = spacemit_i2c_register_scl_clk(i2c, clk);
+ if (IS_ERR(i2c->scl_clk))
+ return dev_err_probe(&pdev->dev, PTR_ERR(i2c->scl_clk),
+ "failed to register scl clock\n");
+
clk = devm_clk_get_enabled(dev, "bus");
if (IS_ERR(clk))
return dev_err_probe(dev, PTR_ERR(clk), "failed to enable bus clock");
+ ret = clk_set_rate(i2c->scl_clk, i2c->clock_freq);
+ if (ret)
+ return dev_err_probe(&pdev->dev, ret, "failed to set rate for SCL clock");
+
+ ret = clk_prepare_enable(i2c->scl_clk);
+ if (ret)
+ return dev_err_probe(&pdev->dev, ret, "failed to prepare and enable clock");
+
+ ret = devm_add_action_or_reset(dev, spacemit_i2c_scl_clk_disable_unprepare, i2c);
+ if (ret)
+ return dev_err_probe(&pdev->dev, ret,
+ "failed to register cleanup action for clk disable and unprepare");
+
spacemit_i2c_reset(i2c);
i2c_set_adapdata(&i2c->adapt, i2c);
--
2.51.2
^ permalink raw reply related
* [PATCH v3 0/4] fix the SpacemiT P1 Kconfig and resend the K1 I2C ILCR patch.
From: Troy Mitchell @ 2025-11-18 6:08 UTC (permalink / raw)
To: Lee Jones, Yixun Lan, Alex Elder, Andi Shyti, Alexandre Belloni,
Liam Girdwood, Mark Brown
Cc: linux-kernel, linux-riscv, spacemit, linux-i2c, linux-rtc,
Troy Mitchell
Since P1 Kconfig directly selects K1_I2C, after the I2C ILCR patch was
merged, the driver would fail [1] when COMMON_CLK was not selected.
This series fixes the P1 Kconfig and resends the I2C ILCR patch(This
patch has reverted by maintainer [2]).
Now, P1 Kconfig patch has been merged[3], so I2C ILCR patch can be
merged as well.
In addition, the Kconfig for P1's two subdevices, regulator and RTC,
has been updated to use 'depends on MFD_SPACEMIT_P1' instead of 'select' and
change default value from `ARCH_SPACEMIT` to `MFD_SPACEMIT_P1`.
Link: https://lore.kernel.org/oe-kbuild-all/202510202150.2qXd8e7Y-lkp@intel.com/ [1]
Link: https://lore.kernel.org/all/sdhkjmi5l2m4ua4zqkwkecbihul5bc2dbmitudwfd57y66mdht@6ipjfyz7dtmx/ [2]
Link: https://lore.kernel.org/all/176244506110.1925720.10807118665958896958.b4-ty@kernel.org/ [3]
Signed-off-by: Troy Mitchell <troy.mitchell@linux.spacemit.com>
---
Troy Mitchell (4):
i2c: spacemit: configure ILCR for accurate SCL frequency
rtc: spacemit: MFD_SPACEMIT_P1 as dependencies
regulator: spacemit: MFD_SPACEMIT_P1 as dependencies
mfd: simple-mfd-i2c: add default value
drivers/i2c/busses/Kconfig | 2 +-
drivers/i2c/busses/i2c-k1.c | 159 ++++++++++++++++++++++++++++++++++++++++----
drivers/mfd/Kconfig | 1 +
drivers/regulator/Kconfig | 5 +-
drivers/rtc/Kconfig | 4 +-
5 files changed, 151 insertions(+), 20 deletions(-)
---
base-commit: 3a8660878839faadb4f1a6dd72c3179c1df56787
change-id: 20251021-p1-kconfig-fix-6d2b59d03b8f
Best regards,
--
Troy Mitchell <troy.mitchell@linux.spacemit.com>
^ permalink raw reply
* Re: [PATCH v5 06/11] hwmon: Add Apple Silicon SMC hwmon driver
From: Guenter Roeck @ 2025-11-17 19:00 UTC (permalink / raw)
To: James Calligeros
Cc: Sven Peter, Janne Grunau, Alyssa Rosenzweig, Neal Gompa,
Lee Jones, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Alexandre Belloni, Jean Delvare, Dmitry Torokhov, Jonathan Corbet,
asahi, linux-arm-kernel, devicetree, linux-kernel, linux-rtc,
linux-hwmon, linux-input, linux-doc
In-Reply-To: <20251112-macsmc-subdevs-v5-6-728e4b91fe81@gmail.com>
On Wed, Nov 12, 2025 at 09:16:52PM +1000, James Calligeros wrote:
> The System Management Controller on Apple Silicon devices is responsible
> for integrating and exposing the data reported by the vast array of
> hardware monitoring sensors present on these devices. It is also
> responsible for fan control, and allows users to manually set fan
> speeds if they so desire. Add a hwmon driver to expose current,
> power, temperature, and voltage monitoring sensors, as well as
> fan speed monitoring and control via the SMC on Apple Silicon devices.
>
> The SMC firmware has no consistency between devices, even when they
> share an SoC. The FourCC keys used to access sensors are almost
> random. An M1 Mac mini will have different FourCCs for its CPU core
> temperature sensors to an M1 MacBook Pro, for example. For this
> reason, the valid sensors for a given device are specified in a
> child of the SMC Devicetree node. The driver uses this information
> to determine which sensors to make available at runtime.
>
> Reviewed-by: Neal Gompa <neal@gompa.dev>
> Acked-by: Guenter Roeck <linux@roeck-us.net>
> Co-developed-by: Janne Grunau <j@jannau.net>
> Signed-off-by: Janne Grunau <j@jannau.net>
> Signed-off-by: James Calligeros <jcalligeros99@gmail.com>
Applied to hwmon-next.
Note that I can not apply the devicetree patch (2/11), presumably since it depends
on the first patch of the series.
Guenter
^ permalink raw reply
* Re: [PATCH v4 04/16] dt-bindings: power: supply: BD72720 managed battery
From: Matti Vaittinen @ 2025-11-17 15:48 UTC (permalink / raw)
To: Rob Herring
Cc: Matti Vaittinen, Krzysztof Kozlowski, Mark Brown, Linus Walleij,
linux-kernel, Sebastian Reichel, Bartosz Golaszewski,
Alexandre Belloni, linux-clk, Michael Turquette, Matti Vaittinen,
linux-leds, Pavel Machek, Liam Girdwood, linux-gpio, linux-pm,
Andreas Kemnade, Conor Dooley, devicetree, linux-rtc, Lee Jones,
Stephen Boyd
In-Reply-To: <20251117152341.GA1944698-robh@kernel.org>
On 17/11/2025 17:23, Rob Herring wrote:
> On Mon, Nov 17, 2025 at 10:12:01AM +0200, Matti Vaittinen wrote:
>> On 14/11/2025 18:39, Rob Herring wrote:
>>> On Fri, Nov 14, 2025 at 11:04:27AM +0200, Matti Vaittinen wrote:
>>>> On 13/11/2025 12:53, Rob Herring (Arm) wrote:
>>>>>
>>>>> On Thu, 13 Nov 2025 10:52:19 +0200, Matti Vaittinen wrote:
>>>>>> From: Matti Vaittinen <mazziesaccount@gmail.com>
>>
>> //snip
>>
>>>>
>> For VDR there are only:
>>
>> rohm,voltage-vdr-thresh-microvolt,
>
> So "voltage voltage drop rate"? And '-microvolt' says this is voltage
> too. :)
Hm. Yes. This is a threshold voltage for applying the "zero-correction"
algorithm, which uses these "VDR" (a.k.a voltage drop rate) tables. Eg,
the algorithm should only used for the correction when battery voltage
drops below this threshold. AFAICS, this is usually designed to be
slightly higher than the voltage where the system stays still operable.
I suppose this could also be "zero-correction-threshold", but this would
introduce another "buzzword".
>> rohm,volt-drop-soc-bp,
>> rohm,volt-drop-temperatures-millicelsius
>>
>> and
>>
>> patternProperties:
>> '^rohm,volt-drop-[0-9]-microvolt':
>>
>> So, from the binding point of view (.yaml), it's not _that_ lot. In the .dts
>> there will be quite some noise as the tables have several values.
>>
>>
>>> If that
>>> happens, either we are doing a poor job of generically describing
>>> battery parameters or chargers and batteries are tightly coupled and
>>> can't be described independently.
>>
>> I am under impression that chargers tend to be pretty flexible, and they can
>> be configured to work with many different batteries by altering the charging
>> profiles. Most of the battery properties (like and charging phases [like
>> pre, CC, CV], their limits, currents and voltages etc) are very generally
>> usable. So, large subset of charging functionality can be handled with
>> standard properties. I believe it is only the fuel-gauging where things get
>> more hairy.
>>
>> I did prepare a series which does the split and adds new compatible for the
>> 'rohm,vdr-battery'. (The power-supply class is not yet modified in the
>> series, but we would probably want to modify the battery-info getters to
>> also accept the 'rohm,vdr-battery' -compatible.)
>
> I don't think that's the right direction. It's not a Rohm battery.
>
>> I wonder if I should actually prepare also a series where these properties
>> are just placed in the existing static battery node without adding new
>> compatible. That way it would be easier to see which way is better.
>
> That seems like the right thing to do here.
>
> The main question for me is whether these should even be Rohm specific?
> That would probably require a 2nd user to answer for sure.
>
This is a question Linus W asked as well :)
I believe this technique could be applied to other batteries. I,
however, am not aware of any other than ROHM charger drivers which
implement the algorithm. Furthermore, I was told that the mechanism to
measure these "VDR-tables" for batteries is one of those things which
should be "kept under your hat". I think ROHM has also patented some
stuff related to that. Hence I prefixed these tables by "rohm,".
I have no strong objections to dropping the "rohm," though - but I doubt
these tables will be heavily used by any other but ROHM chargers.
>> If I do that, should I only spin these bindings as RFC to avoid the
>> unnecessary noise?
>
> Only if you think something is not complete and/or the patches should
> not be applied.
Oh, Ok. Then I will send only one of the approaches - probably the one
where properties are added to the simple-battery.
Thanks for all the support!
Yours,
-- Matti
---
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
^ permalink raw reply
* Re: [PATCH v4 04/16] dt-bindings: power: supply: BD72720 managed battery
From: Rob Herring @ 2025-11-17 15:23 UTC (permalink / raw)
To: Matti Vaittinen
Cc: Matti Vaittinen, Krzysztof Kozlowski, Mark Brown, Linus Walleij,
linux-kernel, Sebastian Reichel, Bartosz Golaszewski,
Alexandre Belloni, linux-clk, Michael Turquette, Matti Vaittinen,
linux-leds, Pavel Machek, Liam Girdwood, linux-gpio, linux-pm,
Andreas Kemnade, Conor Dooley, devicetree, linux-rtc, Lee Jones,
Stephen Boyd
In-Reply-To: <32303b95-3fd5-44c4-bb7d-e2957a6064fc@gmail.com>
On Mon, Nov 17, 2025 at 10:12:01AM +0200, Matti Vaittinen wrote:
> On 14/11/2025 18:39, Rob Herring wrote:
> > On Fri, Nov 14, 2025 at 11:04:27AM +0200, Matti Vaittinen wrote:
> > > On 13/11/2025 12:53, Rob Herring (Arm) wrote:
> > > >
> > > > On Thu, 13 Nov 2025 10:52:19 +0200, Matti Vaittinen wrote:
> > > > > From: Matti Vaittinen <mazziesaccount@gmail.com>
>
> //snip
>
> > >
> > > So, as far as I understand, the only viable options are expanding the
> > > existing battery.yaml with these properties (which I hoped to avoid, see
> > > below)
> > >
> > > > > The right place for them is the battery node, which is described by the
> > > > > generic "battery.yaml". I was not comfortable with adding these
> > > > > properties to the generic battery.yaml because they are:
> > > > > - Meaningful only for those charger drivers which have the VDR
> > > > > algorithm implemented. (And even though the algorithm is not charger
> > > > > specific, AFAICS, it is currently only used by some ROHM PMIC
> > > > > drivers).
> > > > > - Technique of measuring the VDR tables for a battery is not widely
> > > > > known. AFAICS, only folks at ROHM are measuring those for some
> > > > > customer products. We do have those tables available for some of the
> > > > > products though (Kobo?).
> > >
> > > or, to add new compatible for the "vdr-battery".
> > > AFAICS, adding new compatible would require us to wither duplicate the used
> > > properties from battery.yaml here (as battery.yaml mandates the
> > > "simple-battery" - compatible) - or to split the battery.yaml in two files,
> > > one containing the generic properties, other containing the "simple-battery"
> > > -compatible and referencing the generic one. Then the "vdr-battery" could
> > > also reference the generic one.
> > >
> > > Any suggestions for the next path to follow?
> >
> > Probably the latter option. You could do the former and make the new
> > properties conditional on the "vdr-battery" compatible. That's fine with
> > small differences, but gets messy as there are more properties and
> > variations.
> >
> > But is "VDR" a type of battery though? Is there a certain type/chemistry
> > of battery we should be describing where VDR is applicable?
>
> No. Not that I know. My understanding is that the "VDR (voltage drop rate)"
> refers to measured voltage drop-rates under certain conditions - which can
> be used to (more accurately) estimate the remaining capacity when battery is
> nearly depleted. As far as I know, this is only used with Lithium-ion
> batteries (I am not at all sure of this) - but I _assume_ the technique
> could be applied to other type of batteries as well.
>
> > I don't
> > think it scales well if we define battery compatibles for every
> > variation of charger algorithm. Honestly I don't mind just adding 1
> > property. I care more if we allow undocumented properties than
> > allowing documented but invalid for the platform properties.
>
> I see. The "VDR" stuff is really tightly bound to the fuel-gauging
> algorithm. It is measured characteristics of the battery - but those values
> are only usable by the "VDR" algorithm. I don't really have a good insight
> in the amount of fuel-gauging algorithm related properties suggested to be
> added during the years - but don't think there have been that many of them.
> So, I am not that worried about adding the compatible. On the other hand,
> there is no technical reason (other than adding properties which are unused
> on many platforms) why not to add the vdr tables in the static-battey node
> without adding own compatible. And, reading reply from Andreas (I'll copy it
> here to answer it in same mail)
>
> /// Below text is form Andreas:
> > just keep in mind, that several kobo devices have one pmic in one board
> > revision and another one in the other (e.g. Kobo Nia rev A vs rev C).
> > But probably the same battery. So if the "vdr-battery" is a compatible
> > just to allow a more properties,
> > then "simple-battery" should be allowed as fallback.
>
> I didn't know Kobos use multiple chargers. Thanks Andreas! So, in that
> sense, adding the "vdr" tables in static-battery node, without new
> compatible, would maybe be simplest solution. Then the charger(s)
> (fuel-gauge(s)) which implement VDR algorithm, can pick the tables while
> those chargers which don't implement the VDR will just ignore these tables.
>
> > When it
> > becomes 10, 20, 30 properties, then I might start to care.
>
> For VDR there are only:
>
> rohm,voltage-vdr-thresh-microvolt,
So "voltage voltage drop rate"? And '-microvolt' says this is voltage
too. :)
> rohm,volt-drop-soc-bp,
> rohm,volt-drop-temperatures-millicelsius
>
> and
>
> patternProperties:
> '^rohm,volt-drop-[0-9]-microvolt':
>
> So, from the binding point of view (.yaml), it's not _that_ lot. In the .dts
> there will be quite some noise as the tables have several values.
>
>
> > If that
> > happens, either we are doing a poor job of generically describing
> > battery parameters or chargers and batteries are tightly coupled and
> > can't be described independently.
>
> I am under impression that chargers tend to be pretty flexible, and they can
> be configured to work with many different batteries by altering the charging
> profiles. Most of the battery properties (like and charging phases [like
> pre, CC, CV], their limits, currents and voltages etc) are very generally
> usable. So, large subset of charging functionality can be handled with
> standard properties. I believe it is only the fuel-gauging where things get
> more hairy.
>
> I did prepare a series which does the split and adds new compatible for the
> 'rohm,vdr-battery'. (The power-supply class is not yet modified in the
> series, but we would probably want to modify the battery-info getters to
> also accept the 'rohm,vdr-battery' -compatible.)
I don't think that's the right direction. It's not a Rohm battery.
> I wonder if I should actually prepare also a series where these properties
> are just placed in the existing static battery node without adding new
> compatible. That way it would be easier to see which way is better.
That seems like the right thing to do here.
The main question for me is whether these should even be Rohm specific?
That would probably require a 2nd user to answer for sure.
> If I do that, should I only spin these bindings as RFC to avoid the
> unnecessary noise?
Only if you think something is not complete and/or the patches should
not be applied.
Rob
^ permalink raw reply
* Re: [PATCH v4 04/16] dt-bindings: power: supply: BD72720 managed battery
From: Matti Vaittinen @ 2025-11-17 8:12 UTC (permalink / raw)
To: Rob Herring
Cc: Matti Vaittinen, Krzysztof Kozlowski, Mark Brown, Linus Walleij,
linux-kernel, Sebastian Reichel, Bartosz Golaszewski,
Alexandre Belloni, linux-clk, Michael Turquette, Matti Vaittinen,
linux-leds, Pavel Machek, Liam Girdwood, linux-gpio, linux-pm,
Andreas Kemnade, Conor Dooley, devicetree, linux-rtc, Lee Jones,
Stephen Boyd
In-Reply-To: <20251114163954.GA3399895-robh@kernel.org>
On 14/11/2025 18:39, Rob Herring wrote:
> On Fri, Nov 14, 2025 at 11:04:27AM +0200, Matti Vaittinen wrote:
>> On 13/11/2025 12:53, Rob Herring (Arm) wrote:
>>>
>>> On Thu, 13 Nov 2025 10:52:19 +0200, Matti Vaittinen wrote:
>>>> From: Matti Vaittinen <mazziesaccount@gmail.com>
//snip
>>
>> So, as far as I understand, the only viable options are expanding the
>> existing battery.yaml with these properties (which I hoped to avoid, see
>> below)
>>
>>>> The right place for them is the battery node, which is described by the
>>>> generic "battery.yaml". I was not comfortable with adding these
>>>> properties to the generic battery.yaml because they are:
>>>> - Meaningful only for those charger drivers which have the VDR
>>>> algorithm implemented. (And even though the algorithm is not charger
>>>> specific, AFAICS, it is currently only used by some ROHM PMIC
>>>> drivers).
>>>> - Technique of measuring the VDR tables for a battery is not widely
>>>> known. AFAICS, only folks at ROHM are measuring those for some
>>>> customer products. We do have those tables available for some of the
>>>> products though (Kobo?).
>>
>> or, to add new compatible for the "vdr-battery".
>> AFAICS, adding new compatible would require us to wither duplicate the used
>> properties from battery.yaml here (as battery.yaml mandates the
>> "simple-battery" - compatible) - or to split the battery.yaml in two files,
>> one containing the generic properties, other containing the "simple-battery"
>> -compatible and referencing the generic one. Then the "vdr-battery" could
>> also reference the generic one.
>>
>> Any suggestions for the next path to follow?
>
> Probably the latter option. You could do the former and make the new
> properties conditional on the "vdr-battery" compatible. That's fine with
> small differences, but gets messy as there are more properties and
> variations.
>
> But is "VDR" a type of battery though? Is there a certain type/chemistry
> of battery we should be describing where VDR is applicable?
No. Not that I know. My understanding is that the "VDR (voltage drop
rate)" refers to measured voltage drop-rates under certain conditions -
which can be used to (more accurately) estimate the remaining capacity
when battery is nearly depleted. As far as I know, this is only used
with Lithium-ion batteries (I am not at all sure of this) - but I
_assume_ the technique could be applied to other type of batteries as well.
> I don't
> think it scales well if we define battery compatibles for every
> variation of charger algorithm. Honestly I don't mind just adding 1
> property. I care more if we allow undocumented properties than
> allowing documented but invalid for the platform properties.
I see. The "VDR" stuff is really tightly bound to the fuel-gauging
algorithm. It is measured characteristics of the battery - but those
values are only usable by the "VDR" algorithm. I don't really have a
good insight in the amount of fuel-gauging algorithm related properties
suggested to be added during the years - but don't think there have been
that many of them. So, I am not that worried about adding the
compatible. On the other hand, there is no technical reason (other than
adding properties which are unused on many platforms) why not to add the
vdr tables in the static-battey node without adding own compatible. And,
reading reply from Andreas (I'll copy it here to answer it in same mail)
/// Below text is form Andreas:
> just keep in mind, that several kobo devices have one pmic in one board
> revision and another one in the other (e.g. Kobo Nia rev A vs rev C).
> But probably the same battery. So if the "vdr-battery" is a compatible
> just to allow a more properties,
> then "simple-battery" should be allowed as fallback.
I didn't know Kobos use multiple chargers. Thanks Andreas! So, in that
sense, adding the "vdr" tables in static-battery node, without new
compatible, would maybe be simplest solution. Then the charger(s)
(fuel-gauge(s)) which implement VDR algorithm, can pick the tables while
those chargers which don't implement the VDR will just ignore these tables.
> When it
> becomes 10, 20, 30 properties, then I might start to care.
For VDR there are only:
rohm,voltage-vdr-thresh-microvolt,
rohm,volt-drop-soc-bp,
rohm,volt-drop-temperatures-millicelsius
and
patternProperties:
'^rohm,volt-drop-[0-9]-microvolt':
So, from the binding point of view (.yaml), it's not _that_ lot. In the
.dts there will be quite some noise as the tables have several values.
> If that
> happens, either we are doing a poor job of generically describing
> battery parameters or chargers and batteries are tightly coupled and
> can't be described independently.
I am under impression that chargers tend to be pretty flexible, and they
can be configured to work with many different batteries by altering the
charging profiles. Most of the battery properties (like and charging
phases [like pre, CC, CV], their limits, currents and voltages etc) are
very generally usable. So, large subset of charging functionality can be
handled with standard properties. I believe it is only the fuel-gauging
where things get more hairy.
I did prepare a series which does the split and adds new compatible for
the 'rohm,vdr-battery'. (The power-supply class is not yet modified in
the series, but we would probably want to modify the battery-info
getters to also accept the 'rohm,vdr-battery' -compatible.)
I wonder if I should actually prepare also a series where these
properties are just placed in the existing static battery node without
adding new compatible. That way it would be easier to see which way is
better.
If I do that, should I only spin these bindings as RFC to avoid the
unnecessary noise?
Oh, and a big thanks to both of you Rob and Andreas! I feel this gained
more clarity after your feedback :)
Yours,
-- Matti
---
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
^ permalink raw reply
* Re: [PATCH] rtc: isl12026: Implement callbacks for alarm feature
From: Alexandre Belloni @ 2025-11-16 22:08 UTC (permalink / raw)
To: Akhilesh Patil
Cc: andriy.shevchenko, david.daney, ddaney, david.hunter.linux, skhan,
linux-rtc, linux-kernel, akhileshpatilvnit
In-Reply-To: <20251116-51715-3266073@bhairav-test.ee.iitb.ac.in>
On 16/11/2025 10:47:15+0530, Akhilesh Patil wrote:
> > > static const struct rtc_class_ops isl12026_rtc_ops = {
> > > .read_time = isl12026_rtc_read_time,
> > > .set_time = isl12026_rtc_set_time,
> > > + .set_alarm = isl12026_rtc_set_alarm,
> > > + .read_alarm = isl12026_rtc_read_alarm,
> > > + .alarm_irq_enable = isl12026_rtc_alarm_irq_en,
> > > };
> > >
> >
> > This is missing an interrupt handler and proper handling in probe for
> > the wakeup-source property as this seems to be how you use it.
>
> Agree. However, I thought of first implementing alarm callbacks only and
> test them independedntly using ioctls for alarm settings in this patch.
> I will add interrupt handler and wakeup-source in v2 to complete this
> functionality.
Sure, the issue is that without interrupt handling and wakeup-source,
the driver will behave in a way that will break DT backward
compatibility later on.
--
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply
* Re: [PATCH] rtc: isl12026: Implement callbacks for alarm feature
From: Akhilesh Patil @ 2025-11-16 5:17 UTC (permalink / raw)
To: Alexandre Belloni
Cc: andriy.shevchenko, david.daney, ddaney, david.hunter.linux, skhan,
linux-rtc, linux-kernel, akhileshpatilvnit
In-Reply-To: <20251115112421bdd368f5@mail.local>
On Sat, Nov 15, 2025 at 12:24:21PM +0100, Alexandre Belloni wrote:
> On 15/11/2025 16:35:06+0530, Akhilesh Patil wrote:
> > Add alarm support for isl12026 RTC. Implement alarm function rtc
> > class callbacks - set_alarm, read_alarm and alarm_irq_enable.
> > isl12026 rtc has 2 alarms, this patch adds support to configure alarm0.
> > Note: isl12026 rtc chip share same pin(4) for alarm interrupt and square
> > wave frequency generator, hence forcefully disable SQW functionality
> > while writing to device registers in alarm functions.
> >
> > Tested on TI am62x sk board on i2c-2 port using selftests/rtc/rtctest
> >
> > Signed-off-by: Akhilesh Patil <akhilesh@ee.iitb.ac.in>
> > ---
> > Datasheet of RTC chip.
> > https://www.renesas.com/en/document/dst/isl12026-isl12026a-datasheet?srsltid=AfmBOopgN4vtn8XoN-8sOCfTW6yiLH-T7eeH_IWakqZ2VmENmWFqqh7w
> >
> > drivers/rtc/rtc-isl12026.c | 127 +++++++++++++++++++++++++++++++++++++
> > 1 file changed, 127 insertions(+)
> >
> > diff --git a/drivers/rtc/rtc-isl12026.c b/drivers/rtc/rtc-isl12026.c
> > index 2aabb9151d4c..7fa9ec7e4929 100644
> > --- a/drivers/rtc/rtc-isl12026.c
> > +++ b/drivers/rtc/rtc-isl12026.c
> > @@ -34,6 +34,11 @@
> > #define ISL12026_PAGESIZE 16
> > #define ISL12026_NVMEM_WRITE_TIME 20
Hi Alexandre, Thanks for the review.
Please find my comments below.
[...]
> >
> > +static int isl12026_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
> > +{
> > + struct i2c_client *client = to_i2c_client(dev);
> > + int ret;
> > + u8 buf_alrm_vals[7];
> > + struct i2c_msg msg;
> > + int ir;
> > +
> > + msg.addr = client->addr;
> > + msg.flags = 0x0; /* Write operation */
> > + msg.buf = buf_alrm_vals;
> > + msg.len = sizeof(buf_alrm_vals);
> > +
> > + if (!alrm->enabled) {
> > + /* Disable alarm and return */
>
> You still need to set the alarm time and date properly, the application
> is allowed to first set the alarm and then enable it.
Sure. Understood.
Will fix this in v2.
>
> > + ir = isl12026_read_reg(client, ISL12026_REG_INT);
> > + if (ir < 0)
> > + return ir;
> > + ir &= ~ISL12026_AL0E;
[...]
> > +
> > + /* Non-volatile Page write to AL0 registers and enable INT */
> > + ret = isl12026_arm_write(client);
> > + if (ret < 0)
> > + return ret;
> > + ret = i2c_transfer(client->adapter, &msg, 1);
> > + msleep(ISL12026_NVMEM_WRITE_TIME);
> > + if (ret != 1) {
> > + dev_err(&client->dev, "Error writing to alarm registers\n");
>
> No need for an error message.
ACK.
Will remove error message and just return error.
>
> > + return ret < 0 ? ret : -EIO;
> > + }
> > +
> > + /* Enable AL0 interrupt */
> > + ret = isl12026_write_reg(client, ISL12026_REG_INT, ISL12026_AL0E);
> > +
> > + return ret;
> > +}
> > +
> > +static int isl12026_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
> > +{
> > + struct i2c_client *client = to_i2c_client(dev);
> > + int ret;
> > + int sr, ir;
> > + u8 buf_alrm_vals[5];
> > + u8 addr[2] = {0x0, ISL12026_AL0_REG_SC};
> > + struct i2c_msg msgs[2] = { };
> > +
[...]
> > + alrm->pending = !!(sr & ISL12026_SR_AL0) && alrm->enabled;
> > +
> > + /* Page read for alarm registers */
> > + ret = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
> > + if (ret != ARRAY_SIZE(msgs)) {
> > + dev_err(&client->dev, "Error reading alarm registers\n");
>
> Ditto
ACK. Will remove err message.
>
> > + return ret < 0 ? ret : -EIO;
> > + }
> > +
> > + /* Populate values read */
> > + alrm->time.tm_sec = bcd2bin(buf_alrm_vals[0] & 0x7f);
> > + alrm->time.tm_min = bcd2bin(buf_alrm_vals[1] & 0x7f);
> > + alrm->time.tm_hour = bcd2bin(buf_alrm_vals[2] & 0x3f);
> > + alrm->time.tm_mday = bcd2bin(buf_alrm_vals[3] & 0x3f);
> > + alrm->time.tm_mon = bcd2bin(buf_alrm_vals[4] & 0x1f) - 1;
> > +
> > + return 0;
> > +}
> > +
> > +static int isl12026_rtc_alarm_irq_en(struct device *dev, unsigned int enabled)
> > +{
> > + struct i2c_client *client = to_i2c_client(dev);
> > + int ret;
> > + int ir;
> > +
> > + if (enabled) {
> > + ret = isl12026_write_reg(client, ISL12026_REG_INT, ISL12026_AL0E);
> > + return ret;
> > + }
> > +
> > + /* Disable alarm */
> > + ir = isl12026_read_reg(client, ISL12026_REG_INT);
> > + if (ir < 0)
> > + return ir;
> > + ir &= ~ISL12026_AL0E;
> > + ret = isl12026_write_reg(client, ISL12026_REG_INT, ir);
> > +
> > + return ret;
> > +}
> > +
> > static const struct rtc_class_ops isl12026_rtc_ops = {
> > .read_time = isl12026_rtc_read_time,
> > .set_time = isl12026_rtc_set_time,
> > + .set_alarm = isl12026_rtc_set_alarm,
> > + .read_alarm = isl12026_rtc_read_alarm,
> > + .alarm_irq_enable = isl12026_rtc_alarm_irq_en,
> > };
> >
>
> This is missing an interrupt handler and proper handling in probe for
> the wakeup-source property as this seems to be how you use it.
Agree. However, I thought of first implementing alarm callbacks only and
test them independedntly using ioctls for alarm settings in this patch.
I will add interrupt handler and wakeup-source in v2 to complete this
functionality.
Regards,
Akhilesh
>
>
> --
> Alexandre Belloni, co-owner and COO, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
^ permalink raw reply
* Re: [PATCH 06/13] mfd: sec-irq: add support for creating multiple IRQ chips
From: Kaustabh Chakraborty @ 2025-11-15 15:45 UTC (permalink / raw)
To: André Draszik
Cc: Alexandre Belloni, Lee Jones, Pavel Machek, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, MyungJoo Ham, Chanwoo Choi,
Sebastian Reichel, Krzysztof Kozlowski, Jonathan Corbet,
linux-leds, devicetree, linux-kernel, linux-pm, linux-samsung-soc,
linux-rtc, linux-doc
In-Reply-To: <6479a8d84052b326ffeb5609959aaf3a1aac9ff8.camel@linaro.org>
On 2025-11-14 11:55, André Draszik wrote:
> On Fri, 2025-11-14 at 08:50 +0100, Alexandre Belloni wrote:
>> On 14/11/2025 00:35:07+0530, Kaustabh Chakraborty wrote:
>> > The current state of the driver only allows creating only one IRQ chip
>> > per PMIC. On some PMICs, such as Samsung's S2MU005, there are multiple
>> > interrupt blocks, for which the current implementation stands insufficient.
>> >
>> > Add support for creating multiple IRQ chips for a PMIC. Every IRQ chip is
>> > given it's own index, which is used by sub-device drivers to request IRQs.
>> >
>> > A macro is defined which states the maximum number of chips supported.
>> > It's set to 1 as currently, no PMIC requires more than one IRQ chip. The
>> > value must be changed accordingly on adding new PMICs requiring multiple
>> > IRQ chips.
>> >
>> > Moreover, adjust the s5m RTC driver to initialize IRQs with the
>> > appropriate IRQ chip index.
>> >
>> > Signed-off-by: Kaustabh Chakraborty <kauschluss@disroot.org>
>> Acked-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
>>
>> > ---
>> > drivers/mfd/sec-irq.c | 163 +++++++++++++++++++++++----------------
>> > drivers/rtc/rtc-s5m.c | 15 +++-
>> > include/linux/mfd/samsung/core.h | 5 +-
>> > include/linux/mfd/samsung/irq.h | 14 ++++
>> > 4 files changed, 127 insertions(+), 70 deletions(-)
>
> Your patch reminded me to finally send
> https://lore.kernel.org/all/20251114-s5m-alarm-v1-0-c9b3bebae65f@linaro.org/
>
> If applied first, you wouldn't need to touch rtc-s5m.c I believe.
Oo, this cleans things up greatly!
>
> Equally, I can rebase mine on top of yours - no strong feelings.
I will wait for your series to be applied. Your series is much shorter,
so wouldn't hold that back for this. :)
>
> Cheers,
> Andre'
^ permalink raw reply
* Re: [PATCH v4 05/16] dt-bindings: mfd: ROHM BD72720
From: Krzysztof Kozlowski @ 2025-11-15 11:31 UTC (permalink / raw)
To: Matti Vaittinen
Cc: Matti Vaittinen, Lee Jones, Pavel Machek, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Sebastian Reichel,
Liam Girdwood, Mark Brown, Michael Turquette, Stephen Boyd,
Linus Walleij, Bartosz Golaszewski, Alexandre Belloni, linux-leds,
devicetree, linux-kernel, linux-pm, linux-clk, linux-gpio,
linux-rtc, Andreas Kemnade
In-Reply-To: <ec2cb44d9d00f5edaed2fbe17fd9ddbed914ff37.1763022807.git.mazziesaccount@gmail.com>
On Thu, Nov 13, 2025 at 10:52:35AM +0200, Matti Vaittinen wrote:
> From: Matti Vaittinen <mazziesaccount@gmail.com>
>
> The ROHM BD72720 is a power management IC integrating regulators, GPIOs,
> charger, LEDs, RTC and a clock gate.
>
> Add dt-binding doc for ROHM BD72720.
>
> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
>
> ---
> Revision history:
Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH] rtc: isl12026: Implement callbacks for alarm feature
From: Alexandre Belloni @ 2025-11-15 11:24 UTC (permalink / raw)
To: Akhilesh Patil
Cc: andriy.shevchenko, david.daney, ddaney, david.hunter.linux, skhan,
linux-rtc, linux-kernel, akhileshpatilvnit
In-Reply-To: <20251115-1156-3147571@bhairav-test.ee.iitb.ac.in>
On 15/11/2025 16:35:06+0530, Akhilesh Patil wrote:
> Add alarm support for isl12026 RTC. Implement alarm function rtc
> class callbacks - set_alarm, read_alarm and alarm_irq_enable.
> isl12026 rtc has 2 alarms, this patch adds support to configure alarm0.
> Note: isl12026 rtc chip share same pin(4) for alarm interrupt and square
> wave frequency generator, hence forcefully disable SQW functionality
> while writing to device registers in alarm functions.
>
> Tested on TI am62x sk board on i2c-2 port using selftests/rtc/rtctest
>
> Signed-off-by: Akhilesh Patil <akhilesh@ee.iitb.ac.in>
> ---
> Datasheet of RTC chip.
> https://www.renesas.com/en/document/dst/isl12026-isl12026a-datasheet?srsltid=AfmBOopgN4vtn8XoN-8sOCfTW6yiLH-T7eeH_IWakqZ2VmENmWFqqh7w
>
> drivers/rtc/rtc-isl12026.c | 127 +++++++++++++++++++++++++++++++++++++
> 1 file changed, 127 insertions(+)
>
> diff --git a/drivers/rtc/rtc-isl12026.c b/drivers/rtc/rtc-isl12026.c
> index 2aabb9151d4c..7fa9ec7e4929 100644
> --- a/drivers/rtc/rtc-isl12026.c
> +++ b/drivers/rtc/rtc-isl12026.c
> @@ -34,6 +34,11 @@
> #define ISL12026_PAGESIZE 16
> #define ISL12026_NVMEM_WRITE_TIME 20
>
> +#define ISL12026_AL0_REG_SC 0x0
> +#define ISL12026_REG_INT 0x11
> +#define ISL12026_AL0E BIT(5)
> +#define ISL12026_SR_AL0 BIT(5)
> +
> struct isl12026 {
> struct rtc_device *rtc;
> struct i2c_client *nvm_client;
> @@ -269,9 +274,131 @@ static int isl12026_rtc_read_time(struct device *dev, struct rtc_time *tm)
> return ret;
> }
>
> +static int isl12026_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
> +{
> + struct i2c_client *client = to_i2c_client(dev);
> + int ret;
> + u8 buf_alrm_vals[7];
> + struct i2c_msg msg;
> + int ir;
> +
> + msg.addr = client->addr;
> + msg.flags = 0x0; /* Write operation */
> + msg.buf = buf_alrm_vals;
> + msg.len = sizeof(buf_alrm_vals);
> +
> + if (!alrm->enabled) {
> + /* Disable alarm and return */
You still need to set the alarm time and date properly, the application
is allowed to first set the alarm and then enable it.
> + ir = isl12026_read_reg(client, ISL12026_REG_INT);
> + if (ir < 0)
> + return ir;
> + ir &= ~ISL12026_AL0E;
> + ret = isl12026_write_reg(client, ISL12026_REG_INT, ir);
> +
> + return ret;
> + }
> +
> + /* Prepare 5 bytes alarm data SC, MN, HR, DT, MO */
> + buf_alrm_vals[0] = 0x0;
> + buf_alrm_vals[1] = ISL12026_AL0_REG_SC;
> + buf_alrm_vals[2] = (bin2bcd(alrm->time.tm_sec) & 0x7f) | 0x80;
> + buf_alrm_vals[3] = (bin2bcd(alrm->time.tm_min) & 0x7f) | 0x80;
> + buf_alrm_vals[4] = (bin2bcd(alrm->time.tm_hour) & 0x3f) | 0x80;
> + buf_alrm_vals[5] = (bin2bcd(alrm->time.tm_mday) & 0x3f) | 0x80;
> + buf_alrm_vals[6] = (bin2bcd(alrm->time.tm_mon + 1) & 0x1f) | 0x80;
> +
> + /* Non-volatile Page write to AL0 registers and enable INT */
> + ret = isl12026_arm_write(client);
> + if (ret < 0)
> + return ret;
> + ret = i2c_transfer(client->adapter, &msg, 1);
> + msleep(ISL12026_NVMEM_WRITE_TIME);
> + if (ret != 1) {
> + dev_err(&client->dev, "Error writing to alarm registers\n");
No need for an error message.
> + return ret < 0 ? ret : -EIO;
> + }
> +
> + /* Enable AL0 interrupt */
> + ret = isl12026_write_reg(client, ISL12026_REG_INT, ISL12026_AL0E);
> +
> + return ret;
> +}
> +
> +static int isl12026_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
> +{
> + struct i2c_client *client = to_i2c_client(dev);
> + int ret;
> + int sr, ir;
> + u8 buf_alrm_vals[5];
> + u8 addr[2] = {0x0, ISL12026_AL0_REG_SC};
> + struct i2c_msg msgs[2] = { };
> +
> + msgs[0].addr = client->addr;
> + msgs[0].flags = 0x0; /* Write register address */
> + msgs[0].buf = addr;
> + msgs[0].len = sizeof(addr);
> +
> + msgs[1].addr = client->addr;
> + msgs[1].flags = I2C_M_RD; /* Alarm read operation */
> + msgs[1].buf = buf_alrm_vals;
> + msgs[1].len = sizeof(buf_alrm_vals);
> +
> + /* Read alarm enable status */
> + ir = isl12026_read_reg(client, ISL12026_REG_INT);
> + if (ir < 0)
> + return ir;
> + alrm->enabled = !!(ir & ISL12026_AL0E);
> +
> + /* Read alarm pending status */
> + sr = isl12026_read_reg(client, ISL12026_REG_SR);
> + if (sr < 0)
> + return sr;
> + alrm->pending = !!(sr & ISL12026_SR_AL0) && alrm->enabled;
> +
> + /* Page read for alarm registers */
> + ret = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
> + if (ret != ARRAY_SIZE(msgs)) {
> + dev_err(&client->dev, "Error reading alarm registers\n");
Ditto
> + return ret < 0 ? ret : -EIO;
> + }
> +
> + /* Populate values read */
> + alrm->time.tm_sec = bcd2bin(buf_alrm_vals[0] & 0x7f);
> + alrm->time.tm_min = bcd2bin(buf_alrm_vals[1] & 0x7f);
> + alrm->time.tm_hour = bcd2bin(buf_alrm_vals[2] & 0x3f);
> + alrm->time.tm_mday = bcd2bin(buf_alrm_vals[3] & 0x3f);
> + alrm->time.tm_mon = bcd2bin(buf_alrm_vals[4] & 0x1f) - 1;
> +
> + return 0;
> +}
> +
> +static int isl12026_rtc_alarm_irq_en(struct device *dev, unsigned int enabled)
> +{
> + struct i2c_client *client = to_i2c_client(dev);
> + int ret;
> + int ir;
> +
> + if (enabled) {
> + ret = isl12026_write_reg(client, ISL12026_REG_INT, ISL12026_AL0E);
> + return ret;
> + }
> +
> + /* Disable alarm */
> + ir = isl12026_read_reg(client, ISL12026_REG_INT);
> + if (ir < 0)
> + return ir;
> + ir &= ~ISL12026_AL0E;
> + ret = isl12026_write_reg(client, ISL12026_REG_INT, ir);
> +
> + return ret;
> +}
> +
> static const struct rtc_class_ops isl12026_rtc_ops = {
> .read_time = isl12026_rtc_read_time,
> .set_time = isl12026_rtc_set_time,
> + .set_alarm = isl12026_rtc_set_alarm,
> + .read_alarm = isl12026_rtc_read_alarm,
> + .alarm_irq_enable = isl12026_rtc_alarm_irq_en,
> };
>
This is missing an interrupt handler and proper handling in probe for
the wakeup-source property as this seems to be how you use it.
--
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply
* [PATCH] rtc: isl12026: Implement callbacks for alarm feature
From: Akhilesh Patil @ 2025-11-15 11:05 UTC (permalink / raw)
To: alexandre.belloni, andriy.shevchenko, david.daney, ddaney,
david.hunter.linux, skhan
Cc: linux-rtc, linux-kernel, akhileshpatilvnit
Add alarm support for isl12026 RTC. Implement alarm function rtc
class callbacks - set_alarm, read_alarm and alarm_irq_enable.
isl12026 rtc has 2 alarms, this patch adds support to configure alarm0.
Note: isl12026 rtc chip share same pin(4) for alarm interrupt and square
wave frequency generator, hence forcefully disable SQW functionality
while writing to device registers in alarm functions.
Tested on TI am62x sk board on i2c-2 port using selftests/rtc/rtctest
Signed-off-by: Akhilesh Patil <akhilesh@ee.iitb.ac.in>
---
Datasheet of RTC chip.
https://www.renesas.com/en/document/dst/isl12026-isl12026a-datasheet?srsltid=AfmBOopgN4vtn8XoN-8sOCfTW6yiLH-T7eeH_IWakqZ2VmENmWFqqh7w
drivers/rtc/rtc-isl12026.c | 127 +++++++++++++++++++++++++++++++++++++
1 file changed, 127 insertions(+)
diff --git a/drivers/rtc/rtc-isl12026.c b/drivers/rtc/rtc-isl12026.c
index 2aabb9151d4c..7fa9ec7e4929 100644
--- a/drivers/rtc/rtc-isl12026.c
+++ b/drivers/rtc/rtc-isl12026.c
@@ -34,6 +34,11 @@
#define ISL12026_PAGESIZE 16
#define ISL12026_NVMEM_WRITE_TIME 20
+#define ISL12026_AL0_REG_SC 0x0
+#define ISL12026_REG_INT 0x11
+#define ISL12026_AL0E BIT(5)
+#define ISL12026_SR_AL0 BIT(5)
+
struct isl12026 {
struct rtc_device *rtc;
struct i2c_client *nvm_client;
@@ -269,9 +274,131 @@ static int isl12026_rtc_read_time(struct device *dev, struct rtc_time *tm)
return ret;
}
+static int isl12026_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
+{
+ struct i2c_client *client = to_i2c_client(dev);
+ int ret;
+ u8 buf_alrm_vals[7];
+ struct i2c_msg msg;
+ int ir;
+
+ msg.addr = client->addr;
+ msg.flags = 0x0; /* Write operation */
+ msg.buf = buf_alrm_vals;
+ msg.len = sizeof(buf_alrm_vals);
+
+ if (!alrm->enabled) {
+ /* Disable alarm and return */
+ ir = isl12026_read_reg(client, ISL12026_REG_INT);
+ if (ir < 0)
+ return ir;
+ ir &= ~ISL12026_AL0E;
+ ret = isl12026_write_reg(client, ISL12026_REG_INT, ir);
+
+ return ret;
+ }
+
+ /* Prepare 5 bytes alarm data SC, MN, HR, DT, MO */
+ buf_alrm_vals[0] = 0x0;
+ buf_alrm_vals[1] = ISL12026_AL0_REG_SC;
+ buf_alrm_vals[2] = (bin2bcd(alrm->time.tm_sec) & 0x7f) | 0x80;
+ buf_alrm_vals[3] = (bin2bcd(alrm->time.tm_min) & 0x7f) | 0x80;
+ buf_alrm_vals[4] = (bin2bcd(alrm->time.tm_hour) & 0x3f) | 0x80;
+ buf_alrm_vals[5] = (bin2bcd(alrm->time.tm_mday) & 0x3f) | 0x80;
+ buf_alrm_vals[6] = (bin2bcd(alrm->time.tm_mon + 1) & 0x1f) | 0x80;
+
+ /* Non-volatile Page write to AL0 registers and enable INT */
+ ret = isl12026_arm_write(client);
+ if (ret < 0)
+ return ret;
+ ret = i2c_transfer(client->adapter, &msg, 1);
+ msleep(ISL12026_NVMEM_WRITE_TIME);
+ if (ret != 1) {
+ dev_err(&client->dev, "Error writing to alarm registers\n");
+ return ret < 0 ? ret : -EIO;
+ }
+
+ /* Enable AL0 interrupt */
+ ret = isl12026_write_reg(client, ISL12026_REG_INT, ISL12026_AL0E);
+
+ return ret;
+}
+
+static int isl12026_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
+{
+ struct i2c_client *client = to_i2c_client(dev);
+ int ret;
+ int sr, ir;
+ u8 buf_alrm_vals[5];
+ u8 addr[2] = {0x0, ISL12026_AL0_REG_SC};
+ struct i2c_msg msgs[2] = { };
+
+ msgs[0].addr = client->addr;
+ msgs[0].flags = 0x0; /* Write register address */
+ msgs[0].buf = addr;
+ msgs[0].len = sizeof(addr);
+
+ msgs[1].addr = client->addr;
+ msgs[1].flags = I2C_M_RD; /* Alarm read operation */
+ msgs[1].buf = buf_alrm_vals;
+ msgs[1].len = sizeof(buf_alrm_vals);
+
+ /* Read alarm enable status */
+ ir = isl12026_read_reg(client, ISL12026_REG_INT);
+ if (ir < 0)
+ return ir;
+ alrm->enabled = !!(ir & ISL12026_AL0E);
+
+ /* Read alarm pending status */
+ sr = isl12026_read_reg(client, ISL12026_REG_SR);
+ if (sr < 0)
+ return sr;
+ alrm->pending = !!(sr & ISL12026_SR_AL0) && alrm->enabled;
+
+ /* Page read for alarm registers */
+ ret = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
+ if (ret != ARRAY_SIZE(msgs)) {
+ dev_err(&client->dev, "Error reading alarm registers\n");
+ return ret < 0 ? ret : -EIO;
+ }
+
+ /* Populate values read */
+ alrm->time.tm_sec = bcd2bin(buf_alrm_vals[0] & 0x7f);
+ alrm->time.tm_min = bcd2bin(buf_alrm_vals[1] & 0x7f);
+ alrm->time.tm_hour = bcd2bin(buf_alrm_vals[2] & 0x3f);
+ alrm->time.tm_mday = bcd2bin(buf_alrm_vals[3] & 0x3f);
+ alrm->time.tm_mon = bcd2bin(buf_alrm_vals[4] & 0x1f) - 1;
+
+ return 0;
+}
+
+static int isl12026_rtc_alarm_irq_en(struct device *dev, unsigned int enabled)
+{
+ struct i2c_client *client = to_i2c_client(dev);
+ int ret;
+ int ir;
+
+ if (enabled) {
+ ret = isl12026_write_reg(client, ISL12026_REG_INT, ISL12026_AL0E);
+ return ret;
+ }
+
+ /* Disable alarm */
+ ir = isl12026_read_reg(client, ISL12026_REG_INT);
+ if (ir < 0)
+ return ir;
+ ir &= ~ISL12026_AL0E;
+ ret = isl12026_write_reg(client, ISL12026_REG_INT, ir);
+
+ return ret;
+}
+
static const struct rtc_class_ops isl12026_rtc_ops = {
.read_time = isl12026_rtc_read_time,
.set_time = isl12026_rtc_set_time,
+ .set_alarm = isl12026_rtc_set_alarm,
+ .read_alarm = isl12026_rtc_read_alarm,
+ .alarm_irq_enable = isl12026_rtc_alarm_irq_en,
};
static int isl12026_nvm_read(void *p, unsigned int offset,
--
2.34.1
^ permalink raw reply related
* Re: [PATCH v6 2/2] rtc: Add NXP PCF85053 driver support
From: Alexandre Belloni @ 2025-11-15 11:02 UTC (permalink / raw)
To: Lakshay Piplani
Cc: linux-rtc, linux-kernel, robh, krzk+dt, conor+dt, devicetree,
pankit.garg, vikash.bansal, priyanka.jain, shashank.rebbapragada,
Daniel Aguirre
In-Reply-To: <20251113054243.4045820-2-lakshay.piplani@nxp.com>
On 13/11/2025 11:12:43+0530, Lakshay Piplani wrote:
> +#define PCF85053_REG_BAT_MASK 0x07 /* Battery mask */
> +#define PCF85053A_BVL_MASK 0x07
> +#define PCF85053A_BVL_LOW_THRESHOLD 0x02
> +#define PCF85053_REG_CLKO_F_MASK 0x03 /* Frequenc mask */
> +#define PCF85053_REG_CLKO_CKE 0x80 /* clock out enabled */
> +#define PCF85053_BIT_OF BIT(6)
> +
> +#define PCF85053_HR_PM BIT(7)
> +#define PCF85053_HR_24H_MASK GENMASK(5, 0)
> +
> +struct pcf85053_config {
> + const struct regmap_config regmap;
> + unsigned has_alarms:1;
> +};
> +
> +struct pcf85053 {
> + struct rtc_device *rtc;
> + struct i2c_client *client;
This member is probably not necessary
> + struct regmap *regmap;
> +#ifdef CONFIG_COMMON_CLK
> + struct clk_hw clkout_hw;
> +#endif
> + bool is_primary;
> +};
> +
> +static inline int pcf85053_read_two_bit(struct pcf85053 *pcf85053, bool *two)
> +{
> + unsigned int ctrl;
> + int err;
> +
> + err = regmap_read(pcf85053->regmap, PCF85053_REG_CTRL, &ctrl);
> + if (err)
> + return err;
> +
> + *two = !!(ctrl & PCF85053_BIT_TWO);
> +
> + return 0;
> +}
> +
> +static inline bool pcf85053_time_write_access(struct pcf85053 *pcf85053)
> +{
> + bool two;
> +
> + if (pcf85053_read_two_bit(pcf85053, &two))
> + return false;
> +
> + /* Primary writes iff TWO=1; secondary writes iff TWO=0 */
> + return pcf85053->is_primary ? two : !two;
> +}
> +
> +static int pcf85053_set_alarm_mode(struct device *dev, bool on)
This should take the regmap as a parameter, you have multiple extra
level of indirection when calling this function.
> +{
> + struct pcf85053 *pcf85053 = dev_get_drvdata(dev);
> + unsigned int val;
> + int err;
> +
> + val = on ? PCF85053_BIT_AIE : 0;
> + val &= ~(PCF85053_BIT_CIE | PCF85053_BIT_OFIE);
This doesn't do anything
> +
> + err = regmap_update_bits(pcf85053->regmap, PCF85053_REG_CTRL,
> + PCF85053_BIT_AIE | PCF85053_BIT_CIE | PCF85053_BIT_OFIE,
> + val);
> + if (err)
> + return err;
> +
> + return regmap_update_bits(pcf85053->regmap, PCF85053_REG_ST,
> + PCF85053_BIT_AF, 0);
> +}
> +
> +static int pcf85053_get_alarm_mode(struct device *dev,
> + unsigned char *alarm_enable, unsigned char *alarm_flag)
> +{
> + struct pcf85053 *pcf85053 = dev_get_drvdata(dev);
> + unsigned int val;
> + int err;
> +
> + if (alarm_enable) {
> + err = regmap_read(pcf85053->regmap, PCF85053_REG_CTRL, &val);
> + if (err)
> + return err;
> +
> + *alarm_enable = val & PCF85053_BIT_AIE;
> + }
> +
> + if (alarm_flag) {
> + err = regmap_read(pcf85053->regmap, PCF85053_REG_ST, &val);
> + if (err)
> + return err;
> +
> + *alarm_flag = val & PCF85053_BIT_AF;
> + }
> +
> + return 0;
> +}
> +
> +static irqreturn_t pcf85053_irq(int irq, void *dev_id)
> +{
> + struct pcf85053 *pcf85053 = i2c_get_clientdata(dev_id);
> + unsigned char alarm_flag;
> + unsigned char alarm_enable;
> + int err;
> +
> + err = pcf85053_get_alarm_mode(&pcf85053->client->dev, &alarm_enable, &alarm_flag);
> + if (err)
> + return IRQ_NONE;
> +
> + if (!alarm_flag)
> + return IRQ_NONE;
> +
> + rtc_update_irq(pcf85053->rtc, 1, RTC_IRQF | RTC_AF);
> + pcf85053_set_alarm_mode(&pcf85053->client->dev, false);
I feel like this is reading and writing way too much, you could probably
only test and clear PCF85053_REG_ST, with a single
regmap_update_bits_check call
> +
> + return IRQ_HANDLED;
> +}
> +
> +/*
> + * In the routines that deal directly with the PCF85053 hardware, we use
> + * rtc_time -- month 0-11, hour 0-23, yr = calendar year-epoch.
> + */
> +static int pcf85053_rtc_read_time(struct device *dev, struct rtc_time *tm)
> +{
> + struct pcf85053 *pcf85053 = dev_get_drvdata(dev);
> + unsigned int ctrl, st, h12;
> + bool is_24h, is_bin;
> + u8 regs[10], hr;
> + int err;
> +
> + err = regmap_read(pcf85053->regmap, PCF85053_REG_CTRL, &ctrl);
> + if (err)
> + return err;
> +
> + err = regmap_read(pcf85053->regmap, PCF85053_REG_ST, &st);
> + if (err)
> + return err;
> +
> + if (ctrl & PCF85053_BIT_ST)
> + dev_warn(dev, "RTC is stopped; time may be invalid\n");
no message needed but return -EINVAL
> +
> + err = regmap_bulk_read(pcf85053->regmap, PCF85053_REG_SC, regs, sizeof(regs));
> + if (err)
> + return err;
> +
> + if (ctrl & PCF85053_BIT_DM) {
> + tm->tm_sec = regs[PCF85053_REG_SC] & 0x7F;
> + tm->tm_min = regs[PCF85053_REG_MN] & 0x7F;
> + tm->tm_mday = regs[PCF85053_REG_DM] & 0x3F;
> + tm->tm_mon = (regs[PCF85053_REG_MO] & 0x1F) - 1;
> + tm->tm_year = regs[PCF85053_REG_YR] + 100;
> + } else {
> + tm->tm_sec = bcd2bin(regs[PCF85053_REG_SC] & 0x7F);
> + tm->tm_min = bcd2bin(regs[PCF85053_REG_MN] & 0x7F);
> + tm->tm_mday = bcd2bin(regs[PCF85053_REG_DM] & 0x3F);
> + tm->tm_mon = bcd2bin(regs[PCF85053_REG_MO] & 0x1F) - 1;
> + tm->tm_year = bcd2bin(regs[PCF85053_REG_YR]) + 100;
> + }
> + tm->tm_wday = regs[PCF85053_REG_DW] & 0x07;
> +
> + hr = regs[PCF85053_REG_HR];
> + is_24h = ctrl & PCF85053_BIT_HF;
> + is_bin = ctrl & PCF85053_BIT_DM;
> +
> + if (is_24h) {
> + tm->tm_hour = is_bin
> + ? (hr & PCF85053_HR_24H_MASK)
> + : bcd2bin(hr & PCF85053_HR_24H_MASK);
> + } else {
> + if (is_bin) {
> + h12 = hr & PCF85053_HR_24H_MASK;
This doesn't do anything
> + } else {
> + h12 = is_bin ? (hr & PCF85053_HR_24H_MASK) :
> + bcd2bin(hr & PCF85053_HR_24H_MASK);
> +
> + tm->tm_hour = (h12 == 12) ? ((hr & PCF85053_HR_PM) ? 12 : 0) :
> + ((hr & PCF85053_HR_PM) ? h12 + 12 : h12);
> + }
> + }
> +
> + return 0;
> +}
> +
> +static int pcf85053_rtc_set_time(struct device *dev, struct rtc_time *tm)
> +
> +{
> + struct pcf85053 *pcf85053 = dev_get_drvdata(dev);
> + unsigned int ctrl, h12;
> + int err, ret;
> + u8 buf[10];
> + bool pm;
> +
> + /*
> + * By default, secondary have write access to time registers as TWO
> + * bit is 0 by default, if we set nxp,interface = "primary" and the
> + * nxp,write-access in device tree, then TWO bits gets set and primary
> + * gets write access to time registers.
> + */
> + if (!pcf85053_time_write_access(pcf85053))
> + return -EACCES;
> +
> + err = regmap_read(pcf85053->regmap, PCF85053_REG_CTRL, &ctrl);
> + if (err)
> + return err;
> +
> + buf[0] = tm->tm_sec & 0x7F;
> + buf[1] = 0;
> + buf[2] = tm->tm_min & 0x7F;
> + buf[3] = 0;
> + buf[5] = 0;
> + buf[6] = tm->tm_wday & 0x07;
> + buf[7] = tm->tm_mday & 0x3F;
> + buf[8] = (tm->tm_mon + 1) & 0x1F;
> + buf[9] = (tm->tm_year - 100) & 0xFF;
> +
> + if (ctrl & PCF85053_BIT_HF) {
> + buf[4] = tm->tm_hour & PCF85053_HR_24H_MASK;
> + } else {
> + pm = tm->tm_hour >= 12;
> + h12 = (tm->tm_hour % 12) ? (tm->tm_hour % 12) : 12;
> + buf[4] = (h12 & PCF85053_HR_24H_MASK) | (pm << 7);
> + }
> +
> + if (!(ctrl & PCF85053_BIT_DM)) {
> + buf[0] = bin2bcd(buf[0]);
> + buf[2] = bin2bcd(buf[2]);
> + buf[4] = bin2bcd(buf[4] & PCF85053_HR_24H_MASK) | (buf[4] & PCF85053_HR_PM);
> + buf[7] = bin2bcd(buf[7]);
> + buf[8] = bin2bcd(buf[8]);
> + buf[9] = bin2bcd(buf[9]);
> + }
> +
You have write access, simply set the date to the simplest format
instead of supporting all of them.
> + if (pcf85053->is_primary) {
> + err = regmap_update_bits(pcf85053->regmap, PCF85053_REG_CTRL,
> + PCF85053_BIT_ST, PCF85053_BIT_ST);
> + if (err)
> + return err;
> +
> + ret = regmap_bulk_write(pcf85053->regmap, PCF85053_REG_SC, buf, sizeof(buf));
> + err = regmap_update_bits(pcf85053->regmap, PCF85053_REG_CTRL,
> + PCF85053_BIT_ST, 0);
> + return ret ? ret : err;
> + }
> +
> + return regmap_bulk_write(pcf85053->regmap, PCF85053_REG_SC, buf, sizeof(buf));
> +}
> +
> +static int pcf85053_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *tm)
> +{
> + struct pcf85053 *pcf85053 = dev_get_drvdata(dev);
> + unsigned int ctrl, h12;
> + bool is_24h, is_bin, pm;
> + u8 buf[5];
> + u8 hr;
> + int err;
> +
> + err = regmap_read(pcf85053->regmap, PCF85053_REG_CTRL, &ctrl);
> + if (err)
> + return err;
> +
> + err = regmap_bulk_read(pcf85053->regmap, PCF85053_REG_SCA, buf, sizeof(buf));
> + if (err)
> + return err;
> +
> + if (ctrl & PCF85053_BIT_DM) {
> + tm->time.tm_sec = buf[0] & 0x7F; /* SCA */
> + tm->time.tm_min = buf[2] & 0x7F; /* MNA */
> + } else {
> + tm->time.tm_sec = bcd2bin(buf[0] & 0x7F);
> + tm->time.tm_min = bcd2bin(buf[2] & 0x7F);
> + }
> +
> + hr = buf[4];
> + is_24h = !!(ctrl & PCF85053_BIT_HF);
> + is_bin = !!(ctrl & PCF85053_BIT_DM);
> +
> + if (is_24h) {
> + tm->time.tm_hour = is_bin
> + ? (hr & PCF85053_HR_24H_MASK)
> + : bcd2bin(hr & PCF85053_HR_24H_MASK);
> + } else {
> + pm = !!(hr & PCF85053_HR_PM);
> +
> + if (is_bin)
> + h12 = (hr & PCF85053_HR_24H_MASK);
> + else
> + h12 = (bcd2bin(hr & PCF85053_HR_24H_MASK));
> +
> + if (h12 == 12)
> + h12 = 0;
> + tm->time.tm_hour = pm ? (h12 + 12) : h12;
> + }
> +
> + return pcf85053_get_alarm_mode(dev, &tm->enabled, &tm->pending);
> +}
> +
> +static int pcf85053_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *tm)
> +{
> + struct pcf85053 *pcf85053 = dev_get_drvdata(dev);
> + bool is_24h, is_bin, pm;
> + unsigned int ctrl, h12;
> + u8 sec, min, hra;
> + int err;
> +
> + /*
> + * Only primary can set alarm, as secondary have read only access
> + * to alarm, control and status registers
> + */
> + if (!pcf85053->is_primary)
> + return -EACCES;
> +
> + err = regmap_read(pcf85053->regmap, PCF85053_REG_CTRL, &ctrl);
> + if (err)
> + return err;
> +
> + err = regmap_update_bits(pcf85053->regmap, PCF85053_REG_ST,
> + PCF85053_BIT_AF, 0);
> + if (err)
> + return err;
> +
> + is_24h = !!(ctrl & PCF85053_BIT_HF);
> + is_bin = !!(ctrl & PCF85053_BIT_DM);
> +
> + sec = tm->time.tm_sec & 0x7F;
> + min = tm->time.tm_min & 0x7F;
> +
> + if (is_24h) {
> + hra = tm->time.tm_hour & PCF85053_HR_24H_MASK;
> + if (!is_bin)
> + hra = bin2bcd(hra) & PCF85053_HR_24H_MASK;
> + } else {
> + h12 = tm->time.tm_hour % 12;
> + pm = tm->time.tm_hour >= 12;
> + if (h12 == 0)
> + h12 = 12;
> +
> + if (is_bin)
> + hra = (h12 & PCF85053_HR_24H_MASK) | (pm << 7);
> + else
> + hra = (bin2bcd(h12) & PCF85053_HR_24H_MASK) | (pm << 7);
> + }
> +
> + if (!is_bin) {
> + sec = bin2bcd(sec);
> + min = bin2bcd(min);
> + }
> +
Same here.
> + err = regmap_write(pcf85053->regmap, PCF85053_REG_SCA, sec);
> + if (err)
> + return err;
> +
> + err = regmap_write(pcf85053->regmap, PCF85053_REG_MNA, min);
> + if (err)
> + return err;
> +
> + err = regmap_write(pcf85053->regmap, PCF85053_REG_HRA, hra);
> + if (err)
> + return err;
> +
> + return pcf85053_set_alarm_mode(dev, tm->enabled);
> +}
> +
> +static int pcf85053_irq_enable(struct device *dev, unsigned int enabled)
> +{
> + dev_dbg(dev, "%s: alarm enable=%d\n", __func__, enabled);
> +
> + return pcf85053_set_alarm_mode(dev, enabled);
> +}
> +
> +static int pcf85053_ioctl(struct device *dev, unsigned int cmd, unsigned long arg)
> +{
> + struct pcf85053 *pcf85053 = dev_get_drvdata(dev);
> + unsigned int val = 0, vl_status = 0;
> + unsigned int bvl;
> + int status;
> +
> + switch (cmd) {
> + case RTC_VL_READ:
> + status = regmap_read(pcf85053->regmap, PCF85053_REG_ST, &val);
> + if (status)
> + return status;
> +
> + if (val & PCF85053_BIT_OF)
> + vl_status |= RTC_VL_DATA_INVALID;
> +
> + bvl = val & PCF85053A_BVL_MASK;
> +
> + if (bvl == 0x00)
> + vl_status |= RTC_VL_BACKUP_EMPTY;
> + else if (bvl <= PCF85053A_BVL_LOW_THRESHOLD)
> + vl_status |= RTC_VL_BACKUP_LOW;
> +
> + return put_user(vl_status, (unsigned int __user *)arg);
> +
> + default:
> + return -ENOIOCTLCMD;
> + }
> +}
> +
> +#ifdef CONFIG_COMMON_CLK
> +/*
> + * Handling of the clkout
> + */
> +
> +#define clkout_hw_to_pcf85053(_hw) container_of(_hw, struct pcf85053, clkout_hw)
> +
> +static const int clkout_rates[] = {
> + 32768,
> + 1024,
> + 32,
> + 1,
> +};
> +
> +static unsigned long pcf85053_clkout_recalc_rate(struct clk_hw *hw,
> + unsigned long parent_rate)
> +{
> + struct pcf85053 *pcf85053 = clkout_hw_to_pcf85053(hw);
> + unsigned int val = 0;
> + int err;
> +
> + err = regmap_read(pcf85053->regmap, PCF85053_REG_CLKO, &val);
> + if (err)
> + return 0;
> +
> + val &= PCF85053_REG_CLKO_F_MASK;
> + return clkout_rates[val];
> +}
> +
> +static int pcf85053_clkout_determine_rate(struct clk_hw *hw,
> + struct clk_rate_request *req)
> +{
> + int i;
> + unsigned long best = 0;
> +
> + for (i = 0; i < ARRAY_SIZE(clkout_rates); i++) {
> + if (clkout_rates[i] <= req->rate) {
> + best = clkout_rates[i];
> + break;
> + }
> + }
> + if (!best)
> + best = clkout_rates[ARRAY_SIZE(clkout_rates) - 1];
> +
> + req->rate = best;
> + return 0;
> +}
> +
> +static int pcf85053_clkout_set_rate(struct clk_hw *hw, unsigned long rate,
> + unsigned long parent_rate)
> +{
> + struct pcf85053 *pcf85053 = clkout_hw_to_pcf85053(hw);
> + unsigned int val = 0;
> + int err, i;
> +
> + err = regmap_read(pcf85053->regmap, PCF85053_REG_CLKO, &val);
> + if (err)
> + return err;
> +
> + for (i = 0; i < ARRAY_SIZE(clkout_rates); i++)
> + if (clkout_rates[i] == rate) {
> + val &= ~PCF85053_REG_CLKO_F_MASK;
> + val |= i;
> + return regmap_write(pcf85053->regmap, PCF85053_REG_CLKO, val);
> + }
> +
> + return -EINVAL;
> +}
> +
> +static int pcf85053_clkout_control(struct clk_hw *hw, bool enable)
> +{
> + struct pcf85053 *pcf85053 = clkout_hw_to_pcf85053(hw);
> + unsigned int val = 0;
> + int err;
> +
> + if (!pcf85053->is_primary)
> + return -EACCES;
> +
> + val = PCF85053_BIT_XCLK;
> + err = regmap_write(pcf85053->regmap, PCF85053_REG_ACC, val);
> + if (err)
> + return err;
> +
> + err = regmap_read(pcf85053->regmap, PCF85053_REG_CLKO, &val);
> + if (err)
> + return err;
> +
> + if (enable)
> + val |= PCF85053_REG_CLKO_CKE;
> + else
> + val &= ~PCF85053_REG_CLKO_CKE;
> +
> + return regmap_write(pcf85053->regmap, PCF85053_REG_CLKO, val);
> +}
> +
> +static int pcf85053_clkout_prepare(struct clk_hw *hw)
> +{
> + return pcf85053_clkout_control(hw, 1);
> +}
> +
> +static void pcf85053_clkout_unprepare(struct clk_hw *hw)
> +{
> + pcf85053_clkout_control(hw, 0);
> +}
> +
> +static int pcf85053_clkout_is_prepared(struct clk_hw *hw)
> +{
> + struct pcf85053 *pcf85053 = clkout_hw_to_pcf85053(hw);
> + unsigned int val = 0;
> + int err;
> +
> + err = regmap_read(pcf85053->regmap, PCF85053_REG_CLKO, &val);
> + if (err)
> + return err;
> +
> + return val & PCF85053_REG_CLKO_CKE;
> +}
> +
> +static const struct clk_ops pcf85053_clkout_ops = {
> + .prepare = pcf85053_clkout_prepare,
> + .unprepare = pcf85053_clkout_unprepare,
> + .is_prepared = pcf85053_clkout_is_prepared,
> + .recalc_rate = pcf85053_clkout_recalc_rate,
> + .determine_rate = pcf85053_clkout_determine_rate,
> + .set_rate = pcf85053_clkout_set_rate,
> +};
> +
> +static struct clk *pcf85053_clkout_register_clk(struct pcf85053 *pcf85053)
> +{
> + struct i2c_client *client = pcf85053->client;
> + struct device_node *node = client->dev.of_node;
> + struct clk *clk;
> + struct clk_init_data init;
> +
> + init.name = "pcf85053-clkout";
> + init.ops = &pcf85053_clkout_ops;
> + init.flags = 0;
> + init.parent_names = NULL;
> + init.num_parents = 0;
> + pcf85053->clkout_hw.init = &init;
> +
> + /* optional override of the clockname */
> + of_property_read_string(node, "clock-output-names", &init.name);
> +
> + /* register the clock */
> + clk = devm_clk_register(&client->dev, &pcf85053->clkout_hw);
> +
> + if (!IS_ERR(clk))
> + of_clk_add_provider(node, of_clk_src_simple_get, clk);
> +
> + return clk;
> +}
> +#endif
> +
> +static const struct rtc_class_ops pcf85053_rtc_ops = {
> + .read_time = pcf85053_rtc_read_time,
> + .set_time = pcf85053_rtc_set_time,
> + .read_alarm = pcf85053_rtc_read_alarm,
> + .set_alarm = pcf85053_rtc_set_alarm,
> + .alarm_irq_enable = pcf85053_irq_enable,
> + .ioctl = pcf85053_ioctl,
> +};
> +
> +static const struct pcf85053_config config_pcf85053 = {
> + .regmap = {
> + .reg_bits = 8,
> + .val_bits = 8,
> + .max_register = 0x1D,
> + },
> + .has_alarms = 1,
> +};
> +
> +static int pcf85053_probe(struct i2c_client *client)
> +{
> + struct pcf85053 *pcf85053;
> + const struct pcf85053_config *config;
> + const char *iface = NULL;
> + int err;
> +
> + if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C |
> + I2C_FUNC_SMBUS_BYTE |
> + I2C_FUNC_SMBUS_BLOCK_DATA))
> + return -ENODEV;
> +
> + pcf85053 = devm_kzalloc(&client->dev, sizeof(struct pcf85053),
> + GFP_KERNEL);
> + if (!pcf85053)
> + return -ENOMEM;
> +
> + config = i2c_get_match_data(client);
> + if (!config)
> + return -ENODEV;
> +
> + pcf85053->regmap = devm_regmap_init_i2c(client, &config->regmap);
> + if (IS_ERR(pcf85053->regmap))
> + return PTR_ERR(pcf85053->regmap);
> +
> + i2c_set_clientdata(client, pcf85053);
> +
> + pcf85053->client = client;
> + device_set_wakeup_capable(&client->dev, 1);
> +
> + pcf85053->is_primary = true;
> +
> + if (of_property_read_string(client->dev.of_node, "nxp,interface", &iface))
> + return dev_err_probe(&client->dev, -EINVAL,
> + "Missing mandatory property: nxp,interface\n");
> + if (!strcmp(iface, "primary"))
> + pcf85053->is_primary = true;
> + else if (!strcmp(iface, "secondary"))
> + pcf85053->is_primary = false;
> + else
> + return dev_err_probe(&client->dev, -EINVAL,
> + "Invalid value for nxp,interface: %s\n", iface);
> +
> + if (pcf85053->is_primary) {
> + unsigned int ctrl;
> + int err;
> +
> + err = regmap_read(pcf85053->regmap, PCF85053_REG_CTRL, &ctrl);
> + if (err)
> + return err;
> +
> + if (of_property_read_bool(client->dev.of_node, "nxp,write-access")) {
> + if (!(ctrl & PCF85053_BIT_TWO)) {
> + err = regmap_update_bits(pcf85053->regmap, PCF85053_REG_CTRL,
> + PCF85053_BIT_TWO, PCF85053_BIT_TWO);
> + if (err)
> + return err;
> + }
> + dev_dbg(&client->dev, "Ownership set: TWO=1 (primary writes)\n");
> + } else {
> + /* TWO (Time Write Ownership) bit defaults to 0 (Secondary) */
> + dev_dbg(&client->dev, "Default ownership set: TWO=0 (secondary writes)\n");
> + }
> + }
> +
> + pcf85053->rtc = devm_rtc_allocate_device(&client->dev);
> + if (IS_ERR(pcf85053->rtc))
> + return PTR_ERR(pcf85053->rtc);
> +
> + pcf85053->rtc->ops = &pcf85053_rtc_ops;
> + pcf85053->rtc->range_min = RTC_TIMESTAMP_BEGIN_2000;
Was 2000 a leap year for this RTC? The datasheet would say it is.
> + pcf85053->rtc->range_max = RTC_TIMESTAMP_END_2099;
> + clear_bit(RTC_FEATURE_UPDATE_INTERRUPT, pcf85053->rtc->features);
> + clear_bit(RTC_FEATURE_ALARM, pcf85053->rtc->features);
> +
> + if (config->has_alarms && client->irq > 0) {
> + err = devm_request_threaded_irq(&client->dev, client->irq,
> + NULL, pcf85053_irq,
> + IRQF_ONESHOT | IRQF_TRIGGER_FALLING,
> + "pcf85053", client);
> + if (err) {
> + dev_err(&client->dev, "unable to request IRQ %d\n", client->irq);
> + } else {
> + set_bit(RTC_FEATURE_ALARM, pcf85053->rtc->features);
> + device_init_wakeup(&client->dev, true);
Use devm_device_init_wakeup
> + err = dev_pm_set_wake_irq(&client->dev, client->irq);
> + if (err)
> + dev_err(&client->dev, "failed to enable irq wake\n");
> + }
> + }
> +
> +#ifdef CONFIG_COMMON_CLK
> + /* register clk in common clk framework */
> + pcf85053_clkout_register_clk(pcf85053);
> +#endif
> +
> + return devm_rtc_register_device(pcf85053->rtc);
> +}
> +
> +static const struct i2c_device_id pcf85053_id[] = {
> + { "pcf85053", .driver_data = (kernel_ulong_t)&config_pcf85053 },
> + { }
> +};
> +MODULE_DEVICE_TABLE(i2c, pcf85053_id);
> +
> +static const struct of_device_id pcf85053_of_match[] = {
> + { .compatible = "nxp,pcf85053", .data = &config_pcf85053 },
> + {}
> +};
> +MODULE_DEVICE_TABLE(of, pcf85053_of_match);
> +
> +static struct i2c_driver pcf85053_driver = {
> + .driver = {
> + .name = "rtc-pcf85053",
> + .of_match_table = of_match_ptr(pcf85053_of_match),
> + },
> + .probe = pcf85053_probe,
> + .id_table = pcf85053_id,
> +};
> +
> +module_i2c_driver(pcf85053_driver);
> +
> +MODULE_AUTHOR("Pankit Garg <pankit.garg@nxp.com>");
> +MODULE_AUTHOR("Lakshay Piplani <lakshay.piplani@nxp.com>");
> +MODULE_DESCRIPTION("NXP pcf85053 RTC driver");
> +MODULE_LICENSE("GPL");
> --
> 2.25.1
>
--
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply
* 少子化は追い風。次世代の教育ビジネス
From: 石島紗希 @ 2025-11-14 23:38 UTC (permalink / raw)
To: linux-rtc
新規事業をお考えの経営者様へ
いつもお世話になります。
『少子化の時代に、教育事業！？』と、
思うかもしれませんが
実は、塾代補助金や教育無償化、贈与非課税といった
国策もあり、子ども1人にかける教育費は増加し、
市場は堅調に成長しています。
そんな中、 『少子化だから教育はダメ』
という誤った認識の方が多いので、
学習塾市場にはまだ、『手堅く利益がでるホワイトスペース』が残っています。
いったい、教育市場にどのようなビジネスチャンスがあるのか？
をオンラインセミナーでお伝えしますので、ぜひご視聴ください。
11月18日（火）16：00〜17：00 オンライン開催
----------------------------------------------------------
◆フランチャイズ事業 WEB説明会◆
“プログラミング教育×個別指導”
不況に強く、少子化でも成長する
ハイブリッド型の教育事業
▼ 視聴予約 ▼
https://wam-edu-fc.jp/wam2/
◆ ご案内事業 ◆
個別指導 WAM
◇ 提供 ◇
エイチ・エム・グループ
----------------------------------------------------------
少子化が進む中で、多くの人は 「教育市場は縮小する」 と考えています。
しかし実際は、子ども一人にかける教育費の増加に伴い、
市場は成長し続けています。
また、教育費は不況時でも削減されにくいため
コロナ下でも大きく落ち込むことなく底堅さを見せました。
幼児教育無償化などの国策もあり、今後も教育投資の増加が予想されます。
そこでご紹介するのが、「プログラミング×個別指導」の
ハイブリッド教育事業です。
プログラミングは小学校で必修化されたため、保護者の関心も高まっています。
本事業のスタートは「業界未経験」「社員１名」で可能です。
新たな事業収益づくりをお考えの方は、是非ご参加ください。
11月18日（火）16：00〜17：00 オンライン開催
▼ 視聴予約はこちら
https://wam-edu-fc.jp/wam2/
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
教育事業FCセミナー事務局
電話：0120-891-893
住所：東京都中央区銀座7-13-6
―――――――――――――――――――――――――――――――
本メールのご不要な方には大変ご迷惑をおかけいたしました。
今後ご案内が不要な方は下記URLよりお手続きをお願いいたします。
┗ https://wam-edu-fc.jp/mail/
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
^ permalink raw reply
* Re: [PATCH v4 04/16] dt-bindings: power: supply: BD72720 managed battery
From: Andreas Kemnade @ 2025-11-14 17:40 UTC (permalink / raw)
To: Rob Herring
Cc: Matti Vaittinen, Matti Vaittinen, Krzysztof Kozlowski, Mark Brown,
Linus Walleij, linux-kernel, Sebastian Reichel,
Bartosz Golaszewski, Alexandre Belloni, linux-clk,
Michael Turquette, Matti Vaittinen, linux-leds, Pavel Machek,
Liam Girdwood, linux-gpio, linux-pm, Conor Dooley, devicetree,
linux-rtc, Lee Jones, Stephen Boyd
In-Reply-To: <20251114163954.GA3399895-robh@kernel.org>
On Fri, 14 Nov 2025 10:39:54 -0600
Rob Herring <robh@kernel.org> wrote:
> On Fri, Nov 14, 2025 at 11:04:27AM +0200, Matti Vaittinen wrote:
> > On 13/11/2025 12:53, Rob Herring (Arm) wrote:
> > >
> > > On Thu, 13 Nov 2025 10:52:19 +0200, Matti Vaittinen wrote:
> > > > From: Matti Vaittinen <mazziesaccount@gmail.com>
> > > >
> > > > The BD72720 PMIC has a battery charger + coulomb counter block. These
> > > > can be used to manage charging of a lithium-ion battery and to do fuel
> > > > gauging.
> > > >
> > > > ROHM has developed a so called "zero-correction" -algorithm to improve
> > > > the fuel-gauging accuracy close to the point where battery is depleted.
> > > > This relies on battery specific "VDR" tables, which are measured from
> > > > the battery, and which describe the voltage drop rate. More thorough
> > > > explanation about the "zero correction" and "VDR" parameters is here:
> > > > https://lore.kernel.org/all/676253b9-ff69-7891-1f26-a8b5bb5a421b@fi.rohmeurope.com/
> > > >
> > > > Document the VDR zero-correction specific battery properties used by the
> > > > BD72720 and some other ROHM chargers.
> > > >
> > > > Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
> > > > Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> > > >
> > > > ---
> > > > NOTE:
> > > > Linus' rb-tag holds only if there's no further comments from Rob.
> > > >
> > > > Revision history:
> > > > v3 =>:
> > > > - No changes
> > > >
> > > > v2 => v3:
> > > > - Constrain VDR threshold voltage to 48V
> > > > - Use standard '-bp' -suffix for the rohm,volt-drop-soc
> > > >
> > > > RFCv1 => v2:
> > > > - Add units to rohm,volt-drop-soc (tenths of %)
> > > > - Give real temperatures matching the VDR tables, instead of vague
> > > > 'high', 'normal', 'low', 'very low'. (Add table of temperatures and
> > > > use number matching the right temperature index in the VDR table name).
> > > > - Fix typoed 'algorithm' in commit message.
> > > >
> > > > The parameters are describing the battery voltage drop rates - so they
> > > > are properties of the battery, not the charger. Thus they do not belong
> > > > in the charger node.
> > > >
> >
> > // snip
> >
> > > My bot found errors running 'make dt_binding_check' on your patch:
> > >
> > > yamllint warnings/errors:
> > >
> > > dtschema/dtc warnings/errors:
> > > /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/power/supply/rohm,vdr-battery.example.dtb: battery (simple-battery): 'degrade-cycle-microamp-hours', 'rohm,volt-drop-0-microvolt', 'rohm,volt-drop-1-microvolt', 'rohm,volt-drop-2-microvolt', 'rohm,volt-drop-3-temp-microvolt', 'rohm,volt-drop-soc-bp', 'rohm,volt-drop-temperatures-millicelsius', 'rohm,voltage-vdr-thresh-microvolt' do not match any of the regexes: '^ocv-capacity-table-[0-9]+$', '^pinctrl-[0-9]+$'
> > > from schema $id: http://devicetree.org/schemas/power/supply/battery.yaml
> > >
> >
> > Odd. I am pretty sure I didn't see this when I ran the make
> > dt_binding_check. Not 100% sure what happened there. I get this error now
> > though when including all the bindings to the check.
> >
> > Do I get this right - these errors result from the properties used in
> > example not being included in the battery.yaml? So, this means that the
> > check is done based on the binding (battery.yaml) where the compatible
> > (simple-battery) is defined - not based on the properties which are present
> > in this file where the example resides, (and which references the
> > battery.yaml)?
> >
> > ...
> >
> > Oh... Now that I wrote it I feel like an idiot.
> >
> > This approach couldn't work for the validation, right? Let's assume I had a
> > VDR battery, and I added a static-battery -node for it. Running the
> > validation would pick the battery.yaml based on the compatible (just as it
> > does here), and be completely unaware of this vdr-battery.yaml. I have no
> > idea why I thought this would work. Probably because I only thought this
> > from the documentation POV.
> >
> > So, as far as I understand, the only viable options are expanding the
> > existing battery.yaml with these properties (which I hoped to avoid, see
> > below)
> >
> > >> The right place for them is the battery node, which is described by the
> > >> generic "battery.yaml". I was not comfortable with adding these
> > >> properties to the generic battery.yaml because they are:
> > >> - Meaningful only for those charger drivers which have the VDR
> > >> algorithm implemented. (And even though the algorithm is not charger
> > >> specific, AFAICS, it is currently only used by some ROHM PMIC
> > >> drivers).
> > >> - Technique of measuring the VDR tables for a battery is not widely
> > >> known. AFAICS, only folks at ROHM are measuring those for some
> > >> customer products. We do have those tables available for some of the
> > >> products though (Kobo?).
> >
> > or, to add new compatible for the "vdr-battery".
> > AFAICS, adding new compatible would require us to wither duplicate the used
> > properties from battery.yaml here (as battery.yaml mandates the
> > "simple-battery" - compatible) - or to split the battery.yaml in two files,
> > one containing the generic properties, other containing the "simple-battery"
> > -compatible and referencing the generic one. Then the "vdr-battery" could
> > also reference the generic one.
> >
> > Any suggestions for the next path to follow?
>
> Probably the latter option. You could do the former and make the new
> properties conditional on the "vdr-battery" compatible. That's fine with
> small differences, but gets messy as there are more properties and
> variations.
>
just keep in mind, that several kobo devices have one pmic in one board
revision and another one in the other (e.g. Kobo Nia rev A vs rev C).
But probably the same battery. So if the "vdr-battery" is a compatible
just to allow a more properties,
then "simple-battery" should be allowed as fallback.
Regards,
Andreas
^ permalink raw reply
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