Re: [PATCH v2 02/15] media: intel/ipu6: add IPU auxiliary devices

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

Andy,

Thanks for your review.

On 10/24/23 8:58 PM, Andy Shevchenko wrote:
> On Tue, Oct 24, 2023 at 07:29:11PM +0800, bingbu.cao@intel.com wrote:
>> From: Bingbu Cao <bingbu.cao@intel.com>
>>
>> Even the IPU input system and processing system are in a single PCI
>> device, each system has its own power sequence, the processing system
>> power up depends on the input system power up.
>>
>> Besides, input system and processing system have their own MMU
>> hardware for IPU DMA address mapping.
>>
>> Register the IS/PS devices on auxiliary bus and attach power domain
>> to implement the power sequence dependency.
> 
> ...
> 
> Seems again poor / random list of header inclusions...
> Please, go through your files and use IWYU (Include What You Use) principle
> in them.
> 
>> +#include <linux/pci.h>
> 
> auxiliary_bus.h, err.h, list.h, mutex.h, dev_printk.h, slab.h, dma-mapping.h are missing.
> You are lucky it does compile.

auxiliary_bus.h is included in ipu6-bus.h, list.h, mutex.h dev_printk.h are
included in device.h, dma-mapping.h and scatterlist.h are included in pci.h.

I am a little confused about the rule, do you mean we need include the
generic headers we need even it is included in others header?

> 
> ...
> 
>> +#ifndef IPU6_BUS_H
>> +#define IPU6_BUS_H
>> +
>> +#include <linux/auxiliary_bus.h>
> 
> ...Especially for headers which will affect the compilation time.
> 
>> +#include <linux/pci.h>
> 
> This is not used.

Do you mean it just need a 'struct pci_dev;' ?
> 
> ...
> 
>> +	struct list_head list;
> 
> + types.h
> 
>> +	struct sg_table fw_sgt;
> 
> + scatterlist.h
> 
>> +	dma_addr_t pkg_dir_dma_addr;
> 
> types.h
> 
> ...
> 
>> +struct ipu6_auxdrv_data {
>> +	irqreturn_t (*isr)(struct ipu6_bus_device *adev);
>> +	irqreturn_t (*isr_threaded)(struct ipu6_bus_device *adev);
> 
> irqreturn.h
> 
>> +	bool wake_isr_thread;
>> +};
> 
> ...
> 
>> +#define to_ipu6_bus_device(_dev) container_of(to_auxiliary_dev(_dev), \
>> +					      struct ipu6_bus_device, auxdev)
>> +#define auxdev_to_adev(_auxdev) container_of(_auxdev, \
>> +					     struct ipu6_bus_device, auxdev)
> 
> container_of.h
> 
> Also, can you reformat to be more readable like
> 
> #define auxdev_to_adev(_auxdev)					\
> 	container_of(_auxdev, struct ipu6_bus_device, auxdev)

Ack.

> 
>> +#define ipu6_bus_get_drvdata(adev) dev_get_drvdata(&(adev)->auxdev.dev)
> 
> device.h
> 
> ...
> 
>> +	ret = dma_set_max_seg_size(&pdev->dev, UINT_MAX);
>> +	if (ret)
>> +		return dev_err_probe(&pdev->dev, ret,
>> +				     "Failed to set max_seg_size\n");
> 
> With the help of
> 
> 	struct device *dev = &pdev->dev;
> 
> this and other lines become neater
> 
> 
> 	ret = dma_set_max_seg_size(dev, _DMA_SEGMENT_SIZE); // as I commented earlier about
> 	if (ret)
> 		return dev_err_probe(dev, ret, "Failed to set max_seg_size\n");

Ack.

> 

-- 
Best regards,
Bingbu Cao