Re: [PATCH v3 2/2] serial: sc16is7xx: hardware reset chip if reset-gpios is defined in DT

[Hui Wang <hui.wang@canonical.com>]

On 6/12/24 21:50, Hugo Villeneuve wrote:
> On Wed, 12 Jun 2024 21:14:54 +0800
> Hui Wang <hui.wang@canonical.com> wrote:
>
> Hi Hui,
>
>> Some boards connect a GPIO to the reset pin, and the reset pin needs
>> to be setup correctly before accessing the chip.
>>
>> Add a function to handle the chip reset. If the reset-gpios is defined
>> in the DT, do hardware reset through this GPIO, othwerwise do software
>> reset as before.
...
>> +		/* The minimum reset pulse width is 3 us. */
>> +		udelay(5);
>> +		gpiod_set_value_cansleep(reset_gpio, 0);
>> +		/* Deassert GPIO */
> Move this comment after its corresponding statement:
>      gpiod_set_value_cansleep(reset_gpio, 0); /* Deassert GPIO */
>
Got it.
>> +	} else {
>> +		/* Software reset */
>> +		regmap_write(regmap, SC16IS7XX_IOCONTROL_REG,
>> +			     SC16IS7XX_IOCONTROL_SRESET_BIT);
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>>   int sc16is7xx_probe(struct device *dev, const struct sc16is7xx_devtype *devtype,
>>   		    struct regmap *regmaps[], int irq)
>>   {
>> @@ -1527,6 +1552,10 @@ int sc16is7xx_probe(struct device *dev, const struct sc16is7xx_devtype *devtype,
>>   	s->devtype = devtype;
>>   	dev_set_drvdata(dev, s);
>>   
>> +	ret = sc16is7xx_reset(dev, regmaps[0]);
>> +	if (ret)
>> +		goto out_clk;
>> +
> You could move this block where the original software reset was
> located, unless you have a good reason to move it here? This will make
> the review (diff) easier...

If I move it to the place where the original software reset was located, 
I can't use "goto out_clk" anymore, it not only needs to release clk but 
also needs to stop the kthread, so I have to add a new tag before the 
kthread_stop(s->kworker_task) and goto that tag.

Thanks.

>>   	kthread_init_worker(&s->kworker);
>>   	s->kworker_task = kthread_run(kthread_worker_fn, &s->kworker,
>>   				      "sc16is7xx");
>> @@ -1536,10 +1565,6 @@ int sc16is7xx_probe(struct device *dev, const struct sc16is7xx_devtype *devtype,
>>   	}
>>   	sched_set_fifo(s->kworker_task);
>>   
>> -	/* reset device, purging any pending irq / data */
>> -	regmap_write(regmaps[0], SC16IS7XX_IOCONTROL_REG,
>> -		     SC16IS7XX_IOCONTROL_SRESET_BIT);
>> -
>>   	/* Mark each port line and status as uninitialised. */
>>   	for (i = 0; i < devtype->nr_uart; ++i) {
>>   		s->p[i].port.line = SC16IS7XX_MAX_DEVS;
>> -- 
>> 2.34.1
>