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

[Boris Brezillon <boris.brezillon@bootlin.com>]

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. What I can do is:

1/ provide a function to change the status prop in of.h
2/ let each driver call this function if they want to
3/ let the I2C core test the status prop again after the probe function
   has returned an error to determine whether the device (I mean struct
   i2c_client/device object) should be removed

Would that work for you?

Regards,

Boris

[1]https://elinux.org/images/c/cf/Power_ePAPR_APPROVED_v1.1.pdf
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel