| Message ID | 20240621122503.10034-5-hdegoede@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | i2c-i801 / dell-smo8800: Move instantiation of lis3lv02d i2c_client from i2c-i801 to dell-smo8800 | expand |
On Fri, Jun 21, 2024 at 2:26 PM Hans de Goede <hdegoede@redhat.com> wrote: > > The Dell XPS 15 9550 can have a 60Wh battery, leaving space for a 2.5" > sata disk; or a 90Wh battery in which case the battery occupies the space > for the optional 2.5" sata disk. > > On models with the 90Wh battery and thus without a 2.5" sata disk, the BIOS > does not add an IRQ resource to the SMO8810 ACPI device. > > Make the misc-device registration and the requesting of the IRQ optional > and instantiate a lis3lv02d i2c_client independent of the IRQ being there, > so that the non freefall lis3lv02d functionality can still be used. non-freefall > Note that IRQ 0 is not a valid IRQ number for platform IRQs > and this patch relies on that. ... > + err = platform_get_irq_optional(device, 0); > + if (err > 0) > + smo8800->irq = err; > + > + if (smo8800->irq) { You can still do it in one branch less. But I think I understand the motivation of the split. > + err = misc_register(&smo8800->miscdev); > + if (err) { > + dev_err(&device->dev, "failed to register misc dev: %d\n", err); > + return err; > + } > + > + err = request_threaded_irq(smo8800->irq, smo8800_interrupt_quick, > + smo8800_interrupt_thread, > + IRQF_TRIGGER_RISING | IRQF_ONESHOT, > + DRIVER_NAME, smo8800); > + if (err) { > + dev_err(&device->dev, > + "failed to request thread for IRQ %d: %d\n", > + smo8800->irq, err); > + goto error; > + } > + > + dev_dbg(&device->dev, "device /dev/freefall registered with IRQ %d\n", > + smo8800->irq); > } ... > error_free_irq: > - free_irq(smo8800->irq, smo8800); > + if (smo8800->irq) { > + free_irq(smo8800->irq, smo8800); > error: > - misc_deregister(&smo8800->miscdev); > + misc_deregister(&smo8800->miscdev); > + } This is quite unusual. I would rather expect label_foo: if (...) foo() label_bar: if (...) bar() -- With Best Regards, Andy Shevchenko
On Friday 21 June 2024 14:24:59 Hans de Goede wrote: > The Dell XPS 15 9550 can have a 60Wh battery, leaving space for a 2.5" > sata disk; or a 90Wh battery in which case the battery occupies the space > for the optional 2.5" sata disk. > > On models with the 90Wh battery and thus without a 2.5" sata disk, the BIOS > does not add an IRQ resource to the SMO8810 ACPI device. That is a pity, but OK, manufacturer decided that freefall sensor is enabled by BIOS firmware only if the SATA is present. > Make the misc-device registration and the requesting of the IRQ optional > and instantiate a lis3lv02d i2c_client independent of the IRQ being there, > so that the non freefall lis3lv02d functionality can still be used. > > Note that IRQ 0 is not a valid IRQ number for platform IRQs > and this patch relies on that. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > drivers/platform/x86/dell/dell-smo8800.c | 67 +++++++++++++----------- > 1 file changed, 37 insertions(+), 30 deletions(-) > > diff --git a/drivers/platform/x86/dell/dell-smo8800.c b/drivers/platform/x86/dell/dell-smo8800.c > index cd2e48405859..2e49bbb569c6 100644 > --- a/drivers/platform/x86/dell/dell-smo8800.c > +++ b/drivers/platform/x86/dell/dell-smo8800.c > @@ -310,33 +310,32 @@ static int smo8800_probe(struct platform_device *device) > init_waitqueue_head(&smo8800->misc_wait); > INIT_WORK(&smo8800->i2c_work, smo8800_instantiate_i2c_client); > > - err = misc_register(&smo8800->miscdev); > - if (err) { > - dev_err(&device->dev, "failed to register misc dev: %d\n", err); > - return err; > + err = platform_get_irq_optional(device, 0); > + if (err > 0) > + smo8800->irq = err; This code should be rather change to fail immediately. If the IRQ number is not provided by the BIOS then the ACPI SMO88xx is not usable for us at all. So return error from the smo8800_probe function. > + > + if (smo8800->irq) { > + err = misc_register(&smo8800->miscdev); > + if (err) { > + dev_err(&device->dev, "failed to register misc dev: %d\n", err); > + return err; > + } > + > + err = request_threaded_irq(smo8800->irq, smo8800_interrupt_quick, > + smo8800_interrupt_thread, > + IRQF_TRIGGER_RISING | IRQF_ONESHOT, > + DRIVER_NAME, smo8800); > + if (err) { > + dev_err(&device->dev, > + "failed to request thread for IRQ %d: %d\n", > + smo8800->irq, err); > + goto error; > + } > + > + dev_dbg(&device->dev, "device /dev/freefall registered with IRQ %d\n", > + smo8800->irq); > } > > - platform_set_drvdata(device, smo8800); > - > - err = platform_get_irq(device, 0); > - if (err < 0) > - goto error; > - smo8800->irq = err; > - > - err = request_threaded_irq(smo8800->irq, smo8800_interrupt_quick, > - smo8800_interrupt_thread, > - IRQF_TRIGGER_RISING | IRQF_ONESHOT, > - DRIVER_NAME, smo8800); > - if (err) { > - dev_err(&device->dev, > - "failed to request thread for IRQ %d: %d\n", > - smo8800->irq, err); > - goto error; > - } > - > - dev_dbg(&device->dev, "device /dev/freefall registered with IRQ %d\n", > - smo8800->irq); > - > if (dmi_check_system(smo8800_lis3lv02d_devices)) { > /* > * Register i2c-bus notifier + queue initial scan for lis3lv02d > @@ -350,14 +349,20 @@ static int smo8800_probe(struct platform_device *device) > } else { > dev_warn(&device->dev, > "lis3lv02d accelerometer is present on SMBus but its address is unknown, skipping registration\n"); > + if (!smo8800->irq) > + return -ENODEV; > } > > + platform_set_drvdata(device, smo8800); > return 0; > > error_free_irq: > - free_irq(smo8800->irq, smo8800); > + if (smo8800->irq) { > + free_irq(smo8800->irq, smo8800); > error: > - misc_deregister(&smo8800->miscdev); > + misc_deregister(&smo8800->miscdev); > + } > + > return err; > } > > @@ -371,9 +376,11 @@ static void smo8800_remove(struct platform_device *device) > i2c_unregister_device(smo8800->i2c_dev); > } > > - free_irq(smo8800->irq, smo8800); > - misc_deregister(&smo8800->miscdev); > - dev_dbg(&device->dev, "device /dev/freefall unregistered\n"); > + if (smo8800->irq) { > + free_irq(smo8800->irq, smo8800); > + misc_deregister(&smo8800->miscdev); > + dev_dbg(&device->dev, "device /dev/freefall unregistered\n"); > + } > } > > static const struct acpi_device_id smo8800_ids[] = { > -- > 2.45.1 >
Hi Pali, On 6/22/24 3:20 PM, Pali Rohár wrote: > On Friday 21 June 2024 14:24:59 Hans de Goede wrote: >> The Dell XPS 15 9550 can have a 60Wh battery, leaving space for a 2.5" >> sata disk; or a 90Wh battery in which case the battery occupies the space >> for the optional 2.5" sata disk. >> >> On models with the 90Wh battery and thus without a 2.5" sata disk, the BIOS >> does not add an IRQ resource to the SMO8810 ACPI device. > > That is a pity, but OK, manufacturer decided that freefall sensor is > enabled by BIOS firmware only if the SATA is present. > >> Make the misc-device registration and the requesting of the IRQ optional >> and instantiate a lis3lv02d i2c_client independent of the IRQ being there, >> so that the non freefall lis3lv02d functionality can still be used. >> >> Note that IRQ 0 is not a valid IRQ number for platform IRQs >> and this patch relies on that. >> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- >> drivers/platform/x86/dell/dell-smo8800.c | 67 +++++++++++++----------- >> 1 file changed, 37 insertions(+), 30 deletions(-) >> >> diff --git a/drivers/platform/x86/dell/dell-smo8800.c b/drivers/platform/x86/dell/dell-smo8800.c >> index cd2e48405859..2e49bbb569c6 100644 >> --- a/drivers/platform/x86/dell/dell-smo8800.c >> +++ b/drivers/platform/x86/dell/dell-smo8800.c >> @@ -310,33 +310,32 @@ static int smo8800_probe(struct platform_device *device) >> init_waitqueue_head(&smo8800->misc_wait); >> INIT_WORK(&smo8800->i2c_work, smo8800_instantiate_i2c_client); >> >> - err = misc_register(&smo8800->miscdev); >> - if (err) { >> - dev_err(&device->dev, "failed to register misc dev: %d\n", err); >> - return err; >> + err = platform_get_irq_optional(device, 0); >> + if (err > 0) >> + smo8800->irq = err; > > This code should be rather change to fail immediately. If the IRQ number > is not provided by the BIOS then the ACPI SMO88xx is not usable for us > at all. So return error from the smo8800_probe function. The goal of this patch is to still register the bus-notifier for i2c-client instantiation for the lis3lv02d driver. Existing immediately here (as was done before) means we will still not register the bus-notifier. Regards, Hans >> + >> + if (smo8800->irq) { >> + err = misc_register(&smo8800->miscdev); >> + if (err) { >> + dev_err(&device->dev, "failed to register misc dev: %d\n", err); >> + return err; >> + } >> + >> + err = request_threaded_irq(smo8800->irq, smo8800_interrupt_quick, >> + smo8800_interrupt_thread, >> + IRQF_TRIGGER_RISING | IRQF_ONESHOT, >> + DRIVER_NAME, smo8800); >> + if (err) { >> + dev_err(&device->dev, >> + "failed to request thread for IRQ %d: %d\n", >> + smo8800->irq, err); >> + goto error; >> + } >> + >> + dev_dbg(&device->dev, "device /dev/freefall registered with IRQ %d\n", >> + smo8800->irq); >> } >> >> - platform_set_drvdata(device, smo8800); >> - >> - err = platform_get_irq(device, 0); >> - if (err < 0) >> - goto error; >> - smo8800->irq = err; >> - >> - err = request_threaded_irq(smo8800->irq, smo8800_interrupt_quick, >> - smo8800_interrupt_thread, >> - IRQF_TRIGGER_RISING | IRQF_ONESHOT, >> - DRIVER_NAME, smo8800); >> - if (err) { >> - dev_err(&device->dev, >> - "failed to request thread for IRQ %d: %d\n", >> - smo8800->irq, err); >> - goto error; >> - } >> - >> - dev_dbg(&device->dev, "device /dev/freefall registered with IRQ %d\n", >> - smo8800->irq); >> - >> if (dmi_check_system(smo8800_lis3lv02d_devices)) { >> /* >> * Register i2c-bus notifier + queue initial scan for lis3lv02d >> @@ -350,14 +349,20 @@ static int smo8800_probe(struct platform_device *device) >> } else { >> dev_warn(&device->dev, >> "lis3lv02d accelerometer is present on SMBus but its address is unknown, skipping registration\n"); >> + if (!smo8800->irq) >> + return -ENODEV; >> } >> >> + platform_set_drvdata(device, smo8800); >> return 0; >> >> error_free_irq: >> - free_irq(smo8800->irq, smo8800); >> + if (smo8800->irq) { >> + free_irq(smo8800->irq, smo8800); >> error: >> - misc_deregister(&smo8800->miscdev); >> + misc_deregister(&smo8800->miscdev); >> + } >> + >> return err; >> } >> >> @@ -371,9 +376,11 @@ static void smo8800_remove(struct platform_device *device) >> i2c_unregister_device(smo8800->i2c_dev); >> } >> >> - free_irq(smo8800->irq, smo8800); >> - misc_deregister(&smo8800->miscdev); >> - dev_dbg(&device->dev, "device /dev/freefall unregistered\n"); >> + if (smo8800->irq) { >> + free_irq(smo8800->irq, smo8800); >> + misc_deregister(&smo8800->miscdev); >> + dev_dbg(&device->dev, "device /dev/freefall unregistered\n"); >> + } >> } >> >> static const struct acpi_device_id smo8800_ids[] = { >> -- >> 2.45.1 >> >
On Saturday 22 June 2024 16:07:53 Hans de Goede wrote: > Hi Pali, > > On 6/22/24 3:20 PM, Pali Rohár wrote: > > On Friday 21 June 2024 14:24:59 Hans de Goede wrote: > >> The Dell XPS 15 9550 can have a 60Wh battery, leaving space for a 2.5" > >> sata disk; or a 90Wh battery in which case the battery occupies the space > >> for the optional 2.5" sata disk. > >> > >> On models with the 90Wh battery and thus without a 2.5" sata disk, the BIOS > >> does not add an IRQ resource to the SMO8810 ACPI device. > > > > That is a pity, but OK, manufacturer decided that freefall sensor is > > enabled by BIOS firmware only if the SATA is present. > > > >> Make the misc-device registration and the requesting of the IRQ optional > >> and instantiate a lis3lv02d i2c_client independent of the IRQ being there, > >> so that the non freefall lis3lv02d functionality can still be used. > >> > >> Note that IRQ 0 is not a valid IRQ number for platform IRQs > >> and this patch relies on that. > >> > >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> > >> --- > >> drivers/platform/x86/dell/dell-smo8800.c | 67 +++++++++++++----------- > >> 1 file changed, 37 insertions(+), 30 deletions(-) > >> > >> diff --git a/drivers/platform/x86/dell/dell-smo8800.c b/drivers/platform/x86/dell/dell-smo8800.c > >> index cd2e48405859..2e49bbb569c6 100644 > >> --- a/drivers/platform/x86/dell/dell-smo8800.c > >> +++ b/drivers/platform/x86/dell/dell-smo8800.c > >> @@ -310,33 +310,32 @@ static int smo8800_probe(struct platform_device *device) > >> init_waitqueue_head(&smo8800->misc_wait); > >> INIT_WORK(&smo8800->i2c_work, smo8800_instantiate_i2c_client); > >> > >> - err = misc_register(&smo8800->miscdev); > >> - if (err) { > >> - dev_err(&device->dev, "failed to register misc dev: %d\n", err); > >> - return err; > >> + err = platform_get_irq_optional(device, 0); > >> + if (err > 0) > >> + smo8800->irq = err; > > > > This code should be rather change to fail immediately. If the IRQ number > > is not provided by the BIOS then the ACPI SMO88xx is not usable for us > > at all. So return error from the smo8800_probe function. > > The goal of this patch is to still register the bus-notifier for i2c-client > instantiation for the lis3lv02d driver. Existing immediately here (as was > done before) means we will still not register the bus-notifier. If the lis3 part would be moved into separate module then there is no need to add these checks. So it clearly shows that lis3 and smo parts are independent and it could make sense to separate them as pointed under other patch. > Regards, > > Hans > > > > > > >> + > >> + if (smo8800->irq) { > >> + err = misc_register(&smo8800->miscdev); > >> + if (err) { > >> + dev_err(&device->dev, "failed to register misc dev: %d\n", err); > >> + return err; > >> + } > >> + > >> + err = request_threaded_irq(smo8800->irq, smo8800_interrupt_quick, > >> + smo8800_interrupt_thread, > >> + IRQF_TRIGGER_RISING | IRQF_ONESHOT, > >> + DRIVER_NAME, smo8800); > >> + if (err) { > >> + dev_err(&device->dev, > >> + "failed to request thread for IRQ %d: %d\n", > >> + smo8800->irq, err); > >> + goto error; > >> + } > >> + > >> + dev_dbg(&device->dev, "device /dev/freefall registered with IRQ %d\n", > >> + smo8800->irq); > >> } > >> > >> - platform_set_drvdata(device, smo8800); > >> - > >> - err = platform_get_irq(device, 0); > >> - if (err < 0) > >> - goto error; > >> - smo8800->irq = err; > >> - > >> - err = request_threaded_irq(smo8800->irq, smo8800_interrupt_quick, > >> - smo8800_interrupt_thread, > >> - IRQF_TRIGGER_RISING | IRQF_ONESHOT, > >> - DRIVER_NAME, smo8800); > >> - if (err) { > >> - dev_err(&device->dev, > >> - "failed to request thread for IRQ %d: %d\n", > >> - smo8800->irq, err); > >> - goto error; > >> - } > >> - > >> - dev_dbg(&device->dev, "device /dev/freefall registered with IRQ %d\n", > >> - smo8800->irq); > >> - > >> if (dmi_check_system(smo8800_lis3lv02d_devices)) { > >> /* > >> * Register i2c-bus notifier + queue initial scan for lis3lv02d > >> @@ -350,14 +349,20 @@ static int smo8800_probe(struct platform_device *device) > >> } else { > >> dev_warn(&device->dev, > >> "lis3lv02d accelerometer is present on SMBus but its address is unknown, skipping registration\n"); > >> + if (!smo8800->irq) > >> + return -ENODEV; > >> } > >> > >> + platform_set_drvdata(device, smo8800); > >> return 0; > >> > >> error_free_irq: > >> - free_irq(smo8800->irq, smo8800); > >> + if (smo8800->irq) { > >> + free_irq(smo8800->irq, smo8800); > >> error: > >> - misc_deregister(&smo8800->miscdev); > >> + misc_deregister(&smo8800->miscdev); > >> + } > >> + > >> return err; > >> } > >> > >> @@ -371,9 +376,11 @@ static void smo8800_remove(struct platform_device *device) > >> i2c_unregister_device(smo8800->i2c_dev); > >> } > >> > >> - free_irq(smo8800->irq, smo8800); > >> - misc_deregister(&smo8800->miscdev); > >> - dev_dbg(&device->dev, "device /dev/freefall unregistered\n"); > >> + if (smo8800->irq) { > >> + free_irq(smo8800->irq, smo8800); > >> + misc_deregister(&smo8800->miscdev); > >> + dev_dbg(&device->dev, "device /dev/freefall unregistered\n"); > >> + } > >> } > >> > >> static const struct acpi_device_id smo8800_ids[] = { > >> -- > >> 2.45.1 > >> > > >
diff --git a/drivers/platform/x86/dell/dell-smo8800.c b/drivers/platform/x86/dell/dell-smo8800.c index cd2e48405859..2e49bbb569c6 100644 --- a/drivers/platform/x86/dell/dell-smo8800.c +++ b/drivers/platform/x86/dell/dell-smo8800.c @@ -310,33 +310,32 @@ static int smo8800_probe(struct platform_device *device) init_waitqueue_head(&smo8800->misc_wait); INIT_WORK(&smo8800->i2c_work, smo8800_instantiate_i2c_client); - err = misc_register(&smo8800->miscdev); - if (err) { - dev_err(&device->dev, "failed to register misc dev: %d\n", err); - return err; + err = platform_get_irq_optional(device, 0); + if (err > 0) + smo8800->irq = err; + + if (smo8800->irq) { + err = misc_register(&smo8800->miscdev); + if (err) { + dev_err(&device->dev, "failed to register misc dev: %d\n", err); + return err; + } + + err = request_threaded_irq(smo8800->irq, smo8800_interrupt_quick, + smo8800_interrupt_thread, + IRQF_TRIGGER_RISING | IRQF_ONESHOT, + DRIVER_NAME, smo8800); + if (err) { + dev_err(&device->dev, + "failed to request thread for IRQ %d: %d\n", + smo8800->irq, err); + goto error; + } + + dev_dbg(&device->dev, "device /dev/freefall registered with IRQ %d\n", + smo8800->irq); } - platform_set_drvdata(device, smo8800); - - err = platform_get_irq(device, 0); - if (err < 0) - goto error; - smo8800->irq = err; - - err = request_threaded_irq(smo8800->irq, smo8800_interrupt_quick, - smo8800_interrupt_thread, - IRQF_TRIGGER_RISING | IRQF_ONESHOT, - DRIVER_NAME, smo8800); - if (err) { - dev_err(&device->dev, - "failed to request thread for IRQ %d: %d\n", - smo8800->irq, err); - goto error; - } - - dev_dbg(&device->dev, "device /dev/freefall registered with IRQ %d\n", - smo8800->irq); - if (dmi_check_system(smo8800_lis3lv02d_devices)) { /* * Register i2c-bus notifier + queue initial scan for lis3lv02d @@ -350,14 +349,20 @@ static int smo8800_probe(struct platform_device *device) } else { dev_warn(&device->dev, "lis3lv02d accelerometer is present on SMBus but its address is unknown, skipping registration\n"); + if (!smo8800->irq) + return -ENODEV; } + platform_set_drvdata(device, smo8800); return 0; error_free_irq: - free_irq(smo8800->irq, smo8800); + if (smo8800->irq) { + free_irq(smo8800->irq, smo8800); error: - misc_deregister(&smo8800->miscdev); + misc_deregister(&smo8800->miscdev); + } + return err; } @@ -371,9 +376,11 @@ static void smo8800_remove(struct platform_device *device) i2c_unregister_device(smo8800->i2c_dev); } - free_irq(smo8800->irq, smo8800); - misc_deregister(&smo8800->miscdev); - dev_dbg(&device->dev, "device /dev/freefall unregistered\n"); + if (smo8800->irq) { + free_irq(smo8800->irq, smo8800); + misc_deregister(&smo8800->miscdev); + dev_dbg(&device->dev, "device /dev/freefall unregistered\n"); + } } static const struct acpi_device_id smo8800_ids[] = {
The Dell XPS 15 9550 can have a 60Wh battery, leaving space for a 2.5" sata disk; or a 90Wh battery in which case the battery occupies the space for the optional 2.5" sata disk. On models with the 90Wh battery and thus without a 2.5" sata disk, the BIOS does not add an IRQ resource to the SMO8810 ACPI device. Make the misc-device registration and the requesting of the IRQ optional and instantiate a lis3lv02d i2c_client independent of the IRQ being there, so that the non freefall lis3lv02d functionality can still be used. Note that IRQ 0 is not a valid IRQ number for platform IRQs and this patch relies on that. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/platform/x86/dell/dell-smo8800.c | 67 +++++++++++++----------- 1 file changed, 37 insertions(+), 30 deletions(-)