<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Oct 13, 2016 at 10:59 AM, Xo Wang <span dir="ltr"><<a href="mailto:xow@google.com" target="_blank">xow@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">with Hi Jagha,<br>
<span class=""><br>
On Wed, Oct 12, 2016 at 5:41 PM, Jaghathiswari Rankappagounder<br>
Natarajan <<a href="mailto:jaghu@google.com">jaghu@google.com</a>> wrote:<br>
><br>
</span><span class="">> Platform driver to periodically update the display on the POST card.<br>
> POST card hardware assumed is 74HC164 wired to two 7-segment displays.<br>
><br>
> Signed-off-by: Jaghathiswari Rankappagounder Natarajan <<a href="mailto:jaghu@google.com">jaghu@google.com</a>><br>
> ---<br>
> drivers/misc/Kconfig | 6 ++<br>
> drivers/misc/Makefile | 1 +<br>
> drivers/misc/update-postcard.c | 219 ++++++++++++++++++++++++++++++<wbr>+++++++++++<br>
> drivers/misc/update-postcard.h | 1 +<br>
> 4 files changed, 227 insertions(+)<br>
> create mode 100644 drivers/misc/update-postcard.c<br>
> create mode 100644 drivers/misc/update-postcard.h<br>
<br>
</span>I agree with what Rick said. You should consider reframing the driver<br>
as a general purpose "bitbang shift register-attached 7-segment LEDs"<br>
driver and not mention POST codes in here.<br>
<br>
This will be more clear to other kernel reviewers because<br>
<br>
1) they probably don't know what a "POST card" is<br>
2) they're not thinking of BMCs, so they'll confused as to why Linux<br>
would want to display POST codes (usually precedes Linux)<br>
<span class=""><br>
><br>
><br>
> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig<br>
> index 4617ddc..d65b83f 100644<br>
> --- a/drivers/misc/Kconfig<br>
> +++ b/drivers/misc/Kconfig<br>
<br>
</span>Does drivers/misc make sense? Your driver is most similar to those<br>
described in <a href="http://lxr.free-electrons.com/source/Documentation/gpio/drivers-on-gpio.txt" rel="noreferrer" target="_blank">http://lxr.free-electrons.com/<wbr>source/Documentation/gpio/<wbr>drivers-on-gpio.txt</a><br>
<br>
I'm genuinely uncertain since I have little upstream Linux experience.</blockquote><div>I looked at the other drivers given in the link you mentioned. But they do not seem relevant. So I have put them in drivers/misc</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div class="h5">
> @@ -804,6 +804,12 @@ config PANEL_BOOT_MESSAGE<br>
> An empty message will only clear the display at driver init time. Any other<br>
> printf()-formatted message is valid with newline and escape codes.<br>
><br>
> +config ASPEED_UPDATE_POSTCARD<br>
> + tristate "Platform driver to update POST card"<br>
> + help<br>
> + Support to periodically update display on the POST card. The POST card<br>
> + hardware assumed is 74HC164 wired to two 7-segment displays.<br>
> +<br>
> config ASPEED_BT_IPMI_HOST<br>
> tristate "BT IPMI host driver"<br>
> help<br>
> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile<br>
> index 724861b..730c6c2 100644<br>
> --- a/drivers/misc/Makefile<br>
> +++ b/drivers/misc/Makefile<br>
> @@ -57,4 +57,5 @@ obj-$(CONFIG_ECHO) += echo/<br>
> obj-$(CONFIG_VEXPRESS_SYSCFG) += vexpress-syscfg.o<br>
> obj-$(CONFIG_CXL_BASE) += cxl/<br>
> obj-$(CONFIG_PANEL) += panel.o<br>
> +obj-$(CONFIG_ASPEED_UPDATE_<wbr>POSTCARD) += update-postcard.o<br>
> obj-$(CONFIG_ASPEED_BT_IPMI_<wbr>HOST) += bt-host.o<br>
> diff --git a/drivers/misc/update-<wbr>postcard.c b/drivers/misc/update-<wbr>postcard.c<br>
> new file mode 100644<br>
> index 0000000..3516c24<br>
> --- /dev/null<br>
> +++ b/drivers/misc/update-<wbr>postcard.c<br>
> @@ -0,0 +1,219 @@<br>
> +/* Platform driver to periodically update the display on the POST card.<br>
> + * POST card hardware assumed is 74HC164 wired to two 7-segment displays.<br>
> + */<br>
> +<br>
> +#include <linux/kernel.h><br>
> +#include <linux/module.h><br>
> +#include <linux/timer.h><br>
> +#include <linux/jiffies.h><br>
> +#include <linux/sizes.h><br>
> +#include <linux/map_to_7segment.h><br>
> +#include <linux/io.h><br>
> +#include <linux/delay.h><br>
> +#include <linux/uaccess.h><br>
> +#include <linux/mutex.h><br>
> +#include <linux/of_platform.h><br>
> +#include <linux/gpio/consumer.h><br>
> +<br>
> +#define POST_CODE_UPDATE_INTVL 1000<br>
<br>
</div></div>Maybe make this an optional device tree property with 1000 as the<br>
default value. Please also suffix with units (*_ms).<br>
<span class=""><br>
> +<br>
> +#define MAX_PC_SIZE 3<br>
> +<br>
> +#define LED_DOT 0x01<br>
> +<br>
> +#define CLOCK_GPIO_NAME "clk"<br>
> +#define DATA_GPIO_NAME "data"<br>
> +#define CLEAR_GPIO_NAME "clr"<br>
> +<br>
> +static struct mutex mutex;<br>
> +<br>
> +static struct timer_list update_timer;<br>
> +<br>
> +static char curr_postcode[3];<br>
> +static u8 pc_valid;<br>
> +<br>
> +static struct gpio_desc *clk_gpio;<br>
> +static struct gpio_desc *data_gpio;<br>
> +static struct gpio_desc *clr_gpio;<br>
<br>
</span>Agree that these should be wrapped up into a struct. It'll then be<br>
easier to turn this into a module or to support multiple shift<br>
register 7seg displays (not that we want those things right now).<br>
<div><div class="h5"><br>
> +<br>
> +/*<br>
> + * 0 1 2 3 4 5 6 7 8 9 A B C D E F<br>
> + * _ _ _ _ _ _ _ _ _ _ _ _<br>
> + * | | | _| _| |_| |_ |_ | |_| |_| |_| |_ | _| |_ |_<br>
> + * |_| | |_ _| | _| |_| | |_| | | | |_| |_ |_| |_ |<br>
> + *<br>
> + * data[7:1] = led[a:g]<br>
> + * lookup table used for both bytes of lpc post code and lower byte of<br>
> + * bmc post code<br>
> + */<br>
> +const u8 seven_seg_bits[] = {<br>
> + 0xFC, 0x60, 0xDA, 0xF2, 0x66, 0xB6, 0xBE, 0xE0,<br>
> + 0xFE, 0xF6, 0xEE, 0x3E, 0x9C, 0x7A, 0x9E, 0x8E<br>
> + };<br>
> +<br>
> +/*<br>
> + * 0 1 2 3 4 5 6 7 8 9 A B C D E F<br>
> + * _ _ _ _ _<br>
> + * | |_ |_| |_ _ _ _ _ _ _ _ |_ _| _| | |<br>
> + * |_ |_ | | _| |_| |_| | |<br>
> + *<br>
> + * data[7:1] = led[a:g]<br>
> + * lookup table used for higher byte of bmc post code<br>
> + */<br>
> +const u8 special_seven_seg_bits[] = {<br>
> + 0x00, 0x9C, 0x1E, 0xCE, 0x8E, 0x02, 0x02, 0x02,<br>
> + 0x02, 0x02, 0x02, 0x02, 0xB6, 0x7A, 0x7A, 0xEC<br>
> + };<br>
> +<br>
> +void update_postcode(char *buf)<br>
> +{<br>
> + mutex_lock(&mutex);<br>
> + strncpy(curr_postcode, buf, sizeof(curr_postcode));<br>
> + mutex_unlock(&mutex);<br>
> + pc_valid = 1;<br>
> +}<br>
> +EXPORT_SYMBOL(update_<wbr>postcode);<br>
> +<br>
> +/* convert postcode to led pattern<br>
> + * 7-bits used for each 7-seg display and<br>
> + * 1 bit used for the 'dot' on both digits.<br>
> + *<br>
> + * @param N/A<br>
> + * @return N/A<br>
> + */<br>
> +static u16 postcode_to_led_signal(void)<br>
> +{<br>
> + u8 low_display;<br>
> + u8 high_display;<br>
> + u16 led_value;<br>
> + char buf[3];<br>
> +<br>
> + mutex_lock(&mutex);<br>
> + strncpy(buf, curr_postcode, sizeof(buf));<br>
> + mutex_unlock(&mutex);<br>
> +<br>
> + low_display = seven_seg_bits[hex_to_bin(buf[<wbr>2])];<br>
> +<br>
> + high_display = (buf[0] == '1') ?<br>
> + special_seven_seg_bits[hex_to_<wbr>bin(buf[1])] :<br>
> + seven_seg_bits[hex_to_bin(buf[<wbr>1])];<br>
> +<br>
> + led_value = low_display | (high_display << 8);<br>
> + if (buf[0] == '1') {<br>
> + led_value |= LED_DOT | (LED_DOT << 8);<br>
> + }<br>
> +<br>
> + return led_value;<br>
> +}<br>
> +<br>
> +static void update_sgpio(void)<br>
<br>
</div></div>Since you're no longer using the Serial GPIO peripheral, please rename<br>
this to something more descriptive like "write_..._display"<br>
<div><div class="h5"><br>
> +{<br>
> + u16 led_signal;<br>
> + int i;<br>
> +<br>
> + /* Convert two bytes of post code to 16 bit led pattern */<br>
> + led_signal = postcode_to_led_signal();<br>
> +<br>
> + /* Set the clear signal to low */<br>
> + gpiod_set_value(clr_gpio, 0);<br>
> + udelay(1);<br>
> + /* Set the clear clear to high */<br>
> + gpiod_set_value(clr_gpio, 1);<br>
> + udelay(1);<br>
> +<br>
> + /* Bitbang the 16 bit led pattern<br>
> + * 7-bits used for each 7-seg display<br>
> + * 1 bit used for the 'dot' on both digits<br>
> + */<br>
> + for (i = 0; i < 16; i++) {<br>
> + if (led_signal & 0x01) {<br>
> + /* Set the data signal to high */<br>
> + gpiod_set_value(data_gpio, 1);<br>
> + } else {<br>
> + /* Set the data signal to low */<br>
> + gpiod_set_value(data_gpio, 0);<br>
> + }<br>
> + udelay(1);<br>
> +<br>
> + /* Set the clock signal to low */<br>
> + gpiod_set_value(clk_gpio, 0);<br>
> + udelay(1);<br>
<br>
</div></div>Please name this delay value as a constant (or make it configurable, up to you).<br>
<div><div class="h5"><br>
> + /* Set the clock signal to high */<br>
> + gpiod_set_value(clk_gpio, 1);<br>
> + udelay(1);<br>
> +<br>
> + led_signal >>= 1;<br>
> + }<br>
> +}<br>
> +<br>
> +static void update_timer_handler(unsigned long data)<br>
> +{<br>
> + if (pc_valid == 1) {<br>
> + update_sgpio();<br>
> + }<br>
> + mod_timer(&update_timer,<br>
> + jiffies + msecs_to_jiffies(POST_CODE_<wbr>UPDATE_INTVL));<br>
> +}<br>
> +<br>
> +static const struct of_device_id of_postcard_match[] = {<br>
> + { .compatible = "postcard-display" },<br>
> + {},<br>
> +};<br>
> +<br>
> +MODULE_DEVICE_TABLE(of, of_postcard_match);<br>
> +<br>
> +static int postcard_probe(struct platform_device *pdev)<br>
> +{<br>
> + int result;<br>
> + struct device *dev = &pdev->dev;<br>
> +<br>
> + /* Requesting the clock gpio */<br>
> + clk_gpio = devm_gpiod_get(dev, CLOCK_GPIO_NAME,<br>
> + GPIOD_OUT_HIGH);<br>
> + if (IS_ERR(clk_gpio))<br>
> + return PTR_ERR(data_gpio);<br>
> +<br>
> + /* Requesting the data gpio */<br>
> + data_gpio = devm_gpiod_get(dev, DATA_GPIO_NAME,<br>
> + GPIOD_OUT_HIGH);<br>
> + if (IS_ERR(data_gpio))<br>
> + return PTR_ERR(data_gpio);<br>
> +<br>
> + /* Requesting the clear gpio */<br>
> + clr_gpio = devm_gpiod_get(dev, CLEAR_GPIO_NAME,<br>
> + GPIOD_OUT_HIGH);<br>
> + if (IS_ERR(clr_gpio))<br>
> + return PTR_ERR(clr_gpio);<br>
> +<br>
> + /* Start timer to update post code every second */<br>
> + setup_timer(&update_timer, update_timer_handler, 0);<br>
> + result = mod_timer(&update_timer,<br>
> + jiffies + msecs_to_jiffies(POST_CODE_<wbr>UPDATE_INTVL));<br>
> +<br>
> + if (result)<br>
> + return result;<br>
> +<br>
> + mutex_init(&mutex);<br>
> +<br>
> + return 0;<br>
> +}<br>
> +<br>
> +static void postcard_remove(struct platform_device *pdev)<br>
> +{<br>
> + del_timer(&update_timer);<br>
> +}<br>
> +<br>
> +static struct platform_driver postcard_display_driver = {<br>
> + .probe = postcard_probe,<br>
> + .shutdown = postcard_remove,<br>
> + .driver = {<br>
> + .name = "postcard-display",<br>
> + .of_match_table = of_postcard_match,<br>
> + },<br>
> +};<br>
> +<br>
> +module_platform_driver(<wbr>postcard_display_driver);<br>
> +<br>
> +MODULE_LICENSE("GPL");<br>
> +MODULE_AUTHOR("Jaghathiswari Rankappagounder Natarajan <<a href="mailto:jaghu@google.com">jaghu@google.com</a>>");<br>
> +MODULE_DESCRIPTION("Post card display update driver");<br>
> diff --git a/drivers/misc/update-<wbr>postcard.h b/drivers/misc/update-<wbr>postcard.h<br>
> new file mode 100644<br>
> index 0000000..ef82e9091d<br>
> --- /dev/null<br>
> +++ b/drivers/misc/update-<wbr>postcard.h<br>
> @@ -0,0 +1 @@<br>
> +void update_postcode(char *buf);<br>
> --<br>
> 2.8.0.rc3.226.g39d4020<br>
><br>
</div></div><span class="">> ______________________________<wbr>_________________<br>
> openbmc mailing list<br>
> <a href="mailto:openbmc@lists.ozlabs.org">openbmc@lists.ozlabs.org</a><br>
> <a href="https://lists.ozlabs.org/listinfo/openbmc" rel="noreferrer" target="_blank">https://lists.ozlabs.org/<wbr>listinfo/openbmc</a><br>
<br>
</span>cheers<br>
<span class="HOEnZb"><font color="#888888">xo<br>
</font></span></blockquote></div><br></div></div>