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?
