Re: [PATCH][RFC] Add support for Epson S1D13806 FB

-------------------

Looks good.  Just a few comments:

1. If you don't have a check_var function, might as well remove it

for now.

Otherwise, it's possible for the user to enter invalid mode values

and

your driver will accept those unconditionally. The disadvantage, of

course,

is that you cannot change the video mode after driver load.

Right. In any case, only depth changing was handled, and in a very
limited way. This is more preliminary code than anything else. Given
the limited performances of the chip, it's not a big deal anyway :)

check_var function removed.

2. Although it's ugly, might as well include something similar to

this in

s1d13xxxfb_init(void):

if (fb_get_options("s1d13xxfb", NULL)
	return -ENODEV;

to make general fbdev boot options work, such as:

video=xxxfb:off, video=xxxfb:ofonly

Tested and added to the attached patch.

3. You can use pci_resource_len()/pci_resource_start() instead of

Actually I don't think so: this chip is often found on embedded
platforms that don't have PCI bus. Hence all the
platform_device/platform_data glue in the driver. My understanding is
that pci_resource_* macros are only available when CONFIG_PCI is
enabled. As far as I can tell from the documentation, there's no PCI
version of that chip family. That's also the reason why the Kconfig
option only depends on CONFIG_FB.

No need, I'll take care of merging the driver.

Thanks! Please let me know if the attached patch is suitable for
inclusion into mainline :)

Greetings,

Thibaut VARENE