Re: [PATCH v4 02/11] mfd: Add support for Kontron sl28cpld management controller

[Michael Walle]

Hi Andy,

Am 2020-06-05 10:01, schrieb Andy Shevchenko:
> On Fri, Jun 5, 2020 at 12:16 AM Michael Walle <michael@xxxxxxxx> wrote:
> > Add the core support for the board management controller found on the
> > SMARC-sAL28 board. It consists of the following functions:
> >  - watchdog
> >  - GPIO controller
> >  - PWM controller
> >  - fan sensor
> >  - interrupt controller
> >
> > At the moment, this controller is used on the Kontron SMARC-sAL28 
> > board.
> >
> > Please note that the MFD driver is defined as bool in the Kconfig
> > because the next patch will add interrupt support.
>
> ...
>
> > +config MFD_SL28CPLD
> > +       bool "Kontron sl28 core driver"
> > +       depends on I2C=y
>
> Why not module?

There are users of the interupt lines provided by the interrupt 
controller.
For example, the gpio-button driver. If this is compiled into the kernel
(which it is by default in the arm64 defconfig), probing will fail 
because
the interrupt is not found. Is there a better way for that? I guess the 
same
is true for the GPIO driver.

> > +       depends on OF
>
> I didn't find an evidence this is needed.

see below.

> No Compile Test?

ok

> > +       select REGMAP_I2C
> > +       select MFD_CORE
>
> ...
>
> > +#include <linux/of_platform.h>
>
> No evidence of user of this.
> I think you meant mod_devicetable.h.

devm_of_platform_populate(), so I need CONFIG_OF, too right?


> > +static struct i2c_driver sl28cpld_driver = {
> > +       .probe_new = sl28cpld_probe,
> > +       .driver = {
> > +               .name = "sl28cpld",
> > +               .of_match_table = of_match_ptr(sl28cpld_of_match),
>
> Drop of_match_ptr(). It has a little sense in this context (depends 
> OF).
> It will have a little sense even if you drop depends OF b/c you will
> introduce a compiler warning.

ok

> > +       },
> > +};

--
-michael