Re: [PATCH net v11 3/3] net/8390: apne.c - add 100 Mbit support to apne.c driver

[Michael Schmitz <schmitzmic@gmail.com>]

Hi Geert,

On 18/11/21 21:01, Geert Uytterhoeven wrote:
>>>> --- a/drivers/net/ethernet/8390/apne.c
>>>> +++ b/drivers/net/ethernet/8390/apne.c
>>>> @@ -119,6 +119,48 @@ static u32 apne_msg_enable;
>>>>  module_param_named(msg_enable, apne_msg_enable, uint, 0444);
>>>>  MODULE_PARM_DESC(msg_enable, "Debug message level (see linux/netdevice.h for bitmap)");
>>>>
>>>> +static int apne_100_mbit = -1;
>>>> +module_param_named(100_mbit, apne_100_mbit, int, 0444);
>>>> +MODULE_PARM_DESC(100_mbit, "Enable 100 Mbit support");
>>>> +
>>>> +#if IS_ENABLED(CONFIG_PCMCIA)
>>>
>>> What if CONFIG_PCMIA=m, and CONFIG_APNE=y?
>>
>> Fails to build (undefined reference to `pcmcia_parse_tuple').
>>
>> That's what 'select PCMCIA' was avoiding before, but got vetoed. I can
>> add a dependency on PCMCIA in the APNE Kconfig entry which does force
>> APNE the same as what's selected for PCMCIA, but that means we can't
>> build APNE without PCMCIA anymore. Is there a way to express 'constrain
>> build type if PCMCIA is enabled, else leave choice to user' ??
>
> #if IS_REACHABLE(CONFIG_PCMIA)

Thanks, I'll use that then.

>>>> @@ -140,6 +182,13 @@ static struct net_device * __init apne_probe(void)
>>>>
>>>>         pr_info("Looking for PCMCIA ethernet card : ");
>>>>
>>>> +       if (apne_100_mbit == 1)
>>>> +               isa_type = ISA_TYPE_AG16;
>>>> +       else if (apne_100_mbit == 0)
>>>> +               isa_type = ISA_TYPE_AG;
>>>> +       else
>>>> +               pr_cont(" (autoprobing 16 bit mode) ");
>>>> +
>>>>         /* check if a card is inserted */
>>>>         if (!(PCMCIA_INSERTED)) {
>>>>                 pr_cont("NO PCMCIA card inserted\n");
>>>> @@ -167,6 +216,14 @@ static struct net_device * __init apne_probe(void)
>>>>
>>>>         pr_cont("ethernet PCMCIA card inserted\n");
>>>>
>>>> +#if IS_ENABLED(CONFIG_PCMCIA)
>>>> +       if (apne_100_mbit < 0 && pcmcia_is_16bit()) {
>>>> +               pr_info("16-bit PCMCIA card detected!\n");
>>>> +               isa_type = ISA_TYPE_AG16;
>>>> +               apne_100_mbit = 1;
>>>> +       }
>>>
>>> I think you should reset apne_100_mbit to zero if apne_100_mbit < 0
>>> && !pcmcia_is_16bit(), so rmmod + switching card + modprobe
>>> has a chance to work.
>>
>> Good catch - though when switching to another card using this same
>> driver, the module parameter can be used again to select IO mode or
>> force autoprobe.
>
> The autoprobe won't work if the new card is 8-bit.

I see now - adding code to set isa_type to 8 bit if the autoprobe fails, 
and reset isa_type and apne_100_mbit to defaults in module exit and the 
probe error return path ...

Cheers,

	Michael

>
> Gr{oetje,eeting}s,
>
>                         Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds
>