On Sunday, February 02, 2014 at 02:52:52 PM, Mark Brown wrote:
Hi, thanks for preparing this patch!
I have just a few very minor nitpicks, ignore if you please.
[...]
> +static int spi_map_buf(struct spi_master *master, struct device *dev,
> + struct sg_table *sgt, void *buf, size_t len,
> + enum dma_data_direction dir)
> +{
> + const bool vmalloced_buf = is_vmalloc_addr(buf);
> + const int desc_len = vmalloced_buf ? PAGE_SIZE : master->max_dma_len;
You might want to rename this to "sg_chunk_max_size" or something, "desc_len"
doesn't make much sense here. The variable describes the maximum size of one
single scatterlist element.
> + const int sgs = DIV_ROUND_UP(len, desc_len);
Looking at this, the variables could generally use a more meaningful name. I
think it'd be clearer to call this "num_sg_chunks" or so ?
> + struct page *vm_page;
> + void *sg_buf;
> + size_t min;
> + int i, ret;
> +
> + ret = sg_alloc_table(sgt, sgs, GFP_KERNEL);
> + if (ret != 0)
> + return ret;
> +
> + for (i = 0; i < sgs; i++) {
> + min = min_t(size_t, len, desc_len);
> +
> + if (vmalloced_buf) {
> + vm_page = vmalloc_to_page(buf);
Just curious, but shouldn't we check if buf != NULL right at the begining of
this function?
[...]
> +static void spi_unmap_buf(struct spi_master *master, struct device *dev,
> + struct sg_table *sgt, enum dma_data_direction dir)
> +{
> + if (sgt->orig_nents) {
I don't want to nag, but why not use if (!sgt->...) return; ? This would cut
down one level of indent.
> + dma_unmap_sg(dev, sgt->sgl, sgt->orig_nents, dir);
> + sg_free_table(sgt);
> + }
> +}
> +
[...]
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
