On Tue, 2021-02-02 at 14:57 +0100, Linus Walleij wrote: > On Thu, Jan 28, 2021 at 4:36 PM Alban Bedel <[email protected]> > wrote: > > > From a quick glance at various datasheet the PCAL6524 seems to be > > the > > only chip in this familly that support setting the drive mode of > > single pins. Other chips either don't support it at all, or can > > only > > set the drive mode of whole banks, which doesn't map to the GPIO > > API. > > > > Add a new flag, PCAL6524, to mark chips that have the extra > > registers > > needed for this feature. Then mark the needed register banks as > > readable and writable, here we don't set OUT_CONF as writable, > > although it is, as we only need to read it. Finally add a function > > that configure the OUT_INDCONF register when the GPIO API set the > > drive mode of the pins. > > > > Signed-off-by: Alban Bedel <[email protected]> > > Thats's nice! > > > + * Output port configuration 0x40 + 7 * bank_size R > > + * > > + * - PCAL6524 with individual pin configuration > > + * Individual pin output config 0x40 + 12 * bank_size RW > > So this will become 0x70? It's a bit hard for me this weird > register layout...
Correct.
> > +static int pcal6524_gpio_set_drive_mode(struct pca953x_chip *chip,
> > + unsigned int offset,
> > + unsigned long config)
> > +{
> > + u8 out_conf_reg = pca953x_recalc_addr(
> > + chip, PCAL953X_OUT_CONF, 0);
> > + u8 out_indconf_reg = pca953x_recalc_addr(
> > + chip, PCAL6524_OUT_INDCONF, offset);
> > + u8 mask = BIT(offset % BANK_SZ), val;
>
> Split to two variable declarations please, this is hard to read.
Will do.
> > + unsigned int out_conf;
> > + int ret;
>
> So we set mask to the bit index for the line we want to affect.
>
> > + if (config == PIN_CONFIG_DRIVE_OPEN_DRAIN)
> > + val = mask;
> > + else if (config == PIN_CONFIG_DRIVE_PUSH_PULL)
> > + val = 0;
> > + else
> > + return -EINVAL;
>
> And this makes sense, we set it to 1 to enable open drain.
>
> > + /* Invert the value if ODENn is set */
> > + ret = regmap_read(chip->regmap, out_conf_reg, &out_conf);
> > + if (ret)
> > + goto exit;
> > + if (out_conf & BIT(offset / BANK_SZ))
>
> I suppose this could be written if (out_conf & mask)?
No, the mask in ODENn is per bank (group of 8 pins) and not per pin.
> > + val ^= mask;
>
> Invert? Why?
>
> The datasheet says:
>
> "If the ODENx bit is set at logic 0 (push-pull), any bit set to
> logic 1
> in the IOCRx register will reverse the output state of that pin
> only
> to open-drain. When ODENx bit is set at logic 1 (open-drain), a
> logic 1 in IOCRx will set that pin to push-pull."
>
> So your logic is accounting for the fact that someone go and set
> one of the bits in ODENx to 1, but aren't they all by default set
> to zero (or should be programmed by the driver to zero)
> so that you can control open drain individually here by simply
> setting the corresponding bit to 1 for open drain and 0 for
> push-pull?
Yes the ODENx bits are 0 by default, but the bootloader might have
changed them for example. Currently the driver doesn't do any reset so
I think it make sense to correctly handle this case as it doesn't bring
that much extra complexity.
Alban
signature.asc
Description: This is a digitally signed message part
