mbox series

[v18,0/8] Add support for the silergy,sy7636a

Message ID 20220124121009.108649-1-alistair@alistair23.me
Headers show
Series Add support for the silergy,sy7636a | expand

Message

Alistair Francis Jan. 24, 2022, 12:10 p.m. UTC
v18:
 - Rebase
v17:
 - Rebase and fix build issues
v16:
 - Improve vdd regulator comments
v15:
 - Address comments on the patches
v14:
 - Merge the thermal driver and hwmon
v13:
 - Address comments on thermal driver
 - Rebase on master (without other patches)
v12:
 - Rebase
v11:
 - Address comments on hwmon
 - Improve "mfd: simple-mfd-i2c: Add a Kconfig name" commit message
v10:
 - Use dev_get_regmap() instead of dev_get_drvdata()
v9:
 - Convert to use the simple-mfd-i2c instead

Alistair Francis (8):
  dt-bindings: mfd: Initial commit of silergy,sy7636a.yaml
  mfd: simple-mfd-i2c: Add a Kconfig name
  mfd: simple-mfd-i2c: Enable support for the silergy,sy7636a
  regulator: sy7636a: Remove requirement on sy7636a mfd
  hwmon: sy7636a: Add temperature driver for sy7636a
  ARM: imx_v6_v7_defconfig: Enable silergy,sy7636a
  ARM: dts: imx7d-remarkable2: Enable silergy,sy7636a
  ARM: dts: imx7d-remarkable2: Enable lcdif

 .../bindings/mfd/silergy,sy7636a.yaml         |  82 +++++++++++
 Documentation/hwmon/index.rst                 |   1 +
 Documentation/hwmon/sy7636a-hwmon.rst         |  26 ++++
 arch/arm/boot/dts/imx7d-remarkable2.dts       | 136 ++++++++++++++++++
 arch/arm/configs/imx_v6_v7_defconfig          |   3 +
 drivers/hwmon/Kconfig                         |   9 ++
 drivers/hwmon/Makefile                        |   1 +
 drivers/hwmon/sy7636a-hwmon.c                 | 106 ++++++++++++++
 drivers/mfd/Kconfig                           |   2 +-
 drivers/mfd/simple-mfd-i2c.c                  |  11 ++
 drivers/regulator/Kconfig                     |   1 -
 drivers/regulator/sy7636a-regulator.c         |   7 +-
 include/linux/mfd/sy7636a.h                   |  34 +++++
 13 files changed, 415 insertions(+), 4 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/mfd/silergy,sy7636a.yaml
 create mode 100644 Documentation/hwmon/sy7636a-hwmon.rst
 create mode 100644 drivers/hwmon/sy7636a-hwmon.c
 create mode 100644 include/linux/mfd/sy7636a.h

Comments

Geert Uytterhoeven March 8, 2022, 10:53 a.m. UTC | #1
Hi Alistair,

Thanks for your patch, which is now commit bae5a4acef67db88
("mfd: simple-mfd-i2c: Add a Kconfig name") in mfd/for-mfd-next.

On Mon, Jan 24, 2022 at 1:24 PM Alistair Francis <alistair@alistair23.me> wrote:
> Add a Kconfig name to the "Simple Multi-Functional Device support (I2C)"
> device so that it can be enabled via menuconfig.

Which still does not explain why this would be needed...

> Signed-off-by: Alistair Francis <alistair@alistair23.me>
> Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org>

> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -1188,7 +1188,7 @@ config MFD_SI476X_CORE
>           module will be called si476x-core.
>
>  config MFD_SIMPLE_MFD_I2C
> -       tristate
> +       tristate "Simple Multi-Functional Device support (I2C)"
>         depends on I2C
>         select MFD_CORE
>         select REGMAP_I2C

The help text states:

| This driver creates a single register map with the intention for it
| to be shared by all sub-devices.

Yes, that's what MFD does?

| Once the register map has been successfully initialised, any
| sub-devices represented by child nodes in Device Tree will be
| subsequently registered.

OK...?

Still, no clue about what this driver really does, and why and when
it would be needed.

There is one driver symbol that selects MFD_SIMPLE_MFD_I2C.
There are no driver symbols that depend on this symbol.

If you have a driver in the pipeline that can make use of this,
can't it just select MFD_SIMPLE_MFD_I2C, so the symbol itself can
stay invisible?

Thanks!

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
Geert Uytterhoeven March 8, 2022, 11:02 a.m. UTC | #2
Hi Alistair,

On Mon, Jan 24, 2022 at 1:25 PM Alistair Francis <alistair@alistair23.me> wrote:
> Signed-off-by: Alistair Francis <alistair@alistair23.me>
> Acked-by: Mark Brown <broonie@kernel.org>

Thanks for your patch, which is now commit 947d0cce70ae37b8
("regulator: sy7636a: Remove requirement on sy7636a mfd") in
mfd/for-mfd-next.

> --- a/drivers/regulator/Kconfig
> +++ b/drivers/regulator/Kconfig
> @@ -1208,7 +1208,6 @@ config REGULATOR_STW481X_VMMC
>
>  config REGULATOR_SY7636A
>         tristate "Silergy SY7636A voltage regulator"
> -       depends on MFD_SY7636A

As this is an i2c mfd device, you still need a dependency on
MFD and I2C, or some other symbol?

>         help
>           This driver supports Silergy SY3686A voltage regulator.
>
> diff --git a/drivers/regulator/sy7636a-regulator.c b/drivers/regulator/sy7636a-regulator.c
> index 22fddf868e4c..29fc27c2cda0 100644
> --- a/drivers/regulator/sy7636a-regulator.c
> +++ b/drivers/regulator/sy7636a-regulator.c
> @@ -7,11 +7,14 @@
>  // Authors: Lars Ivar Miljeteig <lars.ivar.miljeteig@remarkable.com>
>  //          Alistair Francis <alistair@alistair23.me>
>
> +#include <linux/gpio/consumer.h>
> +#include <linux/i2c.h>
> +#include <linux/mfd/sy7636a.h>
>  #include <linux/module.h>
>  #include <linux/platform_device.h>
> +#include <linux/regulator/driver.h>
> +#include <linux/regulator/machine.h>
>  #include <linux/regmap.h>
> -#include <linux/gpio/consumer.h>
> -#include <linux/mfd/sy7636a.h>
>
>  struct sy7636a_data {
>         struct regmap *regmap;

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
Geert Uytterhoeven March 8, 2022, 11:21 a.m. UTC | #3
Hi Alistair,

On Mon, Jan 24, 2022 at 1:25 PM Alistair Francis <alistair@alistair23.me> wrote:
> This is a multi-function device to interface with the sy7636a
> EPD PMIC chip from Silergy.
>
> Signed-off-by: Alistair Francis <alistair@alistair23.me>
> Acked-by: Guenter Roeck <linux@roeck-us.net>

Thanks for your patch, which is now commit de34a40532507814 ("hwmon:
sy7636a: Add temperature driver for sy7636a") in mfd/for-mfd-next.

> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -1672,6 +1672,15 @@ config SENSORS_SIS5595
>           This driver can also be built as a module. If so, the module
>           will be called sis5595.
>
> +config SENSORS_SY7636A
> +       tristate "Silergy SY7636A"
> +       help
> +         If you say yes here you get support for the thermistor readout of
> +         the Silergy SY7636A PMIC.

As this is an i2c mfd device, you do need a dependency on MFD and I2C,
or some other symbol, unless compile-testing?

> +
> +         This driver can also be built as a module.  If so, the module
> +         will be called sy7636a-hwmon.
> +

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
Alistair Francis March 19, 2022, 2:36 a.m. UTC | #4
On Tue, Mar 8, 2022 at 8:53 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Alistair,
>
> Thanks for your patch, which is now commit bae5a4acef67db88
> ("mfd: simple-mfd-i2c: Add a Kconfig name") in mfd/for-mfd-next.
>
> On Mon, Jan 24, 2022 at 1:24 PM Alistair Francis <alistair@alistair23.me> wrote:
> > Add a Kconfig name to the "Simple Multi-Functional Device support (I2C)"
> > device so that it can be enabled via menuconfig.
>
> Which still does not explain why this would be needed...
>
> > Signed-off-by: Alistair Francis <alistair@alistair23.me>
> > Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org>
>
> > --- a/drivers/mfd/Kconfig
> > +++ b/drivers/mfd/Kconfig
> > @@ -1188,7 +1188,7 @@ config MFD_SI476X_CORE
> >           module will be called si476x-core.
> >
> >  config MFD_SIMPLE_MFD_I2C
> > -       tristate
> > +       tristate "Simple Multi-Functional Device support (I2C)"
> >         depends on I2C
> >         select MFD_CORE
> >         select REGMAP_I2C
>
> The help text states:
>
> | This driver creates a single register map with the intention for it
> | to be shared by all sub-devices.
>
> Yes, that's what MFD does?
>
> | Once the register map has been successfully initialised, any
> | sub-devices represented by child nodes in Device Tree will be
> | subsequently registered.
>
> OK...?
>
> Still, no clue about what this driver really does, and why and when
> it would be needed.
>
> There is one driver symbol that selects MFD_SIMPLE_MFD_I2C.
> There are no driver symbols that depend on this symbol.
>
> If you have a driver in the pipeline that can make use of this,
> can't it just select MFD_SIMPLE_MFD_I2C, so the symbol itself can
> stay invisible?

My patch "mfd: simple-mfd-i2c: Enable support for the silergy,sy7636a"
allows using this driver for the silergy,sy7636a MFD. So it's nice to
be able to enable and disable it as required.

Alistair

>
> Thanks!
>
> Gr{oetje,eeting}s,
>
>                         Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds
Geert Uytterhoeven March 19, 2022, 9:28 a.m. UTC | #5
Hi Alistair,

On Sat, Mar 19, 2022 at 3:36 AM Alistair Francis <alistair23@gmail.com> wrote:
> On Tue, Mar 8, 2022 at 8:53 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > Thanks for your patch, which is now commit bae5a4acef67db88
> > ("mfd: simple-mfd-i2c: Add a Kconfig name") in mfd/for-mfd-next.
> >
> > On Mon, Jan 24, 2022 at 1:24 PM Alistair Francis <alistair@alistair23.me> wrote:
> > > Add a Kconfig name to the "Simple Multi-Functional Device support (I2C)"
> > > device so that it can be enabled via menuconfig.
> >
> > Which still does not explain why this would be needed...
> >
> > > Signed-off-by: Alistair Francis <alistair@alistair23.me>
> > > Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org>
> >
> > > --- a/drivers/mfd/Kconfig
> > > +++ b/drivers/mfd/Kconfig
> > > @@ -1188,7 +1188,7 @@ config MFD_SI476X_CORE
> > >           module will be called si476x-core.
> > >
> > >  config MFD_SIMPLE_MFD_I2C
> > > -       tristate
> > > +       tristate "Simple Multi-Functional Device support (I2C)"
> > >         depends on I2C
> > >         select MFD_CORE
> > >         select REGMAP_I2C
> >
> > The help text states:
> >
> > | This driver creates a single register map with the intention for it
> > | to be shared by all sub-devices.
> >
> > Yes, that's what MFD does?
> >
> > | Once the register map has been successfully initialised, any
> > | sub-devices represented by child nodes in Device Tree will be
> > | subsequently registered.
> >
> > OK...?
> >
> > Still, no clue about what this driver really does, and why and when
> > it would be needed.
> >
> > There is one driver symbol that selects MFD_SIMPLE_MFD_I2C.
> > There are no driver symbols that depend on this symbol.
> >
> > If you have a driver in the pipeline that can make use of this,
> > can't it just select MFD_SIMPLE_MFD_I2C, so the symbol itself can
> > stay invisible?
>
> My patch "mfd: simple-mfd-i2c: Enable support for the silergy,sy7636a"
> allows using this driver for the silergy,sy7636a MFD. So it's nice to
> be able to enable and disable it as required.

So after that patch, enabling MFD_SIMPLE_MFD_I2C will enable
support for an ever-growing random bunch of devices, none of which
is described in the help text?
To me, ghat doesn't look like the way to go forward...

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
Geert Uytterhoeven March 19, 2022, 9:31 a.m. UTC | #6
Hi Alistair,

On Mon, Jan 24, 2022 at 1:24 PM Alistair Francis <alistair@alistair23.me> wrote:
> Signed-off-by: Alistair Francis <alistair@alistair23.me>
> Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org>

> --- a/drivers/mfd/simple-mfd-i2c.c
> +++ b/drivers/mfd/simple-mfd-i2c.c
> @@ -62,8 +62,19 @@ static int simple_mfd_i2c_probe(struct i2c_client *i2c)
>         return ret;
>  }
>
> +static const struct mfd_cell sy7636a_cells[] = {
> +       { .name = "sy7636a-regulator", },
> +       { .name = "sy7636a-temperature", },
> +};
> +
> +static const struct simple_mfd_data silergy_sy7636a = {
> +       .mfd_cell = sy7636a_cells,
> +       .mfd_cell_size = ARRAY_SIZE(sy7636a_cells),
> +};
> +
>  static const struct of_device_id simple_mfd_i2c_of_match[] = {
>         { .compatible = "kontron,sl28cpld" },
> +       { .compatible = "silergy,sy7636a", .data = &silergy_sy7636a},
>         {}
>  };

This is slowly turning into a "board file" for an ever-growing random
bunch of devices, none of which is described in the help text?

See also my reply to patch 2/8
https://lore.kernel.org/all/CAMuHMdVy4E1pX+VLLq_05FX4pM+BPZycQgn68ArGh2s8qL24=w@mail.gmail.com/

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
Guenter Roeck March 19, 2022, 2:48 p.m. UTC | #7
On 3/19/22 02:28, Geert Uytterhoeven wrote:
> Hi Alistair,
> 
> On Sat, Mar 19, 2022 at 3:36 AM Alistair Francis <alistair23@gmail.com> wrote:
>> On Tue, Mar 8, 2022 at 8:53 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>>> Thanks for your patch, which is now commit bae5a4acef67db88
>>> ("mfd: simple-mfd-i2c: Add a Kconfig name") in mfd/for-mfd-next.
>>>
>>> On Mon, Jan 24, 2022 at 1:24 PM Alistair Francis <alistair@alistair23.me> wrote:
>>>> Add a Kconfig name to the "Simple Multi-Functional Device support (I2C)"
>>>> device so that it can be enabled via menuconfig.
>>>
>>> Which still does not explain why this would be needed...
>>>
>>>> Signed-off-by: Alistair Francis <alistair@alistair23.me>
>>>> Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org>
>>>
>>>> --- a/drivers/mfd/Kconfig
>>>> +++ b/drivers/mfd/Kconfig
>>>> @@ -1188,7 +1188,7 @@ config MFD_SI476X_CORE
>>>>            module will be called si476x-core.
>>>>
>>>>   config MFD_SIMPLE_MFD_I2C
>>>> -       tristate
>>>> +       tristate "Simple Multi-Functional Device support (I2C)"
>>>>          depends on I2C
>>>>          select MFD_CORE
>>>>          select REGMAP_I2C
>>>
>>> The help text states:
>>>
>>> | This driver creates a single register map with the intention for it
>>> | to be shared by all sub-devices.
>>>
>>> Yes, that's what MFD does?
>>>
>>> | Once the register map has been successfully initialised, any
>>> | sub-devices represented by child nodes in Device Tree will be
>>> | subsequently registered.
>>>
>>> OK...?
>>>
>>> Still, no clue about what this driver really does, and why and when
>>> it would be needed.
>>>
>>> There is one driver symbol that selects MFD_SIMPLE_MFD_I2C.
>>> There are no driver symbols that depend on this symbol.
>>>
>>> If you have a driver in the pipeline that can make use of this,
>>> can't it just select MFD_SIMPLE_MFD_I2C, so the symbol itself can
>>> stay invisible?
>>
>> My patch "mfd: simple-mfd-i2c: Enable support for the silergy,sy7636a"
>> allows using this driver for the silergy,sy7636a MFD. So it's nice to
>> be able to enable and disable it as required.
> 
> So after that patch, enabling MFD_SIMPLE_MFD_I2C will enable
> support for an ever-growing random bunch of devices, none of which
> is described in the help text?
> To me, ghat doesn't look like the way to go forward...
> 

I am probably missing something. Why not something like the following ?

config MFD_SY7636A
         tristate "Silergy SY7636A voltage regulator"
         depends on I2C
         select MFD_SIMPLE_MFD_I2C
         help
           Enable support for Silergy SY7636A voltage regulator.

           To enable support for building sub-devices as modules,
           choose M here.


This would be quite similar to MFD_SL28CPLD which essentially does
the same (and, unless I am missing something, doesn't have its own
driver either). Sub-devices would then depend on MFD_SY7636A.

Guenter

> Gr{oetje,eeting}s,
> 
>                          Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                  -- Linus Torvalds
Alistair Francis March 21, 2022, 7:34 a.m. UTC | #8
On Tue, Mar 8, 2022 at 9:21 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Alistair,
>
> On Mon, Jan 24, 2022 at 1:25 PM Alistair Francis <alistair@alistair23.me> wrote:
> > This is a multi-function device to interface with the sy7636a
> > EPD PMIC chip from Silergy.
> >
> > Signed-off-by: Alistair Francis <alistair@alistair23.me>
> > Acked-by: Guenter Roeck <linux@roeck-us.net>
>
> Thanks for your patch, which is now commit de34a40532507814 ("hwmon:
> sy7636a: Add temperature driver for sy7636a") in mfd/for-mfd-next.
>
> > --- a/drivers/hwmon/Kconfig
> > +++ b/drivers/hwmon/Kconfig
> > @@ -1672,6 +1672,15 @@ config SENSORS_SIS5595
> >           This driver can also be built as a module. If so, the module
> >           will be called sis5595.
> >
> > +config SENSORS_SY7636A
> > +       tristate "Silergy SY7636A"
> > +       help
> > +         If you say yes here you get support for the thermistor readout of
> > +         the Silergy SY7636A PMIC.
>
> As this is an i2c mfd device, you do need a dependency on MFD and I2C,
> or some other symbol, unless compile-testing?

It doesn't depend on either to build though. It can be built independently.

Alistair

>
> > +
> > +         This driver can also be built as a module.  If so, the module
> > +         will be called sy7636a-hwmon.
> > +
>
> Gr{oetje,eeting}s,
>
>                         Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds
Alistair Francis March 21, 2022, 7:45 a.m. UTC | #9
On Sun, Mar 20, 2022 at 12:48 AM Guenter Roeck <linux@roeck-us.net> wrote:
>
> On 3/19/22 02:28, Geert Uytterhoeven wrote:
> > Hi Alistair,
> >
> > On Sat, Mar 19, 2022 at 3:36 AM Alistair Francis <alistair23@gmail.com> wrote:
> >> On Tue, Mar 8, 2022 at 8:53 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> >>> Thanks for your patch, which is now commit bae5a4acef67db88
> >>> ("mfd: simple-mfd-i2c: Add a Kconfig name") in mfd/for-mfd-next.
> >>>
> >>> On Mon, Jan 24, 2022 at 1:24 PM Alistair Francis <alistair@alistair23.me> wrote:
> >>>> Add a Kconfig name to the "Simple Multi-Functional Device support (I2C)"
> >>>> device so that it can be enabled via menuconfig.
> >>>
> >>> Which still does not explain why this would be needed...
> >>>
> >>>> Signed-off-by: Alistair Francis <alistair@alistair23.me>
> >>>> Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org>
> >>>
> >>>> --- a/drivers/mfd/Kconfig
> >>>> +++ b/drivers/mfd/Kconfig
> >>>> @@ -1188,7 +1188,7 @@ config MFD_SI476X_CORE
> >>>>            module will be called si476x-core.
> >>>>
> >>>>   config MFD_SIMPLE_MFD_I2C
> >>>> -       tristate
> >>>> +       tristate "Simple Multi-Functional Device support (I2C)"
> >>>>          depends on I2C
> >>>>          select MFD_CORE
> >>>>          select REGMAP_I2C
> >>>
> >>> The help text states:
> >>>
> >>> | This driver creates a single register map with the intention for it
> >>> | to be shared by all sub-devices.
> >>>
> >>> Yes, that's what MFD does?
> >>>
> >>> | Once the register map has been successfully initialised, any
> >>> | sub-devices represented by child nodes in Device Tree will be
> >>> | subsequently registered.
> >>>
> >>> OK...?
> >>>
> >>> Still, no clue about what this driver really does, and why and when
> >>> it would be needed.
> >>>
> >>> There is one driver symbol that selects MFD_SIMPLE_MFD_I2C.
> >>> There are no driver symbols that depend on this symbol.
> >>>
> >>> If you have a driver in the pipeline that can make use of this,
> >>> can't it just select MFD_SIMPLE_MFD_I2C, so the symbol itself can
> >>> stay invisible?
> >>
> >> My patch "mfd: simple-mfd-i2c: Enable support for the silergy,sy7636a"
> >> allows using this driver for the silergy,sy7636a MFD. So it's nice to
> >> be able to enable and disable it as required.
> >
> > So after that patch, enabling MFD_SIMPLE_MFD_I2C will enable
> > support for an ever-growing random bunch of devices, none of which
> > is described in the help text?
> > To me, ghat doesn't look like the way to go forward...
> >
>
> I am probably missing something. Why not something like the following ?
>
> config MFD_SY7636A
>          tristate "Silergy SY7636A voltage regulator"
>          depends on I2C
>          select MFD_SIMPLE_MFD_I2C
>          help
>            Enable support for Silergy SY7636A voltage regulator.
>
>            To enable support for building sub-devices as modules,
>            choose M here.
>
>
> This would be quite similar to MFD_SL28CPLD which essentially does
> the same (and, unless I am missing something, doesn't have its own
> driver either). Sub-devices would then depend on MFD_SY7636A.

That's fine with me.

As you said this patch is already in the mfd/for-mfd-next tree, should
I resend the series?

Alistair

>
> Guenter
>
> > Gr{oetje,eeting}s,
> >
> >                          Geert
> >
> > --
> > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> >
> > In personal conversations with technical people, I call myself a hacker. But
> > when I'm talking to journalists I just say "programmer" or something like that.
> >                                  -- Linus Torvalds
>
Geert Uytterhoeven March 21, 2022, 8:02 a.m. UTC | #10
Hi Alistair,

On Mon, Mar 21, 2022 at 8:35 AM Alistair Francis <alistair23@gmail.com> wrote:
> On Tue, Mar 8, 2022 at 9:21 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > On Mon, Jan 24, 2022 at 1:25 PM Alistair Francis <alistair@alistair23.me> wrote:
> > > This is a multi-function device to interface with the sy7636a
> > > EPD PMIC chip from Silergy.
> > >
> > > Signed-off-by: Alistair Francis <alistair@alistair23.me>
> > > Acked-by: Guenter Roeck <linux@roeck-us.net>
> >
> > Thanks for your patch, which is now commit de34a40532507814 ("hwmon:
> > sy7636a: Add temperature driver for sy7636a") in mfd/for-mfd-next.
> >
> > > --- a/drivers/hwmon/Kconfig
> > > +++ b/drivers/hwmon/Kconfig
> > > @@ -1672,6 +1672,15 @@ config SENSORS_SIS5595
> > >           This driver can also be built as a module. If so, the module
> > >           will be called sis5595.
> > >
> > > +config SENSORS_SY7636A
> > > +       tristate "Silergy SY7636A"
> > > +       help
> > > +         If you say yes here you get support for the thermistor readout of
> > > +         the Silergy SY7636A PMIC.
> >
> > As this is an i2c mfd device, you do need a dependency on MFD and I2C,
> > or some other symbol, unless compile-testing?
>
> It doesn't depend on either to build though. It can be built independently.

That is true.

But do you think all users configuring their kernel should be asked
about all (thousands) of config symbols for drivers that can be built,
but won't work or are irrelevant otherwise?

Please read
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/kbuild/kconfig-language.rst#n541

Thanks!

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
Lee Jones March 21, 2022, 8:48 a.m. UTC | #11
On Mon, 21 Mar 2022, Alistair Francis wrote:

> On Sun, Mar 20, 2022 at 12:48 AM Guenter Roeck <linux@roeck-us.net> wrote:
> >
> > On 3/19/22 02:28, Geert Uytterhoeven wrote:
> > > Hi Alistair,
> > >
> > > On Sat, Mar 19, 2022 at 3:36 AM Alistair Francis <alistair23@gmail.com> wrote:
> > >> On Tue, Mar 8, 2022 at 8:53 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > >>> Thanks for your patch, which is now commit bae5a4acef67db88
> > >>> ("mfd: simple-mfd-i2c: Add a Kconfig name") in mfd/for-mfd-next.
> > >>>
> > >>> On Mon, Jan 24, 2022 at 1:24 PM Alistair Francis <alistair@alistair23.me> wrote:
> > >>>> Add a Kconfig name to the "Simple Multi-Functional Device support (I2C)"
> > >>>> device so that it can be enabled via menuconfig.
> > >>>
> > >>> Which still does not explain why this would be needed...
> > >>>
> > >>>> Signed-off-by: Alistair Francis <alistair@alistair23.me>
> > >>>> Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org>
> > >>>
> > >>>> --- a/drivers/mfd/Kconfig
> > >>>> +++ b/drivers/mfd/Kconfig
> > >>>> @@ -1188,7 +1188,7 @@ config MFD_SI476X_CORE
> > >>>>            module will be called si476x-core.
> > >>>>
> > >>>>   config MFD_SIMPLE_MFD_I2C
> > >>>> -       tristate
> > >>>> +       tristate "Simple Multi-Functional Device support (I2C)"
> > >>>>          depends on I2C
> > >>>>          select MFD_CORE
> > >>>>          select REGMAP_I2C
> > >>>
> > >>> The help text states:
> > >>>
> > >>> | This driver creates a single register map with the intention for it
> > >>> | to be shared by all sub-devices.
> > >>>
> > >>> Yes, that's what MFD does?
> > >>>
> > >>> | Once the register map has been successfully initialised, any
> > >>> | sub-devices represented by child nodes in Device Tree will be
> > >>> | subsequently registered.
> > >>>
> > >>> OK...?
> > >>>
> > >>> Still, no clue about what this driver really does, and why and when
> > >>> it would be needed.
> > >>>
> > >>> There is one driver symbol that selects MFD_SIMPLE_MFD_I2C.
> > >>> There are no driver symbols that depend on this symbol.
> > >>>
> > >>> If you have a driver in the pipeline that can make use of this,
> > >>> can't it just select MFD_SIMPLE_MFD_I2C, so the symbol itself can
> > >>> stay invisible?
> > >>
> > >> My patch "mfd: simple-mfd-i2c: Enable support for the silergy,sy7636a"
> > >> allows using this driver for the silergy,sy7636a MFD. So it's nice to
> > >> be able to enable and disable it as required.
> > >
> > > So after that patch, enabling MFD_SIMPLE_MFD_I2C will enable
> > > support for an ever-growing random bunch of devices, none of which
> > > is described in the help text?
> > > To me, ghat doesn't look like the way to go forward...
> > >
> >
> > I am probably missing something. Why not something like the following ?
> >
> > config MFD_SY7636A
> >          tristate "Silergy SY7636A voltage regulator"
> >          depends on I2C
> >          select MFD_SIMPLE_MFD_I2C
> >          help
> >            Enable support for Silergy SY7636A voltage regulator.
> >
> >            To enable support for building sub-devices as modules,
> >            choose M here.
> >
> >
> > This would be quite similar to MFD_SL28CPLD which essentially does
> > the same (and, unless I am missing something, doesn't have its own
> > driver either). Sub-devices would then depend on MFD_SY7636A.
> 
> That's fine with me.
> 
> As you said this patch is already in the mfd/for-mfd-next tree, should
> I resend the series?

Making the symbol selectable-only is fine with me also.

Please send a subsequent patch.