| Message ID | 20190522000753.13300-1-chris.packham@alliedtelesis.co.nz |
|---|---|
| State | Superseded |
| Delegated to: | Richard Weinberger |
| Headers | show |
| Series | [1/2] mtd: concat: refactor concat_lock/concat_unlock | expand |
On Wed, May 22, 2019 at 2:08 AM Chris Packham <chris.packham@alliedtelesis.co.nz> wrote: > > concat_lock() and concat_unlock() only differed in terms of the mtd_xx > operation they called. Refactor them to use a common helper function and > pass mtd_lock or mtd_unlock as an argument. > > Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz> > --- > drivers/mtd/mtdconcat.c | 41 +++++++++-------------------------------- > 1 file changed, 9 insertions(+), 32 deletions(-) > > diff --git a/drivers/mtd/mtdconcat.c b/drivers/mtd/mtdconcat.c > index cbc5925e6440..9514cd2db63c 100644 > --- a/drivers/mtd/mtdconcat.c > +++ b/drivers/mtd/mtdconcat.c > @@ -451,7 +451,8 @@ static int concat_erase(struct mtd_info *mtd, struct erase_info *instr) > return err; > } > > -static int concat_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len) > +static int __concat_xxlock(struct mtd_info *mtd, loff_t ofs, uint64_t len, > + int (*mtd_op)(struct mtd_info *mtd, loff_t ofs, uint64_t len)) > { > struct mtd_concat *concat = CONCAT(mtd); > int i, err = -EINVAL; > @@ -470,7 +471,7 @@ static int concat_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len) > else > size = len; > > - err = mtd_lock(subdev, ofs, size); > + err = mtd_op(subdev, ofs, size); > if (err) > break; > > @@ -485,38 +486,14 @@ static int concat_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len) > return err; > } > > -static int concat_unlock(struct mtd_info *mtd, loff_t ofs, uint64_t len) > +static int concat_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len) > { > - struct mtd_concat *concat = CONCAT(mtd); > - int i, err = 0; > - > - for (i = 0; i < concat->num_subdev; i++) { > - struct mtd_info *subdev = concat->subdev[i]; > - uint64_t size; > - > - if (ofs >= subdev->size) { > - size = 0; > - ofs -= subdev->size; > - continue; > - } > - if (ofs + len > subdev->size) > - size = subdev->size - ofs; > - else > - size = len; > - > - err = mtd_unlock(subdev, ofs, size); > - if (err) > - break; > - > - len -= size; > - if (len == 0) > - break; > - > - err = -EINVAL; > - ofs = 0; > - } > + return __concat_xxlock(mtd, ofs, len, mtd_lock); > +} > > - return err; > +static int concat_unlock(struct mtd_info *mtd, loff_t ofs, uint64_t len) > +{ > + return __concat_xxlock(mtd, ofs, len, mtd_unlock); > } > > static void concat_sync(struct mtd_info *mtd) Not sure if it passing a function pointer is worth it. bool is_lock would also do it. But this is a matter of taste, I guess. :) Reviewed-by: Richard Weinberger <richard@nod.at>
On 23/05/19 8:30 AM, Richard Weinberger wrote: > On Wed, May 22, 2019 at 2:08 AM Chris Packham > <chris.packham@alliedtelesis.co.nz> wrote: >> >> concat_lock() and concat_unlock() only differed in terms of the mtd_xx >> operation they called. Refactor them to use a common helper function and >> pass mtd_lock or mtd_unlock as an argument. >> >> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz> >> --- >> drivers/mtd/mtdconcat.c | 41 +++++++++-------------------------------- >> 1 file changed, 9 insertions(+), 32 deletions(-) >> >> diff --git a/drivers/mtd/mtdconcat.c b/drivers/mtd/mtdconcat.c >> index cbc5925e6440..9514cd2db63c 100644 >> --- a/drivers/mtd/mtdconcat.c >> +++ b/drivers/mtd/mtdconcat.c >> @@ -451,7 +451,8 @@ static int concat_erase(struct mtd_info *mtd, struct erase_info *instr) >> return err; >> } >> >> -static int concat_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len) >> +static int __concat_xxlock(struct mtd_info *mtd, loff_t ofs, uint64_t len, >> + int (*mtd_op)(struct mtd_info *mtd, loff_t ofs, uint64_t len)) >> { >> struct mtd_concat *concat = CONCAT(mtd); >> int i, err = -EINVAL; >> @@ -470,7 +471,7 @@ static int concat_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len) >> else >> size = len; >> >> - err = mtd_lock(subdev, ofs, size); >> + err = mtd_op(subdev, ofs, size); >> if (err) >> break; >> >> @@ -485,38 +486,14 @@ static int concat_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len) >> return err; >> } >> >> -static int concat_unlock(struct mtd_info *mtd, loff_t ofs, uint64_t len) >> +static int concat_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len) >> { >> - struct mtd_concat *concat = CONCAT(mtd); >> - int i, err = 0; >> - >> - for (i = 0; i < concat->num_subdev; i++) { >> - struct mtd_info *subdev = concat->subdev[i]; >> - uint64_t size; >> - >> - if (ofs >= subdev->size) { >> - size = 0; >> - ofs -= subdev->size; >> - continue; >> - } >> - if (ofs + len > subdev->size) >> - size = subdev->size - ofs; >> - else >> - size = len; >> - >> - err = mtd_unlock(subdev, ofs, size); >> - if (err) >> - break; >> - >> - len -= size; >> - if (len == 0) >> - break; >> - >> - err = -EINVAL; >> - ofs = 0; >> - } >> + return __concat_xxlock(mtd, ofs, len, mtd_lock); >> +} >> >> - return err; >> +static int concat_unlock(struct mtd_info *mtd, loff_t ofs, uint64_t len) >> +{ >> + return __concat_xxlock(mtd, ofs, len, mtd_unlock); >> } >> >> static void concat_sync(struct mtd_info *mtd) > > Not sure if it passing a function pointer is worth it. bool is_lock would > also do it. But this is a matter of taste, I guess. :) I briefly considered that. But since mtd_lock(), mtd_unlock() and mtd_is_locked() all take the same arguments I figured it'd benefit from some type checking. A bool wouldn't work (assuming I can convince you about 2/2) but an enum mtd_op or int flags would do the trick if you want me to change it. > > Reviewed-by: Richard Weinberger <richard@nod.at> >
diff --git a/drivers/mtd/mtdconcat.c b/drivers/mtd/mtdconcat.c index cbc5925e6440..9514cd2db63c 100644 --- a/drivers/mtd/mtdconcat.c +++ b/drivers/mtd/mtdconcat.c @@ -451,7 +451,8 @@ static int concat_erase(struct mtd_info *mtd, struct erase_info *instr) return err; } -static int concat_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len) +static int __concat_xxlock(struct mtd_info *mtd, loff_t ofs, uint64_t len, + int (*mtd_op)(struct mtd_info *mtd, loff_t ofs, uint64_t len)) { struct mtd_concat *concat = CONCAT(mtd); int i, err = -EINVAL; @@ -470,7 +471,7 @@ static int concat_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len) else size = len; - err = mtd_lock(subdev, ofs, size); + err = mtd_op(subdev, ofs, size); if (err) break; @@ -485,38 +486,14 @@ static int concat_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len) return err; } -static int concat_unlock(struct mtd_info *mtd, loff_t ofs, uint64_t len) +static int concat_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len) { - struct mtd_concat *concat = CONCAT(mtd); - int i, err = 0; - - for (i = 0; i < concat->num_subdev; i++) { - struct mtd_info *subdev = concat->subdev[i]; - uint64_t size; - - if (ofs >= subdev->size) { - size = 0; - ofs -= subdev->size; - continue; - } - if (ofs + len > subdev->size) - size = subdev->size - ofs; - else - size = len; - - err = mtd_unlock(subdev, ofs, size); - if (err) - break; - - len -= size; - if (len == 0) - break; - - err = -EINVAL; - ofs = 0; - } + return __concat_xxlock(mtd, ofs, len, mtd_lock); +} - return err; +static int concat_unlock(struct mtd_info *mtd, loff_t ofs, uint64_t len) +{ + return __concat_xxlock(mtd, ofs, len, mtd_unlock); } static void concat_sync(struct mtd_info *mtd)
concat_lock() and concat_unlock() only differed in terms of the mtd_xx operation they called. Refactor them to use a common helper function and pass mtd_lock or mtd_unlock as an argument. Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz> --- drivers/mtd/mtdconcat.c | 41 +++++++++-------------------------------- 1 file changed, 9 insertions(+), 32 deletions(-)