Hi Andy, On Fri, Aug 04, 2023 at 09:09:16AM +0300, Andy Shevchenko wrote: > On Thu, Aug 03, 2023 at 11:29:06PM +0530, Sunil V L wrote: > > From: Anup Patel <apatel@xxxxxxxxxxxxxxxx> > > > > swnode framework can be used to create fwnode for interrupt > > controllers. > > Why? What is this for? > Can you elaborate? This commit message is poorly written... > > And why firmware node is not enough for ACPI case? > I assume the fwnode in DT case is already provided by OF. > Thanks a lot for the review!. You are right, OF provides the fwnode for irqchip drivers. However, for ACPI case, it is typically created using irq_domain_alloc_named_fwnode or irq_domain_alloc_fwnode since these are not ACPI devices in the namespace but from MADT. The fwnode created using irq_domain_alloc_fwnode() is a simple one which doesn't support properties similar to the one created by OF framework or software node framework. Hence, lot of data from the MADT structures need to be cached as separate structures in the drivers and also would need several ifdefs to check for ACPI and some amount of code duplication is also required due to the way DT driver gets the information vs ACPI. The beauty of software node framework is, it supports adding properties and also is a supported fwnode type in __irq_domain_create(). So, if we can create the fwnode for these irqchip using software node, we can attach the same properties and the actual irqchip driver which uses the fwnode doesn't need to have any ACPI vs DT checks. Same driver will work seamlessly on both DT and ACPI platforms. But the challenge is, currently swnode expects to be created with sysfs which won't be available during early boot when irqchip drivers need to be probed. So, adding support to create without dependency on sysfs help us to reuse the same framework for irqchip use case also. Apologies for not descriptive in the commit message. Please let us know your feedback on this approach. > > This helps in keeping the drivers same for both > > DT and ACPI. To enable this, enhance the swnode framework so > > that it can be created early during boot without dependency > > on sysfs. > > ... > > > - swnode->kobj.kset = swnode_kset; > > + swnode->kobj.kset = (!early) ? swnode_kset : NULL; > > Too many unneeded characters. Why parentheses? Why negative check? > Sure, will update in next version. > ... > > > + if (early) { > > + ret = 0; > > + kobject_init(&swnode->kobj, &software_node_type_early); > > + swnode->kobj.parent = parent ? &parent->kobj : NULL; > > + if (node->name) > > + ret = kobject_set_name(&swnode->kobj, > > + "%s", node->name); > > + else > > + ret = kobject_set_name(&swnode->kobj, > > + "node%d", swnode->id); > > + if (!ret) { > > + spin_lock(&swnode_early_lock); > > + list_add_tail(&swnode->early, &swnode_early_list); > > + spin_unlock(&swnode_early_lock); > > + } > > + } else { > > + if (node->name) > > + ret = kobject_init_and_add(&swnode->kobj, &software_node_type, > > + parent ? &parent->kobj : NULL, > > This looks like have a duplication. > > > + "%s", node->name); > > + else > > + ret = kobject_init_and_add(&swnode->kobj, &software_node_type, > > + parent ? &parent->kobj : NULL, > > + "node%d", swnode->id); > > + } > > Maybe it's possible to refactor this piece to be more compact? > The issue is, kobject_init_and_add() expects sysfs. Let me try to compact this in next version. Thanks! > ... > > > - return PTR_ERR_OR_ZERO(swnode_register(node, parent, 0)); > > + return PTR_ERR_OR_ZERO(swnode_register(node, parent, 0, 0)); > > In one case you use boolean, here is unsigned int for early flag, why is the > inconsistency added? > Yeah, it should be bool. Let me fix it in next version. > ... > > > -struct fwnode_handle * > > -fwnode_create_software_node(const struct property_entry *properties, > > - const struct fwnode_handle *parent) > > +static struct fwnode_handle * > > +fwnode_create_software_node_common(const struct property_entry *properties, > > + const struct fwnode_handle *parent, > > + bool early) > > Why would you need this API in early stages? > Hope I answered the question above. Thanks! Sunil