| 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: <c0714653-f7b0-4bd3-877e-55682847d619@kernel.org>
Date: Mon, 30 Dec 2024 12:33:45 +0100
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Troy Mitchell <troymitchell988@...il.com>, Lee Jones <lee@...nel.org>,
Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>
Cc: linux-riscv@...ts.infradead.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] mfd: add new driver for P1 PMIC from SpacemiT
On 30/12/2024 11:02, Troy Mitchell wrote:
> Add the core MFD driver for P1 PMIC. I define four sub-devices
I do not see any definition of MFD subdevices.
> for which the drivers will be added in subsequent patches.
>
> For this patch, It supports `reboot` and `shutdown`.
>
> Signed-off-by: Troy Mitchell <TroyMitchell988@...il.com>
> ---
> drivers/mfd/Kconfig | 14 +
> drivers/mfd/Makefile | 1 +
> drivers/mfd/spacemit-pmic.c | 159 ++++++++++
> include/linux/mfd/spacemit/spacemit-p1.h | 491 +++++++++++++++++++++++++++++
> include/linux/mfd/spacemit/spacemit-pmic.h | 39 +++
> 5 files changed, 704 insertions(+)
>
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index ae23b317a64e49f0cb529ae6bd1becbb90b7c282..c062bf6b11fd23d420a6d5f6ee51b3ec97f9fcbb 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -1173,6 +1173,20 @@ config MFD_QCOM_RPM
> Say M here if you want to include support for the Qualcomm RPM as a
> module. This will build a module called "qcom_rpm".
>
> +config MFD_SPACEMIT_PMIC
> + tristate "SpacemiT PMIC"
> + depends on ARCH_SPACEMIT || COMPILE_TEST
> + depends on I2C && OF
> + select MFD_CORE
> + select REGMAP_I2C
> + select REGMAP_IRQ
> + help
> + If this option is turned on, the P1 chip produced by SpacemiT will
> + be supported.
> +
> + This driver can also be compiled as a module. If you choose to build
> + it as a module, the resulting kernel module will be named `spacemit-pmic`.
> +
> config MFD_SPMI_PMIC
> tristate "Qualcomm SPMI PMICs"
> depends on ARCH_QCOM || COMPILE_TEST
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index e057d6d6faef5c1d639789e2560f336fa26cd872..284dbb8fe2ef83bdd994a598504fe315f2eabbdf 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -266,6 +266,7 @@ obj-$(CONFIG_MFD_SUN4I_GPADC) += sun4i-gpadc.o
> obj-$(CONFIG_MFD_STM32_LPTIMER) += stm32-lptimer.o
> obj-$(CONFIG_MFD_STM32_TIMERS) += stm32-timers.o
> obj-$(CONFIG_MFD_MXS_LRADC) += mxs-lradc.o
> +obj-$(CONFIG_MFD_SPACEMIT_PMIC) += spacemit-pmic.o
> obj-$(CONFIG_MFD_SC27XX_PMIC) += sprd-sc27xx-spi.o
> obj-$(CONFIG_RAVE_SP_CORE) += rave-sp.o
> obj-$(CONFIG_MFD_ROHM_BD71828) += rohm-bd71828.o
> diff --git a/drivers/mfd/spacemit-pmic.c b/drivers/mfd/spacemit-pmic.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..d9f6785cecbd405821dead13cdf8d1f9fd64e508
> --- /dev/null
> +++ b/drivers/mfd/spacemit-pmic.c
> @@ -0,0 +1,159 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2024 Troy Mitchell <troymitchell988@...il.com>
> + */
> +
> +#include <linux/i2c.h>
> +#include <linux/mfd/core.h>
> +#include <linux/mfd/spacemit/spacemit-pmic.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
I don't see usage of this header. Include what is used directly.
> +#include <linux/pm_wakeirq.h>
> +#include <linux/reboot.h>
> +
> +P1_REGMAP_CONFIG;
> +P1_IRQS_DESC;
> +P1_IRQ_CHIP_DESC;
> +P1_POWER_KEY_RESOURCES_DESC;
> +P1_RTC_RESOURCES_DESC;
> +P1_MFD_CELL;
> +P1_MFD_MATCH_DATA;
Hm? Declarations and definitions go here, not to somewhere else.
> +
> +static int spacemit_pmic_probe(struct i2c_client *client)
> +{
> + const struct spacemit_pmic_match_data *match_data;
> + const struct mfd_cell *cells;
> + struct spacemit_pmic *pmic;
> + int nr_cells, ret;
> +
> + if (!client->irq)
> + return dev_err_probe(&client->dev, -ENXIO, "no interrupt supported");
And why is this fatal error? If interrupt is not supported by hardware,
why would you add "unsupported" interrupt?
> +
> + match_data = of_device_get_match_data(&client->dev);
> + if (WARN_ON(!match_data))
> + return -EINVAL;
> +
> + pmic = devm_kzalloc(&client->dev, sizeof(*pmic), GFP_KERNEL);
> + if (!pmic)
> + return -ENOMEM;
> +
> + cells = match_data->mfd_cells;
> + nr_cells = match_data->nr_cells;
> +
> + pmic->regmap_cfg = match_data->regmap_cfg;
> + pmic->regmap_irq_chip = match_data->regmap_irq_chip;
> + pmic->i2c = client;
> + pmic->match_data = match_data;
> + pmic->regmap = devm_regmap_init_i2c(client, pmic->regmap_cfg);
> + if (IS_ERR(pmic->regmap))
> + return dev_err_probe(&client->dev,
> + PTR_ERR(pmic->regmap),
> + "regmap initialization failed");
> +
> + regcache_cache_bypass(pmic->regmap, true);
> +
> + i2c_set_clientdata(client, pmic);
> +
> + if (pmic->regmap_irq_chip) {
It's impossible to have it false. Test your driver.
> + ret = regmap_add_irq_chip(pmic->regmap, client->irq, IRQF_ONESHOT, -1,
> + pmic->regmap_irq_chip, &pmic->irq_data);
> + if (ret)
> + return dev_err_probe(&client->dev, ret, "failed to add irqchip");
> + }
> +
> + dev_pm_set_wake_irq(&client->dev, client->irq);
> + device_init_wakeup(&client->dev, true);
> +
> + ret = devm_mfd_add_devices(&client->dev, PLATFORM_DEVID_NONE,
> + cells, nr_cells, NULL, 0,
> + regmap_irq_get_domain(pmic->irq_data));
> + if (ret)
> + return dev_err_probe(&client->dev, ret, "failed to add MFD devices");
> +
> + if (match_data->shutdown.reg) {
Also not possible, useless if.
> + ret = devm_register_sys_off_handler(&client->dev,
> + SYS_OFF_MODE_POWER_OFF_PREPARE,
> + SYS_OFF_PRIO_HIGH,
> + &spacemit_pmic_shutdown,
> + pmic);
> + if (ret)
> + return dev_err_probe(&client->dev,
> + ret,
> + "failed to register restart handler");
> +
> + }
> +
> + if (match_data->reboot.reg) {
Also not possible.
> + ret = devm_register_sys_off_handler(&client->dev,
> + SYS_OFF_MODE_RESTART,
> + SYS_OFF_PRIO_HIGH,
> + &spacemit_pmic_restart,
> + pmic);
> + if (ret)
> + return dev_err_probe(&client->dev,
> + ret,
> + "failed to register restart handler");
> + }
> +
> + return 0;
> +}
> +
> +static const struct of_device_id spacemit_pmic_of_match[] = {
> + { .compatible = "spacemit,p1", .data = &pmic_p1_match_data },
> + { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, spacemit_pmic_of_match);
> +
> +static struct i2c_driver spacemit_pmic_i2c_driver = {
> + .driver = {
> + .name = "spacemit-pmic",
> + .of_match_table = spacemit_pmic_of_match,
> + },
> + .probe = spacemit_pmic_probe,
> +};
> +
> +static int __init spacemit_pmic_init(void)
> +{
> + return platform_driver_register(&spacemit_pmic_i2c_driver);
> +}
> +
> +static void __exit spacemit_pmic_exit(void)
> +{
> + platform_driver_unregister(&spacemit_pmic_i2c_driver);
> +}
> +
> +module_init(spacemit_pmic_init);
> +module_exit(spacemit_pmic_exit);
Use proper wrapper for these above.
> +
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("mfd core driver for the SpacemiT PMIC");
...
> +
> +#define P1_MAX_REG 0xA8
> +
> +#define P1_BUCK_VSEL_MASK 0xff
> +#define P1_BUCK_EN_MASK 0x1
> +
> +#define P1_BUCK1_CTRL_REG 0x47
> +#define P1_BUCK2_CTRL_REG 0x4a
Either lowercase or uppercase hex, not both.
> +#define P1_BUCK3_CTRL_REG 0x4d
> +#define P1_BUCK4_CTRL_REG 0x50
> +#define P1_BUCK5_CTRL_REG 0x53
> +#define P1_BUCK6_CTRL_REG 0x56
> +
> +#define P1_BUCK1_VSEL_REG 0x48
> +#define P1_BUCK2_VSEL_REG 0x4b
> +#define P1_BUCK3_VSEL_REG 0x4e
> +#define P1_BUCK4_VSEL_REG 0x51
> +#define P1_BUCK5_VSEL_REG 0x54
> +#define P1_BUCK6_VSEL_REG 0x57
> +
> +#define P1_ALDO1_CTRL_REG 0x5b
> +#define P1_ALDO2_CTRL_REG 0x5e
> +#define P1_ALDO3_CTRL_REG 0x61
> +#define P1_ALDO4_CTRL_REG 0x64
> +
> +#define P1_ALDO1_VOLT_REG 0x5c
> +#define P1_ALDO2_VOLT_REG 0x5f
> +#define P1_ALDO3_VOLT_REG 0x62
> +#define P1_ALDO4_VOLT_REG 0x65
> +
> +#define P1_ALDO_EN_MASK 0x1
> +#define P1_ALDO_VSEL_MASK 0x7f
> +
> +#define P1_DLDO1_CTRL_REG 0x67
> +#define P1_DLDO2_CTRL_REG 0x6a
> +#define P1_DLDO3_CTRL_REG 0x6d
> +#define P1_DLDO4_CTRL_REG 0x70
> +#define P1_DLDO5_CTRL_REG 0x73
> +#define P1_DLDO6_CTRL_REG 0x76
> +#define P1_DLDO7_CTRL_REG 0x79
> +
> +#define P1_DLDO1_VOLT_REG 0x68
> +#define P1_DLDO2_VOLT_REG 0x6b
> +#define P1_DLDO3_VOLT_REG 0x6e
> +#define P1_DLDO4_VOLT_REG 0x71
> +#define P1_DLDO5_VOLT_REG 0x74
> +#define P1_DLDO6_VOLT_REG 0x77
> +#define P1_DLDO7_VOLT_REG 0x7a
> +
> +#define P1_DLDO_EN_MASK 0x1
> +#define P1_DLDO_VSEL_MASK 0x7f
> +
> +#define P1_SWITCH_CTRL_REG 0x59
> +#define P1_SWTICH_EN_MASK 0x1
> +
> +#define P1_PWR_CTRL2 0x7e
> +#define P1_SW_SHUTDOWN_BIT_MSK 0x4
> +#define P1_SW_RESET_BIT_MSK 0x2
> +
> +#define P1_E_GPI0_MSK BIT(0)
> +#define P1_E_GPI1_MSK BIT(1)
> +#define P1_E_GPI2_MSK BIT(2)
> +#define P1_E_GPI3_MSK BIT(3)
> +#define P1_E_GPI4_MSK BIT(4)
> +#define P1_E_GPI5_MSK BIT(5)
> +
> +#define P1_E_ADC_TEMP_MSK BIT(0)
> +#define P1_E_ADC_EOC_MSK BIT(1)
> +#define P1_E_ADC_EOS_MSK BIT(2)
> +#define P1_E_WDT_TO_MSK BIT(3)
> +#define P1_E_ALARM_MSK BIT(4)
> +#define P1_E_TICK_MSK BIT(5)
> +
> +#define P1_E_LDO_OV_MSK BIT(0)
> +#define P1_E_LDO_UV_MSK BIT(1)
> +#define P1_E_LDO_SC_MSK BIT(2)
> +#define P1_E_SW_SC_MSK BIT(3)
> +#define P1_E_TEMP_WARN_MSK BIT(4)
> +#define P1_E_TEMP_SEVERE_MSK BIT(5)
> +#define P1_E_TEMP_CRIT_MSK BIT(6)
> +
> +#define P1_E_BUCK1_OV_MSK BIT(0)
> +#define P1_E_BUCK2_OV_MSK BIT(1)
> +#define P1_E_BUCK3_OV_MSK BIT(2)
> +#define P1_E_BUCK4_OV_MSK BIT(3)
> +#define P1_E_BUCK5_OV_MSK BIT(4)
> +#define P1_E_BUCK6_OV_MSK BIT(5)
> +
> +#define P1_E_BUCK1_UV_MSK BIT(0)
> +#define P1_E_BUCK2_UV_MSK BIT(1)
> +#define P1_E_BUCK3_UV_MSK BIT(2)
> +#define P1_E_BUCK4_UV_MSK BIT(3)
> +#define P1_E_BUCK5_UV_MSK BIT(4)
> +#define P1_E_BUCK6_UV_MSK BIT(5)
> +
> +#define P1_E_BUCK1_SC_MSK BIT(0)
> +#define P1_E_BUCK2_SC_MSK BIT(1)
> +#define P1_E_BUCK3_SC_MSK BIT(2)
> +#define P1_E_BUCK4_SC_MSK BIT(3)
> +#define P1_E_BUCK5_SC_MSK BIT(4)
> +#define P1_E_BUCK6_SC_MSK BIT(5)
> +
> +#define P1_E_PWRON_RINTR_MSK BIT(0)
> +#define P1_E_PWRON_FINTR_MSK BIT(1)
> +#define P1_E_PWRON_SINTR_MSK BIT(2)
> +#define P1_E_PWRON_LINTR_MSK BIT(3)
> +#define P1_E_PWRON_SDINTR_MSK BIT(4)
> +#define P1_E_VSYS_OV_MSK BIT(5)
> +
> +#define P1_E_STATUS_REG_BASE 0x91
> +#define P1_E_EN_REG_BASE 0x98
> +
> +#define P1_REGMAP_CONFIG \
> + static const struct regmap_config p1_regmap_config = { \
> + .reg_bits = 8, \
> + .val_bits = 8, \
> + .max_register = P1_MAX_REG, \
> + .cache_type = REGCACHE_RBTREE, \
> + }
> +
> +#define P1_IRQS_DESC \
> +static const struct regmap_irq p1_irqs[] = { \
No, all these defines are just not needed, not readable. Please follow
existing kernel style - just look at recent drivers in drivers/mfd/ to
see how they are designed and developed.
> + [P1_E_GPI0] = { \
> + .mask = P1_E_GPI0_MSK, \
> + .reg_offset = 0, \
> + }, \
> + \
> + [P1_E_GPI1] = { \
> + .mask = P1_E_GPI1_MSK, \
Best regards,
Krzysztof
Powered by blists - more mailing lists