Re: [PATCH] TinyDRM display driver for Philips PCD8544 display controller

[Krzysztof Kozlowski]

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@xxxxxxxxx>
> ---
>  .../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@xxxxxxxxx>
> +
> +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