Date:   Tue, 30 Nov 2021 09:12:14 +0100
From:   Krzysztof =?utf-8?Q?Wilczy=C5=84ski?= <kw@linux.com>
To:     Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Cc:     lorenzo.pieralisi@arm.com, bhelgaas@google.com,
        svarbanov@mm-sol.com, bjorn.andersson@linaro.org, robh@kernel.org,
        linux-pci@vger.kernel.org, linux-arm-msm@vger.kernel.org,
        linux-kernel@vger.kernel.org, kernel test robot <lkp@intel.com>
Subject: Re: [PATCH v2] PCI: qcom: Use __be16 for catching cpu_to_be16()
 return instead of u16
Message-ID: <YaXc3mlBbhvEkZCX@rocinante>
References: <20211130064215.207393-1-manivannan.sadhasivam@linaro.org>
 <YaXSoLpIfrTh81/+@rocinante>
 <20211130080456.GE205712@thinkpad>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20211130080456.GE205712@thinkpad>
Precedence: bulk

Hello!

[...]
> > > cpu_to_be16() returns __be16 value but the driver uses u16 and that's
> > > incorrect. Fix it by using __be16 as the datatype of bdf_be variable.
> > 
> > It would be "data type" in the above.
> > 
> > Not really a requirement to do so, but you could include the actual
> > warning, as sometimes this is useful for reference later, as per:
> > 
> >   drivers/pci/controller/dwc/pcie-qcom.c:1346:30: warning: incorrect type in initializer (different base types)
> >   drivers/pci/controller/dwc/pcie-qcom.c:1346:30:    expected unsigned short [usertype] bdf_be
> >   drivers/pci/controller/dwc/pcie-qcom.c:1346:30:    got restricted __be16 [usertype]
> > 
> 
> I usually do but as per Bjorn's comment I thought it is not recommended for PCI
> subsystem (or maybe I misread his comments). Will add.

Ah right.  I must have missed his comment too.  I usually include warnings
myself, where applicable.  Let's wait for what Bjorn says, just in case, so
that we avoid adding something he does not want to have included in the
commit message.

[...]
> > Also, since I have your attention, it seems we have a number of unused
> > macros in the qcom driver, as per:
> > 
> >   drivers/pci/controller/dwc/pcie-qcom.c:#define PCIE20_PARF_BDF_TRANSLATE_CFG            0x24C
> >   drivers/pci/controller/dwc/pcie-qcom.c:#define PCIE20_PARF_SID_OFFSET                   0x234
> >   drivers/pci/controller/dwc/pcie-qcom.c:#define PCIE20_PARF_SLV_ADDR_SPACE_SIZE          0x16C
> > 
> > And also in the qcom-ep driver, as per:
> > 
> 
> These defines are helpful for someone who wants to add some functionality or
> even bug fix in the future. Since these controllers are not publically
> documented, having these definitions helps a lot.

Got it!  I run a nightly CI pipeline and been seeing complains due to the
"-Wunused-macros" show up in the log, so I decided to steal the spotlight,
so to speak, to ask about these.

Thank you for letting me know.  Appreciated!

	Krzysztof