Re: [RFC PATCH 4/5] spi: qpic: Add support for qpic spi nand driver

[Md Sadre Alam <quic_mdalam@quicinc.com>]

On 10/31/2023 7:53 PM, Mark Brown wrote:
> On Tue, Oct 31, 2023 at 05:33:06PM +0530, Md Sadre Alam wrote:
> 
>> +config SPI_QPIC_SNAND
>> +	tristate "QPIC SNAND controller"
>> +	default y
>> +	depends on ARCH_QCOM
>> +	help
>> +	  QPIC_SNAND(Quad SPI) driver for Qualcomm QPIC_SNAND controller.
>> +
> 
> I don't see any build dependencies on anything QC specific so please add
> an || COMPILE_TEST here, this makes it much easier to do generic changes
> without having to build some specific config.

Ok

> 
>> +++ b/drivers/spi/Makefile
>> @@ -153,6 +153,7 @@ obj-$(CONFIG_SPI_XTENSA_XTFPGA)		+= spi-xtensa-xtfpga.o
>>   obj-$(CONFIG_SPI_ZYNQ_QSPI)		+= spi-zynq-qspi.o
>>   obj-$(CONFIG_SPI_ZYNQMP_GQSPI)		+= spi-zynqmp-gqspi.o
>>   obj-$(CONFIG_SPI_AMD)			+= spi-amd.o
>> +obj-$(CONFIG_SPI_QPIC_SNAND)            += spi-qpic-snand.o
> 
> Please keep this alphabetically sorted (there are some mistakes there
> but no need to add to them).

Ok

> 
>> + * 	Sricharan R <quic_srichara@quicinc.com>
>> + */
>> +
>> +#include <linux/mtd/spinand.h>
>> +#include <linux/mtd/nand-qpic-common.h>
>> +
> 
> This should be including the SPI API, and other API headers that are
> used directly like the platform and clock APIs.
>

Ok

>> +static int qcom_snand_init(struct qcom_nand_controller *snandc)
>> +{
>> +	u32 snand_cfg_val = 0x0;
>> +	int ret;
> 
> ...
> 
>> +	ret = submit_descs(snandc);
>> +	if (ret)
>> +		dev_err(snandc->dev, "failure in sbumitting spiinit descriptor\n");
>> +
>> +	free_descs(snandc);
> 
> This seems to be doing a bit more than I would expect an init function
> to, and it's very surprising to see the descriptors freed immediately
> after something called a submit (which suggests that the descriptors are
> still in flight).
>

Our controller supports only bam mode , that means for writing/reading even
single register we have to use bam.
submit_descs() is synchronous so I/O is complete when it returns.
Hence freeing the descriptor after it.


>> +static int qpic_snand_read_page(struct qcom_nand_controller *snandc,
>> +				const struct spi_mem_op *op)
>> +{
>> +	return 0;
>> +}
>> +
>> +static int qpic_snand_write_page(struct qcom_nand_controller *snandc,
>> +				const struct spi_mem_op *op)
>> +{
>> +	return 0;
>> +}
> 
> ...
> 
>> +static int qpic_snand_exec_op(struct spi_mem *mem, const struct spi_mem_op *op)
>> +{
>> +	struct qcom_nand_controller *snandc = spi_controller_get_devdata(mem->spi->master);
>> +	dev_dbg(snandc->dev, "OP %02x ADDR %08llX@%d:%u DATA %d:%u", op->cmd.opcode,
>> +		op->addr.val, op->addr.buswidth, op->addr.nbytes,
>> +		op->data.buswidth, op->data.nbytes);
>> +
>> +	/*
>> +	 * Check for page ops or normal ops
>> +	 */
>> +	if (qpic_snand_is_page_op(op)) {
>> +		if (op->data.dir == SPI_MEM_DATA_IN)
>> +			return qpic_snand_read_page(snandc, op);
>> +		else
>> +			return qpic_snand_write_page(snandc, op);
> 
> So does the device actually support page operations?  The above looks
> like the driver will silently noop them.

Sorry It was to do item and I missed to mention that in commit log.
Will add in V1.

> 
>> +	snandc->base_phys = res->start;
>> +	snandc->base_dma = dma_map_resource(dev, res->start,
>> +					   resource_size(res),
>> +					   DMA_BIDIRECTIONAL, 0);
>> +	if (dma_mapping_error(dev, snandc->base_dma))
>> +		return -ENXIO;
>> +
>> +	ret = clk_prepare_enable(snandc->core_clk);
>> +	if (ret)
>> +		goto err_core_clk;
> 
> The DMA mapping and clock enables only get undone in error handling,
> they're not undone in the normal device release path.

Will fix in V1