[net-next,v2,2/4] net: phy: Add a helper to return the index for of the internal delay

Add a helper function that will return the index in the array for the
passed in internal delay value.  The helper requires the array, size and
delay value.

The helper will then return the index for the exact match or return the
index for the index to the closest smaller value.

Signed-off-by: Dan Murphy <dmurphy@ti.com>
---
 drivers/net/phy/phy_device.c | 45 ++++++++++++++++++++++++++++++++++++
 include/linux/phy.h          |  2 ++
 2 files changed, 47 insertions(+)

On 5/22/2020 5:25 AM, Dan Murphy wrote:
> Add a helper function that will return the index in the array for the
> passed in internal delay value.  The helper requires the array, size and
> delay value.
> 
> The helper will then return the index for the exact match or return the
> index for the index to the closest smaller value.
> 
> Signed-off-by: Dan Murphy <dmurphy@ti.com>
> ---
>  drivers/net/phy/phy_device.c | 45 ++++++++++++++++++++++++++++++++++++
>  include/linux/phy.h          |  2 ++
>  2 files changed, 47 insertions(+)
> 
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 7481135d27ab..40f53b379d2b 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -2661,6 +2661,51 @@ void phy_get_pause(struct phy_device *phydev, bool *tx_pause, bool *rx_pause)
>  }
>  EXPORT_SYMBOL(phy_get_pause);
>  
> +/**
> + * phy_get_delay_index - returns the index of the internal delay
> + * @phydev: phy_device struct
> + * @delay_values: array of delays the PHY supports
> + * @size: the size of the delay array
> + * @delay: the delay to be looked up
> + *
> + * Returns the index within the array of internal delay passed in.

Can we consider using s32 for storage that way the various
of_read_property_read_u32() are a natural fit (int works too, but I
would prefer being explicit).

> + */
> +int phy_get_delay_index(struct phy_device *phydev, int *delay_values, int size,
> +			int delay)
> +{
> +	int i;
> +
> +	if (size <= 0)
> +		return -EINVAL;
> +
> +	if (delay <= delay_values[0])
> +		return 0;
> +
> +	if (delay > delay_values[size - 1])
> +		return size - 1;

Does not that assume that the delays are sorted by ascending order, if
so, can you make it clear in the kernel doc?

> +
> +	for (i = 0; i < size; i++) {
> +		if (delay == delay_values[i])
> +			return i;
> +
> +		/* Find an approximate index by looking up the table */
> +		if (delay > delay_values[i - 1] &&

&& i > 0 so you do not accidentally under-run the array?

> +		    delay < delay_values[i]) {
> +			if (delay - delay_values[i - 1] < delay_values[i] - delay)
> +				return i - 1;
> +			else
> +				return i;
> +		}
> +
> +	}
> +
> +	phydev_err(phydev, "error finding internal delay index for %d\n",
> +		   delay);
> +
> +	return -EINVAL;
> +}
> +EXPORT_SYMBOL(phy_get_delay_index);
> +
>  static bool phy_drv_supports_irq(struct phy_driver *phydrv)
>  {
>  	return phydrv->config_intr && phydrv->ack_interrupt;
> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index 2bcdf19ed3b4..73552612c189 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -1408,6 +1408,8 @@ void phy_set_asym_pause(struct phy_device *phydev, bool rx, bool tx);
>  bool phy_validate_pause(struct phy_device *phydev,
>  			struct ethtool_pauseparam *pp);
>  void phy_get_pause(struct phy_device *phydev, bool *tx_pause, bool *rx_pause);
> +int phy_get_delay_index(struct phy_device *phydev, int *delay_values,
> +			int size, int delay);
>  void phy_resolve_pause(unsigned long *local_adv, unsigned long *partner_adv,
>  		       bool *tx_pause, bool *rx_pause);
>  
>

Florian

On 5/22/20 11:11 AM, Florian Fainelli wrote:
>
> On 5/22/2020 5:25 AM, Dan Murphy wrote:
>> Add a helper function that will return the index in the array for the
>> passed in internal delay value.  The helper requires the array, size and
>> delay value.
>>
>> The helper will then return the index for the exact match or return the
>> index for the index to the closest smaller value.
>>
>> Signed-off-by: Dan Murphy <dmurphy@ti.com>
>> ---
>>   drivers/net/phy/phy_device.c | 45 ++++++++++++++++++++++++++++++++++++
>>   include/linux/phy.h          |  2 ++
>>   2 files changed, 47 insertions(+)
>>
>> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
>> index 7481135d27ab..40f53b379d2b 100644
>> --- a/drivers/net/phy/phy_device.c
>> +++ b/drivers/net/phy/phy_device.c
>> @@ -2661,6 +2661,51 @@ void phy_get_pause(struct phy_device *phydev, bool *tx_pause, bool *rx_pause)
>>   }
>>   EXPORT_SYMBOL(phy_get_pause);
>>   
>> +/**
>> + * phy_get_delay_index - returns the index of the internal delay
>> + * @phydev: phy_device struct
>> + * @delay_values: array of delays the PHY supports
>> + * @size: the size of the delay array
>> + * @delay: the delay to be looked up
>> + *
>> + * Returns the index within the array of internal delay passed in.
> Can we consider using s32 for storage that way the various
> of_read_property_read_u32() are a natural fit (int works too, but I
> would prefer being explicit).

Ack

>
>> + */
>> +int phy_get_delay_index(struct phy_device *phydev, int *delay_values, int size,
>> +			int delay)
>> +{
>> +	int i;
>> +
>> +	if (size <= 0)
>> +		return -EINVAL;
>> +
>> +	if (delay <= delay_values[0])
>> +		return 0;
>> +
>> +	if (delay > delay_values[size - 1])
>> +		return size - 1;
> Does not that assume that the delays are sorted by ascending order, if
> so, can you make it clear in the kernel doc?

Yes I guess it does.  I can add this to the k doc


>
>> +
>> +	for (i = 0; i < size; i++) {
>> +		if (delay == delay_values[i])
>> +			return i;
>> +
>> +		/* Find an approximate index by looking up the table */
>> +		if (delay > delay_values[i - 1] &&
> && i > 0 so you do not accidentally under-run the array?

Yes and no it maybe better to start the for loop with i being 
initialized to 1 since the zeroth element is already validated above.

Dan