Hello Andy,
On Tue, Jul 18, 2023 at 11:05:29PM +0300, Andy Shevchenko wrote:
> On Tue, Jul 18, 2023 at 08:18:32PM +0200, Uwe Kleine-König wrote:
> > This function allocates a struct pwm_chip and driver data. Compared to
> > the status quo the split into pwm_chip and driver data is new, otherwise
> > it doesn't change anything relevant (yet).
> >
> > The intention is that after all drivers are switched to use this
> > allocation function, its possible to add a struct device to struct
> > pwm_chip to properly track the latter's lifetime without touching all
> > drivers again. Proper lifetime tracking is a necessary precondition to
> > introduce character device support for PWMs (that implements atomic
> > setting and doesn't suffer from the sysfs overhead of the /sys/class/pwm
> > userspace support).
> >
> > The new function pwmchip_priv() (obviously?) only works for chips
> > allocated with devm_pwmchip_alloc().
>
> ...
>
> > +void *pwmchip_priv(struct pwm_chip *chip)
> > +{
> > + return (char *)chip + ALIGN(sizeof(*chip), 32);
>
> Why 32? I haven't found any explanation on the choice. I can understand arch
> minimum align, but hard coded value is a bit hard to get.
I copied that part from netdev_priv (without introducing the equivalent
of NETDEV_ALIGN). The 32 there isn't motivated in a comment, and i
didn't think much about it. Alternatives that I'm aware of are:
dma_get_cache_alignment() (spi)
Use a struct (counter's counter_device_allochelper uses ____cacheline_aligned)
(but I wonder about dma_get_cache_alignment() which might return 1 on
some archs and then doesn't even ensure natural aligning for shorts.)
> > +}
>
> ...
>
> > +struct pwm_chip *devm_pwmchip_alloc(struct device *parent, size_t sizeof_priv)
> > +{
> > + struct pwm_chip *chip;
> > + size_t alloc_size;
>
> > + alloc_size = ALIGN(sizeof(*chip), 32) + sizeof_priv;
>
> Ditto.
Of course this needs to match the above. spi uses dev_get_drvdata on the
controller's dev. Also a nice idea.
> Shouldn't it use a macro from overflow.h?
Yupp, that would make sense. Something like:
if (unlikely(check_add_overflow(ALIGN(sizeof(*chip), 32), sizeof_priv, &alloc_size)))
return NULL;
(with 32 replaced by something with a better name.)
Thanks for your feedback,
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
Attachment:
signature.asc
Description: PGP signature
