On Tue, Nov 28, 2023 at 04:32:10PM -0800, Doug Anderson wrote: > On Thu, Nov 23, 2023 at 9:54 AM Uwe Kleine-König wrote: > > > > Introduce a dedicated private data structure for the pwm aux driver > > provided by the sn65dsi86 driver. This way data needed for PWM operation > > is (to a certain degree) nicely separated and doesn't occupy memory in > > the ti_sn65dsi86 core's private data if the PWM isn't used. > > I suspect we still end up at a loss memory-wise. All of the extra code > + the overhead of another kmalloc seems like it would take up more > space than the tiny bit of data in the structure. > > > > The eventual goal is to decouple the PWM driver completely from the > > ti-sn65dsi86 core and maybe even move it to a dedicated driver below > > drivers/pwm. There are a few obstacles to that quest though: > > > > - The busy pin check (implemented in ti_sn_pwm_pin_request() and > > ti_sn_pwm_pin_release()) would need to be available unconditionally. > > > > - The refclk should probably abstracted by a struct clk such that the > > pwm_refclk_freq member that currently still lives in ti-sn65dsi86 > > core driver data can be dropped. > > Right that the above could be done with more abstraction layers. I > guess the question I have is: how much do we gain with that? > > Personally I'm not really sold on the idea. If others think this is a > great change then I won't stand in the way, but IMO without a > compelling reason this is just extra abstraction / complexity without > any gain... I'm not convinced either, especially on moving to a separate driver, but even when it comes to dynamically allocating a new structure. Splitting the PWM fields to a new ti_sn65dsi86_pwm would be fine (and I think would increase readibility), but we can then simply embed it in ti_sn65dsi86. > > +/* > > + * struct ti_sn65dsi86_pwm_ddata - Platform data for ti-sn65dsi86 pwm driver. > > Why "ddata" exactly? It seems like this is just the pwm "data" ? > > > > + * @chip: pwm_chip if the PWM is exposed. > > + * @pwm_enabled: Used to track if the PWM signal is currently enabled. > > + * @regmap: Regmap for accessing i2c. > > + * @pdata: platform data of the parent device > > "pdata" isn't a member of the struct, but "pwm_refclk_freq" is. -- Regards, Laurent Pinchart
