Re: [RFC PATCH 0/6] Support for QPNP PMIC's

[Stanimir Varbanov <svarbanov@mm-sol.com>]

On 06/25/2014 09:04 PM, Courtney Cavin wrote:
> On Wed, Jun 25, 2014 at 01:07:13PM +0200, Stanimir Varbanov wrote:
>> Hi Courtney,
>>
>> Thanks for the comments.
>>
>> On 06/25/2014 01:36 AM, Courtney Cavin wrote:
>>> On Fri, Jun 20, 2014 at 02:21:19PM +0200, Stanimir Varbanov wrote:
>>>> Hello to all,
>>>>
>>>> The goal of this WIP set is to provide support for sub-functional
>>>> hw blocks inside Qualcomm QPNP PMIC chips by creating a new qpnp
>>>> bus (device-driver modeled). On SPMI interface we attach a so called
>>>> Slaves. For example one PMIC chip like pm8941 has two slave id's. On
>>>> every slaveid the PMIC chip has various peripherials. Every peripheral
>>>> driver should expose qpnp_driver structure and implement the .probe
>>>> and .remove callbacks. Every peripheral driver will reside on the
>>>> proper place in /drivers directory. 
>>>>
>>>> The fisrt patch describes the devicetree binding of the slave devices
>>>> attached to the SPMI interface. The second patch adds implemetation of
>>>> the qpnp-bus and a layer connection with the SPMI interface. The third
>>>> patch adds spmi arbiter and underlying slaveid devicetree nodes for
>>>> msm8974 SoC. The other patches are example of rtc-qpnp driver and
>>>> binding documentation.
>>>>
>>>> Since this set is a WIP, it will be used as a starting point for future
>>>> duscussions about implementing QPNP PMIC sub-function hw blocks.
>>>
>>> Ok.  I have a few critical problems with this approach to implementing
>>> support for the PMICs:
>>>   - It's *way* more code than needed if done with of_platform_populate()
>>
>> Agreed, but this will be the price we must pay to keep architectural
>> correctness. The PMIC peripherals are not directly accessible by the CPU
>> bus and thus they shouldn't be treated as platform devices. That fact
>> gives as an opportunity to create a qpnp bus on which we will attach the
>> whole bunch of PMIC peripherals and using DT we can describe register
>> regions (256B each) belonging to it.
>>
> 
> So the price we must pay for *your* vision of architectural correctness
> is to duplicate code without any actual benefit?
> 
> According to Documentation/driver-model/platform.txt your view is
> correct:
> 
>   Platform devices are devices that typically appear as autonomous
>   entities in the system. This includes legacy port-based devices and
>   host bridges to peripheral buses, and most controllers integrated
>   into system-on-chip platforms.  What they usually have in common
>   is direct addressing from a CPU bus.  Rarely, a platform_device will
>   be connected through a segment of some other kind of bus; but its
>   registers will still be directly addressable.
> 
> However, it is clear that with recent changes to the platform code, this
> view is getting distorted somewhere.  I haven't specifically heard any
> decisions on what to do here though.
> 

Could you point me to the discussion?

> Greg, Grant, Rob?  What's the law?
> 
>>>   - AFAICT, the only direct relation between this and 'QPNP' is the 256
>>>     byte block size, which I would argue can be clearly expressed in DT
>>>     instead with #size-cells == 1, and #address-cells == 1
>>
>> The QPNP schema is a private case of Qualcomm PMIC's, so generalising
>> will over-engineering IMO.
>>
> 
> I made a similar attempt to write something like this, but did it
> entirely based on regmap "partitions", abstracting out the need for
> knowing anything in the client drivers but regmap....  It was about the
> same amount of code/complexity as this, but I tossed it out for the same
> reasons I mentioned here.  Over/under-engineering has little to do with
> it.

Hm, I missed it.

-- 
regards,
Stan