Re: [Xen-devel] [PATCH V2 1/3] xen/arm: drivers: scif: Add support for SCIFA compatible UARTs

[Oleksandr <olekstysh@xxxxxxxxx>]

On 18.02.19 16:00, Julien Grall wrote:
> Hi,
Hi


> On 01/02/2019 12:37, Oleksandr Tyshchenko wrote:
> > From: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
> >
> > Extend existing driver to be able to handle SCIFA interface as well.
> In general a patch should do only one thing. In this case, this should 
> have been split in 2 patches: one to extend the driver, the second to 
> add support for SCIFA.
> Please split the patch accordingly.

Not entirely clear to me how the current patch should be split...

- The first patch will be just a copy of the current patch, but without 
new compatible string (SCIFA).
- The second patch will add new compatible string.

Did I get your point correctly?


> > SCIF and SCIFA have lot in common, though SCIFA has different
> > offsets and bits for some registers.
> >
> > The "data" field in struct dt_device_match is used for recognizing
> > what interface is present on a target board.
> >
> > Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
> >
> > ---
> >      Changes in v2:
> >          - Name a enum for describing interfaces this driver supports
> >          - Use local variable for "params" where appropriate
> >          - Use "data" field in struct dt_device_match instead of calling
> >            dt_device_is_compatible()
> >          - Don't check for "overrun_reg != status_reg" condition during
> >            initialization
> > ---
> >   xen/drivers/char/scif-uart.c    | 139 
> > ++++++++++++++++++++++++++++++----------
> >   xen/include/asm-arm/scif-uart.h |  44 +++++++++++--
> >   2 files changed, 142 insertions(+), 41 deletions(-)
> > +static const struct dt_device_match scif_uart_dt_match[] __initconst =
> > +{
> > +    { .compatible = "renesas,scif",  .data = (void *)SCIF_PORT },
> > +    { .compatible = "renesas,scifa", .data = (void *)SCIFA_PORT },
> The port_type is pretty much only used in scif_uart_init. So how about 
> passing a pointer to the structure directly?
Agree. Will do.


> Cheers,
--
Regards,

Oleksandr Tyshchenko


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel