Wow Greg, thanks for the review, this is awesome !!
On Tue, Nov 27, 2018 at 2:54 AM Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
>> + cdev_init(&fb->cdev, &fieldbus_fops);
>> + err = cdev_add(&fb->cdev, devno, 1);
>> + if (err) {
>> + pr_err("fieldbus_dev%d unable to add device %d:%d\n",
>> + fb->id, MAJOR(fieldbus_devt), fb->id);
>> + goto err_cdev;
>> + }
>
> Why do you have a static cdev?
The proposed fieldbus API needs a single /dev/fieldbus_devX node for every
device. I just looked around the drivers/ tree to see how others accomplish
this.
Is there a better way?
>> + fb->online_sd = sysfs_get_dirent(fb->dev->kobj.sd, "online");
>
> Ick, what? Why? Why are you messing around with a raw sysfs attribute?
The proposed fieldbus API has a sysfs attribute that can be poll/select'ed on.
Is this behaviour still allowed / ok?
If so, you're saying that I should not store the raw attribute, but just do:
sysfs_notify(&fb->dev->kobj, NULL, "online") ?
Now that I (hopefully) have a few seconds of your attention...
I suppose the fieldbus API in this patch can't go anywhere, without buy-in from
multiple people who also want to use fieldbus. Right now, there are none.
This might be a chicken-and-egg problem. Perhaps here are no fieldbus
devices because
there's no good general API. There's no good general API because there are no
fieldbus devices yet.
Is there a tried and tested way to break this deadlock?
