| Message ID | cb7e427fd680b3619d5c1d819f4cc6f56a539959.1606986778.git.michal.simek@xilinx.com |
|---|---|
| State | Superseded |
| Delegated to: | Anatolij Gustschin |
| Headers | show |
| Series | video: seps525: Add new driver for seps525 OLED display | expand |
Hi Michal, On Thu, 3 Dec 2020 at 02:13, Michal Simek <michal.simek@xilinx.com> wrote: > > Some drivers like LCD connected via SPI requires explicit sync function > which copy framebuffer content over SPI to controller to display. > This hook doesn't exist yet that's why introduce it via video operations. > > Signed-off-by: Michal Simek <michal.simek@xilinx.com> > --- > > Simon: Please review this. I didn't find existing way how this can be done > that's why I am introducing this hook. Also maybe name can be named a > little bit differently. That's why waiting for better suggestion. > --- > drivers/video/video-uclass.c | 5 +++++ > include/video.h | 13 +++++++++++++ > 2 files changed, 18 insertions(+) > diff --git a/drivers/video/video-uclass.c b/drivers/video/video-uclass.c > index 650891e49dd0..ba52a6c7125b 100644 > --- a/drivers/video/video-uclass.c > +++ b/drivers/video/video-uclass.c > @@ -174,6 +174,11 @@ void video_set_default_colors(struct udevice *dev, bool invert) > /* Flush video activity to the caches */ > void video_sync(struct udevice *vid, bool force) > { > + struct video_ops *ops = video_get_ops(vid); > + > + if (ops && ops->video_sync) > + (void)ops->video_sync(vid); We should update video_sync() to return an error. > + > /* > * flush_dcache_range() is declared in common.h but it seems that some > * architectures do not actually implement it. Is there a way to find > diff --git a/include/video.h b/include/video.h > index 9d09d2409af6..acac3f6b3c8d 100644 > --- a/include/video.h > +++ b/include/video.h > @@ -115,7 +115,20 @@ struct video_priv { > }; > > /* Placeholder - there are no video operations at present */ Need to update comment > +/** > + * struct video_ops - structure for keeping video operations > + */ > struct video_ops { > + /** > + * video_sync - Synchronize FB with device > + * > + * Some device like SPI based LCD displays needs synchronization when > + * data in an FB is available. For these devices implement video_sync > + * hook to call a sync function > + * > + * @vid: Video device udevice structure @return > + */ > + int (*video_sync)(struct udevice *vid); > }; > > #define video_get_ops(dev) ((struct video_ops *)(dev)->driver->ops) > -- > 2.29.2 > Regards, Simon
Hi Simon, On 12. 12. 20 16:35, Simon Glass wrote: > Hi Michal, > > On Thu, 3 Dec 2020 at 02:13, Michal Simek <michal.simek@xilinx.com> wrote: >> >> Some drivers like LCD connected via SPI requires explicit sync function >> which copy framebuffer content over SPI to controller to display. >> This hook doesn't exist yet that's why introduce it via video operations. >> >> Signed-off-by: Michal Simek <michal.simek@xilinx.com> >> --- >> >> Simon: Please review this. I didn't find existing way how this can be done >> that's why I am introducing this hook. Also maybe name can be named a >> little bit differently. That's why waiting for better suggestion. >> --- >> drivers/video/video-uclass.c | 5 +++++ >> include/video.h | 13 +++++++++++++ >> 2 files changed, 18 insertions(+) > >> diff --git a/drivers/video/video-uclass.c b/drivers/video/video-uclass.c >> index 650891e49dd0..ba52a6c7125b 100644 >> --- a/drivers/video/video-uclass.c >> +++ b/drivers/video/video-uclass.c >> @@ -174,6 +174,11 @@ void video_set_default_colors(struct udevice *dev, bool invert) >> /* Flush video activity to the caches */ >> void video_sync(struct udevice *vid, bool force) >> { >> + struct video_ops *ops = video_get_ops(vid); >> + >> + if (ops && ops->video_sync) >> + (void)ops->video_sync(vid); > > We should update video_sync() to return an error. I sent v2 with fixing this. > >> + >> /* >> * flush_dcache_range() is declared in common.h but it seems that some >> * architectures do not actually implement it. Is there a way to find >> diff --git a/include/video.h b/include/video.h >> index 9d09d2409af6..acac3f6b3c8d 100644 >> --- a/include/video.h >> +++ b/include/video.h >> @@ -115,7 +115,20 @@ struct video_priv { >> }; >> >> /* Placeholder - there are no video operations at present */ > > Need to update comment Removed. > >> +/** >> + * struct video_ops - structure for keeping video operations >> + */ >> struct video_ops { >> + /** >> + * video_sync - Synchronize FB with device >> + * >> + * Some device like SPI based LCD displays needs synchronization when >> + * data in an FB is available. For these devices implement video_sync >> + * hook to call a sync function >> + * >> + * @vid: Video device udevice structure > > @return I have looked at kernel-doc format and completely redo this to pass kernel-doc script. There are a lot of issues reported by that script. Would be good to run it by patman automatically to be more visible for everybody. Thanks, Michal
Hi Michal, On Mon, 14 Dec 2020 at 01:39, Michal Simek <michal.simek@xilinx.com> wrote: > > Hi Simon, > > On 12. 12. 20 16:35, Simon Glass wrote: > > Hi Michal, > > > > On Thu, 3 Dec 2020 at 02:13, Michal Simek <michal.simek@xilinx.com> wrote: > >> > >> Some drivers like LCD connected via SPI requires explicit sync function > >> which copy framebuffer content over SPI to controller to display. > >> This hook doesn't exist yet that's why introduce it via video operations. > >> > >> Signed-off-by: Michal Simek <michal.simek@xilinx.com> > >> --- > >> > >> Simon: Please review this. I didn't find existing way how this can be done > >> that's why I am introducing this hook. Also maybe name can be named a > >> little bit differently. That's why waiting for better suggestion. > >> --- > >> drivers/video/video-uclass.c | 5 +++++ > >> include/video.h | 13 +++++++++++++ > >> 2 files changed, 18 insertions(+) > > > >> diff --git a/drivers/video/video-uclass.c b/drivers/video/video-uclass.c > >> index 650891e49dd0..ba52a6c7125b 100644 > >> --- a/drivers/video/video-uclass.c > >> +++ b/drivers/video/video-uclass.c > >> @@ -174,6 +174,11 @@ void video_set_default_colors(struct udevice *dev, bool invert) > >> /* Flush video activity to the caches */ > >> void video_sync(struct udevice *vid, bool force) > >> { > >> + struct video_ops *ops = video_get_ops(vid); > >> + > >> + if (ops && ops->video_sync) > >> + (void)ops->video_sync(vid); > > > > We should update video_sync() to return an error. > > I sent v2 with fixing this. > > > > >> + > >> /* > >> * flush_dcache_range() is declared in common.h but it seems that some > >> * architectures do not actually implement it. Is there a way to find > >> diff --git a/include/video.h b/include/video.h > >> index 9d09d2409af6..acac3f6b3c8d 100644 > >> --- a/include/video.h > >> +++ b/include/video.h > >> @@ -115,7 +115,20 @@ struct video_priv { > >> }; > >> > >> /* Placeholder - there are no video operations at present */ > > > > Need to update comment > > Removed. > > > > >> +/** > >> + * struct video_ops - structure for keeping video operations > >> + */ > >> struct video_ops { > >> + /** > >> + * video_sync - Synchronize FB with device > >> + * > >> + * Some device like SPI based LCD displays needs synchronization when > >> + * data in an FB is available. For these devices implement video_sync > >> + * hook to call a sync function > >> + * > >> + * @vid: Video device udevice structure > > > > @return > > I have looked at kernel-doc format and completely redo this to pass > kernel-doc script. > There are a lot of issues reported by that script. Would be good to run > it by patman automatically to be more visible for everybody. Yes that would be nice. Regards, Simon
diff --git a/drivers/video/video-uclass.c b/drivers/video/video-uclass.c index 650891e49dd0..ba52a6c7125b 100644 --- a/drivers/video/video-uclass.c +++ b/drivers/video/video-uclass.c @@ -174,6 +174,11 @@ void video_set_default_colors(struct udevice *dev, bool invert) /* Flush video activity to the caches */ void video_sync(struct udevice *vid, bool force) { + struct video_ops *ops = video_get_ops(vid); + + if (ops && ops->video_sync) + (void)ops->video_sync(vid); + /* * flush_dcache_range() is declared in common.h but it seems that some * architectures do not actually implement it. Is there a way to find diff --git a/include/video.h b/include/video.h index 9d09d2409af6..acac3f6b3c8d 100644 --- a/include/video.h +++ b/include/video.h @@ -115,7 +115,20 @@ struct video_priv { }; /* Placeholder - there are no video operations at present */ +/** + * struct video_ops - structure for keeping video operations + */ struct video_ops { + /** + * video_sync - Synchronize FB with device + * + * Some device like SPI based LCD displays needs synchronization when + * data in an FB is available. For these devices implement video_sync + * hook to call a sync function + * + * @vid: Video device udevice structure + */ + int (*video_sync)(struct udevice *vid); }; #define video_get_ops(dev) ((struct video_ops *)(dev)->driver->ops)
Some drivers like LCD connected via SPI requires explicit sync function which copy framebuffer content over SPI to controller to display. This hook doesn't exist yet that's why introduce it via video operations. Signed-off-by: Michal Simek <michal.simek@xilinx.com> --- Simon: Please review this. I didn't find existing way how this can be done that's why I am introducing this hook. Also maybe name can be named a little bit differently. That's why waiting for better suggestion. --- drivers/video/video-uclass.c | 5 +++++ include/video.h | 13 +++++++++++++ 2 files changed, 18 insertions(+)