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@xxxxxxxxx wrote:
>> From: Bingbu Cao <bingbu.cao@xxxxxxxxx>
>>
>> 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
