| Message ID | 20240201104443.363786-1-ckeepax@opensource.cirrus.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | spi: cs42l43: Clean up of firmware node | expand |
On Thu, Feb 1, 2024 at 12:44 PM Charles Keepax <ckeepax@opensource.cirrus.com> wrote: > > As we get a child node in the of case, we should also clean up the > references to this, add code for this. ... > + if (is_of_node(fwnode)) > + fwnode = fwnode_get_named_child_node(fwnode, "spi"); > + > + device_set_node(&priv->ctlr->dev, fwnode); > + > ret = devm_spi_register_controller(priv->dev, priv->ctlr); > if (ret) { > dev_err(priv->dev, "Failed to register SPI controller: %d\n", ret); > + goto of_node_err; > } > > + return 0; > + > +of_node_err: > + if (is_of_node(fwnode)) > + fwnode_handle_put(fwnode); Wrong order. What we need is to wrap this by devm_add_action_or_reset(), perhaps even providing devm_fwnode_handle_get() for everyone. > return ret; > } ... > +static void cs42l43_spi_remove(struct platform_device *pdev) > +{ > + struct cs42l43_spi *priv = dev_get_drvdata(&pdev->dev); > + > + if (is_of_node(priv->ctlr->dev.fwnode)) > + fwnode_handle_put(priv->ctlr->dev.fwnode); > +} No need as per above. If you send a new patch series where you add that API, I'll review / ack it immediately.
On Thu, Feb 1, 2024 at 4:43 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Thu, Feb 1, 2024 at 12:44 PM Charles Keepax > <ckeepax@opensource.cirrus.com> wrote: > > > > As we get a child node in the of case, we should also clean up the OF > > references to this, add code for this. ... > > + if (is_of_node(fwnode)) > > + fwnode = fwnode_get_named_child_node(fwnode, "spi"); > > + > > + device_set_node(&priv->ctlr->dev, fwnode); > > + > > ret = devm_spi_register_controller(priv->dev, priv->ctlr); > > if (ret) { > > dev_err(priv->dev, "Failed to register SPI controller: %d\n", ret); > > + goto of_node_err; > > } > > > > + return 0; > > + > > +of_node_err: > > + if (is_of_node(fwnode)) > > + fwnode_handle_put(fwnode); > > Wrong order. What we need is to wrap this by > devm_add_action_or_reset(), perhaps even providing > devm_fwnode_handle_get() for everyone. > > > return ret; > > } ... > > +static void cs42l43_spi_remove(struct platform_device *pdev) > > +{ > > + struct cs42l43_spi *priv = dev_get_drvdata(&pdev->dev); > > + > > + if (is_of_node(priv->ctlr->dev.fwnode)) > > + fwnode_handle_put(priv->ctlr->dev.fwnode); > > +} > > No need as per above. If you send a new patch series where you add > that API, I'll review / ack it immediately. Oh, just realised that this is not a simple fwnode_handle_get()... So, we need to add an action then with devm_add_action(). No need for a separate patch.
diff --git a/drivers/spi/spi-cs42l43.c b/drivers/spi/spi-cs42l43.c index b24190526ce9..1765d752e341 100644 --- a/drivers/spi/spi-cs42l43.c +++ b/drivers/spi/spi-cs42l43.c @@ -218,6 +218,7 @@ static int cs42l43_spi_probe(struct platform_device *pdev) return -ENOMEM; spi_controller_set_devdata(priv->ctlr, priv); + platform_set_drvdata(pdev, priv); priv->dev = &pdev->dev; priv->regmap = cs42l43->regmap; @@ -228,12 +229,6 @@ static int cs42l43_spi_probe(struct platform_device *pdev) priv->ctlr->transfer_one = cs42l43_transfer_one; priv->ctlr->set_cs = cs42l43_set_cs; priv->ctlr->max_transfer_size = cs42l43_spi_max_length; - - if (is_of_node(fwnode)) - fwnode = fwnode_get_named_child_node(fwnode, "spi"); - - device_set_node(&priv->ctlr->dev, fwnode); - priv->ctlr->mode_bits = SPI_3WIRE | SPI_MODE_X_MASK; priv->ctlr->flags = SPI_CONTROLLER_HALF_DUPLEX; priv->ctlr->bits_per_word_mask = SPI_BPW_MASK(8) | SPI_BPW_MASK(16) | @@ -257,14 +252,34 @@ static int cs42l43_spi_probe(struct platform_device *pdev) regmap_write(priv->regmap, CS42L43_SPI_CONFIG3, 0); regmap_write(priv->regmap, CS42L43_SPI_CONFIG4, CS42L43_SPI_STALL_ENA_MASK); + if (is_of_node(fwnode)) + fwnode = fwnode_get_named_child_node(fwnode, "spi"); + + device_set_node(&priv->ctlr->dev, fwnode); + ret = devm_spi_register_controller(priv->dev, priv->ctlr); if (ret) { dev_err(priv->dev, "Failed to register SPI controller: %d\n", ret); + goto of_node_err; } + return 0; + +of_node_err: + if (is_of_node(fwnode)) + fwnode_handle_put(fwnode); + return ret; } +static void cs42l43_spi_remove(struct platform_device *pdev) +{ + struct cs42l43_spi *priv = dev_get_drvdata(&pdev->dev); + + if (is_of_node(priv->ctlr->dev.fwnode)) + fwnode_handle_put(priv->ctlr->dev.fwnode); +} + static const struct platform_device_id cs42l43_spi_id_table[] = { { "cs42l43-spi", }, {} @@ -276,6 +291,7 @@ static struct platform_driver cs42l43_spi_driver = { .name = "cs42l43-spi", }, .probe = cs42l43_spi_probe, + .remove_new = cs42l43_spi_remove, .id_table = cs42l43_spi_id_table, }; module_platform_driver(cs42l43_spi_driver);
As we get a child node in the of case, we should also clean up the references to this, add code for this. Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com> Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com> --- drivers/spi/spi-cs42l43.c | 28 ++++++++++++++++++++++------ 1 file changed, 22 insertions(+), 6 deletions(-)