All of lore.kernel.org
 help / color / mirror / Atom feed
From: Michael Walle <michael@walle.cc>
To: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
Cc: linus.walleij@linaro.org, brgl@bgdev.pl,
	linux-gpio@vger.kernel.org, linux-kernel@vger.kernel.org,
	Andy Shevchenko <andy.shevchenko@gmail.com>
Subject: Re: [PATCH 1/3] gpio: regmap: Support registers with more than one bit per GPIO
Date: Thu, 07 Jul 2022 09:44:13 +0200	[thread overview]
Message-ID: <e3e3b4ed3b05368266d871e45235c899@walle.cc> (raw)
In-Reply-To: <DfKGwB5bggV3msX63bZrjjUX37ipAwv7@localhost>

Am 2022-07-06 22:46, schrieb Aidan MacDonald:
>> Am 2022-07-04 18:01, schrieb Aidan MacDonald:
>>>> Am 2022-07-03 13:10, schrieb Aidan MacDonald:
>>>>> Some devices use a multi-bit register field to change the GPIO
>>>>> input/output direction. Add the ->reg_field_xlate() callback to
>>>>> support such devices in gpio-regmap.
>>>>> ->reg_field_xlate() builds on ->reg_mask_xlate() by allowing the
>>>>> driver to return a mask and values to describe a register field.
>>>>> gpio-regmap will use the mask to isolate the field and compare or
>>>>> update it using the values to implement GPIO level and direction
>>>>> get and set ops.
>>>> Thanks for working on this. Here are my thoughts on how to improve
>>>> it:
>>>>  - I'm wary on the value translation of the set and get, you
>>>>    don't need that at the moment, correct? I'd concentrate on
>>>>    the direction for now.
>>>>  - I'd add a xlate_direction(), see below for an example
>>> Yeah, I only need direction, but there's no advantage to creating a
>>> specific mechanism. I'm not opposed to doing that but I don't see
>>> how it can be done cleanly. Being more general is more consistent
>>> for the API and implementation -- even if the extra flexibility
>>> probably won't be needed, it doesn't hurt.
>> 
>> I'd prefer to keep it to the current use case. I'm not sure if
>> there are many controllers which have more than one bit for
>> the input and output state. And if, we are still limited to
>> one register, what if the bits are distributed among multiple
>> registers..
>> 
> 
> I found three drivers (not exhaustive) that have fields for setting the
> output level: gpio-amd8111, gpio-creg-snps, and gpio-lp3943. Admittedly
> that's more than I expected, so maybe we shouldn't dismiss the idea of
> multi-bit output fields.

I'm not dismissing it, but I want to wait for an actual user
for it :)

> If you still think the API you're suggesting is better then I can go
> with it, but IMHO it's more code and more special cases, even if only
> a little bit.

What bothers me with your approach is that you are returning
an integer and in one case you are interpreting it as gpio
direction and in the other case you are interpreting it as
a gpio state, while mine is more explicit, i.e. a
xlate_direction() for the direction and if there will be
support for multi bit input/output then there would be a
xlate_state() or similar. Granted, it is more code, but
easier to understand IMHO.

-michael

  reply	other threads:[~2022-07-07  7:44 UTC|newest]

Thread overview: 23+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-07-03 11:10 [PATCH 0/3] gpio-regmap support for register fields and other hooks Aidan MacDonald
2022-07-03 11:10 ` [PATCH 1/3] gpio: regmap: Support registers with more than one bit per GPIO Aidan MacDonald
2022-07-03 14:09   ` Andy Shevchenko
2022-07-04 16:03     ` Aidan MacDonald
2022-07-04 12:28   ` Michael Walle
2022-07-04 16:01     ` Aidan MacDonald
2022-07-04 19:46       ` Michael Walle
2022-07-06 20:46         ` Aidan MacDonald
2022-07-07  7:44           ` Michael Walle [this message]
2022-07-07 14:58             ` Aidan MacDonald
2022-07-03 11:10 ` [PATCH 2/3] gpio: regmap: Support combined GPIO and pin control drivers Aidan MacDonald
2022-07-03 14:14   ` Andy Shevchenko
2022-07-04 15:31     ` Aidan MacDonald
2022-07-03 11:10 ` [PATCH 3/3] gpio: regmap: Support a custom ->to_irq() hook Aidan MacDonald
2022-07-03 14:24   ` Andy Shevchenko
2022-07-04 16:38     ` Aidan MacDonald
2022-07-04 23:05   ` Linus Walleij
2022-07-05 11:09     ` Aidan MacDonald
2022-07-06 11:45       ` Andy Shevchenko
2022-07-06 20:53         ` Aidan MacDonald
2022-07-06 12:02       ` Linus Walleij
2022-07-06 13:50         ` Aidan MacDonald
2022-07-11 11:48           ` Linus Walleij

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=e3e3b4ed3b05368266d871e45235c899@walle.cc \
    --to=michael@walle.cc \
    --cc=aidanmacdonald.0x0@gmail.com \
    --cc=andy.shevchenko@gmail.com \
    --cc=brgl@bgdev.pl \
    --cc=linus.walleij@linaro.org \
    --cc=linux-gpio@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.