[RFC,v2,05/13] clk: bd718x7: Support ROHM BD71828 clk block
diff mbox series

Message ID 5c66ac7d43ae1f57c335b6e565553fe1df703a83.1571915550.git.matti.vaittinen@fi.rohmeurope.com
State New
Headers show
Series
  • Support ROHM BD71828 PMIC
Related show

Commit Message

Vaittinen, Matti Oct. 24, 2019, 11:44 a.m. UTC
BD71828GW is a single-chip power management IC for battery-powered portable
devices. Add support for controlling BD71828 clk using bd718x7 driver.

Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
---

No changes since v1

 drivers/clk/Kconfig       |  6 +++---
 drivers/clk/clk-bd718x7.c | 15 ++++++++++-----
 2 files changed, 13 insertions(+), 8 deletions(-)

Comments

Stephen Boyd Oct. 28, 2019, 11:32 p.m. UTC | #1
Quoting Matti Vaittinen (2019-10-24 04:44:40)
> diff --git a/drivers/clk/clk-bd718x7.c b/drivers/clk/clk-bd718x7.c
> index ae6e5baee330..d17a19e04592 100644
> --- a/drivers/clk/clk-bd718x7.c
> +++ b/drivers/clk/clk-bd718x7.c
> @@ -8,6 +8,7 @@
>  #include <linux/platform_device.h>
>  #include <linux/slab.h>
>  #include <linux/mfd/rohm-bd718x7.h>
> +#include <linux/mfd/rohm-bd71828.h>
>  #include <linux/mfd/rohm-bd70528.h>

It would be really great to not need to include these random header
files in this driver and just use raw numbers somehow. Looks like maybe
it can be done by populating a different device name from the mfd driver
depending on the version of the clk controller desired? Then that can be
matched in this clk driver and we can just put the register info in this
file?

>  #include <linux/clk-provider.h>
>  #include <linux/clkdev.h>
> @@ -21,10 +22,8 @@ struct bd718xx_clk {
>         struct rohm_regmap_dev *mfd;
>  };
>  
> -static int bd71837_clk_set(struct clk_hw *hw, int status)
> +static int bd71837_clk_set(struct bd718xx_clk *c, int status)

should it be unsigned int status? Or maybe u32?

>  {
> -       struct bd718xx_clk *c = container_of(hw, struct bd718xx_clk, hw);
> -
>         return regmap_update_bits(c->mfd->regmap, c->reg, c->mask, status);
>  }
>  
> @@ -33,14 +32,16 @@ static void bd71837_clk_disable(struct clk_hw *hw)
>         int rv;
>         struct bd718xx_clk *c = container_of(hw, struct bd718xx_clk, hw);
>  
> -       rv = bd71837_clk_set(hw, 0);
> +       rv = bd71837_clk_set(c, 0);
>         if (rv)
>                 dev_dbg(&c->pdev->dev, "Failed to disable 32K clk (%d)\n", rv);
>  }
>  
>  static int bd71837_clk_enable(struct clk_hw *hw)
>  {
> -       return bd71837_clk_set(hw, 1);
> +       struct bd718xx_clk *c = container_of(hw, struct bd718xx_clk, hw);
> +
> +       return bd71837_clk_set(c, 0xffffffff);

Because now this is passing -1 to unsigned int taking
regmap_update_bits()?
Vaittinen, Matti Oct. 29, 2019, 6:28 a.m. UTC | #2
Hello Stephen,

Thanks for the comments once again :)

On Mon, 2019-10-28 at 16:32 -0700, Stephen Boyd wrote:
> Quoting Matti Vaittinen (2019-10-24 04:44:40)
> > diff --git a/drivers/clk/clk-bd718x7.c b/drivers/clk/clk-bd718x7.c
> > index ae6e5baee330..d17a19e04592 100644
> > --- a/drivers/clk/clk-bd718x7.c
> > +++ b/drivers/clk/clk-bd718x7.c
> > @@ -8,6 +8,7 @@
> >  #include <linux/platform_device.h>
> >  #include <linux/slab.h>
> >  #include <linux/mfd/rohm-bd718x7.h>
> > +#include <linux/mfd/rohm-bd71828.h>
> >  #include <linux/mfd/rohm-bd70528.h>
> 
> It would be really great to not need to include these random header
> files in this driver and just use raw numbers somehow. Looks like
> maybe
> it can be done by populating a different device name from the mfd
> driver
> depending on the version of the clk controller desired? Then that can
> be
> matched in this clk driver and we can just put the register info in
> this
> file?

I still like keeping the chip type information on one header - no
matter what-ever format the clk-controller type/version information is.
Rationale is that MFD and also few other sub-devices (not only the clk)
need to use it. Currently at least the RTC.

But if we define clk register information for all PMICs in this c-file, 
then (I think) we can only include the <linux/mfd/rohm-generic.h> -
which contains the PMIC type defines and the generic MFD data
structure. That would:

-#include <linux/mfd/rohm-bd718x7.h>
-#include <linux/mfd/rohm-bd71828.h>
-#include <linux/mfd/rohm-bd70528.h>
+#include <linux/mfd/rohm-generic.h>

That way the chip-type information could still be same for MFD and all
sub-devices but clk driver would not need to include all the details
for all the PMICs. I understand your point well as clk registers for
these PMICs are really *limited*.

> 
> >  #include <linux/clk-provider.h>
> >  #include <linux/clkdev.h>
> > @@ -21,10 +22,8 @@ struct bd718xx_clk {
> >         struct rohm_regmap_dev *mfd;
> >  };
> >  
> > -static int bd71837_clk_set(struct clk_hw *hw, int status)
> > +static int bd71837_clk_set(struct bd718xx_clk *c, int status)
> 
> should it be unsigned int status? Or maybe u32?
> 
> >  {
> > -       struct bd718xx_clk *c = container_of(hw, struct
> > bd718xx_clk, hw);
> > -
> >         return regmap_update_bits(c->mfd->regmap, c->reg, c->mask,
> > status);
> >  }
> >  
> > @@ -33,14 +32,16 @@ static void bd71837_clk_disable(struct clk_hw
> > *hw)
> >         int rv;
> >         struct bd718xx_clk *c = container_of(hw, struct
> > bd718xx_clk, hw);
> >  
> > -       rv = bd71837_clk_set(hw, 0);
> > +       rv = bd71837_clk_set(c, 0);
> >         if (rv)
> >                 dev_dbg(&c->pdev->dev, "Failed to disable 32K clk
> > (%d)\n", rv);
> >  }
> >  
> >  static int bd71837_clk_enable(struct clk_hw *hw)
> >  {
> > -       return bd71837_clk_set(hw, 1);
> > +       struct bd718xx_clk *c = container_of(hw, struct
> > bd718xx_clk, hw);
> > +
> > +       return bd71837_clk_set(c, 0xffffffff);
> 
> Because now this is passing -1 to unsigned int taking
> regmap_update_bits()?

I think that bit-wise it is all the same. Currently registers are only
8bits wide so it is enough that lowest 8 bits are set. And 0xffffffff
should work nicely up-to 32bit registers as long as int is 32 bit or
more.

But you are correct that this is not looking good. At first sight
unsigned int is much nicer. I prefer unsigned int over forced u32 to
guarantee natural alignment - which does not really matter in this case
either. unsigned int matches regmap though so I'll switch to it. Thanks
for pointing this out :)

I'll try to include these clk changes in v3.


Br,
	Matti Vaittinen
Vaittinen, Matti Nov. 5, 2019, 8:11 a.m. UTC | #3
Hello Stephen,

On Mon, 2019-11-04 at 16:55 -0800, Stephen Boyd wrote:
> Quoting Vaittinen, Matti (2019-10-28 23:28:51)
> > On Mon, 2019-10-28 at 16:32 -0700, Stephen Boyd wrote:
> > > Quoting Matti Vaittinen (2019-10-24 04:44:40)
> > > > diff --git a/drivers/clk/clk-bd718x7.c b/drivers/clk/clk-
> > > > bd718x7.c
> > > > index ae6e5baee330..d17a19e04592 100644
> > > > --- a/drivers/clk/clk-bd718x7.c
> > > > +++ b/drivers/clk/clk-bd718x7.c
> > > > @@ -8,6 +8,7 @@
> > > >  #include <linux/platform_device.h>
> > > >  #include <linux/slab.h>
> > > >  #include <linux/mfd/rohm-bd718x7.h>
> > > > +#include <linux/mfd/rohm-bd71828.h>
> > > >  #include <linux/mfd/rohm-bd70528.h>
> > > 
> > > It would be really great to not need to include these random
> > > header
> > > files in this driver and just use raw numbers somehow. Looks like
> > > maybe
> > > it can be done by populating a different device name from the mfd
> > > driver
> > > depending on the version of the clk controller desired? Then that
> > > can
> > > be
> > > matched in this clk driver and we can just put the register info
> > > in
> > > this
> > > file?
> > 
> > I still like keeping the chip type information on one header - no
> > matter what-ever format the clk-controller type/version information
> > is.
> > Rationale is that MFD and also few other sub-devices (not only the
> > clk)
> > need to use it. Currently at least the RTC.
> > 
> > But if we define clk register information for all PMICs in this c-
> > file, 
> > then (I think) we can only include the <linux/mfd/rohm-generic.h> -
> > which contains the PMIC type defines and the generic MFD data
> > structure. That would:
> > 
> > -#include <linux/mfd/rohm-bd718x7.h>
> > -#include <linux/mfd/rohm-bd71828.h>
> > -#include <linux/mfd/rohm-bd70528.h>
> > +#include <linux/mfd/rohm-generic.h>
> > 
> > That way the chip-type information could still be same for MFD and
> > all
> > sub-devices but clk driver would not need to include all the
> > details
> > for all the PMICs. I understand your point well as clk registers
> > for
> > these PMICs are really *limited*.
> > 
> 
> It's not even just about clk registers. It's also about how we have
> device compatible strings and device names but this driver isn't
> using
> them to differentiate. Instead, it's looking at the parent device. I
> don't get it. Why can't the MFD populate different clk devices for
> the
> different PMICs and make this driver completely oblivious to the
> parent
> device name/structure and these headers?

Probably because I didn't know how MFD/child device 'matching' works.

Do you mean the clk driver could do something like:

static const struct platform_device_id bd718x7_clk_id[] = {
        { "bd71837-clk", FOO},
        { "bd71847-clk", BAR},
        { "bd70528-clk", BAZ},
        { "bd71828-clk", BAF},
        { },
};
MODULE_DEVICE_TABLE(platform, bd718x7_clk_id);

static struct platform_driver bd71837_clk = {
        .driver = {
                .name = "bd718xx-clk",
        },
        .probe = bd71837_clk_probe,
	.id_table = bd718x7_clk_id
};

and then in MFD we just use correct name string for the mfd cell
representing the clk? (Eg. one of the bd71837-clk, bd71847-clk,
bd70528-clk, bd71828-clk) and in clk probe just differentiate based on
FOO, BAR, BAZ and BAF?

I guess we could do that (didn't try it out yet so I only guess for
now) - but I think this don't really mitigate the need for common
header. If we change the sub-device match mechanism to this then the
same mechanism should probably be applied to all sub-devices. And that
would be a case where I would like to see the very same FOO, BAR, BAZ
and BAF being used for all sub-devices - meaning it should still be a
MFD header. I think the drivers/clk/clk-s2mps11.c, drivers/mfd/sec-
core.c and include/linux/mfd/samsung/core.h are examples of this.

But I do like this platform_device_id based PMIC differentiation
better. It looks like the "de facto" way of doing this. Still, as I
said, I don't see we're getting rid of common header this way. Anyways,
I think I can cook-up patches to change this if I get buy-in from Lee,
Alexandre and Mark for changing the existing mechanism.

Thanks for teaching me something new once again! :)

Br,
	Matti Vaittinen

Patch
diff mbox series

diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index 801fa1cd0321..1d61d94cdb29 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -302,10 +302,10 @@  config COMMON_CLK_STM32H7
 	  Support for stm32h7 SoC family clocks
 
 config COMMON_CLK_BD718XX
-	tristate "Clock driver for ROHM BD718x7 PMIC"
-	depends on MFD_ROHM_BD718XX || MFD_ROHM_BD70528
+	tristate "Clock driver for 32K clk gates on ROHM PMICs"
+	depends on MFD_ROHM_BD718XX || MFD_ROHM_BD70528 || MFD_ROHM_BD71828
 	help
-	  This driver supports ROHM BD71837, ROHM BD71847 and
+	  This driver supports ROHM BD71837, ROHM BD71847, ROHM BD71828 and
 	  ROHM BD70528 PMICs clock gates.
 
 config COMMON_CLK_FIXED_MMIO
diff --git a/drivers/clk/clk-bd718x7.c b/drivers/clk/clk-bd718x7.c
index ae6e5baee330..d17a19e04592 100644
--- a/drivers/clk/clk-bd718x7.c
+++ b/drivers/clk/clk-bd718x7.c
@@ -8,6 +8,7 @@ 
 #include <linux/platform_device.h>
 #include <linux/slab.h>
 #include <linux/mfd/rohm-bd718x7.h>
+#include <linux/mfd/rohm-bd71828.h>
 #include <linux/mfd/rohm-bd70528.h>
 #include <linux/clk-provider.h>
 #include <linux/clkdev.h>
@@ -21,10 +22,8 @@  struct bd718xx_clk {
 	struct rohm_regmap_dev *mfd;
 };
 
-static int bd71837_clk_set(struct clk_hw *hw, int status)
+static int bd71837_clk_set(struct bd718xx_clk *c, int status)
 {
-	struct bd718xx_clk *c = container_of(hw, struct bd718xx_clk, hw);
-
 	return regmap_update_bits(c->mfd->regmap, c->reg, c->mask, status);
 }
 
@@ -33,14 +32,16 @@  static void bd71837_clk_disable(struct clk_hw *hw)
 	int rv;
 	struct bd718xx_clk *c = container_of(hw, struct bd718xx_clk, hw);
 
-	rv = bd71837_clk_set(hw, 0);
+	rv = bd71837_clk_set(c, 0);
 	if (rv)
 		dev_dbg(&c->pdev->dev, "Failed to disable 32K clk (%d)\n", rv);
 }
 
 static int bd71837_clk_enable(struct clk_hw *hw)
 {
-	return bd71837_clk_set(hw, 1);
+	struct bd718xx_clk *c = container_of(hw, struct bd718xx_clk, hw);
+
+	return bd71837_clk_set(c, 0xffffffff);
 }
 
 static int bd71837_clk_is_enabled(struct clk_hw *hw)
@@ -93,6 +94,10 @@  static int bd71837_clk_probe(struct platform_device *pdev)
 		c->reg = BD718XX_REG_OUT32K;
 		c->mask = BD718XX_OUT32K_EN;
 		break;
+	case ROHM_CHIP_TYPE_BD71828:
+		c->reg = BD71828_REG_OUT32K;
+		c->mask = BD71828_OUT32K_EN;
+		break;
 	case ROHM_CHIP_TYPE_BD70528:
 		c->reg = BD70528_REG_CLK_OUT;
 		c->mask = BD70528_CLK_OUT_EN_MASK;