Re: [PATCH v6 10/23] drivers/fsi: Add device read/write/peek API

[Christopher Bostic <cbostic-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>]

On 5/10/17 3:13 AM, Joel Stanley wrote:
> On Tue, Apr 11, 2017 at 5:16 AM, Christopher Bostic
> <cbostic-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org> wrote:
>> From: Jeremy Kerr <jk-mnsaURCQ41sdnm+yROfE0A@public.gmane.org>
>>
>> This change introduces the fsi device API: simple read, write and peek
>> accessors for the devices' address spaces.
>>
>> Includes contributions from Chris Bostic <cbostic-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
>> and Edward A. James <eajames-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>.
>>
>> Signed-off-by: Edward A. James <eajames-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
>> Signed-off-by: Jeremy Kerr <jk-mnsaURCQ41sdnm+yROfE0A@public.gmane.org>
>> Signed-off-by: Chris Bostic <cbostic-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
>> Signed-off-by: Joel Stanley <joel-U3u1mxZcP9KHXe+LvDLADg@public.gmane.org>
>> ---
>>   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