On Tue, Oct 08, 2024 at 02:19:15PM +0800, bingbu.cao@xxxxxxxxx wrote:
> From: Bingbu Cao <bingbu.cao@xxxxxxxxx>
>
> This patch fixes the build errors with `i386-allmodconfig`, the
> errors are caused by wrong type cast and 64-bit division.
Thanks for the change, my comments below.
...
> /* Shared structure between driver and FW - do not modify */
> struct ipu6_fw_sys_queue {
> - u64 host_address;
> + uintptr_t host_address;
Okay, in the given semantic this probably should be phys_addr_t.
BUT, is this address somehow is going to be used by IPU6 hardware?
If "yes", the type shall not be changed.
Looking at types used I hope the answer is "no", otherwise the types
in the structures should be properly choose WRT endianess (and what
__packed is doing here? Is it part of the protocol?).
> u32 vied_address;
> u32 size;
> u32 token_size;
> @@ -40,7 +40,7 @@ struct ipu6_fw_sys_queue {
> } __packed;
>
> struct ipu6_fw_sys_queue_res {
> - u64 host_address;
> + uintptr_t host_address;
Ditto.
> u32 vied_address;
> u32 reg;
> } __packed;
...
> - dev_dbg(dev, "write: reg 0x%lx = data 0x%x", base + addr - isys_base,
> - data);
> + dev_dbg(dev, "write: reg 0x%lx = data 0x%x",
> + (ulong)(base + addr - isys_base), data);
No, one should use proper specifiers for this. And what the heck 'ulong' is?
Where is it being defined?
...
> - dev_dbg(dev, "read: reg 0x%lx = data 0x%x", base + addr - isys_base,
> - data);
> + dev_dbg(dev, "read: reg 0x%lx = data 0x%x",
> + (ulong)(base + addr - isys_base), data);
Ditto.
...
> pg_offset = server_fw_addr - dma_addr;
> - prog = (struct ipu6_cell_program *)((u64)isp->cpd_fw->data + pg_offset);
> + prog = (struct ipu6_cell_program *)((uintptr_t)isp->cpd_fw->data +
> + pg_offset);
Side Q: What are the alignment requirements for the prog pointer?
--
With Best Regards,
Andy Shevchenko
