Re: [PATCH 6/7] serial: 8250_dw: Add support for RZ/N1 DMA

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Thu, Mar 10, 2022 at 08:27:43PM +0100, Miquel Raynal wrote:
> andriy.shevchenko@linux.intel.com wrote on Thu, 10 Mar 2022 20:25:52
> +0200:
> > On Thu, Mar 10, 2022 at 05:16:49PM +0100, Miquel Raynal wrote:

...

> > > +/* Offsets for the Renesas RZ/N1 DesignWare specific registers */
> > > +/* DMA Software Ack */
> > > +#define RZN1_UART_DMASA			0xa8  
> > 
> > Is it specific to Renesas? IIRC it's Synopsys DesignWare register, makes
> > sense to use appropriate prefix or no prefix.
> 
> I have no idea, I can use a more common prefix.

It's a register described in Synopsys DesignWare specification. It's not
related to Renesas IP integration.

...

> > > +#define RZN1_UART_xDMACR_1_WORD_BURST	0
> > > +#define RZN1_UART_xDMACR_4_WORD_BURST	BIT(1)
> > > +#define RZN1_UART_xDMACR_8_WORD_BURST	(BIT(1) | BIT(2))  
> > 
> > This looks like incorrect use of BIT() macro.
> > Please, use plain decimal integers. Something like
> > 
> > 	1	(0 << 1)
> > 	4	(1 << 1)
> > 	8	(3 << 1)
> > 
> > If I'm mistaken, describe the meaning of each bit there.
> 
> Matter of taste, I believe, whatever.

Actually no, when one uses BIT() masks it implies that in the datasheet each
bit is meaningful. So, please clarify this and update accordingly.

...

> > > +	data->is_rzn1 = of_device_is_compatible(dev->of_node, "renesas,rzn1-uart");
> > 
> > Device property API.
> 
> I'm not sure to get what you mean here again. The Information is in the
> device tree, the compatible string already gives us what we need, why
> would we need a device property? (or perhaps I misunderstand what
> "device property API" means)

Use non-OF APIs, which usually starts with device_property_ or fwnode_.
But Geert already suggested something better.

> > >  	/* Always ask for fixed clock rate from a property. */
> > >  	device_property_read_u32(dev, "clock-frequency", &p->uartclk);

-- 
With Best Regards,
Andy Shevchenko