| Message ID | 20211108000847.14320-1-granquet@baylibre.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | drm/mediatek/hdmi : split legacy driver and add mt8195 | expand |
Hi Chun-Kuang, Quoting Guillaume Ranquet (2021-11-08 01:08:45) > > Following the review of "[v1,4/4] drm/mediatek: add mt8195 hdmi TX support" [1] , I've been asked to try to merge the "legacy > HDMI driver with the "new" mt8195 HDMI driver. Though the registers and IPs are rather different, I've made an attempt at the > excercise on which I would like some comments before submitting a new version of the patchset. > > I've created a new mtk_hdmi_common.{c,h} which contains what I could identify as common enough to be added in there. > I have not renamed the mtk_hdmi.{c,h} driver to something else yet. > I've then added mtk8195 hdmi driver on top of this work. > > The code is still rather sketchy and I have some questions on best practices for that kind of exercices? > In no particular order, here's a bunch of random questions: > > 1 would you rather have: > - as presented here: a boolean with a bunch of if/else in the common code to handle the discrepencies between the two drivers > - function pointers, with an "ops" structure set in the drvdata... though the ops are a bit random and wouldn't really > make sense to be grouped together? > > 2 I'm not really happy with how the mt8195 driver and ddc are sharing the same __iomem regs and not sure how to exactly > handle this now that both hdmi drivers are tied together... it was hackish from the begining, but now I'm a bit out of > solutions to handle this peculiar "feature" of the mt8195 hdmi/ddc driver. any hints would be appreciated. > > 3 Should the "legacy" and mt8195 drivers be under the same menu entry? > > 4 I'm looking into the clock framework and I'm wondering if there would be benefits to switch to the "bulk" interface? as > both driver seem to request them all once and for all? the code might end up easier to read. > > 5 Would it be beneficial to further split the driver with audio on one side and video on the other? from what I understood, > the main complaint was that the driver was too big to review... that could help with that at least? > > > Thx for your patience... any further comment is welcome. > > Thx, > Guillaume. > > [1] https://patchwork.kernel.org/project/linux-mediatek/patch/20210929094425.745-5-granquet@baylibre.com/ > > -- > Guillaume Ranquet (2): > drm/mediatek: extract common functions from the mtk hdmi driver > drm/mediatek: add mtk8195 hdmi display driver > > drivers/gpu/drm/mediatek/Kconfig | 10 + > drivers/gpu/drm/mediatek/Makefile | 5 +- > drivers/gpu/drm/mediatek/mtk_hdmi.c | 635 +----- > drivers/gpu/drm/mediatek/mtk_hdmi_common.c | 521 +++++ > drivers/gpu/drm/mediatek/mtk_hdmi_common.h | 258 +++ > drivers/gpu/drm/mediatek/mtk_mt8195_hdmi.c | 1835 +++++++++++++++++ > drivers/gpu/drm/mediatek/mtk_mt8195_hdmi.h | 26 + > .../gpu/drm/mediatek/mtk_mt8195_hdmi_ddc.c | 530 +++++ > .../gpu/drm/mediatek/mtk_mt8195_hdmi_ddc.h | 20 + > .../gpu/drm/mediatek/mtk_mt8195_hdmi_regs.h | 329 +++ > 10 files changed, 3560 insertions(+), 609 deletions(-) > create mode 100644 drivers/gpu/drm/mediatek/mtk_hdmi_common.c > create mode 100644 drivers/gpu/drm/mediatek/mtk_hdmi_common.h > create mode 100644 drivers/gpu/drm/mediatek/mtk_mt8195_hdmi.c > create mode 100644 drivers/gpu/drm/mediatek/mtk_mt8195_hdmi.h > create mode 100644 drivers/gpu/drm/mediatek/mtk_mt8195_hdmi_ddc.c > create mode 100644 drivers/gpu/drm/mediatek/mtk_mt8195_hdmi_ddc.h > create mode 100644 drivers/gpu/drm/mediatek/mtk_mt8195_hdmi_regs.h > > -- > 2.32.0 > Since you have requested that I merge both drivers, could you give me your feedback on this RFC so that I know how to move forward? I'm planning on making a v2 soon based on this RFC (and hopefully its outcome). Thx, Guillaume.
Hi, Guillaume: On Thu, 2021-12-02 at 07:44 -0800, Guillaume Ranquet wrote: > Hi Chun-Kuang, > Quoting Guillaume Ranquet (2021-11-08 01:08:45) > > > > Following the review of "[v1,4/4] drm/mediatek: add mt8195 hdmi TX > > support" [1] , I've been asked to try to merge the "legacy > > HDMI driver with the "new" mt8195 HDMI driver. Though the registers > > and IPs are rather different, I've made an attempt at the > > excercise on which I would like some comments before submitting a > > new version of the patchset. > > > > I've created a new mtk_hdmi_common.{c,h} which contains what I > > could identify as common enough to be added in there. > > I have not renamed the mtk_hdmi.{c,h} driver to something else yet. > > I've then added mtk8195 hdmi driver on top of this work. > > > > The code is still rather sketchy and I have some questions on best > > practices for that kind of exercices? > > In no particular order, here's a bunch of random questions: > > > > 1 would you rather have: > > - as presented here: a boolean with a bunch of if/else in the > > common code to handle the discrepencies between the two drivers > > - function pointers, with an "ops" structure set in the drvdata... > > though the ops are a bit random and wouldn't really > > make sense to be grouped together? Both is OK, but I think if some difference is small, use if/else is much simple. If the difference is large, use function pointers is better. > > > > 2 I'm not really happy with how the mt8195 driver and ddc are > > sharing the same __iomem regs and not sure how to exactly > > handle this now that both hdmi drivers are tied together... it was > > hackish from the begining, but now I'm a bit out of > > solutions to handle this peculiar "feature" of the mt8195 hdmi/ddc > > driver. any hints would be appreciated. Is the case the same as phy-mtk-dp with mtk display port driver? If it's this case, mt8195 hdmi driver would use regmap_read/write() and mt8173 hdmi would use readl/writel() and you could use if/else or function pointer to distinguish both SoC. Or you worry about other thing? > > > > 3 Should the "legacy" and mt8195 drivers be under the same menu > > entry? Let's call "legacy" mt8173. If you really want config for mt8195 hdmi driver, yes, it should be under the same menu. But also have a config for mt8173 hdmi driver. > > > > 4 I'm looking into the clock framework and I'm wondering if there > > would be benefits to switch to the "bulk" interface? as > > both driver seem to request them all once and for all? the code > > might end up easier to read. DRM framework has bulk of helper function, and it's not easy to read the DRM framework. These helper functions are help to reduce the duplicated code. I think the limitation of readability is: I could understand the code. Based on the limitation, let's try to reduce duplicated code. > > > > 5 Would it be beneficial to further split the driver with audio on > > one side and video on the other? from what I understood, > > the main complaint was that the driver was too big to review... > > that could help with that at least? It's better that you could break this patch into smaller patches. But be sure that each patch is workable. For example, if apply video patch first, HDMI could work without audio. > > > > > > Thx for your patience... any further comment is welcome. I would review the RFC patch later. Regards, CK > > > > Thx, > > Guillaume. > > > > [1] > > https://urldefense.com/v3/__https://patchwork.kernel.org/project/linux-mediatek/patch/20210929094425.745-5-granquet@baylibre.com/__;!!CTRNKA9wMg0ARbw!0NxH5ig8YRgHlPWYZxLV17O_pVKkKxQK6Mimo0r89xiEmOyTWx-DUEliMqFScg$ > > > > > > -- > > Guillaume Ranquet (2): > > drm/mediatek: extract common functions from the mtk hdmi driver > > drm/mediatek: add mtk8195 hdmi display driver > > > > drivers/gpu/drm/mediatek/Kconfig | 10 + > > drivers/gpu/drm/mediatek/Makefile | 5 +- > > drivers/gpu/drm/mediatek/mtk_hdmi.c | 635 +----- > > drivers/gpu/drm/mediatek/mtk_hdmi_common.c | 521 +++++ > > drivers/gpu/drm/mediatek/mtk_hdmi_common.h | 258 +++ > > drivers/gpu/drm/mediatek/mtk_mt8195_hdmi.c | 1835 > > +++++++++++++++++ > > drivers/gpu/drm/mediatek/mtk_mt8195_hdmi.h | 26 + > > .../gpu/drm/mediatek/mtk_mt8195_hdmi_ddc.c | 530 +++++ > > .../gpu/drm/mediatek/mtk_mt8195_hdmi_ddc.h | 20 + > > .../gpu/drm/mediatek/mtk_mt8195_hdmi_regs.h | 329 +++ > > 10 files changed, 3560 insertions(+), 609 deletions(-) > > create mode 100644 drivers/gpu/drm/mediatek/mtk_hdmi_common.c > > create mode 100644 drivers/gpu/drm/mediatek/mtk_hdmi_common.h > > create mode 100644 drivers/gpu/drm/mediatek/mtk_mt8195_hdmi.c > > create mode 100644 drivers/gpu/drm/mediatek/mtk_mt8195_hdmi.h > > create mode 100644 drivers/gpu/drm/mediatek/mtk_mt8195_hdmi_ddc.c > > create mode 100644 drivers/gpu/drm/mediatek/mtk_mt8195_hdmi_ddc.h > > create mode 100644 drivers/gpu/drm/mediatek/mtk_mt8195_hdmi_regs.h > > > > -- > > 2.32.0 > > > > Since you have requested that I merge both drivers, could you give me > your feedback on this RFC so > that I know how to move forward? I'm planning on making a v2 soon > based on this RFC (and hopefully its > outcome). > > Thx, > Guillaume. > > _______________________________________________ > Linux-mediatek mailing list > Linux-mediatek@lists.infradead.org > https://urldefense.com/v3/__http://lists.infradead.org/mailman/listinfo/linux-mediatek__;!!CTRNKA9wMg0ARbw!0NxH5ig8YRgHlPWYZxLV17O_pVKkKxQK6Mimo0r89xiEmOyTWx-DUEkrO9clHw$ >
Following the review of "[v1,4/4] drm/mediatek: add mt8195 hdmi TX support" [1] , I've been asked to try to merge the "legacy HDMI driver with the "new" mt8195 HDMI driver. Though the registers and IPs are rather different, I've made an attempt at the excercise on which I would like some comments before submitting a new version of the patchset. I've created a new mtk_hdmi_common.{c,h} which contains what I could identify as common enough to be added in there. I have not renamed the mtk_hdmi.{c,h} driver to something else yet. I've then added mtk8195 hdmi driver on top of this work. The code is still rather sketchy and I have some questions on best practices for that kind of exercices? In no particular order, here's a bunch of random questions: 1 would you rather have: - as presented here: a boolean with a bunch of if/else in the common code to handle the discrepencies between the two drivers - function pointers, with an "ops" structure set in the drvdata... though the ops are a bit random and wouldn't really make sense to be grouped together? 2 I'm not really happy with how the mt8195 driver and ddc are sharing the same __iomem regs and not sure how to exactly handle this now that both hdmi drivers are tied together... it was hackish from the begining, but now I'm a bit out of solutions to handle this peculiar "feature" of the mt8195 hdmi/ddc driver. any hints would be appreciated. 3 Should the "legacy" and mt8195 drivers be under the same menu entry? 4 I'm looking into the clock framework and I'm wondering if there would be benefits to switch to the "bulk" interface? as both driver seem to request them all once and for all? the code might end up easier to read. 5 Would it be beneficial to further split the driver with audio on one side and video on the other? from what I understood, the main complaint was that the driver was too big to review... that could help with that at least? Thx for your patience... any further comment is welcome. Thx, Guillaume. [1] https://patchwork.kernel.org/project/linux-mediatek/patch/20210929094425.745-5-granquet@baylibre.com/