* Re: [PATCH v2 18/24] video: da8xx-fb: minimal dt support
From: Darren Etheridge @ 2013-08-01 13:36 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1375208791-15781-19-git-send-email-detheridge@ti.com>
Tomi Valkeinen <tomi.valkeinen@ti.com> wrote on Wed [2013-Jul-31 13:19:01 +0300]:
> > +Required properties:
> > +- compatible:
> > + DA830 - "ti,da830-lcdc"
> > + AM335x SoC's - "ti,am3352-lcdc", "ti,da830-lcdc"
>
> I'm not totally sure about this, but how I understand the compatible
> property, the above reads as: "this device is ti,am3352-lcdc and it's
> fully compatible with ti,da830-lcdc". I.e. if the kernel has a driver
> for da830-lcdc, it should work with AM335x also (without any of the new
> features in AM335x, obviously). Which I believe is not the case, as the
> point of this series is to add the AM335x support.
>
> Or should the current da830-lcdc work with AM335x also, but it just
> didn't because there were bugs in da830-lcdc?
>
> Tomi
>
>
OK I agree there is something wrong here, for one I don't think setting
ti,am3352-lcdc would do anything anyway given the driver only reports
.compatible with ti,da830-lcdc so at the very least the document is
wrong. I will look into this and decide what is the best way of
resolving this. I will go ahead and submit the series without the DT
support anyway and then I will look into this.
Darren
^ permalink raw reply
* Re: [PATCHv3 3/3] fb: backlight: HX8357: Add HX8369 support
From: Tomi Valkeinen @ 2013-08-01 12:39 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1375346437-18991-4-git-send-email-maxime.ripard@free-electrons.com>
[-- Attachment #1: Type: text/plain, Size: 1563 bytes --]
Hi,
On 01/08/13 11:40, Maxime Ripard wrote:
> From: Alexandre Belloni <alexandre.belloni@free-electrons.com>
>
> Add support for the Himax HX8369 controller as it is quite similar to the
> hx8357.
>
> Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> Acked-by: Jingoo Han <jg1.han@samsung.com>
> ---
> drivers/video/backlight/hx8357.c | 219 ++++++++++++++++++++++++++++++++++++---
> 1 file changed, 204 insertions(+), 15 deletions(-)
I don't have comments about this patch as such, but hx8357.c in general:
- The commands used look like MIPI DCS commands. We have defined those
in include/video/mipi_display.h.
- The command arrays could be const
- I don't like command arrays. Rather than having array for
"HX8357_SET_COLUMN_ADDRESS, 0x00, 0x00, 0x01, 0x3f,", I'd much more like
a function hx8357_set_column_address(int x1, int x2) or such.
- Looking at the driver makes me think it resembles very much the panel
driver(s) we have for OMAP. If only CDF was ready... ;)
Those said, I don't see a problem with applying this. We could clean up
later those things mentioned above, but maybe it's better to do that
when moving to CDF.
For this and the two other patches:
Acked-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
I can apply these to my 3.12-fbdev branch some day soon, if
Jean-Christophe hasn't come back yet (I'd normally rather deal only with
trivial patches, and leave the rest to Jean-Christophe).
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]
^ permalink raw reply
* Re: [patch -next] fb: fix recent breakage in correct_chipset()
From: Dan Carpenter @ 2013-08-01 11:50 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <20130702062821.GC24410@elgon.mountain>
On Thu, Aug 01, 2013 at 11:31:07AM +0200, Geert Uytterhoeven wrote:
> On Tue, Jul 2, 2013 at 8:28 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > The 6e36308a6f "fb: fix atyfb build warning" isn't right. It makes all
> > the indexes off by one. This patch reverts it and casts the
> > ARRAY_SIZE() to int to silence the build warning.
> >
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> >
> > diff --git a/drivers/video/aty/atyfb_base.c b/drivers/video/aty/atyfb_base.c
> > index a89c15d..9b0f12c 100644
> > --- a/drivers/video/aty/atyfb_base.c
> > +++ b/drivers/video/aty/atyfb_base.c
> > @@ -435,8 +435,8 @@ static int correct_chipset(struct atyfb_par *par)
> > const char *name;
> > int i;
> >
> > - for (i = ARRAY_SIZE(aty_chips); i > 0; i--)
> > - if (par->pci_id = aty_chips[i - 1].pci_id)
> > + for (i = (int)ARRAY_SIZE(aty_chips) - 1; i >= 0; i--)
> > + if (par->pci_id = aty_chips[i].pci_id)
> > break;
> >
> > if (i < 0)
>
> Sorry for chiming in late, but can't we just revert the order of the loop
> iteration and change i from int to size_t instead of adding a cast?
>
Nope. That would be a nearly-forever loop.
regards,
dan carpenter
^ permalink raw reply
* RE: [PATCH v2 10/24] video: da8xx-fb: fb_set_par support
From: Etheridge, Darren @ 2013-08-01 11:12 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1375208791-15781-11-git-send-email-detheridge@ti.com>
>>> +static int da8xxfb_set_par(struct fb_info *info) {
>>> + struct da8xx_fb_par *par = info->par;
>>> + int ret;
>>> + bool raster = da8xx_fb_is_raster_enabled();
>>> +
>>> + if (raster)
>>> + lcd_disable_raster(true);
>>> + else
>>> + lcd_disable_raster(false);
>>
>> This looks odd. If raster is enabled, you disable it. And if raster is disabled,
>> you disable it.
>
> I corrected this one in patch 0011 - I agree this code is very confusing.
>
>In patch 11 you add the enum. I wasn't referring to that. My point was
>that even if raster is already disabled,
>lcd_disable_raster(dont-wait-for-framedone) is called.
Oh I see, yes you are absolutely right - I will check into what the intent of this code was and see if it is even necessary.
^ permalink raw reply
* Re: [patch -next] fb: fix recent breakage in correct_chipset()
From: Geert Uytterhoeven @ 2013-08-01 9:31 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <20130702062821.GC24410@elgon.mountain>
On Tue, Jul 2, 2013 at 8:28 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> The 6e36308a6f "fb: fix atyfb build warning" isn't right. It makes all
> the indexes off by one. This patch reverts it and casts the
> ARRAY_SIZE() to int to silence the build warning.
>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>
> diff --git a/drivers/video/aty/atyfb_base.c b/drivers/video/aty/atyfb_base.c
> index a89c15d..9b0f12c 100644
> --- a/drivers/video/aty/atyfb_base.c
> +++ b/drivers/video/aty/atyfb_base.c
> @@ -435,8 +435,8 @@ static int correct_chipset(struct atyfb_par *par)
> const char *name;
> int i;
>
> - for (i = ARRAY_SIZE(aty_chips); i > 0; i--)
> - if (par->pci_id = aty_chips[i - 1].pci_id)
> + for (i = (int)ARRAY_SIZE(aty_chips) - 1; i >= 0; i--)
> + if (par->pci_id = aty_chips[i].pci_id)
> break;
>
> if (i < 0)
Sorry for chiming in late, but can't we just revert the order of the loop
iteration and change i from int to size_t instead of adding a cast?
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
* [PATCHv3 3/3] fb: backlight: HX8357: Add HX8369 support
From: Maxime Ripard @ 2013-08-01 8:40 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1375346437-18991-1-git-send-email-maxime.ripard@free-electrons.com>
From: Alexandre Belloni <alexandre.belloni@free-electrons.com>
Add support for the Himax HX8369 controller as it is quite similar to the
hx8357.
Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
Acked-by: Jingoo Han <jg1.han@samsung.com>
---
drivers/video/backlight/hx8357.c | 219 ++++++++++++++++++++++++++++++++++++---
1 file changed, 204 insertions(+), 15 deletions(-)
diff --git a/drivers/video/backlight/hx8357.c b/drivers/video/backlight/hx8357.c
index 47237fa..c7af8c4 100644
--- a/drivers/video/backlight/hx8357.c
+++ b/drivers/video/backlight/hx8357.c
@@ -71,6 +71,18 @@
#define HX8357_SET_POWER_NORMAL 0xd2
#define HX8357_SET_PANEL_RELATED 0xe9
+#define HX8369_SET_DISPLAY_BRIGHTNESS 0x51
+#define HX8369_WRITE_CABC_DISPLAY_VALUE 0x53
+#define HX8369_WRITE_CABC_BRIGHT_CTRL 0x55
+#define HX8369_WRITE_CABC_MIN_BRIGHTNESS 0x5e
+#define HX8369_SET_POWER 0xb1
+#define HX8369_SET_DISPLAY_MODE 0xb2
+#define HX8369_SET_DISPLAY_WAVEFORM_CYC 0xb4
+#define HX8369_SET_VCOM 0xb6
+#define HX8369_SET_EXTENSION_COMMAND 0xb9
+#define HX8369_SET_GIP 0xd5
+#define HX8369_SET_GAMMA_CURVE_RELATED 0xe0
+
struct hx8357_data {
unsigned im_pins[HX8357_NUM_IM_PINS];
unsigned reset;
@@ -144,6 +156,61 @@ static u8 hx8357_seq_display_mode[] = {
HX8357_SET_DISPLAY_MODE_RGB_INTERFACE,
};
+static u8 hx8369_seq_write_CABC_min_brightness[] = {
+ HX8369_WRITE_CABC_MIN_BRIGHTNESS, 0x00,
+};
+
+static u8 hx8369_seq_write_CABC_control[] = {
+ HX8369_WRITE_CABC_DISPLAY_VALUE, 0x24,
+};
+
+static u8 hx8369_seq_set_display_brightness[] = {
+ HX8369_SET_DISPLAY_BRIGHTNESS, 0xFF,
+};
+
+static u8 hx8369_seq_write_CABC_control_setting[] = {
+ HX8369_WRITE_CABC_BRIGHT_CTRL, 0x02,
+};
+
+static u8 hx8369_seq_extension_command[] = {
+ HX8369_SET_EXTENSION_COMMAND, 0xff, 0x83, 0x69,
+};
+
+static u8 hx8369_seq_display_related[] = {
+ HX8369_SET_DISPLAY_MODE, 0x00, 0x2b, 0x03, 0x03, 0x70, 0x00,
+ 0xff, 0x00, 0x00, 0x00, 0x00, 0x03, 0x03, 0x00, 0x01,
+};
+
+static u8 hx8369_seq_panel_waveform_cycle[] = {
+ HX8369_SET_DISPLAY_WAVEFORM_CYC, 0x0a, 0x1d, 0x80, 0x06, 0x02,
+};
+
+static u8 hx8369_seq_set_address_mode[] = {
+ HX8357_SET_ADDRESS_MODE, 0x00,
+};
+
+static u8 hx8369_seq_vcom[] = {
+ HX8369_SET_VCOM, 0x3e, 0x3e,
+};
+
+static u8 hx8369_seq_gip[] = {
+ HX8369_SET_GIP, 0x00, 0x01, 0x03, 0x25, 0x01, 0x02, 0x28, 0x70,
+ 0x11, 0x13, 0x00, 0x00, 0x40, 0x26, 0x51, 0x37, 0x00, 0x00, 0x71,
+ 0x35, 0x60, 0x24, 0x07, 0x0f, 0x04, 0x04,
+};
+
+static u8 hx8369_seq_power[] = {
+ HX8369_SET_POWER, 0x01, 0x00, 0x34, 0x03, 0x00, 0x11, 0x11, 0x32,
+ 0x2f, 0x3f, 0x3f, 0x01, 0x3a, 0x01, 0xe6, 0xe6, 0xe6, 0xe6, 0xe6,
+};
+
+static u8 hx8369_seq_gamma_curve_related[] = {
+ HX8369_SET_GAMMA_CURVE_RELATED, 0x00, 0x0d, 0x19, 0x2f, 0x3b, 0x3d,
+ 0x2e, 0x4a, 0x08, 0x0e, 0x0f, 0x14, 0x16, 0x14, 0x14, 0x14, 0x1e,
+ 0x00, 0x0d, 0x19, 0x2f, 0x3b, 0x3d, 0x2e, 0x4a, 0x08, 0x0e, 0x0f,
+ 0x14, 0x16, 0x14, 0x14, 0x14, 0x1e,
+};
+
static int hx8357_spi_write_then_read(struct lcd_device *lcdev,
u8 *txbuf, u16 txlen,
u8 *rxbuf, u16 rxlen)
@@ -220,6 +287,10 @@ static int hx8357_enter_standby(struct lcd_device *lcdev)
if (ret < 0)
return ret;
+ /*
+ * The controller needs 120ms when entering in sleep mode before we can
+ * send the command to go off sleep mode
+ */
msleep(120);
return 0;
@@ -233,6 +304,10 @@ static int hx8357_exit_standby(struct lcd_device *lcdev)
if (ret < 0)
return ret;
+ /*
+ * The controller needs 120ms when exiting from sleep mode before we
+ * can send the command to enter in sleep mode
+ */
msleep(120);
ret = hx8357_spi_write_byte(lcdev, HX8357_SET_DISPLAY_ON);
@@ -242,6 +317,21 @@ static int hx8357_exit_standby(struct lcd_device *lcdev)
return 0;
}
+static void hx8357_lcd_reset(struct lcd_device *lcdev)
+{
+ struct hx8357_data *lcd = lcd_get_data(lcdev);
+
+ /* Reset the screen */
+ gpio_set_value(lcd->reset, 1);
+ usleep_range(10000, 12000);
+ gpio_set_value(lcd->reset, 0);
+ usleep_range(10000, 12000);
+ gpio_set_value(lcd->reset, 1);
+
+ /* The controller needs 120ms to recover from reset */
+ msleep(120);
+}
+
static int hx8357_lcd_init(struct lcd_device *lcdev)
{
struct hx8357_data *lcd = lcd_get_data(lcdev);
@@ -257,14 +347,6 @@ static int hx8357_lcd_init(struct lcd_device *lcdev)
gpio_set_value_cansleep(lcd->im_pins[2], 1);
}
- /* Reset the screen */
- gpio_set_value(lcd->reset, 1);
- usleep_range(10000, 12000);
- gpio_set_value(lcd->reset, 0);
- usleep_range(10000, 12000);
- gpio_set_value(lcd->reset, 1);
- msleep(120);
-
ret = hx8357_spi_write_array(lcdev, hx8357_seq_power,
ARRAY_SIZE(hx8357_seq_power));
if (ret < 0)
@@ -344,6 +426,9 @@ static int hx8357_lcd_init(struct lcd_device *lcdev)
if (ret < 0)
return ret;
+ /*
+ * The controller needs 120ms to fully recover from exiting sleep mode
+ */
msleep(120);
ret = hx8357_spi_write_byte(lcdev, HX8357_SET_DISPLAY_ON);
@@ -359,6 +444,96 @@ static int hx8357_lcd_init(struct lcd_device *lcdev)
return 0;
}
+static int hx8369_lcd_init(struct lcd_device *lcdev)
+{
+ int ret;
+
+ ret = hx8357_spi_write_array(lcdev, hx8369_seq_extension_command,
+ ARRAY_SIZE(hx8369_seq_extension_command));
+ if (ret < 0)
+ return ret;
+ usleep_range(10000, 12000);
+
+ ret = hx8357_spi_write_array(lcdev, hx8369_seq_display_related,
+ ARRAY_SIZE(hx8369_seq_display_related));
+ if (ret < 0)
+ return ret;
+
+ ret = hx8357_spi_write_array(lcdev, hx8369_seq_panel_waveform_cycle,
+ ARRAY_SIZE(hx8369_seq_panel_waveform_cycle));
+ if (ret < 0)
+ return ret;
+
+ ret = hx8357_spi_write_array(lcdev, hx8369_seq_set_address_mode,
+ ARRAY_SIZE(hx8369_seq_set_address_mode));
+ if (ret < 0)
+ return ret;
+
+ ret = hx8357_spi_write_array(lcdev, hx8369_seq_vcom,
+ ARRAY_SIZE(hx8369_seq_vcom));
+ if (ret < 0)
+ return ret;
+
+ ret = hx8357_spi_write_array(lcdev, hx8369_seq_gip,
+ ARRAY_SIZE(hx8369_seq_gip));
+ if (ret < 0)
+ return ret;
+
+ ret = hx8357_spi_write_array(lcdev, hx8369_seq_power,
+ ARRAY_SIZE(hx8369_seq_power));
+ if (ret < 0)
+ return ret;
+
+ ret = hx8357_spi_write_byte(lcdev, HX8357_EXIT_SLEEP_MODE);
+ if (ret < 0)
+ return ret;
+
+ /*
+ * The controller needs 120ms to fully recover from exiting sleep mode
+ */
+ msleep(120);
+
+ ret = hx8357_spi_write_array(lcdev, hx8369_seq_gamma_curve_related,
+ ARRAY_SIZE(hx8369_seq_gamma_curve_related));
+ if (ret < 0)
+ return ret;
+
+ ret = hx8357_spi_write_byte(lcdev, HX8357_EXIT_SLEEP_MODE);
+ if (ret < 0)
+ return ret;
+ usleep_range(1000, 1200);
+
+ ret = hx8357_spi_write_array(lcdev, hx8369_seq_write_CABC_control,
+ ARRAY_SIZE(hx8369_seq_write_CABC_control));
+ if (ret < 0)
+ return ret;
+ usleep_range(10000, 12000);
+
+ ret = hx8357_spi_write_array(lcdev,
+ hx8369_seq_write_CABC_control_setting,
+ ARRAY_SIZE(hx8369_seq_write_CABC_control_setting));
+ if (ret < 0)
+ return ret;
+
+ ret = hx8357_spi_write_array(lcdev,
+ hx8369_seq_write_CABC_min_brightness,
+ ARRAY_SIZE(hx8369_seq_write_CABC_min_brightness));
+ if (ret < 0)
+ return ret;
+ usleep_range(10000, 12000);
+
+ ret = hx8357_spi_write_array(lcdev, hx8369_seq_set_display_brightness,
+ ARRAY_SIZE(hx8369_seq_set_display_brightness));
+ if (ret < 0)
+ return ret;
+
+ ret = hx8357_spi_write_byte(lcdev, HX8357_SET_DISPLAY_ON);
+ if (ret < 0)
+ return ret;
+
+ return 0;
+}
+
#define POWER_IS_ON(pwr) ((pwr) <= FB_BLANK_NORMAL)
static int hx8357_set_power(struct lcd_device *lcdev, int power)
@@ -391,10 +566,24 @@ static struct lcd_ops hx8357_ops = {
.get_power = hx8357_get_power,
};
+static const struct of_device_id hx8357_dt_ids[] = {
+ {
+ .compatible = "himax,hx8357",
+ .data = hx8357_lcd_init,
+ },
+ {
+ .compatible = "himax,hx8369",
+ .data = hx8369_lcd_init,
+ },
+ {},
+};
+MODULE_DEVICE_TABLE(of, hx8357_dt_ids);
+
static int hx8357_probe(struct spi_device *spi)
{
struct lcd_device *lcdev;
struct hx8357_data *lcd;
+ const struct of_device_id *match;
int i, ret;
lcd = devm_kzalloc(&spi->dev, sizeof(*lcd), GFP_KERNEL);
@@ -411,6 +600,10 @@ static int hx8357_probe(struct spi_device *spi)
lcd->spi = spi;
+ match = of_match_device(hx8357_dt_ids, &spi->dev);
+ if (!match || !match->data)
+ return -EINVAL;
+
lcd->reset = of_get_named_gpio(spi->dev.of_node, "gpios-reset", 0);
if (!gpio_is_valid(lcd->reset)) {
dev_err(&spi->dev, "Missing dt property: gpios-reset\n");
@@ -462,7 +655,9 @@ static int hx8357_probe(struct spi_device *spi)
}
spi_set_drvdata(spi, lcdev);
- ret = hx8357_lcd_init(lcdev);
+ hx8357_lcd_reset(lcdev);
+
+ ret = ((int (*)(struct lcd_device *))match->data)(lcdev);
if (ret) {
dev_err(&spi->dev, "Couldn't initialize panel\n");
goto init_error;
@@ -485,12 +680,6 @@ static int hx8357_remove(struct spi_device *spi)
return 0;
}
-static const struct of_device_id hx8357_dt_ids[] = {
- { .compatible = "himax,hx8357" },
- {},
-};
-MODULE_DEVICE_TABLE(of, hx8357_dt_ids);
-
static struct spi_driver hx8357_driver = {
.probe = hx8357_probe,
.remove = hx8357_remove,
--
1.8.3.4
^ permalink raw reply related
* [PATCHv3 2/3] video: hx8357: Make IM pins optional
From: Maxime Ripard @ 2013-08-01 8:40 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1375346437-18991-1-git-send-email-maxime.ripard@free-electrons.com>
The IM pins of the HX8357 controller are used to define the interface
used to feed pixel stream to the LCD panel.
Most of the time, these pins are directly routed to either the ground or
the VCC to set their values.
Remove the need to assign GPIOs to these pins when we are in such a case.
Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
Acked-by: Jingoo Han <jg1.han@samsung.com>
---
drivers/video/backlight/hx8357.c | 52 ++++++++++++++++++++++++----------------
1 file changed, 31 insertions(+), 21 deletions(-)
diff --git a/drivers/video/backlight/hx8357.c b/drivers/video/backlight/hx8357.c
index a0482b5..47237fa 100644
--- a/drivers/video/backlight/hx8357.c
+++ b/drivers/video/backlight/hx8357.c
@@ -76,6 +76,7 @@ struct hx8357_data {
unsigned reset;
struct spi_device *spi;
int state;
+ bool use_im_pins;
};
static u8 hx8357_seq_power[] = {
@@ -250,9 +251,11 @@ static int hx8357_lcd_init(struct lcd_device *lcdev)
* Set the interface selection pins to SPI mode, with three
* wires
*/
- gpio_set_value_cansleep(lcd->im_pins[0], 1);
- gpio_set_value_cansleep(lcd->im_pins[1], 0);
- gpio_set_value_cansleep(lcd->im_pins[2], 1);
+ if (lcd->use_im_pins) {
+ gpio_set_value_cansleep(lcd->im_pins[0], 1);
+ gpio_set_value_cansleep(lcd->im_pins[1], 0);
+ gpio_set_value_cansleep(lcd->im_pins[2], 1);
+ }
/* Reset the screen */
gpio_set_value(lcd->reset, 1);
@@ -424,25 +427,32 @@ static int hx8357_probe(struct spi_device *spi)
return -EINVAL;
}
- for (i = 0; i < HX8357_NUM_IM_PINS; i++) {
- lcd->im_pins[i] = of_get_named_gpio(spi->dev.of_node,
- "im-gpios", i);
- if (lcd->im_pins[i] = -EPROBE_DEFER) {
- dev_info(&spi->dev, "GPIO requested is not here yet, deferring the probe\n");
- return -EPROBE_DEFER;
- }
- if (!gpio_is_valid(lcd->im_pins[i])) {
- dev_err(&spi->dev, "Missing dt property: im-gpios\n");
- return -EINVAL;
- }
-
- ret = devm_gpio_request_one(&spi->dev, lcd->im_pins[i],
- GPIOF_OUT_INIT_LOW, "im_pins");
- if (ret) {
- dev_err(&spi->dev, "failed to request gpio %d: %d\n",
- lcd->im_pins[i], ret);
- return -EINVAL;
+ if (of_find_property(spi->dev.of_node, "im-gpios", NULL)) {
+ lcd->use_im_pins = 1;
+
+ for (i = 0; i < HX8357_NUM_IM_PINS; i++) {
+ lcd->im_pins[i] = of_get_named_gpio(spi->dev.of_node,
+ "im-gpios", i);
+ if (lcd->im_pins[i] = -EPROBE_DEFER) {
+ dev_info(&spi->dev, "GPIO requested is not here yet, deferring the probe\n");
+ return -EPROBE_DEFER;
+ }
+ if (!gpio_is_valid(lcd->im_pins[i])) {
+ dev_err(&spi->dev, "Missing dt property: im-gpios\n");
+ return -EINVAL;
+ }
+
+ ret = devm_gpio_request_one(&spi->dev, lcd->im_pins[i],
+ GPIOF_OUT_INIT_LOW,
+ "im_pins");
+ if (ret) {
+ dev_err(&spi->dev, "failed to request gpio %d: %d\n",
+ lcd->im_pins[i], ret);
+ return -EINVAL;
+ }
}
+ } else {
+ lcd->use_im_pins = 0;
}
lcdev = lcd_device_register("mxsfb", &spi->dev, lcd, &hx8357_ops);
--
1.8.3.4
^ permalink raw reply related
* [PATCHv3 1/3] video: mxsfb: fix color settings for 18bit data bus and 32bpp
From: Maxime Ripard @ 2013-08-01 8:40 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1375346437-18991-1-git-send-email-maxime.ripard@free-electrons.com>
From: Hector Palacios <hector.palacios@digi.com>
For a combination of 18bit LCD data bus width and a color
mode of 32bpp, the driver was setting the color mapping to
rgb666, which is wrong, as the color in memory realy has an
rgb888 layout.
This patch also removes the setting of flag CTRL_DF24 that
makes the driver dimiss the upper 2 bits when handling 32/24bpp
colors in a diplay with 18bit data bus width. This flag made
true color images display wrong in such configurations.
Finally, the color mapping rgb666 has also been removed as nobody
is using it and high level applications like Qt5 cannot work
with it either.
Reference: https://lkml.org/lkml/2013/5/23/220
Signed-off-by: Hector Palacios <hector.palacios@digi.com>
Acked-by: Juergen Beisert <jbe@pengutronix.de>
Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com>
Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
drivers/video/mxsfb.c | 26 --------------------------
1 file changed, 26 deletions(-)
diff --git a/drivers/video/mxsfb.c b/drivers/video/mxsfb.c
index 3ba3771..dc09ebe 100644
--- a/drivers/video/mxsfb.c
+++ b/drivers/video/mxsfb.c
@@ -239,24 +239,6 @@ static const struct fb_bitfield def_rgb565[] = {
}
};
-static const struct fb_bitfield def_rgb666[] = {
- [RED] = {
- .offset = 16,
- .length = 6,
- },
- [GREEN] = {
- .offset = 8,
- .length = 6,
- },
- [BLUE] = {
- .offset = 0,
- .length = 6,
- },
- [TRANSP] = { /* no support for transparency */
- .length = 0,
- }
-};
-
static const struct fb_bitfield def_rgb888[] = {
[RED] = {
.offset = 16,
@@ -309,9 +291,6 @@ static int mxsfb_check_var(struct fb_var_screeninfo *var,
break;
case STMLCDIF_16BIT:
case STMLCDIF_18BIT:
- /* 24 bit to 18 bit mapping */
- rgb = def_rgb666;
- break;
case STMLCDIF_24BIT:
/* real 24 bit */
rgb = def_rgb888;
@@ -453,11 +432,6 @@ static int mxsfb_set_par(struct fb_info *fb_info)
return -EINVAL;
case STMLCDIF_16BIT:
case STMLCDIF_18BIT:
- /* 24 bit to 18 bit mapping */
- ctrl |= CTRL_DF24; /* ignore the upper 2 bits in
- * each colour component
- */
- break;
case STMLCDIF_24BIT:
/* real 24 bit */
break;
--
1.8.3.4
^ permalink raw reply related
* [PATCHv3 0/3] Few ignored framebuffer fixes/additions
From: Maxime Ripard @ 2013-08-01 8:40 UTC (permalink / raw)
To: linux-arm-kernel
Hi,
This is the v2 of a set of patches that got forgotten.
These have been around for a while without any functionnal comments recently,
so I guess we should be pretty close to merge them.
It would be great if we could have comments from the maintainer on these
patches though (three patches iterations and 41 days since last sign of life of
the maintainer).
Thanks,
Maxime
Changes from v2:
- Few minor coding style fixes
- Removed the useless msleep(100) in the hx8369 init function
Changes from v1:
- Fix line wrapping in patch 2
- Added some comments on the msleep in patch 3
Alexandre Belloni (1):
fb: backlight: HX8357: Add HX8369 support
Hector Palacios (1):
video: mxsfb: fix color settings for 18bit data bus and 32bpp
Maxime Ripard (1):
video: hx8357: Make IM pins optional
drivers/video/backlight/hx8357.c | 269 ++++++++++++++++++++++++++++++++++-----
drivers/video/mxsfb.c | 26 ----
2 files changed, 234 insertions(+), 61 deletions(-)
--
1.8.3.4
^ permalink raw reply
* Re: [PATCH v2 10/24] video: da8xx-fb: fb_set_par support
From: Tomi Valkeinen @ 2013-08-01 5:44 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1375208791-15781-11-git-send-email-detheridge@ti.com>
[-- Attachment #1: Type: text/plain, Size: 701 bytes --]
On 31/07/13 21:56, Etheridge, Darren wrote:
>>>
>>> +static int da8xxfb_set_par(struct fb_info *info) {
>>> + struct da8xx_fb_par *par = info->par;
>>> + int ret;
>>> + bool raster = da8xx_fb_is_raster_enabled();
>>> +
>>> + if (raster)
>>> + lcd_disable_raster(true);
>>> + else
>>> + lcd_disable_raster(false);
>>
>> This looks odd. If raster is enabled, you disable it. And if raster is disabled,
>> you disable it.
>
> I corrected this one in patch 0011 - I agree this code is very confusing.
In patch 11 you add the enum. I wasn't referring to that. My point was
that even if raster is already disabled,
lcd_disable_raster(dont-wait-for-framedone) is called.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]
^ permalink raw reply
* Re: [RESEND PATCH v10 1/8] drivers: phy: add generic PHY framework
From: Greg KH @ 2013-08-01 1:40 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1374842963-13545-2-git-send-email-kishon@ti.com>
On Fri, Jul 26, 2013 at 06:19:16PM +0530, Kishon Vijay Abraham I wrote:
> +static int phy_get_id(void)
> +{
> + int ret;
> + int id;
> +
> + ret = ida_pre_get(&phy_ida, GFP_KERNEL);
> + if (!ret)
> + return -ENOMEM;
> +
> + ret = ida_get_new(&phy_ida, &id);
> + if (ret < 0)
> + return ret;
> +
> + return id;
> +}
ida_simple_get() instead? And if you do that, you can get rid of this
function entirely.
thanks,
greg k-h
^ permalink raw reply
* RE: [PATCH v2 10/24] video: da8xx-fb: fb_set_par support
From: Etheridge, Darren @ 2013-07-31 18:56 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1375208791-15781-11-git-send-email-detheridge@ti.com>
> >
> > +static int da8xxfb_set_par(struct fb_info *info) {
> > + struct da8xx_fb_par *par = info->par;
> > + int ret;
> > + bool raster = da8xx_fb_is_raster_enabled();
> > +
> > + if (raster)
> > + lcd_disable_raster(true);
> > + else
> > + lcd_disable_raster(false);
>
> This looks odd. If raster is enabled, you disable it. And if raster is disabled,
> you disable it.
I corrected this one in patch 0011 - I agree this code is very confusing.
^ permalink raw reply
* Re: [PATCHv2 3/3] fb: backlight: HX8357: Add HX8369 support
From: Alexandre Belloni @ 2013-07-31 18:14 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <002301ce8991$f9b82060$ed286120$@samsung.com>
Hi,
On 26/07/2013 01:52, Jingoo Han wrote:
> On Thursday, July 25, 2013 10:05 PM, Maxime Ripard wrote:
>> From: Alexandre Belloni <alexandre.belloni@free-electrons.com>
>>
>> Add support for the Himax HX8369 controller as it is quite similar to the
>> hx8357.
>>
>> Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
>> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
>> Acked-by: Jingoo Han <jg1.han@samsung.com>
>> ---
>> drivers/video/backlight/hx8357.c | 221 ++++++++++++++++++++++++++++++++++++---
>> 1 file changed, 206 insertions(+), 15 deletions(-)
>>
> [....]
>
>> + ret = hx8357_spi_write_byte(lcdev, HX8357_SET_DISPLAY_ON);
>> + if (ret < 0)
>> + return ret;
>> +
>> + msleep(100);
> Hi Maxime Ripard,
>
> Could you add comment on this huge delay, too?
That one can actually be removed.
Regards,
--
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
^ permalink raw reply
* Re: [PATCH v2 10/24] video: da8xx-fb: fb_set_par support
From: Tomi Valkeinen @ 2013-07-31 11:38 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1375208791-15781-11-git-send-email-detheridge@ti.com>
[-- Attachment #1: Type: text/plain, Size: 2739 bytes --]
On 30/07/13 21:26, Darren Etheridge wrote:
> From: Afzal Mohammed <afzal@ti.com>
>
> fb_set_par helps in runtime configuration of lcd controller like
> changing resolution, pixel clock etc. (eg. using fbset utility)
>
> Reconfigure lcd controller based on information passed by framework.
> Enable raster back if it was already enabled.
>
> As fb_set_par would get invoked indirectly from probe via fb_set_var,
> remove existing lcdc initialization in probe and do lcdc reset in
> probe so that reset happens only at the begining.
>
> Signed-off-by: Afzal Mohammed <afzal@ti.com>
> Signed-off-by: Darren Etheridge <detheridge@ti.com>
> ---
> drivers/video/da8xx-fb.c | 60 +++++++++++++++++++++++++++++++++++++--------
> 1 files changed, 49 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/video/da8xx-fb.c b/drivers/video/da8xx-fb.c
> index 8d73730..b25b810 100644
> --- a/drivers/video/da8xx-fb.c
> +++ b/drivers/video/da8xx-fb.c
> @@ -243,6 +243,11 @@ static struct fb_videomode known_lcd_panels[] = {
> },
> };
>
> +static inline bool da8xx_fb_is_raster_enabled(void)
> +{
> + return !!(lcdc_read(LCD_RASTER_CTRL_REG) & LCD_RASTER_ENABLE);
> +}
See Documentation/CodingStyle about inline.
I think, generally, it's better not to use inline at all in normal
functions. Let the compiler decide. Even more so with funcs like
da8xx_fb_is_raster_enabled(), which I guess is only used rarely.
There are some inlines added in other patches in the series also.
> /* Enable the Raster Engine of the LCD Controller */
> static inline void lcd_enable_raster(void)
> {
> @@ -665,9 +670,6 @@ static int fb_setcolreg(unsigned regno, unsigned red, unsigned green,
>
> static void da8xx_fb_lcd_reset(void)
> {
> - /* Disable the Raster if previously Enabled */
> - lcd_disable_raster(false);
> -
> /* DMA has to be disabled */
> lcdc_write(0, LCD_DMA_CTRL_REG);
> lcdc_write(0, LCD_RASTER_CTRL_REG);
> @@ -720,8 +722,6 @@ static int lcd_init(struct da8xx_fb_par *par, const struct lcd_ctrl_config *cfg,
> u32 bpp;
> int ret = 0;
>
> - da8xx_fb_lcd_reset();
> -
> da8xx_fb_calc_config_clk_divider(par, panel);
>
> if (panel->sync & FB_SYNC_CLK_INVERT)
> @@ -1201,9 +1201,52 @@ static int da8xx_pan_display(struct fb_var_screeninfo *var,
> return ret;
> }
>
> +static int da8xxfb_set_par(struct fb_info *info)
> +{
> + struct da8xx_fb_par *par = info->par;
> + int ret;
> + bool raster = da8xx_fb_is_raster_enabled();
> +
> + if (raster)
> + lcd_disable_raster(true);
> + else
> + lcd_disable_raster(false);
This looks odd. If raster is enabled, you disable it. And if raster is
disabled, you disable it.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]
^ permalink raw reply
* Re: [PATCH v2 18/24] video: da8xx-fb: minimal dt support
From: Tomi Valkeinen @ 2013-07-31 10:19 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1375208791-15781-19-git-send-email-detheridge@ti.com>
[-- Attachment #1: Type: text/plain, Size: 1546 bytes --]
On 30/07/13 21:26, Darren Etheridge wrote:
> From: Afzal Mohammed <afzal@ti.com>
>
> Driver is provided a means to have the probe triggered by DT.
>
> Signed-off-by: Afzal Mohammed <afzal@ti.com>
> Signed-off-by: Darren Etheridge <detheridge@ti.com>
> ---
> .../devicetree/bindings/video/fb-da8xx.txt | 16 ++++++++++++++++
> drivers/video/da8xx-fb.c | 7 +++++++
> 2 files changed, 23 insertions(+), 0 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/video/fb-da8xx.txt
>
> diff --git a/Documentation/devicetree/bindings/video/fb-da8xx.txt b/Documentation/devicetree/bindings/video/fb-da8xx.txt
> new file mode 100644
> index 0000000..581e014
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/video/fb-da8xx.txt
> @@ -0,0 +1,16 @@
> +TI LCD Controller on DA830/DA850/AM335x SoC's
> +
> +Required properties:
> +- compatible:
> + DA830 - "ti,da830-lcdc"
> + AM335x SoC's - "ti,am3352-lcdc", "ti,da830-lcdc"
I'm not totally sure about this, but how I understand the compatible
property, the above reads as: "this device is ti,am3352-lcdc and it's
fully compatible with ti,da830-lcdc". I.e. if the kernel has a driver
for da830-lcdc, it should work with AM335x also (without any of the new
features in AM335x, obviously). Which I believe is not the case, as the
point of this series is to add the AM335x support.
Or should the current da830-lcdc work with AM335x also, but it just
didn't because there were bugs in da830-lcdc?
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]
^ permalink raw reply
* Re: [PATCH v2 00/24] video/da8xx-fb fbdev driver enhance to support TI am335x SoC
From: Tomi Valkeinen @ 2013-07-31 10:04 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1375208791-15781-1-git-send-email-detheridge@ti.com>
[-- Attachment #1: Type: text/plain, Size: 2347 bytes --]
Hi,
On 30/07/13 21:26, Darren Etheridge wrote:
> Changes in v2:
> Addressing review comments from Tomi Valkeinen:
> Dropped readl/writel patch
> Many cosmetic changes to make code easier to understand
>
>
> This is primarily a resend of a series of patches that were original
> submitted to linux-fbdev back in January of 2013 for 3.8 by Afzal
> Mohammed. I have rebased them on 3.10 and also made sure they
> apply cleanly to the 'for-next' branch of linux-fbdev git.
> The patches enable use of the current mainline da8xx-fb driver on the
> TI AM335x SOC along with some bug fixes and cleanup.
>
> The original patch series can be found here:
> https://patchwork.kernel.org/project/linux-fbdev/list/?submitter=39101
> if you want to see the history.
Comments on the whole series:
Most of the patches are originally from Afzal. I believe some of the
patches are unchanged, but some are changed by you. In cases like this
you should pick one of the following options for each patch:
- If the patch is unchanged, send the patch as it is, having From: Afzal
line there.
- If you have changed the patch, send the patch having From: Afzal line,
but marking in the description that you've changed it (and what you
did). This should be done if the changes are small.
- If you changed a lot in the patch, send the patch with yourself as the
author, signed off by only you, but mention that it's based on Afzal's work.
The point here is that if you change the patch, it's no longer Afzal's
original patch. Afzal hasn't reviewed it, so signed-off-by Afzal is not
correct. You could've introduced horrible bugs in the patch, and I'm
sure Afzal doesn't want to see that a patch in the kernel introducing
horrible bugs is from him (when it is not from him).
Of course, if you have actively discussed the patches with Afzal, and
he's okay with all the changes you've made, then the patches are fine.
Another thing are the DT related patches. They should be sent to
devicetree@vger.kernel.org for review. And I think the DT patches should
be squashed into one, as they are quite short and having them as a whole
makes it easier to look at them. You could probably move the DT patches
to a separate series, so that we can merge the rest of the improvements,
and manage DT separately.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]
^ permalink raw reply
* Re: [PATCH v2 21/24] video: da8xx-fb: setup struct lcd_ctrl_config for dt
From: Tomi Valkeinen @ 2013-07-31 9:33 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1375208791-15781-22-git-send-email-detheridge@ti.com>
[-- Attachment #1: Type: text/plain, Size: 1615 bytes --]
On 30/07/13 21:26, Darren Etheridge wrote:
> From: Afzal Mohammed <afzal@ti.com>
>
> strcut lcd_ctrl_config information required for driver is currently
> obtained via platform data. To handle DT probing, create
> lcd_ctrl_config and populate it with default values, these values are
> sufficient for the panels so far used with this controller to work.
>
> Signed-off-by: Afzal Mohammed <afzal@ti.com>
> Signed-off-by: Darren Etheridge <detheridge@ti.com>
> ---
> drivers/video/da8xx-fb.c | 34 +++++++++++++++++++++++++++++++++-
> 1 files changed, 33 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/video/da8xx-fb.c b/drivers/video/da8xx-fb.c
> index 697b07c..fe3d79e 100644
> --- a/drivers/video/da8xx-fb.c
> +++ b/drivers/video/da8xx-fb.c
<snip>
> static struct fb_videomode *da8xx_fb_get_videomode(struct platform_device *dev)
> {
> struct da8xx_lcdc_platform_data *fb_pdata = dev->dev.platform_data;
> @@ -1346,7 +1375,10 @@ static int fb_probe(struct platform_device *device)
> break;
> }
>
> - lcd_cfg = (struct lcd_ctrl_config *)fb_pdata->controller_data;
In the previous patch you allow fb_pdata==NULL if booting with DT.
However, this patch shows that fb_pdata is still accessed. So I think
you need to move the previous patch after this to avoid a crash between
this and the previous patch.
> + if (device->dev.of_node)
> + lcd_cfg = da8xx_fb_create_cfg(device);
> + else
> + lcd_cfg = (struct lcd_ctrl_config *)fb_pdata->controller_data;
fb_pdata->controller_data is void *, so you don't need to cast it
explicitly.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]
^ permalink raw reply
* Re: [patch] vga16fb: remove an unused variable
From: Jingoo Han @ 2013-07-31 9:02 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <20130731085414.GC8210@elgon.mountain>
On Wednesday, July 31, 2013 5:54 PM, Dan Carpenter wrote:
>
> Commit e21d2170f3 "video: remove unnecessary platform_set_drvdata()"
> introduces an unused variable compile warning.
>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
Hi Dan Carpenter,
The same patch was submitted by Luis Henriques 2 weeks ago. :)
Also, the patch was already applied by Tomi Valkeinen.
It will be merged to 3.11-rc4.
Thank you for caring it.
Best regards,
Jingoo Han
>
> diff --git a/drivers/video/vga16fb.c b/drivers/video/vga16fb.c
> index 830ded4..2827333 100644
> --- a/drivers/video/vga16fb.c
> +++ b/drivers/video/vga16fb.c
> @@ -1265,7 +1265,6 @@ static void vga16fb_imageblit(struct fb_info *info, const struct fb_image *image
>
> static void vga16fb_destroy(struct fb_info *info)
> {
> - struct platform_device *dev = container_of(info->device, struct platform_device, dev);
> iounmap(info->screen_base);
> fb_dealloc_cmap(&info->cmap);
> /* XXX unshare VGA regions */
^ permalink raw reply
* [patch] vga16fb: remove an unused variable
From: Dan Carpenter @ 2013-07-31 8:54 UTC (permalink / raw)
To: linux-fbdev
Commit e21d2170f3 "video: remove unnecessary platform_set_drvdata()"
introduces an unused variable compile warning.
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
diff --git a/drivers/video/vga16fb.c b/drivers/video/vga16fb.c
index 830ded4..2827333 100644
--- a/drivers/video/vga16fb.c
+++ b/drivers/video/vga16fb.c
@@ -1265,7 +1265,6 @@ static void vga16fb_imageblit(struct fb_info *info, const struct fb_image *image
static void vga16fb_destroy(struct fb_info *info)
{
- struct platform_device *dev = container_of(info->device, struct platform_device, dev);
iounmap(info->screen_base);
fb_dealloc_cmap(&info->cmap);
/* XXX unshare VGA regions */
^ permalink raw reply related
* Re: [PATCH 01/15] drivers: phy: add generic PHY framework
From: Felipe Balbi @ 2013-07-31 6:15 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <51F8A440.8010803@ti.com>
[-- Attachment #1: Type: text/plain, Size: 3325 bytes --]
Hi,
On Wed, Jul 31, 2013 at 11:14:32AM +0530, Kishon Vijay Abraham I wrote:
> >>>>> IMHO we need a lookup method for PHYs, just like for clocks,
> >>>>> regulators, PWMs or even i2c busses because there are complex cases
> >>>>> when passing just a name using platform data will not work. I would
> >>>>> second what Stephen said [1] and define a structure doing things in a
> >>>>> DT-like way.
> >>>>>
> >>>>> Example;
> >>>>>
> >>>>> [platform code]
> >>>>>
> >>>>> static const struct phy_lookup my_phy_lookup[] = {
> >>>>>
> >>>>> PHY_LOOKUP("s3c-hsotg.0", "otg", "samsung-usbphy.1", "phy.2"),
> >>>>
> >>>> The only problem here is that if *PLATFORM_DEVID_AUTO* is used while
> >>>> creating the device, the ids in the device name would change and
> >>>> PHY_LOOKUP wont be useful.
> >>>
> >>> I don't think this is a problem. All the existing lookup methods already
> >>> use ID to identify devices (see regulators, clkdev, PWMs, i2c, ...). You
> >>> can simply add a requirement that the ID must be assigned manually,
> >>> without using PLATFORM_DEVID_AUTO to use PHY lookup.
> >>
> >> And I'm saying that this idea, of using a specific name and id, is
> >> frought with fragility and will break in the future in various ways when
> >> devices get added to systems, making these strings constantly have to be
> >> kept up to date with different board configurations.
> >>
> >> People, NEVER, hardcode something like an id. The fact that this
> >> happens today with the clock code, doesn't make it right, it makes the
> >> clock code wrong. Others have already said that this is wrong there as
> >> well, as systems change and dynamic ids get used more and more.
> >>
> >> Let's not repeat the same mistakes of the past just because we refuse to
> >> learn from them...
> >>
> >> So again, the "find a phy by a string" functions should be removed, the
> >> device id should be automatically created by the phy core just to make
> >> things unique in sysfs, and no driver code should _ever_ be reliant on
> >> the number that is being created, and the pointer to the phy structure
> >> should be used everywhere instead.
> >>
> >> With those types of changes, I will consider merging this subsystem, but
> >> without them, sorry, I will not.
> >
> > I'll agree with Greg here, the very fact that we see people trying to
> > add a requirement of *NOT* using PLATFORM_DEVID_AUTO already points to a
> > big problem in the framework.
> >
> > The fact is that if we don't allow PLATFORM_DEVID_AUTO we will end up
> > adding similar infrastructure to the driver themselves to make sure we
> > don't end up with duplicate names in sysfs in case we have multiple
> > instances of the same IP in the SoC (or several of the same PCIe card).
> > I really don't want to go back to that.
>
> If we are using PLATFORM_DEVID_AUTO, then I dont see any way we can give the
> correct binding information to the PHY framework. I think we can drop having
> this non-dt support in PHY framework? I see only one platform (OMAP3) going to
> be needing this non-dt support and we can use the USB PHY library for it.
you shouldn't drop support for non-DT platform, in any case we lived
without DT (and still do) for years. Gotta find a better way ;-)
--
balbi
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH 01/15] drivers: phy: add generic PHY framework
From: Kishon Vijay Abraham I @ 2013-07-31 5:56 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20130730071106.GC16441@radagast>
Hi,
On Tuesday 30 July 2013 12:41 PM, Felipe Balbi wrote:
> On Sun, Jul 21, 2013 at 08:46:53AM -0700, Greg KH wrote:
>> On Sun, Jul 21, 2013 at 01:12:07PM +0200, Tomasz Figa wrote:
>>> On Sunday 21 of July 2013 16:37:33 Kishon Vijay Abraham I wrote:
>>>> Hi,
>>>>
>>>> On Sunday 21 July 2013 04:01 PM, Tomasz Figa wrote:
>>>>> Hi,
>>>>>
>>>>> On Saturday 20 of July 2013 19:59:10 Greg KH wrote:
>>>>>> On Sat, Jul 20, 2013 at 10:32:26PM -0400, Alan Stern wrote:
>>>>>>> On Sat, 20 Jul 2013, Greg KH wrote:
>>>>>>>>>>> That should be passed using platform data.
>>>>>>>>>>
>>>>>>>>>> Ick, don't pass strings around, pass pointers. If you have
>>>>>>>>>> platform
>>>>>>>>>> data you can get to, then put the pointer there, don't use a
>>>>>>>>>> "name".
>>>>>>>>>
>>>>>>>>> I don't think I understood you here :-s We wont have phy pointer
>>>>>>>>> when we create the device for the controller no?(it'll be done in
>>>>>>>>> board file). Probably I'm missing something.
>>>>>>>>
>>>>>>>> Why will you not have that pointer? You can't rely on the "name"
>>>>>>>> as
>>>>>>>> the device id will not match up, so you should be able to rely on
>>>>>>>> the pointer being in the structure that the board sets up, right?
>>>>>>>>
>>>>>>>> Don't use names, especially as ids can, and will, change, that is
>>>>>>>> going
>>>>>>>> to cause big problems. Use pointers, this is C, we are supposed to
>>>>>>>> be
>>>>>>>> doing that :)
>>>>>>>
>>>>>>> Kishon, I think what Greg means is this: The name you are using
>>>>>>> must
>>>>>>> be stored somewhere in a data structure constructed by the board
>>>>>>> file,
>>>>>>> right? Or at least, associated with some data structure somehow.
>>>>>>> Otherwise the platform code wouldn't know which PHY hardware
>>>>>>> corresponded to a particular name.
>>>>>>>
>>>>>>> Greg's suggestion is that you store the address of that data
>>>>>>> structure
>>>>>>> in the platform data instead of storing the name string. Have the
>>>>>>> consumer pass the data structure's address when it calls phy_create,
>>>>>>> instead of passing the name. Then you don't have to worry about two
>>>>>>> PHYs accidentally ending up with the same name or any other similar
>>>>>>> problems.
>>>>>>
>>>>>> Close, but the issue is that whatever returns from phy_create()
>>>>>> should
>>>>>> then be used, no need to call any "find" functions, as you can just
>>>>>> use
>>>>>> the pointer that phy_create() returns. Much like all other class api
>>>>>> functions in the kernel work.
>>>>>
>>>>> I think there is a confusion here about who registers the PHYs.
>>>>>
>>>>> All platform code does is registering a platform/i2c/whatever device,
>>>>> which causes a driver (located in drivers/phy/) to be instantiated.
>>>>> Such drivers call phy_create(), usually in their probe() callbacks,
>>>>> so platform_code has no way (and should have no way, for the sake of
>>>>> layering) to get what phy_create() returns.
>>
>> Why not put pointers in the platform data structure that can hold these
>> pointers? I thought that is why we created those structures in the
>> first place. If not, what are they there for?
>
> heh, IMO we shouldn't pass pointers of any kind through platform_data,
> we want to pass data :-)
>
> Allowing to pass pointers through that, is one of the reasons which got
> us in such a big mess in ARM land, well it was much easier for a
> board-file/driver writer to pass a function pointer then to create a
> generic framework :-)
>
>>>>> IMHO we need a lookup method for PHYs, just like for clocks,
>>>>> regulators, PWMs or even i2c busses because there are complex cases
>>>>> when passing just a name using platform data will not work. I would
>>>>> second what Stephen said [1] and define a structure doing things in a
>>>>> DT-like way.
>>>>>
>>>>> Example;
>>>>>
>>>>> [platform code]
>>>>>
>>>>> static const struct phy_lookup my_phy_lookup[] = {
>>>>>
>>>>> PHY_LOOKUP("s3c-hsotg.0", "otg", "samsung-usbphy.1", "phy.2"),
>>>>
>>>> The only problem here is that if *PLATFORM_DEVID_AUTO* is used while
>>>> creating the device, the ids in the device name would change and
>>>> PHY_LOOKUP wont be useful.
>>>
>>> I don't think this is a problem. All the existing lookup methods already
>>> use ID to identify devices (see regulators, clkdev, PWMs, i2c, ...). You
>>> can simply add a requirement that the ID must be assigned manually,
>>> without using PLATFORM_DEVID_AUTO to use PHY lookup.
>>
>> And I'm saying that this idea, of using a specific name and id, is
>> frought with fragility and will break in the future in various ways when
>> devices get added to systems, making these strings constantly have to be
>> kept up to date with different board configurations.
>>
>> People, NEVER, hardcode something like an id. The fact that this
>> happens today with the clock code, doesn't make it right, it makes the
>> clock code wrong. Others have already said that this is wrong there as
>> well, as systems change and dynamic ids get used more and more.
>>
>> Let's not repeat the same mistakes of the past just because we refuse to
>> learn from them...
>>
>> So again, the "find a phy by a string" functions should be removed, the
>> device id should be automatically created by the phy core just to make
>> things unique in sysfs, and no driver code should _ever_ be reliant on
>> the number that is being created, and the pointer to the phy structure
>> should be used everywhere instead.
>>
>> With those types of changes, I will consider merging this subsystem, but
>> without them, sorry, I will not.
>
> I'll agree with Greg here, the very fact that we see people trying to
> add a requirement of *NOT* using PLATFORM_DEVID_AUTO already points to a
> big problem in the framework.
>
> The fact is that if we don't allow PLATFORM_DEVID_AUTO we will end up
> adding similar infrastructure to the driver themselves to make sure we
> don't end up with duplicate names in sysfs in case we have multiple
> instances of the same IP in the SoC (or several of the same PCIe card).
> I really don't want to go back to that.
If we are using PLATFORM_DEVID_AUTO, then I dont see any way we can give the
correct binding information to the PHY framework. I think we can drop having
this non-dt support in PHY framework? I see only one platform (OMAP3) going to
be needing this non-dt support and we can use the USB PHY library for it.
Thanks
Kishon
^ permalink raw reply
* Re: [PATCH] fbdev: fix build warning in vga16fb.c
From: Gu Zheng @ 2013-07-31 3:28 UTC (permalink / raw)
To: Xishi Qiu; +Cc: plagnioj, tomi.valkeinen, linux-fbdev, LKML
In-Reply-To: <51F8829E.4080605@huawei.com>
Hoho, Tomi has applied the patch from Lius to fix this warning.
And this is the sixth patch to fix the same issue since last week.
Thanks,
Gu
On 07/31/2013 11:21 AM, Xishi Qiu wrote:
> When building v3.11-rc3, I get the following warning:
> ...
> drivers/video/vga16fb.c: In function ‘vga16fb_destroy’:
> drivers/video/vga16fb.c:1268: warning: unused variable ‘dev’
> ...
>
> Signed-off-by: Xishi Qiu <qiuxishi@huawei.com>
> ---
> drivers/video/vga16fb.c | 1 -
> 1 files changed, 0 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/video/vga16fb.c b/drivers/video/vga16fb.c
> index 830ded4..2827333 100644
> --- a/drivers/video/vga16fb.c
> +++ b/drivers/video/vga16fb.c
> @@ -1265,7 +1265,6 @@ static void vga16fb_imageblit(struct fb_info *info, const struct fb_image *image
>
> static void vga16fb_destroy(struct fb_info *info)
> {
> - struct platform_device *dev = container_of(info->device, struct platform_device, dev);
> iounmap(info->screen_base);
> fb_dealloc_cmap(&info->cmap);
> /* XXX unshare VGA regions */
^ permalink raw reply
* [PATCH] fbdev: fix build warning in vga16fb.c
From: Xishi Qiu @ 2013-07-31 3:21 UTC (permalink / raw)
To: plagnioj, tomi.valkeinen, linux-fbdev, LKML
When building v3.11-rc3, I get the following warning:
...
drivers/video/vga16fb.c: In function ‘vga16fb_destroy’:
drivers/video/vga16fb.c:1268: warning: unused variable ‘dev’
...
Signed-off-by: Xishi Qiu <qiuxishi@huawei.com>
---
drivers/video/vga16fb.c | 1 -
1 files changed, 0 insertions(+), 1 deletions(-)
diff --git a/drivers/video/vga16fb.c b/drivers/video/vga16fb.c
index 830ded4..2827333 100644
--- a/drivers/video/vga16fb.c
+++ b/drivers/video/vga16fb.c
@@ -1265,7 +1265,6 @@ static void vga16fb_imageblit(struct fb_info *info, const struct fb_image *image
static void vga16fb_destroy(struct fb_info *info)
{
- struct platform_device *dev = container_of(info->device, struct platform_device, dev);
iounmap(info->screen_base);
fb_dealloc_cmap(&info->cmap);
/* XXX unshare VGA regions */
--
1.7.1
^ permalink raw reply related
* [PATCH v2 24/24] video/da8xx-fb adding am33xx as dependency
From: Darren Etheridge @ 2013-07-30 18:26 UTC (permalink / raw)
To: linux-fbdev
Updating Kconfig to allow am33xx to include lcdc fbdev driver
including some extra dependencies needed by device tree mods.
Signed-off-by: Darren Etheridge <detheridge@ti.com>
---
drivers/video/Kconfig | 7 ++++---
1 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
index 2e937bd..8ae6d2a 100644
--- a/drivers/video/Kconfig
+++ b/drivers/video/Kconfig
@@ -2226,15 +2226,16 @@ config FB_SH7760
panels <= 320 pixel horizontal resolution.
config FB_DA8XX
- tristate "DA8xx/OMAP-L1xx Framebuffer support"
- depends on FB && ARCH_DAVINCI_DA8XX
+ tristate "DA8xx/OMAP-L1xx/AM335x Framebuffer support"
+ depends on FB && (ARCH_DAVINCI_DA8XX || SOC_AM33XX)
select FB_CFB_FILLRECT
select FB_CFB_COPYAREA
select FB_CFB_IMAGEBLIT
select FB_CFB_REV_PIXELS_IN_BYTE
+ select VIDEOMODE_HELPERS
---help---
This is the frame buffer device driver for the TI LCD controller
- found on DA8xx/OMAP-L1xx SoCs.
+ found on DA8xx/OMAP-L1xx/AM335x SoCs.
If unsure, say N.
config FB_VIRTUAL
--
1.7.0.4
^ permalink raw reply related
* [PATCH v2 23/24] video: da8xx-fb: make clock naming consistent
From: Darren Etheridge @ 2013-07-30 18:26 UTC (permalink / raw)
To: linux-fbdev
Clean up the code, so that the names of the various clock variables
are consistent to it is clear what variable is associated with what
clock.
Signed-off-by: Darren Etheridge <detheridge@ti.com>
---
drivers/video/da8xx-fb.c | 22 +++++++++++-----------
1 files changed, 11 insertions(+), 11 deletions(-)
diff --git a/drivers/video/da8xx-fb.c b/drivers/video/da8xx-fb.c
index 471931e..131cf4c 100644
--- a/drivers/video/da8xx-fb.c
+++ b/drivers/video/da8xx-fb.c
@@ -179,7 +179,7 @@ struct da8xx_fb_par {
#ifdef CONFIG_CPU_FREQ
struct notifier_block freq_transition;
#endif
- unsigned int lcd_fck_rate;
+ unsigned int lcdc_clk_rate;
void (*panel_power_ctrl)(int);
u32 pseudo_palette[16];
struct fb_videomode mode;
@@ -692,14 +692,14 @@ static int da8xx_fb_config_clk_divider(struct da8xx_fb_par *par,
{
int ret;
- if (par->lcd_fck_rate != lcdc_clk_rate) {
+ if (par->lcdc_clk_rate != lcdc_clk_rate) {
ret = clk_set_rate(par->lcdc_clk, lcdc_clk_rate);
if (IS_ERR_VALUE(ret)) {
dev_err(par->dev,
"unable to set clock rate at %u\n", lcdc_clk_rate);
return ret;
}
- par->lcd_fck_rate = clk_get_rate(par->lcdc_clk);
+ par->lcdc_clk_rate = clk_get_rate(par->lcdc_clk);
}
/* Configure the LCD clock divisor. */
@@ -721,7 +721,7 @@ static unsigned int da8xx_fb_calc_clk_divider(struct da8xx_fb_par *par,
pixclock = PICOS2KHZ(pixclock) * 1000;
- *lcdc_clk_rate = par->lcd_fck_rate;
+ *lcdc_clk_rate = par->lcdc_clk_rate;
if (pixclock < (*lcdc_clk_rate / CLK_MAX_DIV)) {
*lcdc_clk_rate = clk_round_rate(par->lcdc_clk, pixclock * CLK_MAX_DIV);
@@ -1026,8 +1026,8 @@ static int lcd_da8xx_cpufreq_transition(struct notifier_block *nb,
par = container_of(nb, struct da8xx_fb_par, freq_transition);
if (val = CPUFREQ_POSTCHANGE) {
- if (par->lcd_fck_rate != clk_get_rate(par->lcdc_clk)) {
- par->lcd_fck_rate = clk_get_rate(par->lcdc_clk);
+ if (par->lcdc_clk_rate != clk_get_rate(par->lcdc_clk)) {
+ par->lcdc_clk_rate = clk_get_rate(par->lcdc_clk);
lcd_disable_raster(DA8XX_FRAME_WAIT);
da8xx_fb_calc_config_clk_divider(par, &par->mode);
if (par->blank = FB_BLANK_UNBLANK)
@@ -1368,8 +1368,8 @@ static int fb_probe(struct platform_device *device)
struct lcd_ctrl_config *lcd_cfg;
struct fb_videomode *lcdc_info;
struct fb_info *da8xx_fb_info;
- struct clk *fb_clk = NULL;
struct da8xx_fb_par *par;
+ struct clk *tmp_lcdc_clk;
int ret;
unsigned long ulcm;
@@ -1389,8 +1389,8 @@ static int fb_probe(struct platform_device *device)
return -EADDRNOTAVAIL;
}
- fb_clk = devm_clk_get(&device->dev, "fck");
- if (IS_ERR(fb_clk)) {
+ tmp_lcdc_clk = devm_clk_get(&device->dev, "fck");
+ if (IS_ERR(tmp_lcdc_clk)) {
dev_err(&device->dev, "Can not get device clock\n");
return -ENODEV;
}
@@ -1435,8 +1435,8 @@ static int fb_probe(struct platform_device *device)
par = da8xx_fb_info->par;
par->dev = &device->dev;
- par->lcdc_clk = fb_clk;
- par->lcd_fck_rate = clk_get_rate(fb_clk);
+ par->lcdc_clk = tmp_lcdc_clk;
+ par->lcdc_clk_rate = clk_get_rate(par->lcdc_clk);
if (fb_pdata && fb_pdata->panel_power_ctrl) {
par->panel_power_ctrl = fb_pdata->panel_power_ctrl;
par->panel_power_ctrl(1);
--
1.7.0.4
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox