Re: [PATCH 2/3] mtd: mediatek: driver for MTK Smart Device Gen1 NAND

Jorge Ramirez-Ortiz
2016-03-08T17:17:31Z

On 03/08/2016 11:24 AM, Boris Brezillon wrote:

Hi Boris,

>> +
>> > +static int mtk_nfc_subpage_done(struct mtk_nfc_host *host, int sectors)
>> > +{
>> > + unsigned long timeout = msecs_to_jiffies(MTK_TIMEOUT);
>> > + u32 val;
>> > +
>> > + timeout += jiffies;
>> > + do {
>> > + val = mtk_nfi_readl(host, MTKSDG1_NFI_BYTELEN);
>> > + val &= CNTR_MASK;
>> > + if (val >= sectors)
>> > + return 0;
>> > + cpu_relax();
>> > +
>> > + } while (time_before(jiffies, timeout));
>> > +
>> > + return -EIO;
>> > +}
>> > +
>> > +static inline int mtk_nfc_data_ready(struct mtk_nfc_host *host)
>> > +{
>> > + unsigned long timeout = msecs_to_jiffies(MTK_TIMEOUT);
>> > + u8 val;
>> > +
>> > + timeout += jiffies;
>> > + do {
>> > + val = mtk_nfi_readw(host, MTKSDG1_NFI_PIO_DIRDY);
>> > + val &= PIO_DI_RDY;
>> > + if (val)
>> > + return 0;
>> > + cpu_relax();
>> > +
>> > + } while (time_before(jiffies, timeout));
>> > +
>> > + /* data _MUST_ not be accessed */
>> > + return -EIO;
>> > +}
> Nitpick: you seem to have a lot of xxx_ready() functions, which are
> pretty much all doing the same thing. Maybe it's worth creating a
> single which would take a register offset and status flags, instead of
> adding one function per event.
>
> Something like:
>
> static inline int mtk_nfc_ready(struct mtk_nfc_host *host, int reg,
> u32 flags)
> {
> /* implem */
> }
>

yes I did notice that as well (and I hate the ugliness) but the issue is that
each of them not only will access different registers but also in different
lengths (readl/readw) and apply different masks and expect a different exit
condition. anyway I'll put some more thought in.

ah thanks for all the other comments in the RFC (both device tree and driver). I
will start working on v2 now.

cheers