Re: [PATCH 10/34] mfd: sec: split into core and transport (i2c) drivers

[Krzysztof Kozlowski <krzk@kernel.org>]

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.

> +		pdata->manual_poweroff = probedata->manual_poweroff;
> +		pdata->disable_wrstbi = probedata->disable_wrstbi;
>  	}
>  
>  	sec_irq_init(sec_pmic);
> @@ -383,9 +195,9 @@ static int sec_pmic_probe(struct i2c_client *i2c)
>  		num_sec_devs = ARRAY_SIZE(s2mpu05_devs);
>  		break;
>  	default:
> -		dev_err(&i2c->dev, "Unsupported device type (%lu)\n",
> +		dev_err(sec_pmic->dev, "Unsupported device type %lu\n",
>  			sec_pmic->device_type);
> -		return -ENODEV;
> +		return -EINVAL;
>  	}
>  	ret = devm_mfd_add_devices(sec_pmic->dev, -1, sec_devs, num_sec_devs,
>  				   NULL, 0, NULL);
> @@ -397,10 +209,11 @@ static int sec_pmic_probe(struct i2c_client *i2c)
>  
>  	return ret;
>  }
> +EXPORT_SYMBOL_GPL(sec_pmic_probe);
>  
> -static void sec_pmic_shutdown(struct i2c_client *i2c)
> +void sec_pmic_shutdown(struct device *dev)
>  {
> -	struct sec_pmic_dev *sec_pmic = i2c_get_clientdata(i2c);
> +	struct sec_pmic_dev *sec_pmic = dev_get_drvdata(dev);
>  	unsigned int reg, mask;
>  
>  	if (!sec_pmic->pdata->manual_poweroff)
> @@ -424,11 +237,11 @@ static void sec_pmic_shutdown(struct i2c_client *i2c)
>  
>  	regmap_update_bits(sec_pmic->regmap_pmic, reg, mask, 0);
>  }
> +EXPORT_SYMBOL_GPL(sec_pmic_shutdown);
>  
>  static int sec_pmic_suspend(struct device *dev)
>  {
> -	struct i2c_client *i2c = to_i2c_client(dev);
> -	struct sec_pmic_dev *sec_pmic = i2c_get_clientdata(i2c);
> +	struct sec_pmic_dev *sec_pmic = dev_get_drvdata(dev);
>  
>  	if (device_may_wakeup(dev))
>  		enable_irq_wake(sec_pmic->irq);
> @@ -448,8 +261,7 @@ static int sec_pmic_suspend(struct device *dev)
>  
>  static int sec_pmic_resume(struct device *dev)
>  {
> -	struct i2c_client *i2c = to_i2c_client(dev);
> -	struct sec_pmic_dev *sec_pmic = i2c_get_clientdata(i2c);
> +	struct sec_pmic_dev *sec_pmic = dev_get_drvdata(dev);
>  
>  	if (device_may_wakeup(dev))
>  		disable_irq_wake(sec_pmic->irq);
> @@ -458,20 +270,10 @@ static int sec_pmic_resume(struct device *dev)
>  	return 0;
>  }
>  
> -static DEFINE_SIMPLE_DEV_PM_OPS(sec_pmic_pm_ops,
> -				sec_pmic_suspend, sec_pmic_resume);
> -
> -static struct i2c_driver sec_pmic_driver = {
> -	.driver = {
> -		   .name = "sec_pmic",
> -		   .pm = pm_sleep_ptr(&sec_pmic_pm_ops),
> -		   .of_match_table = sec_dt_match,
> -	},
> -	.probe = sec_pmic_probe,
> -	.shutdown = sec_pmic_shutdown,
> -};
> -module_i2c_driver(sec_pmic_driver);
> +DEFINE_SIMPLE_DEV_PM_OPS(sec_pmic_pm_ops, sec_pmic_suspend, sec_pmic_resume);
> +EXPORT_SYMBOL_GPL(sec_pmic_pm_ops);
>  
> +MODULE_AUTHOR("André Draszik <andre.draszik@linaro.org>");
>  MODULE_AUTHOR("Sangbeom Kim <sbkim73@samsung.com>");
> -MODULE_DESCRIPTION("Core support for the S5M MFD");
> +MODULE_DESCRIPTION("Core driver for the Samsung S5M");
>  MODULE_LICENSE("GPL");
> diff --git a/drivers/mfd/sec-core.h b/drivers/mfd/sec-core.h
> index b3fded5f02a0ddc09a9508fd49a5d335f7ad0ee7..58e5b645f377cea5543a215c05957a2c49239a6f 100644
> --- a/drivers/mfd/sec-core.h
> +++ b/drivers/mfd/sec-core.h
> @@ -10,6 +10,23 @@
>  #ifndef __SEC_CORE_INT_H
>  #define __SEC_CORE_INT_H
>  
> +struct i2c_client;
> +
> +extern const struct dev_pm_ops sec_pmic_pm_ops;
> +
> +struct sec_pmic_probe_data {
> +	/* Whether or not manually set PWRHOLD to low during shutdown. */
> +	bool manual_poweroff;
> +	/* Disable the WRSTBI (buck voltage warm reset) when probing? */
> +	bool disable_wrstbi;
> +};
> +
> +int sec_pmic_probe(struct device *dev, unsigned long device_type,
> +		   unsigned int irq, struct regmap *regmap,
> +		   const struct sec_pmic_probe_data *probedata,
> +		   struct i2c_client *client);
> +void sec_pmic_shutdown(struct device *dev);
> +
>  int sec_irq_init(struct sec_pmic_dev *sec_pmic);
>  
>  #endif /* __SEC_CORE_INT_H */
> 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()?


...

> +	{ },
> +};
> +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@linaro.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.

> +MODULE_AUTHOR("Sangbeom Kim <sbkim73@samsung.com>");
> +MODULE_DESCRIPTION("I2C driver for the Samsung S5M");
> +MODULE_LICENSE("GPL");
> 


Best regards,
Krzysztof