Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   Chris Packham <Chris.Packham@alliedtelesis.co.nz>
To:     Richard Weinberger <richard.weinberger@gmail.com>
CC:     David Woodhouse <dwmw2@infradead.org>,
        Brian Norris <computersforpeace@gmail.com>,
        Marek Vasut <marek.vasut@gmail.com>,
        "Miquel Raynal" <miquel.raynal@bootlin.com>,
        Richard Weinberger <richard@nod.at>,
        Vignesh Raghavendra <vigneshr@ti.com>,
        "linux-mtd@lists.infradead.org" <linux-mtd@lists.infradead.org>,
        LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 1/2] mtd: concat: refactor concat_lock/concat_unlock
Thread-Topic: [PATCH 1/2] mtd: concat: refactor concat_lock/concat_unlock
Thread-Index: AQHVEDJo4rpNy/YxbUOF1g0LZpyjfw==
Date:   Wed, 22 May 2019 20:53:36 +0000
Message-ID: <86adfe1f5a18492fbdf4bbe26ca05a93@svr-chch-ex1.atlnz.lc>
References: <20190522000753.13300-1-chris.packham@alliedtelesis.co.nz>
 <CAFLxGvzvAdhmNOaNmPCRXUR9GGgaQ1n2HuRLLCb4Nj-tUrm5yQ@mail.gmail.com>
Accept-Language: en-NZ, en-US
Content-Language: en-US
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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