RE: [PATCH 1/7] pinctrl: Renesas RZ/A1 pin and gpio controller

[Chris Brandt <Chris.Brandt@xxxxxxxxxxx>]

Hi Jacopo,

> > I would suggest:
> >    PIBC_REG = 1 /* always gets set for GPIO mode */
> >  input:
> >    PM_REG = 1
> >    PBDC_REG = 0
> > output:
> >    PM_REG = 0
> >    PBDC_REG = 1
> >
> 
> I assumed "set_direction()" to always be called after a "request()", so
> that the pin is always in reset state.
> This is obviously not true, so I'm going to fix this as you suggested.
> I'm just wondering why we should keep input buffer enabled even if we're
> running in output mode. This seems to be "safe" according to TRM but I'm a
> bit afraid of unexpected consequences (am I too paranoid?)

In the hardware manual, look at "Table 54.4 PPRnm Read Values"

When PM_REG is '0', the input buffer value (PIBC_REG) is don't care (X)



> > I would suggest something like this:
> >
> > 	if (bidir)
> > 		rza1_set_bit(port, PBDC_REG, offset, 1);
> > 	else {
> > 		rza1_set_bit(port, PBDC_REG, offset, 0);
> > 		rza1_set_bit(port, PM_REG, offset, swio_in);
> > 	}
> >
> > 	rza1_set_bit(port, PMC_REG, offset, 1);
> >
> >
> 
> This looks nicer, for sure, but from my testing I found out that the
> sequence reported in section 54.19-c of TRM has to be carefully respected,
> particularly setting PM after PMC (and iirc setting PBDC before PFC*) Am I
> wrong? I will give your suggestion spin anyway...

I admit, I didn't really look at the suggested flow that the manual said.
I just want off the fact that PMC_REG was the last thing being set, so all
the other registers were irrelevant until that point.

However, you can change the order if you like, as longs as the register
settings still end up the same way as I outlined.


> This apart, you suggestion is tied to your comment on [2/7] on replacing
> INPUT_EN with BIDIR and introduce SWIO_IN and SWIO_OUT, which I mostly
> agree on (I'll comment on that as well).
> 
> A sort of middle-ground to get rid of horrible conditions like "if
> ((!swio_en && input_en) || (swio_en && !input_en))"
> which requires a 7 lines comments to be explained and I still found
> obscure after a month I was not looking at it, would be making BIDIR and
> SWIO_[IN|OUT] exclusive, as if a pin is set to be SWIO it already has
> direction specified, and if it is said to be BIDIR, it means its direction
> is decided by the alternate function, not by software.
> Does this fly for you?

Yes, my suggestion is to make BIDIR and SWIO_[IN|OUT] exclusive in the DT. You
only choose one (or nothing). You can specify that in the DT bindings document.

> Do you see cases where those flags may have to be
> specified together?

There should not be case. Actually...when I went through this, I think the HW manual
prohibits this anyway...or rather one cancels the other....but I can't remember which.
But, I did spend a good amount of time re-reading the chapter when I was reviewing
your code.


Thank you,
Chris

--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html