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

[Guochun Mao]

On Mon, 2022-01-17 at 11:16 +0100, AngeloGioacchino Del Regno wrote:
> Il 17/01/22 03:30, Guochun Mao ha scritto:
> > 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?
> > 
> 
> It's more appropriate to keep it in this patch and mentioning it into
> the commit description. Also, please add this comment in the code
> and,
> if possible, also explain why this IP design needs an extra dummy
> bit.
> Otherwise, if not possible, the provided explaination would also be
> fine.
> 
> Thanks,
> - Angelo
> 

OK, thanks.

BR,
Guochun

> > > > 
> > > >    	// 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
> > 
> 
>