| Message ID | 20201104205431.3795207-2-saravanak@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v1,1/2] driver core: Fix lockdep warning on wfs_lock | expand |
On Wed, Nov 04, 2020 at 12:54:31PM -0800, Saravana Kannan wrote: > From: Daniel Mentz <danielmentz@google.com> > > This allows the fw_devlink feature to work for spi devices > too. This avoids unnecessary probe deferrals related to spi devices and > improves suspend/resume ordering for spi devices when fw_devlink=on. > 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 - when would we not want to have the fwnode correspond to the of_node, 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?
On Thu, Nov 5, 2020 at 9:12 AM Mark Brown <broonie@kernel.org> wrote: > > On Wed, Nov 04, 2020 at 12:54:31PM -0800, Saravana Kannan wrote: > > From: Daniel Mentz <danielmentz@google.com> > > > > This allows the fw_devlink feature to work for spi devices > > too. This avoids unnecessary probe deferrals related to spi devices and > > improves suspend/resume ordering for spi devices when fw_devlink=on. > > > 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. > - when would we not want to have the > fwnode correspond to the of_node, Never. > 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. -Saravana
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@kernel.org> 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?
On Fri, Nov 6, 2020 at 7:10 AM Mark Brown <broonie@kernel.org> wrote: > > 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@kernel.org> 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. Since you kicked off another thread while we were still discussing it here, I'll just move to that thread. I don't want to discuss the same thing in two different places. > BTW I'm also missing patch 1 and the cover letter for this series, not > sure what's going on there? Sorry, scripting error. There is no series. -Saravana
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index 0cab239d8e7f..d533aa939cca 100644 --- a/drivers/spi/spi.c +++ b/drivers/spi/spi.c @@ -2037,6 +2037,7 @@ of_register_spi_device(struct spi_controller *ctlr, struct device_node *nc) /* Store a pointer to the node in the device structure */ of_node_get(nc); spi->dev.of_node = nc; + spi->dev.fwnode = of_fwnode_handle(nc); /* Register the new device */ rc = spi_add_device(spi);