Re: [PATCH 2/2] mtd: devices: Add Qualcomm SCM storage driver

[Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>]

On 12/18/25 7:02 PM, Junhao Xie wrote:
> Add MTD driver for accessing storage devices managed by Qualcomm's
> TrustZone firmware. On some platforms, BIOS/firmware storage (typically
> SPI NOR flash) is not directly accessible from the non-secure world and
> all operations must go through SCM (Secure Channel Manager) calls.
> 
> Signed-off-by: Junhao Xie <bigfoot@radxa.com>
> Tested-by: Xilin Wu <sophon@radxa.com>
> ---

[...]

> +struct qcom_scm_storage {
> +	struct device *dev;
> +	struct mutex lock;	/* Protects SCM storage operations */
> +	struct mtd_info mtd;
> +	struct qcom_scm_storage_info info;
> +	size_t buffer_size;
> +	u8 *buffer;
> +};
> +
> +static int qcom_scm_storage_erase(struct mtd_info *mtd,
> +				  struct erase_info *instr)
> +{
> +	struct qcom_scm_storage *host =
> +		container_of(mtd, struct qcom_scm_storage, mtd);
> +
> +	if (instr->addr % host->info.block_size ||
> +	    instr->len % host->info.block_size)

While it's the same value, it seems like mtd->erasesize would be
"idiomatic" here

> +		return -EINVAL;
> +
> +	guard(mutex)(&host->lock);
> +
> +	return qcom_scm_storage_send_cmd(QCOM_SCM_STORAGE_SPINOR,
> +					 QCOM_SCM_STORAGE_ERASE,
> +					 instr->addr / host->info.block_size,
> +					 0, instr->len);
> +}
> +
> +static int qcom_scm_storage_read(struct mtd_info *mtd,
> +				 loff_t from, size_t len,
> +				 size_t *retlen, u_char *buf)
> +{
> +	struct qcom_scm_storage *host =
> +		container_of(mtd, struct qcom_scm_storage, mtd);

Feel free to unwrap this line

> +	size_t block_size = host->info.block_size;
> +	loff_t block_start, block_off, lba;
> +	size_t chunk, to_read;
> +	int ret = 0;

This initialization seems unnecessary

[...]

> +static int qcom_scm_storage_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct qcom_scm_storage *host;
> +	int ret;
> +
> +	host = devm_kzalloc(dev, sizeof(*host), GFP_KERNEL);
> +	if (!host)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, host);
> +	host->dev = dev;
> +
> +	ret = devm_mutex_init(dev, &host->lock);
> +	if (ret)
> +		return ret;
> +
> +	host->buffer_size = SZ_256K;

Should this just be = host->info->page_size?

[...]

> +	dev_info(dev, "scm storage 0x%llx registered with size %llu bytes\n",
> +		 host->info.serial_num, host->mtd.size);

dev_dbg()?

Konrad