Re: [PATCH v3 7/8] serial: exar: add CTI specific setup code

[Greg Kroah-Hartman]

On Wed, Apr 17, 2024 at 09:17:35AM -0400, Parker Newman wrote:
> On Wed, 17 Apr 2024 13:24:20 +0200
> Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
> 
> > On Tue, Apr 16, 2024 at 08:55:34AM -0400, Parker Newman wrote:
> > >  struct exar8250 {
> > >  	unsigned int		nr;
> > > +	unsigned int		osc_freq;
> > > +	struct pci_dev		*pcidev;
> > > +	struct device		*dev;
> >
> > Why do you need both a pci_dev and a device?  Aren't they the same thing
> > here?
> >
> 
> I added dev to make the prints cleaner. I personally prefer:
> 
> 	dev_err(priv->dev, ...);
> to
> 	dev_err(&priv->pcidev->dev, ...);
> 
> or to adding a:
> 	struct device *dev = &priv->pcidev->dev;
> 
> to every function just for printing.
> 
> However, I do understand your point. I can drop dev if you prefer.

Pick one and stick with it, no need for both.  I too like your first
option, and if you never need the pci device pointer, just stick with
"struct dev".  Note, properly reference count it in case you think it
could go away from underneath you (some paths it could, so be careful.)

> > > +static int cti_tristate_disable(struct exar8250 *priv, unsigned int port_num)
> > > +{
> > > +	return _cti_set_tristate(priv, port_num, false);
> > > +}
> >
> > Do you ever call _cti_set_tristate() with "true"?
> >
> 
> Currently no. However, re-enabling tristate via a custom ioctl was a feature in
> our old out-of-tree driver (which was created prior to linux RS485 support).
> 
> I am not sure how it would be activated now, but I left enabling tristate as an
> option in to make it easier down the line when we need it.
> 
> I can add a note to the patch or remove it if you would prefer.

Just remove it for now, no need to ever add code you "might need in the
future", just add that then, when you need it.  Code for what you need
to do now, that makes things simpler and removes extra stuff that you
would have to force others to review :)

thanks,

greg k-h