| Message ID | 20230629094839.757092-8-u.kleine-koenig@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | pwm: Get rid of pwm_[sg]et_chip_data() | expand |
On Thu, Jun 29, 2023 at 11:48:38AM +0200, Uwe Kleine-König wrote: > Instead of an allocation of a single u16 per channel, allocate them all > in a single chunk which greatly reduces memory fragmentation and also > the overhead to track the allocated memory. Also put the channel data in > driver data where it's cheaper to determine the address (no function > call involved, just a trivial pointer addition). > > This also allows to get rid of the request and free callbacks. > > The only cost is that the channel data is allocated early, and even for > unused channels. > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> With comments addressed below: Reviewed-by: Tzung-Bi Shih <tzungbi@kernel.org> > @@ -27,6 +27,7 @@ struct cros_ec_pwm_device { > struct cros_ec_device *ec; > struct pwm_chip chip; > bool use_pwm_type; > + struct cros_ec_pwm *channel; Please update the kernel-doc too. Otherwise: $ ./scripts/kernel-doc -none drivers/pwm/pwm-cros-ec.c drivers/pwm/pwm-cros-ec.c:31: warning: Function parameter or member 'channel' not described in 'cros_ec_pwm_device' I have no strong preference: please consider to use `channels` if it makes sense.
On Fri, Jun 30, 2023 at 12:54:37PM +0800, Tzung-Bi Shih wrote: > On Thu, Jun 29, 2023 at 11:48:38AM +0200, Uwe Kleine-König wrote: > > Instead of an allocation of a single u16 per channel, allocate them all > > in a single chunk which greatly reduces memory fragmentation and also > > the overhead to track the allocated memory. Also put the channel data in > > driver data where it's cheaper to determine the address (no function > > call involved, just a trivial pointer addition). > > > > This also allows to get rid of the request and free callbacks. > > > > The only cost is that the channel data is allocated early, and even for > > unused channels. > > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > > With comments addressed below: > Reviewed-by: Tzung-Bi Shih <tzungbi@kernel.org> > > > @@ -27,6 +27,7 @@ struct cros_ec_pwm_device { > > struct cros_ec_device *ec; > > struct pwm_chip chip; > > bool use_pwm_type; > > + struct cros_ec_pwm *channel; > > Please update the kernel-doc too. Otherwise: > $ ./scripts/kernel-doc -none drivers/pwm/pwm-cros-ec.c > drivers/pwm/pwm-cros-ec.c:31: warning: Function parameter or member 'channel' > not described in 'cros_ec_pwm_device' > > I have no strong preference: please consider to use `channels` if it makes > sense. Thanks for your feedback. Regarding your suggestion about channel vs channels: While I don't have a strong preference either you have to choose between struct cros_ec_pwm *channel; which is singular but for all channels, or you have ec_pwm->channesl[pwm->hwpwm] which is plural but for only a single channel. So no matter how you do it, it's strange at one place or the other. While looking at the patch again I changed to ec_pwm->channel = devm_kcalloc(dev, chip->npwm, sizeof(*ec_pwm->channel), GFP_KERNEL) (from devm_kzalloc with size chip->npwm * sizeof(*ec_pwm->channel)). So in my tree I have the following change on top of the submission which I will send out as a v2 soon: diff --git a/drivers/pwm/pwm-cros-ec.c b/drivers/pwm/pwm-cros-ec.c index 2998153a5e92..63c64c4dbf90 100644 --- a/drivers/pwm/pwm-cros-ec.c +++ b/drivers/pwm/pwm-cros-ec.c @@ -21,6 +21,7 @@ * @ec: Pointer to EC device * @chip: PWM controller chip * @use_pwm_type: Use PWM types instead of generic channels + * @channel: array with per-channel data */ struct cros_ec_pwm_device { struct device *dev; @@ -295,7 +296,8 @@ static int cros_ec_pwm_probe(struct platform_device *pdev) chip->npwm = ret; } - ec_pwm->channel = devm_kzalloc(dev, chip->npwm * sizeof(*ec_pwm->channel), GFP_KERNEL); + ec_pwm->channel = devm_kcalloc(dev, chip->npwm, sizeof(*ec_pwm->channel), + GFP_KERNEL); if (!ec_pwm->channel) return -ENOMEM; I assume that also with this change it's ok to add your Reviewed-by tag. If you don't agree, please let me know. Best regards Uwe
diff --git a/drivers/pwm/pwm-cros-ec.c b/drivers/pwm/pwm-cros-ec.c index 74e863aa1d8d..2998153a5e92 100644 --- a/drivers/pwm/pwm-cros-ec.c +++ b/drivers/pwm/pwm-cros-ec.c @@ -27,6 +27,7 @@ struct cros_ec_pwm_device { struct cros_ec_device *ec; struct pwm_chip chip; bool use_pwm_type; + struct cros_ec_pwm *channel; }; /** @@ -42,26 +43,6 @@ static inline struct cros_ec_pwm_device *pwm_to_cros_ec_pwm(struct pwm_chip *c) return container_of(c, struct cros_ec_pwm_device, chip); } -static int cros_ec_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm) -{ - struct cros_ec_pwm *channel; - - channel = kzalloc(sizeof(*channel), GFP_KERNEL); - if (!channel) - return -ENOMEM; - - pwm_set_chip_data(pwm, channel); - - return 0; -} - -static void cros_ec_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm) -{ - struct cros_ec_pwm *channel = pwm_get_chip_data(pwm); - - kfree(channel); -} - static int cros_ec_dt_type_to_pwm_type(u8 dt_index, u8 *pwm_type) { switch (dt_index) { @@ -157,7 +138,7 @@ static int cros_ec_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, const struct pwm_state *state) { struct cros_ec_pwm_device *ec_pwm = pwm_to_cros_ec_pwm(chip); - struct cros_ec_pwm *channel = pwm_get_chip_data(pwm); + struct cros_ec_pwm *channel = &ec_pwm->channel[pwm->hwpwm]; u16 duty_cycle; int ret; @@ -187,7 +168,7 @@ static int cros_ec_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm, struct pwm_state *state) { struct cros_ec_pwm_device *ec_pwm = pwm_to_cros_ec_pwm(chip); - struct cros_ec_pwm *channel = pwm_get_chip_data(pwm); + struct cros_ec_pwm *channel = &ec_pwm->channel[pwm->hwpwm]; int ret; ret = cros_ec_pwm_get_duty(ec_pwm, pwm->hwpwm); @@ -236,8 +217,6 @@ cros_ec_pwm_xlate(struct pwm_chip *pc, const struct of_phandle_args *args) } static const struct pwm_ops cros_ec_pwm_ops = { - .request = cros_ec_pwm_request, - .free = cros_ec_pwm_free, .get_state = cros_ec_pwm_get_state, .apply = cros_ec_pwm_apply, .owner = THIS_MODULE, @@ -316,6 +295,10 @@ static int cros_ec_pwm_probe(struct platform_device *pdev) chip->npwm = ret; } + ec_pwm->channel = devm_kzalloc(dev, chip->npwm * sizeof(*ec_pwm->channel), GFP_KERNEL); + if (!ec_pwm->channel) + return -ENOMEM; + dev_dbg(dev, "Probed %u PWMs\n", chip->npwm); ret = pwmchip_add(chip);
Instead of an allocation of a single u16 per channel, allocate them all in a single chunk which greatly reduces memory fragmentation and also the overhead to track the allocated memory. Also put the channel data in driver data where it's cheaper to determine the address (no function call involved, just a trivial pointer addition). This also allows to get rid of the request and free callbacks. The only cost is that the channel data is allocated early, and even for unused channels. Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> --- If we'd know the number of channels already when memory is allocated for struct cros_ec_pwm_device *ec_pwm in cros_ec_pwm_probe, the per channel data could live directly in struct cros_ec_pwm_device which would further reduce the memory allocation overhead and fragmentation. drivers/pwm/pwm-cros-ec.c | 31 +++++++------------------------ 1 file changed, 7 insertions(+), 24 deletions(-)