Re: [PATCH v11 2/2] misc: Add iop driver for Sunplus SP7021

[Krzysztof Kozlowski]

On 15/03/2022 03:08, Tony Huang 黃懷厚 wrote:
> Dear Krzysztof:
> 
> 
>>>> On 12/03/2022 17:16, Tony Huang wrote:
>>>>> This driver is load 8051 bin code.
>>>>> The IOP core is DQ8051, so also named IOP8051.
>>>>> Need Install DQ8051, The DQ8051 bin file generated by keil C.
>>>>> We will provide users with 8051 normal and power off source code.
>>>>> Path: https://sunplus.atlassian.net/wiki/spaces/doc/pages/610172933/
>>>>>
>>>>
>> How+to+use+I+O+processor+8051+of+SP7021#5.-Write-C-or-assembly-source
>>>> How+to+use+I+O+processor+8051+of+-
>>>>> files-for-IOP
>>>>> Users can follow the operation steps to generate normal.bin and
>>>>> poweroff.bin.
>>>>> Path: https://sunplus.atlassian.net/wiki/spaces/doc/pages/466190338
>>>>> /26.+IOP8051 26.5?How To Create 8051 bin file
>>>>>
>>>>> PMC in power management purpose:
>>>>> Add sp_iop_poweroff() function. pm_power_off=sp_iop_poweroff.
>>>>> When the power off command is executed.
>>>>> The 8051 has a register(DEF_PWR_EN_0=0) to control the power-on and
>>>>> power-off of the system.
>>>>>
>>>>> Signed-off-by: Tony Huang <tonyhuang.sunplus@xxxxxxxxx>
>>>>> ---
>>>>> Changes in v11:
>>>>>  - Addressed comments from Arnd Bergmann.
>>>>
>>>> How did you address Arnd's comments about splitting the driver to
>>>> proper parts? drivers/clk and drivers/power/reset?
>>>>
>>>
>>> drivers/clk : SP7021 system has clock device driver (clk-sp7021.c).
>>> So I set the IOP clock through the following function.
>>> clk_prepare_enable(iop->iopclk);
>>> clk_disable_unprepare(iop->iopclk);
>>>
>>> drivers/power/reset : SP7021 system does not have a power off device driver.
>>
>> What does it mean? The feedback was to split clk and reset features to
>> separate drivers and I still see only two patches here with a misc driver. So how
>> is his comments addressed? You did not reply in that thread.
>>
> 
> I finished replying to Arnd.
> 
>>>
>>>>>  - Addressed comments from krzysztof.
>>>>>
>>>>>  MAINTAINERS                |   1 +
>>>>>  drivers/misc/Kconfig       |  23 +++
>>>>>  drivers/misc/Makefile      |   1 +
>>>>>  drivers/misc/sunplus_iop.c | 411
>>>>> +++++++++++++++++++++++++++++++++++++++++++++
>>>>
>>>> The driver looks like SoC specific driver. Why did you put it in
>>>> drivers/misc/, not in usual place - drivers/soc/?
>>>
>>> 8051 is designed for processing I/O events, like receiving IR signal from
>> remote controller,
>>> taking care of power on or off requests from RTC, or other hardware events
>> of external peripherals
>>> even when power of main system is off.
>>> So I put it in drivers/misc.
>>
>> Is IOP8061 a separate device? Your datasheet is saying its embedded in
>> SP7021 SoC, so it is a soc driver. This does not fit misc driver description (a
>> "strange device") but a SoC driver description.
>>
> 
> IOP is a separate device. CPU is 8051.		
> SP7021 contains three kinds of CPU.		
> Quad-core ARM Cortex-A7 (CA7)		
> ARM926 real-time core		
> 8051 low-power core		
> 
>>>
>>>> sp_iop_poweroff is still here.
>>>
>>> sp_iop_poweroff(): SP7021 system does not have a power off device driver.
>>> 				I have to put it here.
>>
>> This should be rather in a reset driver, not in a misc one. What is your plan for
>> this driver? You described the hardware and judging by it, there will be quite a
>> lot of separate features so it's reasonable to split the driver into separate
>> logical elements. However keeping all in the same place would be ok, if you do
>> not plan to add any more features.
>> This would mean the driver will handle *only* reset and FW loading.
>>
> 
> Can I put sp_iop_poweroff() here for now?				
> When power off device driver is added in /driver/power/reset is complete, we will move to it.

Not really, because misc drivers is not a place for power off drivers.
The driver here looks now like responsible only for system power
management of a SoC, so most likely drivers/soc. However it has even
less sense to add some feature here and immediately move it somewhere
more appropriate (instead just add it to the appropriate place).

Your moving of parts of it to drivers/power/reset is now confusing. What
will be left here? Please send entire set not just pieces.


Best regards,
Krzysztof