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.1701860672.git.u.kleine-koenig@xxxxxxxxxxxxxx/ 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. -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ |
Attachment:
signature.asc
Description: PGP signature
