[PATCH 0/3] ARM: omap: omap4-embt2ws: 32K clock for WLAN

[PATCH 0/3] ARM: omap: omap4-embt2ws: 32K clock for WLAN

[Andreas Kemnade]

To have WLAN working properly, enable a 32K clock of the TWL6032.
In earlier tests, it was still enabled from a previous boot into
the vendor system.

Andreas Kemnade (3):
  dt-bindings: clock: add TWL6032 32K clocks
  clk: twl: add clock driver for TWL6032
  ARM: dts: omap4-embt2ws: enable 32K clock on WLAN

 .../bindings/clock/ti,twl6032-clk.yaml        |  38 ++++
 .../boot/dts/ti/omap/omap4-epson-embt2ws.dts  |  12 +
 drivers/clk/Kconfig                           |   9 +
 drivers/clk/Makefile                          |   1 +
 drivers/clk/clk-twl.c                         | 205 ++++++++++++++++++
 5 files changed, 265 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/ti,twl6032-clk.yaml
 create mode 100644 drivers/clk/clk-twl.c

-- 
2.39.2

[PATCH 1/3] dt-bindings: clock: add TWL6032 32K clocks

[Andreas Kemnade]

To be able to be referenced from a future yaml-version of
mfd/twl-family.txt depending on toplevel compatible have a separate file
for the 6032

Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
---
 .../bindings/clock/ti,twl6032-clk.yaml        | 38 +++++++++++++++++++
 1 file changed, 38 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/ti,twl6032-clk.yaml

diff --git a/Documentation/devicetree/bindings/clock/ti,twl6032-clk.yaml b/Documentation/devicetree/bindings/clock/ti,twl6032-clk.yaml
new file mode 100644
index 0000000000000..aebd9f8d761a2
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/ti,twl6032-clk.yaml
@@ -0,0 +1,38 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/clock/ti,twl6032-clk.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Clocks of the TWL6032 PMIC
+
+maintainers:
+  - Andreas Kemnade <andreas@kemnade.info>
+
+description:
+  The TWL6032 has some 32Khz clock outputs which can be controlled.
+
+properties:
+  compatible:
+    enum:
+      - ti,twl6032-clk32kaudio
+      - ti,twl6032-clk32kg
+
+  '#clock-cells':
+    const: 0
+
+required:
+  - compatible
+  - '#clock-cells'
+
+additionalProperties: false
+
+examples:
+  - |
+    twl {
+        clk32kaudio {
+            compatible = "ti,twl6032-clk32kaudio";
+            #clock-cells = <0>;
+        };
+    };
+...
-- 
2.39.2

Re: [PATCH 1/3] dt-bindings: clock: add TWL6032 32K clocks

[Rob Herring]

On Sat, Aug 19, 2023 at 03:41:45PM +0200, Andreas Kemnade wrote:
> To be able to be referenced from a future yaml-version of
> mfd/twl-family.txt depending on toplevel compatible have a separate file
> for the 6032

Really, the parent needs to be done first...

> Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
> ---
>  .../bindings/clock/ti,twl6032-clk.yaml        | 38 +++++++++++++++++++
>  1 file changed, 38 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/clock/ti,twl6032-clk.yaml
> 
> diff --git a/Documentation/devicetree/bindings/clock/ti,twl6032-clk.yaml b/Documentation/devicetree/bindings/clock/ti,twl6032-clk.yaml
> new file mode 100644
> index 0000000000000..aebd9f8d761a2
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/ti,twl6032-clk.yaml
> @@ -0,0 +1,38 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/clock/ti,twl6032-clk.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Clocks of the TWL6032 PMIC
> +
> +maintainers:
> +  - Andreas Kemnade <andreas@kemnade.info>
> +
> +description:
> +  The TWL6032 has some 32Khz clock outputs which can be controlled.

outputs? Seems like only 1 with no clock cells to specify which one.

> +
> +properties:
> +  compatible:
> +    enum:
> +      - ti,twl6032-clk32kaudio
> +      - ti,twl6032-clk32kg

Or is it 1 output per compatible? I hope not.

> +
> +  '#clock-cells':
> +    const: 0
> +
> +required:
> +  - compatible
> +  - '#clock-cells'
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    twl {
> +        clk32kaudio {
> +            compatible = "ti,twl6032-clk32kaudio";
> +            #clock-cells = <0>;
> +        };

You don't need a child node to be a clock provider. Just add 
#clock-cells to the parent node.

Rob

Re: [PATCH 1/3] dt-bindings: clock: add TWL6032 32K clocks

[Andreas Kemnade]

On Mon, 21 Aug 2023 15:57:45 -0500
Rob Herring <robh@kernel.org> wrote:

> On Sat, Aug 19, 2023 at 03:41:45PM +0200, Andreas Kemnade wrote:
> > To be able to be referenced from a future yaml-version of
> > mfd/twl-family.txt depending on toplevel compatible have a separate
> > file for the 6032  
> 
> Really, the parent needs to be done first...
> 
well, for some other subdevices, a yaml is already in the tree
and Krzysztof recently added a R-By to another one.

But if the clocks should not have a node, then it is obvious.
What would be the route to conversion here: Is a conversion
of mfd/twl-family.txt without specifying subnodes ok for the first step,
maybe with additionalProperties: yes?


> > Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
> > ---
> >  .../bindings/clock/ti,twl6032-clk.yaml        | 38
> > +++++++++++++++++++ 1 file changed, 38 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/clock/ti,twl6032-clk.yaml
> > 
> > diff --git
> > a/Documentation/devicetree/bindings/clock/ti,twl6032-clk.yaml
> > b/Documentation/devicetree/bindings/clock/ti,twl6032-clk.yaml new
> > file mode 100644 index 0000000000000..aebd9f8d761a2 --- /dev/null
> > +++ b/Documentation/devicetree/bindings/clock/ti,twl6032-clk.yaml
> > @@ -0,0 +1,38 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/clock/ti,twl6032-clk.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Clocks of the TWL6032 PMIC
> > +
> > +maintainers:
> > +  - Andreas Kemnade <andreas@kemnade.info>
> > +
> > +description:
> > +  The TWL6032 has some 32Khz clock outputs which can be
> > controlled.  
> 
> outputs? Seems like only 1 with no clock cells to specify which one.
> 
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - ti,twl6032-clk32kaudio
> > +      - ti,twl6032-clk32kg  
> 
> Or is it 1 output per compatible? I hope not.
> 
yes, it is. It was inspired by the clk-palmas driver:
$ grep palmas.*32 arch/arm/boot/dts/ti/omap/omap5-*
arch/arm/boot/dts/ti/omap/omap5-board-common.dtsi:
clk32kgaudio: palmas_clk32k@1 {
arch/arm/boot/dts/ti/omap/omap5-board-common.dtsi:
	compatible = "ti,palmas-clk32kgaudio";

Well, we have the CLK_IGNORE_UNUSED, so if we use #clock-cells = 1,
an unused clock will not be touched by the kernel, right?

> > +
> > +  '#clock-cells':
> > +    const: 0
> > +
> > +required:
> > +  - compatible
> > +  - '#clock-cells'
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    twl {
> > +        clk32kaudio {
> > +            compatible = "ti,twl6032-clk32kaudio";
> > +            #clock-cells = <0>;
> > +        };  
> 
> You don't need a child node to be a clock provider. Just add 
> #clock-cells to the parent node.
> 
hmm, we have child nodes there for every subdevice in that family,
even if I doubt it is totally technically required.
So why should the clk device be an exception? 

Regards,
Andreas

Re: [PATCH 1/3] dt-bindings: clock: add TWL6032 32K clocks

[Krzysztof Kozlowski]

On 23/08/2023 17:38, Andreas Kemnade wrote:
> On Mon, 21 Aug 2023 15:57:45 -0500
> Rob Herring <robh@kernel.org> wrote:
> 
>> On Sat, Aug 19, 2023 at 03:41:45PM +0200, Andreas Kemnade wrote:
>>> To be able to be referenced from a future yaml-version of
>>> mfd/twl-family.txt depending on toplevel compatible have a separate
>>> file for the 6032  
>>
>> Really, the parent needs to be done first...
>>
> well, for some other subdevices, a yaml is already in the tree
> and Krzysztof recently added a R-By to another one.

Yep, but I am not checking every possible parent-child relationship. It
would not be even possible...

> 
> But if the clocks should not have a node, then it is obvious.
> What would be the route to conversion here: Is a conversion
> of mfd/twl-family.txt without specifying subnodes ok for the first step,
> maybe with additionalProperties: yes?

Yes.

> 
> 
>>> Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
>>> ---
>>>  .../bindings/clock/ti,twl6032-clk.yaml        | 38
>>> +++++++++++++++++++ 1 file changed, 38 insertions(+)
>>>  create mode 100644
>>> Documentation/devicetree/bindings/clock/ti,twl6032-clk.yaml
>>>
>>> diff --git
>>> a/Documentation/devicetree/bindings/clock/ti,twl6032-clk.yaml
>>> b/Documentation/devicetree/bindings/clock/ti,twl6032-clk.yaml new
>>> file mode 100644 index 0000000000000..aebd9f8d761a2 --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/clock/ti,twl6032-clk.yaml
>>> @@ -0,0 +1,38 @@
>>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/clock/ti,twl6032-clk.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: Clocks of the TWL6032 PMIC
>>> +
>>> +maintainers:
>>> +  - Andreas Kemnade <andreas@kemnade.info>
>>> +
>>> +description:
>>> +  The TWL6032 has some 32Khz clock outputs which can be
>>> controlled.  
>>
>> outputs? Seems like only 1 with no clock cells to specify which one.
>>
>>> +
>>> +properties:
>>> +  compatible:
>>> +    enum:
>>> +      - ti,twl6032-clk32kaudio
>>> +      - ti,twl6032-clk32kg  
>>
>> Or is it 1 output per compatible? I hope not.
>>
> yes, it is. It was inspired by the clk-palmas driver:

Creating nodes for single clocks is rather antipattern. Also, many early
designs of drivers and bindings assumed mapping 1-to-1 between driver
and DT nodes. This is also considered an antipattern now.

> $ grep palmas.*32 arch/arm/boot/dts/ti/omap/omap5-*
> arch/arm/boot/dts/ti/omap/omap5-board-common.dtsi:
> clk32kgaudio: palmas_clk32k@1 {
> arch/arm/boot/dts/ti/omap/omap5-board-common.dtsi:
> 	compatible = "ti,palmas-clk32kgaudio";
> 
> Well, we have the CLK_IGNORE_UNUSED, so if we use #clock-cells = 1,
> an unused clock will not be touched by the kernel, right?

I don't understand what OS flag has anything to do with clock-cells...

> 
>>> +
>>> +  '#clock-cells':
>>> +    const: 0
>>> +
>>> +required:
>>> +  - compatible
>>> +  - '#clock-cells'
>>> +
>>> +additionalProperties: false
>>> +
>>> +examples:
>>> +  - |
>>> +    twl {
>>> +        clk32kaudio {
>>> +            compatible = "ti,twl6032-clk32kaudio";
>>> +            #clock-cells = <0>;
>>> +        };  
>>
>> You don't need a child node to be a clock provider. Just add 
>> #clock-cells to the parent node.
>>
> hmm, we have child nodes there for every subdevice in that family,
> even if I doubt it is totally technically required.
> So why should the clk device be an exception? 

There is no rule of having nodes for subdevices, thus there cannot be
such exception. The rule is nodes are created when needed, not to match
some consistency of Linux drivers.


Best regards,
Krzysztof

[PATCH 2/3] clk: twl: add clock driver for TWL6032

[Andreas Kemnade]

The TWL6032 has some clock outputs which are controlled like
fixed-voltage regulators, in some drivers for these chips
found in the wild, just the regulator api is abused for controlling
them, so simply use something similar to the regulator functions.
Due to a lack of hardware available for testing, leave out the
TWL6030-specific part of those functions.

Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
---
 drivers/clk/Kconfig   |   9 ++
 drivers/clk/Makefile  |   1 +
 drivers/clk/clk-twl.c | 205 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 215 insertions(+)
 create mode 100644 drivers/clk/clk-twl.c

diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index 6b3b424addab4..e927f88d6014c 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -277,6 +277,15 @@ config COMMON_CLK_S2MPS11
 	  clock. These multi-function devices have two (S2MPS14) or three
 	  (S2MPS11, S5M8767) fixed-rate oscillators, clocked at 32KHz each.
 
+config CLK_TWL
+	tristate "Clock driver for the TWL PMIC family"
+	depends on TWL4030_CORE
+	help
+	  Enable support for controlling the clock resources on TWL family
+	  PMICs. These devices have some 32K clock outputs which can be
+	  controlled by software. For now, only the TWL6032 clocks are
+	  supported.
+
 config CLK_TWL6040
 	tristate "External McPDM functional clock from twl6040"
 	depends on TWL6040_CORE
diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index 7cb000549b612..31c04e23b7a90 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -73,6 +73,7 @@ obj-$(CONFIG_COMMON_CLK_STM32H7)	+= clk-stm32h7.o
 obj-$(CONFIG_COMMON_CLK_STM32MP157)	+= clk-stm32mp1.o
 obj-$(CONFIG_COMMON_CLK_TPS68470)      += clk-tps68470.o
 obj-$(CONFIG_CLK_TWL6040)		+= clk-twl6040.o
+obj-$(CONFIG_CLK_TWL)			+= clk-twl.o
 obj-$(CONFIG_ARCH_VT8500)		+= clk-vt8500.o
 obj-$(CONFIG_COMMON_CLK_RS9_PCIE)	+= clk-renesas-pcie.o
 obj-$(CONFIG_COMMON_CLK_SI521XX)	+= clk-si521xx.o
diff --git a/drivers/clk/clk-twl.c b/drivers/clk/clk-twl.c
new file mode 100644
index 0000000000000..deb5742393bac
--- /dev/null
+++ b/drivers/clk/clk-twl.c
@@ -0,0 +1,205 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Clock driver for twl device.
+ *
+ * inspired by the driver for the Palmas device
+ */
+
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <linux/mfd/twl.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+#define VREG_STATE              2
+#define TWL6030_CFG_STATE_OFF   0x00
+#define TWL6030_CFG_STATE_ON    0x01
+#define TWL6030_CFG_STATE_MASK  0x03
+
+struct twl_clk32k_desc {
+	const char *clk_name;
+	u8 base;
+};
+
+struct twl_clock_info {
+	struct device *dev;
+	struct clk_hw hw;
+	const struct twl_clk32k_desc *clk_desc;
+};
+
+static inline int
+twlclk_read(struct twl_clock_info *info, unsigned int slave_subgp,
+	    unsigned int offset)
+{
+	u8 value;
+	int status;
+
+	status = twl_i2c_read_u8(slave_subgp, &value,
+				 info->clk_desc->base + offset);
+	return (status < 0) ? status : value;
+}
+
+static inline int
+twlclk_write(struct twl_clock_info *info, unsigned int slave_subgp,
+	     unsigned int offset, u8 value)
+{
+	return twl_i2c_write_u8(slave_subgp, value,
+				info->clk_desc->base + offset);
+}
+
+static inline struct twl_clock_info *to_twl_clks_info(struct clk_hw *hw)
+{
+	return container_of(hw, struct twl_clock_info, hw);
+}
+
+static unsigned long twl_clks_recalc_rate(struct clk_hw *hw,
+					  unsigned long parent_rate)
+{
+	return 32768;
+}
+
+static int twl6032_clks_prepare(struct clk_hw *hw)
+{
+	struct twl_clock_info *cinfo = to_twl_clks_info(hw);
+	int ret;
+
+	ret = twlclk_write(cinfo, TWL_MODULE_PM_RECEIVER, VREG_STATE,
+			   TWL6030_CFG_STATE_ON);
+	if (ret < 0)
+		dev_err(cinfo->dev, "clk prepare failed\n");
+
+	return ret;
+}
+
+static void twl6032_clks_unprepare(struct clk_hw *hw)
+{
+	struct twl_clock_info *cinfo = to_twl_clks_info(hw);
+	int ret;
+
+	ret = twlclk_write(cinfo, TWL_MODULE_PM_RECEIVER, VREG_STATE,
+			   TWL6030_CFG_STATE_OFF);
+	if (ret < 0)
+		dev_err(cinfo->dev, "clk unprepare failed\n");
+}
+
+static int twl6032_clks_is_prepared(struct clk_hw *hw)
+{
+	struct twl_clock_info *cinfo = to_twl_clks_info(hw);
+	int val;
+
+	val = twlclk_read(cinfo, TWL_MODULE_PM_RECEIVER, VREG_STATE);
+	if (val < 0) {
+		dev_err(cinfo->dev, "clk read failed\n");
+		return val;
+	}
+
+	val &= TWL6030_CFG_STATE_MASK;
+
+	return val == TWL6030_CFG_STATE_ON;
+}
+
+static const struct clk_ops twl6032_clks_ops = {
+	.prepare	= twl6032_clks_prepare,
+	.unprepare	= twl6032_clks_unprepare,
+	.is_prepared	= twl6032_clks_is_prepared,
+	.recalc_rate	= twl_clks_recalc_rate,
+};
+
+struct twl_clks_of_match_data {
+	struct clk_init_data init;
+	const struct twl_clk32k_desc desc;
+};
+
+static const struct twl_clks_of_match_data twl6032_of_clk32kg = {
+	.init = {
+		.name = "clk32kg",
+		.ops = &twl6032_clks_ops,
+		.flags = CLK_IGNORE_UNUSED,
+	},
+	.desc = {
+		.clk_name = "clk32kg",
+		.base = 0x8C,
+	},
+};
+
+static const struct twl_clks_of_match_data twl6032_of_clk32kaudio = {
+	.init = {
+		.name = "clk32kaudio",
+		.ops = &twl6032_clks_ops,
+		.flags = CLK_IGNORE_UNUSED,
+	},
+	.desc = {
+		.clk_name = "clk32kaudio",
+		.base = 0x8F,
+	},
+};
+
+static const struct of_device_id twl_clks_of_match[] = {
+	{
+		.compatible = "ti,twl6032-clk32kg",
+		.data = &twl6032_of_clk32kg,
+	},
+	{
+		.compatible = "ti,twl6032-clk32kaudio",
+		.data = &twl6032_of_clk32kaudio,
+	},
+	{ },
+};
+MODULE_DEVICE_TABLE(of, twl_clks_of_match);
+
+static int twl_clks_probe(struct platform_device *pdev)
+{
+	struct device_node *node = pdev->dev.of_node;
+	const struct twl_clks_of_match_data *match_data;
+	struct twl_clock_info *cinfo;
+	int ret;
+
+	match_data = of_device_get_match_data(&pdev->dev);
+	if (!match_data)
+		return 1;
+
+	cinfo = devm_kzalloc(&pdev->dev, sizeof(*cinfo), GFP_KERNEL);
+	if (!cinfo)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, cinfo);
+
+	cinfo->dev = &pdev->dev;
+
+	cinfo->clk_desc = &match_data->desc;
+	cinfo->hw.init = &match_data->init;
+	ret = devm_clk_hw_register(&pdev->dev, &cinfo->hw);
+	if (ret) {
+		dev_err(&pdev->dev, "Fail to register clock %s, %d\n",
+			match_data->desc.clk_name, ret);
+		return ret;
+	}
+
+	ret = of_clk_add_hw_provider(node, of_clk_hw_simple_get, &cinfo->hw);
+	if (ret < 0)
+		dev_err(&pdev->dev, "Fail to add clock driver, %d\n", ret);
+	return ret;
+}
+
+static void twl_clks_remove(struct platform_device *pdev)
+{
+	of_clk_del_provider(pdev->dev.of_node);
+}
+
+static struct platform_driver twl_clks_driver = {
+	.driver = {
+		.name = "twl-clk",
+		.of_match_table = twl_clks_of_match,
+	},
+	.probe = twl_clks_probe,
+	.remove_new = twl_clks_remove,
+};
+
+module_platform_driver(twl_clks_driver);
+
+MODULE_DESCRIPTION("Clock driver for TWL Series Devices");
+MODULE_ALIAS("platform:twl-clk");
+MODULE_LICENSE("GPL");
-- 
2.39.2

Re: [PATCH 2/3] clk: twl: add clock driver for TWL6032

[Stephen Boyd]

Quoting Andreas Kemnade (2023-08-19 06:41:46)
> diff --git a/drivers/clk/clk-twl.c b/drivers/clk/clk-twl.c
> new file mode 100644
> index 0000000000000..deb5742393bac
> --- /dev/null
> +++ b/drivers/clk/clk-twl.c
> @@ -0,0 +1,205 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Clock driver for twl device.
> + *
> + * inspired by the driver for the Palmas device
> + */
> +
> +#include <linux/clk.h>

Is this include used? Hopefully not. Please drop.

> +#include <linux/clk-provider.h>
> +#include <linux/mfd/twl.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>

Is this include used?

> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +
> +#define VREG_STATE              2
> +#define TWL6030_CFG_STATE_OFF   0x00
> +#define TWL6030_CFG_STATE_ON    0x01
> +#define TWL6030_CFG_STATE_MASK  0x03
> +
> +struct twl_clk32k_desc {
> +       const char *clk_name;
> +       u8 base;
> +};
> +
> +struct twl_clock_info {
> +       struct device *dev;
> +       struct clk_hw hw;
> +       const struct twl_clk32k_desc *clk_desc;
> +};
> +
> +static inline int
> +twlclk_read(struct twl_clock_info *info, unsigned int slave_subgp,
> +           unsigned int offset)
> +{
> +       u8 value;
> +       int status;
> +
> +       status = twl_i2c_read_u8(slave_subgp, &value,
> +                                info->clk_desc->base + offset);
> +       return (status < 0) ? status : value;
> +}
> +
> +static inline int
> +twlclk_write(struct twl_clock_info *info, unsigned int slave_subgp,
> +            unsigned int offset, u8 value)
> +{
> +       return twl_i2c_write_u8(slave_subgp, value,
> +                               info->clk_desc->base + offset);
> +}
> +
> +static inline struct twl_clock_info *to_twl_clks_info(struct clk_hw *hw)
> +{
> +       return container_of(hw, struct twl_clock_info, hw);
> +}
> +
> +static unsigned long twl_clks_recalc_rate(struct clk_hw *hw,
> +                                         unsigned long parent_rate)
> +{
> +       return 32768;
> +}
> +
> +static int twl6032_clks_prepare(struct clk_hw *hw)
> +{
> +       struct twl_clock_info *cinfo = to_twl_clks_info(hw);
> +       int ret;
> +
> +       ret = twlclk_write(cinfo, TWL_MODULE_PM_RECEIVER, VREG_STATE,
> +                          TWL6030_CFG_STATE_ON);
> +       if (ret < 0)
> +               dev_err(cinfo->dev, "clk prepare failed\n");
> +
> +       return ret;
> +}
> +
> +static void twl6032_clks_unprepare(struct clk_hw *hw)
> +{
> +       struct twl_clock_info *cinfo = to_twl_clks_info(hw);
> +       int ret;
> +
> +       ret = twlclk_write(cinfo, TWL_MODULE_PM_RECEIVER, VREG_STATE,
> +                          TWL6030_CFG_STATE_OFF);
> +       if (ret < 0)
> +               dev_err(cinfo->dev, "clk unprepare failed\n");
> +}
> +
> +static int twl6032_clks_is_prepared(struct clk_hw *hw)
> +{
> +       struct twl_clock_info *cinfo = to_twl_clks_info(hw);
> +       int val;
> +
> +       val = twlclk_read(cinfo, TWL_MODULE_PM_RECEIVER, VREG_STATE);
> +       if (val < 0) {
> +               dev_err(cinfo->dev, "clk read failed\n");
> +               return val;
> +       }
> +
> +       val &= TWL6030_CFG_STATE_MASK;
> +
> +       return val == TWL6030_CFG_STATE_ON;
> +}
> +
> +static const struct clk_ops twl6032_clks_ops = {
> +       .prepare        = twl6032_clks_prepare,
> +       .unprepare      = twl6032_clks_unprepare,
> +       .is_prepared    = twl6032_clks_is_prepared,
> +       .recalc_rate    = twl_clks_recalc_rate,
> +};
> +
> +struct twl_clks_of_match_data {
> +       struct clk_init_data init;
> +       const struct twl_clk32k_desc desc;
> +};
> +
> +static const struct twl_clks_of_match_data twl6032_of_clk32kg = {
> +       .init = {
> +               .name = "clk32kg",
> +               .ops = &twl6032_clks_ops,
> +               .flags = CLK_IGNORE_UNUSED,
> +       },
> +       .desc = {
> +               .clk_name = "clk32kg",
> +               .base = 0x8C,
> +       },
> +};
> +
> +static const struct twl_clks_of_match_data twl6032_of_clk32kaudio = {
> +       .init = {
> +               .name = "clk32kaudio",
> +               .ops = &twl6032_clks_ops,
> +               .flags = CLK_IGNORE_UNUSED,
> +       },
> +       .desc = {
> +               .clk_name = "clk32kaudio",
> +               .base = 0x8F,
> +       },
> +};
> +
> +static const struct of_device_id twl_clks_of_match[] = {
> +       {
> +               .compatible = "ti,twl6032-clk32kg",
> +               .data = &twl6032_of_clk32kg,
> +       },
> +       {
> +               .compatible = "ti,twl6032-clk32kaudio",
> +               .data = &twl6032_of_clk32kaudio,
> +       },
> +       { },
> +};
> +MODULE_DEVICE_TABLE(of, twl_clks_of_match);

This array can be moved next to the driver so that it isn't tempting to
access the variable directly.

> +
> +static int twl_clks_probe(struct platform_device *pdev)
> +{
> +       struct device_node *node = pdev->dev.of_node;
> +       const struct twl_clks_of_match_data *match_data;
> +       struct twl_clock_info *cinfo;
> +       int ret;
> +
> +       match_data = of_device_get_match_data(&pdev->dev);

Use device_get_match_data() instead.

> +       if (!match_data)
> +               return 1;
> +
> +       cinfo = devm_kzalloc(&pdev->dev, sizeof(*cinfo), GFP_KERNEL);
> +       if (!cinfo)
> +               return -ENOMEM;
> +
> +       platform_set_drvdata(pdev, cinfo);
> +
> +       cinfo->dev = &pdev->dev;
> +
> +       cinfo->clk_desc = &match_data->desc;
> +       cinfo->hw.init = &match_data->init;
> +       ret = devm_clk_hw_register(&pdev->dev, &cinfo->hw);
> +       if (ret) {
> +               dev_err(&pdev->dev, "Fail to register clock %s, %d\n",
> +                       match_data->desc.clk_name, ret);
> +               return ret;
> +       }
> +
> +       ret = of_clk_add_hw_provider(node, of_clk_hw_simple_get, &cinfo->hw);

Use devm?

> +       if (ret < 0)
> +               dev_err(&pdev->dev, "Fail to add clock driver, %d\n", ret);
> +       return ret;
> +}
> +
> +static void twl_clks_remove(struct platform_device *pdev)
> +{
> +       of_clk_del_provider(pdev->dev.of_node);
> +}

And get rid of this entirely?

> +
> +static struct platform_driver twl_clks_driver = {
> +       .driver = {
> +               .name = "twl-clk",
> +               .of_match_table = twl_clks_of_match,
> +       },
> +       .probe = twl_clks_probe,
> +       .remove_new = twl_clks_remove,
> +};
> +
> +module_platform_driver(twl_clks_driver);
> +
> +MODULE_DESCRIPTION("Clock driver for TWL Series Devices");
> +MODULE_ALIAS("platform:twl-clk");

This alias is unnecessary?

> +MODULE_LICENSE("GPL");

Re: [PATCH 2/3] clk: twl: add clock driver for TWL6032

[Andreas Kemnade]

On Tue, 22 Aug 2023 15:34:59 -0700
Stephen Boyd <sboyd@kernel.org> wrote:

> Quoting Andreas Kemnade (2023-08-19 06:41:46)
> > diff --git a/drivers/clk/clk-twl.c b/drivers/clk/clk-twl.c
> > new file mode 100644
> > index 0000000000000..deb5742393bac
> > --- /dev/null
> > +++ b/drivers/clk/clk-twl.c
[...]
> > +
> > +static struct platform_driver twl_clks_driver = {
> > +       .driver = {
> > +               .name = "twl-clk",
> > +               .of_match_table = twl_clks_of_match,
> > +       },
> > +       .probe = twl_clks_probe,
> > +       .remove_new = twl_clks_remove,
> > +};
> > +
> > +module_platform_driver(twl_clks_driver);
> > +
> > +MODULE_DESCRIPTION("Clock driver for TWL Series Devices");
> > +MODULE_ALIAS("platform:twl-clk");  
> 
> This alias is unnecessary?
> 
The question is whether this driver should have a separate dt
node (and if a separate node, then one per clock as the clk-palmas
driver) or not. See Rob's review of the binding document.
So we have basically #clock-cells = <1>; in the twl parent
and a call to mfd_add_device() there in the former case and I guess
that alias is needed then.

But if the overall structure stays as in this version,
then I doubt that we need that alias.

Regards,
Andreas

Re: [PATCH 2/3] clk: twl: add clock driver for TWL6032

[Krzysztof Kozlowski]

On 23/08/2023 16:51, Andreas Kemnade wrote:
> On Tue, 22 Aug 2023 15:34:59 -0700
> Stephen Boyd <sboyd@kernel.org> wrote:
> 
>> Quoting Andreas Kemnade (2023-08-19 06:41:46)
>>> diff --git a/drivers/clk/clk-twl.c b/drivers/clk/clk-twl.c
>>> new file mode 100644
>>> index 0000000000000..deb5742393bac
>>> --- /dev/null
>>> +++ b/drivers/clk/clk-twl.c
> [...]
>>> +
>>> +static struct platform_driver twl_clks_driver = {
>>> +       .driver = {
>>> +               .name = "twl-clk",
>>> +               .of_match_table = twl_clks_of_match,
>>> +       },
>>> +       .probe = twl_clks_probe,
>>> +       .remove_new = twl_clks_remove,
>>> +};
>>> +
>>> +module_platform_driver(twl_clks_driver);
>>> +
>>> +MODULE_DESCRIPTION("Clock driver for TWL Series Devices");
>>> +MODULE_ALIAS("platform:twl-clk");  
>>
>> This alias is unnecessary?
>>
> The question is whether this driver should have a separate dt
> node (and if a separate node, then one per clock as the clk-palmas
> driver) or not. See Rob's review of the binding document.
> So we have basically #clock-cells = <1>; in the twl parent
> and a call to mfd_add_device() there in the former case and I guess
> that alias is needed then.
> 

You should not need the alias in any of these cases. platform alias for
platform driver means you have incomplete tables and use alias instead
of tables. Preference is to use tables.

Best regards,
Krzysztof

Re: [PATCH 2/3] clk: twl: add clock driver for TWL6032

[Andreas Kemnade]

Hi,

On Thu, 24 Aug 2023 09:04:28 +0200
Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:

> >>> +
> >>> +MODULE_DESCRIPTION("Clock driver for TWL Series Devices");
> >>> +MODULE_ALIAS("platform:twl-clk");    
> >>
> >> This alias is unnecessary?
> >>  
> > The question is whether this driver should have a separate dt
> > node (and if a separate node, then one per clock as the clk-palmas
> > driver) or not. See Rob's review of the binding document.
> > So we have basically #clock-cells = <1>; in the twl parent
> > and a call to mfd_add_device() there in the former case and I guess
> > that alias is needed then.
> >   
> 
> You should not need the alias in any of these cases. platform alias for
> platform driver means you have incomplete tables and use alias instead
> of tables. Preference is to use tables.

Is there a general consensus that MODULE_ALIAS("platform:.*") should be
exorcised? Of course for this new driver I will avoid it now anyways.

Regards,
Andreas

Re: [PATCH 2/3] clk: twl: add clock driver for TWL6032

[Krzysztof Kozlowski]

On 28/08/2023 18:24, Andreas Kemnade wrote:
> Hi,
> 
> On Thu, 24 Aug 2023 09:04:28 +0200
> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
> 
>>>>> +
>>>>> +MODULE_DESCRIPTION("Clock driver for TWL Series Devices");
>>>>> +MODULE_ALIAS("platform:twl-clk");    
>>>>
>>>> This alias is unnecessary?
>>>>  
>>> The question is whether this driver should have a separate dt
>>> node (and if a separate node, then one per clock as the clk-palmas
>>> driver) or not. See Rob's review of the binding document.
>>> So we have basically #clock-cells = <1>; in the twl parent
>>> and a call to mfd_add_device() there in the former case and I guess
>>> that alias is needed then.
>>>   
>>
>> You should not need the alias in any of these cases. platform alias for
>> platform driver means you have incomplete tables and use alias instead
>> of tables. Preference is to use tables.
> 
> Is there a general consensus that MODULE_ALIAS("platform:.*") should be
> exorcised? Of course for this new driver I will avoid it now anyways.

Whether "general" I don't know, but I was removing it quite a lot in the
past. I think I removed all at some point, now I guess we have them back :/.

MODULE_ALIAS is not the correct way to solve module matching problem. ID
table with the correct way. Alias is just a workaround which now works,
but later might stop (e.g. ID table will come with additional features).

Best regards,
Krzysztof

[PATCH 3/3] ARM: dts: omap4-embt2ws: enable 32K clock on WLAN

[Andreas Kemnade]

WLAN did only work if clock was left enabled by the original system,
so make it fully enable the needed resources itself.

Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
---
 arch/arm/boot/dts/ti/omap/omap4-epson-embt2ws.dts | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/arch/arm/boot/dts/ti/omap/omap4-epson-embt2ws.dts b/arch/arm/boot/dts/ti/omap/omap4-epson-embt2ws.dts
index ee86981b2e448..9ca540ef86265 100644
--- a/arch/arm/boot/dts/ti/omap/omap4-epson-embt2ws.dts
+++ b/arch/arm/boot/dts/ti/omap/omap4-epson-embt2ws.dts
@@ -69,6 +69,12 @@ unknown_supply: unknown-supply {
 		regulator-name = "unknown";
 	};
 
+	wl12xx_pwrseq: wl12xx-pwrseq {
+		compatible = "mmc-pwrseq-simple";
+		clocks = <&clk32kaudio>;
+		clock-names = "ext_clock";
+	};
+
        /* regulator for wl12xx on sdio2 */
 	wl12xx_vmmc: wl12xx-vmmc {
 		pinctrl-names = "default";
@@ -97,6 +103,11 @@ twl: pmic@48 {
 		interrupt-controller;
 		#interrupt-cells = <1>;
 
+		clk32kaudio: clk-32kaudio {
+			compatible = "ti,twl6032-clk32kaudio";
+			#clock-cells = <0>;
+		};
+
 		rtc {
 			compatible = "ti,twl4030-rtc";
 			interrupts = <11>;
@@ -316,6 +327,7 @@ &mmc3 {
 	pinctrl-names = "default";
 	pinctrl-0 = <&wl12xx_pins>;
 	vmmc-supply = <&wl12xx_vmmc>;
+	mmc-pwrseq = <&wl12xx_pwrseq>;
 	interrupts-extended = <&wakeupgen GIC_SPI 94 IRQ_TYPE_LEVEL_HIGH
 			       &omap4_pmx_core 0x12e>;
 	non-removable;
-- 
2.39.2