Re: [PATCH anybus v4 1/7] fieldbus_dev: add Fieldbus Device subsystem.

[Sven Van Asbroeck]

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?