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: <18b43ec2e9f6edd5597de0023c30e7c949416ac2.camel@linaro.org>
Date: Wed, 26 Mar 2025 09:33:55 +0000
From: André Draszik <andre.draszik@...aro.org>
To: Krzysztof Kozlowski <krzk@...nel.org>, Lee Jones <lee@...nel.org>, Rob
 Herring <robh@...nel.org>, Conor Dooley <conor+dt@...nel.org>, Sylwester
 Nawrocki	 <s.nawrocki@...sung.com>, Chanwoo Choi <cw00.choi@...sung.com>,
 Alim Akhtar	 <alim.akhtar@...sung.com>, Michael Turquette
 <mturquette@...libre.com>,  Stephen Boyd <sboyd@...nel.org>, Russell King
 <linux@...linux.org.uk>, Catalin Marinas	 <catalin.marinas@....com>, Will
 Deacon <will@...nel.org>, Alexandre Belloni	 <alexandre.belloni@...tlin.com>
Cc: Peter Griffin <peter.griffin@...aro.org>, Tudor Ambarus
	 <tudor.ambarus@...aro.org>, Will McVicker <willmcvicker@...gle.com>, 
	kernel-team@...roid.com, linux-kernel@...r.kernel.org, 
	linux-samsung-soc@...r.kernel.org, devicetree@...r.kernel.org, 
	linux-clk@...r.kernel.org, linux-arm-kernel@...ts.infradead.org, 
	linux-rtc@...r.kernel.org
Subject: Re: [PATCH 10/34] mfd: sec: split into core and transport (i2c)
 drivers

Hi Krzysztof,

Thanks for your review.

On Wed, 2025-03-26 at 08:14 +0100, Krzysztof Kozlowski wrote:
> On 23/03/2025 23:39, André Draszik wrote:
> > -
> > -	sec_pmic->regmap_pmic = devm_regmap_init_i2c(i2c, regmap);
> > -	if (IS_ERR(sec_pmic->regmap_pmic)) {
> > -		ret = PTR_ERR(sec_pmic->regmap_pmic);
> > -		dev_err(&i2c->dev, "Failed to allocate register map: %d\n",
> > -			ret);
> > -		return ret;
> > +	if (probedata) {
> 
> I don't get why this is conditional. New transport will also provide
> probedata or at least it should.

The original thinking was that I'll need very different OF parsing for
ACPM and I2C transports, but after reconsidering, I'll keep the OF parsing
in the core instead (to truly have common OF parsing), and then this
becomes unnecessary.

[...]

> > diff --git a/drivers/mfd/sec-i2c.c b/drivers/mfd/sec-i2c.c
> > new file mode 100644
> > index 0000000000000000000000000000000000000000..803a46e657a5a1a639014d442941c0cdc60556a5
> > --- /dev/null
> > +++ b/drivers/mfd/sec-i2c.c
> > @@ -0,0 +1,252 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Copyright 2012 Samsung Electronics Co., Ltd
> > + *                http://www.samsung.com
> > + * Copyright 2025 Linaro Ltd.
> > + *
> > + * Samsung SxM I2C driver
> > + */
> > +
> > +#include <linux/dev_printk.h>
> > +#include <linux/err.h>
> > +#include <linux/i2c.h>
> > +#include <linux/mfd/samsung/core.h>
> > +#include <linux/mfd/samsung/s2mpa01.h>
> > +#include <linux/mfd/samsung/s2mps11.h>
> > +#include <linux/mfd/samsung/s2mps13.h>
> > +#include <linux/mfd/samsung/s2mps14.h>
> > +#include <linux/mfd/samsung/s2mps15.h>
> > +#include <linux/mfd/samsung/s2mpu02.h>
> > +#include <linux/mfd/samsung/s5m8767.h>
> > +#include <linux/mod_devicetable.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/pm.h>
> > +#include <linux/regmap.h>
> > +#include "sec-core.h"
> > +
> > +static bool s2mpa01_volatile(struct device *dev, unsigned int reg)
> > +{
> > +	switch (reg) {
> > +	case S2MPA01_REG_INT1M:
> > +	case S2MPA01_REG_INT2M:
> > +	case S2MPA01_REG_INT3M:
> > +		return false;
> > +	default:
> > +		return true;
> > +	}
> > +}
> > +
> > +static bool s2mps11_volatile(struct device *dev, unsigned int reg)
> > +{
> > +	switch (reg) {
> > +	case S2MPS11_REG_INT1M:
> > +	case S2MPS11_REG_INT2M:
> > +	case S2MPS11_REG_INT3M:
> > +		return false;
> > +	default:
> > +		return true;
> > +	}
> > +}
> > +
> > +static bool s2mpu02_volatile(struct device *dev, unsigned int reg)
> > +{
> > +	switch (reg) {
> > +	case S2MPU02_REG_INT1M:
> > +	case S2MPU02_REG_INT2M:
> > +	case S2MPU02_REG_INT3M:
> > +		return false;
> > +	default:
> > +		return true;
> > +	}
> > +}
> > +
> > +static const struct regmap_config sec_regmap_config = {
> > +	.reg_bits = 8,
> > +	.val_bits = 8,
> > +};
> > +
> > +static const struct regmap_config s2mpa01_regmap_config = {
> > +	.reg_bits = 8,
> > +	.val_bits = 8,
> > +
> > +	.max_register = S2MPA01_REG_LDO_OVCB4,
> > +	.volatile_reg = s2mpa01_volatile,
> > +	.cache_type = REGCACHE_FLAT,
> > +};
> > +
> > +static const struct regmap_config s2mps11_regmap_config = {
> > +	.reg_bits = 8,
> > +	.val_bits = 8,
> > +
> > +	.max_register = S2MPS11_REG_L38CTRL,
> > +	.volatile_reg = s2mps11_volatile,
> > +	.cache_type = REGCACHE_FLAT,
> > +};
> > +
> > +static const struct regmap_config s2mps13_regmap_config = {
> > +	.reg_bits = 8,
> > +	.val_bits = 8,
> > +
> > +	.max_register = S2MPS13_REG_LDODSCH5,
> > +	.volatile_reg = s2mps11_volatile,
> > +	.cache_type = REGCACHE_FLAT,
> > +};
> > +
> > +static const struct regmap_config s2mps14_regmap_config = {
> > +	.reg_bits = 8,
> > +	.val_bits = 8,
> > +
> > +	.max_register = S2MPS14_REG_LDODSCH3,
> > +	.volatile_reg = s2mps11_volatile,
> > +	.cache_type = REGCACHE_FLAT,
> > +};
> > +
> > +static const struct regmap_config s2mps15_regmap_config = {
> > +	.reg_bits = 8,
> > +	.val_bits = 8,
> > +
> > +	.max_register = S2MPS15_REG_LDODSCH4,
> > +	.volatile_reg = s2mps11_volatile,
> > +	.cache_type = REGCACHE_FLAT,
> > +};
> > +
> > +static const struct regmap_config s2mpu02_regmap_config = {
> > +	.reg_bits = 8,
> > +	.val_bits = 8,
> > +
> > +	.max_register = S2MPU02_REG_DVSDATA,
> > +	.volatile_reg = s2mpu02_volatile,
> > +	.cache_type = REGCACHE_FLAT,
> > +};
> > +
> > +static const struct regmap_config s5m8767_regmap_config = {
> > +	.reg_bits = 8,
> > +	.val_bits = 8,
> > +
> > +	.max_register = S5M8767_REG_LDO28CTRL,
> > +	.volatile_reg = s2mps11_volatile,
> > +	.cache_type = REGCACHE_FLAT,
> > +};
> > +
> > +/*
> > + * Only the common platform data elements for s5m8767 are parsed here from the
> > + * device tree. Other sub-modules of s5m8767 such as pmic, rtc , charger and
> > + * others have to parse their own platform data elements from device tree.
> > + */
> > +static void
> > +sec_pmic_i2c_parse_dt_pdata(struct device *dev,
> > +			    struct sec_pmic_probe_data *pd)
> > +{
> > +	pd->manual_poweroff =
> > +		of_property_read_bool(dev->of_node,
> > +				      "samsung,s2mps11-acokb-ground");
> > +	pd->disable_wrstbi =
> > +		of_property_read_bool(dev->of_node,
> > +				      "samsung,s2mps11-wrstbi-ground");
> > +}
> > +
> > +static int sec_pmic_i2c_probe(struct i2c_client *client)
> > +{
> > +	struct sec_pmic_probe_data probedata;
> > +	const struct regmap_config *regmap;
> > +	unsigned long device_type;
> > +	struct regmap *regmap_pmic;
> > +	int ret;
> > +
> > +	sec_pmic_i2c_parse_dt_pdata(&client->dev, &probedata);
> 
> This wasn't tested and it makes no sense. You pass random stack values.
> And what is the point of:
> "pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);"
> in sec_pmic_probe()?

Here, the 'struct sec_pmic_probe_data probedata' is on-stack,
and populated by sec_pmic_i2c_parse_dt_pdata(), to become non-random.
This is then passed into the core's sec_pmic_probe(), which kzalloc()s
pdata, and populates pdata using probedata. i2c's probedata is not
used past .probe(), and the pdata from the core is kzalloc()d, so it's
safe to keep using past .probe().

But I'll change this anyway, to keep it all in the core in the first
place, to have truly common i2c and acpm parsing of DT.

> 
> 
> ...
> 
> > +	{ },
> > +};
> > +MODULE_DEVICE_TABLE(of, sec_pmic_i2c_of_match);
> > +
> > +static struct i2c_driver sec_pmic_i2c_driver = {
> > +	.driver = {
> > +		.name = "sec-pmic-i2c",
> > +		.pm = pm_sleep_ptr(&sec_pmic_pm_ops),
> > +		.of_match_table = sec_pmic_i2c_of_match,
> > +	},
> > +	.probe = sec_pmic_i2c_probe,
> > +	.shutdown = sec_pmic_i2c_shutdown,
> > +};
> > +module_i2c_driver(sec_pmic_i2c_driver);
> > +
> > +MODULE_AUTHOR("André Draszik <andre.draszik@...aro.org>");
> 
> This belongs to the patch adding actual features. Moving existing code
> or splitting it is not really reason to became the author of the code.
> The code was there.

OK.

Cheers,
Andre'


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ