Messages in this thread | | | Subject | Re: [PATCH v2 5/8] pinctrl: Add core support for Aspeed SoCs | From | Andrew Jeffery <> | Date | Tue, 23 Aug 2016 12:00:51 +0930 |
| |
On Mon, 2016-08-22 at 15:45 +0200, Linus Walleij wrote: > On Fri, Aug 19, 2016 at 2:44 PM, Andrew Jeffery <andrew@aj.id.au> wrote: > > > > > +++ b/drivers/pinctrl/aspeed/Kconfig > > @@ -0,0 +1,8 @@ > > +config PINCTRL_ASPEED > > + bool > > + depends on (ARCH_ASPEED || COMPILE_TEST) && OF > > + select PINMUX > > + select PINCONF > > + select GENERIC_PINCONF > > + select MFD_SYSCON > > + select REGMAP_MMIO > Since this device is spawn from the syscon, should it not be > "depends on MFD_SYSCON"? > > (No big deal, if you think this is right then go with it.)
I think that's a fair point, I will look at rearranging it.
> > > > > +#include > What is this include for?
Cruft from past iterations. Will remove.
> > > > > +#include > > +#include > > +#include > > +#include > > +#include "../core.h" > > +#include "pinctrl-aspeed.h" > No #include ? > > Maybe some #includes are centralized to pinctrl-aspeed.h > I don't know, just make sure you don't have implicit includes.
linux/regmap.h is included in pinctrl-aspeed.h
> > > > > + if (regmap_read(map, desc->reg, &val) < 0) > > + return false; > > + > > + val &= ~desc->mask; > > + val |= pattern << __ffs(desc->mask); > > + > > + if (regmap_write(map, desc->reg, val) < 0) > > + return false; > Use: > > regmap_update_bits(map, > desc->reg, > desc->mask, > pattern << __ffs->desc->mask); > > Or something like that instead of reimplementing > mask-and-set. Regmap already knows how to do the > business. > > (Applied everywhere in the driver where you have a > mask-and-set like this).
Right, I will clean that up.
> > The expression core engine is still a complete mystery > for me, I will just trust you that it works as intended.
Gah! However, thanks!
I will clean up the issues above and re-send.
Cheers,
Andrew[unhandled content-type:application/pgp-signature] | |