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:   Wed, 4 Jan 2023 18:59:00 +0200
From:   Andy Shevchenko <andriy.shevchenko@intel.com>
To:     Martin =?utf-8?B?WmHFpW92acSN?= <m.zatovic1@gmail.com>
Cc:     linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
        mani@kernel.org, hemantk@codeaurora.org, quic_jhugo@quicinc.com,
        andersson@kernel.org, Michael.Srba@seznam.cz, arnd@arndb.de,
        dipenp@nvidia.com, bvanassche@acm.org, iwona.winiarska@intel.com,
        ogabbay@kernel.org, tzimmermann@suse.de, fmdefrancesco@gmail.com,
        jason.m.bills@linux.intel.com, jae.hyun.yoo@linux.intel.com,
        gregkh@linuxfoundation.org, krzysztof.kozlowski+dt@linaro.org,
        robh+dt@kernel.org
Subject: Re: [PATCH 3/3] wiegand: add Wiegand GPIO bit-banged controller
 driver
Message-ID: <Y7WwVCqDCXFrTqR9@smile.fi.intel.com>
References: <20230104133414.39305-1-m.zatovic1@gmail.com>
 <20230104133414.39305-4-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: <20230104133414.39305-4-m.zatovic1@gmail.com>
Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo
Precedence: bulk

On Wed, Jan 04, 2023 at 02:34:14PM +0100, Martin Zaťovič wrote:
> This controller formats the data to a Wiegand format - appends
> checksum if one of the defined formats is used - 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 binding documentation file.
> 
> The driver creates a dev file for writing messages on the bus.
> It also creates two sysfs files to control the format and payload
> length of messages. Defined formats are 26, 36 and 37-bit, meaning
> the payloads for these formats are 24, 34 and 35 bit respectively,
> as two bits are allocated for checksum. A custom format is also
> supported and it is set by writing 0 to the format sysfs file.
> Custom format does not calculate and append checksum to messages -
> they are bit-banged as inputted.

Brief look at the code makes me think that this is something from 10 years ago
with slightly removed dust to make it compile. So far I have noticed:
- explicit castings where it's not needed
- bad indentation here and there
- using direct dereference in the cases when we have specific getters available
- NIH this and that

...

> +What:		/sys/devices/platform/.../wiegand-gpio-attributes/format
> +Date:		January 2023
> +Contact:	Martin Zaťovič <m.zatovic1@gmail.com>
> +Description:
> +		Read/set Wiegand communication format.
> +		0 - custom format, payload length is specified by
> +		payload_len file
> +		26 - 26-bit format (24 bit payload, 2 bits checksum)
> +		36 - 36-bit format (34 bit payload, 2 bits checksum)
> +		37 - 37-bit format (35 bit payload, 2 bits checksum)
> +
> +What:		/sys/devices/platform/.../wiegand-gpio-attributes/payload_len
> +Date:		January 2023
> +Contact:	Martin Zaťovič <m.zatovic1@gmail.com>
> +Description:
> +		Read/set Wiegand communication payload length. File is only
> +		writable if custom format is set.

Why all these attributes? What is special about them and how they are specific
to the hardware in question?

To me it all sounds like layering violation: a GPIO driver that has to be
generic provides a complete custom ABIs which we usually put on the upper
layer (in the kernel as a child driver or in the user space).

-- 
With Best Regards,
Andy Shevchenko