Re: [PATCH 2/9] platform/x86: x86-android-tablets: Move core code into new core.c file

[Andy Shevchenko <andy@kernel.org>]

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