Re: [PATCH 2/2] mfd: Add Spreadtrum SC27xx series PMICs driver

[Lee Jones]

On Fri, 13 Oct 2017, Baolin Wang wrote:
> On 13 October 2017 at 17:26, Lee Jones <lee.jones@xxxxxxxxxx> wrote:
> > On Wed, 11 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@xxxxxxxxxxxxxx>
> >> ---
> >>  drivers/mfd/Kconfig           |   10 ++
> >>  drivers/mfd/Makefile          |    1 +
> >>  drivers/mfd/sprd-sc27xx-spi.c |  273 +++++++++++++++++++++++++++++++++++++++++
> >>  3 files changed, 284 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.
> >> +
> >>  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..2b3f2bc
> >> --- /dev/null
> >> +++ b/drivers/mfd/sprd-sc27xx-spi.c
> >> @@ -0,0 +1,273 @@
> >> +/*
> >> + * 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.
> >> + */

[...]

> >> +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;
> >> +
> >> +     rx_buf[0] = *(u32 *)reg;
> >> +     ret = spi_read(spi, rx_buf, 1);
> >> +     if (ret < 0)
> >> +             return ret;
> >> +
> >> +     memcpy(val, rx_buf, val_size);
> >
> > This looks like you're asking for trouble.
> >
> > What if val_size > sizeof(rx_buf)?
> >
> > You should probably check for that.
> 
> Since we always read one register every time, which means the reg_size
> and val_size are always 4. But it will be more helpful to check
> incorrect parameters by adding one check here.
> 
> >
> >> +     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;
> >> +     const struct sprd_pmic_data *data;
> >> +     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);
> >> +     }
> >
> > Why don't you do this live, instead of pre-calculating?
> 
> Since the SC27xx series PMICs have different interrupts number (such
> as SC2731 has 16 interrupts and SC2720 has 6 interrupts).

Right, so save num_irqs in your device data structure.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org â Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog