Re: [PATCH v5] serial: 8250: add gpio support to exar

[Sudip Mukherjee]

On Tue, Jan 19, 2016 at 12:09:08PM +0200, Andy Shevchenko wrote:
> On Tue, Jan 19, 2016 at 8:06 AM, Sudip Mukherjee
> <sudipm.mukherjee@xxxxxxxxx> wrote:
> > Exar XR17V352/354/358 chips have 16 multi-purpose inputs/outputs which
> > can be controlled using gpio interface.
> > Add support to use these pins.
> 
> + Peter Hung.
> 
> Seems Fintek HW is going similar way you, guys, have to decide how to
> proceed in general. I like this way Sudip made here, though I still
> few comments below.

Just had a look at the Fintek patch. Interestingly high baudrate was our
next plan. :)

> 
> First of all, can we split it to two patches like cooking GPIO driver
> first, then extend Exar piece of serial driver?
> 
> I also would like to vote for splitting out first Exar parts from
> 8250_pci like Peter did for Fintek.

Then maybe instead of splitting out if we have the provision of high
baudrate in 8250_pci? And the way I have done, it is just a matter of few
function calls from 8250_pci in case the hardware is present. So then,
what may be the advantage of splitting out?

>
<snip>
 
> > +static void exar_set(struct gpio_chip *chip, unsigned int reg, int val,
> > +                    unsigned int offset)
> 
> This one by implementation looks like exar_update()

well, I made it exar_set() as it is setting the gpio pins.

> 
<snip>
> > +
> > +static int gpio_exar_probe(struct platform_device *pdev)
> > +{
> > +       struct pci_dev *dev = platform_get_drvdata(pdev);
> > +       struct exar_gpio_chip *exar_gpio, *exar_temp;
> > +       void __iomem *p;
> > +       int index = 1;
> > +       int ret;
> > +
> > +       if (dev->vendor != PCI_VENDOR_ID_EXAR)
> > +               return -ENODEV;
> > +
> > +       p = pci_ioremap_bar(dev, 0);
> 
> So, if it would be separate driver for 8250_exar.c (by the way what is
> 8250_exar_st16c554.c?) you will use managed functions hereâ

8250_exar_st16c554.c -> Its probe and all other functions are in 8250_core.c

> 
<snip>
> > +       sprintf(exar_gpio->name, "exar_gpio%d", index);
> > +       exar_gpio->gpio_chip.label = exar_gpio->name;
> > +       exar_gpio->gpio_chip.parent = &dev->dev;
> > +       exar_gpio->gpio_chip.direction_output = exar_direction_output;
> > +       exar_gpio->gpio_chip.direction_input = exar_direction_input;
> > +       exar_gpio->gpio_chip.get_direction = exar_get_direction;
> > +       exar_gpio->gpio_chip.get = exar_get_value;
> > +       exar_gpio->gpio_chip.set = exar_set_value;
> > +       exar_gpio->gpio_chip.base = -1;
> > +       exar_gpio->gpio_chip.ngpio = 16;
> 
> > +       exar_gpio->gpio_chip.owner = THIS_MODULE;
> 
> Does core set it for you?

No, all the gpio drivers are setting it by itself. Maybe we can see if
that feature can be added to gpio core or not..

> 
<snip>
> 
> > +static const struct platform_device_id gpio_exar_id[] = {
> 
> > +       { "gpio_exar", 0},
> 
> This is default fallback. I don't think you need this at all (example
> in my mind is dw_dmac driver, where you can't find such line). But
> please recheck.

somehow in my testing the module was not loading without this. Maybe
module_alias is the key. I will check again while making these
modifications. But now the question is should I split out? What advantage
will be there in splitting out?

regards
sudip