From mboxrd@z Thu Jan 1 00:00:00 1970 From: Christopher Bostic Subject: Re: [PATCH v6 10/23] drivers/fsi: Add device read/write/peek API Date: Wed, 10 May 2017 13:38:21 -0500 Message-ID: <4236d20b-be1e-6458-8a69-7de98bfc47ca@linux.vnet.ibm.com> References: <20170410194706.64280-1-cbostic@linux.vnet.ibm.com> <20170410194706.64280-11-cbostic@linux.vnet.ibm.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: Sender: devicetree-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Joel Stanley Cc: Rob Herring , Mark Rutland , Russell King , rostedt-nx8X9YLhiw1AfugRpC6u6w@public.gmane.org, mingo-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org, Greg KH , devicetree , linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org, Jeremy Kerr , Linux Kernel Mailing List , Andrew Jeffery , Alistair Popple , Benjamin Herrenschmidt , "Edward A . James" List-Id: devicetree@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 >> -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html