| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAPVz0n2BpSeZkoT1YV9q5bkOCkjSvOwAXNVGgM4wPUqV3jyxgg@mail.gmail.com>
Date: Fri, 19 Sep 2025 10:58:04 +0300
From: Svyatoslav Ryhel <clamor95@...il.com>
To: Mikko Perttunen <mperttunen@...dia.com>
Cc: Thierry Reding <thierry.reding@...il.com>, Thierry Reding <treding@...dia.com>,
Jonathan Hunter <jonathanh@...dia.com>, Sowjanya Komatineni <skomatineni@...dia.com>,
Luca Ceresoli <luca.ceresoli@...tlin.com>, David Airlie <airlied@...il.com>,
Simona Vetter <simona@...ll.ch>, Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
Maxime Ripard <mripard@...nel.org>, Thomas Zimmermann <tzimmermann@...e.de>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>,
Prashant Gaikwad <pgaikwad@...dia.com>, Michael Turquette <mturquette@...libre.com>,
Stephen Boyd <sboyd@...nel.org>, Mauro Carvalho Chehab <mchehab@...nel.org>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>, Dmitry Osipenko <digetx@...il.com>,
Jonas Schwöbel <jonasschwoebel@...oo.de>,
Charan Pedumuru <charan.pedumuru@...il.com>, dri-devel@...ts.freedesktop.org,
devicetree@...r.kernel.org, linux-tegra@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-media@...r.kernel.org,
linux-clk@...r.kernel.org, linux-staging@...ts.linux.dev
Subject: Re: [PATCH v2 09/23] gpu: host1x: convert MIPI to use operations
пт, 19 вер. 2025 р. о 09:47 Mikko Perttunen <mperttunen@...dia.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@...il.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_ */
> >
>
All this is good, but how to include into this CSI? Adding support for
CSI is why I am even touching this at the first place.
Powered by blists - more mailing lists