Re: [PATCH V18 4/6] i2c: tegra: Add DMA support

[Wolfram Sang <wsa@the-dreams.de>]

[-- Attachment #1: Type: text/plain, Size: 3021 bytes --]

On Fri, Feb 08, 2019 at 10:59:40AM -0800, Sowjanya Komatineni wrote:
> This patch adds DMA support for Tegra I2C.
> 
> Tegra I2C TX and RX FIFO depth is 8 words. PIO mode is used for
> transfer size of the max FIFO depth and DMA mode is used for
> transfer size higher than max FIFO depth to save CPU overhead.
> 
> PIO mode needs full intervention of CPU to fill or empty FIFO's
> and also need to service multiple data requests interrupt for the
> same transaction. This adds delay between data bytes of the same
> transfer when CPU is fully loaded and some slave devices has
> internal timeout for no bus activity and stops transaction to
> avoid bus hang. DMA mode is helpful in such cases.
> 
> DMA mode is also helpful for Large transfers during downloading or
> uploading FW over I2C to some external devices.
> 
> Acked-by: Thierry Reding <treding@nvidia.com>
> Reviewed-by: Dmitry Osipenko <digetx@gmail.com>
> Tested-by: Dmitry Osipenko <digetx@gmail.com>
> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>

I am not sure if you are aware of this document, so I mention it:

Documentation/i2c/DMA-considerations

I am not pushing you to use the i2c_get_dma_safe_msg_buf() helpers, just
wanted to make sure you know about them. I am also fine with an
incremental patch on top of this if you want to add usage of those
helpers somewhen later.

That all being said, I'd accept the patch as is, except for:

> +static const struct tegra_i2c_hw_feature tegra186_i2c_hw = {
> +	.has_continue_xfer_support = true,
> +	.has_per_pkt_xfer_complete_irq = true,
> +	.has_single_clk_source = true,
> +	.clk_divisor_hs_mode = 1,
> +	.clk_divisor_std_fast_mode = 0x19,
> +	.clk_divisor_fast_plus_mode = 0x10,
> +	.has_config_load_reg = true,
> +	.has_multi_master_mode = true,
> +	.has_slcg_override_reg = true,
> +	.has_mst_fifo = true,
> +	.quirks = &tegra_i2c_quirks,
> +	.supports_bus_clear = true,
> +	.has_apb_dma = false,
>  };
>  
>  /* Match table for of_platform binding */
>  static const struct of_device_id tegra_i2c_of_match[] = {
>  	{ .compatible = "nvidia,tegra194-i2c", .data = &tegra194_i2c_hw, },
> +	{ .compatible = "nvidia,tegra186-i2c", .data = &tegra186_i2c_hw, },
>  	{ .compatible = "nvidia,tegra210-i2c", .data = &tegra210_i2c_hw, },
>  	{ .compatible = "nvidia,tegra124-i2c", .data = &tegra124_i2c_hw, },
>  	{ .compatible = "nvidia,tegra114-i2c", .data = &tegra114_i2c_hw, },

Shouldn't this be a seperate patch?

> -static int __init tegra_i2c_init_driver(void)
> -{
> -	return platform_driver_register(&tegra_i2c_driver);
> -}
> -
> -static void __exit tegra_i2c_exit_driver(void)
> -{
> -	platform_driver_unregister(&tegra_i2c_driver);
> -}
> -
> -subsys_initcall(tegra_i2c_init_driver);
> -module_exit(tegra_i2c_exit_driver);
> +module_platform_driver(tegra_i2c_driver);

This should definately be a seperate patch. While I am all for taking
it, are you sure it does not regress on older Tegra platforms?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]