Re: [PATCH v4 15/27] regulator: add s2dos05 regulator support

[Krzysztof Kozlowski <krzk@xxxxxxxxxx>]

On Fri, Sep 13, 2024 at 06:07:58PM +0300, Dzmitry Sankouski wrote:
> S2dos05 has 1 buck and 4 LDO regulators, used for powering
> panel/touchscreen.
> 
> Signed-off-by: Dzmitry Sankouski <dsankouski@xxxxxxxxx>
> 

...

> +#include <linux/bug.h>
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/slab.h>
> +#include <linux/module.h>
> +#include <linux/regmap.h>
> +#include <linux/interrupt.h>
> +#include <linux/platform_device.h>
> +#include <linux/regulator/driver.h>
> +#include <linux/regulator/machine.h>
> +#include <linux/regulator/of_regulator.h>
> +#include <linux/mfd/samsung/core.h>
> +#include <linux/regulator/s2dos05.h>
> +#include <linux/i2c.h>
> +
> +struct s2dos05_data {
> +	struct regmap *regmap;
> +	struct device *dev;
> +};
> +
> +static const struct regulator_ops s2dos05_ops = {

Keep definitions together. This goes down, after all declarations and
macros.

> +	.list_voltage		= regulator_list_voltage_linear,
> +	.map_voltage		= regulator_map_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	= regulator_set_voltage_time_sel,
> +	.set_active_discharge	= regulator_set_active_discharge_regmap,
> +};
> +
> +#define _BUCK(macro)	S2DOS05_BUCK##macro
> +#define _buck_ops(num)	s2dos05_ops##num
> +
> +#define _LDO(macro)	S2DOS05_LDO##macro
> +#define _REG(ctrl)	S2DOS05_REG##ctrl
> +#define _ldo_ops(num)	s2dos05_ops##num
> +#define _MASK(macro)	S2DOS05_ENABLE_MASK##macro
> +#define _TIME(macro)	S2DOS05_ENABLE_TIME##macro
> +

...

> +
> +static struct regulator_desc regulators[S2DOS05_REGULATOR_MAX] = {

This should be const.

> +		// name, id, ops, min_uv, uV_step, vsel_reg, enable_reg
> +		LDO_DESC("ldo1", _LDO(1), &_ldo_ops(), _LDO(_MIN1),
> +			_LDO(_STEP1), _REG(_LDO1_CFG),
> +			_REG(_EN), _MASK(_L1), _TIME(_LDO), _REG(_LDO1_CFG)),
> +		LDO_DESC("ldo2", _LDO(2), &_ldo_ops(), _LDO(_MIN1),
> +			_LDO(_STEP1), _REG(_LDO2_CFG),
> +			_REG(_EN), _MASK(_L2), _TIME(_LDO), _REG(_LDO2_CFG)),
> +		LDO_DESC("ldo3", _LDO(3), &_ldo_ops(), _LDO(_MIN2),
> +			_LDO(_STEP1), _REG(_LDO3_CFG),
> +			_REG(_EN), _MASK(_L3), _TIME(_LDO), _REG(_LDO3_CFG)),
> +		LDO_DESC("ldo4", _LDO(4), &_ldo_ops(), _LDO(_MIN2),
> +			_LDO(_STEP1), _REG(_LDO4_CFG),
> +			_REG(_EN), _MASK(_L4), _TIME(_LDO), _REG(_LDO4_CFG)),
> +		BUCK_DESC("buck1", _BUCK(1), &_buck_ops(), _BUCK(_MIN1),
> +			_BUCK(_STEP1), _REG(_BUCK_VOUT),
> +			_REG(_EN), _MASK(_B1), _TIME(_BUCK), _REG(_BUCK_CFG)),
> +};
> +
> +static int s2dos05_pmic_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct sec_pmic_dev *iodev = dev_get_drvdata(pdev->dev.parent);
> +	struct of_regulator_match *rdata = NULL;
> +	struct s2dos05_data *s2dos05;
> +	struct regulator_config config = { };
> +	unsigned int rdev_num = ARRAY_SIZE(regulators);
> +	int i, ret;
> +
> +	s2dos05 = devm_kzalloc(dev, sizeof(struct s2dos05_data),
> +				GFP_KERNEL);

sizeof(*)

> +	if (!s2dos05)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, s2dos05);
> +
> +	rdata = devm_kcalloc(dev, rdev_num, sizeof(*rdata), GFP_KERNEL);
> +	if (!rdata)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < rdev_num; i++)
> +		rdata[i].name = regulators[i].name;
> +
> +	s2dos05->regmap = iodev->regmap_pmic;
> +	s2dos05->dev = dev;
> +	if (!dev->of_node)
> +		dev->of_node = dev->parent->of_node;
> +
> +	for (i = 0; i < rdev_num; i++) {
> +		struct regulator_dev *regulator;
> +
> +		config.init_data = rdata[i].init_data;
> +		config.of_node = rdata[i].of_node;
> +		config.dev = dev;
> +		config.driver_data = s2dos05;
> +		regulator = devm_regulator_register(&pdev->dev,
> +						&regulators[i], &config);
> +		if (IS_ERR(regulator)) {
> +			ret = PTR_ERR(regulator);
> +			dev_err(&pdev->dev, "regulator init failed for %d\n",
> +				i);
> +		}
> +	}
> +
> +	return ret;
> +}
> +
> +static const struct platform_device_id s2dos05_pmic_id[] = {
> +	{ "s2dos05-regulator" },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(platform, s2dos05_pmic_id);
> +
> +static struct platform_driver s2dos05_platform_driver = {
> +	.driver = {
> +		.name = "s2dos05",
> +	},
> +	.probe = s2dos05_pmic_probe,
> +	.id_table = s2dos05_pmic_id,
> +};
> +module_platform_driver(s2dos05_platform_driver);
> +
> +MODULE_AUTHOR("Dzmitry Sankouski <dsankouski@xxxxxxxxx>");
> +MODULE_DESCRIPTION("SAMSUNG s2dos05 Regulator Driver");

s/SAMSUNG/Samsung/

Also, your Kconfig used different name, so please use one - probably
Samsung.

This applies to MFD patch as well.

> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/regulator/s2dos05.h b/include/linux/regulator/s2dos05.h
> new file mode 100644
> index 000000000000..2e89fcbce769
> --- /dev/null
> +++ b/include/linux/regulator/s2dos05.h
> @@ -0,0 +1,73 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */

Are you sure that here (and other places) you want any newer GPL? This
is quite odd. Does original code (from which you took 2016 copyrights)
have this as well?

Best regards,
Krzysztof