From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753211AbdEJSib (ORCPT ); Wed, 10 May 2017 14:38:31 -0400 Received: from mx0a-001b2d01.pphosted.com ([148.163.156.1]:37443 "EHLO mx0a-001b2d01.pphosted.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752901AbdEJSi3 (ORCPT ); Wed, 10 May 2017 14:38:29 -0400 Subject: Re: [PATCH v6 10/23] drivers/fsi: Add device read/write/peek API To: Joel Stanley References: <20170410194706.64280-1-cbostic@linux.vnet.ibm.com> <20170410194706.64280-11-cbostic@linux.vnet.ibm.com> Cc: Rob Herring , Mark Rutland , Russell King , rostedt@goodmis.org, mingo@redhat.com, Greg KH , devicetree , linux-arm-kernel@lists.infradead.org, Jeremy Kerr , Linux Kernel Mailing List , Andrew Jeffery , Alistair Popple , Benjamin Herrenschmidt , "Edward A . James" From: Christopher Bostic Date: Wed, 10 May 2017 13:38:21 -0500 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:45.0) Gecko/20100101 Thunderbird/45.7.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit X-TM-AS-GCONF: 00 x-cbid: 17051018-0028-0000-0000-0000078FD396 X-IBM-SpamModules-Scores: X-IBM-SpamModules-Versions: BY=3.00007043; HX=3.00000240; KW=3.00000007; PH=3.00000004; SC=3.00000210; SDB=6.00858776; UDB=6.00425552; IPR=6.00638250; BA=6.00005342; NDR=6.00000001; ZLA=6.00000005; ZF=6.00000009; ZB=6.00000000; ZP=6.00000000; ZH=6.00000000; ZU=6.00000002; MB=3.00015399; XFM=3.00000015; UTC=2017-05-10 18:38:26 X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused x-cbparentid: 17051018-0029-0000-0000-000035B1CE0D Message-Id: <4236d20b-be1e-6458-8a69-7de98bfc47ca@linux.vnet.ibm.com> X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2017-05-10_14:,, signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 spamscore=0 suspectscore=0 malwarescore=0 phishscore=0 adultscore=0 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1703280000 definitions=main-1705100126 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 5/10/17 3:13 AM, Joel Stanley wrote: > On Tue, Apr 11, 2017 at 5:16 AM, Christopher Bostic > wrote: >> From: Jeremy Kerr >> >> This change introduces the fsi device API: simple read, write and peek >> accessors for the devices' address spaces. >> >> Includes contributions from Chris Bostic >> and Edward A. James . >> >> Signed-off-by: Edward A. James >> Signed-off-by: Jeremy Kerr >> Signed-off-by: Chris Bostic >> Signed-off-by: Joel Stanley >> --- >> drivers/fsi/fsi-core.c | 33 +++++++++++++++++++++++++++++++++ >> include/linux/fsi.h | 7 ++++++- >> 2 files changed, 39 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/fsi/fsi-core.c b/drivers/fsi/fsi-core.c >> index a8faa89..4da0b030 100644 >> --- a/drivers/fsi/fsi-core.c >> +++ b/drivers/fsi/fsi-core.c >> @@ -32,6 +32,8 @@ >> #define FSI_SLAVE_CONF_CRC_MASK 0x0000000f >> #define FSI_SLAVE_CONF_DATA_BITS 28 >> >> +#define FSI_PEEK_BASE 0x410 >> + >> static const int engine_page_size = 0x400; >> >> #define FSI_SLAVE_BASE 0x800 >> @@ -73,9 +75,40 @@ static int fsi_master_read(struct fsi_master *master, int link, >> uint8_t slave_id, uint32_t addr, void *val, size_t size); >> static int fsi_master_write(struct fsi_master *master, int link, >> uint8_t slave_id, uint32_t addr, const void *val, size_t size); >> +static int fsi_slave_read(struct fsi_slave *slave, uint32_t addr, >> + void *val, size_t size); >> +static int fsi_slave_write(struct fsi_slave *slave, uint32_t addr, >> + const void *val, size_t size); > I don't think these >> /* FSI endpoint-device support */ >> >> +int fsi_device_read(struct fsi_device *dev, uint32_t addr, void *val, >> + size_t size) > I suggest adding some comments about the use of fsi_device_read, > fsi_device_write and fsi_device_peek APIs for driver authors. > > One important note is that the data in val must be FSI bus endian (big endian). Hi Joel, OK will add some comments here. > It would be nice to have the sparse notation (eg. __be32) as well. > That doesn't make sense for void *, but we could add some helper > functions like: > > int fsi_device_read32(struct fsi_device *dev, uint32_t addr, __be32 *val) > > int fsi_device_write32(struct fsi_device *dev, uint32_t addr, __be32 val) > > We probably only need a 32 bit version, as all of the reads/writes > being done in users of this function are 32 bit. > What do you think? These functions will need to support reads/writes of 8 bit and 16 bit values. Thanks -Chris >> +{ >> + if (addr > dev->size || size > dev->size || addr > dev->size - size) >> + return -EINVAL; >> + >> + return fsi_slave_read(dev->slave, dev->addr + addr, val, size); >> +} >> +EXPORT_SYMBOL_GPL(fsi_device_read); >> + >> +int fsi_device_write(struct fsi_device *dev, uint32_t addr, const void *val, >> + size_t size) >> +{ >> + if (addr > dev->size || size > dev->size || addr > dev->size - size) >> + return -EINVAL; >> + >> + return fsi_slave_write(dev->slave, dev->addr + addr, val, size); >> +} >> +EXPORT_SYMBOL_GPL(fsi_device_write); >> + >> +int fsi_device_peek(struct fsi_device *dev, void *val) >> +{ >> + uint32_t addr = FSI_PEEK_BASE + ((dev->unit - 2) * sizeof(uint32_t)); >> + >> + return fsi_slave_read(dev->slave, addr, val, sizeof(uint32_t)); >> +} >> + >> static void fsi_device_release(struct device *_device) >> { >> struct fsi_device *device = to_fsi_dev(_device); >> diff --git a/include/linux/fsi.h b/include/linux/fsi.h >> index efa55ba..66bce48 100644 >> --- a/include/linux/fsi.h >> +++ b/include/linux/fsi.h >> @@ -27,6 +27,12 @@ struct fsi_device { >> uint32_t size; >> }; >> >> +extern int fsi_device_read(struct fsi_device *dev, uint32_t addr, >> + void *val, size_t size); >> +extern int fsi_device_write(struct fsi_device *dev, uint32_t addr, >> + const void *val, size_t size); >> +extern int fsi_device_peek(struct fsi_device *dev, void *val); >> + >> struct fsi_device_id { >> u8 engine_type; >> u8 version; >> @@ -40,7 +46,6 @@ struct fsi_device_id { >> #define FSI_DEVICE_VERSIONED(t, v) \ >> .engine_type = (t), .version = (v), >> >> - >> struct fsi_driver { >> struct device_driver drv; >> const struct fsi_device_id *id_table; >> -- >> 1.8.2.2 >>