Re: [PATCH v2 01/15] media: intel/ipu6: add Intel IPU6 PCI device driver

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

Hans,

On 11/5/23 10:43 PM, Hans de Goede wrote:
> Hi,
> 
> 
> On 10/24/23 13:29, bingbu.cao@intel.com wrote:
>> From: Bingbu Cao <bingbu.cao@intel.com>
>>
>> Intel Image Processing Unit 6th Gen includes input and processing systems
>> but the hardware presents itself as a single PCI device in system.
>>
>> IPU6 PCI device driver basically does PCI configurations and load
>> the firmware binary, initialises IPU virtual bus, and sets up platform
>> specific variants to support multiple IPU6 devices in single device
>> driver.
>>
>> Signed-off-by: Bingbu Cao <bingbu.cao@intel.com>
>> Reported-by: Hans de Goede <hdegoede@redhat.com>
>> Suggested-by: Andreas Helbech Kleist <andreaskleist@gmail.com>
> 
> Thank you. Just one small remark, not a full review:
> 
> <snip>
> 
>> diff --git a/drivers/media/pci/intel/ipu6/ipu6.c b/drivers/media/pci/intel/ipu6/ipu6.c
>> new file mode 100644
>> index 000000000000..84960a283daf
>> --- /dev/null
>> +++ b/drivers/media/pci/intel/ipu6/ipu6.c
> 
> <snip>
> 
>> +static struct ipu6_bus_device *
>> +ipu6_isys_init(struct pci_dev *pdev, struct device *parent,
>> +	       struct ipu6_buttress_ctrl *ctrl, void __iomem *base,
>> +	       const struct ipu6_isys_internal_pdata *ipdata)
>> +{
>> +	struct fwnode_handle *fwnode = dev_fwnode(&pdev->dev);
>> +	struct ipu6_bus_device *isys_adev;
>> +	struct ipu6_isys_pdata *pdata;
>> +	int ret;
>> +
>> +	/* check fwnode at first, fallback into bridge if no fwnode graph */
>> +	ret = ipu6_isys_check_fwnode_graph(fwnode);
>> +	if (ret) {
>> +		if (fwnode && !IS_ERR_OR_NULL(fwnode->secondary)) {
>> +			dev_err(&pdev->dev,
>> +				"fwnode graph has no endpoints connection\n");
>> +			return ERR_PTR(-EINVAL);
>> +		}
>> +
>> +		ret = ipu_bridge_init(&pdev->dev, ipu_bridge_parse_ssdb);
>> +		if (ret) {
>> +			if (ret != -EPROBE_DEFER)
>> +				dev_err_probe(&pdev->dev, ret,
>> +					      "IPU6 bridge init failed\n");
> 
> dev_err_probe() already silences logging of -EPROBE_DEFER errors itself,
> so this "if (ret != -EPROBE_DEFER)" guard here is not necessary.

Good catch, thanks.

> 
> Regards,
> 
> Hans
> 
> 
> 
> 

-- 
Best regards,
Bingbu Cao