[v6,04/12] spi: intel-dg: spi register with mtd

Commit Message

Usyskin, Alexander Sept. 16, 2024, 1:49 p.m. UTC

From: Tomas Winkler <[e-mail]>

Register the on-die spi device with the mtd subsystem.
Refcount spi object on _get and _put mtd callbacks.

CC: Rodrigo Vivi <[e-mail]>
CC: Lucas De Marchi <[e-mail]>
Signed-off-by: Tomas Winkler <[e-mail]>
Signed-off-by: Alexander Usyskin <[e-mail]>
---
 drivers/spi/spi-intel-dg.c | 111 +++++++++++++++++++++++++++++++++++++
 1 file changed, 111 insertions(+)

Comments

Mark Brown Sept. 18, 2024, 1:38 p.m. UTC | #1

On Mon, Sep 16, 2024 at 04:49:20PM +0300, Alexander Usyskin wrote:

> From: Tomas Winkler <[e-mail]>
> 
> Register the on-die spi device with the mtd subsystem.
> Refcount spi object on _get and _put mtd callbacks.

This is a MTD driver, it should be in drivers/mtd.

> +static int intel_dg_spi_erase(struct mtd_info *mtd, struct erase_info *info)
> +{
> +	return 0;
> +}
> +
> +static int intel_dg_spi_read(struct mtd_info *mtd, loff_t from, size_t len,
> +			     size_t *retlen, u_char *buf)
> +{
> +	return 0;
> +}
> +
> +static int intel_dg_spi_write(struct mtd_info *mtd, loff_t to, size_t len,
> +			      size_t *retlen, const u_char *buf)
> +{
> +	return 0;
> +}

If these functions can legitimately be empty they should be removed.

Winkler, Tomas Sept. 19, 2024, 10:01 a.m. UTC | #2

> 
> On Mon, Sep 16, 2024 at 04:49:20PM +0300, Alexander Usyskin wrote:
> 
> > From: Tomas Winkler <[e-mail]>
> >
> > Register the on-die spi device with the mtd subsystem.
> > Refcount spi object on _get and _put mtd callbacks.
> 
> This is a MTD driver, it should be in drivers/mtd.

Okay.

> 
> > +static int intel_dg_spi_erase(struct mtd_info *mtd, struct erase_info
> > +*info) {
> > +	return 0;
> > +}
> > +
> > +static int intel_dg_spi_read(struct mtd_info *mtd, loff_t from, size_t len,
> > +			     size_t *retlen, u_char *buf)
> > +{
> > +	return 0;
> > +}
> > +
> > +static int intel_dg_spi_write(struct mtd_info *mtd, loff_t to, size_t len,
> > +			      size_t *retlen, const u_char *buf) {
> > +	return 0;
> > +}
> 
> If these functions can legitimately be empty they should be removed.

Those are place holder so the code will compile and implemented in following patches, this is compromise on not making too big changes.
It use dot be acceptable compromise in past.

Mark Brown Sept. 19, 2024, 10:43 a.m. UTC | #3

On Thu, Sep 19, 2024 at 10:01:06AM +0000, Winkler, Tomas wrote:
> > On Mon, Sep 16, 2024 at 04:49:20PM +0300, Alexander Usyskin wrote:

> > > +static int intel_dg_spi_write(struct mtd_info *mtd, loff_t to, size_t len,
> > > +			      size_t *retlen, const u_char *buf) {
> > > +	return 0;
> > > +}

> > If these functions can legitimately be empty they should be removed.

> Those are place holder so the code will compile and implemented in following patches, this is compromise on not making too big changes.
> It use dot be acceptable compromise in past.

If you omit the functions you should obviously entirely omit them
entirely, including putting them in relevant ops struct.  As things
stand this just makes the code look buggy which doesn't help review,
you're adding functions which obviously don't work properly and not even
noting that in the changelog or code.  Just add the assignment of the
ops when you add the implementation of the ops, that way there's no
partially implemented step.

Patch

diff --git a/drivers/spi/spi-intel-dg.c b/drivers/spi/spi-intel-dg.c
index 863898c8739c..a936014f1a76 100644
--- a/drivers/spi/spi-intel-dg.c
+++ b/drivers/spi/spi-intel-dg.c
@@ -10,6 +10,8 @@ 
 #include <linux/io-64-nonatomic-lo-hi.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
+#include <linux/mtd/mtd.h>
+#include <linux/mtd/partitions.h>
 #include <linux/string.h>
 #include <linux/slab.h>
 #include <linux/sizes.h>
@@ -17,6 +19,8 @@ 
 
 struct intel_dg_spi {
 	struct kref refcnt;
+	struct mtd_info mtd;
+	struct mutex lock; /* region access lock */
 	void __iomem *base;
 	size_t size;
 	unsigned int nregions;
@@ -407,6 +411,23 @@  static int intel_dg_spi_init(struct intel_dg_spi *spi, struct device *device)
 	return n;
 }
 
+static int intel_dg_spi_erase(struct mtd_info *mtd, struct erase_info *info)
+{
+	return 0;
+}
+
+static int intel_dg_spi_read(struct mtd_info *mtd, loff_t from, size_t len,
+			     size_t *retlen, u_char *buf)
+{
+	return 0;
+}
+
+static int intel_dg_spi_write(struct mtd_info *mtd, loff_t to, size_t len,
+			      size_t *retlen, const u_char *buf)
+{
+	return 0;
+}
+
 static void intel_dg_spi_release(struct kref *kref)
 {
 	struct intel_dg_spi *spi = container_of(kref, struct intel_dg_spi, refcnt);
@@ -415,9 +436,90 @@  static void intel_dg_spi_release(struct kref *kref)
 	pr_debug("freeing spi memory\n");
 	for (i = 0; i < spi->nregions; i++)
 		kfree(spi->regions[i].name);
+	mutex_destroy(&spi->lock);
 	kfree(spi);
 }
 
+static int intel_dg_spi_get_device(struct mtd_info *mtd)
+{
+	struct mtd_info *master;
+	struct intel_dg_spi *spi;
+
+	if (!mtd)
+		return -ENODEV;
+
+	master = mtd_get_master(mtd);
+	spi = master->priv;
+	if (WARN_ON(!spi))
+		return -EINVAL;
+	pr_debug("get spi %s %d\n", mtd->name, kref_read(&spi->refcnt));
+	kref_get(&spi->refcnt);
+
+	return 0;
+}
+
+static void intel_dg_spi_put_device(struct mtd_info *mtd)
+{
+	struct mtd_info *master;
+	struct intel_dg_spi *spi;
+
+	if (!mtd)
+		return;
+
+	master = mtd_get_master(mtd);
+	spi = master->priv;
+	if (WARN_ON(!spi))
+		return;
+	pr_debug("put spi %s %d\n", mtd->name, kref_read(&spi->refcnt));
+	kref_put(&spi->refcnt, intel_dg_spi_release);
+}
+
+static int intel_dg_spi_init_mtd(struct intel_dg_spi *spi, struct device *device,
+				 unsigned int nparts, bool writeable_override)
+{
+	unsigned int i;
+	unsigned int n;
+	struct mtd_partition *parts = NULL;
+	int ret;
+
+	dev_dbg(device, "registering with mtd\n");
+
+	spi->mtd.owner = THIS_MODULE;
+	spi->mtd.dev.parent = device;
+	spi->mtd.flags = MTD_CAP_NORFLASH | MTD_WRITEABLE;
+	spi->mtd.type = MTD_DATAFLASH;
+	spi->mtd.priv = spi;
+	spi->mtd._write = intel_dg_spi_write;
+	spi->mtd._read = intel_dg_spi_read;
+	spi->mtd._erase = intel_dg_spi_erase;
+	spi->mtd._get_device = intel_dg_spi_get_device;
+	spi->mtd._put_device = intel_dg_spi_put_device;
+	spi->mtd.writesize = SZ_1; /* 1 byte granularity */
+	spi->mtd.erasesize = SZ_4K; /* 4K bytes granularity */
+	spi->mtd.size = spi->size;
+
+	parts = kcalloc(spi->nregions, sizeof(*parts), GFP_KERNEL);
+	if (!parts)
+		return -ENOMEM;
+
+	for (i = 0, n = 0; i < spi->nregions && n < nparts; i++) {
+		if (!spi->regions[i].is_readable)
+			continue;
+		parts[n].name = spi->regions[i].name;
+		parts[n].offset  = spi->regions[i].offset;
+		parts[n].size = spi->regions[i].size;
+		if (!spi->regions[i].is_writable && !writeable_override)
+			parts[n].mask_flags = MTD_WRITEABLE;
+		n++;
+	}
+
+	ret = mtd_device_register(&spi->mtd, parts, n);
+
+	kfree(parts);
+
+	return ret;
+}
+
 static int intel_dg_spi_probe(struct auxiliary_device *aux_dev,
 			      const struct auxiliary_device_id *aux_dev_id)
 {
@@ -449,6 +551,7 @@  static int intel_dg_spi_probe(struct auxiliary_device *aux_dev,
 	if (!spi)
 		return -ENOMEM;
 
+	mutex_init(&spi->lock);
 	kref_init(&spi->refcnt);
 
 	spi->nregions = nregions;
@@ -481,6 +584,12 @@  static int intel_dg_spi_probe(struct auxiliary_device *aux_dev,
 		goto err;
 	}
 
+	ret = intel_dg_spi_init_mtd(spi, device, ret, ispi->writeable_override);
+	if (ret) {
+		dev_err(device, "failed init mtd %d\n", ret);
+		goto err;
+	}
+
 	dev_set_drvdata(&aux_dev->dev, spi);
 
 	return 0;
@@ -497,6 +606,8 @@  static void intel_dg_spi_remove(struct auxiliary_device *aux_dev)
 	if (!spi)
 		return;
 
+	mtd_device_unregister(&spi->mtd);
+
 	dev_set_drvdata(&aux_dev->dev, NULL);
 
 	kref_put(&spi->refcnt, intel_dg_spi_release);