On Tue, Jul 03, 2018 at 05:04:12PM +1000, Andrew Jeffery wrote:

What is this crazy union for?  Why are you messing around with "raw"
kobject attributes?  This is a device, you should never have to mess
with sysfs calls or kobject calls or structures directly.  If you do,
that's a huge hint something is wrong here.

+	};
+};
+
+static ssize_t bmc_misc_rmw_show(struct kobject *kobj,
+					struct kobj_attribute *attr, char *buf)
+{
+	struct bmc_ctrl *ctrl;
+	unsigned int val;
+	int rc;
+
+	ctrl = container_of(attr, struct bmc_ctrl, value_attr);
+	rc = regmap_read(ctrl->map, ctrl->value, &val);
+	if (rc)
+		return rc;
+
+	val = (val & ctrl->mask) >> ctrl->shift;
+
+	return sprintf(buf, "%u\n", val);
+}
+
+static ssize_t bmc_misc_rmw_store(struct kobject *kobj,
+					 struct kobj_attribute *attr,
+					 const char *buf, size_t count)
+{
+	struct bmc_ctrl *ctrl;
+	long val;
+	int rc;
+
+	rc = kstrtol(buf, 0, &val);
+	if (rc)
+		return rc;
+
+	ctrl = container_of(attr, struct bmc_ctrl, value_attr);
+	val <<= ctrl->shift;
+	if (val & ~ctrl->mask)
+		return -EINVAL;
+	rc = regmap_update_bits(ctrl->map, ctrl->value, ctrl->mask, val);
+
+	return rc < 0 ? rc : count;
+}
+
+static void bmc_misc_add_rmw_attrs(struct bmc_ctrl *ctrl,
+				   struct attribute *attrs[6])
+{
+	sysfs_attr_init(&ctrl->attr.attr);
+	ctrl->value_attr.attr.name = "value";
+	ctrl->value_attr.attr.mode = 0664;
+	ctrl->value_attr.show = bmc_misc_rmw_show;
+	ctrl->value_attr.store = bmc_misc_rmw_store;
+	attrs[2] = &ctrl->value_attr.attr;
+	attrs[3] = NULL;

You aren't "adding" any attributes here, you are only setting them up
(in an odd way, see below...)

+}
+
+static int bmc_misc_init_rmw(struct bmc_ctrl *ctrl, struct device_node *node,
+			     struct attribute *attrs[6])

That's an oddly-hard-coded array size for no good reason :(

+{
+	u32 val;
+	int rc;
+
+	rc = of_property_read_u32(node, "offset", &ctrl->value);
+	if (rc < 0)
+		return rc;
+
+	if (!of_property_read_u32(node, "default-value", &val)) {
+		val <<= ctrl->shift;
+		if (val & ~ctrl->mask)
+			return -EINVAL;
+		val &= ctrl->mask;
+		regmap_update_bits(ctrl->map, ctrl->value, ctrl->mask, val);
+	}
+
+	bmc_misc_add_rmw_attrs(ctrl, attrs);
+
+	return 0;
+}
+
+static ssize_t bmc_misc_sc_read_show(struct kobject *kobj,
+					struct kobj_attribute *attr, char *buf)
+{
+	struct bmc_ctrl *ctrl;
+	unsigned int val;
+	int rc;
+
+	ctrl = container_of(attr, struct bmc_ctrl, read_attr);
+	rc = regmap_read(ctrl->map, ctrl->read, &val);
+	if (rc)
+		return rc;
+
+	val = (val & ctrl->mask) >> ctrl->shift;
+
+	return sprintf(buf, "%u\n", val);
+
+}
+
+static ssize_t bmc_misc_sc_set_store(struct kobject *kobj,
+					    struct kobj_attribute *attr,
+					    const char *buf, size_t count)
+{
+	struct bmc_ctrl *ctrl;
+	long val;
+	int rc;
+
+	rc = kstrtol(buf, 0, &val);
+	if (rc)
+		return rc;
+
+	ctrl = container_of(attr, struct bmc_ctrl, set_attr);
+	val <<= ctrl->shift;
+	if (val & ~ctrl->mask)
+		return -EINVAL;
+	val &= ctrl->mask;
+	rc = regmap_write(ctrl->map, ctrl->set, val);
+
+	return rc < 0 ? rc : count;
+}
+
+static ssize_t bmc_misc_sc_clear_store(struct kobject *kobj,
+					      struct kobj_attribute *attr,
+					      const char *buf, size_t count)
+{
+	struct bmc_ctrl *ctrl;
+	long val;
+	int rc;
+
+	rc = kstrtol(buf, 0, &val);
+	if (rc)
+		return rc;
+
+	ctrl = container_of(attr, struct bmc_ctrl, clear_attr);
+	val <<= ctrl->shift;
+	if (val & ~ctrl->mask)
+		return -EINVAL;
+	val &= ctrl->mask;
+	rc = regmap_write(ctrl->map, ctrl->clear, val);
+
+	return rc < 0 ? rc : count;
+}
+
+static void bmc_misc_add_sc_attrs(struct bmc_ctrl *ctrl,
+				  struct attribute *attrs[6])
+{
+	sysfs_attr_init(&ctrl->read_attr.attr);
+	ctrl->read_attr.attr.name = "value";
+	ctrl->read_attr.attr.mode = 0444;
+	ctrl->read_attr.show = bmc_misc_sc_read_show;
+	attrs[2] = &ctrl->read_attr.attr;
+
+	sysfs_attr_init(&ctrl->set_attr.attr);
+	ctrl->set_attr.attr.name = "set";
+	ctrl->set_attr.attr.mode = 0200;
+	ctrl->set_attr.store = bmc_misc_sc_set_store;
+	attrs[3] = &ctrl->set_attr.attr;
+
+	sysfs_attr_init(&ctrl->clear_attr.attr);
+	ctrl->clear_attr.attr.name = "clear";
+	ctrl->clear_attr.attr.mode = 0200;
+	ctrl->clear_attr.store = bmc_misc_sc_clear_store;
+	attrs[4] = &ctrl->clear_attr.attr;
+
+	attrs[5] = NULL;
+}
+
+static int bmc_misc_init_sc(struct bmc_ctrl *ctrl, struct device_node *node,
+			    struct attribute *attrs[6])
+{
+	int rc;
+
+	rc = of_property_read_u32_array(node, "offset", &ctrl->read, 3);
+	if (rc < 0)
+		return rc;
+
+	if (of_property_read_bool(node, "default-set"))
+		regmap_write(ctrl->map, ctrl->set, ctrl->mask);
+	else if (of_property_read_bool(node, "default-clear"))
+		regmap_write(ctrl->map, ctrl->clear, ctrl->mask);
+
+	bmc_misc_add_sc_attrs(ctrl, attrs);
+
+	return 0;
+}
+
+static ssize_t bmc_misc_label_show(struct kobject *kobj,
+				   struct kobj_attribute *attr, char *buf)
+{
+	struct bmc_ctrl *ctrl = container_of(attr, struct bmc_ctrl, label_attr);
+
+	return sprintf(buf, "%s\n", ctrl->label);
+}
+
+static ssize_t bmc_misc_mask_show(struct kobject *kobj,
+				   struct kobj_attribute *attr, char *buf)
+{
+	struct bmc_ctrl *ctrl = container_of(attr, struct bmc_ctrl, mask_attr);
+
+	return sprintf(buf, "0x%x\n", ctrl->mask >> ctrl->shift);
+}
+
+static int bmc_misc_init_dt(struct bmc_ctrl *ctrl, struct device_node *node,
+			    struct attribute *attrs[6])
+{
+	int rc;
+
+	/* Example children:
+	 *
+	 * field at 80.6 {
+	 *      compatible = "bmc-misc-control";
+	 *      reg = <0x80>;
+	 *      bit-mask = <0x1>;
+	 *      bit-shift = <6>;
+	 *      label = "ilpc2ahb-disable";
+	 * };
+	 *
+	 * field at 70.6 {
+	 *      compatible = "bmc-misc-control";
+	 *      // Write-1-set/Write-1-clear semantics
+	 *      set-clear;
+	 *      default-set;
+	 *      reg = <0x70 0x70 0x7c>
+	 *      bit-mask = <0x1>;
+	 *      bit-shift = <6>;
+	 *      label = "lpc-sio-decode-disable";
+	 * };
+	 *
+	 * field at 50.0 {
+	 *	compatible = "bmc-misc-control";
+	 *	read-only;
+	 *	reg = <0x50>;
+	 *	bit-mask = <0xffffffff>;
+	 *	bit-shift = <0>;
+	 *	label = "vga-scratch-1";
+	 * };
+	 */
+	rc = of_property_read_u32(node, "mask", &ctrl->mask);
+	if (rc < 0)
+		return rc;
+
+	ctrl->shift = __ffs(ctrl->mask);
+	ctrl->ro = of_property_read_bool(node, "read-only");
+
+	rc = of_property_read_string(node, "label", &ctrl->label);
+	if (rc < 0)
+		return rc;
+
+	ctrl->label_attr.attr.name = "label";
+	ctrl->label_attr.attr.mode = 0444;
+	ctrl->label_attr.show = bmc_misc_label_show;
+	attrs[0] = &ctrl->label_attr.attr;

This works?  You normally have to manually initialize a dynamic
attribute.  Why are you doing it this way and not using an attribute
group?

+	ctrl->mask_attr.attr.name = "mask";
+	ctrl->mask_attr.attr.mode = 0444;
+	ctrl->mask_attr.show = bmc_misc_mask_show;
+	attrs[1] = &ctrl->mask_attr.attr;
+
+	if (of_property_read_bool(node, "set-clear"))
+		return bmc_misc_init_sc(ctrl, node, attrs);
+
+	return bmc_misc_init_rmw(ctrl, node, attrs);
+}
+
+static struct class bmc_class = {
+	.name =		"bmc",
+};

Why are you using a custom device class for a single device?

you need to document the heck out of this in the changelog to help
explain all of these odd design decisions.

thanks,

greg k-h