On Mon, 2008-04-21 at 18:01 +0800, Jin Zhengxiong wrote: > Hi, Michael, > > Thank you very much for you input, please see my inline answer.
No worries. > > > +static int fsl_msi_reserve_dt_hwirqs(struct fsl_msi *msi) > > > +{ > > > + int i, len; > > > + const u32 *p; > > > + > > > + p = of_get_property(msi->of_node, "msi-available-ranges", &len); > > > + if (!p) { > > > + pr_debug("fsl_msi: no msi-available-ranges > > property found \ > > > + on %s\n", msi->of_node->full_name); > > > + return -ENODEV; > > > + } > > > + > > > + if (len & 0x8 != 0) { > > > + printk(KERN_WARNING "fsl_msi: Malformed > > msi-available-ranges " > > > + "property on %s\n", msi->of_node->full_name); > > > + return -EINVAL; > > > + } > > > > Do you really want a bitwise and with 0x8? > > > The range for the msi interrupt can be seperated to several part. > This can used to check the if the ranges is correct. I don't see how. AFAIK the "msi-available-ranges" property is just a list of u32 pairs, so the only thing that makes sense is to check that the length is a multiple of 8, not that it has the 3rd bit set. > > > +static void fsl_compose_msi_msg(struct pci_dev *pdev, int hwirq, > > > + struct msi_msg *msg) > > > +{ > > > + unsigned int srs; > > > + unsigned int ibs; > > > + struct fsl_msi *msi = fsl_msi; > > > + > > > + srs = hwirq / INT_PER_MSIR; > > > + ibs = hwirq % INT_PER_MSIR; > > > + > > > + msg->address_lo = msi->msi_addr_lo; > > > + msg->address_hi = msi->msi_addr_hi; > > > + msg->data = (srs << 5) | (ibs & 0x1F); > > > > Is the 5 and 0x1F independent of the INT_PER_MSIR value? Given the > > current values isn't this a no-op, or am I missing something? > > > Do you mean there're another way to get the msg->data from the hwirq? No I mean I'm confused about the maths here. If we pull out the variables it boils down to: data = ((hwirq / 32) << 5) | ((hwirq % 32) & 0x1F) Which doesn't seem to actually do anything? > > > +static int fsl_setup_msi_irqs(struct pci_dev *pdev, int > > nvec, int type) > > > +{ > > > + irq_hw_number_t hwirq; > > > + int rc; > > > + unsigned int virq; > > > + struct msi_desc *entry; > > > + struct msi_msg msg; > > > + struct fsl_msi *msi = fsl_msi; > > > > A couple of places you put this into a local called "msi" > > which is not the > > greatest name in the world IMHO :) > > > Thank you, I'll try to use another name, Do you have any suggestion? Oh I dunno, maybe msi_data or msi_state ? cheers -- Michael Ellerman OzLabs, IBM Australia Development Lab wwweb: http://michael.ellerman.id.au phone: +61 2 6212 1183 (tie line 70 21183) We do not inherit the earth from our ancestors, we borrow it from our children. - S.M.A.R.T Person
signature.asc
Description: This is a digitally signed message part
_______________________________________________ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev