[PATCH linux v5 17/18] drivers/fsi: Add SCOM FSI client device driver

[Christopher Bostic]

On Wed, Oct 19, 2016 at 10:27 PM, Jeremy Kerr <jk at ozlabs.org> wrote:
> Hi Chris,
>
>> diff > +static int scom_probe(struct device *);
>> +
>> +struct scom_op {
>> +     uint64_t        data;
>> +     uint32_t        addr;
>> +};
>
> You're making this a userspace interface, so this should go into the
> uapi headers (and then will form part of the unchangeable Linux ABI).
> However, more on the ABI below...
>
>> +struct scom_device {
>> +     struct fsi_device *fsi_dev;
>> +     struct cdev *cdev;
>> +};
>> +
>> +struct scom_device scom_dev;
>> +
>> +struct fsi_device_id scom_ids[] = {
>> +     {
>> +             .engine_type = FSI_ENGID_SCOM,
>> +             .version = FSI_VERSION_ANY,
>> +     },
>> +     { 0 }
>> +};
>> +
>> +struct fsi_driver scom_drv = {
>> +     .id_table = scom_ids,
>> +     .drv = {
>> +             .name = "scom",
>> +             .bus = &fsi_bus_type,
>> +             .probe = scom_probe,
>> +     }
>> +};
>
> Make these static, and put after the code, just before the
> module_fsi_driver. Then you won't need the forward definition of
> scom_probe.
>

Hi Jeremy,

OK will change.

>> > +static ssize_t scom_read(struct file *filep, char __user *buf, size_t len,
>> +                     loff_t *offset)
>> +{
>> +     int rc = 0;
>> +     struct scom_op op;
>> +     struct scom_device *scom = &scom_dev;
>> +     struct device *dev = &scom->fsi_dev->dev;
>> +
>> +     /* todo: Retrieve device to operate on from filep->private_data */
>
> Yep, otherwise you only get one scom interface, and crazy results if you
> get probed twice. I'd suggest not leaving this until later.
>

Will add.

>> +
>> +     if (len != sizeof(struct scom_op))
>> +             return -EINVAL;
>> +
>> +     rc = copy_from_user(&op, buf, len);
>> +     if (rc) {
>> +             dev_dbg(dev, "copy from user failed:%d\n", rc);
>> +             return -EINVAL;
>> +     }
>> +
>> +     rc = get_scom(&op.data, op.addr);
>
> I'm not a huge fan of that ABI to userspace. You already have the
> address in the offset argument, so you can do this without needing new
> uapi headers, using something like:
>
> static ssize_t scom_read(struct file *filep, char __user *buf, size_t len,
>                         loff_t *offset)
> {
>         uint64_t val;
>
>         /* todo: can we do 1/2/4/8 byte operations too? */
>         if (len != sizeof(uint64_t))
>                 return -EINVAL;
>
>         rc = get_scom(&val, offset);
>         if (rc)
>                 return rc;
>
>         rc = copy_to_user(&val, buf, len);
>
>         return rc ? rc : len;
> }
>

OK, will change that.

>
> Also, have you thought about endianness?
>
>

The plan is for endianness is to be dealt with in the fsi-core
so the other engines don't need to worry about it.  Yes there
is likely to be some additions there.  Will need to evaluate.

>> +static int scom_probe(struct device *dev)
>> +{
>> +     struct fsi_device *fsi_dev = to_fsi_dev(dev);
>> +     struct scom_device *scom = &scom_dev;
>> +     int rc;
>> +
>> +     scom->fsi_dev = fsi_dev;
>> +     scom->cdev = cdev_alloc();
>> +     if (!scom->cdev)
>> +             return -ENOMEM;
>> +
>> +     scom->cdev->owner = THIS_MODULE;
>> +
>> +     rc = register_chrdev(0, "scom", &scom_fops);
>> +     if (rc < 0) {
>> +             dev_dbg(dev, "scom major allocation failed:%d\n", rc);
>> +             return rc;
>> +     }
>> +     dev_dbg(dev, "scom major allocated:%d\n", rc);
>
> You need to register the actual device too, right? We'll want scom0,
> scom1, etc.

Don't understand your meaning here.  Also scom0 and scom1 refer to
what exactly?  For a first pass of core code I'd like to be able to deal
with a single engine if possible and expand later.

Thanks,
Chris


>
> Cheers,
>
>
> Jeremy