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@xxxxxxxxxx>");
> MODULE_AUTHOR("Sangbeom Kim <sbkim73@xxxxxxxxxxx>");
> -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@xxxxxxxxxx>");
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@xxxxxxxxxxx>");
> +MODULE_DESCRIPTION("I2C driver for the Samsung S5M");
> +MODULE_LICENSE("GPL");
>
Best regards,
Krzysztof
