Re: [PATCH v2 3/5] PCI: axis: Add ARTPEC-8 PCIe controller driver

[Wangseok Lee]

On 06/11/2022 00:30, Bjorn Helgaas wrote:
> On Fri, Jun 10, 2022 at 09:03:03AM +0900, Wangseok Lee wrote:
>> On 06/04/2022 01:03, Bjorn Helgaas wrote:
>> > In the subject, why do you tag this "axis"?  There's an existing
>> > pcie-artpec6.c that uses the driver name ""artpec6-pcie" and the
>> > subject line tag "artpec6".
>> > 
>> > This adds pcie-artpec8.c with driver name "artpec8-pcie", so the
>> > obvious choice would be "artpec8".
>> > 
>> > I assume you evaluated the possibility of extending artpec6 to support
>> > artpec8 in addition to the artpec6 and artpec7 it already supports?
>>  
>> "pcie-artpec6. c" supports artpec6 and artpec7 H/W.
>> artpec8 can not be expanded because H/W configuration is
>> completely different from artpec6/7.
>> phy and sub controller are different.
> 
> Thanks for this detail.  Can you include this in the commit log next
> time around in case anybody else has a similar question?
> 

Ok, sure.

>> >> +/* FSYS SYSREG Offsets */
>> > 
>> > The list below seems to inclue more than just register offsets.
>> > 
>>  
>> Is it clear to change to "FSYS blue logic system registers" 
>> like Jasper Nilsson`s comment?
>> https://lore.kernel.org/all/20220607070332.GY18902@xxxxxxxx/
>> My opinion is the same.
> 
> Yep, that's fine.  But spell it "glue logic", not "blue logic" :)
> 

Thanks, it was just a typo.

>> >> +static int artpec8_pcie_get_clk_resources(struct platform_device *pdev,
>> >> +                                       struct artpec8_pcie *artpec8_ctrl)
>> >> +{
>> >> +        struct device *dev = &pdev->dev;
>> >> +
>> >> +        artpec8_ctrl->pipe_clk = devm_clk_get(dev, "pipe_clk");
>> >> +        if (IS_ERR(artpec8_ctrl->pipe_clk)) {
>> >> +                dev_err(dev, "couldn't get pipe clock\n");
>> >> +                return -EINVAL;
>> >> +        }
>> >> +
>> >> +        artpec8_ctrl->dbi_clk = devm_clk_get(dev, "dbi_clk");
>> >> +        if (IS_ERR(artpec8_ctrl->dbi_clk)) {
>> >> +                dev_info(dev, "couldn't get dbi clk\n");
>> >> +                return -EINVAL;
>> >> +        }
>> >> +
>> >> +        artpec8_ctrl->slv_clk = devm_clk_get(dev, "slv_clk");
>> >> +        if (IS_ERR(artpec8_ctrl->slv_clk)) {
>> >> +                dev_err(dev, "couldn't get slave clock\n");
>> >> +                return -EINVAL;
>> >> +        }
>> >> +
>> >> +        artpec8_ctrl->mstr_clk = devm_clk_get(dev, "mstr_clk");
>> >> +        if (IS_ERR(artpec8_ctrl->mstr_clk)) {
>> >> +                dev_info(dev, "couldn't get master clk\n");
>> > 
>> > It'd be nice if the err/info messages matched the exact DT name:
>> > "pipe_clk", "dbi_clk", slv_clk", etc.
>> > 
>>  
>> I will fix it.
>>  
>> > Why are some of the above dev_err() and others dev_info() when you
>> > return -EINVAL in all cases?
>>  
>> When property is not found, it just to return error.
>> I will modify to return PTR_ERR.
> 
> 
> Using PTR_ERR() looks like a good idea, since then you return the
> actual error from devm_clk_get() instead of always returning -EINVAL.
> 
> But that wasn't my comment.  My comment was that it looks like these
> should be all dev_err() (or all dev_info()).
> 

I understood your question. I think it was simply a way to generate log
msg.
In this case, is there a more proper print function that use to generate
log msg?
In addition, error return in artpec8_pcie_get_clk_resources() will be
modified to devm_clk_bulk_get().
(according to Krzysztof's review comment.. )

>> >> +        switch (mode) {
>> >> +        case DW_PCIE_RC_TYPE:
>> >> +                artpec8_pcie_writel(artpec8_ctrl->elbi_base, DEVICE_TYPE_RC,
>> >> +                                PCIE_ARTPEC8_DEVICE_TYPE);
>> >> +                ret = artpec8_add_pcie_port(artpec8_ctrl, pdev);
>> >> +                if (ret < 0)
>> > 
>> > Are there positive return values that indicate success?  Most places
>> > above you assume "ret != 0" means failure, so just curious why you
>> > test "ret < 0" instead of just "ret".
>>  
>> There is no special reason, but it seems that the format used 
>> in the existing dw driver is applied.
> 
> Fair enough.  "git grep -A2 add_pcie_port drivers/pci/controller/"
> says all *_add_pcie_port() calls use the same pattern, so thanks for
> following that.
> 
> Bjorn

Thank you for kindness reivew.
 
Best regards,
Wangseok Lee