On Thu, Nov 05, 2020 at 11:26:44AM -0800, Saravana Kannan wrote: > On Thu, Nov 5, 2020 at 9:12 AM Mark Brown <broonie@xxxxxxxxxx> wrote: > > > of_node_get(nc); > > > spi->dev.of_node = nc; > > > + spi->dev.fwnode = of_fwnode_handle(nc); > > Why is this a manual step in an individual subsystem rather than > > something done in the driver core > It can't be done in driver core because "fwnode" is the abstraction > driver core uses. It shouldn't care or know if the firmware is DT, > ACPI or something else -- that's the whole point of fwnode. Clearly it *can* be done in the driver core, the question is do we want to. The abstraction thing feels weaker at init than use after init, "init from X" is a common enough pattern. If it's done by the driver core there would be no possibility of anything that creates devices getting things wrong here, and the driver core already has a bunch of integration with both DT and ACPI so it seems like a weird boundary to have. > > and wouldn't that just be a case of > > checking to see if there is a fwnode already set and only initializing > > if not anyway? > Honestly, we should be deleting device.of_node and always use > device.fwnode. But that's a long way away (lots of clean up). The > "common" place to do this is where a struct device is created from a > firmware (device_node, acpi_device, etc). I don't see a "common place" > for when a device is created out of a device_node, so I think this > patch is a reasonable middle ground. That is obviously a much bigger job that's going to require going through subsystems (and their drivers) properly to eliminate references to of_node, I'm not clear how doing this little bit per subsystem rather than in the core helps or hinders going through and doing that. I don't think you'll ever have a single place where a device is constructed, and I'm not sure that that is even desirable, since there are per subsystem things that need doing. I'd be totally happy with eliminating all references to of_node from the subsystem but for this it seems more sensible to do it in the driver core and cover everything rather than running around everything that creates a device from DT individually and having stuff fall through the cracks - it's been a year since the equivalent change was made in I2C for example, we've had new buses merged in that time never mind SPI not being covered. BTW I'm also missing patch 1 and the cover letter for this series, not sure what's going on there?
Attachment:
signature.asc
Description: PGP signature
