Re: [RFC PATCH linux dev-4.13 1/3] soc: aspeed: Miscellaneous control interfaces

[Andrew Jeffery <andrew@aj.id.au>]

On Thu, 19 Apr 2018, at 12:37, Joel Stanley wrote:
> On 12 April 2018 at 13:21, Andrew Jeffery <andrew@aj.id.au> wrote:
> > The ASPEED BMC SoCs have many knobs and switches that are sometimes
> > design-specific and often defy any approach to unify them under an
> > existing subsystem.
> >
> > Add a driver to translate a devicetree table into sysfs entries to
> > expose bits and fields for manipulation from userspace. This encompasses
> > concepts from scratch registers to boolean conditions to enable or
> > disable host interface features.
> >
> > Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> > ---
> >  drivers/soc/Kconfig                  |   1 +
> >  drivers/soc/Makefile                 |   1 +
> >  drivers/soc/aspeed/Kconfig           |  11 +++
> >  drivers/soc/aspeed/Makefile          |   1 +
> >  drivers/soc/aspeed/aspeed-bmc-misc.c | 187 +++++++++++++++++++++++++++++++++++
> >  5 files changed, 201 insertions(+)
> >  create mode 100644 drivers/soc/aspeed/Kconfig
> >  create mode 100644 drivers/soc/aspeed/Makefile
> >  create mode 100644 drivers/soc/aspeed/aspeed-bmc-misc.c
> >
> > diff --git a/drivers/soc/Kconfig b/drivers/soc/Kconfig
> > index 07fc0ac51c52..776fd5978f70 100644
> > --- a/drivers/soc/Kconfig
> > +++ b/drivers/soc/Kconfig
> > @@ -1,6 +1,7 @@
> >  menu "SOC (System On Chip) specific Drivers"
> >
> >  source "drivers/soc/actions/Kconfig"
> > +source "drivers/soc/aspeed/Kconfig"
> >  source "drivers/soc/atmel/Kconfig"
> >  source "drivers/soc/bcm/Kconfig"
> >  source "drivers/soc/fsl/Kconfig"
> > diff --git a/drivers/soc/Makefile b/drivers/soc/Makefile
> > index 9241125416ba..94a5b97c4466 100644
> > --- a/drivers/soc/Makefile
> > +++ b/drivers/soc/Makefile
> > @@ -3,6 +3,7 @@
> >  #
> >
> >  obj-$(CONFIG_ARCH_ACTIONS)     += actions/
> > +obj-$(CONFIG_ARCH_ASPEED)       += aspeed/
> >  obj-$(CONFIG_ARCH_AT91)                += atmel/
> >  obj-y                          += bcm/
> >  obj-$(CONFIG_ARCH_DOVE)                += dove/
> > diff --git a/drivers/soc/aspeed/Kconfig b/drivers/soc/aspeed/Kconfig
> > new file mode 100644
> > index 000000000000..704a4efe180f
> > --- /dev/null
> > +++ b/drivers/soc/aspeed/Kconfig
> > @@ -0,0 +1,11 @@
> > +menu "ASPEED SoC drivers"
> > +
> > +config ASPEED_BMC_MISC
> > +       bool "Miscellaneous ASPEED BMC interfaces"
> 
> Do you mean bool or tristate? if you mean bool, drop the module-y bits below.

Should be tristate, was just slapping bits together :)

> 
> > +       depends on ARCH_ASPEED || COMPILE_TEST
> > +       default ARCH_ASPEED
> > +       help
> > +         Say yes to expose VGA and LPC scratch registers, and other
> > +         miscellaneous control interfaces specific to the ASPEED BMC SoCs
> > +
> > +endmenu
> > diff --git a/drivers/soc/aspeed/Makefile b/drivers/soc/aspeed/Makefile
> > new file mode 100644
> > index 000000000000..d1a80f9a584f
> > --- /dev/null
> > +++ b/drivers/soc/aspeed/Makefile
> > @@ -0,0 +1 @@
> > +obj-$(CONFIG_ASPEED_BMC_MISC) += aspeed-bmc-misc.o
> > diff --git a/drivers/soc/aspeed/aspeed-bmc-misc.c b/drivers/soc/aspeed/aspeed-bmc-misc.c
> > new file mode 100644
> > index 000000000000..48522a736a05
> > --- /dev/null
> > +++ b/drivers/soc/aspeed/aspeed-bmc-misc.c
> 
> copyright headahs

Good catch.

> 
> > @@ -0,0 +1,187 @@
> > +#include <linux/kobject.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/regmap.h>
> > +#include <linux/mfd/syscon.h>
> > +
> > +#define DEVICE_NAME "aspeed-bmc-misc"
> 
> This is the name of our chardev? No, it appears to just be the driver
> name. You can probably just use it directly.

Yep

> 
> An observation: It's the only thing inside this file that is aspeed
> specific. If not for the name, it could be generic code.

I agree, hence some of the bullet points in the cover letter. But it's name and location was also partly driven by Arnd's suggestion that we stick the junk in drivers/soc, and it felt a bit weird not to use aspeed in the name given the convention there.

> 
> lgtm. I suggest writing up some bindings and putting on the asbestos pants.

Wheee...

> 
> I'll merge this into openbmc with the IBM copyright added to the top.

Sounds good.

> With this adding userspace ABI, anyone who writes code against it will
> need to make themselves available for rework as it goes through
> upstream review.

Yeah. Will try to make sure people are aware of this.

Thanks for the feedback.

Andrew