On Tue, Nov 30, 2021 at 10:32 AM Lh Kuo 郭力豪 <lh.Kuo@xxxxxxxxxxx> wrote: > > > ctlr->dev.of_node = pdev->dev.of_node; > > > > device_set_node() > > Is this funciton set as follows? > device_set_node(&ctlr->dev, of_fwnode_handle(pdev->dev.fwnode)); Can you please do something? For example, figuring out yourself (Elixir is a very good service for that): https://elixir.bootlin.com/linux/latest/A/ident/device_set_node ... > > > pspim->mas_base = devm_platform_ioremap_resource_byname(pdev, "master"); > > > pspim->sla_base = devm_platform_ioremap_resource_byname(pdev, > > > "slave"); > > > > Where are the error checks? > The changes are as follows? is this correct? Almost, but not enough. Please run checkpatch. > pspim->mas_base = devm_platform_ioremap_resource_byname(pdev, "master"); > if (IS_ERR(pspim->mas_base)) { > return dev_err_probe(dev, PTR_ERR(pspim->mas_base), "mas_base get fail\n"); > } > > pspim->sla_base = devm_platform_ioremap_resource_byname(pdev, "slave"); > if (IS_ERR(pspim->sla_base)) { > return dev_err_probe(dev, PTR_ERR(pspim->sla_base), "sla_base get fail\n"); > } ... > > > if (ret) { > > > dev_err_probe(dev, ret, "failed to deassert reset\n"); > > > goto err_free_reset; > > > } > > > > These two need to be wrapped as I explained above. > > I think these changes are depend on remove-function. No, it's the other way around: ->remove() implementation depends on these changes. > These settings are as follows? is this correct? No. > pspim->spi_clk = devm_clk_get(dev, NULL); > if (IS_ERR(pspim->spi_clk)) > return dev_err_probe(dev, PTR_ERR(pspim->spi_clk), "clk get fail\n"); > > pspim->rstc = devm_reset_control_get_exclusive(dev, NULL); > if (IS_ERR(pspim->rstc)) > return dev_err_probe(dev, PTR_ERR(pspim->rstc), "rst get fail\n"); > > ret = clk_prepare_enable(pspim->spi_clk); > if (ret) > return dev_err_probe(dev, ret, "failed to enable clk\n"); > > devm_add_action_or_reset(dev, (void(*)(void *))clk_disable_unprepare, > pspim->spi_clk); Please, find other drivers as examples of how to do that and take care about possible errors. > ret = reset_control_deassert(pspim->rstc); > if (ret) > return dev_err_probe(dev, ret, "failed to deassert reset\n"); > > devm_add_action_or_reset(dev, (void(*)(void *))reset_control_assert, > pspim->rstc); Ditto. > ret = spi_register_controller(ctlr); Read what Lukas said. > pm_runtime_enable(dev); > if (ret) { > pm_runtime_disable(dev); > return dev_err_probe(dev, ret, "spi_register_master fail\n"); > } > > return ret; > > } > > static int sp7021_spi_controller_remove(struct platform_device *pdev) > { > struct spi_controller *ctlr = dev_get_drvdata(&pdev->dev); > > pm_runtime_disable(&pdev->dev); > pm_runtime_set_suspended(&pdev->dev); > > return 0; > } -- With Best Regards, Andy Shevchenko
