* Re: [PATCH -next v2] staging: comedi: Remove unused variable ‘min_full_scale’ and function 'get_min_full_scales'
2021-05-14 8:52 [PATCH -next v2] staging: comedi: Remove unused variable ‘min_full_scale’ and function 'get_min_full_scales' Bixuan Cui
@ 2021-05-14 7:52 ` Greg KH
2021-05-14 11:16 ` Ian Abbott
2021-05-17 9:44 ` Ian Abbott
1 sibling, 1 reply; 6+ messages in thread
From: Greg KH @ 2021-05-14 7:52 UTC (permalink / raw)
To: Bixuan Cui; +Cc: linux-kernel, abbotti, hsweeten, grandmaster
On Fri, May 14, 2021 at 04:52:14PM +0800, Bixuan Cui wrote:
> The variable ‘min_full_scale’ and function 'get_min_full_scales' are
> not used, So delete them.
>
> Signed-off-by: Bixuan Cui <cuibixuan@huawei.com>
> ---
> Changes from v2:
> * Delete function 'get_min_full_scales'
>
> drivers/comedi/drivers/jr3_pci.c | 15 ---------------
> 1 file changed, 15 deletions(-)
>
> diff --git a/drivers/comedi/drivers/jr3_pci.c b/drivers/comedi/drivers/jr3_pci.c
> index 7a02c4fa3cda..f963080dd61f 100644
> --- a/drivers/comedi/drivers/jr3_pci.c
> +++ b/drivers/comedi/drivers/jr3_pci.c
> @@ -186,19 +186,6 @@ static void set_full_scales(struct jr3_sensor __iomem *sensor,
> set_s16(&sensor->command_word0, 0x0a00);
> }
>
> -static struct six_axis_t get_min_full_scales(struct jr3_sensor __iomem *sensor)
> -{
> - struct six_axis_t result;
> -
> - result.fx = get_s16(&sensor->min_full_scale.fx);
> - result.fy = get_s16(&sensor->min_full_scale.fy);
> - result.fz = get_s16(&sensor->min_full_scale.fz);
> - result.mx = get_s16(&sensor->min_full_scale.mx);
> - result.my = get_s16(&sensor->min_full_scale.my);
> - result.mz = get_s16(&sensor->min_full_scale.mz);
> - return result;
Are you _SURE_ that this is ok to do? You are reading from the
hardware, and that is sometimes required depending on the
internal-to-the-device state machine.
Have you tested to verify that not doing these reads does not break
something?
This is a constant theme in these "remove variables that are not being
used" patches, please be aware that how hardware works is very specific
and when you mess with how it is accessed, that has the chance of
causing problems.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH -next v2] staging: comedi: Remove unused variable ‘min_full_scale’ and function 'get_min_full_scales'
@ 2021-05-14 8:52 Bixuan Cui
2021-05-14 7:52 ` Greg KH
2021-05-17 9:44 ` Ian Abbott
0 siblings, 2 replies; 6+ messages in thread
From: Bixuan Cui @ 2021-05-14 8:52 UTC (permalink / raw)
To: linux-kernel; +Cc: abbotti, hsweeten, gregkh, grandmaster
The variable ‘min_full_scale’ and function 'get_min_full_scales' are
not used, So delete them.
Signed-off-by: Bixuan Cui <cuibixuan@huawei.com>
---
Changes from v2:
* Delete function 'get_min_full_scales'
drivers/comedi/drivers/jr3_pci.c | 15 ---------------
1 file changed, 15 deletions(-)
diff --git a/drivers/comedi/drivers/jr3_pci.c b/drivers/comedi/drivers/jr3_pci.c
index 7a02c4fa3cda..f963080dd61f 100644
--- a/drivers/comedi/drivers/jr3_pci.c
+++ b/drivers/comedi/drivers/jr3_pci.c
@@ -186,19 +186,6 @@ static void set_full_scales(struct jr3_sensor __iomem *sensor,
set_s16(&sensor->command_word0, 0x0a00);
}
-static struct six_axis_t get_min_full_scales(struct jr3_sensor __iomem *sensor)
-{
- struct six_axis_t result;
-
- result.fx = get_s16(&sensor->min_full_scale.fx);
- result.fy = get_s16(&sensor->min_full_scale.fy);
- result.fz = get_s16(&sensor->min_full_scale.fz);
- result.mx = get_s16(&sensor->min_full_scale.mx);
- result.my = get_s16(&sensor->min_full_scale.my);
- result.mz = get_s16(&sensor->min_full_scale.mz);
- return result;
-}
-
static struct six_axis_t get_max_full_scales(struct jr3_sensor __iomem *sensor)
{
struct six_axis_t result;
@@ -504,10 +491,8 @@ jr3_pci_poll_subdevice(struct comedi_subdevice *s)
result = poll_delay_min_max(20, 100);
} else {
/* Set full scale */
- struct six_axis_t min_full_scale;
struct six_axis_t max_full_scale;
- min_full_scale = get_min_full_scales(sensor);
max_full_scale = get_max_full_scales(sensor);
set_full_scales(sensor, max_full_scale);
--
2.17.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH -next v2] staging: comedi: Remove unused variable ‘min_full_scale’ and function 'get_min_full_scales'
2021-05-14 7:52 ` Greg KH
@ 2021-05-14 11:16 ` Ian Abbott
2021-05-14 11:54 ` Greg KH
0 siblings, 1 reply; 6+ messages in thread
From: Ian Abbott @ 2021-05-14 11:16 UTC (permalink / raw)
To: Greg KH, Bixuan Cui; +Cc: linux-kernel, hsweeten, grandmaster
On 14/05/2021 08:52, Greg KH wrote:
> On Fri, May 14, 2021 at 04:52:14PM +0800, Bixuan Cui wrote:
>> The variable ‘min_full_scale’ and function 'get_min_full_scales' are
>> not used, So delete them.
>>
>> Signed-off-by: Bixuan Cui <cuibixuan@huawei.com>
>> ---
>> Changes from v2:
>> * Delete function 'get_min_full_scales'
>>
>> drivers/comedi/drivers/jr3_pci.c | 15 ---------------
>> 1 file changed, 15 deletions(-)
>>
>> diff --git a/drivers/comedi/drivers/jr3_pci.c b/drivers/comedi/drivers/jr3_pci.c
>> index 7a02c4fa3cda..f963080dd61f 100644
>> --- a/drivers/comedi/drivers/jr3_pci.c
>> +++ b/drivers/comedi/drivers/jr3_pci.c
>> @@ -186,19 +186,6 @@ static void set_full_scales(struct jr3_sensor __iomem *sensor,
>> set_s16(&sensor->command_word0, 0x0a00);
>> }
>>
>> -static struct six_axis_t get_min_full_scales(struct jr3_sensor __iomem *sensor)
>> -{
>> - struct six_axis_t result;
>> -
>> - result.fx = get_s16(&sensor->min_full_scale.fx);
>> - result.fy = get_s16(&sensor->min_full_scale.fy);
>> - result.fz = get_s16(&sensor->min_full_scale.fz);
>> - result.mx = get_s16(&sensor->min_full_scale.mx);
>> - result.my = get_s16(&sensor->min_full_scale.my);
>> - result.mz = get_s16(&sensor->min_full_scale.mz);
>> - return result;
>
> Are you _SURE_ that this is ok to do? You are reading from the
> hardware, and that is sometimes required depending on the
> internal-to-the-device state machine.
>
> Have you tested to verify that not doing these reads does not break
> something?
>
> This is a constant theme in these "remove variables that are not being
> used" patches, please be aware that how hardware works is very specific
> and when you mess with how it is accessed, that has the chance of
> causing problems.
I don't think removing this would do any harm. The `get_s16` function
is just a simple wrapper around `readl` that converts the type of the
returned value to `s16`. The registers for minimum full scale and
maximum full scale are just informational, and there is nothing in the
manual[1] about reading registers in a particular order. The driver
used to print these values to the kernel log, but that code was removed
in commit a1d16659538a ("staging: comedi: jr3_pci: remove noisy printk").
[1] 5907E - Legacy PCI Receiver Manual
<https://www.jr3.com/component/phocadownload/category/6-product-manuals?download=36:5907e-legacy-pci-receiver-manual>
--
-=( Ian Abbott <abbotti@mev.co.uk> || MEV Ltd. is a company )=-
-=( registered in England & Wales. Regd. number: 02862268. )=-
-=( Regd. addr.: S11 & 12 Building 67, Europa Business Park, )=-
-=( Bird Hall Lane, STOCKPORT, SK3 0XA, UK. || www.mev.co.uk )=-
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH -next v2] staging: comedi: Remove unused variable ‘min_full_scale’ and function 'get_min_full_scales'
2021-05-14 11:16 ` Ian Abbott
@ 2021-05-14 11:54 ` Greg KH
0 siblings, 0 replies; 6+ messages in thread
From: Greg KH @ 2021-05-14 11:54 UTC (permalink / raw)
To: Ian Abbott; +Cc: Bixuan Cui, linux-kernel, hsweeten, grandmaster
On Fri, May 14, 2021 at 12:16:11PM +0100, Ian Abbott wrote:
> On 14/05/2021 08:52, Greg KH wrote:
> > On Fri, May 14, 2021 at 04:52:14PM +0800, Bixuan Cui wrote:
> > > The variable ‘min_full_scale’ and function 'get_min_full_scales' are
> > > not used, So delete them.
> > >
> > > Signed-off-by: Bixuan Cui <cuibixuan@huawei.com>
> > > ---
> > > Changes from v2:
> > > * Delete function 'get_min_full_scales'
> > >
> > > drivers/comedi/drivers/jr3_pci.c | 15 ---------------
> > > 1 file changed, 15 deletions(-)
> > >
> > > diff --git a/drivers/comedi/drivers/jr3_pci.c b/drivers/comedi/drivers/jr3_pci.c
> > > index 7a02c4fa3cda..f963080dd61f 100644
> > > --- a/drivers/comedi/drivers/jr3_pci.c
> > > +++ b/drivers/comedi/drivers/jr3_pci.c
> > > @@ -186,19 +186,6 @@ static void set_full_scales(struct jr3_sensor __iomem *sensor,
> > > set_s16(&sensor->command_word0, 0x0a00);
> > > }
> > > -static struct six_axis_t get_min_full_scales(struct jr3_sensor __iomem *sensor)
> > > -{
> > > - struct six_axis_t result;
> > > -
> > > - result.fx = get_s16(&sensor->min_full_scale.fx);
> > > - result.fy = get_s16(&sensor->min_full_scale.fy);
> > > - result.fz = get_s16(&sensor->min_full_scale.fz);
> > > - result.mx = get_s16(&sensor->min_full_scale.mx);
> > > - result.my = get_s16(&sensor->min_full_scale.my);
> > > - result.mz = get_s16(&sensor->min_full_scale.mz);
> > > - return result;
> >
> > Are you _SURE_ that this is ok to do? You are reading from the
> > hardware, and that is sometimes required depending on the
> > internal-to-the-device state machine.
> >
> > Have you tested to verify that not doing these reads does not break
> > something?
> >
> > This is a constant theme in these "remove variables that are not being
> > used" patches, please be aware that how hardware works is very specific
> > and when you mess with how it is accessed, that has the chance of
> > causing problems.
>
> I don't think removing this would do any harm. The `get_s16` function is
> just a simple wrapper around `readl` that converts the type of the returned
> value to `s16`. The registers for minimum full scale and maximum full scale
> are just informational, and there is nothing in the manual[1] about reading
> registers in a particular order. The driver used to print these values to
> the kernel log, but that code was removed in commit a1d16659538a ("staging:
> comedi: jr3_pci: remove noisy printk").
>
> [1] 5907E - Legacy PCI Receiver Manual
> <https://www.jr3.com/component/phocadownload/category/6-product-manuals?download=36:5907e-legacy-pci-receiver-manual>
>
Ok, can I get a "reviewed-by:" then?
thanks,
greg k-h
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH -next v2] staging: comedi: Remove unused variable ‘min_full_scale’ and function 'get_min_full_scales'
2021-05-14 8:52 [PATCH -next v2] staging: comedi: Remove unused variable ‘min_full_scale’ and function 'get_min_full_scales' Bixuan Cui
2021-05-14 7:52 ` Greg KH
@ 2021-05-17 9:44 ` Ian Abbott
2021-05-19 1:07 ` Bixuan Cui
1 sibling, 1 reply; 6+ messages in thread
From: Ian Abbott @ 2021-05-17 9:44 UTC (permalink / raw)
To: Bixuan Cui, linux-kernel; +Cc: hsweeten, gregkh, grandmaster
On 14/05/2021 09:52, Bixuan Cui wrote:
> The variable ‘min_full_scale’ and function 'get_min_full_scales' are
> not used, So delete them.
>
> Signed-off-by: Bixuan Cui <cuibixuan@huawei.com>
> ---
> Changes from v2:
> * Delete function 'get_min_full_scales'
>
> drivers/comedi/drivers/jr3_pci.c | 15 ---------------
> 1 file changed, 15 deletions(-)
>
> diff --git a/drivers/comedi/drivers/jr3_pci.c b/drivers/comedi/drivers/jr3_pci.c
> index 7a02c4fa3cda..f963080dd61f 100644
> --- a/drivers/comedi/drivers/jr3_pci.c
> +++ b/drivers/comedi/drivers/jr3_pci.c
> @@ -186,19 +186,6 @@ static void set_full_scales(struct jr3_sensor __iomem *sensor,
> set_s16(&sensor->command_word0, 0x0a00);
> }
>
> -static struct six_axis_t get_min_full_scales(struct jr3_sensor __iomem *sensor)
> -{
> - struct six_axis_t result;
> -
> - result.fx = get_s16(&sensor->min_full_scale.fx);
> - result.fy = get_s16(&sensor->min_full_scale.fy);
> - result.fz = get_s16(&sensor->min_full_scale.fz);
> - result.mx = get_s16(&sensor->min_full_scale.mx);
> - result.my = get_s16(&sensor->min_full_scale.my);
> - result.mz = get_s16(&sensor->min_full_scale.mz);
> - return result;
> -}
> -
> static struct six_axis_t get_max_full_scales(struct jr3_sensor __iomem *sensor)
> {
> struct six_axis_t result;
> @@ -504,10 +491,8 @@ jr3_pci_poll_subdevice(struct comedi_subdevice *s)
> result = poll_delay_min_max(20, 100);
> } else {
> /* Set full scale */
> - struct six_axis_t min_full_scale;
> struct six_axis_t max_full_scale;
>
> - min_full_scale = get_min_full_scales(sensor);
> max_full_scale = get_max_full_scales(sensor);
> set_full_scales(sensor, max_full_scale);
>
>
I'm pretty sure this is OK. The min_full_scale registers are just
informational. The driver used to print the values, but not since
commit a1d16659538a ("staging: comedi: jr3_pci: remove noisy printk") so
this code is now redundant.
Reviewed-by: Ian Abbott <abbotti@mev.co.uk>
--
-=( Ian Abbott <abbotti@mev.co.uk> || MEV Ltd. is a company )=-
-=( registered in England & Wales. Regd. number: 02862268. )=-
-=( Regd. addr.: S11 & 12 Building 67, Europa Business Park, )=-
-=( Bird Hall Lane, STOCKPORT, SK3 0XA, UK. || www.mev.co.uk )=-
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH -next v2] staging: comedi: Remove unused variable ‘min_full_scale’ and function 'get_min_full_scales'
2021-05-17 9:44 ` Ian Abbott
@ 2021-05-19 1:07 ` Bixuan Cui
0 siblings, 0 replies; 6+ messages in thread
From: Bixuan Cui @ 2021-05-19 1:07 UTC (permalink / raw)
To: Ian Abbott, linux-kernel; +Cc: hsweeten, gregkh, grandmaster
On 2021/5/17 17:44, Ian Abbott wrote:
> On 14/05/2021 09:52, Bixuan Cui wrote:
>> The variable ‘min_full_scale’ and function 'get_min_full_scales' are
>> not used, So delete them.
>>
>> Signed-off-by: Bixuan Cui <cuibixuan@huawei.com>
>> ---
>> Changes from v2:
>> * Delete function 'get_min_full_scales'
>>
>> drivers/comedi/drivers/jr3_pci.c | 15 ---------------
>> 1 file changed, 15 deletions(-)
>>
>> diff --git a/drivers/comedi/drivers/jr3_pci.c b/drivers/comedi/drivers/jr3_pci.c
>> index 7a02c4fa3cda..f963080dd61f 100644
>> --- a/drivers/comedi/drivers/jr3_pci.c
>> +++ b/drivers/comedi/drivers/jr3_pci.c
>> @@ -186,19 +186,6 @@ static void set_full_scales(struct jr3_sensor __iomem *sensor,
>> set_s16(&sensor->command_word0, 0x0a00);
>> }
>> -static struct six_axis_t get_min_full_scales(struct jr3_sensor __iomem *sensor)
>> -{
>> - struct six_axis_t result;
>> -
>> - result.fx = get_s16(&sensor->min_full_scale.fx);
>> - result.fy = get_s16(&sensor->min_full_scale.fy);
>> - result.fz = get_s16(&sensor->min_full_scale.fz);
>> - result.mx = get_s16(&sensor->min_full_scale.mx);
>> - result.my = get_s16(&sensor->min_full_scale.my);
>> - result.mz = get_s16(&sensor->min_full_scale.mz);
>> - return result;
>> -}
>> -
>> static struct six_axis_t get_max_full_scales(struct jr3_sensor __iomem *sensor)
>> {
>> struct six_axis_t result;
>> @@ -504,10 +491,8 @@ jr3_pci_poll_subdevice(struct comedi_subdevice *s)
>> result = poll_delay_min_max(20, 100);
>> } else {
>> /* Set full scale */
>> - struct six_axis_t min_full_scale;
>> struct six_axis_t max_full_scale;
>> - min_full_scale = get_min_full_scales(sensor);
>> max_full_scale = get_max_full_scales(sensor);
>> set_full_scales(sensor, max_full_scale);
>>
>
> I'm pretty sure this is OK. The min_full_scale registers are just informational. The driver used to print the values, but not since commit a1d16659538a ("staging: comedi: jr3_pci: remove noisy printk") so this code is now redundant.
Thanks for your reply.
Thanks
Bixuan Cui
>
> Reviewed-by: Ian Abbott <abbotti@mev.co.uk>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-05-19 1:07 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-14 8:52 [PATCH -next v2] staging: comedi: Remove unused variable ‘min_full_scale’ and function 'get_min_full_scales' Bixuan Cui
2021-05-14 7:52 ` Greg KH
2021-05-14 11:16 ` Ian Abbott
2021-05-14 11:54 ` Greg KH
2021-05-17 9:44 ` Ian Abbott
2021-05-19 1:07 ` Bixuan Cui
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).