[PATCH 06/15] mfd/ab8500: Remove confusing ab8500-i2c file and merge into ab8500-core
Lee Jones
lee.jones at linaro.org
Fri May 4 17:24:10 EDT 2012
On 04/05/12 21:25, Arnd Bergmann wrote:
> On Friday 04 May 2012, Lee Jones wrote:
>>
>> ab8500-i2c is used as core code to register the ab8500 device.
>> After allocating ab8500 memory, it immediately calls into
>> ab8500-core where the real initialisation takes place. This
>> patch moves all core registration and memory allocation into
>> the true ab8500-core file and removes ab8500-i2c completely.
>>
>> Signed-off-by: Lee Jones<lee.jones at linaro.org>
>
> These changes all look good, but I think I would go further here.
> I believe we discussed this and I agreed that we could leave that
> for later, but upon reading this code, I think now that it's getting
> rather silly.
>
>> @@ -125,6 +126,41 @@ static const char ab8500_version_str[][7] = {
>> [AB8500_VERSION_AB8540] = "AB8540",
>> };
>>
>> +static int ab8500_i2c_write(struct ab8500 *ab8500, u16 addr, u8 data)
>> +{
>> + int ret;
>> +
>> + ret = prcmu_abb_write((u8)(addr>> 8), (u8)(addr& 0xFF),&data, 1);
>> + if (ret< 0)
>> + dev_err(ab8500->dev, "prcmu i2c error %d\n", ret);
>> + return ret;
>> +}
>> +
>> +static int ab8500_i2c_write_masked(struct ab8500 *ab8500, u16 addr, u8 mask,
>> + u8 data)
>> +{
>> + int ret;
>> +
>> + ret = prcmu_abb_write_masked((u8)(addr>> 8), (u8)(addr& 0xFF),&data,
>> +&mask, 1);
>> + if (ret< 0)
>> + dev_err(ab8500->dev, "prcmu i2c error %d\n", ret);
>> + return ret;
>> +}
>> +
>> +static int ab8500_i2c_read(struct ab8500 *ab8500, u16 addr)
>> +{
>> + int ret;
>> + u8 data;
>> +
>> + ret = prcmu_abb_read((u8)(addr>> 8), (u8)(addr& 0xFF),&data, 1);
>> + if (ret< 0) {
>> + dev_err(ab8500->dev, "prcmu i2c error %d\n", ret);
>> + return ret;
>> + }
>> + return (int)data;
>> +}
>> +
>> static int ab8500_get_chip_id(struct device *dev)
>> {
>> struct ab8500 *ab8500;
>
> Each of these functions is called only from a single location, and through an indirect
> function pointer.
>
>> + ab8500->read = ab8500_i2c_read;
>> + ab8500->write = ab8500_i2c_write;
>> + ab8500->write_masked = ab8500_i2c_write_masked;
>
> Which you unconditionally assign here.
>
> If you apply this patch below, then there is no reason to add any of those.
> There is room for additional simplification even, but this is the most
> important one. Note that the ab8500 mutex was only needed to support the
> case where write_masked is not present, and that the debug output
> on error is pointless because the prcmu driver already writes the same
> output. The next step would be to remove all the {get,set}_register functions
> from ab8500 and just call the prcmu directly.
It's something I'm happy to do, but wasn't the point of the patch. I
don't know much about this code, as I didn't write it.
> Signed-off-by: Arnd Bergmann<arnd at arndb.de>
> ---
>
> drivers/mfd/ab8500-core.c | 72 +++---------------------------------------------------------------------
> include/linux/mfd/abx500/ab8500.h | 1 -
> 2 files changed, 3 insertions(+), 70 deletions(-)
>
> diff --git a/drivers/mfd/ab8500-core.c b/drivers/mfd/ab8500-core.c
> index 1f08704..b10bd2f 100644
> --- a/drivers/mfd/ab8500-core.c
> +++ b/drivers/mfd/ab8500-core.c
> @@ -138,24 +138,8 @@ static int ab8500_get_chip_id(struct device *dev)
> static int set_register_interruptible(struct ab8500 *ab8500, u8 bank,
> u8 reg, u8 data)
> {
> - int ret;
> - /*
> - * Put the u8 bank and u8 register together into a an u16.
> - * The bank on higher 8 bits and register in lower 8 bits.
> - * */
> - u16 addr = ((u16)bank)<< 8 | reg;
> -
> dev_vdbg(ab8500->dev, "wr: addr %#x<= %#x\n", addr, data);
> -
> - mutex_lock(&ab8500->lock);
> -
> - ret = ab8500->write(ab8500, addr, data);
> - if (ret< 0)
> - dev_err(ab8500->dev, "failed to write reg %#x: %d\n",
> - addr, ret);
> - mutex_unlock(&ab8500->lock);
> -
> - return ret;
> + return prcmu_abb_write(bank, reg,&data, 1);
> }
>
> static int ab8500_set_register(struct device *dev, u8 bank,
> @@ -169,21 +153,7 @@ static int ab8500_set_register(struct device *dev, u8 bank,
> static int get_register_interruptible(struct ab8500 *ab8500, u8 bank,
> u8 reg, u8 *value)
> {
> - int ret;
> - /* put the u8 bank and u8 reg together into a an u16.
> - * bank on higher 8 bits and reg in lower */
> - u16 addr = ((u16)bank)<< 8 | reg;
> -
> - mutex_lock(&ab8500->lock);
> -
> - ret = ab8500->read(ab8500, addr);
> - if (ret< 0)
> - dev_err(ab8500->dev, "failed to read reg %#x: %d\n",
> - addr, ret);
> - else
> - *value = ret;
> -
> - mutex_unlock(&ab8500->lock);
> + ret = prcmu_abb_read(bank, addr, value, 1);
> dev_vdbg(ab8500->dev, "rd: addr %#x => data %#x\n", addr, ret);
>
> return ret;
> @@ -200,42 +170,7 @@ static int ab8500_get_register(struct device *dev, u8 bank,
> static int mask_and_set_register_interruptible(struct ab8500 *ab8500, u8 bank,
> u8 reg, u8 bitmask, u8 bitvalues)
> {
> - int ret;
> - /* put the u8 bank and u8 reg together into a an u16.
> - * bank on higher 8 bits and reg in lower */
> - u16 addr = ((u16)bank)<< 8 | reg;
> -
> - mutex_lock(&ab8500->lock);
> -
> - if (ab8500->write_masked == NULL) {
> - u8 data;
> -
> - ret = ab8500->read(ab8500, addr);
> - if (ret< 0) {
> - dev_err(ab8500->dev, "failed to read reg %#x: %d\n",
> - addr, ret);
> - goto out;
> - }
> -
> - data = (u8)ret;
> - data = (~bitmask& data) | (bitmask& bitvalues);
> -
> - ret = ab8500->write(ab8500, addr, data);
> - if (ret< 0)
> - dev_err(ab8500->dev, "failed to write reg %#x: %d\n",
> - addr, ret);
> -
> - dev_vdbg(ab8500->dev, "mask: addr %#x => data %#x\n", addr,
> - data);
> - goto out;
> - }
> - ret = ab8500->write_masked(ab8500, addr, bitmask, bitvalues);
> - if (ret< 0)
> - dev_err(ab8500->dev, "failed to modify reg %#x: %d\n", addr,
> - ret);
> -out:
> - mutex_unlock(&ab8500->lock);
> - return ret;
> + return prcmu_abb_write_masked(bank, reg,&bitmask,&bitvalues, 1);
> }
>
> static int ab8500_mask_and_set_register(struct device *dev,
> @@ -1013,7 +948,6 @@ int __devinit ab8500_init(struct ab8500 *ab8500, enum ab8500_version version)
> if (plat)
> ab8500->irq_base = plat->irq_base;
>
> - mutex_init(&ab8500->lock);
> mutex_init(&ab8500->irq_lock);
>
> if (version != AB8500_VERSION_UNDEFINED)
> diff --git a/include/linux/mfd/abx500/ab8500.h b/include/linux/mfd/abx500/ab8500.h
> index fccc300..5f70328 100644
> --- a/include/linux/mfd/abx500/ab8500.h
> +++ b/include/linux/mfd/abx500/ab8500.h
> @@ -232,7 +232,6 @@ enum ab8500_version {
> */
> struct ab8500 {
> struct device *dev;
> - struct mutex lock;
> struct mutex irq_lock;
>
> int irq_base;
--
Lee Jones
Linaro ST-Ericsson Landing Team Lead
M: +44 77 88 633 515
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
More information about the linux-arm-kernel
mailing list