Re: [PATCH v4 3/3] pinctrl: Add Xilinx ZynqMP pinctrl driver support

[Andy Shevchenko <andy.shevchenko@xxxxxxxxx>]

On Mon, Mar 22, 2021 at 5:25 PM Sai Krishna Potthuri
<lakshmis@xxxxxxxxxx> wrote:
> > From: Andy Shevchenko <andy.shevchenko@xxxxxxxxx>
> > Sent: Friday, March 19, 2021 3:53 PM
> > On Thu, Mar 18, 2021 at 4:42 PM Sai Krishna Potthuri <lakshmis@xxxxxxxxxx>
> > wrote:
> > > > From: Andy Shevchenko <andy.shevchenko@xxxxxxxxx>
> > > > Sent: Wednesday, March 17, 2021 6:26 PM On Wed, Mar 17, 2021 at
> > > > 10:27 AM Sai Krishna Potthuri
> > > > <lakshmi.sai.krishna.potthuri@xxxxxxxxxx> wrote:

...

> > > #include <dt-bindings/pinctrl/pinctrl-zynqmp.h>
> >
> > Looking into other drivers with similar includes, shouldn't it go first in the file
> > before any other linux/* asm/* etc?
> I see some drivers are including this header before linux/* and some are using
> after linux/*.

The rule of thumb is that: more generic headers are going first.

I consider dt/* ones are more generic than linux/* ones because they
are covering more than just the Linux kernel.

...

> > > > I'm lost here. What is IO standard exactly? Why it can't be in
> > > > generic headers?
> > > It represents LVCMOS 3.3 volts/ LVCMOS 1.8 volts.
> > > Since this is specific to Xilinx ZynqMP platform, considered to be
> > > added in the driver file.
> >
> > So, why can't we create a couple of bits to represent this voltages in the
> > generic header and gain usability for others as well?
> I see some drivers are maintaining the configuration list in the driver file, if
> the configuration is specific to the driver.

Yes, my point is that this case doesn't sound too specific to the
driver. Many pin control buffers (in hardware way of speaking) have
properties to be different voltage tolerant / produce.

> We can move this to generic header if it is used by others as well.
> Ok, will wait for Linus to comment.
> >
> > Linus?

...

> > > > > +       ret = zynqmp_pm_pinctrl_request(pin);
> > > > > +       if (ret) {
> > > > > +               dev_err(pctldev->dev, "request failed for pin
> > > > > + %u\n", pin);
> > > >
> > > > > +               return -EIO;
> > > >
> > > > Why shadowing error code?
> >
> > So, any comments on the initial Q?
> Xilinx low level secure firmware error codes are different from Linux error codes.
> Secure firmware maintains list of error codes (positive values other than zero).
> Hence we return -EIO, if the return value from firmware is non-zero.

Why the zynqmp_pm_pinctrl_request() can't return codes in Linux error
code space?

> > >>  Since it's the only possible error, why is it not
> > > > reflected in the kernel doc?
> > > I will update the kernel doc with the error value for such cases.
> > > >
> > > > > +       }

-- 
With Best Regards,
Andy Shevchenko