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

[Arnd Bergmann <arnd@arndb.de>]

On Monday 18 December 2006 22:52, Segher Boessenkool wrote:

> > +static int ipmi_of_probe(struct of_device *dev,
> > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 const =
struct of_device_id *match)
>=20
> Shouldn't this (and everything else) be some kind of __init?

It should be __devinit.

> > +=A0=A0=A0=A0=A0regsize =3D get_property(np, "reg-size", NULL);
> > +=A0=A0=A0=A0=A0regspacing =3D get_property(np, "reg-spacing", NULL);
> > +=A0=A0=A0=A0=A0regshift =3D get_property(np, "reg-shift", NULL);
>=20
> You should check whether you get exactly 4 bytes back or not.
>
> > +=A0=A0=A0=A0=A0if (!regsize)
> > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0info->io.regsize =3D DEFAULT_RE=
GSPACING;
> > +=A0=A0=A0=A0=A0else
> > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0info->io.regsize =3D *regsize;
>=20
> info->io_regsize =3D regsize ? *regsize : DEFAULT_REGSIZE;
>=20
> [Please note that fixes a copy/paste bug, too].

These two changes are contradictory. It's either

    regsize =3D get_property(np, "reg-size", NULL);
    info->io_regsize =3D regsize ? *regsize : DEFAULT_REGSIZE;

or

    regsize =3D get_property(np, "reg-size", &proplen);
    info->io_regsize =3D (proplen =3D=3D 4) ? *regsize : DEFAULT_REGSIZE;

> > +static int ipmi_of_remove(struct of_device *dev)
> > +{
> > +=A0=A0=A0=A0=A0/* should call
> > +=A0=A0=A0=A0=A0 * cleanup_one_si(dev->dev.driver_data); */
>=20
> So fix that :-)

That should be a separate patch that fixes the same thing for
pci ipmi devices as well. It needs to move code around for this
(or introduce forward declarations), and the effect is no
different, so it's really just a cleanup.

> > +static struct of_device_id ipmi_match[] =3D
> > +{
> > +=A0=A0=A0=A0=A0{ .type =3D "ipmi", .compatible =3D "ipmi-kcs", =A0.dat=
a =3D (void*)=20
> > SI_KCS },
> > +=A0=A0=A0=A0=A0{ .type =3D "ipmi", .compatible =3D "ipmi-smic", .data =
=3D (void*)=20
> > SI_SMIC },
> > +=A0=A0=A0=A0=A0{ .type =3D "ipmi", .compatible =3D "ipmi-bt", =A0 .dat=
a =3D (void*)SI_BT },
>=20
> That cast-to-pointer is what gives you that warning when
> casting back. =A0Is there no better solution?

The one alternative might be something more complicated like:

static const enum si_type __devinitdata of_ipmi_dev_info[] =3D {
	[SI_KCS] SI_KCS,
	[SI_SMIC] SI_SMIC,
	[SI_BT] SI_BT,
};

static const struct of_device_id of_ipmi_match[] =3D {
	{ .type =3D "ipmi", .compatible =3D "ipmi-kcs",  .data =3D &of_ipmi_dev_in=
fo[SI_KCS] },
	{ .type =3D "ipmi", .compatible =3D "ipmi-kcs",  .data =3D &of_ipmi_dev_in=
fo[SI_SMIC] },
	{ .type =3D "ipmi", .compatible =3D "ipmi-kcs",  .data =3D &of_ipmi_dev_in=
fo[SI_BT] },
};

Not sure if that's worth it.

	Arnd <><