[PATCH v1 2/4] spi: spi-mtk-nor: improve device table for adding more capabilities

[Guochun Mao]

Hello Angelo,

Thanks for your comments.

On Fri, 2022-01-14 at 11:40 +0100, AngeloGioacchino Del Regno wrote:
> Il 14/01/22 07:24, guochun.mao at mediatek.com ha scritto:
> > From: Guochun Mao <guochun.mao at mediatek.com>
> 
> Hello Guochun,
> 
> thanks for the patch! However, I have some comments...
> 
> > 
> > Define a structure for adding more capabilities.
> 
> Please also mention that you're adding the extra_dummy_bit feature
> in the commit description.
> 
> > 
> > +struct mtk_nor_caps {
> > +	u32 dma_bits;
> 
> dma_bits can be u8: I really don't think that we'll ever see more
> than
> 0xff bits...
> 

I'll fix it next version.

> > +	u32 extra_dummy_bit;
> 
> This one looks a bit strange, can you please add a comment explaining
> the
> reason why we have this "extra dummy bit"?
> 

This one is adding for mt8186, which extra_dummy_bit is set to be 1.
Due to the IP design, it need a extra_dummy bit when reading nor
flash's register(status/config...).

Do you think I should move the modification to another patch
"[PATCH v1 3/4]spi:spi-mtk-nor:add new soc mt8186 sopport" ?
Or I just add this comment here?

> > 
> >   	// trigger op
> > -	writel(prg_len * BITS_PER_BYTE, sp->base +
> > MTK_NOR_REG_PRG_CNT);
> > +	if (rx_len)
> > +		writel(prg_len * BITS_PER_BYTE + sp->caps-
> > >extra_dummy_bit,
> > +		       sp->base + MTK_NOR_REG_PRG_CNT);
> > +	else
> > +		writel(prg_len * BITS_PER_BYTE, sp->base +
> > MTK_NOR_REG_PRG_CNT);
> > +
> >   	ret = mtk_nor_cmd_exec(sp, MTK_NOR_CMD_PROGRAM,
> >   			       prg_len * BITS_PER_BYTE);
> >   	if (ret)
> > @@ -743,9 +754,19 @@ static const struct spi_controller_mem_ops
> > mtk_nor_mem_ops = {
> >   	.exec_op = mtk_nor_exec_op
> >   };
> >   
> > +const struct mtk_nor_caps mtk_nor_caps_mt8173 = {
> > +	.dma_bits = 32,
> > +	.extra_dummy_bit = 0,
> > +};
> > +
> > +const struct mtk_nor_caps mtk_nor_caps_mt8192 = {
> > +	.dma_bits = 36,
> > +	.extra_dummy_bit = 0,
> > +};
> > +
> >   static const struct of_device_id mtk_nor_match[] = {
> > -	{ .compatible = "mediatek,mt8192-nor", .data = (void *)36 },
> > -	{ .compatible = "mediatek,mt8173-nor", .data = (void *)32 },
> > +	{ .compatible = "mediatek,mt8173-nor", .data =
> > &mtk_nor_caps_mt8173 },
> > +	{ .compatible = "mediatek,mt8192-nor", .data =
> > &mtk_nor_caps_mt8192 },
> >   	{ /* sentinel */ }
>   };
> >   MODULE_DEVICE_TABLE(of, mtk_nor_match);
> > @@ -754,10 +775,10 @@ static int mtk_nor_probe(struct
> > platform_device *pdev)
> >   {
> >   	struct spi_controller *ctlr;
> >   	struct mtk_nor *sp;
> > +	struct mtk_nor_caps *caps;
> >   	void __iomem *base;
> >   	struct clk *spi_clk, *ctlr_clk, *axi_clk;
> >   	int ret, irq;
> > -	unsigned long dma_bits;
> >   
> >   	base = devm_platform_ioremap_resource(pdev, 0);
> >   	if (IS_ERR(base))
> > @@ -775,9 +796,11 @@ static int mtk_nor_probe(struct
> > platform_device *pdev)
> >   	if (IS_ERR(axi_clk))
> >   		return PTR_ERR(axi_clk);
>   
> > -	dma_bits = (unsigned long)of_device_get_match_data(&pdev->dev);
> > -	if (dma_set_mask_and_coherent(&pdev->dev,
> > DMA_BIT_MASK(dma_bits))) {
> > -		dev_err(&pdev->dev, "failed to set dma mask(%lu)\n",
> > dma_bits);
> > +	caps = (struct mtk_nor_caps *)of_device_get_match_data(&pdev-
> > >dev);
> > +	if (dma_set_mask_and_coherent(&pdev->dev,
> > +				      DMA_BIT_MASK(caps->dma_bits))) {
> 
> While you're at it, can you please also make this return the right
> value?
> The function dma_set_mask_and_coherent() may return an error code, so
> it's
> worth it to just return that.
> 
> P.S.: 83 columns is ok
> 

ok, I'll fix it next version.
Thanks.

BR,
Guochun