Received-SPF: pass (google.com: domain of linux-kernel+bounces-65135-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Wed, 14 Feb 2024 17:04:02 +0530
From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
To: Lukas Wunner <lukas@wunner.de>
Cc: Bjorn Helgaas <bhelgaas@google.com>,
	Bjorn Andersson <andersson@kernel.org>,
	Konrad Dybcio <konrad.dybcio@linaro.org>,
	Lorenzo Pieralisi <lpieralisi@kernel.org>,
	Krzysztof Wilczy??ski <kw@linux.com>, Rob Herring <robh@kernel.org>,
	Mika Westerberg <mika.westerberg@linux.intel.com>,
	quic_krichai@quicinc.com, linux-pci@vger.kernel.org,
	linux-kernel@vger.kernel.org, linux-arm-msm@vger.kernel.org
Subject: Re: [PATCH v2] PCI: Add D3 support for PCI bridges in DT based
 platforms
Message-ID: <20240214113402.GF4618@thinkpad>
References: <20240214-pcie-qcom-bridge-v2-1-9dd6dbb1b817@linaro.org>
 <20240214102837.GA30420@wunner.de>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20240214102837.GA30420@wunner.de>

On Wed, Feb 14, 2024 at 11:28:37AM +0100, Lukas Wunner wrote:
> On Wed, Feb 14, 2024 at 02:18:31PM +0530, Manivannan Sadhasivam wrote:
> > +/**
> > + * of_pci_bridge_d3 - Check if the bridge is supporting D3 states or not
> > + *
> > + * @node: device tree node of the bridge
> > + *
> > + * Return: True if the bridge is supporting D3 states, False otherwise.
> 
> A lot of kernel-doc uses %true and %false.
> 

Ack.

> 
> > +bool of_pci_bridge_d3(struct device_node *node)
> > +{
> > +	return of_property_read_bool(node, "supports-d3");
> > +}
> 
> What's the difference between of_property_read_bool() and
> of_property_present()?  When should one use which?
> The former has 691 occurrences in the tree, the latter 120.
> The latter would seem more "literary" / readable here,
> but maybe that's just me.
> 

of_property_present() just calls of_property_read_bool() and it is fairly new.
But yeah, the API name itself indicates that it is better suited for the
purpose. Will change it.

> 
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> > @@ -1142,6 +1142,9 @@ static inline bool platform_pci_bridge_d3(struct pci_dev *dev)
> >  	if (pci_use_mid_pm())
> >  		return false;
> >  
> > +	if (dev->dev.of_node)
> > +		return of_pci_bridge_d3(dev->dev.of_node);
> > +
> >  	return acpi_pci_bridge_d3(dev);
> >  }
> 
> This will result in an unnecessary test on non-DT platforms (e.g. ACPI)
> whether dev->dev.of_node is set.
> 
> Please use dev_of_node() instead of "dev->dev.of_node" so that the
> code added here can be optimized away by the compiler on non-DT
> platforms (due to the IS_ENABLED(CONFIG_OF)).
> 

Sounds good.

- Mani

-- 
மணிவண்ணன் சதாசிவம்