| Message ID | 1365711890-15168-1-git-send-email-abrestic@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Andrew, On Thu, Apr 11, 2013 at 1:24 PM, Andrew Bresticker <abrestic@chromium.org> wrote: > Platform LCD devices may need to do some device-specific > initialization before they can be used (regulator or GPIO setup, > for example), but currently the driver does not support any way of > doing this. This patch adds a probe() callback to plat_lcd_data > which platform LCD devices can set to indicate that device-specific > initialization is needed. > > Signed-off-by: Andrew Bresticker <abrestic@chromium.org> > --- > drivers/video/backlight/platform_lcd.c | 6 ++++++ > include/video/platform_lcd.h | 1 + > 2 files changed, 7 insertions(+) Reviewed-by: Doug Anderson <dianders@chromium.org> -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Friday, April 12, 2013 5:25 AM, Andrew Bresticker wrote: > > Platform LCD devices may need to do some device-specific > initialization before they can be used (regulator or GPIO setup, > for example), but currently the driver does not support any way of > doing this. This patch adds a probe() callback to plat_lcd_data > which platform LCD devices can set to indicate that device-specific > initialization is needed. > > Signed-off-by: Andrew Bresticker <abrestic@chromium.org> CC'ed Andrew Morton, It looks good. Acked-by: Jingoo Han <jg1.han@samsung.com> Best regards, Jingoo Han > --- > drivers/video/backlight/platform_lcd.c | 6 ++++++ > include/video/platform_lcd.h | 1 + > 2 files changed, 7 insertions(+) > > diff --git a/drivers/video/backlight/platform_lcd.c b/drivers/video/backlight/platform_lcd.c > index 17a6b83..f46180e 100644 > --- a/drivers/video/backlight/platform_lcd.c > +++ b/drivers/video/backlight/platform_lcd.c > @@ -86,6 +86,12 @@ static int platform_lcd_probe(struct platform_device *pdev) > return -EINVAL; > } > > + if (pdata->probe) { > + err = pdata->probe(pdata); > + if (err) > + return err; > + } > + > plcd = devm_kzalloc(&pdev->dev, sizeof(struct platform_lcd), > GFP_KERNEL); > if (!plcd) { > diff --git a/include/video/platform_lcd.h b/include/video/platform_lcd.h > index ad3bdfe..23864b2 100644 > --- a/include/video/platform_lcd.h > +++ b/include/video/platform_lcd.h > @@ -15,6 +15,7 @@ struct plat_lcd_data; > struct fb_info; > > struct plat_lcd_data { > + int (*probe)(struct plat_lcd_data *); > void (*set_power)(struct plat_lcd_data *, unsigned int power); > int (*match_fb)(struct plat_lcd_data *, struct fb_info *); > }; > -- > 1.8.1.3 > -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 11 Apr 2013 13:24:50 -0700 Andrew Bresticker <abrestic@chromium.org> wrote: > Platform LCD devices may need to do some device-specific > initialization before they can be used (regulator or GPIO setup, > for example), but currently the driver does not support any way of > doing this. This patch adds a probe() callback to plat_lcd_data > which platform LCD devices can set to indicate that device-specific > initialization is needed. > > index 17a6b83..f46180e 100644 > --- a/drivers/video/backlight/platform_lcd.c > +++ b/drivers/video/backlight/platform_lcd.c > @@ -86,6 +86,12 @@ static int platform_lcd_probe(struct platform_device *pdev) > return -EINVAL; > } > > + if (pdata->probe) { > + err = pdata->probe(pdata); > + if (err) > + return err; > + } > + > plcd = devm_kzalloc(&pdev->dev, sizeof(struct platform_lcd), > GFP_KERNEL); > if (!plcd) { > diff --git a/include/video/platform_lcd.h b/include/video/platform_lcd.h > index ad3bdfe..23864b2 100644 > --- a/include/video/platform_lcd.h > +++ b/include/video/platform_lcd.h > @@ -15,6 +15,7 @@ struct plat_lcd_data; > struct fb_info; > > struct plat_lcd_data { > + int (*probe)(struct plat_lcd_data *); > void (*set_power)(struct plat_lcd_data *, unsigned int power); > int (*match_fb)(struct plat_lcd_data *, struct fb_info *); > }; Sigh. I see that this entire interface has been lovingly undocumented. It's an invitation for us to grow bugs, incompatibilities, leaks, etc. Possible example: what happens if pdata->probe does some resource allocation or device initialisation which should be backed out if, say, platform_lcd_probe() later fails? -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday, April 16, 2013 8:07 AM, Andrew Morton wrote: > > On Thu, 11 Apr 2013 13:24:50 -0700 Andrew Bresticker <abrestic@chromium.org> wrote: > > > Platform LCD devices may need to do some device-specific > > initialization before they can be used (regulator or GPIO setup, > > for example), but currently the driver does not support any way of > > doing this. This patch adds a probe() callback to plat_lcd_data > > which platform LCD devices can set to indicate that device-specific > > initialization is needed. > > > > index 17a6b83..f46180e 100644 > > --- a/drivers/video/backlight/platform_lcd.c > > +++ b/drivers/video/backlight/platform_lcd.c > > @@ -86,6 +86,12 @@ static int platform_lcd_probe(struct platform_device *pdev) > > return -EINVAL; > > } > > > > + if (pdata->probe) { > > + err = pdata->probe(pdata); > > + if (err) > > + return err; > > + } > > + > > plcd = devm_kzalloc(&pdev->dev, sizeof(struct platform_lcd), > > GFP_KERNEL); > > if (!plcd) { > > diff --git a/include/video/platform_lcd.h b/include/video/platform_lcd.h > > index ad3bdfe..23864b2 100644 > > --- a/include/video/platform_lcd.h > > +++ b/include/video/platform_lcd.h > > @@ -15,6 +15,7 @@ struct plat_lcd_data; > > struct fb_info; > > > > struct plat_lcd_data { > > + int (*probe)(struct plat_lcd_data *); > > void (*set_power)(struct plat_lcd_data *, unsigned int power); > > int (*match_fb)(struct plat_lcd_data *, struct fb_info *); > > }; > > Sigh. I see that this entire interface has been lovingly undocumented. > It's an invitation for us to grow bugs, incompatibilities, leaks, etc. > > Possible example: what happens if pdata->probe does some resource > allocation or device initialisation which should be backed out if, say, > platform_lcd_probe() later fails? Hi Andrew, I agree with you. Indeed, the documentation is necessary. Andrew Bresticker, Would you make the document? It would be very helpful. Best regards, Jingoo Han -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/video/backlight/platform_lcd.c b/drivers/video/backlight/platform_lcd.c index 17a6b83..f46180e 100644 --- a/drivers/video/backlight/platform_lcd.c +++ b/drivers/video/backlight/platform_lcd.c @@ -86,6 +86,12 @@ static int platform_lcd_probe(struct platform_device *pdev) return -EINVAL; } + if (pdata->probe) { + err = pdata->probe(pdata); + if (err) + return err; + } + plcd = devm_kzalloc(&pdev->dev, sizeof(struct platform_lcd), GFP_KERNEL); if (!plcd) { diff --git a/include/video/platform_lcd.h b/include/video/platform_lcd.h index ad3bdfe..23864b2 100644 --- a/include/video/platform_lcd.h +++ b/include/video/platform_lcd.h @@ -15,6 +15,7 @@ struct plat_lcd_data; struct fb_info; struct plat_lcd_data { + int (*probe)(struct plat_lcd_data *); void (*set_power)(struct plat_lcd_data *, unsigned int power); int (*match_fb)(struct plat_lcd_data *, struct fb_info *); };
Platform LCD devices may need to do some device-specific initialization before they can be used (regulator or GPIO setup, for example), but currently the driver does not support any way of doing this. This patch adds a probe() callback to plat_lcd_data which platform LCD devices can set to indicate that device-specific initialization is needed. Signed-off-by: Andrew Bresticker <abrestic@chromium.org> --- drivers/video/backlight/platform_lcd.c | 6 ++++++ include/video/platform_lcd.h | 1 + 2 files changed, 7 insertions(+)