| Message ID | 20220411211243.11121-5-javierm@redhat.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | drm/solomon: Add SSD130x OLED displays SPI support | expand |
Hi Javier, Thanks for your patch! On Mon, Apr 11, 2022 at 11:12 PM Javier Martinez Canillas <javierm@redhat.com> wrote: > These are declared in the ssd130x-i2c transport driver but the information > is not I2C specific, and could be used by other SSD130x transport drivers. > > Move them to the ssd130x core driver and just set the OF device entries to > an ID that could be used to lookup the correct device info from an array. > > While being there, also move the SSD130X_DATA and SSD130X_COMMAND control > bytes. Since even though are used by the I2C interface, it could also be though they are ... they could > useful for other transport protocols such as SPI. > > Suggested-by: Chen-Yu Tsai <wens@kernel.org> > Signed-off-by: Javier Martinez Canillas <javierm@redhat.com> > --- a/drivers/gpu/drm/solomon/ssd130x.c > +++ b/drivers/gpu/drm/solomon/ssd130x.c > @@ -860,7 +890,14 @@ struct ssd130x_device *ssd130x_probe(struct device *dev, struct regmap *regmap) > > ssd130x->dev = dev; > ssd130x->regmap = regmap; > - ssd130x->device_info = device_get_match_data(dev); > + > + variant = (enum ssd130x_variants)device_get_match_data(dev); (uintptr_t), to avoid a cast from pointer to integer of different size warning. > + Please drop the blank line. > + if (variant >= NR_SSD130X_VARIANTS) > + return ERR_PTR(dev_err_probe(dev, -EINVAL, > + "Invalid SSD130x variant\n")); > + > + ssd130x->device_info = &ssd130x_variants[variant]; > > if (ssd130x->device_info->page_mode_only) > ssd130x->page_address_mode = 1; With the above fixed: Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Hi Javier,
I love your patch! Perhaps something to improve:
[auto build test WARNING on drm-tip/drm-tip]
[also build test WARNING on next-20220411]
[cannot apply to drm/drm-next drm-exynos/exynos-drm-next drm-intel/for-linux-next tegra-drm/drm/tegra/for-next linus/master linux/master v5.18-rc2]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/intel-lab-lkp/linux/commits/Javier-Martinez-Canillas/drm-solomon-Add-SSD130x-OLED-displays-SPI-support/20220412-051518
base: git://anongit.freedesktop.org/drm/drm-tip drm-tip
config: x86_64-randconfig-a001-20220411 (https://download.01.org/0day-ci/archive/20220412/202204121542.aU2BiYXN-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project fe2478d44e4f7f191c43fef629ac7a23d0251e72)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/ac5a07cda8a0f8d4948e6a01d0b3bb6ce9fe7830
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Javier-Martinez-Canillas/drm-solomon-Add-SSD130x-OLED-displays-SPI-support/20220412-051518
git checkout ac5a07cda8a0f8d4948e6a01d0b3bb6ce9fe7830
# save the config file to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/gpu/drm/solomon/
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
>> drivers/gpu/drm/solomon/ssd130x.c:894:12: warning: cast to smaller integer type 'enum ssd130x_variants' from 'const void *' [-Wvoid-pointer-to-enum-cast]
variant = (enum ssd130x_variants)device_get_match_data(dev);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 warning generated.
vim +894 drivers/gpu/drm/solomon/ssd130x.c
874
875 struct ssd130x_device *ssd130x_probe(struct device *dev, struct regmap *regmap)
876 {
877 struct ssd130x_device *ssd130x;
878 enum ssd130x_variants variant;
879 struct backlight_device *bl;
880 struct drm_device *drm;
881 int ret;
882
883 ssd130x = devm_drm_dev_alloc(dev, &ssd130x_drm_driver,
884 struct ssd130x_device, drm);
885 if (IS_ERR(ssd130x))
886 return ERR_PTR(dev_err_probe(dev, PTR_ERR(ssd130x),
887 "Failed to allocate DRM device\n"));
888
889 drm = &ssd130x->drm;
890
891 ssd130x->dev = dev;
892 ssd130x->regmap = regmap;
893
> 894 variant = (enum ssd130x_variants)device_get_match_data(dev);
895
896 if (variant >= NR_SSD130X_VARIANTS)
897 return ERR_PTR(dev_err_probe(dev, -EINVAL,
898 "Invalid SSD130x variant\n"));
899
900 ssd130x->device_info = &ssd130x_variants[variant];
901
902 if (ssd130x->device_info->page_mode_only)
903 ssd130x->page_address_mode = 1;
904
905 ssd130x_parse_properties(ssd130x);
906
907 ret = ssd130x_get_resources(ssd130x);
908 if (ret)
909 return ERR_PTR(ret);
910
911 bl = devm_backlight_device_register(dev, dev_name(dev), dev, ssd130x,
912 &ssd130xfb_bl_ops, NULL);
913 if (IS_ERR(bl))
914 return ERR_PTR(dev_err_probe(dev, PTR_ERR(bl),
915 "Unable to register backlight device\n"));
916
917 bl->props.brightness = ssd130x->contrast;
918 bl->props.max_brightness = MAX_CONTRAST;
919 ssd130x->bl_dev = bl;
920
921 ret = ssd130x_init_modeset(ssd130x);
922 if (ret)
923 return ERR_PTR(ret);
924
925 ret = drm_dev_register(drm, 0);
926 if (ret)
927 return ERR_PTR(dev_err_probe(dev, ret, "DRM device register failed\n"));
928
929 drm_fbdev_generic_setup(drm, 0);
930
931 return ssd130x;
932 }
933 EXPORT_SYMBOL_GPL(ssd130x_probe);
934
On 4/12/22 09:23, Geert Uytterhoeven wrote: > Hi Javier, > > Thanks for your patch! > > On Mon, Apr 11, 2022 at 11:12 PM Javier Martinez Canillas > <javierm@redhat.com> wrote: >> These are declared in the ssd130x-i2c transport driver but the information >> is not I2C specific, and could be used by other SSD130x transport drivers. >> >> Move them to the ssd130x core driver and just set the OF device entries to >> an ID that could be used to lookup the correct device info from an array. >> >> While being there, also move the SSD130X_DATA and SSD130X_COMMAND control >> bytes. Since even though are used by the I2C interface, it could also be > > though they are ... they could > Right, will fix it. >> useful for other transport protocols such as SPI. >> >> Suggested-by: Chen-Yu Tsai <wens@kernel.org> >> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com> > >> --- a/drivers/gpu/drm/solomon/ssd130x.c >> +++ b/drivers/gpu/drm/solomon/ssd130x.c > >> @@ -860,7 +890,14 @@ struct ssd130x_device *ssd130x_probe(struct device *dev, struct regmap *regmap) >> >> ssd130x->dev = dev; >> ssd130x->regmap = regmap; >> - ssd130x->device_info = device_get_match_data(dev); >> + >> + variant = (enum ssd130x_variants)device_get_match_data(dev); > > (uintptr_t), to avoid a cast from pointer to integer of different > size warning. > Indeed. The kernel test robot reported the same. >> + > > Please drop the blank line. > Ok. >> + if (variant >= NR_SSD130X_VARIANTS) >> + return ERR_PTR(dev_err_probe(dev, -EINVAL, >> + "Invalid SSD130x variant\n")); >> + >> + ssd130x->device_info = &ssd130x_variants[variant]; >> >> if (ssd130x->device_info->page_mode_only) >> ssd130x->page_address_mode = 1; > > With the above fixed: > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> > Thanks!
On Tue, Apr 12, 2022 at 10:07:02AM +0200, Javier Martinez Canillas wrote: > On 4/12/22 09:23, Geert Uytterhoeven wrote: > > On Mon, Apr 11, 2022 at 11:12 PM Javier Martinez Canillas > > <javierm@redhat.com> wrote: ... > >> - ssd130x->device_info = device_get_match_data(dev); > >> + > >> + variant = (enum ssd130x_variants)device_get_match_data(dev); > > > > (uintptr_t), to avoid a cast from pointer to integer of different > > size warning. > > > > Indeed. The kernel test robot reported the same. Not only because of this, but also with the non-NULL pointers I prefer the old style without ugly castings. Instead, you may export the array (in the driver's namespace) and use &info[ID] pointer for the specific device info.
On Tue, Apr 12, 2022 at 02:21:08PM +0300, Andy Shevchenko wrote: > On Tue, Apr 12, 2022 at 10:07:02AM +0200, Javier Martinez Canillas wrote: > > On 4/12/22 09:23, Geert Uytterhoeven wrote: > > > On Mon, Apr 11, 2022 at 11:12 PM Javier Martinez Canillas > > > <javierm@redhat.com> wrote: ... > > >> - ssd130x->device_info = device_get_match_data(dev); > > >> + > > >> + variant = (enum ssd130x_variants)device_get_match_data(dev); > > > > > > (uintptr_t), to avoid a cast from pointer to integer of different > > > size warning. > > > > > > > Indeed. The kernel test robot reported the same. > > Not only because of this, but also with the non-NULL pointers I prefer the old > style without ugly castings. > > Instead, you may export the array (in the driver's namespace) and use > &info[ID] pointer for the specific device info. Note that device_get_match_data() has no clue if the data is absent or data == (void *)0.
Hello Andy, Thanks for your feedback. On 4/12/22 13:21, Andy Shevchenko wrote: > On Tue, Apr 12, 2022 at 10:07:02AM +0200, Javier Martinez Canillas wrote: >> On 4/12/22 09:23, Geert Uytterhoeven wrote: >>> On Mon, Apr 11, 2022 at 11:12 PM Javier Martinez Canillas >>> <javierm@redhat.com> wrote: > > ... > >>>> - ssd130x->device_info = device_get_match_data(dev); >>>> + >>>> + variant = (enum ssd130x_variants)device_get_match_data(dev); >>> >>> (uintptr_t), to avoid a cast from pointer to integer of different >>> size warning. >>> >> >> Indeed. The kernel test robot reported the same. > > Not only because of this, but also with the non-NULL pointers I prefer the old > style without ugly castings. > > Instead, you may export the array (in the driver's namespace) and use > &info[ID] pointer for the specific device info. > That's a great idea! I'll do that in v3.
On 4/12/22 13:22, Andy Shevchenko wrote: > On Tue, Apr 12, 2022 at 02:21:08PM +0300, Andy Shevchenko wrote: >> On Tue, Apr 12, 2022 at 10:07:02AM +0200, Javier Martinez Canillas wrote: >>> On 4/12/22 09:23, Geert Uytterhoeven wrote: >>>> On Mon, Apr 11, 2022 at 11:12 PM Javier Martinez Canillas >>>> <javierm@redhat.com> wrote: > > ... > >>>>> - ssd130x->device_info = device_get_match_data(dev); >>>>> + >>>>> + variant = (enum ssd130x_variants)device_get_match_data(dev); >>>> >>>> (uintptr_t), to avoid a cast from pointer to integer of different >>>> size warning. >>>> >>> >>> Indeed. The kernel test robot reported the same. >> >> Not only because of this, but also with the non-NULL pointers I prefer the old >> style without ugly castings. >> >> Instead, you may export the array (in the driver's namespace) and use >> &info[ID] pointer for the specific device info. > > Note that device_get_match_data() has no clue if the data is absent or > data == (void *)0. > Yep, we could make the enum start at 1 and check for !variant but that's something that will also be prevented by your suggestion to just use the &info[ID] instead.
diff --git a/drivers/gpu/drm/solomon/ssd130x-i2c.c b/drivers/gpu/drm/solomon/ssd130x-i2c.c index 87abe1fe31fc..8259b10db966 100644 --- a/drivers/gpu/drm/solomon/ssd130x-i2c.c +++ b/drivers/gpu/drm/solomon/ssd130x-i2c.c @@ -53,80 +53,47 @@ static void ssd130x_i2c_shutdown(struct i2c_client *client) ssd130x_shutdown(ssd130x); } -static struct ssd130x_deviceinfo ssd130x_sh1106_deviceinfo = { - .default_vcomh = 0x40, - .default_dclk_div = 1, - .default_dclk_frq = 5, - .page_mode_only = 1, -}; - -static struct ssd130x_deviceinfo ssd130x_ssd1305_deviceinfo = { - .default_vcomh = 0x34, - .default_dclk_div = 1, - .default_dclk_frq = 7, -}; - -static struct ssd130x_deviceinfo ssd130x_ssd1306_deviceinfo = { - .default_vcomh = 0x20, - .default_dclk_div = 1, - .default_dclk_frq = 8, - .need_chargepump = 1, -}; - -static struct ssd130x_deviceinfo ssd130x_ssd1307_deviceinfo = { - .default_vcomh = 0x20, - .default_dclk_div = 2, - .default_dclk_frq = 12, - .need_pwm = 1, -}; - -static struct ssd130x_deviceinfo ssd130x_ssd1309_deviceinfo = { - .default_vcomh = 0x34, - .default_dclk_div = 1, - .default_dclk_frq = 10, -}; - static const struct of_device_id ssd130x_of_match[] = { { .compatible = "sinowealth,sh1106", - .data = &ssd130x_sh1106_deviceinfo, + .data = (void *)SH1106_ID, }, { .compatible = "solomon,ssd1305", - .data = &ssd130x_ssd1305_deviceinfo, + .data = (void *)SSD1305_ID, }, { .compatible = "solomon,ssd1306", - .data = &ssd130x_ssd1306_deviceinfo, + .data = (void *)SSD1306_ID, }, { .compatible = "solomon,ssd1307", - .data = &ssd130x_ssd1307_deviceinfo, + .data = (void *)SSD1307_ID, }, { .compatible = "solomon,ssd1309", - .data = &ssd130x_ssd1309_deviceinfo, + .data = (void *)SSD1309_ID, }, /* Deprecated but kept for backward compatibility */ { .compatible = "sinowealth,sh1106-i2c", - .data = &ssd130x_sh1106_deviceinfo, + .data = (void *)SH1106_ID, }, { .compatible = "solomon,ssd1305fb-i2c", - .data = &ssd130x_ssd1305_deviceinfo, + .data = (void *)SSD1305_ID, }, { .compatible = "solomon,ssd1306fb-i2c", - .data = &ssd130x_ssd1306_deviceinfo, + .data = (void *)SSD1306_ID, }, { .compatible = "solomon,ssd1307fb-i2c", - .data = &ssd130x_ssd1307_deviceinfo, + .data = (void *)SSD1307_ID, }, { .compatible = "solomon,ssd1309fb-i2c", - .data = &ssd130x_ssd1309_deviceinfo, + .data = (void *)SSD1309_ID, }, { /* sentinel */ } }; diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c index a7e784518c69..7d5b43023213 100644 --- a/drivers/gpu/drm/solomon/ssd130x.c +++ b/drivers/gpu/drm/solomon/ssd130x.c @@ -39,11 +39,9 @@ #define DRIVER_MAJOR 1 #define DRIVER_MINOR 0 -#define SSD130X_DATA 0x40 -#define SSD130X_COMMAND 0x80 - #define SSD130X_PAGE_COL_START_LOW 0x00 #define SSD130X_PAGE_COL_START_HIGH 0x10 + #define SSD130X_SET_ADDRESS_MODE 0x20 #define SSD130X_SET_COL_RANGE 0x21 #define SSD130X_SET_PAGE_RANGE 0x22 @@ -94,6 +92,37 @@ #define MAX_CONTRAST 255 +static const struct ssd130x_deviceinfo ssd130x_variants[] = { + [SH1106_ID] = { + .default_vcomh = 0x40, + .default_dclk_div = 1, + .default_dclk_frq = 5, + .page_mode_only = 1, + }, + [SSD1305_ID] = { + .default_vcomh = 0x34, + .default_dclk_div = 1, + .default_dclk_frq = 7, + }, + [SSD1306_ID] = { + .default_vcomh = 0x20, + .default_dclk_div = 1, + .default_dclk_frq = 8, + .need_chargepump = 1, + }, + [SSD1307_ID] = { + .default_vcomh = 0x20, + .default_dclk_div = 2, + .default_dclk_frq = 12, + .need_pwm = 1, + }, + [SSD1309_ID] = { + .default_vcomh = 0x34, + .default_dclk_div = 1, + .default_dclk_frq = 10, + } +}; + static inline struct ssd130x_device *drm_to_ssd130x(struct drm_device *drm) { return container_of(drm, struct ssd130x_device, drm); @@ -846,6 +875,7 @@ static int ssd130x_get_resources(struct ssd130x_device *ssd130x) struct ssd130x_device *ssd130x_probe(struct device *dev, struct regmap *regmap) { struct ssd130x_device *ssd130x; + enum ssd130x_variants variant; struct backlight_device *bl; struct drm_device *drm; int ret; @@ -860,7 +890,14 @@ struct ssd130x_device *ssd130x_probe(struct device *dev, struct regmap *regmap) ssd130x->dev = dev; ssd130x->regmap = regmap; - ssd130x->device_info = device_get_match_data(dev); + + variant = (enum ssd130x_variants)device_get_match_data(dev); + + if (variant >= NR_SSD130X_VARIANTS) + return ERR_PTR(dev_err_probe(dev, -EINVAL, + "Invalid SSD130x variant\n")); + + ssd130x->device_info = &ssd130x_variants[variant]; if (ssd130x->device_info->page_mode_only) ssd130x->page_address_mode = 1; diff --git a/drivers/gpu/drm/solomon/ssd130x.h b/drivers/gpu/drm/solomon/ssd130x.h index f5b062576fdf..ec349239e945 100644 --- a/drivers/gpu/drm/solomon/ssd130x.h +++ b/drivers/gpu/drm/solomon/ssd130x.h @@ -18,6 +18,18 @@ #include <linux/regmap.h> +#define SSD130X_DATA 0x40 +#define SSD130X_COMMAND 0x80 + +enum ssd130x_variants { + SH1106_ID, + SSD1305_ID, + SSD1306_ID, + SSD1307_ID, + SSD1309_ID, + NR_SSD130X_VARIANTS +}; + struct ssd130x_deviceinfo { u32 default_vcomh; u32 default_dclk_div;