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@xxxxxxxxxx>
> >> ---
> >> 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
> >>
> >
>
