Re: [PATCH v2] Add support for GPIOs for SMSC LAN95xx chips.

[Evgeny Boger <boger-hVk9LwgH4SrGCOCKMErq+g@public.gmane.org>]

06/25/2014 02:34 PM, Daniele Forsi пишет:
> 2014-06-25 6:46 GMT+02:00 Evgeny Boger:
>
>> There might be 11 GPIOs in total.
> do you mean "12 GPIOs"? You say later they are 0-based and the last one is "11"


>
>> Last three GPIOs  (offsets 8-11, 0-based) are shared with FDX, LNKA, SPD
>> LEDs respectively.
> so you mean the last "four"?
> and you may want to remove the extra space before the open parenthesis

My mistake. 11 GPIOs in total, eight of them (0-7) are normal GPIOs, 
while three other are multiplexed with link activity LEDs.
So it should read like "offset 8-10, 0-based".

Actually, the numbering scheme according to datasheets differs a bit 
between LAN9500 and LAN951x:

For LAN951x:
GPIOs with offsets 0-7 are named "GPIO3" - "GPIO7", offsets 8-10 are for 
"GPIO0" - "GPIO2" (these three are multiplexed with nFDX_LED, nLNKA_LED, 
nSPD_LED).

For LAN9500:
The datasheet name is the same as the corresponding offset, i.e. offsets 
0-10 are for "GPIO0"-"GPIO10".

What do you think, shoud I write some note about this numbering scheme? 
If so, where it would be better to place such a note?






>
> also there are still several unneeded newlines
>
>> diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
>> @@ -68,6 +70,15 @@ struct smsc95xx_priv {
>>          spinlock_t mac_cr_lock;
>>          u8 features;
>>          u8 suspend_flags;
>> +
>> +       struct usbnet *dev;
>> +static int smsc95xx_gpio_request(struct gpio_chip *gpio, unsigned offset)
>> +       mutex_unlock(&pdata->gpio_lock);
>> +
>> +
>> +       return (ret < 0) ? ret : 0;
>> +}
>> +
>> +static void smsc95xx_gpio_free(struct gpio_chip *gpio, unsigned offset)
>> +       if (ret < 0)
>> +               netif_err(pdata->dev, ifdown, pdata->dev->net,
>> +                       "error freeing gpio %d\n", offset);
>> +
>> +}
> and in other places
>
>> +static void smsc95xx_gpio_set(struct gpio_chip *gpio, unsigned offset,
>> +                               int value)
>> +{
>> +       if (ret < 0) {
>> +               netif_err(pdata->dev, ifdown, pdata->dev->net,
>> +                       "error writing gpio %d=%d\n", offset, value);
>> +               return;
>> +       }
>> +}
> no need to put a "return" there at he end of the function (if it's
> defensive programming then you didn't put return in similar code in a
> previous function)
>


-- 
С уважением,
     Евгений Богер
     ООО Бесконтактные устройства
     http://contactless.ru
     +7 (919) 965 88 36

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html