| Message ID | 20231123175425.496956-1-u.kleine-koenig@pengutronix.de (mailing list archive) |
|---|---|
| Headers | show |
| Series | drm/bridge: ti-sn65dsi86: Some updates | expand |
Hi, On 23/11/2023 18:54, Uwe Kleine-König wrote: > Hello, > > this is a series I created while starring at the ti-sn65dsi86 driver in > the context of my pwm-lifetime series. > > The first patch should be fine. The last one has a few rough edges, but > maybe you like the direction this is going to? The 2nd patch probably > only makes sense if you also take the third. > > Best regards > Uwe > > Uwe Kleine-König (3): > drm/bridge: ti-sn65dsi86: Simplify using pm_runtime_resume_and_get() > drm/bridge: ti-sn65dsi86: Change parameters of > ti_sn65dsi86_{read,write}_u16 > drm/bridge: ti-sn65dsi86: Loosen coupling of PWM to ti-sn65dsi86 core > > drivers/gpu/drm/bridge/ti-sn65dsi86.c | 146 +++++++++++++++----------- > 1 file changed, 83 insertions(+), 63 deletions(-) > > base-commit: 4e87148f80d198ba5febcbcc969c6b9471099a09 It looks fine to me, even without the goal to move the driver to drivers/pwm I think it's same to move the pwm ddata out of the main pdata ans associate it to the pwm aux device lifetime. I don't anything wrong, and so far it's of for me, let's see if there's comments for other people before applying! Thanks, Neil
Hello, On Fri, Nov 24, 2023 at 09:56:55AM +0100, Neil Armstrong wrote: > On 23/11/2023 18:54, Uwe Kleine-König wrote: > > Hello, > > > > this is a series I created while starring at the ti-sn65dsi86 driver in > > the context of my pwm-lifetime series. > > > > The first patch should be fine. The last one has a few rough edges, but > > maybe you like the direction this is going to? The 2nd patch probably > > only makes sense if you also take the third. > > > > Best regards > > Uwe > > > > Uwe Kleine-König (3): > > drm/bridge: ti-sn65dsi86: Simplify using pm_runtime_resume_and_get() > > drm/bridge: ti-sn65dsi86: Change parameters of > > ti_sn65dsi86_{read,write}_u16 > > drm/bridge: ti-sn65dsi86: Loosen coupling of PWM to ti-sn65dsi86 core > > > > drivers/gpu/drm/bridge/ti-sn65dsi86.c | 146 +++++++++++++++----------- > > 1 file changed, 83 insertions(+), 63 deletions(-) > > > > base-commit: 4e87148f80d198ba5febcbcc969c6b9471099a09 > > It looks fine to me, even without the goal to move the driver to drivers/pwm > I think it's same to move the pwm ddata out of the main pdata ans associate > it to the pwm aux device lifetime. > > I don't anything wrong, and so far it's of for me, let's see if there's comments > for other people before applying! I like 1/3 very much, but as mentioned in a reply to 3/3, I'm not convinced by that one at all. Not only does it make the driver more complex for, I believe, very little gain (if any), usage of devm_kzalloc() in ti_sn_pwm_probe() is most likely wrong. Lifetime of driver-specific structures need to be handled through reference counting.
Hello Laurent, On Wed, Nov 29, 2023 at 02:45:33AM +0200, Laurent Pinchart wrote: > On Fri, Nov 24, 2023 at 09:56:55AM +0100, Neil Armstrong wrote: > > On 23/11/2023 18:54, Uwe Kleine-König wrote: > > > Hello, > > > > > > this is a series I created while starring at the ti-sn65dsi86 driver in > > > the context of my pwm-lifetime series. > > > > > > The first patch should be fine. The last one has a few rough edges, but > > > maybe you like the direction this is going to? The 2nd patch probably > > > only makes sense if you also take the third. > > > > > > Best regards > > > Uwe > > > > > > Uwe Kleine-König (3): > > > drm/bridge: ti-sn65dsi86: Simplify using pm_runtime_resume_and_get() > > > drm/bridge: ti-sn65dsi86: Change parameters of > > > ti_sn65dsi86_{read,write}_u16 > > > drm/bridge: ti-sn65dsi86: Loosen coupling of PWM to ti-sn65dsi86 core > > > > > > drivers/gpu/drm/bridge/ti-sn65dsi86.c | 146 +++++++++++++++----------- > > > 1 file changed, 83 insertions(+), 63 deletions(-) > > > > > > base-commit: 4e87148f80d198ba5febcbcc969c6b9471099a09 > > > > It looks fine to me, even without the goal to move the driver to drivers/pwm > > I think it's same to move the pwm ddata out of the main pdata ans associate > > it to the pwm aux device lifetime. > > > > I don't anything wrong, and so far it's of for me, let's see if there's comments > > for other people before applying! > > I like 1/3 very much, but as mentioned in a reply to 3/3, I'm not > convinced by that one at all. OK, I also wasn't completely sure that patches #2 and #3 are a good idea. The benefit I see is only better separation of the two drivers (which is very subjective) and making the main driver struct a bit smaller, saving some memory if the PWM isn't bound (which (maybe) saves very little memory in some corner cases only). > Not only does it make the driver more > complex for, I believe, very little gain (if any), usage of > devm_kzalloc() in ti_sn_pwm_probe() is most likely wrong. It's not more wrong that the same construct in nearly every other pwm driver. With my big life-time series for pwm[1] that idiom is fine. Best regards Uwe [1] https://lore.kernel.org/linux-pwm/20231121134901.208535-1-u.kleine-koenig@pengutronix.de
Hello, this is a series I created while starring at the ti-sn65dsi86 driver in the context of my pwm-lifetime series. The first patch should be fine. The last one has a few rough edges, but maybe you like the direction this is going to? The 2nd patch probably only makes sense if you also take the third. Best regards Uwe Uwe Kleine-König (3): drm/bridge: ti-sn65dsi86: Simplify using pm_runtime_resume_and_get() drm/bridge: ti-sn65dsi86: Change parameters of ti_sn65dsi86_{read,write}_u16 drm/bridge: ti-sn65dsi86: Loosen coupling of PWM to ti-sn65dsi86 core drivers/gpu/drm/bridge/ti-sn65dsi86.c | 146 +++++++++++++++----------- 1 file changed, 83 insertions(+), 63 deletions(-) base-commit: 4e87148f80d198ba5febcbcc969c6b9471099a09