1. the error code of ENOSYS indicates Invalid system call number,
but there is not system call
2. unified the logging error message, when slave_set func is NULL
Signed-off-by: Piro Yang <[email protected]>
---
drivers/staging/vme_user/vme.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/vme_user/vme.c b/drivers/staging/vme_user/vme.c
index 5c416c31ec57..e9461a7a7ab8 100644
--- a/drivers/staging/vme_user/vme.c
+++ b/drivers/staging/vme_user/vme.c
@@ -340,8 +340,8 @@ int vme_slave_set(struct vme_resource *resource, int enabled,
image = list_entry(resource->entry, struct vme_slave_resource, list);
if (!bridge->slave_set) {
- dev_err(bridge->parent, "Function not supported\n");
- return -ENOSYS;
+ dev_err(bridge->parent, "%s not supported\n", __func__);
+ return -EINVAL;
}
if (!(((image->address_attr & aspace) == aspace) &&
--
2.25.1
On Mon, Dec 11, 2023 at 11:54:53PM +0800, Piro Yang wrote:
> 1. the error code of ENOSYS indicates Invalid system call number,
> but there is not system call
>
> 2. unified the logging error message, when slave_set func is NULL
That is two different things, and as such, should be two different
changes, right?
Yes, it's picky, but that's what the staging driver code is for, to get
comfortable doing kernel development changes properly.
Also, are you sure this second change is correct:
>
> Signed-off-by: Piro Yang <[email protected]>
> ---
> drivers/staging/vme_user/vme.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/staging/vme_user/vme.c b/drivers/staging/vme_user/vme.c
> index 5c416c31ec57..e9461a7a7ab8 100644
> --- a/drivers/staging/vme_user/vme.c
> +++ b/drivers/staging/vme_user/vme.c
> @@ -340,8 +340,8 @@ int vme_slave_set(struct vme_resource *resource, int enabled,
> image = list_entry(resource->entry, struct vme_slave_resource, list);
>
> if (!bridge->slave_set) {
> - dev_err(bridge->parent, "Function not supported\n");
> - return -ENOSYS;
> + dev_err(bridge->parent, "%s not supported\n", __func__);
__func__ is not the same here as "Function", right? "Function" is the
functionality of the thing that is attempted here, so replacing that
word with the function name seems odd to me, don't you think?
thanks,
greg k-h
Greg KH <[email protected]> 于2023年12月12日周二 00:01写道：
>
> On Mon, Dec 11, 2023 at 11:54:53PM +0800, Piro Yang wrote:
> > 1. the error code of ENOSYS indicates Invalid system call number,
> > but there is not system call
> >
> > 2. unified the logging error message, when slave_set func is NULL
>
> That is two different things, and as such, should be two different
> changes, right?
>
> Yes, it's picky, but that's what the staging driver code is for, to get
> comfortable doing kernel development changes properly.
>
> Also, are you sure this second change is correct:
yes, I'm sure .
the vme_slave_set function is diffierent
/*****************************vme_slave_set***********************/
int vme_slave_set(struct vme_resource *resource, int enabled,
unsigned long long vme_base, unsigned long long size,
dma_addr_t buf_base, u32 aspace, u32 cycle)
{
...
if (!bridge->slave_set) {
dev_err(bridge->parent, "Function not supported\n");
return -ENOSYS;
}
/******************other functions like below: *******************/
int vme_slave_get(struct vme_resource *resource, int *enabled,
unsigned long long *vme_base, unsigned long long *size,
dma_addr_t *buf_base, u32 *aspace, u32 *cycle)
{
....
if (!bridge->slave_get) {
dev_err(bridge->parent, "%s not supported\n", __func__);
return -EINVAL;
}
int vme_master_set(struct vme_resource *resource, int enabled,
unsigned long long vme_base, unsigned long long size,
u32 aspace, u32 cycle, u32 dwidth)
{
....
if (!bridge->master_set) {
dev_warn(bridge->parent, "%s not supported\n", __func__);
return -EINVAL;
}
int vme_master_get(struct vme_resource *resource, int *enabled,
unsigned long long *vme_base, unsigned long long *size,
u32 *aspace, u32 *cycle, u32 *dwidth)
{
....
if (!bridge->master_get) {
dev_warn(bridge->parent, "%s not supported\n", __func__);
return -EINVAL;
}
>
> >
> > Signed-off-by: Piro Yang <[email protected]>
> > ---
> > drivers/staging/vme_user/vme.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/staging/vme_user/vme.c b/drivers/staging/vme_user/vme.c
> > index 5c416c31ec57..e9461a7a7ab8 100644
> > --- a/drivers/staging/vme_user/vme.c
> > +++ b/drivers/staging/vme_user/vme.c
> > @@ -340,8 +340,8 @@ int vme_slave_set(struct vme_resource *resource, int enabled,
> > image = list_entry(resource->entry, struct vme_slave_resource, list);
> >
> > if (!bridge->slave_set) {
> > - dev_err(bridge->parent, "Function not supported\n");
> > - return -ENOSYS;
> > + dev_err(bridge->parent, "%s not supported\n", __func__);
>
> __func__ is not the same here as "Function", right? "Function" is the
> functionality of the thing that is attempted here, so replacing that
> word with the function name seems odd to me, don't you think?
>
> thanks,
>
> greg k-h
piro yang <[email protected]> 于2023年12月12日周二 00:13写道：
>
> Greg KH <[email protected]> 于2023年12月12日周二 00:01写道：
> >
> > On Mon, Dec 11, 2023 at 11:54:53PM +0800, Piro Yang wrote:
> > > 1. the error code of ENOSYS indicates Invalid system call number,
> > > but there is not system call
> > >
> > > 2. unified the logging error message, when slave_set func is NULL
> >
> > That is two different things, and as such, should be two different
> > changes, right?
> >
> > Yes, it's picky, but that's what the staging driver code is for, to get
> > comfortable doing kernel development changes properly.
> >
> > Also, are you sure this second change is correct:
>
> yes, I'm sure .
> the vme_slave_set function is diffierent
> /*****************************vme_slave_set***********************/
> int vme_slave_set(struct vme_resource *resource, int enabled,
> unsigned long long vme_base, unsigned long long size,
> dma_addr_t buf_base, u32 aspace, u32 cycle)
> {
> ...
> if (!bridge->slave_set) {
> dev_err(bridge->parent, "Function not supported\n");
> return -ENOSYS;
> }
>
> /******************other functions like below: *******************/
> int vme_slave_get(struct vme_resource *resource, int *enabled,
> unsigned long long *vme_base, unsigned long long *size,
> dma_addr_t *buf_base, u32 *aspace, u32 *cycle)
> {
> ....
> if (!bridge->slave_get) {
> dev_err(bridge->parent, "%s not supported\n", __func__);
> return -EINVAL;
> }
>
> int vme_master_set(struct vme_resource *resource, int enabled,
> unsigned long long vme_base, unsigned long long size,
> u32 aspace, u32 cycle, u32 dwidth)
> {
> ....
> if (!bridge->master_set) {
> dev_warn(bridge->parent, "%s not supported\n", __func__);
> return -EINVAL;
> }
>
> int vme_master_get(struct vme_resource *resource, int *enabled,
> unsigned long long *vme_base, unsigned long long *size,
> u32 *aspace, u32 *cycle, u32 *dwidth)
> {
> ....
> if (!bridge->master_get) {
> dev_warn(bridge->parent, "%s not supported\n", __func__);
> return -EINVAL;
> }
>
> >
> > >
> > > Signed-off-by: Piro Yang <[email protected]>
> > > ---
> > > drivers/staging/vme_user/vme.c | 4 ++--
> > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/staging/vme_user/vme.c b/drivers/staging/vme_user/vme.c
> > > index 5c416c31ec57..e9461a7a7ab8 100644
> > > --- a/drivers/staging/vme_user/vme.c
> > > +++ b/drivers/staging/vme_user/vme.c
> > > @@ -340,8 +340,8 @@ int vme_slave_set(struct vme_resource *resource, int enabled,
> > > image = list_entry(resource->entry, struct vme_slave_resource, list);
> > >
> > > if (!bridge->slave_set) {
> > > - dev_err(bridge->parent, "Function not supported\n");
> > > - return -ENOSYS;
> > > + dev_err(bridge->parent, "%s not supported\n", __func__);
> >
> > __func__ is not the same here as "Function", right? "Function" is the
> > functionality of the thing that is attempted here, so replacing that
> > word with the function name seems odd to me, don't you think?
> >
yes, __func__ is not the same here as "Function".
but I think the logging error message format shoud be unified
with other functions.
like: vme_slave_get(){
.....
if (!bridge->slave_get) {
dev_err(bridge->parent, "%s not supported\n", __func__);
> > thanks,
> >
> > greg k-h