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

[AngeloGioacchino Del Regno]

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

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