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

[Christopher Bostic]

On Fri, Oct 21, 2016 at 12:24 AM, Jeremy Kerr <jk at ozlabs.org> wrote:
> Hi Chris,
>
>>> 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.
>
> OK.
>
>>>> +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?
>
> The actual devices. You're only registering a chardev major number
> there, not an actual character device, and so this doesn't result in any
> /dev/scom node being created.
>
> You'll want to do a device_create there, to get the minor allocated and
> have the device appear. However, that means you'll also need to create a
> class too.
>
> If you want to skip all that, just create a miscdev instead. That will
> take care of the major/minor numbers for you, and will go into the
> 'misc' device class.
>

Hi Jeremy,

Ok, I've added this change for the next set to review.

>> For a first pass of core code I'd like to be able to deal with a
>> single engine if possible and expand later.
>
> There's a right and wrong way to do that though. Currently, it looks
> like you're intending to register a /dev/scom. Then, when you want to
> add multiple interfaces in future, that will have to be removed to
> create /dev/scom0, /dev/scom1, etc. That's a userspace ABI change, which
> could break old code.
>
> So, even if your driver only handles one device for now, name that in a
> way that doesn't conflict with that future interface.
>
> However, handling multiple devices is pretty trivial. Just allocate your
> device in the probe, rather than using one static instance, and name it
> accordingly. Here's what I'd suggest for your probe function:
>
> struct scom_device {
>         struct fsi_device       *fsi_dev;
>         struct miscdevice       mdev;
>         char                    name[32];
> };
>
> static atomic_t scom_idx = ATOMIC_INIT(0);
>
> static int scom_probe(struct device *dev)
> {
>         struct fsi_device *fsi_dev = to_fsi_dev(dev);
>         struct scom_device *scom;
>         int rc;
>
>         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;
>
>         return misc_register(&scom->mdev);
> }
>
>
> Then, in your read/write functions, file->private_data is your mdev, and
> you can just do a container_of to get the struct scom_device *.
>

Added these changes as well to the next patch set.


> From your next email:
>
>> Is there any value in appending an FSI device path to scom?  Like
>> cat'ing the fsi device name on end.  That's what is done in existing
>> product and it makes it easy to tell what device is where in the
>> topology.
>
> No, that's not normal convention for providing that sort of information.
> If we set the device parent (ie, scom->mdev.parent), the device will be
> referenced correctly through sysfs, so we have the correct hardware bus
> layout available there. For example:
>
>   # cat /sys/bus/fsi/devices/00:00:00:00/misc/scom1/dev
>   10:62
>   # ls -l /dev/scom1
>   crw-------    1 root     root       10,  62 Jan  1 00:00 /dev/scom1
>
> Then, if we want symlinks in dev, we add the appropriate udev rules to
> create those according to system policy. The names in dev should be
> fairly dynamic (eg, /dev/sdXN, /dev/ipmiN, etc)
>
> [One advantage of using a chardev with a proper class is that 'misc'
> class directory in the sysfs path could be named something more
> appropriate...]
>

Thanks for the clarification.
Chris.


> Cheers,
>
>
> Jeremy