Re: [PATCH v2 3/4] gpio: adp5585: Add Analog Devices ADP5585 support

[Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>]

On Wed, May 29, 2024 at 09:16:43AM +0300, Andy Shevchenko wrote:
> On Tue, May 28, 2024 at 11:20 PM Laurent Pinchart wrote:
> > On Tue, May 28, 2024 at 10:36:06PM +0300, Andy Shevchenko wrote:
> 
> ...
> 
> > > > +   bit = off * 2 + (off > 5 ? 4 : 0);
> > >
> > > Right, but can you use >= 6 here which immediately follows to the next
> > > question, i.e. why not use bank in this conditional?
> >
> > The ADP5585_BANK() macro is meant to be used with ADP5585_BIT(), for a
> > set of registers with the same layout. Here the layout is different, the
> > registers contain multi-bit fields. I can't use ADP5585_BIT(), so I'd
> > rather not use ADP5585_BANK() either. I have decided to use > 5 instead
> > of >= 6 to match the R5 field name in the comment above:
> >
> >         /*
> >          * The bias configuration fields are 2 bits wide and laid down in
> >          * consecutive registers ADP5585_RPULL_CONFIG_*, with a hole of 4 bits
> >          * after R5.
> >          */
> 
> First of all, the 5 sounds misleading as one needs to think about "how
> many are exactly per the register" and the answer AFAIU is 6. >= 6
> shows this. Second, I haven't mentioned _BANK(), what I meant is
> something to
> 
>   unsigned int bank = ... >= 6 ? : ;

That doesn't reflect the organisation of the bits in the registers. If
you're interested, please check the datasheet.

> ...
> 
> > > > +   struct adp5585_dev *adp5585 = dev_get_drvdata(pdev->dev.parent);
> > >
> > > (see below)
> > >
> > > > +   struct adp5585_gpio_dev *adp5585_gpio;
> > > > +   struct device *dev = &pdev->dev;
> > >
> > >       struct adp5585_dev *adp5585 = dev_get_drvdata(dev->parent);
> >
> > I prefer keeping the current ordering, with long lines first, I think
> > that's more readable.
> 
> Does the compiler optimise these two?

If anyone is interested in figuring out, I'll let them test :-)

> > > > +   struct gpio_chip *gc;
> > > > +   int ret;
> 
> ...
> 
> > > > +   device_set_of_node_from_dev(dev, dev->parent);
> > >
> > > Why not device_set_node()?
> >
> > Because device_set_of_node_from_dev() is meant for this exact use case,
> > where the same node is used for multiple devices. It also puts any
> > previous dev->of_node, ensuring proper refcounting when devices are
> > unbound and rebound, without being deleted.
> 
> When will the refcount be dropped (in case of removal of this device)?
> Or you mean it shouldn't?

Any refcount taken on the OF node needs to be dropped. The device core
only drops the refcount when the device is being deleted, not when
there's an unbind-rebind cycle without deletion of the device (as
happens for instance when the module is unloaded and reloaded). This has
to be handled by the driver. device_set_of_node_from_dev() handles it.

-- 
Regards,

Laurent Pinchart