spi: clps711x: Driver refactor
PATCH 1/2 spi: clps711x: Driver refactor
This is a complex patch for refactoring CLPS711X SPI driver. Major changes:
- Eliminate <mach/hardware.h> usage.
- Devicetree support.
Since we do not have any platform users of this driver, non-DT support is removed. DT users will be introduced later once DT support for this platform will be completed.
--- drivers/spi/Kconfig | 2 +- drivers/spi/spi-clps711x.c | 233 ++++++++++------------------- include/linux/platform_data/spi-clps711x.h | 21 --- 3 files changed, 84 insertions(+), 172 deletions(-) delete mode 100644 include/linux/platform_data/spi-clps711x.h
drivers/spi/Kconfig +1 -1
@@ -150,7 +150,7 @@ config SPI_BUTTERFLY config SPI_CLPS711X tristate "CLPS711X host SPI controller" - depends on ARCH_CLPS711X + depends on OF && (ARCH_CLPS711X || COMPILE_TEST) help This enables dedicated general purpose SPI/Microwire1-compatible master mode interface (SSI1) for CLPS711X-based CPUs.
drivers/spi/spi-clps711x.c +83 -150
351 lines not on this page · raw message
include/linux/platform_data/spi-clps711x.h deleted +0 -21
@@ -1,21 +0,0 @@ -/* -* CLPS711X SPI bus driver definitions -* -* Copyright (C) 2012 Alexander Shiyan <[email protected]> -* -* This program is free software; you can redistribute it and/or modify -* it under the terms of the GNU General Public License as published by -* the Free Software Foundation; either version 2 of the License, or -* (at your option) any later version. -*/ - -#ifndef ____LINUX_PLATFORM_DATA_SPI_CLPS711X_H -#define ____LINUX_PLATFORM_DATA_SPI_CLPS711X_H - -/* Board specific platform_data */ -struct spi_clps711x_pdata { -int *chipselect;/* Array of GPIO-numbers */ -int num_chipselect;/* Total count of GPIOs */ -}; - -#endif
Signature · 1 line
1.8.3.2
Mark Brown
maintainerPATCH 1/2 spi: clps711x: Driver refactor
Quoted 4 lines · Alexander Shiyan
This is a complex patch for refactoring CLPS711X SPI driver. Major changes:
- Eliminate <mach/hardware.h> usage.
- Devicetree support.
This really needs to be broken up into smaller changes so it can be reviewed, your summary would be good as the cover mail for a patch series but not for a single commit. We need one change per commit with a clear commit message saying what's going on.
There's a large set of changes here with no explanation of most of them which means I can't really tell if the changes are doing what they're supposed to and at least some of them seem to be doing things beyond either description above. I'd expect at least two changes, one for the mach/hardware.h elimination and one for the bindings, but probably each of those should be split into several changes. For example the bindings changes might have some patches doing refactorings before adding the actual bindings.
drivers/spi/spi-clps711x.c line 46
Quoted 6 lines
drivers/spi/spi-clps711x.c +3
-3-/* We are expect that SPI-device is not selected */ -gpio_direction_output (hw->chipselect[spi->chip_select], - !(spi->mode &SPI_CS_HIGH )); + ret =devm_gpio_request (&spi->master->dev, spi->cs_gpio,NULL ); +if (ret) +return ret;
For example this is a refectoring to use devm and request the GPIO which
is good but definitely not something I'd expect to see happening in the
same commit as anything mentioned in the changelog.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140101/91591fc5/attachment.sig>
Hello.
Quoted 11 lines · Alexander Shiyan
Quoted 4 lines
This is a complex patch for refactoring CLPS711X SPI driver. Major changes:
- Eliminate <mach/hardware.h> usage.
- Devicetree support.This really needs to be broken up into smaller changes so it can be reviewed, your summary would be good as the cover mail for a patch series but not for a single commit. We need one change per commit with a clear commit message saying what's going on.
Quoted 8 lines
There's a large set of changes here with no explanation of most of them which means I can't really tell if the changes are doing what they're supposed to and at least some of them seem to be doing things beyond either description above. I'd expect at least two changes, one for the
mach/hardware.helimination and one for the bindings, but probably each of those should be split into several changes. For example the bindings changes might have some patches doing refactorings before adding the actual bindings.
I wrote that this is a complex patch. This is not a fix, but new driver, many things are done differently.
I do not see a way to separate these changes.
As an alternative, I can create patch to remove current driver, then re-adding new one. Will be more convenient to watch. Is it better? Thanks.
---
Mark Brown
maintainerPATCH 1/2 spi: clps711x: Driver refactor
Quoted 3 lines · Alexander Shiyan
I wrote that this is a complex patch. This is not a fix, but new driver, many things are done differently.
I do not see a way to separate these changes.
I already mentioned one thing that could be done incrementally (the conversion to devm), from the glance I took through the code there's several other things that jump out and like I said at least splitting the hardware.h removal from the device tree changes would be a start.
Nothing in what you said the change did nor in the change itself suggested that it would be impossible to break things up, I'm not convinced that any attempt has been made to split anything out.
Quoted 2 lines
As an alternative, I can create patch to remove current driver, then re-adding new one. Will be more convenient to watch. Is it better?
No, that's really not good. It's very rare that we just completely
rewrite a driver especially not one that's been around for a while -
there needs to be a really good reason for it. This is a key part of
how the development process works, incremental improvements and code
review. Doing this both helps improve quality control on changes and
allows collaboration.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140101/520fd267/attachment.sig>
PATCH 1/2 spi: clps711x: Driver refactor
Quoted 9 lines · Mark Brown
Quoted 2 lines
As an alternative, I can create patch to remove current driver, then re-adding new one. Will be more convenient to watch. Is it better?
No, that's really not good. It's very rare that we just completely rewrite a driver especially not one that's been around for a while - there needs to be a really good reason for it. This is a key part of how the development process works, incremental improvements and code review. Doing this both helps improve quality control on changes and allows collaboration.
I definitely agree with what you say, but please note that Alexander is the only person who ever sends clps711x patches, so there is not much hope of collaboration. I also suspect he's the only one who would suffer from accidental breakage, especially since the existing driver is not used anywhere upstream (no instance of platform data, and no DT support).
Arnd
Quoted 11 lines · Mark Brown
Quoted 9 lines
Quoted 2 lines
As an alternative, I can create patch to remove current driver, then re-adding new one. Will be more convenient to watch. Is it better?
No, that's really not good. It's very rare that we just completely rewrite a driver especially not one that's been around for a while - there needs to be a really good reason for it. This is a key part of how the development process works, incremental improvements and code review. Doing this both helps improve quality control on changes and allows collaboration.
Quoted 6 lines
I definitely agree with what you say, but please note that Alexander is the only person who ever sends clps711x patches, so there is not much hope of collaboration. I also suspect he's the only one who would suffer from accidental breakage, especially since the existing driver is not used anywhere upstream (no instance of platform data, and no DT support).
Thanks for protection Arnd :)
Indeed, all as you say, of course it does not mean that I got VIP status and can make any changes to the kernel tree,
make me comments - I'm learning this.
---
Mark Brown
maintainerPATCH 1/2 spi: clps711x: Driver refactor
In reply to Arnd BergmannQuoted 3 lines · Arnd Bergmann
I definitely agree with what you say, but please note that Alexander is the only person who ever sends clps711x patches, so there is not much hope of collaboration. I also suspect he's the only one who would
There's stuff been going in from the people who do generic cleanups and updates.
Quoted 3 lines
suffer from accidental breakage, especially since the existing driver is not used anywhere upstream (no instance of platform data, and no DT support).
Right. On the other hand I really don't want to get into setting bad precedents that people then use when they complain that I actually want to review their code, that's just going to be more miserable in the long run.
There were also some things in there that I really did want to review
properly but I wasn't entirely sure what they were actually supposed to
be doing. Getting things split up with better changelogs will make that
review much more tractable, it might even be immediately obvious what is
going on.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140102/0da08763/attachment-0001.sig>
PATCH 1/2 spi: clps711x: Driver refactor
Quoted 8 lines · On Thursday 02 January 2014 18:25:49 Mark Brown wrote:
Quoted 3 lines · Arnd Bergmann
I definitely agree with what you say, but please note that Alexander is the only person who ever sends clps711x patches, so there is not much hope of collaboration. I also suspect he's the only one who would
There's stuff been going in from the people who do generic cleanups and updates.
Yes, I meant patches for new clps711x functionality. I saw the cleanups as well.
Quoted 8 lines
Quoted 3 lines
suffer from accidental breakage, especially since the existing driver is not used anywhere upstream (no instance of platform data, and no DT support).
Right. On the other hand I really don't want to get into setting bad precedents that people then use when they complain that I actually want to review their code, that's just going to be more miserable in the long run.
Makes sense.
Arnd