[RESEND PATCH v3 1/3] drm/panel: refactor INNOLUX P079ZCA panel driver

[hl]

Hi Thierry Reding,


On Monday, March 12, 2018 05:21 PM, Thierry Reding wrote:
> On Mon, Dec 04, 2017 at 03:17:48PM +0800, Lin Huang wrote:
>> Refactor Innolux P079ZCA panel driver, let it support
>> multi panel.
>>
>> Signed-off-by: Lin Huang <hl at rock-chips.com>
>> ---
>> Changes in v2:
>> - Change regulator property name to meet the panel datasheet
>> Changes in v3:
>> - this patch only refactor P079ZCA panel to support multi panel, support P097PFG panel in another patch
>>
>>   drivers/gpu/drm/panel/panel-innolux-p079zca.c | 147 ++++++++++++++++++--------
>>   1 file changed, 105 insertions(+), 42 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/panel/panel-innolux-p079zca.c b/drivers/gpu/drm/panel/panel-innolux-p079zca.c
>> index 6ba9344..1597744 100644
>> --- a/drivers/gpu/drm/panel/panel-innolux-p079zca.c
>> +++ b/drivers/gpu/drm/panel/panel-innolux-p079zca.c
>> @@ -20,12 +20,32 @@
>>   
>>   #include <video/mipi_display.h>
>>   
>> +struct panel_desc {
>> +	const struct drm_display_mode *modes;
>> +	unsigned int bpc;
>> +	struct {
>> +		unsigned int width;
>> +		unsigned int height;
>> +	} size;
>> +};
>> +
>> +struct panel_desc_dsi {
>> +	struct panel_desc desc;
>> +
>> +	unsigned long flags;
>> +	enum mipi_dsi_pixel_format format;
>> +	unsigned int lanes;
>> +};
> There's no need for the two layers here. Just move everything from
> struct panel_desc_dsi into struct panel_desc.
Okay, will fix it.
>
>> +
>>   struct innolux_panel {
>>   	struct drm_panel base;
>>   	struct mipi_dsi_device *link;
>> +	const struct panel_desc_dsi *dsi_desc;
> And then this can just become:
>
> 	const struct panel_desc *desc;
>
> The _dsi suffix is redundant because this driver is exclusively for DSI
> devices.
>
Got it.
>> -static int innolux_panel_add(struct innolux_panel *innolux)
>> +static int innolux_panel_add(struct mipi_dsi_device *dsi,
>> +			     const struct panel_desc_dsi *desc)
>>   {
>> -	struct device *dev = &innolux->link->dev;
>> +	struct innolux_panel *innolux;
>> +	struct device *dev = &dsi->dev;
>>   	struct device_node *np;
>>   	int err;
>>   
>> -	innolux->supply = devm_regulator_get(dev, "power");
>> -	if (IS_ERR(innolux->supply))
>> -		return PTR_ERR(innolux->supply);
>> +	innolux = devm_kzalloc(dev, sizeof(*innolux), GFP_KERNEL);
>> +	if (!innolux)
>> +		return -ENOMEM;
>> +
>> +	innolux->dsi_desc = desc;
>> +	innolux->vddi = devm_regulator_get(dev, "power");
>> +	if (IS_ERR(innolux->vddi))
>> +		return PTR_ERR(innolux->vddi);
>> +
>> +	innolux->avdd = devm_regulator_get(dev, "avdd");
>> +	if (IS_ERR(innolux->avdd))
>> +		return PTR_ERR(innolux->avdd);
>> +
>> +	innolux->avee = devm_regulator_get(dev, "avee");
>> +	if (IS_ERR(innolux->avee))
>> +		return PTR_ERR(innolux->avee);
> According to the device tree bindings these regulators are all optional.
> Now devm_regulator_get() will return a dummy regulator if one has not
> been specified in DT, so this seems like it should work fine, even for
> existing DTBs that don't have the avdd and avee regulators. However, the
> DT bindings seem to be wrong, because these are in fact required
> regulators.
Actually, the vddi is request, and avdd and avee is optional, i will 
only check
vddi error later.
>>   
>>   	innolux->enable_gpio = devm_gpiod_get_optional(dev, "enable",
>>   						       GPIOD_OUT_HIGH);
>> @@ -243,14 +306,16 @@ static int innolux_panel_add(struct innolux_panel *innolux)
>>   
>>   	drm_panel_init(&innolux->base);
>>   	innolux->base.funcs = &innolux_panel_funcs;
>> -	innolux->base.dev = &innolux->link->dev;
>> +	innolux->base.dev = dev;
>>   
>>   	err = drm_panel_add(&innolux->base);
>>   	if (err < 0)
>>   		goto put_backlight;
>>   
>> -	return 0;
>> +	dev_set_drvdata(dev, innolux);
>> +	innolux->link = dsi;
>>   
>> +	return 0;
>>   put_backlight:
>>   	put_device(&innolux->backlight->dev);
>>   
>> @@ -267,28 +332,25 @@ static void innolux_panel_del(struct innolux_panel *innolux)
>>   
>>   static int innolux_panel_probe(struct mipi_dsi_device *dsi)
>>   {
>> -	struct innolux_panel *innolux;
>> -	int err;
>>   
>> -	dsi->lanes = 4;
>> -	dsi->format = MIPI_DSI_FMT_RGB888;
>> -	dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_SYNC_PULSE |
>> -			  MIPI_DSI_MODE_LPM;
>> -
>> -	innolux = devm_kzalloc(&dsi->dev, sizeof(*innolux), GFP_KERNEL);
>> -	if (!innolux)
>> -		return -ENOMEM;
>> +	const struct panel_desc_dsi *dsi_desc;
>> +	const struct of_device_id *id;
>> +	int err;
>>   
>> -	mipi_dsi_set_drvdata(dsi, innolux);
>> +	id = of_match_node(innolux_of_match, dsi->dev.of_node);
>> +	if (!id)
>> +		return -ENODEV;
>>   
>> -	innolux->link = dsi;
>> +	dsi_desc = id->data;
>> +	dsi->mode_flags = dsi_desc->flags;
>> +	dsi->format = dsi_desc->format;
>> +	dsi->lanes = dsi_desc->lanes;
>>   
>> -	err = innolux_panel_add(innolux);
>> +	err = innolux_panel_add(dsi, dsi_desc);
>>   	if (err < 0)
>>   		return err;
>>   
>> -	err = mipi_dsi_attach(dsi);
>> -	return err;
>> +	return mipi_dsi_attach(dsi);
>>   }
>>   
>>   static int innolux_panel_remove(struct mipi_dsi_device *dsi)
>> @@ -326,7 +388,7 @@ static void innolux_panel_shutdown(struct mipi_dsi_device *dsi)
>>   
>>   static struct mipi_dsi_driver innolux_panel_driver = {
>>   	.driver = {
>> -		.name = "panel-innolux-p079zca",
>> +		.name = "panel-innolux-dsi",
> No need to change that. Also, probably a bad idea to change it because
> there are (or will be) likely other DSI panels from Innolux that are not
> compatible with this driver.
Okay, will fix it.
>
> Thierry