-
Escolha do Patch e Refatoração
Eu e minha dupla, Bianca Galvão, trabalhamos em uma contribuição para o
subsistema IIO (Industrial I/O). Escolhemos uma das sugestões dos monitores para
eliminar uma duplicação de código no driver do sensor MMC35240. O problema
envolvia duas funções com lógica quase idêntica —
mmc35240_is_writeable_reg, que determina que apenas os
registradores CTRL0 e CTRL1 são graváveis, e
mmc35240_is_volatile_reg, que indica que todos os registradores,
exceto CTRL0 e CTRL1, são voláteis — mas com
resultados opostos para os mesmos casos de teste.
Para resolver isso, criamos uma função auxiliar que verifica se o registrador é
CTRL0 ou CTRL1 e, a partir dela, simplificamos ambas
as funções originais, invertendo o retorno quando necessário. Todas as mudanças
foram feitas em uma branch, seguindo as boas práticas.
-
Teste de Validação da Contribuição
Como não sabíamos como testar diretamente a função, validamos a contribuição
apenas pela compilação do módulo: habilitamos o driver normalmente, inserimos
uma linha de código que certamente quebraria o build (realmente quebrou),
removemos essa linha e, ao compilar novamente, tudo foi gerado sem erros.
-
Commit
Após refatorar, testar e compilar, fizemos o commit seguindo o esqueleto padrão
— identificamos o subsistema e o diretório, o assunto e descrevemos
detalhadamente as modificações. Em seguida, para validar o estilo de código
executamos:
git format-patch -1 --stdout | ./scripts/checkpatch.pl
O script apontou alguns avisos pertinentes, que foram corrigidos.
-
Envio do Patch
Na etapa de envio do patch, a configuração do email foi tranquila: usamos o kw,
que oferece um formato mais interativo e prático. A maior complicação surgiu ao
tentar usar o email da USP — não conseguimos completar a autenticação — então
migramos para o Gmail pessoal, o que simplificou o processo e funcionou.
-
Contribuição
+static bool mmc35240_reg_check(unsigned int reg)
+{
+ return reg == MMC35240_REG_CTRL0 || reg == MMC35240_REG_CTRL1;
+}
+
static bool mmc35240_is_writeable_reg(struct device *dev, unsigned int reg)
{
- switch (reg) {
- case MMC35240_REG_CTRL0:
- case MMC35240_REG_CTRL1:
- return true;
- default:
- return false;
- }
+ return mmc35240_reg_check(reg);
}
static bool mmc35240_is_volatile_reg(struct device *dev, unsigned int reg)
{
- switch (reg) {
- case MMC35240_REG_CTRL0:
- case MMC35240_REG_CTRL1:
- return false;
- default:
- return true;
- }
+ return !mmc35240_reg_check(reg);
}
-
Feedback
Recebemos o feedback do Jonathan Cameron. Ele sugeriu que retirássemos a função
auxiliar e mantivéssemos a estrutura lógica original da função
mmc35240_is_writeable_reg, fazendo com que a função mmc35240_is_volatile_reg
simplesmente retornasse o valor inverso de mmc35240_is_writeable_reg, uma vez
que isso contribuiria para a clareza e documentação do código. Realizamos as
alterações, no entanto, recebemos um novo feedback do Marcelo Schimitt, e
infelizmente fomos pegas no tal limite de colunas permitido em um commit. Além
disso, por algum motivo, nossa versão 2 não entrou na mesma thread do email
inicial, e acabou faltando a explicação da transição da versão 1 para a versão
2. Ainda sobre o feedback, ele comentou que, caso mantivéssemos a abordagem
sugerida pelo Jonathan, seria bom explicar por que os registradores voláteis são
justamente aqueles que não são graváveis.
-static bool mmc35240_reg_check(unsigned int reg)
-{
- return reg == MMC35240_REG_CTRL0 || reg == MMC35240_REG_CTRL1;
-}
static bool mmc35240_is_writeable_reg(struct device *dev, unsigned int reg)
{
- return mmc35240_reg_check(reg);
+ switch (reg) {
+ case MMC35240_REG_CTRL0:
+ case MMC35240_REG_CTRL1:
+ return true;
+ default:
+ return false;
+ }
}
static bool mmc35240_is_volatile_reg(struct device *dev, unsigned int reg)
{
- return !mmc35240_reg_check(reg);
+ return !mmc35240_is_writeable_reg(dev, reg);
}
Ao enviarmos o patch com a adoção da sugestão do Jonathan, tivemos alguns problemas com o checkpatch.
Apesar de ele ter sido executado, por algum motivo não identificou os erros relacionados ao uso de espaços em vez de tabs.
Além disso, recebemos um feedback sobre o formato da nossa cover letter.
Como enviamos a v2 a partir da v1, houve uma confusão em relação à função mmc35240_reg_check(),
que não estava sendo encontrada na branch. Após explicarmos o conteúdo da nossa cover letter e o fato de que a função
mmc35240_reg_check() não existe na branch testing, o Marcelo respondeu.
No entanto, deu a entender que nossa contribuição para uma v3 seria algo pequeno e que talvez não valesse o esforço.
Ele sugeriu que, caso realmente enviássemos a v3, utilizássemos algo mais robusto, como o FIELD_PREP e o FIELD_GET .