Re: [PATCH v1 1/4] fbtft: Unorphan the driver

[Javier Martinez Canillas <javierm@redhat.com>]

Hello Andy,

Thanks a lot for your feedback.

On 1/31/22 12:36, Andy Shevchenko wrote:

[snip]

>>
>> Another hack is that I am just hardcoding the {width, height}_mm, but I don't
>> know what DPI could be used for these panels nor how I could calculate the mm.
> 
> I think the hacks is the first what should be eliminated, also see below.
>

Yes, agreed. But as we discussed with Thomas I'll post anyways since these could
be addressed as a follow-up.
 
> ...
> 
>> +config TINYDRM_SSD130X
>> +	tristate "DRM support for Solomon SSD130X OLED displays"
>> +	depends on DRM && OF && I2C
> 
> Please, make sure that it does NOT dependent on OF.
> 

I actually added this dependency deliberative. It's true that the driver is using
the device properties API and so there isn't anything from the properties parsing
point of view that depends on OF. And the original driver didn't depend on OF.

But the original driver also only would had worked with Device Trees since the
of_device_id table is the only one that contains the device specific data info.

The i2c_device_id table only listed the devices supported to match, but then it
would only had worked with the default values that are set by the driver.

So in practice it *does* depend on OF. I'll be happy to drop that dependency if
you provide an acpi_device_id table to match.

> ...
> 
>> +obj-$(CONFIG_TINYDRM_SSD130X)		+= ssd130x.o
> 
> I would keep the original name since we have I2C (fbdev) implementation, SPI
> and platform (fbtft), and now i2c (drm). I would like to avoid more confusion
> that we already have.
> 

I see. That makes sense. Will I keep the original ssd1307 name then and not
rename it to ssd130x (even though it would be more precise since supports a
family of displays).

Best regards,
-- 
Javier Martinez Canillas
Linux Engineering
Red Hat