Messages in this thread | | | From | Sven Van Asbroeck <> | Date | Wed, 28 Nov 2018 10:39:41 -0500 | Subject | Re: [PATCH anybus v4 1/7] fieldbus_dev: add Fieldbus Device subsystem. |
| |
Wow Greg, thanks for the review, this is awesome !!
On Tue, Nov 27, 2018 at 2:54 AM Greg KH <gregkh@linuxfoundation.org> 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?
| |