Re: [PATCH v22 2/2] drm/bridge: Add I2C based driver for ps8640 bridge
Hi Ezequiel, On 26/12/19 17:38, Ezequiel Garcia wrote: > Hi Enric, > > Note that this series is marked as v22, but it's really the 23th. > Some minor comments below, it's looking really now. > > Reviewed-by: Ezequiel Garcia > > On Mon, 2019-12-23 at 15:35 +0100, Enric Balletbo i Serra wrote: >> From: Jitao Shi >> >> This patch adds drm_bridge driver for parade DSI to eDP bridge chip. >> >> Signed-off-by: Jitao Shi >> Reviewed-by: Daniel Kurtz >> [uli: followed API changes, removed FW update feature] >> Signed-off-by: Ulrich Hecht >> Signed-off-by: Enric Balletbo i Serra >> Tested-by: Hsin-Yi Wang >> --- >> One of the reviews from Laurent was to use 'i2c_new_ancillary_device'. I >> didn't change this for two reasons. >> 1) It doesn't have a devm version, so the remove path is more simple >> using the devm_i2c_new_dummy_device family. >> 2) IIUC the ancillary function is useful when you want to retrieve the >> address from the firmware or DT, that's not really the case here, as we >> have a base address and fixed offset to the base address which I think >> is not configurable. >> >> Let me know if you still think that I should use the ancillary call. >> >> Changes in v23: >> - Merge mute/unmute functions into one (Nicolas Boichat) >> - Use enum for ENABLE/DISABLE instead of bool (Ezequiel Garcia) >> - Rename mute/unmute to vdo_control and fix error messages (Nicolas Boichat >> and Enric) >> - Add space between address and address parameter 'address%02x' (Nicolas >> Boichat) >> - Add Tested-by Hsin-Yi >> - Added me as author after the refactor >> >> Changes in v22: >> - Remove sysfs attributes because are not really used (Enric Balletbo) >> - Use enum for address page offsets (Ezequiel Garcia) >> - Remove enable tracking (Enric Balletbo) >> - Use panel_bridge API (Laurent Pinchart) >> - Do not use kernel-doc format for non kernel-doc formatted commands (Enric >> Balletbo) >> - Remove verbose message for PAGE1_VSTART command (Ezequiel Garcia) >> - Use time_is_after_jiffies idiom (Ezequiel Garcia) >> - Remove unused macros (Ezequiel Garcia) >> - Fix weird alignment in dsi->mode_flags (Laurent Pinchart) >> - Use drm_of_find_panel_or_bridge helper (Laurent Pinchart) >> - Remove mode-sel-gpios as is not used (Laurent Pinchart) >> - Remove error messages to get gpios as the core will already report it >> (Enric Balletbo) >> - Remove redundant message getting the regulators (Laurent Pinchart) >> - Rename sleep-gpios to powerdown-gpios (Laurent Pinchart) >> - Use ARRAY_SIZE(ps_bridge->page) instead of MAX_DEV when possible (Laurent >> Pinchart) >> - Fix race with userspace accessing the sysfs attributes (Laurent Pinchart) >> - Remove id_table as is only used on DR platforms (Laurent Pinchart) >> - Convert to new i2c device probe() (Laurent Pinchart) >> - Use i2c_smbus_read/write helpers instead of open coding it (Laurent >> Pinchart) >> - Remove unnused global variables (Laurent Pinchart) >> - Remove unnused fields in ps8640 struct (Laurent Pinchart) >> - Remove commented-out headers (Laurent Pinchart) >> >> Changes in v21: >> - Use devm_i2c_new_dummy_device and fix build issue using deprecated >> i2c_new_dummy >> - Fix build issue due missing drm_bridge.h >> - Do not remove in ps8640_remove device managed resources >> >> Changes in v19: >> - fixed return value of ps8640_probe() when no panel is found >> >> Changes in v18: >> - followed DRM API changes >> - use DEVICE_ATTR_RO() >> - remove firmware update code >> - add SPDX identifier >> >> Changes in v17: >> - remove some unused head files. >> - add macros for ps8640 pages. >> - remove ddc_i2c client >> - add mipi_dsi_device_register_full >> - remove the manufacturer from the name and i2c_device_id >> >> Changes in v16: >> - Disable ps8640 DSI MCS Function. >> - Rename gpios name more clearly. >> - Tune the ps8640 power on sequence. >> >> Changes in v15: >> - Drop drm_connector_(un)register calls from parade ps8640. >>The main DRM driver mtk_drm_drv now calls >>drm_connector_register_all() after drm_dev_register() in the >>mtk_drm_bind() function. That function should iterate over all >>connectors and call drm_connector_register() for each of them. >>So, remove drm_connector_(un)register calls from parade ps8640. >> >> Changes in v14: >> - update copyright info. >> - change bridge_to_ps8640 and connector_to_ps8640 to inline function. >> - fix some coding style. >> - use sizeof as array counter. >> - use drm_get_edid when read edid. >> - add mutex when firmware updating. >> >> Changes in v13: >> - add const on data, ps8640_write_bytes(struct i2c_client *client, const u8 >> *data, u16 data_len) >> - fix PAGE2_SW_REST tyro. >> - move the buf[3] init to entrance of the function. >> >> Changes in v12: >> - fix hw_chip_id build warning >> >> Changes in v11: >> - Remove depends on I2C, add DRM depends >> - Reuse ps8640_write_bytes() in ps8640_write_byte() >> - Use timer check for polling like the
Re: [PATCH v22 2/2] drm/bridge: Add I2C based driver for ps8640 bridge
Hi Enric, Note that this series is marked as v22, but it's really the 23th. Some minor comments below, it's looking really now. Reviewed-by: Ezequiel Garcia On Mon, 2019-12-23 at 15:35 +0100, Enric Balletbo i Serra wrote: > From: Jitao Shi > > This patch adds drm_bridge driver for parade DSI to eDP bridge chip. > > Signed-off-by: Jitao Shi > Reviewed-by: Daniel Kurtz > [uli: followed API changes, removed FW update feature] > Signed-off-by: Ulrich Hecht > Signed-off-by: Enric Balletbo i Serra > Tested-by: Hsin-Yi Wang > --- > One of the reviews from Laurent was to use 'i2c_new_ancillary_device'. I > didn't change this for two reasons. > 1) It doesn't have a devm version, so the remove path is more simple > using the devm_i2c_new_dummy_device family. > 2) IIUC the ancillary function is useful when you want to retrieve the > address from the firmware or DT, that's not really the case here, as we > have a base address and fixed offset to the base address which I think > is not configurable. > > Let me know if you still think that I should use the ancillary call. > > Changes in v23: > - Merge mute/unmute functions into one (Nicolas Boichat) > - Use enum for ENABLE/DISABLE instead of bool (Ezequiel Garcia) > - Rename mute/unmute to vdo_control and fix error messages (Nicolas Boichat > and Enric) > - Add space between address and address parameter 'address%02x' (Nicolas > Boichat) > - Add Tested-by Hsin-Yi > - Added me as author after the refactor > > Changes in v22: > - Remove sysfs attributes because are not really used (Enric Balletbo) > - Use enum for address page offsets (Ezequiel Garcia) > - Remove enable tracking (Enric Balletbo) > - Use panel_bridge API (Laurent Pinchart) > - Do not use kernel-doc format for non kernel-doc formatted commands (Enric > Balletbo) > - Remove verbose message for PAGE1_VSTART command (Ezequiel Garcia) > - Use time_is_after_jiffies idiom (Ezequiel Garcia) > - Remove unused macros (Ezequiel Garcia) > - Fix weird alignment in dsi->mode_flags (Laurent Pinchart) > - Use drm_of_find_panel_or_bridge helper (Laurent Pinchart) > - Remove mode-sel-gpios as is not used (Laurent Pinchart) > - Remove error messages to get gpios as the core will already report it > (Enric Balletbo) > - Remove redundant message getting the regulators (Laurent Pinchart) > - Rename sleep-gpios to powerdown-gpios (Laurent Pinchart) > - Use ARRAY_SIZE(ps_bridge->page) instead of MAX_DEV when possible (Laurent > Pinchart) > - Fix race with userspace accessing the sysfs attributes (Laurent Pinchart) > - Remove id_table as is only used on DR platforms (Laurent Pinchart) > - Convert to new i2c device probe() (Laurent Pinchart) > - Use i2c_smbus_read/write helpers instead of open coding it (Laurent > Pinchart) > - Remove unnused global variables (Laurent Pinchart) > - Remove unnused fields in ps8640 struct (Laurent Pinchart) > - Remove commented-out headers (Laurent Pinchart) > > Changes in v21: > - Use devm_i2c_new_dummy_device and fix build issue using deprecated > i2c_new_dummy > - Fix build issue due missing drm_bridge.h > - Do not remove in ps8640_remove device managed resources > > Changes in v19: > - fixed return value of ps8640_probe() when no panel is found > > Changes in v18: > - followed DRM API changes > - use DEVICE_ATTR_RO() > - remove firmware update code > - add SPDX identifier > > Changes in v17: > - remove some unused head files. > - add macros for ps8640 pages. > - remove ddc_i2c client > - add mipi_dsi_device_register_full > - remove the manufacturer from the name and i2c_device_id > > Changes in v16: > - Disable ps8640 DSI MCS Function. > - Rename gpios name more clearly. > - Tune the ps8640 power on sequence. > > Changes in v15: > - Drop drm_connector_(un)register calls from parade ps8640. >The main DRM driver mtk_drm_drv now calls >drm_connector_register_all() after drm_dev_register() in the >mtk_drm_bind() function. That function should iterate over all >connectors and call drm_connector_register() for each of them. >So, remove drm_connector_(un)register calls from parade ps8640. > > Changes in v14: > - update copyright info. > - change bridge_to_ps8640 and connector_to_ps8640 to inline function. > - fix some coding style. > - use sizeof as array counter. > - use drm_get_edid when read edid. > - add mutex when firmware updating. > > Changes in v13: > - add const on data, ps8640_write_bytes(struct i2c_client *client, const u8 > *data, u16 data_len) > - fix PAGE2_SW_REST tyro. > - move the buf[3] init to entrance of the function. > > Changes in v12: > - fix hw_chip_id build warning > > Changes in v11: > - Remove depends on I2C, add DRM depends > - Reuse ps8640_write_bytes() in ps8640_write_byte() > - Use timer check for polling like the routines in > - Fix no drm_connector_unregister/drm_connector_cleanup when > ps8640_bridge_attach fail > - Check the ps8640 hardware id in ps8640_validate_firmware > - Remove
[PATCH v22 2/2] drm/bridge: Add I2C based driver for ps8640 bridge
From: Jitao Shi This patch adds drm_bridge driver for parade DSI to eDP bridge chip. Signed-off-by: Jitao Shi Reviewed-by: Daniel Kurtz [uli: followed API changes, removed FW update feature] Signed-off-by: Ulrich Hecht Signed-off-by: Enric Balletbo i Serra Tested-by: Hsin-Yi Wang --- One of the reviews from Laurent was to use 'i2c_new_ancillary_device'. I didn't change this for two reasons. 1) It doesn't have a devm version, so the remove path is more simple using the devm_i2c_new_dummy_device family. 2) IIUC the ancillary function is useful when you want to retrieve the address from the firmware or DT, that's not really the case here, as we have a base address and fixed offset to the base address which I think is not configurable. Let me know if you still think that I should use the ancillary call. Changes in v23: - Merge mute/unmute functions into one (Nicolas Boichat) - Use enum for ENABLE/DISABLE instead of bool (Ezequiel Garcia) - Rename mute/unmute to vdo_control and fix error messages (Nicolas Boichat and Enric) - Add space between address and address parameter 'address%02x' (Nicolas Boichat) - Add Tested-by Hsin-Yi - Added me as author after the refactor Changes in v22: - Remove sysfs attributes because are not really used (Enric Balletbo) - Use enum for address page offsets (Ezequiel Garcia) - Remove enable tracking (Enric Balletbo) - Use panel_bridge API (Laurent Pinchart) - Do not use kernel-doc format for non kernel-doc formatted commands (Enric Balletbo) - Remove verbose message for PAGE1_VSTART command (Ezequiel Garcia) - Use time_is_after_jiffies idiom (Ezequiel Garcia) - Remove unused macros (Ezequiel Garcia) - Fix weird alignment in dsi->mode_flags (Laurent Pinchart) - Use drm_of_find_panel_or_bridge helper (Laurent Pinchart) - Remove mode-sel-gpios as is not used (Laurent Pinchart) - Remove error messages to get gpios as the core will already report it (Enric Balletbo) - Remove redundant message getting the regulators (Laurent Pinchart) - Rename sleep-gpios to powerdown-gpios (Laurent Pinchart) - Use ARRAY_SIZE(ps_bridge->page) instead of MAX_DEV when possible (Laurent Pinchart) - Fix race with userspace accessing the sysfs attributes (Laurent Pinchart) - Remove id_table as is only used on DR platforms (Laurent Pinchart) - Convert to new i2c device probe() (Laurent Pinchart) - Use i2c_smbus_read/write helpers instead of open coding it (Laurent Pinchart) - Remove unnused global variables (Laurent Pinchart) - Remove unnused fields in ps8640 struct (Laurent Pinchart) - Remove commented-out headers (Laurent Pinchart) Changes in v21: - Use devm_i2c_new_dummy_device and fix build issue using deprecated i2c_new_dummy - Fix build issue due missing drm_bridge.h - Do not remove in ps8640_remove device managed resources Changes in v19: - fixed return value of ps8640_probe() when no panel is found Changes in v18: - followed DRM API changes - use DEVICE_ATTR_RO() - remove firmware update code - add SPDX identifier Changes in v17: - remove some unused head files. - add macros for ps8640 pages. - remove ddc_i2c client - add mipi_dsi_device_register_full - remove the manufacturer from the name and i2c_device_id Changes in v16: - Disable ps8640 DSI MCS Function. - Rename gpios name more clearly. - Tune the ps8640 power on sequence. Changes in v15: - Drop drm_connector_(un)register calls from parade ps8640. The main DRM driver mtk_drm_drv now calls drm_connector_register_all() after drm_dev_register() in the mtk_drm_bind() function. That function should iterate over all connectors and call drm_connector_register() for each of them. So, remove drm_connector_(un)register calls from parade ps8640. Changes in v14: - update copyright info. - change bridge_to_ps8640 and connector_to_ps8640 to inline function. - fix some coding style. - use sizeof as array counter. - use drm_get_edid when read edid. - add mutex when firmware updating. Changes in v13: - add const on data, ps8640_write_bytes(struct i2c_client *client, const u8 *data, u16 data_len) - fix PAGE2_SW_REST tyro. - move the buf[3] init to entrance of the function. Changes in v12: - fix hw_chip_id build warning Changes in v11: - Remove depends on I2C, add DRM depends - Reuse ps8640_write_bytes() in ps8640_write_byte() - Use timer check for polling like the routines in - Fix no drm_connector_unregister/drm_connector_cleanup when ps8640_bridge_attach fail - Check the ps8640 hardware id in ps8640_validate_firmware - Remove fw_version check - Move ps8640_validate_firmware before ps8640_enter_bl - Add ddc_i2c unregister when probe fail and ps8640_remove drivers/gpu/drm/bridge/Kconfig | 11 + drivers/gpu/drm/bridge/Makefile| 1 + drivers/gpu/drm/bridge/parade-ps8640.c | 348 + 3 files changed, 360 insertions(+) create mode 100644 drivers/gpu/drm/bridge/parade-ps8640.c diff --git a/drivers/gpu/drm/bridge/Kconfig
Re: [PATCH v22 2/2] drm/bridge: Add I2C based driver for ps8640 bridge
Hi Laurent, On 23/12/19 15:35, Enric Balletbo i Serra wrote: > From: Jitao Shi > > This patch adds drm_bridge driver for parade DSI to eDP bridge chip. > > Signed-off-by: Jitao Shi > Reviewed-by: Daniel Kurtz > [uli: followed API changes, removed FW update feature] > Signed-off-by: Ulrich Hecht > Signed-off-by: Enric Balletbo i Serra > Tested-by: Hsin-Yi Wang > --- > One of the reviews from Laurent was to use 'i2c_new_ancillary_device'. I > didn't change this for two reasons. > 1) It doesn't have a devm version, so the remove path is more simple > using the devm_i2c_new_dummy_device family. > 2) IIUC the ancillary function is useful when you want to retrieve the > address from the firmware or DT, that's not really the case here, as we > have a base address and fixed offset to the base address which I think > is not configurable. > > Let me know if you still think that I should use the ancillary call. > > Changes in v23: > - Merge mute/unmute functions into one (Nicolas Boichat) > - Use enum for ENABLE/DISABLE instead of bool (Ezequiel Garcia) > - Rename mute/unmute to vdo_control and fix error messages (Nicolas Boichat > and Enric) > - Add space between address and address parameter 'address%02x' (Nicolas > Boichat) > - Add Tested-by Hsin-Yi > - Added me as author after the refactor > > Changes in v22: > - Remove sysfs attributes because are not really used (Enric Balletbo) > - Use enum for address page offsets (Ezequiel Garcia) > - Remove enable tracking (Enric Balletbo) > - Use panel_bridge API (Laurent Pinchart) > - Do not use kernel-doc format for non kernel-doc formatted commands (Enric > Balletbo) > - Remove verbose message for PAGE1_VSTART command (Ezequiel Garcia) > - Use time_is_after_jiffies idiom (Ezequiel Garcia) > - Remove unused macros (Ezequiel Garcia) > - Fix weird alignment in dsi->mode_flags (Laurent Pinchart) > - Use drm_of_find_panel_or_bridge helper (Laurent Pinchart) > - Remove mode-sel-gpios as is not used (Laurent Pinchart) > - Remove error messages to get gpios as the core will already report it > (Enric Balletbo) > - Remove redundant message getting the regulators (Laurent Pinchart) > - Rename sleep-gpios to powerdown-gpios (Laurent Pinchart) > - Use ARRAY_SIZE(ps_bridge->page) instead of MAX_DEV when possible (Laurent > Pinchart) > - Fix race with userspace accessing the sysfs attributes (Laurent Pinchart) > - Remove id_table as is only used on DR platforms (Laurent Pinchart) > - Convert to new i2c device probe() (Laurent Pinchart) > - Use i2c_smbus_read/write helpers instead of open coding it (Laurent > Pinchart) > - Remove unnused global variables (Laurent Pinchart) > - Remove unnused fields in ps8640 struct (Laurent Pinchart) > - Remove commented-out headers (Laurent Pinchart) > > Changes in v21: > - Use devm_i2c_new_dummy_device and fix build issue using deprecated > i2c_new_dummy > - Fix build issue due missing drm_bridge.h > - Do not remove in ps8640_remove device managed resources > > Changes in v19: > - fixed return value of ps8640_probe() when no panel is found > > Changes in v18: > - followed DRM API changes > - use DEVICE_ATTR_RO() > - remove firmware update code > - add SPDX identifier > > Changes in v17: > - remove some unused head files. > - add macros for ps8640 pages. > - remove ddc_i2c client > - add mipi_dsi_device_register_full > - remove the manufacturer from the name and i2c_device_id > > Changes in v16: > - Disable ps8640 DSI MCS Function. > - Rename gpios name more clearly. > - Tune the ps8640 power on sequence. > > Changes in v15: > - Drop drm_connector_(un)register calls from parade ps8640. >The main DRM driver mtk_drm_drv now calls >drm_connector_register_all() after drm_dev_register() in the >mtk_drm_bind() function. That function should iterate over all >connectors and call drm_connector_register() for each of them. >So, remove drm_connector_(un)register calls from parade ps8640. > > Changes in v14: > - update copyright info. > - change bridge_to_ps8640 and connector_to_ps8640 to inline function. > - fix some coding style. > - use sizeof as array counter. > - use drm_get_edid when read edid. > - add mutex when firmware updating. > > Changes in v13: > - add const on data, ps8640_write_bytes(struct i2c_client *client, const u8 > *data, u16 data_len) > - fix PAGE2_SW_REST tyro. > - move the buf[3] init to entrance of the function. > > Changes in v12: > - fix hw_chip_id build warning > > Changes in v11: > - Remove depends on I2C, add DRM depends > - Reuse ps8640_write_bytes() in ps8640_write_byte() > - Use timer check for polling like the routines in > - Fix no drm_connector_unregister/drm_connector_cleanup when > ps8640_bridge_attach fail > - Check the ps8640 hardware id in ps8640_validate_firmware > - Remove fw_version check > - Move ps8640_validate_firmware before ps8640_enter_bl > - Add ddc_i2c unregister when probe fail and ps8640_remove > >
Re: [PATCH v22 2/2] drm/bridge: Add I2C based driver for ps8640 bridge
Hi Nicolas,
On 23/12/19 10:14, Nicolas Boichat wrote:
> On Mon, Dec 23, 2019 at 3:10 PM Enric Balletbo i Serra
> wrote:
>>
>> Hi Nicolas,
>>
>> Many thanks for you review. Just preparing a new version with your comments
>> addressed.
>>
>> On 20/12/19 9:44, Nicolas Boichat wrote:
>>> On Fri, Dec 20, 2019 at 4:17 PM Enric Balletbo i Serra
>>> wrote:
From: Jitao Shi
This patch adds drm_bridge driver for parade DSI to eDP bridge chip.
Signed-off-by: Jitao Shi
Reviewed-by: Daniel Kurtz
Reviewed-by: Enric Balletbo i Serra
[uli: followed API changes, removed FW update feature]
Signed-off-by: Ulrich Hecht
Signed-off-by: Enric Balletbo i Serra
---
>> [snip]
+ ret = i2c_smbus_write_byte_data(client, PAGE2_MCS_EN,
+ status & ~MCS_EN);
+ if (ret < 0) {
+ DRM_ERROR("failed write PAGE2_MCS_EN: %d\n", ret);
+ goto err_regulators_disable;
+ }
+
+ ret = ps8640_bridge_unmute(ps_bridge);
+ if (ret)
+ DRM_ERROR("failed to enable unmutevideo: %d\n", ret);
>>>
>>> failed to unmute? Or failed to enable?
>>>
>>
>> failed to unmute sound more clear to me.
>
> I may be wrong, but I have the feeling that the functions
> "mute/unmute" video/display, actually... And that the function naming
> is strange...
>
Yes, that's strange.
> You could just try to remove the calls, as there is no audio on the
> board you have (elm), so if video still works, maybe this is actually
> audio ,-)
>
And without those the display doesn't work. So I suspect that what is wrong and
confusing is the message, instead of mute/unmute, and based on the register
names this looks more like an internal regulator that you need to enable and
disable, so I'll change the error message accordingly.
Thanks,
Enric
> Thanks,
>
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v22 2/2] drm/bridge: Add I2C based driver for ps8640 bridge
On Mon, Dec 23, 2019 at 3:10 PM Enric Balletbo i Serra
wrote:
>
> Hi Nicolas,
>
> Many thanks for you review. Just preparing a new version with your comments
> addressed.
>
> On 20/12/19 9:44, Nicolas Boichat wrote:
> > On Fri, Dec 20, 2019 at 4:17 PM Enric Balletbo i Serra
> > wrote:
> >>
> >> From: Jitao Shi
> >>
> >> This patch adds drm_bridge driver for parade DSI to eDP bridge chip.
> >>
> >> Signed-off-by: Jitao Shi
> >> Reviewed-by: Daniel Kurtz
> >> Reviewed-by: Enric Balletbo i Serra
> >> [uli: followed API changes, removed FW update feature]
> >> Signed-off-by: Ulrich Hecht
> >> Signed-off-by: Enric Balletbo i Serra
> >> ---
> [snip]
> >> + ret = i2c_smbus_write_byte_data(client, PAGE2_MCS_EN,
> >> + status & ~MCS_EN);
> >> + if (ret < 0) {
> >> + DRM_ERROR("failed write PAGE2_MCS_EN: %d\n", ret);
> >> + goto err_regulators_disable;
> >> + }
> >> +
> >> + ret = ps8640_bridge_unmute(ps_bridge);
> >> + if (ret)
> >> + DRM_ERROR("failed to enable unmutevideo: %d\n", ret);
> >
> > failed to unmute? Or failed to enable?
> >
>
> failed to unmute sound more clear to me.
I may be wrong, but I have the feeling that the functions
"mute/unmute" video/display, actually... And that the function naming
is strange...
You could just try to remove the calls, as there is no audio on the
board you have (elm), so if video still works, maybe this is actually
audio ,-)
Thanks,
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v22 2/2] drm/bridge: Add I2C based driver for ps8640 bridge
On Fri, Dec 20, 2019 at 4:17 PM Enric Balletbo i Serra wrote: > > From: Jitao Shi > > This patch adds drm_bridge driver for parade DSI to eDP bridge chip. > > Signed-off-by: Jitao Shi > Reviewed-by: Daniel Kurtz > Reviewed-by: Enric Balletbo i Serra > [uli: followed API changes, removed FW update feature] > Signed-off-by: Ulrich Hecht > Signed-off-by: Enric Balletbo i Serra Tested-by: Hsin-Yi Wang > --- tested on mt8173 chromebook ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v22 2/2] drm/bridge: Add I2C based driver for ps8640 bridge
From: Jitao Shi This patch adds drm_bridge driver for parade DSI to eDP bridge chip. Signed-off-by: Jitao Shi Reviewed-by: Daniel Kurtz Reviewed-by: Enric Balletbo i Serra [uli: followed API changes, removed FW update feature] Signed-off-by: Ulrich Hecht Signed-off-by: Enric Balletbo i Serra --- One of the reviews from Laurent was to use 'i2c_new_ancillary_device'. I didn't change this for two reasons. 1) It doesn't have a devm version, so the remove path is more simple using the devm_i2c_new_dummy_device family. 2) IIUC the ancillary function is useful when you want to retrieve the address from the firmware or DT, that's not really the case here, as we have a base address and fixed offset to the base address which I think is not configurable. Let me know if you still think that I should use the ancillary call. Changes in v22: - Remove sysfs attributes because are not really used (Enric Balletbo) - Use enum for address page offsets (Ezequiel Garcia) - Remove enable tracking (Enric Balletbo) - Use panel_bridge API (Laurent Pinchart) - Do not use kernel-doc format for non kernel-doc formatted commands (Enric Balletbo) - Remove verbose message for PAGE1_VSTART command (Ezequiel Garcia) - Use time_is_after_jiffies idiom (Ezequiel Garcia) - Remove unused macros (Ezequiel Garcia) - Fix weird alignment in dsi->mode_flags (Laurent Pinchart) - Use drm_of_find_panel_or_bridge helper (Laurent Pinchart) - Remove mode-sel-gpios as is not used (Laurent Pinchart) - Remove error messages to get gpios as the core will already report it (Enric Balletbo) - Remove redundant message getting the regulators (Laurent Pinchart) - Rename sleep-gpios to powerdown-gpios (Laurent Pinchart) - Use ARRAY_SIZE(ps_bridge->page) instead of MAX_DEV when possible (Laurent Pinchart) - Fix race with userspace accessing the sysfs attributes (Laurent Pinchart) - Remove id_table as is only used on DR platforms (Laurent Pinchart) - Convert to new i2c device probe() (Laurent Pinchart) - Use i2c_smbus_read/write helpers instead of open coding it (Laurent Pinchart) - Remove unnused global variables (Laurent Pinchart) - Remove unnused fields in ps8640 struct (Laurent Pinchart) - Remove commented-out headers (Laurent Pinchart) Changes in v21: - Use devm_i2c_new_dummy_device and fix build issue using deprecated i2c_new_dummy - Fix build issue due missing drm_bridge.h - Do not remove in ps8640_remove device managed resources Changes in v19: - fixed return value of ps8640_probe() when no panel is found Changes in v18: - followed DRM API changes - use DEVICE_ATTR_RO() - remove firmware update code - add SPDX identifier Changes in v17: - remove some unused head files. - add macros for ps8640 pages. - remove ddc_i2c client - add mipi_dsi_device_register_full - remove the manufacturer from the name and i2c_device_id Changes in v16: - Disable ps8640 DSI MCS Function. - Rename gpios name more clearly. - Tune the ps8640 power on sequence. Changes in v15: - Drop drm_connector_(un)register calls from parade ps8640. The main DRM driver mtk_drm_drv now calls drm_connector_register_all() after drm_dev_register() in the mtk_drm_bind() function. That function should iterate over all connectors and call drm_connector_register() for each of them. So, remove drm_connector_(un)register calls from parade ps8640. Changes in v14: - update copyright info. - change bridge_to_ps8640 and connector_to_ps8640 to inline function. - fix some coding style. - use sizeof as array counter. - use drm_get_edid when read edid. - add mutex when firmware updating. Changes in v13: - add const on data, ps8640_write_bytes(struct i2c_client *client, const u8 *data, u16 data_len) - fix PAGE2_SW_REST tyro. - move the buf[3] init to entrance of the function. Changes in v12: - fix hw_chip_id build warning Changes in v11: - Remove depends on I2C, add DRM depends - Reuse ps8640_write_bytes() in ps8640_write_byte() - Use timer check for polling like the routines in - Fix no drm_connector_unregister/drm_connector_cleanup when ps8640_bridge_attach fail - Check the ps8640 hardware id in ps8640_validate_firmware - Remove fw_version check - Move ps8640_validate_firmware before ps8640_enter_bl - Add ddc_i2c unregister when probe fail and ps8640_remove drivers/gpu/drm/bridge/Kconfig | 11 + drivers/gpu/drm/bridge/Makefile| 1 + drivers/gpu/drm/bridge/parade-ps8640.c | 354 + 3 files changed, 366 insertions(+) create mode 100644 drivers/gpu/drm/bridge/parade-ps8640.c diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig index 4734f6993858..3e0a63011723 100644 --- a/drivers/gpu/drm/bridge/Kconfig +++ b/drivers/gpu/drm/bridge/Kconfig @@ -101,6 +101,17 @@ config DRM_PARADE_PS8622 ---help--- Parade eDP-LVDS bridge chip driver. +config DRM_PARADE_PS8640 + tristate "Parade PS8640 MIPI DSI to eDP Converter" + depends on OF + select
Re: [PATCH v22 2/2] drm/bridge: Add I2C based driver for ps8640 bridge
Hi Nicolas,
Many thanks for you review. Just preparing a new version with your comments
addressed.
On 20/12/19 9:44, Nicolas Boichat wrote:
> On Fri, Dec 20, 2019 at 4:17 PM Enric Balletbo i Serra
> wrote:
>>
>> From: Jitao Shi
>>
>> This patch adds drm_bridge driver for parade DSI to eDP bridge chip.
>>
>> Signed-off-by: Jitao Shi
>> Reviewed-by: Daniel Kurtz
>> Reviewed-by: Enric Balletbo i Serra
>> [uli: followed API changes, removed FW update feature]
>> Signed-off-by: Ulrich Hecht
>> Signed-off-by: Enric Balletbo i Serra
>> ---
>> [snip]
>>
>> drivers/gpu/drm/bridge/Kconfig | 11 +
>> drivers/gpu/drm/bridge/Makefile| 1 +
>> drivers/gpu/drm/bridge/parade-ps8640.c | 354 +
>
> Half the size! Sounds great.
>
> Mostly nits below.
>
>> 3 files changed, 366 insertions(+)
>> create mode 100644 drivers/gpu/drm/bridge/parade-ps8640.c
>>
>> diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c
>> b/drivers/gpu/drm/bridge/parade-ps8640.c
>> new file mode 100644
>> index ..aa0045037f44
>> --- /dev/null
>> +++ b/drivers/gpu/drm/bridge/parade-ps8640.c
>> @@ -0,0 +1,354 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (c) 2016 MediaTek Inc.
>> + */
>> +
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +
>> +#define PAGE2_GPIO_H 0xa7
>> +#define PS_GPIO9 BIT(1)
>> +#define PAGE2_I2C_BYPASS 0xea
>> +#define I2C_BYPASS_EN 0xd0
>> +#define PAGE2_MCS_EN 0xf3
>> +#define MCS_EN BIT(0)
>> +#define PAGE3_SET_ADD 0xfe
>> +#define VDO_CTL_ADD0x13
>> +#define VDO_DIS0x18
>> +#define VDO_EN 0x1c
>> +
>> +/*
>> + * PS8640 uses multiple addresses:
>> + * page[0]: for DP control
>> + * page[1]: for VIDEO Bridge
>> + * page[2]: for control top
>> + * page[3]: for DSI Link Control1
>> + * page[4]: for MIPI Phy
>> + * page[5]: for VPLL
>> + * page[6]: for DSI Link Control2
>> + * page[7]: for SPI ROM mapping
>> + */
>> +enum page_addr_offset {
>> + PAGE0_DP_CNTL = 0,
>> + PAGE1_VDO_BDG,
>> + PAGE2_TOP_CNTL,
>> + PAGE3_DSI_CNTL1,
>> + PAGE4_MIPI_PHY,
>> + PAGE5_VPLL,
>> + PAGE6_DSI_CNTL2,
>> + PAGE7_SPI_CNTL,
>> + MAX_DEVS
>> +};
>> +
>> +struct ps8640 {
>> + struct drm_bridge bridge;
>> + struct drm_bridge *panel_bridge;
>> + struct mipi_dsi_device *dsi;
>> + struct i2c_client *page[MAX_DEVS];
>> + struct regulator_bulk_data supplies[2];
>> + struct gpio_desc *gpio_reset;
>> + struct gpio_desc *gpio_powerdown;
>> +};
>> +
>> +static inline struct ps8640 *bridge_to_ps8640(struct drm_bridge *e)
>> +{
>> + return container_of(e, struct ps8640, bridge);
>> +}
>> +
>> +static int ps8640_bridge_unmute(struct ps8640 *ps_bridge)
>> +{
>> + struct i2c_client *client = ps_bridge->page[PAGE3_DSI_CNTL1];
>> + u8 vdo_ctrl_buf[] = { VDO_CTL_ADD, VDO_EN };
>
> nit: const?
>
>> + int ret;
>> +
>> + ret = i2c_smbus_write_i2c_block_data(client, PAGE3_SET_ADD,
>> +sizeof(vdo_ctrl_buf),
>> +vdo_ctrl_buf);
>> + if (ret < 0)
>> + return ret;
>> +
>> + return 0;
>> +}
>> +
>> +static int ps8640_bridge_mute(struct ps8640 *ps_bridge)
>> +{
>> + struct i2c_client *client = ps_bridge->page[PAGE3_DSI_CNTL1];
>> + u8 vdo_ctrl_buf[] = { VDO_CTL_ADD, VDO_DIS };
>
> ditto
>
>> + int ret;
>> +
>> + ret = i2c_smbus_write_i2c_block_data(client, PAGE3_SET_ADD,
>> +sizeof(vdo_ctrl_buf),
>> +vdo_ctrl_buf);
>> + if (ret < 0)
>> + return ret;
>> +
>> + return 0;
>> +}
>
> Since the 2 functions are almost the same, you could shrink the driver
> a bit further by merging them into one with a boolean parameter? (then
> maybe give up on the const u8 comment).
>
I decided to merge the two functions and use and enum instead of a boolean
parameter, so the "bool" value is more understandable.
>> +
>> +static void ps8640_pre_enable(struct drm_bridge *bridge)
>> +{
>> + struct ps8640 *ps_bridge = bridge_to_ps8640(bridge);
>> + struct i2c_client *client = ps_bridge->page[PAGE2_TOP_CNTL];
>> + unsigned long timeout;
>> + int ret, status;
>> +
>> + ret = regulator_bulk_enable(ARRAY_SIZE(ps_bridge->supplies),
>> + ps_bridge->supplies);
>> + if (ret < 0) {
>> + DRM_ERROR("cannot enable regulators %d\n", ret);
>> + return;
>> + }
>> +
>> + gpiod_set_value(ps_bridge->gpio_powerdown, 1);
>> + gpiod_set_value(ps_bridge->gpio_reset, 0);
>> +
Re: [PATCH v22 2/2] drm/bridge: Add I2C based driver for ps8640 bridge
On Fri, Dec 20, 2019 at 4:17 PM Enric Balletbo i Serra
wrote:
>
> From: Jitao Shi
>
> This patch adds drm_bridge driver for parade DSI to eDP bridge chip.
>
> Signed-off-by: Jitao Shi
> Reviewed-by: Daniel Kurtz
> Reviewed-by: Enric Balletbo i Serra
> [uli: followed API changes, removed FW update feature]
> Signed-off-by: Ulrich Hecht
> Signed-off-by: Enric Balletbo i Serra
> ---
> [snip]
>
> drivers/gpu/drm/bridge/Kconfig | 11 +
> drivers/gpu/drm/bridge/Makefile| 1 +
> drivers/gpu/drm/bridge/parade-ps8640.c | 354 +
Half the size! Sounds great.
Mostly nits below.
> 3 files changed, 366 insertions(+)
> create mode 100644 drivers/gpu/drm/bridge/parade-ps8640.c
>
> diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c
> b/drivers/gpu/drm/bridge/parade-ps8640.c
> new file mode 100644
> index ..aa0045037f44
> --- /dev/null
> +++ b/drivers/gpu/drm/bridge/parade-ps8640.c
> @@ -0,0 +1,354 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2016 MediaTek Inc.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#define PAGE2_GPIO_H 0xa7
> +#define PS_GPIO9 BIT(1)
> +#define PAGE2_I2C_BYPASS 0xea
> +#define I2C_BYPASS_EN 0xd0
> +#define PAGE2_MCS_EN 0xf3
> +#define MCS_EN BIT(0)
> +#define PAGE3_SET_ADD 0xfe
> +#define VDO_CTL_ADD0x13
> +#define VDO_DIS0x18
> +#define VDO_EN 0x1c
> +
> +/*
> + * PS8640 uses multiple addresses:
> + * page[0]: for DP control
> + * page[1]: for VIDEO Bridge
> + * page[2]: for control top
> + * page[3]: for DSI Link Control1
> + * page[4]: for MIPI Phy
> + * page[5]: for VPLL
> + * page[6]: for DSI Link Control2
> + * page[7]: for SPI ROM mapping
> + */
> +enum page_addr_offset {
> + PAGE0_DP_CNTL = 0,
> + PAGE1_VDO_BDG,
> + PAGE2_TOP_CNTL,
> + PAGE3_DSI_CNTL1,
> + PAGE4_MIPI_PHY,
> + PAGE5_VPLL,
> + PAGE6_DSI_CNTL2,
> + PAGE7_SPI_CNTL,
> + MAX_DEVS
> +};
> +
> +struct ps8640 {
> + struct drm_bridge bridge;
> + struct drm_bridge *panel_bridge;
> + struct mipi_dsi_device *dsi;
> + struct i2c_client *page[MAX_DEVS];
> + struct regulator_bulk_data supplies[2];
> + struct gpio_desc *gpio_reset;
> + struct gpio_desc *gpio_powerdown;
> +};
> +
> +static inline struct ps8640 *bridge_to_ps8640(struct drm_bridge *e)
> +{
> + return container_of(e, struct ps8640, bridge);
> +}
> +
> +static int ps8640_bridge_unmute(struct ps8640 *ps_bridge)
> +{
> + struct i2c_client *client = ps_bridge->page[PAGE3_DSI_CNTL1];
> + u8 vdo_ctrl_buf[] = { VDO_CTL_ADD, VDO_EN };
nit: const?
> + int ret;
> +
> + ret = i2c_smbus_write_i2c_block_data(client, PAGE3_SET_ADD,
> +sizeof(vdo_ctrl_buf),
> +vdo_ctrl_buf);
> + if (ret < 0)
> + return ret;
> +
> + return 0;
> +}
> +
> +static int ps8640_bridge_mute(struct ps8640 *ps_bridge)
> +{
> + struct i2c_client *client = ps_bridge->page[PAGE3_DSI_CNTL1];
> + u8 vdo_ctrl_buf[] = { VDO_CTL_ADD, VDO_DIS };
ditto
> + int ret;
> +
> + ret = i2c_smbus_write_i2c_block_data(client, PAGE3_SET_ADD,
> +sizeof(vdo_ctrl_buf),
> +vdo_ctrl_buf);
> + if (ret < 0)
> + return ret;
> +
> + return 0;
> +}
Since the 2 functions are almost the same, you could shrink the driver
a bit further by merging them into one with a boolean parameter? (then
maybe give up on the const u8 comment).
> +
> +static void ps8640_pre_enable(struct drm_bridge *bridge)
> +{
> + struct ps8640 *ps_bridge = bridge_to_ps8640(bridge);
> + struct i2c_client *client = ps_bridge->page[PAGE2_TOP_CNTL];
> + unsigned long timeout;
> + int ret, status;
> +
> + ret = regulator_bulk_enable(ARRAY_SIZE(ps_bridge->supplies),
> + ps_bridge->supplies);
> + if (ret < 0) {
> + DRM_ERROR("cannot enable regulators %d\n", ret);
> + return;
> + }
> +
> + gpiod_set_value(ps_bridge->gpio_powerdown, 1);
> + gpiod_set_value(ps_bridge->gpio_reset, 0);
> + usleep_range(2000, 2500);
> + gpiod_set_value(ps_bridge->gpio_reset, 1);
> +
> + /*
> +* Wait for the ps8640 embedded MCU to be ready
> +* First wait 200ms and then check the MCU ready flag every 20ms
> +*/
> + msleep(200);
> +
> + timeout = jiffies + msecs_to_jiffies(200) + 1;
> +
> + while (time_is_after_jiffies(timeout)) {
> + status = i2c_smbus_read_byte_data(client, PAGE2_GPIO_H);
> +
