[PATCH v3 18/23] regulator: add s2dos05 regulator support

Dzmitry Sankouski posted 23 patches 1 year, 7 months ago
There is a newer version of this series
[PATCH v3 18/23] regulator: add s2dos05 regulator support
Posted by Dzmitry Sankouski 1 year, 7 months ago
S2dos05 has 1 buck and 4 LDO regulators, used for powering
panel/touchscreen.

Signed-off-by: Dzmitry Sankouski <dsankouski@gmail.com>
---
 MAINTAINERS                           |   1 +
 drivers/regulator/Kconfig             |   8 +
 drivers/regulator/Makefile            |   1 +
 drivers/regulator/s2dos05-regulator.c | 362 ++++++++++++++++++++++++++++++++++
 4 files changed, 372 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index b53462684a30..bee700a5e648 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -19910,6 +19910,7 @@ F:	Documentation/devicetree/bindings/regulator/samsung,s5m*.yaml
 F:	drivers/clk/clk-s2mps11.c
 F:	drivers/mfd/s2dos*.c
 F:	drivers/mfd/sec*.c
+F:	drivers/regulator/s2dos*.c
 F:	drivers/regulator/s2m*.c
 F:	drivers/regulator/s5m*.c
 F:	drivers/rtc/rtc-s5m.c
diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index d333be2bea3b..d6d6f571a65d 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -1297,6 +1297,14 @@ config REGULATOR_RTQ2208
 	  and two ldos. It features wide output voltage range from 0.4V to 2.05V
 	  and the capability to configure the corresponding power stages.
 
+config REGULATOR_S2DOS05
+	tristate "SLSI S2DOS05 regulator"
+	depends on MFD_S2DOS_CORE || COMPILE_TEST
+	help
+	  This driver provides support for the voltage regulators of the S2DOS05.
+	  The S2DOS05 is a companion power management IC for the smart phones.
+	  The S2DOS05 has 4 LDOs and 1 BUCK outputs.
+
 config REGULATOR_S2MPA01
 	tristate "Samsung S2MPA01 voltage regulator"
 	depends on MFD_SEC_CORE || COMPILE_TEST
diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index ba15fa5f30ad..80f889404597 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -151,6 +151,7 @@ obj-$(CONFIG_REGULATOR_RTMV20)	+= rtmv20-regulator.o
 obj-$(CONFIG_REGULATOR_RTQ2134) += rtq2134-regulator.o
 obj-$(CONFIG_REGULATOR_RTQ6752)	+= rtq6752-regulator.o
 obj-$(CONFIG_REGULATOR_RTQ2208) += rtq2208-regulator.o
+obj-$(CONFIG_REGULATOR_S2DOS05) += s2dos05-regulator.o
 obj-$(CONFIG_REGULATOR_S2MPA01) += s2mpa01.o
 obj-$(CONFIG_REGULATOR_S2MPS11) += s2mps11.o
 obj-$(CONFIG_REGULATOR_S5M8767) += s5m8767.o
diff --git a/drivers/regulator/s2dos05-regulator.c b/drivers/regulator/s2dos05-regulator.c
new file mode 100644
index 000000000000..3c58a1bd2262
--- /dev/null
+++ b/drivers/regulator/s2dos05-regulator.c
@@ -0,0 +1,362 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * s2dos05.c - Regulator driver for the Samsung s2dos05
+ *
+ * Copyright (C) 2016 Samsung Electronics
+ * Copyright (C) 2023 Dzmitry Sankouski <dsankouski@gmail.com>
+ *
+ */
+
+#include <linux/module.h>
+#include <linux/bug.h>
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/slab.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+#include <linux/interrupt.h>
+#include <linux/platform_device.h>
+#include <linux/regulator/driver.h>
+#include <linux/regulator/machine.h>
+#include <linux/regulator/of_regulator.h>
+#include <linux/mfd/samsung/s2dos-core.h>
+#include <linux/mfd/samsung/s2dos05.h>
+#include <linux/i2c.h>
+#include <linux/debugfs.h>
+
+struct s2dos05_data {
+	struct regmap *regmap;
+	struct device *dev;
+};
+
+static int s2m_enable(struct regulator_dev *rdev)
+{
+	struct s2dos05_data *info = rdev_get_drvdata(rdev);
+	struct regmap *regmap = info->regmap;
+
+	return regmap_update_bits(regmap, rdev->desc->enable_reg,
+				  rdev->desc->enable_mask,
+					rdev->desc->enable_mask);
+}
+
+static int s2m_disable_regmap(struct regulator_dev *rdev)
+{
+	struct s2dos05_data *info = rdev_get_drvdata(rdev);
+	struct regmap *regmap = info->regmap;
+	u8 val;
+
+	if (rdev->desc->enable_is_inverted)
+		val = rdev->desc->enable_mask;
+	else
+		val = 0;
+
+	return regmap_update_bits(regmap, rdev->desc->enable_reg, rdev->desc->enable_mask,
+				   val);
+}
+
+static int s2m_is_enabled_regmap(struct regulator_dev *rdev)
+{
+	struct s2dos05_data *info = rdev_get_drvdata(rdev);
+	struct regmap *regmap = info->regmap;
+	int ret;
+	unsigned int val;
+
+	ret = regmap_read(regmap, rdev->desc->enable_reg, &val);
+	if (ret < 0)
+		return ret;
+
+	if (rdev->desc->enable_is_inverted)
+		return (val & rdev->desc->enable_mask) == 0;
+	else
+		return (val & rdev->desc->enable_mask) != 0;
+}
+
+static int s2m_get_voltage_sel_regmap(struct regulator_dev *rdev)
+{
+	struct s2dos05_data *info = rdev_get_drvdata(rdev);
+	struct regmap *regmap = info->regmap;
+	int ret;
+	unsigned int val;
+
+	ret = regmap_read(regmap, rdev->desc->vsel_reg, &val);
+	if (ret < 0)
+		return ret;
+
+	val &= rdev->desc->vsel_mask;
+
+	return val;
+}
+
+static int s2m_set_voltage_sel_regmap(struct regulator_dev *rdev,
+					unsigned int sel)
+{
+	struct s2dos05_data *info = rdev_get_drvdata(rdev);
+	struct regmap *regmap = info->regmap;
+	int ret;
+
+	ret = regmap_update_bits(regmap, rdev->desc->vsel_reg, rdev->desc->vsel_mask,
+				sel);
+	if (ret < 0)
+		goto out;
+
+	if (rdev->desc->apply_bit)
+		ret = regmap_update_bits(regmap, rdev->desc->apply_reg,
+					 rdev->desc->apply_bit,
+					 rdev->desc->apply_bit);
+	return ret;
+out:
+	pr_warn("%s: failed to set voltage_sel_regmap\n", rdev->desc->name);
+	return ret;
+}
+
+static int s2m_set_voltage_sel_regmap_buck(struct regulator_dev *rdev,
+					unsigned int sel)
+{
+	struct s2dos05_data *info = rdev_get_drvdata(rdev);
+	struct regmap *regmap = info->regmap;
+	int ret;
+
+	ret = regmap_write(regmap, rdev->desc->vsel_reg, sel);
+	if (ret < 0)
+		goto out;
+
+	if (rdev->desc->apply_bit)
+		ret = regmap_update_bits(regmap, rdev->desc->apply_reg,
+					 rdev->desc->apply_bit,
+					 rdev->desc->apply_bit);
+	return ret;
+out:
+	pr_warn("%s: failed to set voltage_sel_regmap\n", rdev->desc->name);
+	return ret;
+}
+
+static int s2m_set_voltage_time_sel(struct regulator_dev *rdev,
+				   unsigned int old_selector,
+				   unsigned int new_selector)
+{
+	int old_volt, new_volt;
+
+	/* sanity check */
+	if (!rdev->desc->ops->list_voltage)
+		return -EINVAL;
+
+	old_volt = rdev->desc->ops->list_voltage(rdev, old_selector);
+	new_volt = rdev->desc->ops->list_voltage(rdev, new_selector);
+
+	if (old_selector < new_selector)
+		return DIV_ROUND_UP(new_volt - old_volt, S2DOS05_RAMP_DELAY);
+
+	return 0;
+}
+
+static int s2m_set_active_discharge(struct regulator_dev *rdev,
+					bool enable)
+{
+	struct s2dos05_data *info = rdev_get_drvdata(rdev);
+	struct regmap *regmap = info->regmap;
+	int ret;
+	u8 val;
+
+	if (enable)
+		val = rdev->desc->active_discharge_on;
+	else
+		val = rdev->desc->active_discharge_off;
+
+	ret = regmap_update_bits(regmap, rdev->desc->active_discharge_reg,
+				rdev->desc->active_discharge_mask, val);
+	return ret;
+}
+
+static const struct regulator_ops s2dos05_ldo_ops = {
+	.list_voltage		= regulator_list_voltage_linear,
+	.map_voltage		= regulator_map_voltage_linear,
+	.is_enabled		= s2m_is_enabled_regmap,
+	.enable			= s2m_enable,
+	.disable		= s2m_disable_regmap,
+	.get_voltage_sel	= s2m_get_voltage_sel_regmap,
+	.set_voltage_sel	= s2m_set_voltage_sel_regmap,
+	.set_voltage_time_sel	= s2m_set_voltage_time_sel,
+	.set_active_discharge	= s2m_set_active_discharge,
+};
+
+static const struct regulator_ops s2dos05_buck_ops = {
+	.list_voltage		= regulator_list_voltage_linear,
+	.map_voltage		= regulator_map_voltage_linear,
+	.is_enabled		= s2m_is_enabled_regmap,
+	.enable			= s2m_enable,
+	.disable		= s2m_disable_regmap,
+	.get_voltage_sel	= s2m_get_voltage_sel_regmap,
+	.set_voltage_sel	= s2m_set_voltage_sel_regmap_buck,
+	.set_voltage_time_sel	= s2m_set_voltage_time_sel,
+	.set_active_discharge	= s2m_set_active_discharge,
+};
+
+#define _BUCK(macro)	S2DOS05_BUCK##macro
+#define _buck_ops(num)	s2dos05_buck_ops##num
+
+#define _LDO(macro)	S2DOS05_LDO##macro
+#define _REG(ctrl)	S2DOS05_REG##ctrl
+#define _ldo_ops(num)	s2dos05_ldo_ops##num
+#define _MASK(macro)	S2DOS05_ENABLE_MASK##macro
+#define _TIME(macro)	S2DOS05_ENABLE_TIME##macro
+
+#define BUCK_DESC(_name, _id, _ops, m, s, v, e, em, t, a) {	\
+	.name		= _name,				\
+	.id		= _id,					\
+	.ops		= _ops,					\
+	.type		= REGULATOR_VOLTAGE,			\
+	.owner		= THIS_MODULE,				\
+	.min_uV		= m,					\
+	.uV_step	= s,					\
+	.n_voltages	= S2DOS05_BUCK_N_VOLTAGES,		\
+	.vsel_reg	= v,					\
+	.vsel_mask	= S2DOS05_BUCK_VSEL_MASK,		\
+	.enable_reg	= e,					\
+	.enable_mask	= em,					\
+	.enable_time	= t,					\
+	.active_discharge_off = 0,				\
+	.active_discharge_on = S2DOS05_BUCK_FD_MASK,		\
+	.active_discharge_reg	= a,				\
+	.active_discharge_mask	= S2DOS05_BUCK_FD_MASK		\
+}
+
+#define LDO_DESC(_name, _id, _ops, m, s, v, e, em, t, a) {	\
+	.name		= _name,				\
+	.id		= _id,					\
+	.ops		= _ops,					\
+	.type		= REGULATOR_VOLTAGE,			\
+	.owner		= THIS_MODULE,				\
+	.min_uV		= m,					\
+	.uV_step	= s,					\
+	.n_voltages	= S2DOS05_LDO_N_VOLTAGES,		\
+	.vsel_reg	= v,					\
+	.vsel_mask	= S2DOS05_LDO_VSEL_MASK,		\
+	.enable_reg	= e,					\
+	.enable_mask	= em,					\
+	.enable_time	= t,					\
+	.active_discharge_off = 0,				\
+	.active_discharge_on = S2DOS05_LDO_FD_MASK,		\
+	.active_discharge_reg	= a,				\
+	.active_discharge_mask	= S2DOS05_LDO_FD_MASK		\
+}
+
+static struct regulator_desc regulators[S2DOS05_REGULATOR_MAX] = {
+		/* name, id, ops, min_uv, uV_step, vsel_reg, enable_reg */
+		LDO_DESC("ldo1", _LDO(1), &_ldo_ops(), _LDO(_MIN1),
+			_LDO(_STEP1), _REG(_LDO1_CFG),
+			_REG(_EN), _MASK(_L1), _TIME(_LDO), _REG(_LDO1_CFG)),
+		LDO_DESC("ldo2", _LDO(2), &_ldo_ops(), _LDO(_MIN1),
+			_LDO(_STEP1), _REG(_LDO2_CFG),
+			_REG(_EN), _MASK(_L2), _TIME(_LDO), _REG(_LDO2_CFG)),
+		LDO_DESC("ldo3", _LDO(3), &_ldo_ops(), _LDO(_MIN2),
+			_LDO(_STEP1), _REG(_LDO3_CFG),
+			_REG(_EN), _MASK(_L3), _TIME(_LDO), _REG(_LDO3_CFG)),
+		LDO_DESC("ldo4", _LDO(4), &_ldo_ops(), _LDO(_MIN2),
+			_LDO(_STEP1), _REG(_LDO4_CFG),
+			_REG(_EN), _MASK(_L4), _TIME(_LDO), _REG(_LDO4_CFG)),
+		BUCK_DESC("buck1", _BUCK(1), &_buck_ops(), _BUCK(_MIN1),
+			_BUCK(_STEP1), _REG(_BUCK_VOUT),
+			_REG(_EN), _MASK(_B1), _TIME(_BUCK), _REG(_BUCK_CFG)),
+};
+
+static int s2dos05_pmic_dt_parse_pdata(struct device *dev,
+					struct of_regulator_match *rdata,
+					unsigned int rdev_num)
+{
+	struct device_node *reg_np;
+	int err;
+
+	reg_np = of_get_child_by_name(dev->parent->of_node, "regulators");
+	if (!reg_np) {
+		dev_err(dev, "could not find regulators sub-node\n");
+		return -EINVAL;
+	}
+
+	err = of_regulator_match(dev, reg_np, rdata, rdev_num);
+	of_node_put(reg_np);
+
+	return err;
+}
+
+static int s2dos05_pmic_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct s2dos_core *iodev = dev_get_drvdata(pdev->dev.parent);
+	struct of_regulator_match *rdata = NULL;
+	struct s2dos05_data *s2dos05;
+	struct regulator_config config = { };
+	unsigned int rdev_num = ARRAY_SIZE(regulators);
+	int i;
+	int ret, err = 0;
+
+	s2dos05 = devm_kzalloc(dev, sizeof(struct s2dos05_data),
+				GFP_KERNEL);
+	if (!s2dos05) {
+		ret = -ENOMEM;
+		goto err_data;
+	}
+	platform_set_drvdata(pdev, s2dos05);
+
+	rdata = kcalloc(rdev_num, sizeof(*rdata), GFP_KERNEL);
+	if (!rdata)
+		return -ENOMEM;
+
+	for (i = 0; i < rdev_num; i++)
+		rdata[i].name = regulators[i].name;
+
+	err = s2dos05_pmic_dt_parse_pdata(dev, rdata, rdev_num);
+	if (err < 0) {
+		dev_err(dev, "Failed to parse regulators device of_node\n");
+		goto err_data;
+	}
+
+	s2dos05->regmap = iodev->regmap;
+	s2dos05->dev = dev;
+
+	for (i = 0; i < rdev_num; i++) {
+		struct regulator_dev *regulator;
+
+		config.init_data = rdata[i].init_data;
+		config.of_node = rdata[i].of_node;
+		config.dev = dev;
+		config.driver_data = s2dos05;
+		regulator = devm_regulator_register(&pdev->dev,
+						&regulators[i], &config);
+		if (IS_ERR(regulator)) {
+			ret = PTR_ERR(regulator);
+			dev_err(&pdev->dev, "regulator init failed for %d\n",
+				i);
+			goto out;
+		}
+	}
+
+out:
+	kfree(rdata);
+
+	return ret;
+
+err_data:
+	devm_kfree(dev, (void *)s2dos05);
+	kfree(s2dos05);
+
+	return ret;
+}
+
+static const struct platform_device_id s2dos05_pmic_id[] = {
+	{ "s2dos05-regulator" },
+	{ },
+};
+MODULE_DEVICE_TABLE(platform, s2dos05_pmic_id);
+
+static struct platform_driver s2dos05_platform_driver = {
+	.driver = {
+		.name = "s2dos05",
+	},
+	.probe = s2dos05_pmic_probe,
+	.id_table = s2dos05_pmic_id,
+};
+module_platform_driver(s2dos05_platform_driver);
+
+MODULE_AUTHOR("Dzmitry Sankouski <dsankouski@gmail.com>");
+MODULE_DESCRIPTION("SAMSUNG s2dos05 Regulator Driver");
+MODULE_LICENSE("GPL");

-- 
2.39.2
Re: [PATCH v3 18/23] regulator: add s2dos05 regulator support
Posted by Krzysztof Kozlowski 1 year, 7 months ago
On 18/06/2024 15:59, Dzmitry Sankouski wrote:
> S2dos05 has 1 buck and 4 LDO regulators, used for powering
> panel/touchscreen.
> 
> Signed-off-by: Dzmitry Sankouski <dsankouski@gmail.com>


> diff --git a/drivers/regulator/s2dos05-regulator.c b/drivers/regulator/s2dos05-regulator.c
> new file mode 100644
> index 000000000000..3c58a1bd2262
> --- /dev/null
> +++ b/drivers/regulator/s2dos05-regulator.c
> @@ -0,0 +1,362 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * s2dos05.c - Regulator driver for the Samsung s2dos05
> + *
> + * Copyright (C) 2016 Samsung Electronics

Not happy. You upstream old issues. :( Please drop all junk Samsung code.

> + * Copyright (C) 2023 Dzmitry Sankouski <dsankouski@gmail.com>
> + *
> + */

...

> +
> +	return ret;
> +
> +err_data:
> +	devm_kfree(dev, (void *)s2dos05);

Why?

> +	kfree(s2dos05);

Please test this first. This is obviously wrong and having such trivial
issue makes me question what else is in this Samsung code.


Best regards,
Krzysztof
Re: [PATCH v3 18/23] regulator: add s2dos05 regulator support
Posted by kernel test robot 1 year, 7 months ago
Hi Dzmitry,

kernel test robot noticed the following build warnings:

[auto build test WARNING on 6906a84c482f098d31486df8dc98cead21cce2d0]

url:    https://github.com/intel-lab-lkp/linux/commits/Dzmitry-Sankouski/power-supply-add-undervoltage-health-status-property/20240618-222456
base:   6906a84c482f098d31486df8dc98cead21cce2d0
patch link:    https://lore.kernel.org/r/20240618-starqltechn_integration_upstream-v3-18-e3f6662017ac%40gmail.com
patch subject: [PATCH v3 18/23] regulator: add s2dos05 regulator support
:::::: branch date: 18 hours ago
:::::: commit date: 18 hours ago
compiler: clang version 18.1.5 (https://github.com/llvm/llvm-project 617a15a9eac96088ae5e9134248d8236e34b91b1)

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/r/202406191516.nzayiXgL-lkp@intel.com/

includecheck warnings: (new ones prefixed by >>)
>> drivers/regulator/s2dos05-regulator.c: linux/module.h is included more than once.

vim +10 drivers/regulator/s2dos05-regulator.c

  > 10	#include <linux/module.h>
    11	#include <linux/bug.h>
    12	#include <linux/delay.h>
    13	#include <linux/err.h>
    14	#include <linux/slab.h>
  > 15	#include <linux/module.h>
    16	#include <linux/regmap.h>
    17	#include <linux/interrupt.h>
    18	#include <linux/platform_device.h>
    19	#include <linux/regulator/driver.h>
    20	#include <linux/regulator/machine.h>
    21	#include <linux/regulator/of_regulator.h>
    22	#include <linux/mfd/samsung/s2dos-core.h>
    23	#include <linux/mfd/samsung/s2dos05.h>
    24	#include <linux/i2c.h>
    25	#include <linux/debugfs.h>
    26	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Re: [PATCH v3 18/23] regulator: add s2dos05 regulator support
Posted by Mark Brown 1 year, 7 months ago
On Tue, Jun 18, 2024 at 04:59:52PM +0300, Dzmitry Sankouski wrote:

> index 000000000000..3c58a1bd2262
> --- /dev/null
> +++ b/drivers/regulator/s2dos05-regulator.c
> @@ -0,0 +1,362 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * s2dos05.c - Regulator driver for the Samsung s2dos05
> + *

Please make the entire comment a C++ one so things look more
intentional.

> +static int s2m_enable(struct regulator_dev *rdev)
> +{
> +	struct s2dos05_data *info = rdev_get_drvdata(rdev);
> +	struct regmap *regmap = info->regmap;
> +
> +	return regmap_update_bits(regmap, rdev->desc->enable_reg,
> +				  rdev->desc->enable_mask,
> +					rdev->desc->enable_mask);
> +}

Please use the generic regmap helpers rather than open coding them.

> +	reg_np = of_get_child_by_name(dev->parent->of_node, "regulators");
> +	if (!reg_np) {
> +		dev_err(dev, "could not find regulators sub-node\n");
> +		return -EINVAL;
> +	}
> +
> +	err = of_regulator_match(dev, reg_np, rdata, rdev_num);
> +	of_node_put(reg_np);

Use of_match for this rather than open coding.

> +	s2dos05 = devm_kzalloc(dev, sizeof(struct s2dos05_data),
> +				GFP_KERNEL);

> +	rdata = kcalloc(rdev_num, sizeof(*rdata), GFP_KERNEL);
> +	if (!rdata)
> +		return -ENOMEM;

Mixing devm_ and regular allocations seems likely to go wrong, please be
consistent.
Re: [PATCH v3 18/23] regulator: add s2dos05 regulator support
Posted by Dzmitry Sankouski 1 year, 7 months ago
вт, 18 июн. 2024 г. в 17:08, Mark Brown <broonie@kernel.org>:
>
> On Tue, Jun 18, 2024 at 04:59:52PM +0300, Dzmitry Sankouski wrote:
>
> > index 000000000000..3c58a1bd2262
> > --- /dev/null
> > +++ b/drivers/regulator/s2dos05-regulator.c
> > @@ -0,0 +1,362 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * s2dos05.c - Regulator driver for the Samsung s2dos05
> > + *
>
> Please make the entire comment a C++ one so things look more
> intentional.
>
Do you mean enclosing the first line (license identifier) in /* */
style comment?
Re: [PATCH v3 18/23] regulator: add s2dos05 regulator support
Posted by Mark Brown 1 year, 7 months ago
On Wed, Jun 19, 2024 at 06:49:06PM +0300, Dzmitry Sankouski wrote:
> вт, 18 июн. 2024 г. в 17:08, Mark Brown <broonie@kernel.org>:
> > On Tue, Jun 18, 2024 at 04:59:52PM +0300, Dzmitry Sankouski wrote:

> > > +// SPDX-License-Identifier: GPL-2.0+
> > > +/*
> > > + * s2dos05.c - Regulator driver for the Samsung s2dos05
> > > + *

> > Please make the entire comment a C++ one so things look more
> > intentional.

> Do you mean enclosing the first line (license identifier) in /* */
> style comment?

No, that would be a C comment.  Please use C++ style for the rest of the
header as well as the first line.