| Message ID | 1dd82fa9d640d076e88b618c0788d27315a51f66.1508154918.git.baolin.wang@spreadtrum.com |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | [v2,1/2] dt-bindings: mfd: Add Spreadtrum SC27xx PMIC documentation | expand |
On Mon, 16 Oct 2017, Baolin Wang wrote: > This patch adds the binding documentation for Spreadtrum SC27xx series > PMIC device. > > Signed-off-by: Baolin Wang <baolin.wang@spreadtrum.com> > --- > Changes since v1: > - Add more documentation to introduce Spreadtrum SC27xx series PMICs. > - Modify compatile string property. > - Modify reg property. > - Remove redundant 'pmic' label. > - Change 'should be' to 'must be' for cells properties. > --- > .../devicetree/bindings/mfd/sprd,sc27xx-pmic.txt | 40 ++++++++++++++++++++ > 1 file changed, 40 insertions(+) > create mode 100644 Documentation/devicetree/bindings/mfd/sprd,sc27xx-pmic.txt Looks fine, but I'll give the DT guys a chance to have a look: Acked-by: Lee Jones <lee.jones@linaro.org>
On Mon, Oct 16, 2017 at 08:03:14PM +0800, Baolin Wang wrote: > This patch adds the binding documentation for Spreadtrum SC27xx series > PMIC device. > > Signed-off-by: Baolin Wang <baolin.wang@spreadtrum.com> > --- > Changes since v1: > - Add more documentation to introduce Spreadtrum SC27xx series PMICs. > - Modify compatile string property. > - Modify reg property. > - Remove redundant 'pmic' label. > - Change 'should be' to 'must be' for cells properties. > --- > .../devicetree/bindings/mfd/sprd,sc27xx-pmic.txt | 40 ++++++++++++++++++++ > 1 file changed, 40 insertions(+) > create mode 100644 Documentation/devicetree/bindings/mfd/sprd,sc27xx-pmic.txt Acked-by: Rob Herring <robh@kernel.org> -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On 24 October 2017 at 06:41, Rob Herring <robh@kernel.org> wrote: > On Mon, Oct 16, 2017 at 08:03:14PM +0800, Baolin Wang wrote: >> This patch adds the binding documentation for Spreadtrum SC27xx series >> PMIC device. >> >> Signed-off-by: Baolin Wang <baolin.wang@spreadtrum.com> >> --- >> Changes since v1: >> - Add more documentation to introduce Spreadtrum SC27xx series PMICs. >> - Modify compatile string property. >> - Modify reg property. >> - Remove redundant 'pmic' label. >> - Change 'should be' to 'must be' for cells properties. >> --- >> .../devicetree/bindings/mfd/sprd,sc27xx-pmic.txt | 40 ++++++++++++++++++++ >> 1 file changed, 40 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/mfd/sprd,sc27xx-pmic.txt > > Acked-by: Rob Herring <robh@kernel.org> Thanks Rob and Lee. Lee, could you apply this patchset into your next branch? Thanks.
On Mon, 16 Oct 2017, Baolin Wang wrote: > This patch adds support for Spreadtrum SC27xx series PMIC MFD core, and It > provides communication through the SPI interfaces. The SC27xx series PMICs > contains the following 6 major components: > - DCDCs > - LDOs > - Battery management system > - Audio codec > - User interface function, such as indicator, flash LED > - IC level function, such as power on/off, type-c > > Signed-off-by: Baolin Wang <baolin.wang@spreadtrum.com> > --- > Changes since v1: > - Re-order the including head files. > - Add one condition to check register size and value size when reading. > - Fix some coding style issues. > --- > drivers/mfd/Kconfig | 10 ++ > drivers/mfd/Makefile | 1 + > drivers/mfd/sprd-sc27xx-spi.c | 264 +++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 275 insertions(+) > create mode 100644 drivers/mfd/sprd-sc27xx-spi.c > > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig > index fc5e4fe..4753cd5 100644 > --- a/drivers/mfd/Kconfig > +++ b/drivers/mfd/Kconfig > @@ -1057,6 +1057,16 @@ config MFD_SMSC > To compile this driver as a module, choose M here: the > module will be called smsc. > > +config MFD_SC27XX_PMIC > + tristate "Spreadtrum SC27xx PMICs" > + depends on ARCH_SPRD || COMPILE_TEST > + depends on SPI_MASTER > + select MFD_CORE > + select REGMAP_SPI > + select REGMAP_IRQ > + help > + This enables support for the Spreadtrum SC27xx PMICs. More information here please. This looks like a pretty big device. It deserves more than a single line of help text. > config ABX500_CORE > bool "ST-Ericsson ABX500 Mixed Signal Circuit register functions" > default y if ARCH_U300 || ARCH_U8500 || COMPILE_TEST > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile > index c3d0a1b..a377e0f 100644 > --- a/drivers/mfd/Makefile > +++ b/drivers/mfd/Makefile > @@ -226,3 +226,4 @@ 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_SC27XX_PMIC) += sprd-sc27xx-spi.o > diff --git a/drivers/mfd/sprd-sc27xx-spi.c b/drivers/mfd/sprd-sc27xx-spi.c > new file mode 100644 > index 0000000..eb7ff5f > --- /dev/null > +++ b/drivers/mfd/sprd-sc27xx-spi.c > @@ -0,0 +1,264 @@ > +/* > + * Copyright (C) 2017 Spreadtrum Communications Inc. > + * > + * This software is licensed under the terms of the GNU General Public > + * License version 2, as published by the Free Software Foundation, and > + * may be copied, distributed, and modified under those terms. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + */ > + > +#include <linux/interrupt.h> > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/mfd/core.h> > +#include <linux/of_device.h> > +#include <linux/regmap.h> > +#include <linux/spi/spi.h> > + > +#define SPRD_PMIC_INT_MASK_STATUS 0x0 > +#define SPRD_PMIC_INT_RAW_STATU 0x4 > +#define SPRD_PMIC_INT_EN 0x8 > + > +struct sprd_pmic { > + struct regmap *regmap; > + struct device *dev; > + struct regmap_irq *irqs; > + struct regmap_irq_chip irq_chip; > + struct regmap_irq_chip_data *irq_data; > + int irq; > +}; > + > +struct sprd_pmic_data { > + u32 irq_base; > + u32 num_irqs; > +}; > + > +/* > + * Since different PMICs of SC27xx series can have different interrupt > + * base address and irq number, we should save irq number and irq base > + * in the device data structure. > + */ > +static const struct sprd_pmic_data sc2731_data = { > + .irq_base = 0x140, Does IRQ 0x140 have a name? Probably best to define it. > + .num_irqs = 16, > +}; > + > +static const struct mfd_cell sprd_pmic_devs[] = { > + { > + .name = "sc27xx-wdt", > + .of_compatible = "sprd,sc27xx-wdt", > + }, { > + .name = "sc27xx-rtc", > + .of_compatible = "sprd,sc27xx-rtc", > + }, { > + .name = "sc27xx-charger", > + .of_compatible = "sprd,sc27xx-charger", > + }, { > + .name = "sc27xx-chg-timer", > + .of_compatible = "sprd,sc27xx-chg-timer", > + }, { > + .name = "sc27xx-fast-chg", > + .of_compatible = "sprd,sc27xx-fast-chg", > + }, { > + .name = "sc27xx-chg-wdt", > + .of_compatible = "sprd,sc27xx-chg-wdt", > + }, { > + .name = "sc27xx-typec", > + .of_compatible = "sprd,sc27xx-typec", > + }, { > + .name = "sc27xx-flash", > + .of_compatible = "sprd,sc27xx-flash", > + }, { > + .name = "sc27xx-eic", > + .of_compatible = "sprd,sc27xx-eic", > + }, { > + .name = "sc27xx-efuse", > + .of_compatible = "sprd,sc27xx-efuse", > + }, { > + .name = "sc27xx-thermal", > + .of_compatible = "sprd,sc27xx-thermal", > + }, { > + .name = "sc27xx-adc", > + .of_compatible = "sprd,sc27xx-adc", > + }, { > + .name = "sc27xx-audio-codec", > + .of_compatible = "sprd,sc27xx-audio-codec", > + }, { > + .name = "sc27xx-regulator", > + .of_compatible = "sprd,sc27xx-regulator", > + }, { > + .name = "sc27xx-vibrator", > + .of_compatible = "sprd,sc27xx-vibrator", > + }, { > + .name = "sc27xx-keypad-led", > + .of_compatible = "sprd,sc27xx-keypad-led", > + }, { > + .name = "sc27xx-bltc", > + .of_compatible = "sprd,sc27xx-bltc", > + }, { > + .name = "sc27xx-fgu", > + .of_compatible = "sprd,sc27xx-fgu", > + }, { > + .name = "sc27xx-7sreset", > + .of_compatible = "sprd,sc27xx-7sreset", > + }, { > + .name = "sc27xx-poweroff", > + .of_compatible = "sprd,sc27xx-poweroff", > + }, > +}; > + > +static int sprd_pmic_spi_write(void *context, const void *data, size_t count) > +{ > + struct device *dev = context; > + struct spi_device *spi = to_spi_device(dev); > + > + return spi_write(spi, data, count); > +} > + > +static int sprd_pmic_spi_read(void *context, > + const void *reg, size_t reg_size, > + void *val, size_t val_size) > +{ > + struct device *dev = context; > + struct spi_device *spi = to_spi_device(dev); > + u32 rx_buf[2] = { 0 }; > + int ret; > + > + /* Now we only support one PMIC register to read every time. */ > + if (reg_size != sizeof(u32) || val_size != sizeof(u32)) > + return -EINVAL; > + > + rx_buf[0] = *(u32 *)reg; This looks dodgy. You should not be doing raw memory manipulation. You need to be passing __iomem and using one of the predefined APIs (i.e. readl) on it. > + ret = spi_read(spi, rx_buf, 1); > + if (ret < 0) > + return ret; > + > + memcpy(val, rx_buf, val_size); > + return 0; > +} > + > +static struct regmap_bus sprd_pmic_regmap = { > + .write = sprd_pmic_spi_write, > + .read = sprd_pmic_spi_read, > + .reg_format_endian_default = REGMAP_ENDIAN_NATIVE, > + .val_format_endian_default = REGMAP_ENDIAN_NATIVE, > +}; > + > +static const struct regmap_config sprd_pmic_config = { > + .reg_bits = 32, > + .val_bits = 32, > + .reg_stride = 4, > + .max_register = 0xffff, > +}; > + > +static int sprd_pmic_probe(struct spi_device *spi) > +{ > + struct sprd_pmic *sprd_pmic; Would prefer that you called this ddata. > + const struct sprd_pmic_data *data; Would prefer that you called this pdata. > + int ret, i; > + > + data = of_device_get_match_data(&spi->dev); > + if (!data) { > + dev_err(&spi->dev, "No matching driver data found\n"); > + return -EINVAL; > + } > + > + sprd_pmic = devm_kzalloc(&spi->dev, sizeof(*sprd_pmic), GFP_KERNEL); > + if (!sprd_pmic) > + return -ENOMEM; > + > + sprd_pmic->regmap = devm_regmap_init(&spi->dev, &sprd_pmic_regmap, > + &spi->dev, &sprd_pmic_config); > + if (IS_ERR(sprd_pmic->regmap)) { > + ret = PTR_ERR(sprd_pmic->regmap); > + dev_err(&spi->dev, "Failed to allocate register map %d\n", ret); > + return ret; > + } > + > + spi_set_drvdata(spi, sprd_pmic); > + sprd_pmic->dev = &spi->dev; > + sprd_pmic->irq = spi->irq; > + > + sprd_pmic->irq_chip.name = dev_name(&spi->dev); > + sprd_pmic->irq_chip.status_base = > + data->irq_base + SPRD_PMIC_INT_MASK_STATUS; > + sprd_pmic->irq_chip.mask_base = data->irq_base + SPRD_PMIC_INT_EN; > + sprd_pmic->irq_chip.ack_base = 0; > + sprd_pmic->irq_chip.num_regs = 1; > + sprd_pmic->irq_chip.num_irqs = data->num_irqs; > + sprd_pmic->irq_chip.mask_invert = true; > + > + sprd_pmic->irqs = devm_kzalloc(&spi->dev, sizeof(struct regmap_irq) * > + data->num_irqs, GFP_KERNEL); > + if (!sprd_pmic->irqs) > + return -ENOMEM; > + > + sprd_pmic->irq_chip.irqs = sprd_pmic->irqs; > + for (i = 0; i < data->num_irqs; i++) { > + sprd_pmic->irqs[i].reg_offset = i / data->num_irqs; > + sprd_pmic->irqs[i].mask = BIT(i % data->num_irqs); > + } > + > + ret = devm_regmap_add_irq_chip(&spi->dev, sprd_pmic->regmap, > + sprd_pmic->irq, > + IRQF_ONESHOT | IRQF_NO_SUSPEND, 0, > + &sprd_pmic->irq_chip, > + &sprd_pmic->irq_data); > + if (ret) { > + dev_err(&spi->dev, "Failed to add PMIC irq chip %d\n", ret); > + return ret; > + } > + > + ret = mfd_add_devices(&spi->dev, PLATFORM_DEVID_AUTO, sprd_pmic_devs, > + ARRAY_SIZE(sprd_pmic_devs), NULL, 0, > + regmap_irq_get_domain(sprd_pmic->irq_data)); > + if (ret) { > + dev_err(&spi->dev, "Failed to register device %d\n", ret); > + return ret; > + } > + > + return 0; > +} > + > +static int sprd_pmic_remove(struct spi_device *spi) > +{ > + mfd_remove_devices(&spi->dev); Anything stopping you from using devm_*? > + return 0; > +} > + > +static const struct of_device_id sprd_pmic_match[] = { > + { .compatible = "sprd,sc2731", .data = &sc2731_data }, > + {}, > +}; > +MODULE_DEVICE_TABLE(of, sprd_pmic_match); > + > +static struct spi_driver sprd_pmic_driver = { > + .driver = { > + .name = "sc27xx-pmic", > + .bus = &spi_bus_type, > + .of_match_table = sprd_pmic_match, of_match_ptr()? > + }, > + .probe = sprd_pmic_probe, > + .remove = sprd_pmic_remove, > +}; > + > +static int __init sprd_pmic_init(void) > +{ > + return spi_register_driver(&sprd_pmic_driver); > +} > +subsys_initcall(sprd_pmic_init); > + > +static void __exit sprd_pmic_exit(void) > +{ > + spi_unregister_driver(&sprd_pmic_driver); > +} > +module_exit(sprd_pmic_exit); > + > +MODULE_LICENSE("GPL v2"); > +MODULE_DESCRIPTION("SPI support for SC27xx PMICs"); "SPI support"? This is not an SPI driver. > +MODULE_AUTHOR("Baolin Wang <baolin.wang@spreadtrum.com>");
Hi Lee, On 24 October 2017 at 17:26, Lee Jones <lee.jones@linaro.org> wrote: > On Mon, 16 Oct 2017, Baolin Wang wrote: > >> This patch adds support for Spreadtrum SC27xx series PMIC MFD core, and It >> provides communication through the SPI interfaces. The SC27xx series PMICs >> contains the following 6 major components: >> - DCDCs >> - LDOs >> - Battery management system >> - Audio codec >> - User interface function, such as indicator, flash LED >> - IC level function, such as power on/off, type-c >> >> Signed-off-by: Baolin Wang <baolin.wang@spreadtrum.com> >> --- >> Changes since v1: >> - Re-order the including head files. >> - Add one condition to check register size and value size when reading. >> - Fix some coding style issues. >> --- >> drivers/mfd/Kconfig | 10 ++ >> drivers/mfd/Makefile | 1 + >> drivers/mfd/sprd-sc27xx-spi.c | 264 +++++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 275 insertions(+) >> create mode 100644 drivers/mfd/sprd-sc27xx-spi.c >> >> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig >> index fc5e4fe..4753cd5 100644 >> --- a/drivers/mfd/Kconfig >> +++ b/drivers/mfd/Kconfig >> @@ -1057,6 +1057,16 @@ config MFD_SMSC >> To compile this driver as a module, choose M here: the >> module will be called smsc. >> >> +config MFD_SC27XX_PMIC >> + tristate "Spreadtrum SC27xx PMICs" >> + depends on ARCH_SPRD || COMPILE_TEST >> + depends on SPI_MASTER >> + select MFD_CORE >> + select REGMAP_SPI >> + select REGMAP_IRQ >> + help >> + This enables support for the Spreadtrum SC27xx PMICs. > > More information here please. This looks like a pretty big device. > It deserves more than a single line of help text. OK. I will add more information. > >> config ABX500_CORE >> bool "ST-Ericsson ABX500 Mixed Signal Circuit register functions" >> default y if ARCH_U300 || ARCH_U8500 || COMPILE_TEST >> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile >> index c3d0a1b..a377e0f 100644 >> --- a/drivers/mfd/Makefile >> +++ b/drivers/mfd/Makefile >> @@ -226,3 +226,4 @@ 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_SC27XX_PMIC) += sprd-sc27xx-spi.o >> diff --git a/drivers/mfd/sprd-sc27xx-spi.c b/drivers/mfd/sprd-sc27xx-spi.c >> new file mode 100644 >> index 0000000..eb7ff5f >> --- /dev/null >> +++ b/drivers/mfd/sprd-sc27xx-spi.c >> @@ -0,0 +1,264 @@ >> +/* >> + * Copyright (C) 2017 Spreadtrum Communications Inc. >> + * >> + * This software is licensed under the terms of the GNU General Public >> + * License version 2, as published by the Free Software Foundation, and >> + * may be copied, distributed, and modified under those terms. >> + * >> + * This program is distributed in the hope that it will be useful, >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >> + * GNU General Public License for more details. >> + */ >> + >> +#include <linux/interrupt.h> >> +#include <linux/kernel.h> >> +#include <linux/module.h> >> +#include <linux/mfd/core.h> >> +#include <linux/of_device.h> >> +#include <linux/regmap.h> >> +#include <linux/spi/spi.h> >> + >> +#define SPRD_PMIC_INT_MASK_STATUS 0x0 >> +#define SPRD_PMIC_INT_RAW_STATU 0x4 >> +#define SPRD_PMIC_INT_EN 0x8 >> + >> +struct sprd_pmic { >> + struct regmap *regmap; >> + struct device *dev; >> + struct regmap_irq *irqs; >> + struct regmap_irq_chip irq_chip; >> + struct regmap_irq_chip_data *irq_data; >> + int irq; >> +}; >> + >> +struct sprd_pmic_data { >> + u32 irq_base; >> + u32 num_irqs; >> +}; >> + >> +/* >> + * Since different PMICs of SC27xx series can have different interrupt >> + * base address and irq number, we should save irq number and irq base >> + * in the device data structure. >> + */ >> +static const struct sprd_pmic_data sc2731_data = { >> + .irq_base = 0x140, > > Does IRQ 0x140 have a name? > > Probably best to define it. OK. I will define 0x140 and 16 with readable macros. > >> + .num_irqs = 16, >> +}; >> + >> +static const struct mfd_cell sprd_pmic_devs[] = { >> + { >> + .name = "sc27xx-wdt", >> + .of_compatible = "sprd,sc27xx-wdt", >> + }, { >> + .name = "sc27xx-rtc", >> + .of_compatible = "sprd,sc27xx-rtc", >> + }, { >> + .name = "sc27xx-charger", >> + .of_compatible = "sprd,sc27xx-charger", >> + }, { >> + .name = "sc27xx-chg-timer", >> + .of_compatible = "sprd,sc27xx-chg-timer", >> + }, { >> + .name = "sc27xx-fast-chg", >> + .of_compatible = "sprd,sc27xx-fast-chg", >> + }, { >> + .name = "sc27xx-chg-wdt", >> + .of_compatible = "sprd,sc27xx-chg-wdt", >> + }, { >> + .name = "sc27xx-typec", >> + .of_compatible = "sprd,sc27xx-typec", >> + }, { >> + .name = "sc27xx-flash", >> + .of_compatible = "sprd,sc27xx-flash", >> + }, { >> + .name = "sc27xx-eic", >> + .of_compatible = "sprd,sc27xx-eic", >> + }, { >> + .name = "sc27xx-efuse", >> + .of_compatible = "sprd,sc27xx-efuse", >> + }, { >> + .name = "sc27xx-thermal", >> + .of_compatible = "sprd,sc27xx-thermal", >> + }, { >> + .name = "sc27xx-adc", >> + .of_compatible = "sprd,sc27xx-adc", >> + }, { >> + .name = "sc27xx-audio-codec", >> + .of_compatible = "sprd,sc27xx-audio-codec", >> + }, { >> + .name = "sc27xx-regulator", >> + .of_compatible = "sprd,sc27xx-regulator", >> + }, { >> + .name = "sc27xx-vibrator", >> + .of_compatible = "sprd,sc27xx-vibrator", >> + }, { >> + .name = "sc27xx-keypad-led", >> + .of_compatible = "sprd,sc27xx-keypad-led", >> + }, { >> + .name = "sc27xx-bltc", >> + .of_compatible = "sprd,sc27xx-bltc", >> + }, { >> + .name = "sc27xx-fgu", >> + .of_compatible = "sprd,sc27xx-fgu", >> + }, { >> + .name = "sc27xx-7sreset", >> + .of_compatible = "sprd,sc27xx-7sreset", >> + }, { >> + .name = "sc27xx-poweroff", >> + .of_compatible = "sprd,sc27xx-poweroff", >> + }, >> +}; >> + >> +static int sprd_pmic_spi_write(void *context, const void *data, size_t count) >> +{ >> + struct device *dev = context; >> + struct spi_device *spi = to_spi_device(dev); >> + >> + return spi_write(spi, data, count); >> +} >> + >> +static int sprd_pmic_spi_read(void *context, >> + const void *reg, size_t reg_size, >> + void *val, size_t val_size) >> +{ >> + struct device *dev = context; >> + struct spi_device *spi = to_spi_device(dev); >> + u32 rx_buf[2] = { 0 }; >> + int ret; >> + >> + /* Now we only support one PMIC register to read every time. */ >> + if (reg_size != sizeof(u32) || val_size != sizeof(u32)) >> + return -EINVAL; >> + >> + rx_buf[0] = *(u32 *)reg; > > This looks dodgy. You should not be doing raw memory manipulation. > > You need to be passing __iomem and using one of the predefined APIs > (i.e. readl) on it. Sorry, I did not get you here. The 'reg' parameter is just one PMIC register offset which want to be read. We do not need to pass __iomem to SPI master driver, we just pass one register offset address to our SPI master driver then the SPI master driver will handle to read values from our PMIC. > >> + ret = spi_read(spi, rx_buf, 1); >> + if (ret < 0) >> + return ret; >> + >> + memcpy(val, rx_buf, val_size); >> + return 0; >> +} >> + >> +static struct regmap_bus sprd_pmic_regmap = { >> + .write = sprd_pmic_spi_write, >> + .read = sprd_pmic_spi_read, >> + .reg_format_endian_default = REGMAP_ENDIAN_NATIVE, >> + .val_format_endian_default = REGMAP_ENDIAN_NATIVE, >> +}; >> + >> +static const struct regmap_config sprd_pmic_config = { >> + .reg_bits = 32, >> + .val_bits = 32, >> + .reg_stride = 4, >> + .max_register = 0xffff, >> +}; >> + >> +static int sprd_pmic_probe(struct spi_device *spi) >> +{ >> + struct sprd_pmic *sprd_pmic; > > Would prefer that you called this ddata. > >> + const struct sprd_pmic_data *data; > > Would prefer that you called this pdata. Sure. > >> + int ret, i; >> + >> + data = of_device_get_match_data(&spi->dev); >> + if (!data) { >> + dev_err(&spi->dev, "No matching driver data found\n"); >> + return -EINVAL; >> + } >> + >> + sprd_pmic = devm_kzalloc(&spi->dev, sizeof(*sprd_pmic), GFP_KERNEL); >> + if (!sprd_pmic) >> + return -ENOMEM; >> + >> + sprd_pmic->regmap = devm_regmap_init(&spi->dev, &sprd_pmic_regmap, >> + &spi->dev, &sprd_pmic_config); >> + if (IS_ERR(sprd_pmic->regmap)) { >> + ret = PTR_ERR(sprd_pmic->regmap); >> + dev_err(&spi->dev, "Failed to allocate register map %d\n", ret); >> + return ret; >> + } >> + >> + spi_set_drvdata(spi, sprd_pmic); >> + sprd_pmic->dev = &spi->dev; >> + sprd_pmic->irq = spi->irq; >> + >> + sprd_pmic->irq_chip.name = dev_name(&spi->dev); >> + sprd_pmic->irq_chip.status_base = >> + data->irq_base + SPRD_PMIC_INT_MASK_STATUS; >> + sprd_pmic->irq_chip.mask_base = data->irq_base + SPRD_PMIC_INT_EN; >> + sprd_pmic->irq_chip.ack_base = 0; >> + sprd_pmic->irq_chip.num_regs = 1; >> + sprd_pmic->irq_chip.num_irqs = data->num_irqs; >> + sprd_pmic->irq_chip.mask_invert = true; >> + >> + sprd_pmic->irqs = devm_kzalloc(&spi->dev, sizeof(struct regmap_irq) * >> + data->num_irqs, GFP_KERNEL); >> + if (!sprd_pmic->irqs) >> + return -ENOMEM; >> + >> + sprd_pmic->irq_chip.irqs = sprd_pmic->irqs; >> + for (i = 0; i < data->num_irqs; i++) { >> + sprd_pmic->irqs[i].reg_offset = i / data->num_irqs; >> + sprd_pmic->irqs[i].mask = BIT(i % data->num_irqs); >> + } >> + >> + ret = devm_regmap_add_irq_chip(&spi->dev, sprd_pmic->regmap, >> + sprd_pmic->irq, >> + IRQF_ONESHOT | IRQF_NO_SUSPEND, 0, >> + &sprd_pmic->irq_chip, >> + &sprd_pmic->irq_data); >> + if (ret) { >> + dev_err(&spi->dev, "Failed to add PMIC irq chip %d\n", ret); >> + return ret; >> + } >> + >> + ret = mfd_add_devices(&spi->dev, PLATFORM_DEVID_AUTO, sprd_pmic_devs, >> + ARRAY_SIZE(sprd_pmic_devs), NULL, 0, >> + regmap_irq_get_domain(sprd_pmic->irq_data)); >> + if (ret) { >> + dev_err(&spi->dev, "Failed to register device %d\n", ret); >> + return ret; >> + } >> + >> + return 0; >> +} >> + >> +static int sprd_pmic_remove(struct spi_device *spi) >> +{ >> + mfd_remove_devices(&spi->dev); > > Anything stopping you from using devm_*? Ah, I missed devm_* and I will change to use devm_* in next version. > >> + return 0; >> +} >> + >> +static const struct of_device_id sprd_pmic_match[] = { >> + { .compatible = "sprd,sc2731", .data = &sc2731_data }, >> + {}, >> +}; >> +MODULE_DEVICE_TABLE(of, sprd_pmic_match); >> + >> +static struct spi_driver sprd_pmic_driver = { >> + .driver = { >> + .name = "sc27xx-pmic", >> + .bus = &spi_bus_type, >> + .of_match_table = sprd_pmic_match, > > of_match_ptr()? The of_match_ptr() did nothing, so I think it is not necessary. > >> + }, >> + .probe = sprd_pmic_probe, >> + .remove = sprd_pmic_remove, >> +}; >> + >> +static int __init sprd_pmic_init(void) >> +{ >> + return spi_register_driver(&sprd_pmic_driver); >> +} >> +subsys_initcall(sprd_pmic_init); >> + >> +static void __exit sprd_pmic_exit(void) >> +{ >> + spi_unregister_driver(&sprd_pmic_driver); >> +} >> +module_exit(sprd_pmic_exit); >> + >> +MODULE_LICENSE("GPL v2"); >> +MODULE_DESCRIPTION("SPI support for SC27xx PMICs"); > > "SPI support"? This is not an SPI driver. OK, I will modify the driver description. Thanks for your comments.
On Tue, 24 Oct 2017, Baolin Wang wrote: > Hi Lee, > > On 24 October 2017 at 17:26, Lee Jones <lee.jones@linaro.org> wrote: > > On Mon, 16 Oct 2017, Baolin Wang wrote: > > > >> This patch adds support for Spreadtrum SC27xx series PMIC MFD core, and It > >> provides communication through the SPI interfaces. The SC27xx series PMICs > >> contains the following 6 major components: > >> - DCDCs > >> - LDOs > >> - Battery management system > >> - Audio codec > >> - User interface function, such as indicator, flash LED > >> - IC level function, such as power on/off, type-c > >> > >> Signed-off-by: Baolin Wang <baolin.wang@spreadtrum.com> > >> --- > >> Changes since v1: > >> - Re-order the including head files. > >> - Add one condition to check register size and value size when reading. > >> - Fix some coding style issues. > >> --- > >> drivers/mfd/Kconfig | 10 ++ > >> drivers/mfd/Makefile | 1 + > >> drivers/mfd/sprd-sc27xx-spi.c | 264 +++++++++++++++++++++++++++++++++++++++++ > >> 3 files changed, 275 insertions(+) > >> create mode 100644 drivers/mfd/sprd-sc27xx-spi.c [...] > >> +static int sprd_pmic_spi_read(void *context, > >> + const void *reg, size_t reg_size, > >> + void *val, size_t val_size) > >> +{ > >> + struct device *dev = context; > >> + struct spi_device *spi = to_spi_device(dev); > >> + u32 rx_buf[2] = { 0 }; > >> + int ret; > >> + > >> + /* Now we only support one PMIC register to read every time. */ > >> + if (reg_size != sizeof(u32) || val_size != sizeof(u32)) > >> + return -EINVAL; > >> + > >> + rx_buf[0] = *(u32 *)reg; > > > > This looks dodgy. You should not be doing raw memory manipulation. > > > > You need to be passing __iomem and using one of the predefined APIs > > (i.e. readl) on it. > > Sorry, I did not get you here. The 'reg' parameter is just one PMIC > register offset which want to be read. We do not need to pass __iomem > to SPI master driver, we just pass one register offset address to our > SPI master driver then the SPI master driver will handle to read > values from our PMIC. You should not be referencing the value at 'reg' manually: rx_buf[0] = readl(reg); [...] > >> +static struct spi_driver sprd_pmic_driver = { > >> + .driver = { > >> + .name = "sc27xx-pmic", > >> + .bus = &spi_bus_type, > >> + .of_match_table = sprd_pmic_match, > > > > of_match_ptr()? > > The of_match_ptr() did nothing, so I think it is not necessary. What do you mean?
On 24 October 2017 at 18:37, Lee Jones <lee.jones@linaro.org> wrote: > On Tue, 24 Oct 2017, Baolin Wang wrote: > >> Hi Lee, >> >> On 24 October 2017 at 17:26, Lee Jones <lee.jones@linaro.org> wrote: >> > On Mon, 16 Oct 2017, Baolin Wang wrote: >> > >> >> This patch adds support for Spreadtrum SC27xx series PMIC MFD core, and It >> >> provides communication through the SPI interfaces. The SC27xx series PMICs >> >> contains the following 6 major components: >> >> - DCDCs >> >> - LDOs >> >> - Battery management system >> >> - Audio codec >> >> - User interface function, such as indicator, flash LED >> >> - IC level function, such as power on/off, type-c >> >> >> >> Signed-off-by: Baolin Wang <baolin.wang@spreadtrum.com> >> >> --- >> >> Changes since v1: >> >> - Re-order the including head files. >> >> - Add one condition to check register size and value size when reading. >> >> - Fix some coding style issues. >> >> --- >> >> drivers/mfd/Kconfig | 10 ++ >> >> drivers/mfd/Makefile | 1 + >> >> drivers/mfd/sprd-sc27xx-spi.c | 264 +++++++++++++++++++++++++++++++++++++++++ >> >> 3 files changed, 275 insertions(+) >> >> create mode 100644 drivers/mfd/sprd-sc27xx-spi.c > > [...] > >> >> +static int sprd_pmic_spi_read(void *context, >> >> + const void *reg, size_t reg_size, >> >> + void *val, size_t val_size) >> >> +{ >> >> + struct device *dev = context; >> >> + struct spi_device *spi = to_spi_device(dev); >> >> + u32 rx_buf[2] = { 0 }; >> >> + int ret; >> >> + >> >> + /* Now we only support one PMIC register to read every time. */ >> >> + if (reg_size != sizeof(u32) || val_size != sizeof(u32)) >> >> + return -EINVAL; >> >> + >> >> + rx_buf[0] = *(u32 *)reg; >> > >> > This looks dodgy. You should not be doing raw memory manipulation. >> > >> > You need to be passing __iomem and using one of the predefined APIs >> > (i.e. readl) on it. >> >> Sorry, I did not get you here. The 'reg' parameter is just one PMIC >> register offset which want to be read. We do not need to pass __iomem >> to SPI master driver, we just pass one register offset address to our >> SPI master driver then the SPI master driver will handle to read >> values from our PMIC. > > You should not be referencing the value at 'reg' manually: > > rx_buf[0] = readl(reg); This is not true, like I said the 'reg' is not __iomem address, it is just one register offset, we should pass the reg offset to SPI master to read. > [...] > >> >> +static struct spi_driver sprd_pmic_driver = { >> >> + .driver = { >> >> + .name = "sc27xx-pmic", >> >> + .bus = &spi_bus_type, >> >> + .of_match_table = sprd_pmic_match, >> > >> > of_match_ptr()? >> >> The of_match_ptr() did nothing, so I think it is not necessary. > > What do you mean? sorry, what I mean is we must enable CONFIG_OF if we want to use this driver, so of_match_ptr() seems redundant.
diff --git a/Documentation/devicetree/bindings/mfd/sprd,sc27xx-pmic.txt b/Documentation/devicetree/bindings/mfd/sprd,sc27xx-pmic.txt new file mode 100644 index 0000000..21b9a89 --- /dev/null +++ b/Documentation/devicetree/bindings/mfd/sprd,sc27xx-pmic.txt @@ -0,0 +1,40 @@ +Spreadtrum SC27xx Power Management Integrated Circuit (PMIC) + +The Spreadtrum SC27xx series PMICs contain SC2720, SC2721, SC2723, SC2730 +and SC2731. The Spreadtrum PMIC belonging to SC27xx series integrates all +mobile handset power management, audio codec, battery management and user +interface support function in a single chip. It has 6 major functional +blocks: +- DCDCs to support CPU, memory. +- LDOs to support both internal and external requirement. +- Battery management system, such as charger, fuel gauge. +- Audio codec. +- User interface function, such as indicator, flash LED and so on. +- IC level interface, such as power on/off control, RTC and typec and so on. + +Required properties: +- compatible: Should be one of the following: + "sprd,sc2720" + "sprd,sc2721" + "sprd,sc2723" + "sprd,sc2730" + "sprd,sc2731" +- reg: The address of the device chip select, should be 0. +- spi-max-frequency: Typically set to 26000000. +- interrupts: The interrupt line the device is connected to. +- interrupt-controller: Marks the device node as an interrupt controller. +- #interrupt-cells: The number of cells to describe an PMIC IRQ, must be 2. +- #address-cells: Child device offset number of cells, must be 1. +- #size-cells: Child device size number of cells, must be 0. + +Example: +pmic@0 { + compatible = "sprd,sc2731"; + reg = <0>; + spi-max-frequency = <26000000>; + interrupts = <GIC_SPI 31 IRQ_TYPE_LEVEL_HIGH>; + interrupt-controller; + #interrupt-cells = <2>; + #address-cells = <1>; + #size-cells = <0>; +};
This patch adds the binding documentation for Spreadtrum SC27xx series PMIC device. Signed-off-by: Baolin Wang <baolin.wang@spreadtrum.com> --- Changes since v1: - Add more documentation to introduce Spreadtrum SC27xx series PMICs. - Modify compatile string property. - Modify reg property. - Remove redundant 'pmic' label. - Change 'should be' to 'must be' for cells properties. --- .../devicetree/bindings/mfd/sprd,sc27xx-pmic.txt | 40 ++++++++++++++++++++ 1 file changed, 40 insertions(+) create mode 100644 Documentation/devicetree/bindings/mfd/sprd,sc27xx-pmic.txt