Re: [RFC PATCH 3/5] i2c: core: add function to request an alias

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Luca,

On Tue, Jan 07, 2020 at 04:03:29PM +0100, Luca Ceresoli wrote:
> On 03/01/20 01:10, Laurent Pinchart wrote:
> > On Thu, Jan 02, 2020 at 11:27:57PM +0100, Luca Ceresoli wrote:
> >> On 02/01/20 22:13, Wolfram Sang wrote:
> >>>>> This looks quite inefficient, especially if the beginning of the range
> >>>>> is populated with devices. Furthermore, I think there's a high risk of
> >>>>> false negatives, as acquiring a free address and reprogramming the
> >>>>> client to make use of it are separate operations.
> >>>>
> >>>> Right. Applying the alias could raise other errors, thus one would need
> >>>> i2c_new_alias_device() to keep the alias locked until programming it has
> >>>> either failed or has been successfully programmed.
> >>>
> >>> Please see my reply to Laurent, I don't think it is racy. But please
> >>> elaborate if you think I am wrong.
> >>
> >> Uhm, you are right here, it's not racy. Sorry, I had read the code
> >> quickly and didn't notice the i2c_new_dummy_device() call.
> >>
> >> So this means if i2c_new_alias_device() succeeds but the caller later
> >> fails while applying the alias, then it has to call
> >> i2c_unregister_device() to free the alias. Correct?
> > 
> > I was wrong as well, sorry about that.
> > 
> >>>>> What happened to the idea of reporting busy address ranges in the
> >>>>> firmware (DT, ACPI, ...) ?
> >>>>
> >>>> Indeed that's how I remember it as well, and I'm a bit suspicious about
> >>>> sending out probe messages that might have side effects (even if the
> >>>> false negative issue mentioned by Laurent were solved). You know, I've
> >>>> been taught to "expect the worse" :) so I'd like to better understand
> >>>> what are the strong reasons in favor of probing, as well as the
> >>>> potential side effects.
> >>>
> >>> As I said to Laurent, too, I think the risk that a bus is not fully
> >>> described is higher than a device which does not respond to a read_byte.
> >>> In both cases, we would wrongly use an address in use.
> > 
> > I don't fully agree with this, I think we shouldn't impose a penalty on
> > every user because some device trees don't fully describe the hardware.
> > I think we should, at the very least, skip the probe and rely on DT if
> > DT explicitly states that all used addresses are listed. We discussed a
> > property to report addresses used by devices not described in DT, if
> > that property is listed I would prefer trusting DT.
> 
> It would be nice, but I'm not sure this is really doable. Say the DT for
> board X lists all the used slave addresses. Then the kernel would assume
> all the other addresses are available. But then somebody includes the DT
> of board X in the DT for product Z, based on board X + add-on board Y.
> Add-on board Y has 2 I2C chips, but only one is described in DT. Now the
> kernel still thinks it knows all the used address, but this is wrong.

That's the fault of the system integrator though. We can't prevent
people from making incorrect DT, and we shouldn't go to great length to
still support them.

> At my current pondering status, I think only two approaches are doable:
> either assuming all DTs fully describe the hardware (which is still a
> good goal to pursue, generally speaking) or use Wolfram's proposal. The
> difference between the two is the call to i2c_unlocked_read_byte_probe().
> 
> However a hybrid approach is to speak out loud if we get a response from
> an address that is not marked as busy, to invite the developers to fix
> their DT. In other words:
> 
>  ret = i2c_scan_for_client(adap, addr, i2c_unlocked_read_byte_probe);
>  if (ret == -ENODEV) {
>          alias = i2c_new_dummy_device(adap, addr);
>          dev_dbg(&adap->dev, "Found alias: 0x%x\n", addr);
>          break;
> +} else if (ret == 0) {
> +        dev_err(&adap->dev,
> +                "alien found at %02x, please add it to your DT!!!\n",
> +                addr);
>  }
> 
> Wolfram, do think this could work? Do we have all the addresses listed
> in DT marked as busy early enough?

-- 
Regards,

Laurent Pinchart