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

[Christopher Bostic]

>> +static atomic_t scom_idx = ATOMIC_INIT(0);
>> +static int scom_probe(struct device *);
>
> You don't need this forward declaration anymore
>

Hi Jeremy,

OK will remove.

>> +static int get_scom(struct scom_device *scom_dev, uint64_t *value,
>> +                     uint32_t addr)
>> +{
>> +     uint32_t result, data;
>> +     int rc;
>> +
>> +     udelay(SCOM_FSI2PIB_DELAY);
>
> Why this udelay()? Does put_scom() not need it too then? If it's a
> matter of scom engine state, should we adjust this based on the last
> operation?

This is a carry over from previous code.  I don't know the history behind why
it was added.  Will remove.

>
> If it's necessary, can you add a short comment?
>
>> +static loff_t scom_llseek(struct file *filep, loff_t offset, int whence)
>> +{
>> +     if (whence != 0)  /* SEEK_SET */
>> +             return -EINVAL;
>> +
>> +     filep->f_pos = offset;
>> +     return offset;
>> +}
>
> I think you can drop this and use .llseek = no_seek_end_llseek instead
> (and then you get SEEK_CUR support too).

Will change.

>
>> +static ssize_t scom_read(struct file *filep, char __user *buf, size_t len,
>> +                     loff_t *offset)
>> +{
>> +     int rc = 0;
>> +     struct miscdevice *mdev =
>> +                             (struct miscdevice *)filep->private_data;
>> +     struct scom_device *scom = to_scom_dev(mdev);
>> +     struct device *dev = &scom->fsi_dev->dev;
>> +     uint64_t val;
>> +
>> +     if (len != sizeof(uint64_t))
>> +             return -EINVAL;
>> +
>> +     rc = get_scom(scom, &val, *offset);
>> +     if (rc) {
>> +             dev_dbg(dev, "get_scom fail:%d\n", rc);
>> +             return rc;
>> +     }
>> +
>> +     rc = copy_to_user(buf, &val, len);
>> +     if (rc)
>> +             dev_dbg(dev, "copy to user failed:%d\n", rc);
>> +
>> +     return rc ? rc : len;
>> +}
>
> What are the semantics of offset after reading & writing? Should two
> reads (with no intervening llseek) read the same address, or subsequent
> addresses?

The user is to specify the address for each scom read and write.   No
support has been provided for any other address auto set.

>
>> +static ssize_t scom_write(struct file *filep, const char __user *buf,
>> +                     size_t len, loff_t *offset)
>> +{
>> +     int rc = 0;
>
> No need to initialise this (and in scom_read too).
>

Will remove.

>> +     struct miscdevice *mdev =
>> +                             (struct miscdevice *)filep->private_data;
>> +     struct scom_device *scom = to_scom_dev(mdev);
>> +     struct device *dev = &scom->fsi_dev->dev;
>> +     uint64_t val;
>> +
>> +     if (len != sizeof(uint64_t))
>> +             return -EINVAL;
>> +
>> +     rc = copy_from_user(&val, buf, len);
>> +     if (rc) {
>> +             dev_dbg(dev, "copy from user failed:%d\n", rc);
>> +             return -EINVAL;
>> +     }
>
> You don't want to squash that return value; just return rc there.
>

OK, will change.

>> +
>> +     rc = put_scom(scom, val, *offset);
>> +     if (rc)
>> +             dev_dbg(dev, "put_scom failed with:%d\n", rc);
>> +
>> +
>> +     return rc ? rc : len;
>> +}
>> +
>> +static const struct file_operations scom_fops = {
>> +     .owner  = THIS_MODULE,
>> +     .llseek = scom_llseek,
>> +     .read   = scom_read,
>> +     .write  = scom_write,
>> +};
>> +
>> +static int scom_probe(struct device *dev)
>> +{
>> +     struct fsi_device *fsi_dev = to_fsi_dev(dev);
>> +     struct scom_device *scom = NULL;
>
> No need to initialise this either.
>

Removing

>> +
>> +     scom = devm_kzalloc(dev, sizeof(*scom), GFP_KERNEL);
>> +     if (!scom)
>> +             return -ENOMEM;
>> +
>> +     snprintf(scom->name, sizeof(scom->name),
>> +                     "scom%d", atomic_inc_return(&scom_idx));
>> +     scom->fsi_dev = fsi_dev;
>> +     scom->mdev.minor = MISC_DYNAMIC_MINOR;
>> +     scom->mdev.fops = &scom_fops;
>> +     scom->mdev.name = scom->name;
>> +     scom->mdev.parent = dev;
>> +     list_add(&scom->link, &scom_devices);
>> +
>> +     return misc_register(&scom->mdev);
>> +}
>> +
>> +static struct fsi_device_id scom_ids[] = {
>> +     {
>> +             .engine_type = FSI_ENGID_SCOM,
>> +             .version = FSI_VERSION_ANY,
>> +     },
>> +     { 0 }
>> +};
>> +
>> +static struct fsi_driver scom_drv = {
>> +     .id_table = scom_ids,
>> +     .drv = {
>> +             .name = "scom",
>> +             .bus = &fsi_bus_type,
>> +             .probe = scom_probe,
>> +     }
>> +};
>> +
>> +static int scom_init(void)
>> +{
>> +     INIT_LIST_HEAD(&scom_devices);
>> +     return fsi_driver_register(&scom_drv);
>> +}
>> +
>> +static void scom_exit(void)
>> +{
>> +     struct list_head *pos;
>> +     struct scom_device *scom = NULL;
>
> ... or this.
>

Will remove

>> +
>> +     list_for_each(pos, &scom_devices) {
>> +             scom = list_entry(pos, struct scom_device, link);
>> +             misc_deregister(&scom->mdev);
>> +             devm_kfree(&scom->fsi_dev->dev, scom);
>> +     }
>> +     fsi_driver_unregister(&scom_drv);
>> +}
>> +
>> +module_init(scom_init);
>> +module_exit(scom_exit);
>> +MODULE_LICENSE("GPL");
>

Thanks,
Chris

> Cheers,
>
>
> Jeremy