[PATCH v2 1/3] drivers/tty/serial/8250: Make Aspeed VUART SIRQ polarity configurable

[Oskar Senft]

Hi Jeremy

Thanks for your comments, they were really helpful!


> > +What:
> /sys/bus/platform/drivers/aspeed-vuart/*/sirq_polarity
> > +Date:                July 2019
> > +Contact:     Oskar Senft <osk at google.com>
> > +Description: Configures the polarity of the serial interrupt to the
> > +             host via the BMC LPC bus.
>
> Can you mention what the value represents? 1/0 don't really indicate a
> specific polarity.
>
Good point. Not sure why I didn't do that initially.


> > @@ -310,6 +379,7 @@ static int aspeed_vuart_probe(struct platform_device
> *pdev)
> >       struct resource *res;
> >       u32 clk, prop;
> >       int rc;
> > +     struct of_phandle_args espi_enabled_args;
> Minor: can you reverse-christmas-tree this?
>
Oops, yeah. Sorry! Conflicting coding styles in my head got confused.


> > +     rc = of_parse_phandle_with_fixed_args(
> > +             np, "espi-enabled", 2, 0, &espi_enabled_args);
> > +     if (rc < 0) {
> > +             dev_warn(&pdev->dev, "espi-enabled property not found\n");
> That may just be a matter of changing this to dev_debug.
>
That was my intent, sorry for that.

I'll send v3 with those changes in a few minutes.

Thanks
Oskar.


On Wed, Sep 4, 2019 at 9:14 PM Jeremy Kerr <jk at ozlabs.org> wrote:

> Hi Oskar,
>
> Looks good to me, some minor comments though:
>
> > +
> > +What:
> /sys/bus/platform/drivers/aspeed-vuart/*/sirq_polarity
> > +Date:                July 2019
> > +Contact:     Oskar Senft <osk at google.com>
> > +Description: Configures the polarity of the serial interrupt to the
> > +             host via the BMC LPC bus.
>
> Can you mention what the value represents? 1/0 don't really indicate a
> specific polarity.
>
> Alternatively, we could use descriptive values (say, "active-low" /
> "idle-low").
>
> > @@ -310,6 +379,7 @@ static int aspeed_vuart_probe(struct platform_device
> *pdev)
> >       struct resource *res;
> >       u32 clk, prop;
> >       int rc;
> > +     struct of_phandle_args espi_enabled_args;
>
> Minor: can you reverse-christmas-tree this?
>
> > @@ -402,6 +472,18 @@ static int aspeed_vuart_probe(struct
> platform_device *pdev)
> >
> >       vuart->line = rc;
> >
> > +     rc = of_parse_phandle_with_fixed_args(
> > +             np, "espi-enabled", 2, 0, &espi_enabled_args);
> > +     if (rc < 0) {
> > +             dev_warn(&pdev->dev, "espi-enabled property not found\n");
>
> In the binding spec, you've listed this property at optional, but here
> we dev_warn() if its not present. Can we default to existing behaviour
> if it's not there?
>
> That may just be a matter of changing this to dev_debug.
>
> Cheers,
>
>
> Jeremy
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.ozlabs.org/pipermail/openbmc/attachments/20190905/a92748e8/attachment.htm>