[v2,02/15] spi: Drop duplicate IDR allocation code in spi_register_controller()
Commit Message
Refactor spi_register_controller() to drop duplicate IDR allocation.
Instead of if-else-if branching use two sequential if:s, which allows
to re-use the logic of IDR allocation in all cases.
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
drivers/spi/spi.c | 50 ++++++++++++++++++++++-------------------------
1 file changed, 23 insertions(+), 27 deletions(-)
Comments
On Mon, Jul 10, 2023 at 06:49:19PM +0300, Andy Shevchenko wrote:
> Refactor spi_register_controller() to drop duplicate IDR allocation.
> Instead of if-else-if branching use two sequential if:s, which allows
> to re-use the logic of IDR allocation in all cases.
For legibility this should have been split into a separate factoring out
of the shared code and rewriting of the logic, that'd make it trivial to
review.
> - mutex_lock(&board_lock);
> - id = idr_alloc(&spi_master_idr, ctlr, first_dynamic,
> - 0, GFP_KERNEL);
> - mutex_unlock(&board_lock);
> - if (WARN(id < 0, "couldn't get idr"))
> - return id;
> - ctlr->bus_num = id;
> + status = spi_controller_id_alloc(ctlr, first_dynamic, 0);
> + if (status)
> + return status;
The original does not do the remapping of return codes that the previous
two copies do...
On Mon, Jul 10, 2023 at 06:09:00PM +0100, Mark Brown wrote:
> On Mon, Jul 10, 2023 at 06:49:19PM +0300, Andy Shevchenko wrote:
>
> > Refactor spi_register_controller() to drop duplicate IDR allocation.
> > Instead of if-else-if branching use two sequential if:s, which allows
> > to re-use the logic of IDR allocation in all cases.
>
> For legibility this should have been split into a separate factoring out
> of the shared code and rewriting of the logic, that'd make it trivial to
> review.
Should I do that for v3?
> > - mutex_lock(&board_lock);
> > - id = idr_alloc(&spi_master_idr, ctlr, first_dynamic,
> > - 0, GFP_KERNEL);
> > - mutex_unlock(&board_lock);
> > - if (WARN(id < 0, "couldn't get idr"))
> > - return id;
> > - ctlr->bus_num = id;
> > + status = spi_controller_id_alloc(ctlr, first_dynamic, 0);
> > + if (status)
> > + return status;
>
> The original does not do the remapping of return codes that the previous
> two copies do...
Yes, I had to mention this in the commit message that in my opinion this makes
no difference. With the dynamically allocated aliases the absence of the slot
has the same effect as in the other cases.
@@ -3081,6 +3081,20 @@ static int spi_controller_check_ops(struct spi_controller *ctlr)
return 0;
}
+/* Allocate dynamic bus number using Linux idr */
+static int spi_controller_id_alloc(struct spi_controller *ctlr, int start, int end)
+{
+ int id;
+
+ mutex_lock(&board_lock);
+ id = idr_alloc(&spi_master_idr, ctlr, start, end, GFP_KERNEL);
+ mutex_unlock(&board_lock);
+ if (WARN(id < 0, "couldn't get idr"))
+ return id == -ENOSPC ? -EBUSY : id;
+ ctlr->bus_num = id;
+ return 0;
+}
+
/**
* spi_register_controller - register SPI master or slave controller
* @ctlr: initialized master, originally from spi_alloc_master() or
@@ -3108,8 +3122,8 @@ int spi_register_controller(struct spi_controller *ctlr)
{
struct device *dev = ctlr->dev.parent;
struct boardinfo *bi;
+ int first_dynamic;
int status;
- int id, first_dynamic;
if (!dev)
return -ENODEV;
@@ -3122,27 +3136,13 @@ int spi_register_controller(struct spi_controller *ctlr)
if (status)
return status;
+ if (ctlr->bus_num < 0)
+ ctlr->bus_num = of_alias_get_id(ctlr->dev.of_node, "spi");
if (ctlr->bus_num >= 0) {
/* Devices with a fixed bus num must check-in with the num */
- mutex_lock(&board_lock);
- id = idr_alloc(&spi_master_idr, ctlr, ctlr->bus_num,
- ctlr->bus_num + 1, GFP_KERNEL);
- mutex_unlock(&board_lock);
- if (WARN(id < 0, "couldn't get idr"))
- return id == -ENOSPC ? -EBUSY : id;
- ctlr->bus_num = id;
- } else {
- /* Allocate dynamic bus number using Linux idr */
- id = of_alias_get_id(ctlr->dev.of_node, "spi");
- if (id >= 0) {
- ctlr->bus_num = id;
- mutex_lock(&board_lock);
- id = idr_alloc(&spi_master_idr, ctlr, ctlr->bus_num,
- ctlr->bus_num + 1, GFP_KERNEL);
- mutex_unlock(&board_lock);
- if (WARN(id < 0, "couldn't get idr"))
- return id == -ENOSPC ? -EBUSY : id;
- }
+ status = spi_controller_id_alloc(ctlr, ctlr->bus_num, ctlr->bus_num + 1);
+ if (status)
+ return status;
}
if (ctlr->bus_num < 0) {
first_dynamic = of_alias_get_highest_id("spi");
@@ -3151,13 +3151,9 @@ int spi_register_controller(struct spi_controller *ctlr)
else
first_dynamic++;
- mutex_lock(&board_lock);
- id = idr_alloc(&spi_master_idr, ctlr, first_dynamic,
- 0, GFP_KERNEL);
- mutex_unlock(&board_lock);
- if (WARN(id < 0, "couldn't get idr"))
- return id;
- ctlr->bus_num = id;
+ status = spi_controller_id_alloc(ctlr, first_dynamic, 0);
+ if (status)
+ return status;
}
ctlr->bus_lock_flag = 0;
init_completion(&ctlr->xfer_completion);