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

[Segher Boessenkool]

>>> +     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.
>>
>>> +     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].
>
> These two changes are contradictory.

No they're not.  If you don't get exactly 4 bytes back, you have
to fail the device, since you don't know how to handle it.  If
on the other hand you don't have the property at all, you use
the default value.

>>> +static int ipmi_of_remove(struct of_device *dev)
>>> +{
>>> +     /* should call
>>> +      * cleanup_one_si(dev->dev.driver_data); */
>>
>> 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.

Oh, this is broken in the existing stuff as well?  Never mind then.

>>> +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?
>
> The one alternative might be something more complicated like:
>
> static const enum si_type __devinitdata of_ipmi_dev_info[] = {
> 	[SI_KCS] SI_KCS,
> 	[SI_SMIC] SI_SMIC,
> 	[SI_BT] SI_BT,
> };
>
> static const struct of_device_id of_ipmi_match[] = {
> 	{ .type = "ipmi", .compatible = "ipmi-kcs",  .data =  
> &of_ipmi_dev_info[SI_KCS] },
> 	{ .type = "ipmi", .compatible = "ipmi-kcs",  .data =  
> &of_ipmi_dev_info[SI_SMIC] },
> 	{ .type = "ipmi", .compatible = "ipmi-kcs",  .data =  
> &of_ipmi_dev_info[SI_BT] },
> };
>
> Not sure if that's worth it.

Yeah, I don't think so either.  Maybe we should have some macro's
though that hide the casts (and make them right!), this stuff is
(ab)used a lot in Linux (and everyone gets it wrong always).

Any way around, the cast in this driver needs fixing.


Segher