Skip to content

Conversation

@rwana1753
Copy link

The driver craetes files for TM1637 LED contoller under sysfs
to control TM1637 LED controller with keyscan over gpio.

The driver craetes files for TM1637 LED contoller under sysfs
to control TM1637 LED controller with keyscan over gpio.
@pelwell
Copy link
Contributor

pelwell commented Nov 30, 2021

There are many problems with this PR:

  1. The biggest problem is that the driver should be sent upstream to the Linux kernel maintainers. The get_maintainers script suggests sending it to:
    Miguel Ojeda Sandonis [email protected]
    [email protected]
  2. DEBUG is still enabled on the driver - that won't go down well upstream.
  3. This PR is against 4.19 - we're currently on 5.10. and soon to move to 5.15.
  4. The overlay and README look as if they have been edited by different people or different editors with different indentation settings. The README rules are no TABs - just spaces - but elsewhere I just expect consistency.

Given all those problems, and the fact that the driver doesn't expose any standard kernel interfaces (only sysfs), wouldn't it be easier to use an external program to control the GPIOs via libgpiod? There's even a Python module for it.

@pelwell pelwell added Close within 30 days Issue will be closed within 30 days unless requested to stay open Waiting for external input Waiting for a comment from the originator of the issue, or a collaborator. labels Dec 2, 2021
if (kstrtoul(buf, 0, &data))
return -EINVAL;
ret = -EINVAL;
goto unlock;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You've not tested this - this goto is unconditional.

But you're wasting your time - as I said previously:

The biggest problem is that the driver should be sent upstream to the Linux kernel maintainers.

Copy link
Author

@rwana1753 rwana1753 Dec 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello

I also found an error in the code part that returns the status value,
so I corrected it about an hour ago and reflected it.

And when controlling using sysfs, high-speed operation is not possible,
so misc is added, and this part is still before the test.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No - not sysfs, libgpiod which is ioctl-based and much quicker. How fast does one need to update a 4x7-segment display?

@raspberrypi raspberrypi deleted a comment Feb 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Close within 30 days Issue will be closed within 30 days unless requested to stay open Waiting for external input Waiting for a comment from the originator of the issue, or a collaborator.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants