diff mbox series

net: usb: smsc95xx: configure external LEDs function for EVB-LAN8670-USB

Message ID 20240522140817.409936-1-Parthiban.Veerasooran@microchip.com (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series net: usb: smsc95xx: configure external LEDs function for EVB-LAN8670-USB | expand

Checks

Context Check Description
netdev/series_format warning Single patches do not need cover letters; Target tree name not specified in the subject
netdev/tree_selection success Guessed tree name to be net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit fail Errors and warnings before: 905 this patch: 907
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers success CCed 7 of 7 maintainers
netdev/build_clang success Errors and warnings before: 909 this patch: 909
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn fail Errors and warnings before: 909 this patch: 911
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 25 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Parthiban Veerasooran May 22, 2024, 2:08 p.m. UTC 
By default, LAN9500A configures the external LEDs to the below function.
nSPD_LED -> Speed Indicator
nLNKA_LED -> Link and Activity Indicator
nFDX_LED -> Full Duplex Link Indicator

But, EVB-LAN8670-USB uses the below external LEDs function which can be
enabled by writing 1 to the LED Select (LED_SEL) bit in the LAN9500A.
nSPD_LED -> Speed Indicator
nLNKA_LED -> Link Indicator
nFDX_LED -> Activity Indicator

Signed-off-by: Parthiban Veerasooran <Parthiban.Veerasooran@microchip.com>
---
 drivers/net/usb/smsc95xx.c | 12 ++++++++++++
 drivers/net/usb/smsc95xx.h |  1 +
 2 files changed, 13 insertions(+)


base-commit: 4b377b4868ef17b040065bd468668c707d2477a5

Comments

Andrew Lunn May 22, 2024, 4:44 p.m. UTC | #1 
On Wed, May 22, 2024 at 07:38:17PM +0530, Parthiban Veerasooran wrote:
> By default, LAN9500A configures the external LEDs to the below function.
> nSPD_LED -> Speed Indicator
> nLNKA_LED -> Link and Activity Indicator
> nFDX_LED -> Full Duplex Link Indicator
> 
> But, EVB-LAN8670-USB uses the below external LEDs function which can be
> enabled by writing 1 to the LED Select (LED_SEL) bit in the LAN9500A.
> nSPD_LED -> Speed Indicator
> nLNKA_LED -> Link Indicator
> nFDX_LED -> Activity Indicator

What else can the LEDs indicate?

> +	/* Set LED Select (LED_SEL) bit for the external LED pins functionality
> +	 * in the Microchip's EVB-LAN8670-USB 10BASE-T1S Ethernet device which

Is this a function of the USB dongle? Or a function of the PHY?

	Andrew
Simon Horman May 22, 2024, 6:54 p.m. UTC | #2 
On Wed, May 22, 2024 at 07:38:17PM +0530, Parthiban Veerasooran wrote:
> By default, LAN9500A configures the external LEDs to the below function.
> nSPD_LED -> Speed Indicator
> nLNKA_LED -> Link and Activity Indicator
> nFDX_LED -> Full Duplex Link Indicator
> 
> But, EVB-LAN8670-USB uses the below external LEDs function which can be
> enabled by writing 1 to the LED Select (LED_SEL) bit in the LAN9500A.
> nSPD_LED -> Speed Indicator
> nLNKA_LED -> Link Indicator
> nFDX_LED -> Activity Indicator
> 
> Signed-off-by: Parthiban Veerasooran <Parthiban.Veerasooran@microchip.com>
> ---
>  drivers/net/usb/smsc95xx.c | 12 ++++++++++++
>  drivers/net/usb/smsc95xx.h |  1 +
>  2 files changed, 13 insertions(+)
> 
> diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
> index cbea24666479..05975461bf10 100644
> --- a/drivers/net/usb/smsc95xx.c
> +++ b/drivers/net/usb/smsc95xx.c
> @@ -1006,6 +1006,18 @@ static int smsc95xx_reset(struct usbnet *dev)
>  	/* Configure GPIO pins as LED outputs */
>  	write_buf = LED_GPIO_CFG_SPD_LED | LED_GPIO_CFG_LNK_LED |
>  		LED_GPIO_CFG_FDX_LED;
> +
> +	/* Set LED Select (LED_SEL) bit for the external LED pins functionality
> +	 * in the Microchip's EVB-LAN8670-USB 10BASE-T1S Ethernet device which
> +	 * uses the below LED function.
> +	 * nSPD_LED -> Speed Indicator
> +	 * nLNKA_LED -> Link Indicator
> +	 * nFDX_LED -> Activity Indicator
> +	 */
> +	if (dev->udev->descriptor.idVendor == 0x184F &&
> +	    dev->udev->descriptor.idProduct == 0x0051)

Hi Parthiban,

There seems to be an endian missmatch here.
The type of .idVendor and .idProduct is __le16,
but here they are compared against host byte-order integers.

Flagged by Sparse.

> +		write_buf |= LED_GPIO_CFG_LED_SEL;
> +
>  	ret = smsc95xx_write_reg(dev, LED_GPIO_CFG, write_buf);
>  	if (ret < 0)
>  		return ret;

...
Woojung Huh May 22, 2024, 7:52 p.m. UTC | #3 
Hi Parthiban,

LED_SEL is configurable option by EEPROM which should be populated on
EVB-LAN8670-USB. I would suggest changing EEPROM configuration than
hard-coded in driver code.

Thanks.
Woojung

> -----Original Message-----
> From: Parthiban Veerasooran <Parthiban.Veerasooran@microchip.com>
> Sent: Wednesday, May 22, 2024 10:08 AM
> To: steve.glendinning@shawell.net; UNGLinuxDriver
> <UNGLinuxDriver@microchip.com>; davem@davemloft.net; edumazet@google.com;
> kuba@kernel.org; pabeni@redhat.com
> Cc: netdev@vger.kernel.org; linux-usb@vger.kernel.org; linux-
> kernel@vger.kernel.org; Parthiban Veerasooran - I17164
> <Parthiban.Veerasooran@microchip.com>
> Subject: [PATCH] net: usb: smsc95xx: configure external LEDs function for
> EVB-LAN8670-USB
> 
> By default, LAN9500A configures the external LEDs to the below function.
> nSPD_LED -> Speed Indicator
> nLNKA_LED -> Link and Activity Indicator
> nFDX_LED -> Full Duplex Link Indicator
> 
> But, EVB-LAN8670-USB uses the below external LEDs function which can be
> enabled by writing 1 to the LED Select (LED_SEL) bit in the LAN9500A.
> nSPD_LED -> Speed Indicator
> nLNKA_LED -> Link Indicator
> nFDX_LED -> Activity Indicator
> 
> Signed-off-by: Parthiban Veerasooran <Parthiban.Veerasooran@microchip.com>
> ---
>  drivers/net/usb/smsc95xx.c | 12 ++++++++++++
>  drivers/net/usb/smsc95xx.h |  1 +
>  2 files changed, 13 insertions(+)
> 
> diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
> index cbea24666479..05975461bf10 100644
> --- a/drivers/net/usb/smsc95xx.c
> +++ b/drivers/net/usb/smsc95xx.c
> @@ -1006,6 +1006,18 @@ static int smsc95xx_reset(struct usbnet *dev)
>  	/* Configure GPIO pins as LED outputs */
>  	write_buf = LED_GPIO_CFG_SPD_LED | LED_GPIO_CFG_LNK_LED |
>  		LED_GPIO_CFG_FDX_LED;
> +
> +	/* Set LED Select (LED_SEL) bit for the external LED pins
> functionality
> +	 * in the Microchip's EVB-LAN8670-USB 10BASE-T1S Ethernet device which
> +	 * uses the below LED function.
> +	 * nSPD_LED -> Speed Indicator
> +	 * nLNKA_LED -> Link Indicator
> +	 * nFDX_LED -> Activity Indicator
> +	 */
> +	if (dev->udev->descriptor.idVendor == 0x184F &&
> +	    dev->udev->descriptor.idProduct == 0x0051)
> +		write_buf |= LED_GPIO_CFG_LED_SEL;
> +
>  	ret = smsc95xx_write_reg(dev, LED_GPIO_CFG, write_buf);
>  	if (ret < 0)
>  		return ret;
> diff --git a/drivers/net/usb/smsc95xx.h b/drivers/net/usb/smsc95xx.h
> index 013bf42e27f2..134f3c2fddd9 100644
> --- a/drivers/net/usb/smsc95xx.h
> +++ b/drivers/net/usb/smsc95xx.h
> @@ -114,6 +114,7 @@
> 
>  /* LED General Purpose IO Configuration Register */
>  #define LED_GPIO_CFG		(0x24)
> +#define LED_GPIO_CFG_LED_SEL	BIT(31)		/* Separate Link/Act LEDs */
>  #define LED_GPIO_CFG_SPD_LED	(0x01000000)	/* GPIOz as Speed LED */
>  #define LED_GPIO_CFG_LNK_LED	(0x00100000)	/* GPIOy as Link LED */
>  #define LED_GPIO_CFG_FDX_LED	(0x00010000)	/* GPIOx as Full Duplex LED
> */
> 
> base-commit: 4b377b4868ef17b040065bd468668c707d2477a5
> --
> 2.34.1
kernel test robot May 23, 2024, 5:42 a.m. UTC | #4 
Hi Parthiban,

kernel test robot noticed the following build warnings:

[auto build test WARNING on 4b377b4868ef17b040065bd468668c707d2477a5]

url:    https://github.com/intel-lab-lkp/linux/commits/Parthiban-Veerasooran/net-usb-smsc95xx-configure-external-LEDs-function-for-EVB-LAN8670-USB/20240522-221645
base:   4b377b4868ef17b040065bd468668c707d2477a5
patch link:    https://lore.kernel.org/r/20240522140817.409936-1-Parthiban.Veerasooran%40microchip.com
patch subject: [PATCH] net: usb: smsc95xx: configure external LEDs function for EVB-LAN8670-USB
config: i386-randconfig-062-20240523 (https://download.01.org/0day-ci/archive/20240523/202405231332.bBXpW9Bj-lkp@intel.com/config)
compiler: gcc-13 (Ubuntu 13.2.0-4ubuntu3) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240523/202405231332.bBXpW9Bj-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202405231332.bBXpW9Bj-lkp@intel.com/

sparse warnings: (new ones prefixed by >>)
>> drivers/net/usb/smsc95xx.c:1017:34: sparse: sparse: restricted __le16 degrades to integer
   drivers/net/usb/smsc95xx.c:1018:34: sparse: sparse: restricted __le16 degrades to integer

vim +1017 drivers/net/usb/smsc95xx.c

   878	
   879	static int smsc95xx_reset(struct usbnet *dev)
   880	{
   881		struct smsc95xx_priv *pdata = dev->driver_priv;
   882		u32 read_buf, write_buf, burst_cap;
   883		int ret = 0, timeout;
   884	
   885		netif_dbg(dev, ifup, dev->net, "entering smsc95xx_reset\n");
   886	
   887		ret = smsc95xx_write_reg(dev, HW_CFG, HW_CFG_LRST_);
   888		if (ret < 0)
   889			return ret;
   890	
   891		timeout = 0;
   892		do {
   893			msleep(10);
   894			ret = smsc95xx_read_reg(dev, HW_CFG, &read_buf);
   895			if (ret < 0)
   896				return ret;
   897			timeout++;
   898		} while ((read_buf & HW_CFG_LRST_) && (timeout < 100));
   899	
   900		if (timeout >= 100) {
   901			netdev_warn(dev->net, "timeout waiting for completion of Lite Reset\n");
   902			return -ETIMEDOUT;
   903		}
   904	
   905		ret = smsc95xx_set_mac_address(dev);
   906		if (ret < 0)
   907			return ret;
   908	
   909		netif_dbg(dev, ifup, dev->net, "MAC Address: %pM\n",
   910			  dev->net->dev_addr);
   911	
   912		ret = smsc95xx_read_reg(dev, HW_CFG, &read_buf);
   913		if (ret < 0)
   914			return ret;
   915	
   916		netif_dbg(dev, ifup, dev->net, "Read Value from HW_CFG : 0x%08x\n",
   917			  read_buf);
   918	
   919		read_buf |= HW_CFG_BIR_;
   920	
   921		ret = smsc95xx_write_reg(dev, HW_CFG, read_buf);
   922		if (ret < 0)
   923			return ret;
   924	
   925		ret = smsc95xx_read_reg(dev, HW_CFG, &read_buf);
   926		if (ret < 0)
   927			return ret;
   928	
   929		netif_dbg(dev, ifup, dev->net,
   930			  "Read Value from HW_CFG after writing HW_CFG_BIR_: 0x%08x\n",
   931			  read_buf);
   932	
   933		if (!turbo_mode) {
   934			burst_cap = 0;
   935			dev->rx_urb_size = MAX_SINGLE_PACKET_SIZE;
   936		} else if (dev->udev->speed == USB_SPEED_HIGH) {
   937			burst_cap = DEFAULT_HS_BURST_CAP_SIZE / HS_USB_PKT_SIZE;
   938			dev->rx_urb_size = DEFAULT_HS_BURST_CAP_SIZE;
   939		} else {
   940			burst_cap = DEFAULT_FS_BURST_CAP_SIZE / FS_USB_PKT_SIZE;
   941			dev->rx_urb_size = DEFAULT_FS_BURST_CAP_SIZE;
   942		}
   943	
   944		netif_dbg(dev, ifup, dev->net, "rx_urb_size=%ld\n",
   945			  (ulong)dev->rx_urb_size);
   946	
   947		ret = smsc95xx_write_reg(dev, BURST_CAP, burst_cap);
   948		if (ret < 0)
   949			return ret;
   950	
   951		ret = smsc95xx_read_reg(dev, BURST_CAP, &read_buf);
   952		if (ret < 0)
   953			return ret;
   954	
   955		netif_dbg(dev, ifup, dev->net,
   956			  "Read Value from BURST_CAP after writing: 0x%08x\n",
   957			  read_buf);
   958	
   959		ret = smsc95xx_write_reg(dev, BULK_IN_DLY, DEFAULT_BULK_IN_DELAY);
   960		if (ret < 0)
   961			return ret;
   962	
   963		ret = smsc95xx_read_reg(dev, BULK_IN_DLY, &read_buf);
   964		if (ret < 0)
   965			return ret;
   966	
   967		netif_dbg(dev, ifup, dev->net,
   968			  "Read Value from BULK_IN_DLY after writing: 0x%08x\n",
   969			  read_buf);
   970	
   971		ret = smsc95xx_read_reg(dev, HW_CFG, &read_buf);
   972		if (ret < 0)
   973			return ret;
   974	
   975		netif_dbg(dev, ifup, dev->net, "Read Value from HW_CFG: 0x%08x\n",
   976			  read_buf);
   977	
   978		if (turbo_mode)
   979			read_buf |= (HW_CFG_MEF_ | HW_CFG_BCE_);
   980	
   981		read_buf &= ~HW_CFG_RXDOFF_;
   982	
   983		/* set Rx data offset=2, Make IP header aligns on word boundary. */
   984		read_buf |= NET_IP_ALIGN << 9;
   985	
   986		ret = smsc95xx_write_reg(dev, HW_CFG, read_buf);
   987		if (ret < 0)
   988			return ret;
   989	
   990		ret = smsc95xx_read_reg(dev, HW_CFG, &read_buf);
   991		if (ret < 0)
   992			return ret;
   993	
   994		netif_dbg(dev, ifup, dev->net,
   995			  "Read Value from HW_CFG after writing: 0x%08x\n", read_buf);
   996	
   997		ret = smsc95xx_write_reg(dev, INT_STS, INT_STS_CLEAR_ALL_);
   998		if (ret < 0)
   999			return ret;
  1000	
  1001		ret = smsc95xx_read_reg(dev, ID_REV, &read_buf);
  1002		if (ret < 0)
  1003			return ret;
  1004		netif_dbg(dev, ifup, dev->net, "ID_REV = 0x%08x\n", read_buf);
  1005	
  1006		/* Configure GPIO pins as LED outputs */
  1007		write_buf = LED_GPIO_CFG_SPD_LED | LED_GPIO_CFG_LNK_LED |
  1008			LED_GPIO_CFG_FDX_LED;
  1009	
  1010		/* Set LED Select (LED_SEL) bit for the external LED pins functionality
  1011		 * in the Microchip's EVB-LAN8670-USB 10BASE-T1S Ethernet device which
  1012		 * uses the below LED function.
  1013		 * nSPD_LED -> Speed Indicator
  1014		 * nLNKA_LED -> Link Indicator
  1015		 * nFDX_LED -> Activity Indicator
  1016		 */
> 1017		if (dev->udev->descriptor.idVendor == 0x184F &&
  1018		    dev->udev->descriptor.idProduct == 0x0051)
  1019			write_buf |= LED_GPIO_CFG_LED_SEL;
  1020	
  1021		ret = smsc95xx_write_reg(dev, LED_GPIO_CFG, write_buf);
  1022		if (ret < 0)
  1023			return ret;
  1024	
  1025		/* Init Tx */
  1026		ret = smsc95xx_write_reg(dev, FLOW, 0);
  1027		if (ret < 0)
  1028			return ret;
  1029	
  1030		ret = smsc95xx_write_reg(dev, AFC_CFG, AFC_CFG_DEFAULT);
  1031		if (ret < 0)
  1032			return ret;
  1033	
  1034		/* Don't need mac_cr_lock during initialisation */
  1035		ret = smsc95xx_read_reg(dev, MAC_CR, &pdata->mac_cr);
  1036		if (ret < 0)
  1037			return ret;
  1038	
  1039		/* Init Rx */
  1040		/* Set Vlan */
  1041		ret = smsc95xx_write_reg(dev, VLAN1, (u32)ETH_P_8021Q);
  1042		if (ret < 0)
  1043			return ret;
  1044	
  1045		/* Enable or disable checksum offload engines */
  1046		ret = smsc95xx_set_features(dev->net, dev->net->features);
  1047		if (ret < 0) {
  1048			netdev_warn(dev->net, "Failed to set checksum offload features\n");
  1049			return ret;
  1050		}
  1051	
  1052		smsc95xx_set_multicast(dev->net);
  1053	
  1054		ret = smsc95xx_read_reg(dev, INT_EP_CTL, &read_buf);
  1055		if (ret < 0)
  1056			return ret;
  1057	
  1058		/* enable PHY interrupts */
  1059		read_buf |= INT_EP_CTL_PHY_INT_;
  1060	
  1061		ret = smsc95xx_write_reg(dev, INT_EP_CTL, read_buf);
  1062		if (ret < 0)
  1063			return ret;
  1064	
  1065		ret = smsc95xx_start_tx_path(dev);
  1066		if (ret < 0) {
  1067			netdev_warn(dev->net, "Failed to start TX path\n");
  1068			return ret;
  1069		}
  1070	
  1071		ret = smsc95xx_start_rx_path(dev);
  1072		if (ret < 0) {
  1073			netdev_warn(dev->net, "Failed to start RX path\n");
  1074			return ret;
  1075		}
  1076	
  1077		netif_dbg(dev, ifup, dev->net, "smsc95xx_reset, return 0\n");
  1078		return 0;
  1079	}
  1080
Parthiban Veerasooran May 23, 2024, 8:51 a.m. UTC | #5 
Hi Woojung,

On 23/05/24 1:22 am, Woojung Huh - C21699 wrote:
> Hi Parthiban,
> 
> LED_SEL is configurable option by EEPROM which should be populated on
> EVB-LAN8670-USB. I would suggest changing EEPROM configuration than
> hard-coded in driver code.
Ah OK. Thanks for letting me know. I tried that EEPROM approach but that 
is needed a fix to work properly. I will send out another fix patch 
separately. Please review it.

Please discard this patch as it is going to be invalid.

Thanks for your understanding.

Best regards,
Parthiban V
> 
> Thanks.
> Woojung
> 
>> -----Original Message-----
>> From: Parthiban Veerasooran <Parthiban.Veerasooran@microchip.com>
>> Sent: Wednesday, May 22, 2024 10:08 AM
>> To: steve.glendinning@shawell.net; UNGLinuxDriver
>> <UNGLinuxDriver@microchip.com>; davem@davemloft.net; edumazet@google.com;
>> kuba@kernel.org; pabeni@redhat.com
>> Cc: netdev@vger.kernel.org; linux-usb@vger.kernel.org; linux-
>> kernel@vger.kernel.org; Parthiban Veerasooran - I17164
>> <Parthiban.Veerasooran@microchip.com>
>> Subject: [PATCH] net: usb: smsc95xx: configure external LEDs function for
>> EVB-LAN8670-USB
>>
>> By default, LAN9500A configures the external LEDs to the below function.
>> nSPD_LED -> Speed Indicator
>> nLNKA_LED -> Link and Activity Indicator
>> nFDX_LED -> Full Duplex Link Indicator
>>
>> But, EVB-LAN8670-USB uses the below external LEDs function which can be
>> enabled by writing 1 to the LED Select (LED_SEL) bit in the LAN9500A.
>> nSPD_LED -> Speed Indicator
>> nLNKA_LED -> Link Indicator
>> nFDX_LED -> Activity Indicator
>>
>> Signed-off-by: Parthiban Veerasooran <Parthiban.Veerasooran@microchip.com>
>> ---
>>   drivers/net/usb/smsc95xx.c | 12 ++++++++++++
>>   drivers/net/usb/smsc95xx.h |  1 +
>>   2 files changed, 13 insertions(+)
>>
>> diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
>> index cbea24666479..05975461bf10 100644
>> --- a/drivers/net/usb/smsc95xx.c
>> +++ b/drivers/net/usb/smsc95xx.c
>> @@ -1006,6 +1006,18 @@ static int smsc95xx_reset(struct usbnet *dev)
>>   	/* Configure GPIO pins as LED outputs */
>>   	write_buf = LED_GPIO_CFG_SPD_LED | LED_GPIO_CFG_LNK_LED |
>>   		LED_GPIO_CFG_FDX_LED;
>> +
>> +	/* Set LED Select (LED_SEL) bit for the external LED pins
>> functionality
>> +	 * in the Microchip's EVB-LAN8670-USB 10BASE-T1S Ethernet device which
>> +	 * uses the below LED function.
>> +	 * nSPD_LED -> Speed Indicator
>> +	 * nLNKA_LED -> Link Indicator
>> +	 * nFDX_LED -> Activity Indicator
>> +	 */
>> +	if (dev->udev->descriptor.idVendor == 0x184F &&
>> +	    dev->udev->descriptor.idProduct == 0x0051)
>> +		write_buf |= LED_GPIO_CFG_LED_SEL;
>> +
>>   	ret = smsc95xx_write_reg(dev, LED_GPIO_CFG, write_buf);
>>   	if (ret < 0)
>>   		return ret;
>> diff --git a/drivers/net/usb/smsc95xx.h b/drivers/net/usb/smsc95xx.h
>> index 013bf42e27f2..134f3c2fddd9 100644
>> --- a/drivers/net/usb/smsc95xx.h
>> +++ b/drivers/net/usb/smsc95xx.h
>> @@ -114,6 +114,7 @@
>>
>>   /* LED General Purpose IO Configuration Register */
>>   #define LED_GPIO_CFG		(0x24)
>> +#define LED_GPIO_CFG_LED_SEL	BIT(31)		/* Separate Link/Act LEDs */
>>   #define LED_GPIO_CFG_SPD_LED	(0x01000000)	/* GPIOz as Speed LED */
>>   #define LED_GPIO_CFG_LNK_LED	(0x00100000)	/* GPIOy as Link LED */
>>   #define LED_GPIO_CFG_FDX_LED	(0x00010000)	/* GPIOx as Full Duplex LED
>> */
>>
>> base-commit: 4b377b4868ef17b040065bd468668c707d2477a5
>> --
>> 2.34.1
>
Parthiban Veerasooran May 23, 2024, 8:51 a.m. UTC | #6 
Hi Simon,

On 23/05/24 12:24 am, Simon Horman wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Wed, May 22, 2024 at 07:38:17PM +0530, Parthiban Veerasooran wrote:
>> By default, LAN9500A configures the external LEDs to the below function.
>> nSPD_LED -> Speed Indicator
>> nLNKA_LED -> Link and Activity Indicator
>> nFDX_LED -> Full Duplex Link Indicator
>>
>> But, EVB-LAN8670-USB uses the below external LEDs function which can be
>> enabled by writing 1 to the LED Select (LED_SEL) bit in the LAN9500A.
>> nSPD_LED -> Speed Indicator
>> nLNKA_LED -> Link Indicator
>> nFDX_LED -> Activity Indicator
>>
>> Signed-off-by: Parthiban Veerasooran <Parthiban.Veerasooran@microchip.com>
>> ---
>>   drivers/net/usb/smsc95xx.c | 12 ++++++++++++
>>   drivers/net/usb/smsc95xx.h |  1 +
>>   2 files changed, 13 insertions(+)
>>
>> diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
>> index cbea24666479..05975461bf10 100644
>> --- a/drivers/net/usb/smsc95xx.c
>> +++ b/drivers/net/usb/smsc95xx.c
>> @@ -1006,6 +1006,18 @@ static int smsc95xx_reset(struct usbnet *dev)
>>        /* Configure GPIO pins as LED outputs */
>>        write_buf = LED_GPIO_CFG_SPD_LED | LED_GPIO_CFG_LNK_LED |
>>                LED_GPIO_CFG_FDX_LED;
>> +
>> +     /* Set LED Select (LED_SEL) bit for the external LED pins functionality
>> +      * in the Microchip's EVB-LAN8670-USB 10BASE-T1S Ethernet device which
>> +      * uses the below LED function.
>> +      * nSPD_LED -> Speed Indicator
>> +      * nLNKA_LED -> Link Indicator
>> +      * nFDX_LED -> Activity Indicator
>> +      */
>> +     if (dev->udev->descriptor.idVendor == 0x184F &&
>> +         dev->udev->descriptor.idProduct == 0x0051)
> 
> Hi Parthiban,
> 
> There seems to be an endian missmatch here.
> The type of .idVendor and .idProduct is __le16,
> but here they are compared against host byte-order integers.
Sorry, somehow I missed to check this. Will try to avoid this in the 
future. Thanks for letting me know.

But I am not going to proceed further with this patch as Woojung pointed 
out EEPROM approach can be used instead of updating the driver. So 
please discard this patch. But the EEPROM approach mentioned by Woojung 
is needed a fix in the driver to work properly. I will send out that fix 
patch separately. Please review it.

Thanks for your understanding.

Best regards,
Parthiban V
> 
> Flagged by Sparse.
> 
>> +             write_buf |= LED_GPIO_CFG_LED_SEL;
>> +
>>        ret = smsc95xx_write_reg(dev, LED_GPIO_CFG, write_buf);
>>        if (ret < 0)
>>                return ret;
> 
> ...
> 
> --
> pw-bot: changes-requested
>
Parthiban Veerasooran May 23, 2024, 8:51 a.m. UTC | #7 
Hi Andrew,

On 22/05/24 10:14 pm, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Wed, May 22, 2024 at 07:38:17PM +0530, Parthiban Veerasooran wrote:
>> By default, LAN9500A configures the external LEDs to the below function.
>> nSPD_LED -> Speed Indicator
>> nLNKA_LED -> Link and Activity Indicator
>> nFDX_LED -> Full Duplex Link Indicator
>>
>> But, EVB-LAN8670-USB uses the below external LEDs function which can be
>> enabled by writing 1 to the LED Select (LED_SEL) bit in the LAN9500A.
>> nSPD_LED -> Speed Indicator
>> nLNKA_LED -> Link Indicator
>> nFDX_LED -> Activity Indicator
> 
> What else can the LEDs indicate?
There is no other indications.
> 
>> +     /* Set LED Select (LED_SEL) bit for the external LED pins functionality
>> +      * in the Microchip's EVB-LAN8670-USB 10BASE-T1S Ethernet device which
> 
> Is this a function of the USB dongle? Or a function of the PHY?
It is the function of USB dongle.

Sorry Andrew, I am not going to proceed further with this patch as 
Woojung pointed out EEPROM approach can be used instead of updating the 
driver. I tried that as well. So please discard this patch. But the 
EEPROM approach mentioned by Woojung is needed a fix in the driver to 
work properly. I will send out that fix patch separately. Please review it.

Thanks for your understanding.

Best regards,
Parthiban V
> 
>          Andrew
Andrew Lunn May 23, 2024, 12:43 p.m. UTC | #8 
On Thu, May 23, 2024 at 08:51:54AM +0000, Parthiban.Veerasooran@microchip.com wrote:
> Hi Andrew,
> 
> On 22/05/24 10:14 pm, Andrew Lunn wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> > 
> > On Wed, May 22, 2024 at 07:38:17PM +0530, Parthiban Veerasooran wrote:
> >> By default, LAN9500A configures the external LEDs to the below function.
> >> nSPD_LED -> Speed Indicator
> >> nLNKA_LED -> Link and Activity Indicator
> >> nFDX_LED -> Full Duplex Link Indicator
> >>
> >> But, EVB-LAN8670-USB uses the below external LEDs function which can be
> >> enabled by writing 1 to the LED Select (LED_SEL) bit in the LAN9500A.
> >> nSPD_LED -> Speed Indicator
> >> nLNKA_LED -> Link Indicator
> >> nFDX_LED -> Activity Indicator
> > 
> > What else can the LEDs indicate?
> There is no other indications.

O.K. So it is probably not worth going the direction of using the
netde LED infrastructure to allow the use to configure the LED.

> >> +     /* Set LED Select (LED_SEL) bit for the external LED pins functionality
> >> +      * in the Microchip's EVB-LAN8670-USB 10BASE-T1S Ethernet device which
> > 
> > Is this a function of the USB dongle? Or a function of the PHY?
> It is the function of USB dongle.

So an OEM designing a dongle could make the LEDs do different things?

You are solving the problem only for your reference design, and OEMs
are going to have to solve the same problem for their own design?

This is why i'm asking is it a function of the PHY or the board. If it
is the PHY, we could have one generic solution for everybody using
that PHY.

	Andrew
Parthiban Veerasooran May 24, 2024, 10:48 a.m. UTC | #9 
Hi Andrew,

On 23/05/24 6:13 pm, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Thu, May 23, 2024 at 08:51:54AM +0000, Parthiban.Veerasooran@microchip.com wrote:
>> Hi Andrew,
>>
>> On 22/05/24 10:14 pm, Andrew Lunn wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>
>>> On Wed, May 22, 2024 at 07:38:17PM +0530, Parthiban Veerasooran wrote:
>>>> By default, LAN9500A configures the external LEDs to the below function.
>>>> nSPD_LED -> Speed Indicator
>>>> nLNKA_LED -> Link and Activity Indicator
>>>> nFDX_LED -> Full Duplex Link Indicator
>>>>
>>>> But, EVB-LAN8670-USB uses the below external LEDs function which can be
>>>> enabled by writing 1 to the LED Select (LED_SEL) bit in the LAN9500A.
>>>> nSPD_LED -> Speed Indicator
>>>> nLNKA_LED -> Link Indicator
>>>> nFDX_LED -> Activity Indicator
>>>
>>> What else can the LEDs indicate?
>> There is no other indications.
> 
> O.K. So it is probably not worth going the direction of using the
> netde LED infrastructure to allow the use to configure the LED.
> 
>>>> +     /* Set LED Select (LED_SEL) bit for the external LED pins functionality
>>>> +      * in the Microchip's EVB-LAN8670-USB 10BASE-T1S Ethernet device which
>>>
>>> Is this a function of the USB dongle? Or a function of the PHY?
>> It is the function of USB dongle.
> 
> So an OEM designing a dongle could make the LEDs do different things?
Yes.
> 
> You are solving the problem only for your reference design, and OEMs
> are going to have to solve the same problem for their own design?
> 
> This is why i'm asking is it a function of the PHY or the board. If it
> is the PHY, we could have one generic solution for everybody using
> that PHY.
OK, it is a function of the board not PHY and also that depends on the 
board design based on the requirement I guess.

Best regards,
Parthiban V
> 
>          Andrew
Oliver Neukum May 27, 2024, 8:28 a.m. UTC | #10 
On 22.05.24 16:08, Parthiban Veerasooran wrote:

Hi,

however you solve this, the descriptors are stored in wire order.

> +	if (dev->udev->descriptor.idVendor == 0x184F &&
> +	    dev->udev->descriptor.idProduct == 0x0051)
> +		write_buf |= LED_GPIO_CFG_LED_SEL;

This needs to be

if (dev->udev->descriptor.idVendor == cpu_to_le16(0x184F) &&
	dev->udev->descriptor.idProduct == cpu_to_le16(0x0051))

	HTH
		Oliver
Parthiban Veerasooran May 27, 2024, 11:42 a.m. UTC | #11 
Hi Oliver,

On 27/05/24 1:58 pm, Oliver Neukum wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know 
> the content is safe
> 
> On 22.05.24 16:08, Parthiban Veerasooran wrote:
> 
> Hi,
> 
> however you solve this, the descriptors are stored in wire order.
> 
>> +     if (dev->udev->descriptor.idVendor == 0x184F &&
>> +         dev->udev->descriptor.idProduct == 0x0051)
>> +             write_buf |= LED_GPIO_CFG_LED_SEL;
> 
> This needs to be
> 
> if (dev->udev->descriptor.idVendor == cpu_to_le16(0x184F) &&
>         dev->udev->descriptor.idProduct == cpu_to_le16(0x0051))
> 
>         HTH
>                 Oliver
Thanks for reviewing the patch. This one was already pointed out by 
Simon Horman and kernel test robot. I agreed that but unfortunately 
there was an another proposal by Woojung with EEPROM. So I asked to 
discard this patch proceeding further and sent out another fix patch for 
supporting with Woojung's EEPROM proposal.

My request to discard this patch:
---------------------------------
https://lore.kernel.org/netdev/3e2b2b18-4dd9-4b22-9690-01e1bdd44828@microchip.com/

My request to review the other/new patch:
-----------------------------------------
https://lore.kernel.org/netdev/20240523085314.167650-1-Parthiban.Veerasooran@microchip.com/

The above patch has been already reviewed by both Woojung and Simon:
--------------------------------------------------------------------
https://lore.kernel.org/netdev/20240523144056.GO883722@kernel.org/

https://lore.kernel.org/netdev/BL0PR11MB2913CE81AFC08F2D619C3B6CE7F42@BL0PR11MB2913.namprd11.prod.outlook.com/

Hope this clarifies.

Best regards,
Parthiban V
diff mbox series

Patch

diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
index cbea24666479..05975461bf10 100644
--- a/drivers/net/usb/smsc95xx.c
+++ b/drivers/net/usb/smsc95xx.c
@@ -1006,6 +1006,18 @@  static int smsc95xx_reset(struct usbnet *dev)
 	/* Configure GPIO pins as LED outputs */
 	write_buf = LED_GPIO_CFG_SPD_LED | LED_GPIO_CFG_LNK_LED |
 		LED_GPIO_CFG_FDX_LED;
+
+	/* Set LED Select (LED_SEL) bit for the external LED pins functionality
+	 * in the Microchip's EVB-LAN8670-USB 10BASE-T1S Ethernet device which
+	 * uses the below LED function.
+	 * nSPD_LED -> Speed Indicator
+	 * nLNKA_LED -> Link Indicator
+	 * nFDX_LED -> Activity Indicator
+	 */
+	if (dev->udev->descriptor.idVendor == 0x184F &&
+	    dev->udev->descriptor.idProduct == 0x0051)
+		write_buf |= LED_GPIO_CFG_LED_SEL;
+
 	ret = smsc95xx_write_reg(dev, LED_GPIO_CFG, write_buf);
 	if (ret < 0)
 		return ret;
diff --git a/drivers/net/usb/smsc95xx.h b/drivers/net/usb/smsc95xx.h
index 013bf42e27f2..134f3c2fddd9 100644
--- a/drivers/net/usb/smsc95xx.h
+++ b/drivers/net/usb/smsc95xx.h
@@ -114,6 +114,7 @@ 
 
 /* LED General Purpose IO Configuration Register */
 #define LED_GPIO_CFG		(0x24)
+#define LED_GPIO_CFG_LED_SEL	BIT(31)		/* Separate Link/Act LEDs */
 #define LED_GPIO_CFG_SPD_LED	(0x01000000)	/* GPIOz as Speed LED */
 #define LED_GPIO_CFG_LNK_LED	(0x00100000)	/* GPIOy as Link LED */
 #define LED_GPIO_CFG_FDX_LED	(0x00010000)	/* GPIOx as Full Duplex LED */