| Message ID | 20240707055341.3656-2-five231003@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Do device node auto cleanup in drivers/soc/ti/ | expand |
On 10:44-20240707, Kousik Sanagavarapu wrote: > Factor out memories setup code from probe() into a new function > pruss_of_setup_memories(). This sets the stage for introducing auto > cleanup of the device node (done in the subsequent patch), since the > clean up depends on the scope of the pointer and factoring out > code into a seperate function obviously limits the scope of the various typo s/seperate/separate - use --codespell with checkpatch to catch :) A follow on patch has the same problem as well. > variables used in that function. > > Apart from the above, this change also has the advantage of making the > code look more neat. > > While at it, use dev_err_probe() instead of plain dev_err() as this new > function is called by the probe(). > > Signed-off-by: Kousik Sanagavarapu <five231003@gmail.com> > --- > drivers/soc/ti/pruss.c | 111 ++++++++++++++++++++++------------------- > 1 file changed, 61 insertions(+), 50 deletions(-) > > diff --git a/drivers/soc/ti/pruss.c b/drivers/soc/ti/pruss.c > index 24a42e0b645c..a3c55a291b0b 100644 > --- a/drivers/soc/ti/pruss.c > +++ b/drivers/soc/ti/pruss.c > @@ -415,6 +415,63 @@ static int pruss_clk_init(struct pruss *pruss, struct device_node *cfg_node) > return ret; > } > > +static int pruss_of_setup_memories(struct device *dev, struct pruss *pruss) > +{ > + struct device_node *np = dev_of_node(dev); > + struct device_node *child; > + const struct pruss_private_data *data = of_device_get_match_data(dev); > + const char *mem_names[PRUSS_MEM_MAX] = { "dram0", "dram1", "shrdram2" }; > + int i; > + > + child = of_get_child_by_name(np, "memories"); > + if (!child) > + return dev_err_probe(dev, -ENODEV, > + "%pOF is missing its 'memories' node\n", > + child); > + > + for (i = 0; i < PRUSS_MEM_MAX; i++) { > + struct resource res; > + int index; > + > + /* > + * On AM437x one of two PRUSS units don't contain Shared RAM, > + * skip it > + */ > + if (data && data->has_no_sharedram && i == PRUSS_MEM_SHRD_RAM2) > + continue; > + > + index = of_property_match_string(child, "reg-names", > + mem_names[i]); > + if (index < 0) { > + of_node_put(child); > + return index; > + } > + > + if (of_address_to_resource(child, index, &res)) { > + of_node_put(child); > + return -EINVAL; > + } > + > + pruss->mem_regions[i].va = devm_ioremap(dev, res.start, > + resource_size(&res)); > + if (!pruss->mem_regions[i].va) { > + of_node_put(child); > + return dev_err_probe(dev, -ENOMEM, > + "failed to parse and map memory resource %d %s\n", > + i, mem_names[i]); > + } > + pruss->mem_regions[i].pa = res.start; > + pruss->mem_regions[i].size = resource_size(&res); > + > + dev_dbg(dev, "memory %8s: pa %pa size 0x%zx va %pK\n", > + mem_names[i], &pruss->mem_regions[i].pa, > + pruss->mem_regions[i].size, pruss->mem_regions[i].va); > + } > + of_node_put(child); > + > + return 0; > +} > + > static struct regmap_config regmap_conf = { > .reg_bits = 32, > .val_bits = 32, > @@ -471,15 +528,8 @@ static int pruss_cfg_of_init(struct device *dev, struct pruss *pruss) > static int pruss_probe(struct platform_device *pdev) > { > struct device *dev = &pdev->dev; > - struct device_node *np = dev_of_node(dev); > - struct device_node *child; > struct pruss *pruss; > - struct resource res; > - int ret, i, index; > - const struct pruss_private_data *data; > - const char *mem_names[PRUSS_MEM_MAX] = { "dram0", "dram1", "shrdram2" }; > - > - data = of_device_get_match_data(&pdev->dev); > + int ret; > > ret = dma_set_coherent_mask(dev, DMA_BIT_MASK(32)); > if (ret) { > @@ -494,48 +544,9 @@ static int pruss_probe(struct platform_device *pdev) > pruss->dev = dev; > mutex_init(&pruss->lock); > > - child = of_get_child_by_name(np, "memories"); > - if (!child) { > - dev_err(dev, "%pOF is missing its 'memories' node\n", child); > - return -ENODEV; > - } > - > - for (i = 0; i < PRUSS_MEM_MAX; i++) { > - /* > - * On AM437x one of two PRUSS units don't contain Shared RAM, > - * skip it > - */ > - if (data && data->has_no_sharedram && i == PRUSS_MEM_SHRD_RAM2) > - continue; > - > - index = of_property_match_string(child, "reg-names", > - mem_names[i]); > - if (index < 0) { > - of_node_put(child); > - return index; > - } > - > - if (of_address_to_resource(child, index, &res)) { > - of_node_put(child); > - return -EINVAL; > - } > - > - pruss->mem_regions[i].va = devm_ioremap(dev, res.start, > - resource_size(&res)); > - if (!pruss->mem_regions[i].va) { > - dev_err(dev, "failed to parse and map memory resource %d %s\n", > - i, mem_names[i]); > - of_node_put(child); > - return -ENOMEM; > - } > - pruss->mem_regions[i].pa = res.start; > - pruss->mem_regions[i].size = resource_size(&res); > - > - dev_dbg(dev, "memory %8s: pa %pa size 0x%zx va %pK\n", > - mem_names[i], &pruss->mem_regions[i].pa, > - pruss->mem_regions[i].size, pruss->mem_regions[i].va); > - } > - of_node_put(child); > + ret = pruss_of_setup_memories(dev, pruss); > + if (ret < 0) > + goto rpm_put; Why? We have not called pm_runtime_enable at this point. > > platform_set_drvdata(pdev, pruss); > > -- > 2.45.2.561.g66ac6e4bcd >
On Sat, Aug 24, 2024 at 01:49:50PM -0500, Nishanth Menon wrote: > On 10:44-20240707, Kousik Sanagavarapu wrote: > > Factor out memories setup code from probe() into a new function > > pruss_of_setup_memories(). This sets the stage for introducing auto > > cleanup of the device node (done in the subsequent patch), since the > > clean up depends on the scope of the pointer and factoring out > > code into a seperate function obviously limits the scope of the various > typo s/seperate/separate - use --codespell with checkpatch to catch :) > > A follow on patch has the same problem as well. Oh, yes should've used --codespell, my bad. Thanks for spotting. > > variables used in that function. [...] > > - of_node_put(child); > > + ret = pruss_of_setup_memories(dev, pruss); > > + if (ret < 0) > > + goto rpm_put; > > Why? We have not called pm_runtime_enable at this point. Didn't catch this too, will change. Thanks
diff --git a/drivers/soc/ti/pruss.c b/drivers/soc/ti/pruss.c index 24a42e0b645c..a3c55a291b0b 100644 --- a/drivers/soc/ti/pruss.c +++ b/drivers/soc/ti/pruss.c @@ -415,6 +415,63 @@ static int pruss_clk_init(struct pruss *pruss, struct device_node *cfg_node) return ret; } +static int pruss_of_setup_memories(struct device *dev, struct pruss *pruss) +{ + struct device_node *np = dev_of_node(dev); + struct device_node *child; + const struct pruss_private_data *data = of_device_get_match_data(dev); + const char *mem_names[PRUSS_MEM_MAX] = { "dram0", "dram1", "shrdram2" }; + int i; + + child = of_get_child_by_name(np, "memories"); + if (!child) + return dev_err_probe(dev, -ENODEV, + "%pOF is missing its 'memories' node\n", + child); + + for (i = 0; i < PRUSS_MEM_MAX; i++) { + struct resource res; + int index; + + /* + * On AM437x one of two PRUSS units don't contain Shared RAM, + * skip it + */ + if (data && data->has_no_sharedram && i == PRUSS_MEM_SHRD_RAM2) + continue; + + index = of_property_match_string(child, "reg-names", + mem_names[i]); + if (index < 0) { + of_node_put(child); + return index; + } + + if (of_address_to_resource(child, index, &res)) { + of_node_put(child); + return -EINVAL; + } + + pruss->mem_regions[i].va = devm_ioremap(dev, res.start, + resource_size(&res)); + if (!pruss->mem_regions[i].va) { + of_node_put(child); + return dev_err_probe(dev, -ENOMEM, + "failed to parse and map memory resource %d %s\n", + i, mem_names[i]); + } + pruss->mem_regions[i].pa = res.start; + pruss->mem_regions[i].size = resource_size(&res); + + dev_dbg(dev, "memory %8s: pa %pa size 0x%zx va %pK\n", + mem_names[i], &pruss->mem_regions[i].pa, + pruss->mem_regions[i].size, pruss->mem_regions[i].va); + } + of_node_put(child); + + return 0; +} + static struct regmap_config regmap_conf = { .reg_bits = 32, .val_bits = 32, @@ -471,15 +528,8 @@ static int pruss_cfg_of_init(struct device *dev, struct pruss *pruss) static int pruss_probe(struct platform_device *pdev) { struct device *dev = &pdev->dev; - struct device_node *np = dev_of_node(dev); - struct device_node *child; struct pruss *pruss; - struct resource res; - int ret, i, index; - const struct pruss_private_data *data; - const char *mem_names[PRUSS_MEM_MAX] = { "dram0", "dram1", "shrdram2" }; - - data = of_device_get_match_data(&pdev->dev); + int ret; ret = dma_set_coherent_mask(dev, DMA_BIT_MASK(32)); if (ret) { @@ -494,48 +544,9 @@ static int pruss_probe(struct platform_device *pdev) pruss->dev = dev; mutex_init(&pruss->lock); - child = of_get_child_by_name(np, "memories"); - if (!child) { - dev_err(dev, "%pOF is missing its 'memories' node\n", child); - return -ENODEV; - } - - for (i = 0; i < PRUSS_MEM_MAX; i++) { - /* - * On AM437x one of two PRUSS units don't contain Shared RAM, - * skip it - */ - if (data && data->has_no_sharedram && i == PRUSS_MEM_SHRD_RAM2) - continue; - - index = of_property_match_string(child, "reg-names", - mem_names[i]); - if (index < 0) { - of_node_put(child); - return index; - } - - if (of_address_to_resource(child, index, &res)) { - of_node_put(child); - return -EINVAL; - } - - pruss->mem_regions[i].va = devm_ioremap(dev, res.start, - resource_size(&res)); - if (!pruss->mem_regions[i].va) { - dev_err(dev, "failed to parse and map memory resource %d %s\n", - i, mem_names[i]); - of_node_put(child); - return -ENOMEM; - } - pruss->mem_regions[i].pa = res.start; - pruss->mem_regions[i].size = resource_size(&res); - - dev_dbg(dev, "memory %8s: pa %pa size 0x%zx va %pK\n", - mem_names[i], &pruss->mem_regions[i].pa, - pruss->mem_regions[i].size, pruss->mem_regions[i].va); - } - of_node_put(child); + ret = pruss_of_setup_memories(dev, pruss); + if (ret < 0) + goto rpm_put; platform_set_drvdata(pdev, pruss);
Factor out memories setup code from probe() into a new function pruss_of_setup_memories(). This sets the stage for introducing auto cleanup of the device node (done in the subsequent patch), since the clean up depends on the scope of the pointer and factoring out code into a seperate function obviously limits the scope of the various variables used in that function. Apart from the above, this change also has the advantage of making the code look more neat. While at it, use dev_err_probe() instead of plain dev_err() as this new function is called by the probe(). Signed-off-by: Kousik Sanagavarapu <five231003@gmail.com> --- drivers/soc/ti/pruss.c | 111 ++++++++++++++++++++++------------------- 1 file changed, 61 insertions(+), 50 deletions(-)