| Message ID | ZmkrgTlxNwm_oHxv@google.com |
|---|---|
| State | Accepted |
| Commit | 50717edb6dd74dbc0726fb2ba8723ba2a22a2011 |
| Headers | show |
| Series | [v2] Input: adc-joystick - move axes data into the main structure | expand |
Hi Dmitry, On 2024-06-12 07:00, Dmitry Torokhov wrote: > There is no need to allocate axes information separately from the main > joystick structure so let's fold the allocation and also drop members > (such as range, flat and fuzz) that are only used during initialization > of the device. > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > --- > > v2: > > - fixed issue with uninitialized "axes" in adc_joystick_set_axes() > pointed out by Dan Carpenter > - fixed issue with checking wrong variable in adc_joystick_probe() > pointed out by Dan Carpenter > > drivers/input/joystick/adc-joystick.c | 113 ++++++++++++++------------ > 1 file changed, 60 insertions(+), 53 deletions(-) > > diff --git a/drivers/input/joystick/adc-joystick.c > b/drivers/input/joystick/adc-joystick.c > index 916e78e4dc9f..1e30cbcd8c61 100644 > --- a/drivers/input/joystick/adc-joystick.c > +++ b/drivers/input/joystick/adc-joystick.c > @@ -15,19 +15,15 @@ > > struct adc_joystick_axis { > u32 code; > - s32 range[2]; > - s32 fuzz; > - s32 flat; > bool inverted; > }; > > struct adc_joystick { > struct input_dev *input; > struct iio_cb_buffer *buffer; > - struct adc_joystick_axis *axes; > struct iio_channel *chans; > - int num_chans; > - bool polled; > + unsigned int num_chans; > + struct adc_joystick_axis axes[] __counted_by(num_chans); > }; > > static int adc_joystick_invert(struct input_dev *dev, > @@ -135,9 +131,11 @@ static void adc_joystick_cleanup(void *data) > > static int adc_joystick_set_axes(struct device *dev, struct > adc_joystick *joy) > { > - struct adc_joystick_axis *axes; > + struct adc_joystick_axis *axes = joy->axes; > struct fwnode_handle *child; > - int num_axes, error, i; > + s32 range[2], fuzz, flat; > + unsigned int num_axes; > + int error, i; > > num_axes = device_get_child_node_count(dev); > if (!num_axes) { > @@ -151,10 +149,6 @@ static int adc_joystick_set_axes(struct device > *dev, struct adc_joystick *joy) > return -EINVAL; > } > > - axes = devm_kmalloc_array(dev, num_axes, sizeof(*axes), GFP_KERNEL); > - if (!axes) > - return -ENOMEM; > - > device_for_each_child_node(dev, child) { > error = fwnode_property_read_u32(child, "reg", &i); > if (error) { > @@ -176,29 +170,25 @@ static int adc_joystick_set_axes(struct device > *dev, struct adc_joystick *joy) > } > > error = fwnode_property_read_u32_array(child, "abs-range", > - axes[i].range, 2); > + range, 2); > if (error) { > dev_err(dev, "abs-range invalid or missing\n"); > goto err_fwnode_put; > } > > - if (axes[i].range[0] > axes[i].range[1]) { > + if (range[0] > range[1]) { > dev_dbg(dev, "abs-axis %d inverted\n", i); > axes[i].inverted = true; > - swap(axes[i].range[0], axes[i].range[1]); > + swap(range[0], range[1]); > } > > - fwnode_property_read_u32(child, "abs-fuzz", &axes[i].fuzz); > - fwnode_property_read_u32(child, "abs-flat", &axes[i].flat); > + fwnode_property_read_u32(child, "abs-fuzz", &fuzz); > + fwnode_property_read_u32(child, "abs-flat", &flat); > > input_set_abs_params(joy->input, axes[i].code, > - axes[i].range[0], axes[i].range[1], > - axes[i].fuzz, axes[i].flat); > - input_set_capability(joy->input, EV_ABS, axes[i].code); > + range[0], range[1], fuzz, flat); > } > > - joy->axes = axes; > - > return 0; > > err_fwnode_put: > @@ -206,23 +196,49 @@ static int adc_joystick_set_axes(struct device > *dev, struct adc_joystick *joy) > return error; > } > > + > +/* > + * Count how many channels we got. NULL terminated. > + * Do not check the storage size if using polling. > + */ > +static int adc_joystick_count_channels(struct device *dev, > + const struct iio_channel *chans, > + bool polled, > + unsigned int *num_chans) You forgot to assign *num_chans = i; at the end of this function, which leaves it uninitialized in the caller context. > +{ > + int bits; > + int i; > + Let's move that "NULL terminated." comment here, since it's about the for loop. With the above comments addressed: Acked-by: Artur Rojek <contact@artur-rojek.eu> Cheers, Artur > + for (i = 0; chans[i].indio_dev; i++) { > + if (polled) > + continue; > + bits = chans[i].channel->scan_type.storagebits; > + if (!bits || bits > 16) { > + dev_err(dev, "Unsupported channel storage size\n"); > + return -EINVAL; > + } > + if (bits != chans[0].channel->scan_type.storagebits) { > + dev_err(dev, "Channels must have equal storage size\n"); > + return -EINVAL; > + } > + } > + > + return i; > +} > + > static int adc_joystick_probe(struct platform_device *pdev) > { > struct device *dev = &pdev->dev; > + struct iio_channel *chans; > struct adc_joystick *joy; > struct input_dev *input; > + unsigned int poll_interval = 0; > + unsigned int num_chans; > int error; > - int bits; > - int i; > - unsigned int poll_interval; > - > - joy = devm_kzalloc(dev, sizeof(*joy), GFP_KERNEL); > - if (!joy) > - return -ENOMEM; > > - joy->chans = devm_iio_channel_get_all(dev); > - if (IS_ERR(joy->chans)) { > - error = PTR_ERR(joy->chans); > + chans = devm_iio_channel_get_all(dev); > + error = PTR_ERR_OR_ZERO(chans); > + if (error) { > if (error != -EPROBE_DEFER) > dev_err(dev, "Unable to get IIO channels"); > return error; > @@ -236,28 +252,19 @@ static int adc_joystick_probe(struct > platform_device *pdev) > } else if (poll_interval == 0) { > dev_err(dev, "Unable to get poll-interval\n"); > return -EINVAL; > - } else { > - joy->polled = true; > } > > - /* > - * Count how many channels we got. NULL terminated. > - * Do not check the storage size if using polling. > - */ > - for (i = 0; joy->chans[i].indio_dev; i++) { > - if (joy->polled) > - continue; > - bits = joy->chans[i].channel->scan_type.storagebits; > - if (!bits || bits > 16) { > - dev_err(dev, "Unsupported channel storage size\n"); > - return -EINVAL; > - } > - if (bits != joy->chans[0].channel->scan_type.storagebits) { > - dev_err(dev, "Channels must have equal storage size\n"); > - return -EINVAL; > - } > - } > - joy->num_chans = i; > + error = adc_joystick_count_channels(dev, chans, poll_interval != 0, > + &num_chans); > + if (error) > + return error; > + > + joy = devm_kzalloc(dev, struct_size(joy, axes, num_chans), > GFP_KERNEL); > + if (!joy) > + return -ENOMEM; > + > + joy->chans = chans; > + joy->num_chans = num_chans; > > input = devm_input_allocate_device(dev); > if (!input) { > @@ -273,7 +280,7 @@ static int adc_joystick_probe(struct > platform_device *pdev) > if (error) > return error; > > - if (joy->polled) { > + if (poll_interval != 0) { > input_setup_polling(input, adc_joystick_poll); > input_set_poll_interval(input, poll_interval); > } else { > -- > 2.45.2.505.gda0bf45e8d-goog
Hi Artur, On Thu, Jun 20, 2024 at 11:13:27PM +0200, Artur Rojek wrote: > Hi Dmitry, > > On 2024-06-12 07:00, Dmitry Torokhov wrote: > > There is no need to allocate axes information separately from the main > > joystick structure so let's fold the allocation and also drop members > > (such as range, flat and fuzz) that are only used during initialization > > of the device. > > > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > > --- > > > > v2: > > > > - fixed issue with uninitialized "axes" in adc_joystick_set_axes() > > pointed out by Dan Carpenter > > - fixed issue with checking wrong variable in adc_joystick_probe() > > pointed out by Dan Carpenter > > > > drivers/input/joystick/adc-joystick.c | 113 ++++++++++++++------------ > > 1 file changed, 60 insertions(+), 53 deletions(-) > > > > diff --git a/drivers/input/joystick/adc-joystick.c > > b/drivers/input/joystick/adc-joystick.c > > index 916e78e4dc9f..1e30cbcd8c61 100644 > > --- a/drivers/input/joystick/adc-joystick.c > > +++ b/drivers/input/joystick/adc-joystick.c > > @@ -15,19 +15,15 @@ > > > > struct adc_joystick_axis { > > u32 code; > > - s32 range[2]; > > - s32 fuzz; > > - s32 flat; > > bool inverted; > > }; > > > > struct adc_joystick { > > struct input_dev *input; > > struct iio_cb_buffer *buffer; > > - struct adc_joystick_axis *axes; > > struct iio_channel *chans; > > - int num_chans; > > - bool polled; > > + unsigned int num_chans; > > + struct adc_joystick_axis axes[] __counted_by(num_chans); > > }; > > > > static int adc_joystick_invert(struct input_dev *dev, > > @@ -135,9 +131,11 @@ static void adc_joystick_cleanup(void *data) > > > > static int adc_joystick_set_axes(struct device *dev, struct > > adc_joystick *joy) > > { > > - struct adc_joystick_axis *axes; > > + struct adc_joystick_axis *axes = joy->axes; > > struct fwnode_handle *child; > > - int num_axes, error, i; > > + s32 range[2], fuzz, flat; > > + unsigned int num_axes; > > + int error, i; > > > > num_axes = device_get_child_node_count(dev); > > if (!num_axes) { > > @@ -151,10 +149,6 @@ static int adc_joystick_set_axes(struct device > > *dev, struct adc_joystick *joy) > > return -EINVAL; > > } > > > > - axes = devm_kmalloc_array(dev, num_axes, sizeof(*axes), GFP_KERNEL); > > - if (!axes) > > - return -ENOMEM; > > - > > device_for_each_child_node(dev, child) { > > error = fwnode_property_read_u32(child, "reg", &i); > > if (error) { > > @@ -176,29 +170,25 @@ static int adc_joystick_set_axes(struct device > > *dev, struct adc_joystick *joy) > > } > > > > error = fwnode_property_read_u32_array(child, "abs-range", > > - axes[i].range, 2); > > + range, 2); > > if (error) { > > dev_err(dev, "abs-range invalid or missing\n"); > > goto err_fwnode_put; > > } > > > > - if (axes[i].range[0] > axes[i].range[1]) { > > + if (range[0] > range[1]) { > > dev_dbg(dev, "abs-axis %d inverted\n", i); > > axes[i].inverted = true; > > - swap(axes[i].range[0], axes[i].range[1]); > > + swap(range[0], range[1]); > > } > > > > - fwnode_property_read_u32(child, "abs-fuzz", &axes[i].fuzz); > > - fwnode_property_read_u32(child, "abs-flat", &axes[i].flat); > > + fwnode_property_read_u32(child, "abs-fuzz", &fuzz); > > + fwnode_property_read_u32(child, "abs-flat", &flat); > > > > input_set_abs_params(joy->input, axes[i].code, > > - axes[i].range[0], axes[i].range[1], > > - axes[i].fuzz, axes[i].flat); > > - input_set_capability(joy->input, EV_ABS, axes[i].code); > > + range[0], range[1], fuzz, flat); > > } > > > > - joy->axes = axes; > > - > > return 0; > > > > err_fwnode_put: > > @@ -206,23 +196,49 @@ static int adc_joystick_set_axes(struct device > > *dev, struct adc_joystick *joy) > > return error; > > } > > > > + > > +/* > > + * Count how many channels we got. NULL terminated. > > + * Do not check the storage size if using polling. > > + */ > > +static int adc_joystick_count_channels(struct device *dev, > > + const struct iio_channel *chans, > > + bool polled, > > + unsigned int *num_chans) > > You forgot to assign *num_chans = i; at the end of this function, > which leaves it uninitialized in the caller context. Indeed, and it needs to return 0 on success, not "i". Fixed. > > > +{ > > + int bits; > > + int i; > > + > > Let's move that "NULL terminated." comment here, since it's about the > for loop. Done and pushed out. > > With the above comments addressed: > Acked-by: Artur Rojek <contact@artur-rojek.eu> Thanks.
diff --git a/drivers/input/joystick/adc-joystick.c b/drivers/input/joystick/adc-joystick.c index 916e78e4dc9f..1e30cbcd8c61 100644 --- a/drivers/input/joystick/adc-joystick.c +++ b/drivers/input/joystick/adc-joystick.c @@ -15,19 +15,15 @@ struct adc_joystick_axis { u32 code; - s32 range[2]; - s32 fuzz; - s32 flat; bool inverted; }; struct adc_joystick { struct input_dev *input; struct iio_cb_buffer *buffer; - struct adc_joystick_axis *axes; struct iio_channel *chans; - int num_chans; - bool polled; + unsigned int num_chans; + struct adc_joystick_axis axes[] __counted_by(num_chans); }; static int adc_joystick_invert(struct input_dev *dev, @@ -135,9 +131,11 @@ static void adc_joystick_cleanup(void *data) static int adc_joystick_set_axes(struct device *dev, struct adc_joystick *joy) { - struct adc_joystick_axis *axes; + struct adc_joystick_axis *axes = joy->axes; struct fwnode_handle *child; - int num_axes, error, i; + s32 range[2], fuzz, flat; + unsigned int num_axes; + int error, i; num_axes = device_get_child_node_count(dev); if (!num_axes) { @@ -151,10 +149,6 @@ static int adc_joystick_set_axes(struct device *dev, struct adc_joystick *joy) return -EINVAL; } - axes = devm_kmalloc_array(dev, num_axes, sizeof(*axes), GFP_KERNEL); - if (!axes) - return -ENOMEM; - device_for_each_child_node(dev, child) { error = fwnode_property_read_u32(child, "reg", &i); if (error) { @@ -176,29 +170,25 @@ static int adc_joystick_set_axes(struct device *dev, struct adc_joystick *joy) } error = fwnode_property_read_u32_array(child, "abs-range", - axes[i].range, 2); + range, 2); if (error) { dev_err(dev, "abs-range invalid or missing\n"); goto err_fwnode_put; } - if (axes[i].range[0] > axes[i].range[1]) { + if (range[0] > range[1]) { dev_dbg(dev, "abs-axis %d inverted\n", i); axes[i].inverted = true; - swap(axes[i].range[0], axes[i].range[1]); + swap(range[0], range[1]); } - fwnode_property_read_u32(child, "abs-fuzz", &axes[i].fuzz); - fwnode_property_read_u32(child, "abs-flat", &axes[i].flat); + fwnode_property_read_u32(child, "abs-fuzz", &fuzz); + fwnode_property_read_u32(child, "abs-flat", &flat); input_set_abs_params(joy->input, axes[i].code, - axes[i].range[0], axes[i].range[1], - axes[i].fuzz, axes[i].flat); - input_set_capability(joy->input, EV_ABS, axes[i].code); + range[0], range[1], fuzz, flat); } - joy->axes = axes; - return 0; err_fwnode_put: @@ -206,23 +196,49 @@ static int adc_joystick_set_axes(struct device *dev, struct adc_joystick *joy) return error; } + +/* + * Count how many channels we got. NULL terminated. + * Do not check the storage size if using polling. + */ +static int adc_joystick_count_channels(struct device *dev, + const struct iio_channel *chans, + bool polled, + unsigned int *num_chans) +{ + int bits; + int i; + + for (i = 0; chans[i].indio_dev; i++) { + if (polled) + continue; + bits = chans[i].channel->scan_type.storagebits; + if (!bits || bits > 16) { + dev_err(dev, "Unsupported channel storage size\n"); + return -EINVAL; + } + if (bits != chans[0].channel->scan_type.storagebits) { + dev_err(dev, "Channels must have equal storage size\n"); + return -EINVAL; + } + } + + return i; +} + static int adc_joystick_probe(struct platform_device *pdev) { struct device *dev = &pdev->dev; + struct iio_channel *chans; struct adc_joystick *joy; struct input_dev *input; + unsigned int poll_interval = 0; + unsigned int num_chans; int error; - int bits; - int i; - unsigned int poll_interval; - - joy = devm_kzalloc(dev, sizeof(*joy), GFP_KERNEL); - if (!joy) - return -ENOMEM; - joy->chans = devm_iio_channel_get_all(dev); - if (IS_ERR(joy->chans)) { - error = PTR_ERR(joy->chans); + chans = devm_iio_channel_get_all(dev); + error = PTR_ERR_OR_ZERO(chans); + if (error) { if (error != -EPROBE_DEFER) dev_err(dev, "Unable to get IIO channels"); return error; @@ -236,28 +252,19 @@ static int adc_joystick_probe(struct platform_device *pdev) } else if (poll_interval == 0) { dev_err(dev, "Unable to get poll-interval\n"); return -EINVAL; - } else { - joy->polled = true; } - /* - * Count how many channels we got. NULL terminated. - * Do not check the storage size if using polling. - */ - for (i = 0; joy->chans[i].indio_dev; i++) { - if (joy->polled) - continue; - bits = joy->chans[i].channel->scan_type.storagebits; - if (!bits || bits > 16) { - dev_err(dev, "Unsupported channel storage size\n"); - return -EINVAL; - } - if (bits != joy->chans[0].channel->scan_type.storagebits) { - dev_err(dev, "Channels must have equal storage size\n"); - return -EINVAL; - } - } - joy->num_chans = i; + error = adc_joystick_count_channels(dev, chans, poll_interval != 0, + &num_chans); + if (error) + return error; + + joy = devm_kzalloc(dev, struct_size(joy, axes, num_chans), GFP_KERNEL); + if (!joy) + return -ENOMEM; + + joy->chans = chans; + joy->num_chans = num_chans; input = devm_input_allocate_device(dev); if (!input) { @@ -273,7 +280,7 @@ static int adc_joystick_probe(struct platform_device *pdev) if (error) return error; - if (joy->polled) { + if (poll_interval != 0) { input_setup_polling(input, adc_joystick_poll); input_set_poll_interval(input, poll_interval); } else {
There is no need to allocate axes information separately from the main joystick structure so let's fold the allocation and also drop members (such as range, flat and fuzz) that are only used during initialization of the device. Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> --- v2: - fixed issue with uninitialized "axes" in adc_joystick_set_axes() pointed out by Dan Carpenter - fixed issue with checking wrong variable in adc_joystick_probe() pointed out by Dan Carpenter drivers/input/joystick/adc-joystick.c | 113 ++++++++++++++------------ 1 file changed, 60 insertions(+), 53 deletions(-)