devicetree.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/2] dt-bindings: add vendor-prefixes and bindings for pcd8544 displays
@ 2023-07-19 10:24 Viktar Simanenka
  2023-07-19 10:24 ` [PATCH 2/2] drm/tiny: add display driver for philips pcd8544 display controller Viktar Simanenka
  2023-07-19 10:29 ` [PATCH 1/2] dt-bindings: add vendor-prefixes and bindings for pcd8544 displays Krzysztof Kozlowski
  0 siblings, 2 replies; 6+ messages in thread
From: Viktar Simanenka @ 2023-07-19 10:24 UTC (permalink / raw)
  To: David Airlie, Daniel Vetter, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley, Viktar Simanenka, dri-devel, devicetree,
	linux-kernel

Signed-off-by: Viktar Simanenka <viteosen@gmail.com>
---
 .../bindings/display/philips,pcd8544.yaml     | 89 +++++++++++++++++++
 .../devicetree/bindings/vendor-prefixes.yaml  |  2 +
 2 files changed, 91 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/philips,pcd8544.yaml

diff --git a/Documentation/devicetree/bindings/display/philips,pcd8544.yaml b/Documentation/devicetree/bindings/display/philips,pcd8544.yaml
new file mode 100644
index 000000000000..ac880d9d8cc1
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/philips,pcd8544.yaml
@@ -0,0 +1,89 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/display/philips,pcd8544.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Philips PCD8544 LCD Display Controller
+
+maintainers:
+  - Viktar Simanenka <viteosen@gmail.com>
+
+description: |
+  Philips PCD8544 LCD Display Controller with SPI control bus.
+  Monochrome 84x48 LCD displays, such as Nokia 5110/3310 LCDs.
+  May contain backlight LED.
+
+allOf:
+  - $ref: panel/panel-common.yaml#
+  - $ref: /schemas/spi/spi-peripheral-props.yaml#
+
+properties:
+  compatible:
+    oneOf:
+      - enum:
+          - philips,pcd8544
+
+  dc-gpios:
+    maxItems: 1
+    description: Data/Command selection pin (D/CX)
+
+  reset-gpios:
+    maxItems: 1
+    description: Display Reset pin (RST)
+
+  philips,inverted:
+    type: boolean
+    description: Display color inversion
+
+  philips,voltage-op:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    minimum: 0
+    maximum: 127
+    description: Display liquid crystal operation voltage
+
+  philips,bias:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    minimum: 0
+    maximum: 7
+    description: Display bias voltage system value
+
+  philips,temperature-coeff:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    minimum: 0
+    maximum: 3
+    description: Display temperature compensation coefficient
+
+required:
+  - compatible
+  - reg
+  - dc-gpios
+  - reset-gpios
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+
+    spi {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        display@0 {
+            compatible = "philips,pcd8544";
+            spi-max-frequency = <8000000>;
+            reg = <0>;
+
+            dc-gpios = <&pio 0 3 GPIO_ACTIVE_HIGH>; /* DC=PA3 */
+            reset-gpios = <&pio 0 1 GPIO_ACTIVE_HIGH>; /* RESET=PA1 */
+            backlight = <&backlight>;
+
+            philips,inverted;
+            philips,voltage-op = <0>;
+            philips,bias = <4>;
+            philips,temperature-coeff = <0>;
+        };
+    };
+
+...
diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml
index af60bf1a6664..0c3844af6776 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
+++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
@@ -1037,6 +1037,8 @@ patternProperties:
     description: Pervasive Displays, Inc.
   "^phicomm,.*":
     description: PHICOMM Co., Ltd.
+  "^philips,.*":
+    description: Koninklijke Philips N.V.
   "^phytec,.*":
     description: PHYTEC Messtechnik GmbH
   "^picochip,.*":
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH] TinyDRM display driver for Philips PCD8544 display controller
@ 2023-07-18  8:35 Krzysztof Kozlowski
  2023-07-19  9:29 ` [PATCH 1/2] dt-bindings: add vendor-prefixes and bindings for pcd8544 displays Viktar Simanenka
  0 siblings, 1 reply; 6+ messages in thread
From: Krzysztof Kozlowski @ 2023-07-18  8:35 UTC (permalink / raw)
  To: Viktar Simanenka, David Airlie, Daniel Vetter, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, dri-devel, devicetree,
	linux-kernel

On 18/07/2023 10:07, Viktar Simanenka wrote:
> Support for common monochrome LCD displays based on PCD8544 (such as Nokia 5110/3310 LCD) SPI controlled displays.

Please wrap commit message according to Linux coding style / submission
process (neither too early nor over the limit):
https://elixir.bootlin.com/linux/v6.4-rc1/source/Documentation/process/submitting-patches.rst#L597

> 
> Signed-off-by: Viktar Simanenka <viteosen@gmail.com>
> ---
>  .../bindings/display/philips,pcd8544.yaml     |  92 ++++

Bindings are always separate patches.

Please run scripts/checkpatch.pl and fix reported warnings. Some
warnings can be ignored, but the code here looks like it needs a fix.
Feel free to get in touch if the warning is not clear.

>  drivers/gpu/drm/tiny/Kconfig                  |  11 +
>  drivers/gpu/drm/tiny/pcd8544.c                | 506 ++++++++++++++++++
>  3 files changed, 609 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/display/philips,pcd8544.yaml
>  create mode 100644 drivers/gpu/drm/tiny/pcd8544.c
> 
> diff --git a/Documentation/devicetree/bindings/display/philips,pcd8544.yaml b/Documentation/devicetree/bindings/display/philips,pcd8544.yaml
> new file mode 100644
> index 000000000000..d56a0c747812
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/philips,pcd8544.yaml
> @@ -0,0 +1,92 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/display/philips,pcd8544.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Philips PCD8544 LCD Display Controller
> +
> +maintainers:
> +  - Viktar Simanenka <viteosen@gmail.com>
> +
> +description: |
> +  Philips PCD8544 LCD Display Controller with SPI control bus.
> +  This is a driver for monochrome 84x48 LCD displays,

LCD Display controller is a driver? Are you sure? Or maybe you are
describing Linux driver? If so, drop references to drivers and describe
the hardware.

> +  such as Nokia 5110/3310 LCDs. May contain backlight LED.
> +
> +allOf:
> +  - $ref: panel/panel-common.yaml#
> +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
> +
> +properties:
> +  compatible:
> +    const: philips,pcd8544
> +
> +  dc-gpios:
> +    maxItems: 1
> +    description: Display data/command selection (D/CX)
> +
> +  reset-gpios:
> +    maxItems: 1
> +    description: Display hardware reset line (RST)
> +
> +  inverted:
> +    maxItems: 1
> +    description: Invert display colors

Missing vendor prefix, ref/type, incorrect maxItems (???).

You described the desired Linux feature or behavior, not the actual
hardware. The bindings are about the latter, so instead you need to
rephrase the property and its description to match actual hardware
capabilities/features/configuration etc.

> +
> +  voltage_op:

No underscores in names.

> +    maxItems: 1
> +    description: Set operation voltage (contrast)

What? Sorry, I don't understand.

> +
> +  bias:
> +    maxItems: 1
> +    description: Bias voltage level
> +
> +  temperature_coeff:
> +    maxItems: 1
> +    description: Temperature coefficient

You just copied the property name into description. Very helpful.

You clearly did not test it.

> +
> +required:
> +  - compatible
> +  - reg
> +  - dc-gpios
> +  - reset-gpios
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    display@0 {
> +        compatible = "philips,pcd8544";
> +        spi-max-frequency = <8000000>;
> +        reg = <0>;
> +
> +        dc-gpios = <&pio 0 3 0>; /* DC=PA3 */
> +        reset-gpios = <&pio 0 1 0>; /* RESET=PA1 */

Use proper defines for flags.

> +        backlight = <&display_backlight>;
> +        write-only;

It does not look like you tested the bindings, at least after quick
look. Please run `make dt_binding_check` (see
Documentation/devicetree/bindings/writing-schema.rst for instructions).
Maybe you need to update your dtschema and yamllint.

> +
> +        /* Default values: */
> +        inverted = <0>;
> +        voltage_op = <0>;
> +        bias = <4>;
> +        temperature_coeff = <0>;

What? Your code is confusing.

> +
> +        width-mm = <35>;
> +        height-mm = <28>;
> +
> +        panel-timing {
> +            hactive = <84>;
> +            vactive = <48>;
> +            hback-porch = <0>;
> +            vback-porch = <0>;
> +
> +            clock-frequency = <0>;
> +            hfront-porch = <0>;
> +            hsync-len = <0>;
> +            vfront-porch = <0>;
> +            vsync-len = <0>;
> +        };
> +    };
> +

...

> +
> +static int pcd8544_probe(struct spi_device *spi)
> +{
> +	struct device *dev = &spi->dev;
> +	struct pcd8544_device *pcd8544_dev;
> +	struct drm_device *drm;
> +	int ret;
> +	static const uint64_t modifiers[] = {
> +		DRM_FORMAT_MOD_LINEAR,
> +		DRM_FORMAT_MOD_INVALID
> +	};

Missing blank line.

> +	pcd8544_dev = devm_drm_dev_alloc(dev, &pcd8544_driver, struct pcd8544_device, drm);
> +

Drop blank line.

> +	if (IS_ERR(pcd8544_dev))
> +		return PTR_ERR(pcd8544_dev);
> +
> +	pcd8544_dev->spi = spi;
> +
> +	pcd8544_dev->reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
> +	if (IS_ERR(pcd8544_dev->reset))
> +		return dev_err_probe(dev, PTR_ERR(pcd8544_dev->reset), "Failed to get GPIO 'reset'\n");

You start in reset mode? Didn't you just confure values? 1 is reset.

> +
> +	pcd8544_dev->dc = devm_gpiod_get(dev, "dc", GPIOD_OUT_LOW);
> +	if (IS_ERR(pcd8544_dev->dc))
> +		return dev_err_probe(dev, PTR_ERR(pcd8544_dev->dc), "Failed to get GPIO 'dc'\n");
> +
> +	pcd8544_dev->backlight = devm_of_find_backlight(dev);
> +	if (IS_ERR(pcd8544_dev->backlight))
> +		return PTR_ERR(pcd8544_dev->backlight);
> +
> +	if (device_property_read_u32(dev, "inverted", &pcd8544_dev->inverted))
> +		pcd8544_dev->inverted = 0;
> +	if (device_property_read_u32(dev, "temperature_coeff", &pcd8544_dev->temperature_coeff))
> +		pcd8544_dev->temperature_coeff = 0;
> +	if (device_property_read_u32(dev, "bias", &pcd8544_dev->bias))
> +		pcd8544_dev->bias = 4;
> +	if (device_property_read_u32(dev, "voltage_op", &pcd8544_dev->voltage_op))
> +		pcd8544_dev->voltage_op = 0;
> +
> +	if (!dev->coherent_dma_mask) {
> +		ret = dma_coerce_mask_and_coherent(dev, DMA_BIT_MASK(32));
> +		if (ret) {
> +			dev_warn(dev, "Failed to set dma mask %d\n", ret);
> +			return ret;
> +		}
> +	}
> +
> +	drm_mode_copy(&pcd8544_dev->mode, &pcd8544_mode);
> +	pcd8544_dev->width = pcd8544_mode.hdisplay;
> +	pcd8544_dev->height = pcd8544_mode.vdisplay;
> +	pcd8544_dev->tx_buflen = pcd8544_dev->width * DIV_ROUND_UP(pcd8544_dev->height, 8);
> +	pcd8544_dev->tx_buf = devm_kzalloc(dev, pcd8544_dev->tx_buflen, GFP_KERNEL);
> +	if (!pcd8544_dev->tx_buf)
> +		return -ENOMEM;
> +
> +	drm = &pcd8544_dev->drm;
> +	ret = drmm_mode_config_init(drm);
> +	if (ret)
> +		return ret;
> +
> +	drm_connector_helper_add(&pcd8544_dev->connector, &pcd8544_connector_hfuncs);
> +	ret = drm_connector_init(drm, &pcd8544_dev->connector, &pcd8544_connector_funcs, DRM_MODE_CONNECTOR_SPI);
> +	if (ret)
> +		return ret;
> +
> +	drm->mode_config.funcs = &pcd8544_mode_config_funcs;
> +	drm->mode_config.min_width = pcd8544_dev->mode.hdisplay;
> +	drm->mode_config.max_width = pcd8544_dev->mode.hdisplay;
> +	drm->mode_config.min_height = pcd8544_dev->mode.vdisplay;
> +	drm->mode_config.max_height = pcd8544_dev->mode.vdisplay;
> +
> +	ret = drm_simple_display_pipe_init(drm, &pcd8544_dev->pipe, &pcd8544_pipe_funcs,
> +					pcd8544_formats, ARRAY_SIZE(pcd8544_formats),
> +					modifiers, &pcd8544_dev->connector);
> +	if (ret)
> +		return ret;
> +
> +	drm_plane_enable_fb_damage_clips(&pcd8544_dev->pipe.plane);
> +
> +	spi_set_drvdata(spi, drm);
> +
> +	drm_mode_config_reset(drm);
> +
> +	ret = drm_dev_register(drm, 0);
> +	if (ret)
> +		return ret;
> +
> +	drm_dbg(drm, "SPI speed: %uMHz\n", spi->max_speed_hz / 1000000);
> +
> +	drm_fbdev_generic_setup(drm, 0);
> +
> +	return 0;
> +}
> +
> +static void pcd8544_remove(struct spi_device *spi)
> +{
> +	struct drm_device *drm = spi_get_drvdata(spi);
> +
> +	drm_dev_unplug(drm);
> +	drm_atomic_helper_shutdown(drm);
> +}
> +
> +static void pcd8544_shutdown(struct spi_device *spi)
> +{
> +	drm_atomic_helper_shutdown(spi_get_drvdata(spi));
> +}
> +
> +static struct spi_driver pcd8544_spi_driver = {
> +	.driver = {
> +		.name = "pcd8544",
> +		.owner = THIS_MODULE,

Drop. And run coccinelle, smatch and sparse.



Best regards,
Krzysztof


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2023-07-19 10:30 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-07-19 10:24 [PATCH 1/2] dt-bindings: add vendor-prefixes and bindings for pcd8544 displays Viktar Simanenka
2023-07-19 10:24 ` [PATCH 2/2] drm/tiny: add display driver for philips pcd8544 display controller Viktar Simanenka
2023-07-19 10:29   ` Krzysztof Kozlowski
2023-07-19 10:29 ` [PATCH 1/2] dt-bindings: add vendor-prefixes and bindings for pcd8544 displays Krzysztof Kozlowski
  -- strict thread matches above, loose matches on Subject: below --
2023-07-18  8:35 [PATCH] TinyDRM display driver for Philips PCD8544 display controller Krzysztof Kozlowski
2023-07-19  9:29 ` [PATCH 1/2] dt-bindings: add vendor-prefixes and bindings for pcd8544 displays Viktar Simanenka
2023-07-19  9:50   ` Krzysztof Kozlowski

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).