[PATCH v3 2/2] regulator: Add support for MAX77686.

[PATCH v3 2/2] regulator: Add support for MAX77686.

[yadi.brar01]

From: Yadwinder Singh Brar <yadi.brar@samsung.com>

Add support for PMIC/regulator portion of MAX77686 multifunction device.
MAX77686 provides LDOs[1-26] and BUCKs[1-9]. This is initial release of driv
which supports setting and getting the voltage of a regulator with I2C
interface.

Signed-off-by: Yadwinder Singh Brar <yadi.brar@samsung.com>
---
 drivers/regulator/Kconfig    |    9 +
 drivers/regulator/Makefile   |    1 +
 drivers/regulator/max77686.c |  387 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 397 insertions(+), 0 deletions(-)
 create mode 100644 drivers/regulator/max77686.c

diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index c86b886..e8f9417 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -195,6 +195,15 @@ config REGULATOR_MAX8998
 	  via I2C bus. The provided regulator is suitable for S3C6410
 	  and S5PC1XX chips to control VCC_CORE and VCC_USIM voltages.
 
+config REGULATOR_MAX77686
+	tristate "Maxim 77686 regulator"
+	depends on MFD_MAX77686
+	help
+	  This driver controls a Maxim 77686 voltage regulator via I2C
+	  bus. The provided regulator is suitable for Exynos5 chips to
+	  control VDD_ARM and VDD_INT voltages.It supports LDOs[1-26]
+	  and BUCKs[1-9].
+
 config REGULATOR_PCAP
 	tristate "Motorola PCAP2 regulator driver"
 	depends on EZX_PCAP
diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index 977fd46..d854453 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -30,6 +30,7 @@ obj-$(CONFIG_REGULATOR_MAX8925) += max8925-regulator.o
 obj-$(CONFIG_REGULATOR_MAX8952) += max8952.o
 obj-$(CONFIG_REGULATOR_MAX8997) += max8997.o
 obj-$(CONFIG_REGULATOR_MAX8998) += max8998.o
+obj-$(CONFIG_REGULATOR_MAX77686) += max77686.o
 obj-$(CONFIG_REGULATOR_MC13783) += mc13783-regulator.o
 obj-$(CONFIG_REGULATOR_MC13892) += mc13892-regulator.o
 obj-$(CONFIG_REGULATOR_MC13XXX_CORE) +=  mc13xxx-regulator-core.o
diff --git a/drivers/regulator/max77686.c b/drivers/regulator/max77686.c
new file mode 100644
index 0000000..98dbd50
--- /dev/null
+++ b/drivers/regulator/max77686.c
@@ -0,0 +1,387 @@
+/*
+ * max77686.c - Regulator driver for the Maxim 77686
+ *
+ * Copyright (C) 2012 Samsung Electronics Co. Ltd.
+ * Chiwoong Byun <woong.byun@samsung.com>
+ * Yadwinder Singh Brar <yadi.brar@samsung.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+ *
+ * This driver is based on max8997.c
+ */
+
+#include <linux/module.h>
+#include <linux/bug.h>
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/gpio.h>
+#include <linux/slab.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/max77686.h>
+#include <linux/mfd/max77686-private.h>
+
+struct max77686_data {
+	struct device *dev;
+	struct max77686_dev *iodev;
+	int num_regulators;
+	struct regulator_dev **rdev;
+	int ramp_delay;		/* index of ramp_delay */
+
+	/*GPIO-DVS feature is not enabled with the
+	 *current version of MAX77686 driver.*/
+};
+
+static int max77686_voltage_dvs_buck_time_sel(struct regulator_dev *rdev,
+					unsigned int old_sel,
+					unsigned int new_sel)
+{
+	struct max77686_data *max77686 = rdev_get_drvdata(rdev);
+	int ramp[] = {13, 27, 55, 100};	/* ramp_rate in mV/uS */
+
+	return DIV_ROUND_UP(rdev->desc->uV_step *
+			    abs(new_sel - old_sel),
+			    ramp[max77686->ramp_delay]);
+}
+
+static int max77686_voltage_time_sel(struct regulator_dev *rdev,
+					unsigned int old_sel,
+					unsigned int new_sel)
+{
+	return DIV_ROUND_UP(rdev->desc->uV_step *
+			    abs(new_sel - old_sel),
+			    100);
+}
+
+static struct regulator_ops max77686_ops = {
+	.map_voltage		= regulator_map_voltage_linear,
+	.list_voltage		= regulator_list_voltage_linear,
+	.is_enabled		= regulator_is_enabled_regmap,
+	.enable			= regulator_enable_regmap,
+	.disable		= regulator_disable_regmap,
+	.get_voltage_sel	= regulator_get_voltage_sel_regmap,
+	.set_voltage_sel	= regulator_set_voltage_sel_regmap,
+	.set_voltage_time_sel	= max77686_voltage_time_sel,
+};
+
+static struct regulator_ops max77686_buck_ops = {
+	.map_voltage		= regulator_map_voltage_linear,
+	.list_voltage		= regulator_list_voltage_linear,
+	.is_enabled		= regulator_is_enabled_regmap,
+	.enable			= regulator_enable_regmap,
+	.disable		= regulator_disable_regmap,
+	.get_voltage_sel	= regulator_get_voltage_sel_regmap,
+	.set_voltage_sel	= regulator_set_voltage_sel_regmap,
+	.set_voltage_time_sel	= max77686_voltage_dvs_buck_time_sel,
+};
+
+#define regulator_desc_ldo(num)		{				\
+	.name		= "LDO"#num,					\
+	.id		= MAX77686_LDO##num,				\
+	.ops		= &max77686_ops,				\
+	.type		= REGULATOR_VOLTAGE,				\
+	.owner		= THIS_MODULE,					\
+	.min_uV		= 800000,					\
+	.uV_step	= 50000,					\
+	.n_voltages	= 64,						\
+	.vsel_reg	= MAX77686_REG_LDO1CTRL1 + num - 1,		\
+	.vsel_mask	= 0x3f,						\
+	.enable_reg	= MAX77686_REG_LDO1CTRL1 + num - 1,		\
+	.enable_mask	= 0x0c,						\
+}
+#define regulator_desc_ldo_low_vol(num)	{				\
+	.name		= "LDO"#num,					\
+	.id		= MAX77686_LDO##num,				\
+	.ops		= &max77686_ops,				\
+	.type		= REGULATOR_VOLTAGE,				\
+	.owner		= THIS_MODULE,					\
+	.min_uV		= 800000,					\
+	.uV_step	= 25000,					\
+	.n_voltages	= 64,						\
+	.vsel_reg	= MAX77686_REG_LDO1CTRL1 + num - 1,		\
+	.vsel_mask	= 0x3f,						\
+	.enable_reg	= MAX77686_REG_LDO1CTRL1 + num - 1,		\
+	.enable_mask	= 0x0c,						\
+}
+#define regulator_desc_buck(num)	{				\
+	.name		= "BUCK"#num,					\
+	.id		= MAX77686_BUCK##num,				\
+	.ops		= &max77686_ops,				\
+	.type		= REGULATOR_VOLTAGE,				\
+	.owner		= THIS_MODULE,					\
+	.min_uV		= 750000,					\
+	.uV_step	= 50000,					\
+	.n_voltages	= 64,						\
+	.vsel_reg	= MAX77686_REG_BUCK5OUT + (num - 5) * 2,	\
+	.vsel_mask	= 0x3f,						\
+	.enable_reg	= MAX77686_REG_BUCK5CTRL + (num - 5) * 2,	\
+	.enable_mask	= 0x03,						\
+}
+#define regulator_desc_buck1(num)	{				\
+	.name		= "BUCK"#num,					\
+	.id		= MAX77686_BUCK##num,				\
+	.ops		= &max77686_ops,				\
+	.type		= REGULATOR_VOLTAGE,				\
+	.owner		= THIS_MODULE,					\
+	.min_uV		= 750000,					\
+	.uV_step	= 50000,					\
+	.n_voltages	= 64,						\
+	.vsel_reg	= MAX77686_REG_BUCK1OUT,			\
+	.vsel_mask	= 0x3f,						\
+	.enable_reg	= MAX77686_REG_BUCK1CTRL,			\
+	.enable_mask	= 0x03,						\
+}
+#define regulator_desc_buck_dvs(num)	{				\
+	.name		= "BUCK"#num,					\
+	.id		= MAX77686_BUCK##num,				\
+	.ops		= &max77686_buck_ops,				\
+	.type		= REGULATOR_VOLTAGE,				\
+	.owner		= THIS_MODULE,					\
+	.min_uV		= 600000,					\
+	.uV_step	= 12500,					\
+	.n_voltages	= 256,						\
+	.vsel_reg	= MAX77686_REG_BUCK2DVS1 + (num - 2) * 10,	\
+	.vsel_mask	= 0xff,						\
+	.enable_reg	= MAX77686_REG_BUCK2CTRL1 + (num - 2) * 10,	\
+	.enable_mask	= 0x30,						\
+}
+
+static struct regulator_desc regulators[] = {
+	regulator_desc_ldo_low_vol(1),
+	regulator_desc_ldo_low_vol(2),
+	regulator_desc_ldo(3),
+	regulator_desc_ldo(4),
+	regulator_desc_ldo(5),
+	regulator_desc_ldo_low_vol(6),
+	regulator_desc_ldo_low_vol(7),
+	regulator_desc_ldo_low_vol(8),
+	regulator_desc_ldo(9),
+	regulator_desc_ldo(10),
+	regulator_desc_ldo(11),
+	regulator_desc_ldo(12),
+	regulator_desc_ldo(13),
+	regulator_desc_ldo(14),
+	regulator_desc_ldo(15),
+	regulator_desc_ldo(16),
+	regulator_desc_ldo(17),
+	regulator_desc_ldo(18),
+	regulator_desc_ldo(19),
+	regulator_desc_ldo(20),
+	regulator_desc_ldo(21),
+	regulator_desc_ldo(22),
+	regulator_desc_ldo(23),
+	regulator_desc_ldo(24),
+	regulator_desc_ldo(25),
+	regulator_desc_ldo(26),
+	regulator_desc_buck1(1),
+	regulator_desc_buck_dvs(2),
+	regulator_desc_buck_dvs(3),
+	regulator_desc_buck_dvs(4),
+	regulator_desc_buck(5),
+	regulator_desc_buck(6),
+	regulator_desc_buck(7),
+	regulator_desc_buck(8),
+	regulator_desc_buck(9),
+};
+
+#ifdef CONFIG_OF
+static int max77686_pmic_dt_parse_pdata(struct max77686_dev *iodev,
+					struct max77686_platform_data *pdata)
+{
+	struct device_node *pmic_np, *regulators_np;
+	struct of_regulator_match *rdata;
+	unsigned int i, ret;
+
+	pmic_np = iodev->dev->of_node;
+	if (!pmic_np) {
+		dev_err(iodev->dev, "could not find pmic sub-node\n");
+		return -ENODEV;
+	}
+
+	regulators_np = of_find_node_by_name(pmic_np, "voltage-regulators");
+	if (!regulators_np) {
+		dev_err(iodev->dev, "could not find regulators sub-node\n");
+		return -EINVAL;
+	}
+
+	/* count the number of regulators to be supported in pmic */
+	pdata->num_regulators = ARRAY_SIZE(regulators);
+
+	rdata = devm_kzalloc(iodev->dev, sizeof(*rdata) *
+				(pdata->num_regulators), GFP_KERNEL);
+	if (!rdata) {
+		dev_err(iodev->dev,
+			"could not allocate memory for regulator data\n");
+		return -ENOMEM;
+	}
+
+	for (i = 0; i < pdata->num_regulators; i++)
+		rdata[i].name = regulators[i].name;
+
+	ret = of_regulator_match(iodev->dev, regulators_np, rdata,
+				pdata->num_regulators);
+
+	if (ret < 0)
+		dev_err(iodev->dev, "Parsing DT for regulators failed\n");
+	else
+		dev_info(iodev->dev, "regulators found in device tree : %d\n"
+			, ret);
+
+	pdata->regulators = rdata;
+
+	if (of_property_read_u32(pmic_np, "max77686,buck_ramp_delay", &i))
+		pdata->ramp_delay = i & 0xff;
+
+	return 0;
+}
+#else
+static int max77686_pmic_dt_parse_pdata(struct max77686_dev *iodev,
+					struct max77686_platform_data *pdata)
+{
+	return 0;
+}
+#endif	/* CONFIG_OF */
+
+static __devinit int max77686_pmic_probe(struct platform_device *pdev)
+{
+	struct max77686_dev *iodev = dev_get_drvdata(pdev->dev.parent);
+	struct max77686_platform_data *pdata = iodev->pdata;
+	struct regulator_dev **rdev;
+	struct max77686_data *max77686;
+	struct i2c_client *i2c = iodev->i2c;
+	struct regulator_config config = { };
+	int i, ret, size;
+
+	if (iodev->dev->of_node) {
+		ret = max77686_pmic_dt_parse_pdata(iodev, pdata);
+		if (ret)
+			return ret;
+	} else {			/* pdata from machine-setup file */
+		if (!pdata) {
+			dev_err(&pdev->dev, "platform data not found\n");
+			return -ENODEV;
+		} else {
+			if (pdata->num_regulators != ARRAY_SIZE(regulators)) {
+				dev_err(&pdev->dev,
+					"incomplete regulator list\n");
+				return -ENODEV;
+			}
+		}
+	}
+
+	max77686 = devm_kzalloc(&pdev->dev, sizeof(struct max77686_data),
+				GFP_KERNEL);
+	if (!max77686)
+		return -ENOMEM;
+
+	size = sizeof(struct regulator_dev *) * pdata->num_regulators;
+	max77686->rdev = devm_kzalloc(&pdev->dev, size, GFP_KERNEL);
+	if (!max77686->rdev) {
+		kfree(max77686);
+		return -ENOMEM;
+	}
+
+	rdev = max77686->rdev;
+
+	max77686->dev = &pdev->dev;
+	max77686->iodev = iodev;
+	max77686->num_regulators = pdata->num_regulators;
+
+	if (pdata->ramp_delay < MAX77686_RAMP_RATE_13MV ||
+		pdata->ramp_delay > MAX77686_RAMP_RATE_100MV)
+		pdata->ramp_delay = MAX77686_RAMP_RATE_27MV;	/* default */
+
+	max77686->ramp_delay = pdata->ramp_delay - 1;
+	max77686_update_reg(i2c, MAX77686_REG_BUCK2CTRL1,
+		max77686->ramp_delay << 6, RAMP_MASK);
+	max77686_update_reg(i2c, MAX77686_REG_BUCK3CTRL1,
+		max77686->ramp_delay << 6, RAMP_MASK);
+	max77686_update_reg(i2c, MAX77686_REG_BUCK4CTRL1,
+		max77686->ramp_delay << 6, RAMP_MASK);
+
+	platform_set_drvdata(pdev, max77686);
+
+	for (i = 0; i < pdata->num_regulators; i++) {
+		config.dev = max77686->dev;
+		config.init_data = pdata->regulators[i].init_data;
+		config.driver_data = max77686;
+		config.regmap = iodev->regmap;
+
+		rdev[i] = regulator_register(&regulators[i], &config);
+		if (IS_ERR(rdev[i])) {
+			ret = PTR_ERR(rdev[i]);
+			dev_err(max77686->dev,
+				"regulator init failed for id : %d\n", i);
+			rdev[i] = NULL;
+			goto err;
+		}
+	}
+
+	return 0;
+ err:
+	for (i = 0; i < max77686->num_regulators; i++)
+		if (rdev[i])
+			regulator_unregister(rdev[i]);
+
+	return ret;
+}
+
+static int __devexit max77686_pmic_remove(struct platform_device *pdev)
+{
+	struct max77686_data *max77686 = platform_get_drvdata(pdev);
+	struct regulator_dev **rdev = max77686->rdev;
+	int i;
+
+	for (i = 0; i < max77686->num_regulators; i++)
+		if (rdev[i])
+			regulator_unregister(rdev[i]);
+
+	return 0;
+}
+
+static const struct platform_device_id max77686_pmic_id[] = {
+	{"max77686-pmic", 0},
+	{},
+};
+MODULE_DEVICE_TABLE(platform, max77686_pmic_id);
+
+static struct platform_driver max77686_pmic_driver = {
+	.driver = {
+		   .name = "max77686-pmic",
+		   .owner = THIS_MODULE,
+		   },
+	.probe = max77686_pmic_probe,
+	.remove = __devexit_p(max77686_pmic_remove),
+	.id_table = max77686_pmic_id,
+};
+
+static int __init max77686_pmic_init(void)
+{
+	return platform_driver_register(&max77686_pmic_driver);
+}
+subsys_initcall(max77686_pmic_init);
+
+static void __exit max77686_pmic_cleanup(void)
+{
+	platform_driver_unregister(&max77686_pmic_driver);
+}
+module_exit(max77686_pmic_cleanup);
+
+MODULE_DESCRIPTION("MAXIM 77686 Regulator Driver");
+MODULE_AUTHOR("Chiwoong Byun <woong.byun@samsung.com>");
+MODULE_AUTHOR("Yadwinder Singh Brar <yadi.brar@samsung.com>");
+MODULE_LICENSE("GPL");
-- 
1.7.0.4

Re: [PATCH v3 2/2] regulator: Add support for MAX77686.

[jonghwa3.lee]

Hi, Yadwinder,
As you know, both of us, recently, had competition for one driver
whether you intend or not. And now, i think it is time to stop this and
to find appropriate goal. From now on, i won't update this driver no
more. I recommend you to review my patch and apply feature that you can
apply. And also check comments that i wrote below.

On 2012년 05월 22일 14:57, yadi.brar01@gmail.com wrote:

> From: Yadwinder Singh Brar <yadi.brar@samsung.com>
> 
> Add support for PMIC/regulator portion of MAX77686 multifunction device.
> MAX77686 provides LDOs[1-26] and BUCKs[1-9]. This is initial release of driv
> which supports setting and getting the voltage of a regulator with I2C
> interface.
> 
> Signed-off-by: Yadwinder Singh Brar <yadi.brar@samsung.com>
> ---
>  drivers/regulator/Kconfig    |    9 +
>  drivers/regulator/Makefile   |    1 +
>  drivers/regulator/max77686.c |  387 ++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 397 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/regulator/max77686.c
> 
> diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
> index c86b886..e8f9417 100644
> --- a/drivers/regulator/Kconfig
> +++ b/drivers/regulator/Kconfig
> @@ -195,6 +195,15 @@ config REGULATOR_MAX8998
>  	  via I2C bus. The provided regulator is suitable for S3C6410
>  	  and S5PC1XX chips to control VCC_CORE and VCC_USIM voltages.
>  
> +config REGULATOR_MAX77686
> +	tristate "Maxim 77686 regulator"
> +	depends on MFD_MAX77686
> +	help
> +	  This driver controls a Maxim 77686 voltage regulator via I2C
> +	  bus. The provided regulator is suitable for Exynos5 chips to
> +	  control VDD_ARM and VDD_INT voltages.It supports LDOs[1-26]
> +	  and BUCKs[1-9].
> +
>  config REGULATOR_PCAP
>  	tristate "Motorola PCAP2 regulator driver"
>  	depends on EZX_PCAP
> diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
> index 977fd46..d854453 100644
> --- a/drivers/regulator/Makefile
> +++ b/drivers/regulator/Makefile
> @@ -30,6 +30,7 @@ obj-$(CONFIG_REGULATOR_MAX8925) += max8925-regulator.o
>  obj-$(CONFIG_REGULATOR_MAX8952) += max8952.o
>  obj-$(CONFIG_REGULATOR_MAX8997) += max8997.o
>  obj-$(CONFIG_REGULATOR_MAX8998) += max8998.o
> +obj-$(CONFIG_REGULATOR_MAX77686) += max77686.o
>  obj-$(CONFIG_REGULATOR_MC13783) += mc13783-regulator.o
>  obj-$(CONFIG_REGULATOR_MC13892) += mc13892-regulator.o
>  obj-$(CONFIG_REGULATOR_MC13XXX_CORE) +=  mc13xxx-regulator-core.o
> diff --git a/drivers/regulator/max77686.c b/drivers/regulator/max77686.c
> new file mode 100644
> index 0000000..98dbd50
> --- /dev/null
> +++ b/drivers/regulator/max77686.c
> @@ -0,0 +1,387 @@
> +/*
> + * max77686.c - Regulator driver for the Maxim 77686
> + *
> + * Copyright (C) 2012 Samsung Electronics Co. Ltd.
> + * Chiwoong Byun <woong.byun@samsung.com>
> + * Yadwinder Singh Brar <yadi.brar@samsung.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
> + *
> + * This driver is based on max8997.c
> + */
> +
> +#include <linux/module.h>
> +#include <linux/bug.h>
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/gpio.h>
> +#include <linux/slab.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/max77686.h>
> +#include <linux/mfd/max77686-private.h>
> +
> +struct max77686_data {
> +	struct device *dev;
> +	struct max77686_dev *iodev;
> +	int num_regulators;
> +	struct regulator_dev **rdev;
> +	int ramp_delay;		/* index of ramp_delay */
> +
> +	/*GPIO-DVS feature is not enabled with the
> +	 *current version of MAX77686 driver.*/
> +};
> +
> +static int max77686_voltage_dvs_buck_time_sel(struct regulator_dev *rdev,
> +					unsigned int old_sel,
> +					unsigned int new_sel)
> +{
> +	struct max77686_data *max77686 = rdev_get_drvdata(rdev);
> +	int ramp[] = {13, 27, 55, 100};	/* ramp_rate in mV/uS */
> +
> +	return DIV_ROUND_UP(rdev->desc->uV_step *
> +			    abs(new_sel - old_sel),
> +			    ramp[max77686->ramp_delay]);
> +}
> +
> +static int max77686_voltage_time_sel(struct regulator_dev *rdev,
> +					unsigned int old_sel,
> +					unsigned int new_sel)
> +{
> +	return DIV_ROUND_UP(rdev->desc->uV_step *
> +			    abs(new_sel - old_sel),
> +			    100);
> +}
> +


Does LDO also need waiting for voltage change? I afraid it's not.

> +static struct regulator_ops max77686_ops = {
> +	.map_voltage		= regulator_map_voltage_linear,
> +	.list_voltage		= regulator_list_voltage_linear,
> +	.is_enabled		= regulator_is_enabled_regmap,
> +	.enable			= regulator_enable_regmap,
> +	.disable		= regulator_disable_regmap,
> +	.get_voltage_sel	= regulator_get_voltage_sel_regmap,
> +	.set_voltage_sel	= regulator_set_voltage_sel_regmap,
> +	.set_voltage_time_sel	= max77686_voltage_time_sel,
> +};
> +
> +static struct regulator_ops max77686_buck_ops = {
> +	.map_voltage		= regulator_map_voltage_linear,
> +	.list_voltage		= regulator_list_voltage_linear,
> +	.is_enabled		= regulator_is_enabled_regmap,
> +	.enable			= regulator_enable_regmap,
> +	.disable		= regulator_disable_regmap,
> +	.get_voltage_sel	= regulator_get_voltage_sel_regmap,
> +	.set_voltage_sel	= regulator_set_voltage_sel_regmap,
> +	.set_voltage_time_sel	= max77686_voltage_dvs_buck_time_sel,
> +};
> +
> +#define regulator_desc_ldo(num)		{				\
> +	.name		= "LDO"#num,					\
> +	.id		= MAX77686_LDO##num,				\
> +	.ops		= &max77686_ops,				\
> +	.type		= REGULATOR_VOLTAGE,				\
> +	.owner		= THIS_MODULE,					\
> +	.min_uV		= 800000,					\
> +	.uV_step	= 50000,					\
> +	.n_voltages	= 64,						\
> +	.vsel_reg	= MAX77686_REG_LDO1CTRL1 + num - 1,		\
> +	.vsel_mask	= 0x3f,						\
> +	.enable_reg	= MAX77686_REG_LDO1CTRL1 + num - 1,		\
> +	.enable_mask	= 0x0c,						\
> +}
> +#define regulator_desc_ldo_low_vol(num)	{				\
> +	.name		= "LDO"#num,					\
> +	.id		= MAX77686_LDO##num,				\
> +	.ops		= &max77686_ops,				\
> +	.type		= REGULATOR_VOLTAGE,				\
> +	.owner		= THIS_MODULE,					\
> +	.min_uV		= 800000,					\
> +	.uV_step	= 25000,					\
> +	.n_voltages	= 64,						\
> +	.vsel_reg	= MAX77686_REG_LDO1CTRL1 + num - 1,		\
> +	.vsel_mask	= 0x3f,						\
> +	.enable_reg	= MAX77686_REG_LDO1CTRL1 + num - 1,		\
> +	.enable_mask	= 0x0c,						\
> +}
> +#define regulator_desc_buck(num)	{				\
> +	.name		= "BUCK"#num,					\
> +	.id		= MAX77686_BUCK##num,				\
> +	.ops		= &max77686_ops,				\
> +	.type		= REGULATOR_VOLTAGE,				\
> +	.owner		= THIS_MODULE,					\
> +	.min_uV		= 750000,					\
> +	.uV_step	= 50000,					\
> +	.n_voltages	= 64,						\
> +	.vsel_reg	= MAX77686_REG_BUCK5OUT + (num - 5) * 2,	\
> +	.vsel_mask	= 0x3f,						\
> +	.enable_reg	= MAX77686_REG_BUCK5CTRL + (num - 5) * 2,	\
> +	.enable_mask	= 0x03,						\
> +}
> +#define regulator_desc_buck1(num)	{				\
> +	.name		= "BUCK"#num,					\
> +	.id		= MAX77686_BUCK##num,				\
> +	.ops		= &max77686_ops,				\
> +	.type		= REGULATOR_VOLTAGE,				\
> +	.owner		= THIS_MODULE,					\
> +	.min_uV		= 750000,					\
> +	.uV_step	= 50000,					\
> +	.n_voltages	= 64,						\
> +	.vsel_reg	= MAX77686_REG_BUCK1OUT,			\
> +	.vsel_mask	= 0x3f,						\
> +	.enable_reg	= MAX77686_REG_BUCK1CTRL,			\
> +	.enable_mask	= 0x03,						\
> +}
> +#define regulator_desc_buck_dvs(num)	{				\
> +	.name		= "BUCK"#num,					\
> +	.id		= MAX77686_BUCK##num,				\
> +	.ops		= &max77686_buck_ops,				\
> +	.type		= REGULATOR_VOLTAGE,				\
> +	.owner		= THIS_MODULE,					\
> +	.min_uV		= 600000,					\
> +	.uV_step	= 12500,					\
> +	.n_voltages	= 256,						\
> +	.vsel_reg	= MAX77686_REG_BUCK2DVS1 + (num - 2) * 10,	\
> +	.vsel_mask	= 0xff,						\
> +	.enable_reg	= MAX77686_REG_BUCK2CTRL1 + (num - 2) * 10,	\
> +	.enable_mask	= 0x30,						\
> +}
> +
> +static struct regulator_desc regulators[] = {
> +	regulator_desc_ldo_low_vol(1),
> +	regulator_desc_ldo_low_vol(2),
> +	regulator_desc_ldo(3),
> +	regulator_desc_ldo(4),
> +	regulator_desc_ldo(5),
> +	regulator_desc_ldo_low_vol(6),
> +	regulator_desc_ldo_low_vol(7),
> +	regulator_desc_ldo_low_vol(8),
> +	regulator_desc_ldo(9),
> +	regulator_desc_ldo(10),
> +	regulator_desc_ldo(11),
> +	regulator_desc_ldo(12),
> +	regulator_desc_ldo(13),
> +	regulator_desc_ldo(14),
> +	regulator_desc_ldo(15),
> +	regulator_desc_ldo(16),
> +	regulator_desc_ldo(17),
> +	regulator_desc_ldo(18),
> +	regulator_desc_ldo(19),
> +	regulator_desc_ldo(20),
> +	regulator_desc_ldo(21),
> +	regulator_desc_ldo(22),
> +	regulator_desc_ldo(23),
> +	regulator_desc_ldo(24),
> +	regulator_desc_ldo(25),
> +	regulator_desc_ldo(26),
> +	regulator_desc_buck1(1),
> +	regulator_desc_buck_dvs(2),
> +	regulator_desc_buck_dvs(3),
> +	regulator_desc_buck_dvs(4),
> +	regulator_desc_buck(5),
> +	regulator_desc_buck(6),
> +	regulator_desc_buck(7),
> +	regulator_desc_buck(8),
> +	regulator_desc_buck(9),
> +};
> +
> +#ifdef CONFIG_OF
> +static int max77686_pmic_dt_parse_pdata(struct max77686_dev *iodev,
> +					struct max77686_platform_data *pdata)
> +{
> +	struct device_node *pmic_np, *regulators_np;
> +	struct of_regulator_match *rdata;
> +	unsigned int i, ret;
> +
> +	pmic_np = iodev->dev->of_node;
> +	if (!pmic_np) {
> +		dev_err(iodev->dev, "could not find pmic sub-node\n");
> +		return -ENODEV;
> +	}
> +
> +	regulators_np = of_find_node_by_name(pmic_np, "voltage-regulators");
> +	if (!regulators_np) {
> +		dev_err(iodev->dev, "could not find regulators sub-node\n");
> +		return -EINVAL;
> +	}
> +
> +	/* count the number of regulators to be supported in pmic */
> +	pdata->num_regulators = ARRAY_SIZE(regulators);
> +
> +	rdata = devm_kzalloc(iodev->dev, sizeof(*rdata) *
> +				(pdata->num_regulators), GFP_KERNEL);
> +	if (!rdata) {
> +		dev_err(iodev->dev,
> +			"could not allocate memory for regulator data\n");
> +		return -ENOMEM;
> +	}
> +
> +	for (i = 0; i < pdata->num_regulators; i++)
> +		rdata[i].name = regulators[i].name;
> +
> +	ret = of_regulator_match(iodev->dev, regulators_np, rdata,
> +				pdata->num_regulators);
> +
> +	if (ret < 0)
> +		dev_err(iodev->dev, "Parsing DT for regulators failed\n");
> +	else
> +		dev_info(iodev->dev, "regulators found in device tree : %d\n"
> +			, ret);
> +
> +	pdata->regulators = rdata;
> +
> +	if (of_property_read_u32(pmic_np, "max77686,buck_ramp_delay", &i))
> +		pdata->ramp_delay = i & 0xff;
> +
> +	return 0;
> +}
> +#else
> +static int max77686_pmic_dt_parse_pdata(struct max77686_dev *iodev,
> +					struct max77686_platform_data *pdata)
> +{
> +	return 0;
> +}
> +#endif	/* CONFIG_OF */
> +
> +static __devinit int max77686_pmic_probe(struct platform_device *pdev)
> +{
> +	struct max77686_dev *iodev = dev_get_drvdata(pdev->dev.parent);
> +	struct max77686_platform_data *pdata = iodev->pdata;
> +	struct regulator_dev **rdev;
> +	struct max77686_data *max77686;
> +	struct i2c_client *i2c = iodev->i2c;
> +	struct regulator_config config = { };
> +	int i, ret, size;
> +
> +	if (iodev->dev->of_node) {
> +		ret = max77686_pmic_dt_parse_pdata(iodev, pdata);
> +		if (ret)
> +			return ret;
> +	} else {			/* pdata from machine-setup file */
> +		if (!pdata) {
> +			dev_err(&pdev->dev, "platform data not found\n");
> +			return -ENODEV;
> +		} else {
> +			if (pdata->num_regulators != ARRAY_SIZE(regulators)) {
> +				dev_err(&pdev->dev,
> +					"incomplete regulator list\n");
> +				return -ENODEV;
> +			}
> +		}
> +	}
> +
> +	max77686 = devm_kzalloc(&pdev->dev, sizeof(struct max77686_data),
> +				GFP_KERNEL);
> +	if (!max77686)
> +		return -ENOMEM;
> +
> +	size = sizeof(struct regulator_dev *) * pdata->num_regulators;
> +	max77686->rdev = devm_kzalloc(&pdev->dev, size, GFP_KERNEL);
> +	if (!max77686->rdev) {
> +		kfree(max77686);
> +		return -ENOMEM;
> +	}
> +
> +	rdev = max77686->rdev;
> +
> +	max77686->dev = &pdev->dev;
> +	max77686->iodev = iodev;
> +	max77686->num_regulators = pdata->num_regulators;
> +
> +	if (pdata->ramp_delay < MAX77686_RAMP_RATE_13MV ||
> +		pdata->ramp_delay > MAX77686_RAMP_RATE_100MV)
> +		pdata->ramp_delay = MAX77686_RAMP_RATE_27MV;	/* default */
> +
> +	max77686->ramp_delay = pdata->ramp_delay - 1;


I think it is better to check pdata->ramp_delay is available.
If pdata doesn't have ramp_delay member it might be error.

> +	max77686_update_reg(i2c, MAX77686_REG_BUCK2CTRL1,
> +		max77686->ramp_delay << 6, RAMP_MASK);
> +	max77686_update_reg(i2c, MAX77686_REG_BUCK3CTRL1,
> +		max77686->ramp_delay << 6, RAMP_MASK);
> +	max77686_update_reg(i2c, MAX77686_REG_BUCK4CTRL1,
> +		max77686->ramp_delay << 6, RAMP_MASK);
> +


Why do you use i2c client still? If you registered regmap you can use
its API. I recommend you to use regmap_update_bits() directly.


> +	platform_set_drvdata(pdev, max77686);
> +
> +	for (i = 0; i < pdata->num_regulators; i++) {
> +		config.dev = max77686->dev;
> +		config.init_data = pdata->regulators[i].init_data;
> +		config.driver_data = max77686;
> +		config.regmap = iodev->regmap;
> +
> +		rdev[i] = regulator_register(&regulators[i], &config);
> +		if (IS_ERR(rdev[i])) {
> +			ret = PTR_ERR(rdev[i]);
> +			dev_err(max77686->dev,
> +				"regulator init failed for id : %d\n", i);
> +			rdev[i] = NULL;
> +			goto err;
> +		}
> +	}
> +
> +	return 0;
> + err:
> +	for (i = 0; i < max77686->num_regulators; i++)
> +		if (rdev[i])
> +			regulator_unregister(rdev[i]);
> +
> +	return ret;
> +}
> +
> +static int __devexit max77686_pmic_remove(struct platform_device *pdev)
> +{
> +	struct max77686_data *max77686 = platform_get_drvdata(pdev);
> +	struct regulator_dev **rdev = max77686->rdev;
> +	int i;
> +
> +	for (i = 0; i < max77686->num_regulators; i++)
> +		if (rdev[i])
> +			regulator_unregister(rdev[i]);
> +
> +	return 0;
> +}
> +
> +static const struct platform_device_id max77686_pmic_id[] = {
> +	{"max77686-pmic", 0},
> +	{},
> +};
> +MODULE_DEVICE_TABLE(platform, max77686_pmic_id);
> +
> +static struct platform_driver max77686_pmic_driver = {
> +	.driver = {
> +		   .name = "max77686-pmic",
> +		   .owner = THIS_MODULE,
> +		   },
> +	.probe = max77686_pmic_probe,
> +	.remove = __devexit_p(max77686_pmic_remove),
> +	.id_table = max77686_pmic_id,
> +};
> +
> +static int __init max77686_pmic_init(void)
> +{
> +	return platform_driver_register(&max77686_pmic_driver);
> +}
> +subsys_initcall(max77686_pmic_init);
> +
> +static void __exit max77686_pmic_cleanup(void)
> +{
> +	platform_driver_unregister(&max77686_pmic_driver);
> +}
> +module_exit(max77686_pmic_cleanup);
> +
> +MODULE_DESCRIPTION("MAXIM 77686 Regulator Driver");
> +MODULE_AUTHOR("Chiwoong Byun <woong.byun@samsung.com>");
> +MODULE_AUTHOR("Yadwinder Singh Brar <yadi.brar@samsung.com>");
> +MODULE_LICENSE("GPL");


MAX77686 has crystal oscillator in it. And original version of this
driver which was written by Chiwoon Byun, registers it as a regulator.
As Mark says, we have to change it to use generic clock API. Where do
you think should we put them into? In my opinion, it is proper that just
leave them in regulator driver because this driver is almost core of
PMIC. I already applied generic API in my local repository but i
couldn't test yet. Because it crashed with SOC's private clock API.
Anyway if you register 32khz clock with generic API ,DEFINE_CLK_GATE()
will help you out which defined in linux/clk-private.h.

Thanks.

Re: [PATCH v3 2/2] regulator: Add support for MAX77686.

[jonghwa3.lee]

Hi, again.
On 2012년 05월 22일 14:57, yadi.brar01@gmail.com wrote:


> +static __devinit int max77686_pmic_probe(struct platform_device *pdev)
> +{

> +
> +	for (i = 0; i < pdata->num_regulators; i++) {
> +		config.dev = max77686->dev;
> +		config.init_data = pdata->regulators[i].init_data;
> +		config.driver_data = max77686;
> +		config.regmap = iodev->regmap;
> +
> +		rdev[i] = regulator_register(&regulators[i], &config);


I'm sorry that i missed one. You have to register all regulators
unconditionally. Mark brown commented about this to my former patch.

'No, you should unconditionally register all regulators the device
physically has.  This is useful for debug and simplifies the code.'
						- from Mark Brown



Thanks.

Re: [PATCH v3 2/2] regulator: Add support for MAX77686.

[Yadwinder Singh Brar]

(adding Kyungmin Park and Samuel Ortiz)

Hi,

Yes, It happened unintentionally. I didn't know about your patch
before submitting
the initial version of my patches. I agree with you, I will review
your patches and
will try to incorporate extra features from your patches.

On Wed, May 23, 2012 at 7:10 AM,  <jonghwa3.lee@samsung.com> wrote:
> Hi, Yadwinder,
> As you know, both of us, recently, had competition for one driver
> whether you intend or not. And now, i think it is time to stop this and
> to find appropriate goal. From now on, i won't update this driver no
> more. I recommend you to review my patch and apply feature that you can
> apply. And also check comments that i wrote below.
>
> On 2012년 05월 22일 14:57, yadi.brar01@gmail.com wrote:
>
>> From: Yadwinder Singh Brar <yadi.brar@samsung.com>
>>
>> Add support for PMIC/regulator portion of MAX77686 multifunction device.
>> MAX77686 provides LDOs[1-26] and BUCKs[1-9]. This is initial release of driv
>> which supports setting and getting the voltage of a regulator with I2C
>> interface.

>> +     return DIV_ROUND_UP(rdev->desc->uV_step *
>> +                         abs(new_sel - old_sel),
>> +                         100);
>> +}
>> +
>
>
> Does LDO also need waiting for voltage change? I afraid it's not.
>

Yes, according to technical reference manual which I have, ramp rate for
LDOs is also 100mV/us.

>> +
>> +     if (pdata->ramp_delay < MAX77686_RAMP_RATE_13MV ||
>> +             pdata->ramp_delay > MAX77686_RAMP_RATE_100MV)
>> +             pdata->ramp_delay = MAX77686_RAMP_RATE_27MV;    /* default */

If pdata doesn't have proper ramp_delay, it will get value
MAX77686_RAMP_RATE_27MV.

>> +
>> +     max77686->ramp_delay = pdata->ramp_delay - 1;
>
>
> I think it is better to check pdata->ramp_delay is available.
> If pdata doesn't have ramp_delay member it might be error.
>

Yes, we have taken care of this case above before setting value of
max77686->ramp_delay.

>> +     max77686_update_reg(i2c, MAX77686_REG_BUCK2CTRL1,
>> +             max77686->ramp_delay << 6, RAMP_MASK);
>> +     max77686_update_reg(i2c, MAX77686_REG_BUCK3CTRL1,
>> +             max77686->ramp_delay << 6, RAMP_MASK);
>> +     max77686_update_reg(i2c, MAX77686_REG_BUCK4CTRL1,
>> +             max77686->ramp_delay << 6, RAMP_MASK);
>> +
>
>
> Why do you use i2c client still? If you registered regmap you can use
> its API. I recommend you to use regmap_update_bits() directly.
>
>

Yes, we are using regmap_update_bits().  max77686_update_reg() is just
a wrapper over it.

>> +     platform_set_drvdata(pdev, max77686);
>> +

>
> MAX77686 has crystal oscillator in it. And original version of this
> driver which was written by Chiwoon Byun, registers it as a regulator.
> As Mark says, we have to change it to use generic clock API. Where do
> you think should we put them into? In my opinion, it is proper that just
> leave them in regulator driver because this driver is almost core of
> PMIC. I already applied generic API in my local repository but i
> couldn't test yet. Because it crashed with SOC's private clock API.
> Anyway if you register 32khz clock with generic API ,DEFINE_CLK_GATE()
> will help you out which defined in linux/clk-private.h.
>

Yes, I was also thinking about where to put it. I am not sure whether this
is a proper place to put them. Anyway I will again think about it.

Thanks,
Yadwinder.

Re: [PATCH v3 2/2] regulator: Add support for MAX77686.

[Yadwinder Singh Brar]

Hi,

On Wed, May 23, 2012 at 7:20 AM,  <jonghwa3.lee@samsung.com> wrote:
> Hi, again.
> On 2012년 05월 22일 14:57, yadi.brar01@gmail.com wrote:
>
>
>> +static __devinit int max77686_pmic_probe(struct platform_device *pdev)
>> +{
>
>> +
>> +     for (i = 0; i < pdata->num_regulators; i++) {
>> +             config.dev = max77686->dev;
>> +             config.init_data = pdata->regulators[i].init_data;
>> +             config.driver_data = max77686;
>> +             config.regmap = iodev->regmap;
>> +
>> +             rdev[i] = regulator_register(&regulators[i], &config);
>
>
> I'm sorry that i missed one. You have to register all regulators
> unconditionally. Mark brown commented about this to my former patch.
>
> 'No, you should unconditionally register all regulators the device
> physically has.  This is useful for debug and simplifies the code.'
>                                                - from Mark Brown
>

Yes, we are registering all regulators here.
As pdata->num_regulators will be equal to ARRAY_SIZE(regulators)

Thanks,
Yadwinder.

Re: [PATCH v3 2/2] regulator: Add support for MAX77686.

[jonghwa3.lee]

On 2012년 05월 23일 13:16, Yadwinder Singh Brar wrote:

>>> +     max77686_update_reg(i2c, MAX77686_REG_BUCK2CTRL1,
>>> +             max77686->ramp_delay << 6, RAMP_MASK);
>>> +     max77686_update_reg(i2c, MAX77686_REG_BUCK3CTRL1,
>>> +             max77686->ramp_delay << 6, RAMP_MASK);
>>> +     max77686_update_reg(i2c, MAX77686_REG_BUCK4CTRL1,
>>> +             max77686->ramp_delay << 6, RAMP_MASK);
>>> +
>>
>>
>> Why do you use i2c client still? If you registered regmap you can use
>> its API. I recommend you to use regmap_update_bits() directly.
>>
>>
> 
> Yes, we are using regmap_update_bits().  max77686_update_reg() is just
> a wrapper over it.
> 


Yes, i know what you mean. However it doesn't need max77686_update_reg()
any more since it uses regmap API. Why don't you just pass iodev->regmap
to regmap_update_bits(). It is clear that there is no reason for using
i2c client as a medium. Please check regulator and mfd driver of my
previous patch.


Thanks.

Re: [PATCH v3 2/2] regulator: Add support for MAX77686.

[Yadwinder Singh Brar]

On Wed, May 23, 2012 at 10:10 AM,  <jonghwa3.lee@samsung.com> wrote:
> On 2012년 05월 23일 13:16, Yadwinder Singh Brar wrote:
>
>>>> +     max77686_update_reg(i2c, MAX77686_REG_BUCK2CTRL1,
>>>> +             max77686->ramp_delay << 6, RAMP_MASK);
>>>> +     max77686_update_reg(i2c, MAX77686_REG_BUCK3CTRL1,
>>>> +             max77686->ramp_delay << 6, RAMP_MASK);
>>>> +     max77686_update_reg(i2c, MAX77686_REG_BUCK4CTRL1,
>>>> +             max77686->ramp_delay << 6, RAMP_MASK);
>>>> +
>>>
>>>
>>> Why do you use i2c client still? If you registered regmap you can use
>>> its API. I recommend you to use regmap_update_bits() directly.
>>>
>>>
>>
>> Yes, we are using regmap_update_bits().  max77686_update_reg() is just
>> a wrapper over it.
>>
>
>
> Yes, i know what you mean. However it doesn't need max77686_update_reg()
> any more since it uses regmap API. Why don't you just pass iodev->regmap
> to regmap_update_bits(). It is clear that there is no reason for using
> i2c client as a medium. Please check regulator and mfd driver of my
> previous patch.
>

I agree with you we can use directly  regmap API. But I preferred
max77686_update_reg() because its a common practice to use
common read/write API which we define in mfd driver to access
that particular mfd device from other drivers.

Regards,
Yadwinder.

Re: [PATCH v3 2/2] regulator: Add support for MAX77686.

[jonghwa3.lee]

On 2012년 05월 23일 14:23, Yadwinder Singh Brar wrote:

> On Wed, May 23, 2012 at 10:10 AM,  <jonghwa3.lee@samsung.com> wrote:
>> On 2012년 05월 23일 13:16, Yadwinder Singh Brar wrote:
>>
>>>>> +     max77686_update_reg(i2c, MAX77686_REG_BUCK2CTRL1,
>>>>> +             max77686->ramp_delay << 6, RAMP_MASK);
>>>>> +     max77686_update_reg(i2c, MAX77686_REG_BUCK3CTRL1,
>>>>> +             max77686->ramp_delay << 6, RAMP_MASK);
>>>>> +     max77686_update_reg(i2c, MAX77686_REG_BUCK4CTRL1,
>>>>> +             max77686->ramp_delay << 6, RAMP_MASK);
>>>>> +
>>>>
>>>>
>>>> Why do you use i2c client still? If you registered regmap you can use
>>>> its API. I recommend you to use regmap_update_bits() directly.
>>>>
>>>>
>>>
>>> Yes, we are using regmap_update_bits().  max77686_update_reg() is just
>>> a wrapper over it.
>>>
>>
>>
>> Yes, i know what you mean. However it doesn't need max77686_update_reg()
>> any more since it uses regmap API. Why don't you just pass iodev->regmap
>> to regmap_update_bits(). It is clear that there is no reason for using
>> i2c client as a medium. Please check regulator and mfd driver of my
>> previous patch.
>>
> 
> I agree with you we can use directly  regmap API. But I preferred
> max77686_update_reg() because its a common practice to use
> common read/write API which we define in mfd driver to access
> that particular mfd device from other drivers.
> 
> Regards,
> Yadwinder.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 


I inform you my mfd driver has been confirmed by Samuel Oritz and there
is no mfd private API. This situation looks unusual that we registers
mfd driver and regulator driver separately. But how should we do? For
corporation , i'm asking you to consider my suggestion.

Thanks.


Thanks.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 2/2] regulator: Add support for MAX77686.

[Yadwinder Singh Brar]

On Wed, May 23, 2012 at 9:46 AM, Yadwinder Singh Brar
<yadi.brar01@gmail.com> wrote:
> (adding Kyungmin Park and Samuel Ortiz)
>
> Hi,
>
> Yes, It happened unintentionally. I didn't know about your patch
> before submitting
> the initial version of my patches. I agree with you, I will review
> your patches and
> will try to incorporate extra features from your patches.
>

Now I have seen your patches for mfd and regulator drivers.
Apparently, it seems that mostly we same features in our patches.
Their is no extra feature to be incorporated form your patches.
Rather I found device tree support is additional in our patches
and mainly their are some differences related to DVS_GPIO and
opmode stuff in our patches:
1- Since we are not implementing and using DVS feature
through GPIOs, so all(incomplete) stuff related to dvs_gpio
is not required currently in our mfd driver presently.
2- Since presently, we are not implementing suspend_enable/disable
callbacks in regulator driver, So we don't need opmode related
stuff now because I think regulators should come up in normal mode
only through .enable callback function.

> On Wed, May 23, 2012 at 7:10 AM,  <jonghwa3.lee@samsung.com> wrote:
>> Hi, Yadwinder,
>> As you know, both of us, recently, had competition for one driver
>> whether you intend or not. And now, i think it is time to stop this and
>> to find appropriate goal. From now on, i won't update this driver no
>> more. I recommend you to review my patch and apply feature that you can
>> apply. And also check comments that i wrote below.
>>

Thanks,
Yadwinder.

Re: [PATCH v3 2/2] regulator: Add support for MAX77686.

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 474 bytes --]

On Wed, May 23, 2012 at 02:33:28PM +0900, jonghwa3.lee@samsung.com wrote:

> I inform you my mfd driver has been confirmed by Samuel Oritz and there
> is no mfd private API. This situation looks unusual that we registers
> mfd driver and regulator driver separately. But how should we do? For
> corporation , i'm asking you to consider my suggestion.

Given all this discussion I'm going to ignore this patch for now and
wait for a repost after you've come to an agreement.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v3 2/2] regulator: Add support for MAX77686.

[Yadwinder Singh Brar]

Hi Jonghwa,

I didn't know about confirmation of your mfd driver by Samuel Oritz.
So please feel free to revise and submit your patch set.
Anyways, I am interested only in getting the driver for max77686 in mainline.

Thanks,
Yadwinder.

On Wed, May 23, 2012 at 3:48 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Wed, May 23, 2012 at 02:33:28PM +0900, jonghwa3.lee@samsung.com wrote:
>
>> I inform you my mfd driver has been confirmed by Samuel Oritz and there
>> is no mfd private API. This situation looks unusual that we registers
>> mfd driver and regulator driver separately. But how should we do? For
>> corporation , i'm asking you to consider my suggestion.
>
> Given all this discussion I'm going to ignore this patch for now and
> wait for a repost after you've come to an agreement.

[PATCHv2 0/5] rtc: s3c: Refactoring s3c-rtc driver and support Exynos3250 RTC

[y]

From: Chanwoo Choi <cw00.choi@samsung.com>

This patchset clean up codes to improve readability as following and support
the RTC of Exynos3250 SoC.
- Remove global variables and then use new s3c_rtc structure
- Remove warn message with checking checkpatch script
- Use variant structure according to SoC type instead of legacy enum variable(s3c_cpu_type)

Changes from v1:
- Fix NULL pointer panic of s3c_rtc_settime()
- Check info->base value to protect NULL pointer panic

Chanwoo Choi (5):
  rtc: s3c: Define s3c_rtc structure to remove global variables.
  rtc: s3c: Remove warning message when checking coding style with checkpatch script
  rtc: s3c: Add s3c_rtc_data structure to use variant data instead of s3c_cpu_type
  rtc: s3c: Add support for RTC of Exynos3250 SoC
  ARM: dts: Fix wrong compatible string of Exynos3250 RTC dt node

 Documentation/devicetree/bindings/rtc/s3c-rtc.txt |   3 +
 arch/arm/boot/dts/exynos3250.dtsi                 |   2 +-
 drivers/rtc/rtc-s3c.c                             | 869 ++++++++++++++--------
 3 files changed, 553 insertions(+), 321 deletions(-)

-- 
1.8.0

[PATCHv2 1/5] rtc: s3c: Define s3c_rtc structure to remove global variables.

[y]

From: Chanwoo Choi <cw00.choi@samsung.com>

This patch define s3c_rtc structure including necessary variables for S3C RTC
device instead of global variables. This patch improves the readability by
removing global variables.

Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/rtc/rtc-s3c.c | 448 ++++++++++++++++++++++++++------------------------
 1 file changed, 235 insertions(+), 213 deletions(-)

diff --git a/drivers/rtc/rtc-s3c.c b/drivers/rtc/rtc-s3c.c
index 4958a36..1d9e158 100644
--- a/drivers/rtc/rtc-s3c.c
+++ b/drivers/rtc/rtc-s3c.c
@@ -43,125 +43,135 @@ struct s3c_rtc_drv_data {
 	int cpu_type;
 };
 
-/* I have yet to find an S3C implementation with more than one
- * of these rtc blocks in */
+struct s3c_rtc {
+	struct device *dev;
+	struct rtc_device *rtc;
+
+	void __iomem *base;
+	struct clk *rtc_clk;
+	bool enabled;
 
-static struct clk *rtc_clk;
-static void __iomem *s3c_rtc_base;
-static int s3c_rtc_alarmno;
-static int s3c_rtc_tickno;
-static enum s3c_cpu_type s3c_rtc_cpu_type;
+	enum s3c_cpu_type cpu_type;
 
-static DEFINE_SPINLOCK(s3c_rtc_pie_lock);
+	int irq_alarm;
+	int irq_tick;
 
-static void s3c_rtc_alarm_clk_enable(bool enable)
+	spinlock_t pie_lock;
+	spinlock_t alarm_clk_lock;
+
+	int ticnt_save, ticnt_en_save;
+	bool wake_en;
+};
+
+static void s3c_rtc_alarm_clk_enable(struct s3c_rtc *info, bool enable)
 {
-	static DEFINE_SPINLOCK(s3c_rtc_alarm_clk_lock);
-	static bool alarm_clk_enabled;
 	unsigned long irq_flags;
 
-	spin_lock_irqsave(&s3c_rtc_alarm_clk_lock, irq_flags);
+	spin_lock_irqsave(&info->alarm_clk_lock, irq_flags);
 	if (enable) {
-		if (!alarm_clk_enabled) {
-			clk_enable(rtc_clk);
-			alarm_clk_enabled = true;
+		if (!info->enabled) {
+			clk_enable(info->rtc_clk);
+			info->enabled = true;
 		}
 	} else {
-		if (alarm_clk_enabled) {
-			clk_disable(rtc_clk);
-			alarm_clk_enabled = false;
+		if (info->enabled) {
+			clk_disable(info->rtc_clk);
+			info->enabled = false;
 		}
 	}
-	spin_unlock_irqrestore(&s3c_rtc_alarm_clk_lock, irq_flags);
+	spin_unlock_irqrestore(&info->alarm_clk_lock, irq_flags);
 }
 
 /* IRQ Handlers */
-
 static irqreturn_t s3c_rtc_alarmirq(int irq, void *id)
 {
-	struct rtc_device *rdev = id;
+	struct s3c_rtc *info = (struct s3c_rtc *)id;
 
-	clk_enable(rtc_clk);
-	rtc_update_irq(rdev, 1, RTC_AF | RTC_IRQF);
+	clk_enable(info->rtc_clk);
+	rtc_update_irq(info->rtc, 1, RTC_AF | RTC_IRQF);
 
-	if (s3c_rtc_cpu_type == TYPE_S3C64XX)
-		writeb(S3C2410_INTP_ALM, s3c_rtc_base + S3C2410_INTP);
+	if (info->cpu_type == TYPE_S3C64XX)
+		writeb(S3C2410_INTP_ALM, info->base + S3C2410_INTP);
 
-	clk_disable(rtc_clk);
+	clk_disable(info->rtc_clk);
 
-	s3c_rtc_alarm_clk_enable(false);
+	s3c_rtc_alarm_clk_enable(info, false);
 
 	return IRQ_HANDLED;
 }
 
 static irqreturn_t s3c_rtc_tickirq(int irq, void *id)
 {
-	struct rtc_device *rdev = id;
+	struct s3c_rtc *info = (struct s3c_rtc *)id;
+
+	clk_enable(info->rtc_clk);
+	rtc_update_irq(info->rtc, 1, RTC_PF | RTC_IRQF);
 
-	clk_enable(rtc_clk);
-	rtc_update_irq(rdev, 1, RTC_PF | RTC_IRQF);
+	if (info->cpu_type == TYPE_S3C64XX)
+		writeb(S3C2410_INTP_TIC, info->base + S3C2410_INTP);
 
-	if (s3c_rtc_cpu_type == TYPE_S3C64XX)
-		writeb(S3C2410_INTP_TIC, s3c_rtc_base + S3C2410_INTP);
+	clk_disable(info->rtc_clk);
 
-	clk_disable(rtc_clk);
 	return IRQ_HANDLED;
 }
 
 /* Update control registers */
 static int s3c_rtc_setaie(struct device *dev, unsigned int enabled)
 {
+	struct s3c_rtc *info = dev_get_drvdata(dev);
 	unsigned int tmp;
 
-	dev_dbg(dev, "%s: aie=%d\n", __func__, enabled);
+	if (!info->base)
+		return -EINVAL;
+
+	dev_dbg(info->dev, "%s: aie=%d\n", __func__, enabled);
 
-	clk_enable(rtc_clk);
-	tmp = readb(s3c_rtc_base + S3C2410_RTCALM) & ~S3C2410_RTCALM_ALMEN;
+	clk_enable(info->rtc_clk);
+	tmp = readb(info->base + S3C2410_RTCALM) & ~S3C2410_RTCALM_ALMEN;
 
 	if (enabled)
 		tmp |= S3C2410_RTCALM_ALMEN;
 
-	writeb(tmp, s3c_rtc_base + S3C2410_RTCALM);
-	clk_disable(rtc_clk);
+	writeb(tmp, info->base + S3C2410_RTCALM);
+	clk_disable(info->rtc_clk);
 
-	s3c_rtc_alarm_clk_enable(enabled);
+	s3c_rtc_alarm_clk_enable(info, enabled);
 
 	return 0;
 }
 
-static int s3c_rtc_setfreq(struct device *dev, int freq)
+static int s3c_rtc_setfreq(struct s3c_rtc *info, int freq)
 {
-	struct platform_device *pdev = to_platform_device(dev);
-	struct rtc_device *rtc_dev = platform_get_drvdata(pdev);
 	unsigned int tmp = 0;
 	int val;
 
 	if (!is_power_of_2(freq))
 		return -EINVAL;
 
-	clk_enable(rtc_clk);
-	spin_lock_irq(&s3c_rtc_pie_lock);
+	clk_enable(info->rtc_clk);
+	spin_lock_irq(&info->pie_lock);
 
-	if (s3c_rtc_cpu_type != TYPE_S3C64XX) {
-		tmp = readb(s3c_rtc_base + S3C2410_TICNT);
+	if (info->cpu_type != TYPE_S3C64XX) {
+		tmp = readb(info->base + S3C2410_TICNT);
 		tmp &= S3C2410_TICNT_ENABLE;
 	}
 
-	val = (rtc_dev->max_user_freq / freq) - 1;
+	val = (info->rtc->max_user_freq / freq) - 1;
 
-	if (s3c_rtc_cpu_type == TYPE_S3C2416 || s3c_rtc_cpu_type == TYPE_S3C2443) {
+	if (info->cpu_type == TYPE_S3C2416 || info->cpu_type == TYPE_S3C2443) {
 		tmp |= S3C2443_TICNT_PART(val);
-		writel(S3C2443_TICNT1_PART(val), s3c_rtc_base + S3C2443_TICNT1);
+		writel(S3C2443_TICNT1_PART(val), info->base + S3C2443_TICNT1);
 
-		if (s3c_rtc_cpu_type == TYPE_S3C2416)
-			writel(S3C2416_TICNT2_PART(val), s3c_rtc_base + S3C2416_TICNT2);
+		if (info->cpu_type == TYPE_S3C2416)
+			writel(S3C2416_TICNT2_PART(val),
+				info->base + S3C2416_TICNT2);
 	} else {
 		tmp |= val;
 	}
 
-	writel(tmp, s3c_rtc_base + S3C2410_TICNT);
-	spin_unlock_irq(&s3c_rtc_pie_lock);
-	clk_disable(rtc_clk);
+	writel(tmp, info->base + S3C2410_TICNT);
+	spin_unlock_irq(&info->pie_lock);
+	clk_disable(info->rtc_clk);
 
 	return 0;
 }
@@ -170,17 +180,20 @@ static int s3c_rtc_setfreq(struct device *dev, int freq)
 
 static int s3c_rtc_gettime(struct device *dev, struct rtc_time *rtc_tm)
 {
+	struct s3c_rtc *info = dev_get_drvdata(dev);
 	unsigned int have_retried = 0;
-	void __iomem *base = s3c_rtc_base;
 
-	clk_enable(rtc_clk);
+	if (!info->base)
+		return -EINVAL;
+
+	clk_enable(info->rtc_clk);
  retry_get_time:
-	rtc_tm->tm_min  = readb(base + S3C2410_RTCMIN);
-	rtc_tm->tm_hour = readb(base + S3C2410_RTCHOUR);
-	rtc_tm->tm_mday = readb(base + S3C2410_RTCDATE);
-	rtc_tm->tm_mon  = readb(base + S3C2410_RTCMON);
-	rtc_tm->tm_year = readb(base + S3C2410_RTCYEAR);
-	rtc_tm->tm_sec  = readb(base + S3C2410_RTCSEC);
+	rtc_tm->tm_min  = readb(info->base + S3C2410_RTCMIN);
+	rtc_tm->tm_hour = readb(info->base + S3C2410_RTCHOUR);
+	rtc_tm->tm_mday = readb(info->base + S3C2410_RTCDATE);
+	rtc_tm->tm_mon  = readb(info->base + S3C2410_RTCMON);
+	rtc_tm->tm_year = readb(info->base + S3C2410_RTCYEAR);
+	rtc_tm->tm_sec  = readb(info->base + S3C2410_RTCSEC);
 
 	/* the only way to work out whether the system was mid-update
 	 * when we read it is to check the second counter, and if it
@@ -207,15 +220,19 @@ static int s3c_rtc_gettime(struct device *dev, struct rtc_time *rtc_tm)
 
 	rtc_tm->tm_mon -= 1;
 
-	clk_disable(rtc_clk);
+	clk_disable(info->rtc_clk);
+
 	return rtc_valid_tm(rtc_tm);
 }
 
 static int s3c_rtc_settime(struct device *dev, struct rtc_time *tm)
 {
-	void __iomem *base = s3c_rtc_base;
+	struct s3c_rtc *info = dev_get_drvdata(dev);
 	int year = tm->tm_year - 100;
 
+	if (!info->base)
+		return -EINVAL;
+
 	dev_dbg(dev, "set time %04d.%02d.%02d %02d:%02d:%02d\n",
 		 1900 + tm->tm_year, tm->tm_mon, tm->tm_mday,
 		 tm->tm_hour, tm->tm_min, tm->tm_sec);
@@ -227,33 +244,38 @@ static int s3c_rtc_settime(struct device *dev, struct rtc_time *tm)
 		return -EINVAL;
 	}
 
-	clk_enable(rtc_clk);
-	writeb(bin2bcd(tm->tm_sec),  base + S3C2410_RTCSEC);
-	writeb(bin2bcd(tm->tm_min),  base + S3C2410_RTCMIN);
-	writeb(bin2bcd(tm->tm_hour), base + S3C2410_RTCHOUR);
-	writeb(bin2bcd(tm->tm_mday), base + S3C2410_RTCDATE);
-	writeb(bin2bcd(tm->tm_mon + 1), base + S3C2410_RTCMON);
-	writeb(bin2bcd(year), base + S3C2410_RTCYEAR);
-	clk_disable(rtc_clk);
+	clk_enable(info->rtc_clk);
+
+	writeb(bin2bcd(tm->tm_sec),  info->base + S3C2410_RTCSEC);
+	writeb(bin2bcd(tm->tm_min),  info->base + S3C2410_RTCMIN);
+	writeb(bin2bcd(tm->tm_hour), info->base + S3C2410_RTCHOUR);
+	writeb(bin2bcd(tm->tm_mday), info->base + S3C2410_RTCDATE);
+	writeb(bin2bcd(tm->tm_mon + 1), info->base + S3C2410_RTCMON);
+	writeb(bin2bcd(year), info->base + S3C2410_RTCYEAR);
+
+	clk_disable(info->rtc_clk);
 
 	return 0;
 }
 
 static int s3c_rtc_getalarm(struct device *dev, struct rtc_wkalrm *alrm)
 {
+	struct s3c_rtc *info = dev_get_drvdata(dev);
 	struct rtc_time *alm_tm = &alrm->time;
-	void __iomem *base = s3c_rtc_base;
 	unsigned int alm_en;
 
-	clk_enable(rtc_clk);
-	alm_tm->tm_sec  = readb(base + S3C2410_ALMSEC);
-	alm_tm->tm_min  = readb(base + S3C2410_ALMMIN);
-	alm_tm->tm_hour = readb(base + S3C2410_ALMHOUR);
-	alm_tm->tm_mon  = readb(base + S3C2410_ALMMON);
-	alm_tm->tm_mday = readb(base + S3C2410_ALMDATE);
-	alm_tm->tm_year = readb(base + S3C2410_ALMYEAR);
+	if (!info->base)
+		return -EINVAL;
 
-	alm_en = readb(base + S3C2410_RTCALM);
+	clk_enable(info->rtc_clk);
+	alm_tm->tm_sec  = readb(info->base + S3C2410_ALMSEC);
+	alm_tm->tm_min  = readb(info->base + S3C2410_ALMMIN);
+	alm_tm->tm_hour = readb(info->base + S3C2410_ALMHOUR);
+	alm_tm->tm_mon  = readb(info->base + S3C2410_ALMMON);
+	alm_tm->tm_mday = readb(info->base + S3C2410_ALMDATE);
+	alm_tm->tm_year = readb(info->base + S3C2410_ALMYEAR);
+
+	alm_en = readb(info->base + S3C2410_RTCALM);
 
 	alrm->enabled = (alm_en & S3C2410_RTCALM_ALMEN) ? 1 : 0;
 
@@ -297,65 +319,73 @@ static int s3c_rtc_getalarm(struct device *dev, struct rtc_wkalrm *alrm)
 	else
 		alm_tm->tm_year = -1;
 
-	clk_disable(rtc_clk);
+	clk_disable(info->rtc_clk);
 	return 0;
 }
 
 static int s3c_rtc_setalarm(struct device *dev, struct rtc_wkalrm *alrm)
 {
+	struct s3c_rtc *info = dev_get_drvdata(dev);
 	struct rtc_time *tm = &alrm->time;
-	void __iomem *base = s3c_rtc_base;
 	unsigned int alrm_en;
 
-	clk_enable(rtc_clk);
+	if (!info->base)
+		return -EINVAL;
+
+	clk_enable(info->rtc_clk);
 	dev_dbg(dev, "s3c_rtc_setalarm: %d, %04d.%02d.%02d %02d:%02d:%02d\n",
 		 alrm->enabled,
 		 1900 + tm->tm_year, tm->tm_mon + 1, tm->tm_mday,
 		 tm->tm_hour, tm->tm_min, tm->tm_sec);
 
-	alrm_en = readb(base + S3C2410_RTCALM) & S3C2410_RTCALM_ALMEN;
-	writeb(0x00, base + S3C2410_RTCALM);
+	alrm_en = readb(info->base + S3C2410_RTCALM) & S3C2410_RTCALM_ALMEN;
+	writeb(0x00, info->base + S3C2410_RTCALM);
 
 	if (tm->tm_sec < 60 && tm->tm_sec >= 0) {
 		alrm_en |= S3C2410_RTCALM_SECEN;
-		writeb(bin2bcd(tm->tm_sec), base + S3C2410_ALMSEC);
+		writeb(bin2bcd(tm->tm_sec), info->base + S3C2410_ALMSEC);
 	}
 
 	if (tm->tm_min < 60 && tm->tm_min >= 0) {
 		alrm_en |= S3C2410_RTCALM_MINEN;
-		writeb(bin2bcd(tm->tm_min), base + S3C2410_ALMMIN);
+		writeb(bin2bcd(tm->tm_min), info->base + S3C2410_ALMMIN);
 	}
 
 	if (tm->tm_hour < 24 && tm->tm_hour >= 0) {
 		alrm_en |= S3C2410_RTCALM_HOUREN;
-		writeb(bin2bcd(tm->tm_hour), base + S3C2410_ALMHOUR);
+		writeb(bin2bcd(tm->tm_hour), info->base + S3C2410_ALMHOUR);
 	}
 
 	dev_dbg(dev, "setting S3C2410_RTCALM to %08x\n", alrm_en);
 
-	writeb(alrm_en, base + S3C2410_RTCALM);
+	writeb(alrm_en, info->base + S3C2410_RTCALM);
 
 	s3c_rtc_setaie(dev, alrm->enabled);
 
-	clk_disable(rtc_clk);
+	clk_disable(info->rtc_clk);
+
 	return 0;
 }
 
 static int s3c_rtc_proc(struct device *dev, struct seq_file *seq)
 {
+	struct s3c_rtc *info = dev_get_drvdata(dev);
 	unsigned int ticnt;
 
-	clk_enable(rtc_clk);
-	if (s3c_rtc_cpu_type == TYPE_S3C64XX) {
-		ticnt = readw(s3c_rtc_base + S3C2410_RTCCON);
+	if (!info->base)
+		return -EINVAL;
+
+	clk_enable(info->rtc_clk);
+	if (info->cpu_type == TYPE_S3C64XX) {
+		ticnt = readw(info->base + S3C2410_RTCCON);
 		ticnt &= S3C64XX_RTCCON_TICEN;
 	} else {
-		ticnt = readb(s3c_rtc_base + S3C2410_TICNT);
+		ticnt = readb(info->base + S3C2410_TICNT);
 		ticnt &= S3C2410_TICNT_ENABLE;
 	}
 
 	seq_printf(seq, "periodic_IRQ\t: %s\n", ticnt  ? "yes" : "no");
-	clk_disable(rtc_clk);
+	clk_disable(info->rtc_clk);
 	return 0;
 }
 
@@ -368,63 +398,64 @@ static const struct rtc_class_ops s3c_rtcops = {
 	.alarm_irq_enable = s3c_rtc_setaie,
 };
 
-static void s3c_rtc_enable(struct platform_device *pdev, int en)
+static void s3c_rtc_enable(struct s3c_rtc *info, int en)
 {
-	void __iomem *base = s3c_rtc_base;
 	unsigned int tmp;
 
-	if (s3c_rtc_base == NULL)
+	if (!info->base)
 		return;
 
-	clk_enable(rtc_clk);
+	clk_enable(info->rtc_clk);
 	if (!en) {
-		tmp = readw(base + S3C2410_RTCCON);
-		if (s3c_rtc_cpu_type == TYPE_S3C64XX)
+		tmp = readw(info->base + S3C2410_RTCCON);
+		if (info->cpu_type == TYPE_S3C64XX)
 			tmp &= ~S3C64XX_RTCCON_TICEN;
 		tmp &= ~S3C2410_RTCCON_RTCEN;
-		writew(tmp, base + S3C2410_RTCCON);
+		writew(tmp, info->base + S3C2410_RTCCON);
 
-		if (s3c_rtc_cpu_type != TYPE_S3C64XX) {
-			tmp = readb(base + S3C2410_TICNT);
+		if (info->cpu_type != TYPE_S3C64XX) {
+			tmp = readb(info->base + S3C2410_TICNT);
 			tmp &= ~S3C2410_TICNT_ENABLE;
-			writeb(tmp, base + S3C2410_TICNT);
+			writeb(tmp, info->base + S3C2410_TICNT);
 		}
 	} else {
 		/* re-enable the device, and check it is ok */
 
-		if ((readw(base+S3C2410_RTCCON) & S3C2410_RTCCON_RTCEN) == 0) {
-			dev_info(&pdev->dev, "rtc disabled, re-enabling\n");
+		if ((readw(info->base + S3C2410_RTCCON) & S3C2410_RTCCON_RTCEN) == 0) {
+			dev_info(info->dev, "rtc disabled, re-enabling\n");
 
-			tmp = readw(base + S3C2410_RTCCON);
+			tmp = readw(info->base + S3C2410_RTCCON);
 			writew(tmp | S3C2410_RTCCON_RTCEN,
-				base + S3C2410_RTCCON);
+				info->base + S3C2410_RTCCON);
 		}
 
-		if ((readw(base + S3C2410_RTCCON) & S3C2410_RTCCON_CNTSEL)) {
-			dev_info(&pdev->dev, "removing RTCCON_CNTSEL\n");
+		if ((readw(info->base + S3C2410_RTCCON) & S3C2410_RTCCON_CNTSEL)) {
+			dev_info(info->dev, "removing RTCCON_CNTSEL\n");
 
-			tmp = readw(base + S3C2410_RTCCON);
+			tmp = readw(info->base + S3C2410_RTCCON);
 			writew(tmp & ~S3C2410_RTCCON_CNTSEL,
-				base + S3C2410_RTCCON);
+				info->base + S3C2410_RTCCON);
 		}
 
-		if ((readw(base + S3C2410_RTCCON) & S3C2410_RTCCON_CLKRST)) {
-			dev_info(&pdev->dev, "removing RTCCON_CLKRST\n");
+		if ((readw(info->base + S3C2410_RTCCON) & S3C2410_RTCCON_CLKRST)) {
+			dev_info(info->dev, "removing RTCCON_CLKRST\n");
 
-			tmp = readw(base + S3C2410_RTCCON);
+			tmp = readw(info->base + S3C2410_RTCCON);
 			writew(tmp & ~S3C2410_RTCCON_CLKRST,
-				base + S3C2410_RTCCON);
+				info->base + S3C2410_RTCCON);
 		}
 	}
-	clk_disable(rtc_clk);
+	clk_disable(info->rtc_clk);
 }
 
-static int s3c_rtc_remove(struct platform_device *dev)
+static int s3c_rtc_remove(struct platform_device *pdev)
 {
-	s3c_rtc_setaie(&dev->dev, 0);
+	struct s3c_rtc *info = platform_get_drvdata(pdev);
 
-	clk_unprepare(rtc_clk);
-	rtc_clk = NULL;
+	s3c_rtc_setaie(info->dev, 0);
+
+	clk_unprepare(info->rtc_clk);
+	info->rtc_clk = NULL;
 
 	return 0;
 }
@@ -447,73 +478,85 @@ static inline int s3c_rtc_get_driver_data(struct platform_device *pdev)
 
 static int s3c_rtc_probe(struct platform_device *pdev)
 {
-	struct rtc_device *rtc;
+	struct s3c_rtc *info = NULL;
 	struct rtc_time rtc_tm;
 	struct resource *res;
 	int ret;
 	int tmp;
 
-	dev_dbg(&pdev->dev, "%s: probe=%p\n", __func__, pdev);
+	info = devm_kzalloc(&pdev->dev, sizeof(*info), GFP_KERNEL);
+	if (!info)
+		return -ENOMEM;
 
 	/* find the IRQs */
-
-	s3c_rtc_tickno = platform_get_irq(pdev, 1);
-	if (s3c_rtc_tickno < 0) {
+	info->irq_tick = platform_get_irq(pdev, 1);
+	if (info->irq_tick < 0) {
 		dev_err(&pdev->dev, "no irq for rtc tick\n");
-		return s3c_rtc_tickno;
+		return info->irq_tick;
 	}
 
-	s3c_rtc_alarmno = platform_get_irq(pdev, 0);
-	if (s3c_rtc_alarmno < 0) {
+	info->dev = &pdev->dev;
+	info->cpu_type = s3c_rtc_get_driver_data(pdev);
+	spin_lock_init(&info->pie_lock);
+	spin_lock_init(&info->alarm_clk_lock);
+
+	platform_set_drvdata(pdev, info);
+
+	info->irq_alarm = platform_get_irq(pdev, 0);
+	if (info->irq_alarm < 0) {
 		dev_err(&pdev->dev, "no irq for alarm\n");
-		return s3c_rtc_alarmno;
+		return info->irq_alarm;
 	}
 
 	dev_dbg(&pdev->dev, "s3c2410_rtc: tick irq %d, alarm irq %d\n",
-		 s3c_rtc_tickno, s3c_rtc_alarmno);
+		 info->irq_tick, info->irq_alarm);
 
 	/* get the memory region */
-
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	s3c_rtc_base = devm_ioremap_resource(&pdev->dev, res);
-	if (IS_ERR(s3c_rtc_base))
-		return PTR_ERR(s3c_rtc_base);
+	info->base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(info->base))
+		return PTR_ERR(info->base);
 
-	rtc_clk = devm_clk_get(&pdev->dev, "rtc");
-	if (IS_ERR(rtc_clk)) {
+	info->rtc_clk = devm_clk_get(&pdev->dev, "rtc");
+	if (IS_ERR(info->rtc_clk)) {
 		dev_err(&pdev->dev, "failed to find rtc clock source\n");
-		ret = PTR_ERR(rtc_clk);
-		rtc_clk = NULL;
-		return ret;
+		return PTR_ERR(info->rtc_clk);
 	}
-
-	clk_prepare_enable(rtc_clk);
+	clk_prepare_enable(info->rtc_clk);
 
 	/* check to see if everything is setup correctly */
-
-	s3c_rtc_enable(pdev, 1);
+	s3c_rtc_enable(info, 1);
 
 	dev_dbg(&pdev->dev, "s3c2410_rtc: RTCCON=%02x\n",
-		 readw(s3c_rtc_base + S3C2410_RTCCON));
+		 readw(info->base + S3C2410_RTCCON));
 
 	device_init_wakeup(&pdev->dev, 1);
 
 	/* register RTC and exit */
-
-	rtc = devm_rtc_device_register(&pdev->dev, "s3c", &s3c_rtcops,
+	info->rtc = devm_rtc_device_register(&pdev->dev, "s3c", &s3c_rtcops,
 				  THIS_MODULE);
-
-	if (IS_ERR(rtc)) {
+	if (IS_ERR(info->rtc)) {
 		dev_err(&pdev->dev, "cannot attach rtc\n");
-		ret = PTR_ERR(rtc);
+		ret = PTR_ERR(info->rtc);
 		goto err_nortc;
 	}
 
-	s3c_rtc_cpu_type = s3c_rtc_get_driver_data(pdev);
+	ret = devm_request_irq(&pdev->dev, info->irq_alarm, s3c_rtc_alarmirq,
+			  0,  "s3c2410-rtc alarm", info);
+	if (ret) {
+		dev_err(&pdev->dev, "IRQ%d error %d\n", info->irq_alarm, ret);
+		goto err_nortc;
+	}
 
-	/* Check RTC Time */
+	ret = devm_request_irq(&pdev->dev, info->irq_tick, s3c_rtc_tickirq,
+			  0,  "s3c2410-rtc tick", info);
+	if (ret) {
+		dev_err(&pdev->dev, "IRQ%d error %d\n", info->irq_tick, ret);
+		goto err_nortc;
+	}
 
-	s3c_rtc_gettime(NULL, &rtc_tm);
+	/* Check RTC Time */
+	s3c_rtc_gettime(&pdev->dev, &rtc_tm);
 
 	if (rtc_valid_tm(&rtc_tm)) {
 		rtc_tm.tm_year	= 100;
@@ -523,111 +566,90 @@ static int s3c_rtc_probe(struct platform_device *pdev)
 		rtc_tm.tm_min	= 0;
 		rtc_tm.tm_sec	= 0;
 
-		s3c_rtc_settime(NULL, &rtc_tm);
+		s3c_rtc_settime(&pdev->dev, &rtc_tm);
 
 		dev_warn(&pdev->dev, "warning: invalid RTC value so initializing it\n");
 	}
 
-	if (s3c_rtc_cpu_type != TYPE_S3C2410)
-		rtc->max_user_freq = 32768;
+	if (info->cpu_type != TYPE_S3C2410)
+		info->rtc->max_user_freq = 32768;
 	else
-		rtc->max_user_freq = 128;
+		info->rtc->max_user_freq = 128;
 
-	if (s3c_rtc_cpu_type == TYPE_S3C2416 || s3c_rtc_cpu_type == TYPE_S3C2443) {
-		tmp = readw(s3c_rtc_base + S3C2410_RTCCON);
+	if (info->cpu_type == TYPE_S3C2416 || info->cpu_type == TYPE_S3C2443) {
+		tmp = readw(info->base + S3C2410_RTCCON);
 		tmp |= S3C2443_RTCCON_TICSEL;
-		writew(tmp, s3c_rtc_base + S3C2410_RTCCON);
+		writew(tmp, info->base + S3C2410_RTCCON);
 	}
 
-	platform_set_drvdata(pdev, rtc);
+	s3c_rtc_setfreq(info, 1);
 
-	s3c_rtc_setfreq(&pdev->dev, 1);
-
-	ret = devm_request_irq(&pdev->dev, s3c_rtc_alarmno, s3c_rtc_alarmirq,
-			  0,  "s3c2410-rtc alarm", rtc);
-	if (ret) {
-		dev_err(&pdev->dev, "IRQ%d error %d\n", s3c_rtc_alarmno, ret);
-		goto err_nortc;
-	}
-
-	ret = devm_request_irq(&pdev->dev, s3c_rtc_tickno, s3c_rtc_tickirq,
-			  0,  "s3c2410-rtc tick", rtc);
-	if (ret) {
-		dev_err(&pdev->dev, "IRQ%d error %d\n", s3c_rtc_tickno, ret);
-		goto err_nortc;
-	}
-
-	clk_disable(rtc_clk);
+	clk_disable(info->rtc_clk);
 
 	return 0;
 
  err_nortc:
-	s3c_rtc_enable(pdev, 0);
-	clk_disable_unprepare(rtc_clk);
+	s3c_rtc_enable(info, 0);
+	clk_disable_unprepare(info->rtc_clk);
 
 	return ret;
 }
 
 #ifdef CONFIG_PM_SLEEP
-/* RTC Power management control */
-
-static int ticnt_save, ticnt_en_save;
-static bool wake_en;
 
 static int s3c_rtc_suspend(struct device *dev)
 {
-	struct platform_device *pdev = to_platform_device(dev);
+	struct s3c_rtc *info = dev_get_drvdata(dev);
 
-	clk_enable(rtc_clk);
+	clk_enable(info->rtc_clk);
 	/* save TICNT for anyone using periodic interrupts */
-	if (s3c_rtc_cpu_type == TYPE_S3C64XX) {
-		ticnt_en_save = readw(s3c_rtc_base + S3C2410_RTCCON);
-		ticnt_en_save &= S3C64XX_RTCCON_TICEN;
-		ticnt_save = readl(s3c_rtc_base + S3C2410_TICNT);
+	if (info->cpu_type == TYPE_S3C64XX) {
+		info->ticnt_en_save = readw(info->base + S3C2410_RTCCON);
+		info->ticnt_en_save &= S3C64XX_RTCCON_TICEN;
+		info->ticnt_save = readl(info->base + S3C2410_TICNT);
 	} else {
-		ticnt_save = readb(s3c_rtc_base + S3C2410_TICNT);
+		info->ticnt_save = readb(info->base + S3C2410_TICNT);
 	}
-	s3c_rtc_enable(pdev, 0);
+	s3c_rtc_enable(info, 0);
 
-	if (device_may_wakeup(dev) && !wake_en) {
-		if (enable_irq_wake(s3c_rtc_alarmno) == 0)
-			wake_en = true;
+	if (device_may_wakeup(dev) && !info->wake_en) {
+		if (enable_irq_wake(info->irq_alarm) == 0)
+			info->wake_en = true;
 		else
 			dev_err(dev, "enable_irq_wake failed\n");
 	}
-	clk_disable(rtc_clk);
+	clk_disable(info->rtc_clk);
 
 	return 0;
 }
 
 static int s3c_rtc_resume(struct device *dev)
 {
-	struct platform_device *pdev = to_platform_device(dev);
+	struct s3c_rtc *info = dev_get_drvdata(dev);
 	unsigned int tmp;
 
-	clk_enable(rtc_clk);
-	s3c_rtc_enable(pdev, 1);
-	if (s3c_rtc_cpu_type == TYPE_S3C64XX) {
-		writel(ticnt_save, s3c_rtc_base + S3C2410_TICNT);
-		if (ticnt_en_save) {
-			tmp = readw(s3c_rtc_base + S3C2410_RTCCON);
-			writew(tmp | ticnt_en_save,
-					s3c_rtc_base + S3C2410_RTCCON);
+	clk_enable(info->rtc_clk);
+	s3c_rtc_enable(info, 1);
+	if (info->cpu_type == TYPE_S3C64XX) {
+		writel(info->ticnt_save, info->base + S3C2410_TICNT);
+		if (info->ticnt_en_save) {
+			tmp = readw(info->base + S3C2410_RTCCON);
+			writew(tmp | info->ticnt_en_save,
+					info->base + S3C2410_RTCCON);
 		}
 	} else {
-		writeb(ticnt_save, s3c_rtc_base + S3C2410_TICNT);
+		writeb(info->ticnt_save, info->base + S3C2410_TICNT);
 	}
 
-	if (device_may_wakeup(dev) && wake_en) {
-		disable_irq_wake(s3c_rtc_alarmno);
-		wake_en = false;
+	if (device_may_wakeup(dev) && info->wake_en) {
+		disable_irq_wake(info->irq_alarm);
+		info->wake_en = false;
 	}
-	clk_disable(rtc_clk);
+	clk_disable(info->rtc_clk);
 
 	return 0;
 }
 #endif
-
 static SIMPLE_DEV_PM_OPS(s3c_rtc_pm_ops, s3c_rtc_suspend, s3c_rtc_resume);
 
 #ifdef CONFIG_OF
-- 
1.8.0

[PATCHv2 2/5] rtc: s3c: Remove warning message when checking coding style with checkpatch script

[y]

From: Chanwoo Choi <cw00.choi@samsung.com>

This patch remove warning message when checking codeing style with checkpatch
script and reduce un-necessary i2c read operation on s3c_rtc_enable.

	WARNING: line over 80 characters
	#406: FILE: drivers/rtc/rtc-s3c.c:406:
	+		if ((readw(info->base + S3C2410_RTCCON) & S3C2410_RTCCON_RTCEN) == 0) {

	WARNING: line over 80 characters
	#414: FILE: drivers/rtc/rtc-s3c.c:414:
	+		if ((readw(info->base + S3C2410_RTCCON) & S3C2410_RTCCON_CNTSEL)) {

	WARNING: line over 80 characters
	#422: FILE: drivers/rtc/rtc-s3c.c:422:
	+		if ((readw(info->base + S3C2410_RTCCON) & S3C2410_RTCCON_CLKRST)) {

	WARNING: Missing a blank line after declarations
	#451: FILE: drivers/rtc/rtc-s3c.c:451:
	+	struct s3c_rtc_drv_data *data;
	+	if (pdev->dev.of_node) {

	WARNING: Missing a blank line after declarations
	#453: FILE: drivers/rtc/rtc-s3c.c:453:
	+		const struct of_device_id *match;
	+		match = of_match_node(s3c_rtc_dt_match, pdev->dev.of_node);

	WARNING: DT compatible string "samsung,s3c2416-rtc" appears un-documented -- check ./Documentation/devicetree/bindings/
	#650: FILE: drivers/rtc/rtc-s3c.c:650:
	+		.compatible = "samsung,s3c2416-rtc",

	WARNING: DT compatible string "samsung,s3c2443-rtc" appears un-documented -- check ./Documentation/devicetree/bindings/
	#653: FILE: drivers/rtc/rtc-s3c.c:653:
	+		.compatible = "samsung,s3c2443-rtc",

Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 Documentation/devicetree/bindings/rtc/s3c-rtc.txt |  2 ++
 drivers/rtc/rtc-s3c.c                             | 26 ++++++++++++-----------
 2 files changed, 16 insertions(+), 12 deletions(-)

diff --git a/Documentation/devicetree/bindings/rtc/s3c-rtc.txt b/Documentation/devicetree/bindings/rtc/s3c-rtc.txt
index 7ac7259..06db446 100644
--- a/Documentation/devicetree/bindings/rtc/s3c-rtc.txt
+++ b/Documentation/devicetree/bindings/rtc/s3c-rtc.txt
@@ -3,6 +3,8 @@
 Required properties:
 - compatible: should be one of the following.
     * "samsung,s3c2410-rtc" - for controllers compatible with s3c2410 rtc.
+    * "samsung,s3c2416-rtc" - for controllers compatible with s3c2416 rtc.
+    * "samsung,s3c2443-rtc" - for controllers compatible with s3c2443 rtc.
     * "samsung,s3c6410-rtc" - for controllers compatible with s3c6410 rtc.
 - reg: physical base address of the controller and length of memory mapped
   region.
diff --git a/drivers/rtc/rtc-s3c.c b/drivers/rtc/rtc-s3c.c
index 1d9e158..d8f25bd 100644
--- a/drivers/rtc/rtc-s3c.c
+++ b/drivers/rtc/rtc-s3c.c
@@ -400,28 +400,28 @@ static const struct rtc_class_ops s3c_rtcops = {
 
 static void s3c_rtc_enable(struct s3c_rtc *info, int en)
 {
-	unsigned int tmp;
+	unsigned int con, tmp;
 
 	if (!info->base)
 		return;
 
 	clk_enable(info->rtc_clk);
+
+	con = readw(info->base + S3C2410_RTCCON);
 	if (!en) {
-		tmp = readw(info->base + S3C2410_RTCCON);
 		if (info->cpu_type == TYPE_S3C64XX)
-			tmp &= ~S3C64XX_RTCCON_TICEN;
-		tmp &= ~S3C2410_RTCCON_RTCEN;
-		writew(tmp, info->base + S3C2410_RTCCON);
+			con &= ~S3C64XX_RTCCON_TICEN;
+		con &= ~S3C2410_RTCCON_RTCEN;
+		writew(con, info->base + S3C2410_RTCCON);
 
 		if (info->cpu_type != TYPE_S3C64XX) {
-			tmp = readb(info->base + S3C2410_TICNT);
-			tmp &= ~S3C2410_TICNT_ENABLE;
-			writeb(tmp, info->base + S3C2410_TICNT);
+			con = readb(info->base + S3C2410_TICNT);
+			con &= ~S3C2410_TICNT_ENABLE;
+			writeb(con, info->base + S3C2410_TICNT);
 		}
 	} else {
 		/* re-enable the device, and check it is ok */
-
-		if ((readw(info->base + S3C2410_RTCCON) & S3C2410_RTCCON_RTCEN) == 0) {
+		if ((con & S3C2410_RTCCON_RTCEN) == 0) {
 			dev_info(info->dev, "rtc disabled, re-enabling\n");
 
 			tmp = readw(info->base + S3C2410_RTCCON);
@@ -429,7 +429,7 @@ static void s3c_rtc_enable(struct s3c_rtc *info, int en)
 				info->base + S3C2410_RTCCON);
 		}
 
-		if ((readw(info->base + S3C2410_RTCCON) & S3C2410_RTCCON_CNTSEL)) {
+		if (con & S3C2410_RTCCON_CNTSEL) {
 			dev_info(info->dev, "removing RTCCON_CNTSEL\n");
 
 			tmp = readw(info->base + S3C2410_RTCCON);
@@ -437,7 +437,7 @@ static void s3c_rtc_enable(struct s3c_rtc *info, int en)
 				info->base + S3C2410_RTCCON);
 		}
 
-		if ((readw(info->base + S3C2410_RTCCON) & S3C2410_RTCCON_CLKRST)) {
+		if (con & S3C2410_RTCCON_CLKRST) {
 			dev_info(info->dev, "removing RTCCON_CLKRST\n");
 
 			tmp = readw(info->base + S3C2410_RTCCON);
@@ -466,8 +466,10 @@ static inline int s3c_rtc_get_driver_data(struct platform_device *pdev)
 {
 #ifdef CONFIG_OF
 	struct s3c_rtc_drv_data *data;
+
 	if (pdev->dev.of_node) {
 		const struct of_device_id *match;
+
 		match = of_match_node(s3c_rtc_dt_match, pdev->dev.of_node);
 		data = (struct s3c_rtc_drv_data *) match->data;
 		return data->cpu_type;
-- 
1.8.0

[PATCHv2 3/5] rtc: s3c: Add s3c_rtc_data structure to use variant data instead of s3c_cpu_type

[y]

From: Chanwoo Choi <cw00.choi@samsung.com>

This patch add s3c_rtc_data structure to variant data according to SoC type.
The s3c_rtc_data structure includes some functions to control RTC operation
and specific data dependent on SoC type.

Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/rtc/rtc-s3c.c | 464 +++++++++++++++++++++++++++++++-------------------
 1 file changed, 289 insertions(+), 175 deletions(-)

diff --git a/drivers/rtc/rtc-s3c.c b/drivers/rtc/rtc-s3c.c
index d8f25bd..26a88f9 100644
--- a/drivers/rtc/rtc-s3c.c
+++ b/drivers/rtc/rtc-s3c.c
@@ -32,17 +32,6 @@
 #include <asm/irq.h>
 #include "rtc-s3c.h"
 
-enum s3c_cpu_type {
-	TYPE_S3C2410,
-	TYPE_S3C2416,
-	TYPE_S3C2443,
-	TYPE_S3C64XX,
-};
-
-struct s3c_rtc_drv_data {
-	int cpu_type;
-};
-
 struct s3c_rtc {
 	struct device *dev;
 	struct rtc_device *rtc;
@@ -51,7 +40,7 @@ struct s3c_rtc {
 	struct clk *rtc_clk;
 	bool enabled;
 
-	enum s3c_cpu_type cpu_type;
+	struct s3c_rtc_data *data;
 
 	int irq_alarm;
 	int irq_tick;
@@ -63,6 +52,19 @@ struct s3c_rtc {
 	bool wake_en;
 };
 
+struct s3c_rtc_data {
+	int max_user_freq;
+
+	void (*irq_handler) (struct s3c_rtc *info, int mask);
+	void (*set_freq) (struct s3c_rtc *info, int freq);
+	void (*enable_tick) (struct s3c_rtc *info, struct seq_file *seq);
+	void (*select_tick_clk) (struct s3c_rtc *info);
+	void (*save_tick_cnt) (struct s3c_rtc *info);
+	void (*restore_tick_cnt) (struct s3c_rtc *info);
+	void (*enable) (struct s3c_rtc *info);
+	void (*disable) (struct s3c_rtc *info);
+};
+
 static void s3c_rtc_alarm_clk_enable(struct s3c_rtc *info, bool enable)
 {
 	unsigned long irq_flags;
@@ -83,34 +85,22 @@ static void s3c_rtc_alarm_clk_enable(struct s3c_rtc *info, bool enable)
 }
 
 /* IRQ Handlers */
-static irqreturn_t s3c_rtc_alarmirq(int irq, void *id)
+static irqreturn_t s3c_rtc_tickirq(int irq, void *id)
 {
 	struct s3c_rtc *info = (struct s3c_rtc *)id;
 
-	clk_enable(info->rtc_clk);
-	rtc_update_irq(info->rtc, 1, RTC_AF | RTC_IRQF);
-
-	if (info->cpu_type == TYPE_S3C64XX)
-		writeb(S3C2410_INTP_ALM, info->base + S3C2410_INTP);
-
-	clk_disable(info->rtc_clk);
-
-	s3c_rtc_alarm_clk_enable(info, false);
+	if (info->data->irq_handler)
+		info->data->irq_handler(info, S3C2410_INTP_TIC);
 
 	return IRQ_HANDLED;
 }
 
-static irqreturn_t s3c_rtc_tickirq(int irq, void *id)
+static irqreturn_t s3c_rtc_alarmirq(int irq, void *id)
 {
 	struct s3c_rtc *info = (struct s3c_rtc *)id;
 
-	clk_enable(info->rtc_clk);
-	rtc_update_irq(info->rtc, 1, RTC_PF | RTC_IRQF);
-
-	if (info->cpu_type == TYPE_S3C64XX)
-		writeb(S3C2410_INTP_TIC, info->base + S3C2410_INTP);
-
-	clk_disable(info->rtc_clk);
+	if (info->data->irq_handler)
+		info->data->irq_handler(info, S3C2410_INTP_ALM);
 
 	return IRQ_HANDLED;
 }
@@ -140,36 +130,18 @@ static int s3c_rtc_setaie(struct device *dev, unsigned int enabled)
 	return 0;
 }
 
+/* Set RTC frequency */
 static int s3c_rtc_setfreq(struct s3c_rtc *info, int freq)
 {
-	unsigned int tmp = 0;
-	int val;
-
 	if (!is_power_of_2(freq))
 		return -EINVAL;
 
 	clk_enable(info->rtc_clk);
 	spin_lock_irq(&info->pie_lock);
 
-	if (info->cpu_type != TYPE_S3C64XX) {
-		tmp = readb(info->base + S3C2410_TICNT);
-		tmp &= S3C2410_TICNT_ENABLE;
-	}
-
-	val = (info->rtc->max_user_freq / freq) - 1;
-
-	if (info->cpu_type == TYPE_S3C2416 || info->cpu_type == TYPE_S3C2443) {
-		tmp |= S3C2443_TICNT_PART(val);
-		writel(S3C2443_TICNT1_PART(val), info->base + S3C2443_TICNT1);
+	if (info->data->set_freq)
+		info->data->set_freq(info, freq);
 
-		if (info->cpu_type == TYPE_S3C2416)
-			writel(S3C2416_TICNT2_PART(val),
-				info->base + S3C2416_TICNT2);
-	} else {
-		tmp |= val;
-	}
-
-	writel(tmp, info->base + S3C2410_TICNT);
 	spin_unlock_irq(&info->pie_lock);
 	clk_disable(info->rtc_clk);
 
@@ -177,7 +149,6 @@ static int s3c_rtc_setfreq(struct s3c_rtc *info, int freq)
 }
 
 /* Time read/write */
-
 static int s3c_rtc_gettime(struct device *dev, struct rtc_time *rtc_tm)
 {
 	struct s3c_rtc *info = dev_get_drvdata(dev);
@@ -370,22 +341,17 @@ static int s3c_rtc_setalarm(struct device *dev, struct rtc_wkalrm *alrm)
 static int s3c_rtc_proc(struct device *dev, struct seq_file *seq)
 {
 	struct s3c_rtc *info = dev_get_drvdata(dev);
-	unsigned int ticnt;
 
 	if (!info->base)
 		return -EINVAL;
 
 	clk_enable(info->rtc_clk);
-	if (info->cpu_type == TYPE_S3C64XX) {
-		ticnt = readw(info->base + S3C2410_RTCCON);
-		ticnt &= S3C64XX_RTCCON_TICEN;
-	} else {
-		ticnt = readb(info->base + S3C2410_TICNT);
-		ticnt &= S3C2410_TICNT_ENABLE;
-	}
 
-	seq_printf(seq, "periodic_IRQ\t: %s\n", ticnt  ? "yes" : "no");
+	if (info->data->enable_tick)
+		info->data->enable_tick(info, seq);
+
 	clk_disable(info->rtc_clk);
+
 	return 0;
 }
 
@@ -398,53 +364,69 @@ static const struct rtc_class_ops s3c_rtcops = {
 	.alarm_irq_enable = s3c_rtc_setaie,
 };
 
-static void s3c_rtc_enable(struct s3c_rtc *info, int en)
+static void s3c24xx_rtc_enable(struct s3c_rtc *info)
 {
 	unsigned int con, tmp;
 
-	if (!info->base)
-		return;
-
 	clk_enable(info->rtc_clk);
 
 	con = readw(info->base + S3C2410_RTCCON);
-	if (!en) {
-		if (info->cpu_type == TYPE_S3C64XX)
-			con &= ~S3C64XX_RTCCON_TICEN;
-		con &= ~S3C2410_RTCCON_RTCEN;
-		writew(con, info->base + S3C2410_RTCCON);
-
-		if (info->cpu_type != TYPE_S3C64XX) {
-			con = readb(info->base + S3C2410_TICNT);
-			con &= ~S3C2410_TICNT_ENABLE;
-			writeb(con, info->base + S3C2410_TICNT);
-		}
-	} else {
-		/* re-enable the device, and check it is ok */
-		if ((con & S3C2410_RTCCON_RTCEN) == 0) {
-			dev_info(info->dev, "rtc disabled, re-enabling\n");
+	/* re-enable the device, and check it is ok */
+	if ((con & S3C2410_RTCCON_RTCEN) == 0) {
+		dev_info(info->dev, "rtc disabled, re-enabling\n");
 
-			tmp = readw(info->base + S3C2410_RTCCON);
-			writew(tmp | S3C2410_RTCCON_RTCEN,
-				info->base + S3C2410_RTCCON);
-		}
+		tmp = readw(info->base + S3C2410_RTCCON);
+		writew(tmp | S3C2410_RTCCON_RTCEN,
+			info->base + S3C2410_RTCCON);
+	}
 
-		if (con & S3C2410_RTCCON_CNTSEL) {
-			dev_info(info->dev, "removing RTCCON_CNTSEL\n");
+	if (con & S3C2410_RTCCON_CNTSEL) {
+		dev_info(info->dev, "removing RTCCON_CNTSEL\n");
 
-			tmp = readw(info->base + S3C2410_RTCCON);
-			writew(tmp & ~S3C2410_RTCCON_CNTSEL,
-				info->base + S3C2410_RTCCON);
-		}
+		tmp = readw(info->base + S3C2410_RTCCON);
+		writew(tmp & ~S3C2410_RTCCON_CNTSEL,
+			info->base + S3C2410_RTCCON);
+	}
 
-		if (con & S3C2410_RTCCON_CLKRST) {
-			dev_info(info->dev, "removing RTCCON_CLKRST\n");
+	if (con & S3C2410_RTCCON_CLKRST) {
+		dev_info(info->dev, "removing RTCCON_CLKRST\n");
 
-			tmp = readw(info->base + S3C2410_RTCCON);
-			writew(tmp & ~S3C2410_RTCCON_CLKRST,
-				info->base + S3C2410_RTCCON);
-		}
+		tmp = readw(info->base + S3C2410_RTCCON);
+		writew(tmp & ~S3C2410_RTCCON_CLKRST,
+			info->base + S3C2410_RTCCON);
 	}
+
+	clk_disable(info->rtc_clk);
+}
+
+static void s3c24xx_rtc_disable(struct s3c_rtc *info)
+{
+	unsigned int con;
+
+	clk_enable(info->rtc_clk);
+
+	con = readw(info->base + S3C2410_RTCCON);
+	con &= ~S3C2410_RTCCON_RTCEN;
+	writew(con, info->base + S3C2410_RTCCON);
+
+	con = readb(info->base + S3C2410_TICNT);
+	con &= ~S3C2410_TICNT_ENABLE;
+	writeb(con, info->base + S3C2410_TICNT);
+
+	clk_disable(info->rtc_clk);
+}
+
+static void s3c6410_rtc_disable(struct s3c_rtc *info)
+{
+	unsigned int con;
+
+	clk_enable(info->rtc_clk);
+
+	con = readw(info->base + S3C2410_RTCCON);
+	con &= ~S3C64XX_RTCCON_TICEN;
+	con &= ~S3C2410_RTCCON_RTCEN;
+	writew(con, info->base + S3C2410_RTCCON);
+
 	clk_disable(info->rtc_clk);
 }
 
@@ -462,20 +444,12 @@ static int s3c_rtc_remove(struct platform_device *pdev)
 
 static const struct of_device_id s3c_rtc_dt_match[];
 
-static inline int s3c_rtc_get_driver_data(struct platform_device *pdev)
+static struct s3c_rtc_data *s3c_rtc_get_data(struct platform_device *pdev)
 {
-#ifdef CONFIG_OF
-	struct s3c_rtc_drv_data *data;
-
-	if (pdev->dev.of_node) {
-		const struct of_device_id *match;
+	const struct of_device_id *match;
 
-		match = of_match_node(s3c_rtc_dt_match, pdev->dev.of_node);
-		data = (struct s3c_rtc_drv_data *) match->data;
-		return data->cpu_type;
-	}
-#endif
-	return platform_get_device_id(pdev)->driver_data;
+	match = of_match_node(s3c_rtc_dt_match, pdev->dev.of_node);
+	return (struct s3c_rtc_data *)match->data;
 }
 
 static int s3c_rtc_probe(struct platform_device *pdev)
@@ -484,7 +458,6 @@ static int s3c_rtc_probe(struct platform_device *pdev)
 	struct rtc_time rtc_tm;
 	struct resource *res;
 	int ret;
-	int tmp;
 
 	info = devm_kzalloc(&pdev->dev, sizeof(*info), GFP_KERNEL);
 	if (!info)
@@ -498,7 +471,11 @@ static int s3c_rtc_probe(struct platform_device *pdev)
 	}
 
 	info->dev = &pdev->dev;
-	info->cpu_type = s3c_rtc_get_driver_data(pdev);
+	info->data = s3c_rtc_get_data(pdev);
+	if (!info->data) {
+		dev_err(&pdev->dev, "failed getting s3c_rtc_data\n");
+		return -EINVAL;
+	}
 	spin_lock_init(&info->pie_lock);
 	spin_lock_init(&info->alarm_clk_lock);
 
@@ -527,7 +504,8 @@ static int s3c_rtc_probe(struct platform_device *pdev)
 	clk_prepare_enable(info->rtc_clk);
 
 	/* check to see if everything is setup correctly */
-	s3c_rtc_enable(info, 1);
+	if (info->data->enable)
+		info->data->enable(info);
 
 	dev_dbg(&pdev->dev, "s3c2410_rtc: RTCCON=%02x\n",
 		 readw(info->base + S3C2410_RTCCON));
@@ -573,16 +551,8 @@ static int s3c_rtc_probe(struct platform_device *pdev)
 		dev_warn(&pdev->dev, "warning: invalid RTC value so initializing it\n");
 	}
 
-	if (info->cpu_type != TYPE_S3C2410)
-		info->rtc->max_user_freq = 32768;
-	else
-		info->rtc->max_user_freq = 128;
-
-	if (info->cpu_type == TYPE_S3C2416 || info->cpu_type == TYPE_S3C2443) {
-		tmp = readw(info->base + S3C2410_RTCCON);
-		tmp |= S3C2443_RTCCON_TICSEL;
-		writew(tmp, info->base + S3C2410_RTCCON);
-	}
+	if (info->data->select_tick_clk)
+		info->data->select_tick_clk(info);
 
 	s3c_rtc_setfreq(info, 1);
 
@@ -591,7 +561,8 @@ static int s3c_rtc_probe(struct platform_device *pdev)
 	return 0;
 
  err_nortc:
-	s3c_rtc_enable(info, 0);
+	if (info->data->disable)
+		info->data->disable(info);
 	clk_disable_unprepare(info->rtc_clk);
 
 	return ret;
@@ -604,15 +575,13 @@ static int s3c_rtc_suspend(struct device *dev)
 	struct s3c_rtc *info = dev_get_drvdata(dev);
 
 	clk_enable(info->rtc_clk);
+
 	/* save TICNT for anyone using periodic interrupts */
-	if (info->cpu_type == TYPE_S3C64XX) {
-		info->ticnt_en_save = readw(info->base + S3C2410_RTCCON);
-		info->ticnt_en_save &= S3C64XX_RTCCON_TICEN;
-		info->ticnt_save = readl(info->base + S3C2410_TICNT);
-	} else {
-		info->ticnt_save = readb(info->base + S3C2410_TICNT);
-	}
-	s3c_rtc_enable(info, 0);
+	if (info->data->save_tick_cnt)
+		info->data->save_tick_cnt(info);
+
+	if (info->data->disable)
+		info->data->disable(info);
 
 	if (device_may_wakeup(dev) && !info->wake_en) {
 		if (enable_irq_wake(info->irq_alarm) == 0)
@@ -620,6 +589,7 @@ static int s3c_rtc_suspend(struct device *dev)
 		else
 			dev_err(dev, "enable_irq_wake failed\n");
 	}
+
 	clk_disable(info->rtc_clk);
 
 	return 0;
@@ -628,25 +598,20 @@ static int s3c_rtc_suspend(struct device *dev)
 static int s3c_rtc_resume(struct device *dev)
 {
 	struct s3c_rtc *info = dev_get_drvdata(dev);
-	unsigned int tmp;
 
 	clk_enable(info->rtc_clk);
-	s3c_rtc_enable(info, 1);
-	if (info->cpu_type == TYPE_S3C64XX) {
-		writel(info->ticnt_save, info->base + S3C2410_TICNT);
-		if (info->ticnt_en_save) {
-			tmp = readw(info->base + S3C2410_RTCCON);
-			writew(tmp | info->ticnt_en_save,
-					info->base + S3C2410_RTCCON);
-		}
-	} else {
-		writeb(info->ticnt_save, info->base + S3C2410_TICNT);
-	}
+
+	if (info->data->enable)
+		info->data->enable(info);
+
+	if (info->data->restore_tick_cnt)
+		info->data->restore_tick_cnt(info);
 
 	if (device_may_wakeup(dev) && info->wake_en) {
 		disable_irq_wake(info->irq_alarm);
 		info->wake_en = false;
 	}
+
 	clk_disable(info->rtc_clk);
 
 	return 0;
@@ -654,56 +619,206 @@ static int s3c_rtc_resume(struct device *dev)
 #endif
 static SIMPLE_DEV_PM_OPS(s3c_rtc_pm_ops, s3c_rtc_suspend, s3c_rtc_resume);
 
-#ifdef CONFIG_OF
-static struct s3c_rtc_drv_data s3c_rtc_drv_data_array[] = {
-	[TYPE_S3C2410] = { TYPE_S3C2410 },
-	[TYPE_S3C2416] = { TYPE_S3C2416 },
-	[TYPE_S3C2443] = { TYPE_S3C2443 },
-	[TYPE_S3C64XX] = { TYPE_S3C64XX },
+static void s3c24xx_rtc_irq(struct s3c_rtc *info, int mask)
+{
+	clk_enable(info->rtc_clk);
+	rtc_update_irq(info->rtc, 1, RTC_AF | RTC_IRQF);
+	clk_disable(info->rtc_clk);
+
+	s3c_rtc_alarm_clk_enable(info, false);
+}
+
+static void s3c6410_rtc_irq(struct s3c_rtc *info, int mask)
+{
+	clk_enable(info->rtc_clk);
+	rtc_update_irq(info->rtc, 1, RTC_AF | RTC_IRQF);
+	writeb(mask, info->base + S3C2410_INTP);
+	clk_disable(info->rtc_clk);
+
+	s3c_rtc_alarm_clk_enable(info, false);
+}
+
+static void s3c2410_rtc_setfreq(struct s3c_rtc *info, int freq)
+{
+	unsigned int tmp = 0;
+	int val;
+
+	tmp = readb(info->base + S3C2410_TICNT);
+	tmp &= S3C2410_TICNT_ENABLE;
+
+	val = (info->rtc->max_user_freq / freq) - 1;
+	tmp |= val;
+
+	writel(tmp, info->base + S3C2410_TICNT);
+}
+
+static void s3c2416_rtc_setfreq(struct s3c_rtc *info, int freq)
+{
+	unsigned int tmp = 0;
+	int val;
+
+	tmp = readb(info->base + S3C2410_TICNT);
+	tmp &= S3C2410_TICNT_ENABLE;
+
+	val = (info->rtc->max_user_freq / freq) - 1;
+
+	tmp |= S3C2443_TICNT_PART(val);
+	writel(S3C2443_TICNT1_PART(val), info->base + S3C2443_TICNT1);
+
+	writel(S3C2416_TICNT2_PART(val), info->base + S3C2416_TICNT2);
+
+	writel(tmp, info->base + S3C2410_TICNT);
+}
+
+static void s3c2443_rtc_setfreq(struct s3c_rtc *info, int freq)
+{
+	unsigned int tmp = 0;
+	int val;
+
+	tmp = readb(info->base + S3C2410_TICNT);
+	tmp &= S3C2410_TICNT_ENABLE;
+
+	val = (info->rtc->max_user_freq / freq) - 1;
+
+	tmp |= S3C2443_TICNT_PART(val);
+	writel(S3C2443_TICNT1_PART(val), info->base + S3C2443_TICNT1);
+
+	writel(tmp, info->base + S3C2410_TICNT);
+}
+
+static void s3c6410_rtc_setfreq(struct s3c_rtc *info, int freq)
+{
+	int val;
+
+	val = (info->rtc->max_user_freq / freq) - 1;
+	writel(val, info->base + S3C2410_TICNT);
+}
+
+static void s3c24xx_rtc_enable_tick(struct s3c_rtc *info, struct seq_file *seq)
+{
+	unsigned int ticnt;
+
+	ticnt = readb(info->base + S3C2410_TICNT);
+	ticnt &= S3C2410_TICNT_ENABLE;
+
+	seq_printf(seq, "periodic_IRQ\t: %s\n", ticnt  ? "yes" : "no");
+}
+
+static void s3c2416_rtc_select_tick_clk(struct s3c_rtc *info)
+{
+	unsigned int con;
+
+	con = readw(info->base + S3C2410_RTCCON);
+	con |= S3C2443_RTCCON_TICSEL;
+	writew(con, info->base + S3C2410_RTCCON);
+}
+
+static void s3c6410_rtc_enable_tick(struct s3c_rtc *info, struct seq_file *seq)
+{
+	unsigned int ticnt;
+
+	ticnt = readw(info->base + S3C2410_RTCCON);
+	ticnt &= S3C64XX_RTCCON_TICEN;
+
+	seq_printf(seq, "periodic_IRQ\t: %s\n", ticnt  ? "yes" : "no");
+}
+
+static void s3c24xx_rtc_save_tick_cnt(struct s3c_rtc *info)
+{
+	info->ticnt_save = readb(info->base + S3C2410_TICNT);
+}
+
+static void s3c24xx_rtc_restore_tick_cnt(struct s3c_rtc *info)
+{
+	writeb(info->ticnt_save, info->base + S3C2410_TICNT);
+}
+
+static void s3c6410_rtc_save_tick_cnt(struct s3c_rtc *info)
+{
+	info->ticnt_en_save = readw(info->base + S3C2410_RTCCON);
+	info->ticnt_en_save &= S3C64XX_RTCCON_TICEN;
+	info->ticnt_save = readl(info->base + S3C2410_TICNT);
+}
+
+static void s3c6410_rtc_restore_tick_cnt(struct s3c_rtc *info)
+{
+	unsigned int con;
+
+	writel(info->ticnt_save, info->base + S3C2410_TICNT);
+	if (info->ticnt_en_save) {
+		con = readw(info->base + S3C2410_RTCCON);
+		writew(con | info->ticnt_en_save,
+				info->base + S3C2410_RTCCON);
+	}
+}
+
+static struct s3c_rtc_data const s3c2410_rtc_data = {
+	.max_user_freq		= 128,
+	.irq_handler		= s3c24xx_rtc_irq,
+	.set_freq		= s3c2410_rtc_setfreq,
+	.enable_tick		= s3c24xx_rtc_enable_tick,
+	.save_tick_cnt		= s3c24xx_rtc_save_tick_cnt,
+	.restore_tick_cnt	= s3c24xx_rtc_restore_tick_cnt,
+	.enable			= s3c24xx_rtc_enable,
+	.disable		= s3c24xx_rtc_disable,
+};
+
+static struct s3c_rtc_data const s3c2416_rtc_data = {
+	.max_user_freq		= 32768,
+	.irq_handler		= s3c24xx_rtc_irq,
+	.set_freq		= s3c2416_rtc_setfreq,
+	.enable_tick		= s3c24xx_rtc_enable_tick,
+	.select_tick_clk	= s3c2416_rtc_select_tick_clk,
+	.save_tick_cnt		= s3c24xx_rtc_save_tick_cnt,
+	.restore_tick_cnt	= s3c24xx_rtc_restore_tick_cnt,
+	.enable			= s3c24xx_rtc_enable,
+	.disable		= s3c24xx_rtc_disable,
+};
+
+static struct s3c_rtc_data const s3c2443_rtc_data = {
+	.max_user_freq		= 32768,
+	.irq_handler		= s3c24xx_rtc_irq,
+	.set_freq		= s3c2443_rtc_setfreq,
+	.enable_tick		= s3c24xx_rtc_enable_tick,
+	.select_tick_clk	= s3c2416_rtc_select_tick_clk,
+	.save_tick_cnt		= s3c24xx_rtc_save_tick_cnt,
+	.restore_tick_cnt	= s3c24xx_rtc_restore_tick_cnt,
+	.enable			= s3c24xx_rtc_enable,
+	.disable		= s3c24xx_rtc_disable,
+};
+
+static struct s3c_rtc_data const s3c6410_rtc_data = {
+	.max_user_freq		= 32768,
+	.irq_handler		= s3c6410_rtc_irq,
+	.set_freq		= s3c6410_rtc_setfreq,
+	.enable_tick		= s3c6410_rtc_enable_tick,
+	.save_tick_cnt		= s3c6410_rtc_save_tick_cnt,
+	.restore_tick_cnt	= s3c6410_rtc_restore_tick_cnt,
+	.enable			= s3c24xx_rtc_enable,
+	.disable		= s3c6410_rtc_disable,
 };
 
 static const struct of_device_id s3c_rtc_dt_match[] = {
 	{
 		.compatible = "samsung,s3c2410-rtc",
-		.data = &s3c_rtc_drv_data_array[TYPE_S3C2410],
+		.data = (void *)&s3c2410_rtc_data,
 	}, {
 		.compatible = "samsung,s3c2416-rtc",
-		.data = &s3c_rtc_drv_data_array[TYPE_S3C2416],
+		.data = (void *)&s3c2416_rtc_data,
 	}, {
 		.compatible = "samsung,s3c2443-rtc",
-		.data = &s3c_rtc_drv_data_array[TYPE_S3C2443],
+		.data = (void *)&s3c2443_rtc_data,
 	}, {
 		.compatible = "samsung,s3c6410-rtc",
-		.data = &s3c_rtc_drv_data_array[TYPE_S3C64XX],
+		.data = (void *)&s3c6410_rtc_data,
 	},
-	{},
+	{ /* sentinel */ },
 };
 MODULE_DEVICE_TABLE(of, s3c_rtc_dt_match);
-#endif
-
-static struct platform_device_id s3c_rtc_driver_ids[] = {
-	{
-		.name		= "s3c2410-rtc",
-		.driver_data	= TYPE_S3C2410,
-	}, {
-		.name		= "s3c2416-rtc",
-		.driver_data	= TYPE_S3C2416,
-	}, {
-		.name		= "s3c2443-rtc",
-		.driver_data	= TYPE_S3C2443,
-	}, {
-		.name		= "s3c64xx-rtc",
-		.driver_data	= TYPE_S3C64XX,
-	},
-	{ }
-};
-
-MODULE_DEVICE_TABLE(platform, s3c_rtc_driver_ids);
 
 static struct platform_driver s3c_rtc_driver = {
 	.probe		= s3c_rtc_probe,
 	.remove		= s3c_rtc_remove,
-	.id_table	= s3c_rtc_driver_ids,
 	.driver		= {
 		.name	= "s3c-rtc",
 		.owner	= THIS_MODULE,
@@ -711,7 +826,6 @@ static struct platform_driver s3c_rtc_driver = {
 		.of_match_table	= of_match_ptr(s3c_rtc_dt_match),
 	},
 };
-
 module_platform_driver(s3c_rtc_driver);
 
 MODULE_DESCRIPTION("Samsung S3C RTC Driver");
-- 
1.8.0

[PATCHv2 4/5] rtc: s3c: Add support for RTC of Exynos3250 SoC

[y]

From: Chanwoo Choi <cw00.choi@samsung.com>

This patch add support for RTC of Exynos3250 SoC. The Exynos3250 needs source
clock(32.768KHz) for RTC block. If source clock of RTC is registerd on clock
list of common clk framework, Exynos RTC drvier have to control this clock.

Clock list for s3c-rtc device:
- rtc : CLK_RTC of CLK_GATE_IP_PERIR is gate clock for RTC.
- rtc_src : XrtcXTI is 32.768.kHz source clock for RTC.
 (XRTCXTI: Specifies a clock from 32.768 kHz crystal pad with XRTCXTI and
 XRTCXTO pins. RTC uses this clock as the source of a real-time clock.)

Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 Documentation/devicetree/bindings/rtc/s3c-rtc.txt |  1 +
 drivers/rtc/rtc-s3c.c                             | 93 ++++++++++++++++++++++-
 2 files changed, 93 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/rtc/s3c-rtc.txt b/Documentation/devicetree/bindings/rtc/s3c-rtc.txt
index 06db446..ab757b84 100644
--- a/Documentation/devicetree/bindings/rtc/s3c-rtc.txt
+++ b/Documentation/devicetree/bindings/rtc/s3c-rtc.txt
@@ -6,6 +6,7 @@ Required properties:
     * "samsung,s3c2416-rtc" - for controllers compatible with s3c2416 rtc.
     * "samsung,s3c2443-rtc" - for controllers compatible with s3c2443 rtc.
     * "samsung,s3c6410-rtc" - for controllers compatible with s3c6410 rtc.
+    * "samsung,exynos3250-rtc" - for controllers compatible with exynos3250 rtc.
 - reg: physical base address of the controller and length of memory mapped
   region.
 - interrupts: Two interrupt numbers to the cpu should be specified. First
diff --git a/drivers/rtc/rtc-s3c.c b/drivers/rtc/rtc-s3c.c
index 26a88f9..90dcf51 100644
--- a/drivers/rtc/rtc-s3c.c
+++ b/drivers/rtc/rtc-s3c.c
@@ -38,6 +38,7 @@ struct s3c_rtc {
 
 	void __iomem *base;
 	struct clk *rtc_clk;
+	struct clk *rtc_src_clk;
 	bool enabled;
 
 	struct s3c_rtc_data *data;
@@ -54,6 +55,7 @@ struct s3c_rtc {
 
 struct s3c_rtc_data {
 	int max_user_freq;
+	bool needs_src_clk;
 
 	void (*irq_handler) (struct s3c_rtc *info, int mask);
 	void (*set_freq) (struct s3c_rtc *info, int freq);
@@ -73,10 +75,14 @@ static void s3c_rtc_alarm_clk_enable(struct s3c_rtc *info, bool enable)
 	if (enable) {
 		if (!info->enabled) {
 			clk_enable(info->rtc_clk);
+			if (info->data->needs_src_clk)
+				clk_enable(info->rtc_src_clk);
 			info->enabled = true;
 		}
 	} else {
 		if (info->enabled) {
+			if (info->data->needs_src_clk)
+				clk_disable(info->rtc_src_clk);
 			clk_disable(info->rtc_clk);
 			info->enabled = false;
 		}
@@ -117,12 +123,16 @@ static int s3c_rtc_setaie(struct device *dev, unsigned int enabled)
 	dev_dbg(info->dev, "%s: aie=%d\n", __func__, enabled);
 
 	clk_enable(info->rtc_clk);
+	if (info->data->needs_src_clk)
+		clk_enable(info->rtc_src_clk);
 	tmp = readb(info->base + S3C2410_RTCALM) & ~S3C2410_RTCALM_ALMEN;
 
 	if (enabled)
 		tmp |= S3C2410_RTCALM_ALMEN;
 
 	writeb(tmp, info->base + S3C2410_RTCALM);
+	if (info->data->needs_src_clk)
+		clk_disable(info->rtc_src_clk);
 	clk_disable(info->rtc_clk);
 
 	s3c_rtc_alarm_clk_enable(info, enabled);
@@ -137,12 +147,16 @@ static int s3c_rtc_setfreq(struct s3c_rtc *info, int freq)
 		return -EINVAL;
 
 	clk_enable(info->rtc_clk);
+	if (info->data->needs_src_clk)
+		clk_enable(info->rtc_src_clk);
 	spin_lock_irq(&info->pie_lock);
 
 	if (info->data->set_freq)
 		info->data->set_freq(info, freq);
 
 	spin_unlock_irq(&info->pie_lock);
+	if (info->data->needs_src_clk)
+		clk_disable(info->rtc_src_clk);
 	clk_disable(info->rtc_clk);
 
 	return 0;
@@ -158,6 +172,9 @@ static int s3c_rtc_gettime(struct device *dev, struct rtc_time *rtc_tm)
 		return -EINVAL;
 
 	clk_enable(info->rtc_clk);
+	if (info->data->needs_src_clk)
+		clk_enable(info->rtc_src_clk);
+
  retry_get_time:
 	rtc_tm->tm_min  = readb(info->base + S3C2410_RTCMIN);
 	rtc_tm->tm_hour = readb(info->base + S3C2410_RTCHOUR);
@@ -191,6 +208,8 @@ static int s3c_rtc_gettime(struct device *dev, struct rtc_time *rtc_tm)
 
 	rtc_tm->tm_mon -= 1;
 
+	if (info->data->needs_src_clk)
+		clk_disable(info->rtc_src_clk);
 	clk_disable(info->rtc_clk);
 
 	return rtc_valid_tm(rtc_tm);
@@ -216,6 +235,8 @@ static int s3c_rtc_settime(struct device *dev, struct rtc_time *tm)
 	}
 
 	clk_enable(info->rtc_clk);
+	if (info->data->needs_src_clk)
+		clk_enable(info->rtc_src_clk);
 
 	writeb(bin2bcd(tm->tm_sec),  info->base + S3C2410_RTCSEC);
 	writeb(bin2bcd(tm->tm_min),  info->base + S3C2410_RTCMIN);
@@ -224,6 +245,8 @@ static int s3c_rtc_settime(struct device *dev, struct rtc_time *tm)
 	writeb(bin2bcd(tm->tm_mon + 1), info->base + S3C2410_RTCMON);
 	writeb(bin2bcd(year), info->base + S3C2410_RTCYEAR);
 
+	if (info->data->needs_src_clk)
+		clk_disable(info->rtc_src_clk);
 	clk_disable(info->rtc_clk);
 
 	return 0;
@@ -239,6 +262,9 @@ static int s3c_rtc_getalarm(struct device *dev, struct rtc_wkalrm *alrm)
 		return -EINVAL;
 
 	clk_enable(info->rtc_clk);
+	if (info->data->needs_src_clk)
+		clk_enable(info->rtc_src_clk);
+
 	alm_tm->tm_sec  = readb(info->base + S3C2410_ALMSEC);
 	alm_tm->tm_min  = readb(info->base + S3C2410_ALMMIN);
 	alm_tm->tm_hour = readb(info->base + S3C2410_ALMHOUR);
@@ -290,7 +316,10 @@ static int s3c_rtc_getalarm(struct device *dev, struct rtc_wkalrm *alrm)
 	else
 		alm_tm->tm_year = -1;
 
+	if (info->data->needs_src_clk)
+		clk_disable(info->rtc_src_clk);
 	clk_disable(info->rtc_clk);
+
 	return 0;
 }
 
@@ -304,6 +333,9 @@ static int s3c_rtc_setalarm(struct device *dev, struct rtc_wkalrm *alrm)
 		return -EINVAL;
 
 	clk_enable(info->rtc_clk);
+	if (info->data->needs_src_clk)
+		clk_enable(info->rtc_src_clk);
+
 	dev_dbg(dev, "s3c_rtc_setalarm: %d, %04d.%02d.%02d %02d:%02d:%02d\n",
 		 alrm->enabled,
 		 1900 + tm->tm_year, tm->tm_mon + 1, tm->tm_mday,
@@ -333,6 +365,8 @@ static int s3c_rtc_setalarm(struct device *dev, struct rtc_wkalrm *alrm)
 
 	s3c_rtc_setaie(dev, alrm->enabled);
 
+	if (info->data->needs_src_clk)
+		clk_disable(info->rtc_src_clk);
 	clk_disable(info->rtc_clk);
 
 	return 0;
@@ -346,10 +380,14 @@ static int s3c_rtc_proc(struct device *dev, struct seq_file *seq)
 		return -EINVAL;
 
 	clk_enable(info->rtc_clk);
+	if (info->data->needs_src_clk)
+		clk_enable(info->rtc_src_clk);
 
 	if (info->data->enable_tick)
 		info->data->enable_tick(info, seq);
 
+	if (info->data->needs_src_clk)
+		clk_disable(info->rtc_src_clk);
 	clk_disable(info->rtc_clk);
 
 	return 0;
@@ -369,6 +407,8 @@ static void s3c24xx_rtc_enable(struct s3c_rtc *info)
 	unsigned int con, tmp;
 
 	clk_enable(info->rtc_clk);
+	if (info->data->needs_src_clk)
+		clk_enable(info->rtc_src_clk);
 
 	con = readw(info->base + S3C2410_RTCCON);
 	/* re-enable the device, and check it is ok */
@@ -396,6 +436,8 @@ static void s3c24xx_rtc_enable(struct s3c_rtc *info)
 			info->base + S3C2410_RTCCON);
 	}
 
+	if (info->data->needs_src_clk)
+		clk_disable(info->rtc_src_clk);
 	clk_disable(info->rtc_clk);
 }
 
@@ -404,6 +446,8 @@ static void s3c24xx_rtc_disable(struct s3c_rtc *info)
 	unsigned int con;
 
 	clk_enable(info->rtc_clk);
+	if (info->data->needs_src_clk)
+		clk_enable(info->rtc_src_clk);
 
 	con = readw(info->base + S3C2410_RTCCON);
 	con &= ~S3C2410_RTCCON_RTCEN;
@@ -413,6 +457,8 @@ static void s3c24xx_rtc_disable(struct s3c_rtc *info)
 	con &= ~S3C2410_TICNT_ENABLE;
 	writeb(con, info->base + S3C2410_TICNT);
 
+	if (info->data->needs_src_clk)
+		clk_disable(info->rtc_src_clk);
 	clk_disable(info->rtc_clk);
 }
 
@@ -421,12 +467,16 @@ static void s3c6410_rtc_disable(struct s3c_rtc *info)
 	unsigned int con;
 
 	clk_enable(info->rtc_clk);
+	if (info->data->needs_src_clk)
+		clk_enable(info->rtc_src_clk);
 
 	con = readw(info->base + S3C2410_RTCCON);
 	con &= ~S3C64XX_RTCCON_TICEN;
 	con &= ~S3C2410_RTCCON_RTCEN;
 	writew(con, info->base + S3C2410_RTCCON);
 
+	if (info->data->needs_src_clk)
+		clk_disable(info->rtc_src_clk);
 	clk_disable(info->rtc_clk);
 }
 
@@ -498,11 +548,19 @@ static int s3c_rtc_probe(struct platform_device *pdev)
 
 	info->rtc_clk = devm_clk_get(&pdev->dev, "rtc");
 	if (IS_ERR(info->rtc_clk)) {
-		dev_err(&pdev->dev, "failed to find rtc clock source\n");
+		dev_err(&pdev->dev, "failed to find rtc clock\n");
 		return PTR_ERR(info->rtc_clk);
 	}
 	clk_prepare_enable(info->rtc_clk);
 
+	info->rtc_src_clk = devm_clk_get(&pdev->dev, "rtc_src");
+	if (IS_ERR(info->rtc_src_clk)) {
+		dev_err(&pdev->dev, "failed to find rtc source clock\n");
+		return PTR_ERR(info->rtc_src_clk);
+	}
+	clk_prepare_enable(info->rtc_src_clk);
+
+
 	/* check to see if everything is setup correctly */
 	if (info->data->enable)
 		info->data->enable(info);
@@ -556,6 +614,8 @@ static int s3c_rtc_probe(struct platform_device *pdev)
 
 	s3c_rtc_setfreq(info, 1);
 
+	if (info->data->needs_src_clk)
+		clk_disable(info->rtc_src_clk);
 	clk_disable(info->rtc_clk);
 
 	return 0;
@@ -575,6 +635,8 @@ static int s3c_rtc_suspend(struct device *dev)
 	struct s3c_rtc *info = dev_get_drvdata(dev);
 
 	clk_enable(info->rtc_clk);
+	if (info->data->needs_src_clk)
+		clk_enable(info->rtc_src_clk);
 
 	/* save TICNT for anyone using periodic interrupts */
 	if (info->data->save_tick_cnt)
@@ -590,6 +652,8 @@ static int s3c_rtc_suspend(struct device *dev)
 			dev_err(dev, "enable_irq_wake failed\n");
 	}
 
+	if (info->data->needs_src_clk)
+		clk_disable(info->rtc_src_clk);
 	clk_disable(info->rtc_clk);
 
 	return 0;
@@ -600,6 +664,8 @@ static int s3c_rtc_resume(struct device *dev)
 	struct s3c_rtc *info = dev_get_drvdata(dev);
 
 	clk_enable(info->rtc_clk);
+	if (info->data->needs_src_clk)
+		clk_enable(info->rtc_src_clk);
 
 	if (info->data->enable)
 		info->data->enable(info);
@@ -612,6 +678,8 @@ static int s3c_rtc_resume(struct device *dev)
 		info->wake_en = false;
 	}
 
+	if (info->data->needs_src_clk)
+		clk_disable(info->rtc_src_clk);
 	clk_disable(info->rtc_clk);
 
 	return 0;
@@ -622,7 +690,11 @@ static SIMPLE_DEV_PM_OPS(s3c_rtc_pm_ops, s3c_rtc_suspend, s3c_rtc_resume);
 static void s3c24xx_rtc_irq(struct s3c_rtc *info, int mask)
 {
 	clk_enable(info->rtc_clk);
+	if (info->data->needs_src_clk)
+		clk_enable(info->rtc_src_clk);
 	rtc_update_irq(info->rtc, 1, RTC_AF | RTC_IRQF);
+	if (info->data->needs_src_clk)
+		clk_disable(info->rtc_src_clk);
 	clk_disable(info->rtc_clk);
 
 	s3c_rtc_alarm_clk_enable(info, false);
@@ -631,8 +703,12 @@ static void s3c24xx_rtc_irq(struct s3c_rtc *info, int mask)
 static void s3c6410_rtc_irq(struct s3c_rtc *info, int mask)
 {
 	clk_enable(info->rtc_clk);
+	if (info->data->needs_src_clk)
+		clk_enable(info->rtc_src_clk);
 	rtc_update_irq(info->rtc, 1, RTC_AF | RTC_IRQF);
 	writeb(mask, info->base + S3C2410_INTP);
+	if (info->data->needs_src_clk)
+		clk_disable(info->rtc_src_clk);
 	clk_disable(info->rtc_clk);
 
 	s3c_rtc_alarm_clk_enable(info, false);
@@ -798,6 +874,18 @@ static struct s3c_rtc_data const s3c6410_rtc_data = {
 	.disable		= s3c6410_rtc_disable,
 };
 
+static struct s3c_rtc_data const exynos3250_rtc_data = {
+	.max_user_freq		= 32768,
+	.needs_src_clk		= true,
+	.irq_handler		= s3c6410_rtc_irq,
+	.set_freq		= s3c6410_rtc_setfreq,
+	.enable_tick		= s3c6410_rtc_enable_tick,
+	.save_tick_cnt		= s3c6410_rtc_save_tick_cnt,
+	.restore_tick_cnt	= s3c6410_rtc_restore_tick_cnt,
+	.enable			= s3c24xx_rtc_enable,
+	.disable		= s3c6410_rtc_disable,
+};
+
 static const struct of_device_id s3c_rtc_dt_match[] = {
 	{
 		.compatible = "samsung,s3c2410-rtc",
@@ -811,6 +899,9 @@ static const struct of_device_id s3c_rtc_dt_match[] = {
 	}, {
 		.compatible = "samsung,s3c6410-rtc",
 		.data = (void *)&s3c6410_rtc_data,
+	}, {
+		.compatible = "samsung,exynos3250-rtc",
+		.data = (void *)&exynos3250_rtc_data,
 	},
 	{ /* sentinel */ },
 };
-- 
1.8.0

[PATCHv2 5/5] ARM: dts: Fix wrong compatible string of Exynos3250 RTC dt node

[y]

From: Chanwoo Choi <cw00.choi@samsung.com>

This patch fix wrong compatible string of Exynos3250 RTC (Real-Time Clock) dt
node. The RTC of Exynos3250 must need additional source clock (XrtcXTI).

Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 arch/arm/boot/dts/exynos3250.dtsi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/exynos3250.dtsi b/arch/arm/boot/dts/exynos3250.dtsi
index cd6a69a..1be5ef8 100644
--- a/arch/arm/boot/dts/exynos3250.dtsi
+++ b/arch/arm/boot/dts/exynos3250.dtsi
@@ -192,7 +192,7 @@
 		};
 
 		rtc: rtc@10070000 {
-			compatible = "samsung,s3c6410-rtc";
+			compatible = "samsung,exynos3250-rtc";
 			reg = <0x10070000 0x100>;
 			interrupts = <0 73 0>, <0 74 0>;
 			status = "disabled";
-- 
1.8.0

Re: [PATCHv2 1/5] rtc: s3c: Define s3c_rtc structure to remove global variables.

[Andrew Morton]

On Tue, 12 Aug 2014 11:01:07 +0900 y@samsung.com wrote:

> This patch define s3c_rtc structure including necessary variables for S3C RTC
> device instead of global variables. This patch improves the readability by
> removing global variables.

Below is the v1->v2 delta.

Why were all those tests of info->base added?  Can it really be zero? 
I don't see how.

--- a/drivers/rtc/rtc-s3c.c~rtc-s3c-define-s3c_rtc-structure-to-remove-global-variables-v2
+++ a/drivers/rtc/rtc-s3c.c
@@ -121,6 +121,9 @@ static int s3c_rtc_setaie(struct device
 	struct s3c_rtc *info = dev_get_drvdata(dev);
 	unsigned int tmp;
 
+	if (!info->base)
+		return -EINVAL;
+
 	dev_dbg(info->dev, "%s: aie=%d\n", __func__, enabled);
 
 	clk_enable(info->rtc_clk);
@@ -180,6 +183,9 @@ static int s3c_rtc_gettime(struct device
 	struct s3c_rtc *info = dev_get_drvdata(dev);
 	unsigned int have_retried = 0;
 
+	if (!info->base)
+		return -EINVAL;
+
 	clk_enable(info->rtc_clk);
  retry_get_time:
 	rtc_tm->tm_min  = readb(info->base + S3C2410_RTCMIN);
@@ -224,6 +230,9 @@ static int s3c_rtc_settime(struct device
 	struct s3c_rtc *info = dev_get_drvdata(dev);
 	int year = tm->tm_year - 100;
 
+	if (!info->base)
+		return -EINVAL;
+
 	dev_dbg(dev, "set time %04d.%02d.%02d %02d:%02d:%02d\n",
 		 1900 + tm->tm_year, tm->tm_mon, tm->tm_mday,
 		 tm->tm_hour, tm->tm_min, tm->tm_sec);
@@ -255,6 +264,9 @@ static int s3c_rtc_getalarm(struct devic
 	struct rtc_time *alm_tm = &alrm->time;
 	unsigned int alm_en;
 
+	if (!info->base)
+		return -EINVAL;
+
 	clk_enable(info->rtc_clk);
 	alm_tm->tm_sec  = readb(info->base + S3C2410_ALMSEC);
 	alm_tm->tm_min  = readb(info->base + S3C2410_ALMMIN);
@@ -317,6 +329,9 @@ static int s3c_rtc_setalarm(struct devic
 	struct rtc_time *tm = &alrm->time;
 	unsigned int alrm_en;
 
+	if (!info->base)
+		return -EINVAL;
+
 	clk_enable(info->rtc_clk);
 	dev_dbg(dev, "s3c_rtc_setalarm: %d, %04d.%02d.%02d %02d:%02d:%02d\n",
 		 alrm->enabled,
@@ -357,6 +372,9 @@ static int s3c_rtc_proc(struct device *d
 	struct s3c_rtc *info = dev_get_drvdata(dev);
 	unsigned int ticnt;
 
+	if (!info->base)
+		return -EINVAL;
+
 	clk_enable(info->rtc_clk);
 	if (info->cpu_type == TYPE_S3C64XX) {
 		ticnt = readw(info->base + S3C2410_RTCCON);
@@ -548,7 +566,7 @@ static int s3c_rtc_probe(struct platform
 		rtc_tm.tm_min	= 0;
 		rtc_tm.tm_sec	= 0;
 
-		s3c_rtc_settime(NULL, &rtc_tm);
+		s3c_rtc_settime(&pdev->dev, &rtc_tm);
 
 		dev_warn(&pdev->dev, "warning: invalid RTC value so initializing it\n");
 	}
_

Re: [PATCHv2 1/5] rtc: s3c: Define s3c_rtc structure to remove global variables.

[Chanwoo Choi]

Dear Andrew, 

On 08/23/2014 05:42 AM, Andrew Morton wrote:
> On Tue, 12 Aug 2014 11:01:07 +0900 y@samsung.com wrote:
> 
>> This patch define s3c_rtc structure including necessary variables for S3C RTC
>> device instead of global variables. This patch improves the readability by
>> removing global variables.
> 
> Below is the v1->v2 delta.
> 
> Why were all those tests of info->base added?  Can it really be zero? 
> I don't see how.

If some functions (e.g., s3c_rtc_settime) accesses the rtc register
by using info->base before the initialization of info->base in s3c_rtc_probe,
I thought that null pointer error would happen.

But, I missed one point which info->base might have the garbate data instead of NULL.
I'll add the initialization code for info->base.
	info->base = NULL;

If you don't agree it, I'll drop this code checking the state of info->base on next patchset(v3).

Best Regads,
Chanwoo Choi

> 
> --- a/drivers/rtc/rtc-s3c.c~rtc-s3c-define-s3c_rtc-structure-to-remove-global-variables-v2
> +++ a/drivers/rtc/rtc-s3c.c
> @@ -121,6 +121,9 @@ static int s3c_rtc_setaie(struct device
>  	struct s3c_rtc *info = dev_get_drvdata(dev);
>  	unsigned int tmp;
>  
> +	if (!info->base)
> +		return -EINVAL;
> +
>  	dev_dbg(info->dev, "%s: aie=%d\n", __func__, enabled);
>  
>  	clk_enable(info->rtc_clk);
> @@ -180,6 +183,9 @@ static int s3c_rtc_gettime(struct device
>  	struct s3c_rtc *info = dev_get_drvdata(dev);
>  	unsigned int have_retried = 0;
>  
> +	if (!info->base)
> +		return -EINVAL;
> +
>  	clk_enable(info->rtc_clk);
>   retry_get_time:
>  	rtc_tm->tm_min  = readb(info->base + S3C2410_RTCMIN);
> @@ -224,6 +230,9 @@ static int s3c_rtc_settime(struct device
>  	struct s3c_rtc *info = dev_get_drvdata(dev);
>  	int year = tm->tm_year - 100;
>  
> +	if (!info->base)
> +		return -EINVAL;
> +
>  	dev_dbg(dev, "set time %04d.%02d.%02d %02d:%02d:%02d\n",
>  		 1900 + tm->tm_year, tm->tm_mon, tm->tm_mday,
>  		 tm->tm_hour, tm->tm_min, tm->tm_sec);
> @@ -255,6 +264,9 @@ static int s3c_rtc_getalarm(struct devic
>  	struct rtc_time *alm_tm = &alrm->time;
>  	unsigned int alm_en;
>  
> +	if (!info->base)
> +		return -EINVAL;
> +
>  	clk_enable(info->rtc_clk);
>  	alm_tm->tm_sec  = readb(info->base + S3C2410_ALMSEC);
>  	alm_tm->tm_min  = readb(info->base + S3C2410_ALMMIN);
> @@ -317,6 +329,9 @@ static int s3c_rtc_setalarm(struct devic
>  	struct rtc_time *tm = &alrm->time;
>  	unsigned int alrm_en;
>  
> +	if (!info->base)
> +		return -EINVAL;
> +
>  	clk_enable(info->rtc_clk);
>  	dev_dbg(dev, "s3c_rtc_setalarm: %d, %04d.%02d.%02d %02d:%02d:%02d\n",
>  		 alrm->enabled,
> @@ -357,6 +372,9 @@ static int s3c_rtc_proc(struct device *d
>  	struct s3c_rtc *info = dev_get_drvdata(dev);
>  	unsigned int ticnt;
>  
> +	if (!info->base)
> +		return -EINVAL;
> +
>  	clk_enable(info->rtc_clk);
>  	if (info->cpu_type == TYPE_S3C64XX) {
>  		ticnt = readw(info->base + S3C2410_RTCCON);
> @@ -548,7 +566,7 @@ static int s3c_rtc_probe(struct platform
>  		rtc_tm.tm_min	= 0;
>  		rtc_tm.tm_sec	= 0;
>  
> -		s3c_rtc_settime(NULL, &rtc_tm);
> +		s3c_rtc_settime(&pdev->dev, &rtc_tm);
>  
>  		dev_warn(&pdev->dev, "warning: invalid RTC value so initializing it\n");
>  	}
> _
> 
> 

Re: [PATCHv2 1/5] rtc: s3c: Define s3c_rtc structure to remove global variables.

[Andrew Morton]

On Mon, 25 Aug 2014 09:57:33 +0900 Chanwoo Choi <cw00.choi@samsung.com> wrote:

> Dear Andrew, 
> 
> On 08/23/2014 05:42 AM, Andrew Morton wrote:
> > On Tue, 12 Aug 2014 11:01:07 +0900 y@samsung.com wrote:
> > 
> >> This patch define s3c_rtc structure including necessary variables for S3C RTC
> >> device instead of global variables. This patch improves the readability by
> >> removing global variables.
> > 
> > Below is the v1->v2 delta.
> > 
> > Why were all those tests of info->base added?  Can it really be zero? 
> > I don't see how.
> 
> If some functions (e.g., s3c_rtc_settime) accesses the rtc register
> by using info->base before the initialization of info->base in s3c_rtc_probe,
> I thought that null pointer error would happen.

probe() should be called before anything else.  If we're somehow
calling s3c_rtc_settime() before probe() has completed then something
very bad is happening - for example, the device may have been
registered far too early.  But I don't think that's the case here.

That being said, it does seem strange that s3c_rtc_probe() calls
devm_rtc_device_register() *before* trying to request its IRQs.  So if
IRQ requesting fails, we go and immediately unregister the device. 
Some other drivers do it this way, others do not.  Wouldn't it be
better to defer registration until we know that all the probe() setup
operations have succeeded?

> But, I missed one point which info->base might have the garbate data instead of NULL.
> I'll add the initialization code for info->base.
> 	info->base = NULL;
> 
> If you don't agree it, I'll drop this code checking the state of info->base on next patchset(v3).

Well, we should have those checks in there unless we know they're
needed.  And if they *are* needed, we should have a good understanding
of why they're needed, and we should be sure that we're not just
working around some underlying problem.

Re: [PATCHv2 1/5] rtc: s3c: Define s3c_rtc structure to remove global variables.

[Chanwoo Choi]

Dear Andrew,

On 08/27/2014 06:31 AM, Andrew Morton wrote:
> On Mon, 25 Aug 2014 09:57:33 +0900 Chanwoo Choi <cw00.choi@samsung.com> wrote:
> 
>> Dear Andrew, 
>>
>> On 08/23/2014 05:42 AM, Andrew Morton wrote:
>>> On Tue, 12 Aug 2014 11:01:07 +0900 y@samsung.com wrote:
>>>
>>>> This patch define s3c_rtc structure including necessary variables for S3C RTC
>>>> device instead of global variables. This patch improves the readability by
>>>> removing global variables.
>>>
>>> Below is the v1->v2 delta.
>>>
>>> Why were all those tests of info->base added?  Can it really be zero? 
>>> I don't see how.
>>
>> If some functions (e.g., s3c_rtc_settime) accesses the rtc register
>> by using info->base before the initialization of info->base in s3c_rtc_probe,
>> I thought that null pointer error would happen.
> 
> probe() should be called before anything else.  If we're somehow
> calling s3c_rtc_settime() before probe() has completed then something
> very bad is happening - for example, the device may have been
> registered far too early.  But I don't think that's the case here.

I means that existing rtc-s3c.c driver executed s3c_rtc_settime() in s3c_rtc_probe()
before initialization of info->base. But, It is my mistake. so, I modified it just as following:

-		s3c_rtc_settime(NULL, &rtc_tm);
+		s3c_rtc_settime(&pdev->dev, &rtc_tm);

> 
> That being said, it does seem strange that s3c_rtc_probe() calls
> devm_rtc_device_register() *before* trying to request its IRQs.  So if
> IRQ requesting fails, we go and immediately unregister the device. 
> Some other drivers do it this way, others do not.  Wouldn't it be
> better to defer registration until we know that all the probe() setup
> operations have succeeded?

You're right. I missed this point. If rtc-s3c.c driver completed the probe function,
info->base has always right address.

+	if (!info->base)
+		return -EINVAL;
+

As you said, checking state of 'info-base' is un-needed.
I'll send new patchset(v3) to fix it.

> 
>> But, I missed one point which info->base might have the garbate data instead of NULL.
>> I'll add the initialization code for info->base.
>> 	info->base = NULL;
>>
>> If you don't agree it, I'll drop this code checking the state of info->base on next patchset(v3).
> 
> Well, we should have those checks in there unless we know they're
> needed.  And if they *are* needed, we should have a good understanding
> of why they're needed, and we should be sure that we're not just
> working around some underlying problem.

You are right. Thanks for your comment and advice.

Best Regards,
Chanwoo Choi