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 <[email protected]>
---
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
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
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 <[email protected]>
> ---
> 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;
..
--
pw-bot: changes-requested
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 <[email protected]>
> Sent: Wednesday, May 22, 2024 10:08 AM
> To: [email protected]; UNGLinuxDriver
> <[email protected]>; [email protected]; [email protected];
> [email protected]; [email protected]
> Cc: [email protected]; [email protected]; linux-
> [email protected]; Parthiban Veerasooran - I17164
> <[email protected]>
> 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 <[email protected]>
> ---
> 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
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/[email protected]/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/[email protected]/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 <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
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
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
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 <[email protected]>
>> Sent: Wednesday, May 22, 2024 10:08 AM
>> To: [email protected]; UNGLinuxDriver
>> <[email protected]>; [email protected]; [email protected];
>> [email protected]; [email protected]
>> Cc: [email protected]; [email protected]; linux-
>> [email protected]; Parthiban Veerasooran - I17164
>> <[email protected]>
>> 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 <[email protected]>
>> ---
>> 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
>
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 <[email protected]>
>> ---
>> 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
>
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
On Thu, May 23, 2024 at 08:51:54AM +0000, [email protected] 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
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, [email protected] 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
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
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/[email protected]/
My request to review the other/new patch:
-----------------------------------------
https://lore.kernel.org/netdev/[email protected]/
The above patch has been already reviewed by both Woojung and Simon:
--------------------------------------------------------------------
https://lore.kernel.org/netdev/[email protected]/
https://lore.kernel.org/netdev/BL0PR11MB2913CE81AFC08F2D619C3B6CE7F42@BL0PR11MB2913.namprd11.prod.outlook.com/
Hope this clarifies.
Best regards,
Parthiban V