diff mbox series

[3/5] gpu: drm: adp: Add a backlight driver for the Summit LCD

Message ID 20241124-adpdrm-v1-3-3191d8e6e49a@gmail.com (mailing list archive)
State New
Headers show
Series Driver for pre-DCP apple display controller. | expand

Commit Message

Sasha Finkelstein via B4 Relay Nov. 24, 2024, 10:29 p.m. UTC 
From: Sasha Finkelstein <fnkl.kernel@gmail.com>

This is the display panel used for the touchbar on laptops that have it.

Co-developed-by: Nick Chan <towinchenmi@gmail.com>
Signed-off-by: Nick Chan <towinchenmi@gmail.com>
Signed-off-by: Sasha Finkelstein <fnkl.kernel@gmail.com>
---
 drivers/gpu/drm/adp/panel-summit.c | 108 +++++++++++++++++++++++++++++++++++++
 1 file changed, 108 insertions(+)

Comments

Neil Armstrong Nov. 25, 2024, 8:45 a.m. UTC | #1 
Hi,

On 24/11/2024 23:29, Sasha Finkelstein via B4 Relay wrote:
> From: Sasha Finkelstein <fnkl.kernel@gmail.com>
> 
> This is the display panel used for the touchbar on laptops that have it.
> 
> Co-developed-by: Nick Chan <towinchenmi@gmail.com>
> Signed-off-by: Nick Chan <towinchenmi@gmail.com>
> Signed-off-by: Sasha Finkelstein <fnkl.kernel@gmail.com>
> ---
>   drivers/gpu/drm/adp/panel-summit.c | 108 +++++++++++++++++++++++++++++++++++++
>   1 file changed, 108 insertions(+)
> 
> diff --git a/drivers/gpu/drm/adp/panel-summit.c b/drivers/gpu/drm/adp/panel-summit.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..2dcbddd925ce3863742aa60164369ba9db0bbfff
> --- /dev/null
> +++ b/drivers/gpu/drm/adp/panel-summit.c
> @@ -0,0 +1,108 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +#include <linux/backlight.h>
> +#include <drm/drm_mipi_dsi.h>
> +#include <video/mipi_display.h>
> +
> +struct summit_data {
> +	struct mipi_dsi_device *dsi;
> +	struct backlight_device *bl;
> +};
> +
> +static int summit_set_brightness(struct device *dev)
> +{
> +	struct summit_data *panel = dev_get_drvdata(dev);
> +	int level = backlight_get_brightness(panel->bl);
> +	int err = mipi_dsi_dcs_set_display_brightness(panel->dsi, level);
> +
> +	if (err < 0)
> +		return err;
> +	return 0;

Just return err here.

> +}
> +
> +static int summit_bl_update_status(struct backlight_device *dev)
> +{
> +	return summit_set_brightness(&dev->dev);
> +}
> +
> +static int summit_bl_get_brightness(struct backlight_device *dev)
> +{
> +	return backlight_get_brightness(dev);
> +}
> +
> +static const struct backlight_ops summit_bl_ops = {
> +	.get_brightness = summit_bl_get_brightness,
> +	.update_status	= summit_bl_update_status,
> +};
> +
> +static int summit_probe(struct mipi_dsi_device *dsi)
> +{
> +	struct backlight_properties props = { 0 };
> +	struct device *dev = &dsi->dev;
> +	struct summit_data *panel;
> +
> +	panel = devm_kzalloc(dev, sizeof(*panel), GFP_KERNEL);
> +	if (!panel)
> +		return -ENOMEM;
> +
> +	mipi_dsi_set_drvdata(dsi, panel);
> +	panel->dsi = dsi;
> +
> +	int ret = device_property_read_u32(dev, "max-brightness", &props.max_brightness);
> +
> +	if (ret)
> +		props.max_brightness = 255;
> +	props.type = BACKLIGHT_RAW;
> +
> +	panel->bl = devm_backlight_device_register(dev, dev_name(dev),
> +						   dev, panel, &summit_bl_ops, &props);
> +	if (IS_ERR(panel->bl))
> +		return PTR_ERR(panel->bl);
> +
> +	return mipi_dsi_attach(dsi);
> +}
> +
> +static void summit_remove(struct mipi_dsi_device *dsi)
> +{
> +	mipi_dsi_detach(dsi);
> +}
> +
> +static int summit_resume(struct device *dev)
> +{
> +	return summit_set_brightness(dev);
> +}
> +
> +static int summit_suspend(struct device *dev)
> +{
> +	struct summit_data *panel = dev_get_drvdata(dev);
> +
> +	int err = mipi_dsi_dcs_set_display_brightness(panel->dsi, 0);
> +
> +	if (err < 0)
> +		return err;
> +	return 0;

Just return err here, add a common function to set a brighness value and
avoid duplicate code like here.

Neil


> +}
> +
> +static DEFINE_SIMPLE_DEV_PM_OPS(summit_pm_ops, summit_suspend,
> +				summit_resume);
> +
> +static const struct of_device_id summit_of_match[] = {
> +	{ .compatible = "apple,summit" },
> +	{},
> +};
> +
> +MODULE_DEVICE_TABLE(of, summit_of_match);
> +
> +static struct mipi_dsi_driver summit_driver = {
> +	.probe = summit_probe,
> +	.remove = summit_remove,
> +	.driver = {
> +		.name = "panel-summit",
> +		.of_match_table = summit_of_match,
> +		.pm = pm_sleep_ptr(&summit_pm_ops),
> +	},
> +};
> +module_mipi_dsi_driver(summit_driver);
> +
> +MODULE_DESCRIPTION("Summit Display Panel Driver");
> +MODULE_LICENSE("GPL");
> 

Please move the driver into drivers/gpu/drm/panels

Thanks,
Neil
Sasha Finkelstein Nov. 25, 2024, 11:28 a.m. UTC | #2 
On Mon, 25 Nov 2024 at 09:45, Neil Armstrong <neil.armstrong@linaro.org> wrote:
> > +static int summit_suspend(struct device *dev)
> > +{
> > +     struct summit_data *panel = dev_get_drvdata(dev);
> > +
> > +     int err = mipi_dsi_dcs_set_display_brightness(panel->dsi, 0);
> > +
> > +     if (err < 0)
> > +             return err;
> > +     return 0;
>
> Just return err here, add a common function to set a brighness value and
> avoid duplicate code like here.

I felt that mipi_dsi_dcs_set_display_brightness is common enough, is it not?
Ack on all other changes, will be done for v2.
Krzysztof Kozlowski Nov. 25, 2024, 2:49 p.m. UTC | #3 
On 24/11/2024 23:29, Sasha Finkelstein via B4 Relay wrote:
> From: Sasha Finkelstein <fnkl.kernel@gmail.com>
> 
> This is the display panel used for the touchbar on laptops that have it.


...


> +static int summit_probe(struct mipi_dsi_device *dsi)
> +{
> +	struct backlight_properties props = { 0 };
> +	struct device *dev = &dsi->dev;
> +	struct summit_data *panel;
> +
> +	panel = devm_kzalloc(dev, sizeof(*panel), GFP_KERNEL);
> +	if (!panel)
> +		return -ENOMEM;
> +
> +	mipi_dsi_set_drvdata(dsi, panel);
> +	panel->dsi = dsi;
> +
> +	int ret = device_property_read_u32(dev, "max-brightness", &props.max_brightness);
That's an undocumented property, which suggests you did not test your DTS.

It does not look like you tested the DTS against bindings. Please run
`make dtbs_check W=1` (see
Documentation/devicetree/bindings/writing-schema.rst or
https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/
for instructions).

Best regards,
Krzysztof
Nick Chan Nov. 25, 2024, 3:03 p.m. UTC | #4 
Krzysztof Kozlowski 於 2024/11/25 晚上10:49 寫道:
> On 24/11/2024 23:29, Sasha Finkelstein via B4 Relay wrote:
>> From: Sasha Finkelstein <fnkl.kernel@gmail.com>
>>
>> This is the display panel used for the touchbar on laptops that have it.
> 
> 
> ...
> 
> 
>> +static int summit_probe(struct mipi_dsi_device *dsi)
>> +{
>> +	struct backlight_properties props = { 0 };
>> +	struct device *dev = &dsi->dev;
>> +	struct summit_data *panel;
>> +
>> +	panel = devm_kzalloc(dev, sizeof(*panel), GFP_KERNEL);
>> +	if (!panel)
>> +		return -ENOMEM;
>> +
>> +	mipi_dsi_set_drvdata(dsi, panel);
>> +	panel->dsi = dsi;
>> +
>> +	int ret = device_property_read_u32(dev, "max-brightness", &props.max_brightness);
> That's an undocumented property, which suggests you did not test your DTS.

Actually, testing the DTS would not have caught this issue. For more
context,
all summit panels found in touch bar have a max brightness of 255, but the
summit panel in Apple A11 devices like the iPhone X is latter found to have
a max brightness of 2047.

However, A11 cannot be properly supported right now due to not having a
driver
for the DART IOMMU.

In the meantime, max-brightness could documented and be made required,
and the
default 255 brightness could be removed.

> 
> It does not look like you tested the DTS against bindings. Please run
> `make dtbs_check W=1` (see
> Documentation/devicetree/bindings/writing-schema.rst or
> https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/
> for instructions).
> 
> Best regards,
> Krzysztof

Nick Chan
Krzysztof Kozlowski Nov. 25, 2024, 3:06 p.m. UTC | #5 
On 25/11/2024 16:03, Nick Chan wrote:
>>> +static int summit_probe(struct mipi_dsi_device *dsi)
>>> +{
>>> +	struct backlight_properties props = { 0 };
>>> +	struct device *dev = &dsi->dev;
>>> +	struct summit_data *panel;
>>> +
>>> +	panel = devm_kzalloc(dev, sizeof(*panel), GFP_KERNEL);
>>> +	if (!panel)
>>> +		return -ENOMEM;
>>> +
>>> +	mipi_dsi_set_drvdata(dsi, panel);
>>> +	panel->dsi = dsi;
>>> +
>>> +	int ret = device_property_read_u32(dev, "max-brightness", &props.max_brightness);
>> That's an undocumented property, which suggests you did not test your DTS.
> 
> Actually, testing the DTS would not have caught this issue. For more
> context,

If you mean that property is not in your DTS, then right, it would not
be caught. But otherwise it would.

Anyway, I pointed out testing as helping tool for your development, just
like building with clang W=1 will spare you some review comments or bugs.

> all summit panels found in touch bar have a max brightness of 255, but the
> summit panel in Apple A11 devices like the iPhone X is latter found to have
> a max brightness of 2047.
> 
> However, A11 cannot be properly supported right now due to not having a
> driver
> for the DART IOMMU.
> 
> In the meantime, max-brightness could documented and be made required,
> and the
> default 255 brightness could be removed.

BTW, max-brightness is a property of backlight, not panel, I think.

Best regards,
Krzysztof
Sasha Finkelstein Nov. 26, 2024, 4:34 p.m. UTC | #6 
On Mon, 25 Nov 2024 at 16:07, Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> BTW, max-brightness is a property of backlight, not panel, I think.
This is an oled panel, so no separate backlight device, the mipi commands
just change the pixel brightness. There is prior art in other bindings on having
the max-brightness property attached to the panel itself.
Krzysztof Kozlowski Nov. 26, 2024, 4:52 p.m. UTC | #7 
On 26/11/2024 17:34, Sasha Finkelstein wrote:
> On Mon, 25 Nov 2024 at 16:07, Krzysztof Kozlowski <krzk@kernel.org> wrote:
>>
>> BTW, max-brightness is a property of backlight, not panel, I think.
> This is an oled panel, so no separate backlight device, the mipi commands
> just change the pixel brightness. There is prior art in other bindings on having
> the max-brightness property attached to the panel itself.

Where? git grep gave me only one result for bindings - old Samsung panel
from 15 years ago. If you refer to this one, then use it as example for
the bindings with similar explanation as the commit introducing brightness?

Best regards,
Krzysztof
diff mbox series

Patch

diff --git a/drivers/gpu/drm/adp/panel-summit.c b/drivers/gpu/drm/adp/panel-summit.c
new file mode 100644
index 0000000000000000000000000000000000000000..2dcbddd925ce3863742aa60164369ba9db0bbfff
--- /dev/null
+++ b/drivers/gpu/drm/adp/panel-summit.c
@@ -0,0 +1,108 @@ 
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include <linux/backlight.h>
+#include <drm/drm_mipi_dsi.h>
+#include <video/mipi_display.h>
+
+struct summit_data {
+	struct mipi_dsi_device *dsi;
+	struct backlight_device *bl;
+};
+
+static int summit_set_brightness(struct device *dev)
+{
+	struct summit_data *panel = dev_get_drvdata(dev);
+	int level = backlight_get_brightness(panel->bl);
+	int err = mipi_dsi_dcs_set_display_brightness(panel->dsi, level);
+
+	if (err < 0)
+		return err;
+	return 0;
+}
+
+static int summit_bl_update_status(struct backlight_device *dev)
+{
+	return summit_set_brightness(&dev->dev);
+}
+
+static int summit_bl_get_brightness(struct backlight_device *dev)
+{
+	return backlight_get_brightness(dev);
+}
+
+static const struct backlight_ops summit_bl_ops = {
+	.get_brightness = summit_bl_get_brightness,
+	.update_status	= summit_bl_update_status,
+};
+
+static int summit_probe(struct mipi_dsi_device *dsi)
+{
+	struct backlight_properties props = { 0 };
+	struct device *dev = &dsi->dev;
+	struct summit_data *panel;
+
+	panel = devm_kzalloc(dev, sizeof(*panel), GFP_KERNEL);
+	if (!panel)
+		return -ENOMEM;
+
+	mipi_dsi_set_drvdata(dsi, panel);
+	panel->dsi = dsi;
+
+	int ret = device_property_read_u32(dev, "max-brightness", &props.max_brightness);
+
+	if (ret)
+		props.max_brightness = 255;
+	props.type = BACKLIGHT_RAW;
+
+	panel->bl = devm_backlight_device_register(dev, dev_name(dev),
+						   dev, panel, &summit_bl_ops, &props);
+	if (IS_ERR(panel->bl))
+		return PTR_ERR(panel->bl);
+
+	return mipi_dsi_attach(dsi);
+}
+
+static void summit_remove(struct mipi_dsi_device *dsi)
+{
+	mipi_dsi_detach(dsi);
+}
+
+static int summit_resume(struct device *dev)
+{
+	return summit_set_brightness(dev);
+}
+
+static int summit_suspend(struct device *dev)
+{
+	struct summit_data *panel = dev_get_drvdata(dev);
+
+	int err = mipi_dsi_dcs_set_display_brightness(panel->dsi, 0);
+
+	if (err < 0)
+		return err;
+	return 0;
+}
+
+static DEFINE_SIMPLE_DEV_PM_OPS(summit_pm_ops, summit_suspend,
+				summit_resume);
+
+static const struct of_device_id summit_of_match[] = {
+	{ .compatible = "apple,summit" },
+	{},
+};
+
+MODULE_DEVICE_TABLE(of, summit_of_match);
+
+static struct mipi_dsi_driver summit_driver = {
+	.probe = summit_probe,
+	.remove = summit_remove,
+	.driver = {
+		.name = "panel-summit",
+		.of_match_table = summit_of_match,
+		.pm = pm_sleep_ptr(&summit_pm_ops),
+	},
+};
+module_mipi_dsi_driver(summit_driver);
+
+MODULE_DESCRIPTION("Summit Display Panel Driver");
+MODULE_LICENSE("GPL");