Re: [PATCH v1 14/15] dt-bindings: auxdisplay: Add Maxim MAX6958/6959

[Conor Dooley]

On Thu, Feb 08, 2024 at 08:16:56PM +0200, Andy Shevchenko wrote:
> On Thu, Feb 08, 2024 at 05:50:51PM +0000, Conor Dooley wrote:
> > On Thu, Feb 08, 2024 at 06:58:57PM +0200, Andy Shevchenko wrote:
> > > Add initial device tree documentation for Maxim MAX6958/6959.
> > 
> > Why "initial"? Are there elements this display that you've not
> > documented yet? 
> 
> s/documented/implemented/

Oh no, I meant documented. Whether or not you implement support in the
driver doesn't really matter, but you should endeavour to document as
much of the hardware as possible. Certainly simple things like
interrupts.

> There are features of the hardware that may need additional properties.
> 
> > > +title: MAX6958/6959 7-segment LED display controller with keyscan
> > 
> > > +properties:
> > > +  compatible:
> > > +    const: maxim,max6959
> > 
> > Where's the max6958's compatible? I don't see it in your driver either.
> 
> For now, see above, there is no need. 

I don't know what I am supposed to see above.

> Moreover, there is no need at all
> as we have autodetection mechanism. I don't see why we should have two
> compatible strings just for the sake of having them.
> 
> > It seems that the max6959 has some interrupt capabilities that are not
> > available on the max6958, so a dedicated compatible seems suitable to
> > me.
> 
> So, please clarify the DT's p.o.v. on the hardware that can be autodetected.
> Do we need to still have different compatible strings? What for? I don't
> see the need, sorry for my (silly) questions.

If there's an auto-detection mechanism, which is a valid justification,
you need to put it in the commit message. If you don't it just looks
like you're missing a compatible.
The advantage of a dedicated compatible is restricting the properties
that would only apply to either the 6958 or 6959 to just that device.

You've only partially documented these devices though, so it is not
clear how much of a divergence there'd actually be.

cheers,
Conor

Attachment: signature.asc
Description: PGP signature