net: phy: at803x: don't depend on GPIOLIB

On 22/03/2016 20:42, Uwe Kleine-König 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:



Regards.

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

Hi Sergei,

On 03/23/2016 11:39 AM, Sergei Shtylyov wrote:
>>          gpiod_reset = devm_gpiod_get_optional(dev, "reset",
>> GPIOD_OUT_HIGH);
> 
>    We shouldn't call _optional() then, should we?

I could imagine the original intention was to be backward compatible.

Indeed, if this call is not optional, systems using AT8030 and lacking a
reset line on DT will have their behaviour affected.
NOTE: they would still work because even if this driver fails to bind a
generic one will take over.

Best regards,

Sebastian