Re: [PATCH v2 6/6] drm/panel: rpi-touchscreen: Set status to "fail" when ->probe() fails

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1.1: Type: text/plain, Size: 4728 bytes --]

On Fri, May 04, 2018 at 10:06:53AM +0200, Boris Brezillon wrote:
> Hi Rob,
> 
> On Thu, 3 May 2018 12:12:39 -0500
> Rob Herring <robh+dt@kernel.org> wrote:
> 
> > On Thu, May 3, 2018 at 11:40 AM, Boris Brezillon
> > <boris.brezillon@bootlin.com> wrote:
> > > The device might be described in the device tree but not connected to
> > > the I2C bus. Update the status property so that the DRM panel logic
> > > returns -ENODEV when someone tries to get the panel attached to this
> > > DT node.
> > >
> > > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
> > > ---
> > >  .../gpu/drm/panel/panel-raspberrypi-touchscreen.c  | 35 ++++++++++++++++++++++
> > >  1 file changed, 35 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c b/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c
> > > index 2c9c9722734f..b8fcb1acef75 100644
> > > --- a/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c
> > > +++ b/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c
> > > @@ -358,6 +358,39 @@ static const struct drm_panel_funcs rpi_touchscreen_funcs = {
> > >         .get_modes = rpi_touchscreen_get_modes,
> > >  };
> > >
> > > +static void rpi_touchscreen_set_status_fail(struct i2c_client *i2c)
> > > +{
> > > +       struct property *newprop;
> > > +
> > > +       newprop = kzalloc(sizeof(*newprop), GFP_KERNEL);
> > > +       if (!newprop)
> > > +               return;
> > > +
> > > +       newprop->name = kstrdup("status", GFP_KERNEL);
> > > +       if (!newprop->name)
> > > +               goto err;
> > > +
> > > +       newprop->value = kstrdup("fail", GFP_KERNEL);
> > > +       if (!newprop->value)
> > > +               goto err;
> > > +
> > > +       newprop->length = sizeof("fail");
> > > +
> > > +       if (of_update_property(i2c->dev.of_node, newprop))
> > > +               goto err;
> > > +  
> > 
> > As I mentioned on irc, can you make this a common DT function.
> 
> Yep, will move that to drivers/of/base.c and make it generic.
> 
> > 
> > I'm not sure if it matters that we set status to fail vs. disabled. I
> > somewhat prefer the latter as we already have other cases and I'd
> > rather the api not pass a string in. I can't think of any reason to
> > distinguish the difference between fail and disabled.
> 
> Well, I just read the ePAPR doc pointed by Thierry [1] (section 2.3.4),
> and "fail" seemed like a good match for what we are trying to express
> here: "we failed to communicate with the device in the probe function
> and want to mark it unusable", which is a bit different from "the
> device was explicitly disabled by the user".
> 
> Anyway, if you think "disabled" is more appropriate, I'll use that.
> 
> > 
> > > +       /* We intentionally leak the memory we allocate here, because the new
> > > +        * OF property might live longer than the underlying dev, so no way
> > > +        * we can use devm_kzalloc() here.
> > > +        */
> > > +       return;
> > > +
> > > +err:
> > > +       kfree(newprop->value);
> > > +       kfree(newprop->name);
> > > +       kfree(newprop);
> > > +}
> > > +
> > >  static int rpi_touchscreen_probe(struct i2c_client *i2c,
> > >                                  const struct i2c_device_id *id)
> > >  {
> > > @@ -382,6 +415,7 @@ static int rpi_touchscreen_probe(struct i2c_client *i2c,
> > >
> > >         ver = rpi_touchscreen_i2c_read(ts, REG_ID);
> > >         if (ver < 0) {
> > > +               rpi_touchscreen_set_status_fail(i2c);  
> > 
> > I've thought some more about this and I still think this should be
> > handled in the driver core or i2c core.
> > 
> > The reason is simple. I think the state of the system should be the
> > same after this as if you booted with 'status = "disabled"' for this
> > node. And that means the device should be removed completely because
> > we don't create struct device's for disabled nodes.
> 
> That was my feeling to when first discussing the issue with Daniel and
> Thierry on IRC, but after digging a bit in the code I'm no longer sure
> this is a good idea. At least, I don't think basing the decision to
> disable the device (or mark it unusable) based on the return value of
> the probe function is a good idea.

I'm not so sure about that. -ENODEV seems like a very suitable error
code to base that decision on. A random sampling of a handful of drivers
confirms that this is primarily used to report situations where it is
impossible for the device to ever be probed successfully, so might as
well just remove it.

At the very least I think it is worth proposing the patch and let Greg
and others weigh in.

Thierry

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel