[patch 0/1] ipmi: update: add autosensing of ipmi devices on powerpc using of device tree

[Segher Boessenkool]

> The driver will be probed for all devices with the type ipmi. It's  
> supporting
> devices with compatible settings ipmi-kcs, ipmi-smic and ipmi-bt.
> Only ipmi-kcs could be tested.

I'd only include that one then.  But the code is trivial,
the risk is minimal, why not.


> +/* only exists on powerpc */
> +#ifdef CONFIG_PPC_OF
> +#include <asm/of_device.h>
> +#include <asm/of_platform.h>
> +#endif

Comment is inexact -- just kill it.

> +static int ipmi_of_probe(struct of_device *dev,
> +			 const struct of_device_id *match)

Shouldn't this (and everything else) be some kind of __init?

> +	regsize = get_property(np, "reg-size", NULL);
> +	regspacing = get_property(np, "reg-spacing", NULL);
> +	regshift = get_property(np, "reg-shift", NULL);

You should check whether you get exactly 4 bytes back or not.

> +	info->si_type		= (enum si_type) match->data;

That lands you a compiler warning (cast from pointer to shorter
integer) on 64-bit.

> +	if (!regsize)
> +		info->io.regsize = DEFAULT_REGSPACING;
> +	else
> +		info->io.regsize = *regsize;

info->io_regsize = regsize ? *regsize : DEFAULT_REGSIZE;

[Please note that fixes a copy/paste bug, too].

> +	dev_dbg(&dev->dev, "addr 0x%lx regsize %ld spacing %ld irq %x\n",
> +		info->io.addr_data, info->io.regsize, info->io.regspacing,
> +		info->irq);

Please all addresses/sizes/spacings in hexadecimal?  And
you might want to output regshift, too.

> +static int ipmi_of_remove(struct of_device *dev)
> +{
> +	/* should call
> +	 * cleanup_one_si(dev->dev.driver_data); */

So fix that :-)

> +	return 0;
> +}

> +static struct of_device_id ipmi_match[] =
> +{
> +	{ .type = "ipmi", .compatible = "ipmi-kcs",  .data = (void*) 
> SI_KCS },
> +	{ .type = "ipmi", .compatible = "ipmi-smic", .data = (void*) 
> SI_SMIC },
> +	{ .type = "ipmi", .compatible = "ipmi-bt",   .data = (void*)SI_BT },

That cast-to-pointer is what gives you that warning when
casting back.  Is there no better solution?


All pretty minor, but please fix.  Looks like you're almost
there :-)


Segher