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:
> > You don't need to touch all drivers because you didn't change struct
> > pwm_chip::dev yet. (If you really want, you don't need to change that,
> > but then you have some duplication as chip->dev holds the same value as
> > priv->dev.parent in the end.)
>
> I don't think that's a problem. These are for two logically separate
> things, after all.
How are they different? I'd say one is the initializer for the other and
(ideally) unused after that. With that interpretation they are indeed
different, but then it's ugly that the initializer keeps staying around.
> Duplication can also sometimes be useful to simplify
> things. There are plently of cases where we use local variables for the
> same reason.
local variables go away though after the respective function is left.
chip->dev and its copy priv->dev.parent stay around for the full
lifetime of the chip.
> > > @@ -58,23 +60,24 @@ static struct pwm_chip *pwmchip_find_by_name(const char *name)
> > >
> > > static int pwm_device_request(struct pwm_device *pwm, const char *label)
> > > {
> > > + struct pwm_chip *chip = pwm->priv->chip;
> >
> > With my approach getting the chip of a struct pwm_device is only one
> > pointer dereference away. You need two.
>
> None of the functions here are called very often, so even if this isn't
> optimized away it would hardly matter.
I'd say pwm_apply_state() at least matters. Also I think that making a
slow path quicker is a good thing.
I wonder how we'll converge to an approach that can go into the
mainline given that we both have our strong opinions.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
Attachment:
signature.asc
Description: PGP signature
