[PATCH 1/3] [PATCH] geode: Update OLPC power management

[Arnd Bergmann]

On Friday 02 March 2007 22:36:37 Jordan Crouse wrote:
>
> +#define MFGPT_NAME "geode-mfgpt"
> +
>  static struct {
> -	char *name;
> -	u32 msr;
> -	int size;
> -	u32 base;
> -} lbars[] = {
> -	{ "geode-pms",   MSR_LBAR_PMS, LBAR_PMS_SIZE, 0 },
> -	{ "geode-acpi",  MSR_LBAR_ACPI, LBAR_ACPI_SIZE, 0 },
> -	{ "geode-gpio",  MSR_LBAR_GPIO, LBAR_GPIO_SIZE, 0 },
> -	{ "geode-mfgpt", MSR_LBAR_MFGPT, LBAR_MFGPT_SIZE, 0 }
> +	const char *name;
> +	void __iomem *base;
> +	int (*suspend)(pm_message_t, void *);
> +	int (*resume)(void *);
> +	void *data;
> +} geode_devices[] = {
> +	{ "geode-smb" },
> +	{ "geode-gpio" },
> +	{ MFGPT_NAME },
> +	{ "geode-interrupt" },
> +	{ "geode-pms" },
> +	{ "geode-acpi" }
>  };

Not sure I understand this array. Are you enumerating functions of
a pci device or bars of a device function, or something else?

If it's functions, you should not need the list at all, just
have different pci_driver structures for them. If it's bars,
then it should not be called 'geode_devices'. Also, the use
of the MFGPT_NAME macro in there looks weird.


> -int
> -geode_gpio_isset(unsigned int gpio, unsigned int reg)
> +int geode_gpio_isset(unsigned int gpio, unsigned int reg)
>  {
> -	u32 base = geode_get_dev_base(GEODE_DEV_GPIO);
> +	void __iomem *base = geode_get_dev_base(GEODE_DEV_GPIO);
>
> -	if (!base)
> +	if (base == NULL)
>  		return 0;

Huh? normally, 'if (!base)' is preferable.

>  	if (gpio < 16)
> -		return (inl(base + reg) & (1 << gpio)) ? 1 :0;
> +		return (ioread32(base + reg) & (1 << gpio)) ? 1 :0;
>  	else
> -		return (inl(base + 0x80 + reg) & (1 << (gpio - 16))) ? 1 : 0;
> +		return (ioread32(base + 0x80 + reg) & (1 << (gpio - 16))) ? 1 : 0;
>  }

This change looks useful, but does the conversion to pci_iomap have anything
to do with the power management implementation? Maybe that should be a 
separate patch instead.

> +static struct pci_driver geode_dev_driver = {
> +	.name = "geode-southbridge",
> +	.id_table = geode_dev_id_table,
> +	.probe = geode_dev_probe,
> +#ifdef CONFIG_PM
> +	.suspend = geode_dev_suspend,
> +	.resume = geode_dev_resume
> +#endif
> +};

I'd put a comma after geode_dev_resume, which has the advantage
that another patch could add one more function after the #endif.

> +static int __init geode_southbridge_init(void) {
> +
> +	if (is_geode())
> +		return pci_register_driver(&geode_dev_driver);
> +	else
> +		return -1;
> +}
> +
> +fs_initcall(geode_southbridge_init);

fs_initcall? It does not make much sense to me, but it may be
the best you can do, in which case it at least needs a comment.
Have you tried subsys_initcall?

> -static __init int scx200_scan_pci(void)
> +static __init int scx200_do_scan(void)
>  {
> -	int data, dev;
> -	int rc = -ENODEV;
>  	struct pci_dev *pdev;
> +	int ret;
>
> -	for(dev = 0; dev < ARRAY_SIZE(scx200_pci); dev++) {
> -		pdev = pci_get_device(scx200_pci[dev].vendor,
> -				scx200_pci[dev].device, NULL);
> +	/* Search for a PCI device that will give us a clue as to which
> +	   SMBUS block we are dealing with. */

Is it possible to use pci_register_driver here instead of doing
your own scanning? That should make this driver somewhat cleaner.

> +struct geode_mfgpt_timer_t {
> +	int timer;
> +	int domain;
> +	struct module *owner;
> +	void (*suspend)(int);
> +	void (*resume)(int);
> +};

The _t suffix is normally used for typedefs, which this isn't.

	Arnd <><