[PATCH v5 0/4] mfd/regulator/clk: bd71837: ROHM BD71837 PMIC driver

[PATCH v5 0/4] mfd/regulator/clk: bd71837: ROHM BD71837 PMIC driver

[Matti Vaittinen]

Patch series adding support for ROHM BD71837 PMIC.

BD71837 is a programmable Power Management IC for powering single-core,
dual-core, and quad-core SoC’s such as NXP-i.MX 8M. It is optimized for
low BOM cost and compact solution footprint. It integrates 8 buck
regulators and 7 LDO’s to provide all the power rails required by the
SoC and the commonly used peripherals.

The driver aims to not limit the usage of PMIC. Thus the buck and LDO
naming is generic and not tied to any specific purposes. However there
is following limitations which make it mostly suitable for use cases
where the processor where PMIC driver is running is powered by the PMIC:

- The PMIC is not re-initialized if it resets. PMIC may reset as a
  result of voltage monitoring (over/under voltage) or due to reset
  request. Driver is only initializing PMIC at probe. This is not
  problem as long as processor controlling PMIC is powered by PMIC.

- The PMIC internal state machine is ignored by driver. Driver assumes
  the PMIC is wired so that it is always in "run" state when controlled
  by the driver.

Changelog v5
- dropped regulator patches which are already applied to Mark's tree
Based on feedback from Rob Herring and Stephen Boyd
- mfd bindings:	explain why this can be interrupt-controller
- mfd bindings:	describe interrupts better
- mfd bindings:	require one cell interrupt specifier
- mfd bindings:	use generic node names in example
- mfd driver:	ack masked interrupt once at init
- clk bindings:	use generic node names in example
- clk driver:	use devm
- clk driver:	use of_clk_add_hw_provider
- clk driver:	change severity of print and how prints are emitted at
		probe error path.
- clk driver:	dropped forward declared functions
- clk configs:	drop unnecessary dependencies
- clk driver:	other styling issues
- mfd/clk DT:	drop clk node.

Changelog v4
- remove mutex from regulator state check as core prevents simultaneous
  accesses
- allow voltage change for bucks 1 to 4 when regulator is enabled
- fix indentiation problems
- properly correct SPDX comments

Changelog v3
- kill unused variable
- kill unused definitions
- use REGMAP_IRQ_REG

Changelog v2
Based on feedback from Mark Brown
- Squashed code and buildfile changes to same patch
- Fixed some styling issues
- Changed SPDX comments to CPP style
- Error out if voltage is changed when regulator is enabled instead of
  Disabling the regulator for duration of change
- Use devm_regulator_register
- Remove compatible usage from regulators - use parent dev for config
- Add a note about using regulator-boot-on for BUCK6 and 7
- fixed warnings from kbuild test robot

patch 1: 
        MFD driver and definitions bringing interrupt support and
        enabling clk and regulator subsystems.
Patches 2 and 3

This patch series is based on for-mfd-next

---

Matti Vaittinen (4):
  mfd: bd71837: mfd driver for ROHM BD71837 PMIC
  mfd: bd71837: Devicetree bindings for ROHM BD71837 PMIC
  clk: bd71837: Devicetree bindings for ROHM BD71837 PMIC
  clk: bd71837: Add driver for BD71837 PMIC clock

 .../bindings/clock/rohm,bd71837-clock.txt          |  38 +++
 .../devicetree/bindings/mfd/rohm,bd71837-pmic.txt  |  76 ++++++
 drivers/clk/Kconfig                                |   7 +
 drivers/clk/Makefile                               |   1 +
 drivers/clk/clk-bd71837.c                          | 146 +++++++++++
 drivers/mfd/Kconfig                                |  13 +
 drivers/mfd/Makefile                               |   1 +
 drivers/mfd/bd71837.c                              | 223 ++++++++++++++++
 include/linux/mfd/bd71837.h                        | 288 +++++++++++++++++++++
 9 files changed, 793 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/rohm,bd71837-clock.txt
 create mode 100644 Documentation/devicetree/bindings/mfd/rohm,bd71837-pmic.txt
 create mode 100644 drivers/clk/clk-bd71837.c
 create mode 100644 drivers/mfd/bd71837.c
 create mode 100644 include/linux/mfd/bd71837.h

-- 
2.14.3

[PATCH v5 1/4] mfd: bd71837: mfd driver for ROHM BD71837 PMIC

[Matti Vaittinen]

ROHM BD71837 PMIC MFD driver providing interrupts and support
for two subsystems:
- clk
- Regulators

Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
---
 drivers/mfd/Kconfig         |  13 ++
 drivers/mfd/Makefile        |   1 +
 drivers/mfd/bd71837.c       | 223 ++++++++++++++++++++++++++++++++++
 include/linux/mfd/bd71837.h | 288 ++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 525 insertions(+)
 create mode 100644 drivers/mfd/bd71837.c
 create mode 100644 include/linux/mfd/bd71837.h

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index b860eb5aa194..7aa05fc9ed8e 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -1787,6 +1787,19 @@ config MFD_STW481X
 	  in various ST Microelectronics and ST-Ericsson embedded
 	  Nomadik series.
 
+config MFD_BD71837
+	bool "BD71837 Power Management chip"
+	depends on I2C=y
+	depends on OF
+	select REGMAP_I2C
+	select REGMAP_IRQ
+	select MFD_CORE
+	help
+	  Select this option to get support for the ROHM BD71837
+	  Power Management chips. BD71837 is designed to power processors like
+	  NXP i.MX8. It contains 8 BUCK outputs and 7 LDOs, voltage monitoring
+	  and emergency shut down as well as 32,768KHz clock output.
+
 config MFD_STM32_LPTIMER
 	tristate "Support for STM32 Low-Power Timer"
 	depends on (ARCH_STM32 && OF) || COMPILE_TEST
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index e9fd20dba18d..09dc9eb3782c 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -227,4 +227,5 @@ obj-$(CONFIG_MFD_STM32_TIMERS) 	+= stm32-timers.o
 obj-$(CONFIG_MFD_MXS_LRADC)     += mxs-lradc.o
 obj-$(CONFIG_MFD_SC27XX_PMIC)	+= sprd-sc27xx-spi.o
 obj-$(CONFIG_RAVE_SP_CORE)	+= rave-sp.o
+obj-$(CONFIG_MFD_BD71837)	+= bd71837.o
 
diff --git a/drivers/mfd/bd71837.c b/drivers/mfd/bd71837.c
new file mode 100644
index 000000000000..93930f1f2893
--- /dev/null
+++ b/drivers/mfd/bd71837.c
@@ -0,0 +1,223 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (C) 2018 ROHM Semiconductors
+// bd71837.c -- ROHM BD71837MWV mfd driver
+
+#include <linux/module.h>
+#include <linux/moduleparam.h>
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/i2c.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/irqdomain.h>
+#include <linux/gpio.h>
+#include <linux/regmap.h>
+#include <linux/of_device.h>
+#include <linux/of_gpio.h>
+#include <linux/mfd/core.h>
+#include <linux/mfd/bd71837.h>
+
+/* bd71837 multi function cells */
+static struct mfd_cell bd71837_mfd_cells[] = {
+	{
+		.name = "bd71837-clk",
+		.of_compatible = "rohm,bd71837-clk",
+	}, {
+		.name = "bd71837-pmic",
+	},
+};
+
+static const struct regmap_irq bd71837_irqs[] = {
+	REGMAP_IRQ_REG(BD71837_INT_SWRST, 0, BD71837_INT_SWRST_MASK),
+	REGMAP_IRQ_REG(BD71837_INT_PWRBTN_S, 0, BD71837_INT_PWRBTN_S_MASK),
+	REGMAP_IRQ_REG(BD71837_INT_PWRBTN_L, 0, BD71837_INT_PWRBTN_L_MASK),
+	REGMAP_IRQ_REG(BD71837_INT_PWRBTN, 0, BD71837_INT_PWRBTN_MASK),
+	REGMAP_IRQ_REG(BD71837_INT_WDOG, 0, BD71837_INT_WDOG_MASK),
+	REGMAP_IRQ_REG(BD71837_INT_ON_REQ, 0, BD71837_INT_ON_REQ_MASK),
+	REGMAP_IRQ_REG(BD71837_INT_STBY_REQ, 0, BD71837_INT_STBY_REQ_MASK),
+};
+
+static struct regmap_irq_chip bd71837_irq_chip = {
+	.name = "bd71837-irq",
+	.irqs = bd71837_irqs,
+	.num_irqs = ARRAY_SIZE(bd71837_irqs),
+	.num_regs = 1,
+	.irq_reg_stride = 1,
+	.status_base = BD71837_REG_IRQ,
+	.mask_base = BD71837_REG_MIRQ,
+	.init_ack_masked = true,
+	.mask_invert = false,
+};
+
+static int bd71837_irq_exit(struct bd71837 *bd71837)
+{
+	if (bd71837->chip_irq > 0)
+		regmap_del_irq_chip(bd71837->chip_irq, bd71837->irq_data);
+	return 0;
+}
+
+static const struct regmap_range pmic_status_range = {
+	.range_min = BD71837_REG_IRQ,
+	.range_max = BD71837_REG_POW_STATE,
+};
+
+static const struct regmap_access_table volatile_regs = {
+	.yes_ranges = &pmic_status_range,
+	.n_yes_ranges = 1,
+};
+
+static const struct regmap_config bd71837_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.volatile_table = &volatile_regs,
+	.max_register = BD71837_MAX_REGISTER - 1,
+	.cache_type = REGCACHE_RBTREE,
+};
+
+#ifdef CONFIG_OF
+static const struct of_device_id bd71837_of_match[] = {
+	{ .compatible = "rohm,bd71837", .data = (void *)0},
+	{ },
+};
+MODULE_DEVICE_TABLE(of, bd71837_of_match);
+
+static int bd71837_parse_dt(struct i2c_client *client, struct bd71837_board **b)
+{
+	struct device_node *np = client->dev.of_node;
+	struct bd71837_board *board_info;
+	unsigned int prop;
+	int r;
+	int rv = -ENOMEM;
+
+	board_info = devm_kzalloc(&client->dev, sizeof(*board_info),
+			GFP_KERNEL);
+	if (!board_info)
+		goto err_out;
+
+	if (client->irq) {
+		dev_dbg(&client->dev, "Got irq %d\n", client->irq);
+		board_info->gpio_intr = client->irq;
+	} else {
+		dev_err(&client->dev, "no pmic intr pin available\n");
+		rv = -ENOENT;
+		goto err_intr;
+	}
+	r = of_property_read_u32(np, "irq_base", &prop);
+	if (!r)
+		board_info->irq_base = prop;
+	else
+		board_info->irq_base = 0;
+
+	rv = 0;
+	*b = board_info;
+	if (0) {
+err_intr:
+		devm_kfree(&client->dev, board_info);
+err_out:
+		dev_err(&client->dev, "failed to parse device-tree (%d)\n", rv);
+	}
+	return rv;
+}
+#endif //CONFIG_OF
+
+static int bd71837_i2c_probe(struct i2c_client *i2c,
+			    const struct i2c_device_id *id)
+{
+	struct bd71837 *bd71837;
+	struct bd71837_board *pmic_plat_data;
+	int ret = -EINVAL;
+
+	pmic_plat_data = dev_get_platdata(&i2c->dev);
+
+	if (!pmic_plat_data && i2c->dev.of_node)
+		ret = bd71837_parse_dt(i2c, &pmic_plat_data);
+
+	if (!pmic_plat_data)
+		return ret;
+
+	bd71837 = devm_kzalloc(&i2c->dev, sizeof(struct bd71837), GFP_KERNEL);
+	if (bd71837 == NULL)
+		return -ENOMEM;
+
+	bd71837->of_plat_data = pmic_plat_data;
+	i2c_set_clientdata(i2c, bd71837);
+	bd71837->dev = &i2c->dev;
+	bd71837->i2c_client = i2c;
+	bd71837->chip_irq = pmic_plat_data->gpio_intr;
+
+	bd71837->regmap = devm_regmap_init_i2c(i2c, &bd71837_regmap_config);
+	if (IS_ERR(bd71837->regmap)) {
+		ret = PTR_ERR(bd71837->regmap);
+		dev_err(&i2c->dev, "regmap initialization failed: %d\n", ret);
+		goto err_out;
+	}
+
+	ret = bd71837_reg_read(bd71837, BD71837_REG_REV);
+	if (ret < 0) {
+		dev_err(bd71837->dev,
+			"%s(): Read BD71837_REG_DEVICE failed!\n", __func__);
+		goto err_out;
+	}
+
+	ret = regmap_add_irq_chip(bd71837->regmap, bd71837->chip_irq,
+		IRQF_ONESHOT, 0,
+		&bd71837_irq_chip, &bd71837->irq_data);
+	if (ret < 0) {
+		dev_err(bd71837->dev, "Failed to add irq_chip %d\n", ret);
+		goto err_out;
+	}
+
+	ret = mfd_add_devices(bd71837->dev, PLATFORM_DEVID_AUTO,
+			      bd71837_mfd_cells, ARRAY_SIZE(bd71837_mfd_cells),
+			      NULL, 0,
+			      regmap_irq_get_domain(bd71837->irq_data));
+	if (ret)
+		regmap_del_irq_chip(bd71837->chip_irq, bd71837->irq_data);
+err_out:
+
+	return ret;
+}
+
+static int bd71837_i2c_remove(struct i2c_client *i2c)
+{
+	struct bd71837 *bd71837 = i2c_get_clientdata(i2c);
+
+	bd71837_irq_exit(bd71837);
+	mfd_remove_devices(bd71837->dev);
+
+	return 0;
+}
+
+static const struct i2c_device_id bd71837_i2c_id[] = {
+	{ .name = "bd71837", },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, bd71837_i2c_id);
+
+static struct i2c_driver bd71837_i2c_driver = {
+	.driver = {
+		.name = "bd71837-mfd",
+		.owner = THIS_MODULE,
+		.of_match_table = of_match_ptr(bd71837_of_match),
+	},
+	.probe = bd71837_i2c_probe,
+	.remove = bd71837_i2c_remove,
+	.id_table = bd71837_i2c_id,
+};
+
+static int __init bd71837_i2c_init(void)
+{
+	return i2c_add_driver(&bd71837_i2c_driver);
+}
+/* init early so consumer devices can complete system boot */
+subsys_initcall(bd71837_i2c_init);
+
+static void __exit bd71837_i2c_exit(void)
+{
+	i2c_del_driver(&bd71837_i2c_driver);
+}
+module_exit(bd71837_i2c_exit);
+
+MODULE_AUTHOR("Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>");
+MODULE_DESCRIPTION("BD71837 chip multi-function driver");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/mfd/bd71837.h b/include/linux/mfd/bd71837.h
new file mode 100644
index 000000000000..641b7dba3076
--- /dev/null
+++ b/include/linux/mfd/bd71837.h
@@ -0,0 +1,288 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/* Copyright (C) 2018 ROHM Semiconductors */
+
+/*
+ * ROHM BD71837MWV header file
+ */
+
+#ifndef __LINUX_MFD_BD71837_H__
+#define __LINUX_MFD_BD71837_H__
+
+#include <linux/regmap.h>
+
+enum {
+	BD71837_BUCK1	=	0,
+	BD71837_BUCK2,
+	BD71837_BUCK3,
+	BD71837_BUCK4,
+	BD71837_BUCK5,
+	BD71837_BUCK6,
+	BD71837_BUCK7,
+	BD71837_BUCK8,
+	BD71837_LDO1,
+	BD71837_LDO2,
+	BD71837_LDO3,
+	BD71837_LDO4,
+	BD71837_LDO5,
+	BD71837_LDO6,
+	BD71837_LDO7,
+	BD71837_REGULATOR_CNT,
+};
+
+#define BD71837_BUCK1_VOLTAGE_NUM	0x40
+#define BD71837_BUCK2_VOLTAGE_NUM	0x40
+#define BD71837_BUCK3_VOLTAGE_NUM	0x40
+#define BD71837_BUCK4_VOLTAGE_NUM	0x40
+
+#define BD71837_BUCK5_VOLTAGE_NUM	0x08
+#define BD71837_BUCK6_VOLTAGE_NUM	0x04
+#define BD71837_BUCK7_VOLTAGE_NUM	0x08
+#define BD71837_BUCK8_VOLTAGE_NUM	0x40
+
+#define BD71837_LDO1_VOLTAGE_NUM	0x04
+#define BD71837_LDO2_VOLTAGE_NUM	0x02
+#define BD71837_LDO3_VOLTAGE_NUM	0x10
+#define BD71837_LDO4_VOLTAGE_NUM	0x10
+#define BD71837_LDO5_VOLTAGE_NUM	0x10
+#define BD71837_LDO6_VOLTAGE_NUM	0x10
+#define BD71837_LDO7_VOLTAGE_NUM	0x10
+
+enum {
+	BD71837_REG_REV                = 0x00,
+	BD71837_REG_SWRESET            = 0x01,
+	BD71837_REG_I2C_DEV            = 0x02,
+	BD71837_REG_PWRCTRL0           = 0x03,
+	BD71837_REG_PWRCTRL1           = 0x04,
+	BD71837_REG_BUCK1_CTRL         = 0x05,
+	BD71837_REG_BUCK2_CTRL         = 0x06,
+	BD71837_REG_BUCK3_CTRL         = 0x07,
+	BD71837_REG_BUCK4_CTRL         = 0x08,
+	BD71837_REG_BUCK5_CTRL         = 0x09,
+	BD71837_REG_BUCK6_CTRL         = 0x0A,
+	BD71837_REG_BUCK7_CTRL         = 0x0B,
+	BD71837_REG_BUCK8_CTRL         = 0x0C,
+	BD71837_REG_BUCK1_VOLT_RUN     = 0x0D,
+	BD71837_REG_BUCK1_VOLT_IDLE    = 0x0E,
+	BD71837_REG_BUCK1_VOLT_SUSP    = 0x0F,
+	BD71837_REG_BUCK2_VOLT_RUN     = 0x10,
+	BD71837_REG_BUCK2_VOLT_IDLE    = 0x11,
+	BD71837_REG_BUCK3_VOLT_RUN     = 0x12,
+	BD71837_REG_BUCK4_VOLT_RUN     = 0x13,
+	BD71837_REG_BUCK5_VOLT         = 0x14,
+	BD71837_REG_BUCK6_VOLT         = 0x15,
+	BD71837_REG_BUCK7_VOLT         = 0x16,
+	BD71837_REG_BUCK8_VOLT         = 0x17,
+	BD71837_REG_LDO1_VOLT          = 0x18,
+	BD71837_REG_LDO2_VOLT          = 0x19,
+	BD71837_REG_LDO3_VOLT          = 0x1A,
+	BD71837_REG_LDO4_VOLT          = 0x1B,
+	BD71837_REG_LDO5_VOLT          = 0x1C,
+	BD71837_REG_LDO6_VOLT          = 0x1D,
+	BD71837_REG_LDO7_VOLT          = 0x1E,
+	BD71837_REG_TRANS_COND0        = 0x1F,
+	BD71837_REG_TRANS_COND1        = 0x20,
+	BD71837_REG_VRFAULTEN          = 0x21,
+	BD71837_REG_MVRFLTMASK0        = 0x22,
+	BD71837_REG_MVRFLTMASK1        = 0x23,
+	BD71837_REG_MVRFLTMASK2        = 0x24,
+	BD71837_REG_RCVCFG             = 0x25,
+	BD71837_REG_RCVNUM             = 0x26,
+	BD71837_REG_PWRONCONFIG0       = 0x27,
+	BD71837_REG_PWRONCONFIG1       = 0x28,
+	BD71837_REG_RESETSRC           = 0x29,
+	BD71837_REG_MIRQ               = 0x2A,
+	BD71837_REG_IRQ                = 0x2B,
+	BD71837_REG_IN_MON             = 0x2C,
+	BD71837_REG_POW_STATE          = 0x2D,
+	BD71837_REG_OUT32K             = 0x2E,
+	BD71837_REG_REGLOCK            = 0x2F,
+	BD71837_REG_OTPVER             = 0xFF,
+	BD71837_MAX_REGISTER           = 0x100,
+};
+
+#define REGLOCK_PWRSEQ	0x1
+#define REGLOCK_VREG	0x10
+
+/* Generic BUCK control masks */
+#define BD71837_BUCK_SEL	0x02
+#define BD71837_BUCK_EN		0x01
+#define BD71837_BUCK_RUN_ON	0x04
+
+/* Generic LDO masks */
+#define BD71837_LDO_SEL		0x80
+#define BD71837_LDO_EN		0x40
+
+/* BD71837 BUCK ramp rate CTRL reg bits */
+#define BUCK_RAMPRATE_MASK	0xC0
+#define BUCK_RAMPRATE_10P00MV	0x0
+#define BUCK_RAMPRATE_5P00MV	0x1
+#define BUCK_RAMPRATE_2P50MV	0x2
+#define BUCK_RAMPRATE_1P25MV	0x3
+
+/* BD71837_REG_BUCK1_VOLT_RUN bits */
+#define BUCK1_RUN_MASK		0x3F
+#define BUCK1_RUN_DEFAULT	0x14
+
+/* BD71837_REG_BUCK1_VOLT_SUSP bits */
+#define BUCK1_SUSP_MASK		0x3F
+#define BUCK1_SUSP_DEFAULT	0x14
+
+/* BD71837_REG_BUCK1_VOLT_IDLE bits */
+#define BUCK1_IDLE_MASK		0x3F
+#define BUCK1_IDLE_DEFAULT	0x14
+
+/* BD71837_REG_BUCK2_VOLT_RUN bits */
+#define BUCK2_RUN_MASK		0x3F
+#define BUCK2_RUN_DEFAULT	0x1E
+
+/* BD71837_REG_BUCK2_VOLT_IDLE bits */
+#define BUCK2_IDLE_MASK		0x3F
+#define BUCK2_IDLE_DEFAULT	0x14
+
+/* BD71837_REG_BUCK3_VOLT_RUN bits */
+#define BUCK3_RUN_MASK		0x3F
+#define BUCK3_RUN_DEFAULT	0x1E
+
+/* BD71837_REG_BUCK4_VOLT_RUN bits */
+#define BUCK4_RUN_MASK		0x3F
+#define BUCK4_RUN_DEFAULT	0x1E
+
+/* BD71837_REG_BUCK5_VOLT bits */
+#define BUCK5_MASK		0x07
+#define BUCK5_DEFAULT		0x02
+
+/* BD71837_REG_BUCK6_VOLT bits */
+#define BUCK6_MASK		0x03
+#define BUCK6_DEFAULT		0x03
+
+/* BD71837_REG_BUCK7_VOLT bits */
+#define BUCK7_MASK		0x07
+#define BUCK7_DEFAULT		0x03
+
+/* BD71837_REG_BUCK8_VOLT bits */
+#define BUCK8_MASK		0x3F
+#define BUCK8_DEFAULT		0x1E
+
+/* BD71837_REG_IRQ bits */
+#define IRQ_SWRST		0x40
+#define IRQ_PWRON_S		0x20
+#define IRQ_PWRON_L		0x10
+#define IRQ_PWRON		0x08
+#define IRQ_WDOG		0x04
+#define IRQ_ON_REQ		0x02
+#define IRQ_STBY_REQ		0x01
+
+/* BD71837_REG_OUT32K bits */
+#define BD71837_OUT32K_EN	0x01
+
+/* BD71837 gated clock rate */
+#define BD71837_CLK_RATE 32768
+
+/* BD71837 irqs */
+enum {
+	BD71837_INT_STBY_REQ,
+	BD71837_INT_ON_REQ,
+	BD71837_INT_WDOG,
+	BD71837_INT_PWRBTN,
+	BD71837_INT_PWRBTN_L,
+	BD71837_INT_PWRBTN_S,
+	BD71837_INT_SWRST
+};
+
+/* BD71837 interrupt masks */
+#define BD71837_INT_SWRST_MASK		0x40
+#define BD71837_INT_PWRBTN_S_MASK	0x20
+#define BD71837_INT_PWRBTN_L_MASK	0x10
+#define BD71837_INT_PWRBTN_MASK		0x8
+#define BD71837_INT_WDOG_MASK		0x4
+#define BD71837_INT_ON_REQ_MASK		0x2
+#define BD71837_INT_STBY_REQ_MASK	0x1
+
+/* BD71837_REG_LDO1_VOLT bits */
+#define LDO1_MASK		0x03
+
+/* BD71837_REG_LDO1_VOLT bits */
+#define LDO2_MASK		0x20
+
+/* BD71837_REG_LDO3_VOLT bits */
+#define LDO3_MASK		0x0F
+
+/* BD71837_REG_LDO4_VOLT bits */
+#define LDO4_MASK		0x0F
+
+/* BD71837_REG_LDO5_VOLT bits */
+#define LDO5_MASK		0x0F
+
+/* BD71837_REG_LDO6_VOLT bits */
+#define LDO6_MASK		0x0F
+
+/* BD71837_REG_LDO7_VOLT bits */
+#define LDO7_MASK		0x0F
+
+struct bd71837;
+struct bd71837_pmic;
+struct bd71837_clk;
+
+/*
+ * Board platform data may be used to initialize regulators.
+ */
+
+struct bd71837_board {
+	struct regulator_init_data *init_data[BD71837_REGULATOR_CNT];
+	int	gpio_intr;
+	int	irq_base;
+};
+
+struct bd71837 {
+	struct device *dev;
+	struct i2c_client *i2c_client;
+	struct regmap *regmap;
+	unsigned long int id;
+
+	int chip_irq;
+	struct regmap_irq_chip_data *irq_data;
+
+	struct bd71837_pmic *pmic;
+	struct bd71837_clk *clk;
+
+	struct bd71837_board *of_plat_data;
+};
+
+/*
+ * bd71837 sub-driver chip access routines
+ */
+
+static inline int bd71837_reg_read(struct bd71837 *bd71837, u8 reg)
+{
+	int r, val;
+
+	r = regmap_read(bd71837->regmap, reg, &val);
+	if (r < 0)
+		return r;
+	return val;
+}
+
+static inline int bd71837_reg_write(struct bd71837 *bd71837, u8 reg,
+				    unsigned int val)
+{
+	return regmap_write(bd71837->regmap, reg, val);
+}
+
+static inline int bd71837_set_bits(struct bd71837 *bd71837, u8 reg, u8 mask)
+{
+	return regmap_update_bits(bd71837->regmap, reg, mask, mask);
+}
+
+static inline int bd71837_clear_bits(struct bd71837 *bd71837, u8 reg,
+				     u8 mask)
+{
+	return regmap_update_bits(bd71837->regmap, reg, mask, 0);
+}
+
+static inline int bd71837_update_bits(struct bd71837 *bd71837, u8 reg,
+				      u8 mask, u8 val)
+{
+	return regmap_update_bits(bd71837->regmap, reg, mask, val);
+}
+
+#endif /* __LINUX_MFD_BD71837_H__ */
-- 
2.14.3

Re: [PATCH v5 1/4] mfd: bd71837: mfd driver for ROHM BD71837 PMIC

[Matti Vaittinen]

On Mon, Jun 04, 2018 at 04:18:07PM +0300, Matti Vaittinen wrote:
> +static struct regmap_irq_chip bd71837_irq_chip = {
> +	.name = "bd71837-irq",
> +	.irqs = bd71837_irqs,
> +	.num_irqs = ARRAY_SIZE(bd71837_irqs),
> +	.num_regs = 1,
> +	.irq_reg_stride = 1,
> +	.status_base = BD71837_REG_IRQ,
> +	.mask_base = BD71837_REG_MIRQ,
> +	.init_ack_masked = true,
> +	.mask_invert = false,
> +};

.ack_base = BD71837_REG_IRQ, is missing. I'll send yet another version
with this fixed - unless Rob tells me to drop the whole irq handling
from the patch series.

Br,
	Matti Vaittinen

[PATCH v5 2/4] mfd: bd71837: Devicetree bindings for ROHM BD71837 PMIC

[Matti Vaittinen]

Document devicetree bindings for ROHM BD71837 PMIC MFD.

Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
---
 .../devicetree/bindings/mfd/rohm,bd71837-pmic.txt  | 76 ++++++++++++++++++++++
 1 file changed, 76 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mfd/rohm,bd71837-pmic.txt

diff --git a/Documentation/devicetree/bindings/mfd/rohm,bd71837-pmic.txt b/Documentation/devicetree/bindings/mfd/rohm,bd71837-pmic.txt
new file mode 100644
index 000000000000..ac2b66181f17
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/rohm,bd71837-pmic.txt
@@ -0,0 +1,76 @@
+* ROHM BD71837 Power Management Integrated Circuit bindings
+
+BD71837MWV is a programmable Power Management IC for powering single-core,
+dual-core, and quad-core SoC’s such as NXP-i.MX 8M. It is optimized for
+low BOM cost and compact solution footprint. It integrates 8 Buck
+egulators and 7 LDO’s to provide all the power rails required by the SoC and
+the commonly used peripherals.
+
+Required properties:
+ - compatible		: Should be "rohm,bd71837".
+ - reg			: I2C slave address.
+ - interrupt-parent	: Phandle to the parent interrupt controller.
+ - interrupts		: The interrupt line the device is connected to.
+ - regulators:		: List of child nodes that specify the regulators
+			  Please see ../regulator/rohm,bd71837-regulator.txt
+ - clock:		: Please see ../clock/rohm,bd71837-clock.txt
+
+Optional properties:
+ - interrupt-controller	: Marks the device node as an interrupt controller.
+			  BD71837MWV can report different power state change
+			  events to other drivers. Different events can be seen
+			  as separate BD71837 domain interrupts.
+			  The BD71837 driver only provides the infrastructure
+			  for the IRQs. The users should write own driver to
+			  convert the IRQ into the event they wish. The IRQ can
+			  be used with the standard
+			  request_irq/enable_irq/disable_irq API inside the
+			  kernel.
+ - #interrupt-cells	: The number of cells to describe an IRQ should be 1.
+			    The value in cell is the IRQ number.
+			    Meaningfull numbers are:
+			      0 => PMIC_STBY_REQ level change
+			      1 => PMIC_ON_REQ level change
+			      2 => WDOG_B level change
+			      3 => Power Button level change
+			      4 => Power Button Long Push
+			      5 => Power Button Short Push
+			      6 => SWRESET register is written 1
+
+Example:
+
+	pmic: pmic@4b {
+		compatible = "rohm,bd71837";
+		#address-cells = <1>;
+		#size-cells = <0>;
+		reg = <0x4b>;
+		interrupt-parent = <&gpio1>;
+		interrupts = <29 GPIO_ACTIVE_LOW>;
+		interrupt-names = "irq";
+		#interrupt-cells = <1>;
+		interrupt-controller;
+		#clock-cells = <0>;
+		clock-frequency = <32768>;
+
+		regulators {
+			buck1: BUCK1 {
+				regulator-name = "buck1";
+				regulator-min-microvolt = <700000>;
+				regulator-max-microvolt = <1300000>;
+				regulator-boot-on;
+				regulator-ramp-delay = <1250>;
+			};
+			/* ... */
+		};
+	};
+
+	/* driver consuming PMIC interrupts */
+
+	my-power-button: power-button {
+		compatible = "foo";
+		interrupt-parent = <&pmic>;
+		interrupts = <3>, <4>, <5>;
+		interrupt-names = "pwrb", "pwrb-l", "pwrb-s";
+		/* ... */
+	};
+
-- 
2.14.3

Re: [PATCH v5 2/4] mfd: bd71837: Devicetree bindings for ROHM BD71837 PMIC

[Rob Herring]

On Mon, Jun 04, 2018 at 04:18:30PM +0300, Matti Vaittinen wrote:
> Document devicetree bindings for ROHM BD71837 PMIC MFD.
> 
> Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
> ---
>  .../devicetree/bindings/mfd/rohm,bd71837-pmic.txt  | 76 ++++++++++++++++++++++
>  1 file changed, 76 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mfd/rohm,bd71837-pmic.txt

I've replied on the prior version discussion. Please don't send new 
versions if the last one is still under discussion.

Rob

Re: [PATCH v5 2/4] mfd: bd71837: Devicetree bindings for ROHM BD71837 PMIC

[Matti Vaittinen]

On Tue, Jun 05, 2018 at 09:47:57AM -0600, Rob Herring wrote:
> On Mon, Jun 04, 2018 at 04:18:30PM +0300, Matti Vaittinen wrote:
> > Document devicetree bindings for ROHM BD71837 PMIC MFD.
> > 
> > Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
> > ---
> >  .../devicetree/bindings/mfd/rohm,bd71837-pmic.txt  | 76 ++++++++++++++++++++++
> >  1 file changed, 76 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/mfd/rohm,bd71837-pmic.txt
> 
> I've replied on the prior version discussion. Please don't send new 
> versions if the last one is still under discussion.

Allright. I thought this was a good way to suggest something which I
thought might address your concerns. But you are correct, it is better
to continue discussion in one emai thread. Sorry for adding this
version. I'll write further replies in v4 thread untill we get some
conclusion.

Br,
	Matti Vaittinen

[PATCH v5 3/4] clk: bd71837: Devicetree bindings for ROHM BD71837 PMIC

[Matti Vaittinen]

Document devicetree bindings for ROHM BD71837 PMIC clock output.

Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
---
 .../bindings/clock/rohm,bd71837-clock.txt          | 38 ++++++++++++++++++++++
 1 file changed, 38 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/rohm,bd71837-clock.txt

diff --git a/Documentation/devicetree/bindings/clock/rohm,bd71837-clock.txt b/Documentation/devicetree/bindings/clock/rohm,bd71837-clock.txt
new file mode 100644
index 000000000000..771acfe34114
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/rohm,bd71837-clock.txt
@@ -0,0 +1,38 @@
+ROHM BD71837 Power Management Integrated Circuit clock bindings
+
+This is a part of device tree bindings of ROHM BD71837 multi-function
+device. See generic BD71837 MFD bindings at:
+	Documentation/devicetree/bindings/mfd/rohm,bd71837-pmic.txt
+
+BD71837 contains one 32,768 KHz clock output which can be enabled and
+disabled via i2c.
+
+Following properties should be present in main device node of the MFD chip.
+
+Required properties:
+- clock-frequency	: Should be 32768
+- #clock-cells		: Should be 0
+
+Optional properties:
+- clock-output-names	: Should contain name for output clock.
+
+Example:
+
+/* MFD node */
+
+pmic: pmic@4b {
+	compatible = "rohm,bd71837";
+	/* ... */
+	#clock-cells = <0>;
+	clock-frequency  = <32768>;
+	/* ... */
+};
+
+/* Clock consumer node */
+
+foo@0 {
+	compatible = "bar,foo";
+	/* ... */
+	clock-names = "my-clock";
+	clocks = <&pmic>;
+};
-- 
2.14.3

Re: [PATCH v5 3/4] clk: bd71837: Devicetree bindings for ROHM BD71837 PMIC

[Rob Herring]

On Mon, Jun 04, 2018 at 04:18:53PM +0300, Matti Vaittinen wrote:
> Document devicetree bindings for ROHM BD71837 PMIC clock output.
> 
> Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
> ---
>  .../bindings/clock/rohm,bd71837-clock.txt          | 38 ++++++++++++++++++++++
>  1 file changed, 38 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/clock/rohm,bd71837-clock.txt
> 
> diff --git a/Documentation/devicetree/bindings/clock/rohm,bd71837-clock.txt b/Documentation/devicetree/bindings/clock/rohm,bd71837-clock.txt
> new file mode 100644
> index 000000000000..771acfe34114
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/rohm,bd71837-clock.txt
> @@ -0,0 +1,38 @@
> +ROHM BD71837 Power Management Integrated Circuit clock bindings

This needs to be added to the MFD doc. One node should be covered by at 
most 1 document.

Rob

Re: [PATCH v5 3/4] clk: bd71837: Devicetree bindings for ROHM BD71837 PMIC

[Matti Vaittinen]

On Tue, Jun 05, 2018 at 09:49:32AM -0600, Rob Herring wrote:
> On Mon, Jun 04, 2018 at 04:18:53PM +0300, Matti Vaittinen wrote:
> > Document devicetree bindings for ROHM BD71837 PMIC clock output.
> > 
> > Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
> > ---
> >  .../bindings/clock/rohm,bd71837-clock.txt          | 38 ++++++++++++++++++++++
> >  1 file changed, 38 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/clock/rohm,bd71837-clock.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/clock/rohm,bd71837-clock.txt b/Documentation/devicetree/bindings/clock/rohm,bd71837-clock.txt
> > new file mode 100644
> > index 000000000000..771acfe34114
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/clock/rohm,bd71837-clock.txt
> > @@ -0,0 +1,38 @@
> > +ROHM BD71837 Power Management Integrated Circuit clock bindings
> 
> This needs to be added to the MFD doc. One node should be covered by at 
> most 1 document.

I was thinking of that too. But then I asked why? I also thought that if
one knows there is clock block in the chip - where does he look for
binding document? From clock folder. Then I saw how bindings for
MAX77686 chip were written and thought that this is beneficial for all.
MFD document directs to clock and regulator docs and on othe other hand,
clock document clearly states that properties it describes must be
present "in main device node of the MFD chip".

Don't you think on searching for clock bindings should find something
from clock folder? I can follow your instruction here but I think
the user might be happy if he found something under bindings/clock for
clock related properties.

Br,
	Matti Vaittinen

[PATCH v5 4/4] clk: bd71837: Add driver for BD71837 PMIC clock

[Matti Vaittinen]

Support BD71837 gateable 32768 Hz clock.

Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
---
 drivers/clk/Kconfig       |   7 +++
 drivers/clk/Makefile      |   1 +
 drivers/clk/clk-bd71837.c | 146 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 154 insertions(+)
 create mode 100644 drivers/clk/clk-bd71837.c

diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index 41492e980ef4..e693496f202a 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -279,6 +279,13 @@ config COMMON_CLK_STM32H7
 	---help---
 	  Support for stm32h7 SoC family clocks
 
+config COMMON_CLK_BD71837
+	tristate "Clock driver for ROHM BD71837 PMIC MFD"
+	depends on MFD_BD71837
+	help
+	  This driver supports ROHM BD71837 PMIC clock.
+
+
 source "drivers/clk/bcm/Kconfig"
 source "drivers/clk/hisilicon/Kconfig"
 source "drivers/clk/imgtec/Kconfig"
diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index de6d06ac790b..8393c4af7d5a 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -21,6 +21,7 @@ endif
 obj-$(CONFIG_MACH_ASM9260)		+= clk-asm9260.o
 obj-$(CONFIG_COMMON_CLK_AXI_CLKGEN)	+= clk-axi-clkgen.o
 obj-$(CONFIG_ARCH_AXXIA)		+= clk-axm5516.o
+obj-$(CONFIG_COMMON_CLK_BD71837)	+= clk-bd71837.o
 obj-$(CONFIG_COMMON_CLK_CDCE706)	+= clk-cdce706.o
 obj-$(CONFIG_COMMON_CLK_CDCE925)	+= clk-cdce925.o
 obj-$(CONFIG_ARCH_CLPS711X)		+= clk-clps711x.o
diff --git a/drivers/clk/clk-bd71837.c b/drivers/clk/clk-bd71837.c
new file mode 100644
index 000000000000..5ba6c05c5a98
--- /dev/null
+++ b/drivers/clk/clk-bd71837.c
@@ -0,0 +1,146 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (C) 2018 ROHM Semiconductors
+// bd71837.c  -- ROHM BD71837MWV clock driver
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/err.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/mfd/bd71837.h>
+#include <linux/clk-provider.h>
+#include <linux/clkdev.h>
+
+
+struct bd71837_clk {
+	struct clk_hw hw;
+	uint8_t reg;
+	uint8_t mask;
+	unsigned long rate;
+	struct platform_device *pdev;
+	struct bd71837 *mfd;
+};
+
+static int bd71837_clk_set(struct clk_hw *hw, int status)
+{
+	struct bd71837_clk *c = container_of(hw, struct bd71837_clk, hw);
+
+	return bd71837_update_bits(c->mfd, c->reg, c->mask, status);
+}
+
+static void bd71837_clk_disable(struct clk_hw *hw)
+{
+	int rv;
+	struct bd71837_clk *c = container_of(hw, struct bd71837_clk, hw);
+
+	rv = bd71837_clk_set(hw, 0);
+	if (rv)
+		dev_dbg(&c->pdev->dev, "Failed to disable 32K clk (%d)\n", rv);
+}
+
+static int bd71837_clk_enable(struct clk_hw *hw)
+{
+	return bd71837_clk_set(hw, 1);
+}
+
+static int bd71837_clk_is_enabled(struct clk_hw *hw)
+{
+	struct bd71837_clk *c = container_of(hw, struct bd71837_clk, hw);
+
+	return c->mask & bd71837_reg_read(c->mfd, c->reg);
+}
+
+static unsigned long bd71837_clk_recalc_rate(struct clk_hw *hw,
+					     unsigned long parent_rate)
+{
+	struct bd71837_clk *c = container_of(hw, struct bd71837_clk, hw);
+
+	return c->rate;
+}
+
+static const struct clk_ops bd71837_clk_ops = {
+	.recalc_rate = &bd71837_clk_recalc_rate,
+	.prepare = &bd71837_clk_enable,
+	.unprepare = &bd71837_clk_disable,
+	.is_prepared = &bd71837_clk_is_enabled,
+};
+
+static int bd71837_clk_probe(struct platform_device *pdev)
+{
+	struct bd71837_clk *c;
+	int rval = -ENOMEM;
+	struct bd71837 *mfd = dev_get_drvdata(pdev->dev.parent);
+	struct clk_init_data init = {
+		.name = "bd71837-32k-out",
+		.ops = &bd71837_clk_ops,
+	};
+
+	c = devm_kzalloc(&pdev->dev, sizeof(*c), GFP_KERNEL);
+	if (!c)
+		goto err_out;
+
+	c->reg = BD71837_REG_OUT32K;
+	c->mask = BD71837_OUT32K_EN;
+	c->rate = BD71837_CLK_RATE;
+	c->mfd = mfd;
+	c->pdev = pdev;
+
+	of_property_read_string_index(pdev->dev.parent->of_node,
+					      "clock-output-names", 0,
+					      &init.name);
+
+	c->hw.init = &init;
+
+	rval = devm_clk_hw_register(&pdev->dev, &c->hw);
+	if (rval) {
+		dev_err(&pdev->dev, "failed to register 32K clk");
+		goto err_out;
+	}
+
+	if (pdev->dev.parent->of_node) {
+		rval = of_clk_add_hw_provider(pdev->dev.parent->of_node,
+					     of_clk_hw_simple_get,
+					     &c->hw);
+		if (rval) {
+			dev_err(&pdev->dev, "adding clk provider failed\n");
+			goto err_out;
+		}
+	}
+
+	rval = clk_hw_register_clkdev(&c->hw, init.name, NULL);
+	if (rval) {
+		dev_err(&pdev->dev, "failed to register clkdev for bd71837");
+		goto err_clean_provider;
+	}
+
+	platform_set_drvdata(pdev, c);
+
+	return 0;
+
+err_clean_provider:
+	of_clk_del_provider(pdev->dev.parent->of_node);
+err_out:
+	return rval;
+}
+
+static int bd71837_clk_remove(struct platform_device *pdev)
+{
+	if (pdev->dev.parent->of_node)
+		of_clk_del_provider(pdev->dev.parent->of_node);
+	return 0;
+}
+
+static struct platform_driver bd71837_clk = {
+	.driver = {
+		.name = "bd71837-clk",
+	},
+	.probe = bd71837_clk_probe,
+	.remove = bd71837_clk_remove,
+};
+
+module_platform_driver(bd71837_clk);
+
+MODULE_AUTHOR("Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>");
+MODULE_DESCRIPTION("BD71837 chip clk driver");
+MODULE_LICENSE("GPL");
-- 
2.14.3

Re: [PATCH v5 4/4] clk: bd71837: Add driver for BD71837 PMIC clock

[Stephen Boyd]

Quoting Matti Vaittinen (2018-06-04 06:19:13)
> diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
> index 41492e980ef4..e693496f202a 100644
> --- a/drivers/clk/Kconfig
> +++ b/drivers/clk/Kconfig
> @@ -279,6 +279,13 @@ config COMMON_CLK_STM32H7
>         ---help---
>           Support for stm32h7 SoC family clocks
>  
> +config COMMON_CLK_BD71837
> +       tristate "Clock driver for ROHM BD71837 PMIC MFD"
> +       depends on MFD_BD71837
> +       help
> +         This driver supports ROHM BD71837 PMIC clock.
> +
> +

Drop one newline above.

>  source "drivers/clk/bcm/Kconfig"
>  source "drivers/clk/hisilicon/Kconfig"
>  source "drivers/clk/imgtec/Kconfig"
> diff --git a/drivers/clk/clk-bd71837.c b/drivers/clk/clk-bd71837.c
> new file mode 100644
> index 000000000000..5ba6c05c5a98
> --- /dev/null
> +++ b/drivers/clk/clk-bd71837.c
> @@ -0,0 +1,146 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (C) 2018 ROHM Semiconductors
> +// bd71837.c  -- ROHM BD71837MWV clock driver
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/err.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <linux/mfd/bd71837.h>
> +#include <linux/clk-provider.h>
> +#include <linux/clkdev.h>
> +
> +

Drop one newline above.

> +struct bd71837_clk {
> +       struct clk_hw hw;
> +       uint8_t reg;
> +       uint8_t mask;

Use u8 instead of uint8_t.

> +       unsigned long rate;
> +       struct platform_device *pdev;
> +       struct bd71837 *mfd;
> +};
> +
> +static int bd71837_clk_set(struct clk_hw *hw, int status)
> +{
> +       struct bd71837_clk *c = container_of(hw, struct bd71837_clk, hw);
> +
> +       return bd71837_update_bits(c->mfd, c->reg, c->mask, status);

Any reason we can't use a regmap?

> +}
> +
> +static void bd71837_clk_disable(struct clk_hw *hw)
> +{
> +       int rv;
> +       struct bd71837_clk *c = container_of(hw, struct bd71837_clk, hw);
> +
> +       rv = bd71837_clk_set(hw, 0);
> +       if (rv)
> +               dev_dbg(&c->pdev->dev, "Failed to disable 32K clk (%d)\n", rv);
> +}
> +
> +static int bd71837_clk_enable(struct clk_hw *hw)
> +{
> +       return bd71837_clk_set(hw, 1);
> +}
> +
> +static int bd71837_clk_is_enabled(struct clk_hw *hw)
> +{
> +       struct bd71837_clk *c = container_of(hw, struct bd71837_clk, hw);
> +
> +       return c->mask & bd71837_reg_read(c->mfd, c->reg);

Didn't I ask for local variable for reg_read result?

> +}
> +
> +static unsigned long bd71837_clk_recalc_rate(struct clk_hw *hw,
> +                                            unsigned long parent_rate)
> +{
> +       struct bd71837_clk *c = container_of(hw, struct bd71837_clk, hw);
> +
> +       return c->rate;
> +}
> +
> +static const struct clk_ops bd71837_clk_ops = {
> +       .recalc_rate = &bd71837_clk_recalc_rate,
> +       .prepare = &bd71837_clk_enable,
> +       .unprepare = &bd71837_clk_disable,
> +       .is_prepared = &bd71837_clk_is_enabled,
> +};
> +
> +static int bd71837_clk_probe(struct platform_device *pdev)
> +{
> +       struct bd71837_clk *c;
> +       int rval = -ENOMEM;
> +       struct bd71837 *mfd = dev_get_drvdata(pdev->dev.parent);
> +       struct clk_init_data init = {
> +               .name = "bd71837-32k-out",
> +               .ops = &bd71837_clk_ops,
> +       };
> +
> +       c = devm_kzalloc(&pdev->dev, sizeof(*c), GFP_KERNEL);
> +       if (!c)
> +               goto err_out;
> +
> +       c->reg = BD71837_REG_OUT32K;
> +       c->mask = BD71837_OUT32K_EN;
> +       c->rate = BD71837_CLK_RATE;

The PMIC has an 'XIN' pin that would be the clk input for this chip, and
the output clk, this driver, would specify that xin clock as the parent.
The 'xin' clock would then be listed in DT as a fixed-rate clock. That
way this driver doesn't hardcode the frequency.

> +       c->mfd = mfd;
> +       c->pdev = pdev;
> +
> +       of_property_read_string_index(pdev->dev.parent->of_node,
> +                                             "clock-output-names", 0,
> +                                             &init.name);
> +
> +       c->hw.init = &init;

Do this next to all the other c-> things?

> +
> +       rval = devm_clk_hw_register(&pdev->dev, &c->hw);
> +       if (rval) {
> +               dev_err(&pdev->dev, "failed to register 32K clk");
> +               goto err_out;
> +       }
> +
> +       if (pdev->dev.parent->of_node) {
> +               rval = of_clk_add_hw_provider(pdev->dev.parent->of_node,
> +                                            of_clk_hw_simple_get,
> +                                            &c->hw);
> +               if (rval) {
> +                       dev_err(&pdev->dev, "adding clk provider failed\n");
> +                       goto err_out;

Just return rval instead of goto and then remove err_out label.

> +               }
> +       }
> +
> +       rval = clk_hw_register_clkdev(&c->hw, init.name, NULL);

This leaks a clkdev lookup. Sad we don't have a devm version. Maybe it
should be added. But I really doubt this chip will be used with clkdev
lookups so please remove clkdev until you have a user who needs it.

> +       if (rval) {
> +               dev_err(&pdev->dev, "failed to register clkdev for bd71837");
> +               goto err_clean_provider;
> +       }
> +
> +       platform_set_drvdata(pdev, c);
> +
> +       return 0;
> +
> +err_clean_provider:
> +       of_clk_del_provider(pdev->dev.parent->of_node);
> +err_out:
> +       return rval;
> +}
> +
> +static int bd71837_clk_remove(struct platform_device *pdev)
> +{
> +       if (pdev->dev.parent->of_node)
> +               of_clk_del_provider(pdev->dev.parent->of_node);

Use devm so this can go away. Or is devm not used because the parent
of_node is the provider? That's annoying.

> +       return 0;
> +}
> +

Re: [PATCH v5 4/4] clk: bd71837: Add driver for BD71837 PMIC clock

[Matti Vaittinen]

Hello Stephen,

Thanks again for the review. I'll do new patch which fixes these issues.

On Tue, Jun 12, 2018 at 12:44:11AM -0700, Stephen Boyd wrote:
> Quoting Matti Vaittinen (2018-06-04 06:19:13)
> > +config COMMON_CLK_BD71837
> > +       tristate "Clock driver for ROHM BD71837 PMIC MFD"
> > +       depends on MFD_BD71837
> > +       help
> > +         This driver supports ROHM BD71837 PMIC clock.
> > +
> > +
> 
> Drop one newline above.
Right.

> 
> > +++ b/drivers/clk/clk-bd71837.c
> > +#include <linux/mfd/bd71837.h>
> > +#include <linux/clk-provider.h>
> > +#include <linux/clkdev.h>
> > +
> > +
> 
> Drop one newline above.
Right.

> 
> > +struct bd71837_clk {
> > +       struct clk_hw hw;
> > +       uint8_t reg;
> > +       uint8_t mask;
> 
> Use u8 instead of uint8_t.
I can do that but I am afraid I have missed the reason for this. Why u8
is preferred? I would have assumed the standard uint8_t would be
preferred - can you please educate me as to what is this reason?

> > +       unsigned long rate;
> > +       struct platform_device *pdev;
> > +       struct bd71837 *mfd;
> > +};
> > +
> > +static int bd71837_clk_set(struct clk_hw *hw, int status)
> > +{
> > +       struct bd71837_clk *c = container_of(hw, struct bd71837_clk, hw);
> > +
> > +       return bd71837_update_bits(c->mfd, c->reg, c->mask, status);
> 
> Any reason we can't use a regmap?

Not really. These wrappers might become handy if the next chip in series
requires some quirks for access but at the moment I see no reason for
the wrappers. I could switch to direct regmap calls but that change
should then be done to all subdevices and not just for clk one. This
means I should rework also the already applied regulator part. I have
some other changes pending for regulators (adding support for next chip,
renaming bd71837 to bd718x7 or bd718xx, and few comments from  Andy
Shevchenko and Rob Herring). I would rather switch to direct regmap usage
for all subdevices at the same time. So if it is acceptable to keep the
wrappers for now (and then create own patch set to remove them from all
subdevices and the header) I would like to do so.

> 
> > +}
> > +
> > +static void bd71837_clk_disable(struct clk_hw *hw)
> > +{
> > +       int rv;
> > +       struct bd71837_clk *c = container_of(hw, struct bd71837_clk, hw);
> > +
> > +       rv = bd71837_clk_set(hw, 0);
> > +       if (rv)
> > +               dev_dbg(&c->pdev->dev, "Failed to disable 32K clk (%d)\n", rv);
> > +}
> > +
> > +static int bd71837_clk_enable(struct clk_hw *hw)
> > +{
> > +       return bd71837_clk_set(hw, 1);
> > +}
> > +
> > +static int bd71837_clk_is_enabled(struct clk_hw *hw)
> > +{
> > +       struct bd71837_clk *c = container_of(hw, struct bd71837_clk, hw);
> > +
> > +       return c->mask & bd71837_reg_read(c->mfd, c->reg);
> 
> Didn't I ask for local variable for reg_read result?

Yes! Sorry! And thanks for pointing this out again. It seems I missed
this one.

> 
> > +}
> > +
> > +static unsigned long bd71837_clk_recalc_rate(struct clk_hw *hw,
> > +                                            unsigned long parent_rate)
> > +{
> > +       struct bd71837_clk *c = container_of(hw, struct bd71837_clk, hw);
> > +
> > +       return c->rate;
> > +}
> > +
> > +static const struct clk_ops bd71837_clk_ops = {
> > +       .recalc_rate = &bd71837_clk_recalc_rate,
> > +       .prepare = &bd71837_clk_enable,
> > +       .unprepare = &bd71837_clk_disable,
> > +       .is_prepared = &bd71837_clk_is_enabled,
> > +};
> > +
> > +static int bd71837_clk_probe(struct platform_device *pdev)
> > +{
> > +       struct bd71837_clk *c;
> > +       int rval = -ENOMEM;
> > +       struct bd71837 *mfd = dev_get_drvdata(pdev->dev.parent);
> > +       struct clk_init_data init = {
> > +               .name = "bd71837-32k-out",
> > +               .ops = &bd71837_clk_ops,
> > +       };
> > +
> > +       c = devm_kzalloc(&pdev->dev, sizeof(*c), GFP_KERNEL);
> > +       if (!c)
> > +               goto err_out;
> > +
> > +       c->reg = BD71837_REG_OUT32K;
> > +       c->mask = BD71837_OUT32K_EN;
> > +       c->rate = BD71837_CLK_RATE;
> 
> The PMIC has an 'XIN' pin that would be the clk input for this chip, and
> the output clk, this driver, would specify that xin clock as the parent.
> The 'xin' clock would then be listed in DT as a fixed-rate clock. That
> way this driver doesn't hardcode the frequency.

I see. This makes sense. I need to verify from HW colleagues whether
this chip has internal oscillator or not. I originally thought we have
on-chip oscillator - but as you say, we do have XIN pin in documentation.
So now I am not sure if the test board I have contains oscillator driving
the clk on PMIC - or if the PMIC has internal oscillator. I'll clarify this.

> > +       c->mfd = mfd;
> > +       c->pdev = pdev;
> > +
> > +       of_property_read_string_index(pdev->dev.parent->of_node,
> > +                                             "clock-output-names", 0,
> > +                                             &init.name);
> > +
> > +       c->hw.init = &init;
> 
> Do this next to all the other c-> things?

I can do so. I just like the idea of assigning pointers to objects only
after the objects have been initialized. Eg, in this case I add pointer
to init only after I have filled the init.name (if it is given).

> 
> > +
> > +       rval = devm_clk_hw_register(&pdev->dev, &c->hw);
> > +       if (rval) {
> > +               dev_err(&pdev->dev, "failed to register 32K clk");
> > +               goto err_out;
> > +       }
> > +
> > +       if (pdev->dev.parent->of_node) {
> > +               rval = of_clk_add_hw_provider(pdev->dev.parent->of_node,
> > +                                            of_clk_hw_simple_get,
> > +                                            &c->hw);
> > +               if (rval) {
> > +                       dev_err(&pdev->dev, "adding clk provider failed\n");
> > +                       goto err_out;
> 
> Just return rval instead of goto and then remove err_out label.

Is this 'hard requirement'? I prefer single point of exit from functions
(when easily doable) because I have done so many errors with locks /
resources being reserved when exiting from some error branch. For my
personal taste maintaining the one nice cleanup sequence is easier. But
if this is 'hard requirement' this can of course be changed.

> 
> > +               }
> > +       }
> > +
> > +       rval = clk_hw_register_clkdev(&c->hw, init.name, NULL);
> 
> This leaks a clkdev lookup. Sad we don't have a devm version. Maybe it
> should be added. But I really doubt this chip will be used with clkdev
> lookups so please remove clkdev until you have a user who needs it.

Yep. It leaks. I tried to look for an example where the clkdev was
nicely freed at remove - but I didn't find any. Every example I spotted
did leak the clkdev in same way as this does :( And I agree, devm
variant for freeing would be nice - or freeing routines based on hw.

As the leaked clkdev can cause oops at module unload/reload/use your
suggestion about removing it for now makes sense. I'll drop it.

> 
> > +       if (rval) {
> > +               dev_err(&pdev->dev, "failed to register clkdev for bd71837");
> > +               goto err_clean_provider;
> > +       }
> > +
> > +       platform_set_drvdata(pdev, c);
> > +
> > +       return 0;
> > +
> > +err_clean_provider:
> > +       of_clk_del_provider(pdev->dev.parent->of_node);
> > +err_out:
> > +       return rval;
> > +}
> > +
> > +static int bd71837_clk_remove(struct platform_device *pdev)
> > +{
> > +       if (pdev->dev.parent->of_node)
> > +               of_clk_del_provider(pdev->dev.parent->of_node);
> 
> Use devm so this can go away. Or is devm not used because the parent
> of_node is the provider? That's annoying.

What would be the correct workaround for this?

> 
> > +       return 0;
> > +}
> > +

Br,
    Matti Vaittinen

Re: [PATCH v5 4/4] clk: bd71837: Add driver for BD71837 PMIC clock

[Matti Vaittinen]

On Tue, Jun 12, 2018 at 11:23:54AM +0300, Matti Vaittinen wrote:
> Hello Stephen,
> 
> Thanks again for the review. I'll do new patch which fixes these issues.
> 
> On Tue, Jun 12, 2018 at 12:44:11AM -0700, Stephen Boyd wrote:
> > Quoting Matti Vaittinen (2018-06-04 06:19:13)
> > > +}
> > > +
> > > +static unsigned long bd71837_clk_recalc_rate(struct clk_hw *hw,
> > > +                                            unsigned long parent_rate)
> > > +{
> > > +       struct bd71837_clk *c = container_of(hw, struct bd71837_clk, hw);
> > > +
> > > +       return c->rate;
> > > +}
> > > +
> > > +static const struct clk_ops bd71837_clk_ops = {
> > > +       .recalc_rate = &bd71837_clk_recalc_rate,
> > > +       .prepare = &bd71837_clk_enable,
> > > +       .unprepare = &bd71837_clk_disable,
> > > +       .is_prepared = &bd71837_clk_is_enabled,
> > > +};
> > > +
> > > +static int bd71837_clk_probe(struct platform_device *pdev)
> > > +{
> > > +       struct bd71837_clk *c;
> > > +       int rval = -ENOMEM;
> > > +       struct bd71837 *mfd = dev_get_drvdata(pdev->dev.parent);
> > > +       struct clk_init_data init = {
> > > +               .name = "bd71837-32k-out",
> > > +               .ops = &bd71837_clk_ops,
> > > +       };
> > > +
> > > +       c = devm_kzalloc(&pdev->dev, sizeof(*c), GFP_KERNEL);
> > > +       if (!c)
> > > +               goto err_out;
> > > +
> > > +       c->reg = BD71837_REG_OUT32K;
> > > +       c->mask = BD71837_OUT32K_EN;
> > > +       c->rate = BD71837_CLK_RATE;
> > 
> > The PMIC has an 'XIN' pin that would be the clk input for this chip, and
> > the output clk, this driver, would specify that xin clock as the parent.
> > The 'xin' clock would then be listed in DT as a fixed-rate clock. That
> > way this driver doesn't hardcode the frequency.
> 
> I see. This makes sense. I need to verify from HW colleagues whether
> this chip has internal oscillator or not. I originally thought we have
> on-chip oscillator - but as you say, we do have XIN pin in documentation.
> So now I am not sure if the test board I have contains oscillator driving
> the clk on PMIC - or if the PMIC has internal oscillator. I'll clarify this.

It really turned out that the PMIC just acts as a clock buffer. So I do
as you suggested and add lookup for parent clock to the driver. I
planned to do it so that if no parent is found from DT - then we assume
the 32.768KHz clock (as described in documentation). Eg, something along
the lines:

        init.parent_names = of_clk_get_parent_name(pdev->dev.parent->of_node, 0);
        if (init.parent_names) {
                init.num_parents = 1;
        } else {
                /* If parent is not given from DT we assume the typical use-case with
                 * 32.768 KHz oscillator for RTC (Maybe we could just error out here?)
                 */
                c->rate = BD71837_CLK_RATE;
                bd71837_clk_ops.recalc_rate = &bd71837_clk_recalc_rate;
        }

Does this make sense?

Br,
    Matti Vaittinen 

Re: [PATCH v5 4/4] clk: bd71837: Add driver for BD71837 PMIC clock

[Stephen Boyd]

Quoting Matti Vaittinen (2018-06-13 06:03:38)
> On Tue, Jun 12, 2018 at 11:23:54AM +0300, Matti Vaittinen wrote:
> > 
> > I see. This makes sense. I need to verify from HW colleagues whether
> > this chip has internal oscillator or not. I originally thought we have
> > on-chip oscillator - but as you say, we do have XIN pin in documentation.
> > So now I am not sure if the test board I have contains oscillator driving
> > the clk on PMIC - or if the PMIC has internal oscillator. I'll clarify this.
> 
> It really turned out that the PMIC just acts as a clock buffer. So I do
> as you suggested and add lookup for parent clock to the driver. I
> planned to do it so that if no parent is found from DT - then we assume
> the 32.768KHz clock (as described in documentation). Eg, something along
> the lines:
> 
>         init.parent_names = of_clk_get_parent_name(pdev->dev.parent->of_node, 0);
>         if (init.parent_names) {
>                 init.num_parents = 1;
>         } else {
>                 /* If parent is not given from DT we assume the typical use-case with
>                  * 32.768 KHz oscillator for RTC (Maybe we could just error out here?)
>                  */
>                 c->rate = BD71837_CLK_RATE;
>                 bd71837_clk_ops.recalc_rate = &bd71837_clk_recalc_rate;
>         }

You can also add a clk directly in this driver in that case there isn't
one in DT with the rate and name of your choosing. Then the logic is the
same and we don't need a c->rate variable.

Re: [PATCH v5 4/4] clk: bd71837: Add driver for BD71837 PMIC clock

[Matti Vaittinen]

Hello Stephen,

On Mon, Jun 25, 2018 at 04:46:24PM -0700, Stephen Boyd wrote:
> Quoting Matti Vaittinen (2018-06-13 06:03:38)
> > On Tue, Jun 12, 2018 at 11:23:54AM +0300, Matti Vaittinen wrote:
> > > 
> > > I see. This makes sense. I need to verify from HW colleagues whether
> > > this chip has internal oscillator or not. I originally thought we have
> > > on-chip oscillator - but as you say, we do have XIN pin in documentation.
> > > So now I am not sure if the test board I have contains oscillator driving
> > > the clk on PMIC - or if the PMIC has internal oscillator. I'll clarify this.
> > 
> > It really turned out that the PMIC just acts as a clock buffer. So I do
> > as you suggested and add lookup for parent clock to the driver. I
> > planned to do it so that if no parent is found from DT - then we assume
> > the 32.768KHz clock (as described in documentation). Eg, something along
> > the lines:
> > 
> >         init.parent_names = of_clk_get_parent_name(pdev->dev.parent->of_node, 0);
> >         if (init.parent_names) {
> >                 init.num_parents = 1;
> >         } else {
> >                 /* If parent is not given from DT we assume the typical use-case with
> >                  * 32.768 KHz oscillator for RTC (Maybe we could just error out here?)
> >                  */
> >                 c->rate = BD71837_CLK_RATE;
> >                 bd71837_clk_ops.recalc_rate = &bd71837_clk_recalc_rate;
> >         }
> 
> You can also add a clk directly in this driver in that case there isn't
> one in DT with the rate and name of your choosing. Then the logic is the
> same and we don't need a c->rate variable.

So you mean that I should use clk_hw_register_fixed_rate and create new
clk if parent is not found? Isn't this a bit of an overkill? Downside is
that then we do need remove/cleanup functionality for deleting this
parent clock - and I didn't find devm support for fixed clock. Furthermore
I guess that since it is parent, it can't be removed before child is removed.

Or did you mean something else but creating a fixed rate clock as parent
here?

Best Regards
    Matti Vaittinen

Re: [PATCH v5 4/4] clk: bd71837: Add driver for BD71837 PMIC clock

[Matti Vaittinen]

Hello Again,

On Wed, Jun 27, 2018 at 11:40:00AM +0300, Matti Vaittinen wrote:
> Hello Stephen,
> 
> On Mon, Jun 25, 2018 at 04:46:24PM -0700, Stephen Boyd wrote:
> > Quoting Matti Vaittinen (2018-06-13 06:03:38)
> > > On Tue, Jun 12, 2018 at 11:23:54AM +0300, Matti Vaittinen wrote:
> > > > 
> > > > I see. This makes sense. I need to verify from HW colleagues whether
> > > > this chip has internal oscillator or not. I originally thought we have
> > > > on-chip oscillator - but as you say, we do have XIN pin in documentation.
> > > > So now I am not sure if the test board I have contains oscillator driving
> > > > the clk on PMIC - or if the PMIC has internal oscillator. I'll clarify this.
> > > 
> > > It really turned out that the PMIC just acts as a clock buffer. So I do
> > > as you suggested and add lookup for parent clock to the driver. I
> > > planned to do it so that if no parent is found from DT - then we assume
> > > the 32.768KHz clock (as described in documentation). Eg, something along
> > > the lines:
> > > 
> > >         init.parent_names = of_clk_get_parent_name(pdev->dev.parent->of_node, 0);
> > >         if (init.parent_names) {
> > >                 init.num_parents = 1;
> > >         } else {
> > >                 /* If parent is not given from DT we assume the typical use-case with
> > >                  * 32.768 KHz oscillator for RTC (Maybe we could just error out here?)
> > >                  */
> > >                 c->rate = BD71837_CLK_RATE;
> > >                 bd71837_clk_ops.recalc_rate = &bd71837_clk_recalc_rate;
> > >         }
> > 
> > You can also add a clk directly in this driver in that case there isn't
> > one in DT with the rate and name of your choosing. Then the logic is the
> > same and we don't need a c->rate variable.
> 
> So you mean that I should use clk_hw_register_fixed_rate and create new
> clk if parent is not found? Isn't this a bit of an overkill? Downside is
> that then we do need remove/cleanup functionality for deleting this
> parent clock - and I didn't find devm support for fixed clock. Furthermore
> I guess that since it is parent, it can't be removed before child is removed.
> 
> Or did you mean something else but creating a fixed rate clock as parent
> here?

I would be grateful for any tips on how to proceed. Should I

1. Really create a fixed rate clock - and build cleanup sequence myself
2. Keep this simple and fail if no parent is found using of_clk_get_parent_name

Best regards
    Matti Vaittinen

Re: [PATCH v5 4/4] clk: bd71837: Add driver for BD71837 PMIC clock

[Stephen Boyd]

Quoting Matti Vaittinen (2018-06-12 01:23:54)
> On Tue, Jun 12, 2018 at 12:44:11AM -0700, Stephen Boyd wrote:
> > Quoting Matti Vaittinen (2018-06-04 06:19:13)
> 
> > 
> > > +struct bd71837_clk {
> > > +       struct clk_hw hw;
> > > +       uint8_t reg;
> > > +       uint8_t mask;
> > 
> > Use u8 instead of uint8_t.
> I can do that but I am afraid I have missed the reason for this. Why u8
> is preferred? I would have assumed the standard uint8_t would be
> preferred - can you please educate me as to what is this reason?

This is kernel style to prefer the shorter types within the kernel code.
Outside of the kernel proper, uint*_t types are used in the UAPI
headers. You can look through the mailing list about this, but this is
how I've known it to be for a while now.

> 
> > > +       unsigned long rate;
> > > +       struct platform_device *pdev;
> > > +       struct bd71837 *mfd;
> > > +};
> > > +
> > > +static int bd71837_clk_set(struct clk_hw *hw, int status)
> > > +{
> > > +       struct bd71837_clk *c = container_of(hw, struct bd71837_clk, hw);
> > > +
> > > +       return bd71837_update_bits(c->mfd, c->reg, c->mask, status);
> > 
> > Any reason we can't use a regmap?
> 
> Not really. These wrappers might become handy if the next chip in series
> requires some quirks for access but at the moment I see no reason for
> the wrappers. I could switch to direct regmap calls but that change
> should then be done to all subdevices and not just for clk one. This
> means I should rework also the already applied regulator part. I have
> some other changes pending for regulators (adding support for next chip,
> renaming bd71837 to bd718x7 or bd718xx, and few comments from  Andy
> Shevchenko and Rob Herring). I would rather switch to direct regmap usage
> for all subdevices at the same time. So if it is acceptable to keep the
> wrappers for now (and then create own patch set to remove them from all
> subdevices and the header) I would like to do so.

Ok. Sounds fine.

> 
> > 
> > > +}
> > > +
> > > +static void bd71837_clk_disable(struct clk_hw *hw)
> > > +{
> > > +       int rv;
> > > +       struct bd71837_clk *c = container_of(hw, struct bd71837_clk, hw);
> > > +
> > > +       rv = bd71837_clk_set(hw, 0);
> > > +       if (rv)
> > > +               dev_dbg(&c->pdev->dev, "Failed to disable 32K clk (%d)\n", rv);
> > > +}
> > > +
> > > +static int bd71837_clk_enable(struct clk_hw *hw)
> > > +{
> > > +       return bd71837_clk_set(hw, 1);
> > > +}
> > > +
> > > +static int bd71837_clk_is_enabled(struct clk_hw *hw)
> > > +{
> > > +       struct bd71837_clk *c = container_of(hw, struct bd71837_clk, hw);
> > > +
> > > +       return c->mask & bd71837_reg_read(c->mfd, c->reg);
> > 
> > Didn't I ask for local variable for reg_read result?
> 
> Yes! Sorry! And thanks for pointing this out again. It seems I missed
> this one.

Ok no worries.

> 
> > 
> > > +}
> > > +
> > > +static unsigned long bd71837_clk_recalc_rate(struct clk_hw *hw,
> > > +                                            unsigned long parent_rate)
> > > +{
> > > +       struct bd71837_clk *c = container_of(hw, struct bd71837_clk, hw);
> > > +
> > > +       return c->rate;
> > > +}
> > > +
> > > +static const struct clk_ops bd71837_clk_ops = {
> > > +       .recalc_rate = &bd71837_clk_recalc_rate,
> > > +       .prepare = &bd71837_clk_enable,
> > > +       .unprepare = &bd71837_clk_disable,
> > > +       .is_prepared = &bd71837_clk_is_enabled,
> > > +};
> > > +
> > > +static int bd71837_clk_probe(struct platform_device *pdev)
> > > +{
> > > +       struct bd71837_clk *c;
> > > +       int rval = -ENOMEM;
> > > +       struct bd71837 *mfd = dev_get_drvdata(pdev->dev.parent);
> > > +       struct clk_init_data init = {
> > > +               .name = "bd71837-32k-out",
> > > +               .ops = &bd71837_clk_ops,
> > > +       };
> > > +
> > > +       c = devm_kzalloc(&pdev->dev, sizeof(*c), GFP_KERNEL);
> > > +       if (!c)
> > > +               goto err_out;
> > > +
> > > +       c->reg = BD71837_REG_OUT32K;
> > > +       c->mask = BD71837_OUT32K_EN;
> > > +       c->rate = BD71837_CLK_RATE;
> > 
> > The PMIC has an 'XIN' pin that would be the clk input for this chip, and
> > the output clk, this driver, would specify that xin clock as the parent.
> > The 'xin' clock would then be listed in DT as a fixed-rate clock. That
> > way this driver doesn't hardcode the frequency.
> 
> I see. This makes sense. I need to verify from HW colleagues whether
> this chip has internal oscillator or not. I originally thought we have
> on-chip oscillator - but as you say, we do have XIN pin in documentation.
> So now I am not sure if the test board I have contains oscillator driving
> the clk on PMIC - or if the PMIC has internal oscillator. I'll clarify this.

Alright.

> 
> > > +       c->mfd = mfd;
> > > +       c->pdev = pdev;
> > > +
> > > +       of_property_read_string_index(pdev->dev.parent->of_node,
> > > +                                             "clock-output-names", 0,
> > > +                                             &init.name);
> > > +
> > > +       c->hw.init = &init;
> > 
> > Do this next to all the other c-> things?
> 
> I can do so. I just like the idea of assigning pointers to objects only
> after the objects have been initialized. Eg, in this case I add pointer
> to init only after I have filled the init.name (if it is given).

Alright, sure.

> 
> > 
> > > +
> > > +       rval = devm_clk_hw_register(&pdev->dev, &c->hw);
> > > +       if (rval) {
> > > +               dev_err(&pdev->dev, "failed to register 32K clk");
> > > +               goto err_out;
> > > +       }
> > > +
> > > +       if (pdev->dev.parent->of_node) {
> > > +               rval = of_clk_add_hw_provider(pdev->dev.parent->of_node,
> > > +                                            of_clk_hw_simple_get,
> > > +                                            &c->hw);
> > > +               if (rval) {
> > > +                       dev_err(&pdev->dev, "adding clk provider failed\n");
> > > +                       goto err_out;
> > 
> > Just return rval instead of goto and then remove err_out label.
> 
> Is this 'hard requirement'? I prefer single point of exit from functions
> (when easily doable) because I have done so many errors with locks /
> resources being reserved when exiting from some error branch. For my
> personal taste maintaining the one nice cleanup sequence is easier. But
> if this is 'hard requirement' this can of course be changed.

There are no locks though. And this uses devm. So please remove the goto
so my style alarms don't go off. Thanks!

> 
> > 
> > > +               }
> > > +       }
> > > +
> > > +       rval = clk_hw_register_clkdev(&c->hw, init.name, NULL);
> > 
> > This leaks a clkdev lookup. Sad we don't have a devm version. Maybe it
> > should be added. But I really doubt this chip will be used with clkdev
> > lookups so please remove clkdev until you have a user who needs it.
> 
> Yep. It leaks. I tried to look for an example where the clkdev was
> nicely freed at remove - but I didn't find any. Every example I spotted
> did leak the clkdev in same way as this does :( And I agree, devm
> variant for freeing would be nice - or freeing routines based on hw.
> 
> As the leaked clkdev can cause oops at module unload/reload/use your
> suggestion about removing it for now makes sense. I'll drop it.

Ok! Sometimes drivers don't free them because they're lazy and don't
expect to be unloaded or to fail. I guess you ran into those. I'm happy
to apply patches to clean those up.

> 
> > 
> > > +       if (rval) {
> > > +               dev_err(&pdev->dev, "failed to register clkdev for bd71837");
> > > +               goto err_clean_provider;
> > > +       }
> > > +
> > > +       platform_set_drvdata(pdev, c);
> > > +
> > > +       return 0;
> > > +
> > > +err_clean_provider:
> > > +       of_clk_del_provider(pdev->dev.parent->of_node);
> > > +err_out:
> > > +       return rval;
> > > +}
> > > +
> > > +static int bd71837_clk_remove(struct platform_device *pdev)
> > > +{
> > > +       if (pdev->dev.parent->of_node)
> > > +               of_clk_del_provider(pdev->dev.parent->of_node);
> > 
> > Use devm so this can go away. Or is devm not used because the parent
> > of_node is the provider? That's annoying.
> 
> What would be the correct workaround for this?

Smash the clk driver into the overall PMIC node. That should work. Or
possibly assign the same of_node to the child device when creating it?
I'm not sure if that causes some sort of problem with DT code though, so
it would be good to check with Rob H if that's a bad idea or not.

Re: [PATCH v5 4/4] clk: bd71837: Add driver for BD71837 PMIC clock

[Matti Vaittinen]

Hello Stephen, Rob and all others

Thanks again Stephen. I appreciate your help. And just to help you
sorting your inbox a bit - I have written patch series v6 and v7 before
receiving these comments. I will write v8 after I get some further input
from you and Rob on how to solve the issue with having the clk stuff in
parent node and usage of devm_of_clk_add_hw_provider where DT node is
fetched from dev pointer. 

The v8 should then address rest of the issues - except the regmap wrappers
which I will send as separate patch. So I guess you can skip the v6 and v7
and just please check for the v8 when I get it done.

On Mon, Jun 25, 2018 at 04:44:57PM -0700, Stephen Boyd wrote:
> Quoting Matti Vaittinen (2018-06-12 01:23:54)
> > On Tue, Jun 12, 2018 at 12:44:11AM -0700, Stephen Boyd wrote:
> > > Quoting Matti Vaittinen (2018-06-04 06:19:13)
> > > Use u8 instead of uint8_t.
> > can you please educate me as to what is this reason?
> 
> This is kernel style to prefer the shorter types within the kernel code.
> Outside of the kernel proper, uint*_t types are used in the UAPI
> headers. You can look through the mailing list about this, but this is
> how I've known it to be for a while now.

Thanks. I have changed this from patch v6 onwards.
> > > > +
> > > > +       rval = devm_clk_hw_register(&pdev->dev, &c->hw);
> > > > +       if (rval) {
> > > > +               dev_err(&pdev->dev, "failed to register 32K clk");
> > > > +               goto err_out;
> > > > +       }
> > > > +
> > > > +       if (pdev->dev.parent->of_node) {
> > > > +               rval = of_clk_add_hw_provider(pdev->dev.parent->of_node,
> > > > +                                            of_clk_hw_simple_get,
> > > > +                                            &c->hw);
> > > > +               if (rval) {
> > > > +                       dev_err(&pdev->dev, "adding clk provider failed\n");
> > > > +                       goto err_out;
> > > 
> > > Just return rval instead of goto and then remove err_out label.
> > 
> > Is this 'hard requirement'? I prefer single point of exit from functions
> > (when easily doable) because I have done so many errors with locks /
> > resources being reserved when exiting from some error branch. For my
> > personal taste maintaining the one nice cleanup sequence is easier. But
> > if this is 'hard requirement' this can of course be changed.
> 
> There are no locks though. And this uses devm. So please remove the goto
> so my style alarms don't go off. Thanks!

I did rework this piece from patch v6 onwards so that there is no goto but
we still have single exit point. I hope that is Ok, so please re-check
this when you find the time.

> > > > +       rval = clk_hw_register_clkdev(&c->hw, init.name, NULL);
> > > 
> > > This leaks a clkdev lookup. Sad we don't have a devm version. Maybe it
> > > should be added. But I really doubt this chip will be used with clkdev
> > > lookups so please remove clkdev until you have a user who needs it.
> > 
> > Yep. It leaks. I tried to look for an example where the clkdev was
> > nicely freed at remove - but I didn't find any. Every example I spotted
> > did leak the clkdev in same way as this does :( And I agree, devm
> > variant for freeing would be nice - or freeing routines based on hw.
> > 
> > As the leaked clkdev can cause oops at module unload/reload/use your
> > suggestion about removing it for now makes sense. I'll drop it.
> 
> Ok! Sometimes drivers don't free them because they're lazy and don't
> expect to be unloaded or to fail. I guess you ran into those. I'm happy
> to apply patches to clean those up.

When I get the moment of "hmm, what should I do next" - I'll keep this
in mind... Unfortunately those moments have been pretty rare occasions
since I found my wife and got my kids - but OTOH, we have working
pension scheme in Finland - so I expect this to change during the next
30 years or so =] (Truth is that I'll keep this in mind but can't
promise anything more as promises might be empty).

> > > > +       if (rval) {
> > > > +               dev_err(&pdev->dev, "failed to register clkdev for bd71837");
> > > > +               goto err_clean_provider;
> > > > +       }
> > > > +
> > > > +       platform_set_drvdata(pdev, c);
> > > > +
> > > > +       return 0;
> > > > +
> > > > +err_clean_provider:
> > > > +       of_clk_del_provider(pdev->dev.parent->of_node);
> > > > +err_out:
> > > > +       return rval;
> > > > +}
> > > > +
> > > > +static int bd71837_clk_remove(struct platform_device *pdev)
> > > > +{
> > > > +       if (pdev->dev.parent->of_node)
> > > > +               of_clk_del_provider(pdev->dev.parent->of_node);
> > > 
> > > Use devm so this can go away. Or is devm not used because the parent
> > > of_node is the provider? That's annoying.
> > 
> > What would be the correct workaround for this?
> 
> Smash the clk driver into the overall PMIC node. That should work. Or
> possibly assign the same of_node to the child device when creating it?
> I'm not sure if that causes some sort of problem with DT code though, so
> it would be good to check with Rob H if that's a bad idea or not.

I'd rather keep the clk in own subdevice so that it can be used or not
used based on the clk Kconfig options. I also rather keep the clk codes
in clk folders. So I guess the use of devm is not correct justification
for bundling the MFD and clk. I basically see 3 ways:

1. Assign MFD node to subdevice node in MFD when creating the cells.
2. Assign parent->of_node to dev.of_node in clk subdevice.
3. Create devm_of_clk_add_hw_provider_w_node() which does something
like (not compiled pseudo) code below

int devm_of_clk_add_hw_provider_w_node(struct device *dev,
			struct clk_hw *(*get)(struct of_phandle_args *clkspec,
					      void *data),
			struct device_node *of_node,
			void *data)
{
	struct device_node **ptr;
	int ret;
	ptr = devres_alloc(devm_of_clk_release_provider, sizeof(*ptr),
			   GFP_KERNEL);
	if (!ptr)
		return -ENOMEM;

	*ptr = of_node;
	ret = of_clk_add_hw_provider(of_node, get, data);
	if (!ret)
		devres_add(dev, ptr);
	else
		devres_free(ptr);

	return ret;
}
EXPORT_SYMBOL_GPL(devm_of_clk_add_hw_provider_w_node);

int devm_of_clk_add_hw_provider(struct device *dev,
			struct clk_hw *(*get)(struct of_phandle_args *clkspec,
					      void *data),
			void *data)
{
	return devm_of_clk_add_hw_provider_w_node(dev, get, dev->of_node,
						  data);
}
EXPORT_SYMBOL_GPL(devm_of_clk_add_hw_provider);

To me the third option sounds like correct one - but you guys probably
have the idea how these subsystems should look like in the future - so I
trust on your judgement on this. So whatäs your take on this?

I'll also add Rob in the 'to' field of this email so maybe we get his opinion
on this.

Best Regards
    Matti Vaittinen

Re: [PATCH v5 4/4] clk: bd71837: Add driver for BD71837 PMIC clock

[Matti Vaittinen]

On Tue, Jun 26, 2018 at 11:13:19AM +0300, Matti Vaittinen wrote:
> On Mon, Jun 25, 2018 at 04:44:57PM -0700, Stephen Boyd wrote:
> > Quoting Matti Vaittinen (2018-06-12 01:23:54)
> > > On Tue, Jun 12, 2018 at 12:44:11AM -0700, Stephen Boyd wrote:
> > > > Quoting Matti Vaittinen (2018-06-04 06:19:13)

[snip]

> > > > > +       if (rval) {
> > > > > +               dev_err(&pdev->dev, "failed to register clkdev for bd71837");
> > > > > +               goto err_clean_provider;
> > > > > +       }
> > > > > +
> > > > > +       platform_set_drvdata(pdev, c);
> > > > > +
> > > > > +       return 0;
> > > > > +
> > > > > +err_clean_provider:
> > > > > +       of_clk_del_provider(pdev->dev.parent->of_node);
> > > > > +err_out:
> > > > > +       return rval;
> > > > > +}
> > > > > +
> > > > > +static int bd71837_clk_remove(struct platform_device *pdev)
> > > > > +{
> > > > > +       if (pdev->dev.parent->of_node)
> > > > > +               of_clk_del_provider(pdev->dev.parent->of_node);
> > > > 
> > > > Use devm so this can go away. Or is devm not used because the parent
> > > > of_node is the provider? That's annoying.
> > > 
> > > What would be the correct workaround for this?
> > 
> > Smash the clk driver into the overall PMIC node. That should work. Or
> > possibly assign the same of_node to the child device when creating it?
> > I'm not sure if that causes some sort of problem with DT code though, so
> > it would be good to check with Rob H if that's a bad idea or not.
> 
> 1. Assign MFD node to subdevice node in MFD when creating the cells.
> 2. Assign parent->of_node to dev.of_node in clk subdevice.
> 3. Create devm_of_clk_add_hw_provider_w_node() which does something
> like (not compiled pseudo) code below
> 
> int devm_of_clk_add_hw_provider_w_node(struct device *dev,
> 			struct clk_hw *(*get)(struct of_phandle_args *clkspec,
> 					      void *data),
> 			struct device_node *of_node,
> 			void *data)
> {
> 	struct device_node **ptr;
> 	int ret;
> 	ptr = devres_alloc(devm_of_clk_release_provider, sizeof(*ptr),
> 			   GFP_KERNEL);
> 	if (!ptr)
> 		return -ENOMEM;
> 
> 	*ptr = of_node;
> 	ret = of_clk_add_hw_provider(of_node, get, data);
> 	if (!ret)
> 		devres_add(dev, ptr);
> 	else
> 		devres_free(ptr);
> 
> 	return ret;
> }
> EXPORT_SYMBOL_GPL(devm_of_clk_add_hw_provider_w_node);
> 
> int devm_of_clk_add_hw_provider(struct device *dev,
> 			struct clk_hw *(*get)(struct of_phandle_args *clkspec,
> 					      void *data),
> 			void *data)
> {
> 	return devm_of_clk_add_hw_provider_w_node(dev, get, dev->of_node,
> 						  data);
> }
> EXPORT_SYMBOL_GPL(devm_of_clk_add_hw_provider);

just a friendly reminder, what's your opinion on adding this kind of
function (devm_of_clk_add_hw_provider_w_node)? or solutions 1/2? And are these options safe what comes to
reference counting of of_nodes?

Best regards
	Matti Vaittinen

Re: [PATCH v5 4/4] clk: bd71837: Add driver for BD71837 PMIC clock

[Matti Vaittinen]

On Tue, Jul 31, 2018 at 11:28:58AM +0300, Matti Vaittinen wrote:
> On Tue, Jun 26, 2018 at 11:13:19AM +0300, Matti Vaittinen wrote:
> > On Mon, Jun 25, 2018 at 04:44:57PM -0700, Stephen Boyd wrote:
> > > Quoting Matti Vaittinen (2018-06-12 01:23:54)
> > > > On Tue, Jun 12, 2018 at 12:44:11AM -0700, Stephen Boyd wrote:
> > > > > Quoting Matti Vaittinen (2018-06-04 06:19:13)

[snip]

> 
> > 3. Create devm_of_clk_add_hw_provider_w_node() which does something
> 

After giving this second thought - I think there is limited amount of
use cases where other than own or parent nodes should be used. Actually,
the MFD node being parent is pretty much only use case I can think of
where something else but own node should be used. Hence function like
suggested devm_of_clk_add_hw_provider_w_node might invite thinking of
clever hacks... So, perhaps introducing
devm_of_clk_add_hw_provider_parent() (see idea below) would be option to
consider? I feel the bd71837 driver is not only case where MFD is being
parent which has the clock stuff in DT.

static int __devm_of_clk_add_hw_provider(struct device *dev,
	struct clk_hw *(*get)(struct of_phandle_args *clkspec, void *data),
	struct device_node *of_node, void *data)
{
	struct device_node **ptr;
	int ret;
	ptr = devres_alloc(devm_of_clk_release_provider, sizeof(*ptr),
			   GFP_KERNEL);
	if (!ptr)
		return -ENOMEM;

	*ptr = of_node;
	ret = of_clk_add_hw_provider(of_node, get, data);
	if (!ret)
		devres_add(dev, ptr);
	else
		devres_free(ptr);
	return ret;
}

int devm_of_clk_add_hw_provider(struct device *dev,
	struct clk_hw *(*get)(struct of_phandle_args *clkspec, void *data),
	void *data)
{
	return __devm_of_clk_add_hw_provider(dev, get, dev->of_node, data);
}
EXPORT_SYMBOL_GPL(devm_of_clk_add_hw_provider);

int devm_of_clk_add_hw_provider_parent(struct device *dev,
	struct clk_hw *(*get)(struct of_phandle_args *clkspec, void *data),
	void *data)
{
	return __devm_of_clk_add_hw_provider(*dev, get, dev->parent->of_node,
					     data);
}
EXPORT_SYMBOL_GPL(devm_of_clk_add_hw_provider_parent);

> just a friendly reminder, what's your opinion on adding this kind of
> function (devm_of_clk_add_hw_provider_w_node)? or solutions 1/2? And are
> these options safe what comes to reference counting of of_nodes?

I thik the reference counting should not be a problem when use is
limited to (MFD) parent device nodes, right?

Best regards
    Matti Vaittinen