Re: [PATCH v3] USB: PHY: Palmas USB Transceiver Driver

[Stephen Warren <swarren@wwwdotorg.org>]

On 03/26/2013 04:28 AM, Laxman Dewangan wrote:
> On Tuesday 26 March 2013 03:51 PM, Felipe Balbi wrote:
>> On Tue, Mar 26, 2013 at 09:01:42AM +0000, Graeme Gregory wrote:
...
>>>>> +       return regmap_read(palmas->regmap[slave], addr, dest);
>>>>>
>>>>>
>>>>> Please use the generic api for palmas_read()/palmas_write(0 as it will
>>>>> be ease on debugging on register access.
>>>>> Direct regmap_read() does not help much on this.
>>>>
>>>> Any reason why you dint use palmas_read()/palmas_write here?
>>>> Btw palmas_read()/palmas_write() internally uses regmap APIs.
>>>
>>> Because I was not a fan of tightly coupling the child devices to the
>>> parent MFD. palmas_read/write were added by Laxman.
>>
>> I guess regmap would also help abstracting SPI versus I2C connection.
>> IMHO, palmas_read/write should be removed.
>>
>> Laxman's complaint that it doesn't help with debugging is utter
>> nonsense.
>
> palams read/write api uses the regmap only and hence not break anything
> on abstraction.
> in place of doing the following three statement in whole word, it
> provides wrapper of palmas_read()
> which actually does the same.
> 
> slave = PALMAS_BASE_TO_SLAVE(PALMAS_USB_OTG_BASE);
> addr = PALMAS_BASE_TO_REG(PALMAS_USB_OTG_BASE, reg);
> regmap_read(palmas->regmap[slave], addr, dest);

Why would you ever do that? Surely each module's probe should create or
obtain a regmap object somehow, and then all other code in that driver
should simply use regmap_read()/regmap_write() without having to perform
any kind of calculations at all.

If the MFD components truly are pluggable mix/match IP blocks, then all
the register offset #defines must all be relative to the base of the IP
block, so there would be no need for any calculations there.

The I2C address and base register address for the regmap object should
come from DT or platform data, and be used one time in probe() to create
the regmap object.

Then, there would be no need for palmas_read()/palmas_write().

> Above 3 lines in all the places for resgister access or make single call:
> palmas_read(palmas, PALMAS_USB_OTG_BASE, PALMAS_USB_OTG_BASE, dest).
> 
> This function implement the above 3 lines.