[rt2x00-users] [PATCH4/4] rt2x00: Add support for RT5390 chip

[RA-Shiang Tu]

Hi Helmut,

Thanks for your comments patiently. :)
I'll try this tool before submit the patches.

For your questions, please see my inline comments.


> -----Original Message-----
> From: Helmut Schaa [mailto:helmut.schaa at googlemail.com]
> Sent: Tuesday, February 08, 2011 8:09 PM
> To: users at rt2x00.serialmonkey.com
> Cc: RA-Shiang Tu; gregkh at suse.de
> Subject: Re: [rt2x00-users] [PATCH4/4] rt2x00: Add support
> for RT5390 chip
>
> Hi Shiang,
>
> it would make sense to always run checkpatch.pl on your
> patches prior to submission. In that way you can avoid quite
> a number of comments.
>
> See inlined comments.
>
> Am Dienstag, 8. Februar 2011 schrieb RA-Shiang Tu:
> > Add code segments to support RT5390 chip
> >
> > Signed-off-by: Shiang Tu <shiang_tu at ralinktech.com>
> > ---
> >
> > @@ -3177,11 +3489,17 @@ int rt2800_init_eeprom(struct
> rt2x00_dev *rt2x00dev)
> >             !rt2x00_rt(rt2x00dev, RT3071) &&
> >             !rt2x00_rt(rt2x00dev, RT3090) &&
> >             !rt2x00_rt(rt2x00dev, RT3390) &&
> > -           !rt2x00_rt(rt2x00dev, RT3572)) {
> > +           !rt2x00_rt(rt2x00dev, RT3572) &&
> > +           !rt2x00_rt(rt2x00dev, RT5390)) {
> >                 ERROR(rt2x00dev, "Invalid RT chipset detected.\n");
> >                 return -ENODEV;
> >         }
> >
> > +       if (rt2x00_rt(rt2x00dev, RT5390)) {
> > +               rt2x00_eeprom_read(rt2x00dev,
> EEPROM_CHIP_ID, &value);
> > +               rt2x00_set_chip(rt2x00dev, RT5390, value,
> > + rt2x00_rev(rt2x00dev));
>
> Is the chip id eeprom value also available on older hardware?

Yes, it is available on older hardware too.
But for rt53xx series (and upcoming new single chips), the "EEPROM_NIC_CONF0_RF_TYPE" are no more used for the identification of rf type, instead  it use this field to idnetification the rf type.

>
> > +       }
> > +
> >         if (!rt2x00_rf(rt2x00dev, RF2820) &&
> >             !rt2x00_rf(rt2x00dev, RF2850) &&
> >             !rt2x00_rf(rt2x00dev, RF2720) && @@ -3191,7
> +3509,8 @@ int
> > rt2800_init_eeprom(struct rt2x00_dev *rt2x00dev)
> >             !rt2x00_rf(rt2x00dev, RF3021) &&
> >             !rt2x00_rf(rt2x00dev, RF3022) &&
> >             !rt2x00_rf(rt2x00dev, RF3052) &&
> > -           !rt2x00_rf(rt2x00dev, RF3320)) {
> > +           !rt2x00_rf(rt2x00dev, RF3320) &&
> > +           !rt2x00_rf(rt2x00dev, RF5390)) {
> >                 ERROR(rt2x00dev, "Invalid RF chipset detected.\n");
> >                 return -ENODEV;
> >         }
> > @@ -3250,6 +3569,9 @@ int rt2800_init_eeprom(struct
> rt2x00_dev *rt2x00dev)
> >         if (rt2x00_get_field16(eeprom, EEPROM_NIC_CONF1_HW_RADIO))
> >                 __set_bit(CONFIG_SUPPORT_HW_BUTTON,
> > &rt2x00dev->flags);
> >
> > +       if (rt2x00_get_field16(eeprom, EEPROM_NIC_CONF1_BT_COEXIST))
> > +               __set_bit(CONFIG_SUPPORT_BT_COEXIST,
> > + &rt2x00dev->flags);
> > +
>
> Couldn't we just check the eeprom value directly in every
> place that needs to know if we support bt coext? I mean, the
> eeprom data is cached in memory and we could save one flag
> that's only used by rt2800pci so far.
>

I add this flag just thought this is some kind of "Driver features" and try to reduce the duplicated eeprom read/get_field operations.
Sure, I can change it because now only few place need to check this.

>
>
> Thanks a lot,
> Helmut
>
CONFIDENTIALITY STATEMENT : The information, attachments and any rights attaching in this e-mail are confidential and privileged; it is intended only for the individual or entity named as the recipient hereof.Any disclosure, copying, distribution, dissemination or use of the contents of this e-mail by persons other than the intended recipient is STRICTLY PROHIBITED and may violate applicable laws.If you have received this e-mail in error, please delete the original message and notify us by return email or collect call immediately. Thank you.