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
| ||
|
Date: Sat, 9 Jul 2022 13:56:27 +0300 From: Markuss Broks <markuss.broks@...il.com> To: Lee Jones <lee.jones@...aro.org> Cc: linux-kernel@...r.kernel.org, phone-devel@...r.kernel.org, ~postmarketos/upstreaming@...ts.sr.ht, Matti Vaittinen <mazziesaccount@...il.com>, Rob Herring <robh+dt@...nel.org>, Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>, Liam Girdwood <lgirdwood@...il.com>, Mark Brown <broonie@...nel.org>, MyungJoo Ham <myungjoo.ham@...sung.com>, Chanwoo Choi <cw00.choi@...sung.com>, devicetree@...r.kernel.org Subject: Re: [PATCH v5 4/5] mfd: sm5703: Add support for SM5703 MFD Hi Lee, On 6/15/22 00:32, Lee Jones wrote: > On Sat, 23 Apr 2022, Markuss Broks wrote: > >> Silicon Mitus SM5703 is a multi-function device, meant to be > Please avoid using the term MFD. > > How is the device described in the data-sheet? I don't have a data-sheet for this device. The only reference I have is downstream Linux driver. Maybe a better way to call it would be PMIC? > > What do you mean by "meant to be"? "designed to be", it appears that this part is almost exclusively used by Samsung in its mobile phones. > >> used in mobile devices. It has several modules: LDO, BUCK regulators, > Modules or functions? Some of "modules" are quite separate, so I decided to call it that way. How should it be called? > >> charger circuit, flash LED driver, a fuel gauge for monitoring the battery >> and a MUIC USB switcher. The MUIC and fuel gauge parts are separate in that >> they use separate i2c lines to communicate with the device, while charger > "I2C" > >> circuit, LED driver and regulators are on the main i2c line the device is >> controlled with. >> >> Signed-off-by: Markuss Broks <markuss.broks@...il.com> >> --- >> MAINTAINERS | 8 +++ >> drivers/mfd/Kconfig | 13 +++++ >> drivers/mfd/Makefile | 1 + >> drivers/mfd/sm5703.c | 78 +++++++++++++++++++++++++++ >> include/linux/mfd/sm5703.h | 105 +++++++++++++++++++++++++++++++++++++ >> 5 files changed, 205 insertions(+) >> create mode 100644 drivers/mfd/sm5703.c >> create mode 100644 include/linux/mfd/sm5703.h >> >> diff --git a/MAINTAINERS b/MAINTAINERS >> index 6157e706ed02..6125ed1a3be4 100644 >> --- a/MAINTAINERS >> +++ b/MAINTAINERS >> @@ -18172,6 +18172,14 @@ T: git git://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-rcu.git dev >> F: include/linux/srcu*.h >> F: kernel/rcu/srcu*.c >> >> +SM5703 MFD DRIVER >> +M: Markuss Broks <markuss.broks@...il.com> >> +S: Maintained >> +F: Documentation/devicetree/bindings/mfd/siliconmitus,sm5703.yaml >> +F: Documentation/devicetree/bindings/regulator/siliconmitus,sm5703-regulator.yaml >> +F: drivers/mfd/sm5703.c >> +F: drivers/regulator/sm5703-regulator.c >> + >> SMACK SECURITY MODULE >> M: Casey Schaufler <casey@...aufler-ca.com> >> L: linux-security-module@...r.kernel.org >> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig >> index 3b59456f5545..c13a99ceae99 100644 >> --- a/drivers/mfd/Kconfig >> +++ b/drivers/mfd/Kconfig >> @@ -1237,6 +1237,19 @@ config MFD_SM501 >> interface. The device may be connected by PCI or local bus with >> varying functions enabled. >> >> +config MFD_SM5703 >> + tristate "Silicon Mitus SM5703 MFD" >> + depends on I2C >> + depends on OF >> + select MFD_CORE >> + select REGMAP_I2C >> + help >> + This is the core driver for the Silicon Mitus SM5703 multi-function > Please drop the MFD term, as above. > >> + device. This device is meant to be used in phones and it has numerous > "meant to be"? > >> + modules, including LED controller, regulators, fuel gauge, MUIC and > Either "an LED controller" or "LED controllers" > > Same with "charger circuit" below. > >> + charger circuit. It also support muxing a serial interface over USB > "supports" > > What kind of serial? UART as a standard Samsung debug port interface (619K Ohm resistor between data lines is Samsung's debug cable). > >> + data lines. >> + >> config MFD_SM501_GPIO >> bool "Export GPIO via GPIO layer" >> depends on MFD_SM501 && GPIOLIB >> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile >> index 858cacf659d6..ca8b86736a36 100644 >> --- a/drivers/mfd/Makefile >> +++ b/drivers/mfd/Makefile >> @@ -275,3 +275,4 @@ rsmu-i2c-objs := rsmu_core.o rsmu_i2c.o >> rsmu-spi-objs := rsmu_core.o rsmu_spi.o >> obj-$(CONFIG_MFD_RSMU_I2C) += rsmu-i2c.o >> obj-$(CONFIG_MFD_RSMU_SPI) += rsmu-spi.o >> +obj-$(CONFIG_MFD_SM5703) += sm5703.o >> diff --git a/drivers/mfd/sm5703.c b/drivers/mfd/sm5703.c >> new file mode 100644 >> index 000000000000..7f9838149051 >> --- /dev/null >> +++ b/drivers/mfd/sm5703.c >> @@ -0,0 +1,78 @@ >> +// SPDX-License-Identifier: GPL-2.0-only >> + >> +#include <linux/err.h> >> +#include <linux/delay.h> >> +#include <linux/gpio/consumer.h> >> +#include <linux/i2c.h> >> +#include <linux/mfd/core.h> >> +#include <linux/mfd/sm5703.h> >> +#include <linux/module.h> >> +#include <linux/of_device.h> >> +#include <linux/regmap.h> >> + >> +static const struct mfd_cell sm5703_devs[] = { >> + { .name = "sm5703-regulator", }, >> +}; > Where are the rest of the child drivers? > >> +static const struct regmap_config sm5703_regmap_config = { >> + .reg_bits = 8, >> + .val_bits = 8, > Tabbing looks odd. Just use a space. > >> +}; >> + >> +static int sm5703_i2c_probe(struct i2c_client *i2c, >> + const struct i2c_device_id *id) >> +{ >> + struct sm5703_dev *sm5703; >> + struct device *dev = &i2c->dev; >> + unsigned int dev_id; >> + int ret; >> + >> + sm5703 = devm_kzalloc(dev, sizeof(*sm5703), GFP_KERNEL); >> + if (!sm5703) >> + return -ENOMEM; >> + >> + i2c_set_clientdata(i2c, sm5703); >> + sm5703->dev = dev; >> + >> + sm5703->regmap = devm_regmap_init_i2c(i2c, &sm5703_regmap_config); >> + if (IS_ERR(sm5703->regmap)) >> + return dev_err_probe(dev, PTR_ERR(sm5703->regmap), >> + "Failed to allocate the register map\n"); >> + >> + sm5703->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH); >> + if (IS_ERR(sm5703->reset_gpio)) >> + return dev_err_probe(dev, PTR_ERR(sm5703->reset_gpio), "Cannot get reset GPIO\n"); >> + >> + gpiod_set_value_cansleep(sm5703->reset_gpio, 1); >> + msleep(20); >> + >> + ret = regmap_read(sm5703->regmap, SM5703_DEVICE_ID, &dev_id); >> + if (ret) >> + return dev_err_probe(dev, -ENODEV, "Device not found\n"); >> + >> + ret = devm_mfd_add_devices(sm5703->dev, -1, sm5703_devs, > Not -1. Please use the defines provided. > >> + ARRAY_SIZE(sm5703_devs), NULL, 0, NULL); >> + if (ret) >> + return dev_err_probe(dev, ret, "Failed to add child devices\n"); >> + >> + return 0; >> +} >> + >> +static const struct of_device_id sm5703_of_match[] = { >> + { .compatible = "siliconmitus,sm5703", }, >> + { } >> +}; >> +MODULE_DEVICE_TABLE(of, sm5703_of_match); >> + >> +static struct i2c_driver sm5703_driver = { >> + .driver = { >> + .name = "sm5703", >> + .of_match_table = sm5703_of_match, >> + }, >> + .probe = sm5703_i2c_probe, >> +}; >> +module_i2c_driver(sm5703_driver); >> + >> +MODULE_DESCRIPTION("Silicon Mitus SM5703 multi-function device driver"); > There is no such thing as an MFD. > >> +MODULE_AUTHOR("Markuss Broks <markuss.broks@...il.com>"); >> +MODULE_LICENSE("GPL"); >> diff --git a/include/linux/mfd/sm5703.h b/include/linux/mfd/sm5703.h >> new file mode 100644 >> index 000000000000..c62affa0d3f1 >> --- /dev/null >> +++ b/include/linux/mfd/sm5703.h >> @@ -0,0 +1,105 @@ >> +/* SPDX-License-Identifier: GPL-2.0-only */ >> + >> +#ifndef _SM5703_H >> +#define _SM5703_H >> + >> +struct sm5703_dev { >> + struct device *dev; >> + struct regmap *regmap; >> + struct gpio_desc *reset_gpio; >> +}; >> + >> +// Regulator-related defines > No C++ style comments. > >> +#define SM5703_REG_LDO1 0x1A > I'd drop the REG parts from these. > > What are these registers called in the data-sheet? No data-sheet, sadly. > >> +#define SM5703_REG_LDO2 0x1B >> +#define SM5703_REG_LDO3 0x1C >> +#define SM5703_LDO_EN BIT(3) > Some people like to space out the bits to differentiate them from the > register addresses. > >> +#define SM5703_LDO_VOLT_MASK 0x7 > Please keep the bit length consistent, so 0x07 here if these are Byte > length registers. > >> +#define SM5703_BUCK_VOLT_MASK 0x1F >> +#define SM5703_REG_USBLDO12 0x1C >> +#define SM5703_REG_EN_USBLDO1 BIT(6) >> +#define SM5703_REG_EN_USBLDO2 BIT(7) >> +#define SM5703_REG_BUCK 0x1D > Please place these in order. > >> +#define SM5703_REG_EN_BUCK BIT(6) >> +#define SM5703_USBLDO_MICROVOLT 4800000 >> +#define SM5703_VBUS_MICROVOLT 5000000 >> + >> +// Fuel-Gauge-related defines >> + >> +#define SM5703_FG_REG_DEVICE_ID 0x00 >> +#define SM5703_FG_REG_CNTL 0x01 >> +#define SM5703_FG_REG_INTFG 0x02 >> +#define SM5703_FG_REG_INTFG_MASK 0x03 >> +#define SM5703_FG_REG_STATUS 0x04 >> +#define SM5703_FG_REG_SOC 0x05 >> +#define SM5703_FG_REG_OCV 0x06 >> +#define SM5703_FG_REG_VOLTAGE 0x07 >> +#define SM5703_FG_REG_CURRENT 0x08 >> +#define SM5703_FG_REG_TEMPERATURE 0x09 >> +#define SM5703_FG_REG_CURRENT_EST 0x85 >> +#define SM5703_FG_REG_FG_OP_STATUS 0x10 >> + >> +// Flash LED driver-related defines >> + >> +#define SM5703_FLEDCNTL1 0x14 >> +#define SM5703_FLEDCNTL2 0x15 >> +#define SM5703_FLEDCNTL3 0x16 >> +#define SM5703_FLEDCNTL4 0x17 >> +#define SM5703_FLEDCNTL5 0x18 >> +#define SM5703_FLEDCNTL6 0x19 >> + >> +#define SM5703_FLEDEN_MASK 0x03 >> +#define SM5703_FLEDEN_DISABLE 0x00 >> +#define SM5703_FLEDEN_MOVIE_MODE 0x01 >> +#define SM5703_FLEDEN_FLASH_MODE 0x02 >> +#define SM5703_FLEDEN_EXTERNAL 0x03 >> + >> +#define SM5703_IFLED_MASK 0x1F >> +#define SM5703_IMLED_MASK 0x1F >> + >> +// Charger-related, IRQ and device ID defines >> + >> +#define SM5703_REG_CNTL 0x0C >> +#define SM5703_VBUSCNTL 0x0D >> +#define SM5703_CHGCNTL1 0x0E >> +#define SM5703_CHGCNTL2 0x0F >> +#define SM5703_CHGCNTL3 0x10 >> +#define SM5703_CHGCNTL4 0x11 >> +#define SM5703_CHGCNTL5 0x12 >> +#define SM5703_CHGCNTL6 0x13 >> +#define SM5703_OTGCURRENTCNTL 0x60 >> +#define SM5703_Q3LIMITCNTL 0x66 >> +#define SM5703_DEVICE_ID 0x1E >> +#define SM5703_OPERATION_MODE 0x07 >> +#define SM5703_OPERATION_MODE_MASK 0x07 >> + >> +#define SM5703_OPERATION_MODE_SUSPEND 0x00 >> +#define SM5703_OPERATION_MODE_CHARGING_OFF 0x04 >> +#define SM5703_OPERATION_MODE_CHARGING_ON 0x05 >> +#define SM5703_OPERATION_MODE_FLASH_BOOST_MODE 0x06 >> +#define SM5703_OPERATION_MODE_USB_OTG_MODE 0x07 >> + >> +#define SM5703_BSTOUT 0x0F >> +#define SM5703_BSTOUT_MASK 0x0F >> +#define SM5703_BSTOUT_SHIFT 0 >> + >> +#define SM5703_BSTOUT_4P5 0x05 >> +#define SM5703_BSTOUT_5P0 0x0A >> +#define SM5703_BSTOUT_5P1 0x0B >> + >> +#define SM5703_IRQ_STATUS1 0x08 >> +#define SM5703_IRQ_STATUS2 0x09 >> +#define SM5703_IRQ_STATUS3 0x0A >> +#define SM5703_IRQ_STATUS4 0x0B >> +#define SM5703_IRQ_STATUS5 0x6B >> + >> +#define SM5703_STATUS1_OTGFAIL 0x80 >> +#define SM5703_STATUS3_DONE 0x08 >> +#define SM5703_STATUS3_TOPOFF 0x04 >> +#define SM5703_STATUS3_CHGON 0x01 >> +#define SM5703_STATUS5_VBUSOVP 0x80 >> +#define SM5703_STATUS5_VBUSUVLO 0x40 >> +#define SM5703_STATUS5_VBUSOK 0x20 >> + >> +#endif - Markuss
Powered by blists - more mailing lists