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

[Lee Jones <lee.jones@linaro.org>]

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?

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