linux-kernel - Re: [PATCH v3 2/2] mfd: Add Spreadtrum SC27xx series PMICs driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAMz4ku+3MMoj40=jpKRG=ySojG3+SGLsUqZseziHmzFKJWP=ug@mail.gmail.com>
Date:   Wed, 1 Nov 2017 09:37:01 +0800
From:   Baolin Wang <baolin.wang@...aro.org>
To:     Lee Jones <lee.jones@...aro.org>
Cc:     Baolin Wang <baolin.wang@...eadtrum.com>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        devicetree@...r.kernel.org, LKML <linux-kernel@...r.kernel.org>,
        Mark Brown <broonie@...nel.org>
Subject: Re: [PATCH v3 2/2] mfd: Add Spreadtrum SC27xx series PMICs driver

Hi Lee,

On 31 October 2017 at 23:28, Lee Jones <lee.jones@...aro.org> wrote:
> On Thu, 26 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@...eadtrum.com>
>> ---
>> Changes since v2:
>>  - Add more help information.
>>  - Define macros for irq base and irq number.
>>  - Use devm_mfd_add_devices() instead of mfd_add_devices(), which means
>>  we can remove sprd_pmic_remove().
>>  - Rename local variables in sprd_pmic_probe().
>>
>> 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.
>> ---
>>  drivers/mfd/Kconfig           |   16 +++
>>  drivers/mfd/Makefile          |    1 +
>>  drivers/mfd/sprd-sc27xx-spi.c |  258 +++++++++++++++++++++++++++++++++++++++++
>>  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;
>
> I still don't like this.  It's uuuuuugly!
>
> At the very least, please use an interim variable using a nomenclature
> which describes what it is you're trying to achieve.  For instance:
>
>   u32 *addr = reg;
>
>   rx_buf[0] = addr[0];
>
> Or better still ...
>
>   /* Copy address to read from into first element of SPI buffer */
>   memcpy(rx_buf, reg, sizeof(u32));

OK. I will change it like you suggested in next version. Thanks.

-- 
Baolin.wang
Best Regards