On Fri, 2023-10-13 at 09:27 -0500, David Lechner wrote:
> On Fri, Oct 13, 2023 at 2:50 AM Nuno Sá <noname.nuno@xxxxxxxxx> wrote:
> >
> > Hi David,
> >
> > Couple of minor things...
> >
> > On Thu, 2023-10-12 at 15:45 -0500, David Lechner wrote:
> > > It is possible to use the AD2S1210 with hardwired mode pins (A0 and A1).
> > > According to the devicetree bindings, in this case the adi,fixed-mode
> > > property will specify which of the 3 possible modes the mode pins are
> > > hardwired for and the gpio-modes property is not allowed.
> > >
> > > This adds support for the case where the mode pins are hardwired for
> > > config mode. In this configuration, the position and velocity must be read
> > > from the config register.
> > >
> > > The cases of hardwired position or velocity modes is not supported as
> > > there would be no way to configure the device.
> > >
> > > Signed-off-by: David Lechner <dlechner@xxxxxxxxxxxx>
> > > ---
> > > drivers/iio/resolver/ad2s1210.c | 193 +++++++++++++++++++++++++++-----
> > > 1 file changed, 162 insertions(+), 31 deletions(-)
> > >
> > > diff --git a/drivers/iio/resolver/ad2s1210.c
> > > b/drivers/iio/resolver/ad2s1210.c
> > > index 1bd1b950e7cc..e6d3f31d529f 100644
> > > --- a/drivers/iio/resolver/ad2s1210.c
> > > +++ b/drivers/iio/resolver/ad2s1210.c
> > > @@ -141,7 +141,7 @@ struct ad2s1210_state {
> > > struct spi_device *sdev;
> > > /** GPIO pin connected to SAMPLE line. */
> > > struct gpio_desc *sample_gpio;
> > > - /** GPIO pins connected to A0 and A1 lines. */
> > > + /** GPIO pins connected to A0 and A1 lines (optional). */
> > > struct gpio_descs *mode_gpios;
> > > /** Used to access config registers. */
> > > struct regmap *regmap;
> > > @@ -149,6 +149,8 @@ struct ad2s1210_state {
> > > unsigned long clkin_hz;
> > > /** Available raw hysteresis values based on resolution. */
> > > int hysteresis_available[2];
> > > + /* adi,fixed-mode property - only valid when mode_gpios == NULL. */
> > > + enum ad2s1210_mode fixed_mode;
> > > /** The selected resolution */
> > > enum ad2s1210_resolution resolution;
> > > /** Copy of fault register from the previous read. */
> > > @@ -175,6 +177,9 @@ static int ad2s1210_set_mode(struct ad2s1210_state
> > > *st,
> > > enum ad2s1210_mode mode)
> > > struct gpio_descs *gpios = st->mode_gpios;
> > > DECLARE_BITMAP(bitmap, 2);
> > >
> > > + if (!gpios)
> > > + return mode == st->fixed_mode ? 0 : -EOPNOTSUPP;
> > > +
> > > bitmap[0] = mode;
> > >
> > > return gpiod_set_array_value(gpios->ndescs, gpios->desc, gpios-
> > > >info,
> > > @@ -276,7 +281,8 @@ static int ad2s1210_regmap_reg_read(void *context,
> > > unsigned int reg,
> > > * parity error. The fault register is read-only and the D7 bit
> > > means
> > > * something else there.
> > > */
> > > - if (reg != AD2S1210_REG_FAULT && st->rx[1] & AD2S1210_ADDRESS_DATA)
> > > + if ((reg > AD2S1210_REG_VELOCITY_LSB && reg != AD2S1210_REG_FAULT)
> > > + && st->rx[1] & AD2S1210_ADDRESS_DATA)
> > > return -EBADMSG;
> > >
> > > *val = st->rx[1];
> > > @@ -437,6 +443,40 @@ static void ad2s1210_push_events(struct iio_dev
> > > *indio_dev,
> > > st->prev_fault_flags = flags;
> > > }
> > >
> > > +/**
> > > + * Reads position or velocity from the config registers.
> > > + *
> > > + * This is used when the mode gpios are not available.
> > > + *
> > > + * Must be called with the lock held.
> > > + *
> > > + * @param st The device state.
> > > + * @param val Pointer to hold the value read.
> > > + * @param msb_reg The register address of the MSB register.
> > > + * @param lsb_reg The register address of the LSB register.
> > > + * @return 0 on success, negative error code otherwise.
> > > + */
> > > +static int ad2s1210_read_val_from_config(struct ad2s1210_state *st,
> > > __be16
> > > *val,
> > > + u8 msb_reg, u8 lsb_reg)
> > > +{
> > > + unsigned int reg_val;
> > > + int ret;
> > > +
> > > + ret = regmap_read(st->regmap, msb_reg, ®_val);
> > > + if (ret < 0)
> > > + return ret;
> > > +
> > > + ((u8 *)val)[0] = reg_val;
> > > +
> > > + ret = regmap_read(st->regmap, lsb_reg, ®_val);
> > > + if (ret < 0)
> > > + return ret;
> > > +
> > > + ((u8 *)val)[1] = reg_val;
> >
> > These casts are not that nice... Is sparse even ok with this without
> > __force?
> > I didn't looked at the datasheet so I have no idea but is regmap_bulk_read()
> > an
> > option? It would simplify things.
>
> regmap_bulk_read() does work, thanks for the suggestion.
>
> >
> > > +
> > > + return 0;
> > > +}
> >
> > ...
> >
> > >
> > > ad2s1210_push_events(indio_dev, st->sample.fault, pf->timestamp);
> > > @@ -1299,9 +1397,33 @@ static const struct iio_info ad2s1210_info = {
> > > static int ad2s1210_setup_properties(struct ad2s1210_state *st)
> > > {
> > > struct device *dev = &st->sdev->dev;
> > > + const char *str_val;
> > > u32 val;
> > > int ret;
> > >
> > > + ret = device_property_read_string(dev, "adi,fixed-mode", &str_val);
> > > + if (ret == -EINVAL)
> > > + st->fixed_mode = -1;
> > > + else if (ret < 0)
> > > + return dev_err_probe(dev, ret,
> > > + "failed to read adi,fixed-mode property\n");
> > > + else {
> > > + if (strcmp(str_val, "position") == 0)
> > > + st->fixed_mode = MOD_POS;
> > > + else if (strcmp(str_val, "velocity") == 0)
> > > + st->fixed_mode = MOD_VEL;
> > > + else if (strcmp(str_val, "config") == 0)
> > > + st->fixed_mode = MOD_CONFIG;
> > > + else
> > > + return dev_err_probe(dev, -EINVAL,
> > > + "invalid adi,fixed-mode property value:
> > > %s\n",
> > > + str_val);
> > > +
> > > + if (st->fixed_mode != MOD_CONFIG)
> > > + return dev_err_probe(dev, -EINVAL,
> > > + "only adi,fixed-mode=\"config\" is
> > > supported\n");
> >
> > Why not?
> >
> > if (strcmp(str_val, "config"))
> > return dev_err_probe();
> >
> > st->fixed_mode = MOD_CONFIG;
> >
> > Am I missing something obvious?
>
> I made different error messages to differentiate between values not
> allowed by the device tree vs. not supported by the driver. I don't
> have a problem with simplifying it though.
>
> >
Yeah, I also don't feel too strong about it but, personally, I would just go to
the simpler form. For example, the message "only adi,fixed-mode=\"config\" is
supported" should already be sufficient and explicit about the error... Bah, up
to you :)
- Nuno Sá
>
>
