Message ID | 20240916134928.3654054-2-alexander.usyskin@intel.com (mailing list archive) |
---|---|
State | New |
Headers | show |
Series | spi: add driver for Intel discrete graphics | expand |
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...
> > 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.
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...?
diff --git a/MAINTAINERS b/MAINTAINERS index 333ed0718175..b2c1aa743bc6 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -11214,6 +11214,13 @@ L: linux-kernel@vger.kernel.org S: Supported F: arch/x86/include/asm/intel-family.h +INTEL DISCRETE GRAPHIC SPI FLASH DRIVER +M: Alexander Usyskin <alexander.usyskin@intel.com> +L: linux-spi@vger.kernel.org +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 <jani.nikula@linux.intel.com> M: Rodrigo Vivi <rodrigo.vivi@intel.com> 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__ */