Message ID | 20230710154932.68377-3-andriy.shevchenko@linux.intel.com (mailing list archive) |
---|---|
State | Superseded |
Headers | show |
Series | spi: Header and core clean up and refactoring | expand |
Context | Check | Description |
---|---|---|
conchuod/cover_letter | success | Series has a cover letter |
conchuod/tree_selection | success | Guessed tree name to be for-next at HEAD e8605e8fdf42 |
conchuod/fixes_present | success | Fixes tag not required for -next series |
conchuod/maintainers_pattern | success | MAINTAINERS pattern errors before the patch: 4 and now 4 |
conchuod/verify_signedoff | success | Signed-off-by tag matches author and committer |
conchuod/kdoc | success | Errors and warnings before: 0 this patch: 0 |
conchuod/build_rv64_clang_allmodconfig | success | Errors and warnings before: 9 this patch: 9 |
conchuod/module_param | success | Was 0 now: 0 |
conchuod/build_rv64_gcc_allmodconfig | success | Errors and warnings before: 9 this patch: 9 |
conchuod/build_rv32_defconfig | success | Build OK |
conchuod/dtb_warn_rv64 | success | Errors and warnings before: 3 this patch: 3 |
conchuod/header_inline | success | No static functions without inline keyword in header files |
conchuod/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 77 lines checked |
conchuod/build_rv64_nommu_k210_defconfig | success | Build OK |
conchuod/verify_fixes | success | No Fixes tag |
conchuod/build_rv64_nommu_virt_defconfig | success | Build OK |
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.
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index 8f3282a71c63..6d74218cf38e 100644 --- a/drivers/spi/spi.c +++ b/drivers/spi/spi.c @@ -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);
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(-)