Re: [PATCH v5 2/2] OMAP4 HSMMC: Adding card detect support for MMC1 Controller

[kishore kadiyala <kishorek.kadiyala@xxxxxxxxx>]

On Fri, Jun 18, 2010 at 1:57 AM, Andrew Morton
<akpm@xxxxxxxxxxxxxxxxxxxx> wrote:
> On Thu, 17 Jun 2010 20:57:19 +0530 (IST)
> "kishore kadiyala" <kishore.kadiyala@xxxxxx> wrote:
>
>> Adding card detect callback function which gives the status of
>> the card .For MMC1 Controller, Card detect interrupt source is
>> twl6030 and card present/absent status is provided by MMCCTRL
>> register of twl6030.
>>
>>
>> ...
>>
>>
>> +     int ret = -ENOSYS;
>> +     int ret = -ENOSYS;
>
> ENOSYS seems an inappropriate errno to use in a driver.
>
> "ENOSYS -- The system doesn't support that function.  For example, if
> you call setpgid() on a system without job control, you'll get an
> ENOSYS error."
>
> I think it means "you the programmer tried to do something in a syscall
> which didn't make sense in this context".
>
> I'm not sure what _is_ appropraite here.  There's always EIO I guess.
> ENODEV?

Agree, I will make changes with "EIO" .

>
> This happens a lot.  The userspace errnos just don't map well onto
> kernel-internal operations.  it was a mistake - we should have defined a kernel-internal namespace and perhaps type for such things.  Oh well.
>
>> +/* Configuring Card Detect for MMC1 */
>> +static inline int omap4_hsmmc1_card_detect_config(void)
>> +{
>> +     int res = -1;
>> +     u8 reg_val = 0;
>> +
>> +     /* Unmasking the Card detect Interrupt line for MMC1 from Phoenix */
>> +     if (twl_class_is_6030()) {
>> +             twl6030_interrupt_unmask(TWL6030_MMCDETECT_INT_MASK,
>> +                                                     REG_INT_MSK_LINE_B);
>> +             twl6030_interrupt_unmask(TWL6030_MMCDETECT_INT_MASK,
>> +                                                     REG_INT_MSK_STS_B);
>> +     }
>> +
>> +
>> +     /*
>> +      * Intially Configuring MMC_CTRL for receving interrupts &
>> +      * Card status on TWL6030 for MMC1
>> +      */
>> +     res = twl_i2c_read_u8(TWL6030_MODULE_ID0, &reg_val, TWL6030_MMCCTRL);
>> +     if (res < 0)
>> +             return res;
>> +     reg_val &= ~VMMC_AUTO_OFF;
>> +     reg_val |= SW_FC;
>> +     twl_i2c_write_u8(TWL6030_MODULE_ID0, reg_val, TWL6030_MMCCTRL);
>> +
>> +      /* Configuring CFG_INPUT_PUPD3 */
>> +     res = twl_i2c_read_u8(TWL6030_MODULE_ID0, &reg_val,
>> +                                             TWL6030_CFG_INPUT_PUPD3);
>> +     if (res < 0)
>> +             return res;
>> +     reg_val &= ~(MMC_PU | MMC_PD);
>> +     twl_i2c_write_u8(TWL6030_MODULE_ID0, reg_val, TWL6030_CFG_INPUT_PUPD3);
>> +     return res;
>> +}
>
> This is waaaaay to large to be inlined.  Why not put it in a .c file?

ok, I will repost with the inlined moved as a function in .c file .

Regards,
Kishore

>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html