Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 22 Jun 2023 16:39:43 +0300
From:   Andy Shevchenko <andriy.shevchenko@intel.com>
To:     Martin =?utf-8?B?WmHFpW92acSN?= <m.zatovic1@gmail.com>
Cc:     linux-kernel@vger.kernel.org, robh+dt@kernel.org,
        krzysztof.kozlowski+dt@linaro.org, conor+dt@kernel.org,
        gregkh@linuxfoundation.org, beanhuo@micron.com,
        nipun.gupta@amd.com, linus.walleij@linaro.org, mwen@igalia.com,
        bvanassche@acm.org, arnd@arndb.de, ogabbay@kernel.org,
        linux@zary.sk, jacek.lawrynowicz@linux.intel.com,
        geert+renesas@glider.be, benjamin.tissoires@redhat.com,
        masahiroy@kernel.org, yangyicong@hisilicon.com,
        devicetree@vger.kernel.org
Subject: Re: [PATCHv4 4/4] wiegand: add Wiegand GPIO bitbanged controller
 driver
Message-ID: <ZJRPH8YFV0ieCxHi@smile.fi.intel.com>
References: <20230510162243.95820-1-m.zatovic1@gmail.com>
 <20230510162243.95820-5-m.zatovic1@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20230510162243.95820-5-m.zatovic1@gmail.com>
Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo
Precedence: bulk

On Wed, May 10, 2023 at 06:22:43PM +0200, Martin Zaťovič wrote:
> This controller formats the data to a Wiegand format and bit-bangs
> the message on devicetree defined GPIO lines.
> 
> Several attributes need to be defined in the devicetree in order
> for this driver to work, namely the data-hi-gpios, data-lo-gpios,
> pulse-len, frame-gap and interval-len. These attributes are
> documented in the devicetree bindings documentation files.
> 
> The driver creates a dev file for writing messages on the bus.
> It also creates a sysfs file to control the payload length of
> messages(in bits). If a message is shorter than the set payload
> length, it will be discarded. On the other hand, if a message is
> longer, the additional bits will be stripped off.

...

> +Date:		May 2023

Taking into account the estimated release date I think this should be changed
to Aug 2023.

...

> +#include <linux/delay.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/poll.h>
> +#include <linux/slab.h>
> +#include <linux/wiegand.h>

...

> +#define UP_TO_100_USEC_DEVIATION 1
> +#define MORE_THAN_100_USEC_DEVIATION 3

These require some comments. And maybe better naming (depending on the content
of those comments).

...

> +static ssize_t store_ulong(u32 *val, const char *buf, size_t size, unsigned long max)
> +{
> +	int rc;
> +	u32 new;
> +
> +	rc = kstrtou32(buf, 0, &new);
> +	if (rc)
> +		return rc;
> +
> +	if (new > max)
> +		return -EINVAL;

ERANGE?

> +	*val = new;
> +	return size;
> +}

...

> +	if (sleep_len < 10)
> +		udelay(sleep_len);
> +	else if (sleep_len < 100)
> +		usleep_range(sleep_len - UP_TO_100_USEC_DEVIATION,
> +			     sleep_len + UP_TO_100_USEC_DEVIATION);
> +	else
> +		usleep_range(sleep_len - MORE_THAN_100_USEC_DEVIATION,
> +			     sleep_len + MORE_THAN_100_USEC_DEVIATION);

NIH fsleep()

...

> +static int wiegand_gpio_write_by_bits(struct wiegand_gpio *wiegand_gpio, u16 bitlen)
> +{
> +	size_t i;
> +	bool bit_value, is_last_bit;
> +
> +	for (i = 0; i < bitlen; i++) {
> +		bit_value = test_bit(i, wiegand_gpio->ctlr->data_bitmap);

> +		is_last_bit = (i + 1) == bitlen;

This is idempotent from for-loop, so...

> +		wiegand_gpio_send_bit(wiegand_gpio, bit_value, is_last_bit);
> +	}

	unsigned int i;
	bool value;

	if (bitlen == 0)
		return 0;

	for (i = 0; i < bitlen - 1; i++) {
		value = test_bit(i, wiegand_gpio->ctlr->data_bitmap);
		wiegand_gpio_send_bit(wiegand_gpio, value, false);
	}

	value = test_bit(bitlen - 1, wiegand_gpio->ctlr->data_bitmap);
	wiegand_gpio_send_bit(wiegand_gpio, value, true);

> +	return 0;
> +}

...

> +static int wiegand_gpio_request(struct device *dev, struct wiegand_gpio *wiegand_gpio)
> +{
> +	wiegand_gpio->data0_gpio = devm_gpiod_get(dev, "data-lo", GPIOD_OUT_HIGH);
> +	if (IS_ERR(wiegand_gpio->data0_gpio))
> +		return PTR_ERR(wiegand_gpio->data0_gpio);
> +
> +	wiegand_gpio->data1_gpio = devm_gpiod_get(dev, "data-hi", GPIOD_OUT_HIGH);
> +	return PTR_ERR_OR_ZERO(wiegand_gpio->data1_gpio);

Maybe you can use devm_gpiod_get_array()?

> +}

...

> +static int wiegand_gpio_probe(struct platform_device *device)
> +{

> +	int status = 0;

Redundant assignment.

> +	struct wiegand_controller *primary;
> +	struct wiegand_gpio *wiegand_gpio;
> +	struct device *dev = &device->dev;

...

> +	primary->payload_len = 26; // set standard 26-bit format

Instead of comment, make a self-explanatory definition?

...

> +	status = wiegand_register_controller(primary);
> +	if (status)
> +		dev_err_probe(wiegand_gpio->dev, status, "failed to register primary\n");

Why out of a sudden it uses this device and not real one?

> +	return status;

With above

		return dev_err_probe(dev, status, "failed to register primary\n");

	return 0;

> +}

...

> +static const struct of_device_id wiegand_gpio_dt_idtable[] = {
> +	{ .compatible = "wiegand-gpio", },

Inner comma is not needed.

> +	{}
> +};

...

> +static struct platform_driver wiegand_gpio_driver = {
> +	.driver = {
> +		.name	= "wiegand-gpio",
> +		.of_match_table = wiegand_gpio_dt_idtable,
> +		.dev_groups = wiegand_gpio_groups

Leave trailing comma when it's not about termination.

> +	},
> +	.probe		= wiegand_gpio_probe

Ditto.

> +};

-- 
With Best Regards,
Andy Shevchenko