From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754125AbdKABhF (ORCPT ); Tue, 31 Oct 2017 21:37:05 -0400 Received: from mail-oi0-f65.google.com ([209.85.218.65]:44227 "EHLO mail-oi0-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754044AbdKABhC (ORCPT ); Tue, 31 Oct 2017 21:37:02 -0400 X-Google-Smtp-Source: ABhQp+RzBGmo9byvogHKcDirQYPd7zbi1b1hoxj13mCNZrxL+/IKkig1JskKNTf8ZiCxStati/xQ9DFpThjPyalCYKE= MIME-Version: 1.0 In-Reply-To: <20171031152801.njf3ib7lrnu5e3ji@dell> References: <6f9ec429b84973201dc39aeaa31fb829c345b6e0.1508982713.git.baolin.wang@linaro.org> <20171031152801.njf3ib7lrnu5e3ji@dell> From: Baolin Wang Date: Wed, 1 Nov 2017 09:37:01 +0800 Message-ID: Subject: Re: [PATCH v3 2/2] mfd: Add Spreadtrum SC27xx series PMICs driver To: Lee Jones Cc: Baolin Wang , Rob Herring , Mark Rutland , devicetree@vger.kernel.org, LKML , Mark Brown Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Lee, On 31 October 2017 at 23:28, Lee Jones 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 >> --- >> 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