Re: [PATCH v4 000/115] pwm: Fix lifetime issues for pwm_chips

Hello Thierry,

On Mon, Dec 11, 2023 at 12:33:04PM +0100, Thierry Reding wrote:

On Fri, Dec 08, 2023 at 07:50:33PM +0100, Uwe Kleine-König wrote:

quoted

The TL;DR; is essentially what I already wrote in my last reply to Bart
in the v3 thread[1]:

 - My approach needs more changes to the individual drivers (which I
   don't consider a relevant disadvantage given that the resulting code
   is better);
 - My approach works with less pointer dereferences which IMHO also
   simplifies understanding the code as all relevant data is in a single
   place.
 - My approach has a weaker separation between the core and the lowlevel
   drivers. That's ok in my book given that this doesn't complicate the
   lowlevel drivers and that hiding details considerably better doesn't
   work anyhow (see the drivers that need internal.h in your patch).

For me the single allocation issue is only an added bonus. The relevant
advantage of my approach is that the code is easier and (probably) more
efficient.

I happen to disagree. I think adding pwmchip_alloc() makes things much
more complicated for low level drivers.

Looking at e.g.
https://lore.kernel.org/linux-pwm/2dda818b8bbbe8ba4b9df5ab54f960ff4a4f1ab5.1701860672.git.u.kleine-koenig@pengutronix.de/ (local)
I wonder where you see "much more complication". OK, there are two
pointers now for chip and private data, but I'd call that at most a
"mild" complication[1] which is more than balanced out by the
simplifications in the remaining parts of that patch.

Best regards
Uwe

[1] I'm not sure I'd refuse someone suggesting the following patch on
    top of today's next:

	diff --git a/drivers/pwm/pwm-microchip-core.c b/drivers/pwm/pwm-microchip-core.c
	index c0c53968f3e9..d32e65914599 100644
	--- a/drivers/pwm/pwm-microchip-core.c
	+++ b/drivers/pwm/pwm-microchip-core.c
	@@ -448,12 +448,14 @@ MODULE_DEVICE_TABLE(of, mchp_core_of_match);
	 static int mchp_core_pwm_probe(struct platform_device *pdev)
	 {
		struct mchp_core_pwm_chip *mchp_core_pwm;
	+	struct pwm_chip *chip;
		struct resource *regs;
		int ret;
	 
		mchp_core_pwm = devm_kzalloc(&pdev->dev, sizeof(*mchp_core_pwm), GFP_KERNEL);
		if (!mchp_core_pwm)
			return -ENOMEM;
	+	chip = &mchp_core_pwm->chip;
	 
		mchp_core_pwm->base = devm_platform_get_and_ioremap_resource(pdev, 0, &regs);
		if (IS_ERR(mchp_core_pwm->base))
	@@ -470,9 +472,9 @@ static int mchp_core_pwm_probe(struct platform_device *pdev)
	 
		mutex_init(&mchp_core_pwm->lock);
	 
	-	mchp_core_pwm->chip.dev = &pdev->dev;
	-	mchp_core_pwm->chip.ops = &mchp_core_pwm_ops;
	-	mchp_core_pwm->chip.npwm = 16;
	+	chip->dev = &pdev->dev;
	+	chip->ops = &mchp_core_pwm_ops;
	+	chip->npwm = 16;
	 
		mchp_core_pwm->channel_enabled = readb_relaxed(mchp_core_pwm->base + MCHPCOREPWM_EN(0));
		mchp_core_pwm->channel_enabled |=
	@@ -485,7 +487,7 @@ static int mchp_core_pwm_probe(struct platform_device *pdev)
		writel_relaxed(1U, mchp_core_pwm->base + MCHPCOREPWM_SYNC_UPD);
		mchp_core_pwm->update_timestamp = ktime_get();
	 
	-	ret = devm_pwmchip_add(&pdev->dev, &mchp_core_pwm->chip);
	+	ret = devm_pwmchip_add(&pdev->dev, chip);
		if (ret)
			return dev_err_probe(&pdev->dev, ret, "Failed to add pwmchip\n");
 
    With that applied before the above mentioned patch there is no
    complication at all in my eyes.


-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |