Re: [PATCH] serial: sc16is7xx: improve regmap debugfs by using one regmap per port

[Hugo Villeneuve]

On Thu, 23 Nov 2023 23:37:33 +0200
Andy Shevchenko <andriy.shevchenko@xxxxxxxxx> wrote:

> On Mon, Oct 30, 2023 at 05:14:47PM -0400, Hugo Villeneuve wrote:
> > From: Hugo Villeneuve <hvilleneuve@xxxxxxxxxxxx>
> > 
> > With this current driver regmap implementation, it is hard to make sense
> > of the register addresses displayed using the regmap debugfs interface,
> > because they do not correspond to the actual register addresses documented
> > in the datasheet. For example, register 1 is displayed as registers 04 thru
> > 07:
> > 
> > $ cat /sys/kernel/debug/regmap/spi0.0/registers
> >   04: 10 -> Port 0, register offset 1
> >   05: 10 -> Port 1, register offset 1
> >   06: 00 -> Port 2, register offset 1 -> invalid
> >   07: 00 -> port 3, register offset 1 -> invalid
> >   ...
> > 
> > The reason is that bits 0 and 1 of the register address correspond to the
> > channel (port) bits, so the register address itself starts at bit 2, and we
> > must 'mentally' shift each register address by 2 bits to get its real
> > address/offset.
> > 
> > Also, only channels 0 and 1 are supported by the chip, so channel mask
> > combinations of 10b and 11b are invalid, and the display of these
> > registers is useless.
> > 
> > This patch adds a separate regmap configuration for each port, similar to
> > what is done in the max310x driver, so that register addresses displayed
> > match the register addresses in the chip datasheet. Also, each port now has
> > its own debugfs entry.
> > 
> > Example with new regmap implementation:
> > 
> > $ cat /sys/kernel/debug/regmap/spi0.0-port0/registers
> > 1: 10
> > 2: 01
> > 3: 00
> > ...
> > 
> > $ cat /sys/kernel/debug/regmap/spi0.0-port1/registers
> > 1: 10
> > 2: 01
> > 3: 00
> > 
> > As an added bonus, this also simplifies some operations (read/write/modify)
> > because it is no longer necessary to manually shift register addresses.
> 
> This change might be problematic, i.e. ...
> 
> ...
> 
> >  		regmap_update_bits(
> >  			s->regmap,
> > -			SC16IS7XX_IOCONTROL_REG << SC16IS7XX_REG_SHIFT,
> > +			SC16IS7XX_IOCONTROL_REG,
> >  			SC16IS7XX_IOCONTROL_MODEM_A_BIT |
> >  			SC16IS7XX_IOCONTROL_MODEM_B_BIT, s->mctrl_mask);
> 
> ...if this happens inside another regmap operation it might collide with this
> as there is no more shared locking (and if driver is going to be converted to
> use an external lock, the one in regmap might be disabled). But I haven't
> checked anyhow deeply this, so just a heads up for the potential issue.

Hi Andy,
are you refering to the above piece of code as the only location where
this could be problematic?

If yes, then it is located inside sc16is7xx_setup_mctrl_ports(), which
is called only during sc16is7xx_probe(), and I assume it should be ok.

> 
> ...
> 
> > -	ret = regmap_read(regmap,
> > -			  SC16IS7XX_LSR_REG << SC16IS7XX_REG_SHIFT, &val);
> > +	ret = regmap_read(regmaps[0], SC16IS7XX_LSR_REG, &val);
> 
> Here is a probe, most likely no issues.

Ok.

> 
> >  	if (ret < 0)
> >  		return -EPROBE_DEFER;
> 
> ...
> 
> > +static const char *sc16is7xx_regmap_name(unsigned int port_id)
> > +{
> > +	static char buf[6];
> > +
> > +	snprintf(buf, sizeof(buf), "port%d", port_id);
> 
> Should be %u.

Yes. I just noticed that Greg has applied the patch to its tty-testing
branch, I assume I should just send a new patch to fix it?

Thank you for the review,
Hugo.

 
> > +	return buf;
> > +}
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 
>