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

[Md Sadre Alam]

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@xxxxxxxxxxx>
> > + */
> > +
> > +#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