On Thu, 7 Apr 2016 15:44:35 +0200
Micka <mickamusset@xxxxxxxxx> wrote:
Hello.
> I can't config the mail server .... on git, I don't have any mail
> server .... i'm using gmail.
>
> I prefer to keep the property clock-frequency: Because that way i
> don't need a node clock and I don't need to initialize a clock in the
> processor ( ret = clk_prepare_enable(s->clk) ).
I still can not understand why use a separate property to specify the frequency
Why you can not use "fixed-clock" node and just specify the type "osc" or "xtal"?
> I added the property no-gpio in case u don't want to use the gpio on
> the max310x.
You can re-use existing property "gpio-controller" for this, like as:
if (!of_find_property(np, "gpio-controller", NULL))
...
> Now, I'm concerned about two max310x working at the same time. What
> will happen ? There is no mutex to protect that in the driver. I hope
> that the spi driver will handle that problem. What do you think
> Alexander.
This is a work for SPI core.
...
> diff --git a/drivers/tty/serial/max310x.c b/drivers/tty/serial/max310x.c
> index 182549f..328bbe0 100644
> --- a/drivers/tty/serial/max310x.c
> +++ b/drivers/tty/serial/max310x.c
...
> struct max310x_port {
> - struct uart_driver uart;
> + int nr;
> + int index;
> +// struct uart_driver uart;
Linux style for comments is the C89 "/* ... */" style.
...
> static u8 max310x_port_read(struct uart_port *port, u8 reg)
> {
> struct max310x_port *s = dev_get_drvdata(port->dev);
> unsigned int val = 0;
> -
> regmap_read(s->regmap, port->iobase + reg, &val);
???
...
> @@ -1081,46 +1094,87 @@ static int
> max310x_gpio_direction_output(struct gpio_chip *chip,
> static int max310x_probe(struct device *dev, struct max310x_devtype *devtype,
> struct regmap *regmap, int irq, unsigned long flags)
> {
> - int i, ret, fmin, fmax, freq, uartclk;
> - struct clk *clk_osc, *clk_xtal;
> - struct max310x_port *s;
> - bool xtal = false;
> + int j,k;
> + int i, ret, fmin, fmax, freq=0, uartclk;
> + struct clk *clk_osc, *clk_xtal;
> + struct max310x_port *s;
> + bool xtal = false;
> + int retval = 0;
>
> if (IS_ERR(regmap))
> return PTR_ERR(regmap);
> + // did we registered the driver ?
> + if (!uart_driver_registered) {
> + // no, let's do it :
> + // set the flag to 1 for the next time.
> + uart_driver_registered = 1;
> + // register the driver :
> + retval = uart_register_driver(&max310x_uart_driver);
> + if (retval) {
> + // something goes wrong :
> + return retval;
> + }
> + number_of_uarts=0;
> + }
> + // find a free spot in the array :
> + for (k = 0; k < MAX_MAX310X; k++)
> + if (!max310xs[k])
> + break;
Are you sure max310xs[k] will be NULL after remove driver?
> + // test if we reach the end of the array
> + if (k == MAX_MAX310X) {
> + // no more space available
> + dev_warn(dev, "too many MAX310X chips\n");
> + return -ENOMEM;
> + }
Hmm... I think moving uart_register_driver in the "module_init" initcall
will be more elegant, like:
static int __init max310x_init(void)
{
ret = uart_register_driver(&max310x_uart_driver);
if (!ret) {
ret = spi_register_driver(&max310x_spi_driver);
...
}
return ret;
}
module_init(max310x_init);
...
> - clk_osc = devm_clk_get(dev, "osc");
> - clk_xtal = devm_clk_get(dev, "xtal");
> - if (!IS_ERR(clk_osc)) {
> - s->clk = clk_osc;
> - fmin = 500000;
> - fmax = 35000000;
> - } else if (!IS_ERR(clk_xtal)) {
> - s->clk = clk_xtal;
> - fmin = 1000000;
> - fmax = 4000000;
> - xtal = true;
> - } else if (PTR_ERR(clk_osc) == -EPROBE_DEFER ||
> - PTR_ERR(clk_xtal) == -EPROBE_DEFER) {
> - return -EPROBE_DEFER;
> - } else {
> - dev_err(dev, "Cannot get clock\n");
> - return -EINVAL;
> + // save the index of the array
> + s->index=k;
> + // test if we can get the frequency of the clock from the property :
> + if (of_find_property(dev->of_node, "clock-frequency", NULL)){
> + // cool, let's get it :
> + of_property_read_u32(dev->of_node, "clock-frequency", &freq);
> + }
> + // check if the frequency is indicated :
> + if(freq!=0){
> + // yes ! we don't need to build a clock in the processor :
> + fmin = 1000000;
> + fmax = 4000000;
> + xtal = 0;
> + }else{
> + // we need an internal clock
> + clk_osc = devm_clk_get(dev, "osc");
> + clk_xtal = devm_clk_get(dev, "xtal");
> + if (!IS_ERR(clk_osc)) {
> + s->clk = clk_osc;
> + fmin = 500000;
> + fmax = 35000000;
> + } else if (!IS_ERR(clk_xtal)) {
> + s->clk = clk_xtal;
> + fmin = 1000000;
> + fmax = 4000000;
> + xtal = true;
> + } else if (PTR_ERR(clk_osc) == -EPROBE_DEFER ||
> + PTR_ERR(clk_xtal) == -EPROBE_DEFER) {
> + return -EPROBE_DEFER;
> + } else {
> + dev_err(dev, "Cannot get clock\n");
> + return -EINVAL;
> + }
> + ret = clk_prepare_enable(s->clk);
> + if (ret)
> + return ret;
> + freq = clk_get_rate(s->clk);
> }
> -
> - ret = clk_prepare_enable(s->clk);
> - if (ret)
> - return ret;
> -
> - freq = clk_get_rate(s->clk);
> /* Check frequency limits */
> if (freq < fmin || freq > fmax) {
> ret = -ERANGE;
In any case please make clock & gpio changes as a separate part of patch.
...
> @@ -1133,23 +1187,27 @@ static int max310x_probe(struct device *dev,
> struct max310x_devtype *devtype,
>
> /* Check device to ensure we are talking to what we expect */
> ret = devtype->detect(dev);
> - if (ret)
> + if (ret){
> goto out_clk;
> -
> + }
??
> for (i = 0; i < devtype->nr; i++) {
> unsigned int offs = i << 5;
>
> - /* Reset port */
> - regmap_write(s->regmap, MAX310X_MODE2_REG + offs,
> - MAX310X_MODE2_RST_BIT);
> - /* Clear port reset */
> - regmap_write(s->regmap, MAX310X_MODE2_REG + offs, 0);
> -
> /* Wait for port startup */
> - do {
> - regmap_read(s->regmap,
> - MAX310X_BRGDIVLSB_REG + offs, &ret);
> - } while (ret != 0x01);
> + do{
> + /* Reset port */
> + regmap_write(s->regmap, MAX310X_MODE2_REG + offs,
> MAX310X_MODE2_RST_BIT);
> + /* Clear port reset */
> + regmap_write(s->regmap, MAX310X_MODE2_REG + offs, 0);
> +
> + j=0;
> + ret=0;
> + for(j=0;(j<5 && ret!=0x01);j++){
> + regmap_read(s->regmap,
> + MAX310X_BRGDIVLSB_REG + offs, &ret);
> + }
> + // did the port start ? if not, restart the process ... maybe we
> need to stop at some point the wait ....
> + }while(ret!=0x01);
for(j=0;(j<5... Not a good idea.
If you want to make an infinite loop protect, let's use time_is_after_jiffies().
...
Thanks.
--
Alexander Shiyan <shc_work@xxxxxxx>
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
