Re: [PATCH anybus v4 1/7] fieldbus_dev: add Fieldbus Device subsystem.
On Wed, Nov 28, 2018 at 1:36 PM Greg KH wrote: > So you still need a char device, and you will have have a load of them, > just use the misc device api. It's simple, clean, and is hard to get > wrong. The cdev api is hard, complex, and trivial to get wrong in any > number of different ways that it can be used :) Awesome little nugget! These things you just can't tell by reading the kernel tree. In many cases, it's not immediately obvious to me which of the various methods are 'recommended', and which ones are historic leftovers. > And your /sys/class/fieldbus_dev/ is "interesting", why name it that? Not sure why it's interesting? I figured it would allow userspace to enumerate all fieldbus devices simply by going to /sys/class/fieldbus_dev/ and process every subdirectory. Example with 3 fieldbus cards: /sys/class/fieldbus_dev/fieldbus_dev0/name etc /sys/class/fieldbus_dev/fieldbus_dev1/name etc /sys/class/fieldbus_dev/fieldbus_dev2/name etc Is there a better way? > So keep submitting, I'll try to review it the next time around, and all > should be fine. But keep your user api as simple as possible for now, > only doing what you need it to do, worry about future stuff then. Thanks, that's awesome !! I figured that I should take baby steps. I simplified the userspace API to the absolute bare minimum. To the point that the profinet card isn't useable in our company's specific use case, because there's no configuration API yet. > Why not just /sys/class/fieldbus/ ? No need for "dev", right? Good question... Two very different classes of devices sit on a fieldbus: Fieldbus device is like a slave Fieldbus controller is like a master This subsystem is specific for fieldbus *devices*. I figured I'd encode this in the API name, in case someone wants to support fieldbus controllers later.
Re: [PATCH anybus v4 1/7] fieldbus_dev: add Fieldbus Device subsystem.
On Wed, Nov 28, 2018 at 1:36 PM Greg KH wrote: > So you still need a char device, and you will have have a load of them, > just use the misc device api. It's simple, clean, and is hard to get > wrong. The cdev api is hard, complex, and trivial to get wrong in any > number of different ways that it can be used :) Awesome little nugget! These things you just can't tell by reading the kernel tree. In many cases, it's not immediately obvious to me which of the various methods are 'recommended', and which ones are historic leftovers. > And your /sys/class/fieldbus_dev/ is "interesting", why name it that? Not sure why it's interesting? I figured it would allow userspace to enumerate all fieldbus devices simply by going to /sys/class/fieldbus_dev/ and process every subdirectory. Example with 3 fieldbus cards: /sys/class/fieldbus_dev/fieldbus_dev0/name etc /sys/class/fieldbus_dev/fieldbus_dev1/name etc /sys/class/fieldbus_dev/fieldbus_dev2/name etc Is there a better way? > So keep submitting, I'll try to review it the next time around, and all > should be fine. But keep your user api as simple as possible for now, > only doing what you need it to do, worry about future stuff then. Thanks, that's awesome !! I figured that I should take baby steps. I simplified the userspace API to the absolute bare minimum. To the point that the profinet card isn't useable in our company's specific use case, because there's no configuration API yet. > Why not just /sys/class/fieldbus/ ? No need for "dev", right? Good question... Two very different classes of devices sit on a fieldbus: Fieldbus device is like a slave Fieldbus controller is like a master This subsystem is specific for fieldbus *devices*. I figured I'd encode this in the API name, in case someone wants to support fieldbus controllers later.
Re: [PATCH anybus v4 1/7] fieldbus_dev: add Fieldbus Device subsystem.
On Wed, Nov 28, 2018 at 01:19:05PM -0500, Sven Van Asbroeck wrote:
> On Wed, Nov 28, 2018 at 12:42 PM Greg KH wrote:
>
> > It depends on what you want to do with this device node. You can use a
> > static one in your structure if you use it to "cast back" to your real
> > structure in the open() call, do you do that?
>
> I do...
>
> static int fieldbus_open(struct inode *inode, struct file *filp)
> {
> struct fieldbus_dev *fbdev = container_of(inode->i_cdev,
> struct fieldbus_dev,
> cdev);
Ok, good, that's a nice way to use this, nevermind my objection.
> > Or use a misc device? How many of these do you need?
>
> Just one per fieldbus device.
> But my main concern is naming in sysfs. A misc device will always show up as
> /sys/class/misc/..., right? Given that this is a userspace fieldbus API,
> we'd prefer /sys/class/fieldbus_dev/..., and cdev is the only way?
Ah, the common misunderstanding.
A cdev has NOTHING to do with the /sys/class/ entries. It is only there
to register a char device with the kernel core and to handle the file
operations that are caused by it. It does not do much, and not even the
/dev/NODE stuff either. That is up to your class device code.
So you still need a char device, and you will have have a load of them,
just use the misc device api. It's simple, clean, and is hard to get
wrong. The cdev api is hard, complex, and trivial to get wrong in any
number of different ways that it can be used :)
That being said, it looks like you used it correctly, so all should be
fine here.
And your /sys/class/fieldbus_dev/ is "interesting", why name it that?
> > For an online/offline attribute, no need to poll on it, just do a
> > 'kobject change' event for online/offline and all should be fine. This
> > is not a high-frequency event, right?
>
> Grepping... you mean kobject_uevent(KOBJ_CHANGE) ?
> THAT mechanism seems to fit much better, thanks !!
Yes, use that please.
> > Hey, if no one wants to use it, either no need to write any code at all,
> > or you get to decide everything. Either way, you're in charge! :)
>
> I did get the impression that people are reluctant to take my patch partly
> because of an unproven userspace API. Maybe they are (rightly) worried that,
> once in, we will have to support this userspace API for evermore.
>
> I'd love to get others involved in Fieldbus on Linux, but I'm not sure
> how.
Adding a new driver subsystem is messy, it takes a lot of work and
finding the right reviewers is hard. It's not just you, it happens all
the time (look at how long the i3c code took to get merged as one
example...)
So keep submitting, I'll try to review it the next time around, and all
should be fine. But keep your user api as simple as possible for now,
only doing what you need it to do, worry about future stuff then.
> > But you do need to document the thing, in Documentation/ABI/ to get it
> > correct and able to be reviewed.
>
> The documentation is already included in this patch? Should I indicate this
> in some standardized fashion?
>
> create mode 100644 Documentation/ABI/testing/fieldbus-dev-cdev
> create mode 100644 Documentation/ABI/testing/sysfs-class-fieldbus-dev
Ah, sorry, I didn't read it, my fault :(
Why not just /sys/class/fieldbus/ ? No need for "dev", right?
greg k-h
Re: [PATCH anybus v4 1/7] fieldbus_dev: add Fieldbus Device subsystem.
On Wed, Nov 28, 2018 at 01:19:05PM -0500, Sven Van Asbroeck wrote:
> On Wed, Nov 28, 2018 at 12:42 PM Greg KH wrote:
>
> > It depends on what you want to do with this device node. You can use a
> > static one in your structure if you use it to "cast back" to your real
> > structure in the open() call, do you do that?
>
> I do...
>
> static int fieldbus_open(struct inode *inode, struct file *filp)
> {
> struct fieldbus_dev *fbdev = container_of(inode->i_cdev,
> struct fieldbus_dev,
> cdev);
Ok, good, that's a nice way to use this, nevermind my objection.
> > Or use a misc device? How many of these do you need?
>
> Just one per fieldbus device.
> But my main concern is naming in sysfs. A misc device will always show up as
> /sys/class/misc/..., right? Given that this is a userspace fieldbus API,
> we'd prefer /sys/class/fieldbus_dev/..., and cdev is the only way?
Ah, the common misunderstanding.
A cdev has NOTHING to do with the /sys/class/ entries. It is only there
to register a char device with the kernel core and to handle the file
operations that are caused by it. It does not do much, and not even the
/dev/NODE stuff either. That is up to your class device code.
So you still need a char device, and you will have have a load of them,
just use the misc device api. It's simple, clean, and is hard to get
wrong. The cdev api is hard, complex, and trivial to get wrong in any
number of different ways that it can be used :)
That being said, it looks like you used it correctly, so all should be
fine here.
And your /sys/class/fieldbus_dev/ is "interesting", why name it that?
> > For an online/offline attribute, no need to poll on it, just do a
> > 'kobject change' event for online/offline and all should be fine. This
> > is not a high-frequency event, right?
>
> Grepping... you mean kobject_uevent(KOBJ_CHANGE) ?
> THAT mechanism seems to fit much better, thanks !!
Yes, use that please.
> > Hey, if no one wants to use it, either no need to write any code at all,
> > or you get to decide everything. Either way, you're in charge! :)
>
> I did get the impression that people are reluctant to take my patch partly
> because of an unproven userspace API. Maybe they are (rightly) worried that,
> once in, we will have to support this userspace API for evermore.
>
> I'd love to get others involved in Fieldbus on Linux, but I'm not sure
> how.
Adding a new driver subsystem is messy, it takes a lot of work and
finding the right reviewers is hard. It's not just you, it happens all
the time (look at how long the i3c code took to get merged as one
example...)
So keep submitting, I'll try to review it the next time around, and all
should be fine. But keep your user api as simple as possible for now,
only doing what you need it to do, worry about future stuff then.
> > But you do need to document the thing, in Documentation/ABI/ to get it
> > correct and able to be reviewed.
>
> The documentation is already included in this patch? Should I indicate this
> in some standardized fashion?
>
> create mode 100644 Documentation/ABI/testing/fieldbus-dev-cdev
> create mode 100644 Documentation/ABI/testing/sysfs-class-fieldbus-dev
Ah, sorry, I didn't read it, my fault :(
Why not just /sys/class/fieldbus/ ? No need for "dev", right?
greg k-h
Re: [PATCH anybus v4 1/7] fieldbus_dev: add Fieldbus Device subsystem.
On Wed, Nov 28, 2018 at 12:42 PM Greg KH wrote:
> It depends on what you want to do with this device node. You can use a
> static one in your structure if you use it to "cast back" to your real
> structure in the open() call, do you do that?
I do...
static int fieldbus_open(struct inode *inode, struct file *filp)
{
struct fieldbus_dev *fbdev = container_of(inode->i_cdev,
struct fieldbus_dev,
cdev);
> Or use a misc device? How many of these do you need?
Just one per fieldbus device.
But my main concern is naming in sysfs. A misc device will always show up as
/sys/class/misc/..., right? Given that this is a userspace fieldbus API,
we'd prefer /sys/class/fieldbus_dev/..., and cdev is the only way?
> For an online/offline attribute, no need to poll on it, just do a
> 'kobject change' event for online/offline and all should be fine. This
> is not a high-frequency event, right?
Grepping... you mean kobject_uevent(KOBJ_CHANGE) ?
THAT mechanism seems to fit much better, thanks !!
> Hey, if no one wants to use it, either no need to write any code at all,
> or you get to decide everything. Either way, you're in charge! :)
I did get the impression that people are reluctant to take my patch partly
because of an unproven userspace API. Maybe they are (rightly) worried that,
once in, we will have to support this userspace API for evermore.
I'd love to get others involved in Fieldbus on Linux, but I'm not sure
how.
> But you do need to document the thing, in Documentation/ABI/ to get it
> correct and able to be reviewed.
The documentation is already included in this patch? Should I indicate this
in some standardized fashion?
create mode 100644 Documentation/ABI/testing/fieldbus-dev-cdev
create mode 100644 Documentation/ABI/testing/sysfs-class-fieldbus-dev
Thanks :)
Sven
Re: [PATCH anybus v4 1/7] fieldbus_dev: add Fieldbus Device subsystem.
On Wed, Nov 28, 2018 at 12:42 PM Greg KH wrote:
> It depends on what you want to do with this device node. You can use a
> static one in your structure if you use it to "cast back" to your real
> structure in the open() call, do you do that?
I do...
static int fieldbus_open(struct inode *inode, struct file *filp)
{
struct fieldbus_dev *fbdev = container_of(inode->i_cdev,
struct fieldbus_dev,
cdev);
> Or use a misc device? How many of these do you need?
Just one per fieldbus device.
But my main concern is naming in sysfs. A misc device will always show up as
/sys/class/misc/..., right? Given that this is a userspace fieldbus API,
we'd prefer /sys/class/fieldbus_dev/..., and cdev is the only way?
> For an online/offline attribute, no need to poll on it, just do a
> 'kobject change' event for online/offline and all should be fine. This
> is not a high-frequency event, right?
Grepping... you mean kobject_uevent(KOBJ_CHANGE) ?
THAT mechanism seems to fit much better, thanks !!
> Hey, if no one wants to use it, either no need to write any code at all,
> or you get to decide everything. Either way, you're in charge! :)
I did get the impression that people are reluctant to take my patch partly
because of an unproven userspace API. Maybe they are (rightly) worried that,
once in, we will have to support this userspace API for evermore.
I'd love to get others involved in Fieldbus on Linux, but I'm not sure
how.
> But you do need to document the thing, in Documentation/ABI/ to get it
> correct and able to be reviewed.
The documentation is already included in this patch? Should I indicate this
in some standardized fashion?
create mode 100644 Documentation/ABI/testing/fieldbus-dev-cdev
create mode 100644 Documentation/ABI/testing/sysfs-class-fieldbus-dev
Thanks :)
Sven
Re: [PATCH anybus v4 1/7] fieldbus_dev: add Fieldbus Device subsystem.
On Wed, Nov 28, 2018 at 10:39:41AM -0500, Sven Van Asbroeck wrote:
> Wow Greg, thanks for the review, this is awesome !!
>
> On Tue, Nov 27, 2018 at 2:54 AM Greg KH wrote:
>
> >> + cdev_init(>cdev, _fops);
> >> + err = cdev_add(>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?
It depends on what you want to do with this device node. You can use a
static one in your structure if you use it to "cast back" to your real
structure in the open() call, do you do that? If not, just create one
dynamically.
Or use a misc device? How many of these do you need?
> >> + 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?
For an online/offline attribute, no need to poll on it, just do a
'kobject change' event for online/offline and all should be fine. This
is not a high-frequency event, right?
> 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.
Hey, if no one wants to use it, either no need to write any code at all,
or you get to decide everything. Either way, you're in charge! :)
But you do need to document the thing, in Documentation/ABI/ to get it
correct and able to be reviewed.
thanks,
greg k-h
Re: [PATCH anybus v4 1/7] fieldbus_dev: add Fieldbus Device subsystem.
On Wed, Nov 28, 2018 at 10:39:41AM -0500, Sven Van Asbroeck wrote:
> Wow Greg, thanks for the review, this is awesome !!
>
> On Tue, Nov 27, 2018 at 2:54 AM Greg KH wrote:
>
> >> + cdev_init(>cdev, _fops);
> >> + err = cdev_add(>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?
It depends on what you want to do with this device node. You can use a
static one in your structure if you use it to "cast back" to your real
structure in the open() call, do you do that? If not, just create one
dynamically.
Or use a misc device? How many of these do you need?
> >> + 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?
For an online/offline attribute, no need to poll on it, just do a
'kobject change' event for online/offline and all should be fine. This
is not a high-frequency event, right?
> 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.
Hey, if no one wants to use it, either no need to write any code at all,
or you get to decide everything. Either way, you're in charge! :)
But you do need to document the thing, in Documentation/ABI/ to get it
correct and able to be reviewed.
thanks,
greg k-h
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 wrote:
>> + cdev_init(>cdev, _fops);
>> + err = cdev_add(>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(>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?
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 wrote:
>> + cdev_init(>cdev, _fops);
>> + err = cdev_add(>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(>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?
Re: [PATCH anybus v4 1/7] fieldbus_dev: add Fieldbus Device subsystem.
On Wed, Nov 21, 2018 at 10:07:03AM -0500, thesve...@gmail.com wrote:
> +static struct attribute *fieldbus_attrs[] = {
> + _attr_enabled.attr,
> + _attr_card_name.attr,
> + _attr_fieldbus_id.attr,
> + _attr_read_area_size.attr,
> + _attr_write_area_size.attr,
> + _attr_online.attr,
> + _attr_fieldbus_type.attr,
> + NULL,
> +};
> +
> +static umode_t fieldbus_is_visible(struct kobject *kobj, struct attribute
> *attr,
> + int n)
> +{
> + struct device *dev = container_of(kobj, struct device, kobj);
> + struct fieldbus_dev *fb = dev_get_drvdata(dev);
> + umode_t mode = attr->mode;
> +
> + if (attr == _attr_enabled.attr) {
> + mode = 0;
> + if (fb->enable_get)
> + mode |= 0444;
> + if (fb->simple_enable_set)
> + mode |= 0200;
> + }
> +
> + return mode;
> +}
> +
> +static const struct attribute_group fieldbus_group = {
> + .attrs = fieldbus_attrs,
> + .is_visible = fieldbus_is_visible,
> +};
> +__ATTRIBUTE_GROUPS(fieldbus);
Why not just use ATTRIBUTE_GROUPS()?
> +static int __fieldbus_dev_register(struct fieldbus_dev *fb)
> +{
> + dev_t devno;
> + int err;
> +
> + if (!fb)
> + return -EINVAL;
> + if (!fb->read_area || !fb->write_area || !fb->fieldbus_id_get)
> + return -EINVAL;
> + fb->id = ida_simple_get(_ida, 0, MAX_FIELDBUSES, GFP_KERNEL);
> + if (fb->id < 0)
> + return fb->id;
> + devno = MKDEV(MAJOR(fieldbus_devt), fb->id);
> + init_waitqueue_head(>dc_wq);
> + cdev_init(>cdev, _fops);
> + err = cdev_add(>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?
> + fb->dev = device_create(_class, fb->parent, devno, fb,
> + "fieldbus_dev%d", fb->id);
> + if (IS_ERR(fb->dev)) {
> + err = PTR_ERR(fb->dev);
> + goto err_dev_create;
> + }
> + fb->online_sd = sysfs_get_dirent(fb->dev->kobj.sd, "online");
Ick, what? Why? Why are you messing around with a raw sysfs attribute?
Also, you are creating sysfs files and you are not documenting any of
them in Documentation/ABI/ which is not allowed. Please add that to
this patch for the next round.
thanks,
greg k-h
Re: [PATCH anybus v4 1/7] fieldbus_dev: add Fieldbus Device subsystem.
On Wed, Nov 21, 2018 at 10:07:03AM -0500, thesve...@gmail.com wrote:
> +static struct attribute *fieldbus_attrs[] = {
> + _attr_enabled.attr,
> + _attr_card_name.attr,
> + _attr_fieldbus_id.attr,
> + _attr_read_area_size.attr,
> + _attr_write_area_size.attr,
> + _attr_online.attr,
> + _attr_fieldbus_type.attr,
> + NULL,
> +};
> +
> +static umode_t fieldbus_is_visible(struct kobject *kobj, struct attribute
> *attr,
> + int n)
> +{
> + struct device *dev = container_of(kobj, struct device, kobj);
> + struct fieldbus_dev *fb = dev_get_drvdata(dev);
> + umode_t mode = attr->mode;
> +
> + if (attr == _attr_enabled.attr) {
> + mode = 0;
> + if (fb->enable_get)
> + mode |= 0444;
> + if (fb->simple_enable_set)
> + mode |= 0200;
> + }
> +
> + return mode;
> +}
> +
> +static const struct attribute_group fieldbus_group = {
> + .attrs = fieldbus_attrs,
> + .is_visible = fieldbus_is_visible,
> +};
> +__ATTRIBUTE_GROUPS(fieldbus);
Why not just use ATTRIBUTE_GROUPS()?
> +static int __fieldbus_dev_register(struct fieldbus_dev *fb)
> +{
> + dev_t devno;
> + int err;
> +
> + if (!fb)
> + return -EINVAL;
> + if (!fb->read_area || !fb->write_area || !fb->fieldbus_id_get)
> + return -EINVAL;
> + fb->id = ida_simple_get(_ida, 0, MAX_FIELDBUSES, GFP_KERNEL);
> + if (fb->id < 0)
> + return fb->id;
> + devno = MKDEV(MAJOR(fieldbus_devt), fb->id);
> + init_waitqueue_head(>dc_wq);
> + cdev_init(>cdev, _fops);
> + err = cdev_add(>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?
> + fb->dev = device_create(_class, fb->parent, devno, fb,
> + "fieldbus_dev%d", fb->id);
> + if (IS_ERR(fb->dev)) {
> + err = PTR_ERR(fb->dev);
> + goto err_dev_create;
> + }
> + fb->online_sd = sysfs_get_dirent(fb->dev->kobj.sd, "online");
Ick, what? Why? Why are you messing around with a raw sysfs attribute?
Also, you are creating sysfs files and you are not documenting any of
them in Documentation/ABI/ which is not allowed. Please add that to
this patch for the next round.
thanks,
greg k-h
Re: [PATCH anybus v4 1/7] fieldbus_dev: add Fieldbus Device subsystem.
On Wed, Nov 21, 2018 at 10:07:03AM -0500, thesve...@gmail.com wrote: > Fieldbus device (client) adapters allow data exchange with a PLC aka. > "Fieldbus Controller" over a fieldbus (Profinet, FLNet, etc.) > > They are typically used when a Linux device wants to expose itself > as an actuator, motor, console light, switch, etc. over the fieldbus. > > This framework is designed to provide a generic interface to Fieldbus > Devices from both the Linux Kernel and the userspace. > > Signed-off-by: Sven Van Asbroeck Your "From:" line does not match this address or name at all, which means no one can apply this :( Please fix for your next round of patches. thanks, greg k-h
Re: [PATCH anybus v4 1/7] fieldbus_dev: add Fieldbus Device subsystem.
On Wed, Nov 21, 2018 at 10:07:03AM -0500, thesve...@gmail.com wrote: > Fieldbus device (client) adapters allow data exchange with a PLC aka. > "Fieldbus Controller" over a fieldbus (Profinet, FLNet, etc.) > > They are typically used when a Linux device wants to expose itself > as an actuator, motor, console light, switch, etc. over the fieldbus. > > This framework is designed to provide a generic interface to Fieldbus > Devices from both the Linux Kernel and the userspace. > > Signed-off-by: Sven Van Asbroeck Your "From:" line does not match this address or name at all, which means no one can apply this :( Please fix for your next round of patches. thanks, greg k-h
Re: [PATCH anybus v4 1/7] fieldbus_dev: add Fieldbus Device subsystem.
On Wed, Nov 21, 2018 at 10:07:03AM -0500, thesve...@gmail.com wrote:
> Fieldbus device (client) adapters allow data exchange with a PLC aka.
> "Fieldbus Controller" over a fieldbus (Profinet, FLNet, etc.)
>
> They are typically used when a Linux device wants to expose itself
> as an actuator, motor, console light, switch, etc. over the fieldbus.
>
> This framework is designed to provide a generic interface to Fieldbus
> Devices from both the Linux Kernel and the userspace.
>
> Signed-off-by: Sven Van Asbroeck
License nit:
> --- /dev/null
> +++ b/drivers/fieldbus/dev_core.c
> @@ -0,0 +1,355 @@
> +// SPDX-License-Identifier: GPL-2.0
That's great, but then you write:
> +MODULE_LICENSE("GPL");
Which means "GPLv2+". So this MODULE_LICENSE() should be "GPL v2",
right?
thanks,
greg k-h
Re: [PATCH anybus v4 1/7] fieldbus_dev: add Fieldbus Device subsystem.
On Wed, Nov 21, 2018 at 10:07:03AM -0500, thesve...@gmail.com wrote:
> Fieldbus device (client) adapters allow data exchange with a PLC aka.
> "Fieldbus Controller" over a fieldbus (Profinet, FLNet, etc.)
>
> They are typically used when a Linux device wants to expose itself
> as an actuator, motor, console light, switch, etc. over the fieldbus.
>
> This framework is designed to provide a generic interface to Fieldbus
> Devices from both the Linux Kernel and the userspace.
>
> Signed-off-by: Sven Van Asbroeck
License nit:
> --- /dev/null
> +++ b/drivers/fieldbus/dev_core.c
> @@ -0,0 +1,355 @@
> +// SPDX-License-Identifier: GPL-2.0
That's great, but then you write:
> +MODULE_LICENSE("GPL");
Which means "GPLv2+". So this MODULE_LICENSE() should be "GPL v2",
right?
thanks,
greg k-h
