Re: [PATCH v2 09/23] gpu: host1x: convert MIPI to use operations

[Mikko Perttunen <mperttunen@nvidia.com>]

On Saturday, September 6, 2025 10:53 PM Svyatoslav Ryhel wrote:
> This commit converts the existing MIPI code to use operations, which is a
> necessary step for the Tegra20/Tegra30 SoCs. Additionally, it creates a
> dedicated header file, tegra-mipi-cal.h, to contain the MIPI calibration
> functions, improving code organization and readability.

I'd write out "operation function pointers", at least the first time. Just "operations" isn't clear to me.

Please write the commit message in imperative mood (like you've done in other patches).

> 
> Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> ---
>  drivers/gpu/drm/tegra/dsi.c             |   1 +
>  drivers/gpu/host1x/mipi.c               |  40 +++------
>  drivers/staging/media/tegra-video/csi.c |   1 +
>  include/linux/host1x.h                  |  10 ---
>  include/linux/tegra-mipi-cal.h          | 111 ++++++++++++++++++++++++
>  5 files changed, 126 insertions(+), 37 deletions(-)
>  create mode 100644 include/linux/tegra-mipi-cal.h
> 
> diff --git a/drivers/gpu/drm/tegra/dsi.c b/drivers/gpu/drm/tegra/dsi.c
> index 64f12a85a9dd..278bf2c85524 100644
> --- a/drivers/gpu/drm/tegra/dsi.c
> +++ b/drivers/gpu/drm/tegra/dsi.c
> @@ -14,6 +14,7 @@
>  #include <linux/pm_runtime.h>
>  #include <linux/regulator/consumer.h>
>  #include <linux/reset.h>
> +#include <linux/tegra-mipi-cal.h>
>  
>  #include <video/mipi_display.h>
>  
> diff --git a/drivers/gpu/host1x/mipi.c b/drivers/gpu/host1x/mipi.c
> index e51b43dd15a3..2fa339a428f3 100644
> --- a/drivers/gpu/host1x/mipi.c
> +++ b/drivers/gpu/host1x/mipi.c
> @@ -27,6 +27,7 @@
>  #include <linux/of_platform.h>
>  #include <linux/platform_device.h>
>  #include <linux/slab.h>
> +#include <linux/tegra-mipi-cal.h>
>  
>  #include "dev.h"
>  
> @@ -116,23 +117,6 @@ struct tegra_mipi_soc {
>  	u8 hsclkpuos;
>  };
>  
> -struct tegra_mipi {
> -	const struct tegra_mipi_soc *soc;
> -	struct device *dev;
> -	void __iomem *regs;
> -	struct mutex lock;
> -	struct clk *clk;
> -
> -	unsigned long usage_count;
> -};
> -
> -struct tegra_mipi_device {
> -	struct platform_device *pdev;
> -	struct tegra_mipi *mipi;
> -	struct device *device;
> -	unsigned long pads;
> -};
> -
>  static inline u32 tegra_mipi_readl(struct tegra_mipi *mipi,
>  				   unsigned long offset)
>  {
> @@ -261,7 +245,7 @@ void tegra_mipi_free(struct tegra_mipi_device *device)
>  }
>  EXPORT_SYMBOL(tegra_mipi_free);
>  
> -int tegra_mipi_enable(struct tegra_mipi_device *dev)
> +static int tegra114_mipi_enable(struct tegra_mipi_device *dev)
>  {
>  	int err = 0;
>  
> @@ -273,11 +257,9 @@ int tegra_mipi_enable(struct tegra_mipi_device *dev)
>  	mutex_unlock(&dev->mipi->lock);
>  
>  	return err;
> -
>  }
> -EXPORT_SYMBOL(tegra_mipi_enable);
>  
> -int tegra_mipi_disable(struct tegra_mipi_device *dev)
> +static int tegra114_mipi_disable(struct tegra_mipi_device *dev)
>  {
>  	int err = 0;
>  
> @@ -289,11 +271,9 @@ int tegra_mipi_disable(struct tegra_mipi_device *dev)
>  	mutex_unlock(&dev->mipi->lock);
>  
>  	return err;
> -
>  }
> -EXPORT_SYMBOL(tegra_mipi_disable);
>  
> -int tegra_mipi_finish_calibration(struct tegra_mipi_device *device)
> +static int tegra114_mipi_finish_calibration(struct tegra_mipi_device *device)
>  {
>  	struct tegra_mipi *mipi = device->mipi;
>  	void __iomem *status_reg = mipi->regs + (MIPI_CAL_STATUS << 2);
> @@ -309,9 +289,8 @@ int tegra_mipi_finish_calibration(struct tegra_mipi_device *device)
>  
>  	return err;
>  }
> -EXPORT_SYMBOL(tegra_mipi_finish_calibration);
>  
> -int tegra_mipi_start_calibration(struct tegra_mipi_device *device)
> +static int tegra114_mipi_start_calibration(struct tegra_mipi_device *device)
>  {
>  	const struct tegra_mipi_soc *soc = device->mipi->soc;
>  	unsigned int i;
> @@ -384,7 +363,13 @@ int tegra_mipi_start_calibration(struct tegra_mipi_device *device)
>  
>  	return 0;
>  }
> -EXPORT_SYMBOL(tegra_mipi_start_calibration);
> +
> +static const struct tegra_mipi_ops tegra114_mipi_ops = {
> +	.tegra_mipi_enable = tegra114_mipi_enable,
> +	.tegra_mipi_disable = tegra114_mipi_disable,
> +	.tegra_mipi_start_calibration = tegra114_mipi_start_calibration,
> +	.tegra_mipi_finish_calibration = tegra114_mipi_finish_calibration,
> +};
>  
>  static const struct tegra_mipi_pad tegra114_mipi_pads[] = {
>  	{ .data = MIPI_CAL_CONFIG_CSIA },
> @@ -512,6 +497,7 @@ static int tegra_mipi_probe(struct platform_device *pdev)
>  
>  	mipi->soc = match->data;
>  	mipi->dev = &pdev->dev;
> +	mipi->ops = &tegra114_mipi_ops;
>  
>  	mipi->regs = devm_platform_get_and_ioremap_resource(pdev, 0, NULL);
>  	if (IS_ERR(mipi->regs))
> diff --git a/drivers/staging/media/tegra-video/csi.c b/drivers/staging/media/tegra-video/csi.c
> index 74c92db1032f..9e3bd6109781 100644
> --- a/drivers/staging/media/tegra-video/csi.c
> +++ b/drivers/staging/media/tegra-video/csi.c
> @@ -12,6 +12,7 @@
>  #include <linux/of_graph.h>
>  #include <linux/platform_device.h>
>  #include <linux/pm_runtime.h>
> +#include <linux/tegra-mipi-cal.h>
>  
>  #include <media/v4l2-fwnode.h>
>  
> diff --git a/include/linux/host1x.h b/include/linux/host1x.h
> index 9fa9c30a34e6..b1c6514859d3 100644
> --- a/include/linux/host1x.h
> +++ b/include/linux/host1x.h
> @@ -453,16 +453,6 @@ void host1x_client_unregister(struct host1x_client *client);
>  int host1x_client_suspend(struct host1x_client *client);
>  int host1x_client_resume(struct host1x_client *client);
>  
> -struct tegra_mipi_device;
> -
> -struct tegra_mipi_device *tegra_mipi_request(struct device *device,
> -					     struct device_node *np);
> -void tegra_mipi_free(struct tegra_mipi_device *device);
> -int tegra_mipi_enable(struct tegra_mipi_device *device);
> -int tegra_mipi_disable(struct tegra_mipi_device *device);
> -int tegra_mipi_start_calibration(struct tegra_mipi_device *device);
> -int tegra_mipi_finish_calibration(struct tegra_mipi_device *device);
> -
>  /* host1x memory contexts */
>  
>  struct host1x_memory_context {
> diff --git a/include/linux/tegra-mipi-cal.h b/include/linux/tegra-mipi-cal.h
> new file mode 100644
> index 000000000000..2bfdbfd3cb77
> --- /dev/null
> +++ b/include/linux/tegra-mipi-cal.h
> @@ -0,0 +1,111 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +#ifndef __TEGRA_MIPI_CAL_H_
> +#define __TEGRA_MIPI_CAL_H_
> +
> +struct tegra_mipi {
> +	const struct tegra_mipi_soc *soc;
> +	const struct tegra_mipi_ops *ops;
> +	struct device *dev;
> +	void __iomem *regs;
> +	struct mutex lock;
> +	struct clk *clk;
> +
> +	unsigned long usage_count;
> +};
> +
> +struct tegra_mipi_device {
> +	struct platform_device *pdev;
> +	struct tegra_mipi *mipi;
> +	struct device *device;
> +	unsigned long pads;
> +};

We should avoid putting implementation details / chip-specific things in the public header. Here's a sketch of what I'm thinking about:

--- tegra-mipi-cal.h:

struct tegra_mipi_device;

struct tegra_mipi_ops {
	// ...
};

int tegra_mipi_add_provider(struct device_node *np, struct tegra_mipi_ops *ops);

int tegra_mipi_enable(...);
// ...

--- host1x/mipi.c:

// move tegra114-mipi specific stuff to a new file, e.g. host1x/tegra114-mipi.c

struct tegra_mipi_device {
	struct tegra_mipi_ops *ops;
	struct platform_device *pdev;
};

/* only need to support one provider */
static struct {
	struct device_node *np;
	struct tegra_mipi_ops *ops;
} provider;

int tegra_mipi_add_provider(struct device_node *np, struct tegra_mipi_ops *ops)
{
	if (provider.np)
		return -EBUSY;

	provider.np = np;
	provider.ops = ops;

	return 0;
}

struct tegra_mipi_device *tegra_mipi_request(struct *device, struct device_node *np)
{
	struct device_node *phandle_np = /* ... */;
	struct platform_device *pdev;
	struct tegra_mipi_device *mipidev;

	if (provider.np != phandle_np)
		return -ENODEV;

	pdev = /* ... */;

	mipidev = kzalloc(...);
	mipidev->ops = provider.ops;
	mipidev->pdev = pdev;
	mipidev->cells = phandle_cells;

	return mipidev;
}

int tegra_mipi_enable(struct tegra_mipi_device *device)
{
	return device->ops->enable(platform_get_drvdata(device->pdev), device->cells);
}

> +
> +/**
> + * Operations for Tegra MIPI calibration device
> + */
> +struct tegra_mipi_ops {
> +	/**
> +	 * @tegra_mipi_enable:
> +	 *
> +	 * Enable MIPI calibration device
> +	 */
> +	int (*tegra_mipi_enable)(struct tegra_mipi_device *device);

The tegra_mipi_ prefix should be dropped for the field names.

> +
> +	/**
> +	 * @tegra_mipi_disable:
> +	 *
> +	 * Disable MIPI calibration device
> +	 */
> +	int (*tegra_mipi_disable)(struct tegra_mipi_device *device);
> +
> +	/**
> +	 * @tegra_mipi_start_calibration:
> +	 *
> +	 * Start MIPI calibration
> +	 */
> +	int (*tegra_mipi_start_calibration)(struct tegra_mipi_device *device);
> +
> +	/**
> +	 * @tegra_mipi_finish_calibration:
> +	 *
> +	 * Finish MIPI calibration
> +	 */
> +	int (*tegra_mipi_finish_calibration)(struct tegra_mipi_device *device);
> +};
> +
> +struct tegra_mipi_device *tegra_mipi_request(struct device *device,
> +					     struct device_node *np);
> +
> +void tegra_mipi_free(struct tegra_mipi_device *device);
> +
> +static inline int tegra_mipi_enable(struct tegra_mipi_device *device)
> +{
> +	/* Tegra114+ has a dedicated MIPI calibration block */
> +	if (device->mipi) {
> +		if (!device->mipi->ops->tegra_mipi_enable)
> +			return 0;
> +
> +		return device->mipi->ops->tegra_mipi_enable(device);
> +	}
> +
> +	return -ENOSYS;
> +}
> +
> +static inline int tegra_mipi_disable(struct tegra_mipi_device *device)
> +{
> +	if (device->mipi) {
> +		if (!device->mipi->ops->tegra_mipi_disable)
> +			return 0;
> +
> +		return device->mipi->ops->tegra_mipi_disable(device);
> +	}
> +
> +	return -ENOSYS;
> +}
> +
> +static inline int tegra_mipi_start_calibration(struct tegra_mipi_device *device)
> +{
> +	if (device->mipi) {
> +		if (!device->mipi->ops->tegra_mipi_start_calibration)
> +			return 0;
> +
> +		return device->mipi->ops->tegra_mipi_start_calibration(device);
> +	}
> +
> +	return -ENOSYS;
> +}
> +
> +static inline int tegra_mipi_finish_calibration(struct tegra_mipi_device *device)
> +{
> +	if (device->mipi) {
> +		if (!device->mipi->ops->tegra_mipi_finish_calibration)
> +			return 0;
> +
> +		return device->mipi->ops->tegra_mipi_finish_calibration(device);
> +	}
> +
> +	return -ENOSYS;
> +}
> +
> +#endif /* __TEGRA_MIPI_CAL_H_ */
>