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

[Tony Huang 黃懷厚]

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.
> 

I may not fully understand your requirements.				
You: What is your plan for this driver? 				
< ----- reset driver? Misc driver?				
You: 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.				
< ------ 	 Because system does not have a power off device driver.			
	sp_iop_poweroff() can be placed in iop driver?