| Message ID | 20220423085319.483524-5-markuss.broks@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add support for Silicon Mitus SM5703 MFD | expand |
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? What do you mean by "meant to be"? > used in mobile devices. It has several modules: LDO, BUCK regulators, Modules or functions? > 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@gmail.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@gmail.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@schaufler-ca.com> > L: linux-security-module@vger.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? > + 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@gmail.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? > +#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
Hi Lee, Sorry to bother you again, but I've got additional questions while I was preparing the next version of the series: 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? > > What do you mean by "meant to be"? > >> used in mobile devices. It has several modules: LDO, BUCK regulators, > Modules or functions? > >> 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@gmail.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@gmail.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@schaufler-ca.com> >> L: linux-security-module@vger.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? > >> + 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? Should those devices still be present even though there's no driver for them (yet) ? I have a WIP version of driver for almost every function, but I currently lack time to get them done. > >> +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@gmail.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. I have no issues with that, however the already upstreamed sm5703-regulator driver uses those defines. If I change the define name, how should I make changes in that driver, would it be reasonable to send an additional patch together with the new MFD series? > > What are these registers called in the data-sheet? > >> +#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
On Tue, 19 Jul 2022, Markuss Broks wrote: > Hi Lee, > > On 7/18/22 11:17, Lee Jones wrote: > > On Fri, 15 Jul 2022, Markuss Broks wrote: > > > > > Hi Lee, > > > > > > Sorry to bother you again, but I've got additional questions while I was > > > preparing the next version of the series: > > > > > > 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? > > > > > > > > What do you mean by "meant to be"? > > > > > > > > > used in mobile devices. It has several modules: LDO, BUCK regulators, > > > > Modules or functions? > > > > > > > > > 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@gmail.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@gmail.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@schaufler-ca.com> > > > > > L: linux-security-module@vger.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? > > > > > > > > > + 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? > > > Should those devices still be present even though there's no driver for them > > > (yet) ? I have a WIP version of driver for almost every function, but I > > > currently lack time to get them done. > > Without them the driver-set is useless, no? > > > > We try to refrain from applying dead code. > > > > A lot of it has a tendency to stay that way. > Well, in my opinion, having just the regulator driver is already useful > enough: my board (Samsung Galaxy J5 (2015) ) uses one of LDO outputs for > powering the touchscreen. Touchscreen is kind of vital functionality for a > phone so I decided to upstream parts that are necessary for it first and > finish up other stuff later. It's not the only board that uses SM5703's > regulators for supplying all different kinds of hardware, either. Upstreaming functionality which is useful on its own is fine, but that doesn't tick all of the boxes to justify an MFD. This is a lot of code which is hard to justify if it only registers a Regulator driver.
Hi Lee, On 7/20/22 11:22, Lee Jones wrote: > On Tue, 19 Jul 2022, Markuss Broks wrote: > >> Hi Lee, >> >> On 7/18/22 11:17, Lee Jones wrote: >>> On Fri, 15 Jul 2022, Markuss Broks wrote: >>> >>>> Hi Lee, >>>> >>>> Sorry to bother you again, but I've got additional questions while I was >>>> preparing the next version of the series: >>>> >>>> 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? >>>>> >>>>> What do you mean by "meant to be"? >>>>> >>>>>> used in mobile devices. It has several modules: LDO, BUCK regulators, >>>>> Modules or functions? >>>>> >>>>>> 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@gmail.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@gmail.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@schaufler-ca.com> >>>>>> L: linux-security-module@vger.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? >>>>> >>>>>> + 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? >>>> Should those devices still be present even though there's no driver for them >>>> (yet) ? I have a WIP version of driver for almost every function, but I >>>> currently lack time to get them done. >>> Without them the driver-set is useless, no? >>> >>> We try to refrain from applying dead code. >>> >>> A lot of it has a tendency to stay that way. >> Well, in my opinion, having just the regulator driver is already useful >> enough: my board (Samsung Galaxy J5 (2015) ) uses one of LDO outputs for >> powering the touchscreen. Touchscreen is kind of vital functionality for a >> phone so I decided to upstream parts that are necessary for it first and >> finish up other stuff later. It's not the only board that uses SM5703's >> regulators for supplying all different kinds of hardware, either. > Upstreaming functionality which is useful on its own is fine, but that > doesn't tick all of the boxes to justify an MFD. This is a lot of > code which is hard to justify if it only registers a Regulator driver. Do you think I should hold on this series until I have other things done? Alternatively, I could make the regulator driver standalone, dedicated, but then when I'd add other functionality I'd have to redo it and add the MFD driver back, that I believe would be quite annoying from maintainers' and sanity perspective. The other functions left on the main control I2C are also not really "vital" to device's functionality (Flash LED and charger), so the regulator function makes the most sense to be available first, which was my motivation behind upstreaming that first. > - Markuss
On Wed, 20 Jul 2022, Markuss Broks wrote: > Hi Lee, > > On 7/20/22 11:22, Lee Jones wrote: > > On Tue, 19 Jul 2022, Markuss Broks wrote: > > > > > Hi Lee, > > > > > > On 7/18/22 11:17, Lee Jones wrote: > > > > On Fri, 15 Jul 2022, Markuss Broks wrote: > > > > > > > > > Hi Lee, > > > > > > > > > > Sorry to bother you again, but I've got additional questions while I was > > > > > preparing the next version of the series: > > > > > > > > > > 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? > > > > > > > > > > > > What do you mean by "meant to be"? > > > > > > > > > > > > > used in mobile devices. It has several modules: LDO, BUCK regulators, > > > > > > Modules or functions? > > > > > > > > > > > > > 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@gmail.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@gmail.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@schaufler-ca.com> > > > > > > > L: linux-security-module@vger.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? > > > > > > > > > > > > > + 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? > > > > > Should those devices still be present even though there's no driver for them > > > > > (yet) ? I have a WIP version of driver for almost every function, but I > > > > > currently lack time to get them done. > > > > Without them the driver-set is useless, no? > > > > > > > > We try to refrain from applying dead code. > > > > > > > > A lot of it has a tendency to stay that way. > > > Well, in my opinion, having just the regulator driver is already useful > > > enough: my board (Samsung Galaxy J5 (2015) ) uses one of LDO outputs for > > > powering the touchscreen. Touchscreen is kind of vital functionality for a > > > phone so I decided to upstream parts that are necessary for it first and > > > finish up other stuff later. It's not the only board that uses SM5703's > > > regulators for supplying all different kinds of hardware, either. > > Upstreaming functionality which is useful on its own is fine, but that > > doesn't tick all of the boxes to justify an MFD. This is a lot of > > code which is hard to justify if it only registers a Regulator driver. > Do you think I should hold on this series until I have other things done? > Alternatively, I could make the regulator driver standalone, dedicated, but > then when I'd add other functionality I'd have to redo it and add the MFD > driver back, that I believe would be quite annoying from maintainers' and > sanity perspective. The other functions left on the main control I2C are > also not really "vital" to device's functionality (Flash LED and charger), > so the regulator function makes the most sense to be available first, which > was my motivation behind upstreaming that first. What's the reason for this being an MFD in the first place?
Hi Lee, On 7/20/22 16:29, Lee Jones wrote: > On Wed, 20 Jul 2022, Markuss Broks wrote: > >> Hi Lee, >> >> On 7/20/22 11:22, Lee Jones wrote: >>> On Tue, 19 Jul 2022, Markuss Broks wrote: >>> >>>> Hi Lee, >>>> >>>> On 7/18/22 11:17, Lee Jones wrote: >>>>> On Fri, 15 Jul 2022, Markuss Broks wrote: >>>>> >>>>>> Hi Lee, >>>>>> >>>>>> Sorry to bother you again, but I've got additional questions while I was >>>>>> preparing the next version of the series: >>>>>> >>>>>> 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? >>>>>>> >>>>>>> What do you mean by "meant to be"? >>>>>>> >>>>>>>> used in mobile devices. It has several modules: LDO, BUCK regulators, >>>>>>> Modules or functions? >>>>>>> >>>>>>>> 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@gmail.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@gmail.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@schaufler-ca.com> >>>>>>>> L: linux-security-module@vger.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? >>>>>>> >>>>>>>> + 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? >>>>>> Should those devices still be present even though there's no driver for them >>>>>> (yet) ? I have a WIP version of driver for almost every function, but I >>>>>> currently lack time to get them done. >>>>> Without them the driver-set is useless, no? >>>>> >>>>> We try to refrain from applying dead code. >>>>> >>>>> A lot of it has a tendency to stay that way. >>>> Well, in my opinion, having just the regulator driver is already useful >>>> enough: my board (Samsung Galaxy J5 (2015) ) uses one of LDO outputs for >>>> powering the touchscreen. Touchscreen is kind of vital functionality for a >>>> phone so I decided to upstream parts that are necessary for it first and >>>> finish up other stuff later. It's not the only board that uses SM5703's >>>> regulators for supplying all different kinds of hardware, either. >>> Upstreaming functionality which is useful on its own is fine, but that >>> doesn't tick all of the boxes to justify an MFD. This is a lot of >>> code which is hard to justify if it only registers a Regulator driver. >> Do you think I should hold on this series until I have other things done? >> Alternatively, I could make the regulator driver standalone, dedicated, but >> then when I'd add other functionality I'd have to redo it and add the MFD >> driver back, that I believe would be quite annoying from maintainers' and >> sanity perspective. The other functions left on the main control I2C are >> also not really "vital" to device's functionality (Flash LED and charger), >> so the regulator function makes the most sense to be available first, which >> was my motivation behind upstreaming that first. > What's the reason for this being an MFD in the first place? Well, the "main" I2C interface is shared between three functions: regulators, Flash LED and charger. I call this one the "main" one because the device is controlled with it: you can select internal clock rate, enable or disable the PMIC and do other things that more of apply to the whole PMIC, not just the separate functions (as is the case with fuel gauge and MUIC I2C) . That's the reason for this being an MFD: those three functions share one I2C interface. While they might not be implemented at the moment, this places a foundation for adding future support. - Markuss
On Wed, 20 Jul 2022, Markuss Broks wrote: > On 7/20/22 16:29, Lee Jones wrote: > > On Wed, 20 Jul 2022, Markuss Broks wrote: > > > > > > > > > +static const struct mfd_cell sm5703_devs[] = { > > > > > > > > > + { .name = "sm5703-regulator", }, > > > > > > > > > +}; > > > > > > > > Where are the rest of the child drivers? > > > > > > > Should those devices still be present even though there's no driver for them > > > > > > > (yet) ? I have a WIP version of driver for almost every function, but I > > > > > > > currently lack time to get them done. > > > > > > Without them the driver-set is useless, no? > > > > > > > > > > > > We try to refrain from applying dead code. > > > > > > > > > > > > A lot of it has a tendency to stay that way. > > > > > Well, in my opinion, having just the regulator driver is already useful > > > > > enough: my board (Samsung Galaxy J5 (2015) ) uses one of LDO outputs for > > > > > powering the touchscreen. Touchscreen is kind of vital functionality for a > > > > > phone so I decided to upstream parts that are necessary for it first and > > > > > finish up other stuff later. It's not the only board that uses SM5703's > > > > > regulators for supplying all different kinds of hardware, either. > > > > Upstreaming functionality which is useful on its own is fine, but that > > > > doesn't tick all of the boxes to justify an MFD. This is a lot of > > > > code which is hard to justify if it only registers a Regulator driver. > > > Do you think I should hold on this series until I have other things done? > > > Alternatively, I could make the regulator driver standalone, dedicated, but > > > then when I'd add other functionality I'd have to redo it and add the MFD > > > driver back, that I believe would be quite annoying from maintainers' and > > > sanity perspective. The other functions left on the main control I2C are > > > also not really "vital" to device's functionality (Flash LED and charger), > > > so the regulator function makes the most sense to be available first, which > > > was my motivation behind upstreaming that first. Can you configure your mailer to stop removing the space between messages please? > > What's the reason for this being an MFD in the first place? > Well, the "main" I2C interface is shared between three functions: > regulators, Flash LED and charger. I call this one the "main" one because > the device is controlled with it: you can select internal clock rate, enable > or disable the PMIC and do other things that more of apply to the whole > PMIC, not just the separate functions (as is the case with fuel gauge and > MUIC I2C) . That's the reason for this being an MFD: those three functions > share one I2C interface. While they might not be implemented at the moment, > this places a foundation for adding future support. Okay, so the functions cannot be controlled separately? That's fine. For acceptance into MFD, you need to enable more than one function.
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@gmail.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@schaufler-ca.com> L: linux-security-module@vger.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 + device. This device is meant to be used in phones and it has numerous + modules, including LED controller, regulators, fuel gauge, MUIC and + charger circuit. It also support muxing a serial interface over USB + 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", }, +}; + +static const struct regmap_config sm5703_regmap_config = { + .reg_bits = 8, + .val_bits = 8, +}; + +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, + 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"); +MODULE_AUTHOR("Markuss Broks <markuss.broks@gmail.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 + +#define SM5703_REG_LDO1 0x1A +#define SM5703_REG_LDO2 0x1B +#define SM5703_REG_LDO3 0x1C +#define SM5703_LDO_EN BIT(3) +#define SM5703_LDO_VOLT_MASK 0x7 +#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 +#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
Silicon Mitus SM5703 is a multi-function device, meant to be used in mobile devices. It has several modules: LDO, BUCK regulators, 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 circuit, LED driver and regulators are on the main i2c line the device is controlled with. Signed-off-by: Markuss Broks <markuss.broks@gmail.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