[RFC PATCH 2/9] drm/i915/spi: intel_spi_region map

[Winkler, Tomas]

> On Wed, 17 Feb 2021, "Winkler, Tomas" <tomas.winkler at intel.com> wrote:
> >> On Tue, 16 Feb 2021, Tomas Winkler <tomas.winkler at intel.com> wrote:
> >> > Add the dGFX spi region map and convey it via mfd cell platform
> >> > data to the spi child device.
> >> >
> >> > Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
> >> > Cc: Lucas De Marchi <lucas.demarchi at intel.com>
> >> > Signed-off-by: Tomas Winkler <tomas.winkler at intel.com>
> >> > ---
> >> >  drivers/gpu/drm/i915/spi/intel_spi.c | 9 +++++++++
> >> > drivers/gpu/drm/i915/spi/intel_spi.h | 5 +++++
> >> >  2 files changed, 14 insertions(+)
> >> >
> >> > diff --git a/drivers/gpu/drm/i915/spi/intel_spi.c
> >> > b/drivers/gpu/drm/i915/spi/intel_spi.c
> >> > index 07da7197bd5d..6f83f24f7208 100644
> >> > --- a/drivers/gpu/drm/i915/spi/intel_spi.c
> >> > +++ b/drivers/gpu/drm/i915/spi/intel_spi.c
> >> > @@ -14,11 +14,20 @@ static const struct resource spi_resources[] = {
> >> >  	DEFINE_RES_MEM_NAMED(GEN12_GUNIT_SPI_BASE, 0x80, "i915-
> >> spi-mmio"),
> >> > };
> >> >
> >> > +static const struct i915_spi_region regions[I915_SPI_REGIONS] = {
> >> > +	[0] = { .name = "DESCRIPTOR", },
> >> > +	[2] = { .name = "GSC", },
> >> > +	[11] = { .name = "OptionROM", },
> >> > +	[12] = { .name = "DAM", },
> >> > +};
> >> > +
> >> >  static const struct mfd_cell intel_spi_cell = {
> >> >  	.id = 2,
> >> >  	.name = "i915-spi",
> >> >  	.num_resources = ARRAY_SIZE(spi_resources),
> >> >  	.resources = spi_resources,
> >> > +	.platform_data = (void *)regions,
> >> > +	.pdata_size    = sizeof(regions),
> >> >  };
> >> >
> >> >  void intel_spi_init(struct intel_spi *spi, struct drm_i915_private
> >> > *dev_priv) diff --git a/drivers/gpu/drm/i915/spi/intel_spi.h
> >> > b/drivers/gpu/drm/i915/spi/intel_spi.h
> >> > index 276551fed993..6b5bf053f7d3 100644
> >> > --- a/drivers/gpu/drm/i915/spi/intel_spi.h
> >> > +++ b/drivers/gpu/drm/i915/spi/intel_spi.h
> >> > @@ -8,6 +8,11 @@
> >> >
> >> >  struct drm_i915_private;
> >> >
> >> > +#define I915_SPI_REGIONS 13
> >> > +struct i915_spi_region {
> >> > +	const char *name;
> >> > +};
> >>
> >> Does this need to be exposed to the rest of i915?
> > This part is between the device which is part of i915 and the driver.
> >>If we're trying to isolate
> >> spi/, I'd prefer it if this header was the only header included from
> >>the rest of  i915, and contained the minimum required information.
> >
> >> As the driver has grown bigger, we've tried to minimize the
> >> interconnections between the modules, and it's slow going. Let's try
> >> to keep the new parts isolated.
> >>
> > So do you prefer we create another header or duplicate the structure
> definition?
> 
> I didn't see the struct being used in i915, or am I missing something?
This file is part of i915
 
> Have a header that contains the interface exposed to the rest of i915, and
> another header with stuff internal to spi/?

The spi `device` is part of i915 (as a mfd cell) and  struct i915_spi_region is  passed to the spi `driver` as platform_data of the device.
In order for driver to be able to understand  it needs to know ' struct i915_spi_region' 

Thanks
Tomas