[v6,01/12] spi: add driver for intel graphics on-die spi device

Commit Message

Alexander Usyskin Sept. 16, 2024, 1:49 p.m. UTC

Add auxiliary driver for intel discrete graphics
on-die spi device.

CC: Rodrigo Vivi <[e-mail]>
CC: Lucas De Marchi <[e-mail]>
Signed-off-by: Tomas Winkler <[e-mail]>
Signed-off-by: Alexander Usyskin <[e-mail]>
---
 MAINTAINERS                      |   7 ++
 drivers/spi/Kconfig              |  11 +++
 drivers/spi/Makefile             |   1 +
 drivers/spi/spi-intel-dg.c       | 142 +++++++++++++++++++++++++++++++
 include/linux/intel_dg_spi_aux.h |  27 ++++++
 5 files changed, 188 insertions(+)
 create mode 100644 drivers/spi/spi-intel-dg.c
 create mode 100644 include/linux/intel_dg_spi_aux.h

Comments

Mark Brown Sept. 18, 2024, 1:33 p.m. UTC | #1

On Mon, Sep 16, 2024 at 04:49:17PM +0300, Alexander Usyskin wrote:

> Add auxiliary driver for intel discrete graphics
> on-die spi device.

This doesn't actually do anything AFAICT?  It doesn't register with any
subsystem or anything.  Please don't submit empty boilerplate like this,
submit a driver that is at least minimally useful - assuming some other
patches in the series add functionality squash at least a basic set of
functionality into this.  This makes review and test easier.

> +++ b/drivers/spi/spi-intel-dg.c
> @@ -0,0 +1,142 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright(c) 2019-2024, Intel Corporation. All rights reserved.
> + */

Please make the entire comment a C++ one so things look more
intentional.

> +struct intel_dg_spi {
> +	struct kref refcnt;
> +	void __iomem *base;
> +	size_t size;
> +	unsigned int nregions;
> +	struct {
> +		const char *name;
> +		u8 id;
> +		u64 offset;
> +		u64 size;
> +	} regions[];

Use __counted_by() for the benefit of the static checkers.

> +	size = sizeof(*spi) + sizeof(spi->regions[0]) * nregions;
> +	spi = kzalloc(size, GFP_KERNEL);

Use at least array_size().

> +	kref_init(&spi->refcnt);

This refcount feels more complex than just freeing in the error and
release paths, it's not a common pattern for drivers.

> +	spi->nregions = nregions;
> +	for (n = 0, i = 0; i < INTEL_DG_SPI_REGIONS; i++) {
> +		if (ispi->regions[i].name) {
> +			name_size = strlen(dev_name(&aux_dev->dev)) +
> +				    strlen(ispi->regions[i].name) + 2; /* for point */
> +			name = kzalloc(name_size, GFP_KERNEL);
> +			if (!name)
> +				continue;
> +			snprintf(name, name_size, "%s.%s",
> +				 dev_name(&aux_dev->dev), ispi->regions[i].name);

kasprintf().

> +static void intel_dg_spi_remove(struct auxiliary_device *aux_dev)
> +{
> +	struct intel_dg_spi *spi = dev_get_drvdata(&aux_dev->dev);
> +
> +	if (!spi)
> +		return;
> +
> +	dev_set_drvdata(&aux_dev->dev, NULL);

If the above is doing anything there's a problem...

Winkler, Tomas Sept. 19, 2024, 9:54 a.m. UTC | #2

> 
> On Mon, Sep 16, 2024 at 04:49:17PM +0300, Alexander Usyskin wrote:
> 
> > Add auxiliary driver for intel discrete graphics on-die spi device.
> 
> This doesn't actually do anything AFAICT?  It doesn't register with any
> subsystem or anything.  Please don't submit empty boilerplate like this,
> submit a driver that is at least minimally useful - assuming some other
> patches in the series add functionality squash at least a basic set of
> functionality into this.  This makes review and test easier.
> 
> > +++ b/drivers/spi/spi-intel-dg.c
> > @@ -0,0 +1,142 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright(c) 2019-2024, Intel Corporation. All rights reserved.
> > + */
> 
> Please make the entire comment a C++ one so things look more intentional.

This is how it is required by Linux spdx checker, 

> 
> > +struct intel_dg_spi {
> > +	struct kref refcnt;
> > +	void __iomem *base;
> > +	size_t size;
> > +	unsigned int nregions;
> > +	struct {
> > +		const char *name;
> > +		u8 id;
> > +		u64 offset;
> > +		u64 size;
> > +	} regions[];
> 
> Use __counted_by() for the benefit of the static checkers.


Good point, it's a new C feature. 

> 
> > +	size = sizeof(*spi) + sizeof(spi->regions[0]) * nregions;
> > +	spi = kzalloc(size, GFP_KERNEL);
> 
> Use at least array_size().

Regions is not fixed size array, it will not work.

> 
> > +	kref_init(&spi->refcnt);
> 
> This refcount feels more complex than just freeing in the error and release
> paths, it's not a common pattern for drivers.


What are you suggesting


> 
> > +	spi->nregions = nregions;
> > +	for (n = 0, i = 0; i < INTEL_DG_SPI_REGIONS; i++) {
> > +		if (ispi->regions[i].name) {
> > +			name_size = strlen(dev_name(&aux_dev->dev)) +
> > +				    strlen(ispi->regions[i].name) + 2; /* for
> point */
> > +			name = kzalloc(name_size, GFP_KERNEL);
> > +			if (!name)
> > +				continue;
> > +			snprintf(name, name_size, "%s.%s",
> > +				 dev_name(&aux_dev->dev), ispi-
> >regions[i].name);
> 
> kasprintf().

As I understand kasprintf allocates memory, this is not required here.


> 
> > +static void intel_dg_spi_remove(struct auxiliary_device *aux_dev) {
> > +	struct intel_dg_spi *spi = dev_get_drvdata(&aux_dev->dev);
> > +
> > +	if (!spi)
> > +		return;
> > +
> > +	dev_set_drvdata(&aux_dev->dev, NULL);
> 
> If the above is doing anything there's a problem...

It makes sure the data is set to NULL.

Mark Brown Sept. 19, 2024, 10:32 a.m. UTC | #3

On Thu, Sep 19, 2024 at 09:54:24AM +0000, Winkler, Tomas wrote:
> > On Mon, Sep 16, 2024 at 04:49:17PM +0300, Alexander Usyskin wrote:

> > > @@ -0,0 +1,142 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * Copyright(c) 2019-2024, Intel Corporation. All rights reserved.
> > > + */

> > Please make the entire comment a C++ one so things look more intentional.

> This is how it is required by Linux spdx checker, 

There is no incompatibility between SPDX and what I'm asking for...

> > > +	size = sizeof(*spi) + sizeof(spi->regions[0]) * nregions;
> > > +	spi = kzalloc(size, GFP_KERNEL);

> > Use at least array_size().

> Regions is not fixed size array, it will not work.

Yes, that's the wrong helper - there is a relevent one though which I'm
not remembering right now.

> > > +	kref_init(&spi->refcnt);

> > This refcount feels more complex than just freeing in the error and release
> > paths, it's not a common pattern for drivers.

> What are you suggesting

Just do normal open coded allocations, the reference counting is just
obscure.

> > > +		if (ispi->regions[i].name) {
> > > +			name_size = strlen(dev_name(&aux_dev->dev)) +
> > > +				    strlen(ispi->regions[i].name) + 2; /* for
> > point */
> > > +			name = kzalloc(name_size, GFP_KERNEL);
> > > +			if (!name)
> > > +				continue;
> > > +			snprintf(name, name_size, "%s.%s",
> > > +				 dev_name(&aux_dev->dev), ispi-
> > >regions[i].name);

> > kasprintf().

> As I understand kasprintf allocates memory, this is not required here.

There is a kzalloc() in the above code block?

> > > +static void intel_dg_spi_remove(struct auxiliary_device *aux_dev) {
> > > +	struct intel_dg_spi *spi = dev_get_drvdata(&aux_dev->dev);

> > > +	if (!spi)
> > > +		return;

> > > +	dev_set_drvdata(&aux_dev->dev, NULL);

> > If the above is doing anything there's a problem...
o
> It makes sure the data is set to NULL.

Which is needed because...?

Winkler, Tomas Sept. 21, 2024, 1 p.m. UTC | #4

> 
> On Thu, Sep 19, 2024 at 09:54:24AM +0000, Winkler, Tomas wrote:
> > > On Mon, Sep 16, 2024 at 04:49:17PM +0300, Alexander Usyskin wrote:
> 
> > > > @@ -0,0 +1,142 @@
> > > > +// SPDX-License-Identifier: GPL-2.0
> > > > +/*
> > > > + * Copyright(c) 2019-2024, Intel Corporation. All rights reserved.
> > > > + */
> 
> > > Please make the entire comment a C++ one so things look more
> intentional.
> 
> > This is how it is required by Linux spdx checker,
> 
> There is no incompatibility between SPDX and what I'm asking for...
> 
> > > > +	size = sizeof(*spi) + sizeof(spi->regions[0]) * nregions;
> > > > +	spi = kzalloc(size, GFP_KERNEL);
> 
> > > Use at least array_size().
> 
> > Regions is not fixed size array, it will not work.
> 
> Yes, that's the wrong helper - there is a relevent one though which I'm not
> remembering right now.


I don't think there is one, you can allocate arrays but this is not the case here. 

> 
> > > > +	kref_init(&spi->refcnt);
> 
> > > This refcount feels more complex than just freeing in the error and
> > > release paths, it's not a common pattern for drivers.
> 
> > What are you suggesting
> 
> Just do normal open coded allocations, the reference counting is just
> obscure.

The kref here is for reason so we don't need to hunt the close open, I frankly don't understand
what is wrong with it, 



> > > > +		if (ispi->regions[i].name) {
> > > > +			name_size = strlen(dev_name(&aux_dev->dev)) +
> > > > +				    strlen(ispi->regions[i].name) + 2; /* for
> > > point */
> > > > +			name = kzalloc(name_size, GFP_KERNEL);
> > > > +			if (!name)
> > > > +				continue;
> > > > +			snprintf(name, name_size, "%s.%s",
> > > > +				 dev_name(&aux_dev->dev), ispi-
> > > >regions[i].name);
> 
> > > kasprintf().
> 
> > As I understand kasprintf allocates memory, this is not required here.
> 
> There is a kzalloc() in the above code block?
Sorry  you are right. 

> 
> > > > +static void intel_dg_spi_remove(struct auxiliary_device *aux_dev) {
> > > > +	struct intel_dg_spi *spi = dev_get_drvdata(&aux_dev->dev);
> 
> > > > +	if (!spi)
> > > > +		return;
> 
> > > > +	dev_set_drvdata(&aux_dev->dev, NULL);
> 
> > > If the above is doing anything there's a problem...
> o
> > It makes sure the data is set to NULL.
> 
> Which is needed because...?

This is a boilerplate part, the content is consequent patches.

Tvrtko Ursulin Sept. 23, 2024, 8:02 a.m. UTC | #5

On 21/09/2024 14:00, Winkler, Tomas wrote:
> 
> 
>>
>> On Thu, Sep 19, 2024 at 09:54:24AM +0000, Winkler, Tomas wrote:
>>>> On Mon, Sep 16, 2024 at 04:49:17PM +0300, Alexander Usyskin wrote:
>>
>>>>> @@ -0,0 +1,142 @@
>>>>> +// SPDX-License-Identifier: GPL-2.0
>>>>> +/*
>>>>> + * Copyright(c) 2019-2024, Intel Corporation. All rights reserved.
>>>>> + */
>>
>>>> Please make the entire comment a C++ one so things look more
>> intentional.
>>
>>> This is how it is required by Linux spdx checker,
>>
>> There is no incompatibility between SPDX and what I'm asking for...
>>
>>>>> +	size = sizeof(*spi) + sizeof(spi->regions[0]) * nregions;
>>>>> +	spi = kzalloc(size, GFP_KERNEL);
>>
>>>> Use at least array_size().
>>
>>> Regions is not fixed size array, it will not work.
>>
>> Yes, that's the wrong helper - there is a relevent one though which I'm not
>> remembering right now.
> 
> 
> I don't think there is one, you can allocate arrays but this is not the case here.

struct_size() probably.

Regards,

Tvrtko

Mark Brown Sept. 23, 2024, 12:49 p.m. UTC | #6

On Sat, Sep 21, 2024 at 01:00:52PM +0000, Winkler, Tomas wrote:
> > On Thu, Sep 19, 2024 at 09:54:24AM +0000, Winkler, Tomas wrote:
> > > > On Mon, Sep 16, 2024 at 04:49:17PM +0300, Alexander Usyskin wrote:

> > Just do normal open coded allocations, the reference counting is just
> > obscure.

> The kref here is for reason so we don't need to hunt the close open, I frankly don't understand
> what is wrong with it, 

It's locking/refcounting stuff that looks nothing like any other SPI
controller driver.  Even if it works it's obviously fragile since the
driver does surprising things which break assumptions that will be made
by people not looking at this specific driver, and causes people to have
to spend more effort figuring out what you're doing.  If there is any
benefit to doing this then open coding it in one specific driver is
clearly not the right place to do it.

> > > > > +static void intel_dg_spi_remove(struct auxiliary_device *aux_dev) {
> > > > > +	struct intel_dg_spi *spi = dev_get_drvdata(&aux_dev->dev);

> > > > > +	if (!spi)
> > > > > +		return;

> > > > > +	dev_set_drvdata(&aux_dev->dev, NULL);

> > > > If the above is doing anything there's a problem...
> > o
> > > It makes sure the data is set to NULL.

> > Which is needed because...?

> This is a boilerplate part, the content is consequent patches. 

Which would come back to the issues created by the random splitting of
the series were it not for the fact that if anything tries to look at
the driver data of a removed device it's buggy, the reference is gone
and the device may have been deallocated and it's certainly freed from
the perspective of this user.  Notice how other drivers don't do this.
The driver core will also overwrite the driver data of released
devices...

At a high level a lot of the issues with this series is that both in
terms of how it's been sent and what it's doing there's a bunch of
things that look nothing like how we normally handle things.  At best
this means that problems are being solved at the wrong level, but it's
hard to see that this is the case.

Alexander Usyskin Sept. 25, 2024, 12:31 p.m. UTC | #7

> On Sat, Sep 21, 2024 at 01:00:52PM +0000, Winkler, Tomas wrote:
> > > On Thu, Sep 19, 2024 at 09:54:24AM +0000, Winkler, Tomas wrote:
> > > > > On Mon, Sep 16, 2024 at 04:49:17PM +0300, Alexander Usyskin
> wrote:
> 
> > > Just do normal open coded allocations, the reference counting is just
> > > obscure.
> 
> > The kref here is for reason so we don't need to hunt the close open, I frankly
> don't understand
> > what is wrong with it,
> 
> It's locking/refcounting stuff that looks nothing like any other SPI
> controller driver.  Even if it works it's obviously fragile since the
> driver does surprising things which break assumptions that will be made
> by people not looking at this specific driver, and causes people to have
> to spend more effort figuring out what you're doing.  If there is any
> benefit to doing this then open coding it in one specific driver is
> clearly not the right place to do it.
> 

The reason for all this refcounting that the device itself is discrete card and
the SPI memory backend can disappear at any moment leaving open connections dangling.
Refcount ensures that object behind the MTD device will be freed only
when the last user-space is disconnected.

Is there any other model that can keep the object alive in this situation?

- - 
Thanks,
Sasha

Mark Brown Sept. 25, 2024, 1:12 p.m. UTC | #8

On Wed, Sep 25, 2024 at 12:31:34PM +0000, Usyskin, Alexander wrote:

> > It's locking/refcounting stuff that looks nothing like any other SPI
> > controller driver.  Even if it works it's obviously fragile since the
> > driver does surprising things which break assumptions that will be made
> > by people not looking at this specific driver, and causes people to have
> > to spend more effort figuring out what you're doing.  If there is any
> > benefit to doing this then open coding it in one specific driver is
> > clearly not the right place to do it.

> The reason for all this refcounting that the device itself is discrete card and
> the SPI memory backend can disappear at any moment leaving open connections dangling.
> Refcount ensures that object behind the MTD device will be freed only
> when the last user-space is disconnected.

> Is there any other model that can keep the object alive in this situation?

If devices connected to the SPI controller can be hotplugged then those
devices should be registered and unregistered like any other spi_device
would be, the driver core and relevant subsystems will deal with the
lifetime issues.  This is all invisible to the SPI controllers, the fact
that things are being open coded in the driver should be a big warning
sign.

Patch

diff --git a/MAINTAINERS b/MAINTAINERS
index 333ed0718175..b2c1aa743bc6 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11214,6 +11214,13 @@  L:	[e-mail]
 S:	Supported
 F:	arch/x86/include/asm/intel-family.h
 
+INTEL DISCRETE GRAPHIC SPI FLASH DRIVER
+M:	Alexander Usyskin <[e-mail]>
+L:	[e-mail]
+S:	Supported
+F:	drivers/spi/spi-intel-dg.c
+F:	include/linux/intel_dg_spi_aux.h
+
 INTEL DRM DISPLAY FOR XE AND I915 DRIVERS
 M:	Jani Nikula <[e-mail]>
 M:	Rodrigo Vivi <[e-mail]>
diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index ec1550c698d5..95d6667dc127 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -524,6 +524,17 @@  config SPI_INTEL_PLATFORM
 	  To compile this driver as a module, choose M here: the module
 	  will be called spi-intel-platform.
 
+config SPI_INTEL_DG
+	tristate "Intel Discrete Graphic SPI flash driver"
+	depends on AUXILIARY_BUS
+	depends on MTD
+	help
+	  This enables support for Intel Discrete Graphic SPI
+	  auxiliary device.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called spi-intel-dg.
+
 config SPI_JCORE
 	tristate "J-Core SPI Master"
 	depends on OF && (SUPERH || COMPILE_TEST)
diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
index a9b1bc259b68..ae8398930f3b 100644
--- a/drivers/spi/Makefile
+++ b/drivers/spi/Makefile
@@ -71,6 +71,7 @@  obj-$(CONFIG_SPI_INGENIC)		+= spi-ingenic.o
 obj-$(CONFIG_SPI_INTEL)			+= spi-intel.o
 obj-$(CONFIG_SPI_INTEL_PCI)		+= spi-intel-pci.o
 obj-$(CONFIG_SPI_INTEL_PLATFORM)	+= spi-intel-platform.o
+obj-$(CONFIG_SPI_INTEL_DG)		+= spi-intel-dg.o
 obj-$(CONFIG_SPI_LANTIQ_SSC)		+= spi-lantiq-ssc.o
 obj-$(CONFIG_SPI_JCORE)			+= spi-jcore.o
 obj-$(CONFIG_SPI_LJCA)			+= spi-ljca.o
diff --git a/drivers/spi/spi-intel-dg.c b/drivers/spi/spi-intel-dg.c
new file mode 100644
index 000000000000..4e302957f077
--- /dev/null
+++ b/drivers/spi/spi-intel-dg.c
@@ -0,0 +1,142 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright(c) 2019-2024, Intel Corporation. All rights reserved.
+ */
+
+#include <linux/device.h>
+#include <linux/intel_dg_spi_aux.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/string.h>
+#include <linux/slab.h>
+#include <linux/types.h>
+
+struct intel_dg_spi {
+	struct kref refcnt;
+	void __iomem *base;
+	size_t size;
+	unsigned int nregions;
+	struct {
+		const char *name;
+		u8 id;
+		u64 offset;
+		u64 size;
+	} regions[];
+};
+
+static void intel_dg_spi_release(struct kref *kref)
+{
+	struct intel_dg_spi *spi = container_of(kref, struct intel_dg_spi, refcnt);
+	int i;
+
+	pr_debug("freeing spi memory\n");
+	for (i = 0; i < spi->nregions; i++)
+		kfree(spi->regions[i].name);
+	kfree(spi);
+}
+
+static int intel_dg_spi_probe(struct auxiliary_device *aux_dev,
+			      const struct auxiliary_device_id *aux_dev_id)
+{
+	struct intel_dg_spi_dev *ispi = auxiliary_dev_to_intel_dg_spi_dev(aux_dev);
+	struct device *device;
+	struct intel_dg_spi *spi;
+	unsigned int nregions;
+	unsigned int i, n;
+	size_t size;
+	char *name;
+	size_t name_size;
+	int ret;
+
+	device = &aux_dev->dev;
+
+	/* count available regions */
+	for (nregions = 0, i = 0; i < INTEL_DG_SPI_REGIONS; i++) {
+		if (ispi->regions[i].name)
+			nregions++;
+	}
+
+	if (!nregions) {
+		dev_err(device, "no regions defined\n");
+		return -ENODEV;
+	}
+
+	size = sizeof(*spi) + sizeof(spi->regions[0]) * nregions;
+	spi = kzalloc(size, GFP_KERNEL);
+	if (!spi)
+		return -ENOMEM;
+
+	kref_init(&spi->refcnt);
+
+	spi->nregions = nregions;
+	for (n = 0, i = 0; i < INTEL_DG_SPI_REGIONS; i++) {
+		if (ispi->regions[i].name) {
+			name_size = strlen(dev_name(&aux_dev->dev)) +
+				    strlen(ispi->regions[i].name) + 2; /* for point */
+			name = kzalloc(name_size, GFP_KERNEL);
+			if (!name)
+				continue;
+			snprintf(name, name_size, "%s.%s",
+				 dev_name(&aux_dev->dev), ispi->regions[i].name);
+			spi->regions[n].name = name;
+			spi->regions[n].id = i;
+			n++;
+		}
+	}
+
+	spi->base = devm_ioremap_resource(device, &ispi->bar);
+	if (IS_ERR(spi->base)) {
+		dev_err(device, "mmio not mapped\n");
+		ret = PTR_ERR(spi->base);
+		goto err;
+	}
+
+	dev_set_drvdata(&aux_dev->dev, spi);
+
+	return 0;
+
+err:
+	kref_put(&spi->refcnt, intel_dg_spi_release);
+	return ret;
+}
+
+static void intel_dg_spi_remove(struct auxiliary_device *aux_dev)
+{
+	struct intel_dg_spi *spi = dev_get_drvdata(&aux_dev->dev);
+
+	if (!spi)
+		return;
+
+	dev_set_drvdata(&aux_dev->dev, NULL);
+
+	kref_put(&spi->refcnt, intel_dg_spi_release);
+}
+
+static const struct auxiliary_device_id intel_dg_spi_id_table[] = {
+	{
+		.name = "i915.spi",
+	},
+	{
+		.name = "xe.spi",
+	},
+	{
+		/* sentinel */
+	}
+};
+MODULE_DEVICE_TABLE(auxiliary, intel_dg_spi_id_table);
+
+static struct auxiliary_driver intel_dg_spi_driver = {
+	.probe  = intel_dg_spi_probe,
+	.remove = intel_dg_spi_remove,
+	.driver = {
+		/* auxiliary_driver_register() sets .name to be the modname */
+	},
+	.id_table = intel_dg_spi_id_table
+};
+
+module_auxiliary_driver(intel_dg_spi_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Intel Corporation");
+MODULE_DESCRIPTION("Intel DGFX SPI driver");
diff --git a/include/linux/intel_dg_spi_aux.h b/include/linux/intel_dg_spi_aux.h
new file mode 100644
index 000000000000..d4c3830d56d6
--- /dev/null
+++ b/include/linux/intel_dg_spi_aux.h
@@ -0,0 +1,27 @@ 
+/* SPDX-License-Identifier: MIT */
+/*
+ * Copyright(c) 2019-2024, Intel Corporation. All rights reserved.
+ */
+
+#ifndef __INTEL_DG_SPI_AUX_H__
+#define __INTEL_DG_SPI_AUX_H__
+
+#include <linux/auxiliary_bus.h>
+
+#define INTEL_DG_SPI_REGIONS 13
+
+struct intel_dg_spi_region {
+	const char *name;
+};
+
+struct intel_dg_spi_dev {
+	struct auxiliary_device aux_dev;
+	bool writeable_override;
+	struct resource bar;
+	const struct intel_dg_spi_region *regions;
+};
+
+#define auxiliary_dev_to_intel_dg_spi_dev(auxiliary_dev) \
+	container_of(auxiliary_dev, struct intel_dg_spi_dev, aux_dev)
+
+#endif /* __INTEL_DG_SPI_AUX_H__ */