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@xxxxxxxxx>
> > ---
> >
> > 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@xxxxxxxxxxxxxx>
Thanks.
--
Dmitry
