| Message ID | 20220621061532.3108909-1-yunkec@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] media: uvcvideo: user entity get_cur in uvc_ctrl_set | expand |
Hi Yunke
Thanks for your patch
Another way to do it could be:
__uvc_ctrl_load_cur() {
if (loaded)
return
if ((ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR) {
memset
loaded = true;
return
}
...
}
int __uvc_ctrl_get() {
if ((ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR) == 0)
return -EACCES;
....
}
Then you could even simplify more the code in uvc_ctrl_set, and limit
the m handling of the loaded flag.
if ( (ctrl->info.size * 8) != mapping->size)
___uvc_ctrl_load_cur()
But It is probably just a matter of taste ;). It is up to you and
Laurent to pick what do you prefer.
Regards!
On Tue, 21 Jun 2022 at 08:15, Yunke Cao <yunkec@google.com> wrote:
>
> Entity controls should get_cur using an entity-defined function
> instead of via a query. Fix this in uvc_ctrl_set.
>
> Fixes: 65900c581d01 ("media: uvcvideo: Allow entity-defined get_info and get_cur")
> Signed-off-by: Yunke Cao <yunkec@google.com>
Reviewed-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
> Changelog since v1:
> -Factored out common logic into __uvc_ctrl_load_cur().
>
> drivers/media/usb/uvc/uvc_ctrl.c | 62 ++++++++++++++++++--------------
> 1 file changed, 35 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> index 0e78233fc8a0..e25177c95571 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -963,36 +963,48 @@ static s32 __uvc_ctrl_get_value(struct uvc_control_mapping *mapping,
> return value;
> }
>
> -static int __uvc_ctrl_get(struct uvc_video_chain *chain,
> - struct uvc_control *ctrl, struct uvc_control_mapping *mapping,
> - s32 *value)
> +static int __uvc_ctrl_load_cur(struct uvc_video_chain *chain,
> + struct uvc_control *ctrl)
> {
> - int ret;
> + int ret = 0;
nit : why do you init to 0?
>
> if ((ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR) == 0)
> return -EACCES;
>
> - if (!ctrl->loaded) {
> - if (ctrl->entity->get_cur) {
nit: is this spaced properly?
> - ret = ctrl->entity->get_cur(chain->dev,
> - ctrl->entity,
> - ctrl->info.selector,
> - uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT),
> - ctrl->info.size);
> - } else {
> - ret = uvc_query_ctrl(chain->dev, UVC_GET_CUR,
> - ctrl->entity->id,
> - chain->dev->intfnum,
> - ctrl->info.selector,
> - uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT),
> - ctrl->info.size);
> - }
> - if (ret < 0)
> - return ret;
> + if (ctrl->loaded)
> + return 0;
>
> - ctrl->loaded = 1;
> + if (ctrl->entity->get_cur) {
> + ret = ctrl->entity->get_cur(chain->dev,
> + ctrl->entity,
> + ctrl->info.selector,
> + uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT),
> + ctrl->info.size);
> + } else {
> + ret = uvc_query_ctrl(chain->dev, UVC_GET_CUR,
> + ctrl->entity->id, chain->dev->intfnum,
> + ctrl->info.selector,
> + uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT),
> + ctrl->info.size);
> }
>
> + if (ret < 0)
> + return ret;
> +
> + ctrl->loaded = 1;
> +
> + return ret;
> +}
> +
> +static int __uvc_ctrl_get(struct uvc_video_chain *chain,
> + struct uvc_control *ctrl, struct uvc_control_mapping *mapping,
> + s32 *value)
> +{
nit: Is the alignment correct here?
> + int ret = __uvc_ctrl_load_cur(chain, ctrl);
> +
> + if (ret < 0)
> + return ret;
> +
> *value = __uvc_ctrl_get_value(mapping,
> uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT));
>
> @@ -1788,11 +1800,7 @@ int uvc_ctrl_set(struct uvc_fh *handle,
> memset(uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT),
> 0, ctrl->info.size);
> } else {
> - ret = uvc_query_ctrl(chain->dev, UVC_GET_CUR,
> - ctrl->entity->id, chain->dev->intfnum,
> - ctrl->info.selector,
> - uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT),
> - ctrl->info.size);
> + ret = __uvc_ctrl_load_cur(chain, ctrl);
> if (ret < 0)
> return ret;
> }
> --
> 2.37.0.rc0.104.g0611611a94-goog
Thanks Ricardo for the review! On Wed, Jun 22, 2022 at 4:58 AM Ricardo Ribalda <ribalda@chromium.org> wrote: > > Hi Yunke > > Thanks for your patch > > Another way to do it could be: > > __uvc_ctrl_load_cur() { > if (loaded) > return > if ((ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR) { > memset > loaded = true; > return > } > ... > } > > int __uvc_ctrl_get() { > if ((ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR) == 0) > return -EACCES; > .... > } > > Then you could even simplify more the code in uvc_ctrl_set, and limit > the m handling of the loaded flag. > > > if ( (ctrl->info.size * 8) != mapping->size) > ___uvc_ctrl_load_cur() > > > But It is probably just a matter of taste ;). It is up to you and > Laurent to pick what do you prefer. > > Regards! Thanks for the suggestion. This does look cleaner. I like your idea better. Laurent, do you have any preference? > > > On Tue, 21 Jun 2022 at 08:15, Yunke Cao <yunkec@google.com> wrote: > > > > Entity controls should get_cur using an entity-defined function > > instead of via a query. Fix this in uvc_ctrl_set. > > > > Fixes: 65900c581d01 ("media: uvcvideo: Allow entity-defined get_info and get_cur") > > Signed-off-by: Yunke Cao <yunkec@google.com> > Reviewed-by: Ricardo Ribalda <ribalda@chromium.org> > > --- > > Changelog since v1: > > -Factored out common logic into __uvc_ctrl_load_cur(). > > > > drivers/media/usb/uvc/uvc_ctrl.c | 62 ++++++++++++++++++-------------- > > 1 file changed, 35 insertions(+), 27 deletions(-) > > > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c > > index 0e78233fc8a0..e25177c95571 100644 > > --- a/drivers/media/usb/uvc/uvc_ctrl.c > > +++ b/drivers/media/usb/uvc/uvc_ctrl.c > > @@ -963,36 +963,48 @@ static s32 __uvc_ctrl_get_value(struct uvc_control_mapping *mapping, > > return value; > > } > > > > -static int __uvc_ctrl_get(struct uvc_video_chain *chain, > > - struct uvc_control *ctrl, struct uvc_control_mapping *mapping, > > - s32 *value) > > +static int __uvc_ctrl_load_cur(struct uvc_video_chain *chain, > > + struct uvc_control *ctrl) > > { > > - int ret; > > + int ret = 0; > nit : why do you init to 0? I will remove it in v3. > > > > if ((ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR) == 0) > > return -EACCES; > > > > - if (!ctrl->loaded) { > > - if (ctrl->entity->get_cur) { > nit: is this spaced properly? This is deleted code or am I looking in the wrong place. > > - ret = ctrl->entity->get_cur(chain->dev, > > - ctrl->entity, > > - ctrl->info.selector, > > - uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT), > > - ctrl->info.size); > > - } else { > > - ret = uvc_query_ctrl(chain->dev, UVC_GET_CUR, > > - ctrl->entity->id, > > - chain->dev->intfnum, > > - ctrl->info.selector, > > - uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT), > > - ctrl->info.size); > > - } > > - if (ret < 0) > > - return ret; > > + if (ctrl->loaded) > > + return 0; > > > > - ctrl->loaded = 1; > > + if (ctrl->entity->get_cur) { > > + ret = ctrl->entity->get_cur(chain->dev, > > + ctrl->entity, > > + ctrl->info.selector, > > + uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT), > > + ctrl->info.size); > > + } else { > > + ret = uvc_query_ctrl(chain->dev, UVC_GET_CUR, > > + ctrl->entity->id, chain->dev->intfnum, > > + ctrl->info.selector, > > + uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT), > > + ctrl->info.size); > > } > > > > + if (ret < 0) > > + return ret; > > + > > + ctrl->loaded = 1; > > + > > + return ret; > > +} > > + > > +static int __uvc_ctrl_get(struct uvc_video_chain *chain, > > + struct uvc_control *ctrl, struct uvc_control_mapping *mapping, > > + s32 *value) > > +{ > nit: Is the alignment correct here? Will fix it in v3. Best, Yunke > > + int ret = __uvc_ctrl_load_cur(chain, ctrl); > > + > > + if (ret < 0) > > + return ret; > > + > > *value = __uvc_ctrl_get_value(mapping, > > uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT)); > > > > @@ -1788,11 +1800,7 @@ int uvc_ctrl_set(struct uvc_fh *handle, > > memset(uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT), > > 0, ctrl->info.size); > > } else { > > - ret = uvc_query_ctrl(chain->dev, UVC_GET_CUR, > > - ctrl->entity->id, chain->dev->intfnum, > > - ctrl->info.selector, > > - uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT), > > - ctrl->info.size); > > + ret = __uvc_ctrl_load_cur(chain, ctrl); > > if (ret < 0) > > return ret; > > } > > -- > > 2.37.0.rc0.104.g0611611a94-goog > > > > -- > Ricardo Ribalda
On Mon, 27 Jun 2022 at 03:08, Yunke Cao <yunkec@google.com> wrote: > > Thanks Ricardo for the review! > > On Wed, Jun 22, 2022 at 4:58 AM Ricardo Ribalda <ribalda@chromium.org> wrote: > > > > Hi Yunke > > > > Thanks for your patch > > > > Another way to do it could be: > > > > __uvc_ctrl_load_cur() { > > if (loaded) > > return > > if ((ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR) { > > memset > > loaded = true; > > return > > } > > ... > > } > > > > int __uvc_ctrl_get() { > > if ((ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR) == 0) > > return -EACCES; > > .... > > } > > > > Then you could even simplify more the code in uvc_ctrl_set, and limit > > the m handling of the loaded flag. > > > > > > if ( (ctrl->info.size * 8) != mapping->size) > > ___uvc_ctrl_load_cur() > > > > > > But It is probably just a matter of taste ;). It is up to you and > > Laurent to pick what do you prefer. > > > > Regards! > > Thanks for the suggestion. This does look cleaner. I like your idea better. > Laurent, do you have any preference? > > > > > > > On Tue, 21 Jun 2022 at 08:15, Yunke Cao <yunkec@google.com> wrote: > > > > > > Entity controls should get_cur using an entity-defined function > > > instead of via a query. Fix this in uvc_ctrl_set. > > > > > > Fixes: 65900c581d01 ("media: uvcvideo: Allow entity-defined get_info and get_cur") > > > Signed-off-by: Yunke Cao <yunkec@google.com> > > Reviewed-by: Ricardo Ribalda <ribalda@chromium.org> > > > --- > > > Changelog since v1: > > > -Factored out common logic into __uvc_ctrl_load_cur(). > > > > > > drivers/media/usb/uvc/uvc_ctrl.c | 62 ++++++++++++++++++-------------- > > > 1 file changed, 35 insertions(+), 27 deletions(-) > > > > > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c > > > index 0e78233fc8a0..e25177c95571 100644 > > > --- a/drivers/media/usb/uvc/uvc_ctrl.c > > > +++ b/drivers/media/usb/uvc/uvc_ctrl.c > > > @@ -963,36 +963,48 @@ static s32 __uvc_ctrl_get_value(struct uvc_control_mapping *mapping, > > > return value; > > > } > > > > > > -static int __uvc_ctrl_get(struct uvc_video_chain *chain, > > > - struct uvc_control *ctrl, struct uvc_control_mapping *mapping, > > > - s32 *value) > > > +static int __uvc_ctrl_load_cur(struct uvc_video_chain *chain, > > > + struct uvc_control *ctrl) > > > { > > > - int ret; > > > + int ret = 0; > > nit : why do you init to 0? > > I will remove it in v3. > > > > > > > if ((ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR) == 0) > > > return -EACCES; > > > > > > - if (!ctrl->loaded) { > > > - if (ctrl->entity->get_cur) { > > nit: is this spaced properly? > > This is deleted code or am I looking in the wrong place. I probably screwed up, sorry ;( > > > > - ret = ctrl->entity->get_cur(chain->dev, > > > - ctrl->entity, > > > - ctrl->info.selector, > > > - uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT), > > > - ctrl->info.size); > > > - } else { > > > - ret = uvc_query_ctrl(chain->dev, UVC_GET_CUR, > > > - ctrl->entity->id, > > > - chain->dev->intfnum, > > > - ctrl->info.selector, > > > - uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT), > > > - ctrl->info.size); > > > - } > > > - if (ret < 0) > > > - return ret; > > > + if (ctrl->loaded) > > > + return 0; > > > > > > - ctrl->loaded = 1; > > > + if (ctrl->entity->get_cur) { > > > + ret = ctrl->entity->get_cur(chain->dev, > > > + ctrl->entity, > > > + ctrl->info.selector, > > > + uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT), > > > + ctrl->info.size); > > > + } else { > > > + ret = uvc_query_ctrl(chain->dev, UVC_GET_CUR, > > > + ctrl->entity->id, chain->dev->intfnum, > > > + ctrl->info.selector, > > > + uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT), > > > + ctrl->info.size); > > > } > > > > > > + if (ret < 0) > > > + return ret; > > > + > > > + ctrl->loaded = 1; > > > + > > > + return ret; > > > +} > > > + > > > +static int __uvc_ctrl_get(struct uvc_video_chain *chain, > > > + struct uvc_control *ctrl, struct uvc_control_mapping *mapping, > > > + s32 *value) > > > +{ > > nit: Is the alignment correct here? > > Will fix it in v3. > > Best, > Yunke > > > > + int ret = __uvc_ctrl_load_cur(chain, ctrl); > > > + > > > + if (ret < 0) > > > + return ret; > > > + > > > *value = __uvc_ctrl_get_value(mapping, > > > uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT)); > > > > > > @@ -1788,11 +1800,7 @@ int uvc_ctrl_set(struct uvc_fh *handle, > > > memset(uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT), > > > 0, ctrl->info.size); > > > } else { > > > - ret = uvc_query_ctrl(chain->dev, UVC_GET_CUR, > > > - ctrl->entity->id, chain->dev->intfnum, > > > - ctrl->info.selector, > > > - uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT), > > > - ctrl->info.size); > > > + ret = __uvc_ctrl_load_cur(chain, ctrl); > > > if (ret < 0) > > > return ret; > > > } > > > -- > > > 2.37.0.rc0.104.g0611611a94-goog > > > > > > > > -- > > Ricardo Ribalda
On Mon, Jun 27, 2022 at 10:08:14AM +0900, Yunke Cao wrote: > Thanks Ricardo for the review! > > On Wed, Jun 22, 2022 at 4:58 AM Ricardo Ribalda <ribalda@chromium.org> wrote: > > > > Hi Yunke > > > > Thanks for your patch > > > > Another way to do it could be: > > > > __uvc_ctrl_load_cur() { > > if (loaded) > > return > > if ((ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR) { > > memset > > loaded = true; > > return > > } > > ... > > } > > > > int __uvc_ctrl_get() { > > if ((ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR) == 0) > > return -EACCES; > > .... > > } > > > > Then you could even simplify more the code in uvc_ctrl_set, and limit > > the m handling of the loaded flag. > > > > > > if ( (ctrl->info.size * 8) != mapping->size) > > ___uvc_ctrl_load_cur() > > > > > > But It is probably just a matter of taste ;). It is up to you and > > Laurent to pick what do you prefer. > > > > Regards! > > Thanks for the suggestion. This does look cleaner. I like your idea better. > Laurent, do you have any preference? I agree, it looks cleaner. Do I understand correctly that this patch will currently have no effect, given that only the fake GPIO entity has a .get_cur() function, and that entity doesn't expose any control that can be set ? > > On Tue, 21 Jun 2022 at 08:15, Yunke Cao <yunkec@google.com> wrote: > > > > > > Entity controls should get_cur using an entity-defined function > > > instead of via a query. Fix this in uvc_ctrl_set. > > > > > > Fixes: 65900c581d01 ("media: uvcvideo: Allow entity-defined get_info and get_cur") > > > Signed-off-by: Yunke Cao <yunkec@google.com> > > Reviewed-by: Ricardo Ribalda <ribalda@chromium.org> > > > --- > > > Changelog since v1: > > > -Factored out common logic into __uvc_ctrl_load_cur(). > > > > > > drivers/media/usb/uvc/uvc_ctrl.c | 62 ++++++++++++++++++-------------- > > > 1 file changed, 35 insertions(+), 27 deletions(-) > > > > > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c > > > index 0e78233fc8a0..e25177c95571 100644 > > > --- a/drivers/media/usb/uvc/uvc_ctrl.c > > > +++ b/drivers/media/usb/uvc/uvc_ctrl.c > > > @@ -963,36 +963,48 @@ static s32 __uvc_ctrl_get_value(struct uvc_control_mapping *mapping, > > > return value; > > > } > > > > > > -static int __uvc_ctrl_get(struct uvc_video_chain *chain, > > > - struct uvc_control *ctrl, struct uvc_control_mapping *mapping, > > > - s32 *value) > > > +static int __uvc_ctrl_load_cur(struct uvc_video_chain *chain, > > > + struct uvc_control *ctrl) > > > { > > > - int ret; > > > + int ret = 0; > > nit : why do you init to 0? > > I will remove it in v3. > > > > > > > if ((ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR) == 0) > > > return -EACCES; > > > > > > - if (!ctrl->loaded) { > > > - if (ctrl->entity->get_cur) { > > nit: is this spaced properly? > > This is deleted code or am I looking in the wrong place. > > > > - ret = ctrl->entity->get_cur(chain->dev, > > > - ctrl->entity, > > > - ctrl->info.selector, > > > - uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT), > > > - ctrl->info.size); > > > - } else { > > > - ret = uvc_query_ctrl(chain->dev, UVC_GET_CUR, > > > - ctrl->entity->id, > > > - chain->dev->intfnum, > > > - ctrl->info.selector, > > > - uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT), > > > - ctrl->info.size); > > > - } > > > - if (ret < 0) > > > - return ret; > > > + if (ctrl->loaded) > > > + return 0; > > > > > > - ctrl->loaded = 1; > > > + if (ctrl->entity->get_cur) { > > > + ret = ctrl->entity->get_cur(chain->dev, > > > + ctrl->entity, > > > + ctrl->info.selector, > > > + uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT), > > > + ctrl->info.size); > > > + } else { > > > + ret = uvc_query_ctrl(chain->dev, UVC_GET_CUR, > > > + ctrl->entity->id, chain->dev->intfnum, > > > + ctrl->info.selector, > > > + uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT), > > > + ctrl->info.size); > > > } > > > > > > + if (ret < 0) > > > + return ret; > > > + > > > + ctrl->loaded = 1; > > > + > > > + return ret; > > > +} > > > + > > > +static int __uvc_ctrl_get(struct uvc_video_chain *chain, > > > + struct uvc_control *ctrl, struct uvc_control_mapping *mapping, > > > + s32 *value) > > > +{ > > nit: Is the alignment correct here? > > Will fix it in v3. > > Best, > Yunke > > > > + int ret = __uvc_ctrl_load_cur(chain, ctrl); > > > + > > > + if (ret < 0) > > > + return ret; > > > + > > > *value = __uvc_ctrl_get_value(mapping, > > > uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT)); > > > > > > @@ -1788,11 +1800,7 @@ int uvc_ctrl_set(struct uvc_fh *handle, > > > memset(uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT), > > > 0, ctrl->info.size); > > > } else { > > > - ret = uvc_query_ctrl(chain->dev, UVC_GET_CUR, > > > - ctrl->entity->id, chain->dev->intfnum, > > > - ctrl->info.selector, > > > - uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT), > > > - ctrl->info.size); > > > + ret = __uvc_ctrl_load_cur(chain, ctrl); > > > if (ret < 0) > > > return ret; > > > }
Hi Laurent, Thanks for the feedback. On Thu, Jul 7, 2022 at 8:39 AM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > On Mon, Jun 27, 2022 at 10:08:14AM +0900, Yunke Cao wrote: > > Thanks Ricardo for the review! > > > > On Wed, Jun 22, 2022 at 4:58 AM Ricardo Ribalda <ribalda@chromium.org> wrote: > > > > > > Hi Yunke > > > > > > Thanks for your patch > > > > > > Another way to do it could be: > > > > > > __uvc_ctrl_load_cur() { > > > if (loaded) > > > return > > > if ((ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR) { > > > memset > > > loaded = true; > > > return > > > } > > > ... > > > } > > > > > > int __uvc_ctrl_get() { > > > if ((ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR) == 0) > > > return -EACCES; > > > .... > > > } > > > > > > Then you could even simplify more the code in uvc_ctrl_set, and limit > > > the m handling of the loaded flag. > > > > > > > > > if ( (ctrl->info.size * 8) != mapping->size) > > > ___uvc_ctrl_load_cur() > > > > > > > > > But It is probably just a matter of taste ;). It is up to you and > > > Laurent to pick what do you prefer. > > > > > > Regards! > > > > Thanks for the suggestion. This does look cleaner. I like your idea better. > > Laurent, do you have any preference? > > I agree, it looks cleaner. > > Do I understand correctly that this patch will currently have no effect, > given that only the fake GPIO entity has a .get_cur() function, and that > entity doesn't expose any control that can be set ? > Yes, That's my understanding as well. Ricardo, please correct me if I'm wrong. Do we still want to merge this? If so, I will send v3 shortly. Best, Yunke > > > On Tue, 21 Jun 2022 at 08:15, Yunke Cao <yunkec@google.com> wrote: > > > > > > > > Entity controls should get_cur using an entity-defined function > > > > instead of via a query. Fix this in uvc_ctrl_set. > > > > > > > > Fixes: 65900c581d01 ("media: uvcvideo: Allow entity-defined get_info and get_cur") > > > > Signed-off-by: Yunke Cao <yunkec@google.com> > > > Reviewed-by: Ricardo Ribalda <ribalda@chromium.org> > > > > --- > > > > Changelog since v1: > > > > -Factored out common logic into __uvc_ctrl_load_cur(). > > > > > > > > drivers/media/usb/uvc/uvc_ctrl.c | 62 ++++++++++++++++++-------------- > > > > 1 file changed, 35 insertions(+), 27 deletions(-) > > > > > > > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c > > > > index 0e78233fc8a0..e25177c95571 100644 > > > > --- a/drivers/media/usb/uvc/uvc_ctrl.c > > > > +++ b/drivers/media/usb/uvc/uvc_ctrl.c > > > > @@ -963,36 +963,48 @@ static s32 __uvc_ctrl_get_value(struct uvc_control_mapping *mapping, > > > > return value; > > > > } > > > > > > > > -static int __uvc_ctrl_get(struct uvc_video_chain *chain, > > > > - struct uvc_control *ctrl, struct uvc_control_mapping *mapping, > > > > - s32 *value) > > > > +static int __uvc_ctrl_load_cur(struct uvc_video_chain *chain, > > > > + struct uvc_control *ctrl) > > > > { > > > > - int ret; > > > > + int ret = 0; > > > nit : why do you init to 0? > > > > I will remove it in v3. > > > > > > > > > > if ((ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR) == 0) > > > > return -EACCES; > > > > > > > > - if (!ctrl->loaded) { > > > > - if (ctrl->entity->get_cur) { > > > nit: is this spaced properly? > > > > This is deleted code or am I looking in the wrong place. > > > > > > - ret = ctrl->entity->get_cur(chain->dev, > > > > - ctrl->entity, > > > > - ctrl->info.selector, > > > > - uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT), > > > > - ctrl->info.size); > > > > - } else { > > > > - ret = uvc_query_ctrl(chain->dev, UVC_GET_CUR, > > > > - ctrl->entity->id, > > > > - chain->dev->intfnum, > > > > - ctrl->info.selector, > > > > - uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT), > > > > - ctrl->info.size); > > > > - } > > > > - if (ret < 0) > > > > - return ret; > > > > + if (ctrl->loaded) > > > > + return 0; > > > > > > > > - ctrl->loaded = 1; > > > > + if (ctrl->entity->get_cur) { > > > > + ret = ctrl->entity->get_cur(chain->dev, > > > > + ctrl->entity, > > > > + ctrl->info.selector, > > > > + uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT), > > > > + ctrl->info.size); > > > > + } else { > > > > + ret = uvc_query_ctrl(chain->dev, UVC_GET_CUR, > > > > + ctrl->entity->id, chain->dev->intfnum, > > > > + ctrl->info.selector, > > > > + uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT), > > > > + ctrl->info.size); > > > > } > > > > > > > > + if (ret < 0) > > > > + return ret; > > > > + > > > > + ctrl->loaded = 1; > > > > + > > > > + return ret; > > > > +} > > > > + > > > > +static int __uvc_ctrl_get(struct uvc_video_chain *chain, > > > > + struct uvc_control *ctrl, struct uvc_control_mapping *mapping, > > > > + s32 *value) > > > > +{ > > > nit: Is the alignment correct here? > > > > Will fix it in v3. > > > > Best, > > Yunke > > > > > > + int ret = __uvc_ctrl_load_cur(chain, ctrl); > > > > + > > > > + if (ret < 0) > > > > + return ret; > > > > + > > > > *value = __uvc_ctrl_get_value(mapping, > > > > uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT)); > > > > > > > > @@ -1788,11 +1800,7 @@ int uvc_ctrl_set(struct uvc_fh *handle, > > > > memset(uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT), > > > > 0, ctrl->info.size); > > > > } else { > > > > - ret = uvc_query_ctrl(chain->dev, UVC_GET_CUR, > > > > - ctrl->entity->id, chain->dev->intfnum, > > > > - ctrl->info.selector, > > > > - uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT), > > > > - ctrl->info.size); > > > > + ret = __uvc_ctrl_load_cur(chain, ctrl); > > > > if (ret < 0) > > > > return ret; > > > > } > > -- > Regards, > > Laurent Pinchart
Hi Yunke, On Thu, Jul 07, 2022 at 01:54:52PM +0900, Yunke Cao wrote: > On Thu, Jul 7, 2022 at 8:39 AM Laurent Pinchart wrote: > > On Mon, Jun 27, 2022 at 10:08:14AM +0900, Yunke Cao wrote: > > > On Wed, Jun 22, 2022 at 4:58 AM Ricardo Ribalda wrote: > > > > > > > > Hi Yunke > > > > > > > > Thanks for your patch > > > > > > > > Another way to do it could be: > > > > > > > > __uvc_ctrl_load_cur() { > > > > if (loaded) > > > > return > > > > if ((ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR) { > > > > memset > > > > loaded = true; > > > > return > > > > } > > > > ... > > > > } > > > > > > > > int __uvc_ctrl_get() { > > > > if ((ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR) == 0) > > > > return -EACCES; > > > > .... > > > > } > > > > > > > > Then you could even simplify more the code in uvc_ctrl_set, and limit > > > > the m handling of the loaded flag. > > > > > > > > > > > > if ( (ctrl->info.size * 8) != mapping->size) > > > > ___uvc_ctrl_load_cur() > > > > > > > > > > > > But It is probably just a matter of taste ;). It is up to you and > > > > Laurent to pick what do you prefer. > > > > > > > > Regards! > > > > > > Thanks for the suggestion. This does look cleaner. I like your idea better. > > > Laurent, do you have any preference? > > > > I agree, it looks cleaner. > > > > Do I understand correctly that this patch will currently have no effect, > > given that only the fake GPIO entity has a .get_cur() function, and that > > entity doesn't expose any control that can be set ? > > Yes, That's my understanding as well. Ricardo, please correct me if I'm wrong. > Do we still want to merge this? If so, I will send v3 shortly. It's cleaner and will prevent future problems in case more users of .get_cur() appear, so I think the patch has value. > > > > On Tue, 21 Jun 2022 at 08:15, Yunke Cao <yunkec@google.com> wrote: > > > > > > > > > > Entity controls should get_cur using an entity-defined function > > > > > instead of via a query. Fix this in uvc_ctrl_set. > > > > > > > > > > Fixes: 65900c581d01 ("media: uvcvideo: Allow entity-defined get_info and get_cur") > > > > > Signed-off-by: Yunke Cao <yunkec@google.com> > > > > Reviewed-by: Ricardo Ribalda <ribalda@chromium.org> > > > > > --- > > > > > Changelog since v1: > > > > > -Factored out common logic into __uvc_ctrl_load_cur(). > > > > > > > > > > drivers/media/usb/uvc/uvc_ctrl.c | 62 ++++++++++++++++++-------------- > > > > > 1 file changed, 35 insertions(+), 27 deletions(-) > > > > > > > > > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c > > > > > index 0e78233fc8a0..e25177c95571 100644 > > > > > --- a/drivers/media/usb/uvc/uvc_ctrl.c > > > > > +++ b/drivers/media/usb/uvc/uvc_ctrl.c > > > > > @@ -963,36 +963,48 @@ static s32 __uvc_ctrl_get_value(struct uvc_control_mapping *mapping, > > > > > return value; > > > > > } > > > > > > > > > > -static int __uvc_ctrl_get(struct uvc_video_chain *chain, > > > > > - struct uvc_control *ctrl, struct uvc_control_mapping *mapping, > > > > > - s32 *value) > > > > > +static int __uvc_ctrl_load_cur(struct uvc_video_chain *chain, > > > > > + struct uvc_control *ctrl) > > > > > { > > > > > - int ret; > > > > > + int ret = 0; > > > > nit : why do you init to 0? > > > > > > I will remove it in v3. > > > > > > > > > > > > > if ((ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR) == 0) > > > > > return -EACCES; > > > > > > > > > > - if (!ctrl->loaded) { > > > > > - if (ctrl->entity->get_cur) { > > > > nit: is this spaced properly? > > > > > > This is deleted code or am I looking in the wrong place. > > > > > > > > - ret = ctrl->entity->get_cur(chain->dev, > > > > > - ctrl->entity, > > > > > - ctrl->info.selector, > > > > > - uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT), > > > > > - ctrl->info.size); > > > > > - } else { > > > > > - ret = uvc_query_ctrl(chain->dev, UVC_GET_CUR, > > > > > - ctrl->entity->id, > > > > > - chain->dev->intfnum, > > > > > - ctrl->info.selector, > > > > > - uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT), > > > > > - ctrl->info.size); > > > > > - } > > > > > - if (ret < 0) > > > > > - return ret; > > > > > + if (ctrl->loaded) > > > > > + return 0; > > > > > > > > > > - ctrl->loaded = 1; > > > > > + if (ctrl->entity->get_cur) { > > > > > + ret = ctrl->entity->get_cur(chain->dev, > > > > > + ctrl->entity, > > > > > + ctrl->info.selector, > > > > > + uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT), > > > > > + ctrl->info.size); > > > > > + } else { > > > > > + ret = uvc_query_ctrl(chain->dev, UVC_GET_CUR, > > > > > + ctrl->entity->id, chain->dev->intfnum, > > > > > + ctrl->info.selector, > > > > > + uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT), > > > > > + ctrl->info.size); > > > > > } > > > > > > > > > > + if (ret < 0) > > > > > + return ret; > > > > > + > > > > > + ctrl->loaded = 1; > > > > > + > > > > > + return ret; > > > > > +} > > > > > + > > > > > +static int __uvc_ctrl_get(struct uvc_video_chain *chain, > > > > > + struct uvc_control *ctrl, struct uvc_control_mapping *mapping, > > > > > + s32 *value) > > > > > +{ > > > > nit: Is the alignment correct here? > > > > > > Will fix it in v3. > > > > > > > > + int ret = __uvc_ctrl_load_cur(chain, ctrl); > > > > > + > > > > > + if (ret < 0) > > > > > + return ret; > > > > > + > > > > > *value = __uvc_ctrl_get_value(mapping, > > > > > uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT)); > > > > > > > > > > @@ -1788,11 +1800,7 @@ int uvc_ctrl_set(struct uvc_fh *handle, > > > > > memset(uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT), > > > > > 0, ctrl->info.size); > > > > > } else { > > > > > - ret = uvc_query_ctrl(chain->dev, UVC_GET_CUR, > > > > > - ctrl->entity->id, chain->dev->intfnum, > > > > > - ctrl->info.selector, > > > > > - uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT), > > > > > - ctrl->info.size); > > > > > + ret = __uvc_ctrl_load_cur(chain, ctrl); > > > > > if (ret < 0) > > > > > return ret; > > > > > }
Thanks Laurent! Just sent out v3: Changelog since v2: -Move the logic of setting ctrl_data to 0 into load_cur. -Do not initialize ret=0. -Fix __uvc_ctrl_get() spacing. -Fix typo in the title. Best, Yunke On Thu, Jul 7, 2022 at 5:12 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Yunke, > > On Thu, Jul 07, 2022 at 01:54:52PM +0900, Yunke Cao wrote: > > On Thu, Jul 7, 2022 at 8:39 AM Laurent Pinchart wrote: > > > On Mon, Jun 27, 2022 at 10:08:14AM +0900, Yunke Cao wrote: > > > > On Wed, Jun 22, 2022 at 4:58 AM Ricardo Ribalda wrote: > > > > > > > > > > Hi Yunke > > > > > > > > > > Thanks for your patch > > > > > > > > > > Another way to do it could be: > > > > > > > > > > __uvc_ctrl_load_cur() { > > > > > if (loaded) > > > > > return > > > > > if ((ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR) { > > > > > memset > > > > > loaded = true; > > > > > return > > > > > } > > > > > ... > > > > > } > > > > > > > > > > int __uvc_ctrl_get() { > > > > > if ((ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR) == 0) > > > > > return -EACCES; > > > > > .... > > > > > } > > > > > > > > > > Then you could even simplify more the code in uvc_ctrl_set, and limit > > > > > the m handling of the loaded flag. > > > > > > > > > > > > > > > if ( (ctrl->info.size * 8) != mapping->size) > > > > > ___uvc_ctrl_load_cur() > > > > > > > > > > > > > > > But It is probably just a matter of taste ;). It is up to you and > > > > > Laurent to pick what do you prefer. > > > > > > > > > > Regards! > > > > > > > > Thanks for the suggestion. This does look cleaner. I like your idea better. > > > > Laurent, do you have any preference? > > > > > > I agree, it looks cleaner. > > > > > > Do I understand correctly that this patch will currently have no effect, > > > given that only the fake GPIO entity has a .get_cur() function, and that > > > entity doesn't expose any control that can be set ? > > > > Yes, That's my understanding as well. Ricardo, please correct me if I'm wrong. > > Do we still want to merge this? If so, I will send v3 shortly. > > It's cleaner and will prevent future problems in case more users of > .get_cur() appear, so I think the patch has value. > > > > > > On Tue, 21 Jun 2022 at 08:15, Yunke Cao <yunkec@google.com> wrote: > > > > > > > > > > > > Entity controls should get_cur using an entity-defined function > > > > > > instead of via a query. Fix this in uvc_ctrl_set. > > > > > > > > > > > > Fixes: 65900c581d01 ("media: uvcvideo: Allow entity-defined get_info and get_cur") > > > > > > Signed-off-by: Yunke Cao <yunkec@google.com> > > > > > Reviewed-by: Ricardo Ribalda <ribalda@chromium.org> > > > > > > --- > > > > > > Changelog since v1: > > > > > > -Factored out common logic into __uvc_ctrl_load_cur(). > > > > > > > > > > > > drivers/media/usb/uvc/uvc_ctrl.c | 62 ++++++++++++++++++-------------- > > > > > > 1 file changed, 35 insertions(+), 27 deletions(-) > > > > > > > > > > > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c > > > > > > index 0e78233fc8a0..e25177c95571 100644 > > > > > > --- a/drivers/media/usb/uvc/uvc_ctrl.c > > > > > > +++ b/drivers/media/usb/uvc/uvc_ctrl.c > > > > > > @@ -963,36 +963,48 @@ static s32 __uvc_ctrl_get_value(struct uvc_control_mapping *mapping, > > > > > > return value; > > > > > > } > > > > > > > > > > > > -static int __uvc_ctrl_get(struct uvc_video_chain *chain, > > > > > > - struct uvc_control *ctrl, struct uvc_control_mapping *mapping, > > > > > > - s32 *value) > > > > > > +static int __uvc_ctrl_load_cur(struct uvc_video_chain *chain, > > > > > > + struct uvc_control *ctrl) > > > > > > { > > > > > > - int ret; > > > > > > + int ret = 0; > > > > > nit : why do you init to 0? > > > > > > > > I will remove it in v3. > > > > > > > > > > > > > > > > if ((ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR) == 0) > > > > > > return -EACCES; > > > > > > > > > > > > - if (!ctrl->loaded) { > > > > > > - if (ctrl->entity->get_cur) { > > > > > nit: is this spaced properly? > > > > > > > > This is deleted code or am I looking in the wrong place. > > > > > > > > > > - ret = ctrl->entity->get_cur(chain->dev, > > > > > > - ctrl->entity, > > > > > > - ctrl->info.selector, > > > > > > - uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT), > > > > > > - ctrl->info.size); > > > > > > - } else { > > > > > > - ret = uvc_query_ctrl(chain->dev, UVC_GET_CUR, > > > > > > - ctrl->entity->id, > > > > > > - chain->dev->intfnum, > > > > > > - ctrl->info.selector, > > > > > > - uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT), > > > > > > - ctrl->info.size); > > > > > > - } > > > > > > - if (ret < 0) > > > > > > - return ret; > > > > > > + if (ctrl->loaded) > > > > > > + return 0; > > > > > > > > > > > > - ctrl->loaded = 1; > > > > > > + if (ctrl->entity->get_cur) { > > > > > > + ret = ctrl->entity->get_cur(chain->dev, > > > > > > + ctrl->entity, > > > > > > + ctrl->info.selector, > > > > > > + uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT), > > > > > > + ctrl->info.size); > > > > > > + } else { > > > > > > + ret = uvc_query_ctrl(chain->dev, UVC_GET_CUR, > > > > > > + ctrl->entity->id, chain->dev->intfnum, > > > > > > + ctrl->info.selector, > > > > > > + uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT), > > > > > > + ctrl->info.size); > > > > > > } > > > > > > > > > > > > + if (ret < 0) > > > > > > + return ret; > > > > > > + > > > > > > + ctrl->loaded = 1; > > > > > > + > > > > > > + return ret; > > > > > > +} > > > > > > + > > > > > > +static int __uvc_ctrl_get(struct uvc_video_chain *chain, > > > > > > + struct uvc_control *ctrl, struct uvc_control_mapping *mapping, > > > > > > + s32 *value) > > > > > > +{ > > > > > nit: Is the alignment correct here? > > > > > > > > Will fix it in v3. > > > > > > > > > > + int ret = __uvc_ctrl_load_cur(chain, ctrl); > > > > > > + > > > > > > + if (ret < 0) > > > > > > + return ret; > > > > > > + > > > > > > *value = __uvc_ctrl_get_value(mapping, > > > > > > uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT)); > > > > > > > > > > > > @@ -1788,11 +1800,7 @@ int uvc_ctrl_set(struct uvc_fh *handle, > > > > > > memset(uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT), > > > > > > 0, ctrl->info.size); > > > > > > } else { > > > > > > - ret = uvc_query_ctrl(chain->dev, UVC_GET_CUR, > > > > > > - ctrl->entity->id, chain->dev->intfnum, > > > > > > - ctrl->info.selector, > > > > > > - uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT), > > > > > > - ctrl->info.size); > > > > > > + ret = __uvc_ctrl_load_cur(chain, ctrl); > > > > > > if (ret < 0) > > > > > > return ret; > > > > > > } > > -- > Regards, > > Laurent Pinchart
diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c index 0e78233fc8a0..e25177c95571 100644 --- a/drivers/media/usb/uvc/uvc_ctrl.c +++ b/drivers/media/usb/uvc/uvc_ctrl.c @@ -963,36 +963,48 @@ static s32 __uvc_ctrl_get_value(struct uvc_control_mapping *mapping, return value; } -static int __uvc_ctrl_get(struct uvc_video_chain *chain, - struct uvc_control *ctrl, struct uvc_control_mapping *mapping, - s32 *value) +static int __uvc_ctrl_load_cur(struct uvc_video_chain *chain, + struct uvc_control *ctrl) { - int ret; + int ret = 0; if ((ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR) == 0) return -EACCES; - if (!ctrl->loaded) { - if (ctrl->entity->get_cur) { - ret = ctrl->entity->get_cur(chain->dev, - ctrl->entity, - ctrl->info.selector, - uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT), - ctrl->info.size); - } else { - ret = uvc_query_ctrl(chain->dev, UVC_GET_CUR, - ctrl->entity->id, - chain->dev->intfnum, - ctrl->info.selector, - uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT), - ctrl->info.size); - } - if (ret < 0) - return ret; + if (ctrl->loaded) + return 0; - ctrl->loaded = 1; + if (ctrl->entity->get_cur) { + ret = ctrl->entity->get_cur(chain->dev, + ctrl->entity, + ctrl->info.selector, + uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT), + ctrl->info.size); + } else { + ret = uvc_query_ctrl(chain->dev, UVC_GET_CUR, + ctrl->entity->id, chain->dev->intfnum, + ctrl->info.selector, + uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT), + ctrl->info.size); } + if (ret < 0) + return ret; + + ctrl->loaded = 1; + + return ret; +} + +static int __uvc_ctrl_get(struct uvc_video_chain *chain, + struct uvc_control *ctrl, struct uvc_control_mapping *mapping, + s32 *value) +{ + int ret = __uvc_ctrl_load_cur(chain, ctrl); + + if (ret < 0) + return ret; + *value = __uvc_ctrl_get_value(mapping, uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT)); @@ -1788,11 +1800,7 @@ int uvc_ctrl_set(struct uvc_fh *handle, memset(uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT), 0, ctrl->info.size); } else { - ret = uvc_query_ctrl(chain->dev, UVC_GET_CUR, - ctrl->entity->id, chain->dev->intfnum, - ctrl->info.selector, - uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT), - ctrl->info.size); + ret = __uvc_ctrl_load_cur(chain, ctrl); if (ret < 0) return ret; }
Entity controls should get_cur using an entity-defined function instead of via a query. Fix this in uvc_ctrl_set. Fixes: 65900c581d01 ("media: uvcvideo: Allow entity-defined get_info and get_cur") Signed-off-by: Yunke Cao <yunkec@google.com> --- Changelog since v1: -Factored out common logic into __uvc_ctrl_load_cur(). drivers/media/usb/uvc/uvc_ctrl.c | 62 ++++++++++++++++++-------------- 1 file changed, 35 insertions(+), 27 deletions(-)