Re: [PATCH v10 04/10] mtd: intel-dg: register with mtd

From: Raag Jadav <raag.jadav@intel.com>
To: Alexander Usyskin <alexander.usyskin@intel.com>
Cc: "Miquel Raynal" <miquel.raynal@bootlin.com>,
	"Richard Weinberger" <richard@nod.at>,
	"Vignesh Raghavendra" <vigneshr@ti.com>,
	"Lucas De Marchi" <lucas.demarchi@intel.com>,
	"Thomas Hellström" <thomas.hellstrom@linux.intel.com>,
	"Rodrigo Vivi" <rodrigo.vivi@intel.com>,
	"Maarten Lankhorst" <maarten.lankhorst@linux.intel.com>,
	"Maxime Ripard" <mripard@kernel.org>,
	"Thomas Zimmermann" <tzimmermann@suse.de>,
	"David Airlie" <airlied@gmail.com>,
	"Simona Vetter" <simona@ffwll.ch>,
	"Jani Nikula" <jani.nikula@linux.intel.com>,
	"Joonas Lahtinen" <joonas.lahtinen@linux.intel.com>,
	"Tvrtko Ursulin" <tursulin@ursulin.net>,
	"Karthik Poosa" <karthik.poosa@intel.com>,
	"Reuven Abliyev" <reuven.abliyev@intel.com>,
	"Oren Weil" <oren.jer.weil@intel.com>,
	linux-mtd@lists.infradead.org, intel-xe@lists.freedesktop.org,
	dri-devel@lists.freedesktop.org, intel-gfx@lists.freedesktop.org,
	linux-kernel@vger.kernel.org, "Tomas Winkler" <tomasw@gmail.com>,
	"Vitaly Lubart" <lubvital@gmail.com>
Subject: Re: [PATCH v10 04/10] mtd: intel-dg: register with mtd
Date: Thu, 22 May 2025 00:37:17 +0300	[thread overview]
Message-ID: <aC5HjQKRf_4Ewous@black.fi.intel.com> (raw)
In-Reply-To: <20250515133345.2805031-5-alexander.usyskin@intel.com>

On Thu, May 15, 2025 at 04:33:39PM +0300, Alexander Usyskin wrote:
> Register the on-die nvm device with the mtd subsystem.
> Refcount nvm object on _get and _put mtd callbacks.
> For erase operation address and size should be 4K aligned.
> For write operation address and size has to be 4bytes aligned.

...

> +static int intel_dg_nvm_init_mtd(struct intel_dg_nvm *nvm, struct device *device,
> +				 unsigned int nparts, bool writable_override)
> +{
> +	unsigned int i;
> +	unsigned int n;
> +	struct mtd_partition *parts = NULL;
> +	int ret;

Reverse xmas order (along with all other places).

> +	dev_dbg(device, "registering with mtd\n");
> +
> +	nvm->mtd.owner = THIS_MODULE;
> +	nvm->mtd.dev.parent = device;
> +	nvm->mtd.flags = MTD_CAP_NORFLASH | MTD_WRITEABLE;

Isn't MTD_CAP_NORFLASH already writable?

> +	nvm->mtd.type = MTD_DATAFLASH;
> +	nvm->mtd.priv = nvm;
> +	nvm->mtd._write = intel_dg_mtd_write;
> +	nvm->mtd._read = intel_dg_mtd_read;
> +	nvm->mtd._erase = intel_dg_mtd_erase;
> +	nvm->mtd._get_device = intel_dg_mtd_get_device;
> +	nvm->mtd._put_device = intel_dg_mtd_put_device;
> +	nvm->mtd.writesize = SZ_1; /* 1 byte granularity */
> +	nvm->mtd.erasesize = SZ_4K; /* 4K bytes granularity */
> +	nvm->mtd.size = nvm->size;
> +
> +	parts = kcalloc(nvm->nregions, sizeof(*parts), GFP_KERNEL);
> +	if (!parts)
> +		return -ENOMEM;
> +
> +	for (i = 0, n = 0; i < nvm->nregions && n < nparts; i++) {
> +		if (!nvm->regions[i].is_readable)
> +			continue;
> +		parts[n].name = nvm->regions[i].name;
> +		parts[n].offset  = nvm->regions[i].offset;
> +		parts[n].size = nvm->regions[i].size;
> +		if (!nvm->regions[i].is_writable && !writable_override)
> +			parts[n].mask_flags = MTD_WRITEABLE;
> +		n++;
> +	}
> +
> +	ret = mtd_device_register(&nvm->mtd, parts, n);
> +
> +	kfree(parts);

I didn't find anything questionable about this, but hope this is tested
and there are no side-effects of this.

> +

Nit: Redundant blank line.

> +	return ret;
> +}

Raag

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