| Message ID | d99c8762b0fbbcb18ec4f4d104191364c0ea798c.1528117485.git.matti.vaittinen@fi.rohmeurope.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
Quoting Matti Vaittinen (2018-06-04 06:19:13) > diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig > index 41492e980ef4..e693496f202a 100644 > --- a/drivers/clk/Kconfig > +++ b/drivers/clk/Kconfig > @@ -279,6 +279,13 @@ config COMMON_CLK_STM32H7 > ---help--- > Support for stm32h7 SoC family clocks > > +config COMMON_CLK_BD71837 > + tristate "Clock driver for ROHM BD71837 PMIC MFD" > + depends on MFD_BD71837 > + help > + This driver supports ROHM BD71837 PMIC clock. > + > + Drop one newline above. > source "drivers/clk/bcm/Kconfig" > source "drivers/clk/hisilicon/Kconfig" > source "drivers/clk/imgtec/Kconfig" > diff --git a/drivers/clk/clk-bd71837.c b/drivers/clk/clk-bd71837.c > new file mode 100644 > index 000000000000..5ba6c05c5a98 > --- /dev/null > +++ b/drivers/clk/clk-bd71837.c > @@ -0,0 +1,146 @@ > +// 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/bd71837.h> > +#include <linux/clk-provider.h> > +#include <linux/clkdev.h> > + > + Drop one newline above. > +struct bd71837_clk { > + struct clk_hw hw; > + uint8_t reg; > + uint8_t mask; Use u8 instead of uint8_t. > + unsigned long rate; > + struct platform_device *pdev; > + struct bd71837 *mfd; > +}; > + > +static int bd71837_clk_set(struct clk_hw *hw, int status) > +{ > + struct bd71837_clk *c = container_of(hw, struct bd71837_clk, hw); > + > + return bd71837_update_bits(c->mfd, c->reg, c->mask, status); Any reason we can't use a regmap? > +} > + > +static void bd71837_clk_disable(struct clk_hw *hw) > +{ > + int rv; > + struct bd71837_clk *c = container_of(hw, struct bd71837_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) > +{ > + struct bd71837_clk *c = container_of(hw, struct bd71837_clk, hw); > + > + return c->mask & bd71837_reg_read(c->mfd, c->reg); Didn't I ask for local variable for reg_read result? > +} > + > +static unsigned long bd71837_clk_recalc_rate(struct clk_hw *hw, > + unsigned long parent_rate) > +{ > + struct bd71837_clk *c = container_of(hw, struct bd71837_clk, hw); > + > + return c->rate; > +} > + > +static const struct clk_ops bd71837_clk_ops = { > + .recalc_rate = &bd71837_clk_recalc_rate, > + .prepare = &bd71837_clk_enable, > + .unprepare = &bd71837_clk_disable, > + .is_prepared = &bd71837_clk_is_enabled, > +}; > + > +static int bd71837_clk_probe(struct platform_device *pdev) > +{ > + struct bd71837_clk *c; > + int rval = -ENOMEM; > + struct bd71837 *mfd = dev_get_drvdata(pdev->dev.parent); > + struct clk_init_data init = { > + .name = "bd71837-32k-out", > + .ops = &bd71837_clk_ops, > + }; > + > + c = devm_kzalloc(&pdev->dev, sizeof(*c), GFP_KERNEL); > + if (!c) > + goto err_out; > + > + c->reg = BD71837_REG_OUT32K; > + c->mask = BD71837_OUT32K_EN; > + c->rate = BD71837_CLK_RATE; The PMIC has an 'XIN' pin that would be the clk input for this chip, and the output clk, this driver, would specify that xin clock as the parent. The 'xin' clock would then be listed in DT as a fixed-rate clock. That way this driver doesn't hardcode the frequency. > + c->mfd = mfd; > + c->pdev = pdev; > + > + of_property_read_string_index(pdev->dev.parent->of_node, > + "clock-output-names", 0, > + &init.name); > + > + c->hw.init = &init; Do this next to all the other c-> things? > + > + rval = devm_clk_hw_register(&pdev->dev, &c->hw); > + if (rval) { > + dev_err(&pdev->dev, "failed to register 32K clk"); > + goto err_out; > + } > + > + if (pdev->dev.parent->of_node) { > + rval = of_clk_add_hw_provider(pdev->dev.parent->of_node, > + of_clk_hw_simple_get, > + &c->hw); > + if (rval) { > + dev_err(&pdev->dev, "adding clk provider failed\n"); > + goto err_out; Just return rval instead of goto and then remove err_out label. > + } > + } > + > + rval = clk_hw_register_clkdev(&c->hw, init.name, NULL); This leaks a clkdev lookup. Sad we don't have a devm version. Maybe it should be added. But I really doubt this chip will be used with clkdev lookups so please remove clkdev until you have a user who needs it. > + if (rval) { > + dev_err(&pdev->dev, "failed to register clkdev for bd71837"); > + goto err_clean_provider; > + } > + > + platform_set_drvdata(pdev, c); > + > + return 0; > + > +err_clean_provider: > + of_clk_del_provider(pdev->dev.parent->of_node); > +err_out: > + return rval; > +} > + > +static int bd71837_clk_remove(struct platform_device *pdev) > +{ > + if (pdev->dev.parent->of_node) > + of_clk_del_provider(pdev->dev.parent->of_node); Use devm so this can go away. Or is devm not used because the parent of_node is the provider? That's annoying. > + return 0; > +} > + -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello Stephen, Thanks again for the review. I'll do new patch which fixes these issues. On Tue, Jun 12, 2018 at 12:44:11AM -0700, Stephen Boyd wrote: > Quoting Matti Vaittinen (2018-06-04 06:19:13) > > +config COMMON_CLK_BD71837 > > + tristate "Clock driver for ROHM BD71837 PMIC MFD" > > + depends on MFD_BD71837 > > + help > > + This driver supports ROHM BD71837 PMIC clock. > > + > > + > > Drop one newline above. Right. > > > +++ b/drivers/clk/clk-bd71837.c > > +#include <linux/mfd/bd71837.h> > > +#include <linux/clk-provider.h> > > +#include <linux/clkdev.h> > > + > > + > > Drop one newline above. Right. > > > +struct bd71837_clk { > > + struct clk_hw hw; > > + uint8_t reg; > > + uint8_t mask; > > Use u8 instead of uint8_t. I can do that but I am afraid I have missed the reason for this. Why u8 is preferred? I would have assumed the standard uint8_t would be preferred - can you please educate me as to what is this reason? > > + unsigned long rate; > > + struct platform_device *pdev; > > + struct bd71837 *mfd; > > +}; > > + > > +static int bd71837_clk_set(struct clk_hw *hw, int status) > > +{ > > + struct bd71837_clk *c = container_of(hw, struct bd71837_clk, hw); > > + > > + return bd71837_update_bits(c->mfd, c->reg, c->mask, status); > > Any reason we can't use a regmap? Not really. These wrappers might become handy if the next chip in series requires some quirks for access but at the moment I see no reason for the wrappers. I could switch to direct regmap calls but that change should then be done to all subdevices and not just for clk one. This means I should rework also the already applied regulator part. I have some other changes pending for regulators (adding support for next chip, renaming bd71837 to bd718x7 or bd718xx, and few comments from Andy Shevchenko and Rob Herring). I would rather switch to direct regmap usage for all subdevices at the same time. So if it is acceptable to keep the wrappers for now (and then create own patch set to remove them from all subdevices and the header) I would like to do so. > > > +} > > + > > +static void bd71837_clk_disable(struct clk_hw *hw) > > +{ > > + int rv; > > + struct bd71837_clk *c = container_of(hw, struct bd71837_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) > > +{ > > + struct bd71837_clk *c = container_of(hw, struct bd71837_clk, hw); > > + > > + return c->mask & bd71837_reg_read(c->mfd, c->reg); > > Didn't I ask for local variable for reg_read result? Yes! Sorry! And thanks for pointing this out again. It seems I missed this one. > > > +} > > + > > +static unsigned long bd71837_clk_recalc_rate(struct clk_hw *hw, > > + unsigned long parent_rate) > > +{ > > + struct bd71837_clk *c = container_of(hw, struct bd71837_clk, hw); > > + > > + return c->rate; > > +} > > + > > +static const struct clk_ops bd71837_clk_ops = { > > + .recalc_rate = &bd71837_clk_recalc_rate, > > + .prepare = &bd71837_clk_enable, > > + .unprepare = &bd71837_clk_disable, > > + .is_prepared = &bd71837_clk_is_enabled, > > +}; > > + > > +static int bd71837_clk_probe(struct platform_device *pdev) > > +{ > > + struct bd71837_clk *c; > > + int rval = -ENOMEM; > > + struct bd71837 *mfd = dev_get_drvdata(pdev->dev.parent); > > + struct clk_init_data init = { > > + .name = "bd71837-32k-out", > > + .ops = &bd71837_clk_ops, > > + }; > > + > > + c = devm_kzalloc(&pdev->dev, sizeof(*c), GFP_KERNEL); > > + if (!c) > > + goto err_out; > > + > > + c->reg = BD71837_REG_OUT32K; > > + c->mask = BD71837_OUT32K_EN; > > + c->rate = BD71837_CLK_RATE; > > The PMIC has an 'XIN' pin that would be the clk input for this chip, and > the output clk, this driver, would specify that xin clock as the parent. > The 'xin' clock would then be listed in DT as a fixed-rate clock. That > way this driver doesn't hardcode the frequency. I see. This makes sense. I need to verify from HW colleagues whether this chip has internal oscillator or not. I originally thought we have on-chip oscillator - but as you say, we do have XIN pin in documentation. So now I am not sure if the test board I have contains oscillator driving the clk on PMIC - or if the PMIC has internal oscillator. I'll clarify this. > > + c->mfd = mfd; > > + c->pdev = pdev; > > + > > + of_property_read_string_index(pdev->dev.parent->of_node, > > + "clock-output-names", 0, > > + &init.name); > > + > > + c->hw.init = &init; > > Do this next to all the other c-> things? I can do so. I just like the idea of assigning pointers to objects only after the objects have been initialized. Eg, in this case I add pointer to init only after I have filled the init.name (if it is given). > > > + > > + rval = devm_clk_hw_register(&pdev->dev, &c->hw); > > + if (rval) { > > + dev_err(&pdev->dev, "failed to register 32K clk"); > > + goto err_out; > > + } > > + > > + if (pdev->dev.parent->of_node) { > > + rval = of_clk_add_hw_provider(pdev->dev.parent->of_node, > > + of_clk_hw_simple_get, > > + &c->hw); > > + if (rval) { > > + dev_err(&pdev->dev, "adding clk provider failed\n"); > > + goto err_out; > > Just return rval instead of goto and then remove err_out label. Is this 'hard requirement'? I prefer single point of exit from functions (when easily doable) because I have done so many errors with locks / resources being reserved when exiting from some error branch. For my personal taste maintaining the one nice cleanup sequence is easier. But if this is 'hard requirement' this can of course be changed. > > > + } > > + } > > + > > + rval = clk_hw_register_clkdev(&c->hw, init.name, NULL); > > This leaks a clkdev lookup. Sad we don't have a devm version. Maybe it > should be added. But I really doubt this chip will be used with clkdev > lookups so please remove clkdev until you have a user who needs it. Yep. It leaks. I tried to look for an example where the clkdev was nicely freed at remove - but I didn't find any. Every example I spotted did leak the clkdev in same way as this does :( And I agree, devm variant for freeing would be nice - or freeing routines based on hw. As the leaked clkdev can cause oops at module unload/reload/use your suggestion about removing it for now makes sense. I'll drop it. > > > + if (rval) { > > + dev_err(&pdev->dev, "failed to register clkdev for bd71837"); > > + goto err_clean_provider; > > + } > > + > > + platform_set_drvdata(pdev, c); > > + > > + return 0; > > + > > +err_clean_provider: > > + of_clk_del_provider(pdev->dev.parent->of_node); > > +err_out: > > + return rval; > > +} > > + > > +static int bd71837_clk_remove(struct platform_device *pdev) > > +{ > > + if (pdev->dev.parent->of_node) > > + of_clk_del_provider(pdev->dev.parent->of_node); > > Use devm so this can go away. Or is devm not used because the parent > of_node is the provider? That's annoying. What would be the correct workaround for this? > > > + return 0; > > +} > > + Br, Matti Vaittinen -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jun 12, 2018 at 11:23:54AM +0300, Matti Vaittinen wrote: > Hello Stephen, > > Thanks again for the review. I'll do new patch which fixes these issues. > > On Tue, Jun 12, 2018 at 12:44:11AM -0700, Stephen Boyd wrote: > > Quoting Matti Vaittinen (2018-06-04 06:19:13) > > > +} > > > + > > > +static unsigned long bd71837_clk_recalc_rate(struct clk_hw *hw, > > > + unsigned long parent_rate) > > > +{ > > > + struct bd71837_clk *c = container_of(hw, struct bd71837_clk, hw); > > > + > > > + return c->rate; > > > +} > > > + > > > +static const struct clk_ops bd71837_clk_ops = { > > > + .recalc_rate = &bd71837_clk_recalc_rate, > > > + .prepare = &bd71837_clk_enable, > > > + .unprepare = &bd71837_clk_disable, > > > + .is_prepared = &bd71837_clk_is_enabled, > > > +}; > > > + > > > +static int bd71837_clk_probe(struct platform_device *pdev) > > > +{ > > > + struct bd71837_clk *c; > > > + int rval = -ENOMEM; > > > + struct bd71837 *mfd = dev_get_drvdata(pdev->dev.parent); > > > + struct clk_init_data init = { > > > + .name = "bd71837-32k-out", > > > + .ops = &bd71837_clk_ops, > > > + }; > > > + > > > + c = devm_kzalloc(&pdev->dev, sizeof(*c), GFP_KERNEL); > > > + if (!c) > > > + goto err_out; > > > + > > > + c->reg = BD71837_REG_OUT32K; > > > + c->mask = BD71837_OUT32K_EN; > > > + c->rate = BD71837_CLK_RATE; > > > > The PMIC has an 'XIN' pin that would be the clk input for this chip, and > > the output clk, this driver, would specify that xin clock as the parent. > > The 'xin' clock would then be listed in DT as a fixed-rate clock. That > > way this driver doesn't hardcode the frequency. > > I see. This makes sense. I need to verify from HW colleagues whether > this chip has internal oscillator or not. I originally thought we have > on-chip oscillator - but as you say, we do have XIN pin in documentation. > So now I am not sure if the test board I have contains oscillator driving > the clk on PMIC - or if the PMIC has internal oscillator. I'll clarify this. It really turned out that the PMIC just acts as a clock buffer. So I do as you suggested and add lookup for parent clock to the driver. I planned to do it so that if no parent is found from DT - then we assume the 32.768KHz clock (as described in documentation). Eg, something along the lines: init.parent_names = of_clk_get_parent_name(pdev->dev.parent->of_node, 0); if (init.parent_names) { init.num_parents = 1; } else { /* If parent is not given from DT we assume the typical use-case with * 32.768 KHz oscillator for RTC (Maybe we could just error out here?) */ c->rate = BD71837_CLK_RATE; bd71837_clk_ops.recalc_rate = &bd71837_clk_recalc_rate; } Does this make sense? Br, Matti Vaittinen -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoting Matti Vaittinen (2018-06-12 01:23:54) > On Tue, Jun 12, 2018 at 12:44:11AM -0700, Stephen Boyd wrote: > > Quoting Matti Vaittinen (2018-06-04 06:19:13) > > > > > > +struct bd71837_clk { > > > + struct clk_hw hw; > > > + uint8_t reg; > > > + uint8_t mask; > > > > Use u8 instead of uint8_t. > I can do that but I am afraid I have missed the reason for this. Why u8 > is preferred? I would have assumed the standard uint8_t would be > preferred - can you please educate me as to what is this reason? This is kernel style to prefer the shorter types within the kernel code. Outside of the kernel proper, uint*_t types are used in the UAPI headers. You can look through the mailing list about this, but this is how I've known it to be for a while now. > > > > + unsigned long rate; > > > + struct platform_device *pdev; > > > + struct bd71837 *mfd; > > > +}; > > > + > > > +static int bd71837_clk_set(struct clk_hw *hw, int status) > > > +{ > > > + struct bd71837_clk *c = container_of(hw, struct bd71837_clk, hw); > > > + > > > + return bd71837_update_bits(c->mfd, c->reg, c->mask, status); > > > > Any reason we can't use a regmap? > > Not really. These wrappers might become handy if the next chip in series > requires some quirks for access but at the moment I see no reason for > the wrappers. I could switch to direct regmap calls but that change > should then be done to all subdevices and not just for clk one. This > means I should rework also the already applied regulator part. I have > some other changes pending for regulators (adding support for next chip, > renaming bd71837 to bd718x7 or bd718xx, and few comments from Andy > Shevchenko and Rob Herring). I would rather switch to direct regmap usage > for all subdevices at the same time. So if it is acceptable to keep the > wrappers for now (and then create own patch set to remove them from all > subdevices and the header) I would like to do so. Ok. Sounds fine. > > > > > > +} > > > + > > > +static void bd71837_clk_disable(struct clk_hw *hw) > > > +{ > > > + int rv; > > > + struct bd71837_clk *c = container_of(hw, struct bd71837_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) > > > +{ > > > + struct bd71837_clk *c = container_of(hw, struct bd71837_clk, hw); > > > + > > > + return c->mask & bd71837_reg_read(c->mfd, c->reg); > > > > Didn't I ask for local variable for reg_read result? > > Yes! Sorry! And thanks for pointing this out again. It seems I missed > this one. Ok no worries. > > > > > > +} > > > + > > > +static unsigned long bd71837_clk_recalc_rate(struct clk_hw *hw, > > > + unsigned long parent_rate) > > > +{ > > > + struct bd71837_clk *c = container_of(hw, struct bd71837_clk, hw); > > > + > > > + return c->rate; > > > +} > > > + > > > +static const struct clk_ops bd71837_clk_ops = { > > > + .recalc_rate = &bd71837_clk_recalc_rate, > > > + .prepare = &bd71837_clk_enable, > > > + .unprepare = &bd71837_clk_disable, > > > + .is_prepared = &bd71837_clk_is_enabled, > > > +}; > > > + > > > +static int bd71837_clk_probe(struct platform_device *pdev) > > > +{ > > > + struct bd71837_clk *c; > > > + int rval = -ENOMEM; > > > + struct bd71837 *mfd = dev_get_drvdata(pdev->dev.parent); > > > + struct clk_init_data init = { > > > + .name = "bd71837-32k-out", > > > + .ops = &bd71837_clk_ops, > > > + }; > > > + > > > + c = devm_kzalloc(&pdev->dev, sizeof(*c), GFP_KERNEL); > > > + if (!c) > > > + goto err_out; > > > + > > > + c->reg = BD71837_REG_OUT32K; > > > + c->mask = BD71837_OUT32K_EN; > > > + c->rate = BD71837_CLK_RATE; > > > > The PMIC has an 'XIN' pin that would be the clk input for this chip, and > > the output clk, this driver, would specify that xin clock as the parent. > > The 'xin' clock would then be listed in DT as a fixed-rate clock. That > > way this driver doesn't hardcode the frequency. > > I see. This makes sense. I need to verify from HW colleagues whether > this chip has internal oscillator or not. I originally thought we have > on-chip oscillator - but as you say, we do have XIN pin in documentation. > So now I am not sure if the test board I have contains oscillator driving > the clk on PMIC - or if the PMIC has internal oscillator. I'll clarify this. Alright. > > > > + c->mfd = mfd; > > > + c->pdev = pdev; > > > + > > > + of_property_read_string_index(pdev->dev.parent->of_node, > > > + "clock-output-names", 0, > > > + &init.name); > > > + > > > + c->hw.init = &init; > > > > Do this next to all the other c-> things? > > I can do so. I just like the idea of assigning pointers to objects only > after the objects have been initialized. Eg, in this case I add pointer > to init only after I have filled the init.name (if it is given). Alright, sure. > > > > > > + > > > + rval = devm_clk_hw_register(&pdev->dev, &c->hw); > > > + if (rval) { > > > + dev_err(&pdev->dev, "failed to register 32K clk"); > > > + goto err_out; > > > + } > > > + > > > + if (pdev->dev.parent->of_node) { > > > + rval = of_clk_add_hw_provider(pdev->dev.parent->of_node, > > > + of_clk_hw_simple_get, > > > + &c->hw); > > > + if (rval) { > > > + dev_err(&pdev->dev, "adding clk provider failed\n"); > > > + goto err_out; > > > > Just return rval instead of goto and then remove err_out label. > > Is this 'hard requirement'? I prefer single point of exit from functions > (when easily doable) because I have done so many errors with locks / > resources being reserved when exiting from some error branch. For my > personal taste maintaining the one nice cleanup sequence is easier. But > if this is 'hard requirement' this can of course be changed. There are no locks though. And this uses devm. So please remove the goto so my style alarms don't go off. Thanks! > > > > > > + } > > > + } > > > + > > > + rval = clk_hw_register_clkdev(&c->hw, init.name, NULL); > > > > This leaks a clkdev lookup. Sad we don't have a devm version. Maybe it > > should be added. But I really doubt this chip will be used with clkdev > > lookups so please remove clkdev until you have a user who needs it. > > Yep. It leaks. I tried to look for an example where the clkdev was > nicely freed at remove - but I didn't find any. Every example I spotted > did leak the clkdev in same way as this does :( And I agree, devm > variant for freeing would be nice - or freeing routines based on hw. > > As the leaked clkdev can cause oops at module unload/reload/use your > suggestion about removing it for now makes sense. I'll drop it. Ok! Sometimes drivers don't free them because they're lazy and don't expect to be unloaded or to fail. I guess you ran into those. I'm happy to apply patches to clean those up. > > > > > > + if (rval) { > > > + dev_err(&pdev->dev, "failed to register clkdev for bd71837"); > > > + goto err_clean_provider; > > > + } > > > + > > > + platform_set_drvdata(pdev, c); > > > + > > > + return 0; > > > + > > > +err_clean_provider: > > > + of_clk_del_provider(pdev->dev.parent->of_node); > > > +err_out: > > > + return rval; > > > +} > > > + > > > +static int bd71837_clk_remove(struct platform_device *pdev) > > > +{ > > > + if (pdev->dev.parent->of_node) > > > + of_clk_del_provider(pdev->dev.parent->of_node); > > > > Use devm so this can go away. Or is devm not used because the parent > > of_node is the provider? That's annoying. > > What would be the correct workaround for this? Smash the clk driver into the overall PMIC node. That should work. Or possibly assign the same of_node to the child device when creating it? I'm not sure if that causes some sort of problem with DT code though, so it would be good to check with Rob H if that's a bad idea or not. -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoting Matti Vaittinen (2018-06-13 06:03:38) > On Tue, Jun 12, 2018 at 11:23:54AM +0300, Matti Vaittinen wrote: > > > > I see. This makes sense. I need to verify from HW colleagues whether > > this chip has internal oscillator or not. I originally thought we have > > on-chip oscillator - but as you say, we do have XIN pin in documentation. > > So now I am not sure if the test board I have contains oscillator driving > > the clk on PMIC - or if the PMIC has internal oscillator. I'll clarify this. > > It really turned out that the PMIC just acts as a clock buffer. So I do > as you suggested and add lookup for parent clock to the driver. I > planned to do it so that if no parent is found from DT - then we assume > the 32.768KHz clock (as described in documentation). Eg, something along > the lines: > > init.parent_names = of_clk_get_parent_name(pdev->dev.parent->of_node, 0); > if (init.parent_names) { > init.num_parents = 1; > } else { > /* If parent is not given from DT we assume the typical use-case with > * 32.768 KHz oscillator for RTC (Maybe we could just error out here?) > */ > c->rate = BD71837_CLK_RATE; > bd71837_clk_ops.recalc_rate = &bd71837_clk_recalc_rate; > } You can also add a clk directly in this driver in that case there isn't one in DT with the rate and name of your choosing. Then the logic is the same and we don't need a c->rate variable. -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello Stephen, Rob and all others Thanks again Stephen. I appreciate your help. And just to help you sorting your inbox a bit - I have written patch series v6 and v7 before receiving these comments. I will write v8 after I get some further input from you and Rob on how to solve the issue with having the clk stuff in parent node and usage of devm_of_clk_add_hw_provider where DT node is fetched from dev pointer. The v8 should then address rest of the issues - except the regmap wrappers which I will send as separate patch. So I guess you can skip the v6 and v7 and just please check for the v8 when I get it done. On Mon, Jun 25, 2018 at 04:44:57PM -0700, Stephen Boyd wrote: > Quoting Matti Vaittinen (2018-06-12 01:23:54) > > On Tue, Jun 12, 2018 at 12:44:11AM -0700, Stephen Boyd wrote: > > > Quoting Matti Vaittinen (2018-06-04 06:19:13) > > > Use u8 instead of uint8_t. > > can you please educate me as to what is this reason? > > This is kernel style to prefer the shorter types within the kernel code. > Outside of the kernel proper, uint*_t types are used in the UAPI > headers. You can look through the mailing list about this, but this is > how I've known it to be for a while now. Thanks. I have changed this from patch v6 onwards. > > > > + > > > > + rval = devm_clk_hw_register(&pdev->dev, &c->hw); > > > > + if (rval) { > > > > + dev_err(&pdev->dev, "failed to register 32K clk"); > > > > + goto err_out; > > > > + } > > > > + > > > > + if (pdev->dev.parent->of_node) { > > > > + rval = of_clk_add_hw_provider(pdev->dev.parent->of_node, > > > > + of_clk_hw_simple_get, > > > > + &c->hw); > > > > + if (rval) { > > > > + dev_err(&pdev->dev, "adding clk provider failed\n"); > > > > + goto err_out; > > > > > > Just return rval instead of goto and then remove err_out label. > > > > Is this 'hard requirement'? I prefer single point of exit from functions > > (when easily doable) because I have done so many errors with locks / > > resources being reserved when exiting from some error branch. For my > > personal taste maintaining the one nice cleanup sequence is easier. But > > if this is 'hard requirement' this can of course be changed. > > There are no locks though. And this uses devm. So please remove the goto > so my style alarms don't go off. Thanks! I did rework this piece from patch v6 onwards so that there is no goto but we still have single exit point. I hope that is Ok, so please re-check this when you find the time. > > > > + rval = clk_hw_register_clkdev(&c->hw, init.name, NULL); > > > > > > This leaks a clkdev lookup. Sad we don't have a devm version. Maybe it > > > should be added. But I really doubt this chip will be used with clkdev > > > lookups so please remove clkdev until you have a user who needs it. > > > > Yep. It leaks. I tried to look for an example where the clkdev was > > nicely freed at remove - but I didn't find any. Every example I spotted > > did leak the clkdev in same way as this does :( And I agree, devm > > variant for freeing would be nice - or freeing routines based on hw. > > > > As the leaked clkdev can cause oops at module unload/reload/use your > > suggestion about removing it for now makes sense. I'll drop it. > > Ok! Sometimes drivers don't free them because they're lazy and don't > expect to be unloaded or to fail. I guess you ran into those. I'm happy > to apply patches to clean those up. When I get the moment of "hmm, what should I do next" - I'll keep this in mind... Unfortunately those moments have been pretty rare occasions since I found my wife and got my kids - but OTOH, we have working pension scheme in Finland - so I expect this to change during the next 30 years or so =] (Truth is that I'll keep this in mind but can't promise anything more as promises might be empty). > > > > + if (rval) { > > > > + dev_err(&pdev->dev, "failed to register clkdev for bd71837"); > > > > + goto err_clean_provider; > > > > + } > > > > + > > > > + platform_set_drvdata(pdev, c); > > > > + > > > > + return 0; > > > > + > > > > +err_clean_provider: > > > > + of_clk_del_provider(pdev->dev.parent->of_node); > > > > +err_out: > > > > + return rval; > > > > +} > > > > + > > > > +static int bd71837_clk_remove(struct platform_device *pdev) > > > > +{ > > > > + if (pdev->dev.parent->of_node) > > > > + of_clk_del_provider(pdev->dev.parent->of_node); > > > > > > Use devm so this can go away. Or is devm not used because the parent > > > of_node is the provider? That's annoying. > > > > What would be the correct workaround for this? > > Smash the clk driver into the overall PMIC node. That should work. Or > possibly assign the same of_node to the child device when creating it? > I'm not sure if that causes some sort of problem with DT code though, so > it would be good to check with Rob H if that's a bad idea or not. I'd rather keep the clk in own subdevice so that it can be used or not used based on the clk Kconfig options. I also rather keep the clk codes in clk folders. So I guess the use of devm is not correct justification for bundling the MFD and clk. I basically see 3 ways: 1. Assign MFD node to subdevice node in MFD when creating the cells. 2. Assign parent->of_node to dev.of_node in clk subdevice. 3. Create devm_of_clk_add_hw_provider_w_node() which does something like (not compiled pseudo) code below int devm_of_clk_add_hw_provider_w_node(struct device *dev, struct clk_hw *(*get)(struct of_phandle_args *clkspec, void *data), struct device_node *of_node, void *data) { struct device_node **ptr; int ret; ptr = devres_alloc(devm_of_clk_release_provider, sizeof(*ptr), GFP_KERNEL); if (!ptr) return -ENOMEM; *ptr = of_node; ret = of_clk_add_hw_provider(of_node, get, data); if (!ret) devres_add(dev, ptr); else devres_free(ptr); return ret; } EXPORT_SYMBOL_GPL(devm_of_clk_add_hw_provider_w_node); int devm_of_clk_add_hw_provider(struct device *dev, struct clk_hw *(*get)(struct of_phandle_args *clkspec, void *data), void *data) { return devm_of_clk_add_hw_provider_w_node(dev, get, dev->of_node, data); } EXPORT_SYMBOL_GPL(devm_of_clk_add_hw_provider); To me the third option sounds like correct one - but you guys probably have the idea how these subsystems should look like in the future - so I trust on your judgement on this. So whatäs your take on this? I'll also add Rob in the 'to' field of this email so maybe we get his opinion on this. Best Regards Matti Vaittinen -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello Stephen, On Mon, Jun 25, 2018 at 04:46:24PM -0700, Stephen Boyd wrote: > Quoting Matti Vaittinen (2018-06-13 06:03:38) > > On Tue, Jun 12, 2018 at 11:23:54AM +0300, Matti Vaittinen wrote: > > > > > > I see. This makes sense. I need to verify from HW colleagues whether > > > this chip has internal oscillator or not. I originally thought we have > > > on-chip oscillator - but as you say, we do have XIN pin in documentation. > > > So now I am not sure if the test board I have contains oscillator driving > > > the clk on PMIC - or if the PMIC has internal oscillator. I'll clarify this. > > > > It really turned out that the PMIC just acts as a clock buffer. So I do > > as you suggested and add lookup for parent clock to the driver. I > > planned to do it so that if no parent is found from DT - then we assume > > the 32.768KHz clock (as described in documentation). Eg, something along > > the lines: > > > > init.parent_names = of_clk_get_parent_name(pdev->dev.parent->of_node, 0); > > if (init.parent_names) { > > init.num_parents = 1; > > } else { > > /* If parent is not given from DT we assume the typical use-case with > > * 32.768 KHz oscillator for RTC (Maybe we could just error out here?) > > */ > > c->rate = BD71837_CLK_RATE; > > bd71837_clk_ops.recalc_rate = &bd71837_clk_recalc_rate; > > } > > You can also add a clk directly in this driver in that case there isn't > one in DT with the rate and name of your choosing. Then the logic is the > same and we don't need a c->rate variable. So you mean that I should use clk_hw_register_fixed_rate and create new clk if parent is not found? Isn't this a bit of an overkill? Downside is that then we do need remove/cleanup functionality for deleting this parent clock - and I didn't find devm support for fixed clock. Furthermore I guess that since it is parent, it can't be removed before child is removed. Or did you mean something else but creating a fixed rate clock as parent here? Best Regards Matti Vaittinen -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jun 26, 2018 at 11:13:19AM +0300, Matti Vaittinen wrote: > Hello Stephen, Rob and all others > > Thanks again Stephen. I appreciate your help. And just to help you > sorting your inbox a bit - I have written patch series v6 and v7 before > receiving these comments. I will write v8 after I get some further input > from you and Rob on how to solve the issue with having the clk stuff in > parent node and usage of devm_of_clk_add_hw_provider where DT node is > fetched from dev pointer. Just for your information - I did send v8 without clk part as I was unsure how to proceed with it. I still wanted to get the MFD fixes sent before I start my vacations and family time. I will wait for input from you / Rob and continue the clk portion latest after 4 weeks. Sorry for long delay =( Have a nice Summer -- Matti Vaittinen -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jun 26, 2018 at 11:13:19AM +0300, Matti Vaittinen wrote: > On Mon, Jun 25, 2018 at 04:44:57PM -0700, Stephen Boyd wrote: > > Quoting Matti Vaittinen (2018-06-12 01:23:54) > > > On Tue, Jun 12, 2018 at 12:44:11AM -0700, Stephen Boyd wrote: > > > > Quoting Matti Vaittinen (2018-06-04 06:19:13) [snip] > > > > > + if (rval) { > > > > > + dev_err(&pdev->dev, "failed to register clkdev for bd71837"); > > > > > + goto err_clean_provider; > > > > > + } > > > > > + > > > > > + platform_set_drvdata(pdev, c); > > > > > + > > > > > + return 0; > > > > > + > > > > > +err_clean_provider: > > > > > + of_clk_del_provider(pdev->dev.parent->of_node); > > > > > +err_out: > > > > > + return rval; > > > > > +} > > > > > + > > > > > +static int bd71837_clk_remove(struct platform_device *pdev) > > > > > +{ > > > > > + if (pdev->dev.parent->of_node) > > > > > + of_clk_del_provider(pdev->dev.parent->of_node); > > > > > > > > Use devm so this can go away. Or is devm not used because the parent > > > > of_node is the provider? That's annoying. > > > > > > What would be the correct workaround for this? > > > > Smash the clk driver into the overall PMIC node. That should work. Or > > possibly assign the same of_node to the child device when creating it? > > I'm not sure if that causes some sort of problem with DT code though, so > > it would be good to check with Rob H if that's a bad idea or not. > > 1. Assign MFD node to subdevice node in MFD when creating the cells. > 2. Assign parent->of_node to dev.of_node in clk subdevice. > 3. Create devm_of_clk_add_hw_provider_w_node() which does something > like (not compiled pseudo) code below > > int devm_of_clk_add_hw_provider_w_node(struct device *dev, > struct clk_hw *(*get)(struct of_phandle_args *clkspec, > void *data), > struct device_node *of_node, > void *data) > { > struct device_node **ptr; > int ret; > ptr = devres_alloc(devm_of_clk_release_provider, sizeof(*ptr), > GFP_KERNEL); > if (!ptr) > return -ENOMEM; > > *ptr = of_node; > ret = of_clk_add_hw_provider(of_node, get, data); > if (!ret) > devres_add(dev, ptr); > else > devres_free(ptr); > > return ret; > } > EXPORT_SYMBOL_GPL(devm_of_clk_add_hw_provider_w_node); > > int devm_of_clk_add_hw_provider(struct device *dev, > struct clk_hw *(*get)(struct of_phandle_args *clkspec, > void *data), > void *data) > { > return devm_of_clk_add_hw_provider_w_node(dev, get, dev->of_node, > data); > } > EXPORT_SYMBOL_GPL(devm_of_clk_add_hw_provider); just a friendly reminder, what's your opinion on adding this kind of function (devm_of_clk_add_hw_provider_w_node)? or solutions 1/2? And are these options safe what comes to reference counting of of_nodes? Best regards Matti Vaittinen -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello Again, On Wed, Jun 27, 2018 at 11:40:00AM +0300, Matti Vaittinen wrote: > Hello Stephen, > > On Mon, Jun 25, 2018 at 04:46:24PM -0700, Stephen Boyd wrote: > > Quoting Matti Vaittinen (2018-06-13 06:03:38) > > > On Tue, Jun 12, 2018 at 11:23:54AM +0300, Matti Vaittinen wrote: > > > > > > > > I see. This makes sense. I need to verify from HW colleagues whether > > > > this chip has internal oscillator or not. I originally thought we have > > > > on-chip oscillator - but as you say, we do have XIN pin in documentation. > > > > So now I am not sure if the test board I have contains oscillator driving > > > > the clk on PMIC - or if the PMIC has internal oscillator. I'll clarify this. > > > > > > It really turned out that the PMIC just acts as a clock buffer. So I do > > > as you suggested and add lookup for parent clock to the driver. I > > > planned to do it so that if no parent is found from DT - then we assume > > > the 32.768KHz clock (as described in documentation). Eg, something along > > > the lines: > > > > > > init.parent_names = of_clk_get_parent_name(pdev->dev.parent->of_node, 0); > > > if (init.parent_names) { > > > init.num_parents = 1; > > > } else { > > > /* If parent is not given from DT we assume the typical use-case with > > > * 32.768 KHz oscillator for RTC (Maybe we could just error out here?) > > > */ > > > c->rate = BD71837_CLK_RATE; > > > bd71837_clk_ops.recalc_rate = &bd71837_clk_recalc_rate; > > > } > > > > You can also add a clk directly in this driver in that case there isn't > > one in DT with the rate and name of your choosing. Then the logic is the > > same and we don't need a c->rate variable. > > So you mean that I should use clk_hw_register_fixed_rate and create new > clk if parent is not found? Isn't this a bit of an overkill? Downside is > that then we do need remove/cleanup functionality for deleting this > parent clock - and I didn't find devm support for fixed clock. Furthermore > I guess that since it is parent, it can't be removed before child is removed. > > Or did you mean something else but creating a fixed rate clock as parent > here? I would be grateful for any tips on how to proceed. Should I 1. Really create a fixed rate clock - and build cleanup sequence myself 2. Keep this simple and fail if no parent is found using of_clk_get_parent_name Best regards Matti Vaittinen -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jul 31, 2018 at 11:28:58AM +0300, Matti Vaittinen wrote: > On Tue, Jun 26, 2018 at 11:13:19AM +0300, Matti Vaittinen wrote: > > On Mon, Jun 25, 2018 at 04:44:57PM -0700, Stephen Boyd wrote: > > > Quoting Matti Vaittinen (2018-06-12 01:23:54) > > > > On Tue, Jun 12, 2018 at 12:44:11AM -0700, Stephen Boyd wrote: > > > > > Quoting Matti Vaittinen (2018-06-04 06:19:13) [snip] > > > 3. Create devm_of_clk_add_hw_provider_w_node() which does something > After giving this second thought - I think there is limited amount of use cases where other than own or parent nodes should be used. Actually, the MFD node being parent is pretty much only use case I can think of where something else but own node should be used. Hence function like suggested devm_of_clk_add_hw_provider_w_node might invite thinking of clever hacks... So, perhaps introducing devm_of_clk_add_hw_provider_parent() (see idea below) would be option to consider? I feel the bd71837 driver is not only case where MFD is being parent which has the clock stuff in DT. static int __devm_of_clk_add_hw_provider(struct device *dev, struct clk_hw *(*get)(struct of_phandle_args *clkspec, void *data), struct device_node *of_node, void *data) { struct device_node **ptr; int ret; ptr = devres_alloc(devm_of_clk_release_provider, sizeof(*ptr), GFP_KERNEL); if (!ptr) return -ENOMEM; *ptr = of_node; ret = of_clk_add_hw_provider(of_node, get, data); if (!ret) devres_add(dev, ptr); else devres_free(ptr); return ret; } int devm_of_clk_add_hw_provider(struct device *dev, struct clk_hw *(*get)(struct of_phandle_args *clkspec, void *data), void *data) { return __devm_of_clk_add_hw_provider(dev, get, dev->of_node, data); } EXPORT_SYMBOL_GPL(devm_of_clk_add_hw_provider); int devm_of_clk_add_hw_provider_parent(struct device *dev, struct clk_hw *(*get)(struct of_phandle_args *clkspec, void *data), void *data) { return __devm_of_clk_add_hw_provider(*dev, get, dev->parent->of_node, data); } EXPORT_SYMBOL_GPL(devm_of_clk_add_hw_provider_parent); > just a friendly reminder, what's your opinion on adding this kind of > function (devm_of_clk_add_hw_provider_w_node)? or solutions 1/2? And are > these options safe what comes to reference counting of of_nodes? I thik the reference counting should not be a problem when use is limited to (MFD) parent device nodes, right? Best regards Matti Vaittinen -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig index 41492e980ef4..e693496f202a 100644 --- a/drivers/clk/Kconfig +++ b/drivers/clk/Kconfig @@ -279,6 +279,13 @@ config COMMON_CLK_STM32H7 ---help--- Support for stm32h7 SoC family clocks +config COMMON_CLK_BD71837 + tristate "Clock driver for ROHM BD71837 PMIC MFD" + depends on MFD_BD71837 + help + This driver supports ROHM BD71837 PMIC clock. + + source "drivers/clk/bcm/Kconfig" source "drivers/clk/hisilicon/Kconfig" source "drivers/clk/imgtec/Kconfig" diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile index de6d06ac790b..8393c4af7d5a 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_BD71837) += clk-bd71837.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-bd71837.c b/drivers/clk/clk-bd71837.c new file mode 100644 index 000000000000..5ba6c05c5a98 --- /dev/null +++ b/drivers/clk/clk-bd71837.c @@ -0,0 +1,146 @@ +// 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/bd71837.h> +#include <linux/clk-provider.h> +#include <linux/clkdev.h> + + +struct bd71837_clk { + struct clk_hw hw; + uint8_t reg; + uint8_t mask; + unsigned long rate; + struct platform_device *pdev; + struct bd71837 *mfd; +}; + +static int bd71837_clk_set(struct clk_hw *hw, int status) +{ + struct bd71837_clk *c = container_of(hw, struct bd71837_clk, hw); + + return bd71837_update_bits(c->mfd, c->reg, c->mask, status); +} + +static void bd71837_clk_disable(struct clk_hw *hw) +{ + int rv; + struct bd71837_clk *c = container_of(hw, struct bd71837_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) +{ + struct bd71837_clk *c = container_of(hw, struct bd71837_clk, hw); + + return c->mask & bd71837_reg_read(c->mfd, c->reg); +} + +static unsigned long bd71837_clk_recalc_rate(struct clk_hw *hw, + unsigned long parent_rate) +{ + struct bd71837_clk *c = container_of(hw, struct bd71837_clk, hw); + + return c->rate; +} + +static const struct clk_ops bd71837_clk_ops = { + .recalc_rate = &bd71837_clk_recalc_rate, + .prepare = &bd71837_clk_enable, + .unprepare = &bd71837_clk_disable, + .is_prepared = &bd71837_clk_is_enabled, +}; + +static int bd71837_clk_probe(struct platform_device *pdev) +{ + struct bd71837_clk *c; + int rval = -ENOMEM; + struct bd71837 *mfd = dev_get_drvdata(pdev->dev.parent); + struct clk_init_data init = { + .name = "bd71837-32k-out", + .ops = &bd71837_clk_ops, + }; + + c = devm_kzalloc(&pdev->dev, sizeof(*c), GFP_KERNEL); + if (!c) + goto err_out; + + c->reg = BD71837_REG_OUT32K; + c->mask = BD71837_OUT32K_EN; + c->rate = BD71837_CLK_RATE; + c->mfd = mfd; + c->pdev = pdev; + + of_property_read_string_index(pdev->dev.parent->of_node, + "clock-output-names", 0, + &init.name); + + c->hw.init = &init; + + rval = devm_clk_hw_register(&pdev->dev, &c->hw); + if (rval) { + dev_err(&pdev->dev, "failed to register 32K clk"); + goto err_out; + } + + if (pdev->dev.parent->of_node) { + rval = of_clk_add_hw_provider(pdev->dev.parent->of_node, + of_clk_hw_simple_get, + &c->hw); + if (rval) { + dev_err(&pdev->dev, "adding clk provider failed\n"); + goto err_out; + } + } + + rval = clk_hw_register_clkdev(&c->hw, init.name, NULL); + if (rval) { + dev_err(&pdev->dev, "failed to register clkdev for bd71837"); + goto err_clean_provider; + } + + platform_set_drvdata(pdev, c); + + return 0; + +err_clean_provider: + of_clk_del_provider(pdev->dev.parent->of_node); +err_out: + return rval; +} + +static int bd71837_clk_remove(struct platform_device *pdev) +{ + if (pdev->dev.parent->of_node) + of_clk_del_provider(pdev->dev.parent->of_node); + return 0; +} + +static struct platform_driver bd71837_clk = { + .driver = { + .name = "bd71837-clk", + }, + .probe = bd71837_clk_probe, + .remove = bd71837_clk_remove, +}; + +module_platform_driver(bd71837_clk); + +MODULE_AUTHOR("Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>"); +MODULE_DESCRIPTION("BD71837 chip clk driver"); +MODULE_LICENSE("GPL");
Support BD71837 gateable 32768 Hz clock. Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> --- drivers/clk/Kconfig | 7 +++ drivers/clk/Makefile | 1 + drivers/clk/clk-bd71837.c | 146 ++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 154 insertions(+) create mode 100644 drivers/clk/clk-bd71837.c