From: Baolin Wang <baolin.wang@linaro.org>
To: Lee Jones <lee.jones@linaro.org>
Cc: Baolin Wang <baolin.wang@spreadtrum.com>,
Rob Herring <robh+dt@kernel.org>,
Mark Rutland <mark.rutland@arm.com>,
devicetree@vger.kernel.org, LKML <linux-kernel@vger.kernel.org>,
Mark Brown <broonie@kernel.org>
Subject: Re: [PATCH v3 2/2] mfd: Add Spreadtrum SC27xx series PMICs driver
Date: Wed, 1 Nov 2017 09:37:01 +0800 [thread overview]
Message-ID: <CAMz4ku+3MMoj40=jpKRG=ySojG3+SGLsUqZseziHmzFKJWP=ug@mail.gmail.com> (raw)
In-Reply-To: <20171031152801.njf3ib7lrnu5e3ji@dell>
Hi Lee,
On 31 October 2017 at 23:28, Lee Jones <lee.jones@linaro.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@spreadtrum.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
prev parent reply other threads:[~2017-11-01 1:37 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-10-26 1:57 [PATCH v3 1/2] dt-bindings: mfd: Add Spreadtrum SC27xx PMIC documentation Baolin Wang
2017-10-26 1:57 ` [PATCH v3 2/2] mfd: Add Spreadtrum SC27xx series PMICs driver Baolin Wang
2017-10-31 15:28 ` Lee Jones
2017-11-01 1:37 ` Baolin Wang [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAMz4ku+3MMoj40=jpKRG=ySojG3+SGLsUqZseziHmzFKJWP=ug@mail.gmail.com' \
--to=baolin.wang@linaro.org \
--cc=baolin.wang@spreadtrum.com \
--cc=broonie@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=lee.jones@linaro.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mark.rutland@arm.com \
--cc=robh+dt@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).