[RFC][PATCH] regulator: rpi-panel: Add regulator/backlight driver for RPi panel

[RFC][PATCH] regulator: rpi-panel: Add regulator/backlight driver for RPi panel

[Marek Vasut]

This regulator/backlight driver handles the ATTINY88 present on the
RPi 7" touchscreen panel and exposes the power/backlight interfaces.

Signed-off-by: Marek Vasut <marex@denx.de>
To: dri-devel@lists.freedesktop.org
Cc: Eric Anholt <eric@anholt.net>
Cc: Mark Brown <broonie@kernel.org>
Cc: Sam Ravnborg <sam@ravnborg.org>
---
 drivers/regulator/Kconfig                     |  10 +
 drivers/regulator/Makefile                    |   1 +
 .../regulator/rpi-panel-attiny-regulator.c    | 214 ++++++++++++++++++
 3 files changed, 225 insertions(+)
 create mode 100644 drivers/regulator/rpi-panel-attiny-regulator.c

diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index de17ef7e18f0..439cc5226bae 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -864,6 +864,16 @@ config REGULATOR_QCOM_USB_VBUS
 	  Say M here if you want to include support for enabling the VBUS output
 	  as a module. The module will be named "qcom_usb_vbus_regulator".
 
+config REGULATOR_RASPBERRYPI_TOUCHSCREEN_ATTINY
+	tristate "Raspberry Pi 7-inch touchscreen panel ATTINY regulator"
+	depends on BACKLIGHT_CLASS_DEVICE
+	depends on I2C
+	select REGMAP_I2C
+	help
+	  This driver supports ATTINY regulator on the Raspberry Pi 7-inch
+	  touchscreen unit. The regulator is used to enable power to the
+	  TC358762, display and to control backlight.
+
 config REGULATOR_RC5T583
 	tristate "RICOH RC5T583 Power regulators"
 	depends on MFD_RC5T583
diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index d8d3ecf526a8..6e39ea87901e 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -107,6 +107,7 @@ obj-$(CONFIG_REGULATOR_TPS51632) += tps51632-regulator.o
 obj-$(CONFIG_REGULATOR_PBIAS) += pbias-regulator.o
 obj-$(CONFIG_REGULATOR_PCAP) += pcap-regulator.o
 obj-$(CONFIG_REGULATOR_PCF50633) += pcf50633-regulator.o
+obj-$(CONFIG_REGULATOR_RASPBERRYPI_TOUCHSCREEN_ATTINY)  += rpi-panel-attiny-regulator.o
 obj-$(CONFIG_REGULATOR_RC5T583)  += rc5t583-regulator.o
 obj-$(CONFIG_REGULATOR_RK808)   += rk808-regulator.o
 obj-$(CONFIG_REGULATOR_RN5T618) += rn5t618-regulator.o
diff --git a/drivers/regulator/rpi-panel-attiny-regulator.c b/drivers/regulator/rpi-panel-attiny-regulator.c
new file mode 100644
index 000000000000..ee46bfbf5eee
--- /dev/null
+++ b/drivers/regulator/rpi-panel-attiny-regulator.c
@@ -0,0 +1,214 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2020 Marek Vasut <marex@denx.de>
+ *
+ * Based on rpi_touchscreen.c by Eric Anholt <eric@anholt.net>
+ */
+
+#include <linux/backlight.h>
+#include <linux/err.h>
+#include <linux/gpio.h>
+#include <linux/i2c.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+#include <linux/regulator/driver.h>
+#include <linux/regulator/machine.h>
+#include <linux/regulator/of_regulator.h>
+#include <linux/slab.h>
+
+/* I2C registers of the Atmel microcontroller. */
+#define REG_ID		0x80
+#define REG_PORTA	0x81
+#define REG_PORTA_HF	BIT(2)
+#define REG_PORTA_VF	BIT(3)
+#define REG_PORTB	0x82
+#define REG_POWERON	0x85
+#define REG_PWM		0x86
+
+static const struct regmap_config attiny_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.max_register = REG_PWM,
+	.cache_type = REGCACHE_NONE,
+};
+
+static int attiny_lcd_power_enable(struct regulator_dev *rdev)
+{
+	unsigned int data;
+
+	regmap_write(rdev->regmap, REG_POWERON, 1);
+	/* Wait for nPWRDWN to go low to indicate poweron is done. */
+	regmap_read_poll_timeout(rdev->regmap, REG_PORTB, data,
+					data & BIT(0), 10, 1000000);
+
+	/* Default to the same orientation as the closed source
+	 * firmware used for the panel.  Runtime rotation
+	 * configuration will be supported using VC4's plane
+	 * orientation bits.
+	 */
+	regmap_write(rdev->regmap, REG_PORTA, BIT(2));
+
+	return 0;
+}
+
+static int attiny_lcd_power_disable(struct regulator_dev *rdev)
+{
+	regmap_write(rdev->regmap, REG_PWM, 0);
+	regmap_write(rdev->regmap, REG_POWERON, 0);
+	udelay(1);
+	return 0;
+}
+
+static int attiny_lcd_power_is_enabled(struct regulator_dev *rdev)
+{
+	unsigned int data;
+	int ret;
+
+	ret = regmap_read(rdev->regmap, REG_POWERON, &data);
+	if (ret < 0)
+		return ret;
+
+	if (!(data & BIT(0)))
+		return 0;
+
+	ret = regmap_read(rdev->regmap, REG_PORTB, &data);
+	if (ret < 0)
+		return ret;
+
+	return data & BIT(0);
+}
+
+static const struct regulator_init_data attiny_regulator_default = {
+	.constraints = {
+		.valid_ops_mask = REGULATOR_CHANGE_STATUS,
+	},
+};
+
+static const struct regulator_ops attiny_regulator_ops = {
+	.enable = attiny_lcd_power_enable,
+	.disable = attiny_lcd_power_disable,
+	.is_enabled = attiny_lcd_power_is_enabled,
+};
+
+static const struct regulator_desc attiny_regulator = {
+	.name	= "tc358762-power",
+	.ops	= &attiny_regulator_ops,
+	.type	= REGULATOR_VOLTAGE,
+	.owner	= THIS_MODULE,
+};
+
+static int attiny_update_status(struct backlight_device *bl)
+{
+	struct regmap *regmap = bl_get_data(bl);
+	int brightness = bl->props.brightness;
+
+	if (bl->props.power != FB_BLANK_UNBLANK ||
+	    bl->props.fb_blank != FB_BLANK_UNBLANK)
+		brightness = 0;
+
+	return regmap_write(regmap, REG_PWM, brightness);
+}
+
+static int attiny_get_brightness(struct backlight_device *bl)
+{
+	struct regmap *regmap = bl_get_data(bl);
+	int ret, brightness;
+
+	ret = regmap_read(regmap, REG_PWM, &brightness);
+	if (ret)
+		return ret;
+
+	return brightness;
+}
+
+static const struct backlight_ops attiny_bl = {
+	.update_status	= attiny_update_status,
+	.get_brightness	= attiny_get_brightness,
+};
+
+/*
+ * I2C driver interface functions
+ */
+static int attiny_i2c_probe(struct i2c_client *i2c,
+		const struct i2c_device_id *id)
+{
+	struct backlight_properties props = { };
+	struct regulator_config config = { };
+	struct backlight_device *bl;
+	struct regulator_dev *rdev;
+	struct regmap *regmap;
+	unsigned int data;
+	int ret;
+
+	regmap = devm_regmap_init_i2c(i2c, &attiny_regmap_config);
+	if (IS_ERR(regmap)) {
+		ret = PTR_ERR(regmap);
+		dev_err(&i2c->dev, "Failed to allocate register map: %d\n",
+			ret);
+		return ret;
+	}
+
+	ret = regmap_read(regmap, REG_ID, &data);
+	if (ret < 0) {
+		dev_err(&i2c->dev, "Failed to read REG_ID reg: %d\n", ret);
+		return ret;
+	}
+
+	switch (data) {
+	case 0xde: /* ver 1 */
+	case 0xc3: /* ver 2 */
+		break;
+	default:
+		dev_err(&i2c->dev, "Unknown Atmel firmware revision: 0x%02x\n", data);
+		return -ENODEV;
+	}
+
+	regmap_write(regmap, REG_POWERON, 0);
+	mdelay(1);
+
+	config.dev = &i2c->dev;
+	config.regmap = regmap;
+	config.of_node = i2c->dev.of_node;
+	config.init_data = &attiny_regulator_default;
+
+	rdev = devm_regulator_register(&i2c->dev, &attiny_regulator, &config);
+	if (IS_ERR(rdev)) {
+		dev_err(&i2c->dev, "Failed to register ATTINY regulator\n");
+		return PTR_ERR(rdev);
+	}
+
+	props.type = BACKLIGHT_RAW;
+	props.max_brightness = 0xff;
+	bl = devm_backlight_device_register(&i2c->dev,
+					    "7inch-touchscreen-panel-bl",
+					    &i2c->dev, regmap, &attiny_bl,
+					    &props);
+	if (IS_ERR(bl))
+		return PTR_ERR(bl);
+
+	bl->props.brightness = 0xff;
+
+	return 0;
+}
+
+static const struct of_device_id attiny_dt_ids[] = {
+	{ .compatible = "raspberrypi,7inch-touchscreen-panel-regulator" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, attiny_dt_ids);
+
+static struct i2c_driver attiny_regulator_driver = {
+	.driver = {
+		.name = "rpi_touchscreen_attiny",
+		.of_match_table = of_match_ptr(attiny_dt_ids),
+	},
+	.probe = attiny_i2c_probe,
+};
+
+module_i2c_driver(attiny_regulator_driver);
+
+MODULE_AUTHOR("Marek Vasut <marex@denx.de>");
+MODULE_DESCRIPTION("Regulator device driver for Raspberry Pi 7-inch touchscreen");
+MODULE_LICENSE("GPL v2");
-- 
2.27.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC][PATCH] regulator: rpi-panel: Add regulator/backlight driver for RPi panel

[Sam Ravnborg]

Hi Marek

On Wed, Jul 29, 2020 at 11:46:45PM +0200, Marek Vasut wrote:
> This regulator/backlight driver handles the ATTINY88 present on the
> RPi 7" touchscreen panel and exposes the power/backlight interfaces.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> To: dri-devel@lists.freedesktop.org
> Cc: Eric Anholt <eric@anholt.net>
> Cc: Mark Brown <broonie@kernel.org>
> Cc: Sam Ravnborg <sam@ravnborg.org>

It looks strange that the regulator and the backligth are defined in the
same module like this.

The usual approach is to have an independent regulator and an
independent backlight. Each are represented by their own node in the DT.

Also the compatible "raspberrypi,7inch-touchscreen-panel-regulator",
is unknown. We need a binding for the compatible.

For backlight drivers, and modules that includes backlight support it
would be good to include the backlight gang in cc:
Jingoo, Lee, Daniel.

One detail below - but I otherwise did not look at the code.

	Sam



> ---
>  drivers/regulator/Kconfig                     |  10 +
>  drivers/regulator/Makefile                    |   1 +
>  .../regulator/rpi-panel-attiny-regulator.c    | 214 ++++++++++++++++++
>  3 files changed, 225 insertions(+)
>  create mode 100644 drivers/regulator/rpi-panel-attiny-regulator.c
> 
> diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
> index de17ef7e18f0..439cc5226bae 100644
> --- a/drivers/regulator/Kconfig
> +++ b/drivers/regulator/Kconfig
> @@ -864,6 +864,16 @@ config REGULATOR_QCOM_USB_VBUS
>  	  Say M here if you want to include support for enabling the VBUS output
>  	  as a module. The module will be named "qcom_usb_vbus_regulator".
>  
> +config REGULATOR_RASPBERRYPI_TOUCHSCREEN_ATTINY
> +	tristate "Raspberry Pi 7-inch touchscreen panel ATTINY regulator"
> +	depends on BACKLIGHT_CLASS_DEVICE
> +	depends on I2C
> +	select REGMAP_I2C
> +	help
> +	  This driver supports ATTINY regulator on the Raspberry Pi 7-inch
> +	  touchscreen unit. The regulator is used to enable power to the
> +	  TC358762, display and to control backlight.
> +
>  config REGULATOR_RC5T583
>  	tristate "RICOH RC5T583 Power regulators"
>  	depends on MFD_RC5T583
> diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
> index d8d3ecf526a8..6e39ea87901e 100644
> --- a/drivers/regulator/Makefile
> +++ b/drivers/regulator/Makefile
> @@ -107,6 +107,7 @@ obj-$(CONFIG_REGULATOR_TPS51632) += tps51632-regulator.o
>  obj-$(CONFIG_REGULATOR_PBIAS) += pbias-regulator.o
>  obj-$(CONFIG_REGULATOR_PCAP) += pcap-regulator.o
>  obj-$(CONFIG_REGULATOR_PCF50633) += pcf50633-regulator.o
> +obj-$(CONFIG_REGULATOR_RASPBERRYPI_TOUCHSCREEN_ATTINY)  += rpi-panel-attiny-regulator.o
>  obj-$(CONFIG_REGULATOR_RC5T583)  += rc5t583-regulator.o
>  obj-$(CONFIG_REGULATOR_RK808)   += rk808-regulator.o
>  obj-$(CONFIG_REGULATOR_RN5T618) += rn5t618-regulator.o
> diff --git a/drivers/regulator/rpi-panel-attiny-regulator.c b/drivers/regulator/rpi-panel-attiny-regulator.c
> new file mode 100644
> index 000000000000..ee46bfbf5eee
> --- /dev/null
> +++ b/drivers/regulator/rpi-panel-attiny-regulator.c
> @@ -0,0 +1,214 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2020 Marek Vasut <marex@denx.de>
> + *
> + * Based on rpi_touchscreen.c by Eric Anholt <eric@anholt.net>
> + */
> +
> +#include <linux/backlight.h>
> +#include <linux/err.h>
> +#include <linux/gpio.h>
> +#include <linux/i2c.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/module.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/driver.h>
> +#include <linux/regulator/machine.h>
> +#include <linux/regulator/of_regulator.h>
> +#include <linux/slab.h>
> +
> +/* I2C registers of the Atmel microcontroller. */
> +#define REG_ID		0x80
> +#define REG_PORTA	0x81
> +#define REG_PORTA_HF	BIT(2)
> +#define REG_PORTA_VF	BIT(3)
> +#define REG_PORTB	0x82
> +#define REG_POWERON	0x85
> +#define REG_PWM		0x86
> +
> +static const struct regmap_config attiny_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +	.max_register = REG_PWM,
> +	.cache_type = REGCACHE_NONE,
> +};
> +
> +static int attiny_lcd_power_enable(struct regulator_dev *rdev)
> +{
> +	unsigned int data;
> +
> +	regmap_write(rdev->regmap, REG_POWERON, 1);
> +	/* Wait for nPWRDWN to go low to indicate poweron is done. */
> +	regmap_read_poll_timeout(rdev->regmap, REG_PORTB, data,
> +					data & BIT(0), 10, 1000000);
> +
> +	/* Default to the same orientation as the closed source
> +	 * firmware used for the panel.  Runtime rotation
> +	 * configuration will be supported using VC4's plane
> +	 * orientation bits.
> +	 */
> +	regmap_write(rdev->regmap, REG_PORTA, BIT(2));
> +
> +	return 0;
> +}
> +
> +static int attiny_lcd_power_disable(struct regulator_dev *rdev)
> +{
> +	regmap_write(rdev->regmap, REG_PWM, 0);
> +	regmap_write(rdev->regmap, REG_POWERON, 0);
> +	udelay(1);
> +	return 0;
> +}
> +
> +static int attiny_lcd_power_is_enabled(struct regulator_dev *rdev)
> +{
> +	unsigned int data;
> +	int ret;
> +
> +	ret = regmap_read(rdev->regmap, REG_POWERON, &data);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (!(data & BIT(0)))
> +		return 0;
> +
> +	ret = regmap_read(rdev->regmap, REG_PORTB, &data);
> +	if (ret < 0)
> +		return ret;
> +
> +	return data & BIT(0);
> +}
> +
> +static const struct regulator_init_data attiny_regulator_default = {
> +	.constraints = {
> +		.valid_ops_mask = REGULATOR_CHANGE_STATUS,
> +	},
> +};
> +
> +static const struct regulator_ops attiny_regulator_ops = {
> +	.enable = attiny_lcd_power_enable,
> +	.disable = attiny_lcd_power_disable,
> +	.is_enabled = attiny_lcd_power_is_enabled,
> +};
> +
> +static const struct regulator_desc attiny_regulator = {
> +	.name	= "tc358762-power",
> +	.ops	= &attiny_regulator_ops,
> +	.type	= REGULATOR_VOLTAGE,
> +	.owner	= THIS_MODULE,
> +};
> +
> +static int attiny_update_status(struct backlight_device *bl)
> +{
> +	struct regmap *regmap = bl_get_data(bl);
> +	int brightness = bl->props.brightness;
> +
> +	if (bl->props.power != FB_BLANK_UNBLANK ||
> +	    bl->props.fb_blank != FB_BLANK_UNBLANK)
> +		brightness = 0;
> +
We have recently introduced backlight_get_brightness().
Using this function will make this implemenation much simpler.
But it is only availabe in the backlight tree for now.

	Sam

> +	return regmap_write(regmap, REG_PWM, brightness);
> +}
> +
> +static int attiny_get_brightness(struct backlight_device *bl)
> +{
> +	struct regmap *regmap = bl_get_data(bl);
> +	int ret, brightness;
> +
> +	ret = regmap_read(regmap, REG_PWM, &brightness);
> +	if (ret)
> +		return ret;
> +
> +	return brightness;
> +}
> +
> +static const struct backlight_ops attiny_bl = {
> +	.update_status	= attiny_update_status,
> +	.get_brightness	= attiny_get_brightness,
> +};
> +
> +/*
> + * I2C driver interface functions
> + */
> +static int attiny_i2c_probe(struct i2c_client *i2c,
> +		const struct i2c_device_id *id)
> +{
> +	struct backlight_properties props = { };
> +	struct regulator_config config = { };
> +	struct backlight_device *bl;
> +	struct regulator_dev *rdev;
> +	struct regmap *regmap;
> +	unsigned int data;
> +	int ret;
> +
> +	regmap = devm_regmap_init_i2c(i2c, &attiny_regmap_config);
> +	if (IS_ERR(regmap)) {
> +		ret = PTR_ERR(regmap);
> +		dev_err(&i2c->dev, "Failed to allocate register map: %d\n",
> +			ret);
> +		return ret;
> +	}
> +
> +	ret = regmap_read(regmap, REG_ID, &data);
> +	if (ret < 0) {
> +		dev_err(&i2c->dev, "Failed to read REG_ID reg: %d\n", ret);
> +		return ret;
> +	}
> +
> +	switch (data) {
> +	case 0xde: /* ver 1 */
> +	case 0xc3: /* ver 2 */
> +		break;
> +	default:
> +		dev_err(&i2c->dev, "Unknown Atmel firmware revision: 0x%02x\n", data);
> +		return -ENODEV;
> +	}
> +
> +	regmap_write(regmap, REG_POWERON, 0);
> +	mdelay(1);
> +
> +	config.dev = &i2c->dev;
> +	config.regmap = regmap;
> +	config.of_node = i2c->dev.of_node;
> +	config.init_data = &attiny_regulator_default;
> +
> +	rdev = devm_regulator_register(&i2c->dev, &attiny_regulator, &config);
> +	if (IS_ERR(rdev)) {
> +		dev_err(&i2c->dev, "Failed to register ATTINY regulator\n");
> +		return PTR_ERR(rdev);
> +	}
> +
> +	props.type = BACKLIGHT_RAW;
> +	props.max_brightness = 0xff;
> +	bl = devm_backlight_device_register(&i2c->dev,
> +					    "7inch-touchscreen-panel-bl",
> +					    &i2c->dev, regmap, &attiny_bl,
> +					    &props);
> +	if (IS_ERR(bl))
> +		return PTR_ERR(bl);
> +
> +	bl->props.brightness = 0xff;
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id attiny_dt_ids[] = {
> +	{ .compatible = "raspberrypi,7inch-touchscreen-panel-regulator" },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, attiny_dt_ids);
> +
> +static struct i2c_driver attiny_regulator_driver = {
> +	.driver = {
> +		.name = "rpi_touchscreen_attiny",
> +		.of_match_table = of_match_ptr(attiny_dt_ids),
> +	},
> +	.probe = attiny_i2c_probe,
> +};
> +
> +module_i2c_driver(attiny_regulator_driver);
> +
> +MODULE_AUTHOR("Marek Vasut <marex@denx.de>");
> +MODULE_DESCRIPTION("Regulator device driver for Raspberry Pi 7-inch touchscreen");
> +MODULE_LICENSE("GPL v2");
> -- 
> 2.27.0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC][PATCH] regulator: rpi-panel: Add regulator/backlight driver for RPi panel

[Marek Vasut]

On 7/30/20 5:59 PM, Sam Ravnborg wrote:
> Hi Marek

Hi,

> On Wed, Jul 29, 2020 at 11:46:45PM +0200, Marek Vasut wrote:
>> This regulator/backlight driver handles the ATTINY88 present on the
>> RPi 7" touchscreen panel and exposes the power/backlight interfaces.
[...]
> It looks strange that the regulator and the backligth are defined in the
> same module like this.

It's one chip, attiny with custom firmware, what do you want me to do
about it ? I can over-complicate this and split it into multiple
drivers, but I don't think it's worth the complexity, considering that
this is likely a one-off device which will never be re-used elsewhere,
except on this one particular display module for RPi.

> The usual approach is to have an independent regulator and an
> independent backlight. Each are represented by their own node in the DT.
> 
> Also the compatible "raspberrypi,7inch-touchscreen-panel-regulator",
> is unknown. We need a binding for the compatible.

I submitted the patch as RFC to get feedback on how to handle this, so
yes, there are no DT bindings, that's on the todo.

> For backlight drivers, and modules that includes backlight support it
> would be good to include the backlight gang in cc:
> Jingoo, Lee, Daniel.

[...]
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC][PATCH] regulator: rpi-panel: Add regulator/backlight driver for RPi panel

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1263 bytes --]

On Thu, Jul 30, 2020 at 06:28:07PM +0200, Marek Vasut wrote:
> On 7/30/20 5:59 PM, Sam Ravnborg wrote:
> > On Wed, Jul 29, 2020 at 11:46:45PM +0200, Marek Vasut wrote:

> >> This regulator/backlight driver handles the ATTINY88 present on the
> >> RPi 7" touchscreen panel and exposes the power/backlight interfaces.

> > It looks strange that the regulator and the backligth are defined in the
> > same module like this.

> It's one chip, attiny with custom firmware, what do you want me to do
> about it ? I can over-complicate this and split it into multiple
> drivers, but I don't think it's worth the complexity, considering that
> this is likely a one-off device which will never be re-used elsewhere,
> except on this one particular display module for RPi.

Now you've written that you've pretty much guaranteed someone's going to
use the same component elsewhere :)

I think my main question would be that if this is going to be written
like this shouldn't it be a backlight driver rather than a regulator
driver?  I don't 100% follow how this would actually get used in a
system (perhaps the binding would help) but for these things if there's
only one tightly coupled user that's possible it's sometimes simpler to
just skip APIs and do things directly.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC][PATCH] regulator: rpi-panel: Add regulator/backlight driver for RPi panel

[Marek Vasut]

On 7/30/20 9:13 PM, Mark Brown wrote:
> On Thu, Jul 30, 2020 at 06:28:07PM +0200, Marek Vasut wrote:
>> On 7/30/20 5:59 PM, Sam Ravnborg wrote:
>>> On Wed, Jul 29, 2020 at 11:46:45PM +0200, Marek Vasut wrote:
> 
>>>> This regulator/backlight driver handles the ATTINY88 present on the
>>>> RPi 7" touchscreen panel and exposes the power/backlight interfaces.
> 
>>> It looks strange that the regulator and the backligth are defined in the
>>> same module like this.
> 
>> It's one chip, attiny with custom firmware, what do you want me to do
>> about it ? I can over-complicate this and split it into multiple
>> drivers, but I don't think it's worth the complexity, considering that
>> this is likely a one-off device which will never be re-used elsewhere,
>> except on this one particular display module for RPi.
> 
> Now you've written that you've pretty much guaranteed someone's going to
> use the same component elsewhere :)

How? The firmware is closed and not available, neither is documentation
for it, sadly.

> I think my main question would be that if this is going to be written
> like this shouldn't it be a backlight driver rather than a regulator
> driver?

Well no, because it enables power to the display backlight and TC358762
DSI-to-DPI bridge first, and then also controls some PWM implementation
in the attiny firmware later on. So I think it has to be regulator, as
that is the primary function. The backlight is somewhat secondary.

> I don't 100% follow how this would actually get used in a
> system (perhaps the binding would help) but for these things if there's
> only one tightly coupled user that's possible it's sometimes simpler to
> just skip APIs and do things directly.

That's what I'm trying to replace by this patch and tc358762 bridge
driver and panel driver, the combined version is already in tree:
drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c
but the tc358762 is clearly a generic bridge and the panel is generic
too, so combining it into one panel driver doesn't seem right.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC][PATCH] regulator: rpi-panel: Add regulator/backlight driver for RPi panel

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1688 bytes --]

On Thu, Jul 30, 2020 at 09:37:48PM +0200, Marek Vasut wrote:
> On 7/30/20 9:13 PM, Mark Brown wrote:
> > On Thu, Jul 30, 2020 at 06:28:07PM +0200, Marek Vasut wrote:

> >> about it ? I can over-complicate this and split it into multiple
> >> drivers, but I don't think it's worth the complexity, considering that
> >> this is likely a one-off device which will never be re-used elsewhere,
> >> except on this one particular display module for RPi.

> > Now you've written that you've pretty much guaranteed someone's going to
> > use the same component elsewhere :)

> How? The firmware is closed and not available, neither is documentation
> for it, sadly.

Companies often find other markets for their one off things, the
original RPi is a great example of this!

> > I don't 100% follow how this would actually get used in a
> > system (perhaps the binding would help) but for these things if there's
> > only one tightly coupled user that's possible it's sometimes simpler to
> > just skip APIs and do things directly.

> That's what I'm trying to replace by this patch and tc358762 bridge
> driver and panel driver, the combined version is already in tree:
> drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c
> but the tc358762 is clearly a generic bridge and the panel is generic
> too, so combining it into one panel driver doesn't seem right.

I see, so this is the remaining bits.  Perhaps the binding might help me
see how things fit together - I don't know anything about the system
really.  It's not doing anything that looks like it should cause the
framework too many problems so I'm not overly worried from that point of
view but equally well it's obviously not ideal.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC][PATCH] regulator: rpi-panel: Add regulator/backlight driver for RPi panel

[Marek Vasut]

On 8/1/20 3:16 AM, Mark Brown wrote:
> On Thu, Jul 30, 2020 at 09:37:48PM +0200, Marek Vasut wrote:
>> On 7/30/20 9:13 PM, Mark Brown wrote:
>>> On Thu, Jul 30, 2020 at 06:28:07PM +0200, Marek Vasut wrote:
> 
>>>> about it ? I can over-complicate this and split it into multiple
>>>> drivers, but I don't think it's worth the complexity, considering that
>>>> this is likely a one-off device which will never be re-used elsewhere,
>>>> except on this one particular display module for RPi.
> 
>>> Now you've written that you've pretty much guaranteed someone's going to
>>> use the same component elsewhere :)
> 
>> How? The firmware is closed and not available, neither is documentation
>> for it, sadly.
> 
> Companies often find other markets for their one off things, the
> original RPi is a great example of this!

I suspect the firmware in this ATTINY88 is written specifically for this
board, so I doubt re-use in its current form will happen. If there is
ever another display board, the firmware will likely be different to
match the new board. But that's all pure speculation.

>>> I don't 100% follow how this would actually get used in a
>>> system (perhaps the binding would help) but for these things if there's
>>> only one tightly coupled user that's possible it's sometimes simpler to
>>> just skip APIs and do things directly.
> 
>> That's what I'm trying to replace by this patch and tc358762 bridge
>> driver and panel driver, the combined version is already in tree:
>> drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c
>> but the tc358762 is clearly a generic bridge and the panel is generic
>> too, so combining it into one panel driver doesn't seem right.
> 
> I see, so this is the remaining bits.  Perhaps the binding might help me
> see how things fit together - I don't know anything about the system
> really.  It's not doing anything that looks like it should cause the
> framework too many problems so I'm not overly worried from that point of
> view but equally well it's obviously not ideal.

See below:

/ {
  panel: panel {
    compatible = "powertip,ph800480t013-idf02";
    power-supply = <&attiny>;
    backlight = <&attiny>;
    port {
      panel_in: endpoint {
        remote-endpoint = <&bridge_out>;
      };
    };
  };
};

&i2c {
  attiny: regulator@45 {
    compatible = "raspberrypi,7inch-touchscreen-panel-regulator";
    reg = <0x45>;
  };
};

&ltdc {
  status = "okay";

  port {
    ltdc_ep0_out: endpoint@0 {
      reg = <0>;
      remote-endpoint = <&dsi_in>;
    };
  };
};

&dsi {
  #address-cells = <1>;
  #size-cells = <0>;
  status = "okay";
  phy-dsi-supply = <&reg18>;

  bridge@0 {
    compatible = "toshiba,tc358762";
    reg = <0>;
    vddc-supply = <&attiny>;
    #address-cells = <1>;
    #size-cells = <0>;
    status = "okay";

    ports {
      #address-cells = <1>;
      #size-cells = <0>;

      port@0 {
        reg = <0>;
        bridge_in: endpoint {
          remote-endpoint = <&dsi_out>;
        };
      };

      port@1 {
        reg = <1>;
        bridge_out: endpoint {
          remote-endpoint = <&panel_in>;
        };
      };
    };
  };

  ports {
    #address-cells = <1>;
    #size-cells = <0>;

    port@0 {
      reg = <0>;
      dsi_in: endpoint {
        remote-endpoint = <&ltdc_ep0_out>;
      };
    };

    port@1 {
      reg = <1>;
      dsi_out: endpoint {
        remote-endpoint = <&bridge_in>;
      };
    };
  };
};
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC][PATCH] regulator: rpi-panel: Add regulator/backlight driver for RPi panel

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 587 bytes --]

On Mon, Aug 03, 2020 at 09:21:25AM +0200, Marek Vasut wrote:
> On 8/1/20 3:16 AM, Mark Brown wrote:

> > I see, so this is the remaining bits.  Perhaps the binding might help me
> > see how things fit together - I don't know anything about the system
> > really.  It's not doing anything that looks like it should cause the
> > framework too many problems so I'm not overly worried from that point of
> > view but equally well it's obviously not ideal.

> See below:

OK, basically I've got no real objection from a regulator point of view
- it's not ideal but not the end of the world.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC][PATCH] regulator: rpi-panel: Add regulator/backlight driver for RPi panel

[Marek Vasut]

On 8/3/20 9:48 PM, Mark Brown wrote:
> On Mon, Aug 03, 2020 at 09:21:25AM +0200, Marek Vasut wrote:
>> On 8/1/20 3:16 AM, Mark Brown wrote:
> 
>>> I see, so this is the remaining bits.  Perhaps the binding might help me
>>> see how things fit together - I don't know anything about the system
>>> really.  It's not doing anything that looks like it should cause the
>>> framework too many problems so I'm not overly worried from that point of
>>> view but equally well it's obviously not ideal.
> 
>> See below:
> 
> OK, basically I've got no real objection from a regulator point of view
> - it's not ideal but not the end of the world.

Then, how shall we proceed ?
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC][PATCH] regulator: rpi-panel: Add regulator/backlight driver for RPi panel

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 342 bytes --]

On Tue, Aug 04, 2020 at 12:29:49AM +0200, Marek Vasut wrote:
> On 8/3/20 9:48 PM, Mark Brown wrote:

> > OK, basically I've got no real objection from a regulator point of view
> > - it's not ideal but not the end of the world.

> Then, how shall we proceed ?

Submitting the patch properly with the binding document seems like a
good start.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel