diff mbox series

[2/2] clk: bd718x7: Initial support for ROHM bd71837/bd71847 PMIC clock

Message ID 0e6f19f99a4321d37472635c9210f6c13ac53147.1535630942.git.matti.vaittinen@fi.rohmeurope.com (mailing list archive)
State Superseded, archived
Headers show
Series clk: Support ROHM BD71837 (BD71847) | expand

Commit Message

Vaittinen, Matti Aug. 30, 2018, 12:15 p.m. UTC 
ROHM bd71837 and bd71847 contain 32768Hz clock gate. Support the clock
using generic clock framework. Note, only bd71837 is tested but bd71847
should be identical what comes to clk parts.

Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
---
 drivers/clk/Kconfig       |   6 +++
 drivers/clk/Makefile      |   1 +
 drivers/clk/clk-bd718x7.c | 131 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 138 insertions(+)
 create mode 100644 drivers/clk/clk-bd718x7.c

Comments

kernel test robot Aug. 31, 2018, 1:18 a.m. UTC | #1 
Hi Matti,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on clk/clk-next]
[also build test ERROR on v4.19-rc1 next-20180830]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Matti-Vaittinen/clk-clkdev-of_clk-add-managed-lookup-and-provider-registrations/20180831-045158
base:   https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git clk-next
config: x86_64-allmodconfig (attached as .config)
compiler: gcc-7 (Debian 7.3.0-16) 7.3.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All error/warnings (new ones prefixed by >>):

   In file included from include/linux/mfd/rohm-bd718x7.h:7:0,
                    from drivers/clk/clk-bd718x7.c:11:
   drivers/clk/clk-bd718x7.c: In function 'bd71837_clk_set':
>> drivers/clk/clk-bd718x7.c:28:34: error: dereferencing pointer to incomplete type 'struct bd718xx'
     return regmap_update_bits(c->mfd->regmap, c->reg, c->mask, status);
                                     ^
   include/linux/regmap.h:77:26: note: in definition of macro 'regmap_update_bits'
     regmap_update_bits_base(map, reg, mask, val, NULL, false, false)
                             ^~~
   drivers/clk/clk-bd718x7.c: In function 'bd71837_clk_probe':
>> drivers/clk/clk-bd718x7.c:91:11: error: 'BD718XX_REG_OUT32K' undeclared (first use in this function); did you mean 'BD71837_REG_OUT32K'?
     c->reg = BD718XX_REG_OUT32K;
              ^~~~~~~~~~~~~~~~~~
              BD71837_REG_OUT32K
   drivers/clk/clk-bd718x7.c:91:11: note: each undeclared identifier is reported only once for each function it appears in
>> drivers/clk/clk-bd718x7.c:92:12: error: 'BD718XX_OUT32K_EN' undeclared (first use in this function); did you mean 'BD71837_OUT32K_EN'?
     c->mask = BD718XX_OUT32K_EN;
               ^~~~~~~~~~~~~~~~~
               BD71837_OUT32K_EN
   drivers/clk/clk-bd718x7.c: In function 'bd71837_clk_set':
>> drivers/clk/clk-bd718x7.c:29:1: warning: control reaches end of non-void function [-Wreturn-type]
    }
    ^

vim +28 drivers/clk/clk-bd718x7.c

     4	
     5	#include <linux/kernel.h>
     6	#include <linux/module.h>
     7	#include <linux/init.h>
     8	#include <linux/err.h>
     9	#include <linux/platform_device.h>
    10	#include <linux/slab.h>
  > 11	#include <linux/mfd/rohm-bd718x7.h>
    12	#include <linux/clk-provider.h>
    13	#include <linux/clkdev.h>
    14	#include <linux/regmap.h>
    15	
    16	struct bd718xx_clk {
    17		struct clk_hw hw;
    18		u8 reg;
    19		u8 mask;
    20		struct platform_device *pdev;
    21		struct bd718xx *mfd;
    22	};
    23	
    24	static int bd71837_clk_set(struct clk_hw *hw, int status)
    25	{
    26		struct bd718xx_clk *c = container_of(hw, struct bd718xx_clk, hw);
    27	
  > 28		return regmap_update_bits(c->mfd->regmap, c->reg, c->mask, status);
  > 29	}
    30	
    31	static void bd71837_clk_disable(struct clk_hw *hw)
    32	{
    33		int rv;
    34		struct bd718xx_clk *c = container_of(hw, struct bd718xx_clk, hw);
    35	
    36		rv = bd71837_clk_set(hw, 0);
    37		if (rv)
    38			dev_dbg(&c->pdev->dev, "Failed to disable 32K clk (%d)\n", rv);
    39	}
    40	
    41	static int bd71837_clk_enable(struct clk_hw *hw)
    42	{
    43		return bd71837_clk_set(hw, 1);
    44	}
    45	
    46	static int bd71837_clk_is_enabled(struct clk_hw *hw)
    47	{
    48		int enabled;
    49		int rval;
    50		struct bd718xx_clk *c = container_of(hw, struct bd718xx_clk, hw);
    51	
    52		rval = regmap_read(c->mfd->regmap, c->reg, &enabled);
    53	
    54		if (rval)
    55			return rval;
    56	
    57		return enabled & c->mask;
    58	}
    59	
    60	static const struct clk_ops bd71837_clk_ops = {
    61		.prepare = &bd71837_clk_enable,
    62		.unprepare = &bd71837_clk_disable,
    63		.is_prepared = &bd71837_clk_is_enabled,
    64	};
    65	
    66	static int bd71837_clk_probe(struct platform_device *pdev)
    67	{
    68		struct bd718xx_clk *c;
    69		int rval = -ENOMEM;
    70		const char *parent_clk;
    71		struct device *parent = pdev->dev.parent;
    72		struct bd718xx *mfd = dev_get_drvdata(parent);
    73		struct clk_init_data init = {
    74			.name = "bd718xx-32k-out",
    75			.ops = &bd71837_clk_ops,
    76		};
    77	
    78		c = devm_kzalloc(&pdev->dev, sizeof(*c), GFP_KERNEL);
    79		if (!c)
    80			return -ENOMEM;
    81	
    82		init.num_parents = 1;
    83		parent_clk = of_clk_get_parent_name(parent->of_node, 0);
    84	
    85		init.parent_names = &parent_clk;
    86		if (!parent_clk) {
    87			dev_err(&pdev->dev, "No parent clk found\n");
    88			return -EINVAL;
    89		}
    90	
  > 91		c->reg = BD718XX_REG_OUT32K;
  > 92		c->mask = BD718XX_OUT32K_EN;
    93		c->mfd = mfd;
    94		c->pdev = pdev;
    95		c->hw.init = &init;
    96	
    97		of_property_read_string_index(parent->of_node,
    98					      "clock-output-names", 0, &init.name);
    99	
   100		rval = devm_clk_hw_register(&pdev->dev, &c->hw);
   101		if (!rval) {
   102			rval = devm_clk_hw_register_clkdev(&pdev->dev,
   103							   &c->hw, init.name, NULL);
   104			if (rval)
   105				dev_warn(&pdev->dev, "Failed to register clkdev\n");
   106			if (parent->of_node) {
   107				rval = devm_of_clk_add_parent_hw_provider(&pdev->dev,
   108						     of_clk_hw_simple_get, &c->hw);
   109				if (rval)
   110					dev_err(&pdev->dev,
   111						"adding clk provider failed\n");
   112			}
   113		} else {
   114			dev_err(&pdev->dev, "failed to register 32K clk");
   115		}
   116	
   117		return rval;
   118	}
   119	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
Vaittinen, Matti Aug. 31, 2018, 10:21 a.m. UTC | #2 
Hello All,

Just wanted to point out for the reviewers that this patch depends on
not yet accepted MFD/regulator patch. (struct/defines in
include/linux/mfd/rohm-bd718x7.h were changed)
https://lore.kernel.org/lkml/cover.1535545377.git.matti.vaittinen@fi.rohmeurope.com/

On Fri, Aug 31, 2018 at 09:18:19AM +0800, kbuild test robot wrote:
> Hi Matti,
> 
> Thank you for the patch! Yet something to improve:
> 
> [auto build test ERROR on clk/clk-next]
> [also build test ERROR on v4.19-rc1 next-20180830]
> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

Thus this error was expected. What is the generally best way to work
when there is changes to more than one subsystem? With this approach
the patch set here won't compile until MFD part gets applied. But if
I use old definitions/structs here, then clk tree gets broken when
MFD/regulator part changes defines. I see only bad and worse options =)
Anyways, I guess sending this patch with new defines (and applying it
only after MFD) is still better than applying this with old defines and
breaking it when MFD changes. (Assuming all of the patches get applied
at some point).

Br,
	Matti Vaittinen
>
kernel test robot Sept. 1, 2018, 3:57 a.m. UTC | #3 
Hi Matti,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on clk/clk-next]
[also build test ERROR on v4.19-rc1 next-20180831]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Matti-Vaittinen/clk-clkdev-of_clk-add-managed-lookup-and-provider-registrations/20180831-045158
base:   https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git clk-next
config: i386-allyesconfig (attached as .config)
compiler: gcc-7 (Debian 7.3.0-16) 7.3.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

   drivers/clk/clk-bd718x7.c: In function 'bd71837_clk_set':
   drivers/clk/clk-bd718x7.c:28:39: error: dereferencing pointer to incomplete type 'struct bd718xx'
     return regmap_update_bits(c->mfd->regmap, c->reg, c->mask, status);
                                          ^~
   drivers/clk/clk-bd718x7.c: In function 'bd71837_clk_probe':
   drivers/clk/clk-bd718x7.c:91:11: error: 'BD718XX_REG_OUT32K' undeclared (first use in this function); did you mean 'BD71837_REG_OUT32K'?
     c->reg = BD718XX_REG_OUT32K;
              ^~~~~~~~~~~~~~~~~~
              BD71837_REG_OUT32K
   drivers/clk/clk-bd718x7.c:91:11: note: each undeclared identifier is reported only once for each function it appears in
>> drivers/clk/clk-bd718x7.c:92:12: error: 'BD718XX_OUT32K_EN' undeclared (first use in this function); did you mean 'BD718XX_REG_OUT32K'?
     c->mask = BD718XX_OUT32K_EN;
               ^~~~~~~~~~~~~~~~~
               BD718XX_REG_OUT32K
   drivers/clk/clk-bd718x7.c: In function 'bd71837_clk_set':
   drivers/clk/clk-bd718x7.c:29:1: warning: control reaches end of non-void function [-Wreturn-type]
    }
    ^

vim +92 drivers/clk/clk-bd718x7.c

    23	
    24	static int bd71837_clk_set(struct clk_hw *hw, int status)
    25	{
    26		struct bd718xx_clk *c = container_of(hw, struct bd718xx_clk, hw);
    27	
  > 28		return regmap_update_bits(c->mfd->regmap, c->reg, c->mask, status);
    29	}
    30	
    31	static void bd71837_clk_disable(struct clk_hw *hw)
    32	{
    33		int rv;
    34		struct bd718xx_clk *c = container_of(hw, struct bd718xx_clk, hw);
    35	
    36		rv = bd71837_clk_set(hw, 0);
    37		if (rv)
    38			dev_dbg(&c->pdev->dev, "Failed to disable 32K clk (%d)\n", rv);
    39	}
    40	
    41	static int bd71837_clk_enable(struct clk_hw *hw)
    42	{
    43		return bd71837_clk_set(hw, 1);
    44	}
    45	
    46	static int bd71837_clk_is_enabled(struct clk_hw *hw)
    47	{
    48		int enabled;
    49		int rval;
    50		struct bd718xx_clk *c = container_of(hw, struct bd718xx_clk, hw);
    51	
    52		rval = regmap_read(c->mfd->regmap, c->reg, &enabled);
    53	
    54		if (rval)
    55			return rval;
    56	
    57		return enabled & c->mask;
    58	}
    59	
    60	static const struct clk_ops bd71837_clk_ops = {
    61		.prepare = &bd71837_clk_enable,
    62		.unprepare = &bd71837_clk_disable,
    63		.is_prepared = &bd71837_clk_is_enabled,
    64	};
    65	
    66	static int bd71837_clk_probe(struct platform_device *pdev)
    67	{
    68		struct bd718xx_clk *c;
    69		int rval = -ENOMEM;
    70		const char *parent_clk;
    71		struct device *parent = pdev->dev.parent;
    72		struct bd718xx *mfd = dev_get_drvdata(parent);
    73		struct clk_init_data init = {
    74			.name = "bd718xx-32k-out",
    75			.ops = &bd71837_clk_ops,
    76		};
    77	
    78		c = devm_kzalloc(&pdev->dev, sizeof(*c), GFP_KERNEL);
    79		if (!c)
    80			return -ENOMEM;
    81	
    82		init.num_parents = 1;
    83		parent_clk = of_clk_get_parent_name(parent->of_node, 0);
    84	
    85		init.parent_names = &parent_clk;
    86		if (!parent_clk) {
    87			dev_err(&pdev->dev, "No parent clk found\n");
    88			return -EINVAL;
    89		}
    90	
    91		c->reg = BD718XX_REG_OUT32K;
  > 92		c->mask = BD718XX_OUT32K_EN;
    93		c->mfd = mfd;
    94		c->pdev = pdev;
    95		c->hw.init = &init;
    96	
    97		of_property_read_string_index(parent->of_node,
    98					      "clock-output-names", 0, &init.name);
    99	
   100		rval = devm_clk_hw_register(&pdev->dev, &c->hw);
   101		if (!rval) {
   102			rval = devm_clk_hw_register_clkdev(&pdev->dev,
   103							   &c->hw, init.name, NULL);
   104			if (rval)
   105				dev_warn(&pdev->dev, "Failed to register clkdev\n");
   106			if (parent->of_node) {
   107				rval = devm_of_clk_add_parent_hw_provider(&pdev->dev,
   108						     of_clk_hw_simple_get, &c->hw);
   109				if (rval)
   110					dev_err(&pdev->dev,
   111						"adding clk provider failed\n");
   112			}
   113		} else {
   114			dev_err(&pdev->dev, "failed to register 32K clk");
   115		}
   116	
   117		return rval;
   118	}
   119	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
Stephen Boyd Sept. 1, 2018, 7:13 p.m. UTC | #4 
Quoting Matti Vaittinen (2018-08-31 03:21:23)
> Hello All,
> 
> Just wanted to point out for the reviewers that this patch depends on
> not yet accepted MFD/regulator patch. (struct/defines in
> include/linux/mfd/rohm-bd718x7.h were changed)
> https://lore.kernel.org/lkml/cover.1535545377.git.matti.vaittinen@fi.rohmeurope.com/
> 
> On Fri, Aug 31, 2018 at 09:18:19AM +0800, kbuild test robot wrote:
> > Hi Matti,
> > 
> > Thank you for the patch! Yet something to improve:
> > 
> > [auto build test ERROR on clk/clk-next]
> > [also build test ERROR on v4.19-rc1 next-20180830]
> > [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
> 
> Thus this error was expected. What is the generally best way to work
> when there is changes to more than one subsystem? With this approach
> the patch set here won't compile until MFD part gets applied. But if
> I use old definitions/structs here, then clk tree gets broken when
> MFD/regulator part changes defines. I see only bad and worse options =)
> Anyways, I guess sending this patch with new defines (and applying it
> only after MFD) is still better than applying this with old defines and
> breaking it when MFD changes. (Assuming all of the patches get applied
> at some point).
> 

Does anything besides the clk driver need the defines that are in the
header which are used in this file? If not, then it's better to put
those defines in the C file for the clk driver so we don't have to hop
between files and have merge dependencies. Also, the regmap could
possibly by grabbed from the dev->parent pointer instead of passing it
down through an mfd structure, allowing this driver to be more generic
assuming it is always a child of some mfd device that has a regmap.
Vaittinen, Matti Sept. 3, 2018, 6:38 a.m. UTC | #5 
Hello Stephen

On Sat, Sep 01, 2018 at 12:13:26PM -0700, Stephen Boyd wrote:
> Quoting Matti Vaittinen (2018-08-31 03:21:23)
> > Hello All,
> > 
> > Just wanted to point out for the reviewers that this patch depends on
> > not yet accepted MFD/regulator patch. (struct/defines in
> > include/linux/mfd/rohm-bd718x7.h were changed)
> > https://lore.kernel.org/lkml/cover.1535545377.git.matti.vaittinen@fi.rohmeurope.com/
> > 
> > On Fri, Aug 31, 2018 at 09:18:19AM +0800, kbuild test robot wrote:
> > > Hi Matti,
> > > 
> > > Thank you for the patch! Yet something to improve:
> > > 
> > > [auto build test ERROR on clk/clk-next]
> > > [also build test ERROR on v4.19-rc1 next-20180830]
> > > [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
> > 
> > Thus this error was expected. What is the generally best way to work
> > when there is changes to more than one subsystem? With this approach
> > the patch set here won't compile until MFD part gets applied. But if
> > I use old definitions/structs here, then clk tree gets broken when
> > MFD/regulator part changes defines. I see only bad and worse options =)
> > Anyways, I guess sending this patch with new defines (and applying it
> > only after MFD) is still better than applying this with old defines and
> > breaking it when MFD changes. (Assuming all of the patches get applied
> > at some point).
> > 
> 
> Does anything besides the clk driver need the defines that are in the
> header which are used in this file?

The register address for clk enable and bit mask are not used by any
other driver. But I'd like to keep all the register addresses in one
enum. That eases checking changes when new chips come.

The private data for MFD (which contains for example the regmap and chip
type) are required by other drivers too.

> If not, then it's better to put
> those defines in the C file for the clk driver so we don't have to hop
> between files and have merge dependencies. Also, the regmap could
> possibly by grabbed from the dev->parent pointer instead of passing it
> down through an mfd structure, allowing this driver to be more generic
> assuming it is always a child of some mfd device that has a regmap.

Currently clk only uses regmap directly from the struct bd718xx. But I
would like to have the access to chip_type as well. And even if we forget
the chip_type. Well, I only see two bad ways of omitting the struct
bd718xx:

1. Always keep the regmap as first member in parent device's driver_data.
Then we can just get the driver data and do cast to regmap. I am not fan
of this as it breaks as fast as someone changes the struct bd718xx - and
as this is part of MFD - well, there is no guarantees the clk people are
even reviewing such change. Also if we need the chip type we are back
using the struct.

2. Use some regmnap wrapper functions which would only take the parent
dev pointer and dig out the regmap details. This would be doable but
then we still have dependency to these wrappers (so dependency is not
solved). Additionally these wrappers are something we decided to get rid
of few patch versions ago.

I don't expect much renamings to take place after the latest patch set
to MFD/regulator tree has been applied. So maybe the clk patch set can
just wait untill MFD/regulator stuff gets integrated. I would be
gratefull if we could review the clk patch set already now though so I
could do improvements/changes to clk side while waiting for
MFD/regulator to get applied. Also, the devm_ functions (patch 1) are
not depending on MFD - so maybe we can get them reviewed/applied? If I
have the spare time I can look if I can clean up some of those not freed
clkdev lookups.

Br,
	Matti Vaittinen
>
Vaittinen, Matti Nov. 12, 2018, 7:45 a.m. UTC | #6 
Hello Stephen & All,

On Mon, Sep 03, 2018 at 09:38:43AM +0300, Matti Vaittinen wrote:
> 
> On Sat, Sep 01, 2018 at 12:13:26PM -0700, Stephen Boyd wrote:
> > Quoting Matti Vaittinen (2018-08-31 03:21:23)
> > > Hello All,
> > > 
> > > Just wanted to point out for the reviewers that this patch depends on
> > > not yet accepted MFD/regulator patch. (struct/defines in
> > > include/linux/mfd/rohm-bd718x7.h were changed)
> > > https://lore.kernel.org/lkml/cover.1535545377.git.matti.vaittinen@fi.rohmeurope.com/
> > 
> > Does anything besides the clk driver need the defines that are in the
> > header which are used in this file?
> 
> The register address for clk enable and bit mask are not used by any
> other driver. But I'd like to keep all the register addresses in one
> enum. That eases checking changes when new chips come.
> 
> The private data for MFD (which contains for example the regmap and chip
> type) are required by other drivers too.
> 
> > If not, then it's better to put
> > those defines in the C file for the clk driver so we don't have to hop
> > between files and have merge dependencies. Also, the regmap could
> > possibly by grabbed from the dev->parent pointer instead of passing it
> > down through an mfd structure, allowing this driver to be more generic
> > assuming it is always a child of some mfd device that has a regmap.
> 
> Currently clk only uses regmap directly from the struct bd718xx. But I
> would like to have the access to chip_type as well. And even if we forget
> the chip_type. Well, I only see two bad ways of omitting the struct
> bd718xx:
> 
> 1. Always keep the regmap as first member in parent device's driver_data.
> Then we can just get the driver data and do cast to regmap. I am not fan
> of this as it breaks as fast as someone changes the struct bd718xx - and
> as this is part of MFD - well, there is no guarantees the clk people are
> even reviewing such change. Also if we need the chip type we are back
> using the struct.
> 
> 2. Use some regmnap wrapper functions which would only take the parent
> dev pointer and dig out the regmap details. This would be doable but
> then we still have dependency to these wrappers (so dependency is not
> solved). Additionally these wrappers are something we decided to get rid
> of few patch versions ago.
> 
> I don't expect much renamings to take place after the latest patch set
> to MFD/regulator tree has been applied. So maybe the clk patch set can
> just wait untill MFD/regulator stuff gets integrated. I would be
> gratefull if we could review the clk patch set already now though so I
> could do improvements/changes to clk side while waiting for
> MFD/regulator to get applied. Also, the devm_ functions (patch 1) are
> not depending on MFD - so maybe we can get them reviewed/applied? If I
> have the spare time I can look if I can clean up some of those not freed
> clkdev lookups.
> 
The MFD renaming patch has now been merged to clk-next. So dependencies
to MFD should now be in-tree. I will rebase this to clk-next and do
resend. This patch still requires the devm variants for clkdev lookups
an devm variant of "parent of provider"-registration. I did submit those
as part of the cleanup series:
https://lore.kernel.org/linux-clk/cover.1541161503.git.matti.vaittinen@fi.rohmeurope.com/

My question is if you prefer me to add this driver in that series and
resend the series as v4? Or do you want me to send this in own
individual patch with just a note about the dependency?

Best Regards
	Matti Vaittinen
Stephen Boyd Nov. 13, 2018, 5:58 a.m. UTC | #7 
Quoting Matti Vaittinen (2018-11-11 23:45:53)
> Hello Stephen & All,
> 
> On Mon, Sep 03, 2018 at 09:38:43AM +0300, Matti Vaittinen wrote:
> > 
> > On Sat, Sep 01, 2018 at 12:13:26PM -0700, Stephen Boyd wrote:
> > > Quoting Matti Vaittinen (2018-08-31 03:21:23)
> > > > Hello All,
> > > > 
> > > > Just wanted to point out for the reviewers that this patch depends on
> > > > not yet accepted MFD/regulator patch. (struct/defines in
> > > > include/linux/mfd/rohm-bd718x7.h were changed)
> > > > https://lore.kernel.org/lkml/cover.1535545377.git.matti.vaittinen@fi.rohmeurope.com/
> > > 
> > > Does anything besides the clk driver need the defines that are in the
> > > header which are used in this file?
> > 
> > The register address for clk enable and bit mask are not used by any
> > other driver. But I'd like to keep all the register addresses in one
> > enum. That eases checking changes when new chips come.
> > 
> > The private data for MFD (which contains for example the regmap and chip
> > type) are required by other drivers too.
> > 
> > > If not, then it's better to put
> > > those defines in the C file for the clk driver so we don't have to hop
> > > between files and have merge dependencies. Also, the regmap could
> > > possibly by grabbed from the dev->parent pointer instead of passing it
> > > down through an mfd structure, allowing this driver to be more generic
> > > assuming it is always a child of some mfd device that has a regmap.
> > 
> > Currently clk only uses regmap directly from the struct bd718xx. But I
> > would like to have the access to chip_type as well. And even if we forget
> > the chip_type. Well, I only see two bad ways of omitting the struct
> > bd718xx:
> > 
> > 1. Always keep the regmap as first member in parent device's driver_data.
> > Then we can just get the driver data and do cast to regmap. I am not fan
> > of this as it breaks as fast as someone changes the struct bd718xx - and
> > as this is part of MFD - well, there is no guarantees the clk people are
> > even reviewing such change. Also if we need the chip type we are back
> > using the struct.
> > 
> > 2. Use some regmnap wrapper functions which would only take the parent
> > dev pointer and dig out the regmap details. This would be doable but
> > then we still have dependency to these wrappers (so dependency is not
> > solved). Additionally these wrappers are something we decided to get rid
> > of few patch versions ago.
> > 
> > I don't expect much renamings to take place after the latest patch set
> > to MFD/regulator tree has been applied. So maybe the clk patch set can
> > just wait untill MFD/regulator stuff gets integrated. I would be
> > gratefull if we could review the clk patch set already now though so I
> > could do improvements/changes to clk side while waiting for
> > MFD/regulator to get applied. Also, the devm_ functions (patch 1) are
> > not depending on MFD - so maybe we can get them reviewed/applied? If I
> > have the spare time I can look if I can clean up some of those not freed
> > clkdev lookups.
> > 
> The MFD renaming patch has now been merged to clk-next. So dependencies
> to MFD should now be in-tree. I will rebase this to clk-next and do
> resend. This patch still requires the devm variants for clkdev lookups
> an devm variant of "parent of provider"-registration. I did submit those
> as part of the cleanup series:
> https://lore.kernel.org/linux-clk/cover.1541161503.git.matti.vaittinen@fi.rohmeurope.com/
> 
> My question is if you prefer me to add this driver in that series and
> resend the series as v4? Or do you want me to send this in own
> individual patch with just a note about the dependency?
> 

Either way is fine. Just be explicit and we'll figure it out. Of course,
be aware that kbuild robot may complain if it can't figure out the
dependency and something fails to compile so it may be easier to resend
the whole combined series as something that compiles.
diff mbox series

Patch

diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index 292056bbb30e..b2aee8fa4445 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -283,6 +283,12 @@  config COMMON_CLK_STM32H7
 	---help---
 	  Support for stm32h7 SoC family clocks
 
+config COMMON_CLK_BD718XX
+	tristate "Clock driver for ROHM BD71837 PMIC MFD"
+	depends on MFD_ROHM_BD718XX
+	help
+	  This driver supports ROHM BD71837 PMIC clock.
+
 source "drivers/clk/actions/Kconfig"
 source "drivers/clk/bcm/Kconfig"
 source "drivers/clk/hisilicon/Kconfig"
diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index a84c5573cabe..886dce9f9bec 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -21,6 +21,7 @@  endif
 obj-$(CONFIG_MACH_ASM9260)		+= clk-asm9260.o
 obj-$(CONFIG_COMMON_CLK_AXI_CLKGEN)	+= clk-axi-clkgen.o
 obj-$(CONFIG_ARCH_AXXIA)		+= clk-axm5516.o
+obj-$(CONFIG_COMMON_CLK_BD718XX)	+= clk-bd718x7.o
 obj-$(CONFIG_COMMON_CLK_CDCE706)	+= clk-cdce706.o
 obj-$(CONFIG_COMMON_CLK_CDCE925)	+= clk-cdce925.o
 obj-$(CONFIG_ARCH_CLPS711X)		+= clk-clps711x.o
diff --git a/drivers/clk/clk-bd718x7.c b/drivers/clk/clk-bd718x7.c
new file mode 100644
index 000000000000..df5f1068ce8e
--- /dev/null
+++ b/drivers/clk/clk-bd718x7.c
@@ -0,0 +1,131 @@ 
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (C) 2018 ROHM Semiconductors
+// bd71837.c  -- ROHM BD71837MWV clock driver
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/err.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/mfd/rohm-bd718x7.h>
+#include <linux/clk-provider.h>
+#include <linux/clkdev.h>
+#include <linux/regmap.h>
+
+struct bd718xx_clk {
+	struct clk_hw hw;
+	u8 reg;
+	u8 mask;
+	struct platform_device *pdev;
+	struct bd718xx *mfd;
+};
+
+static int bd71837_clk_set(struct clk_hw *hw, 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);
+}
+
+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);
+	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);
+}
+
+static int bd71837_clk_is_enabled(struct clk_hw *hw)
+{
+	int enabled;
+	int rval;
+	struct bd718xx_clk *c = container_of(hw, struct bd718xx_clk, hw);
+
+	rval = regmap_read(c->mfd->regmap, c->reg, &enabled);
+
+	if (rval)
+		return rval;
+
+	return enabled & c->mask;
+}
+
+static const struct clk_ops bd71837_clk_ops = {
+	.prepare = &bd71837_clk_enable,
+	.unprepare = &bd71837_clk_disable,
+	.is_prepared = &bd71837_clk_is_enabled,
+};
+
+static int bd71837_clk_probe(struct platform_device *pdev)
+{
+	struct bd718xx_clk *c;
+	int rval = -ENOMEM;
+	const char *parent_clk;
+	struct device *parent = pdev->dev.parent;
+	struct bd718xx *mfd = dev_get_drvdata(parent);
+	struct clk_init_data init = {
+		.name = "bd718xx-32k-out",
+		.ops = &bd71837_clk_ops,
+	};
+
+	c = devm_kzalloc(&pdev->dev, sizeof(*c), GFP_KERNEL);
+	if (!c)
+		return -ENOMEM;
+
+	init.num_parents = 1;
+	parent_clk = of_clk_get_parent_name(parent->of_node, 0);
+
+	init.parent_names = &parent_clk;
+	if (!parent_clk) {
+		dev_err(&pdev->dev, "No parent clk found\n");
+		return -EINVAL;
+	}
+
+	c->reg = BD718XX_REG_OUT32K;
+	c->mask = BD718XX_OUT32K_EN;
+	c->mfd = mfd;
+	c->pdev = pdev;
+	c->hw.init = &init;
+
+	of_property_read_string_index(parent->of_node,
+				      "clock-output-names", 0, &init.name);
+
+	rval = devm_clk_hw_register(&pdev->dev, &c->hw);
+	if (!rval) {
+		rval = devm_clk_hw_register_clkdev(&pdev->dev,
+						   &c->hw, init.name, NULL);
+		if (rval)
+			dev_warn(&pdev->dev, "Failed to register clkdev\n");
+		if (parent->of_node) {
+			rval = devm_of_clk_add_parent_hw_provider(&pdev->dev,
+					     of_clk_hw_simple_get, &c->hw);
+			if (rval)
+				dev_err(&pdev->dev,
+					"adding clk provider failed\n");
+		}
+	} else {
+		dev_err(&pdev->dev, "failed to register 32K clk");
+	}
+
+	return rval;
+}
+
+static struct platform_driver bd71837_clk = {
+	.driver = {
+		.name = "bd718xx-clk",
+	},
+	.probe = bd71837_clk_probe,
+};
+
+module_platform_driver(bd71837_clk);
+
+MODULE_AUTHOR("Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>");
+MODULE_DESCRIPTION("BD71837 chip clk driver");
+MODULE_LICENSE("GPL");