Re: [PATCH 2/7] PM / devfreq: exynos-ppmu: Use the regmap interface to handle the registers

[Chanwoo Choi]

2016-12-20 22:47 GMT+09:00 Tobias Jakobi <tjakobi@xxxxxxxxxxxxxxxxxxxxx>:
> Hey Chanwoo,
>
>
> Chanwoo Choi wrote:
>> On 2016ë 12ì 20ì 17:08, Tobias Jakobi wrote:
>>> Hello Chanwoo,
>>>
>>>
>>> Chanwoo Choi wrote:
>>>> Hi,
>>>>
>>>> On 2016ë 12ì 20ì 04:47, Tobias Jakobi wrote:
>>>>> Hello,
>>>>>
>>>>> I was just wondering what is improved by moving to regmap. For me this
>>>>> looks like it only complicates the code. Lots of regmap_{read,write}()
>>>>> and for each one of these we need to check the return code.
>>>>
>>>> It is correct to check the return value. It cover all of exception.
>>> that doesn't really answer my question. Which 'exceptions' are we
>>> talking about? What can go wrong with  __raw_{writel,readl}(), that
>>
>> When using __raw_readl/writel() don't check the any return value, it it not correct.
>> When calling the function, basically we should check whether return value is error or success.
>> What is problem to check the return value?
> So what you're saying is the following. When using
> __raw_{readl,writel}() somde error can occur, that we can't catch by
> using __raw_{readl,writel}(), but only by using the regmap API on top.

>
> So, what error would that be? Do you have an example where such an error
> occurs? In particular this leads me to the following question: What bug
> does the conversion to regmap actually fix?

I don't mention that this patch is bug fix.

No. It is well working. There is no any know error. As I already said,
First is checking the return value of function call as following.
>> When calling the function, basically we should check whether return value is error or success.

>
>
>>> makes it necessary to put another layer on top of it? AFAIK regmap was
>>> introduced to handle read/writes to slow busses like I2C and SPI. I
>>> don't see that this applies here.
>>
>> The regmap support the MMIO on following driver.
>> - drivers/base/regmap/regmap-mmio.c
> I know, but just because something exist isn't enough reason for me to
> using it. There should be a benefit here.
>
> At the moment I only see that this does the following:
> - makes the code more convoluted

I don't agree. As I already said as following, first is checking the
return value of function call as following.
">> When calling the function, basically we should check whether
return value is error or success."

> - does some dubious error checking

I don't want you use 'dobious' word. I need correct reason why you do
obeject to use it.
The error checking is clear.

> - impact of performance (__raw_{readl,writel}() maps to some load/stores
> on the assembler level, now we have go through a whole subsystem to
> achieve the same thing)

Do you have the performance result between regmap and __raw_readl/writel?
Do you mean that regmap-mmio is unneeded?

It is not reasonable. The system is enough fast to use the regmap. The
many device driver use the 'regmap-mmio.c' driver in mainline kernel.
You can find them.

>
>
>>>>> Also when exactly did __raw_writel() and friends become legacy?
>>>>
>>>> Also, I want to use the regmap interface (regmap-io) instead of __raw_readl/__raw_writel using the base address
>>>> directly.
>>> I see, but why are __raw_{writel,readl}() legacy as you say? I don't see
>>> this anywhere else in the kernel.
>>
>> If you just don't like the 'legacy' expression. I'll remove it.
> No, actually the 'legacy' part is important, if it were true. If
> __raw_{writel,readl}() would indeed be legacy and there was a consensus
> that using a different interface is better, then I would agree to this
> change.
> But the calls are not legacy, hence I'm missing some reason for this change.

When using devm_regmap_init_mmio(), the device driver don't need to
consider the 'iounmap' becaue it is automatically by framework. And
when using __raw_readl/writel to read and write the registers, they
check whether register is writable/readable is not.

The __raw_readl/writel don't consider them and support them.

>
>
>> It is not any important. The key point of this patch uses the regmap interface.
>> Usually, when adding new device driver, I use the regmap mmio interface
>> instead of __raw_readl/writel. So, this patch changes it.
> That doesn't sound like a good reasoning. What improvement do we get by
> this change? And no, I don't buy the error checking argument ;)

I replied already why regmap interface is used.

-- 
Best Regards,
Chanwoo Choi