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

[Doug Anderson]

Hi,

On Thu, Nov 23, 2023 at 9:54 AM Uwe Kleine-König
<u.kleine-koenig at pengutronix.de> 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...

> +/*
> + * 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.


-Doug