Date:   Tue, 27 Jun 2023 18:55:07 +0000
From:   "Hawkins, Nick" <nick.hawkins@....com>
To:     Andy Shevchenko <andy.shevchenko@...il.com>
CC:     "Verdun, Jean-Marie" <verdun@....com>,
        "linus.walleij@...aro.org" <linus.walleij@...aro.org>,
        "brgl@...ev.pl" <brgl@...ev.pl>,
        "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "krzysztof.kozlowski+dt@...aro.org" 
        <krzysztof.kozlowski+dt@...aro.org>,
        "jdelvare@...e.com" <jdelvare@...e.com>,
        "linux@...ck-us.net" <linux@...ck-us.net>,
        "linux-gpio@...r.kernel.org" <linux-gpio@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-hwmon@...r.kernel.org" <linux-hwmon@...r.kernel.org>
Subject: Re: [PATCH v4 1/5] gpio: gxp: Add HPE GXP GPIO

> defining (I have a déjà vu of already showing you typos in the commit
> message and comments and it looks like you ignored all of that. If so,
> then why?)

Apologies Andy,

I somehow completely missed / lost your mention of a typo in both the
commit and comments. I have no intention of ignoring it. I will correct
this. Thank you for the input you have provided. I have several
questions and comments below.


> > + * are specifically defined in DTS and offsets are used here.
> > + */
> > +enum gxp_gpio_pn {
> > + RESET = 192,
> > + VPBTN = 210, /* aka POWER_OK */
> > + PGOOD = 211, /* aka PS_PWROK */
> > + PERST = 212, /* aka PCIERST */
> > + POST_COMPLETE = 213,


> So, vbtn is a GPIO? Why does it need a special treatment?

I was specifically grabbing the areas of memory that I needed instead of
mapping the entire fn2 area of memory. I believe I can map the entire
area instead.

...

> > + case 64 ... 95:
> > + reg_offset = GPODAT;
> > + reg_mask = BIT(offset - 64);
> > + break;
> > + case 96 ... 127:
> > + reg_offset = GPODAT + 0x04;
> > + reg_mask = BIT(offset - 96);
> > + break;


> ...and here (between two groups of GPO) is 0x48. Looks a bit weird.
> Does this GPIO have more functions than simply being a GPIO? To me
> looks like a PMIC-ish one. Is there any datasheet available?

Unfortunately, there is no public datasheet available currently. There
are however some special functions others than being a simple GPIO.
There are ownership bits as the same area is accessible VIA PCI.

> > + case RESET:
> > + /* SW_RESET */
> > + reg_offset = ASR_OFS;
> > + reg_mask = BIT(15);
> > + break;

> Does it really belong to this driver? Maybe it should be an MFD with
> GPIO and special functions with valid_mask properly assigned?

Unlike your suggestion I quote directly below are you implying that
My accesses to the CSM area of memory can be its own separate
driver that is MFD and provides GPIO lines to read?

> ... 

> > + case 192:
> > + if (value) {
> > + regmap_update_bits(drvdata->csm_map, ASR_OFS,
> > + BIT(0), BIT(0));
> > + regmap_update_bits(drvdata->csm_map, ASR_OFS,
> > + BIT(15), BIT(15));
> > + } else {
> > + regmap_update_bits(drvdata->csm_map, ASR_OFS,
> > + BIT(15), 0);
> > + }
> > + break;


> Again, seems like a special function of GPIO that should probably have
> another driver that shares regmap with GPIO and GPIO marks this one is
> not valid for the GPIO operations.

What do you mean by GPIO marking this one as not valid for GPIO
operations?

...

> > +static int gxp_gpio_vuhc_get(struct gpio_chip *chip, unsigned int offset)
> > +{
> > + struct gxp_gpio_drvdata *drvdata = dev_get_drvdata(chip->parent);
> > + unsigned int val;
> > + int ret = 0;
> > +
> > + if (offset < 8) {
> > + regmap_read(drvdata->vuhc0_map, VUHC_OFS + 4 * offset, &val);
> > + ret = (val & BIT(13)) ? 1 : 0;
> > + }
> > +
> > + return ret;
> > +}
> > +
> > +static void gxp_gpio_vuhc_set(struct gpio_chip *chip, unsigned int offset,
> > + int value)
> > +{
> > + /* Currently we are not supporting setting of these values yet */
> > + switch (offset) {
> > + default:
> > + break;
> > + }
> > +}
> > +
> > +static int gxp_gpio_vuhc_get_direction(struct gpio_chip *chip,
> > + unsigned int offset)
> > +{
> > + switch (offset) {
> > + case 0:
> > + case 1:
> > + case 2:
> > + return GXP_GPIO_DIR_IN;
> > + default:
> > + return -ENOTSUPP;
> > + }
> > +}
> > +
> > +static int gxp_gpio_vuhc_direction_input(struct gpio_chip *chip,
> > + unsigned int offset)
> > +{
> > + switch (offset) {
> > + case 0:
> > + case 1:
> > + case 2:
> > + return 0;
> > + default:
> > + return -ENOTSUPP;
> > + }
> > +}
> > +
> > +static int gxp_gpio_vuhc_direction_output(struct gpio_chip *chip,
> > + unsigned int offset, int value)
> > +{
> > + switch (offset) {
> > + default:
> > + return -ENOTSUPP;
> > + }
> > +}


> I'm not sure this belongs to the GPIO driver.

By this do you mean that it needs to be in a separate non GPIO driver
that shares a regmap as suggested above?

...

> So, overall it looks to me like an MFD device which should be split to
> GPIO, GPIO with IRQ (fh2), special cases and designated
> functionalities (somelike ~5 drivers all together). Without having a
> datasheet it's hard to say.

Yes that sounds like a good plan to me I will see what I can work up.
I might end up removing thils file entirely and just sticking with
gpio-gxp-pl.c As for the gpio-gxp-pl.c are you okay with it?


Thank you for the assistance and review,

-Nick Hawkins

Powered by blists - more mailing lists