Re: [PATCH] net: usb: smsc95xx: configure external LEDs function for EVB-LAN8670-USB

[Parthiban.Veerasooran]

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@xxxxxxxxxxxxx>
>> ---
>>   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
>