Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1584574 > unrolled thread

[PATCH v2 0/3] mtd: nand: Rework/cleanup the Atmel NAND driver

Back to article view | Back to linux.kernel

Contents

  [PATCH v2 0/3] mtd: nand: Rework/cleanup the Atmel NAND driver Boris Brezillon <boris.brezillon@free-electrons.com> - 2017-02-20 13:30 +0100
    [PATCH v2 3/3] mtd: nand: Remove unused chip->write_page() hook Boris Brezillon <boris.brezillon@free-electrons.com> - 2017-02-20 13:30 +0100
    Re: [PATCH v2 1/3] mtd: nand: Cleanup/rework the atmel_nand driver Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-02-20 21:30 +0100
      Re: [PATCH v2 1/3] mtd: nand: Cleanup/rework the atmel_nand driver Boris Brezillon <boris.brezillon@free-electrons.com> - 2017-02-20 21:40 +0100
        Re: [PATCH v2 1/3] mtd: nand: Cleanup/rework the atmel_nand driver Boris Brezillon <boris.brezillon@free-electrons.com> - 2017-02-20 22:00 +0100
          Re: [PATCH v2 1/3] mtd: nand: Cleanup/rework the atmel_nand driver Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-02-21 00:50 +0100
            Re: [PATCH v2 1/3] mtd: nand: Cleanup/rework the atmel_nand driver Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-02-21 01:00 +0100
              Re: [PATCH v2 1/3] mtd: nand: Cleanup/rework the atmel_nand driver Boris Brezillon <boris.brezillon@free-electrons.com> - 2017-02-21 09:10 +0100
                Re: [PATCH v2 1/3] mtd: nand: Cleanup/rework the atmel_nand driver Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-02-21 11:10 +0100
                  Re: [PATCH v2 1/3] mtd: nand: Cleanup/rework the atmel_nand driver Boris Brezillon <boris.brezillon@free-electrons.com> - 2017-02-21 11:30 +0100
                    Re: [PATCH v2 1/3] mtd: nand: Cleanup/rework the atmel_nand driver Nicolas Ferre <nicolas.ferre@atmel.com> - 2017-02-21 11:50 +0100
                    Re: [PATCH v2 1/3] mtd: nand: Cleanup/rework the atmel_nand driver Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-02-21 12:10 +0100
                      Re: [PATCH v2 1/3] mtd: nand: Cleanup/rework the atmel_nand driver Alexandre Belloni <alexandre.belloni@free-electrons.com> - 2017-02-21 12:30 +0100
                        Re: [PATCH v2 1/3] mtd: nand: Cleanup/rework the atmel_nand driver Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-02-21 17:10 +0100
                          Re: [PATCH v2 1/3] mtd: nand: Cleanup/rework the atmel_nand driver Alexandre Belloni <alexandre.belloni@free-electrons.com> - 2017-02-21 17:30 +0100
                            Re: [PATCH v2 1/3] mtd: nand: Cleanup/rework the atmel_nand driver Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-02-21 17:40 +0100
                              Re: [PATCH v2 1/3] mtd: nand: Cleanup/rework the atmel_nand driver Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-02-21 17:50 +0100
                                Re: [PATCH v2 1/3] mtd: nand: Cleanup/rework the atmel_nand driver Alexandre Belloni <alexandre.belloni@free-electrons.com> - 2017-02-21 18:20 +0100
                                  Re: [PATCH v2 1/3] mtd: nand: Cleanup/rework the atmel_nand driver Håvard Skinnemoen <hskinnemoen@gmail.com> - 2017-02-24 06:20 +0100
                                    Re: [PATCH v2 1/3] mtd: nand: Cleanup/rework the atmel_nand driver Boris Brezillon <boris.brezillon@free-electrons.com> - 2017-02-24 09:30 +0100
                                      Re: [PATCH v2 1/3] mtd: nand: Cleanup/rework the atmel_nand driver Boris Brezillon <boris.brezillon@free-electrons.com> - 2017-02-24 10:00 +0100
                                        Re: [PATCH v2 1/3] mtd: nand: Cleanup/rework the atmel_nand driver Boris Brezillon <boris.brezillon@free-electrons.com> - 2017-02-24 10:40 +0100
                                        Re: [PATCH v2 1/3] mtd: nand: Cleanup/rework the atmel_nand driver Alexandre Belloni <alexandre.belloni@free-electrons.com> - 2017-02-24 11:00 +0100
                                          Re: [PATCH v2 1/3] mtd: nand: Cleanup/rework the atmel_nand driver Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-02-24 12:50 +0100
                                        Re: [PATCH v2 1/3] mtd: nand: Cleanup/rework the atmel_nand driver Hans-Christian Noren Egtvedt <egtvedt@samfundet.no> - 2017-02-24 11:10 +0100
                                      Re: [PATCH v2 1/3] mtd: nand: Cleanup/rework the atmel_nand driver Hans-Christian Noren Egtvedt <egtvedt@samfundet.no> - 2017-02-24 10:40 +0100
                                    Re: [PATCH v2 1/3] mtd: nand: Cleanup/rework the atmel_nand driver Alexandre Belloni <alexandre.belloni@free-electrons.com> - 2017-02-24 10:30 +0100
                                    Re: [PATCH v2 1/3] mtd: nand: Cleanup/rework the atmel_nand driver Hans-Christian Noren Egtvedt <egtvedt@samfundet.no> - 2017-02-24 10:40 +0100
                              Re: [PATCH v2 1/3] mtd: nand: Cleanup/rework the atmel_nand driver Alexandre Belloni <alexandre.belloni@free-electrons.com> - 2017-02-21 18:10 +0100
                      Re: [PATCH v2 1/3] mtd: nand: Cleanup/rework the atmel_nand driver Boris Brezillon <boris.brezillon@free-electrons.com> - 2017-02-21 12:30 +0100
                        Re: [PATCH v2 1/3] mtd: nand: Cleanup/rework the atmel_nand driver Nicolas Ferre <nicolas.ferre@microchip.com> - 2017-02-21 14:50 +0100
                        Re: [PATCH v2 1/3] mtd: nand: Cleanup/rework the atmel_nand driver Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-02-21 17:00 +0100
                          Re: [PATCH v2 1/3] mtd: nand: Cleanup/rework the atmel_nand driver Alexandre Belloni <alexandre.belloni@free-electrons.com> - 2017-02-21 17:20 +0100
                      Re: [PATCH v2 1/3] mtd: nand: Cleanup/rework the atmel_nand driver Russell King - ARM Linux <linux@armlinux.org.uk> - 2017-02-21 15:00 +0100
    Re: [PATCH v2 1/3] mtd: nand: Cleanup/rework the atmel_nand driver Nicolas Ferre <nicolas.ferre@microchip.com> - 2017-02-21 14:10 +0100

Page 1 of 2  [1] 2  Next page →

#1584574 — [PATCH v2 0/3] mtd: nand: Rework/cleanup the Atmel NAND driver

This is a complete rewrite of the driver whose main purpose is to
support the new DT representation where the NAND controller node is now
really visible in the DT and appears under the EBI bus. With this new
representation, we can add other devices under the EBI bus without
risking pinmuxing conflicts (the NAND controller is under the EBI
bus logic and as such, share some of its pins with other devices
connected on this bus).

Even though the goal of this rework was not necessarily to add new
features, the new driver has been designed with this in mind. With a
clearer separation between the different blocks and different IP
revisions, adding new functionalities should be easier (we already
have plans to support SMC timing configuration so that we no longer
have to rely on the configuration done by the bootloader/bootstrap).

Also note that we no longer have a custom ->cmdfunc() implementation,
which means we can now benefit from new features added in the core
implementation for free (support for new NAND operations for example).

The last thing that we gain with this rework is support for multi-chips
and multi-dies chips, thanks to the clean NAND controller <-> NAND
devices representation.

This new driver has been tested on several platforms (at91sam9261,
at91sam9g45, at91sam9x5, sama5d3 and sama5d4) to make sure it did not
introduce regressions, and it's worth mentioning that old bindings are
still supported (which partly explain the positive diffstat).

Regards,

Boris

Changes since v1:
- change function/structure prefixes (asked by Nicolas)
- drop applied patches
- use new GPIO helpers
- set ->chip_delay to 40 as done in the old driver (reported by Nicolas)
- rework read_page to improve perfs
- add a better commit message to patch 2

Boris Brezillon (3):
  mtd: nand: Cleanup/rework the atmel_nand driver
  mtd: nand: atmel: Document the new DT bindings
  mtd: nand: Remove unused chip->write_page() hook

 .../devicetree/bindings/mtd/atmel-nand.txt         |  107 +-
 MAINTAINERS                                        |    2 +-
 drivers/mtd/nand/Makefile                          |    2 +-
 drivers/mtd/nand/atmel/Makefile                    |    4 +
 drivers/mtd/nand/atmel/nand-controller.c           | 2269 ++++++++++++++++++
 drivers/mtd/nand/atmel/pmecc.c                     | 1020 ++++++++
 drivers/mtd/nand/atmel/pmecc.h                     |   73 +
 drivers/mtd/nand/atmel_nand.c                      | 2479 --------------------
 drivers/mtd/nand/atmel_nand_ecc.h                  |  163 --
 drivers/mtd/nand/atmel_nand_nfc.h                  |  103 -
 drivers/mtd/nand/nand_base.c                       |   10 +-
 include/linux/mtd/nand.h                           |    4 -
 12 files changed, 3478 insertions(+), 2758 deletions(-)
 create mode 100644 drivers/mtd/nand/atmel/Makefile
 create mode 100644 drivers/mtd/nand/atmel/nand-controller.c
 create mode 100644 drivers/mtd/nand/atmel/pmecc.c
 create mode 100644 drivers/mtd/nand/atmel/pmecc.h
 delete mode 100644 drivers/mtd/nand/atmel_nand.c
 delete mode 100644 drivers/mtd/nand/atmel_nand_ecc.h
 delete mode 100644 drivers/mtd/nand/atmel_nand_nfc.h

-- 
2.7.4

[toc] | [next] | [standalone]

#1584575 — [PATCH v2 3/3] mtd: nand: Remove unused chip->write_page() hook

The last/only user of the chip->write_page() hook (the Atmel NAND
controller driver) has been reworked and is no longer specifying a custom
->write_page() implementation.
Drop this hook before someone else start abusing it.

Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
---
 drivers/mtd/nand/nand_base.c | 10 ++++------
 include/linux/mtd/nand.h     |  4 ----
 2 files changed, 4 insertions(+), 10 deletions(-)

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index ec1c28aaaf23..c8894f31392e 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -2839,9 +2839,10 @@ static int nand_do_write_ops(struct mtd_info *mtd, loff_t to,
 			/* We still need to erase leftover OOB data */
 			memset(chip->oob_poi, 0xff, mtd->oobsize);
 		}
-		ret = chip->write_page(mtd, chip, column, bytes, wbuf,
-					oob_required, page, cached,
-					(ops->mode == MTD_OPS_RAW));
+
+		ret = nand_write_page(mtd, chip, column, bytes, wbuf,
+				      oob_required, page, cached,
+				      (ops->mode == MTD_OPS_RAW));
 		if (ret)
 			break;
 
@@ -4623,9 +4624,6 @@ int nand_scan_tail(struct mtd_info *mtd)
 		}
 	}
 
-	if (!chip->write_page)
-		chip->write_page = nand_write_page;
-
 	/*
 	 * Check ECC mode, default to software if 3byte/512byte hardware ECC is
 	 * selected and we have 256 byte pagesize fallback to software ECC
diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
index c5f3a012ae62..9d51dee53be4 100644
--- a/include/linux/mtd/nand.h
+++ b/include/linux/mtd/nand.h
@@ -818,7 +818,6 @@ nand_get_sdr_timings(const struct nand_data_interface *conf)
  * @errstat:		[OPTIONAL] hardware specific function to perform
  *			additional error status checks (determine if errors are
  *			correctable).
- * @write_page:		[REPLACEABLE] High-level page write function
  */
 
 struct nand_chip {
@@ -843,9 +842,6 @@ struct nand_chip {
 	int (*scan_bbt)(struct mtd_info *mtd);
 	int (*errstat)(struct mtd_info *mtd, struct nand_chip *this, int state,
 			int status, int page);
-	int (*write_page)(struct mtd_info *mtd, struct nand_chip *chip,
-			uint32_t offset, int data_len, const uint8_t *buf,
-			int oob_required, int page, int cached, int raw);
 	int (*onfi_set_features)(struct mtd_info *mtd, struct nand_chip *chip,
 			int feature_addr, uint8_t *subfeature_para);
 	int (*onfi_get_features)(struct mtd_info *mtd, struct nand_chip *chip,
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1584883 — Re: [PATCH v2 1/3] mtd: nand: Cleanup/rework the atmel_nand driver

On Mon, Feb 20, 2017 at 2:28 PM, Boris Brezillon
<boris.brezillon@free-electrons.com> wrote:

>  drivers/mtd/nand/atmel/nand-controller.c | 2269 +++++++++++++++++++++++++++
>  drivers/mtd/nand/atmel_nand.c            | 2479 ------------------------------

Does -M -C help you?
At least it would help reviewers

-- 
With Best Regards,
Andy Shevchenko

[toc] | [prev] | [next] | [standalone]

#1584888 — Re: [PATCH v2 1/3] mtd: nand: Cleanup/rework the atmel_nand driver

On Mon, 20 Feb 2017 22:27:17 +0200
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Mon, Feb 20, 2017 at 2:28 PM, Boris Brezillon
> <boris.brezillon@free-electrons.com> wrote:
> 
> >  drivers/mtd/nand/atmel/nand-controller.c | 2269 +++++++++++++++++++++++++++
> >  drivers/mtd/nand/atmel_nand.c            | 2479 ------------------------------  
> 
> Does -M -C help you?
> At least it would help reviewers
> 

No it doesn't, because files were not just moved around using git mv,
it's a complete rewrite of the driver. IIUC, you're about to review
this submission, or are you just trolling like last time?

[toc] | [prev] | [next] | [standalone]

#1584903 — Re: [PATCH v2 1/3] mtd: nand: Cleanup/rework the atmel_nand driver

On Mon, 20 Feb 2017 21:38:03 +0100
Boris Brezillon <boris.brezillon@free-electrons.com> wrote:

> On Mon, 20 Feb 2017 22:27:17 +0200
> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> 
> > On Mon, Feb 20, 2017 at 2:28 PM, Boris Brezillon
> > <boris.brezillon@free-electrons.com> wrote:
> >   
> > >  drivers/mtd/nand/atmel/nand-controller.c | 2269 +++++++++++++++++++++++++++
> > >  drivers/mtd/nand/atmel_nand.c            | 2479 ------------------------------    
> > 
> > Does -M -C help you?
> > At least it would help reviewers
> >   
> 
> No it doesn't, because files were not just moved around using git mv,
> it's a complete rewrite of the driver. IIUC, you're about to review
> this submission, or are you just trolling like last time?

My bad, I mistaken you with someone else. Sorry for being harsh, but my
explanation stands ;-).

[toc] | [prev] | [next] | [standalone]

#1584964 — Re: [PATCH v2 1/3] mtd: nand: Cleanup/rework the atmel_nand driver

On Mon, Feb 20, 2017 at 10:50 PM, Boris Brezillon
<boris.brezillon@free-electrons.com> wrote:
> On Mon, 20 Feb 2017 21:38:03 +0100
> Boris Brezillon <boris.brezillon@free-electrons.com> wrote:
>
>> On Mon, 20 Feb 2017 22:27:17 +0200
>> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>>
>> > On Mon, Feb 20, 2017 at 2:28 PM, Boris Brezillon
>> > <boris.brezillon@free-electrons.com> wrote:
>> >
>> > >  drivers/mtd/nand/atmel/nand-controller.c | 2269 +++++++++++++++++++++++++++
>> > >  drivers/mtd/nand/atmel_nand.c            | 2479 ------------------------------
>> >
>> > Does -M -C help you?
>> > At least it would help reviewers
>> >
>>
>> No it doesn't, because files were not just moved around using git mv,
>> it's a complete rewrite of the driver. IIUC, you're about to review
>> this submission, or are you just trolling like last time?
>
> My bad, I mistaken you with someone else. Sorry for being harsh, but my
> explanation stands ;-).

No problem. I was asking since it so big and on first glance looks
like a partial copy (I dunno if parameter to -C makes it somehow
useful), though I can't review this. It's too big to me. Sorry I'm
really not trolling, just didn't read commit message carefully.

-- 
With Best Regards,
Andy Shevchenko

[toc] | [prev] | [next] | [standalone]

#1584967 — Re: [PATCH v2 1/3] mtd: nand: Cleanup/rework the atmel_nand driver

On Tue, Feb 21, 2017 at 1:40 AM, Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Mon, Feb 20, 2017 at 10:50 PM, Boris Brezillon
> <boris.brezillon@free-electrons.com> wrote:
>> On Mon, 20 Feb 2017 21:38:03 +0100
>> Boris Brezillon <boris.brezillon@free-electrons.com> wrote:
>>
>>> On Mon, 20 Feb 2017 22:27:17 +0200
>>> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>>>
>>> > On Mon, Feb 20, 2017 at 2:28 PM, Boris Brezillon
>>> > <boris.brezillon@free-electrons.com> wrote:
>>> >
>>> > >  drivers/mtd/nand/atmel/nand-controller.c | 2269 +++++++++++++++++++++++++++
>>> > >  drivers/mtd/nand/atmel_nand.c            | 2479 ------------------------------
>>> >
>>> > Does -M -C help you?
>>> > At least it would help reviewers
>>> >
>>>
>>> No it doesn't, because files were not just moved around using git mv,
>>> it's a complete rewrite of the driver. IIUC, you're about to review
>>> this submission, or are you just trolling like last time?
>>
>> My bad, I mistaken you with someone else. Sorry for being harsh, but my
>> explanation stands ;-).
>
> No problem. I was asking since it so big and on first glance looks
> like a partial copy (I dunno if parameter to -C makes it somehow
> useful), though I can't review this. It's too big to me. Sorry I'm
> really not trolling, just didn't read commit message carefully.

Okay, I very quickly looked into the code, what I noticed
- you like extra parens and empty lines in some cases (not big deal)
- some functions perhaps might have been refactored to have common
pieces in error handling, though I didn't read core carefully.

Most important part I have noticed is a GPIO request.
I didn't get why you almost repeat gpiod_get() in case of platform data?
Shouldn't we have GPIO look up table?
Can we use builtin device properties (for GPIO and/or overall)?


-- 
With Best Regards,
Andy Shevchenko

[toc] | [prev] | [next] | [standalone]

#1585099 — Re: [PATCH v2 1/3] mtd: nand: Cleanup/rework the atmel_nand driver

On Tue, 21 Feb 2017 01:54:37 +0200
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Tue, Feb 21, 2017 at 1:40 AM, Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > On Mon, Feb 20, 2017 at 10:50 PM, Boris Brezillon
> > <boris.brezillon@free-electrons.com> wrote:  
> >> On Mon, 20 Feb 2017 21:38:03 +0100
> >> Boris Brezillon <boris.brezillon@free-electrons.com> wrote:
> >>  
> >>> On Mon, 20 Feb 2017 22:27:17 +0200
> >>> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> >>>  
> >>> > On Mon, Feb 20, 2017 at 2:28 PM, Boris Brezillon
> >>> > <boris.brezillon@free-electrons.com> wrote:
> >>> >  
> >>> > >  drivers/mtd/nand/atmel/nand-controller.c | 2269 +++++++++++++++++++++++++++
> >>> > >  drivers/mtd/nand/atmel_nand.c            | 2479 ------------------------------  
> >>> >
> >>> > Does -M -C help you?
> >>> > At least it would help reviewers
> >>> >  
> >>>
> >>> No it doesn't, because files were not just moved around using git mv,
> >>> it's a complete rewrite of the driver. IIUC, you're about to review
> >>> this submission, or are you just trolling like last time?  
> >>
> >> My bad, I mistaken you with someone else. Sorry for being harsh, but my
> >> explanation stands ;-).  
> >
> > No problem. I was asking since it so big and on first glance looks
> > like a partial copy (I dunno if parameter to -C makes it somehow
> > useful), though I can't review this. It's too big to me. Sorry I'm
> > really not trolling, just didn't read commit message carefully.  
> 
> Okay, I very quickly looked into the code, what I noticed
> - you like extra parens and empty lines in some cases (not big deal)

Can you point specific places where you think these are not needed?

> - some functions perhaps might have been refactored to have common
> pieces in error handling, though I didn't read core carefully.

Again, be more precise.

> 
> Most important part I have noticed is a GPIO request.
> I didn't get why you almost repeat gpiod_get() in case of platform data?
> Shouldn't we have GPIO look up table?
> Can we use builtin device properties (for GPIO and/or overall)?

Sorry but I don't get it. Can give an example of what you'd like me to
do?

Thanks,

Boris

[toc] | [prev] | [next] | [standalone]

#1585182 — Re: [PATCH v2 1/3] mtd: nand: Cleanup/rework the atmel_nand driver

On Tue, Feb 21, 2017 at 10:06 AM, Boris Brezillon
<boris.brezillon@free-electrons.com> wrote:
> On Tue, 21 Feb 2017 01:54:37 +0200
> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>
>> On Tue, Feb 21, 2017 at 1:40 AM, Andy Shevchenko
>> <andy.shevchenko@gmail.com> wrote:
>> > On Mon, Feb 20, 2017 at 10:50 PM, Boris Brezillon
>> > <boris.brezillon@free-electrons.com> wrote:
>> >> On Mon, 20 Feb 2017 21:38:03 +0100
>> >> Boris Brezillon <boris.brezillon@free-electrons.com> wrote:
>> >>
>> >>> On Mon, 20 Feb 2017 22:27:17 +0200
>> >>> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>> >>>
>> >>> > On Mon, Feb 20, 2017 at 2:28 PM, Boris Brezillon
>> >>> > <boris.brezillon@free-electrons.com> wrote:
>> >>> >
>> >>> > >  drivers/mtd/nand/atmel/nand-controller.c | 2269 +++++++++++++++++++++++++++
>> >>> > >  drivers/mtd/nand/atmel_nand.c            | 2479 ------------------------------
>> >>> >
>> >>> > Does -M -C help you?
>> >>> > At least it would help reviewers
>> >>> >
>> >>>
>> >>> No it doesn't, because files were not just moved around using git mv,
>> >>> it's a complete rewrite of the driver. IIUC, you're about to review
>> >>> this submission, or are you just trolling like last time?
>> >>
>> >> My bad, I mistaken you with someone else. Sorry for being harsh, but my
>> >> explanation stands ;-).
>> >
>> > No problem. I was asking since it so big and on first glance looks
>> > like a partial copy (I dunno if parameter to -C makes it somehow
>> > useful), though I can't review this. It's too big to me. Sorry I'm
>> > really not trolling, just didn't read commit message carefully.
>>
>> Okay, I very quickly looked into the code, what I noticed
>> - you like extra parens and empty lines in some cases (not big deal)
>
> Can you point specific places where you think these are not needed?

1. For example,

#define ATMEL_NFC_CMD(pos, cmd)                        ((cmd) <<
(((pos) * 8) + 2))

 *events ^= (status & *events);

 (((x) * 0x4) + 0x28)

  memset(&si[1], 0, sizeof(s16) * ((2 * strength) - 1));

Perhaps more in the code. I'm not a LISP programmer.

2. For empty lines it's solely matter of style, I don't care. My motto
"less LOC better, but keep common sense in mind".

>> - some functions perhaps might have been refactored to have common
>> pieces in error handling, though I didn't read core carefully.
>
> Again, be more precise.

3. I don't remember anymore, sorry. Something I would refactor.

>> Most important part I have noticed is a GPIO request.
>> I didn't get why you almost repeat gpiod_get() in case of platform data?
>> Shouldn't we have GPIO look up table?
>> Can we use builtin device properties (for GPIO and/or overall)?
>
> Sorry but I don't get it. Can give an example of what you'd like me to
> do?
>

4. First of all, why do you need this function in the first place?

+struct gpio_desc *
+atmel_nand_pdata_get_gpio(struct atmel_nand_controller *nc, int gpioid,
+                         const char *name, bool active_low,
+                         enum gpiod_flags flags)

5. BIT() macro:

   const unsigned int k = 1 << deg(poly);
   unsigned int nn = (1 << mm) - 1;

6. Why this casting (unsigned int) ?

 dev_dbg(pmecc->dev,
                       "Bit flip in %s area, byte %d: 0x%02x -> 0x%02x\n",
                       area, byte, *ptr, (unsigned int)(*ptr ^ BIT(bit)));

7. Question to all that distribution or whatever functions, don't you
have a common helper? Or each vendor requires different logic behind
it?

8. Have you checked what kernel library provides?

And I believe there are still issues like those. After, who is on
topic, might even find some logical and other issues...

P.S. TBH, so big change is unreviewable in meaningful time. To have a
comprehensive review I, for example, spend ~1h/250LOC, and
~2.5h/1000LOC, I would estimate ~4h/2000LOC. Imagine one to spend one
day for this. Any volunteer? Not me.

-- 
With Best Regards,
Andy Shevchenko

[toc] | [prev] | [next] | [standalone]

#1585189 — Re: [PATCH v2 1/3] mtd: nand: Cleanup/rework the atmel_nand driver

On Tue, 21 Feb 2017 12:03:45 +0200
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Tue, Feb 21, 2017 at 10:06 AM, Boris Brezillon
> <boris.brezillon@free-electrons.com> wrote:
> > On Tue, 21 Feb 2017 01:54:37 +0200
> > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> >  
> >> On Tue, Feb 21, 2017 at 1:40 AM, Andy Shevchenko
> >> <andy.shevchenko@gmail.com> wrote:  
> >> > On Mon, Feb 20, 2017 at 10:50 PM, Boris Brezillon
> >> > <boris.brezillon@free-electrons.com> wrote:  
> >> >> On Mon, 20 Feb 2017 21:38:03 +0100
> >> >> Boris Brezillon <boris.brezillon@free-electrons.com> wrote:
> >> >>  
> >> >>> On Mon, 20 Feb 2017 22:27:17 +0200
> >> >>> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> >> >>>  
> >> >>> > On Mon, Feb 20, 2017 at 2:28 PM, Boris Brezillon
> >> >>> > <boris.brezillon@free-electrons.com> wrote:
> >> >>> >  
> >> >>> > >  drivers/mtd/nand/atmel/nand-controller.c | 2269 +++++++++++++++++++++++++++
> >> >>> > >  drivers/mtd/nand/atmel_nand.c            | 2479 ------------------------------  
> >> >>> >
> >> >>> > Does -M -C help you?
> >> >>> > At least it would help reviewers
> >> >>> >  
> >> >>>
> >> >>> No it doesn't, because files were not just moved around using git mv,
> >> >>> it's a complete rewrite of the driver. IIUC, you're about to review
> >> >>> this submission, or are you just trolling like last time?  
> >> >>
> >> >> My bad, I mistaken you with someone else. Sorry for being harsh, but my
> >> >> explanation stands ;-).  
> >> >
> >> > No problem. I was asking since it so big and on first glance looks
> >> > like a partial copy (I dunno if parameter to -C makes it somehow
> >> > useful), though I can't review this. It's too big to me. Sorry I'm
> >> > really not trolling, just didn't read commit message carefully.  
> >>
> >> Okay, I very quickly looked into the code, what I noticed
> >> - you like extra parens and empty lines in some cases (not big deal)  
> >
> > Can you point specific places where you think these are not needed?  
> 
> 1. For example,
> 
> #define ATMEL_NFC_CMD(pos, cmd)                        ((cmd) <<
> (((pos) * 8) + 2))

Well, I like to explicitly put parenthesis even when operator
precedence guarantees the order of the calculation ('*' is preceding
'+').

For the parenthesis around (cmd) and (pos), they are required to
guarantee that things like ATMEL_NFC_CMD(x + y, cmd) are working
correctly.

> 
>  *events ^= (status & *events);

I agree with this one, it's uneeded.

> 
>  (((x) * 0x4) + 0x28)

See my comment about ATMEL_NFC_CMD().

> 
>   memset(&si[1], 0, sizeof(s16) * ((2 * strength) - 1));

Ditto.

> 
> Perhaps more in the code. I'm not a LISP programmer.
> 
> 2. For empty lines it's solely matter of style, I don't care. My motto
> "less LOC better, but keep common sense in mind".
> 
> >> - some functions perhaps might have been refactored to have common
> >> pieces in error handling, though I didn't read core carefully.  
> >
> > Again, be more precise.  
> 
> 3. I don't remember anymore, sorry. Something I would refactor.
> 
> >> Most important part I have noticed is a GPIO request.
> >> I didn't get why you almost repeat gpiod_get() in case of platform data?
> >> Shouldn't we have GPIO look up table?
> >> Can we use builtin device properties (for GPIO and/or overall)?  
> >
> > Sorry but I don't get it. Can give an example of what you'd like me to
> > do?
> >  
> 
> 4. First of all, why do you need this function in the first place?
> 
> +struct gpio_desc *
> +atmel_nand_pdata_get_gpio(struct atmel_nand_controller *nc, int gpioid,
> +                         const char *name, bool active_low,
> +                         enum gpiod_flags flags)

Because I don't want to duplicate the code done in
atmel_nand_pdata_get_gpio() each time I have to convert a GPIO number
into a GPIO descriptor, and that is needed to support platforms that
haven't moved to DT yet (in this case, avr32).

> 
> 5. BIT() macro:
> 
>    const unsigned int k = 1 << deg(poly);
>    unsigned int nn = (1 << mm) - 1;

Yes, I must admit I didn't polish the code in PMECC, and most of it has
been copied from the old driver.
We could probably use BIT() in a few places.

> 
> 6. Why this casting (unsigned int) ?
> 
>  dev_dbg(pmecc->dev,
>                        "Bit flip in %s area, byte %d: 0x%02x -> 0x%02x\n",
>                        area, byte, *ptr, (unsigned int)(*ptr ^ BIT(bit)));

Again, this has been copied from the old driver. I'll have a closer
look.

> 
> 7. Question to all that distribution or whatever functions, don't you
> have a common helper? Or each vendor requires different logic behind
> it?

What are you talking about? nand_chip hooks?

> 
> 8. Have you checked what kernel library provides?

I think so, but again, this is really vague, what kind of
open-coded functions do you think could be replaced with core libraries
helpers?

> 
> And I believe there are still issues like those. After, who is on
> topic, might even find some logical and other issues...
> 
> P.S. TBH, so big change is unreviewable in meaningful time. To have a
> comprehensive review I, for example, spend ~1h/250LOC, and
> ~2.5h/1000LOC, I would estimate ~4h/2000LOC. Imagine one to spend one
> day for this. Any volunteer? Not me.

I'm not asking you to review the whole driver, but you started to
comment on the code without pointing clearly to the things you wanted
me to address.

[toc] | [prev] | [next] | [standalone]

#1585201 — Re: [PATCH v2 1/3] mtd: nand: Cleanup/rework the atmel_nand driver

Le 21/02/2017 à 11:26, Boris Brezillon a écrit :
> On Tue, 21 Feb 2017 12:03:45 +0200
> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> 
>> On Tue, Feb 21, 2017 at 10:06 AM, Boris Brezillon
>> <boris.brezillon@free-electrons.com> wrote:
>>> On Tue, 21 Feb 2017 01:54:37 +0200
>>> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>>>  
>>>> On Tue, Feb 21, 2017 at 1:40 AM, Andy Shevchenko
>>>> <andy.shevchenko@gmail.com> wrote:  
>>>>> On Mon, Feb 20, 2017 at 10:50 PM, Boris Brezillon
>>>>> <boris.brezillon@free-electrons.com> wrote:  
>>>>>> On Mon, 20 Feb 2017 21:38:03 +0100
>>>>>> Boris Brezillon <boris.brezillon@free-electrons.com> wrote:
>>>>>>  
>>>>>>> On Mon, 20 Feb 2017 22:27:17 +0200
>>>>>>> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>>>>>>>  
>>>>>>>> On Mon, Feb 20, 2017 at 2:28 PM, Boris Brezillon
>>>>>>>> <boris.brezillon@free-electrons.com> wrote:
>>>>>>>>  
>>>>>>>>>  drivers/mtd/nand/atmel/nand-controller.c | 2269 +++++++++++++++++++++++++++
>>>>>>>>>  drivers/mtd/nand/atmel_nand.c            | 2479 ------------------------------  
>>>>>>>>
>>>>>>>> Does -M -C help you?
>>>>>>>> At least it would help reviewers
>>>>>>>>  
>>>>>>>
>>>>>>> No it doesn't, because files were not just moved around using git mv,
>>>>>>> it's a complete rewrite of the driver. IIUC, you're about to review
>>>>>>> this submission, or are you just trolling like last time?  
>>>>>>
>>>>>> My bad, I mistaken you with someone else. Sorry for being harsh, but my
>>>>>> explanation stands ;-).  
>>>>>
>>>>> No problem. I was asking since it so big and on first glance looks
>>>>> like a partial copy (I dunno if parameter to -C makes it somehow
>>>>> useful), though I can't review this. It's too big to me. Sorry I'm
>>>>> really not trolling, just didn't read commit message carefully.  
>>>>
>>>> Okay, I very quickly looked into the code, what I noticed
>>>> - you like extra parens and empty lines in some cases (not big deal)  
>>>
>>> Can you point specific places where you think these are not needed?  
>>
>> 1. For example,
>>
>> #define ATMEL_NFC_CMD(pos, cmd)                        ((cmd) <<
>> (((pos) * 8) + 2))
> 
> Well, I like to explicitly put parenthesis even when operator
> precedence guarantees the order of the calculation ('*' is preceding
> '+').

Yes

> For the parenthesis around (cmd) and (pos), they are required to
> guarantee that things like ATMEL_NFC_CMD(x + y, cmd) are working
> correctly.

Absolutely.


Even when it's not needed, please keep this habit of using more
parenthesis than required by precedence to make code clearer.


>>  *events ^= (status & *events);
> 
> I agree with this one, it's uneeded.
> 
>>
>>  (((x) * 0x4) + 0x28)
> 
> See my comment about ATMEL_NFC_CMD().
> 
>>
>>   memset(&si[1], 0, sizeof(s16) * ((2 * strength) - 1));
> 
> Ditto.
> 
>>
>> Perhaps more in the code. I'm not a LISP programmer.

[..]

>> 8. Have you checked what kernel library provides?
> 
> I think so, but again, this is really vague, what kind of
> open-coded functions do you think could be replaced with core libraries
> helpers?
> 
>> And I believe there are still issues like those. After, who is on
>> topic, might even find some logical and other issues...
>>
>> P.S. TBH, so big change is unreviewable in meaningful time. To have a
>> comprehensive review I, for example, spend ~1h/250LOC, and
>> ~2.5h/1000LOC, I would estimate ~4h/2000LOC. Imagine one to spend one
>> day for this. Any volunteer? Not me.
> 
> I'm not asking you to review the whole driver, but you started to
> comment on the code without pointing clearly to the things you wanted
> me to address.

Moreover, it's not like if the driver would come without previous code.
So, this re-factoring comes with the experience of previous driver and
its aim is to be comparable feature-wise with the old one. So the amount
of changes doesn't surprise me.

As Boris noted in his patch series, additional optimization and use of
common BCH code can be studied afterwards.

Best regards,
-- 
Nicolas Ferre

[toc] | [prev] | [next] | [standalone]

#1585224 — Re: [PATCH v2 1/3] mtd: nand: Cleanup/rework the atmel_nand driver

On Tue, Feb 21, 2017 at 12:26 PM, Boris Brezillon
<boris.brezillon@free-electrons.com> wrote:
> On Tue, 21 Feb 2017 12:03:45 +0200
> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

>> 1. For example,
>>
>> #define ATMEL_NFC_CMD(pos, cmd)                        ((cmd) <<
>> (((pos) * 8) + 2))
>
> Well, I like to explicitly put parenthesis even when operator
> precedence guarantees the order of the calculation ('*' is preceding
> '+').

That's my point. I'm not a LISP programmer.
Personally I think it makes readability worse.

> For the parenthesis around (cmd) and (pos), they are required to
> guarantee that things like ATMEL_NFC_CMD(x + y, cmd) are working
> correctly.

I know that.

>> >> Most important part I have noticed is a GPIO request.
>> >> I didn't get why you almost repeat gpiod_get() in case of platform data?
>> >> Shouldn't we have GPIO look up table?
>> >> Can we use builtin device properties (for GPIO and/or overall)?
>> >
>> > Sorry but I don't get it. Can give an example of what you'd like me to
>> > do?
>> >
>>
>> 4. First of all, why do you need this function in the first place?
>>
>> +struct gpio_desc *
>> +atmel_nand_pdata_get_gpio(struct atmel_nand_controller *nc, int gpioid,
>> +                         const char *name, bool active_low,
>> +                         enum gpiod_flags flags)
>
> Because I don't want to duplicate the code done in
> atmel_nand_pdata_get_gpio() each time I have to convert a GPIO number
> into a GPIO descriptor, and that is needed to support platforms that
> haven't moved to DT yet

They should use GPIO lookup tables.

We don't encourage people to use platform data anymore.

We have unified device properties for something like "timeout-us", we
have look up tables when you need specifics like pwm, gpio, pinctrl,
...

Abusing platform data with pointers is also not welcome.

> (in this case, avr32).

It's dead de facto.

When last time did you compile kernel for it? What was the version of kernel?
Did it get successfully?

When are we going to remove avr32 support from kernel completely?

>> 5. BIT() macro:

> We could probably use BIT() in a few places.

There are more places including data structures assignments.

> Again, this has been copied from the old driver. I'll have a closer
> look.

Exactly. You overlooked due to enormous LOC in the one change. See my
point below.

>> 7. Question to all that distribution or whatever functions, don't you
>> have a common helper? Or each vendor requires different logic behind
>> it?
>
> What are you talking about? nand_chip hooks?

That long arithmetic with some data.

>> 8. Have you checked what kernel library provides?
>
> I think so, but again, this is really vague, what kind of
> open-coded functions do you think could be replaced with core libraries
> helpers?

I dunno, I'm asking you. Usually if I see a pattern I got a clue to
check lib/ and similar places. From time to time I discover something
new and interesting there.

>> And I believe there are still issues like those. After, who is on
>> topic, might even find some logical and other issues...
>>
>> P.S. TBH, so big change is unreviewable in meaningful time. To have a
>> comprehensive review I, for example, spend ~1h/250LOC, and
>> ~2.5h/1000LOC, I would estimate ~4h/2000LOC. Imagine one to spend one
>> day for this. Any volunteer? Not me.
>
> I'm not asking you to review the whole driver, but you started to
> comment on the code without pointing clearly to the things you wanted
> me to address.

Yes, because my point is *split* this to be reviewable.

-- 
With Best Regards,
Andy Shevchenko

[toc] | [prev] | [next] | [standalone]

#1585233 — Re: [PATCH v2 1/3] mtd: nand: Cleanup/rework the atmel_nand driver

(adding Hans-Christian)

On 21/02/2017 at 13:02:21 +0200, Andy Shevchenko wrote:
> Abusing platform data with pointers is also not welcome.
> 
> > (in this case, avr32).
> 
> It's dead de facto.
> 
> When last time did you compile kernel for it? What was the version of kernel?
> Did it get successfully?
> 

v4.10-rc3 was building successfully but had some issues in the network
code.

> When are we going to remove avr32 support from kernel completely?
> 

Ask that to the avr32 maintainers. It still builds and is still booted
by some people. And that actually seems to be you as you reported a bug
we introduced in 4.3. I don't think we had any other report after that.

It can be frustrating at times to handle that platform but if it is
working for someone, I don't see why we would remove it.


-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[toc] | [prev] | [next] | [standalone]

#1585479 — Re: [PATCH v2 1/3] mtd: nand: Cleanup/rework the atmel_nand driver

On Tue, Feb 21, 2017 at 1:27 PM, Alexandre Belloni
<alexandre.belloni@free-electrons.com> wrote:
> (adding Hans-Christian)
>
> On 21/02/2017 at 13:02:21 +0200, Andy Shevchenko wrote:
>> Abusing platform data with pointers is also not welcome.
>>
>> > (in this case, avr32).
>>
>> It's dead de facto.
>>
>> When last time did you compile kernel for it? What was the version of kernel?
>> Did it get successfully?
>>
>
> v4.10-rc3 was building successfully but had some issues in the network
> code.

Newer kernel doesn't link...

>> When are we going to remove avr32 support from kernel completely?

> Ask that to the avr32 maintainers. It still builds and is still booted
> by some people. And that actually seems to be you as you reported a bug
> we introduced in 4.3. I don't think we had any other report after that.

https://patchwork.kernel.org/patch/9505727/

After that I gave up on it. Next time I will escalate directly to
Linus. It's a complete necrophilia. I spent already enough time to
look at that code. It brings now more burden than supports someone
somewhere.

> It can be frustrating at times to handle that platform but if it is
> working for someone, I don't see why we would remove it.

How it's working if it's not linked?

-- 
With Best Regards,
Andy Shevchenko

[toc] | [prev] | [next] | [standalone]

#1585494 — Re: [PATCH v2 1/3] mtd: nand: Cleanup/rework the atmel_nand driver

On 21/02/2017 at 18:09:09 +0200, Andy Shevchenko wrote:
> On Tue, Feb 21, 2017 at 1:27 PM, Alexandre Belloni
> <alexandre.belloni@free-electrons.com> wrote:
> > (adding Hans-Christian)
> >
> > On 21/02/2017 at 13:02:21 +0200, Andy Shevchenko wrote:
> >> Abusing platform data with pointers is also not welcome.
> >>
> >> > (in this case, avr32).
> >>
> >> It's dead de facto.
> >>
> >> When last time did you compile kernel for it? What was the version of kernel?
> >> Did it get successfully?
> >>
> >
> > v4.10-rc3 was building successfully but had some issues in the network
> > code.
> 
> Newer kernel doesn't link...
> 
> >> When are we going to remove avr32 support from kernel completely?
> 
> > Ask that to the avr32 maintainers. It still builds and is still booted
> > by some people. And that actually seems to be you as you reported a bug
> > we introduced in 4.3. I don't think we had any other report after that.
> 
> https://patchwork.kernel.org/patch/9505727/
> 
> After that I gave up on it. Next time I will escalate directly to
> Linus. It's a complete necrophilia. I spent already enough time to
> look at that code. It brings now more burden than supports someone
> somewhere.
> 

As said, it builds fine without networking. Maybe the first step is to
ask the avr32 maintainers. If you already did so, please feel free to
send a patch to remove the whole architecture.
The benefits for atmel will be: proper big endian support, removal of
platform data from all the drivers, better clocksource handling.

> > It can be frustrating at times to handle that platform but if it is
> > working for someone, I don't see why we would remove it.
> 
> How it's working if it's not linked?
> 

Come on, v4.10 has just been release and v4.9 was building just fine. Do
you really expect everybody to closely follow linux-next or update
overnight?

-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[toc] | [prev] | [next] | [standalone]

#1585508 — Re: [PATCH v2 1/3] mtd: nand: Cleanup/rework the atmel_nand driver

On Tue, Feb 21, 2017 at 6:21 PM, Alexandre Belloni
<alexandre.belloni@free-electrons.com> wrote:
> On 21/02/2017 at 18:09:09 +0200, Andy Shevchenko wrote:
>> On Tue, Feb 21, 2017 at 1:27 PM, Alexandre Belloni
>> <alexandre.belloni@free-electrons.com> wrote:
>> > On 21/02/2017 at 13:02:21 +0200, Andy Shevchenko wrote:
>> >> Abusing platform data with pointers is also not welcome.

>> >> > (in this case, avr32).
>> >>
>> >> It's dead de facto.
>> >>
>> >> When last time did you compile kernel for it? What was the version of kernel?
>> >> Did it get successfully?
>> >>
>> >
>> > v4.10-rc3 was building successfully but had some issues in the network
>> > code.
>>
>> Newer kernel doesn't link...
>>
>> >> When are we going to remove avr32 support from kernel completely?
>>
>> > Ask that to the avr32 maintainers. It still builds and is still booted
>> > by some people. And that actually seems to be you as you reported a bug
>> > we introduced in 4.3. I don't think we had any other report after that.
>>
>> https://patchwork.kernel.org/patch/9505727/
>>
>> After that I gave up on it. Next time I will escalate directly to
>> Linus. It's a complete necrophilia. I spent already enough time to
>> look at that code. It brings now more burden than supports someone
>> somewhere.
>>
>
> As said, it builds fine without networking.

It sounds a bit sarcastic. Irony is that I *have* hardware here which
was dedicated as Network Gateway (ATNGW100). I'm accessing to it
remotely.
How useful it would be?

> Maybe the first step is to
> ask the avr32 maintainers. If you already did so,

I did it ~year or so before where another relocation bug was discovered (fixed).

> please feel free to
> send a patch to remove the whole architecture.
> The benefits for atmel will be: proper big endian support, removal of
> platform data from all the drivers, better clocksource handling.

That is good point, but if maintainers don't care, why anyone else should?
Neither do I.

>> > It can be frustrating at times to handle that platform but if it is
>> > working for someone, I don't see why we would remove it.
>>
>> How it's working if it's not linked?
>>
>
> Come on, v4.10 has just been release and v4.9 was building just fine. Do
> you really expect everybody to closely follow linux-next or update
> overnight?

What version do you use as compiler?

Today's linux-next:
$ make O=~/prj/TMP/out/avr32 C=1 CF=-D__CHECK_ENDIAN__ -j64 CONFIG_DEBUG_INFO=
y CONFIG_DEBUG_SECTION_MISMATCH=y

  CC      lib/sbitmap.o
{standard input}: Assembler messages:
{standard input}:378: Warning: Unary operator + ignored because bad
operand follows
{standard input}:378: Warning: missing operand; zero assumed
{standard input}:378: Internal error!
Assertion failure in finish_insn at .././gas/config/tc-avr32.c line 3498.
Please report this bug.
scripts/Makefile.build:294: recipe for target 'lib/sbitmap.o' failed

$ avr32-linux-gcc --version
avr32-linux-gcc (GCC) 4.2.2-atmel.1.0.8


-- 
With Best Regards,
Andy Shevchenko

[toc] | [prev] | [next] | [standalone]

#1585518 — Re: [PATCH v2 1/3] mtd: nand: Cleanup/rework the atmel_nand driver

On Tue, Feb 21, 2017 at 6:32 PM, Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Tue, Feb 21, 2017 at 6:21 PM, Alexandre Belloni
> <alexandre.belloni@free-electrons.com> wrote:
>> On 21/02/2017 at 18:09:09 +0200, Andy Shevchenko wrote:
>>> On Tue, Feb 21, 2017 at 1:27 PM, Alexandre Belloni
>>> <alexandre.belloni@free-electrons.com> wrote:
>>> > On 21/02/2017 at 13:02:21 +0200, Andy Shevchenko wrote:
>>> >> Abusing platform data with pointers is also not welcome.
>
>>> >> > (in this case, avr32).
>>> >>
>>> >> It's dead de facto.
>>> >>
>>> >> When last time did you compile kernel for it? What was the version of kernel?
>>> >> Did it get successfully?
>>> >>
>>> >
>>> > v4.10-rc3 was building successfully but had some issues in the network
>>> > code.
>>>
>>> Newer kernel doesn't link...
>>>
>>> >> When are we going to remove avr32 support from kernel completely?
>>>
>>> > Ask that to the avr32 maintainers. It still builds and is still booted
>>> > by some people. And that actually seems to be you as you reported a bug
>>> > we introduced in 4.3. I don't think we had any other report after that.
>>>
>>> https://patchwork.kernel.org/patch/9505727/
>>>
>>> After that I gave up on it. Next time I will escalate directly to
>>> Linus. It's a complete necrophilia. I spent already enough time to
>>> look at that code. It brings now more burden than supports someone
>>> somewhere.
>>>
>>
>> As said, it builds fine without networking.
>
> It sounds a bit sarcastic. Irony is that I *have* hardware here which
> was dedicated as Network Gateway (ATNGW100). I'm accessing to it
> remotely.
> How useful it would be?
>
>> Maybe the first step is to
>> ask the avr32 maintainers. If you already did so,
>
> I did it ~year or so before where another relocation bug was discovered (fixed).
>
>> please feel free to
>> send a patch to remove the whole architecture.
>> The benefits for atmel will be: proper big endian support, removal of
>> platform data from all the drivers, better clocksource handling.
>
> That is good point, but if maintainers don't care, why anyone else should?
> Neither do I.
>
>>> > It can be frustrating at times to handle that platform but if it is
>>> > working for someone, I don't see why we would remove it.
>>>
>>> How it's working if it's not linked?
>>>
>>
>> Come on, v4.10 has just been release and

It doesn't build anymore. And current case even worse
Face it. It's dead.

  MODPOST vmlinux.o
WARNING: vmlinux.o(.text+0x1f2bd4): Section mismatch in reference from
the variable __param_ops_mtd to the functio
n .init.text:ubi_mtd_param_parse()
The function __param_ops_mtd() references
the function __init ubi_mtd_param_parse().
This is often because __param_ops_mtd lacks a __init
annotation or the annotation of ubi_mtd_param_parse is wrong.

crypto/built-in.o: warning: input is not relaxable
virt/built-in.o: warning: input is not relaxable
net/built-in.o: In function `rtnl_fill_ifinfo':
net/socket.c:451: relocation truncated to fit: R_AVR32_11H_PCREL
against `.text'+22768
Makefile:969: recipe for target 'vmlinux' failed


> v4.9 was building just fine. Do
>> you really expect everybody to closely follow linux-next or update
>> overnight?
>
> What version do you use as compiler?
>
> Today's linux-next:
> $ make O=~/prj/TMP/out/avr32 C=1 CF=-D__CHECK_ENDIAN__ -j64 CONFIG_DEBUG_INFO=
> y CONFIG_DEBUG_SECTION_MISMATCH=y
>
>   CC      lib/sbitmap.o
> {standard input}: Assembler messages:
> {standard input}:378: Warning: Unary operator + ignored because bad
> operand follows
> {standard input}:378: Warning: missing operand; zero assumed
> {standard input}:378: Internal error!
> Assertion failure in finish_insn at .././gas/config/tc-avr32.c line 3498.
> Please report this bug.
> scripts/Makefile.build:294: recipe for target 'lib/sbitmap.o' failed
>
> $ avr32-linux-gcc --version
> avr32-linux-gcc (GCC) 4.2.2-atmel.1.0.8

-- 
With Best Regards,
Andy Shevchenko

[toc] | [prev] | [next] | [standalone]

#1585546 — Re: [PATCH v2 1/3] mtd: nand: Cleanup/rework the atmel_nand driver

On 21/02/2017 at 18:43:35 +0200, Andy Shevchenko wrote:
> >> Come on, v4.10 has just been release and
> 
> It doesn't build anymore. And current case even worse
> Face it. It's dead.
> 

I agree it hasn't seen any significant development in a while but I'm
not the on able to take that decision.

A few weeks ago, I was telling Boris to let it not build for a while and
then remove it. You already went out of your way to make it work. Again,
feel free to send a patch removing avr32. I can only see a lot of
benefits for the Atmel ARM SoCs and the many cleanups that will follow.

If nobody complains about the 4.10 breakage, You'll have plenty of time
to remove it for 4.12

-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[toc] | [prev] | [next] | [standalone]

#1587302 — Re: [PATCH v2 1/3] mtd: nand: Cleanup/rework the atmel_nand driver

On Tue, Feb 21, 2017 at 9:14 AM, Alexandre Belloni
<alexandre.belloni@free-electrons.com> wrote:
> On 21/02/2017 at 18:43:35 +0200, Andy Shevchenko wrote:
>> >> Come on, v4.10 has just been release and
>>
>> It doesn't build anymore. And current case even worse
>> Face it. It's dead.
>>
>
> I agree it hasn't seen any significant development in a while but I'm
> not the on able to take that decision.

I've been wanting to add devicetree support for some time, but I no
longer remember how to build the toolchain, and I don't have fond
memories of that whole process. And the fact that
4.2.4-atmel.1.1.3.avr32linux.1 is still the most current version of
gcc doesn't make me very optimistic.

So while Hans-Christian and others have been doing a great job of
keeping AVR32 on life support, I tend to think that if there's not
enough enthusiasm for the architecture to build a modern toolchain or
add support for device tree, avr32-linux probably isn't going anywhere
exciting.

> A few weeks ago, I was telling Boris to let it not build for a while and
> then remove it. You already went out of your way to make it work. Again,
> feel free to send a patch removing avr32. I can only see a lot of
> benefits for the Atmel ARM SoCs and the many cleanups that will follow.

Agree, I can't help but feel that the AVR32 support is doing more harm
than good at this point.

> If nobody complains about the 4.10 breakage, You'll have plenty of time
> to remove it for 4.12

I'm fine with that, but I haven't put much effort into keeping it
alive lately. If Hans-Christian agrees, I'm willing to post a patch to
remove it, or ack someone else's patch.

Håvard

[toc] | [prev] | [next] | [standalone]

#1587340 — Re: [PATCH v2 1/3] mtd: nand: Cleanup/rework the atmel_nand driver

Hi Hans-Cristrian,

On Fri, 24 Feb 2017 09:14:30 +0100
Hans-Christian Noren Egtvedt <egtvedt@samfundet.no> wrote:

> Around Thu 23 Feb 2017 21:18:13 -0800 or thereabout, Håvard Skinnemoen wrote:
> > On Tue, Feb 21, 2017 at 9:14 AM, Alexandre Belloni
> > <alexandre.belloni@free-electrons.com> wrote:  
> >> On 21/02/2017 at 18:43:35 +0200, Andy Shevchenko wrote:  
> 
> <snipp>
> 
> >> A few weeks ago, I was telling Boris to let it not build for a while and
> >> then remove it. You already went out of your way to make it work. Again,
> >> feel free to send a patch removing avr32. I can only see a lot of
> >> benefits for the Atmel ARM SoCs and the many cleanups that will follow.  
> > 
> > Agree, I can't help but feel that the AVR32 support is doing more harm
> > than good at this point.  
> 
> I also agree on this, I can relate to Nicolas (and Atmel friends) having to
> always think about the less-maintained AVR32 parts when improving drivers.

Indeed, that should make atmel drivers maintainance a bit easier.

> 
> >> If nobody complains about the 4.10 breakage, You'll have plenty of time
> >> to remove it for 4.12  
> > 
> > I'm fine with that, but I haven't put much effort into keeping it
> > alive lately. If Hans-Christian agrees, I'm willing to post a patch to
> > remove it, or ack someone else's patch.  
> 
> Then lets plan this for 4.12, either you Håvard whip up a patch or I can
> eventually do it.
> 
> I can push it through the linux-avr32 git tree on kernel.org.
> 

Can you do that just after 4.11-rc1 is released and provide a topic
branch I can pull in my nand/next branch, so that I can rework this
patch and drop all the pdata-compat code (as suggested by Andy).

Thanks,

Boris

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web