RE: [RFC PATCH 3/9] drm/i915/spi: add driver for on-die spi device

["Winkler, Tomas" <tomas.winkler@intel.com>]

> 
> On Tue, 16 Feb 2021, Tomas Winkler <tomas.winkler@intel.com> wrote:
> > Add the platform driver for i915 on-die spi device, exposed via mfd
> > framework.
> >
> > 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/Kconfig             |   2 +
> >  drivers/gpu/drm/i915/Makefile            |   3 +
> >  drivers/gpu/drm/i915/spi/intel_spi_drv.c | 116
> > +++++++++++++++++++++++
> >  3 files changed, 121 insertions(+)
> >  create mode 100644 drivers/gpu/drm/i915/spi/intel_spi_drv.c
> >
> > diff --git a/drivers/gpu/drm/i915/Kconfig
> > b/drivers/gpu/drm/i915/Kconfig index abcaa8da45ac..13c870e5878e 100644
> > --- a/drivers/gpu/drm/i915/Kconfig
> > +++ b/drivers/gpu/drm/i915/Kconfig
> > @@ -27,6 +27,8 @@ config DRM_I915
> >  	select CEC_CORE if CEC_NOTIFIER
> >  	select VMAP_PFN
> >  	select MFD_CORE
> > +	select MTD
> 
> Selecting MTD does not seem to be a popular thing to do, which is usually a
> clue it's probably the wrong thing to do.
Depends, if it is not selected you'll end with wrongly configured system. 

> > +	select MTD_PARTITIONED_MASTER
> >  	help
> >  	  Choose this option if you have a system that has "Intel Graphics
> >  	  Media Accelerator" or "HD Graphics" integrated graphics, diff
> > --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> > index 7fa9120feb8d..f209cd541eec 100644
> > --- a/drivers/gpu/drm/i915/Makefile
> > +++ b/drivers/gpu/drm/i915/Makefile
> > @@ -299,6 +299,9 @@ endif
> >  obj-$(CONFIG_DRM_I915) += i915.o
> >  obj-$(CONFIG_DRM_I915_GVT_KVMGT) += gvt/kvmgt.o
> >
> > +obj-m += i915_spi.o
> 
> And literally nobody does this.
> 
> > +i915_spi-y := spi/intel_spi_drv.o
> 
> Time to add a separate Kconfig and Makefile in spi/?
Can be done. 
> 
> BR,
> Jani.
> 
> > +
> >  # header test
> >
> >  # exclude some broken headers from the test coverage diff --git
> > a/drivers/gpu/drm/i915/spi/intel_spi_drv.c
> > b/drivers/gpu/drm/i915/spi/intel_spi_drv.c
> > new file mode 100644
> > index 000000000000..23261f35b71f
> > --- /dev/null
> > +++ b/drivers/gpu/drm/i915/spi/intel_spi_drv.c
> > @@ -0,0 +1,116 @@
> > +// SPDX-License-Identifier: MIT
> > +/*
> > + * Copyright(c) 2019-2021, Intel Corporation. All rights reserved.
> > + */
> > +#include <linux/module.h>
> > +#include <linux/types.h>
> > +#include <linux/kernel.h>
> > +#include <linux/string.h>
> > +#include <linux/io.h>
> > +#include <linux/device.h>
> > +#include <linux/slab.h>
> > +#include <linux/platform_device.h>
> > +#include <spi/intel_spi.h>
> 
> Should this have "" instead of <>?
> 
> > +
> > +struct i915_spi {
> > +	void __iomem *base;
> > +	size_t size;
> > +	unsigned int nregions;
> > +	struct {
> > +		const char *name;
> > +		u8 id;
> > +		u64 offset;
> > +		u64 size;
> > +	} regions[];
> > +};
> > +
> > +static int i915_spi_probe(struct platform_device *platdev) {
> > +	struct resource *bar;
> > +	struct device *device;
> > +	struct i915_spi *spi;
> > +	struct i915_spi_region *regions;
> > +	unsigned int nregions;
> > +	unsigned int i, n;
> > +	size_t size;
> > +	char *name;
> > +	size_t name_size;
> > +
> > +	device = &platdev->dev;
> > +
> > +	regions = dev_get_platdata(&platdev->dev);
> > +	if (!regions) {
> > +		dev_err(device, "no regions defined\n");
> > +		return -ENODEV;
> > +	}
> > +
> > +	/* count available regions */
> > +	for (nregions = 0, i = 0; i < I915_SPI_REGIONS; i++) {
> > +		if (regions[i].name)
> > +			nregions++;
> > +	}
> > +
> > +	if (!nregions) {
> > +		dev_err(device, "no regions defined\n");
> > +		return -ENODEV;
> > +	}
> > +
> > +	size = sizeof(*spi) + sizeof(spi->regions[0]) * nregions;
> > +	spi = devm_kzalloc(device, size, GFP_KERNEL);
> > +	if (!spi)
> > +		return -ENOMEM;
> > +
> > +	spi->nregions = nregions;
> > +	for (n = 0, i = 0; i < I915_SPI_REGIONS; i++) {
> > +		if (regions[i].name) {
> > +			name_size = strlen(dev_name(&platdev->dev)) +
> > +				    strlen(regions[i].name) + 2; /* for point */
> > +			name = devm_kzalloc(device, name_size,
> GFP_KERNEL);
> > +			if (!name)
> > +				continue;
> > +			snprintf(name, name_size, "%s.%s",
> > +				 dev_name(&platdev->dev),
> regions[i].name);
> > +			spi->regions[n].name = name;
> > +			spi->regions[n].id = i;
> > +			n++;
> > +		}
> > +	}
> > +
> > +	bar = platform_get_resource(platdev, IORESOURCE_MEM, 0);
> > +	if (!bar)
> > +		return -ENODEV;
> > +
> > +	spi->base = devm_ioremap_resource(device, bar);
> > +	if (IS_ERR(spi->base)) {
> > +		dev_err(device, "mmio not mapped\n");
> > +		return PTR_ERR(spi->base);
> > +	}
> > +
> > +	platform_set_drvdata(platdev, spi);
> > +
> > +	dev_dbg(device, "i915-spi is bound\n");
> > +
> > +	return 0;
> > +}
> > +
> > +static int i915_spi_remove(struct platform_device *platdev) {
> > +	platform_set_drvdata(platdev, NULL);
> > +
> > +	return 0;
> > +}
> > +
> > +MODULE_ALIAS("platform:i915-spi");
> > +static struct platform_driver i915_spi_driver = {
> > +	.probe  = i915_spi_probe,
> > +	.remove = i915_spi_remove,
> > +	.driver = {
> > +		.name = "i915-spi",
> > +	},
> > +};
> > +
> > +module_platform_driver(i915_spi_driver);
> > +
> > +MODULE_LICENSE("GPL and additional rights"); MODULE_AUTHOR("Intel
> > +Corporation"); MODULE_DESCRIPTION("Intel DGFX SPI driver");
> 
> --
> Jani Nikula, Intel Open Source Graphics Center

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/