<div dir="ltr"><div dir="ltr">Hi Jeremy</div><div dir="ltr"><br></div><div>Thanks for your comments, they were really helpful!</div><div class="gmail_quote"><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">> +What:                /sys/bus/platform/drivers/aspeed-vuart/*/sirq_polarity<br>
> +Date:                July 2019<br>
> +Contact:     Oskar Senft <<a href="mailto:osk@google.com" target="_blank">osk@google.com</a>><br>
> +Description: Configures the polarity of the serial interrupt to the<br>
> +             host via the BMC LPC bus.<br>
<br>
Can you mention what the value represents? 1/0 don't really indicate a<br>
specific polarity.<br></blockquote><div>Good point. Not sure why I didn't do that initially.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">> @@ -310,6 +379,7 @@ static int aspeed_vuart_probe(struct platform_device *pdev)<br>
>       struct resource *res;<br>
>       u32 clk, prop;<br>
>       int rc;<br>
> +     struct of_phandle_args espi_enabled_args;<br>
Minor: can you reverse-christmas-tree this?<br></blockquote><div>Oops, yeah. Sorry! Conflicting coding styles in my head got confused.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">> +     rc = of_parse_phandle_with_fixed_args(<br>
> +             np, "espi-enabled", 2, 0, &espi_enabled_args);<br>
> +     if (rc < 0) {<br>
> +             dev_warn(&pdev->dev, "espi-enabled property not found\n");<br>That may just be a matter of changing this to dev_debug.<br></blockquote><div>That was my intent, sorry for that.</div><div><br></div><div>I'll send v3 with those changes in a few minutes.</div><div><br></div><div>Thanks</div><div>Oskar.</div><div><br></div></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Sep 4, 2019 at 9:14 PM Jeremy Kerr <<a href="mailto:jk@ozlabs.org">jk@ozlabs.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Oskar,<br>
<br>
Looks good to me, some minor comments though:<br>
<br>
> +<br>
> +What:                /sys/bus/platform/drivers/aspeed-vuart/*/sirq_polarity<br>
> +Date:                July 2019<br>
> +Contact:     Oskar Senft <<a href="mailto:osk@google.com" target="_blank">osk@google.com</a>><br>
> +Description: Configures the polarity of the serial interrupt to the<br>
> +             host via the BMC LPC bus.<br>
<br>
Can you mention what the value represents? 1/0 don't really indicate a<br>
specific polarity.<br>
<br>
Alternatively, we could use descriptive values (say, "active-low" /<br>
"idle-low").<br>
<br>
> @@ -310,6 +379,7 @@ static int aspeed_vuart_probe(struct platform_device *pdev)<br>
>       struct resource *res;<br>
>       u32 clk, prop;<br>
>       int rc;<br>
> +     struct of_phandle_args espi_enabled_args;<br>
<br>
Minor: can you reverse-christmas-tree this?<br>
<br>
> @@ -402,6 +472,18 @@ static int aspeed_vuart_probe(struct platform_device *pdev)<br>
>  <br>
>       vuart->line = rc;<br>
>  <br>
> +     rc = of_parse_phandle_with_fixed_args(<br>
> +             np, "espi-enabled", 2, 0, &espi_enabled_args);<br>
> +     if (rc < 0) {<br>
> +             dev_warn(&pdev->dev, "espi-enabled property not found\n");<br>
<br>
In the binding spec, you've listed this property at optional, but here<br>
we dev_warn() if its not present. Can we default to existing behaviour<br>
if it's not there?<br>
<br>
That may just be a matter of changing this to dev_debug.<br>
<br>
Cheers,<br>
<br>
<br>
Jeremy<br>
<br>
</blockquote></div>