| Message ID | 20200122105403.30035-1-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v1,1/4] drm/tiny/repaper: Make driver OF-independent | expand |
Hi Andy. On Wed, Jan 22, 2020 at 12:54:00PM +0200, Andy Shevchenko wrote: > There is one OF call in the driver that limits its area of use. > Replace it to generic device_get_match_data() and get rid of OF dependency. > > While here, cast SPI driver data to certain enumerator type. > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > drivers/gpu/drm/tiny/repaper.c | 11 ++++++----- > 1 file changed, 6 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/tiny/repaper.c b/drivers/gpu/drm/tiny/repaper.c > index 76d179200775..fd9e95ce3201 100644 > --- a/drivers/gpu/drm/tiny/repaper.c > +++ b/drivers/gpu/drm/tiny/repaper.c > @@ -17,7 +17,7 @@ > #include <linux/dma-buf.h> > #include <linux/gpio/consumer.h> > #include <linux/module.h> > -#include <linux/of_device.h> > +#include <linux/property.h> > #include <linux/sched/clock.h> > #include <linux/spi/spi.h> > #include <linux/thermal.h> > @@ -40,6 +40,7 @@ > #define REPAPER_RID_G2_COG_ID 0x12 > > enum repaper_model { > + EXXXXCSXXX = 0, > E1144CS021 = 1, > E1190CS021, > E2200CS021, The new enum value is not used in the following - is it necessary? Sam > @@ -995,21 +996,21 @@ static int repaper_probe(struct spi_device *spi) > { > const struct drm_display_mode *mode; > const struct spi_device_id *spi_id; > - const struct of_device_id *match; > struct device *dev = &spi->dev; > enum repaper_model model; > const char *thermal_zone; > struct repaper_epd *epd; > size_t line_buffer_size; > struct drm_device *drm; > + const void *match; > int ret; > > - match = of_match_device(repaper_of_match, dev); > + match = device_get_match_data(dev); > if (match) { > - model = (enum repaper_model)match->data; > + model = (enum repaper_model)match; > } else { > spi_id = spi_get_device_id(spi); > - model = spi_id->driver_data; > + model = (enum repaper_model)spi_id->driver_data; > } > > /* The SPI device is used to allocate dma memory */ > -- > 2.24.1 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Fri, Jan 24, 2020 at 05:42:33PM +0100, Sam Ravnborg wrote: > On Wed, Jan 22, 2020 at 12:54:00PM +0200, Andy Shevchenko wrote: > > There is one OF call in the driver that limits its area of use. > > Replace it to generic device_get_match_data() and get rid of OF dependency. > > > > While here, cast SPI driver data to certain enumerator type. > > enum repaper_model { > > + EXXXXCSXXX = 0, > > E1144CS021 = 1, > > E1190CS021, > > E2200CS021, > The new enum value is not used in the following - is it necessary? Yes. It explicitly prevents to use 0 for real device. This is due to device_get_match_data() returns content of data pointer and thus we may not distinguish 0 from NULL pointer.
Hi Andy. On Fri, Jan 24, 2020 at 07:31:34PM +0200, Andy Shevchenko wrote: > On Fri, Jan 24, 2020 at 05:42:33PM +0100, Sam Ravnborg wrote: > > On Wed, Jan 22, 2020 at 12:54:00PM +0200, Andy Shevchenko wrote: > > > There is one OF call in the driver that limits its area of use. > > > Replace it to generic device_get_match_data() and get rid of OF dependency. > > > > > > While here, cast SPI driver data to certain enumerator type. > > > > enum repaper_model { > > > + EXXXXCSXXX = 0, > > > E1144CS021 = 1, > > > E1190CS021, > > > E2200CS021, > > The new enum value is not used in the following - is it necessary? > > Yes. It explicitly prevents to use 0 for real device. > > This is due to device_get_match_data() returns content of data pointer and thus > we may not distinguish 0 from NULL pointer. A name that told this was not a valid name would be descriptive. As it is now it looks like a wildcard that matches everythign else. With a more descriptive name: Reviewed-by: Sam Ravnborg <sam@ravnborg.org> Sam
On Fri, Jan 24, 2020 at 07:18:12PM +0100, Sam Ravnborg wrote: > On Fri, Jan 24, 2020 at 07:31:34PM +0200, Andy Shevchenko wrote: > > On Fri, Jan 24, 2020 at 05:42:33PM +0100, Sam Ravnborg wrote: > > > On Wed, Jan 22, 2020 at 12:54:00PM +0200, Andy Shevchenko wrote: > > > > There is one OF call in the driver that limits its area of use. > > > > Replace it to generic device_get_match_data() and get rid of OF dependency. > > > > > > > > While here, cast SPI driver data to certain enumerator type. > > > > > > enum repaper_model { > > > > + EXXXXCSXXX = 0, > > > > E1144CS021 = 1, > > > > E1190CS021, > > > > E2200CS021, > > > The new enum value is not used in the following - is it necessary? > > > > Yes. It explicitly prevents to use 0 for real device. > > > > This is due to device_get_match_data() returns content of data pointer and thus > > we may not distinguish 0 from NULL pointer. > A name that told this was not a valid name would be descriptive. > As it is now it looks like a wildcard that matches everythign else. Can you be more precise what you would like to see? Perhaps simple comment will help? > With a more descriptive name: > Reviewed-by: Sam Ravnborg <sam@ravnborg.org>
Hi Andy. On Mon, Jan 27, 2020 at 11:39:39AM +0200, Andy Shevchenko wrote: > On Fri, Jan 24, 2020 at 07:18:12PM +0100, Sam Ravnborg wrote: > > On Fri, Jan 24, 2020 at 07:31:34PM +0200, Andy Shevchenko wrote: > > > On Fri, Jan 24, 2020 at 05:42:33PM +0100, Sam Ravnborg wrote: > > > > On Wed, Jan 22, 2020 at 12:54:00PM +0200, Andy Shevchenko wrote: > > > > > There is one OF call in the driver that limits its area of use. > > > > > Replace it to generic device_get_match_data() and get rid of OF dependency. > > > > > > > > > > While here, cast SPI driver data to certain enumerator type. > > > > > > > > enum repaper_model { > > > > > + EXXXXCSXXX = 0, > > > > > E1144CS021 = 1, > > > > > E1190CS021, > > > > > E2200CS021, > > > > The new enum value is not used in the following - is it necessary? > > > > > > Yes. It explicitly prevents to use 0 for real device. > > > > > > This is due to device_get_match_data() returns content of data pointer and thus > > > we may not distinguish 0 from NULL pointer. > > A name that told this was not a valid name would be descriptive. > > As it is now it looks like a wildcard that matches everythign else. > > Can you be more precise what you would like to see? > Perhaps simple comment will help? Maybe just add something like: /* 0 is reserved to avoid clashing with NULL */ And then skip the, at least to my eyes, cryptic EXXXXCSXXX. Up to you. Sam
On Wed, Jan 29, 2020 at 09:29:14PM +0100, Sam Ravnborg wrote: > On Mon, Jan 27, 2020 at 11:39:39AM +0200, Andy Shevchenko wrote: > > On Fri, Jan 24, 2020 at 07:18:12PM +0100, Sam Ravnborg wrote: > > > On Fri, Jan 24, 2020 at 07:31:34PM +0200, Andy Shevchenko wrote: > > > > On Fri, Jan 24, 2020 at 05:42:33PM +0100, Sam Ravnborg wrote: > > > > > On Wed, Jan 22, 2020 at 12:54:00PM +0200, Andy Shevchenko wrote: > > > > > > There is one OF call in the driver that limits its area of use. > > > > > > Replace it to generic device_get_match_data() and get rid of OF dependency. > > > > > > > > > > > > While here, cast SPI driver data to certain enumerator type. > > > > > > > > > > enum repaper_model { > > > > > > + EXXXXCSXXX = 0, > > > > > > E1144CS021 = 1, > > > > > > E1190CS021, > > > > > > E2200CS021, > > > > > The new enum value is not used in the following - is it necessary? > > > > > > > > Yes. It explicitly prevents to use 0 for real device. > > > > > > > > This is due to device_get_match_data() returns content of data pointer and thus > > > > we may not distinguish 0 from NULL pointer. > > > A name that told this was not a valid name would be descriptive. > > > As it is now it looks like a wildcard that matches everythign else. > > > > Can you be more precise what you would like to see? > > Perhaps simple comment will help? > > Maybe just add something like: > /* 0 is reserved to avoid clashing with NULL */ > > And then skip the, at least to my eyes, cryptic EXXXXCSXXX. > Up to you. Fine with me, I'll update accordingly. Thanks!
diff --git a/drivers/gpu/drm/tiny/repaper.c b/drivers/gpu/drm/tiny/repaper.c index 76d179200775..fd9e95ce3201 100644 --- a/drivers/gpu/drm/tiny/repaper.c +++ b/drivers/gpu/drm/tiny/repaper.c @@ -17,7 +17,7 @@ #include <linux/dma-buf.h> #include <linux/gpio/consumer.h> #include <linux/module.h> -#include <linux/of_device.h> +#include <linux/property.h> #include <linux/sched/clock.h> #include <linux/spi/spi.h> #include <linux/thermal.h> @@ -40,6 +40,7 @@ #define REPAPER_RID_G2_COG_ID 0x12 enum repaper_model { + EXXXXCSXXX = 0, E1144CS021 = 1, E1190CS021, E2200CS021, @@ -995,21 +996,21 @@ static int repaper_probe(struct spi_device *spi) { const struct drm_display_mode *mode; const struct spi_device_id *spi_id; - const struct of_device_id *match; struct device *dev = &spi->dev; enum repaper_model model; const char *thermal_zone; struct repaper_epd *epd; size_t line_buffer_size; struct drm_device *drm; + const void *match; int ret; - match = of_match_device(repaper_of_match, dev); + match = device_get_match_data(dev); if (match) { - model = (enum repaper_model)match->data; + model = (enum repaper_model)match; } else { spi_id = spi_get_device_id(spi); - model = spi_id->driver_data; + model = (enum repaper_model)spi_id->driver_data; } /* The SPI device is used to allocate dma memory */
There is one OF call in the driver that limits its area of use. Replace it to generic device_get_match_data() and get rid of OF dependency. While here, cast SPI driver data to certain enumerator type. Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- drivers/gpu/drm/tiny/repaper.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)