Re: [PATCH v3 3/7] mfd: Add MFD driver for ATC260x PMICs
On Mon, Dec 21, 2020 at 01:18:01PM +0100, Linus Walleij wrote:
> On Mon, Dec 21, 2020 at 12:59 PM Cristian Ciocaltea
> wrote:
>
> > enum atc260x_ver {
> > ATC260X_A = 0,
> > ATC260X_B,
> > ATC260X_C,
> > ATC260X_D,
> > ATC260X_E,
> > ATC260X_F,
> > ATC260X_G,
> > ATC260X_H,
> > };
>
> This makes it look like the driver is actually so generic that it makes space
> for all revisions back to ATC2603A which is in the Ainol Hero 10 tablet.
For ATC2603A we need an SPI driver, currently only the I2C interface is
supported.
> This is nice because there are millions of these devices (especially in
> China) that people want to get to run the latest Linux.
>
> I even wonder how much different the ATM7029 is from S500, I suspect
> not super much apart from the ARM cores.
Cannot tell, for the moment I can only "play" with the S500..
> Good work overall! I'll be happy to deal with the GPIO
> driver when you get there.
Great, thanks!
> Yours,
> Linus Walleij
Re: [PATCH v3 3/7] mfd: Add MFD driver for ATC260x PMICs
On Mon, Dec 21, 2020 at 12:59 PM Cristian Ciocaltea
wrote:
> enum atc260x_ver {
> ATC260X_A = 0,
> ATC260X_B,
> ATC260X_C,
> ATC260X_D,
> ATC260X_E,
> ATC260X_F,
> ATC260X_G,
> ATC260X_H,
> };
This makes it look like the driver is actually so generic that it makes space
for all revisions back to ATC2603A which is in the Ainol Hero 10 tablet.
This is nice because there are millions of these devices (especially in
China) that people want to get to run the latest Linux.
I even wonder how much different the ATM7029 is from S500, I suspect
not super much apart from the ARM cores.
Good work overall! I'll be happy to deal with the GPIO
driver when you get there.
Yours,
Linus Walleij
Re: [PATCH v3 3/7] mfd: Add MFD driver for ATC260x PMICs
On Mon, Dec 21, 2020 at 08:10:15AM +, Lee Jones wrote:
> On Fri, 18 Dec 2020, Cristian Ciocaltea wrote:
>
> > On Fri, Dec 18, 2020 at 01:21:39PM +, Lee Jones wrote:
> > > On Fri, 18 Dec 2020, Cristian Ciocaltea wrote:
> > >
> > > > Hi Lee,
> > > >
> > > > Thank you for the detailed review!
> > > >
> > > > I will prepare a new revision, but there are still a couple of open
> > > > points..
> > >
> > > Could you please snip your replies, leaving only the open points.
> > >
> > > Scrolling through lots of empty quotes or "done" comments is quite
> > > time consuming. Thanks.
> >
> > Sure, I'll take that into account.
> >
> > > [...]
> > >
> > > > > > + ret = regmap_read(atc260x->regmap, atc260x->rev_reg, _rev);
> > > > > > + if (ret) {
> > > > > > + dev_err(dev, "Failed to get chip revision\n");
> > > > > > + return ret;
> > > > > > + }
> > > > > > +
> > > > > > + if (chip_rev < 0 || chip_rev > 31) {
> > > > > > + dev_err(dev, "Unknown chip revision: %d\n", ret);
> > > > > > + return -EINVAL;
> > > > > > + }
> > > > >
> > > > > This still seems limiting.
> > > >
> > > > This is based on the vendor implementation. Unfortunately I don't have
> > > > access to a data sheet or any other source of information about the
> > > > management of the chip revisions.
> > >
> > > So which versions does this driver work with? All 32?
> >
> > I'm not even sure there are so many revisions, I guess that's just a
> > rough validation for a vendor reserved range.
> >
> > For the moment, the only place where the functionality is affected
> > by the chip revision is in the regulator driver - there is a special
> > handling for the ATC2603C rev.B chip variant.
> >
> > I expect some additional handling might be required for new drivers
> > bringing support for the other functions provided by the hardware.
>
> The current patch seems to insinuate that 32 versions are currently
> supported. What is the chip_rev for the ATC2603C rev.B?
I only own the rev.A for the ATC2603C variant, for which I read '0' from
the chip rev register.
However what really matters for the driver is not the raw value, but the
one computed via:
atc260x->ic_ver = __ffs(chip_rev + 1U);
This is basically a translation of the raw value to a chip version
that is used in the context of the special handling mentioned above:
enum atc260x_ver {
ATC260X_A = 0,
ATC260X_B,
ATC260X_C,
ATC260X_D,
ATC260X_E,
ATC260X_F,
ATC260X_G,
ATC260X_H,
};
So we actually could handle up to 8 chip versions with the current
management scheme.
> --
> Lee Jones [李琼斯]
> Senior Technical Lead - Developer Services
> Linaro.org │ Open source software for Arm SoCs
> Follow Linaro: Facebook | Twitter | Blog
Re: [PATCH v3 3/7] mfd: Add MFD driver for ATC260x PMICs
On Fri, 18 Dec 2020, Cristian Ciocaltea wrote:
> On Fri, Dec 18, 2020 at 01:21:39PM +, Lee Jones wrote:
> > On Fri, 18 Dec 2020, Cristian Ciocaltea wrote:
> >
> > > Hi Lee,
> > >
> > > Thank you for the detailed review!
> > >
> > > I will prepare a new revision, but there are still a couple of open
> > > points..
> >
> > Could you please snip your replies, leaving only the open points.
> >
> > Scrolling through lots of empty quotes or "done" comments is quite
> > time consuming. Thanks.
>
> Sure, I'll take that into account.
>
> > [...]
> >
> > > > > + ret = regmap_read(atc260x->regmap, atc260x->rev_reg, _rev);
> > > > > + if (ret) {
> > > > > + dev_err(dev, "Failed to get chip revision\n");
> > > > > + return ret;
> > > > > + }
> > > > > +
> > > > > + if (chip_rev < 0 || chip_rev > 31) {
> > > > > + dev_err(dev, "Unknown chip revision: %d\n", ret);
> > > > > + return -EINVAL;
> > > > > + }
> > > >
> > > > This still seems limiting.
> > >
> > > This is based on the vendor implementation. Unfortunately I don't have
> > > access to a data sheet or any other source of information about the
> > > management of the chip revisions.
> >
> > So which versions does this driver work with? All 32?
>
> I'm not even sure there are so many revisions, I guess that's just a
> rough validation for a vendor reserved range.
>
> For the moment, the only place where the functionality is affected
> by the chip revision is in the regulator driver - there is a special
> handling for the ATC2603C rev.B chip variant.
>
> I expect some additional handling might be required for new drivers
> bringing support for the other functions provided by the hardware.
The current patch seems to insinuate that 32 versions are currently
supported. What is the chip_rev for the ATC2603C rev.B?
--
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog
Re: [PATCH v3 3/7] mfd: Add MFD driver for ATC260x PMICs
On Fri, Dec 18, 2020 at 01:21:39PM +, Lee Jones wrote:
> On Fri, 18 Dec 2020, Cristian Ciocaltea wrote:
>
> > Hi Lee,
> >
> > Thank you for the detailed review!
> >
> > I will prepare a new revision, but there are still a couple of open
> > points..
>
> Could you please snip your replies, leaving only the open points.
>
> Scrolling through lots of empty quotes or "done" comments is quite
> time consuming. Thanks.
Sure, I'll take that into account.
> [...]
>
> > > > + ret = regmap_read(atc260x->regmap, atc260x->rev_reg, _rev);
> > > > + if (ret) {
> > > > + dev_err(dev, "Failed to get chip revision\n");
> > > > + return ret;
> > > > + }
> > > > +
> > > > + if (chip_rev < 0 || chip_rev > 31) {
> > > > + dev_err(dev, "Unknown chip revision: %d\n", ret);
> > > > + return -EINVAL;
> > > > + }
> > >
> > > This still seems limiting.
> >
> > This is based on the vendor implementation. Unfortunately I don't have
> > access to a data sheet or any other source of information about the
> > management of the chip revisions.
>
> So which versions does this driver work with? All 32?
I'm not even sure there are so many revisions, I guess that's just a
rough validation for a vendor reserved range.
For the moment, the only place where the functionality is affected
by the chip revision is in the regulator driver - there is a special
handling for the ATC2603C rev.B chip variant.
I expect some additional handling might be required for new drivers
bringing support for the other functions provided by the hardware.
> [...]
Thanks,
Cristi
Re: [PATCH v3 3/7] mfd: Add MFD driver for ATC260x PMICs
On Fri, 18 Dec 2020, Cristian Ciocaltea wrote:
> Hi Lee,
>
> Thank you for the detailed review!
>
> I will prepare a new revision, but there are still a couple of open
> points..
Could you please snip your replies, leaving only the open points.
Scrolling through lots of empty quotes or "done" comments is quite
time consuming. Thanks.
[...]
> > > + /*
> > > + * Using regmap within an atomic context (e.g. accessing a PMIC when
> > > + * powering system down) is normally allowed only if the regmap type
> > > + * is MMIO and the regcache type is either REGCACHE_NONE or
> > > + * REGCACHE_FLAT. For slow buses like I2C and SPI, the regmap is
> > > + * internally protected by a mutex which is acquired non-atomically.
> > > + *
> > > + * Let's improve this by using a customized locking scheme inspired
> > > + * from I2C atomic transfer. See i2c_in_atomic_xfer_mode() for a
> > > + * starting point.
> > > + */
> > > + if (system_state > SYSTEM_RUNNING && irqs_disabled())
> >
> > Were does system_state come from?
>
> It is declared in 'include/linux/kernel.h':
>
> extern enum system_states {
> SYSTEM_BOOTING,
> SYSTEM_SCHEDULING,
> SYSTEM_RUNNING,
> SYSTEM_HALT,
> SYSTEM_POWER_OFF,
> SYSTEM_RESTART,
> SYSTEM_SUSPEND,
> } system_state;
>
> The definition is in 'init/main.c':
>
> enum system_states system_state __read_mostly;
> EXPORT_SYMBOL(system_state);
Ah, it's a system wide thing. No problem.
[...]
> > > + ret = regmap_read(atc260x->regmap, atc260x->rev_reg, _rev);
> > > + if (ret) {
> > > + dev_err(dev, "Failed to get chip revision\n");
> > > + return ret;
> > > + }
> > > +
> > > + if (chip_rev < 0 || chip_rev > 31) {
> > > + dev_err(dev, "Unknown chip revision: %d\n", ret);
> > > + return -EINVAL;
> > > + }
> >
> > This still seems limiting.
>
> This is based on the vendor implementation. Unfortunately I don't have
> access to a data sheet or any other source of information about the
> management of the chip revisions.
So which versions does this driver work with? All 32?
[...]
> > > +const struct of_device_id atc260x_i2c_of_match[] = {
> > > + { .compatible = "actions,atc2603c", .data = (void *)ATC2603C },
> > > + { .compatible = "actions,atc2609a", .data = (void *)ATC2609A },
> > > + { /* sentinel */ }
> >
> > I think you can drop the (void *) casts.
>
> Without the cast, I get the following compiler warning:
>
> drivers/mfd/atc260x-i2c.c:46:46: warning: initialization of ‘const void *’
> from ‘int’ makes pointer from integer without a cast [-Wint-conversion]
> { .compatible = "actions,atc2603c", .data = ATC2603C },
Perhaps I'm getting confused with addresses of things. Never mind.
--
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog
Re: [PATCH v3 3/7] mfd: Add MFD driver for ATC260x PMICs
Hi Lee,
Thank you for the detailed review!
I will prepare a new revision, but there are still a couple of open
points..
On Wed, Dec 16, 2020 at 10:10:00AM +, Lee Jones wrote:
> On Sun, 06 Dec 2020, Cristian Ciocaltea wrote:
>
> > Add initial support for the Actions Semi ATC260x PMICs. ATC260x series
> > PMICs integrates Audio Codec, Power management, Clock generation and
>
> "which integrates"
Done.
> > GPIO controller blocks.
> >
> > For the moment this driver only supports Regulator, Poweroff and Onkey
> > functionalities for the ATC2603C and ATC2609A chip variants.
> >
> > Since the PMICs can be accessed using both I2C and SPI buses, the
> > following driver structure has been adopted:
> >
> >-> atc260x-core.c (Implements core functionalities)
> > /
> > ATC260x > atc260x-i2c.c (Implements I2C interface)
> > \
> >-> atc260x-spi.c (Implements SPI interface - TODO)
> >
> > Co-developed-by: Manivannan Sadhasivam
> > Signed-off-by: Manivannan Sadhasivam
> > Signed-off-by: Cristian Ciocaltea
> > ---
> > Changes in v3:
> > - Fixed the issues reported by Lee's kernel test robot:
> >WARNING: modpost: missing MODULE_LICENSE() in drivers/mfd/atc260x-core.o
> >>> FATAL: modpost: drivers/mfd/atc260x-i2c: sizeof(struct
> > i2c_device_id)=24 is
> > not a modulo of the size of section
> > __mod_i2c___device_table=588.
> >>> Fix definition of struct i2c_device_id in mod_devicetable.h
> > - Dropped the usage of '.of_compatible' fields in
> > {atc2603c,atc2609a}_mfd_cells[]
> > - Added 'Co-developed-by' tag in commit message and dropped [cristian:
> > ...] line
> >
> > drivers/mfd/Kconfig | 18 ++
> > drivers/mfd/Makefile | 3 +
> > drivers/mfd/atc260x-core.c | 290 +
> > drivers/mfd/atc260x-i2c.c| 73 +++
> > include/linux/mfd/atc260x/atc2603c.h | 281
> > include/linux/mfd/atc260x/atc2609a.h | 308 +++
> > include/linux/mfd/atc260x/core.h | 86
> > 7 files changed, 1059 insertions(+)
> > create mode 100644 drivers/mfd/atc260x-core.c
> > create mode 100644 drivers/mfd/atc260x-i2c.c
> > create mode 100644 include/linux/mfd/atc260x/atc2603c.h
> > create mode 100644 include/linux/mfd/atc260x/atc2609a.h
> > create mode 100644 include/linux/mfd/atc260x/core.h
> >
> > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> > index 8b99a13669bf..5556182af41c 100644
> > --- a/drivers/mfd/Kconfig
> > +++ b/drivers/mfd/Kconfig
> > @@ -2076,6 +2076,24 @@ config MFD_WCD934X
> > This driver provides common support WCD934x audio codec and its
> > associated Pin Controller, Soundwire Controller and Audio codec.
> >
> > +config MFD_ATC260X
> > + tristate
> > + select MFD_CORE
> > + select REGMAP
> > + select REGMAP_IRQ
> > +
> > +config MFD_ATC260X_I2C
> > + tristate "Actions Semi ATC260x PMICs with I2C"
> > + select MFD_ATC260X
> > + select REGMAP_I2C
> > + depends on I2C
> > + help
> > + Support for the Actions Semi ATC260x PMICs controlled via I2C.
> > +
> > + This driver provides common support for accessing the ATC2603C
> > + and ATC2609A chip variants, additional drivers must be enabled
> > + in order to use the functionality of the device.
> > +
> > config MFD_KHADAS_MCU
> > tristate "Support for Khadas System control Microcontroller"
> > depends on I2C
> > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> > index 1780019d2474..d10362670ac3 100644
> > --- a/drivers/mfd/Makefile
> > +++ b/drivers/mfd/Makefile
> > @@ -267,3 +267,6 @@ obj-$(CONFIG_MFD_KHADAS_MCU)+= khadas-mcu.o
> > obj-$(CONFIG_SGI_MFD_IOC3) += ioc3.o
> > obj-$(CONFIG_MFD_SIMPLE_MFD_I2C) += simple-mfd-i2c.o
> > obj-$(CONFIG_MFD_INTEL_M10_BMC) += intel-m10-bmc.o
> > +
> > +obj-$(CONFIG_MFD_ATC260X) += atc260x-core.o
> > +obj-$(CONFIG_MFD_ATC260X_I2C) += atc260x-i2c.o
> > diff --git a/drivers/mfd/atc260x-core.c b/drivers/mfd/atc260x-core.c
> > new file mode 100644
> > index ..fd3a43b4030d
> > --- /dev/null
> > +++ b/drivers/mfd/atc260x-core.c
> > @@ -0,0 +1,290 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Core MFD support for ATC260x PMICs
>
> Drop the MFD part please.
Done.
> > + * Copyright (C) 2019 Manivannan Sadhasivam
> >
> > + * Copyright (C) 2020 Cristian Ciocaltea
> > + */
> > +
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> > +static void regmap_lock_mutex(void *__mutex)
> > +{
> > + struct mutex *mutex = __mutex;
> > +
> > + /*
> > +* Using regmap within an atomic context (e.g. accessing a PMIC when
> > +* powering system down) is normally allowed only if the regmap type
> > +* is MMIO and the regcache type is either REGCACHE_NONE or
> > +* REGCACHE_FLAT. For slow buses like I2C and SPI, the regmap is
Re: [PATCH v3 3/7] mfd: Add MFD driver for ATC260x PMICs
On Sun, 06 Dec 2020, Cristian Ciocaltea wrote:
> Add initial support for the Actions Semi ATC260x PMICs. ATC260x series
> PMICs integrates Audio Codec, Power management, Clock generation and
"which integrates"
> GPIO controller blocks.
>
> For the moment this driver only supports Regulator, Poweroff and Onkey
> functionalities for the ATC2603C and ATC2609A chip variants.
>
> Since the PMICs can be accessed using both I2C and SPI buses, the
> following driver structure has been adopted:
>
>-> atc260x-core.c (Implements core functionalities)
> /
> ATC260x > atc260x-i2c.c (Implements I2C interface)
> \
>-> atc260x-spi.c (Implements SPI interface - TODO)
>
> Co-developed-by: Manivannan Sadhasivam
> Signed-off-by: Manivannan Sadhasivam
> Signed-off-by: Cristian Ciocaltea
> ---
> Changes in v3:
> - Fixed the issues reported by Lee's kernel test robot:
>WARNING: modpost: missing MODULE_LICENSE() in drivers/mfd/atc260x-core.o
>>> FATAL: modpost: drivers/mfd/atc260x-i2c: sizeof(struct
> i2c_device_id)=24 is
> not a modulo of the size of section
> __mod_i2c___device_table=588.
>>> Fix definition of struct i2c_device_id in mod_devicetable.h
> - Dropped the usage of '.of_compatible' fields in
> {atc2603c,atc2609a}_mfd_cells[]
> - Added 'Co-developed-by' tag in commit message and dropped [cristian: ...]
> line
>
> drivers/mfd/Kconfig | 18 ++
> drivers/mfd/Makefile | 3 +
> drivers/mfd/atc260x-core.c | 290 +
> drivers/mfd/atc260x-i2c.c| 73 +++
> include/linux/mfd/atc260x/atc2603c.h | 281
> include/linux/mfd/atc260x/atc2609a.h | 308 +++
> include/linux/mfd/atc260x/core.h | 86
> 7 files changed, 1059 insertions(+)
> create mode 100644 drivers/mfd/atc260x-core.c
> create mode 100644 drivers/mfd/atc260x-i2c.c
> create mode 100644 include/linux/mfd/atc260x/atc2603c.h
> create mode 100644 include/linux/mfd/atc260x/atc2609a.h
> create mode 100644 include/linux/mfd/atc260x/core.h
>
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 8b99a13669bf..5556182af41c 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -2076,6 +2076,24 @@ config MFD_WCD934X
> This driver provides common support WCD934x audio codec and its
> associated Pin Controller, Soundwire Controller and Audio codec.
>
> +config MFD_ATC260X
> + tristate
> + select MFD_CORE
> + select REGMAP
> + select REGMAP_IRQ
> +
> +config MFD_ATC260X_I2C
> + tristate "Actions Semi ATC260x PMICs with I2C"
> + select MFD_ATC260X
> + select REGMAP_I2C
> + depends on I2C
> + help
> + Support for the Actions Semi ATC260x PMICs controlled via I2C.
> +
> + This driver provides common support for accessing the ATC2603C
> + and ATC2609A chip variants, additional drivers must be enabled
> + in order to use the functionality of the device.
> +
> config MFD_KHADAS_MCU
> tristate "Support for Khadas System control Microcontroller"
> depends on I2C
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 1780019d2474..d10362670ac3 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -267,3 +267,6 @@ obj-$(CONFIG_MFD_KHADAS_MCU) += khadas-mcu.o
> obj-$(CONFIG_SGI_MFD_IOC3) += ioc3.o
> obj-$(CONFIG_MFD_SIMPLE_MFD_I2C) += simple-mfd-i2c.o
> obj-$(CONFIG_MFD_INTEL_M10_BMC) += intel-m10-bmc.o
> +
> +obj-$(CONFIG_MFD_ATC260X)+= atc260x-core.o
> +obj-$(CONFIG_MFD_ATC260X_I2C)+= atc260x-i2c.o
> diff --git a/drivers/mfd/atc260x-core.c b/drivers/mfd/atc260x-core.c
> new file mode 100644
> index ..fd3a43b4030d
> --- /dev/null
> +++ b/drivers/mfd/atc260x-core.c
> @@ -0,0 +1,290 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Core MFD support for ATC260x PMICs
Drop the MFD part please.
> + * Copyright (C) 2019 Manivannan Sadhasivam
>
> + * Copyright (C) 2020 Cristian Ciocaltea
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +static void regmap_lock_mutex(void *__mutex)
> +{
> + struct mutex *mutex = __mutex;
> +
> + /*
> + * Using regmap within an atomic context (e.g. accessing a PMIC when
> + * powering system down) is normally allowed only if the regmap type
> + * is MMIO and the regcache type is either REGCACHE_NONE or
> + * REGCACHE_FLAT. For slow buses like I2C and SPI, the regmap is
> + * internally protected by a mutex which is acquired non-atomically.
> + *
> + * Let's improve this by using a customized locking scheme inspired
> + * from I2C atomic transfer. See i2c_in_atomic_xfer_mode() for a
> + * starting point.
> + */
> + if (system_state > SYSTEM_RUNNING && irqs_disabled())
Were does system_state come from?
> +
