Re: [PATCH] ARM/pxa/mfd/power/sound: Switch Tosa to GPIO descriptors

[Linus Walleij <linus.walleij@xxxxxxxxxx>]

Hi Lee,

On Fri, Jul 16, 2021 at 9:51 AM Lee Jones <lee.jones@xxxxxxxxxx> wrote:

Sorry for taking halfyears to get back to patches... cleanup tends
to get low prio. :/

Fixed most review comments.

> > +#define TOSA_GPIO_TG_ON                      0
> > +#define TOSA_GPIO_L_MUTE             1
> > +#define TOSA_GPIO_BL_C20MA           3
> > +#define TOSA_GPIO_CARD_VCC_ON                4
> > +#define TOSA_GPIO_CHARGE_OFF         6
> > +#define TOSA_GPIO_CHARGE_OFF_JC              7
> > +#define TOSA_GPIO_BAT0_V_ON          9
> > +#define TOSA_GPIO_BAT1_V_ON          10
> > +#define TOSA_GPIO_BU_CHRG_ON         11
> > +#define TOSA_GPIO_BAT_SW_ON          12
> > +#define TOSA_GPIO_BAT0_TH_ON         14
> > +#define TOSA_GPIO_BAT1_TH_ON         15
>
> Okay, I have to ask - what are 5, 8 and 13?

Apparently unused, just picked from:
arch/arm/mach-pxa/include/mach/tosa.h

Put there in commit 8459c159f7de832eaf888398d2abf466c388dfa6
"[ARM] 3088/1: PXA: Add machine support for the Sharp SL-6000x series of PDAs"
Dirk who authored the commit is on CC so maybe he can say.
I suppose he has access to the board documentation.
(I couldn't find any.)

> > +static struct gpiod_lookup_table tosa_audio_gpio_lookup = {
> > +     .dev_id = "tosa-audio",
> > +     .table = {
> > +             GPIO_LOOKUP(TOSA_GC_NAME, TOSA_GPIO_L_MUTE, NULL, GPIO_ACTIVE_HIGH),
> > +             { },
> > +     },
> > +};
>
> Are these structures going to be peppered all over the kernel now?

Yeah well for legacy systems, for the reasons given in
drivers/gpio/TODO

It's not millions of occurences, just hundreds.
$ git grep GPIO_LOOKUP|wc -l
507

> Maybe a helper can be added to make these single line entries one line
> each?

Hmmm I will try to sketch something out for v2!

> > +             GPIO_LOOKUP(TOSA_GC_NAME, TOSA_GPIO_CHARGE_OFF,
> > +                         "main charge off", GPIO_ACTIVE_HIGH),
> > +             GPIO_LOOKUP(TOSA_GC_NAME, TOSA_GPIO_CHARGE_OFF_JC,
> > +                         "jacket charge off", GPIO_ACTIVE_HIGH),
> > +             GPIO_LOOKUP(TOSA_GC_NAME, TOSA_GPIO_BAT0_V_ON,
> > +                         "main battery", GPIO_ACTIVE_HIGH),
> > +             GPIO_LOOKUP(TOSA_GC_NAME, TOSA_GPIO_BAT1_V_ON,
> > +                         "jacket battery", GPIO_ACTIVE_HIGH),
> > +             GPIO_LOOKUP(TOSA_GC_NAME, TOSA_GPIO_BU_CHRG_ON,
> > +                         "backup battery", GPIO_ACTIVE_HIGH),
> > +             /* BAT1 and BAT0 thermistors appear to be swapped */
> > +             GPIO_LOOKUP(TOSA_GC_NAME, TOSA_GPIO_BAT1_TH_ON,
> > +                         "main battery temp", GPIO_ACTIVE_HIGH),
> > +             GPIO_LOOKUP(TOSA_GC_NAME, TOSA_GPIO_BAT0_TH_ON,
> > +                         "jacket battery temp", GPIO_ACTIVE_HIGH),
> > +             GPIO_LOOKUP(TOSA_GC_NAME, TOSA_GPIO_BAT_SW_ON,
> > +                         "battery switch", GPIO_ACTIVE_HIGH),
>
> These are soooo close to being <100 chars.
>
> What does Checkpatch currently warn on?  Is it 100 or 120?

100...

WARNING: line length of 104 exceeds 100 columns
#283: FILE: drivers/mfd/tc6393xb.c:532:
+        GPIO_LOOKUP("tc6393xb", TOSA_GPIO_CHARGE_OFF_JC, "jacket
charge off", GPIO_ACTIVE_HIGH),

WARNING: line length of 101 exceeds 100 columns
#288: FILE: drivers/mfd/tc6393xb.c:537:
+        GPIO_LOOKUP("tc6393xb", TOSA_GPIO_BAT1_TH_ON, "main battery
temp", GPIO_ACTIVE_HIGH),

(...)

If you want to, we can ignore it of course. Just tell me what to do.

> > +     gc->ngpio = 16;
> > +     gc->set = tc6393xb_gpio_set;
> > +     gc->get = tc6393xb_gpio_get;
> > +     gc->direction_input = tc6393xb_gpio_direction_input;
> > +     gc->direction_output = tc6393xb_gpio_direction_output;
> > +
> > +     ret = devm_gpiochip_add_data(dev, gc, tc6393xb);
> > +     if (ret)
> > +             return dev_err_probe(dev, ret, "failed to add GPIO chip\n");
> > +
> > +     /* Register descriptor look-ups for consumers */
> > +     gpiod_add_lookup_tables(tc6393xb_gpio_lookups, ARRAY_SIZE(tc6393xb_gpio_lookups));
> > +
> > +     /* Request some of our own GPIOs */
> > +     tc6393xb->vcc_on = gpiochip_request_own_desc(gc, TOSA_GPIO_CARD_VCC_ON, "VCC ON",
> > +                                                  GPIO_ACTIVE_HIGH, GPIOD_OUT_HIGH);
> > +     if (IS_ERR(tc6393xb->vcc_on))
> > +             return dev_err_probe(dev, PTR_ERR(tc6393xb->vcc_on),
> > +                                  "failed to request VCC ON GPIO\n");
> > +
>
> So much more code to do the same thing?

Not quite the same thing.

The old code does not report any errors from gpiochip_add_data()
(hence all the dev_err_probe()).

The added gpiochip_request_own_desc() call moves the similar code out
of arch/arm
to here where the gpiochip is and is actually using fewer lines now.
(See higher up the patch.)

> > +     tc6393xb->dev = &dev->dev;
>
> That confused me at first.
>
> Please consider changing the platform_device to pdev (separately).

OK I can follow up with that once this looks like we want it,
this problem is a bit all over the kernel actually, especially
in legacy code from the early 2000s.

Have a look at v2 and see what you think!

Yours,
Linus Walleij