[PATCH 3/3] drm/bridge: ti-sn65dsi86: Loosen coupling of PWM to ti-sn65dsi86 core

[Laurent Pinchart]

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