On Mon, Feb 20, 2023 at 11:12:05PM +0100, Hans de Goede wrote:
> Move the helpers to get irqs + gpios as well as the core code for
> instantiating all the devices missing from ACPI into a new core.c file.
...
> +static int gpiochip_find_match_label(struct gpio_chip *gc, void *data)
> +{
> + return gc->label && !strcmp(gc->label, data);
> +}
Can we actually export find_chip_by_name() from GPIOLIB and reuse it here?
(Perhaps it may require a separate patch(es) to be added)
...
> + case X86_ACPI_IRQ_TYPE_GPIOINT:
> + /* Like acpi_dev_gpio_irq_get(), but without parsing ACPI resources */
> + ret = x86_android_tablet_get_gpiod(data->chip, data->index, &gpiod);
> + if (ret)
> + return ret;
> +
> + irq = gpiod_to_irq(gpiod);
> + if (irq < 0) {
> + pr_err("error %d getting IRQ %s %d\n", irq, data->chip, data->index);
> + return irq;
> + }
> +
> + irq_type = acpi_dev_get_irq_type(data->trigger, data->polarity);
> + if (irq_type != IRQ_TYPE_NONE && irq_type != irq_get_trigger_type(irq))
> + irq_set_irq_type(irq, irq_type);
> +
> + return irq;
Similar Q here.
...
> +static void x86_android_tablet_cleanup(void)
> +{
> + int i;
unsigned?
> + for (i = 0; i < serdev_count; i++) {
> + if (serdevs[i])
Interesting that serdev requires this check in the callers.
> + serdev_device_remove(serdevs[i]);
> + }
> +
> + kfree(serdevs);
> +
> + for (i = 0; i < pdev_count; i++)
> + platform_device_unregister(pdevs[i]);
> +
> + kfree(pdevs);
> +
> + for (i = 0; i < i2c_client_count; i++)
> + i2c_unregister_device(i2c_clients[i]);
> +
> + kfree(i2c_clients);
> +
> + if (exit_handler)
> + exit_handler();
> +
> + for (i = 0; gpiod_lookup_tables && gpiod_lookup_tables[i]; i++)
> + gpiod_remove_lookup_table(gpiod_lookup_tables[i]);
> +
> + software_node_unregister(bat_swnode);
> +}
...
> +extern const struct dmi_system_id x86_android_tablet_ids[];
Why not in the header?
...
> + gpiod_lookup_tables = dev_info->gpiod_lookup_tables;
> + for (i = 0; gpiod_lookup_tables && gpiod_lookup_tables[i]; i++)
> + gpiod_add_lookup_table(gpiod_lookup_tables[i]);
gpiod_add_lookup_tables() ?
...
> +#ifndef __X86_ANDROID_TABLETS_H
> +#define __X86_ANDROID_TABLETS_H
> +#include <linux/gpio/consumer.h>
> +#include <linux/gpio/machine.h>
I don't see users of these headers here (forward declarations may do the job).
...
> +#include <linux/platform_device.h>
Ditto.
And seems missing other forward declaration(s?), like software_node.
--
With Best Regards,
Andy Shevchenko
