Re: [RFC PATCH 2/7] media: ipu7: add Intel IPU7 PCI device driver

[Bingbu Cao <bingbu.cao@linux.intel.com>]

On 2/26/25 6:00 PM, Philipp Stanner wrote:
> On Fri, 2025-02-21 at 15:52 +0800, bingbu.cao@intel.com wrote:
>> +
> 
> [SNIP]
> 
>> +static int ipu7_pci_probe(struct pci_dev *pdev, const struct
>> pci_device_id *id)
>> +{
>> +	struct ipu_buttress_ctrl *isys_ctrl = NULL, *psys_ctrl =
>> NULL;
>> +	struct fwnode_handle *fwnode = dev_fwnode(&pdev->dev);
>> +	const struct ipu_buttress_ctrl *isys_buttress_ctrl;
>> +	const struct ipu_buttress_ctrl *psys_buttress_ctrl;
>> +	struct ipu_isys_internal_pdata *isys_ipdata;
>> +	struct ipu_psys_internal_pdata *psys_ipdata;
>> +	unsigned int dma_mask = IPU_DMA_MASK;
>> +	struct device *dev = &pdev->dev;
>> +	void __iomem *isys_base = NULL;
>> +	void __iomem *psys_base = NULL;
>> +	void __iomem *const *iomap;
>> +	phys_addr_t phys, pb_phys;
>> +	struct ipu7_device *isp;
>> +	u32 is_es;
>> +	int ret;
>> +
>> +	if (!fwnode || fwnode_property_read_u32(fwnode, "is_es",
>> &is_es))
>> +		is_es = 0;
>> +
>> +	isp = devm_kzalloc(dev, sizeof(*isp), GFP_KERNEL);
>> +	if (!isp)
>> +		return -ENOMEM;
>> +
>> +	dev_set_name(dev, "intel-ipu7");
>> +	isp->pdev = pdev;
>> +	INIT_LIST_HEAD(&isp->devices);
>> +
>> +	ret = pcim_enable_device(pdev);
>> +	if (ret)
>> +		return dev_err_probe(dev, ret, "Enable PCI device
>> failed\n");
>> +
>> +	dev_info(dev, "Device 0x%x (rev: 0x%x)\n",
>> +		 pdev->device, pdev->revision);
>> +
>> +	phys = pci_resource_start(pdev, IPU_PCI_BAR);
>> +	pb_phys = pci_resource_start(pdev, IPU_PCI_PBBAR);
>> +	dev_info(dev, "IPU7 PCI BAR0 base %llx BAR2 base %llx\n",
>> +		 phys, pb_phys);
>> +
>> +	ret = pcim_iomap_regions(pdev, BIT(IPU_PCI_BAR) |
>> BIT(IPU_PCI_PBBAR),
>> +				 pci_name(pdev));
> 
> Oh and btw, since I just recognized this:
> PCI request functions must always get the *driver's* name as their last
> parameter.
> 
> This string will be printed if there is a collision, i.e., when another
> driver tries to request the same resource. The output is only useful
> when the print contains the name of the party who actually stole your
> PCI region. Saying on which PCI device the region is won't be helpful.
> 
> 
> btw, did you watch my Fosdem talk before or after I answered to this
> RFC? ;)

After. ;)

> 
> 
> Thx
> P.
> 
>> +	if (ret)
>> +		return dev_err_probe(dev, ret,
>> +				     "Failed to I/O memory remapping
>> (%d)\n",
>> +				     ret);
>> +
>> +	iomap = pcim_iomap_table(pdev);
>> +	if (!iomap)
>> +		return dev_err_probe(dev, -ENODEV, "Failed to iomap
>> table\n");
>> +
> 
> 

-- 
Best regards,
Bingbu Cao