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:
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.1... 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, ®s); 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.