Re: [PATCH v2 3/3] ath10k: make target endianess more explicit

[Kalle Valo <kvalo@qca.qualcomm.com>]

Michal Kazior <michal.kazior@tieto.com> writes:

> Some copy engine structures are target specific
> and are uploaded to the device during
> init/configuration.
>
> This also cleans up a bit diag_mem_read/write
> implicit byteswap mess leaving only
> diag_access_read/write with an implicit endianess
> byteswap.
>
> Signed-off-by: Michal Kazior <michal.kazior@tieto.com>

[...]

>  /* Write 4B data to Target memory or register */
>  static int ath10k_pci_diag_write_access(struct ath10k *ar, u32 address,
>  					u32 data)
>  {
>  	/* Assume range doesn't cross this boundary */
>  	if (address >= DRAM_BASE_ADDRESS)
> -		return ath10k_pci_diag_write_mem(ar, address, &data,
> -						 sizeof(u32));
> +		return ath10k_pci_diag_write32(ar, address, data);

Nothing wrong with your patch, but I really despise functions with split
personality like this one. The caller should know which area it's
writing to. And we have similar stuff in ath10k_pci_diag_read_mem() as
well:

	/*
	 * This code cannot handle reads to non-memory space. Redirect to the
	 * register read fn but preserve the multi word read capability of
	 * this fn
	 */
	if (address < DRAM_BASE_ADDRESS) {
		if (!IS_ALIGNED(address, 4) ||
		    !IS_ALIGNED((unsigned long)data, 4))
			return -EIO;

		while ((nbytes >= 4) &&  ((ret = ath10k_pci_diag_read_access(
					   ar, address, (u32 *)data)) == 0)) {
			nbytes -= sizeof(u32);
			address += sizeof(u32);
			data += sizeof(u32);
		}
		return ret;
	}


Can you guess what's the idea behind this? I would prefer that we get
rid of all the ugly _access() functions in pci.c.

-- 
Kalle Valo



_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k