Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20200417202859.35427-1-contact@artur-rojek.eu> <20200417202859.35427-5-contact@artur-rojek.eu>
In-Reply-To: <20200417202859.35427-5-contact@artur-rojek.eu>
From:   Andy Shevchenko <andy.shevchenko@gmail.com>
Date:   Sat, 18 Apr 2020 00:10:49 +0300
Message-ID: <CAHp75VfRbnnuUhfyXpu+5dp4TutHSrHus=sX_vG_5F0dX4k0fQ@mail.gmail.com>
Subject: Re: [RESEND PATCH v5 5/5] input: joystick: Add ADC attached joystick driver.
To:     Artur Rojek <contact@artur-rojek.eu>
Cc:     Dmitry Torokhov <dmitry.torokhov@gmail.com>,
        Rob Herring <robh+dt@kernel.org>,
        Mark Rutland <mark.rutland@arm.com>,
        Jonathan Cameron <jic23@kernel.org>,
        Paul Cercueil <paul@crapouillou.net>,
        Heiko Stuebner <heiko@sntech.de>,
        linux-input <linux-input@vger.kernel.org>,
        devicetree <devicetree@vger.kernel.org>,
        linux-iio <linux-iio@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Fri, Apr 17, 2020 at 11:21 PM Artur Rojek <contact@artur-rojek.eu> wrote:
>
> Add a driver for joystick devices connected to ADC controllers
> supporting the Industrial I/O subsystem.

...

> +#include <linux/of.h>

Do you really need this? (See below as well)

...

> +               sign = (tolower(joy->chans[i].channel->scan_type.sign) == 's');

Too many parentheses. But here it's up to you,

...

> +               case 2:

> +                       val = ((const u16 *)data)[i];

Can't you do this in each branch below?

> +                       if (endianness == IIO_BE)
> +                               val = be16_to_cpu(val);
> +                       else if (endianness == IIO_LE)
> +                               val = le16_to_cpu(val);
> +                       break;

...

> +       device_for_each_child_node(dev, child) {
> +               ret = fwnode_property_read_u32(child, "reg", &i);
> +               if (ret || i >= num_axes) {
> +                       dev_err(dev, "reg invalid or missing");
> +                       goto err;
> +               }
> +
> +               if (fwnode_property_read_u32(child, "linux,code",
> +                                            &axes[i].code)) {
> +                       dev_err(dev, "linux,code invalid or missing");
> +                       goto err;
> +               }
> +
> +               if (fwnode_property_read_u32_array(child, "abs-range",
> +                                                  axes[i].range, 2)) {
> +                       dev_err(dev, "abs-range invalid or missing");
> +                       goto err;
> +               }

> +       }
> +
> +       joy->axes = axes;
> +
> +       return 0;
> +
> +err:
> +       fwnode_handle_put(child);

> +       return -EINVAL;

Can we avoid shadowing the actual error code?

...

> +       bits = joy->chans[0].channel->scan_type.storagebits;

> +       if (!bits || (bits >> 3) > 2) {

Wouldn't be clear to use simple 'bits > 16'?

> +               dev_err(dev, "Unsupported channel storage size");
> +               return -EINVAL;
> +       }

...

> +static const struct of_device_id adc_joystick_of_match[] = {
> +       { .compatible = "adc-joystick", },
> +       { },
> +};
> +MODULE_DEVICE_TABLE(of, adc_joystick_of_match);
> +
> +static struct platform_driver adc_joystick_driver = {
> +       .driver = {
> +               .name = "adc-joystick",

> +               .of_match_table = of_match_ptr(adc_joystick_of_match),

Drop this a bit harmful of_match_ptr() macro. It should go with ugly
#ifdeffery. Here you simple introduced a compiler warning.
On top of that, you are using device property API, OF use in this case
is contradictory (at lest to some extend).

> +       },
> +       .probe = adc_joystick_probe,
> +};

-- 
With Best Regards,
Andy Shevchenko