On 24 October 2017 at 18:37, Lee Jones <lee.jones@xxxxxxxxxx> wrote:
> On Tue, 24 Oct 2017, Baolin Wang wrote:
>
>> Hi Lee,
>>
>> On 24 October 2017 at 17:26, Lee Jones <lee.jones@xxxxxxxxxx> 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@xxxxxxxxxxxxxx>
>> >> ---
>> >> 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.
--
Baolin.wang
Best Regards
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
