| Message ID | 20230403114033.8336-1-edmund.berenson@emlix.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/2] dt-bindings: gpio: max7317: add spi gpio extender documentation | expand |
On Mon, Apr 3, 2023 at 1:41 PM Edmund Berenson <edmund.berenson@emlix.com> wrote: > Add driver for maxim MAX7317 SPI-Interfaced 10 Port > GPIO Expander. > > v2: adjust driver to use regmap > > Co-developed-by: Lukasz Zemla <Lukasz.Zemla@woodward.com> > Signed-off-by: Lukasz Zemla <Lukasz.Zemla@woodward.com> > Signed-off-by: Edmund Berenson <edmund.berenson@emlix.com> Notwithstanding the other comments from Bartosz, this seems like a driver that should be using the regmap GPIO helper library. git grep GPIO_REGMAP will show you examples of other drivers that use this and how it is used. Yours, Linus Walleij
On 4/4/23 16:05, Linus Walleij wrote: > On Mon, Apr 3, 2023 at 1:41 PM Edmund Berenson > <edmund.berenson@emlix.com> wrote: > >> Add driver for maxim MAX7317 SPI-Interfaced 10 Port >> GPIO Expander. >> >> v2: adjust driver to use regmap >> >> Co-developed-by: Lukasz Zemla <Lukasz.Zemla@woodward.com> >> Signed-off-by: Lukasz Zemla <Lukasz.Zemla@woodward.com> >> Signed-off-by: Edmund Berenson <edmund.berenson@emlix.com> > > Notwithstanding the other comments from Bartosz, this seems like > a driver that should be using the regmap GPIO helper library. > git grep GPIO_REGMAP will show you examples of other drivers > that use this and how it is used. > > Yours, > Linus Walleij Hi, thanks for the review and suggestion. I tried following your suggestion and use GPIO_REGMAP to implement the driver. Unfortunately I ran into two issues 1. reg_set_base == 0: for the devcie reg_set base is 0x0. In gpio-regmap there are several tests for !reg_set_base. There doesn't seem a way to distinguish between is set to 0 and is not set. :) 2. input/output direction: to set a gpio pin to input one has to write 0x1 to the corresponding output register. The issue starts when I configure a port to be an output, set output to 0x1, check the direction of the pin, doing so trough sysfs the system will now assume the pin is an input and I can't set its values anymore. Avoiding this I would like to track the direction of the pin separately from the device register, which is atm done in the corresponding bespoke in/out functions. I could probably solve both of these issues trough the reg_mask_xlate function but I believe this would introduce unneeded obscurity in the driver. I do not believe there are any other easy obvious/better fixes for this. (or maybe you prove me wrong :)) Would you be okay for this driver to stick with direct regmap usage? (obviously fixing the review suggestions) BR Edmund
Am 2023-05-03 10:37, schrieb Edmund Berenson: > On 4/4/23 16:05, Linus Walleij wrote: >> On Mon, Apr 3, 2023 at 1:41 PM Edmund Berenson >> <edmund.berenson@emlix.com> wrote: >> >>> Add driver for maxim MAX7317 SPI-Interfaced 10 Port >>> GPIO Expander. >>> >>> v2: adjust driver to use regmap >>> >>> Co-developed-by: Lukasz Zemla <Lukasz.Zemla@woodward.com> >>> Signed-off-by: Lukasz Zemla <Lukasz.Zemla@woodward.com> >>> Signed-off-by: Edmund Berenson <edmund.berenson@emlix.com> >> >> Notwithstanding the other comments from Bartosz, this seems like >> a driver that should be using the regmap GPIO helper library. >> git grep GPIO_REGMAP will show you examples of other drivers >> that use this and how it is used. >> >> Yours, >> Linus Walleij > > Hi, > > thanks for the review and suggestion. I tried following your suggestion > and use > GPIO_REGMAP to implement the driver. > > Unfortunately I ran into two issues > 1. reg_set_base == 0: for the devcie reg_set base is 0x0. In > gpio-regmap there > are several tests for !reg_set_base. There doesn't seem a way to > distinguish > between is set to 0 and is not set. :) That's what GPIO_REGMAP_ADDR(addr) is for :) > 2. input/output direction: to set a gpio pin to input one has to write > 0x1 to > the corresponding output register. The issue starts when I configure a > port to > be an output, set output to 0x1, check the direction of the pin, doing > so trough > sysfs the system will now assume the pin is an input and I can't set > its values > anymore. Avoiding this I would like to track the direction of the pin > separately > from the device register, which is atm done in the corresponding > bespoke in/out > functions. I can't follow you here exactly. But I've briefly looked at the datasheet and the output is an open drain one. Just like the one we are currently discussing in [1]. Here too, there seems to be no direction register, although it is mentioned in the datasheet. But Table 1. Register Address Map is super confusing, so please correct me if I'm wrong. Since we now have already two different chips with this OD output and always active input buffer, it makes sense to add support to gpio-regmap for these kind of devices. To me it is still unclear wether we need the direction at all. Linus answered my question yesterday, but I haven't found time to dig into that topic myself. Please go ahead and make some suggestions :) > I could probably solve both of these issues trough the reg_mask_xlate > function > but I believe this would introduce unneeded obscurity in the driver. > > I do not believe there are any other easy obvious/better fixes for > this. (or > maybe you prove me wrong :)) > Would you be okay for this driver to stick with direct regmap usage? > (obviously > fixing the review suggestions) -michael [1] https://lore.kernel.org/r/20230502084406.3529645-1-michael@walle.cc/
diff --git a/Documentation/devicetree/bindings/gpio/gpio-max7317.yaml b/Documentation/devicetree/bindings/gpio/gpio-max7317.yaml new file mode 100644 index 000000000000..ad5a796c704e --- /dev/null +++ b/Documentation/devicetree/bindings/gpio/gpio-max7317.yaml @@ -0,0 +1,50 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/gpio/gpio-max7317.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Maxim MAX7317 SPI-Interfaced I/O Expander + +maintainers: + - Edmund Berenson <edmund.berenson@emlix.com> + +description: + Bindings for 10-Port Maxim MAX7317 SPI GPIO expanders. + +properties: + compatible: + const: maxim,max7317 + + reg: + maxItems: 1 + + gpio-controller: true + + '#gpio-cells': + const: 2 + +required: + - compatible + - reg + - gpio-controller + - "#gpio-cells" + +unevaluatedProperties: false + +allOf: + - $ref: /schemas/spi/spi-peripheral-props.yaml# + +examples: + - | + spi { + #address-cells = <1>; + #size-cells = <0>; + + gpio@0 { + compatible = "maxim,max7317"; + reg = <0>; + gpio-controller; + #gpio-cells = <2>; + }; + };