Re: [PATCH v1 5/5] tinydrm: add winstar wg160160 driver
Hi Noralf.
On Tue, Aug 07, 2018 at 07:35:30PM +0200, Noralf Trønnes wrote:
>
> Den 02.08.2018 21.45, skrev Sam Ravnborg:
> >Add driver for the winstar wg160160 display.
> >The driver utilises pardata-dbi that
> >again utilise the pardata subsystem.
> >
> >Signed-off-by: Sam Ravnborg
> >---
> > MAINTAINERS| 5 +
> > drivers/gpu/drm/tinydrm/Kconfig| 10 ++
> > drivers/gpu/drm/tinydrm/Makefile | 1 +
> > drivers/gpu/drm/tinydrm/wg160160.c | 298
> > +
> > 4 files changed, 314 insertions(+)
> > create mode 100644 drivers/gpu/drm/tinydrm/wg160160.c
> >
>
> [...]
>
> >+
> >+/**
> >+ * write_reg - Write instruction on parallel bus to controller
> >+ *
> >+ * Check BUSY flag and write instruction
> >+ *
> >+ * @pdd: pardata data
> >+ * @reg: The register to write
> >+ * @value: The value of the register
> >+ *
> >+ * Returns:
> >+ * Zero on success, negative error code on failure
> >+ */
> >+int write_reg(struct pardata_data *pdd, unsigned int reg, unsigned int
> >value)
> >+{
> >+int ins[PIN_NUM];
> >+int val[PIN_NUM];
> >+int i;
> >+
> >+for (i = 0; i < PIN_NUM; i++)
> >+ins[PIN_DB0 + i] = !!BIT(reg);
> >+
> >+for (i = 0; i < PIN_NUM; i++)
> >+val[PIN_DB0 + i] = !!(value & BIT(i));
> >+
> >+gpiod_set_value_cansleep(pdd->bus->pin_rs, 1);
> >+gpiod_set_array_value_cansleep(PIN_NUM, pdd->bus->data_pins->desc, ins);
> >+wait_busy(pdd);
> >+pardata_strobe_write(pdd);
> >+
> >+gpiod_set_value_cansleep(pdd->bus->pin_rs, 0);
> >+gpiod_set_array_value_cansleep(PIN_NUM, pdd->bus->data_pins->desc, val);
> >+wait_busy(pdd);
> >+pardata_strobe_write(pdd);
> >+
> >+return 0;
> >+}
>
> If this controller has normal registers, you could do a regmap
> implementation for pardata: drivers/base/regmap.
If I understand you correct then you suggest to add a regmap implementation
on top of the registers replacing the current gpio support?
So we in regmap can optimize access to the registers better.
Or did you have something else in mind?
As speed is not the main challenge here I will likely wait with this
and continue using gpio.
Sam
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v1 5/5] tinydrm: add winstar wg160160 driver
Den 08.08.2018 10.32, skrev Sam Ravnborg:
Hi Noralf.
On Tue, Aug 07, 2018 at 07:35:30PM +0200, Noralf Trønnes wrote:
Den 02.08.2018 21.45, skrev Sam Ravnborg:
Add driver for the winstar wg160160 display.
The driver utilises pardata-dbi that
again utilise the pardata subsystem.
Signed-off-by: Sam Ravnborg
---
MAINTAINERS| 5 +
drivers/gpu/drm/tinydrm/Kconfig| 10 ++
drivers/gpu/drm/tinydrm/Makefile | 1 +
drivers/gpu/drm/tinydrm/wg160160.c | 298 +
4 files changed, 314 insertions(+)
create mode 100644 drivers/gpu/drm/tinydrm/wg160160.c
[...]
+
+/**
+ * write_reg - Write instruction on parallel bus to controller
+ *
+ * Check BUSY flag and write instruction
+ *
+ * @pdd: pardata data
+ * @reg: The register to write
+ * @value: The value of the register
+ *
+ * Returns:
+ * Zero on success, negative error code on failure
+ */
+int write_reg(struct pardata_data *pdd, unsigned int reg, unsigned int value)
+{
+ int ins[PIN_NUM];
+ int val[PIN_NUM];
+ int i;
+
+ for (i = 0; i < PIN_NUM; i++)
+ ins[PIN_DB0 + i] = !!BIT(reg);
+
+ for (i = 0; i < PIN_NUM; i++)
+ val[PIN_DB0 + i] = !!(value & BIT(i));
+
+ gpiod_set_value_cansleep(pdd->bus->pin_rs, 1);
+ gpiod_set_array_value_cansleep(PIN_NUM, pdd->bus->data_pins->desc, ins);
+ wait_busy(pdd);
+ pardata_strobe_write(pdd);
+
+ gpiod_set_value_cansleep(pdd->bus->pin_rs, 0);
+ gpiod_set_array_value_cansleep(PIN_NUM, pdd->bus->data_pins->desc, val);
+ wait_busy(pdd);
+ pardata_strobe_write(pdd);
+
+ return 0;
+}
If this controller has normal registers, you could do a regmap
implementation for pardata: drivers/base/regmap.
If I understand you correct then you suggest to add a regmap implementation
on top of the registers replacing the current gpio support?
So we in regmap can optimize access to the registers better.
Or did you have something else in mind?
I was thinking of a regmap-pardata.c like drivers/base/regmap/regmap-i2c.c.
It's not much code an we would avoid having generic register access
functions in DRM. It's not about speed or caching, but just to try and
put code where it logically belongs.
Here is one experiment where I have created a regmap on a 8080 bus:
https://github.com/notro/tinydrm/blob/master/tinydrm-regmap.c
Used here: https://github.com/notro/tinydrm/blob/master/fb_ili9325.c
I don't advocate this particular approach, I mention it just as an
example of using regmap in a tinydrm driver.
Noralf.
As speed is not the main challenge here I will likely wait with this
and continue using gpio.
Sam
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v1 5/5] tinydrm: add winstar wg160160 driver
Den 02.08.2018 21.45, skrev Sam Ravnborg:
Add driver for the winstar wg160160 display.
The driver utilises pardata-dbi that
again utilise the pardata subsystem.
Signed-off-by: Sam Ravnborg
---
MAINTAINERS| 5 +
drivers/gpu/drm/tinydrm/Kconfig| 10 ++
drivers/gpu/drm/tinydrm/Makefile | 1 +
drivers/gpu/drm/tinydrm/wg160160.c | 298 +
4 files changed, 314 insertions(+)
create mode 100644 drivers/gpu/drm/tinydrm/wg160160.c
[...]
+
+/**
+ * write_reg - Write instruction on parallel bus to controller
+ *
+ * Check BUSY flag and write instruction
+ *
+ * @pdd: pardata data
+ * @reg: The register to write
+ * @value: The value of the register
+ *
+ * Returns:
+ * Zero on success, negative error code on failure
+ */
+int write_reg(struct pardata_data *pdd, unsigned int reg, unsigned int value)
+{
+ int ins[PIN_NUM];
+ int val[PIN_NUM];
+ int i;
+
+ for (i = 0; i < PIN_NUM; i++)
+ ins[PIN_DB0 + i] = !!BIT(reg);
+
+ for (i = 0; i < PIN_NUM; i++)
+ val[PIN_DB0 + i] = !!(value & BIT(i));
+
+ gpiod_set_value_cansleep(pdd->bus->pin_rs, 1);
+ gpiod_set_array_value_cansleep(PIN_NUM, pdd->bus->data_pins->desc, ins);
+ wait_busy(pdd);
+ pardata_strobe_write(pdd);
+
+ gpiod_set_value_cansleep(pdd->bus->pin_rs, 0);
+ gpiod_set_array_value_cansleep(PIN_NUM, pdd->bus->data_pins->desc, val);
+ wait_busy(pdd);
+ pardata_strobe_write(pdd);
+
+ return 0;
+}
If this controller has normal registers, you could do a regmap
implementation for pardata: drivers/base/regmap.
Noralf.
+
+/**
+ * write_buf - write buffer on parallel bus to controller
+ *
+ * @pdd: pardata data
+ * @offset: offset into display RAM
+ * @data: pointer to data to write
+ * @len: number of bytes to write
+ *
+ * Returns:
+ * Zero on success, negative error code on failure
+ */
+int write_buf(struct pardata_data *pdd, u8 offset, u8 *data, size_t len)
+{
+ int ins[PIN_NUM];
+ int val[PIN_NUM];
+ int bit;
+ int i;
+
+ /* Setup address */
+ write_reg(pdd, WG160160_ADDRSL_REG, offset & 0xff);
+ write_reg(pdd, WG160160_ADDRSL_REG, (offset >> 8) & 0xff);
+
+ /* prepare to write data */
+ for (i = 0; i < PIN_NUM; i++)
+ ins[PIN_DB0 + i] = !!(WG160160_WRITE_REG & BIT(i));
+
+ gpiod_set_value_cansleep(pdd->bus->pin_rs, 1);
+ gpiod_set_array_value_cansleep(PIN_NUM, pdd->bus->data_pins->desc, ins);
+ wait_busy(pdd);
+ pardata_strobe_write(pdd);
+
+ /* Write data byte - by byte */
+ gpiod_set_value_cansleep(pdd->bus->pin_rs, 0);
+
+ for (i = offset; i < (offset + len); i++) {
+ for (bit = 0; bit < PIN_NUM; bit++)
+ val[PIN_DB0 + bit] = !!(data[i] & BIT(bit));
+
+ gpiod_set_array_value_cansleep(PIN_NUM,
+ pdd->bus->data_pins->desc,
+ val);
+ wait_busy(pdd);
+ pardata_strobe_write(pdd);
+ }
+
+ return 0;
+}
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v1 5/5] tinydrm: add winstar wg160160 driver
On Mon, Aug 06, 2018 at 12:15:48PM +0300, Dan Carpenter wrote:
> Hi Sam,
>
> I love your patch! Perhaps something to improve:
>
> url:
> https://github.com/0day-ci/linux/commits/Sam-Ravnborg/dt-bindings-add-parallel-data-bus-pardata/20180803-090135
>
> smatch warnings:
> drivers/gpu/drm/tinydrm/wg160160.c:145 write_buf() warn: right shifting more
> than type allows 8 vs 8
> include/drm/tinydrm/pardata-dbi.h:165 pardata_write_buf() error: we
> previously assumed 'pdd' could be null (see line 162)
>
> #
> https://github.com/0day-ci/linux/commit/9fcebca9e208029e06eea5e4858c1055132f06b6
> git remote add linux-review https://github.com/0day-ci/linux
> git remote update linux-review
> git checkout 9fcebca9e208029e06eea5e4858c1055132f06b6
> vim +145 drivers/gpu/drm/tinydrm/wg160160.c
>
> 9fcebca9 Sam Ravnborg 2018-08-02 124
> 9fcebca9 Sam Ravnborg 2018-08-02 125 /**
> 9fcebca9 Sam Ravnborg 2018-08-02 126 * write_buf - write buffer on
> parallel bus to controller
> 9fcebca9 Sam Ravnborg 2018-08-02 127 *
> 9fcebca9 Sam Ravnborg 2018-08-02 128 * @pdd: pardata data
> 9fcebca9 Sam Ravnborg 2018-08-02 129 * @offset: offset into display RAM
> 9fcebca9 Sam Ravnborg 2018-08-02 130 * @data: pointer to data to write
> 9fcebca9 Sam Ravnborg 2018-08-02 131 * @len: number of bytes to write
> 9fcebca9 Sam Ravnborg 2018-08-02 132 *
> 9fcebca9 Sam Ravnborg 2018-08-02 133 * Returns:
> 9fcebca9 Sam Ravnborg 2018-08-02 134 * Zero on success, negative error
> code on failure
> 9fcebca9 Sam Ravnborg 2018-08-02 135 */
> 9fcebca9 Sam Ravnborg 2018-08-02 136 int write_buf(struct pardata_data
> *pdd, u8 offset, u8 *data, size_t len)
> 9fcebca9 Sam Ravnborg 2018-08-02 137 {
> 9fcebca9 Sam Ravnborg 2018-08-02 138 int ins[PIN_NUM];
> 9fcebca9 Sam Ravnborg 2018-08-02 139 int val[PIN_NUM];
> 9fcebca9 Sam Ravnborg 2018-08-02 140 int bit;
> 9fcebca9 Sam Ravnborg 2018-08-02 141 int i;
> 9fcebca9 Sam Ravnborg 2018-08-02 142
> 9fcebca9 Sam Ravnborg 2018-08-02 143 /* Setup address */
> 9fcebca9 Sam Ravnborg 2018-08-02 144 write_reg(pdd,
> WG160160_ADDRSL_REG, offset & 0xff);
> 9fcebca9 Sam Ravnborg 2018-08-02 @145 write_reg(pdd,
> WG160160_ADDRSL_REG, (offset >> 8) & 0xff);
>
>
> Probably this is fine. I don't know. If so then feel free to ignore
> the warning.
Thanks!
I will fix both issues in v2.
Sam
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v1 5/5] tinydrm: add winstar wg160160 driver
Hi Sam,
I love your patch! Perhaps something to improve:
url:
https://github.com/0day-ci/linux/commits/Sam-Ravnborg/dt-bindings-add-parallel-data-bus-pardata/20180803-090135
smatch warnings:
drivers/gpu/drm/tinydrm/wg160160.c:145 write_buf() warn: right shifting more
than type allows 8 vs 8
include/drm/tinydrm/pardata-dbi.h:165 pardata_write_buf() error: we previously
assumed 'pdd' could be null (see line 162)
#
https://github.com/0day-ci/linux/commit/9fcebca9e208029e06eea5e4858c1055132f06b6
git remote add linux-review https://github.com/0day-ci/linux
git remote update linux-review
git checkout 9fcebca9e208029e06eea5e4858c1055132f06b6
vim +145 drivers/gpu/drm/tinydrm/wg160160.c
9fcebca9 Sam Ravnborg 2018-08-02 124
9fcebca9 Sam Ravnborg 2018-08-02 125 /**
9fcebca9 Sam Ravnborg 2018-08-02 126 * write_buf - write buffer on parallel
bus to controller
9fcebca9 Sam Ravnborg 2018-08-02 127 *
9fcebca9 Sam Ravnborg 2018-08-02 128 * @pdd: pardata data
9fcebca9 Sam Ravnborg 2018-08-02 129 * @offset: offset into display RAM
9fcebca9 Sam Ravnborg 2018-08-02 130 * @data: pointer to data to write
9fcebca9 Sam Ravnborg 2018-08-02 131 * @len: number of bytes to write
9fcebca9 Sam Ravnborg 2018-08-02 132 *
9fcebca9 Sam Ravnborg 2018-08-02 133 * Returns:
9fcebca9 Sam Ravnborg 2018-08-02 134 * Zero on success, negative error code
on failure
9fcebca9 Sam Ravnborg 2018-08-02 135 */
9fcebca9 Sam Ravnborg 2018-08-02 136 int write_buf(struct pardata_data *pdd,
u8 offset, u8 *data, size_t len)
9fcebca9 Sam Ravnborg 2018-08-02 137 {
9fcebca9 Sam Ravnborg 2018-08-02 138 int ins[PIN_NUM];
9fcebca9 Sam Ravnborg 2018-08-02 139 int val[PIN_NUM];
9fcebca9 Sam Ravnborg 2018-08-02 140 int bit;
9fcebca9 Sam Ravnborg 2018-08-02 141 int i;
9fcebca9 Sam Ravnborg 2018-08-02 142
9fcebca9 Sam Ravnborg 2018-08-02 143 /* Setup address */
9fcebca9 Sam Ravnborg 2018-08-02 144 write_reg(pdd, WG160160_ADDRSL_REG,
offset & 0xff);
9fcebca9 Sam Ravnborg 2018-08-02 @145 write_reg(pdd, WG160160_ADDRSL_REG,
(offset >> 8) & 0xff);
Probably this is fine. I don't know. If so then feel free to ignore
the warning.
9fcebca9 Sam Ravnborg 2018-08-02 146
9fcebca9 Sam Ravnborg 2018-08-02 147 /* prepare to write data */
9fcebca9 Sam Ravnborg 2018-08-02 148 for (i = 0; i < PIN_NUM; i++)
9fcebca9 Sam Ravnborg 2018-08-02 149 ins[PIN_DB0 + i] =
!!(WG160160_WRITE_REG & BIT(i));
9fcebca9 Sam Ravnborg 2018-08-02 150
9fcebca9 Sam Ravnborg 2018-08-02 151
gpiod_set_value_cansleep(pdd->bus->pin_rs, 1);
9fcebca9 Sam Ravnborg 2018-08-02 152 gpiod_set_array_value_cansleep(PIN_NUM,
pdd->bus->data_pins->desc, ins);
9fcebca9 Sam Ravnborg 2018-08-02 153 wait_busy(pdd);
9fcebca9 Sam Ravnborg 2018-08-02 154 pardata_strobe_write(pdd);
9fcebca9 Sam Ravnborg 2018-08-02 155
9fcebca9 Sam Ravnborg 2018-08-02 156 /* Write data byte - by byte */
9fcebca9 Sam Ravnborg 2018-08-02 157
gpiod_set_value_cansleep(pdd->bus->pin_rs, 0);
9fcebca9 Sam Ravnborg 2018-08-02 158
9fcebca9 Sam Ravnborg 2018-08-02 159 for (i = offset; i < (offset + len);
i++) {
9fcebca9 Sam Ravnborg 2018-08-02 160 for (bit = 0; bit < PIN_NUM;
bit++)
9fcebca9 Sam Ravnborg 2018-08-02 161 val[PIN_DB0 + bit] =
!!(data[i] & BIT(bit));
9fcebca9 Sam Ravnborg 2018-08-02 162
9fcebca9 Sam Ravnborg 2018-08-02 163
gpiod_set_array_value_cansleep(PIN_NUM,
9fcebca9 Sam Ravnborg 2018-08-02 164
pdd->bus->data_pins->desc,
9fcebca9 Sam Ravnborg 2018-08-02 165
val);
9fcebca9 Sam Ravnborg 2018-08-02 166 wait_busy(pdd);
9fcebca9 Sam Ravnborg 2018-08-02 167 pardata_strobe_write(pdd);
9fcebca9 Sam Ravnborg 2018-08-02 168 }
9fcebca9 Sam Ravnborg 2018-08-02 169
9fcebca9 Sam Ravnborg 2018-08-02 170 return 0;
9fcebca9 Sam Ravnborg 2018-08-02 171 }
9fcebca9 Sam Ravnborg 2018-08-02 172
---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
