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

[Greg KH]

On Wed, Nov 21, 2018 at 10:07:03AM -0500, thesven73@xxxxxxxxx wrote:
> +static struct attribute *fieldbus_attrs[] = {
> +	&dev_attr_enabled.attr,
> +	&dev_attr_card_name.attr,
> +	&dev_attr_fieldbus_id.attr,
> +	&dev_attr_read_area_size.attr,
> +	&dev_attr_write_area_size.attr,
> +	&dev_attr_online.attr,
> +	&dev_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 == &dev_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(&fieldbus_ida, 0, MAX_FIELDBUSES, GFP_KERNEL);
> +	if (fb->id < 0)
> +		return fb->id;
> +	devno = MKDEV(MAJOR(fieldbus_devt), fb->id);
> +	init_waitqueue_head(&fb->dc_wq);
> +	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?

> +	fb->dev = device_create(&fieldbus_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