| 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
| ||
|
Message-ID: <20190403100101.GK11301@dell>
Date: Wed, 3 Apr 2019 11:01:01 +0100
From: Lee Jones <lee.jones@...aro.org>
To: Amelie Delaunay <amelie.delaunay@...com>
Cc: Linus Walleij <linus.walleij@...aro.org>,
Rob Herring <robh+dt@...nel.org>,
Mark Rutland <mark.rutland@....com>,
Alexandre Torgue <alexandre.torgue@...com>,
Maxime Coquelin <mcoquelin.stm32@...il.com>,
linux-gpio@...r.kernel.org, linux-kernel@...r.kernel.org,
devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
linux-stm32@...md-mailman.stormreply.com
Subject: Re: [PATCH v4 2/9] mfd: Add ST Multi-Function eXpander (STMFX) core
driver
On Wed, 27 Feb 2019, Amelie Delaunay wrote:
> STMicroelectronics Multi-Function eXpander (STMFX) is a slave controller
> using I2C for communication with the main MCU. Main features are:
> - 16 fast GPIOs individually configurable in input/output
> - 8 alternate GPIOs individually configurable in input/output when other
> STMFX functions are not used
> - Main MCU IDD measurement
> - Resistive touchscreen controller
>
> Signed-off-by: Amelie Delaunay <amelie.delaunay@...com>
> ---
> drivers/mfd/Kconfig | 13 ++
> drivers/mfd/Makefile | 2 +-
> drivers/mfd/stmfx.c | 568 ++++++++++++++++++++++++++++++++++++++++++++++
> include/linux/mfd/stmfx.h | 123 ++++++++++
> 4 files changed, 705 insertions(+), 1 deletion(-)
> create mode 100644 drivers/mfd/stmfx.c
> create mode 100644 include/linux/mfd/stmfx.h
Very nice first attempt for what is a pretty complex driver.
Just a couple of nits below.
[...]
> +static int stmfx_chip_init(struct i2c_client *client)
> +{
> + struct stmfx *stmfx = i2c_get_clientdata(client);
> + u32 id;
> + u8 version[2];
> + int ret;
> +
> + stmfx->vdd = devm_regulator_get_optional(&client->dev, "vdd");
> + if (IS_ERR(stmfx->vdd)) {
> + ret = PTR_ERR(stmfx->vdd);
> + if (ret != -ENODEV) {
> + if (ret != -EPROBE_DEFER)
> + dev_err(&client->dev,
> + "No VDD regulator found:%d\n", ret);
Actually -ENODEV means this, which is okay.
In this case we failed to obtain a provided regulator.
> + return ret;
> + }
> + } else {
if (IS_ERR(stmfx->vdd) && PTR_ERR(stmfx->vdd) == -EPROBE_DEFER) {
return PTR_ERR(stmfx->vdd);
} else (IS_ERR(stmfx->vdd) && PTR_ERR(stmfx->vdd) == -ENODEV) {
stmfx->vdd = NULL;
} else (IS_ERR(stmfx->vdd))) {
dev_err(&client->dev, "Failed to get VDD regulator:%d\n", ret);
return PTR_ERR(stmfx->vdd);
}
if (stmfx->vdd) {
> + ret = regulator_enable(stmfx->vdd);
> + if (ret) {
> + dev_err(&client->dev, "VDD enable failed: %d\n", ret);
> + return ret;
> + }
> + }
> +
[...]
> +static const struct resource stmfx_ts_resources[] = {
> + DEFINE_RES_IRQ(STMFX_REG_IRQ_SRC_EN_TS_DET),
> + DEFINE_RES_IRQ(STMFX_REG_IRQ_SRC_EN_TS_NE),
> + DEFINE_RES_IRQ(STMFX_REG_IRQ_SRC_EN_TS_TH),
> + DEFINE_RES_IRQ(STMFX_REG_IRQ_SRC_EN_TS_FULL),
> + DEFINE_RES_IRQ(STMFX_REG_IRQ_SRC_EN_TS_OVF),
> +};
Please move everything from here --------------->
> +static struct mfd_cell stmfx_cells[] = {
> + {
> + .of_compatible = "st,stmfx-0300-pinctrl",
> + .name = "stmfx-pinctrl",
> + .resources = stmfx_pinctrl_resources,
> + .num_resources = ARRAY_SIZE(stmfx_pinctrl_resources),
> + },
> + {
> + .of_compatible = "st,stmfx-0300-idd",
> + .name = "stmfx-idd",
> + .resources = stmfx_idd_resources,
> + .num_resources = ARRAY_SIZE(stmfx_idd_resources),
> + },
> + {
> + .of_compatible = "st,stmfx-0300-ts",
> + .name = "stmfx-ts",
> + .resources = stmfx_ts_resources,
> + .num_resources = ARRAY_SIZE(stmfx_ts_resources),
> + },
> +};
> +
> +static bool stmfx_reg_volatile(struct device *dev, unsigned int reg)
> +{
> + switch (reg) {
> + case STMFX_REG_SYS_CTRL:
> + case STMFX_REG_IRQ_SRC_EN:
> + case STMFX_REG_IRQ_PENDING:
> + case STMFX_REG_IRQ_GPI_PENDING1:
> + case STMFX_REG_IRQ_GPI_PENDING2:
> + case STMFX_REG_IRQ_GPI_PENDING3:
> + case STMFX_REG_GPIO_STATE1:
> + case STMFX_REG_GPIO_STATE2:
> + case STMFX_REG_GPIO_STATE3:
> + case STMFX_REG_IRQ_GPI_SRC1:
> + case STMFX_REG_IRQ_GPI_SRC2:
> + case STMFX_REG_IRQ_GPI_SRC3:
> + case STMFX_REG_GPO_SET1:
> + case STMFX_REG_GPO_SET2:
> + case STMFX_REG_GPO_SET3:
> + case STMFX_REG_GPO_CLR1:
> + case STMFX_REG_GPO_CLR2:
> + case STMFX_REG_GPO_CLR3:
> + return true;
> + default:
> + return false;
> + }
> +}
> +
> +static bool stmfx_reg_writeable(struct device *dev, unsigned int reg)
> +{
> + return (reg >= STMFX_REG_SYS_CTRL);
> +}
> +
> +static const struct regmap_config stmfx_regmap_config = {
> + .reg_bits = 8,
> + .reg_stride = 1,
> + .val_bits = 8,
> + .max_register = STMFX_REG_MAX,
> + .volatile_reg = stmfx_reg_volatile,
> + .writeable_reg = stmfx_reg_writeable,
> + .cache_type = REGCACHE_RBTREE,
> +};
----------------------->
... to here, up to the top, just below the includes.
> +static int stmfx_probe(struct i2c_client *client,
> + const struct i2c_device_id *id)
> +{
> + struct device *dev = &client->dev;
> + struct stmfx *stmfx;
> + int i, ret;
> +
> + stmfx = devm_kzalloc(dev, sizeof(*stmfx), GFP_KERNEL);
> + if (!stmfx)
> + return -ENOMEM;
> +
> + i2c_set_clientdata(client, stmfx);
> +
> + stmfx->dev = dev;
> +
> + stmfx->map = devm_regmap_init_i2c(client, &stmfx_regmap_config);
> + if (IS_ERR(stmfx->map)) {
> + ret = PTR_ERR(stmfx->map);
> + dev_err(dev, "Failed to allocate register map: %d\n", ret);
> + return ret;
> + }
> +
> + mutex_init(&stmfx->lock);
> +
> + ret = stmfx_chip_init(client);
> + if (ret) {
> + if (ret == -ETIMEDOUT)
> + return -EPROBE_DEFER;
> + return ret;
> + }
> +
> + if (client->irq < 0) {
> + dev_err(dev, "Failed to get IRQ: %d\n", client->irq);
> + ret = client->irq;
> + goto err_chip_exit;
> + }
> +
> + ret = stmfx_irq_init(client);
> + if (ret)
> + goto err_chip_exit;
> +
> + for (i = 0; i < ARRAY_SIZE(stmfx_cells); i++) {
> + stmfx_cells[i].platform_data = stmfx;
> + stmfx_cells[i].pdata_size = sizeof(struct stmfx);
> + }
Pass this though dev_get_drvdata() instead.
...
Actually, didn't you already set this with i2c_set_clientdata()? That
does exactly the same thing. So you can get this back from the client
via i2c_get_clientdata(). No need to send it though platform data.
> + ret = devm_mfd_add_devices(dev, PLATFORM_DEVID_NONE,
> + stmfx_cells, ARRAY_SIZE(stmfx_cells), NULL,
> + 0, stmfx->irq_domain);
> + if (ret)
> + goto err_irq_exit;
> +
> + return 0;
> +
> +err_irq_exit:
> + stmfx_irq_exit(client);
> +err_chip_exit:
> + stmfx_chip_exit(client);
> +
> + return ret;
> +}
> +
> +static int stmfx_remove(struct i2c_client *client)
> +{
> + stmfx_irq_exit(client);
> +
> + return stmfx_chip_exit(client);
> +}
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int stmfx_backup_regs(struct stmfx *stmfx)
> +{
> + int ret;
> +
> + ret = regmap_raw_read(stmfx->map, STMFX_REG_SYS_CTRL,
> + &stmfx->bkp_sysctrl, sizeof(stmfx->bkp_sysctrl));
> + if (ret)
> + return ret;
'\n' here.
> + ret = regmap_raw_read(stmfx->map, STMFX_REG_IRQ_OUT_PIN,
> + &stmfx->bkp_irqoutpin,
> + sizeof(stmfx->bkp_irqoutpin));
> + if (ret)
> + return ret;
> +
> + return 0;
> +}
> +
> +static int stmfx_restore_regs(struct stmfx *stmfx)
> +{
> + int ret;
> +
> + ret = regmap_raw_write(stmfx->map, STMFX_REG_SYS_CTRL,
> + &stmfx->bkp_sysctrl, sizeof(stmfx->bkp_sysctrl));
> + if (ret)
> + return ret;
'\n' here.
> + ret = regmap_raw_write(stmfx->map, STMFX_REG_IRQ_OUT_PIN,
> + &stmfx->bkp_irqoutpin,
> + sizeof(stmfx->bkp_irqoutpin));
> + if (ret)
> + return ret;
'\n' here.
> + ret = regmap_raw_write(stmfx->map, STMFX_REG_IRQ_SRC_EN,
> + &stmfx->irq_src, sizeof(stmfx->irq_src));
> + if (ret)
> + return ret;
> +
> + return 0;
> +}
> +
> +static int stmfx_suspend(struct device *dev)
> +{
> + struct stmfx *stmfx = dev_get_drvdata(dev);
> + int ret;
> +
> + ret = stmfx_backup_regs(stmfx);
Don't think you need a separate function for this. Just move the
regmap_raw_write() commands here.
> + if (ret) {
> + dev_err(stmfx->dev, "Registers backup failure\n");
> + return ret;
> + }
> +
> + if (!IS_ERR(stmfx->vdd)) {
> + ret = regulator_disable(stmfx->vdd);
> + if (ret)
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static int stmfx_resume(struct device *dev)
> +{
> + struct stmfx *stmfx = dev_get_drvdata(dev);
> + int ret;
> +
> + if (!IS_ERR(stmfx->vdd)) {
> + ret = regulator_enable(stmfx->vdd);
> + if (ret) {
> + dev_err(stmfx->dev,
> + "VDD enable failed: %d\n", ret);
> + return ret;
> + }
> + }
> +
> + ret = stmfx_restore_regs(stmfx);
As above.
> + if (ret) {
> + dev_err(stmfx->dev, "Registers restoration failure\n");
> + return ret;
> + }
> +
> + return 0;
> +}
> +#endif
> +
> +static SIMPLE_DEV_PM_OPS(stmfx_dev_pm_ops, stmfx_suspend, stmfx_resume);
> +
> +static const struct of_device_id stmfx_of_match[] = {
> + { .compatible = "st,stmfx-0300", },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, stmfx_of_match);
> +
> +static struct i2c_driver stmfx_driver = {
> + .driver = {
> + .name = "stmfx-core",
> + .of_match_table = of_match_ptr(stmfx_of_match),
> + .pm = &stmfx_dev_pm_ops,
> + },
> + .probe = stmfx_probe,
> + .remove = stmfx_remove,
> +};
> +module_i2c_driver(stmfx_driver);
> +
> +MODULE_DESCRIPTION("STMFX core driver");
> +MODULE_AUTHOR("Amelie Delaunay <amelie.delaunay@...com>");
> +MODULE_LICENSE("GPL v2");
[...]
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
Powered by blists - more mailing lists