Hi Wolfgang,
On Mon, Nov 14, 2011 at 4:18 AM, Wolfgang Grandegger <w...@grandegger.com>
wrote:
> Hi Simon,
>
> On 11/11/2011 04:35 PM, Simon Glass wrote:
>> Hi Wolfgang,
>>
>> On Fri, Nov 11, 2011 at 2:59 AM, Wolfgang Grandegger <w...@denx.de> wrote:
>>> The write to the mac_cr register was missing. This usually not
>>> cause an issue before, since the next function writing the
>>> register's shadow copy into the register would do it as a side
>>> effect.
>>>
>>> Signed-off-by: Wolfgang Grandegger <w...@denx.de>
>>> Cc: Simon Glass <s...@chromium.org>
>>> ---
>>> drivers/usb/eth/smsc95xx.c | 2 ++
>>> 1 files changed, 2 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/drivers/usb/eth/smsc95xx.c b/drivers/usb/eth/smsc95xx.c
>>> index eb529f1..16e24bd 100644
>>> --- a/drivers/usb/eth/smsc95xx.c
>>> +++ b/drivers/usb/eth/smsc95xx.c
>>> @@ -428,6 +428,8 @@ static void smsc95xx_set_multicast(struct ueth_data
>>> *dev)
>>> {
>>> /* No multicast in u-boot */
>>> dev->mac_cr &= ~(MAC_CR_PRMS_ | MAC_CR_MCPAS_ | MAC_CR_HPFILT_);
>>> +
>>> + smsc95xx_write_reg(dev, MAC_CR, dev->mac_cr);
>>
>> It seems a bit odd - what problem does your addition actually fix? At
>> present both smsc95xx_start_tx_path() and smsc95xx_start_rx_path()
>> write to this register, so it will be written three times in quick
>> succession. If there are no other callers to smsc95xx_set_multicast()
>> outside init, then perhaps we should just write it once in init, after
>> calling the three functions?
>
> The name "set_multicast" associated to me that the relevant register is
> really set.
Yes, although it is a static function.
> But from the functional poit of few, it is not necessary.
> Well, it's a minor issue and maybe not worth fixing. So, let's drop this
> patch.
If you like, although I agree a clean-up would be useful here.
Regards,
Simon
>
> Wolfgang.
>
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
