Re: [PATCH 1/7] MFD: ASIC3: add API for EXTCF and SDHWCTRL register manipulation

[pHilipp Zabel <philipp.zabel@gmail.com>]

Hi Samuel,

On Fri, Jun 5, 2009 at 1:46 AM, Samuel Ortiz<sameo@openedhand.com> wrote:
> Hi Philipp
>
> On Thu, Jun 04, 2009 at 08:36:10PM +0200, Philipp Zabel wrote:
>> Those registers are needed for PCMCIA and MMC/SDIO operation as
>> well as for the DS1WM.
>>
>> Signed-off-by: Philipp Zabel <philipp.zabel@gmail.com>
>> ---
>>  drivers/mfd/asic3.c       |   48 +++++++++++++++++++++++++++++++++++++++++++++
>>  include/linux/mfd/asic3.h |   13 +++++++----
>>  2 files changed, 56 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/mfd/asic3.c b/drivers/mfd/asic3.c
>> index 9e48545..8e1653a 100644
>> --- a/drivers/mfd/asic3.c
>> +++ b/drivers/mfd/asic3.c
>> @@ -52,6 +52,54 @@ static inline u32 asic3_read_register(struct asic3 *asic,
>>                       (reg >> asic->bus_shift));
>>  }
>>
>> +void asic3_set_extcf_select(struct device *dev, u32 bits, int value)
> 2 remarks here:
> - The "value" parameter could be a bool, and could use a better name, e.g.
> "set".
> - Those 3 routines are basically doing the same thing. Why not having a
> generic:
> void asic3_set_register(struct device *dev, u32 reg, u32 bits, bool set) ?

Thanks. The reasoning behind this split (besides the fact that the
hh.org driver did it this way) was that only asic3_set_extcf_select
has to be exported for the pcmcia driver and I'd like to move the
register addresses from the public header into asic3.c or into a
private header some time.

I think it is better to have one (unexported) function as you suggest
and then export a custom function to handle the EXTCF
SLEEP_MODE/BUF_EN/PWAIT_EN bits in the future.

Will combine and resend.

> Cheers,
> Samuel.
>
>> +{
>> +     struct asic3 *asic = dev->driver_data;
>> +     unsigned long flags;
>> +     u32 v;
>> +
>> +     spin_lock_irqsave(&asic->lock, flags);
>> +     v = asic3_read_register(asic, ASIC3_OFFSET(EXTCF, SELECT));
>> +     if (value)
>> +             v |= bits;
>> +     else
>> +             v &= ~bits;
>> +     asic3_write_register(asic, ASIC3_OFFSET(EXTCF, SELECT), v);
>> +     spin_unlock_irqrestore(&asic->lock, flags);
>> +}
>> +EXPORT_SYMBOL(asic3_set_extcf_select);
>> +
>> +void asic3_set_extcf_reset(struct device *dev, u32 bits, int value)
>> +{
>> +     struct asic3 *asic = dev->driver_data;
>> +     unsigned long flags;
>> +     u32 v;
>> +
>> +     spin_lock_irqsave(&asic->lock, flags);
>> +     v = asic3_read_register(asic, ASIC3_OFFSET(EXTCF, RESET));
>> +     if (value)
>> +             v |= bits;
>> +     else
>> +             v &= ~bits;
>> +     asic3_write_register(asic, ASIC3_OFFSET(EXTCF, RESET), v);
>> +     spin_unlock_irqrestore(&asic->lock, flags);
>> +}
>> +
>> +void asic3_set_sdhwctrl(struct asic3 *asic, u32 bits, int value)
>> +{
>> +     unsigned long flags;
>> +     u32 v;
>> +
>> +     spin_lock_irqsave(&asic->lock, flags);
>> +     v = asic3_read_register(asic, ASIC3_OFFSET(SDHWCTRL, SDCONF));
>> +     if (value)
>> +             v |= bits;
>> +     else
>> +             v &= ~bits;
>> +     asic3_write_register(asic, ASIC3_OFFSET(SDHWCTRL, SDCONF), v);
>> +     spin_unlock_irqrestore(&asic->lock, flags);
>> +}
>> +
>>  /* IRQs */
>>  #define MAX_ASIC_ISR_LOOPS    20
>>  #define ASIC3_GPIO_BASE_INCR \
>> diff --git a/include/linux/mfd/asic3.h b/include/linux/mfd/asic3.h
>> index 322cd6d..cd02d20 100644
>> --- a/include/linux/mfd/asic3.h
>> +++ b/include/linux/mfd/asic3.h
>> @@ -16,6 +16,9 @@
>>
>>  #include <linux/types.h>
>>
>> +/* for PCMCIA */
>> +extern void asic3_set_extcf_select(struct device *dev, u32 bits, int value);
>> +
>>  struct asic3_platform_data {
>>       u16 *gpio_config;
>>       unsigned int gpio_config_num;
>> @@ -227,8 +230,8 @@ struct asic3_platform_data {
>>
>>
>>  /* Basic control of the SD ASIC */
>> -#define ASIC3_SDHWCTRL_Base  0x0E00
>> -#define ASIC3_SDHWCTRL_SDConf    0x00
>> +#define ASIC3_SDHWCTRL_BASE     0x0E00
>> +#define ASIC3_SDHWCTRL_SDCONF     0x00
>>
>>  #define ASIC3_SDHWCTRL_SUSPEND    (1 << 0)  /* 1=suspend all SD operations */
>>  #define ASIC3_SDHWCTRL_CLKSEL     (1 << 1)  /* 1=SDICK, 0=HCLK */
>> @@ -242,10 +245,10 @@ struct asic3_platform_data {
>>  /* SD card power supply ctrl 1=enable */
>>  #define ASIC3_SDHWCTRL_SDPWR      (1 << 6)
>>
>> -#define ASIC3_EXTCF_Base             0x1100
>> +#define ASIC3_EXTCF_BASE        0x1100
>>
>> -#define ASIC3_EXTCF_Select         0x00
>> -#define ASIC3_EXTCF_Reset          0x04
>> +#define ASIC3_EXTCF_SELECT        0x00
>> +#define ASIC3_EXTCF_RESET         0x04
>>
>>  #define ASIC3_EXTCF_SMOD0             (1 << 0)  /* slot number of mode 0 */
>>  #define ASIC3_EXTCF_SMOD1             (1 << 1)  /* slot number of mode 1 */
>> --
>> 1.6.3.1
>>
>
> --
> Intel Open Source Technology Centre
> http://oss.intel.com/
>