Hello Bart, On Mon, Nov 27, 2023 at 09:22:48PM +0100, Bartosz Golaszewski wrote: > On Mon, Nov 27, 2023 at 11:58 AM Uwe Kleine-König > <u.kleine-koenig@xxxxxxxxxxxxxx> wrote: > > On Fri, Nov 24, 2023 at 10:16:40PM +0100, Bartosz Golaszewski wrote: > > > I admit I've been quite busy but I do plan on going through Uwe's > > > series next week and maybe running tests similar to what I have for > > > GPIO on it. > > > > That's great. If you want to do that on my tree that already saw a few > > improvements compared to what I sent out, get it at > > > > https://git.pengutronix.de/git/ukl/linux pwm-lifetime-tracking > > > > . The improvements are only on the driver level, so unless you're using > > one of the improved drivers, the difference wouldn't be that big I > > guess. For (maybe) quicker feedback loops, you can find me on irc (e.g. > > on libera's #linux-pwm) if that's a communication channel you like. > > I don't see anything obviously wrong with the approach. Is this the result of "running tests similar to what I have for GPIO on it" or did you only find the time for some high-level code inspection? > I see the > chip->operational field that is set to false on release. In my > version, we just use a NULL-pointer to carry the same information. Yup, sounds obvious. Your usage of "just" sounds as if your variant was better. To give the alternative view where the suggested approach sounds better would be: You need a pointer and I "just" a bool that even has a name implying its function. You need to dereference the pointer in several places as the needed information is distributed over two structures while it's all together in a single struct for the usual foo_alloc() + foo_register() approach. > Interestingly you DO have a pwm_device and pwm_chip structures. I'd > say it would be more logical to have the pwm_device embed struct > device. A pwm_chip represents a piece of hardware that provides (possibly) several PWM lines. A pwm_device is the abstraction for a single PWM line. So that's two different concepts and I wonder why you find it interesting that we have two different structures for it. Today the pwm framework already has a struct device for the pwm_chip that appears in /sys/class/pwm/pwmchipX. If a PWM line is exported in sysfs, another struct containing a struct device is allocated (struct pwm_export) to manage /sys/class/pwm/pwmchipX/pwmY/. I think it's good to have a struct device in the gpio_chip. I'd be open to put a struct device into pwm_device (unconditionally, not only when it's exported), but that's a change that is out of scope for this series. Also note that this would change the behaviour of /sys/class/pwm/ which I'd like to prevent (at least today until the character support is established, available for some time and known to be in use). > My approach is more about maintaining the logical scope and not > changing the ownership of objects allocated in the driver. I also > don't see a reason to expose the internals of the subsystem (struct > device) to the provider drivers other than in callbacks where it is > relevant. Subsystems should handle as much as possible and any data > structures not relevant to what the driver does should be hidden from > it. Drivers see struct pwm_chip today and IMHO that's fine. I also feel little incentive to hide something from the driver in .probe() and then have to expose (more of) it in .apply() anyhow. Also I don't think the series would benefit from putting yet more changes into it. Struct pwm_chip currently contains the following members: struct device dev; struct cdev cdev; const struct pwm_ops *ops; struct module *owner; unsigned int id; unsigned int npwm; struct pwm_device * (*of_xlate)(struct pwm_chip *chip, const struct of_phandle_args *args); unsigned int of_pwm_n_cells; /* only used internally by the PWM framework */ struct mutex lock; bool uses_pwmchip_alloc; bool operational; void *drvdata; struct pwm_device pwms[] __counted_by(npwm); Some of them should be moved below the "only used internally" comment. (i.e. dev, cdev, owner, id). For me this is "hidden" good enough then. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ |
Attachment:
signature.asc
Description: PGP signature
