saa7134 and �¼PD61151 MPEG2 coder

Message ID 20100209144150.17fafc52@glory.loctelecom.ru (mailing list archive)
State Superseded, archived
Headers

Commit Message

Dmitri Belimov Feb. 9, 2010, 5:41 a.m. UTC
  
  

Comments

Hans Verkuil Feb. 12, 2010, 3:44 p.m. UTC | #1
On Tuesday 09 February 2010 06:41:50 Dmitri Belimov wrote:
> Hi Hans
> 
> This is my last state for review.
> After small time I'll finish process of initialize the encoder.
> Configure some register, upload two firmware for video and for audio.
> Configure the frontends.
> 
> I have the questions.
> For configuring audio frontend need know samplerate of audio.
> saa7134 can only 32kHz
> saa7131/3/5 on I2S 32??? from SIF source and 32/44.1/48 from external i.e.
> RCA stereo audio input. 
> 
> Hardcode 32kHz or need a function for determine mode of audio??

See struct v4l2_subdev_audio_ops: it has a s_clock_freq op for precisely that
purpose. The saa7134 should call that whenever it sets a new samplerate.

> 
> Other question. For configure VideoFrontend need know 50 or 60Hz
> Now I use videomode from h structure. I think more correct detect it
> on saa7134.

Whether it is 50 or 60 Hz depends on the video standard that you receive via
the s_std core op. Just implement that and when you get a new standard you
can use something like this: is_60hz = (std & V4L2_STD_525_60) ? 1 : 0;

Some more review comments:

linux/drivers/media/video/saa7134/saa7134.h:

@@ -355,6 +377,10 @@
        unsigned char           empress_addr;
        unsigned char           rds_addr;
 
+       /* SPI info */
+       struct saa7134_software_spi     spi;
+       struct spi_board_info   spi_conf;

Make this a struct spi_board_info *. This struct is too large: it is only used
in one board but all elements of the board array will suddenly get this whole
struct increasing the memory footprint substantially. In this case you can just
make it a pointer, that will work just as well.

+
        unsigned int            tda9887_conf;
        unsigned int            tuner_config;

linux/drivers/media/video/v4l2-common.c, in v4l2_spi_subdev_init():

+       /* initialize name */
+       snprintf(sd->name, sizeof(sd->name), "%s",
+               spi->dev.driver->name);

Use strlcpy here.

saa7134-spi.c:


static inline u32 getmiso(struct spi_device *dev)
{
        struct saa7134_spi_gpio *sb = to_sb(dev);
        unsigned long status;

        status = saa7134_get_gpio(sb->controller_data);
        if ( status & (1 << sb->controller_data->spi.miso))
                return 1;
        else
                return 0;
}

Simplify to:

static inline u32 getmiso(struct spi_device *dev)
{
        struct saa7134_spi_gpio *sb = to_sb(dev);
        u32 status;

        status = saa7134_get_gpio(sb->controller_data);
        return !!(status & (1 << sb->controller_data->spi.miso));
}

Also note that saa7134_get_gpio should return an u32 since unsigned long is
64 bits when compiled on a 64-bit kernel, which is probably not what you want.

saa7134_spi_unregister can be a void function as the result code is always 0.

There seems to be some old stuff in upd61151.h. Please remove what is not
needed.

In upd61151.c I highly recommend that all functions will use struct v4l2_subdev *sd
as argument. Only at the lowest level should you go from sd to spi. Among
others this allows you to use the standard v4l2_info/dbg etc. logging functions.

Don't use RESULT_SUCCESS. Just return 0.

Remove upd61151_init. The init op is rarely needed and should in general not
be used.

Remove those emacs editor comments at the end of the files. That's bad practice.

Regards,

	Hans
  

Patch

diff -r b6b82258cf5e linux/drivers/media/video/saa7134/Makefile
--- a/linux/drivers/media/video/saa7134/Makefile	Thu Dec 31 19:14:54 2009 -0200
+++ b/linux/drivers/media/video/saa7134/Makefile	Tue Feb 09 07:38:53 2010 +0900
@@ -1,9 +1,9 @@ 
 
 saa7134-objs :=	saa7134-cards.o saa7134-core.o saa7134-i2c.o	\
 		saa7134-ts.o saa7134-tvaudio.o saa7134-vbi.o    \
-		saa7134-video.o saa7134-input.o
+		saa7134-video.o saa7134-input.o saa7134-spi.o
 
-obj-$(CONFIG_VIDEO_SAA7134) +=  saa6752hs.o saa7134.o saa7134-empress.o
+obj-$(CONFIG_VIDEO_SAA7134) +=  saa6752hs.o saa7134.o saa7134-empress.o upd61151.o
 
 obj-$(CONFIG_VIDEO_SAA7134_ALSA) += saa7134-alsa.o
 
diff -r b6b82258cf5e linux/drivers/media/video/saa7134/saa7134-cards.c
--- a/linux/drivers/media/video/saa7134/saa7134-cards.c	Thu Dec 31 19:14:54 2009 -0200
+++ b/linux/drivers/media/video/saa7134/saa7134-cards.c	Tue Feb 09 07:38:53 2010 +0900
@@ -4619,6 +4619,7 @@ 
 			.name = name_radio,
 			.amux = LINE2,
 		},
+		.encoder_type = SAA7134_ENCODER_SAA6752HS,
 		.mpeg  = SAA7134_MPEG_EMPRESS,
 		.video_out = CCIR656,
 		.vid_port_opts  = (SET_T_CODE_POLARITY_NON_INVERTED |
@@ -4656,6 +4657,7 @@ 
 			.name = name_radio,
 			.amux = LINE2,
 		},
+		.encoder_type = SAA7134_ENCODER_SAA6752HS,
 		.mpeg  = SAA7134_MPEG_EMPRESS,
 		.video_out = CCIR656,
 		.vid_port_opts  = (SET_T_CODE_POLARITY_NON_INVERTED |
@@ -4695,6 +4697,7 @@ 
 			.name = name_radio,
 			.amux = LINE2,
 		},
+		.encoder_type = SAA7134_ENCODER_SAA6752HS,
 		.mpeg  = SAA7134_MPEG_EMPRESS,
 		.video_out = CCIR656,
 		.vid_port_opts  = (SET_T_CODE_POLARITY_NON_INVERTED |
@@ -5279,23 +5282,51 @@ 
 		.tuner_addr     = ADDR_UNSET,
 		.radio_addr     = ADDR_UNSET,
 		.mpeg           = SAA7134_MPEG_DVB,
+		.gpiomask       = 0x00860000,
 		.inputs         = { {
 			.name = name_tv,
 			.vmux = 2,
 			.amux = TV,
 			.tv   = 1,
-		}, {
-			.name = name_comp1,
-			.vmux = 0,
-			.amux = LINE1,
+			.gpio = 0x00860000
+		}, {
+			.name = name_comp1,
+			.vmux = 0,
+			.amux = LINE1,
+			.gpio = 0x00860000
 		}, {
 			.name = name_svideo,
 			.vmux = 9,
 			.amux = LINE1,
-		} },
-		.radio = {
-			.name = name_radio,
-			.amux = TV,
+			.gpio = 0x00860000
+		} },
+		.radio = {
+			.name = name_radio,
+			.amux = TV,
+			.gpio = 0x00860000
+		},
+		.encoder_type = SAA7134_ENCODER_muPD61151,
+		.mpeg  = SAA7134_MPEG_EMPRESS,
+		.video_out = CCIR656,
+		.vid_port_opts  = (SET_T_CODE_POLARITY_NON_INVERTED |
+					SET_CLOCK_NOT_DELAYED |
+					SET_CLOCK_INVERTED |
+					SET_VSYNC_OFF),
+		.spi = {
+			.cs    = 17,
+			.clock = 18,
+			.mosi  = 23,
+			.miso  = 21,
+			.num_chipselect = 1,
+			.spi_enable = 1,
+		},
+		.spi_conf = {
+			.modalias	= "upd61151",
+			.max_speed_hz	= 50000000,
+			.chip_select	= 0,
+			.mode		= SPI_MODE_0,
+			.controller_data = NULL,
+			.platform_data  = NULL,
 		},
 	},
 	[SAA7134_BOARD_ZOLID_HYBRID_PCI] = {
diff -r b6b82258cf5e linux/drivers/media/video/saa7134/saa7134-core.c
--- a/linux/drivers/media/video/saa7134/saa7134-core.c	Thu Dec 31 19:14:54 2009 -0200
+++ b/linux/drivers/media/video/saa7134/saa7134-core.c	Tue Feb 09 07:38:53 2010 +0900
@@ -139,6 +139,18 @@ 
 		break;
 	}
 }
+
+unsigned long saa7134_get_gpio(struct saa7134_dev *dev)
+{
+	unsigned long status;
+
+	/* rising SAA7134_GPIO_GPRESCAN reads the status */
+	saa_andorb(SAA7134_GPIO_GPMODE3,SAA7134_GPIO_GPRESCAN,0);
+	saa_andorb(SAA7134_GPIO_GPMODE3,SAA7134_GPIO_GPRESCAN,SAA7134_GPIO_GPRESCAN);
+	status = saa_readl(SAA7134_GPIO_GPSTATUS0 >> 2) & 0xfffffff;
+	return status;
+}
+
 
 /* ------------------------------------------------------------------ */
 
@@ -1057,12 +1069,42 @@ 
 
 	saa7134_hwinit2(dev);
 
-	/* load i2c helpers */
+	/* initialize software SPI bus */
+	if (saa7134_boards[dev->board].spi.spi_enable)
+	{
+		dev->spi = saa7134_boards[dev->board].spi;
+
+		/* register SPI master and SPI slave */
+		if (saa7134_spi_register(dev, &saa7134_boards[dev->board].spi_conf))
+			saa7134_boards[dev->board].spi.spi_enable = 0;
+	}
+
+	/* load bus helpers */
 	if (card_is_empress(dev)) {
-		struct v4l2_subdev *sd =
-			v4l2_i2c_new_subdev(&dev->v4l2_dev, &dev->i2c_adap,
+		struct v4l2_subdev *sd = NULL;
+
+		dev->encoder_type = saa7134_boards[dev->board].encoder_type;
+
+		switch (dev->encoder_type) {
+		case SAA7134_ENCODER_muPD61151:
+		{
+			printk(KERN_INFO "%s: found muPD61151 MPEG encoder\n", dev->name);
+
+			if (saa7134_boards[dev->board].spi.spi_enable)
+				sd = v4l2_spi_new_subdev(&dev->v4l2_dev, dev->spi_adap, &saa7134_boards[dev->board].spi_conf);
+		}
+			break;
+		case SAA7134_ENCODER_SAA6752HS:
+		{
+			sd = v4l2_i2c_new_subdev(&dev->v4l2_dev, &dev->i2c_adap,
 				"saa6752hs", "saa6752hs",
 				saa7134_boards[dev->board].empress_addr, NULL);
+		}
+			break;
+		default:
+			printk(KERN_INFO "%s: MPEG encoder is not configured\n", dev->name);
+		    break;
+		}
 
 		if (sd)
 			sd->grp_id = GRP_EMPRESS;
@@ -1139,6 +1181,8 @@ 
 	return 0;
 
  fail4:
+	if ((card_is_empress(dev)) && (dev->encoder_type == SAA7134_ENCODER_muPD61151))
+		saa7134_spi_unregister(dev);
 	saa7134_unregister_video(dev);
 	saa7134_i2c_unregister(dev);
 	free_irq(pci_dev->irq, dev);
@@ -1412,6 +1456,7 @@ 
 /* ----------------------------------------------------------- */
 
 EXPORT_SYMBOL(saa7134_set_gpio);
+EXPORT_SYMBOL(saa7134_get_gpio);
 EXPORT_SYMBOL(saa7134_boards);
 
 /* ----------------- for the DMA sound modules --------------- */
diff -r b6b82258cf5e linux/drivers/media/video/saa7134/saa7134.h
--- a/linux/drivers/media/video/saa7134/saa7134.h	Thu Dec 31 19:14:54 2009 -0200
+++ b/linux/drivers/media/video/saa7134/saa7134.h	Tue Feb 09 07:38:53 2010 +0900
@@ -30,6 +30,13 @@ 
 #include <linux/notifier.h>
 #include <linux/delay.h>
 #include <linux/mutex.h>
+
+/* ifdef software SPI insert here start */
+#include <linux/platform_device.h>
+#include <linux/spi/spi.h>
+#include <linux/spi/spi_gpio.h>
+#include <linux/spi/spi_bitbang.h>
+/* ifdef software SPI insert here stop */
 
 #include <asm/io.h>
 
@@ -337,6 +344,21 @@ 
 	SAA7134_MPEG_TS_SERIAL,
 };
 
+enum saa7134_encoder_type {
+	SAA7134_ENCODER_UNUSED,
+	SAA7134_ENCODER_SAA6752HS,
+	SAA7134_ENCODER_muPD61151,
+};
+
+struct saa7134_software_spi {
+	unsigned char cs:5;
+	unsigned char clock:5;
+	unsigned char mosi:5;
+	unsigned char miso:5;
+	unsigned char num_chipselect:3;
+	unsigned char spi_enable:1;
+};
+
 struct saa7134_board {
 	char                    *name;
 	unsigned int            audio_clock;
@@ -355,6 +377,10 @@ 
 	unsigned char		empress_addr;
 	unsigned char		rds_addr;
 
+	/* SPI info */
+	struct saa7134_software_spi	spi;
+	struct spi_board_info   spi_conf;
+
 	unsigned int            tda9887_conf;
 	unsigned int            tuner_config;
 
@@ -362,6 +388,7 @@ 
 	enum saa7134_video_out  video_out;
 	enum saa7134_mpeg_type  mpeg;
 	enum saa7134_mpeg_ts_type ts_type;
+	enum saa7134_encoder_type encoder_type;
 	unsigned int            vid_port_opts;
 	unsigned int            ts_force_val:1;
 };
@@ -506,6 +533,12 @@ 
 	void                       (*signal_change)(struct saa7134_dev *dev);
 };
 
+struct saa7134_spi_gpio {
+	struct spi_bitbang         bitbang;
+	struct spi_master          *master;
+	struct saa7134_dev         *controller_data;
+};
+
 /* global device status */
 struct saa7134_dev {
 	struct list_head           devlist;
@@ -553,6 +586,10 @@ 
 	struct i2c_client          i2c_client;
 	unsigned char              eedata[256];
 	int 			   has_rds;
+
+	/* software spi */
+	struct saa7134_software_spi spi;
+	struct spi_master          *spi_adap;
 
 	/* video overlay */
 	struct v4l2_framebuffer    ovbuf;
@@ -615,6 +652,7 @@ 
 	atomic_t 		   empress_users;
 	struct work_struct         empress_workqueue;
 	int                        empress_started;
+	enum saa7134_encoder_type  encoder_type;
 
 #if defined(CONFIG_VIDEO_SAA7134_DVB) || defined(CONFIG_VIDEO_SAA7134_DVB_MODULE)
 	/* SAA7134_MPEG_DVB only */
@@ -681,6 +719,7 @@ 
 
 void saa7134_track_gpio(struct saa7134_dev *dev, char *msg);
 void saa7134_set_gpio(struct saa7134_dev *dev, int bit_no, int value);
+unsigned long saa7134_get_gpio(struct saa7134_dev *dev);
 
 #define SAA7134_PGTABLE_SIZE 4096
 
@@ -726,6 +765,11 @@ 
 int saa7134_i2c_register(struct saa7134_dev *dev);
 int saa7134_i2c_unregister(struct saa7134_dev *dev);
 
+/* ----------------------------------------------------------- */
+/* saa7134-spi.c                                               */
+
+int saa7134_spi_register(struct saa7134_dev *dev, struct spi_board_info *info);
+int saa7134_spi_unregister(struct saa7134_dev *dev);
 
 /* ----------------------------------------------------------- */
 /* saa7134-video.c                                             */
diff -r b6b82258cf5e linux/drivers/media/video/v4l2-common.c
--- a/linux/drivers/media/video/v4l2-common.c	Thu Dec 31 19:14:54 2009 -0200
+++ b/linux/drivers/media/video/v4l2-common.c	Tue Feb 09 07:38:53 2010 +0900
@@ -51,6 +51,7 @@ 
 #include <linux/string.h>
 #include <linux/errno.h>
 #include <linux/i2c.h>
+#include <linux/spi/spi.h>
 #include <asm/uaccess.h>
 #include <asm/system.h>
 #include <asm/pgtable.h>
@@ -1069,6 +1070,67 @@ 
 
 #endif /* defined(CONFIG_I2C) */
 
+//#if defined(CONFIG_SPI) || (defined(CONFIG_SPI_MODULE) && defined(MODULE)) + SPI_BITBANG
+
+/* Load an spi sub-device. */
+
+void v4l2_spi_subdev_init(struct v4l2_subdev *sd, struct spi_device *spi,
+		const struct v4l2_subdev_ops *ops)
+{
+	v4l2_subdev_init(sd, ops);
+	sd->flags |= V4L2_SUBDEV_FL_IS_SPI;
+	/* the owner is the same as the spi_device's driver owner */
+	sd->owner = spi->dev.driver->owner;
+	/* spi_device and v4l2_subdev point to one another */
+	v4l2_set_subdevdata(sd, spi);
+	spi_set_drvdata(spi, sd);
+	/* initialize name */
+	snprintf(sd->name, sizeof(sd->name), "%s",
+		spi->dev.driver->name);
+}
+EXPORT_SYMBOL_GPL(v4l2_spi_subdev_init);
+
+struct v4l2_subdev *v4l2_spi_new_subdev(struct v4l2_device *v4l2_dev,
+		struct spi_master *master, struct spi_board_info *info)
+{
+	struct v4l2_subdev *sd = NULL;
+	struct spi_device *spi = NULL;
+
+	BUG_ON(!v4l2_dev);
+
+	if (info->modalias)
+		request_module(info->modalias);
+
+	spi = spi_new_device(master,info);
+
+	if (spi == NULL || spi->dev.driver ==NULL)
+		goto error;
+
+	if (!try_module_get(spi->dev.driver->owner))
+		goto error;
+
+	sd = spi_get_drvdata(spi);
+
+	/* Register with the v4l2_device which increases the module's
+	   use count as well. */
+	if (v4l2_device_register_subdev(v4l2_dev, sd))
+		sd = NULL;
+
+	/* Decrease the module use count to match the first try_module_get. */
+	module_put(spi->dev.driver->owner);
+
+error:
+	/* If we have a client but no subdev, then something went wrong and
+	   we must unregister the client. */
+	if (spi && sd == NULL)
+		spi_unregister_device(spi);
+
+	return sd;
+}
+EXPORT_SYMBOL_GPL(v4l2_spi_new_subdev);
+
+//#endif /* defined(CONFIG_SPI) */
+
 /* Clamp x to be between min and max, aligned to a multiple of 2^align.  min
  * and max don't have to be aligned, but there must be at least one valid
  * value.  E.g., min=17,max=31,align=4 is not allowed as there are no multiples
diff -r b6b82258cf5e linux/include/media/v4l2-common.h
--- a/linux/include/media/v4l2-common.h	Thu Dec 31 19:14:54 2009 -0200
+++ b/linux/include/media/v4l2-common.h	Tue Feb 09 07:38:53 2010 +0900
@@ -191,6 +191,25 @@ 
 
 /* ------------------------------------------------------------------------- */
 
+/* SPI Helper functions */
+
+#include <linux/spi/spi.h>
+
+struct spi_device_id;
+struct spi_device;
+
+/* Load an spi module and return an initialized v4l2_subdev struct.
+   Only call request_module if module_name != NULL.
+   The client_type argument is the name of the chip that's on the adapter. */
+struct v4l2_subdev *v4l2_spi_new_subdev(struct v4l2_device *v4l2_dev,
+		struct spi_master *master, struct spi_board_info *info);
+
+/* Initialize an v4l2_subdev with data from an spi_device struct */
+void v4l2_spi_subdev_init(struct v4l2_subdev *sd, struct spi_device *spi,
+		const struct v4l2_subdev_ops *ops);
+
+/* ------------------------------------------------------------------------- */
+
 /* Note: these remaining ioctls/structs should be removed as well, but they are
    still used in tuner-simple.c (TUNER_SET_CONFIG), cx18/ivtv (RESET) and
    v4l2-int-device.h (v4l2_routing). To remove these ioctls some more cleanup
diff -r b6b82258cf5e linux/include/media/v4l2-subdev.h
--- a/linux/include/media/v4l2-subdev.h	Thu Dec 31 19:14:54 2009 -0200
+++ b/linux/include/media/v4l2-subdev.h	Tue Feb 09 07:38:53 2010 +0900
@@ -387,6 +387,8 @@ 
 
 /* Set this flag if this subdev is a i2c device. */
 #define V4L2_SUBDEV_FL_IS_I2C (1U << 0)
+/* Set this flag if this subdev is a spi device. */
+#define V4L2_SUBDEV_FL_IS_SPI (1U << 1)
 
 /* Each instance of a subdev driver should create this struct, either
    stand-alone or embedded in a larger struct.