Re: [PATCH v2] Input: adc-joystick - move axes data into the main structure

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

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.

-- 
Dmitry