| Message ID | 20190918104634.15216-3-s.nawrocki@samsung.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v1,1/9] ASoC: wm8994: Do not register inapplicable controls for WM1811 | expand |
Cc: lee.jones@linaro.org Excuse me Lee, I forgot to Cc entire series to you, will do it in case of posting v2. On 9/18/19 12:46, Sylwester Nawrocki wrote: > The WM1811/WM8994/WM8958 audio CODEC DT bindings specify two optional > clocks: "MCLK1", "MCLK2". Add code for getting those clocks in the MFD > part of the wm8994 driver so they can be further handled in the audio > CODEC part. > > Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com> > --- > drivers/mfd/wm8994-core.c | 9 +++++++++ > include/linux/mfd/wm8994/core.h | 9 +++++++++ > 2 files changed, 18 insertions(+) > > diff --git a/drivers/mfd/wm8994-core.c b/drivers/mfd/wm8994-core.c > index 1e9fe7d92597..02c19a0bdeb0 100644 > --- a/drivers/mfd/wm8994-core.c > +++ b/drivers/mfd/wm8994-core.c > @@ -7,6 +7,7 @@ > * Author: Mark Brown <broonie@opensource.wolfsonmicro.com> > */ > > +#include <linux/clk.h> > #include <linux/kernel.h> > #include <linux/module.h> > #include <linux/slab.h> > @@ -333,6 +334,14 @@ static int wm8994_device_init(struct wm8994 *wm8994, int irq) > > dev_set_drvdata(wm8994->dev, wm8994); > > + wm8994->mclk[WM8994_MCLK1].id = "MCLK1"; > + wm8994->mclk[WM8994_MCLK2].id = "MCLK2"; > + > + ret = devm_clk_bulk_get_optional(wm8994->dev, ARRAY_SIZE(wm8994->mclk), > + wm8994->mclk); > + if (ret != 0) > + return ret; > + > /* Add the on-chip regulators first for bootstrapping */ > ret = mfd_add_devices(wm8994->dev, 0, > wm8994_regulator_devs, > diff --git a/include/linux/mfd/wm8994/core.h b/include/linux/mfd/wm8994/core.h > index e8b093522ffd..320297a1b70c 100644 > --- a/include/linux/mfd/wm8994/core.h > +++ b/include/linux/mfd/wm8994/core.h > @@ -10,12 +10,19 @@ > #ifndef __MFD_WM8994_CORE_H__ > #define __MFD_WM8994_CORE_H__ > > +#include <linux/clk.h> > #include <linux/mutex.h> > #include <linux/interrupt.h> > #include <linux/regmap.h> > > #include <linux/mfd/wm8994/pdata.h> > > +enum { > + WM8994_MCLK1, > + WM8994_MCLK2, > + WM8994_NUM_MCLK > +}; > + > enum wm8994_type { > WM8994 = 0, > WM8958 = 1, > @@ -60,6 +67,8 @@ struct wm8994 { > struct device *dev; > struct regmap *regmap; > > + struct clk_bulk_data mclk[WM8994_NUM_MCLK]; > + > bool ldo_ena_always_driven; > > int gpio_base; -- Regards, Sylwester
On Wed, Sep 18, 2019 at 12:59:28PM +0200, Sylwester Nawrocki wrote: > On 9/18/19 12:46, Sylwester Nawrocki wrote: > > The WM1811/WM8994/WM8958 audio CODEC DT bindings specify two optional > > clocks: "MCLK1", "MCLK2". Add code for getting those clocks in the MFD > > part of the wm8994 driver so they can be further handled in the audio > > CODEC part. > > > > Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com> > > --- > > > > + wm8994->mclk[WM8994_MCLK1].id = "MCLK1"; > > + wm8994->mclk[WM8994_MCLK2].id = "MCLK2"; > > + > > + ret = devm_clk_bulk_get_optional(wm8994->dev, ARRAY_SIZE(wm8994->mclk), > > + wm8994->mclk); > > + if (ret != 0) > > + return ret; Would be nice to print a message here as well, make it clear what failed in the log. Apart from that minor nit: Acked-by: Charles Keepax <ckeepax@opensource.cirrus.com> Thanks, Charles
On 9/18/19 14:54, Charles Keepax wrote: >>> + ret = devm_clk_bulk_get_optional(wm8994->dev, ARRAY_SIZE(wm8994->mclk), >>> + wm8994->mclk); >>> + if (ret != 0) >>> + return ret; > > Would be nice to print a message here as well, make it clear what > failed in the log. Apart from that minor nit: Thanks for reviewing, I will add that modification for v2.
On Wed, Sep 18, 2019 at 12:46:27PM +0200, Sylwester Nawrocki wrote: > The WM1811/WM8994/WM8958 audio CODEC DT bindings specify two optional > clocks: "MCLK1", "MCLK2". Add code for getting those clocks in the MFD > part of the wm8994 driver so they can be further handled in the audio > CODEC part. I think these are needed only for the codec so how about getting them in codec's probe? Best regards, Krzysztof > > Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com> > --- > drivers/mfd/wm8994-core.c | 9 +++++++++ > include/linux/mfd/wm8994/core.h | 9 +++++++++ > 2 files changed, 18 insertions(+) > > diff --git a/drivers/mfd/wm8994-core.c b/drivers/mfd/wm8994-core.c > index 1e9fe7d92597..02c19a0bdeb0 100644 > --- a/drivers/mfd/wm8994-core.c > +++ b/drivers/mfd/wm8994-core.c > @@ -7,6 +7,7 @@ > * Author: Mark Brown <broonie@opensource.wolfsonmicro.com> > */ > > +#include <linux/clk.h> > #include <linux/kernel.h> > #include <linux/module.h> > #include <linux/slab.h> > @@ -333,6 +334,14 @@ static int wm8994_device_init(struct wm8994 *wm8994, int irq) > > dev_set_drvdata(wm8994->dev, wm8994); > > + wm8994->mclk[WM8994_MCLK1].id = "MCLK1"; > + wm8994->mclk[WM8994_MCLK2].id = "MCLK2"; > + > + ret = devm_clk_bulk_get_optional(wm8994->dev, ARRAY_SIZE(wm8994->mclk), > + wm8994->mclk); > + if (ret != 0) > + return ret; > + > /* Add the on-chip regulators first for bootstrapping */ > ret = mfd_add_devices(wm8994->dev, 0, > wm8994_regulator_devs, > diff --git a/include/linux/mfd/wm8994/core.h b/include/linux/mfd/wm8994/core.h > index e8b093522ffd..320297a1b70c 100644 > --- a/include/linux/mfd/wm8994/core.h > +++ b/include/linux/mfd/wm8994/core.h > @@ -10,12 +10,19 @@ > #ifndef __MFD_WM8994_CORE_H__ > #define __MFD_WM8994_CORE_H__ > > +#include <linux/clk.h> > #include <linux/mutex.h> > #include <linux/interrupt.h> > #include <linux/regmap.h> > > #include <linux/mfd/wm8994/pdata.h> > > +enum { > + WM8994_MCLK1, > + WM8994_MCLK2, > + WM8994_NUM_MCLK > +}; > + > enum wm8994_type { > WM8994 = 0, > WM8958 = 1, > @@ -60,6 +67,8 @@ struct wm8994 { > struct device *dev; > struct regmap *regmap; > > + struct clk_bulk_data mclk[WM8994_NUM_MCLK]; > + > bool ldo_ena_always_driven; > > int gpio_base; > -- > 2.17.1 >
On 9/19/19 09:59, Krzysztof Kozlowski wrote: > On Wed, Sep 18, 2019 at 12:46:27PM +0200, Sylwester Nawrocki wrote: >> The WM1811/WM8994/WM8958 audio CODEC DT bindings specify two optional >> clocks: "MCLK1", "MCLK2". Add code for getting those clocks in the MFD >> part of the wm8994 driver so they can be further handled in the audio >> CODEC part. > > I think these are needed only for the codec so how about getting them in > codec's probe? The clocks are specified in the (I2C slave device) DT node which corresponds to the device as a whole and to which binds the MFD driver. AFAICT those clocks are only needed by the CODEC sub-driver but in general such clocks could be used to provide device's system clock used by other functionalities than audio. We are doing something similar for other MFD drivers already and I would rather stick to one pattern. IMHO it's clearer to see what device the clocks belong to that way.
On Thu, Sep 19, 2019 at 09:59:24AM +0200, Krzysztof Kozlowski wrote: > On Wed, Sep 18, 2019 at 12:46:27PM +0200, Sylwester Nawrocki wrote: > > The WM1811/WM8994/WM8958 audio CODEC DT bindings specify two optional > > clocks: "MCLK1", "MCLK2". Add code for getting those clocks in the MFD > > part of the wm8994 driver so they can be further handled in the audio > > CODEC part. > I think these are needed only for the codec so how about getting them in > codec's probe? Yeah. IIRC when these were added a machine driver was grabbing them. I don't think that machine driver ever made it's way upstream though.
On Thu, 19 Sep 2019 at 14:49, Sylwester Nawrocki <s.nawrocki@samsung.com> wrote: > > On 9/19/19 09:59, Krzysztof Kozlowski wrote: > > On Wed, Sep 18, 2019 at 12:46:27PM +0200, Sylwester Nawrocki wrote: > >> The WM1811/WM8994/WM8958 audio CODEC DT bindings specify two optional > >> clocks: "MCLK1", "MCLK2". Add code for getting those clocks in the MFD > >> part of the wm8994 driver so they can be further handled in the audio > >> CODEC part. > > > > I think these are needed only for the codec so how about getting them in > > codec's probe? > > The clocks are specified in the (I2C slave device) DT node which corresponds > to the device as a whole and to which binds the MFD driver. AFAICT those > clocks are only needed by the CODEC sub-driver but in general such clocks > could be used to provide device's system clock used by other functionalities > than audio. We are doing something similar for other MFD drivers already > and I would rather stick to one pattern. IMHO it's clearer to see what > device the clocks belong to that way. The existing pattern is not an excuse for doing some things in a uglier way... If we agree that these are codec-only resources, then they should be acquired by codec driver. This is one minor step to solve difficult inter-device dependencies which stops from reusing parts of the code (some child of MFD heavily depends on parent). Existing MFD drivers sometimes follow this pattern but that's wrong. They follow the ugly or even crappy pattern. Your codec driver still refers to parent and it has its resources, e.g. parent's device node pointer. Best regards, Krzysztof
On Thu, Sep 19, 2019 at 01:50:20PM +0100, Mark Brown wrote: > On Thu, Sep 19, 2019 at 09:59:24AM +0200, Krzysztof Kozlowski wrote: > > On Wed, Sep 18, 2019 at 12:46:27PM +0200, Sylwester Nawrocki wrote: > > > The WM1811/WM8994/WM8958 audio CODEC DT bindings specify two optional > > > clocks: "MCLK1", "MCLK2". Add code for getting those clocks in the MFD > > > part of the wm8994 driver so they can be further handled in the audio > > > CODEC part. > > > I think these are needed only for the codec so how about getting them in > > codec's probe? > > Yeah. IIRC when these were added a machine driver was grabbing them. I > don't think that machine driver ever made it's way upstream though. If you mean for the Arizona stuff, the machine driver using that was sound/soc/samsung/tm2_wm5110.c. Sylwester upstreamed it along with the patches. I think on wm8994 the clocks probably are only needed by the audio part of the driver, so probably can be moved in there, although as a disclaimer I have done a lot less with parts of that era. However on Arizona the clocking is needed from various parts of the driver so couldn't be moved exclusively to the codec driver. Thanks, Charles
On Thu, Sep 19, 2019 at 02:31:16PM +0000, Charles Keepax wrote: > On Thu, Sep 19, 2019 at 01:50:20PM +0100, Mark Brown wrote: > > Yeah. IIRC when these were added a machine driver was grabbing them. I > > don't think that machine driver ever made it's way upstream though. > If you mean for the Arizona stuff, the machine driver using that > was sound/soc/samsung/tm2_wm5110.c. Sylwester upstreamed it along > with the patches. No, there was a WM8994 thing before that. > I think on wm8994 the clocks probably are only needed by the > audio part of the driver, so probably can be moved in there, > although as a disclaimer I have done a lot less with parts > of that era. However on Arizona the clocking is needed from > various parts of the driver so couldn't be moved exclusively > to the codec driver. Yes, they're only needed by the audio part of the driver.
diff --git a/drivers/mfd/wm8994-core.c b/drivers/mfd/wm8994-core.c index 1e9fe7d92597..02c19a0bdeb0 100644 --- a/drivers/mfd/wm8994-core.c +++ b/drivers/mfd/wm8994-core.c @@ -7,6 +7,7 @@ * Author: Mark Brown <broonie@opensource.wolfsonmicro.com> */ +#include <linux/clk.h> #include <linux/kernel.h> #include <linux/module.h> #include <linux/slab.h> @@ -333,6 +334,14 @@ static int wm8994_device_init(struct wm8994 *wm8994, int irq) dev_set_drvdata(wm8994->dev, wm8994); + wm8994->mclk[WM8994_MCLK1].id = "MCLK1"; + wm8994->mclk[WM8994_MCLK2].id = "MCLK2"; + + ret = devm_clk_bulk_get_optional(wm8994->dev, ARRAY_SIZE(wm8994->mclk), + wm8994->mclk); + if (ret != 0) + return ret; + /* Add the on-chip regulators first for bootstrapping */ ret = mfd_add_devices(wm8994->dev, 0, wm8994_regulator_devs, diff --git a/include/linux/mfd/wm8994/core.h b/include/linux/mfd/wm8994/core.h index e8b093522ffd..320297a1b70c 100644 --- a/include/linux/mfd/wm8994/core.h +++ b/include/linux/mfd/wm8994/core.h @@ -10,12 +10,19 @@ #ifndef __MFD_WM8994_CORE_H__ #define __MFD_WM8994_CORE_H__ +#include <linux/clk.h> #include <linux/mutex.h> #include <linux/interrupt.h> #include <linux/regmap.h> #include <linux/mfd/wm8994/pdata.h> +enum { + WM8994_MCLK1, + WM8994_MCLK2, + WM8994_NUM_MCLK +}; + enum wm8994_type { WM8994 = 0, WM8958 = 1, @@ -60,6 +67,8 @@ struct wm8994 { struct device *dev; struct regmap *regmap; + struct clk_bulk_data mclk[WM8994_NUM_MCLK]; + bool ldo_ena_always_driven; int gpio_base;
The WM1811/WM8994/WM8958 audio CODEC DT bindings specify two optional clocks: "MCLK1", "MCLK2". Add code for getting those clocks in the MFD part of the wm8994 driver so they can be further handled in the audio CODEC part. Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com> --- drivers/mfd/wm8994-core.c | 9 +++++++++ include/linux/mfd/wm8994/core.h | 9 +++++++++ 2 files changed, 18 insertions(+)