Hi,
On 10/25/23 09:14, Bingbu Cao wrote:
> 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?
Yes you must *not* rely on other headers including headers from
which you directly use symbols.
People are working on reducing the number of includes inside headers
to speed up building the kernel because including unnecessary headers
inside another header has a big impact on build time.
Regards,
Hans
>
>>
>> ...
>>
>>> +#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.
>
>>
>
