Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1646327 > unrolled thread

gpio-exar: Why filtering out Commtech devices?

Back to article view | Back to linux.kernel

Contents

  gpio-exar: Why filtering out Commtech devices? Jan Kiszka <jan.kiszka@siemens.com> - 2017-05-21 13:50 +0200
    Re: gpio-exar: Why filtering out Commtech devices? Sudip Mukherjee <sudip.mukherjee@codethink.co.uk> - 2017-05-21 22:10 +0200
      Re: gpio-exar: Why filtering out Commtech devices? Jan Kiszka <jan.kiszka@siemens.com> - 2017-05-22 07:50 +0200
        Re: gpio-exar: Why filtering out Commtech devices? Jan Kiszka <jan.kiszka@siemens.com> - 2017-05-22 08:00 +0200
          Re: gpio-exar: Why filtering out Commtech devices? Jan Kiszka <jan.kiszka@siemens.com> - 2017-05-22 18:30 +0200
            Re: gpio-exar: Why filtering out Commtech devices? Jan Kiszka <jan.kiszka@siemens.com> - 2017-05-22 20:20 +0200
              Re: gpio-exar: Why filtering out Commtech devices? Jan Kiszka <jan.kiszka@siemens.com> - 2017-05-23 17:10 +0200
                Re: gpio-exar: Why filtering out Commtech devices? Jan Kiszka <jan.kiszka@siemens.com> - 2017-05-23 23:20 +0200

#1646327 — gpio-exar: Why filtering out Commtech devices?

Hi Sudip,

why do we carry

	if (pcidev->vendor != PCI_VENDOR_ID_EXAR)
		return -ENODEV;

in gpio_exar_probe? This effectively prevents that

	EXAR_DEVICE(COMMTECH, COMMTECH_4222PCIE, pbn_exar_XR17V35x),
	EXAR_DEVICE(COMMTECH, COMMTECH_4224PCIE, pbn_exar_XR17V35x),
	EXAR_DEVICE(COMMTECH, COMMTECH_4228PCIE, pbn_exar_XR17V35x),

gain GPIO support. Do those devices lack access to the pins? Or can we
drop the filter. I don't have access to those devices, just wondering
because the code is not explaining the reason.

Jan

-- 
Siemens AG, Corporate Technology, CT RDA ITP SES-DE
Corporate Competence Center Embedded Linux

[toc] | [next] | [standalone]

#1646405

Hi Jan,

On 21/05/17 12:46, Jan Kiszka wrote:
> Hi Sudip,
>
> why do we carry
>
> 	if (pcidev->vendor != PCI_VENDOR_ID_EXAR)
> 		return -ENODEV;
>
> in gpio_exar_probe? This effectively prevents that
>
> 	EXAR_DEVICE(COMMTECH, COMMTECH_4222PCIE, pbn_exar_XR17V35x),
> 	EXAR_DEVICE(COMMTECH, COMMTECH_4224PCIE, pbn_exar_XR17V35x),
> 	EXAR_DEVICE(COMMTECH, COMMTECH_4228PCIE, pbn_exar_XR17V35x),
>
> gain GPIO support. Do those devices lack access to the pins? Or can we
> drop the filter. I don't have access to those devices, just wondering
> because the code is not explaining the reason.

Same here. I do not have these devices and have no idea if they support 
the gpio pins or not.

Adding Matt Schulte in the Cc list, maybe he can comment.


-- 
Regards
Sudip

[toc] | [prev] | [next] | [standalone]

#1646526

On 2017-05-21 22:08, Sudip Mukherjee wrote:
> Hi Jan,
> 
> On 21/05/17 12:46, Jan Kiszka wrote:
>> Hi Sudip,
>>
>> why do we carry
>>
>>     if (pcidev->vendor != PCI_VENDOR_ID_EXAR)
>>         return -ENODEV;
>>
>> in gpio_exar_probe? This effectively prevents that
>>
>>     EXAR_DEVICE(COMMTECH, COMMTECH_4222PCIE, pbn_exar_XR17V35x),
>>     EXAR_DEVICE(COMMTECH, COMMTECH_4224PCIE, pbn_exar_XR17V35x),
>>     EXAR_DEVICE(COMMTECH, COMMTECH_4228PCIE, pbn_exar_XR17V35x),
>>
>> gain GPIO support. Do those devices lack access to the pins? Or can we
>> drop the filter. I don't have access to those devices, just wondering
>> because the code is not explaining the reason.
> 
> Same here. I do not have these devices and have no idea if they support
> the gpio pins or not.
> 
> Adding Matt Schulte in the Cc list, maybe he can comment.
> 
> 

If we need to keep the condition, it should be moved over to 8250_exar:
there is no point in creating the platform device at all then. But let's
wait for Matt's comment.

Jan

-- 
Siemens AG, Corporate Technology, CT RDA ITP SES-DE
Corporate Competence Center Embedded Linux

[toc] | [prev] | [next] | [standalone]

#1646531

On 2017-05-22 07:48, Jan Kiszka wrote:
> On 2017-05-21 22:08, Sudip Mukherjee wrote:
>> Hi Jan,
>>
>> On 21/05/17 12:46, Jan Kiszka wrote:
>>> Hi Sudip,
>>>
>>> why do we carry
>>>
>>>     if (pcidev->vendor != PCI_VENDOR_ID_EXAR)
>>>         return -ENODEV;
>>>
>>> in gpio_exar_probe? This effectively prevents that
>>>
>>>     EXAR_DEVICE(COMMTECH, COMMTECH_4222PCIE, pbn_exar_XR17V35x),
>>>     EXAR_DEVICE(COMMTECH, COMMTECH_4224PCIE, pbn_exar_XR17V35x),
>>>     EXAR_DEVICE(COMMTECH, COMMTECH_4228PCIE, pbn_exar_XR17V35x),
>>>
>>> gain GPIO support. Do those devices lack access to the pins? Or can we
>>> drop the filter. I don't have access to those devices, just wondering
>>> because the code is not explaining the reason.
>>
>> Same here. I do not have these devices and have no idea if they support
>> the gpio pins or not.
>>
>> Adding Matt Schulte in the Cc list, maybe he can comment.
>>
>>
> 
> If we need to keep the condition, it should be moved over to 8250_exar:
> there is no point in creating the platform device at all then. But let's
> wait for Matt's comment.

Unfortunately, his account is no longer existing. Is there anyone else
we can ask?

Jan

-- 
Siemens AG, Corporate Technology, CT RDA ITP SES-DE
Corporate Competence Center Embedded Linux

[toc] | [prev] | [next] | [standalone]

#1647110

On 2017-05-22 17:17, Technical Support wrote:
> Hello,
> 
> The Exar MPIO pins are used by our device driver to control features of
> the line driver and can't be used as GPIO pins.  I agree, the condition
> can be moved to 8250_exar prior to a platform device being created for
> the gpio_exar driver.
> 

Thanks a lot for the feedback! I will send a refactoring patch.

Jan

> Regards,
> Landon Unruh       
> 
> Commtech, Inc.
> Voice: 316-636-1131
> http://www.fastcomproducts.com
> 
> 
> 
> 
> On Mon, May 22, 2017 at 12:51 AM, Jan Kiszka <jan.kiszka@siemens.com
> <mailto:jan.kiszka@siemens.com>> wrote:
> 
>     On 2017-05-22 07:48, Jan Kiszka wrote:
>     > On 2017-05-21 22:08, Sudip Mukherjee wrote:
>     >> Hi Jan,
>     >>
>     >> On 21/05/17 12:46, Jan Kiszka wrote:
>     >>> Hi Sudip,
>     >>>
>     >>> why do we carry
>     >>>
>     >>>     if (pcidev->vendor != PCI_VENDOR_ID_EXAR)
>     >>>         return -ENODEV;
>     >>>
>     >>> in gpio_exar_probe? This effectively prevents that
>     >>>
>     >>>     EXAR_DEVICE(COMMTECH, COMMTECH_4222PCIE, pbn_exar_XR17V35x),
>     >>>     EXAR_DEVICE(COMMTECH, COMMTECH_4224PCIE, pbn_exar_XR17V35x),
>     >>>     EXAR_DEVICE(COMMTECH, COMMTECH_4228PCIE, pbn_exar_XR17V35x),
>     >>>
>     >>> gain GPIO support. Do those devices lack access to the pins? Or can we
>     >>> drop the filter. I don't have access to those devices, just wondering
>     >>> because the code is not explaining the reason.
>     >>
>     >> Same here. I do not have these devices and have no idea if they support
>     >> the gpio pins or not.
>     >>
>     >> Adding Matt Schulte in the Cc list, maybe he can comment.
>     >>
>     >>
>     >
>     > If we need to keep the condition, it should be moved over to 8250_exar:
>     > there is no point in creating the platform device at all then. But let's
>     > wait for Matt's comment.
> 
>     Unfortunately, his account is no longer existing. Is there anyone else
>     we can ask?
> 
>     Jan
> 
>     --
>     Siemens AG, Corporate Technology, CT RDA ITP SES-DE
>     Corporate Competence Center Embedded Linux
>     --
>     To unsubscribe from this list: send the line "unsubscribe
>     linux-serial" in
>     the body of a message to majordomo@vger.kernel.org
>     <mailto:majordomo@vger.kernel.org>
>     More majordomo info at  http://vger.kernel.org/majordomo-info.html
>     <http://vger.kernel.org/majordomo-info.html>
> 
> 

[toc] | [prev] | [next] | [standalone]

#1647237

On 2017-05-22 18:24, Jan Kiszka wrote:
> On 2017-05-22 17:17, Technical Support wrote:
>> Hello,
>>
>> The Exar MPIO pins are used by our device driver to control features of
>> the line driver and can't be used as GPIO pins.  I agree, the condition
>> can be moved to 8250_exar prior to a platform device being created for
>> the gpio_exar driver.
>>
> 
> Thanks a lot for the feedback! I will send a refactoring patch.

Hmm, are you possibly talking about PCI device with the IDs 0x2, 0x4,
0xa, 0xb (4222PCI335, 4224PCI335, 2324PCI335, 2328PCI335), because those
actually set the GPIOs for apparent internal reasons. However,
0x20..0x22 (4222PCIE, 4224PCIE, 4228PCIE) already share setup_gpio() via
pci_xr17v35x_setup(), and that looks different.

I'm also asking again because we just changed the default MPIO settings
for the latter to all inputs. If you depended on them to be all outputs
and 0, we may have broken something.

Thanks,
Jan

-- 
Siemens AG, Corporate Technology, CT RDA ITP SES-DE
Corporate Competence Center Embedded Linux

[toc] | [prev] | [next] | [standalone]

#1648167

On 2017-05-23 16:33, Technical Support wrote:
> Both the PCI and the PCIe cards use the MPIO pins and the device driver
> actually sets the pins as outputs after registering the device with the
> PCI core.  So the changes made to the default settings shouldn't break
> anything. 
> 
> Do the new default settings keep the UART_EXAR_MPIOINT_X_X values at
> 0x00 to disable their interrupts? 

Look at
https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git/commit/?h=tty-next&id=7dea8165f1d6434dc7572004363f86339f0f4322

Interrupts remain off, but all pins are now inputs. Again, if the logic
on the PCIe cards need any of them to be driven low, my patch causes a
regression. In that case, we also need to make the MPIO setup card-specific.

Thanks,
Jan

> 
> -Landon 
>         
> Commtech, Inc.
> Voice: 316-636-1131 <tel:%28316%29%20636-1131>
> http://www.fastcomproducts.com
> 
> 
> 
> 
> On Mon, May 22, 2017 at 1:18 PM, Jan Kiszka <jan.kiszka@siemens.com
> <mailto:jan.kiszka@siemens.com>> wrote:
> 
>     On 2017-05-22 18:24, Jan Kiszka wrote:
>     > On 2017-05-22 17:17, Technical Support wrote:
>     >> Hello,
>     >>
>     >> The Exar MPIO pins are used by our device driver to control features of
>     >> the line driver and can't be used as GPIO pins.  I agree, the condition
>     >> can be moved to 8250_exar prior to a platform device being created for
>     >> the gpio_exar driver.
>     >>
>     >
>     > Thanks a lot for the feedback! I will send a refactoring patch.
> 
>     Hmm, are you possibly talking about PCI device with the IDs 0x2, 0x4,
>     0xa, 0xb (4222PCI335, 4224PCI335, 2324PCI335, 2328PCI335), because those
>     actually set the GPIOs for apparent internal reasons. However,
>     0x20..0x22 (4222PCIE, 4224PCIE, 4228PCIE) already share setup_gpio() via
>     pci_xr17v35x_setup(), and that looks different.
> 
>     I'm also asking again because we just changed the default MPIO settings
>     for the latter to all inputs. If you depended on them to be all outputs
>     and 0, we may have broken something.
> 
>     Thanks,
>     Jan
> 
>     --
>     Siemens AG, Corporate Technology, CT RDA ITP SES-DE
>     Corporate Competence Center Embedded Linux
> 
> 

-- 
Siemens AG, Corporate Technology, CT RDA ITP SES-DE
Corporate Competence Center Embedded Linux

[toc] | [prev] | [next] | [standalone]

#1648753

On 2017-05-23 22:05, Technical Support wrote:
> ​Our kernel module handles card specific MPIO setup, ensuring the pins
> are set correctly for our hardware regardless of what exar_8250
> initializes them as.   
> 
> https://github.com/commtech/serialfc-linux/blob/784e1321e00d4f72c93dae8da11cdc7f99c15dee/src/utils.c#L1463-L1491

OK, that code shows what we need to do in the official driver: handle
the commtech devices like before, i.e. keep the GPIOs as output for
them. I'll update my patch.


But now some good advice: you should rather work on the upstream code
and schedule your special driver for retirement. This will ensure that
your users have the best experience when they plug in any of your cards
and boot a standard Linux disto: it will just work.

Right now, your out-of-tree driver competes with the official one for
the same resources, apparently comes with own userspace interfaces to
configure special features instead of building on existing APIs (you
don't support TIOCSRS485 e.g., do you?), and seems to contain support
for a number of cards that are still missing in upstream. This is very
unhandy for your users.

For the same reason, we are currently upstreaming the code for our
Exar-based design (and that's why I came across all this): We want to
get rid of special board support to improve user experience and
long-term availability.

Jan

-- 
Siemens AG, Corporate Technology, CT RDA ITP SES-DE
Corporate Competence Center Embedded Linux

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web