| Message ID | 20200703184546.144664-6-sam@ravnborg.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | backlight: backlight updates | expand |
On Fri, Jul 03, 2020 at 08:45:31PM +0200, Sam Ravnborg wrote: > Improve the documentation for backlight_device and > adapt it to kernel-doc style. > > The updated documentation is more strict on how locking is used. > With the update neither update_lock nor ops_lock may be used > outside the backlight core. > This restriction was introduced to keep the locking simple > by keeping it in the core. > It was verified that this documents the current state by renaming > update_lock => bl_update_lock and ops_lock => bl_ops_lock. > The rename did not reveal any uses outside the backlight core. > The rename is NOT part of this patch. > > v3: > - Update changelog to explain locking details (Daniel) > > v2: > - Add short intro to all fields (Daniel) > - Updated description of update_lock (Daniel) > > Signed-off-by: Sam Ravnborg <sam@ravnborg.org> > Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com> > Cc: Lee Jones <lee.jones@linaro.org> > Cc: Daniel Thompson <daniel.thompson@linaro.org> > Cc: Jingoo Han <jingoohan1@gmail.com> Reviewed-by: Daniel Thompson <daniel.thompson@linaro.org> Daniel.
On Fri, 03 Jul 2020, Sam Ravnborg wrote: > Improve the documentation for backlight_device and > adapt it to kernel-doc style. > > The updated documentation is more strict on how locking is used. > With the update neither update_lock nor ops_lock may be used > outside the backlight core. > This restriction was introduced to keep the locking simple > by keeping it in the core. > It was verified that this documents the current state by renaming > update_lock => bl_update_lock and ops_lock => bl_ops_lock. > The rename did not reveal any uses outside the backlight core. > The rename is NOT part of this patch. > > v3: > - Update changelog to explain locking details (Daniel) > > v2: > - Add short intro to all fields (Daniel) > - Updated description of update_lock (Daniel) > > Signed-off-by: Sam Ravnborg <sam@ravnborg.org> > Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com> > Cc: Lee Jones <lee.jones@linaro.org> > Cc: Daniel Thompson <daniel.thompson@linaro.org> > Cc: Jingoo Han <jingoohan1@gmail.com> > --- > include/linux/backlight.h | 72 ++++++++++++++++++++++++++------------- > 1 file changed, 49 insertions(+), 23 deletions(-) Some of these do not apply cleanly. Please collect the *-bys already received, rebase and resubmit.
Hi Lee. On Thu, Jul 16, 2020 at 03:39:41PM +0100, Lee Jones wrote: > On Fri, 03 Jul 2020, Sam Ravnborg wrote: > > > Improve the documentation for backlight_device and > > adapt it to kernel-doc style. > > > > The updated documentation is more strict on how locking is used. > > With the update neither update_lock nor ops_lock may be used > > outside the backlight core. > > This restriction was introduced to keep the locking simple > > by keeping it in the core. > > It was verified that this documents the current state by renaming > > update_lock => bl_update_lock and ops_lock => bl_ops_lock. > > The rename did not reveal any uses outside the backlight core. > > The rename is NOT part of this patch. > > > > v3: > > - Update changelog to explain locking details (Daniel) > > > > v2: > > - Add short intro to all fields (Daniel) > > - Updated description of update_lock (Daniel) > > > > Signed-off-by: Sam Ravnborg <sam@ravnborg.org> > > Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com> > > Cc: Lee Jones <lee.jones@linaro.org> > > Cc: Daniel Thompson <daniel.thompson@linaro.org> > > Cc: Jingoo Han <jingoohan1@gmail.com> > > --- > > include/linux/backlight.h | 72 ++++++++++++++++++++++++++------------- > > 1 file changed, 49 insertions(+), 23 deletions(-) > > Some of these do not apply cleanly. > > Please collect the *-bys already received, rebase and resubmit. Will do. The patch-set is based on drm-misc-next. Are there another tree that I should use? Sam
On Thu, 16 Jul 2020, Sam Ravnborg wrote: > Hi Lee. > > On Thu, Jul 16, 2020 at 03:39:41PM +0100, Lee Jones wrote: > > On Fri, 03 Jul 2020, Sam Ravnborg wrote: > > > > > Improve the documentation for backlight_device and > > > adapt it to kernel-doc style. > > > > > > The updated documentation is more strict on how locking is used. > > > With the update neither update_lock nor ops_lock may be used > > > outside the backlight core. > > > This restriction was introduced to keep the locking simple > > > by keeping it in the core. > > > It was verified that this documents the current state by renaming > > > update_lock => bl_update_lock and ops_lock => bl_ops_lock. > > > The rename did not reveal any uses outside the backlight core. > > > The rename is NOT part of this patch. > > > > > > v3: > > > - Update changelog to explain locking details (Daniel) > > > > > > v2: > > > - Add short intro to all fields (Daniel) > > > - Updated description of update_lock (Daniel) > > > > > > Signed-off-by: Sam Ravnborg <sam@ravnborg.org> > > > Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com> > > > Cc: Lee Jones <lee.jones@linaro.org> > > > Cc: Daniel Thompson <daniel.thompson@linaro.org> > > > Cc: Jingoo Han <jingoohan1@gmail.com> > > > --- > > > include/linux/backlight.h | 72 ++++++++++++++++++++++++++------------- > > > 1 file changed, 49 insertions(+), 23 deletions(-) > > > > Some of these do not apply cleanly. > > > > Please collect the *-bys already received, rebase and resubmit. > > Will do. > The patch-set is based on drm-misc-next. Are there another > tree that I should use? I don't have anything to do with that tree. Either Backlight [0] or Next would be fine. [0] https://git.kernel.org/pub/scm/linux/kernel/git/lee/backlight.git/
On 7/3/20, 2:46 PM, Sam Ravnborg wrote: > > Improve the documentation for backlight_device and > adapt it to kernel-doc style. > > The updated documentation is more strict on how locking is used. > With the update neither update_lock nor ops_lock may be used > outside the backlight core. > This restriction was introduced to keep the locking simple > by keeping it in the core. > It was verified that this documents the current state by renaming > update_lock => bl_update_lock and ops_lock => bl_ops_lock. > The rename did not reveal any uses outside the backlight core. > The rename is NOT part of this patch. > > v3: > - Update changelog to explain locking details (Daniel) > > v2: > - Add short intro to all fields (Daniel) > - Updated description of update_lock (Daniel) > > Signed-off-by: Sam Ravnborg <sam@ravnborg.org> > Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com> > Cc: Lee Jones <lee.jones@linaro.org> > Cc: Daniel Thompson <daniel.thompson@linaro.org> > Cc: Jingoo Han <jingoohan1@gmail.com> It looks good! Reviewed-by: Jingoo Han <jingoohan1@gmail.com> For the rebase, if you don't know which branch of maintainer's git can be used, linux-next tree [1] is useful. The linux-next git collects all next branches from other maintainers' git every day. [1] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/ Thank you. Best regards, Jingoo Han > --- > include/linux/backlight.h | 72 ++++++++++++++++++++++++++------------- > 1 file changed, 49 insertions(+), 23 deletions(-) .....
Hi Jingoo On Sat, Jul 18, 2020 at 05:18:39AM +0000, Jingoo Han wrote: > On 7/3/20, 2:46 PM, Sam Ravnborg wrote: > > > > Improve the documentation for backlight_device and > > adapt it to kernel-doc style. > > > > The updated documentation is more strict on how locking is used. > > With the update neither update_lock nor ops_lock may be used > > outside the backlight core. > > This restriction was introduced to keep the locking simple > > by keeping it in the core. > > It was verified that this documents the current state by renaming > > update_lock => bl_update_lock and ops_lock => bl_ops_lock. > > The rename did not reveal any uses outside the backlight core. > > The rename is NOT part of this patch. > > > > v3: > > - Update changelog to explain locking details (Daniel) > > > > v2: > > - Add short intro to all fields (Daniel) > > - Updated description of update_lock (Daniel) > > > > Signed-off-by: Sam Ravnborg <sam@ravnborg.org> > > Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com> > > Cc: Lee Jones <lee.jones@linaro.org> > > Cc: Daniel Thompson <daniel.thompson@linaro.org> > > Cc: Jingoo Han <jingoohan1@gmail.com> > > It looks good! > Reviewed-by: Jingoo Han <jingoohan1@gmail.com> Thanks! > > For the rebase, if you don't know which branch of maintainer's git can be used, > linux-next tree [1] is useful. The linux-next git collects all next branches from > other maintainers' git every day. > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/ I had used drm-misc-next because the original focus was to clean up drivers in gpu/drm/ - and then I just continued to use this wrong tree. linux-next is indeed a good place to catch the latest and greatest - but as I now have the URL for the backlight tree (thanks to Lee) I will use it here. Will try to find time this weekend so we can land these. Sam
diff --git a/include/linux/backlight.h b/include/linux/backlight.h index 10518b00b059..7654fe5f1589 100644 --- a/include/linux/backlight.h +++ b/include/linux/backlight.h @@ -14,21 +14,6 @@ #include <linux/mutex.h> #include <linux/notifier.h> -/* Notes on locking: - * - * backlight_device->ops_lock is an internal backlight lock protecting the - * ops pointer and no code outside the core should need to touch it. - * - * Access to update_status() is serialised by the update_lock mutex since - * most drivers seem to need this and historically get it wrong. - * - * Most drivers don't need locking on their get_brightness() method. - * If yours does, you need to implement it in the driver. You can use the - * update_lock mutex if appropriate. - * - * Any other use of the locks below is probably wrong. - */ - enum backlight_update_reason { BACKLIGHT_UPDATE_HOTKEY, BACKLIGHT_UPDATE_SYSFS, @@ -215,30 +200,71 @@ struct backlight_properties { enum backlight_scale scale; }; +/** + * struct backlight_device - backlight device data + * + * This structure holds all data required by a backlight device. + */ struct backlight_device { - /* Backlight properties */ + /** + * @props: Backlight properties + */ struct backlight_properties props; - /* Serialise access to update_status method */ + /** + * @update_lock: The lock used when calling the update_status() operation. + * + * update_lock is an internal backlight lock that serialise access + * to the update_status() operation. The backlight core holds the update_lock + * when calling the update_status() operation. The update_lock shall not + * be used by backlight drivers. + */ struct mutex update_lock; - /* This protects the 'ops' field. If 'ops' is NULL, the driver that - registered this device has been unloaded, and if class_get_devdata() - points to something in the body of that driver, it is also invalid. */ + /** + * @ops_lock: The lock used around everything related to backlight_ops. + * + * ops_lock is an internal backlight lock that protects the ops pointer + * and is used around all accesses to ops and when the operations are + * invoked. The ops_lock shall not be used by backlight drivers. + */ struct mutex ops_lock; + + /** + * @ops: Pointer to the backlight operations. + * + * If ops is NULL, the driver that registered this device has been unloaded, + * and if class_get_devdata() points to something in the body of that driver, + * it is also invalid. + */ const struct backlight_ops *ops; - /* The framebuffer notifier block */ + /** + * @fb_notif: The framebuffer notifier block + */ struct notifier_block fb_notif; - /* list entry of all registered backlight devices */ + /** + * @entry: List entry of all registered backlight devices + */ struct list_head entry; + /** + * @dev: Parent device. + */ struct device dev; - /* Multiple framebuffers may share one backlight device */ + /** + * @fb_bl_on: The state of individual fbdev's. + * + * Multiple fbdev's may share one backlight device. The fb_bl_on + * records the state of the individual fbdev. + */ bool fb_bl_on[FB_MAX]; + /** + * @use_count: The number of uses of fb_bl_on. + */ int use_count; };