lkml.org 
[lkml]   [2016]   [Mar]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH] net: phy: at803x: don't depend on GPIOLIB
From
Date
Hello.

On 3/23/2016 1:17 PM, Mason wrote:

>> Preconditions:
>> - Some of the devices a given driver handles have a reset line and
>> others don't.
>> - A non-empty subset (maybe all) of the devices that have a reset line
>> require that this reset line is used.
>>
>> Then the way to handle this in the driver should be done as follows:
>>
>> unless reset_handling_not_necessary():
>> gpio = gpiod_get_optional("reset")
>> if IS_ERR(gpio):
>> return PTR_ERR(gpio)
>>
>> Checking for -ENOSYS or GPIOLIB=n is not allowed because the device
>> you're currently handling might need the GPIO, so you must not continue
>> without the ability to control the line.
>>
>> So the options you have (as you have a phy that doesn't need the reset
>> handling):
>>
>> - enable GPIOLIB (either in your .config or introduce a Kconfig
>> dependency)
>> - improve reset_handling_not_necessary() to return true for your case
>>
>> There is nothing else.
>
> Here are some numbers for GPIOLIB, on an ARM build:
>
> text data bss dec hex filename
> 1830 0 0 1830 726 devres.o
> 627 0 0 627 273 gpiolib-legacy.o
> 11018 40 4 11062 2b36 gpiolib.o
> 1598 0 0 1598 63e gpiolib-of.o
> --------------------------------------------------------
> 15073 40 4 15117 3b0d built-in.o
>
> So ~15 kilobytes.
>
>
> By the way, since the "reset-by-GPIO" solution is used only for
> the Atheros 8030, would it be possible to make the
> devm_gpiod_get_optional conditional on ATH8030_PHY_ID?
>
> I'm thinking of something along these lines, for illustration:
>
> diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
> index 2d020a3ec0b5..576e7873e049 100644
> --- a/drivers/net/phy/at803x.c
> +++ b/drivers/net/phy/at803x.c
> @@ -198,12 +198,16 @@ static int at803x_probe(struct phy_device *phydev)
> if (!priv)
> return -ENOMEM;
>
> + if (phydev->drv->phy_id != ATH8030_PHY_ID)
> + goto no_gpio;
> +
> gpiod_reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);

We shouldn't call _optional() then, should we?

> if (IS_ERR(gpiod_reset))
> return PTR_ERR(gpiod_reset);
>
> priv->gpiod_reset = gpiod_reset;
>
> +no_gpio:
> phydev->priv = priv;
>
> return 0;
>
>
> Regards.

MBR, Sergei

\
 
 \ /
  Last update: 2016-03-23 12:01    [W:1.007 / U:0.112 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site