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

Kadiyala, Kishore
2011-02-24T13:58:59Z

On Thu, Feb 24, 2011 at 4:29 PM, Cousson, Benoit <b-cousson@ti.com> wrote:
> 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?

Ok

>
> 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.

ok

>
>> +
>>  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.

ok

>
> [...]
>
>> 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:

ok, will have changes as below.

>
> 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
>