[PATCH v4 000/115] pwm: Fix lifetime issues for pwm_chips
Uwe Kleine-König
u.kleine-koenig at pengutronix.de
Mon Dec 11 04:18:07 PST 2023
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@pengutronix.de/
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/ |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-amlogic/attachments/20231211/dac7004b/attachment.sig>
More information about the linux-amlogic
mailing list