Re: [PATCH] PCI: hotplug: Add extension driver for Ampere Altra hotplug LED control

[Bjorn Helgaas <helgaas@kernel.org>]

On Fri, Sep 29, 2023 at 09:06:02PM +0200, Rafael J. Wysocki wrote:
> On Thu, Sep 28, 2023 at 5:47 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > On Wed, Sep 27, 2023 at 01:23:47PM -0700, D Scott Phillips wrote:
> > > On Ampere Altra, PCIe hotplug is handled through ACPI. A side interface is
> > > also present to request system firmware control of attention LEDs. Add an
> > > ACPI PCI Hotplug companion driver to support attention LED control.

> > > +static int __init acpiphp_ampere_altra_init(void)
> > > +{
> > > +     struct fwnode_handle *fwnode;
> > > +     acpi_handle leds_handle = NULL;
> > > +     struct acpi_device *leds;
> > > +     acpi_status status;
> > > +     int ret;
> > > +
> > > +     status = acpi_get_devices("AMPC0008", get_acpi_handle, NULL,
> > > +                               &leds_handle);
> >
> > Rafael, can you comment on whether we should use acpi_get_devices(),
> > acpi_bus_register_driver(), acpi_acpi_scan_add_handler(), or something
> > else here?
> 
> Personally, I would go for a platform driver, because the ACPI core
> should create a platform device for this object.
> 
> acpi_get_devices() carries out a namespace walk that is costly and
> entirely avoidable.
> 
> >  I try to avoid pci_get_device() because it subverts the
> > driver model (no hotplug support, no driver/device binding).
> >
> > I see Documentation/driver-api/acpi/scan_handlers.rst, but I'm not
> > clear on when to use acpi_bus_register_driver() vs
> 
> Never.
> 
> > acpi_acpi_scan_add_handler().
> 
> When you don't want the ACPI core to create a platform device for your
> ACPI device object.  There are cases like that, but they are rare.

Ah, so none of the above (not acpi_get_devices(),
acpi_bus_register_driver(), OR acpi_acpi_scan_add_handler()).

IIUC, what you propose would look something like this:

  static u32 led_service_id[4];

  static int altra_led_probe(struct platform_device *pdev)
  {
    struct fwnode_handle *fwnode = dev_fwnode(&pdev->dev);

    fwnode_property_read_u32_array(fwnode, "uuid", led_service_id, 4);
  }

  static const struct acpi_device_id altra_led_ids[] = {
    {"AMPC0008", 0}, {}
  };
  MODULE_DEVICE_TABLE(acpi, altra_led_ids);

  static struct platform_driver altra_led_driver = {
    .driver = {
      .acpi_match_table = altra_led_ids,
    },
    .probe = altra_led_probe,
  };
  module_platform_driver(altra_led_driver);

Bjorn