Re: [PATCH] mfd: Support SiRF audio modules

Re: [PATCH] mfd: Support SiRF audio modules

[Lee Jones]

CC'ing the DT ML, please don't forget in future.

> From: Rongjun Ying <rongjun.ying-kQvG35nSl+M@public.gmane.org>
> 
> The audio modules inside the consist of an internal audio codec, internal
> audio port and i2s controller.
> 
> These modules sharing same register range and size.
> sirf-audio.c provides common regmap mmio handling for all audio modules.
> The sound drivers operation are via regmap.
> 
> Signed-off-by: Rongjun Ying <rongjun.ying-kQvG35nSl+M@public.gmane.org>
> ---
>  .../devicetree/bindings/mfd/sirf-audio.txt         |   67 +++++++++++

DT documentation should be provided in a separate patch.

>  drivers/mfd/Kconfig                                |   11 ++
>  drivers/mfd/Makefile                               |    1 +
>  drivers/mfd/sirf-audio.c                           |  119 ++++++++++++++++++++
>  include/linux/mfd/sirf/audio.h                     |   17 +++
>  5 files changed, 215 insertions(+), 0 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/mfd/sirf-audio.txt
>  create mode 100644 drivers/mfd/sirf-audio.c
>  create mode 100644 include/linux/mfd/sirf/audio.h
> 
> diff --git a/Documentation/devicetree/bindings/mfd/sirf-audio.txt b/Documentation/devicetree/bindings/mfd/sirf-audio.txt
> new file mode 100644
> index 0000000..cc6a39b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/sirf-audio.txt
> @@ -0,0 +1,67 @@
> +SiRF SoC audio modules
> +
> +The audio modules inside the consist of an internal audio codec, internal

... inside the <what?>

> +audio port and i2s controller.
> +
> +Required properties:
> +- compatible: "sirf,prima2-audio" or "sirf,atlas6-audio"
> +- reg : the register address of the device.

Nit: s/the/The

> +- #address-cells: Must be 1
> +- #size-cells: Must be 1
> +
> +Nodes:
> +Internal audio codec:
> +  - compatible : "sirf,atlas6-audio-codec" or "sirf,prima2-audio-codec"
> +  - clocks: the clock of SiRF internal audio codec

Nit: s/the/The

Might be worth mentioning that clocks expects a phandle.

> +Internal audio port:
> +- compatible: "sirf,audio-port"
> +- dmas: List of DMA controller phandle and DMA request line ordered pairs.

s/phandle/phandles

> +- dma-names: Identifier string for each DMA request line in the dmas property.
> +  These strings correspond 1:1 with the ordered pairs in dmas.
> +
> +  One of the DMA channels will be responsible for transmission (should be
> +  named "tx") and one for reception (should be named "rx").
> +
> +I2S controller:
> +- compatible: "sirf,prima2-i2s"
> +- dmas: List of DMA controller phandle and DMA request line ordered pairs.

s/phandle/phandles

> +- dma-names: Identifier string for each DMA request line in the dmas property.
> +  These strings correspond 1:1 with the ordered pairs in dmas.
> +
> +  One of the DMA channels will be responsible for transmission (should be
> +  named "tx") and one for reception (should be named "rx").

No need to keep mentioning these same lines over and over - just point
to:
  Documentation/devicetree/bindings/dma/dma.txt

> +- clocks: I2S controller clock source
> +- pinctrl-names: Must contain a "default" entry.
> +- pinctrl-NNN: One property must exist for each entry in pinctrl-names.

Again, rather than making up your own interpretation of the standard
pinctrl properties, just point to:
  Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt

Full stops and colons are not consistent throughout.

> +Example:
> +
> +sirfaudio: sirfaudio@b0040000 {
> +	compatible = "sirf,atlas6-audio";
> +	reg = <0xb0040000 0x10000>;
> +	#address-cells = <1>;
> +	#size-cells = <1>;
> +
> +	audiocodec: audiocodec@b0040000 {
> +		compatible = "sirf,atlas6-audio-codec";
> +		interrupts = <35>;

No flags?

> +		clocks = <&clks 27>;
> +	};
> +
> +	audioport: audioport@b0040000 {

No need for the @b0040000 on child properties.

> +		compatible = "sirf,audio-port";
> +		dmas = <&dmac1 3>, <&dmac1 8>;
> +		dma-names = "rx", "tx";
> +	};
> +
> +	i2s: i2s@b0040000 {
> +		compatible = "sirf,prima2-i2s";
> +		dmas = <&dmac1 6>, <&dmac1 12>;
> +		dma-names = "rx", "tx";
> +		clocks = <&clks 27>;
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&i2s_pins_a>;
> +	};
> +};
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index b7c74a7..1c13019 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -546,6 +546,17 @@ config MFD_SI476X_CORE
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called si476x-core.
>  
> +config MFD_SIRF_AUDIO
> +	tristate "SiRF Audio modules"
> +	depends on ARCH_SIRF

depends on OF

> +	select MFD_CORE
> +	select REGMAP_MMIO
> +	help
> +	  This is the driver for the SiRF audio modules, include I2S,

s/include/which consists of (or similar)

> +	  internal audio codec, internal audio port and AC97. These modules
> +	  share same register map region and size. So this MFD driver used

s/same/the same

s/register map region and size/address space

s/used/is used

> +	  to manage regmap.

s/regmap/the regmap OR via regmap

> +
>  config MFD_SM501
>  	tristate "Silicon Motion SM501"
>  	 ---help---
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 8a28dc9..9182170 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -164,3 +164,4 @@ obj-$(CONFIG_MFD_RETU)		+= retu-mfd.o
>  obj-$(CONFIG_MFD_AS3711)	+= as3711.o
>  obj-$(CONFIG_MFD_AS3722)	+= as3722.o
>  obj-$(CONFIG_MFD_STW481X)	+= stw481x.o
> +obj-$(CONFIG_MFD_SIRF_AUDIO)	+= sirf-audio.o
> diff --git a/drivers/mfd/sirf-audio.c b/drivers/mfd/sirf-audio.c
> new file mode 100644
> index 0000000..078f0a2
> --- /dev/null
> +++ b/drivers/mfd/sirf-audio.c
> @@ -0,0 +1,119 @@
> +/*
> + * airf-audio.c
> + *
> + * Copyright (c) 2014 Cambridge Silicon Radio Limited, a CSR plc group company.
> + *
> + * Licensed under GPLv2 or later.
> + */

Please use a more standard header. There are plenty of good examples
in other drivers.

> +#include <linux/module.h>
> +#include <linux/moduleparam.h>
> +#include <linux/init.h>
> +#include <linux/err.h>
> +#include <linux/of.h>
> +#include <linux/mfd/core.h>
> +#include <linux/mfd/sirf/audio.h>
> +#include <linux/regmap.h>
> +
> +static const struct mfd_cell sirf_audio_atlas6_devs[] = {
> +	{
> +		.of_compatible = "sirf,prima2-i2s",
> +		.name = "sirf-i2s",
> +	}, {
> +		.of_compatible = "sirf,atlas6-audio-codec",
> +		.name = "sirf-audio-codec",
> +	}, {
> +		.of_compatible = "sirf,audio-port",
> +		.name = "sirf-audio-port",
> +	}
> +};
> +
> +static const struct mfd_cell sirf_audio_prima2_devs[] = {
> +	{
> +		.of_compatible = "sirf,prima2-i2s",
> +		.name = "sirf-i2s",
> +	}, {
> +		.of_compatible = "sirf,prima2-audio-codec",
> +		.name = "sirf-audio-codec",
> +	}, {
> +		.of_compatible = "sirf,audio-port",
> +		.name = "sirf-audio-port",
> +	}
> +};
> +
> +static const struct regmap_config sirf_audio_regmap_config = {
> +	.reg_bits = 32,
> +	.reg_stride = 4,
> +	.val_bits = 32,
> +	.cache_type = REGCACHE_NONE,
> +};
> +
> +static int sirf_audio_probe(struct platform_device *pdev)
> +{
> +	struct resource *mem_res;
> +	struct sirf_audio_dev *sirf_audio;
> +	void __iomem *base;
> +	struct mfd_cell *cell;
> +	int cell_number;
> +
> +	sirf_audio = devm_kzalloc(&pdev->dev, sizeof(struct sirf_audio_dev),

sizeof(*sirf_audio)

> +				GFP_KERNEL);
> +	if (sirf_audio == NULL)

if (!sirf_audio)

> +		return -ENOMEM;
> +
> +	mem_res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	base = devm_ioremap_resource(&pdev->dev, mem_res);
> +	if (base == NULL)

if (!base)

> +		return -ENOMEM;
> +
> +	sirf_audio->regmap = devm_regmap_init_mmio(&pdev->dev, base,
> +					    &sirf_audio_regmap_config);
> +	if (IS_ERR(sirf_audio->regmap))
> +		return PTR_ERR(sirf_audio->regmap);

Use a local regmap variable here and assign it to sirf_audio->regmap
if successful.

> +	platform_set_drvdata(pdev, sirf_audio);
> +
> +	if (of_device_is_compatible(pdev->dev.of_node,
> +			"sirf,prima2-audio")) {
> +		cell = sirf_audio_prima2_devs;
> +		cell_number = ARRAY_SIZE(sirf_audio_prima2_devs);

s/cell_number/n_devs

> +	} else if (of_device_is_compatible(pdev->dev.of_node,
> +			"sirf,atlas6-audio")) {
> +		cell = sirf_audio_atlas6_devs;
> +		cell_number = ARRAY_SIZE(sirf_audio_atlas6_devs);

s/cell_number/n_devs

> +	} else
> +		return -EINVAL;

I'm not sure there's any need for this. Why don't you just parse the
child nodes? Are you even sure you need an MFD at all? It appears
you're just using an MFD to share a regmap. Seems like over-kill to me.

> +	return mfd_add_devices(&pdev->dev, -1, cell, cell_number,
> +			NULL, 0, NULL);
> +}
> +
> +static int sirf_audio_remove(struct platform_device *pdev)
> +{
> +	mfd_remove_devices(&pdev->dev);
> +	return 0;
> +}
> +
> +static const struct of_device_id sirf_audio_of_match[] = {
> +	{ .compatible = "sirf,prima2-audio", },
> +	{ .compatible = "sirf,atlas6-audio", },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, sirf_audio_of_match);
> +
> +static struct platform_driver sirf_audio_driver = {
> +	.driver = {
> +		.name = "sirf-audio",
> +		.owner = THIS_MODULE,
> +		.of_match_table = sirf_audio_of_match,
> +	},
> +	.probe = sirf_audio_probe,
> +	.remove = sirf_audio_remove,
> +};
> +
> +module_platform_driver(sirf_audio_driver);
> +
> +MODULE_DESCRIPTION("SiRF Audio mfd driver");

s/mfd/MFD

> +MODULE_AUTHOR("RongJun Ying <Rongjun.Ying-kQvG35nSl+M@public.gmane.org>");
> +MODULE_LICENSE("GPL v2");
> +
> diff --git a/include/linux/mfd/sirf/audio.h b/include/linux/mfd/sirf/audio.h
> new file mode 100644
> index 0000000..d8cfff9
> --- /dev/null
> +++ b/include/linux/mfd/sirf/audio.h
> @@ -0,0 +1,17 @@
> +/*
> + * audio.h
> + *
> + * Copyright (c) 2014 Cambridge Silicon Radio Limited, a CSR plc group company.
> + *
> + * Licensed under GPLv2 or later.
> + */

Use a standard header please.

> +
> +

Superfluous new lines.

> +#ifndef __LINUX_MFD_SIRF_AUDIO_H
> +#define __LINUX_MFD_SIRF_AUDIO_H

The LINUX_ is superfluous also.

> +struct sirf_audio_dev {
> +	struct regmap *regmap;
> +};

Is this going to be utilised in the subordinate drivers?

> +#endif /* __LINUX_MFD_SIRF_AUDIO_H */

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] mfd: Support SiRF audio modules

[RongJun Ying]

2014-03-10 18:35 GMT+08:00 Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>:
> CC'ing the DT ML, please don't forget in future.
>
>> From: Rongjun Ying <rongjun.ying-kQvG35nSl+M@public.gmane.org>
>>
>> The audio modules inside the consist of an internal audio codec, internal
>> audio port and i2s controller.
>>
>> These modules sharing same register range and size.
>> sirf-audio.c provides common regmap mmio handling for all audio modules.
>> The sound drivers operation are via regmap.
>>
>> Signed-off-by: Rongjun Ying <rongjun.ying-kQvG35nSl+M@public.gmane.org>
>> ---
>>  .../devicetree/bindings/mfd/sirf-audio.txt         |   67 +++++++++++
>
> DT documentation should be provided in a separate patch.
>
>>  drivers/mfd/Kconfig                                |   11 ++
>>  drivers/mfd/Makefile                               |    1 +
>>  drivers/mfd/sirf-audio.c                           |  119 ++++++++++++++++++++
>>  include/linux/mfd/sirf/audio.h                     |   17 +++
>>  5 files changed, 215 insertions(+), 0 deletions(-)
>>  create mode 100644 Documentation/devicetree/bindings/mfd/sirf-audio.txt
>>  create mode 100644 drivers/mfd/sirf-audio.c
>>  create mode 100644 include/linux/mfd/sirf/audio.h
>>
>> diff --git a/Documentation/devicetree/bindings/mfd/sirf-audio.txt b/Documentation/devicetree/bindings/mfd/sirf-audio.txt
>> new file mode 100644
>> index 0000000..cc6a39b
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/mfd/sirf-audio.txt
>> @@ -0,0 +1,67 @@
>> +SiRF SoC audio modules
>> +
>> +The audio modules inside the consist of an internal audio codec, internal
>
> ... inside the <what?>
>
>> +audio port and i2s controller.
>> +
>> +Required properties:
>> +- compatible: "sirf,prima2-audio" or "sirf,atlas6-audio"
>> +- reg : the register address of the device.
>
> Nit: s/the/The
>
>> +- #address-cells: Must be 1
>> +- #size-cells: Must be 1
>> +
>> +Nodes:
>> +Internal audio codec:
>> +  - compatible : "sirf,atlas6-audio-codec" or "sirf,prima2-audio-codec"
>> +  - clocks: the clock of SiRF internal audio codec
>
> Nit: s/the/The
>
> Might be worth mentioning that clocks expects a phandle.
>
>> +Internal audio port:
>> +- compatible: "sirf,audio-port"
>> +- dmas: List of DMA controller phandle and DMA request line ordered pairs.
>
> s/phandle/phandles
>
>> +- dma-names: Identifier string for each DMA request line in the dmas property.
>> +  These strings correspond 1:1 with the ordered pairs in dmas.
>> +
>> +  One of the DMA channels will be responsible for transmission (should be
>> +  named "tx") and one for reception (should be named "rx").
>> +
>> +I2S controller:
>> +- compatible: "sirf,prima2-i2s"
>> +- dmas: List of DMA controller phandle and DMA request line ordered pairs.
>
> s/phandle/phandles
>
>> +- dma-names: Identifier string for each DMA request line in the dmas property.
>> +  These strings correspond 1:1 with the ordered pairs in dmas.
>> +
>> +  One of the DMA channels will be responsible for transmission (should be
>> +  named "tx") and one for reception (should be named "rx").
>
> No need to keep mentioning these same lines over and over - just point
> to:
>   Documentation/devicetree/bindings/dma/dma.txt
>
>> +- clocks: I2S controller clock source
>> +- pinctrl-names: Must contain a "default" entry.
>> +- pinctrl-NNN: One property must exist for each entry in pinctrl-names.
>
> Again, rather than making up your own interpretation of the standard
> pinctrl properties, just point to:
>   Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
>
> Full stops and colons are not consistent throughout.
>
>> +Example:
>> +
>> +sirfaudio: sirfaudio@b0040000 {
>> +     compatible = "sirf,atlas6-audio";
>> +     reg = <0xb0040000 0x10000>;
>> +     #address-cells = <1>;
>> +     #size-cells = <1>;
>> +
>> +     audiocodec: audiocodec@b0040000 {
>> +             compatible = "sirf,atlas6-audio-codec";
>> +             interrupts = <35>;
>
> No flags?
>
>> +             clocks = <&clks 27>;
>> +     };
>> +
>> +     audioport: audioport@b0040000 {
>
> No need for the @b0040000 on child properties.
>
>> +             compatible = "sirf,audio-port";
>> +             dmas = <&dmac1 3>, <&dmac1 8>;
>> +             dma-names = "rx", "tx";
>> +     };
>> +
>> +     i2s: i2s@b0040000 {
>> +             compatible = "sirf,prima2-i2s";
>> +             dmas = <&dmac1 6>, <&dmac1 12>;
>> +             dma-names = "rx", "tx";
>> +             clocks = <&clks 27>;
>> +             pinctrl-names = "default";
>> +             pinctrl-0 = <&i2s_pins_a>;
>> +     };
>> +};
>> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
>> index b7c74a7..1c13019 100644
>> --- a/drivers/mfd/Kconfig
>> +++ b/drivers/mfd/Kconfig
>> @@ -546,6 +546,17 @@ config MFD_SI476X_CORE
>>         To compile this driver as a module, choose M here: the
>>         module will be called si476x-core.
>>
>> +config MFD_SIRF_AUDIO
>> +     tristate "SiRF Audio modules"
>> +     depends on ARCH_SIRF
>
> depends on OF
>
>> +     select MFD_CORE
>> +     select REGMAP_MMIO
>> +     help
>> +       This is the driver for the SiRF audio modules, include I2S,
>
> s/include/which consists of (or similar)
>
>> +       internal audio codec, internal audio port and AC97. These modules
>> +       share same register map region and size. So this MFD driver used
>
> s/same/the same
>
> s/register map region and size/address space
>
> s/used/is used
>
>> +       to manage regmap.
>
> s/regmap/the regmap OR via regmap
>
>> +
>>  config MFD_SM501
>>       tristate "Silicon Motion SM501"
>>        ---help---
>> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
>> index 8a28dc9..9182170 100644
>> --- a/drivers/mfd/Makefile
>> +++ b/drivers/mfd/Makefile
>> @@ -164,3 +164,4 @@ obj-$(CONFIG_MFD_RETU)            += retu-mfd.o
>>  obj-$(CONFIG_MFD_AS3711)     += as3711.o
>>  obj-$(CONFIG_MFD_AS3722)     += as3722.o
>>  obj-$(CONFIG_MFD_STW481X)    += stw481x.o
>> +obj-$(CONFIG_MFD_SIRF_AUDIO) += sirf-audio.o
>> diff --git a/drivers/mfd/sirf-audio.c b/drivers/mfd/sirf-audio.c
>> new file mode 100644
>> index 0000000..078f0a2
>> --- /dev/null
>> +++ b/drivers/mfd/sirf-audio.c
>> @@ -0,0 +1,119 @@
>> +/*
>> + * airf-audio.c
>> + *
>> + * Copyright (c) 2014 Cambridge Silicon Radio Limited, a CSR plc group company.
>> + *
>> + * Licensed under GPLv2 or later.
>> + */
>
> Please use a more standard header. There are plenty of good examples
> in other drivers.
>
>> +#include <linux/module.h>
>> +#include <linux/moduleparam.h>
>> +#include <linux/init.h>
>> +#include <linux/err.h>
>> +#include <linux/of.h>
>> +#include <linux/mfd/core.h>
>> +#include <linux/mfd/sirf/audio.h>
>> +#include <linux/regmap.h>
>> +
>> +static const struct mfd_cell sirf_audio_atlas6_devs[] = {
>> +     {
>> +             .of_compatible = "sirf,prima2-i2s",
>> +             .name = "sirf-i2s",
>> +     }, {
>> +             .of_compatible = "sirf,atlas6-audio-codec",
>> +             .name = "sirf-audio-codec",
>> +     }, {
>> +             .of_compatible = "sirf,audio-port",
>> +             .name = "sirf-audio-port",
>> +     }
>> +};
>> +
>> +static const struct mfd_cell sirf_audio_prima2_devs[] = {
>> +     {
>> +             .of_compatible = "sirf,prima2-i2s",
>> +             .name = "sirf-i2s",
>> +     }, {
>> +             .of_compatible = "sirf,prima2-audio-codec",
>> +             .name = "sirf-audio-codec",
>> +     }, {
>> +             .of_compatible = "sirf,audio-port",
>> +             .name = "sirf-audio-port",
>> +     }
>> +};
>> +
>> +static const struct regmap_config sirf_audio_regmap_config = {
>> +     .reg_bits = 32,
>> +     .reg_stride = 4,
>> +     .val_bits = 32,
>> +     .cache_type = REGCACHE_NONE,
>> +};
>> +
>> +static int sirf_audio_probe(struct platform_device *pdev)
>> +{
>> +     struct resource *mem_res;
>> +     struct sirf_audio_dev *sirf_audio;
>> +     void __iomem *base;
>> +     struct mfd_cell *cell;
>> +     int cell_number;
>> +
>> +     sirf_audio = devm_kzalloc(&pdev->dev, sizeof(struct sirf_audio_dev),
>
> sizeof(*sirf_audio)
>
>> +                             GFP_KERNEL);
>> +     if (sirf_audio == NULL)
>
> if (!sirf_audio)
>
>> +             return -ENOMEM;
>> +
>> +     mem_res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +     base = devm_ioremap_resource(&pdev->dev, mem_res);
>> +     if (base == NULL)
>
> if (!base)
>
>> +             return -ENOMEM;
>> +
>> +     sirf_audio->regmap = devm_regmap_init_mmio(&pdev->dev, base,
>> +                                         &sirf_audio_regmap_config);
>> +     if (IS_ERR(sirf_audio->regmap))
>> +             return PTR_ERR(sirf_audio->regmap);
>
> Use a local regmap variable here and assign it to sirf_audio->regmap
> if successful.
>
>> +     platform_set_drvdata(pdev, sirf_audio);
>> +
>> +     if (of_device_is_compatible(pdev->dev.of_node,
>> +                     "sirf,prima2-audio")) {
>> +             cell = sirf_audio_prima2_devs;
>> +             cell_number = ARRAY_SIZE(sirf_audio_prima2_devs);
>
> s/cell_number/n_devs
>
>> +     } else if (of_device_is_compatible(pdev->dev.of_node,
>> +                     "sirf,atlas6-audio")) {
>> +             cell = sirf_audio_atlas6_devs;
>> +             cell_number = ARRAY_SIZE(sirf_audio_atlas6_devs);
>
> s/cell_number/n_devs
>
>> +     } else
>> +             return -EINVAL;
>
> I'm not sure there's any need for this. Why don't you just parse the
> child nodes? Are you even sure you need an MFD at all? It appears
> you're just using an MFD to share a regmap. Seems like over-kill to me.

In my case, the prima2 and atlas6 codec have small different. But
they use the same register address space.
The sound codec driver need know what codec device is applied.

>
>> +     return mfd_add_devices(&pdev->dev, -1, cell, cell_number,
>> +                     NULL, 0, NULL);
>> +}
>> +
>> +static int sirf_audio_remove(struct platform_device *pdev)
>> +{
>> +     mfd_remove_devices(&pdev->dev);
>> +     return 0;
>> +}
>> +
>> +static const struct of_device_id sirf_audio_of_match[] = {
>> +     { .compatible = "sirf,prima2-audio", },
>> +     { .compatible = "sirf,atlas6-audio", },
>> +     {}
>> +};
>> +MODULE_DEVICE_TABLE(of, sirf_audio_of_match);
>> +
>> +static struct platform_driver sirf_audio_driver = {
>> +     .driver = {
>> +             .name = "sirf-audio",
>> +             .owner = THIS_MODULE,
>> +             .of_match_table = sirf_audio_of_match,
>> +     },
>> +     .probe = sirf_audio_probe,
>> +     .remove = sirf_audio_remove,
>> +};
>> +
>> +module_platform_driver(sirf_audio_driver);
>> +
>> +MODULE_DESCRIPTION("SiRF Audio mfd driver");
>
> s/mfd/MFD
>
>> +MODULE_AUTHOR("RongJun Ying <Rongjun.Ying-kQvG35nSl+M@public.gmane.org>");
>> +MODULE_LICENSE("GPL v2");
>> +
>> diff --git a/include/linux/mfd/sirf/audio.h b/include/linux/mfd/sirf/audio.h
>> new file mode 100644
>> index 0000000..d8cfff9
>> --- /dev/null
>> +++ b/include/linux/mfd/sirf/audio.h
>> @@ -0,0 +1,17 @@
>> +/*
>> + * audio.h
>> + *
>> + * Copyright (c) 2014 Cambridge Silicon Radio Limited, a CSR plc group company.
>> + *
>> + * Licensed under GPLv2 or later.
>> + */
>
> Use a standard header please.
>
>> +
>> +
>
> Superfluous new lines.
>
>> +#ifndef __LINUX_MFD_SIRF_AUDIO_H
>> +#define __LINUX_MFD_SIRF_AUDIO_H
>
> The LINUX_ is superfluous also.
>
>> +struct sirf_audio_dev {
>> +     struct regmap *regmap;
>> +};
>
> Is this going to be utilised in the subordinate drivers?

Yes, The subordinate drivers such as I2S, SiRF internal audio codec and port,
which will used this regmap to shared the same register address space.
The sirf_audio_dev object is the parent driver data, and child drivers
will get it.

>
>> +#endif /* __LINUX_MFD_SIRF_AUDIO_H */
>
> --
> Lee Jones
> Linaro STMicroelectronics Landing Team Lead
> Linaro.org │ Open source software for ARM SoCs
> Follow Linaro: Facebook | Twitter | Blog


Thanks
-- 
------------------------------
Rongjun Ying
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] mfd: Support SiRF audio modules

[Lee Jones]

> >> +     if (of_device_is_compatible(pdev->dev.of_node,
> >> +                     "sirf,prima2-audio")) {
> >> +             cell = sirf_audio_prima2_devs;
> >> +             cell_number = ARRAY_SIZE(sirf_audio_prima2_devs);
> >
> > s/cell_number/n_devs
> >
> >> +     } else if (of_device_is_compatible(pdev->dev.of_node,
> >> +                     "sirf,atlas6-audio")) {
> >> +             cell = sirf_audio_atlas6_devs;
> >> +             cell_number = ARRAY_SIZE(sirf_audio_atlas6_devs);
> >
> > s/cell_number/n_devs
> >
> >> +     } else
> >> +             return -EINVAL;
> >
> > I'm not sure there's any need for this. Why don't you just parse the
> > child nodes? Are you even sure you need an MFD at all? It appears
> > you're just using an MFD to share a regmap. Seems like over-kill to me.
> 
> In my case, the prima2 and atlas6 codec have small different. But
> they use the same register address space.
> The sound codec driver need know what codec device is applied.

Mark,

I understand it was you who made the original request for the regmap
to be shared between devices. Was it an MFD that you had in mind?

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] mfd: Support SiRF audio modules

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 866 bytes --]

On Mon, Mar 10, 2014 at 04:31:58PM +0000, Lee Jones wrote:

> > > I'm not sure there's any need for this. Why don't you just parse the
> > > child nodes? Are you even sure you need an MFD at all? It appears
> > > you're just using an MFD to share a regmap. Seems like over-kill to me.

> > In my case, the prima2 and atlas6 codec have small different. But
> > they use the same register address space.
> > The sound codec driver need know what codec device is applied.

> I understand it was you who made the original request for the regmap
> to be shared between devices. Was it an MFD that you had in mind?

I was expecting an audio driver that registered several interfaces with
the ASoC core rather than a MFD.  It's possible there's some good reason
for doing things this way, I've not looked at the new patches (I only
seem to have one for the MFD itself...).

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] mfd: Support SiRF audio modules

[RongJun Ying]

2014-03-11 0:48 GMT+08:00 Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>:
> On Mon, Mar 10, 2014 at 04:31:58PM +0000, Lee Jones wrote:
>
>> > > I'm not sure there's any need for this. Why don't you just parse the
>> > > child nodes? Are you even sure you need an MFD at all? It appears
>> > > you're just using an MFD to share a regmap. Seems like over-kill to me.
>
>> > In my case, the prima2 and atlas6 codec have small different. But
>> > they use the same register address space.
>> > The sound codec driver need know what codec device is applied.
>
>> I understand it was you who made the original request for the regmap
>> to be shared between devices. Was it an MFD that you had in mind?
>
> I was expecting an audio driver that registered several interfaces with
> the ASoC core rather than a MFD.  It's possible there's some good reason
> for doing things this way, I've not looked at the new patches (I only
> seem to have one for the MFD itself...).

I think the MFD driver is better than the ASoC core. Because the mfd_add_devices
can register all child device into system and the parent driver data
can be got the regmap
to all child drivers.

Hi Mark Brown:
I will send new patches of sound, if the MFD driver is applied.

Thanks.
-- 
------------------------------
Rongjun Ying
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] mfd: Support SiRF audio modules

[Barry Song]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1394 bytes --]

> > diff --git a/include/linux/mfd/sirf/audio.h b/include/linux/mfd/sirf/audio.h
> > new file mode 100644
> > index 0000000..d8cfff9
> > --- /dev/null
> > +++ b/include/linux/mfd/sirf/audio.h
> > @@ -0,0 +1,17 @@
> > +/*
> > + * audio.h
> > + *
> > + * Copyright (c) 2014 Cambridge Silicon Radio Limited, a CSR plc group
> company.
> > + *
> > + * Licensed under GPLv2 or later.
> > + */
> 
> Use a standard header please

I think this has been a standard header. It is one simpler version for license.
If there is any problem here, it should be the 1st line, we might replace audio.h by a simple description of this file.

-barry



Member of the CSR plc group of companies. CSR plc registered in England and Wales, registered number 4187346, registered office Churchill House, Cambridge Business Park, Cowley Road, Cambridge, CB4 0WZ, United Kingdom
More information can be found at www.csr.com. Keep up to date with CSR on our technical blog, www.csr.com/blog, CSR people blog, www.csr.com/people, YouTube, www.youtube.com/user/CSRplc, Facebook, www.facebook.com/pages/CSR/191038434253534, or follow us on Twitter at www.twitter.com/CSR_plc.
New for 2014, you can now access the wide range of products powered by aptX at www.aptx.com.
N‹§²æìr¸›yúèšØb²X¬¶Ç§vØ^–)Þº{.nÇ+‰·zøœzÚÞz)í…æèw*\x1fjg¬±¨\x1e¶‰šŽŠÝ¢j.ïÛ°\½½MŽúgjÌæa×\x02››–' ™©Þ¢¸\f¢·¦j:+v‰¨ŠwèjØm¶Ÿÿ¾\a«‘êçzZ+ƒùšŽŠÝ¢j"ú!¶i

Re: [PATCH] mfd: Support SiRF audio modules

[Lee Jones]

> > > diff --git a/include/linux/mfd/sirf/audio.h b/include/linux/mfd/sirf/audio.h
> > > new file mode 100644
> > > index 0000000..d8cfff9
> > > --- /dev/null
> > > +++ b/include/linux/mfd/sirf/audio.h
> > > @@ -0,0 +1,17 @@
> > > +/*
> > > + * audio.h
> > > + *
> > > + * Copyright (c) 2014 Cambridge Silicon Radio Limited, a CSR plc group
> > company.
> > > + *
> > > + * Licensed under GPLv2 or later.
> > > + */
> > 
> > Use a standard header please
> 
> I think this has been a standard header. It is one simpler version for license.
> If there is any problem here, it should be the 1st line, we might replace audio.h by a simple description of this file.

My issue wasn't with the license, but the header in general. It
started off without a proper name for the driver followed by no
description of the code, then I noticed the author (who is listed
at the bottom) is not mentioned. My issue is that it's sparse and
uninformative.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] mfd: Support SiRF audio modules

[Barry Song]

> -----Original Message-----
> From: Lee Jones [mailto:lee.jones@linaro.org]
> Sent: Tuesday, March 11, 2014 1:49 PM
> To: Barry Song
> Cc: RongJun Ying; Rob Herring; Pawel Moll; Mark Rutland; Stephen Warren;
> Ian Campbell; Rob Landley; Samuel Ortiz; Grant Likely; DL-SHA-
> WorkGroupLinux; Rongjun Ying; devicetree@vger.kernel.org
> Subject: Re: [PATCH] mfd: Support SiRF audio modules
> 
> > > > diff --git a/include/linux/mfd/sirf/audio.h
> > > > b/include/linux/mfd/sirf/audio.h new file mode 100644 index
> > > > 0000000..d8cfff9
> > > > --- /dev/null
> > > > +++ b/include/linux/mfd/sirf/audio.h
> > > > @@ -0,0 +1,17 @@
> > > > +/*
> > > > + * audio.h
> > > > + *
> > > > + * Copyright (c) 2014 Cambridge Silicon Radio Limited, a CSR plc
> > > > +group
> > > company.
> > > > + *
> > > > + * Licensed under GPLv2 or later.
> > > > + */
> > >
> > > Use a standard header please
> >
> > I think this has been a standard header. It is one simpler version for license.
> > If there is any problem here, it should be the 1st line, we might replace
> audio.h by a simple description of this file.
> 
> My issue wasn't with the license, but the header in general. It started off
> without a proper name for the driver followed by no description of the code,
> then I noticed the author (who is listed at the bottom) is not mentioned. My
> issue is that it's sparse and uninformative.

I do think Rongjun should have a simple description instead of a "audio.h".

-barry



Member of the CSR plc group of companies. CSR plc registered in England and Wales, registered number 4187346, registered office Churchill House, Cambridge Business Park, Cowley Road, Cambridge, CB4 0WZ, United Kingdom
More information can be found at www.csr.com. Keep up to date with CSR on our technical blog, www.csr.com/blog, CSR people blog, www.csr.com/people, YouTube, www.youtube.com/user/CSRplc, Facebook, www.facebook.com/pages/CSR/191038434253534, or follow us on Twitter at www.twitter.com/CSR_plc.
New for 2014, you can now access the wide range of products powered by aptX at www.aptx.com.