RE: [PATCH 1/2] pinctrl: Add RZ/A2 pin and gpio controller

[Chris Brandt <Chris.Brandt@xxxxxxxxxxx>]

Hi Jacopo,

>    thanks for the patches.
Thanks for the review!

On Thursday, October 18, 2018, jacopo mondi wrote:

> > + * Combined GPIO and pin controller support for Renesas RZ/A2
> (R7S72100) SoC
> 
> R7S9210

Thanks.
hmm,  I wonder what pinctrl driver I copied that from... ;)

> > +#include <linux/gpio.h>
> > +#include <linux/bitops.h>
> 
> Headers sorted please :)

OK, sorted.


> > +#define RZA2_NPORTS		(PM + 1)
> 
> Why not making this part of the enum itself?

Good idea.

> > +#define MUX_FUNC(pinconf)	((pinconf & MUX_FUNC_MASK) >> MUX_FUNC_OFFS)
> > +#define MUX_FUNC(pinconf)	((pinconf & MUX_FUNC_MASK) >> MUX_FUNC_OFFS)
> 
> Seems like the same line to me.
> Also, double empty line.

Oops, thanks.


> > +enum pfc_pin_port_name {P0, P1, P2, P3, P4, P5, P6, P7, P8, P9, PA, PB,
> PC, PD,
> > +			PE, PF, PG, PH,	    PJ, PK, PL, PM};
> weird spacing                   ^^^^

I was doing that to point out that there is no "I" port (they skipped 
over it). But if you didn't realize that, then it is not obvious, so I'll 
just format it normally.




> > +static const char port_names[] = "0123456789ABCDEFGHJKLM";
> > +
> > +struct rza2_pinctrl_priv {
> > +	struct device *dev;
> > +	void __iomem *base;
> > +
> > +	struct pinctrl_pin_desc *pins;
> > +	struct pinctrl_desc desc;
> > +	struct pinctrl_dev *pctl;
> > +};
> > +
> > +#define PDR_BASE(b)	(b + 0x0000)	/* 16-bit, 2 bytes apart */
> 
> () around macros parameters when used:
>                         ((b) + 0x0000)
>                         which is just (b) by the way :)

OK.
The "+ 0x0000" was more for documentation reason.

> Also, please prefix all defines with RZA2_ not to pollute the global
> symbols space.


OK.

> > +#define PODR_BASE(b)	(b + 0x0040)	/* 8-bit, 1 byte apart */
> > +#define PIDR_BASE(b)	(b + 0x0060)	/* 8-bit, 1 byte apart */
> > +#define PMR_BASE(b)	(b + 0x0080)	/* 8-bit, 1 byte apart */
> > +#define DSCR_BASE(b)	(b + 0x0140)	/* 16-bit, 2 bytes apart */
> > +#define PFS_BASE(b)	(b + 0x0200)	/* 8-bit, 8 bytes apart */
> 
> Here you define registers address bases, but instead of computing
> everytime the port-pin dedicated register address (I'm thinking about
> PFS specifically but it applies to others), would you consider providing
> accessors helpers to write the offset computations process just once here?
> 
> Eg.
> #define RZA2_PFS(_b, _port, _pin) (RZA2_PFS_BASE(_b) + (_port) * 8 +
> (_pin))
> 
> Same for other register which needs offset calculation based on port
> and pin values.

I like that idea. Thanks.


> > +#define PWPR(b)		(b + 0x02FF)	/* 8-bit */
> > +#define PFENET(b)	(b + 0x0820)	/* 8-bit */
> > +#define PPOC(b)		(b + 0x0900)	/* 32-bit */
> > +#define PHMOMO(b)	(b + 0x0980)	/* 32-bit */
> > +#define PMODEPFS(b)	(b + 0x09C0)	/* 32-bit */
> > +#define PCKIO(b)	(b + 0x09D0)	/* 8-bit */
> 
> > +
> > +void rza2_set_pin_function(void __iomem *pfc_base, u8 port, u8 pin, u8
> func)
> static ?

I actually left that as global for a reason (because for some testing, I
need a way to set pin directly in a non-standard way).
But, I might as well make it static and just use a different hack for my
testing.

> > +	/* Set pin to 'Non-use (Hi-z input protection)'  */
> > +	reg16 = readw(PDR_BASE(pfc_base) + (port * 2));
> 
> as in example,
> 
>         reg16 = readw(RZA2_PDR(pfc_base), port));
> 
> it's nicer and isolates the offset calculations, which differs
> from one register to another (the 'x bytes apart' comment you added in
> the registers definitions represents this, right? )

Yup, that's what my code looks like now after the change :)

> > +	if (dir == GPIOF_DIR_IN)
> > +		reg16 |= 2 << (pin * 2);	// pin as input
> 
> C++ comments style!

OK, I changed it.

However....
I can't find anywhere it says that // is not proper.

For example, just look at checkpatch.pl:

$ grep -m1  allow_c99_comments  scripts/checkpatch.pl
my $allow_c99_comments = 1;

Also every file has this as the very first line:
// SPDX-License-Identifier: GPL-2.0

;)


> > +	else
> > +		reg16 |= 3 << (pin * 2);	// pin as output
> 
> Also, you are using those '2' and '3' values here and there in the
> code, as they represent the pin 'status. Would you consider making
> a define for them? Actually, for all of them
> 
> #define RZA2_PIN_STATE_HIGHZ    0x00
> #define RZA2_PIN_STATE_INPUT    0x02
> #define RZA2_PIN_STATE_OUTPUT   0x03

OK.


> > +	/*
> > +	 * This GPIO controller has a default Hi-Z state that is not input
> or
> > +	 * output, so force the pin to input now.
> > +	 */
> I wonder if it is fine for the .get_direction callback to change the
> pin state.

I see no other option. I can only return "input" or "output", but Hi-Z 
is neither one of those.
The .get_direction is the first one to get called after assigning a pin.
If I return that it's an 'input', it will think it does not have to call
.direction_input, and then the pin doesn't work.


> > +	rza2_pin_to_gpio(priv->base, port, pin, GPIOF_DIR_IN);
> 
> Empty line before return is nice (here and in other places).

True. I added it.

> I'll stop here with comments on the driver, as I should have a look at
> bindings before. I'll comment there too and the get back to this.

OK. I've made all these changes.

I see just sent some more comments, so I'll have a look at them first 
before I send a V2.

Thanks,
Chris