Re: [PATCH v3 4/5] OMAP: hwmod data: Add dev_attr and use in the host driver

Cousson, Benoit
2011-02-24T10:59:57Z

On 2/23/2011 6:47 PM, Kadiyala, Kishore wrote:
> Add a device attribute to hwmod data of omap2430, omap3, omap4.
> Currently the device attribute holds information regarding dual volt MMC card
> support by the controller which will be later passed to the host driver via
> platform data.
>
> Signed-off-by: Kevin Hilman<khilman@deeprootsystems.com>
> Signed-off-by: Kishore Kadiyala<kishore.kadiyala@ti.com>
> Cc: Benoit Cousson<b-cousson@ti.com>
> Cc: Paul Walmsley<paul@pwsan.com>

There are few minor comments to fix hence the:
Almost-Acked-by: Benoit Cousson<b-cousson@ti.com>

> ---
> arch/arm/mach-omap2/omap_hwmod_2430_data.c | 9 +++++++++
> arch/arm/mach-omap2/omap_hwmod_3xxx_data.c | 12 ++++++++++++
> arch/arm/mach-omap2/omap_hwmod_44xx_data.c | 16 ++++++++++++++++
> arch/arm/plat-omap/include/plat/mmc.h | 10 ++++++++++
> drivers/mmc/host/omap_hsmmc.c | 4 ++--
> 5 files changed, 49 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/omap_hwmod_2430_data.c b/arch/arm/mach-omap2/omap_hwmod_2430_data.c
> index 4d45b7d..e050355 100644
> --- a/arch/arm/mach-omap2/omap_hwmod_2430_data.c
> +++ b/arch/arm/mach-omap2/omap_hwmod_2430_data.c
> @@ -19,6 +19,7 @@
> #include<plat/i2c.h>
> #include<plat/gpio.h>
> #include<plat/mcspi.h>
> +#include<plat/mmc.h>
>
> #include "omap_hwmod_common_data.h"
>
> @@ -1290,6 +1291,10 @@ static struct omap_hwmod_class mmc_class = {
>
> /* MMC/SD/SDIO1 */
>
> +static struct mmc_dev_attr mmc1_dev_attr = {

Could you rename the struct omap_mmc_dev_attr to stick to the convention?

The dev_attr should be just above "static struct omap_hwmod". See the
OMAP4 example below.

> + .flags = OMAP_HSMMC_SUPPORTS_DUAL_VOLT,
> +};
> +
> static struct omap_hwmod_irq_info mmc1_mpu_irqs[] = {
> { .irq = 83 },
> };
> @@ -1328,11 +1333,14 @@ static struct omap_hwmod omap2430_mmc1_hwmod = {
> .slaves = omap2430_mmc1_slaves,
> .slaves_cnt = ARRAY_SIZE(omap2430_mmc1_slaves),
> .class =&mmc_class,
> + .dev_attr =&mmc1_dev_attr,

dev_attr should be above .slaves.

> .omap_chip = OMAP_CHIP_INIT(CHIP_IS_OMAP2430),
> };
>
> /* MMC/SD/SDIO2 */
>
> +static struct mmc_dev_attr mmc2_dev_attr;

If you do not have any dev_attr for that instance, do not add it here
and test for null pointer in your driver.
This comment applies for all instances in all OMAPs.

> +
> static struct omap_hwmod_irq_info mmc2_mpu_irqs[] = {
> { .irq = 86 },
> };
> @@ -1371,6 +1379,7 @@ static struct omap_hwmod omap2430_mmc2_hwmod = {
> .slaves = omap2430_mmc2_slaves,
> .slaves_cnt = ARRAY_SIZE(omap2430_mmc2_slaves),
> .class =&mmc_class,
> + .dev_attr =&mmc2_dev_attr,

Not needed if there is not data in the structure.

[...]

> diff --git a/arch/arm/mach-omap2/omap_hwmod_44xx_data.c b/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
> index dd39e75..6c4ccfd 100644
> --- a/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
> +++ b/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
> @@ -25,6 +25,7 @@
> #include<plat/gpio.h>
> #include<plat/dma.h>
> #include<plat/mcspi.h>
> +#include<plat/mmc.h>
>
> #include "omap_hwmod_common_data.h"
>
> @@ -3383,6 +3384,10 @@ static struct omap_hwmod_class omap44xx_mmc_hwmod_class = {
> };
>
> /* mmc1 */
> +static struct mmc_dev_attr omap_mmc1_dev_attr = {
> + .flags = OMAP_HSMMC_SUPPORTS_DUAL_VOLT,
> +};
> +
> static struct omap_hwmod_irq_info omap44xx_mmc1_irqs[] = {
> { .irq = 83 + OMAP44XX_IRQ_GIC_START },
> };
> @@ -3437,10 +3442,14 @@ static struct omap_hwmod omap44xx_mmc1_hwmod = {
> .slaves_cnt = ARRAY_SIZE(omap44xx_mmc1_slaves),
> .masters = omap44xx_mmc1_masters,
> .masters_cnt = ARRAY_SIZE(omap44xx_mmc1_masters),
> + .dev_attr =&omap_mmc1_dev_attr,
> .omap_chip = OMAP_CHIP_INIT(CHIP_IS_OMAP4430),
> };
>
> /* mmc2 */
> +static struct omap_hwmod omap44xx_mmc2_hwmod;
> +static struct mmc_dev_attr omap_mmc2_dev_attr;
> +
> static struct omap_hwmod_irq_info omap44xx_mmc2_irqs[] = {
> { .irq = 86 + OMAP44XX_IRQ_GIC_START },
> };
> @@ -3495,11 +3504,13 @@ static struct omap_hwmod omap44xx_mmc2_hwmod = {
> .slaves_cnt = ARRAY_SIZE(omap44xx_mmc2_slaves),
> .masters = omap44xx_mmc2_masters,
> .masters_cnt = ARRAY_SIZE(omap44xx_mmc2_masters),
> + .dev_attr =&omap_mmc2_dev_attr,
> .omap_chip = OMAP_CHIP_INIT(CHIP_IS_OMAP4430),
> };
>
> /* mmc3 */
> static struct omap_hwmod omap44xx_mmc3_hwmod;
> +static struct mmc_dev_attr omap_mmc3_dev_attr;
> static struct omap_hwmod_irq_info omap44xx_mmc3_irqs[] = {
> { .irq = 94 + OMAP44XX_IRQ_GIC_START },
> };
> @@ -3547,11 +3558,13 @@ static struct omap_hwmod omap44xx_mmc3_hwmod = {
> },
> .slaves = omap44xx_mmc3_slaves,
> .slaves_cnt = ARRAY_SIZE(omap44xx_mmc3_slaves),
> + .dev_attr =&omap_mmc3_dev_attr,
> .omap_chip = OMAP_CHIP_INIT(CHIP_IS_OMAP4430),
> };
>
> /* mmc4 */
> static struct omap_hwmod omap44xx_mmc4_hwmod;
> +static struct mmc_dev_attr omap_mmc4_dev_attr;
> static struct omap_hwmod_irq_info omap44xx_mmc4_irqs[] = {
> { .irq = 96 + OMAP44XX_IRQ_GIC_START },
> };
> @@ -3599,11 +3612,13 @@ static struct omap_hwmod omap44xx_mmc4_hwmod = {
> },
> .slaves = omap44xx_mmc4_slaves,
> .slaves_cnt = ARRAY_SIZE(omap44xx_mmc4_slaves),
> + .dev_attr =&omap_mmc4_dev_attr,
> .omap_chip = OMAP_CHIP_INIT(CHIP_IS_OMAP4430),
> };
>
> /* mmc5 */
> static struct omap_hwmod omap44xx_mmc5_hwmod;
> +static struct mmc_dev_attr omap_mmc5_dev_attr;
> static struct omap_hwmod_irq_info omap44xx_mmc5_irqs[] = {
> { .irq = 59 + OMAP44XX_IRQ_GIC_START },
> };
> @@ -3651,6 +3666,7 @@ static struct omap_hwmod omap44xx_mmc5_hwmod = {
> },
> .slaves = omap44xx_mmc5_slaves,
> .slaves_cnt = ARRAY_SIZE(omap44xx_mmc5_slaves),
> + .dev_attr =&omap_mmc5_dev_attr,
> .omap_chip = OMAP_CHIP_INIT(CHIP_IS_OMAP4430),
> };
>

In order to be aligned with the generator, and assuming that only the
mm1 needs dev_attr, the OMAP4 diff should be:

diff --git a/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
b/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
index 79a8601..958651c 100644
--- a/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
+++ b/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
@@ -3420,6 +3420,11 @@ static struct omap_hwmod_ocp_if
*omap44xx_mmc1_slaves[] = {
&omap44xx_l4_per__mmc1,
};

+/* mmc1 dev_attr */
+static struct omap_mmc_dev_attr mmc1_dev_attr = {
+ .flags = OMAP_HSMMC_SUPPORTS_DUAL_VOLT,
+};
+
static struct omap_hwmod omap44xx_mmc1_hwmod = {
.name = "mmc1",
.class = &omap44xx_mmc_hwmod_class,
@@ -3433,6 +3438,7 @@ static struct omap_hwmod omap44xx_mmc1_hwmod = {
.clkctrl_reg = OMAP4430_CM_L3INIT_MMC1_CLKCTRL,
},
},
+ .dev_attr = &mmc1_dev_attr,
.slaves = omap44xx_mmc1_slaves,
.slaves_cnt = ARRAY_SIZE(omap44xx_mmc1_slaves),
.masters = omap44xx_mmc1_masters,
---

Regards,
Benoit