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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <35liocltjuxv3gjueuvpaytx44crebbc4c63atztakuq5dfpax@bquve7tkrvtx>
Date: Mon, 16 Sep 2024 11:21:57 +0200
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Dzmitry Sankouski <dsankouski@...il.com>
Cc: Sebastian Reichel <sre@...nel.org>, 
	Bjorn Andersson <andersson@...nel.org>, Michael Turquette <mturquette@...libre.com>, 
	Stephen Boyd <sboyd@...nel.org>, Neil Armstrong <neil.armstrong@...aro.org>, 
	Jessica Zhang <quic_jesszhan@...cinc.com>, Sam Ravnborg <sam@...nborg.org>, 
	Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>, Maxime Ripard <mripard@...nel.org>, 
	Thomas Zimmermann <tzimmermann@...e.de>, David Airlie <airlied@...il.com>, Rob Herring <robh@...nel.org>, 
	Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, Lee Jones <lee@...nel.org>, 
	Dmitry Torokhov <dmitry.torokhov@...il.com>, Pavel Machek <pavel@....cz>, Liam Girdwood <lgirdwood@...il.com>, 
	Mark Brown <broonie@...nel.org>, Uwe Kleine-König <ukleinek@...nel.org>, 
	Chanwoo Choi <cw00.choi@...sung.com>, Simona Vetter <simona@...ll.ch>, 
	cros-qcom-dts-watchers@...omium.org, Konrad Dybcio <konradybcio@...nel.org>, 
	Simona Vetter <simona.vetter@...ll.ch>, linux-pm@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-arm-msm@...r.kernel.org, linux-clk@...r.kernel.org, dri-devel@...ts.freedesktop.org, 
	devicetree@...r.kernel.org, linux-input@...r.kernel.org, linux-leds@...r.kernel.org, 
	linux-pwm@...r.kernel.org, linux-samsung-soc@...r.kernel.org
Subject: Re: [PATCH v4 15/27] regulator: add s2dos05 regulator support

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@...il.com>
> 

...

> +#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@...il.com>");
> +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