Re: [PATCH v14 06/17] counter: Internalize sysfs interface code

[William Breathitt Gray <vilhelm.gray@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 7778 bytes --]

On Sun, Aug 08, 2021 at 06:23:25PM +0100, Jonathan Cameron wrote:
> On Tue,  3 Aug 2021 21:06:16 +0900
> William Breathitt Gray <vilhelm.gray@gmail.com> wrote:
> 
> > This is a reimplementation of the Generic Counter driver interface.
> > There are no modifications to the Counter subsystem userspace interface,
> > so existing userspace applications should continue to run seamlessly.
> > 
> > The purpose of this patch is to internalize the sysfs interface code
> > among the various counter drivers into a shared module. Counter drivers
> > pass and take data natively (i.e. u8, u64, etc.) and the shared counter
> > module handles the translation between the sysfs interface and the
> > device drivers. This guarantees a standard userspace interface for all
> > counter drivers, and helps generalize the Generic Counter driver ABI in
> > order to support the Generic Counter chrdev interface (introduced in a
> > subsequent patch) without significant changes to the existing counter
> > drivers.
> > 
> > Note, Counter device registration is the same as before: drivers
> > populate a struct counter_device with components and callbacks, then
> > pass the structure to the devm_counter_register function. However,
> > what's different now is how the Counter subsystem code handles this
> > registration internally.
> > 
> > Whereas before callbacks would interact directly with sysfs data, this
> > interaction is now abstracted and instead callbacks interact with native
> > C data types. The counter_comp structure forms the basis for Counter
> > extensions.
> > 
> > The counter-sysfs.c file contains the code to parse through the
> > counter_device structure and register the requested components and
> > extensions. Attributes are created and populated based on type, with
> > respective translation functions to handle the mapping between sysfs and
> > the counter driver callbacks.
> > 
> > The translation performed for each attribute is straightforward: the
> > attribute type and data is parsed from the counter_attribute structure,
> > the respective counter driver read/write callback is called, and sysfs
> > I/O is handled before or after the driver read/write function is called.
> 
> Hi William,
> 
> There are some issues with this even after I've fixed the merge conflict
> because of the earlier change..
> 
>  DESCEND objtool
>   CALL    scripts/atomic/check-atomics.sh
>   CALL    scripts/checksyscalls.sh
>   CHK     include/generated/compile.h
>   CC [M]  drivers/counter/counter-core.o
>   CC [M]  drivers/counter/counter-sysfs.o
>   CC [M]  drivers/counter/104-quad-8.o
>   CC [M]  drivers/counter/interrupt-cnt.o
>   CC [M]  drivers/counter/stm32-timer-cnt.o
>   CC [M]  drivers/counter/stm32-lptimer-cnt.o
> drivers/counter/counter-core.c: In function ‘counter_device_release’:
> drivers/counter/counter-core.c:23:9: error: implicit declaration of function ‘counter_chrdev_remove’ [-Werror=implicit-function-declaration]
>    23 |         counter_chrdev_remove(counter);
>       |         ^~~~~~~~~~~~~~~~~~~~~
> cc1: some warnings being treated as errors
> make[2]: *** [scripts/Makefile.build:271: drivers/counter/counter-core.o] Error 1
> make[2]: *** Waiting for unfinished jobs....
>   CHECK   drivers/counter/interrupt-cnt.c
>   CHECK   drivers/counter/104-quad-8.c
>   CHECK   drivers/counter/counter-sysfs.c
> drivers/counter/stm32-timer-cnt.c:55:9: warning: implicit conversion from ‘enum stm32_count_function’ to ‘enum counter_function’ [-Wenum-conversion]
>    55 |         STM32_COUNT_SLAVE_MODE_DISABLED,
>       |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> drivers/counter/stm32-timer-cnt.c:56:9: warning: implicit conversion from ‘enum stm32_count_function’ to ‘enum counter_function’ [-Wenum-conversion]
>    56 |         STM32_COUNT_ENCODER_MODE_1,
>       |         ^~~~~~~~~~~~~~~~~~~~~~~~~~
> drivers/counter/stm32-timer-cnt.c:57:9: warning: implicit conversion from ‘enum stm32_count_function’ to ‘enum counter_function’ [-Wenum-conversion]
>    57 |         STM32_COUNT_ENCODER_MODE_2,
>       |         ^~~~~~~~~~~~~~~~~~~~~~~~~~
> drivers/counter/stm32-timer-cnt.c:58:9: warning: implicit conversion from ‘enum stm32_count_function’ to ‘enum counter_function’ [-Wenum-conversion]
>    58 |         STM32_COUNT_ENCODER_MODE_3,
>       |         ^~~~~~~~~~~~~~~~~~~~~~~~~~
> drivers/counter/stm32-timer-cnt.c: In function ‘stm32_count_function_read’:
> drivers/counter/stm32-timer-cnt.c:97:27: warning: implicit conversion from ‘enum stm32_count_function’ to ‘enum counter_function’ [-Wenum-conversion]
>    97 |                 *function = STM32_COUNT_SLAVE_MODE_DISABLED;
>       |                           ^
> drivers/counter/stm32-timer-cnt.c:100:27: warning: implicit conversion from ‘enum stm32_count_function’ to ‘enum counter_function’ [-Wenum-conversion]
>   100 |                 *function = STM32_COUNT_ENCODER_MODE_1;
>       |                           ^
> drivers/counter/stm32-timer-cnt.c:103:27: warning: implicit conversion from ‘enum stm32_count_function’ to ‘enum counter_function’ [-Wenum-conversion]
>   103 |                 *function = STM32_COUNT_ENCODER_MODE_2;
>       |                           ^
> drivers/counter/stm32-timer-cnt.c:106:27: warning: implicit conversion from ‘enum stm32_count_function’ to ‘enum counter_function’ [-Wenum-conversion]
>   106 |                 *function = STM32_COUNT_ENCODER_MODE_3;
>       |                           ^
> drivers/counter/stm32-lptimer-cnt.c:139:9: warning: implicit conversion from ‘enum stm32_lptim_cnt_function’ to ‘enum counter_function’ [-Wenum-conversion]
>   139 |         STM32_LPTIM_COUNTER_INCREASE,
>       |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
> drivers/counter/stm32-lptimer-cnt.c:140:9: warning: implicit conversion from ‘enum stm32_lptim_cnt_function’ to ‘enum counter_function’ [-Wenum-conversion]
>   140 |         STM32_LPTIM_ENCODER_BOTH_EDGE,
>       |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> drivers/counter/stm32-lptimer-cnt.c: In function ‘stm32_lptim_cnt_function_read’:
> drivers/counter/stm32-lptimer-cnt.c:195:27: warning: implicit conversion from ‘enum stm32_lptim_cnt_function’ to ‘enum counter_function’ [-Wenum-conversion]
>   195 |                 *function = STM32_LPTIM_COUNTER_INCREASE;
>       |                           ^
> drivers/counter/stm32-lptimer-cnt.c:200:27: warning: implicit conversion from ‘enum stm32_lptim_cnt_function’ to ‘enum counter_function’ [-Wenum-conversion]
>   200 |                 *function = STM32_LPTIM_ENCODER_BOTH_EDGE;
>       |                           ^
> drivers/counter/104-quad-8.c:58: warning: Function parameter or member 'lock' not described in 'quad8'
>   CHECK   drivers/counter/stm32-timer-cnt.c
>   CHECK   drivers/counter/stm32-lptimer-cnt.c
> make[1]: *** [scripts/Makefile.build:514: drivers/counter] Error 2
> make: *** [Makefile:1841: drivers] Error 2
> 
> I could wait through these but it feels a little to likely I'll mess something up fixing things
> so please fix the errors and warnings this patch introduces
> with a W=1 C=1 build.
> 
> Also, do a patch by patch build of the rest of the set in case there are any other cases
> like this hiding.  It's very easy for these to build up when you have a lot of
> versions of a patch series.  I've been caught out a few times myself!
> 
> In the meantime, I'll keep the first 5 patches on my tree unless you shout otherwise.
> 
> Thanks,
> 
> Jonathan

Thanks,

I'll fix this up and submit a v15 to make sure these patches merge in
smoothly.

William Breathitt Gray

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]