| Message ID | 20240916134928.3654054-1-alexander.usyskin@intel.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | spi: add driver for Intel discrete graphics | expand |
On Mon, Sep 16, 2024 at 04:49:16PM +0300, Alexander Usyskin wrote: > Add driver for access to Intel discrete graphics card > internal SPI device. > Expose device on auxiliary bus by i915 and Xe drivers and > provide spi driver to register this device with MTD framework. As far as I can tell this does not actually provide a SPI driver, there is no call to any SPI API that I've noticed here. The SPI framework does have support for SPI controllers with specific flash support via spi_controller_mem_ops but this does not appear to use them. Either it should do that or it should just be a MTD driver. The series is also split up into too many patches with minimal explanation, making it hard to follow what's going on. I would recommend making the first patch be a minimal functional driver and then building on top of that.
> On Mon, Sep 16, 2024 at 04:49:16PM +0300, Alexander Usyskin wrote: > > Add driver for access to Intel discrete graphics card > > internal SPI device. > > Expose device on auxiliary bus by i915 and Xe drivers and > > provide spi driver to register this device with MTD framework. > > As far as I can tell this does not actually provide a SPI driver, there > is no call to any SPI API that I've noticed here. The SPI framework > does have support for SPI controllers with specific flash support via > spi_controller_mem_ops but this does not appear to use them. Either it > should do that or it should just be a MTD driver. > I have had some talks with Miquel on this and he was not sure where this driver belongs. Miquel - can this driver be put in drivers/mtd, as spi cleanly do not want it? > The series is also split up into too many patches with minimal > explanation, making it hard to follow what's going on. I would > recommend making the first patch be a minimal functional driver and then > building on top of that. As I understand, better to have small amount of big patches than big list of smaller patches? I'll try, according to your comments in other patches, but we have there three big parts (changes in i915, changes in Xe and actual spi/mtd driver) and they should come together as there is no point in any of them alone. - - Thanks, Sasha
On Thu, Sep 19, 2024 at 06:56:39AM +0000, Usyskin, Alexander wrote: > > On Mon, Sep 16, 2024 at 04:49:16PM +0300, Alexander Usyskin wrote: > > As far as I can tell this does not actually provide a SPI driver, there > > is no call to any SPI API that I've noticed here. The SPI framework > > does have support for SPI controllers with specific flash support via > > spi_controller_mem_ops but this does not appear to use them. Either it > > should do that or it should just be a MTD driver. > I have had some talks with Miquel on this and he was not sure where this driver belongs. > Miquel - can this driver be put in drivers/mtd, as spi cleanly do not want it? To be clear, it's not that this hardware shouldn't be supported in the SPI subsystem it's that if the driver is in the SPI subsystem it should be written in terms of the relevant APIs. Simply taking a MTD driver and dropping it in a random other directory with no further updates is clearly not the way forwards. > > The series is also split up into too many patches with minimal > > explanation, making it hard to follow what's going on. I would > > recommend making the first patch be a minimal functional driver and then > > building on top of that. > As I understand, better to have small amount of big patches > than big list of smaller patches? The patches should be individually standalone and useful, the problem here is that it's difficult to review any of the patches by themselves because they've not been split up with a clear goal.
Add driver for access to Intel discrete graphics card internal SPI device. Expose device on auxiliary bus by i915 and Xe drivers and provide spi driver to register this device with MTD framework. This is a rewrite of "drm/i915/spi: spi access for discrete graphics" series with connection to the Xe driver and splitting the spi driver part to separate module in spi subsystem. This series intended to be pushed through drm-xe-next. V6: rebase over latest drm-xe-next, update to change in Xe APIs V5: depend solely on AUXILIARY_BUS, not on COMPILE_TEST disable spi driver on virtual function in Xe, no spi device there V4: fix white-spaces add check for discrete graphics missed in i915 intel_spi_fini V3: rebase over drm-xe-next to enable CI run V2: fix review comments fix signatures order depend spi presence in Xe on special flag, as not all new discrete cards have such spi Alexander Usyskin (6): spi: add driver for intel graphics on-die spi device spi: intel-dg: align 64bit read and write spi: intel-dg: wake card on operations drm/i915/spi: add support for access mode drm/xe/spi: add on-die spi device drm/xe/spi: add support for access mode Tomas Winkler (6): spi: intel-dg: implement region enumeration spi: intel-dg: implement spi access functions spi: intel-dg: spi register with mtd spi: intel-dg: implement mtd access handlers drm/i915/spi: add spi device for discrete graphics drm/i915/spi: add intel_spi_region map MAINTAINERS | 7 + drivers/gpu/drm/i915/Makefile | 4 + drivers/gpu/drm/i915/i915_driver.c | 6 + drivers/gpu/drm/i915/i915_drv.h | 4 + drivers/gpu/drm/i915/i915_reg.h | 1 + drivers/gpu/drm/i915/spi/intel_spi.c | 101 +++ drivers/gpu/drm/i915/spi/intel_spi.h | 15 + drivers/gpu/drm/xe/Makefile | 1 + drivers/gpu/drm/xe/regs/xe_gsc_regs.h | 4 + drivers/gpu/drm/xe/xe_device.c | 3 + drivers/gpu/drm/xe/xe_device_types.h | 8 + drivers/gpu/drm/xe/xe_heci_gsc.c | 5 +- drivers/gpu/drm/xe/xe_pci.c | 5 + drivers/gpu/drm/xe/xe_spi.c | 113 ++++ drivers/gpu/drm/xe/xe_spi.h | 15 + drivers/spi/Kconfig | 11 + drivers/spi/Makefile | 1 + drivers/spi/spi-intel-dg.c | 863 ++++++++++++++++++++++++++ include/linux/intel_dg_spi_aux.h | 27 + 19 files changed, 1190 insertions(+), 4 deletions(-) create mode 100644 drivers/gpu/drm/i915/spi/intel_spi.c create mode 100644 drivers/gpu/drm/i915/spi/intel_spi.h create mode 100644 drivers/gpu/drm/xe/xe_spi.c create mode 100644 drivers/gpu/drm/xe/xe_spi.h create mode 100644 drivers/spi/spi-intel-dg.c create mode 100644 include/linux/intel_dg_spi_aux.h