Re: [PATCH RFC 05/13] dmaengine: sdxi: Add software data structures

Jonathan Cameron
2025-09-15T11:59:40Z

On Fri, 05 Sep 2025 13:48:28 -0500
Nathan Lynch via B4 Relay <devnull+nathan.lynch.amd.com@kernel.org> wrote:

> From: Nathan Lynch <nathan.lynch@amd.com>
>
> Add the driver's central header sdxi.h, which brings in the major
> software abstractions used throughout the driver -- mainly the SDXI
> device or function (sdxi_dev) and context (sdxi_cxt).
>
> Co-developed-by: Wei Huang <wei.huang2@amd.com>
> Signed-off-by: Wei Huang <wei.huang2@amd.com>
> Signed-off-by: Nathan Lynch <nathan.lynch@amd.com>

I'm not personally a fan of 'header' patches. It's find of reasonable if it's
just stuff of the datasheet, but once we get function definitions, we should
have the function implementations in the same patch.

> ---
> drivers/dma/sdxi/sdxi.h | 206 ++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 206 insertions(+)
>
> diff --git a/drivers/dma/sdxi/sdxi.h b/drivers/dma/sdxi/sdxi.h
> new file mode 100644
> index 0000000000000000000000000000000000000000..13e02f0541e0d60412c99b0b75bd37155a531e1d
> --- /dev/null
> +++ b/drivers/dma/sdxi/sdxi.h
> @@ -0,0 +1,206 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * SDXI device driver header
> + *
> + * Copyright (C) 2025 Advanced Micro Devices, Inc.
> + */
> +
> +#ifndef __SDXI_H
> +#define __SDXI_H
> +
> +#include <linux/dev_printk.h>
> +#include <linux/device.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/dmapool.h>

Some of these could I think be removed in favor of one or two forwards
definitions. In general good to keep to minimal includes following principles of
include what you use din each file.

> +#include <linux/dmaengine.h>
> +#include <linux/io-64-nonatomic-lo-hi.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/types.h>

> +/* Device Control */

Superficially these don't seem have anything to do with controlling
the device. So this comment is confusing to me rather than helpful.

> +int sdxi_device_init(struct sdxi_dev *sdxi, const struct sdxi_dev_ops *ops);
> +void sdxi_device_exit(struct sdxi_dev *sdxi);

Bring these in with the code, not in an earlier patch.
Ideally set things up so the code is build able after each patch.