Re: [Intel-gfx] [RFC PATCH 2/9] drm/i915/spi: intel_spi_region map

[Jani Nikula <jani.nikula@linux.intel.com>]

On Wed, 17 Feb 2021, "Winkler, Tomas" <tomas.winkler@intel.com> wrote:
>> On Tue, 16 Feb 2021, Tomas Winkler <tomas.winkler@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@intel.com>
>> > Cc: Lucas De Marchi <lucas.demarchi@intel.com>
>> > Signed-off-by: Tomas Winkler <tomas.winkler@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?

Have a header that contains the interface exposed to the rest of i915,
and another header with stuff internal to spi/?

BR,
Jani.




-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx