Re: [PATCH v11 2/6] clocksource/drivers: Add Renesas RZ/G2L MTU3a core driver

[Lee Jones <lee@kernel.org>]

On Thu, 26 Jan 2023, Biju Das wrote:

> Hi Daniel,
> 
> + Rob and Krzysztof Kozlowski 
> 
> > -----Original Message-----
> > From: Daniel Lezcano <daniel.lezcano@linaro.org>
> > Sent: Thursday, January 26, 2023 10:53 AM
> > To: Biju Das <biju.das.jz@bp.renesas.com>; Philipp Zabel
> > <p.zabel@pengutronix.de>
> > Cc: Thomas Gleixner <tglx@linutronix.de>; Geert Uytterhoeven
> > <geert+renesas@glider.be>; Chris Paterson <Chris.Paterson2@renesas.com>;
> > Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@bp.renesas.com>; linux-
> > renesas-soc@vger.kernel.org; Lee Jones <lee@kernel.org>
> > Subject: Re: [PATCH v11 2/6] clocksource/drivers: Add Renesas RZ/G2L MTU3a
> > core driver
> > 
> > On 13/01/2023 17:17, Biju Das wrote:
> > 
> > [ ... ]
> > 
> > > +config RZ_MTU3
> > > +	bool "Renesas RZ/G2L MTU3a core driver"
> > > +	select MFD_CORE
> > > +	depends on (ARCH_RZG2L && OF) || COMPILE_TEST
> > > +	help
> > > +	  Select this option to enable Renesas RZ/G2L MTU3a core driver for
> > > +	  the Multi-Function Timer Pulse Unit 3 (MTU3a) hardware available
> > > +	  on SoCs from Renesas. The core driver shares the clk and channel
> > > +	  register access for the other child devices like Counter, PWM,
> > > +	  Clock Source, and Clock event.
> > 
> > Do you really want to have this option manually selectable? Usually we try
> > to avoid that and keep a silent option which is selected by the platform
> > config.
> 
> For critical drivers like CPG, Pinctrl we enable it by default by silent option in platform config.
> For the others we add it to defconfig, once the device tree support is available.
> 
> 
> > 
> > [ ... ]
> > 
> > > +
> > > +	ret = mfd_add_devices(&pdev->dev, 0, rz_mtu3_devs,
> > > +			      ARRAY_SIZE(rz_mtu3_devs), NULL, 0, NULL);
> > > +	if (ret < 0)
> > > +		goto err_assert;
> > > +
> > > +	return devm_add_action_or_reset(&pdev->dev, rz_mtu3_reset_assert,
> > > +					&pdev->dev);
> > > +
> > > +err_assert:
> > > +	reset_control_assert(ddata->rstc);
> > > +	return ret;
> > > +}
> > 
> > I'm not sure this driver falls under the clocksource umbrella but under
> > mfd [cc'ed Lee Jones]
> > 
> 
> 
> Please find [1],
> 
> After a long discussion with dt maintainers, counter maintainer, MFD maintainer and PWM maintainer, it is concluded to
> Add the core driver to timer subsystem.

Which is fine.  However, you cannot then use the MFD API.                                                              
                                                                                                                       
> [1] https://patchwork.kernel.org/project/linux-renesas-soc/patch/20221010145222.1047748-2-biju.das.jz@bp.renesas.com/
                                                                                                                       
  "The TL;DR is: if you're not using the MFD Core (and including                                                       
  mfd/core.h), it's not an MFD.  You *could* split this up into its                                                    
  component parts, place them into their own subsystems and use an MFD                                                 
  core driver to register them all, but as Thierry says, this is not a                                                 
  hard requirement either."

-- 
Lee Jones [李琼斯]