[PATCH 6/7] MPIC MSI allocator

[Milton Miller]

On Apr 22, 2007, at 11:04 PM, Michael Ellerman wrote:

> On Sat, 2007-04-21 at 18:17 -0500, Milton Miller wrote:
>> On Apr 19, 2007, Michael Ellerman wrote:
>>> To support MSI on MPIC we need a way to reserve and allocate hardware
>>> irq
>>> numbers, this patch implements an allocator for that.

>>> +	/* Reserve source numbers we know are reserved in the HW */
>>> +	for (i = 0;   i < 8;   i++) __mpic_msi_reserve_hwirq(mpic, i);
>>> +	for (i = 42;  i < 46;  i++) __mpic_msi_reserve_hwirq(mpic, i);
>>> +	for (i = 100; i < 105; i++) __mpic_msi_reserve_hwirq(mpic, i);
>>
>> More lines please.
>>
>>> +#else
>>> +static int mpic_msi_reserve_u3_hwirqs(struct mpic *mpic) { return 
>>> -1;
>>> }
>>
>> and here.
>
> Blank lines aren't free you know!

I didn't ask for any blank ones, so you can keep the bill. :-)

>>
>>> +	if (len % 8 != 0) {

Here you use 8 as the size of the element to process.

>>> +		printk(KERN_WARNING "mpic: Malformed msi-available-ranges "
>>> +		       "property on %s\n", mpic->of_node->full_name);
>>> +		return -EINVAL;
>>> +	}
>>> +
>>> +	bitmap_allocate_region(mpic->hwirq_bitmap, 0,
>>> +			       fls(mpic->irq_count) - 1);
>>> +
>>> +	/* Format is: (<u32 start> <u32 count>)+ */
>>> +	len /= sizeof(u32);

Here you hide a divide by 4 and follow with divide by 2.

>>> +	for (i = 0; i < len / 2; i++, p += 2)
>>
>> how about just dividing by the calculated 8 above?  or use
>> count = len / 8.
>
> I'm not sure I follow. If you can think of a clearer way I'm all ears,
> or are you just trying to save a divide :)

I'm saying that it doesn't make sense to do both of the above.
I'd move the comment on being pairs of cells to before the
if () EINVAL then follow immediately with the divide.  If you
think that hides length being adjusted then I was optionally
suggesting making a new variable count to use in the for loop.

Actually, since i isn't used in the loop you could just
increment i by 8 while < len, and save both divides.

milton