Re: [PATCH v4] media: spi: Add support for LMH0395

[Jean-Michel Hautbois]

Hi Laurent,

Thanks for the review.

2014-11-03 14:13 GMT+01:00 Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>:
> Hi Jean-Michel,
>
> Thank you for the patch.
>
> On Wednesday 10 September 2014 11:43:54 Jean-Michel Hautbois wrote:
>> This device is a SPI based device from TI.
>> It is a 3 Gbps HD/SD SDI Dual Output Low Power
>> Extended Reach Adaptive Cable Equalizer.
>>
>> LMH0395 enables the use of up to two outputs.
>> These can be configured using DT.
>>
>> Controls should be accessible from userspace too.
>> This will have to be done later.
>>
>> v2: Add DT support
>> v3: Change the bit set/clear in output_type as disabled means 'set the bit'
>> v4: Clearer description of endpoints usage in Doc, and some init changes.
>>     Add a dependency on OF and don't test CONFIG_OF anymore.
>>
>> Signed-off-by: Jean-Michel Hautbois <jean-michel.hautbois@xxxxxxxxxxx>
>> ---
>>  .../devicetree/bindings/media/spi/lmh0395.txt      |  48 +++
>>  MAINTAINERS                                        |   6 +
>>  drivers/media/spi/Kconfig                          |  14 +
>>  drivers/media/spi/Makefile                         |   1 +
>>  drivers/media/spi/lmh0395.c                        | 354 ++++++++++++++++++
>>  5 files changed, 423 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/media/spi/lmh0395.txt
>>  create mode 100644 drivers/media/spi/Kconfig
>>  create mode 100644 drivers/media/spi/Makefile
>>  create mode 100644 drivers/media/spi/lmh0395.c
>>
>> diff --git a/Documentation/devicetree/bindings/media/spi/lmh0395.txt
>> b/Documentation/devicetree/bindings/media/spi/lmh0395.txt new file mode
>> 100644
>> index 0000000..7cc0026
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/media/spi/lmh0395.txt
>> @@ -0,0 +1,48 @@
>> +* Texas Instruments lmh0395 3G HD/SD SDI equalizer
>> +
>> +The LMH0395 3 Gbps HD/SD SDI Dual Output Low Power Extended Reach Adaptive
>> +Cable Equalizer is designed to equalize data transmitted over cable (or any
>> +media with similar dispersive loss characteristics).
>> +The equalizer operates over a wide range of data rates from 125 Mbps to
>> 2.97 Gbps
>> +and supports SMPTE 424M, SMPTE 292M, SMPTE344M, SMPTE 259M, and DVB-ASI
>> standards.
>
> That's copied verbatim from the datasheet and not very relevant from a DT
> bindings point of view. I would rather explain what the chip does. Something
> like
>
> "The LMH0395 is an SDI equalizer designed to extend the reach of SDI signals
> transmitted over cable by equalizing the input signal and generating clean
> outputs. It has one differential input and two differential output that can be
> independently controlled."

OK, applied.

>> +
>> +Required Properties :
>> +- compatible: Must be "ti,lmh0395"
>> +
>> +The device node must contain one 'port' child node per device input and
>> output
>> +port, in accordance with the video interface bindings defined in
>> +Documentation/devicetree/bindings/media/video-interfaces.txt. The port
>> nodes
>> +are numbered as follows.
>> +
>> +  Port                       LMH0395
>> +------------------------------------------------------------
>> +  SDI input          0
>> +  SDI output         1,2
>
> While there shouldn't be much doubt about SDO0 corresponding to port 1 and
> SDO1 to port 2, I'd like that to be mentioned explicitly.
>
> The device node must contain one 'port' child node per device input and output
> port, in accordance with the video interface bindings defined in
> Documentation/devicetree/bindings/media/video-interfaces.txt.
>
> The LMH0395 has three ports numbered as follows.
>
> Description            Number
> ------------------------------------------------------------
> SDI (SDI input)         0
> SDO0 (SDI output 0)     1
> SDO1 (SDI output 1)     2

OK.

>> +Example:
>> +
>> +ecspi@02010000 {
>> +     ...
>> +     ...
>> +
>> +     lmh0395@1 {
>> +             compatible = "ti,lmh0395";
>> +             reg = <1>;
>> +             spi-max-frequency = <20000000>;
>> +             ports {
>
> You need to add
>
> #address-cells = <1>;
> #size-cells = <0>;
>
> here.
>

Of course, applied.

>> +                     port@0 {
>> +                             reg = <0>;
>> +                             sdi0_in: endpoint {};
>> +                     };
>> +                     port@1 {
>> +                             reg = <1>;
>> +                             sdi0_out0: endpoint {};
>> +                     };
>> +                     port@2 {
>> +                             reg = <2>;
>> +                             /* endpoint not specified, disable output */
>> +                     };
>> +             };
>> +     };
>> +     ...
>> +};
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index cf24bb5..ca42b9e 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -9141,6 +9141,12 @@ S:     Maintained
>>  F:   sound/soc/codecs/lm49453*
>>  F:   sound/soc/codecs/isabelle*
>>
>> +TI LMH0395 DRIVER
>> +M:   Jean-Michel Hautbois <jean-michel.hautbois@xxxxxxxxxxx>
>> +L:   linux-media@xxxxxxxxxxxxxxx
>> +S:   Maintained
>> +F:   drivers/media/spi/lmh0395*
>> +
>>  TI LP855x BACKLIGHT DRIVER
>>  M:   Milo Kim <milo.kim@xxxxxx>
>>  S:   Maintained
>> diff --git a/drivers/media/spi/Kconfig b/drivers/media/spi/Kconfig
>> new file mode 100644
>> index 0000000..bcb1ab1
>> --- /dev/null
>> +++ b/drivers/media/spi/Kconfig
>> @@ -0,0 +1,14 @@
>> +if VIDEO_V4L2
>> +
>> +config VIDEO_LMH0395
>> +     tristate "LMH0395 equalizer"
>> +     depends on VIDEO_V4L2 && SPI && MEDIA_CONTROLLER && OF
>> +     ---help---
>> +       Support for TI LMH0395 3G HD/SD SDI Dual Output Low Power
>> +       Extended Reach Adaptive Cable Equalizer.
>> +
>> +       To compile this driver as a module, choose M here: the
>> +       module will be called lmh0395.
>> +
>> +
>> +endif
>> diff --git a/drivers/media/spi/Makefile b/drivers/media/spi/Makefile
>> new file mode 100644
>> index 0000000..6c587e5
>> --- /dev/null
>> +++ b/drivers/media/spi/Makefile
>> @@ -0,0 +1 @@
>> +obj-$(CONFIG_VIDEO_LMH0395)  += lmh0395.o
>> diff --git a/drivers/media/spi/lmh0395.c b/drivers/media/spi/lmh0395.c
>> new file mode 100644
>> index 0000000..3eca0df
>> --- /dev/null
>> +++ b/drivers/media/spi/lmh0395.c
>> @@ -0,0 +1,354 @@
>> +/*
>> + * LMH0395 SPI driver.
>> + * Copyright (C) 2014  Jean-Michel Hautbois
>> + *
>> + * 3G HD/SD SDI Dual Output Low Power Extended Reach Adaptive Cable
>> Equalizer
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +#include <linux/ioctl.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/types.h>
>> +#include <linux/slab.h>
>> +#include <linux/uaccess.h>
>> +#include <linux/spi/spi.h>
>
> Please keep the headers alphabetically ordered.

OK.

>> +#include <linux/videodev2.h>
>> +#include <media/v4l2-device.h>
>> +#include <media/v4l2-of.h>
>> +
>> +static int debug;
>> +module_param(debug, int, 0644);
>> +MODULE_PARM_DESC(debug, "debug level (0-1)");
>
> I wonder whether this is really needed. If you follow my proposal to implement
> the log_status handler most v4l2_dbg call will turn into info calls. Of the
> two remaining v4l2_dbg calls one could be removed (please see below again),
> and the other turned into a dev_dbg.

I removed it, and corrected debug messages accordingly.

>> +
>> +#define      LMH0395_SPI_CMD_WRITE   0x00
>> +#define      LMH0395_SPI_CMD_READ    0x80
>> +
>> +/* Registers of LMH0395 */
>> +#define LMH0395_GENERAL_CTRL         0x00
>> +#define LMH0395_OUTPUT_DRIVER                0x01
>> +#define LMH0395_LAUNCH_AMP_CTRL              0x02
>> +#define LMH0395_MUTE_REF             0x03
>> +#define LMH0395_DEVICE_ID            0x04
>> +#define      LMH0395_RATE_INDICATOR          0x05
>> +#define      LMH0395_CABLE_LENGTH_INDICATOR  0x06
>> +#define      LMH0395_LAUNCH_AMP_INDICATION   0x07
>> +
>> +/* This is a one input, dual output device */
>> +#define LMH0395_SDI_INPUT    0
>> +#define LMH0395_SDI_OUT0     1
>> +#define LMH0395_SDI_OUT1     2
>> +
>> +#define LMH0395_PADS_NUM     3
>> +
>> +/* Register LMH0395_MUTE_REF bits [7:6] */
>> +enum lmh0395_output_type {
>> +     LMH0395_OUTPUT_TYPE_NONE,
>> +     LMH0395_OUTPUT_TYPE_SDO0,
>> +     LMH0395_OUTPUT_TYPE_SDO1,
>> +     LMH0395_OUTPUT_TYPE_BOTH
>> +};
>> +
>> +static const char * const output_strs[] = {
>> +     "No Output Driver",
>> +     "Output Driver 0",
>> +     "Output Driver 1",
>> +     "Output Driver 0+1",
>> +};
>> +
>> +
>> +/* spi implementation */
>> +
>> +static int lmh0395_spi_write(struct spi_device *spi, u8 reg, u8 data)
>> +{
>> +     int err;
>> +     u8 cmd[2];
>> +
>> +     cmd[0] = LMH0395_SPI_CMD_WRITE | reg;
>> +     cmd[1] = data;
>> +
>> +     err = spi_write(spi, cmd, 2);
>> +     if (err < 0) {
>> +             dev_err(&spi->dev, "SPI failed to select reg\n");
>
> I don't think the message is really accurate. "SPI write failed" would seem a
> better description to me. I would also print the error value, that can be
> useful for debugging.
>

OK.

>> +             return err;
>> +     }
>> +
>> +     return err;
>> +}
>> +
>> +static int lmh0395_spi_read(struct spi_device *spi, u8 reg, u8 *data)
>> +{
>> +     int err;
>> +     u8 cmd[2];
>> +     u8 read_data[2];
>> +
>> +     cmd[0] = LMH0395_SPI_CMD_READ | reg;
>> +     cmd[1] = 0xff;
>> +
>> +     err = spi_write(spi, cmd, 2);
>> +     if (err < 0) {
>> +             dev_err(&spi->dev, "SPI failed to select reg\n");
>
> Same here and below, the error value can be useful.
>
>> +             return err;
>> +     }
>> +
>> +     err = spi_read(spi, read_data, 2);
>> +     if (err < 0) {
>> +             dev_err(&spi->dev, "SPI failed to read reg\n");
>> +             return err;
>> +     }
>> +     /* The first 8 bits is the adress used, drop it */
>> +     *data = read_data[1];
>> +
>> +     return err;
>> +}
>> +
>> +struct lmh0395_state {
>> +     struct v4l2_subdev sd;
>> +     struct media_pad pads[LMH0395_PADS_NUM];
>> +     enum lmh0395_output_type output_type;
>> +};
>> +
>> +static inline struct lmh0395_state *to_state(struct v4l2_subdev *sd)
>> +{
>> +     return container_of(sd, struct lmh0395_state, sd);
>> +}
>> +
>> +static int lmh0395_set_output_type(struct v4l2_subdev *sd, u32 output)
>> +{
>> +     struct lmh0395_state *state = to_state(sd);
>> +     struct spi_device *spi = v4l2_get_subdevdata(sd);
>> +     u8 muteref_reg;
>> +
>> +     switch (output) {
>> +     case LMH0395_OUTPUT_TYPE_SDO0:
>> +             lmh0395_spi_read(spi, LMH0395_MUTE_REF, &muteref_reg);
>> +             muteref_reg &= ~(1 << 6);
>> +             break;
>> +     case LMH0395_OUTPUT_TYPE_SDO1:
>> +             lmh0395_spi_read(spi, LMH0395_MUTE_REF, &muteref_reg);
>> +             muteref_reg &= (1 << 7);
>> +             break;
>> +     case LMH0395_OUTPUT_TYPE_BOTH:
>> +             lmh0395_spi_read(spi, LMH0395_MUTE_REF, &muteref_reg);
>> +             muteref_reg |= 0x00 << 6;
>
> This line does absolutely nothing.
>
>> +             break;
>> +     case LMH0395_OUTPUT_TYPE_NONE:
>> +             lmh0395_spi_read(spi, LMH0395_MUTE_REF, &muteref_reg);
>> +             muteref_reg |= 0x11 << 6;
>
> 0x11 << 6, really ?
>
>> +             break;
>> +     default:
>> +             return -EINVAL;
>> +     }
>> +     v4l2_dbg(1, debug, sd, "Selecting %s output type\n",
>> +                                     output_strs[output]);
>> +
>> +     /* The following settings will have to be dynamic */
>> +     muteref_reg &= ~(0x1f); /* Muteref enable */
>> +     muteref_reg |= 1 << 5; /* Digital Muteref */
>
> Just set all the bits explicitly and get rid of the various reads in the case
> statements above, the function will be simpler.
>

OK, using now set_bit() and clear_bit().

>> +     lmh0395_spi_write(spi, LMH0395_MUTE_REF, muteref_reg);
>> +
>> +     state->output_type = output;
>> +     return 0;
>> +
>> +}
>> +
>> +static int lmh0395_get_rate(struct v4l2_subdev *sd, u8 *rate)
>> +{
>> +     struct spi_device *spi = v4l2_get_subdevdata(sd);
>> +     int err;
>> +     u8 ctrl;
>> +
>> +     err = lmh0395_spi_read(spi, LMH0395_RATE_INDICATOR, &ctrl);
>> +     if (err < 0)
>> +             return err;
>> +
>> +     *rate = ctrl & 0x20;
>> +     v4l2_dbg(1, debug, sd, "Rate : %s\n", (ctrl & 0x20)?"3G/HD":"SD");
>
> You need spaces around the ? and the :.
>
>> +     return 0;
>> +}
>> +
>> +static int lmh0395_get_cable_length(struct v4l2_subdev *sd, u8 rate)
>> +{
>> +     struct spi_device *spi = v4l2_get_subdevdata(sd);
>> +     u8 length;
>> +     u8 cli;
>> +     int err;
>> +
>> +     err = lmh0395_spi_read(spi, LMH0395_CABLE_LENGTH_INDICATOR, &cli);
>> +     if (err < 0)
>> +             return err;
>> +
>> +     /* The cable length indicator (CLI) provides an indication of the
>> +      * length of the cable attached to input. CLI is accessible via bits
>> +      * [7:0] of SPI register 06h.
>> +      * The 8-bit setting ranges in decimal value from 0 to 247
>> +      * ("00000000" to "11110111" binary), corresponding to 0 to 400m of
>> +      * Belden 1694A cable.
>> +      * For 3G and HD input, CLI is 1.25m per step.
>> +      * For SD input, CLI is 1.25m per step, less 20m, from 0 to 191 decimal
>> +      * and 3.5m per step from 192 to 247 decimal.
>> +      */
>> +
>> +     length = cli*5/4;
>> +     if (rate == 0) {
>> +             if (cli <= 191)
>> +                     length -= 20;
>> +             else
>> +                     length = ((191*5/4)-20) + ((cli-191)*7/2);
>> +
>> +     }
>> +     v4l2_dbg(1, debug, sd, "Length estimated (BELDEN 1694A cables) : %dm\n",
>> +                     length);
>> +     return 0;
>> +}
>> +
>> +static int lmh0395_get_control(struct v4l2_subdev *sd)
>> +{
>> +     int err;
>> +     struct spi_device *spi = v4l2_get_subdevdata(sd);
>> +     u8 ctrl;
>> +     u8 rate;
>> +
>> +     err = lmh0395_spi_read(spi, LMH0395_GENERAL_CTRL, &ctrl);
>> +     if (err < 0)
>> +             return err;
>> +
>> +     if (ctrl & 0x80) {
>> +             v4l2_dbg(1, debug, sd, "Carrier detected\n");
>> +             lmh0395_get_rate(sd, &rate);
>> +             lmh0395_get_cable_length(sd, rate);
>> +     }
>> +     return 0;
>> +}
>
> This function is only called at probe time. Is it really useful ? If you want
> to keep it you should turn it into a subdev log_status handler instead.
>

Yes, I added a log status function.

>> +static int lmh0395_s_routing(struct v4l2_subdev *sd, u32 input, u32 output,
>> +                             u32 config)
>> +{
>> +     struct lmh0395_state *state = to_state(sd);
>> +     int err = 0;
>> +
>> +     if (state->output_type != output)
>> +             err = lmh0395_set_output_type(sd, output);
>> +
>> +     return err;
>
>         if (state->output_type == output)
>                 return 0;
>
>         return lmh0395_set_output_type(sd, output);
>
> You can then get rid of the err variable.
>
> I don't really like this implementation though, the output argument is device-
> specific, you thus require explicit knowledge of the LMH0395 in your bridge
> driver.
>
> I'm not sure what the config argument is used for, but naively I would have
> used it to tell whether to enable (1) or disable (0) the route from input to
> output. The input value should then always be 0, and the output value can be 1
> or 2. Another option would be to use the new S_ROUTING userspace ioctl I've
> proposed in
> http://git.linuxtv.org/cgit.cgi/pinchartl/media.git/commit/?h=xilinx-wip&id=fc094c354338861673464ed4b8fa198089fe7bf0.

Well, right now, this is one of the two points which block me for a
new review. As this API is not there, I don't use it, maybe could I
propose the driver with the "old fashion way" ?

> Hans, could you comment on that ?
>
>> +}
>> +
>> +static const struct v4l2_subdev_video_ops lmh0395_video_ops = {
>> +     .s_routing = lmh0395_s_routing,
>> +};
>> +
>> +static const struct v4l2_subdev_ops lmh0395_ops = {
>> +     .video = &lmh0395_video_ops,
>> +};
>> +
>> +static const struct spi_device_id lmh0395_id[] = {
>> +     { "lmh0395", 0 },
>> +     { }
>> +};
>> +MODULE_DEVICE_TABLE(spi, lmh0395_id);
>> +
>> +static const struct of_device_id lmh0395_of_match[] = {
>> +     {.compatible = "ti,lmh0395", },
>> +     { /* sentinel */ },
>> +};
>> +MODULE_DEVICE_TABLE(of, lmh0395_of_match);
>> +
>> +static int lmh0395_probe(struct spi_device *spi)
>> +{
>> +     u8 device_id;
>> +     struct lmh0395_state *state;
>> +     struct v4l2_subdev *sd;
>> +     int err;
>> +
>> +     err = lmh0395_spi_read(spi, LMH0395_DEVICE_ID, &device_id);
>> +     if (err < 0)
>> +             return err;
>> +
>> +     dev_dbg(&spi->dev, "device_id 0x%x\n", device_id);
>
> Shouldn't you validate the device ID value ?

Yes, and done now.

>> +     /* Now that the device is here, let's init V4L2 */
>> +     state = devm_kzalloc(&spi->dev, sizeof(*state), GFP_KERNEL);
>> +     if (!state)
>> +             return -ENOMEM;
>> +
>> +     sd = &state->sd;
>> +     v4l2_spi_subdev_init(sd, spi, &lmh0395_ops);
>> +
>> +     v4l2_dbg(1, debug, sd, "Configuring equalizer\n");
>
> Is this message really useful ? It will always be printed after the device_id
> message above, and thus adds little value.
>
>> +     lmh0395_get_control(sd);
>> +     /* Default is no output */
>> +     lmh0395_set_output_type(sd, LMH0395_OUTPUT_TYPE_NONE);
>> +
>> +     if (spi->dev.of_node) {
>> +             struct device_node *n = NULL;
>> +             struct of_endpoint ep;
>> +
>> +             while ((n = of_graph_get_next_endpoint(spi->dev.of_node, n))
>> +                                                             != NULL) {
>
> Philipp Zabel has submitted a patch named "of: Add for_each_endpoint_of_node
> helper macro" that should get merged in v3.19. The patch series modifies the
> behaviour of of_graph_get_next_endpoint() to drop the reference to the prev
> node. This would thus create a problem in the driver, which you should fix by
> using the for_each_endpoint_of_node() macro.
>

This is the second blocking point. Again, even if it has 7 versions
proposed right now, it is not going into 3.19 but in 3.20 at best.
So, what should I do :) ?

>> +                     of_graph_parse_endpoint(n, &ep);
>
> You should check the return value.
>

Done.

>> +                     dev_dbg(&spi->dev, "endpoint %d on port %d\n",
>> +                                             ep.id, ep.port);
>> +                     /* port 1 => SDO0 */
>> +                     if (ep.port >= 1)
>> +                             lmh0395_set_output_type(sd, ep.port);
>> +                     of_node_put(n);
>> +             }
>> +     } else {
>> +             dev_dbg(&spi->dev, "No DT configuration\n");
>
> Is that a case you want to support ?

No, but with the message, you at least know what goes wrong...

>> +     }
>> +
>> +     state->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
>> +     state->pads[LMH0395_SDI_INPUT].flags = MEDIA_PAD_FL_SINK;
>> +     state->pads[LMH0395_SDI_OUT0].flags = MEDIA_PAD_FL_SOURCE;
>> +     state->pads[LMH0395_SDI_OUT1].flags = MEDIA_PAD_FL_SOURCE;
>> +     err = media_entity_init(&sd->entity, LMH0395_PADS_NUM, state->pads, 0);
>> +     if (err)
>> +             return err;
>> +
>> +     err = v4l2_async_register_subdev(sd);
>> +     if (err < 0) {
>> +             media_entity_cleanup(&sd->entity);
>> +             return err;
>> +     }
>> +
>> +     dev_dbg(&spi->dev, "device probed\n");
>> +
>> +     return 0;
>> +}
>> +
>> +static int lmh0395_remove(struct spi_device *spi)
>> +{
>> +     struct v4l2_subdev *sd = spi_get_drvdata(spi);
>> +
>> +     v4l2_async_unregister_subdev(sd);
>> +     media_entity_cleanup(&sd->entity);
>> +     return 0;
>> +}
>> +
>> +
>
> There's an extra blank line here.
>
>> +static struct spi_driver lmh0395_driver = {
>> +     .driver = {
>> +             .of_match_table = lmh0395_of_match,
>> +             .name = "lmh0395",
>> +             .owner = THIS_MODULE,
>> +     },
>> +     .probe = lmh0395_probe,
>> +     .remove = lmh0395_remove,
>> +     .id_table = lmh0395_id,
>> +};
>> +
>> +module_spi_driver(lmh0395_driver);
>> +
>> +MODULE_DESCRIPTION("spi device driver for LMH0395 equalizer");
>> +MODULE_AUTHOR("Jean-Michel Hautbois");
>> +MODULE_LICENSE("GPL");
>
> --
> Regards,
>
> Laurent Pinchart
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/