[net-next,0/5] Modernize bitbanged GPIO MDIO

Message ID 20180225125132.25275-1-linus.walleij@linaro.org
Headers show
Series
  • Modernize bitbanged GPIO MDIO
Related show

Message

Linus Walleij Feb. 25, 2018, 12:51 p.m.
This kills off the platform data support from the bitbanged
GPIO-based MDIO driver and moves it over to using GPIO
descriptors exclusively.

We are certainly not going to merge any more platforms into
the kernel using platform data, and nothing is using it at the
moment. The only concern would be out-of-tree platforms, and
those are not the concern of the kernel community. They need
to move to use device tree (or ACPI etc) like everyone else.

This was tested on the bit-banged GPIO MDIO on the D-Link
DNS-313 and works fine for me.

Linus Walleij (5):
  net: mdio-gpio: Localize platform data
  net: mdio-gpio: Allocate state in probe()
  net: mdio-gpio: Remove non-DT probe path
  net: mdio-gpio: Merge platform data into state
  net: mdio-gpio: Move to gpiod API

 MAINTAINERS                             |   1 -
 drivers/net/phy/Kconfig                 |   2 +-
 drivers/net/phy/mdio-gpio.c             | 151 ++++++++++----------------------
 include/linux/platform_data/mdio-gpio.h |  33 -------
 4 files changed, 47 insertions(+), 140 deletions(-)
 delete mode 100644 include/linux/platform_data/mdio-gpio.h

-- 
2.14.3

Comments

Andrew Lunn Feb. 25, 2018, 7:08 p.m. | #1
On Sun, Feb 25, 2018 at 01:51:27PM +0100, Linus Walleij wrote:
> This kills off the platform data support from the bitbanged

> GPIO-based MDIO driver and moves it over to using GPIO

> descriptors exclusively.


Hi Linus

I like where this ends up. I wounder about the path it takes to get
there. There seems to be quite a lot of code which gets moved around
and then in the end deleted. Maybe changing the order of the patches
would help. Converting to devm_gpiod_get_index() first would remove
all the active_low flags. Then remove all the unused reset callback,
irq, etc?

     Thanks
	Andrew
Linus Walleij Feb. 27, 2018, 8:53 a.m. | #2
On Sun, Feb 25, 2018 at 8:08 PM, Andrew Lunn <andrew@lunn.ch> wrote:
> On Sun, Feb 25, 2018 at 01:51:27PM +0100, Linus Walleij wrote:

>> This kills off the platform data support from the bitbanged

>> GPIO-based MDIO driver and moves it over to using GPIO

>> descriptors exclusively.

>

> Hi Linus

>

> I like where this ends up. I wounder about the path it takes to get

> there. There seems to be quite a lot of code which gets moved around

> and then in the end deleted. Maybe changing the order of the patches

> would help. Converting to devm_gpiod_get_index() first would remove

> all the active_low flags. Then remove all the unused reset callback,

> irq, etc?


It's mainly a matter of which order we want bisects to end up...

I was thinking to order the patches from least-likely to cause
regressions to most-likely to cause regressions so that it is
later possible to revert the more likely regression cause without
getting into conflict with the rest.

Yours,
Linus Walleij
Florian Fainelli Feb. 27, 2018, 2 p.m. | #3
On 02/25/2018 04:51 AM, Linus Walleij wrote:
> This kills off the platform data support from the bitbanged

> GPIO-based MDIO driver and moves it over to using GPIO

> descriptors exclusively.

> 

> We are certainly not going to merge any more platforms into

> the kernel using platform data, and nothing is using it at the

> moment. The only concern would be out-of-tree platforms, and

> those are not the concern of the kernel community. They need

> to move to use device tree (or ACPI etc) like everyone else.


Please refrain from making statements like those because there are
perfectly valid use cases for never seeing ACPI or Device Tree for a
given platform, yes there will be push back, and yes, if DT or ACPI is
possible, it should be used but I can guarantee you there are platforms
out there that won't be converted to DT (e.g: tons of MIPS-based router,
x86 add-on modules etc.).

Nack on patches 1 and 3, because I am slowly resuming work on an x86
platform driver that uses the mdio-gpio driver with platform data, and
DT is not an option there, and I would rather not have to revert your
changes.

> 

> This was tested on the bit-banged GPIO MDIO on the D-Link

> DNS-313 and works fine for me.

> 

> Linus Walleij (5):

>   net: mdio-gpio: Localize platform data

>   net: mdio-gpio: Allocate state in probe()

>   net: mdio-gpio: Remove non-DT probe path

>   net: mdio-gpio: Merge platform data into state

>   net: mdio-gpio: Move to gpiod API

> 

>  MAINTAINERS                             |   1 -

>  drivers/net/phy/Kconfig                 |   2 +-

>  drivers/net/phy/mdio-gpio.c             | 151 ++++++++++----------------------

>  include/linux/platform_data/mdio-gpio.h |  33 -------

>  4 files changed, 47 insertions(+), 140 deletions(-)

>  delete mode 100644 include/linux/platform_data/mdio-gpio.h

> 


-- 
Florian
Andrew Lunn Feb. 27, 2018, 11:10 p.m. | #4
> Nack on patches 1 and 3, because I am slowly resuming work on an x86

> platform driver that uses the mdio-gpio driver with platform data, and

> DT is not an option there, and I would rather not have to revert your

> changes.


Hi Florian

Would it be O.K. to change the platform_data to use a gpio descriptor,
rather than gpio number and active_low flag? That would simplify a lot
of code.

   Thanks
      Andrew
Florian Fainelli Feb. 27, 2018, 11:46 p.m. | #5
On 02/27/2018 03:10 PM, Andrew Lunn wrote:
>> Nack on patches 1 and 3, because I am slowly resuming work on an x86

>> platform driver that uses the mdio-gpio driver with platform data, and

>> DT is not an option there, and I would rather not have to revert your

>> changes.

> 

> Hi Florian

> 

> Would it be O.K. to change the platform_data to use a gpio descriptor,

> rather than gpio number and active_low flag? That would simplify a lot

> of code.


Oh absolutely, the move towards gpio descriptors is entirely fine and
reasonable, it was just the plain removal of platform_data instantiation
that I am objecting to. Thanks!
-- 
Florian
Linus Walleij March 2, 2018, 9:44 a.m. | #6
On Tue, Feb 27, 2018 at 3:00 PM, Florian Fainelli <f.fainelli@gmail.com> wrote:
> On 02/25/2018 04:51 AM, Linus Walleij wrote:

>> This kills off the platform data support from the bitbanged

>> GPIO-based MDIO driver and moves it over to using GPIO

>> descriptors exclusively.

>>

>> We are certainly not going to merge any more platforms into

>> the kernel using platform data, and nothing is using it at the

>> moment. The only concern would be out-of-tree platforms, and

>> those are not the concern of the kernel community. They need

>> to move to use device tree (or ACPI etc) like everyone else.

>

> Please refrain from making statements like those because there are

> perfectly valid use cases for never seeing ACPI or Device Tree for a

> given platform, yes there will be push back, and yes, if DT or ACPI is

> possible, it should be used but I can guarantee you there are platforms

> out there that won't be converted to DT (e.g: tons of MIPS-based router,

> x86 add-on modules etc.).

>

> Nack on patches 1 and 3, because I am slowly resuming work on an x86

> platform driver that uses the mdio-gpio driver with platform data, and

> DT is not an option there, and I would rather not have to revert your

> changes.


OK fair enough, there are some platform data use cases that
remain or are on their way in.

I will try to rewrite the series a bit as Andrew suggested to just do
descriptor conversion and take it from there, and also
try to make sure that it will work fine with any descriptor also if
it is coming from a board file.

Anyway, the platform data we have is:

struct mdio_gpio_platform_data {
        /* GPIO numbers for bus pins */
        unsigned int mdc;
        unsigned int mdio;
        unsigned int mdo;

        bool mdc_active_low;
        bool mdio_active_low;
        bool mdo_active_low;

        u32 phy_mask;
        u32 phy_ignore_ta_mask;
        int irqs[PHY_MAX_ADDR];
        /* reset callback */
        int (*reset)(struct mii_bus *bus);
};

So moving to using descriptors will get rid of the six first
platform data entries.

What about the last four things, phy_mask, phy_ignore_ta_mask,
irqs[] and the .reset() callback? None of this is documented or
used anywhere so I was a bit confused about what to do with it...
Does boardfiles adding GPIO MDIO need all this?

Yours,
Linus Walleij