[PATCH v2] gpio: octeon_gpio: Add GPIO controller driver for Octeon

[Simon Glass]

Hi Daniel,

On Wed, 22 Jul 2020 at 05:47, Daniel Schwierzeck
<daniel.schwierzeck at gmail.com> wrote:
>
> > Hi Daniel,
> >
> > On 21.07.20 16:59, Daniel Schwierzeck wrote:
> > > > Hi Daniel,
> > > >
> > > > On 18.07.20 15:25, Daniel Schwierzeck wrote:
> > > > > > From: Suneel Garapati <sgarapati at marvell.com>
> > > ...
> > > > > found another issue:
> > > > >
> > > > > drivers/gpio/octeon_gpio.c: In function 'octeon_gpio_probe':
> > > > > drivers/gpio/octeon_gpio.c:189:16: warning: implicit declaration of
> > > > > function 'dm_pci_map_bar'; did you mean 'pci_map_bar'? [-Wimplicit-
> > > > > function-declaration]
> > > > >     189 |   priv->base = dm_pci_map_bar(dev, PCI_BASE_ADDRESS_0,
> > > > >         |                ^~~~~~~~~~~~~~
> > > > >         |                pci_map_bar
> > > > > drivers/gpio/octeon_gpio.c:189:14: warning: assignment to 'void *' from
> > > > > 'int' makes pointer from integer without a cast [-Wint-conversion]
> > > > >     189 |   priv->base = dm_pci_map_bar(dev, PCI_BASE_ADDRESS_0,
> > > > >         |              ^
> > > >
> > > > Ah, I did not see this in my local builds, as I have enabled DM_PCI
> > > > here and forgot to add this dependency.
> > > >
> > > > > due to the dependency on DM_PCI you need to express this in Kconfig
> > > > > with "depends on DM_PCI ...". Alternatively you need to wrap the PCI
> > > > > specific code with a "#ifdef CONFIG_DM_PCI" as the DM_PCI specific
> > > > > function prototypes in pci.h are also wrapped with "#ifdef
> > > > > CONFIG_DM_PCI". The former option will build and link DM PCI code which
> > > > > is not used and therefore bloats the U-Boot binary.
> > > > >
> > > > > I guess the choice mainly depends on whether you'll add a PCI host
> > > > > controller driver for Octeon MIPS64 in the future and can live with the
> > > > > extra but unused PCI code until then.
> > > >
> > > > We can definitely live with the temporarily unused PCI code. I don't
> > > > want to add these #ifdefs again, which I removed explicitly upon Simon's
> > > > request.
> > > >
> > > > To solve this, I would prefer to add a "select DM_PCI & PCI" to
> > > > arch/mips/Kconfig for Octeon, as this PCI probing construct is not
> > > > only used in this GPIO driver, but also in many other drivers shared
> > > > between MIPS Octeon and the also upcoming ARM64 Octeon TX/TX2 support,
> > > > like I2C, SPI etc. Here all devices are PCI based hence the PCI probing
> > > > dependency.
> > > >
> > > > Is this okay with you? Or should I stay with the dependency in the
> > > > drivers Kconfig file?
> > > >
> > >
> > > I would also prefer a "select DM_PCI". I just struggle a bit with the
> > > "& PCI" part. IMHO DM_PCI should not depend on PCI because DM_PCI
> > > should be a hidden option which is selected by the SoC dependent on the
> > > SoC specific drivers and whether them support DM. It doesn't make sense
> > > to let the user choose all those DM_* options. Most other DM_* options
> > > already work like this except that them are not hidden.
> > >
> > > BTW: When you prepare a patch, you can also "select DM_I2C" and
> > > "DM_SPI" which have the same issues with the PCI dependency.
> >
> > Sure. But selecting only "DM_PCI" and not "PCI" leads to these issues:
> >
> > WARNING: unmet direct dependencies detected for DM_PCI
> >    Depends on [n]: PCI [=n] && DM [=y]
> >    Selected by [y]:
> >    - ARCH_OCTEON [=y] && <choice>
> >
> > It doesn't make sense to have DM_PCI enabled and PCI not. Same happens
> > with DM_SPI BTW.
>
> exactly that is my problem. The dependency doesn't make sense and
> should be removed IMHO. Options like DM_PCI should be interpreted as
> "this platform with its drivers support PCI witrh DM". It doesn't nake
> sense to let an user choose this option. I just wanted to point out
> this inconsistency. Maybe Simon has an opinion about that.

Yes that's right. We have it as an option since supporting DM_PCI can
be board-by-board at present. Once migration is finished, we can drop
DM_PCI.

[..]

Regards,
Simon