Re: [RFC 1/4] video: panel: add CLAA101WA01A panel support

[Mark Zhang <nvmarkzhang@gmail.com>]

On 01/30/2013 11:02 AM, Alexandre Courbot wrote:
> Add support for the Chunghwa CLAA101WA01A display panel.
> 
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
> ---
[...]
> +
> +#include <video/display.h>
> +
> +#define CLAA101WA01A_WIDTH	223
> +#define CLAA101WA01A_HEIGHT	125

If I remember correct, the physical size of the panel can be fetched in
EDID. If this is correct, we don't have to hard-code here.

> +
> +struct panel_claa101 {
> +	struct display_entity entity;
> +	struct regulator *vdd_pnl;
> +	struct regulator *vdd_bl;
> +	/* Enable GPIOs */
> +	int pnl_enable;
> +	int bl_enable;
> +};
> +
> +#define to_panel_claa101(p)	container_of(p, struct panel_claa101, entity)
> +
> +static int panel_claa101_set_state(struct display_entity *entity,
> +				   enum display_entity_state state)
> +{
> +	struct panel_claa101 *panel = to_panel_claa101(entity);
> +
> +	/* OFF and STANDBY are equivalent to us */
> +	if (state = DISPLAY_ENTITY_STATE_STANDBY)
> +		state = DISPLAY_ENTITY_STATE_OFF;

Do we need this? The "switch" below handles the same thing already.

> +
> +	switch (state) {
> +	case DISPLAY_ENTITY_STATE_OFF:
> +	case DISPLAY_ENTITY_STATE_STANDBY:
> +		if (entity->source)
> +			display_entity_set_stream(entity->source,
> +						 DISPLAY_ENTITY_STREAM_STOPPED);
> +
> +		/* TODO error checking? */
> +		gpio_set_value_cansleep(panel->bl_enable, 0);
> +		usleep_range(10000, 10000);
> +		regulator_disable(panel->vdd_bl);
> +		usleep_range(200000, 200000);
> +		gpio_set_value_cansleep(panel->pnl_enable, 0);
> +		regulator_disable(panel->vdd_pnl);
> +		break;
> +
> +	case DISPLAY_ENTITY_STATE_ON:
> +		regulator_enable(panel->vdd_pnl);
> +		gpio_set_value_cansleep(panel->pnl_enable, 1);
> +		usleep_range(200000, 200000);
> +		regulator_enable(panel->vdd_bl);
> +		usleep_range(10000, 10000);
> +		gpio_set_value_cansleep(panel->bl_enable, 1);
> +
> +		if (entity->source)
> +			display_entity_set_stream(entity->source,
> +					      DISPLAY_ENTITY_STREAM_CONTINUOUS);
> +		break;
> +	}
> +
> +	return 0;
> +}
> +
> +static int panel_claa101_get_modes(struct display_entity *entity,
> +				   const struct videomode **modes)
> +{
> +	/* TODO get modes from EDID? */

Why not move the "nvidia,ddc" from encoder's DT to panel's DT? In that
case, you can get EDID here. I know drm has some helpers to fetch EDID
but I recall there are some other functions which has no drm
dependencies which may be suitable for you.

> +	return 0;
> +}
[...]
> +
> +static int __exit panel_claa101_remove(struct platform_device *pdev)
> +{
> +	struct panel_claa101 *panel = platform_get_drvdata(pdev);
> +
> +	display_entity_unregister(&panel->entity);
> +
> +	return 0;
> +}
> +
> +#ifdef CONFIG_OF

We don't need this kind of "ifdef" in upstream kernel.

> +static struct of_device_id panel_claa101_of_match[] = {
> +	{ .compatible = "chunghwa,claa101wa01a", },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, pwm_backlight_of_match);

What does this mean? Why we need this?

> +#else
> +#endif
> +
> +static const struct dev_pm_ops panel_claa101_dev_pm_ops = {
> +};
> +
> +static struct platform_driver panel_claa101_driver = {
> +	.probe = panel_claa101_probe,
> +	.remove = panel_claa101_remove,
> +	.driver = {
> +		.name = "panel_claa101wa01a",
> +		.owner = THIS_MODULE,
> +#ifdef CONFIG_PM
> +		.pm = &panel_claa101_dev_pm_ops,

If you haven't implemented this in this patch set, I suggest removing this.

> +#endif
> +#ifdef CONFIG_OF
> +		.of_match_table	= of_match_ptr(panel_claa101_of_match),
> +#endif
> +	},
> +};
> +
> +module_platform_driver(panel_claa101_driver);
> +
> +MODULE_AUTHOR("Alexandre Courbot <acourbot@nvidia.com>");
> +MODULE_DESCRIPTION("Chunghwa CLAA101WA01A Display Panel");
> +MODULE_LICENSE("GPL");
>