| Message ID | 20211130172913.9727-2-kabel@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Lorenzo Pieralisi |
| Headers | show |
| Series | PCI: aardvark controller fixes BATCH 3 | expand |
On Tue, Nov 30, 2021 at 06:29:03PM +0100, Marek Behún wrote: > From: Pali Rohár <pali@kernel.org> > > The current assignment to the class_revision member > > class_revision |= cpu_to_le32(PCI_CLASS_BRIDGE_PCI << 16); > > can make the reader think that class is at high 16 bits of the member and > revision at low 16 bits. > > In reality, class is at high 24 bits, but the class for PCI Bridge Normal > Decode is PCI_CLASS_BRIDGE_PCI << 8. > > Change the assignment and add a comment to make this clearer. > > Signed-off-by: Pali Rohár <pali@kernel.org> > Signed-off-by: Marek Behún <kabel@kernel.org> > --- > drivers/pci/pci-bridge-emul.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/pci/pci-bridge-emul.c b/drivers/pci/pci-bridge-emul.c > index db97cddfc85e..a4af1a533d71 100644 > --- a/drivers/pci/pci-bridge-emul.c > +++ b/drivers/pci/pci-bridge-emul.c > @@ -265,7 +265,11 @@ int pci_bridge_emul_init(struct pci_bridge_emul *bridge, > { > BUILD_BUG_ON(sizeof(bridge->conf) != PCI_BRIDGE_CONF_END); > > - bridge->conf.class_revision |= cpu_to_le32(PCI_CLASS_BRIDGE_PCI << 16); > + /* > + * class_revision: Class is high 24 bits and revision is low 8 bit of this member, > + * while class for PCI Bridge Normal Decode has the 24-bit value: PCI_CLASS_BRIDGE_PCI << 8 > + */ Can you please re-wrap this comment so it fits in 80 columns like the rest of the file? I'd do something with the assignment, too. It's hard to read when it wraps, especially since at 80 columns it splits the "<<" in half. I assume from the commit log that this is purely a readability change, not a fix, right? > + bridge->conf.class_revision |= cpu_to_le32((PCI_CLASS_BRIDGE_PCI << 8) << 8); > bridge->conf.header_type = PCI_HEADER_TYPE_BRIDGE; > bridge->conf.cache_line_size = 0x10; > bridge->conf.status = cpu_to_le16(PCI_STATUS_CAP_LIST); > -- > 2.32.0 >
On Fri, 3 Dec 2021 10:36:49 -0600 Bjorn Helgaas <helgaas@kernel.org> wrote: > On Tue, Nov 30, 2021 at 06:29:03PM +0100, Marek Behún wrote: > > From: Pali Rohár <pali@kernel.org> > > > > The current assignment to the class_revision member > > > > class_revision |= cpu_to_le32(PCI_CLASS_BRIDGE_PCI << 16); > > > > can make the reader think that class is at high 16 bits of the member and > > revision at low 16 bits. > > > > In reality, class is at high 24 bits, but the class for PCI Bridge Normal > > Decode is PCI_CLASS_BRIDGE_PCI << 8. > > > > Change the assignment and add a comment to make this clearer. > > > > Signed-off-by: Pali Rohár <pali@kernel.org> > > Signed-off-by: Marek Behún <kabel@kernel.org> > > --- > > drivers/pci/pci-bridge-emul.c | 6 +++++- > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/pci/pci-bridge-emul.c b/drivers/pci/pci-bridge-emul.c > > index db97cddfc85e..a4af1a533d71 100644 > > --- a/drivers/pci/pci-bridge-emul.c > > +++ b/drivers/pci/pci-bridge-emul.c > > @@ -265,7 +265,11 @@ int pci_bridge_emul_init(struct pci_bridge_emul *bridge, > > { > > BUILD_BUG_ON(sizeof(bridge->conf) != PCI_BRIDGE_CONF_END); > > > > - bridge->conf.class_revision |= cpu_to_le32(PCI_CLASS_BRIDGE_PCI << 16); > > + /* > > + * class_revision: Class is high 24 bits and revision is low 8 bit of this member, > > + * while class for PCI Bridge Normal Decode has the 24-bit value: PCI_CLASS_BRIDGE_PCI << 8 > > + */ > > Can you please re-wrap this comment so it fits in 80 columns like the > rest of the file? Bjorn, Lorenzo already applied this patches to his branch. Should I send him a fixup, or try to persuade him to rebase? :) > I'd do something with the assignment, too. It's hard to read when it > wraps, especially since at 80 columns it splits the "<<" in half. > > I assume from the commit log that this is purely a readability change, > not a fix, right? Yes, you are right. No functional change.
diff --git a/drivers/pci/pci-bridge-emul.c b/drivers/pci/pci-bridge-emul.c index db97cddfc85e..a4af1a533d71 100644 --- a/drivers/pci/pci-bridge-emul.c +++ b/drivers/pci/pci-bridge-emul.c @@ -265,7 +265,11 @@ int pci_bridge_emul_init(struct pci_bridge_emul *bridge, { BUILD_BUG_ON(sizeof(bridge->conf) != PCI_BRIDGE_CONF_END); - bridge->conf.class_revision |= cpu_to_le32(PCI_CLASS_BRIDGE_PCI << 16); + /* + * class_revision: Class is high 24 bits and revision is low 8 bit of this member, + * while class for PCI Bridge Normal Decode has the 24-bit value: PCI_CLASS_BRIDGE_PCI << 8 + */ + bridge->conf.class_revision |= cpu_to_le32((PCI_CLASS_BRIDGE_PCI << 8) << 8); bridge->conf.header_type = PCI_HEADER_TYPE_BRIDGE; bridge->conf.cache_line_size = 0x10; bridge->conf.status = cpu_to_le16(PCI_STATUS_CAP_LIST);